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.

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

FEAT: AWS secret extension #3340

Merged
merged 59 commits into from
Sep 19, 2023
Merged

FEAT: AWS secret extension #3340

merged 59 commits into from
Sep 19, 2023

Conversation

chenqi0805
Copy link
Collaborator

@chenqi0805 chenqi0805 commented Sep 14, 2023

Description

This PR adds support for AWS secret extension plugin:

pipeline_extensions:
  aws:
    secrets:
      secret1:
        name: my-es-secret
        region: us-east-1
      secret2:
        name: my-es-secret-max-retries
        region: us-east-1
log-pipeline:
  workers: 1
  source:
    http:
      path: "/log/ingest"
  buffer:
    bounded_blocking:
      batch_size: ${{aws_secrets:secret1:batch_size}} # max number of records the buffer will drain for each read
  sink:
    - opensearch:
        hosts: [ "..." ]
        index: test-es7-compression
        username: ${{aws_secrets:secret1:username}}
        password: ${{aws_secrets:secret1:password}}
        flush_timeout: -1
        max_retries: ${{aws_secrets:secret2}}

The scope of settings that support secret reference is limited to plugins (source/buffer/sink/processor) in pipeline.YAML. The reference pattern is explained as follows:

  • ${{aws_secrets:secret1:username}} means the secret value under secret1 will be parsed as json and the setting value is swapped with the one according to key username in the secret json
  • ${{aws_secrets:secret2}} means the secret value under secret2 is parsed as a literal string and the setting value is swapped with the literal string value

Issues Resolved

Resolves #2780

Check List

  • New functionality includes testing.
  • New functionality has a documentation issue. Please link to it in this PR.
    • New functionality has javadoc added
  • Commits are signed with a real name per the DCO

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.

Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
Signed-off-by: George Chen <[email protected]>
@github-actions
Copy link

github-actions bot commented Sep 14, 2023

Unit Test Results

  1 448 files  +  16    1 448 suites  +16   46m 7s ⏱️ + 6m 17s
  6 034 tests +  67    6 033 ✔️ +  67  1 💤 ±0  0 ±0 
11 798 runs  +160  11 796 ✔️ +160  2 💤 ±0  0 ±0 

Results for commit 73a265d. ± Comparison against base commit b2b3249.

♻️ This comment has been updated with latest results.

Signed-off-by: George Chen <[email protected]>
package org.opensearch.dataprepper.model.plugin;

public interface PluginConfigValueTranslator {
String translate(final String value);
Copy link
Member

Choose a reason for hiding this comment

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

This should return a generic type as it can vary.

@@ -0,0 +1,7 @@
package org.opensearch.dataprepper.aws.api;

public interface SecretsSupplier {
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to expose this in the aws-plugin-api. It could couple plugins with AWS SecretsManager which is not desirable.

@@ -16,6 +16,7 @@ dependencies {
implementation project(':data-prepper-expression')
implementation project(':data-prepper-plugins:blocking-buffer')
implementation project(':data-prepper-plugins:common')
implementation project(':data-prepper-plugins:aws-plugin')
Copy link
Member

Choose a reason for hiding this comment

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

Data Prepper core should not depend on aws-plugin. The extensions framework is what will let this load dynamically.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the catch! It is unnecessary

@@ -16,7 +20,7 @@ jacocoTestCoverageVerification {
violationRules {
rule {
limit {
minimum = 1.0
minimum = 0.98
Copy link
Member

Choose a reason for hiding this comment

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

What can we do to this back to 100%?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is one JsonProcessingException unreachable:

@Override
    public Object retrieveValue(String secretId) {
        if (!secretIdToValue.containsKey(secretId)) {
            throw new IllegalArgumentException(String.format("Unable to find secretId: %s", secretId));
        }
        try {
            final Object secretValue = secretIdToValue.get(secretId);
            return secretValue instanceof Map ? OBJECT_MAPPER.writeValueAsString(secretIdToValue.get(secretId)) :
                    secretValue;
        } catch (JsonProcessingException e) {
            throw new IllegalArgumentException(String.format("Unable to read the value under secretId: %s as string",
                    secretId));
        }
    }

This is about writing value back into a string when ${{aws_secrets:secretId}} is used but the actual secret Value is a valid json, which means we interpret it as literal value

Copy link
Member

Choose a reason for hiding this comment

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

Can you mock secretIdToValue such that get(secretId) returns non-JSON?

private static final String AWS_IAM_ROLE = "role";
private static final String AWS_IAM = "iam";

@JsonProperty("name")
Copy link
Member

Choose a reason for hiding this comment

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

.build();
}

private AwsCredentialsProvider authenticateAwsConfiguration() {
Copy link
Member

Choose a reason for hiding this comment

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

Let's use the existing AWSCredentialsProvider as provided by the AWS Plugin.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

try {
getSecretValueResponse = secretsManagerClient.getSecretValue(getSecretValueRequest);
} catch (Exception e) {
throw ResourceNotFoundException.builder()
Copy link
Member

Choose a reason for hiding this comment

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

Why are you throwing an AWS SDK exception here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was trying to wrap it with a more explicit error message. Maybe RuntimeException is more appropriate?

@@ -78,7 +78,7 @@ public static RetryConfiguration readRetryConfig(final PluginSetting pluginSetti
if (dlqFile != null) {
builder = builder.withDlqFile(dlqFile);
}
final Integer maxRetries = (Integer) pluginSetting.getAttributeFromSettings(MAX_RETRIES);
final Integer maxRetries = pluginSetting.getIntegerOrDefault(MAX_RETRIES, null);
Copy link
Member

Choose a reason for hiding this comment

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

Are these part of another PR? This seems unrelated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Casting would not work for secret value substitution especially when it is a String. But it is possible with generic type in PluginConfigValueTranslator this would not be necessary

@chenqi0805 chenqi0805 merged commit b47b3d5 into main Sep 19, 2023
49 checks passed
@chenqi0805 chenqi0805 deleted the add/aws-secret-extension branch September 19, 2023 04:48
asifsmohammed pushed a commit to asifsmohammed/data-prepper that referenced this pull request Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support AWS secrets in configuring Data Prepper pipelines
2 participants