feat(auth) - support aws sdk v2 for registry auth #3806
+2,262
−429
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Eclipse JKube CI ReportStarted new GH workflow run for #3806 (2025-12-05T15:19:13Z) ⚙️ JKube E2E Tests (19933929469)
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3806 +/- ##
=============================================
+ Coverage 59.36% 72.13% +12.77%
- Complexity 4586 4845 +259
=============================================
Files 500 492 -8
Lines 21211 19259 -1952
Branches 2830 2569 -261
=============================================
+ Hits 12591 13892 +1301
+ Misses 7370 4160 -3210
+ Partials 1250 1207 -43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
|
Since we don't have access to a real AWS instance, we might be able to leverage https://github.com/localstack/localstack to ensure the changes are working |
Contributor
Author
|
Tested the AWS SDK v2 with localstack, works fines. Attaching screenshots: 
|
ash-thakur-rh
requested review from
manusa,
rohanKanojia and
sunix
as code owners
|
manusa
reviewed
Dec 11, 2025
Comment on lines
+18
to
+24
| /** | ||
| * Factory for AWS SDK helpers that supports both AWS SDK v1 and v2. | ||
| * Automatically detects which SDK version is available on the classpath. | ||
| * Maintains backward compatibility with the original AwsSdkHelper API. | ||
| */ | ||
| public class AwsSdkHelper { | ||
| private static final String ACCESS_KEY_ID = "AWS_ACCESS_KEY_ID"; | ||
| private static final String SECRET_ACCESS_KEY = "AWS_SECRET_ACCESS_KEY"; | ||
| private static final String SESSION_TOKEN = "AWS_SESSION_TOKEN"; | ||
| private static final String CONTAINER_CREDENTIALS_RELATIVE_URI = "AWS_CONTAINER_CREDENTIALS_RELATIVE_URI"; | ||
| private static final String METADATA_ENDPOINT = "ECS_METADATA_ENDPOINT"; | ||
| private static final String AWS_INSTANCE_LINK_LOCAL_ADDRESS = "http://169.254.170.2"; | ||
| private static final String DEFAULT_AWSCREDENTIALS_PROVIDER_CHAIN = "com.amazonaws.auth.DefaultAWSCredentialsProviderChain"; | ||
| private static final String AWS_SESSION_CREDENTIALS = "com.amazonaws.auth.AWSSessionCredentials"; | ||
| private static final String AWS_CREDENTIALS = "com.amazonaws.auth.AWSCredentials"; | ||
|
|
||
| public boolean isDefaultAWSCredentialsProviderChainPresentInClassPath() { | ||
| try { | ||
| Class.forName(DEFAULT_AWSCREDENTIALS_PROVIDER_CHAIN); | ||
| return true; | ||
| } catch (ClassNotFoundException e) { | ||
| return false; | ||
| } | ||
| } | ||
| private final AwsSdkAuthHelper delegate; |
Member
There was a problem hiding this comment.
Should this class implement the AwsSdkAuthHelper interface too to ensure all methods are delegated?
Member
There was a problem hiding this comment.
This would also prevent naming inconsistencies:
AuthConfig getCredentialsFromDefaultCredentialsProvider();
AuthConfig getAuthConfigFromDefaultCredentialsProvider();
manusa
reviewed
Dec 11, 2025
| * @throws IllegalAccessException if AWS SDK method access fails | ||
| * @deprecated Use {@link #getAuthConfigFromDefaultCredentialsProvider()} instead | ||
| */ | ||
| @Deprecated |
Member
There was a problem hiding this comment.
The deprecations in this and the rest of the methods are weird.
They throw an UnsupportedOperationException which will effectively be as removing from a functional perspective.
SDK-wise, I don't think there's anyone consuming JKube as a library.
I think we should directly remove the methods (unless I'm missing something else here).
Affected methods:
- getCredentialsFromDefaultAWSCredentialsProviderChain()
- getSessionTokenFromCrendentials()
- getAWSAccessKeyIdFromCredentials()
- getAwsSecretKeyFromCredentials()
manusa
reviewed
Dec 11, 2025
| @@ -0,0 +1,10 @@ | |||
| /* | |||
Member
There was a problem hiding this comment.
This (and other classes are lacking the license header.
Not sure why the CI check is not failing on this (we should probably revisit that too -separate PR-)
manusa
reviewed
Dec 11, 2025
| @@ -0,0 +1,9 @@ | |||
| /* | |||
Member
There was a problem hiding this comment.
manusa
reviewed
Dec 11, 2025
| @@ -0,0 +1,25 @@ | |||
| /* | |||
Member
There was a problem hiding this comment.
manusa
reviewed
Dec 11, 2025
| @@ -0,0 +1,32 @@ | |||
| /* | |||
Member
There was a problem hiding this comment.
manusa
reviewed
Dec 11, 2025
| @@ -0,0 +1,37 @@ | |||
| /* | |||
Member
There was a problem hiding this comment.
manusa
reviewed
Dec 11, 2025
| * Utility class for modifying environment variables in tests. | ||
| * Uses reflection to modify the environment map. | ||
| */ | ||
| public class EnvironmentVariablesTestUtil { |
Member
There was a problem hiding this comment.
This is kind of fragile.
We have other tests that depend on the environment.
Maybe it's a good time to implement a proper Environment wrapper (in a separate PR) that can be leveraged by this feature's production code so that it can be set in tests.
manusa
reviewed
Dec 11, 2025
|
|
||
| @Override | ||
| public String getEcsMetadataEndpoint() { | ||
| return System.getenv(METADATA_ENDPOINT); |
Member
There was a problem hiding this comment.
In the prior version this was falling back to (http://169.254.170.2)
I see this documented here: https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task-metadata-endpoint-v2.html
We might want to keep the fallback
manusa
reviewed
Dec 11, 2025
| * @deprecated Use {@link #getAuthConfigFromDefaultCredentialsProvider()} instead | ||
| */ | ||
| @Deprecated | ||
| public String getSessionTokenFromCrendentials(Object credentials) throws ClassNotFoundException, NoSuchMethodException, InvocationTargetException, IllegalAccessException { |
Member
There was a problem hiding this comment.
getSessionTokenFromCrendentials there was a typo here.
Maybe a good time to rename to getSessionTokenFromCredentials
manusa
requested changes
Dec 11, 2025
Member
manusa
left a comment
manusa
left a comment
There was a problem hiding this comment.
Thx @ash-thakur-rh
Overall looks good, I added a few comments.
Contributor
Author
Thanks @manusa. I will take care of comments once the OpenShift model upgrade is complete |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #3732
Adds AWS SDK v2 support for ECR authentication while maintaining backward compatibility with AWS SDK v1.
Changes
1. AWS SDK v2 Support
New Architecture:
AbstractAwsSdkHelper- Abstract base class with common AWS credential handling logicAwsSdkHelperV1- AWS SDK v1 implementationAwsSdkHelperV2- AWS SDK v2 implementationAwsSdkAuthHelperinterface - Contract for SDK version abstractionKey Features:
AWS_ACCESS_KEY_ID,AWS_SECRET_ACCESS_KEY,AWS_SESSION_TOKEN)AWS_CONTAINER_CREDENTIALS_RELATIVE_URI)2. LocalStack Support & Error Handling
LocalStack Registry Support for development and testing:
*.dkr.ecr.*.localhost.localstack.cloud:PORTAWS_ENDPOINT_URLenvironment variable support for custom ECR endpoints. If set, uses custom ECR instead of real ECR for signing requestsEnhanced Error Messages:
AWS authentication failure. Status: 400, Response: {"__type": "AccessDeniedException", "message": "User: arn:aws:iam::000000000000:user/ash is not authorized to perform: ecr:GetAuthorizationToken..."}
Fully backward compatible
Type of change
test, version modification, documentation, etc.)
Checklist