feat(dev-infrastructure): migrate manage-acr-replication.sh to a Go CLI (AROSLSRE-1596) - #6369
Conversation
…LI (AROSLSRE-1596) Replace the ocp-acr-replication and svc-acr-replication region-pipeline Shell steps' shell script with a small Go CLI (scripts/acr-replication) using the armcontainerregistry SDK. It reproduces the script's create + endpoint-set + drift-reconcile behavior with unit test coverage for the desired-endpoint logic, following the same pattern as scripts/postgres-access and scripts/grafana-group-roles. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR migrates the per-region ACR replication management logic from a Bash script to a Go CLI, and updates the region EV2 pipeline to build and invoke that binary during rollouts.
Changes:
- Add a new Go module/CLI (
dev-infrastructure/scripts/acr-replication) that manages ACR replications via thearmcontainerregistrySDK. - Remove the legacy
dev-infrastructure/scripts/manage-acr-replication.shscript. - Update
dev-infrastructure/region-pipeline.yamlto prebuild the binary and run it in theocp-acr-replication/svc-acr-replicationsteps; add the module togo.work.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| go.work | Adds the new Go script module to the workspace. |
| dev-infrastructure/scripts/manage-acr-replication.sh | Removes the legacy Bash implementation. |
| dev-infrastructure/scripts/acr-replication/main.go | Introduces the Go CLI implementation for replica create/recreate/update. |
| dev-infrastructure/scripts/acr-replication/main_test.go | Adds unit tests for env parsing and desired endpoint-state logic. |
| dev-infrastructure/scripts/acr-replication/go.mod | New standalone Go module for the CLI. |
| dev-infrastructure/scripts/acr-replication/go.sum | Dependency locks for the new module. |
| dev-infrastructure/scripts/acr-replication/.gitignore | Ignores the compiled binary output. |
| dev-infrastructure/region-pipeline.yaml | Builds the CLI in buildStep and updates replication steps to run it with explicit env vars. |
…n (AROSLSRE-1596) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
ci/prow/verify failed with a go.sum drift in the new acr-replication module (verify-deepcopy step, which also runs go mod tidy across all modules): github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources v1.2.0/go.mod were missing their hash lines. Ran go mod tidy in dev-infrastructure/scripts/acr-replication locally and it produced the identical diff, so this is the mechanical consequence of this PR adding the new module, not an unrelated issue. Pushed 9758c41 with the fix. |
…oncile and location-based replica lookup (AROSLSRE-1596) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Pushed 1791c06 addressing the two Copilot review findings, both genuine behavior-preservation bugs vs the original shell script:
go build/vet/test all pass locally. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
dev-infrastructure/scripts/acr-replication/main.go:250
- The old script supported DRY_RUN; without a guard here, what-if runs can still mutate endpoint routing via BeginUpdate. Please make endpoint updates a no-op when DRY_RUN is set.
func createReplication(ctx context.Context, client *armcontainerregistry.ReplicationsClient, cfg *config, desiredEnabled bool) error {
slog.Info("creating replication", "region", cfg.region, "endpointEnabled", desiredEnabled)
dev-infrastructure/scripts/acr-replication/main.go:236
- The previous shell script honored DRY_RUN; without a DRY_RUN guard here,
make -C dev-infrastructure region.what-ifcan delete replicas if it encounters a Failed state. Please ensure delete operations are no-ops in DRY_RUN mode.
if err != nil {
return nil, fmt.Errorf("list replications: %w", err)
dev-infrastructure/scripts/acr-replication/main.go:206
- This changes behavior compared to manage-acr-replication.sh: the shell script only reconciled endpoint routing when the desired state was disabled (it explicitly skipped reconciliation when desired was enabled). Here, any drift (including re-enabling) will be reconciled. If the intent is truly “no behavior change”, consider keeping the old semantics (only enforce the disabled list) or update the PR description to call out the behavior change.
if err := deleteReplication(ctx, client, cfg, name); err != nil {
return err
}
return createReplication(ctx, client, cfg, desiredEnabled)
case armcontainerregistry.ProvisioningStateSucceeded:
…RE-1596) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
dev-infrastructure/scripts/acr-replication/main_test.go:101
- TestIsNotFound currently only exercises the false cases; add a positive-case assertion so regressions where isNotFound never returns true are caught.
func TestIsNotFound(t *testing.T) {
if isNotFound(nil) {
t.Fatalf("nil error should not be not-found")
}
if isNotFound(errPlain("boom")) {
t.Fatalf("plain error should not be not-found")
}
}
dev-infrastructure/scripts/acr-replication/main.go:123
- ENDPOINT_DISABLED_REGIONS matching is currently case-sensitive (map lookup on c.region), but other region comparisons (e.g. home-region check) are case-insensitive. This can silently skip disabling if the env var uses different casing. Normalize both the disabled list entries and the lookup key to a consistent case (e.g. strings.ToLower).
c.disabledRegions = map[string]bool{}
for _, r := range strings.Fields(env("ENDPOINT_DISABLED_REGIONS")) {
c.disabledRegions[r] = true
}
return c, nil
dev-infrastructure/scripts/acr-replication/main_test.go:20
- To add a positive-case assertion for TestIsNotFound, the test file needs to import azcore (with a blank line separating stdlib and third-party imports to satisfy gci ordering).
This issue also appears on line 94 of the same file.
import (
"strings"
"testing"
)
|
Pushed d5de8b1 addressing another genuine Copilot finding: the original shell script wraps mutating az calls in an execute() function gated by DRY_RUN, used by make region.what-if to keep it non-mutating. The Go CLI was ignoring DRY_RUN entirely, so what-if runs would have actually created/deleted/updated replicas. Added a dryRun field to config (parsed from DRY_RUN env var, same as the original) and gated createReplication/deleteReplication/reconcileEndpoint to log-only when set. go build/vet/test pass locally. |
…ly for name discovery (AROSLSRE-1596) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Pushed 1370fc6 addressing a third genuine Copilot finding: findReplicationByLocation was returning Properties straight from the List response, but the replaced shell script explicitly avoided reading state from az resource list / az acr replication list due to known bugs reporting the wrong provisioning/endpoint state, doing a separate az resource show on the discovered replica ID instead. Renamed the helper to findReplicationNameByLocation, it now only returns the name from the List match; reconcile() then calls Get on that name for authoritative state. go build/vet/test pass locally. |
…response (AROSLSRE-1596) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Pushed 209eab1 for a defensive fix on top of the earlier list/get change: Copilot flagged that some ARM list responses return nested-resource names as "/" (the shell script's cut -f 2 -d "/" existed for exactly this). findReplicationNameByLocation now takes only the last "/" segment of the returned name before using it, so Get/Update/Delete always receive the bare replication name regardless of the SDK's list response shape. go build/vet/test pass locally. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
dev-infrastructure/scripts/acr-replication/main.go:145
- azidentity.NewDefaultAzureCredential(nil) enables the full default credential chain (including shared token cache), which differs from the pattern used in most of this repo (and in dev-infrastructure/scripts/grafana-group-roles) where RequireAzureTokenCredentials is set to satisfy CodeQL/security scanning and to constrain credential sources. Consider passing DefaultAzureCredentialOptions{RequireAzureTokenCredentials:true} here as well; it should still work for EV2 MSI/workload identity and local
az login.
// DefaultAzureCredential resolves to the rollout managed identity in EV2
// and to the operator's `az login` locally; it never prompts interactively.
cred, err := azidentity.NewDefaultAzureCredential(nil)
if err != nil {
return fmt.Errorf("azidentity: %w", err)
|
/test e2e-parallel Retesting: pull-ci-Azure-ARO-HCP-main-e2e-parallel/2084167757842092032 failed entirely inside infra provisioning, before any ACR/replication step ran. The failing step is swift-vnet-aks-net (network setup container), which timed out on DNS readiness for login.microsoftonline.com after 49 retries over 480s: [swift-vnet] dns readiness (login.microsoftonline.com): giving up after 49 attempt(s) / 488s (limit 480s) This is a network/DNS cold-start flake in the shared CI infra, unrelated to this PR's diff (a standalone Go CLI module + pipeline wiring, no changes to VNet/networking setup). No failed ARM deployments were found either (gather-provision-failure step reported none). |
|
/test e2e-parallel Retesting: build 2084177988856844288 failed across many unrelated specs (KMS key rotation, cilium CNI, back-level 4.19, Authorized CIDRs, boot diagnostics 409, candidate-channel 4.21 install), all with the same signature: context deadline exceeded (13x) None of these specs touch ACR replication (this PR's only change). Same job is also failing right now with matching signatures on other unrelated open PRs (#6370, #6365, #6364), while passing on others (#6371, #6368) - consistent with a shared-CI/fleet-wide episode (API server or route connectivity flakiness across many guest clusters), not something caused by this PR's diff. |
| case armcontainerregistry.ProvisioningStateFailed: | ||
| slog.Info("replication is in failed state; deleting and recreating", "region", cfg.region, "name", name) | ||
| if err := deleteReplication(ctx, client, cfg, name); err != nil { | ||
| return err | ||
| } | ||
| return createReplication(ctx, client, cfg, desiredEnabled) |
There was a problem hiding this comment.
I assume this was already the case with the script, but I'm not sure this is the right place to perform reconcile-like operations. This will only run when the step changes for incremental rollouts, so it can be confusing when someone wonders "why the script didn't reconcile the ACR replication if it was in a failed state?".
There was a problem hiding this comment.
Checked this: it's pre-existing behavior, not something this PR introduces. The original manage-acr-replication.sh was wired into region-pipeline.yaml the exact same way (per-region Shell step in ocp-acr-replication/svc-acr-replication), so drift-reconciliation already only ran when the pipeline step executed, same as here.
The one thing that did change is workingDir: the old script used ./scripts, this port uses . (whole repo root), matching the postgres-access precedent already in svc-pipeline.yaml. I haven't been able to confirm from our docs whether workingDir: . is treated the same as leaving it unset (which docs/pipeline-concept.md says always re-runs in incremental mode) vs a narrower, cacheable working dir, so I can't say with certainty whether this makes the step run on literally every rollout or not.
Given it's unchanged from the original script's behavior, I'm leaving it out of scope for this PR. Filed AROSLSRE-1699 to track evaluating whether the reconcile logic needs a trigger independent of pipeline-step caching (e.g. a scheduled/periodic reconcile), or at minimum documenting the current limitation explicitly in the CLI's package doc.
There was a problem hiding this comment.
Follow-up: opened #6398 to actually fix this.
I got this wrong on the first attempt (unsetting workingDir entirely, closed as #6382): that breaks EV2, since EV2 needs workingDir set to build a scoped package archive for the step. Unset falls back to an implicit whole-repo archive that fails in EV2.
The real problem was that workingDir: . scopes the archived/hashed package to the whole repo, so the incremental-rollout cache key ends up decoupled from what the step actually depends on. #6398 scopes workingDir down to the step's own scripts/acr-replication folder instead (same pattern already used by housekeeping, upgrade-aks-cluster, etc.), so it keeps the non-empty workingDir EV2 needs, but the cache now actually reflects this step's own inputs.
Tracked in AROSLSRE-1699.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: raelga, roivaz 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 |
…ia configRef (AROSLSRE-1699) Replace acr.ocp.regionEndpointDisabledRegions (a single global space-separated list of regions, checked for membership in the CLI) with acr.ocp.replicationState (a per-region boolean), threaded to the ocp-acr-replication step as REPLICATION_STATE. Each region now carries its own desired replication endpoint state via the normal region-override config mechanism (config.msft.clouds-overlay.yaml regions.<name>.acr.ocp.replicationState), the same pattern already used for other per-region overrides like kusto.location. The Go CLI reads REPLICATION_STATE directly and acts on it idempotently, instead of parsing a list and checking region membership. eastus2euap keeps its override (regionEndpointDisabledRegions: 'eastus2euap' -> regions.eastus2euap.acr.ocp.replicationState: false), everything else defaults to enabled (true). Suggested by Steve K. as a config-shape simplification during review of PR Azure#6369.
AROSLSRE-1596
What
Replaces
dev-infrastructure/scripts/manage-acr-replication.shwith a Go CLI (dev-infrastructure/scripts/acr-replication) that theocp-acr-replicationandsvc-acr-replicationregion-pipeline steps now call. It reproduces the script's behavior using thearmcontainerregistrySDK: create the replica if missing, delete and recreate it if it's stuckFailed, and reconcile the regional data endpoint on drift, all gated by the ACR home-region check and theregionEndpointDisabledRegionslist. The old shell script is removed andregion-pipeline.yamlgets abuildStepto compile the binary, following the same pattern asscripts/postgres-accessandscripts/grafana-group-roles.Why
The shell script grew conditional branching and drift comparison (from AROSLSRE-1593) with no automated tests. Bugs here directly affect prod ACR data-path routing, which was the root cause of AROSLSRE-1592. Moving this to Go with unit tests lets the logic evolve safely.
Testing
go test ./...passes for the new module, covering the required-env-var validation and the desired-endpoint-state logic (default enabled, region in/out of the disabled list).make validate-config-pipelinespasses against the updatedregion-pipeline.yaml. Manually built the binary via the exactbuildStepcommand sequence and confirmed it runs and reports missing env vars correctly.Special notes for your reviewer
No behavior change intended: same replica naming (region name), same home-region no-op, same failed-state recreate, same endpoint drift reconcile.
SUBSCRIPTION_IDandRESOURCE_GROUPare now passed explicitly as step variables instead of being derived inside the script viaaz acr show, since the ARM SDK needs the resource group up front.PR Checklist