-
Notifications
You must be signed in to change notification settings - Fork 210
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update renewed Jira refresh token back to the secrets store #5324
base: main
Are you sure you want to change the base?
Conversation
… plugins to get access to their underlying aws secrets store member to be able to update if needed Signed-off-by: Santhosh Gandhe <[email protected]>
…ecrets store Signed-off-by: Santhosh Gandhe <[email protected]>
Signed-off-by: Santhosh Gandhe <[email protected]>
… used for refreshToken Signed-off-by: Santhosh Gandhe <[email protected]>
Signed-off-by: Santhosh Gandhe <[email protected]>
* @param newValueToSet The value of the secret to be updated | ||
* @return The version id of the secret after the update | ||
*/ | ||
String updateValue(String secretId, String keyToUpdate, Object newValueToSet); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above method assumes JSON(key-value pair) secret value. I think we will need the same method with more generic signature
String updateValue(String secretId, Object newValueToSet);
to support plain text secret value. Or we can try to replace the above with this more generic use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newValuetoSet
can be plain text as well. The json we are constructing is the format accepted by the aws api to update the secret store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we just support null
here. When we construct the AwsPluginConfigVariable
, we can provide a null
value in that situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we trying to support a value without having kay for it? keyToUpdate
can be null is what you are suggesting @dlvenable ?
@@ -78,6 +79,14 @@ public GetSecretValueRequest createGetSecretValueRequest() { | |||
.build(); | |||
} | |||
|
|||
public PutSecretValueRequest putSecretValueRequest(String keyToUpdate, Object newValue) { | |||
String updatedSecretString = String.format("{\"%s\": \"%s\"}", keyToUpdate, newValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible we have other key-value pairs in the existing secret string. Shall we not overwriting them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. I thought this will only update the specific key that we are passing in the json string based on reading the documentation. I will verify and put in the fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chenqi0805 Fixed the logic to only update the specific key and keeping all the existing values as is.
Signed-off-by: Santhosh Gandhe <[email protected]>
/** | ||
* If this variable is updatable, this method helps to set a new value for this variable | ||
*/ | ||
void setValue(Object someValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
void setValue(Object someValue); | |
void setValue(Object updatedValue); |
Let's use a clearer name here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified 👍
* Interface for a Extension Plugin configuration variable. | ||
* It gives access to the details of a defined extension variable. | ||
* | ||
* @since 1.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @since 1.2 | |
* @since 2.11 |
The next release is 2.11.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified
@@ -48,8 +48,13 @@ public <T> T translate(final JsonParser jsonParser, final Class<T> destinationTy | |||
.filter(entry -> entry.getKey().matches()) | |||
.map(entry -> { | |||
final String valueReferenceKey = entry.getKey().group(VALUE_REFERENCE_KEY); | |||
return objectMapper.convertValue( | |||
entry.getValue().translate(valueReferenceKey), destinationType); | |||
if (destinationType.equals(PluginConfigVariable.class)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be unit tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added unit tests to cover the cases 👍
public String updateValue(String secretId, String keyToUpdate, Object newValue) { | ||
final Map<String, Object> keyValuePairs = (Map<String, Object>) secretIdToValue.get(secretId); | ||
keyValuePairs.put(keyToUpdate, newValue); | ||
String secretKeyValueMapAsString = (String) retrieveValue(secretId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keys are optional, so we should have support for both keyed and keyless secrets.
A key just sets a convention of having a JSON object.
* | ||
* @since 1.2 | ||
*/ | ||
public interface PluginConfigVariable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a boolean as well to indicate if the PluginConfigVariable
supports updates.
e.g.
boolean isSetValueSupported();
/** | ||
* Utility class to help test private fields | ||
*/ | ||
public class TestUtilForPrivateFields { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have this that you can use: ReflectivelySetField
Lines 29 to 37 in 6648d9f
public static <T> void setField(final Class<T> configurationClass, final Object configurationObject, | |
final String fieldName, final Object value) throws NoSuchFieldException, IllegalAccessException { | |
final Field field = configurationClass.getDeclaredField(fieldName); | |
try { | |
field.setAccessible(true); | |
field.set(configurationObject, value); | |
} finally { | |
field.setAccessible(false); | |
} |
@@ -4,4 +4,6 @@ public interface PluginConfigValueTranslator { | |||
Object translate(final String value); | |||
|
|||
String getPrefix(); | |||
|
|||
PluginConfigVariable translateToPluginConfigVariable(final String value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add Javadocs to this method.
public interface PluginConfigVariable { | ||
|
||
/** | ||
* Returns the value of this variable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please complete the Javadocs for this. Your IDE should help.
e.g. @returns
, @throws
if applicable, @since
Object getValue(); | ||
|
||
/** | ||
* If this variable is updatable, this method helps to set a new value for this variable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Complete Javadocs.
@@ -0,0 +1,67 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also use the new copyright notice in this file:
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*
*/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
Description
In Jira OAuth2 based authentication, if one node renew the refresh token, others are not aware of the renewed refreshed token. This PR enables a way to write the renewed token back into the secret store so that other OCU nodes can pick up the renewed token and continue their execution. The changes introduced in this PR are
Issues Resolved
Resolves #[Issue number to be closed when this PR is merged]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.