-
Notifications
You must be signed in to change notification settings - Fork 53
host-ctr: use docker resolver #760
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
base: develop
Are you sure you want to change the base?
Conversation
e9a464e to
1270ef8
Compare
f3e0301 to
0f5fa44
Compare
sources/host-ctr/cmd/host-ctr/ecr.go
Outdated
| }) | ||
| authorizer := docker.NewDockerAuthorizer(authOpt) | ||
| c.Resolver = docker.NewResolver(docker.ResolverOptions{ | ||
| // TODO: Consider adding support for user-provided credentials with registryConfig as fallback, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same TODO as before:
https://github.com/bottlerocket-os/bottlerocket-core-kit/blob/develop/sources/host-ctr/cmd/host-ctr/main.go#L1250
0f5fa44 to
48d1c7e
Compare
| // | ||
| // Capture groups: [1] = account ID, [2] = "-fips" or empty, [3] = region | ||
| // | ||
| // ECR hostname pattern also used in the ecr-credential-provider: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've since deviated from this in order to support .eu domain suffix.
I think this regex predates your PR. It's a gnarly one to read. It would be great if we could reign it in or do away with it somehow. I know in Python regexes have "verbose" mode where you can add inline comments explaining parts of the regex.
I want to say I remember interacting with this one in the past, so there's a chance I tried to do battle with it and failed. Might be a dead end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to better match:
https://github.com/kubernetes/cloud-provider-aws/blob/d1c7c02d2da22e87175802ec94c73bd8871691bc/cmd/ecr-credential-provider/main.go#L46
which includes eu and also I added in-line comments.
Let me know what you think!
sources/host-ctr/cmd/host-ctr/ecr.go
Outdated
| // A set of the currently supported FIPS regions for ECR: https://docs.aws.amazon.com/general/latest/gr/ecr.html | ||
| // FIPS-supported ECR regions: https://docs.aws.amazon.com/general/latest/gr/ecr.html | ||
| var fipsSupportedEcrRegionSet = map[string]bool{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: dupe comment and the official list from the link appears to be larger now.
not nit: Is this something we can lean on the SDK for now? It seems like in the old code we needed to understand if we were doing FIPS to avoid hitting an error condition in the resolver.
Now we only use it to raise an error - but the SDK might take care of that for us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: dupe comment
Fixed.
Re: Not Not:
I did dig into this before the PR... and checked the SDK. U
Unfortunately it won't catch this for us. The SDK has a default FIPS hostname template (ecr-fips.{region}.amazonaws.com) that it falls back to for any region, so requesting FIPS for eu-west-1 (a non-fips region) would just construct ecr-fips.eu-west-1.amazonaws.com without error.
The failure would only happen later as a DNS/connection error, which would be confusing for users.
Keeping the validation gives a clear "invalid FIPS region" error upfront.
Also ECR doesn't appear to support all FIPS endpoints, which is another variable here:
https://aws.amazon.com/compliance/fips/ - see ca-west-1
the official list from the link appears to be larger now.
This is not quite true, I have updated the comment to be more clear. We current support the list of fips end points that ECR supports. There is not new FIPS regions for ECR.
48d1c7e to
7dbbae0
Compare
Signed-off-by: Kyle Sessions <[email protected]>
Signed-off-by: Kyle Sessions <[email protected]>
Signed-off-by: Kyle Sessions <[email protected]>
7dbbae0 to
5869966
Compare
Replace the amazon-ecr-containerd-resolver dependency with direct implementation using containerd's Docker resolver. Signed-off-by: Kyle Sessions <[email protected]>
5869966 to
08a14e4
Compare
cbgbt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Issue Related to aws/amazon-ecs-agent#4538
Code change dependency: bottlerocket-os/bottlerocket#4715
Description of changes:
Recent ecs-agent updates introduced an incompatibility when using FIPS ECR endpoints alongside use_fips_endpoint=true, requiring us to choose one approach.
We opted to let users specify FIPS ECR endpoints directly. However, amazon-ecr-containerd-resolver doesn't support FIPS endpoints without use_fips_endpoint=true, and the library has been tech debt we've wanted to migrate away from.
This change replaces amazon-ecr-containerd-resolver with containerd's Docker resolver.
Testing done:
ECS Agent Conformance Testing
Ran internal ECS conformance tests across multiple variants and architectures with use_fips_endpoint=false:
Additionally verified ECS task execution with both FIPS and non-FIPS containers to confirm expected behavior.
Host Container Image Pull Testing
GovCloud (us-gov-west-1)
China Region (cn-north-1)
Special Region Test For New Regions (ap-southeast-7)
Verified host container image pull works in special region:
Digest test:
Details
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.