fix(dev): move prd root Application to bootstrap for reproducibility - #2550
fix(dev): move prd root Application to bootstrap for reproducibility#2550asad-miah wants to merge 3 commits into
Conversation
prd-base depended on two fixes applied by hand on the cluster (Prune=false annotation, removed tracking-id) that git could not express because base.yaml was excluded from its own kustomization. Moving it to kubernetes/bootstrap/prd-base.yaml makes the whole arrangement reproducible, and a missing prd-base now self-heals during release instead of failing as PermissionDenied. Signed-off-by: Md Asaduzzaman Miah <asad.miah@rhesis.ai>
There was a problem hiding this comment.
Overall this makes the prd root Application location/self-healing behavior much more reproducible and the README updates are clear.
Improvement: tighten the “prd-base missing” guard in publish-release.yml so we only app create when we’re sure it’s actually missing (not just any CLI failure).
Found 1 issue (1 improvement).
| # Guarded rather than --upsert: upsert would reset targetRevision to the | ||
| # committed value and automated sync could fire on it before the pin below. | ||
| if ! argocd app get prd-base \ | ||
| --server "$ARGOCD_SERVER" --auth-token "$ARGOCD_AUTH_TOKEN" --grpc-web >/dev/null 2>&1; then |
There was a problem hiding this comment.
Improvement: This argocd app get check treats any failure as “missing app”, so transient connectivity issues / TLS errors / RBAC misconfig could incorrectly trigger app create and then fail the job even if prd-base actually exists.
Fix: capture and inspect the error output and only create on the specific “missing app” signature you’re expecting, otherwise fail with the original error. Alternatively: attempt
argocd app setfirst; if it fails with the “missing” error, thenapp createand retryapp set/sync.
There was a problem hiding this comment.
the argocd app get check is a bare one-shot call with no retry — every other ArgoCD call in this same job goes through retry_argocd, which already exists precisely to absorb transient network blips (per its own comment). A single connectivity hiccup on get immediately falls into the create branch today, which is inconsistent with the rest of the file's pattern.
There was a problem hiding this comment.
Yep — agree. The app get should also go through retry_argocd to avoid a one-off blip dropping into the create path.
I’d still avoid treating any post-retry failure as “missing”: wrap retry_argocd argocd app get ... while capturing stderr, and only app create if the output matches the specific “missing app” signature you’re seeing; otherwise print the error and fail the job.
There was a problem hiding this comment.
Yep, agreed — routing app get through retry_argocd is the right fix for consistency and to avoid a transient blip falling into the create branch. The updated approach (retry + only create on the specific PermissionDenied signature, otherwise fail with the original error) addresses my original concern.
There was a problem hiding this comment.
Fixes pushed in a346ea8 — added kubernetes/bootstrap/prd-base.yaml to the drift-check diff paths per Harry's review, and reworded the retry_argocd comment/log message for accuracy (it retries network-level failures, not the PermissionDenied/missing-app case, by design).
There was a problem hiding this comment.
Thanks — yep, this addresses my original concern.
- Adding
kubernetes/bootstrap/prd-base.yamlto the drift-check path closes the “unvalidated change slips into prd” gap. - The
retry_argocd-wrappedapp get+ “only attempt create on PermissionDenied, otherwise fail with the original error” is the right behavior, and the clarified comment/logging makes the intent much clearer.
No further changes needed from my side on this thread.
Addresses peqy review on #2550: a bare argocd app get treated any failure as "missing app" and fell into create, so a transient blip could be misdiagnosed the same way PermissionDenied once was. Route the get through retry_argocd and only create on that specific signature; anything else fails the job with the real error. Signed-off-by: Md Asaduzzaman Miah <asad.miah@rhesis.ai>
There was a problem hiding this comment.
Looks good — moving prd-base into kubernetes/bootstrap/ makes the bootstrap state reproducible and avoids the self-reference/prune footguns described.
One small suggestion: in publish-release.yml, the branch that keys off PermissionDenied logs prd-base not found — creating..., but your own comment notes this signature can also be a real RBAC issue. Might be worth wording that message as “PermissionDenied from app get; attempting create in case it’s the missing-app case” to avoid misleading diagnosis.
harry-rhesis
left a comment
There was a problem hiding this comment.
Thanks for this, Asad. The README rewrite is really thorough, and making the bootstrap state reproducible from git is a big operational win. A couple of things worth looking at before merge.
Drift check no longer covers prd-base.yaml
The "Verify main hasn't drifted" step (line 66) diffs only kubernetes/clusters/prd. Now that prd-base.yaml lives in kubernetes/bootstrap/, a non-targetRevision change to it (e.g. path, repoURL, syncPolicy) would be promoted silently without stg validation. Same class of risk the check was built to prevent.
Fix is small:
UNEXPECTED=$(git diff "$GITHUB_SHA" HEAD -- kubernetes/clusters/prd \
kubernetes/bootstrap/prd-base.yaml \
| grep -E '^[+-][^+-]' \
| grep -v 'targetRevision:' || true)retry_argocd fast-exits on "permission denied", so the retry is a no-op for missing apps
retry_argocd (line 161) treats permission denied as non-retryable and returns on the first attempt. Since ArgoCD reports a non-existent app as "PermissionDenied", the retry_argocd argocd app get prd-base call will never actually retry the missing-app case. It fails immediately and falls through to the create branch.
The commit message says "route the get through retry_argocd so a transient blip gets retried," but that only holds for transient errors that don't contain "permission denied." The specific failure mode this guard was built for (missing app = PermissionDenied) still gets no retry.
The end behavior is still correct (the create path fires), so this is more about the comment being honest than the logic being wrong. Either update the comment to note the retry only covers network-level failures, or use a separate retry loop for this call that doesn't fast-exit on "permission denied."
Minor: log message and comment path
- The
"prd-base not found — creating..."message runs on any "permission denied" match, including real RBAC failures. The peqy reviewer suggested something like"PermissionDenied on app get; attempting create (may be a missing app or a real RBAC issue)", which would make diagnosis clearer. rhesis-application.yamlcomment says(bootstrap/prd-base.yaml), which is relative tokubernetes/. From four levels deep inkubernetes/clusters/prd/rhesis/, that reads ambiguously.kubernetes/bootstrap/prd-base.yaml(full repo-relative path) would be clearer.
Addresses Harry's review on #2550: the "verify main hasn't drifted" check only diffed kubernetes/clusters/prd, so a non-targetRevision edit to prd-base.yaml (now outside that tree) would promote to prd without stg validation. Add it to the diffed paths. Also corrects the retry_argocd comment (it doesn't actually retry the PermissionDenied/missing-app case, by design), clarifies the ambiguous create log message, and fixes a relative path in rhesis-application.yaml's comment. Signed-off-by: Md Asaduzzaman Miah <asad.miah@rhesis.ai>
There was a problem hiding this comment.
Looks good. Moving prd-base into kubernetes/bootstrap/ + committing Prune=false makes the prd root reproducible, and the updated publish-release.yml now retries app get and only attempts create on the expected PermissionDenied signature (otherwise fails with the real error). Ship it.
|
@harry-rhesis thanks for the thorough review — all three points fixed and pushed in a346ea8:
|
Purpose
prd-base(the prd root app-of-apps) currently depends on two fixes that exist only on the cluster and cannot be reproduced from git: aPrune=falseannotation and a removedtracking-id. Both were applied by hand withkubectlduring the v0.13.0 release because the only file that could carry them (kubernetes/clusters/prd/base.yaml) is deliberately excluded from its own kustomization. Ifprd-baseis ever recreated, both fixes would have to be reapplied from memory or prd's whole app tree gets pruned again. This moves the manifest tokubernetes/bootstrap/prd-base.yaml, outside every synced tree, so the arrangement is self-describing and survives a rebuild.What Changed
kubernetes/clusters/prd/base.yamltokubernetes/bootstrap/prd-base.yaml, and added theargocd.argoproj.io/sync-options: Prune=falseannotation directly to the committed manifest.kubernetes/clusters/prd/kustomization.yaml's comment to explain the new location instead of describing an in-directory exclusion..github/workflows/publish-release.yml: theFILESarray now points at the new path, and a guardedargocd app get/app createstep runs before pinningprd-base, so a missing root Application self-heals during release instead of failing as aPermissionDeniedauth error.kubernetes/clusters/prd/rhesis/rhesis-application.yaml.kubernetes/README.md: bootstrap command, directory tree, promotion-gate paragraphs, and a new "Why prd's root Application lives inbootstrap/" section covering both the self-reference problem and the prune/tracking-id problem.Additional Context
Closes #2544. The exclusion of
base.yaml/prd-base.yamlfrom its own kustomization is intentional and unchanged —publish-release.ymloverwritestargetRevisionimperatively viaargocd app set, and a commit can't embed its own hash, so a self-managed manifest would revert the pin on every sync. Only the file's location changes here. Per the issue, this must land onmainbetween releases (not ride a release branch) —promote-prd-config's drift check diffskubernetes/clusters/prdagainstmainand only toleratestargetRevision:differences.Testing
kubectl kustomize kubernetes/clusters/prdstill renders 17 Applications and does not containprd-base.grep -rn "clusters/prd/base.yaml" .github kubernetesreturns nothing.kubectl apply -f kubernetes/bootstrap/prd-base.yaml --dry-run=clientvalidates the manifest.publish-release.ymlwrite-back logic against a scratch copy of the repo: every file in the updatedFILESarray (including the newkubernetes/bootstrap/prd-base.yamlpath) has exactly onetargetRevision:line, and thesedrewrite only touches that line, leaving annotations/comments intact.prd-baseobject, and exercising the newargocd app createguarded path by deletingprd-baseon dev/stg first (flagged in the issue as a manual follow-up).