feat(acrauth): add ACR login helper that writes container auth files - #321
Conversation
|
Hi Wenqi He (@weherdh). Thanks for your PR. I'm waiting for a Azure member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Pull request overview
Adds a new tools/acrauth Go module intended to provide a stable, CLI-internal ACR authentication flow (managed identity → ACR refresh token) and safely upsert the resulting credential into a container auth file without clobbering other registry entries.
Changes:
- Introduces
acrauth loginCobra command with option validation/completion and Azure credential selection (default vs user-assigned MI). - Implements ACR refresh-token exchange (with bounded retry) and auth-file upsert logic that preserves unrelated registries/fields and writes via temp file + rename.
- Wires the new module into the repo workspace (
go.work) and adds module/workspace dependency sums.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/acrauth/options.go | CLI flag binding and options lifecycle; constructs Azure credential and performs login flow. |
| tools/acrauth/command.go | Defines the login Cobra command and runs validation/completion/login with interrupt-aware context. |
| tools/acrauth/acr.go | Implements Entra/ARM token → ACR refresh token exchange and retry loop for role propagation delays. |
| tools/acrauth/authfile.go | Upserts a single registry credential into a container auth JSON file via read/merge + atomic replace. |
| tools/acrauth/authfile_test.go | Unit tests covering merge-preservation, replacement, file creation/permissions, and empty-registry rejection. |
| tools/acrauth/go.mod | New module definition and direct/indirect dependency list for the acrauth tool. |
| tools/acrauth/go.sum | Checksums for the new module’s dependencies. |
| go.work | Adds ./tools/acrauth to the workspace modules list. |
| go.work.sum | Updates workspace sums to account for newly introduced dependencies. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tools/acrauth/acr.go:40
acrFQDNis treated as a bare registry host, but if a caller passes a URL (e.g.https://myregistry.azurecr.io) or includes a path/trailing slash,fmt.Sprintf("https://%s", acrFQDN)will produce an invalid endpoint and a misleading hostname (e.g. host becomeshttps). Consider validating the input and failing fast with a clear error.
// ExchangeForRefreshToken trades an Entra token for an ACR refresh token, which is what
// container tooling stores as the registry password.
func ExchangeForRefreshToken(ctx context.Context, cred azcore.TokenCredential, acrFQDN string) (string, error) {
endpoint, err := url.Parse(fmt.Sprintf("https://%s", acrFQDN))
if err != nil {
return "", fmt.Errorf("failed to parse ACR endpoint: %w", err)
}
tools/acrauth/acr.go:82
- This loop won't compile:
rangecannot be used over anintconstant. Use a conventional counter loop so the retry behavior actually builds and runs.
for attempt := range retryAttempts {
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
tools/acrauth/command.go:31
- The command ignores positional args; without an Args validator, extra arguments are accepted silently even though
Use: "login"implies none. AddArgs: cobra.NoArgsso user mistakes fail fast.
cmd := &cobra.Command{
Use: "login",
Short: "Authenticate to an Azure Container Registry and record the credential in a container auth file",
SilenceUsage: true,
SilenceErrors: true,
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tools/acrauth/authfile.go:110
tmp.Write(raw)can (rarely) return a short write witherr == nil, which would produce a truncated auth file and break subsequent reads. It’s safer to assert the full buffer was written before proceeding.
if _, err := tmp.Write(raw); err != nil {
_ = tmp.Close()
return fmt.Errorf("failed to write temporary auth file: %w", err)
}
if err := tmp.Close(); err != nil {
|
Validated end-to-end against a live ACR, driven from the consumer PR (Azure/ARO-HCP#6668) with a local Against
Full container run — oc-mirror image built with this binary, real mirror, no So Two bugs surfaced during that testing and are already fixed in this branch:
Only the managed-identity branch of |
|
/ok-to-test |
Rael Garcia (raelga)
left a comment
Rael Garcia (raelga)
left a comment
There was a problem hiding this comment.
PR-standard note: please link a Jira or GitHub tracking issue as required by CONTRIBUTING.md. The current references are both pull requests.
The oc-mirror image authenticates to ACR by running 'az acr login' with DOCKER_COMMAND pointed at a shim script, so that az's internal 'docker login' call is intercepted and the credential can be scraped out of the argv. That coupling broke silently when azure-cli 2.88.0 switched to --password-stdin, and the shim's jq rewrite also clobbered the source-registry credentials that the init container had already written to the same auth file. Do the exchange directly instead: trade the managed identity's Entra token for an ACR refresh token and upsert a single registry key into the auth file, leaving other registries and unmodelled fields intact.
…t be renamed over A bind-mounted auth file cannot be replaced with rename(2) -- the kernel returns EBUSY. The oc-mirror container mounts auth.json directly, so the atomic write path fails there and the login aborts. Fall back to writing through the existing file in that case.
A non-nil pointer to an empty string would have been written out as a credential and reported as success -- the same silent-success failure this tool exists to remove.
os.WriteFile only applies its mode argument when it creates the file, so the fallback path left a pre-existing credential file at whatever mode it already had -- observed as 0644 in a container run. Chmod it explicitly, and stop claiming atomicity for a path that cannot have it.
Updated that in the description: https://redhat.atlassian.net/browse/AROSLSRE-1819 |
containers/image prefers a non-empty identitytoken over auth and uses it for the OAuth refresh-token flow, so a stale token left on the entry we are replacing silently shadows the credential we just wrote -- oc-mirror would authenticate with the old value and fail. Delete it on the entry being replaced, while still preserving unrelated registries and their fields. Also normalise the files to LF line endings so gofmt and gci pass, and tidy go.sum.
e6d42e1 to
24fe0aa
Compare
os.WriteFile only applies its mode when it creates the file, so writing the refreshed token into an existing world-readable file exposed it until the follow-up chmod landed. Chmod first and treat ErrNotExist as fine, since WriteFile then creates the file at 0600. Also commit the 'make tidy' output the verify job requires, including the go work sync results across the workspace.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 33 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
tools/acrauth/options.go:80
--registryis only validated for non-empty, so values likehttps://myregistry.azurecr.ioormyregistry.azurecr.io/some/pathwill be accepted and then mis-parsed in the token exchange, leading to confusing downstream errors. Consider rejecting URLs/paths early with a clearer validation error.
func (o *RawOptions) Validate() (*ValidatedOptions, error) {
if o.Registry == "" {
return nil, errors.New("--registry is required")
}
if o.AuthFile == "" {
tools/acrauth/command.go:31
- The command currently accepts and ignores positional arguments. This can hide user mistakes (e.g., typos or copy/pasted extra args). If the command doesn't take args, set
Args: cobra.NoArgsso Cobra will surface a clear error.
cmd := &cobra.Command{
Use: "login",
Short: "Authenticate to an Azure Container Registry and record the credential in a container auth file",
SilenceUsage: true,
SilenceErrors: true,
The tidy target is not idempotent in a single pass: 'go mod tidy' prunes each module's go.sum, then 'go work sync' adds back hashes the workspace build list needs. A second pass adds 48 go.sum lines that the first pass dropped, which is what the verify job was reporting.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 33 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
tools/acrauth/authfile_test.go:112
readAuthFileexplicitly treats a 0-byte auth file as empty input, but there is no test covering that behavior (only missing-file and malformed/valid JSON paths are exercised). Adding a test helps ensure regressions don’t reintroduce clobbering/parse failures for empty files.
func TestUpsertCredentialRejectsEmptyRegistry(t *testing.T) {
path := filepath.Join(t.TempDir(), "auth.json")
require.Error(t, UpsertCredential(path, "", NullGUIDUsername, "token"))
}
tools/acrauth/acr.go:82
for attempt := range retryAttemptsdoes not compile becauserangecan't be used over anintconstant; this makes the retry helper unusable.
for attempt := range retryAttempts {
|
Re-tested with a hostile fixture after the Seeded the entry being replaced with a stale identity token, and an unrelated registry with its own:
After
Same fixture through the real container — oc-mirror image built with this binary, actual mirror, no With the previous code the stale token would have won and oc-mirror would have authenticated with the old credential. Re-read the auth file after the run to confirm the ACR entry still had Also in this push: LF line endings ( |
Rael Garcia (raelga)
left a comment
Rael Garcia (raelga)
left a comment
There was a problem hiding this comment.
/lgtm
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: raelga, weherdh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Why
The oc-mirror image authenticates to ACR by running
az acr loginwithDOCKER_COMMANDpointed at a shim script, so that az's internal
docker logincall is intercepted and thecredential scraped out of
argv. Two problems:--password-stdin— the shim'spositional args shifted and it wrote a garbage credential. Mirroring reported success
while syncing nothing.
jqrewrite overwrote the auth file, clobbering theregistry.redhat.iocredentials an init container had already written there.
Coupling our auth to the internals of a CLI we don't pin is not a stable contract.
What
This is to resolve https://redhat.atlassian.net/browse/AROSLSRE-1819
A small
acrauth logincommand that does the exchange directly:acr.go— trades the managed identity's Entra token for an ACR refresh token, withbounded retry for role-assignment propagation.
authfile.go— upserts a single registry key into the container auth file.Round-trips through
map[string]anyso other registries and fields we don't model(
email,identitytoken) survive. Writes via temp file + atomic rename.Tests cover the merge-preserves-other-registries case, credential replacement, cold start
with file-mode assertion, and empty input.
Note on #257
This overlaps with the ACR token exchange in #257, where the equivalent helpers are
unexported. Happy to rebase onto that and drop
acr.goif it lands first — or the reverse.The rest is disjoint: #257 replaces
on-demand.shand copies via ORAS in-process, so itnever writes an auth file; oc-mirror is an external binary that must read one.
Follow-up
The ARO-HCP change (thin
main, Dockerfile,mirror.sh, deleting the shim) landsseparately once this is available as a pseudo-version.