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

AWS SDK v2 support #64

Merged
merged 13 commits into from
Sep 14, 2022
Merged

AWS SDK v2 support #64

merged 13 commits into from
Sep 14, 2022

Conversation

Kirik0
Copy link
Contributor

@Kirik0 Kirik0 commented Sep 1, 2022

Performed local testing using GETs, PUTs, DELETEs. Performed a couple secret rotations, pulled credential history, updated metadata.

@Kirik0 Kirik0 requested a review from SMxJrz September 1, 2022 16:28
client = clientBuilder.build();
awsSecurityTokenService = stsBuilder.build();
jCredStash = new JCredStash(ddbBuilder.build(), kmsBuilder.build(), awsSecurityTokenService);
if(envConfig.hasProxyEnv()) {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if I understand this if condition, don't we have to initialize all the clients if proxy env is provided and if not provided, like if.....else....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're still building the Clients on lines 128-131. For AWS SDK v2, client builders like the DynamoDbClientBuilder need an additional HTTP Client built-in for proxy configuration to work. The if-block there takes care of that if proxy configuration is provided but skips if it's not.

protected AWSSecurityTokenService awsSecurityTokenService;
protected DynamoDB dynamoDB;
protected StsClient stsClient;
protected static List<String> TABLE_HEADERS = Arrays.asList("name", "component", "sdlc", "contents", "version", "updatedBy", "updatedOn", "key", "hmac", "source", "sourceType");
Copy link
Member

Choose a reason for hiding this comment

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

Unused variable. You can remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Will remove.

}

// Creates a new DynamoDBMapper object
public DynamoDBMapper createMapper(String account, String region, String tableName) {
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 need mapping?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DynamoDBMapper is no longer supported in SDK v2. Needed to do some workarounds to keep the logic as is.

@@ -78,44 +78,60 @@
</developers>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we update the maven compiler plugin to line up with this PR? #65

@Kirik0 Kirik0 requested a review from SMxJrz September 13, 2022 16:33
@Kirik0 Kirik0 merged commit 0ca7716 into FINRAOS:master Sep 14, 2022
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.

3 participants