Skip to content

Commit 7360f59

Browse files
BhattacharyaSumitBhattacharyaSumit
committed
refactor: Address PR feedback
- Rename validate_at_bootstrap to skip_validation_on_start - Change default from true to false (validate by default per DataPrepper convention) - Remove ellipses from log messages - Extract duplicate lazy-loading code into loadSecretIfNeeded() helper method - Update tests to use random values as member variables initialized in @beforeeach - Update all test mocks to use new method name - Remove example configuration file - Remove accidentally committed build artifacts Signed-off-by: Sumit Bhattacharya <sumit4739@gmail.com>
1 parent 227c332 commit 7360f59

7 files changed

Lines changed: 71 additions & 91 deletions

File tree

‎data-prepper-plugins/aws-plugin/src/main/java/org/opensearch/dataprepper/plugins/aws/AwsSecretManagerConfiguration.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,8 @@ public class AwsSecretManagerConfiguration {
5252
@JsonProperty("disable_refresh")
5353
private boolean disableRefresh = false;
5454

55-
@JsonProperty("validate_at_bootstrap")
56-
private boolean validateAtBootstrap = true; // Default: true (safe-by-default)
55+
@JsonProperty("skip_validation_on_start")
56+
private boolean skipValidationOnStart = false; // Default: false (validate by default)
5757

5858
public String getAwsSecretId() {
5959
return awsSecretId;
@@ -71,8 +71,8 @@ public boolean isDisableRefresh() {
7171
return disableRefresh;
7272
}
7373

74-
public boolean isValidateAtBootstrap() {
75-
return validateAtBootstrap;
74+
public boolean isSkipValidationOnStart() {
75+
return skipValidationOnStart;
7676
}
7777

7878
public SecretsManagerClient createSecretManagerClient(final AwsCredentialsSupplier awsCredentialsSupplier) {

‎data-prepper-plugins/aws-plugin/src/main/java/org/opensearch/dataprepper/plugins/aws/AwsSecretsSupplier.java‎

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -61,9 +61,9 @@ private ConcurrentMap<String, Object> toSecretMap(
6161
awsSecretManagerConfigurationMap.get(secretConfigurationId);
6262
final SecretsManagerClient secretsManagerClient = entry.getValue();
6363

64-
// Check if validation at bootstrap is disabled for this secret
65-
if (!awsSecretManagerConfiguration.isValidateAtBootstrap()) {
66-
LOG.info("Skipping secret retrieval at bootstrap for secret: {} (validate_at_bootstrap=false)",
64+
// Check if validation on start is skipped for this secret
65+
if (awsSecretManagerConfiguration.isSkipValidationOnStart()) {
66+
LOG.info("Skipping secret retrieval on start for secret: {} (skip_validation_on_start=true)",
6767
awsSecretManagerConfiguration.getAwsSecretId());
6868
return NOT_LOADED_SENTINEL; // Mark as not loaded, will be loaded on first access
6969
}
@@ -88,13 +88,8 @@ public Object retrieveValue(String secretId, String key) {
8888
throw new IllegalArgumentException(String.format("Unable to find secretId: %s", secretId));
8989
}
9090

91-
// Check if secret was skipped at bootstrap and needs to be loaded now
92-
Object keyValuePairs = secretIdToValue.get(secretId);
93-
if (keyValuePairs == NOT_LOADED_SENTINEL) {
94-
LOG.info("Secret {} was not loaded at bootstrap, loading now on first access...", secretId);
95-
refresh(secretId);
96-
keyValuePairs = secretIdToValue.get(secretId);
97-
}
91+
// Load secret if it was skipped on start
92+
final Object keyValuePairs = loadSecretIfNeeded(secretId);
9893

9994
if (!(keyValuePairs instanceof Map)) {
10095
throw new IllegalArgumentException(String.format("The value under secretId: %s is not a valid json.",
@@ -114,13 +109,8 @@ public Object retrieveValue(String secretId) {
114109
throw new IllegalArgumentException(String.format("Unable to find secretId: %s", secretId));
115110
}
116111

117-
// Check if secret was skipped at bootstrap and needs to be loaded now
118-
Object secretValue = secretIdToValue.get(secretId);
119-
if (secretValue == NOT_LOADED_SENTINEL) {
120-
LOG.info("Secret {} was not loaded at bootstrap, loading now on first access...", secretId);
121-
refresh(secretId);
122-
secretValue = secretIdToValue.get(secretId);
123-
}
112+
// Load secret if it was skipped on start
113+
final Object secretValue = loadSecretIfNeeded(secretId);
124114

125115
try {
126116
return secretValue instanceof Map ? objectMapper.writeValueAsString(secretValue) :
@@ -131,6 +121,22 @@ public Object retrieveValue(String secretId) {
131121
}
132122
}
133123

124+
/**
125+
* Loads a secret if it was skipped on start (lazy-loading).
126+
*
127+
* @param secretId The secret configuration ID
128+
* @return The loaded secret value
129+
*/
130+
private Object loadSecretIfNeeded(String secretId) {
131+
Object value = secretIdToValue.get(secretId);
132+
if (value == NOT_LOADED_SENTINEL) {
133+
LOG.info("Secret {} was not loaded on start, loading now on first access.", secretId);
134+
refresh(secretId);
135+
value = secretIdToValue.get(secretId);
136+
}
137+
return value;
138+
}
139+
134140

135141
@Override
136142
public void refresh(String secretConfigId) {

‎data-prepper-plugins/aws-plugin/src/test/java/org/opensearch/dataprepper/plugins/aws/AwsSecretManagerConfigurationValidateAtBootstrapTest.java‎

Lines changed: 9 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -23,52 +23,38 @@ class AwsSecretManagerConfigurationValidateAtBootstrapTest {
2323
.registerModule(new JavaTimeModule());
2424

2525
@Test
26-
void testDefaultValidateAtBootstrap() {
26+
void testDefaultSkipValidationOnStart() {
2727
final AwsSecretManagerConfiguration config = new AwsSecretManagerConfiguration();
2828

29-
// Default should be true (safe-by-default)
30-
assertThat(config.isValidateAtBootstrap(), equalTo(true));
29+
// Default should be false (validate by default)
30+
assertThat(config.isSkipValidationOnStart(), equalTo(false));
3131
}
3232

3333
@Test
34-
void testValidateAtBootstrapFromYaml_Enabled() throws Exception {
34+
void testSkipValidationOnStartFromYaml_Enabled() throws Exception {
3535
final String yaml =
3636
"secret_id: my-secret\n" +
3737
"region: us-east-1\n" +
3838
"refresh_interval: PT1H\n" +
39-
"validate_at_bootstrap: true\n";
39+
"skip_validation_on_start: true\n";
4040

4141
final AwsSecretManagerConfiguration config =
4242
objectMapper.readValue(yaml, AwsSecretManagerConfiguration.class);
4343

44-
assertThat(config.isValidateAtBootstrap(), equalTo(true));
44+
assertThat(config.isSkipValidationOnStart(), equalTo(true));
4545
}
4646

4747
@Test
48-
void testValidateAtBootstrapFromYaml_Disabled() throws Exception {
48+
void testSkipValidationOnStartFromYaml_Disabled() throws Exception {
4949
final String yaml =
5050
"secret_id: my-secret\n" +
5151
"region: us-east-1\n" +
5252
"refresh_interval: PT1H\n" +
53-
"validate_at_bootstrap: false\n";
53+
"skip_validation_on_start: false\n";
5454

5555
final AwsSecretManagerConfiguration config =
5656
objectMapper.readValue(yaml, AwsSecretManagerConfiguration.class);
5757

58-
assertThat(config.isValidateAtBootstrap(), equalTo(false));
59-
}
60-
61-
@Test
62-
void testValidateAtBootstrapFromYaml_NotSpecified_UsesDefault() throws Exception {
63-
final String yaml =
64-
"secret_id: my-secret\n" +
65-
"region: us-east-1\n" +
66-
"refresh_interval: PT1H\n";
67-
68-
final AwsSecretManagerConfiguration config =
69-
objectMapper.readValue(yaml, AwsSecretManagerConfiguration.class);
70-
71-
// Should default to true when not specified
72-
assertThat(config.isValidateAtBootstrap(), equalTo(true));
58+
assertThat(config.isSkipValidationOnStart(), equalTo(false));
7359
}
7460
}

‎data-prepper-plugins/aws-plugin/src/test/java/org/opensearch/dataprepper/plugins/aws/AwsSecretPluginIT.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ void testInitializationWithNonNullConfig() {
9999
when(awsSecretPluginConfig.getAwsSecretManagerConfigurationMap()).thenReturn(
100100
Map.of(TEST_SECRET_CONFIG_ID, awsSecretManagerConfiguration));
101101
when(awsSecretManagerConfiguration.getRefreshInterval()).thenReturn(testInterval);
102-
when(awsSecretManagerConfiguration.isValidateAtBootstrap()).thenReturn(true); // Default behavior
102+
when(awsSecretManagerConfiguration.isSkipValidationOnStart()).thenReturn(false); // Default behavior
103103
when(awsSecretManagerConfiguration.createSecretManagerClient(awsCredentialsSupplier)).thenReturn(secretsManagerClient);
104104
when(awsSecretManagerConfiguration.createGetSecretValueRequest()).thenReturn(getSecretValueRequest);
105105
when(secretsManagerClient.getSecretValue(eq(getSecretValueRequest))).thenReturn(getSecretValueResponse);
@@ -134,7 +134,7 @@ void testInitializationWithDisableRefresh() {
134134
when(awsSecretPluginConfig.getAwsSecretManagerConfigurationMap()).thenReturn(
135135
Map.of(TEST_SECRET_CONFIG_ID, awsSecretManagerConfiguration));
136136
when(awsSecretManagerConfiguration.isDisableRefresh()).thenReturn(true);
137-
when(awsSecretManagerConfiguration.isValidateAtBootstrap()).thenReturn(true); // Default behavior
137+
when(awsSecretManagerConfiguration.isSkipValidationOnStart()).thenReturn(false); // Default behavior
138138
when(awsSecretManagerConfiguration.createSecretManagerClient(awsCredentialsSupplier)).thenReturn(secretsManagerClient);
139139
when(awsSecretManagerConfiguration.createGetSecretValueRequest()).thenReturn(getSecretValueRequest);
140140
when(secretsManagerClient.getSecretValue(eq(getSecretValueRequest))).thenReturn(getSecretValueResponse);

‎data-prepper-plugins/aws-plugin/src/test/java/org/opensearch/dataprepper/plugins/aws/AwsSecretsSupplierLazyLoadTest.java‎

Lines changed: 32 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import software.amazon.awssdk.services.secretsmanager.model.GetSecretValueResponse;
2323

2424
import java.util.Map;
25+
import java.util.UUID;
2526

2627
import static org.hamcrest.CoreMatchers.equalTo;
2728
import static org.hamcrest.MatcherAssert.assertThat;
@@ -32,15 +33,15 @@
3233
import static org.mockito.Mockito.when;
3334

3435
/**
35-
* Tests for lazy-loading behavior when validate_at_bootstrap is false.
36+
* Tests for lazy-loading behavior when skip_validation_on_start is true.
3637
*/
3738
@ExtendWith(MockitoExtension.class)
3839
class AwsSecretsSupplierLazyLoadTest {
3940

40-
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
41-
private static final String TEST_SECRET_ID = "test-secret";
42-
private static final String TEST_KEY = "test-key";
43-
private static final String TEST_VALUE = "test-value";
41+
private ObjectMapper objectMapper;
42+
private String testSecretId;
43+
private String testKey;
44+
private String testValue;
4445

4546
@Mock
4647
private SecretValueDecoder secretValueDecoder;
@@ -63,63 +64,71 @@ class AwsSecretsSupplierLazyLoadTest {
6364
@Mock
6465
private AwsCredentialsSupplier awsCredentialsSupplier;
6566

67+
@BeforeEach
68+
void setUp() {
69+
objectMapper = new ObjectMapper();
70+
testSecretId = UUID.randomUUID().toString();
71+
testKey = UUID.randomUUID().toString();
72+
testValue = UUID.randomUUID().toString();
73+
}
74+
6675
@Test
67-
void testSecretWithValidateAtBootstrapFalse_LoadsOnFirstAccess() throws JsonProcessingException {
68-
// Given: Secret configured with validate_at_bootstrap=false
76+
void testSecretWithSkipValidationOnStartTrue_LoadsOnFirstAccess() throws JsonProcessingException {
77+
// Given: Secret configured with skip_validation_on_start=true
6978
when(awsSecretPluginConfig.getAwsSecretManagerConfigurationMap()).thenReturn(
70-
Map.of(TEST_SECRET_ID, awsSecretManagerConfiguration)
79+
Map.of(testSecretId, awsSecretManagerConfiguration)
7180
);
72-
when(awsSecretManagerConfiguration.isValidateAtBootstrap()).thenReturn(false); // Skip at bootstrap
81+
when(awsSecretManagerConfiguration.isSkipValidationOnStart()).thenReturn(true); // Skip on start
7382
when(awsSecretManagerConfiguration.createSecretManagerClient(awsCredentialsSupplier)).thenReturn(secretsManagerClient);
7483
when(awsSecretManagerConfiguration.createGetSecretValueRequest()).thenReturn(getSecretValueRequest);
75-
when(secretValueDecoder.decode(eq(getSecretValueResponse))).thenReturn(OBJECT_MAPPER.writeValueAsString(
76-
Map.of(TEST_KEY, TEST_VALUE)
84+
when(secretValueDecoder.decode(eq(getSecretValueResponse))).thenReturn(objectMapper.writeValueAsString(
85+
Map.of(testKey, testValue)
7786
));
7887
when(secretsManagerClient.getSecretValue(eq(getSecretValueRequest))).thenReturn(getSecretValueResponse);
7988

8089
// When: AwsSecretsSupplier is constructed
8190
final AwsSecretsSupplier supplier = new AwsSecretsSupplier(
82-
secretValueDecoder, awsSecretPluginConfig, OBJECT_MAPPER, awsCredentialsSupplier
91+
secretValueDecoder, awsSecretPluginConfig, objectMapper, awsCredentialsSupplier
8392
);
8493

8594
// Then: Secret is NOT retrieved at construction time
8695
verify(secretsManagerClient, never()).getSecretValue(eq(getSecretValueRequest));
8796

8897
// When: Secret is accessed for the first time
89-
final Object value = supplier.retrieveValue(TEST_SECRET_ID, TEST_KEY);
98+
final Object value = supplier.retrieveValue(testSecretId, testKey);
9099

91100
// Then: Secret is loaded on-demand
92101
verify(secretsManagerClient, times(1)).getSecretValue(eq(getSecretValueRequest));
93-
assertThat(value, equalTo(TEST_VALUE));
102+
assertThat(value, equalTo(testValue));
94103
}
95104

96105
@Test
97-
void testSecretWithValidateAtBootstrapTrue_LoadsAtConstruction() throws JsonProcessingException {
98-
// Given: Secret configured with validate_at_bootstrap=true (default)
106+
void testSecretWithSkipValidationOnStartFalse_LoadsAtConstruction() throws JsonProcessingException {
107+
// Given: Secret configured with skip_validation_on_start=false (default)
99108
when(awsSecretPluginConfig.getAwsSecretManagerConfigurationMap()).thenReturn(
100-
Map.of(TEST_SECRET_ID, awsSecretManagerConfiguration)
109+
Map.of(testSecretId, awsSecretManagerConfiguration)
101110
);
102-
when(awsSecretManagerConfiguration.isValidateAtBootstrap()).thenReturn(true); // Load at bootstrap
111+
when(awsSecretManagerConfiguration.isSkipValidationOnStart()).thenReturn(false); // Load on start
103112
when(awsSecretManagerConfiguration.createSecretManagerClient(awsCredentialsSupplier)).thenReturn(secretsManagerClient);
104113
when(awsSecretManagerConfiguration.createGetSecretValueRequest()).thenReturn(getSecretValueRequest);
105-
when(secretValueDecoder.decode(eq(getSecretValueResponse))).thenReturn(OBJECT_MAPPER.writeValueAsString(
106-
Map.of(TEST_KEY, TEST_VALUE)
114+
when(secretValueDecoder.decode(eq(getSecretValueResponse))).thenReturn(objectMapper.writeValueAsString(
115+
Map.of(testKey, testValue)
107116
));
108117
when(secretsManagerClient.getSecretValue(eq(getSecretValueRequest))).thenReturn(getSecretValueResponse);
109118

110119
// When: AwsSecretsSupplier is constructed
111120
final AwsSecretsSupplier supplier = new AwsSecretsSupplier(
112-
secretValueDecoder, awsSecretPluginConfig, OBJECT_MAPPER, awsCredentialsSupplier
121+
secretValueDecoder, awsSecretPluginConfig, objectMapper, awsCredentialsSupplier
113122
);
114123

115124
// Then: Secret IS retrieved at construction time
116125
verify(secretsManagerClient, times(1)).getSecretValue(eq(getSecretValueRequest));
117126

118127
// When: Secret is accessed
119-
final Object value = supplier.retrieveValue(TEST_SECRET_ID, TEST_KEY);
128+
final Object value = supplier.retrieveValue(testSecretId, testKey);
120129

121130
// Then: No additional retrieval (already loaded)
122131
verify(secretsManagerClient, times(1)).getSecretValue(eq(getSecretValueRequest));
123-
assertThat(value, equalTo(TEST_VALUE));
132+
assertThat(value, equalTo(testValue));
124133
}
125134
}

‎data-prepper-plugins/aws-plugin/src/test/java/org/opensearch/dataprepper/plugins/aws/AwsSecretsSupplierTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ class AwsSecretsSupplierTest {
8686
@BeforeEach
8787
void setUp() throws JsonProcessingException {
8888
when(awsSecretManagerConfiguration.createGetSecretValueRequest()).thenReturn(getSecretValueRequest);
89-
when(awsSecretManagerConfiguration.isValidateAtBootstrap()).thenReturn(true); // Default: validate at bootstrap
89+
when(awsSecretManagerConfiguration.isSkipValidationOnStart()).thenReturn(false); // Default: validate on start
9090
when(awsSecretPluginConfig.getAwsSecretManagerConfigurationMap()).thenReturn(
9191
Map.of(TEST_AWS_SECRET_CONFIGURATION_NAME, awsSecretManagerConfiguration)
9292
);

‎examples/config/data-prepper-config-secret-validation.yaml‎

Lines changed: 0 additions & 21 deletions
This file was deleted.

0 commit comments

Comments
 (0)