Skip to content
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.

Sign up for GitHub

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

ENH: keep retrying when getSecretValue fails #4102

Closed
wants to merge 6 commits into from
Closed

ENH: keep retrying when getSecretValue fails #4102

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e3df0f3
ENH: keep retrying when getSecretValue fails
chenqi0805 Feb 9, 2024
7a7f525
MAINT: reset
chenqi0805 Feb 10, 2024
9157c3f
STY: unused import
chenqi0805 Feb 10, 2024
bd44678
MAINT: skip thread.sleep coverage
chenqi0805 Feb 10, 2024
7e90a8b
MAINT: float up infinite retry
chenqi0805 Feb 13, 2024
ffea654
Merge branch 'main' into enh/keep-retrying-when-getSecretValue-failure
chenqi0805 Feb 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 17 additions & 7 deletions ...s/aws-plugin/src/main/java/org/opensearch/dataprepper/plugins/aws/AwsSecretsSupplier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.ObjectMapper;
import org.opensearch.dataprepper.model.annotations.SkipTestCoverageGenerated;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import software.amazon.awssdk.services.secretsmanager.SecretsManagerClient;
Expand Down Expand Up @@ -107,17 +108,26 @@ public void refresh(String secretConfigId) {
LOG.info("Finished retrieving latest secret in aws:secrets:{}.", secretConfigId);
}

@SkipTestCoverageGenerated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intended for generated code and maybe boilerplate. This code is significant and should be tested.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The motivation to put this is due to coverage shortage on the exception here:

try {
                    Thread.sleep(waitTimeInMillis);
                } catch (InterruptedException ex) {
                    throw new RuntimeException(ex);
                }

but it might no avoidable with some logic improvement

private Object retrieveSecretsFromSecretManager(final AwsSecretManagerConfiguration awsSecretManagerConfiguration,
final SecretsManagerClient secretsManagerClient) {
final GetSecretValueRequest getSecretValueRequest = awsSecretManagerConfiguration
.createGetSecretValueRequest();
final GetSecretValueResponse getSecretValueResponse;
try {
getSecretValueResponse = secretsManagerClient.getSecretValue(getSecretValueRequest);
} catch (Exception e) {
throw new RuntimeException(
String.format("Unable to retrieve secret: %s",
awsSecretManagerConfiguration.getAwsSecretId()), e);
GetSecretValueResponse getSecretValueResponse;
while (true) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think having an infinite loop this deep is a good idea. We should manage this in a thread specifically for this purpose.

try {
getSecretValueResponse = secretsManagerClient.getSecretValue(getSecretValueRequest);
break;
} catch (Exception e) {
final long waitTimeInMillis = 1000L;
LOG.error(String.format("Unable to retrieve secret: %s, wait for %dms to retry again.",
awsSecretManagerConfiguration.getAwsSecretId(), waitTimeInMillis), e);
try {
Thread.sleep(waitTimeInMillis);
} catch (InterruptedException ex) {
throw new RuntimeException(ex);
}
}
}

try {
Expand Down
20 changes: 14 additions & 6 deletions ...s-plugin/src/test/java/org/opensearch/dataprepper/plugins/aws/AwsSecretsSupplierTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
import org.mockito.ArgumentMatchers;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;
import software.amazon.awssdk.services.secretsmanager.SecretsManagerClient;
Expand All @@ -26,8 +25,12 @@
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.reset;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.opensearch.dataprepper.plugins.aws.AwsSecretsSupplier.MAP_TYPE_REFERENCE;

Expand Down Expand Up @@ -114,7 +117,7 @@ void testRetrieveValueBySecretIdOnlyNotSerializable() throws JsonProcessingExcep
final JsonProcessingException mockedJsonProcessingException = mock(JsonProcessingException.class);
final String testValue = "{\"a\":\"b\"}";
when(mockedObjectMapper.readValue(eq(testValue), eq(MAP_TYPE_REFERENCE))).thenReturn(Map.of("a", "b"));
when(mockedObjectMapper.writeValueAsString(ArgumentMatchers.any())).thenThrow(mockedJsonProcessingException);
when(mockedObjectMapper.writeValueAsString(any())).thenThrow(mockedJsonProcessingException);
when(secretValueDecoder.decode(eq(getSecretValueResponse))).thenReturn(testValue);
objectUnderTest = new AwsSecretsSupplier(secretValueDecoder, awsSecretPluginConfig, mockedObjectMapper);
final Exception exception = assertThrows(IllegalArgumentException.class,
Expand All @@ -132,10 +135,15 @@ void testRetrieveValueWithoutKey(String testValue) {
}

@Test
void testConstructorWithGetSecretValueFailure() {
when(secretsManagerClient.getSecretValue(eq(getSecretValueRequest))).thenThrow(secretsManagerException);
assertThrows(RuntimeException.class, () -> new AwsSecretsSupplier(
secretValueDecoder, awsSecretPluginConfig, OBJECT_MAPPER));
void testConstructorWithGetSecretValueFailureFirstThenSuccess() {
reset(secretsManagerClient);
final String testValue = "{\"a\":\"b\"}";
when(secretValueDecoder.decode(eq(getSecretValueResponse))).thenReturn(testValue);
when(secretsManagerClient.getSecretValue(eq(getSecretValueRequest)))
.thenThrow(secretsManagerException).thenReturn(getSecretValueResponse);
objectUnderTest = new AwsSecretsSupplier(secretValueDecoder, awsSecretPluginConfig, OBJECT_MAPPER);
verify(secretsManagerClient, times(2)).getSecretValue(eq(getSecretValueRequest));
assertThat(objectUnderTest.retrieveValue(TEST_AWS_SECRET_CONFIGURATION_NAME), equalTo(testValue));
}

@Test
Expand Down
Loading