Add aws/ecr-public integration for ECR Public authentication#2231
Open
Add aws/ecr-public integration for ECR Public authentication#2231
Conversation
…c registry access - Add ECR Public support via new aws/ecr-public integration kind - Implement region validation (us-east-1 and us-west-2 supported) - Add cloud layer with GetPublicAuthorizationToken and region validation - Add integration layer with factory and execution - Update documentation for ecr-login command and tutorial - Add blog post announcing feature - Update roadmap with shipped milestone Closes #2079 (as part of Native CI/CD integration feature)
Dependency Review✅ No vulnerabilities or license issues found.Scanned Files
|
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds AWS ECR Public authentication support to Atmos via new cloud authentication module, integration layer, error sentinels, and AWS SDK dependency. Introduces token retrieval pinned to us-east-1, Docker credential management, and comprehensive documentation covering implementation details and user guidance. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
pkg/auth/cloud/aws/ecr_public.go (1)
26-32: Don't export a mutable region set.
ECRPublicSupportedRegionsbehaves like constant data, but exposing it as a writable map lets any caller change validation globally and makes concurrent access fragile. Keep the set private and expose a helper or copy if other packages need the list.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/cloud/aws/ecr_public.go` around lines 26 - 32, ECRPublicSupportedRegions is exported as a mutable map which lets callers modify global state; make it unexported (rename to ecrPublicSupportedRegions) and stop exposing the map directly, then add a safe accessor: either a function IsECRPublicSupportedRegion(region string) bool for validation or a GetECRPublicSupportedRegions() []string that returns a new slice copy of keys; update callers to use the new accessor and remove direct map usage to prevent concurrent mutation and preserve immutability.pkg/auth/integrations/aws/ecr_public_test.go (1)
31-35: Test the interface contract, not the struct internals.These assertions downcast to
*ECRPublicIntegrationand inspect private fields, so harmless implementation refactors will break the suite. Prefer assertingKind()andGetIdentity()on the returned interface instead.Based on learnings "Test behavior, not implementation; never test stub functions; avoid tautological tests; make code testable via DI; no coverage theater; remove always-skipped tests; use errors.Is() for error checking."
Also applies to: 50-52, 86-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/integrations/aws/ecr_public_test.go` around lines 31 - 35, The test currently downcasts to *ECRPublicIntegration and asserts private fields (ecrPublicIntegration.name / identity); change it to test the interface contract instead by using the returned integration variable's methods: assert integration.Kind() equals the expected kind (e.g., "ecr-public") and assert integration.GetIdentity() equals "dev-admin"; remove the downcast to ECRPublicIntegration and the direct field assertions and apply the same replacement for the other occurrences that inspect private fields (the blocks referencing ECRPublicIntegration around the other failing assertions).pkg/auth/integrations/aws/ecr_public.go (1)
36-52: Validatevia.identityduring construction for clearer failures.
identityis optional in this constructor today, so a bad config can fail later in execution with less actionable errors. Fail fast here whenvia.identityis empty.Suggested validation
identity := "" if config.Config.Via != nil { identity = config.Config.Via.Identity } + if identity == "" { + return nil, fmt.Errorf("%w: integration '%s': missing required via.identity", errUtils.ErrIntegrationFailed, config.Name) + } // Validate region if user specified one in spec.registry.As per coding guidelines: "Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/integrations/aws/ecr_public.go` around lines 36 - 52, The constructor currently allows an empty via.identity and returns an ECRPublicIntegration with identity == "" which defers failure; update the initialization logic (where identity is derived from config.Config.Via and the ECRPublicIntegration is returned) to validate that config.Config.Via != nil and config.Config.Via.Identity is non-empty and, if not, return a clear error (wrap with errUtils.ErrIntegrationFailed and include config.Name and a helpful message about missing via.identity) before creating the ECRPublicIntegration instance; keep the existing region validation for config.Config.Spec.Registry as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/auth/cloud/aws/ecr_public_test.go`:
- Around line 11-140: Add unit tests for GetPublicAuthorizationToken that cover
all error and success branches: build a test suite in ecr_public_test.go that
injects a mocked ECR Public client (or a test double) into
GetPublicAuthorizationToken to simulate (1) AWS config build failure, (2) API
call returning an error, (3) API response with missing Auth token/ExpiresAt, (4)
API response with invalid base64 Auth token, (5) API response with decoded token
missing the "username:password" format, and (6) a successful response that
returns expected username/password; use table-driven tests and assert returned
error values or parsed credentials accordingly, referencing the
GetPublicAuthorizationToken function and the client method it calls
(GetAuthorizationToken) so tests remain resilient to line changes.
In `@pkg/auth/integrations/aws/ecr_public_test.go`:
- Around line 182-193: The test TestECRPublicIntegration_Execute_NilCredentials
only asserts an error for nil creds; add unit tests that exercise
ECRPublicIntegration.Execute's happy path and the failure branches (auth token
retrieval failure, docker-config write/login failure) by injecting mocks for any
external calls (e.g., credential provider, token fetcher, docker-config writer
or registry client) so you can simulate success and each error case; create
table-driven tests that call Execute with mocked credentials and assert expected
outcomes and side-effects (token used, docker config written, errors wrapped),
targeting >80% coverage for the pkg/auth/integrations/aws package and
referencing ECRPublicIntegration.Execute and any helper interfaces used for
token retrieval and docker-config operations.
In `@website/docs/cli/configuration/settings/pro.mdx`:
- Around line 185-191: The example YAML for the pro settings omits the required
workspace_id; update the snippet that defines settings.pro to include both token
(settings.pro.token) and workspace_id (settings.pro.workspace_id) so users can
copy/paste a complete bearer-token configuration, e.g., add the workspace_id key
alongside the token key under the pro section.
In `@website/docs/cli/configuration/version/use.mdx`:
- Around line 89-97: The precedence table and explanatory note disagree: update
the table so `--use-version` and the `ATMOS_VERSION_USE` env var appear at the
same priority level (i.e., both as Priority 1) and adjust the remaining
priorities accordingly, or alternatively change the note to state that the
`--use-version` flag sets `ATMOS_VERSION_USE` internally so the flag and the env
var share the same priority; ensure the table rows reference the exact symbols
`--use-version`, `ATMOS_VERSION_USE`, `ATMOS_VERSION`, and `version.use` so they
match the explanatory sentence.
In `@website/src/data/roadmap.js`:
- Line 175: The roadmap entry object with label 'ECR Public registry
authentication' is missing its pr reference; add a pr property to that object
(matching the style used elsewhere in roadmap.js, e.g. pr: '#1234' or pr:
'https://github.com/owner/repo/pull/1234') so the shipped milestone includes the
PR link, and ensure the new property follows the same quoting/comma formatting
as other entries.
---
Nitpick comments:
In `@pkg/auth/cloud/aws/ecr_public.go`:
- Around line 26-32: ECRPublicSupportedRegions is exported as a mutable map
which lets callers modify global state; make it unexported (rename to
ecrPublicSupportedRegions) and stop exposing the map directly, then add a safe
accessor: either a function IsECRPublicSupportedRegion(region string) bool for
validation or a GetECRPublicSupportedRegions() []string that returns a new slice
copy of keys; update callers to use the new accessor and remove direct map usage
to prevent concurrent mutation and preserve immutability.
In `@pkg/auth/integrations/aws/ecr_public_test.go`:
- Around line 31-35: The test currently downcasts to *ECRPublicIntegration and
asserts private fields (ecrPublicIntegration.name / identity); change it to test
the interface contract instead by using the returned integration variable's
methods: assert integration.Kind() equals the expected kind (e.g., "ecr-public")
and assert integration.GetIdentity() equals "dev-admin"; remove the downcast to
ECRPublicIntegration and the direct field assertions and apply the same
replacement for the other occurrences that inspect private fields (the blocks
referencing ECRPublicIntegration around the other failing assertions).
In `@pkg/auth/integrations/aws/ecr_public.go`:
- Around line 36-52: The constructor currently allows an empty via.identity and
returns an ECRPublicIntegration with identity == "" which defers failure; update
the initialization logic (where identity is derived from config.Config.Via and
the ECRPublicIntegration is returned) to validate that config.Config.Via != nil
and config.Config.Via.Identity is non-empty and, if not, return a clear error
(wrap with errUtils.ErrIntegrationFailed and include config.Name and a helpful
message about missing via.identity) before creating the ECRPublicIntegration
instance; keep the existing region validation for config.Config.Spec.Registry
as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f0ec1758-7b8d-42c3-ad11-a7dd27fe83bb
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
NOTICEdocs/prd/ecr-public-authentication.mderrors/errors.gogo.modpkg/auth/cloud/aws/ecr_public.gopkg/auth/cloud/aws/ecr_public_test.gopkg/auth/integrations/aws/ecr_public.gopkg/auth/integrations/aws/ecr_public_test.gopkg/auth/integrations/types.gowebsite/blog/2026-03-18-ecr-public-authentication.mdxwebsite/docs/cli/commands/auth/ecr-login.mdxwebsite/docs/cli/configuration/configuration.mdxwebsite/docs/cli/configuration/settings/pro.mdxwebsite/docs/cli/configuration/version/use.mdxwebsite/docs/cli/configuration/version/version.mdxwebsite/docs/cli/environment-variables.mdxwebsite/docs/tutorials/ecr-authentication.mdxwebsite/src/data/roadmap.js
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2231 +/- ##
==========================================
+ Coverage 76.85% 76.90% +0.05%
==========================================
Files 1001 1003 +2
Lines 95392 95477 +85
==========================================
+ Hits 73316 73430 +114
+ Misses 17811 17784 -27
+ Partials 4265 4263 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
- Add unit tests for GetPublicAuthorizationToken covering all error/success branches - Add unit tests for ECRPublicIntegration.Execute happy path and failure branches - Introduce ECRPublicClient interface with options pattern for testability - Add injectable dependencies to ECRPublicIntegration for mock-based testing - Fix missing workspace_id in pro.mdx bearer-token example - Fix precedence table contradiction in version/use.mdx - Add pr: 2231 to ECR Public roadmap milestone Coverage: cloud/aws 81.5%, integrations/aws 87.0% (both above 80% threshold) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
Resolve conflicts in pro.mdx and version/use.mdx docs, accepting main's simplified bearer-token example and expanded version precedence table. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/auth/cloud/aws/ecr_public.go (2)
56-61: Add tracing to the exported option helper.
WithECRPublicClientis public, but it skips theperf.Trackhook used by the other public functions in this file. Keeping that contract consistent avoids holes in tracing.♻️ Proposed fix
func WithECRPublicClient(client ECRPublicClient) ECRPublicAuthOption { + defer perf.Track(nil, "aws.WithECRPublicClient")() + return func(c *ecrPublicAuthConfig) { c.client = client } }As per coding guidelines: "Add
defer perf.Track(atmosConfig, "pkg.FuncName")()+ blank line to all public functions, usenilif no atmosConfig param".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/cloud/aws/ecr_public.go` around lines 56 - 61, The exported helper WithECRPublicClient is missing the perf.Track tracing hook used by other public functions; update WithECRPublicClient (the function that returns ECRPublicAuthOption and mutates ecrPublicAuthConfig) to call defer perf.Track(nil, "pkg.WithECRPublicClient")() as the first statement in the function body and add a blank line after that defer to match the file's convention so tracing is consistent for all public helpers.
34-39: Pin the mock generator version.Using
go run ...mockgen@latestmakes the generated file depend on whatever version is current whengo generateruns. Please lock this to a fixed version, ideally the one already declared ingo.mod, so mock output stays reproducible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/cloud/aws/ecr_public.go` around lines 34 - 39, Replace the go:generate directive that currently uses "go run go.uber.org/mock/mockgen@latest" with a pinned module version: run "go list -m -json go.uber.org/mock/mockgen" or inspect go.mod to find the exact version used, then change the directive to "go run go.uber.org/mock/mockgen@<version>" (keeping the same -source=ecr_public.go and -destination=mock_ecr_public_client_test.go flags). This ensures the mock for ECRPublicClient is reproducible; update the go:generate line in the same file where ECRPublicClient is declared.pkg/auth/integrations/aws/ecr_public_test.go (1)
204-219: Generate the Docker writer mock instead of hand-rolling it.This double only captures arguments; it can't express expectations like "must not be called", so the token-error path still can't prove
WriteAuthwas skipped. Please switchdockerAuthWriterto amockgen-generated mock like theECRPublicClientone in this PR.As per coding guidelines: "Use go.uber.org/mock/mockgen with //go:generate directives for mock generation - never create manual mocks".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/integrations/aws/ecr_public_test.go` around lines 204 - 219, The test uses a hand-rolled mock type mockDockerWriter and its WriteAuth method which only captures args and cannot express expectations (e.g. "must not be called"); replace it with a mockgen-generated mock for the dockerAuthWriter interface (similar to the ECRPublicClient mock used elsewhere). Add a //go:generate directive for go.uber.org/mock/mockgen, run mockgen to produce the concrete mock type, update tests to import and use that generated mock (set EXPECT() expectations like NotToBeCalled or Return) and remove the manual mockDockerWriter type and its WriteAuth implementation so tests can assert call/non-call behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@website/src/data/roadmap.js`:
- Line 175: Find the initiatives array and update the auth initiative's progress
field (the object with label: 'auth') to account for the newly shipped milestone
— change progress from 80 to 81 so the roadmap bar reflects the added shipped
item; ensure the new milestone already includes pr: 2231 and changelog:
'ecr-public-authentication' as shown.
---
Nitpick comments:
In `@pkg/auth/cloud/aws/ecr_public.go`:
- Around line 56-61: The exported helper WithECRPublicClient is missing the
perf.Track tracing hook used by other public functions; update
WithECRPublicClient (the function that returns ECRPublicAuthOption and mutates
ecrPublicAuthConfig) to call defer perf.Track(nil, "pkg.WithECRPublicClient")()
as the first statement in the function body and add a blank line after that
defer to match the file's convention so tracing is consistent for all public
helpers.
- Around line 34-39: Replace the go:generate directive that currently uses "go
run go.uber.org/mock/mockgen@latest" with a pinned module version: run "go list
-m -json go.uber.org/mock/mockgen" or inspect go.mod to find the exact version
used, then change the directive to "go run go.uber.org/mock/mockgen@<version>"
(keeping the same -source=ecr_public.go and
-destination=mock_ecr_public_client_test.go flags). This ensures the mock for
ECRPublicClient is reproducible; update the go:generate line in the same file
where ECRPublicClient is declared.
In `@pkg/auth/integrations/aws/ecr_public_test.go`:
- Around line 204-219: The test uses a hand-rolled mock type mockDockerWriter
and its WriteAuth method which only captures args and cannot express
expectations (e.g. "must not be called"); replace it with a mockgen-generated
mock for the dockerAuthWriter interface (similar to the ECRPublicClient mock
used elsewhere). Add a //go:generate directive for go.uber.org/mock/mockgen, run
mockgen to produce the concrete mock type, update tests to import and use that
generated mock (set EXPECT() expectations like NotToBeCalled or Return) and
remove the manual mockDockerWriter type and its WriteAuth implementation so
tests can assert call/non-call behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 54c6a18b-9cf1-4e9a-92b2-a7e24ed15260
📒 Files selected for processing (8)
pkg/auth/cloud/aws/ecr_public.gopkg/auth/cloud/aws/ecr_public_test.gopkg/auth/cloud/aws/mock_ecr_public_client_test.gopkg/auth/integrations/aws/ecr_public.gopkg/auth/integrations/aws/ecr_public_test.gowebsite/docs/cli/configuration/settings/pro.mdxwebsite/docs/cli/configuration/version/use.mdxwebsite/src/data/roadmap.js
🚧 Files skipped from review as they are similar to previous changes (3)
- website/docs/cli/configuration/version/use.mdx
- pkg/auth/cloud/aws/ecr_public_test.go
- pkg/auth/integrations/aws/ecr_public.go
| { label: 'Automatic EKS kubeconfig tied to identities', status: 'in-progress', quarter: 'q4-2025', pr: 1903, changelog: 'helmfile-eks-modernization', description: 'Automatic kubeconfig generation for EKS clusters using Atmos-managed AWS credentials with flexible cluster name configuration.', benefits: 'No aws eks update-kubeconfig commands. Kubectl works immediately after Atmos auth.' }, | ||
| { label: 'Flexible EKS cluster name configuration', status: 'in-progress', quarter: 'q4-2025', pr: 1903, changelog: 'helmfile-eks-modernization', description: 'Four-level precedence for EKS cluster names: --cluster-name flag, cluster_name config, cluster_name_template (Go templates), or legacy cluster_name_pattern. EKS integration is now opt-in with use_eks setting.', benefits: 'Use Go templates for dynamic cluster names. Non-EKS Kubernetes clusters work without EKS configuration.' }, | ||
| { label: 'Automatic ECR authentication tied to identities', status: 'shipped', quarter: 'q4-2025', pr: 1859, docs: '/tutorials/ecr-authentication', changelog: 'ecr-authentication-integration', description: 'Native ECR login for container image operations without external tooling.', benefits: 'Docker push/pull to ECR works without aws ecr get-login-password or external credential helpers.', category: 'featured', priority: 'high' }, | ||
| { label: 'ECR Public registry authentication', status: 'shipped', quarter: 'q1-2026', pr: 2231, changelog: 'ecr-public-authentication', description: 'Authenticated pulls from public.ecr.aws to eliminate rate limits on public ECR images.', benefits: 'CI builds pulling BuildKit, binfmt, or other public ECR images no longer hit unauthenticated rate limits.' }, |
Contributor
There was a problem hiding this comment.
Bump the auth initiative progress too.
This adds another shipped auth milestone, but the auth initiative's progress value above stays at 80. Please update that percentage in the same change so the roadmap bar reflects the new milestone state.
Based on learnings: Applies to website/src/data/roadmap.js : PRs labeled minor/major must update website/src/data/roadmap.js with shipped milestones, changelog links, PR references, and progress percentages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@website/src/data/roadmap.js` at line 175, Find the initiatives array and
update the auth initiative's progress field (the object with label: 'auth') to
account for the newly shipped milestone — change progress from 80 to 81 so the
roadmap bar reflects the added shipped item; ensure the new milestone already
includes pr: 2231 and changelog: 'ecr-public-authentication' as shown.
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
what
Add
aws/ecr-publicintegration kind to Atmos for authenticated access to AWS ECR Public (public.ecr.aws). Solves Docker rate limiting on public ECR images by enabling authenticated pulls with significantly higher or no rate limits.Key changes:
pkg/auth/cloud/aws/ecr_public.go):GetPublicAuthorizationToken()function that callsecrpublic:GetAuthorizationTokenAPI, always using us-east-1 regionpkg/auth/integrations/aws/ecr_public.go):ECRPublicIntegrationfactory that registers the newaws/ecr-publickind, validates regions at config timewhy
Docker pulls from
public.ecr.awshit rate limits when unauthenticated. This blocks CI workflows, especially those usingcloudposse/github-action-docker-build-pushwhich pulls BuildKit/binfmt images on every run. Authenticated pulls have significantly higher (or no) rate limits.ECR Public differs fundamentally from private ECR: uses
ecrpublicSDK service, bearer token instead of SigV4, hardcoded us-east-1 auth region, fixedpublic.ecr.awsregistry URL. Requiresecr-public:GetAuthorizationTokenandsts:GetServiceBearerTokenIAM permissions.references
Summary by CodeRabbit
New Features
Documentation
Chores