Skip to content

Token caching for IMDS Client #3543

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

Jump to bottom
Merged
L-Applin merged 16 commits into from
Dec 1, 2022
Merged

Token caching for IMDS Client #3543

L-Applin merged 16 commits into from
Dec 1, 2022

Conversation

@L-Applin
Copy link
Contributor

@L-Applin L-Applin commented Nov 14, 2022

Implements token caching for both the Ec2MetadataClient and its Async version.

Motivation and Context

Required as part of the v2 parity.

Modifications

Add an opt-in option to use a CachedSupplier to prevent sending a token request for every MetadataRequest. The token is cached until the token time to live has expired.

Testing

Added unit tests for token caching, both for regular and async clients.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed
  • I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

@L-Applin L-Applin requested a review from a team as a code owner November 14, 2022 19:54
@L-Applin
PR Comments:
a36d750 
- safe async client closing
- Ec2MetadataEndpointProvider singleton instance
- default attribute map for IMDS
- Fix javadoc <p> tag
@L-Applin
Attempt to fix timeouts in Async Client tests on GitHub
985bb01
zoewangg
zoewangg reviewed Nov 16, 2022
View reviewed changes
@L-Applin
Pr comments
e1f2032 
- upgrade to Junit 5
- TokenCacheStrategy as an interface
- added Token class
- Improved Asynchronous request retry and caching logic
@L-Applin
Fix propagation of the cancellation down the Future chain
4b7cb5e
@L-Applin
Attempt to fix read-timout error in GH unit tests
4f22b29
@L-Applin
Remove unnecessary future error forwarding
2f0b7bd
zoewangg
zoewangg reviewed Nov 23, 2022
View reviewed changes
core/imds/src/main/java/software/amazon/awssdk/imds/internal/DefaultEc2MetadataAsyncClient.java Outdated
CompletableFutureUtils.forwardExceptionTo(tokenFuture, tokenValueFuture);
return tokenFuture;
};
this.tokenCache = tokenCacheStrategy.getCachedSupplier(valueSupplier, this.tokenTtl);
Copy link
Contributor

@zoewangg zoewangg Nov 22, 2022

Choose a reason for hiding this comment

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

Should we use the token TTL from the response header X-aws-ec2-metadata-token-ttl-seconds? Do we know what happens if the token TTL provided is greater than the max token TTL allowed on the server side? Does it fail or succeed with the max token TTL?

Copy link
Contributor

@zoewangg zoewangg Nov 25, 2022

Choose a reason for hiding this comment

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

Can we track this somewhere if we haven't done so already?

Copy link
Contributor

@zoewangg zoewangg Nov 25, 2022

Choose a reason for hiding this comment

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

Maybe add a TODO or add a backlog item?

core/imds/src/main/java/software/amazon/awssdk/imds/internal/DefaultEc2MetadataAsyncClient.java Outdated
ThreadFactory threadFactory = new ThreadFactoryBuilder().threadNamePrefix("IMDS-ScheduledExecutor").build();
return Executors.newScheduledThreadPool(DEFAULT_RETRY_THREAD_POOL_SIZE, threadFactory);
});
TokenCacheStrategy tokenCacheStrategy = Validate.getOrDefault(builder.tokenCacheStrategy, () -> TokenCacheStrategy.NONE);
Copy link
Contributor

@zoewangg zoewangg Nov 22, 2022

Choose a reason for hiding this comment

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

Should we cache it by default? Let's discuss it in the API surface area review meeting

@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 24, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 78 Code Smells

73.7% 73.7% Coverage
1.4% 1.4% Duplication

@L-Applin L-Applin merged commit b35bf06 into feature/master/imds Dec 1, 2022
@L-Applin L-Applin deleted the olapplin/imds-token branch January 10, 2023 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zoewangg zoewangg zoewangg approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@L-Applin @zoewangg