diff --git a/scripts/refresh-flux-ghcr-auth.sh b/scripts/refresh-flux-ghcr-auth.sh index 9efa2964c..701917257 100755 --- a/scripts/refresh-flux-ghcr-auth.sh +++ b/scripts/refresh-flux-ghcr-auth.sh @@ -4665,8 +4665,47 @@ flux_policy_parent_is_released() { ' "${flux_policy_parent_state_file}" >/dev/null } +flux_policy_parent_has_any_owner() { + jq -e \ + --arg annotation "${FLUX_POLICY_PARENT_OWNER_ANNOTATION}" ' + ((.metadata.annotations // {})[$annotation] // "") != "" + ' "${flux_policy_parent_state_file}" >/dev/null +} + +# A killed transaction leaves the owner annotation behind, so this refusal — not the +# malformed/suspended one — is what an orphaned parent fence actually hits. It needs +# the same pointer the child-handoff refusal already carries. Emitted from two places +# (the pre-acquisition check and the contention re-read), so the wording is shared: +# an operator must not have to tell two differently-worded refusals apart. +refuse_owned_flux_policy_parent() { + echo "::error::Another transaction already owns the parent Flux policy handoff; refusing cluster mutation. Run './scripts/refresh-flux-ghcr-auth.sh --fences' to list every held fence with its liveness evidence and exact release command, and see docs/dr/runbook.md → 'Recover an orphaned GHCR deploy fence'." +} + +# Whether a re-read parent is still the object this transaction may fence: the same +# object (UID), well-formed, unfenced by anyone, and not suspended. Only then is a +# rejected acquisition contention rather than a conflict. +flux_policy_parent_claim_preconditions_still_hold() { + jq -e \ + --arg uid "${flux_policy_parent_uid}" \ + --arg annotation "${FLUX_POLICY_PARENT_OWNER_ANNOTATION}" ' + .kind == "Kustomization" + and .metadata.uid == $uid + and (.metadata.resourceVersion | type == "string" and length > 0) + and ((.metadata.annotations // {}) | type == "object") + and (((.metadata.annotations // {})[$annotation] // "") == "") + and ((.spec.suspend // false) == false) + ' "${flux_policy_parent_state_file}" >/dev/null +} + pause_flux_policy_parent() { local resource_version attempt annotations_present + local max_attempts="${FLUX_POLICY_PARENT_CLAIM_MAX_ATTEMPTS:-5}" + local reread_resource_version + # The re-read below needs its own stderr sink. Pointed at the result file it would + # succeed, write nothing, and truncate the rejection that explains why the fence was + # refused — leaving a bare refusal with no cause in exactly the case that matters + # most, another actor or the controller competing for the object. + local reread_error_file="${flux_policy_parent_result_file}.reread" # The parent/child ownership annotations are a separate fail-closed fence: # even if this process loses the synchronization Lease during acquisition, a @@ -4687,14 +4726,8 @@ pause_flux_policy_parent() { echo "::error::Could not inspect the parent Flux reconciliation before the image-verification policy handoff." return 1 fi - if jq -e \ - --arg annotation "${FLUX_POLICY_PARENT_OWNER_ANNOTATION}" ' - ((.metadata.annotations // {})[$annotation] // "") != "" - ' "${flux_policy_parent_state_file}" >/dev/null; then - # A killed transaction leaves this annotation behind, so this branch — not the - # malformed/suspended one below — is what an orphaned parent fence actually hits. - # It needs the same pointer the child-handoff refusal already carries. - echo "::error::Another transaction already owns the parent Flux policy handoff; refusing cluster mutation. Run './scripts/refresh-flux-ghcr-auth.sh --fences' to list every held fence with its liveness evidence and exact release command, and see docs/dr/runbook.md → 'Recover an orphaned GHCR deploy fence'." + if flux_policy_parent_has_any_owner; then + refuse_owned_flux_policy_parent return 1 fi if ! jq -e ' @@ -4711,44 +4744,55 @@ pause_flux_policy_parent() { return 1 fi - resource_version="$(jq -er '.metadata.resourceVersion' \ - "${flux_policy_parent_state_file}")" flux_policy_parent_uid="$(jq -er '.metadata.uid' \ "${flux_policy_parent_state_file}")" flux_policy_parent_owner="${sync_lease_holder}" - annotations_present="$(jq -r \ - '(.metadata.annotations? | type) == "object"' \ - "${flux_policy_parent_state_file}")" - jq -n \ - --arg resource_version "${resource_version}" \ - --arg uid "${flux_policy_parent_uid}" \ - --arg owner_path "${FLUX_POLICY_PARENT_OWNER_JSON_PATH}" \ - --arg owner "${flux_policy_parent_owner}" \ - --argjson annotations_present "${annotations_present}" ' - [ - {op: "test", path: "/metadata/resourceVersion", value: $resource_version}, - {op: "test", path: "/metadata/uid", value: $uid} - ] - + (if $annotations_present then [] else - [{op: "add", path: "/metadata/annotations", value: {}}] - end) - + [ - {op: "add", path: $owner_path, value: $owner}, - {op: "add", path: "/spec/suspend", value: true} - ] - ' >"${flux_policy_parent_patch_file}" - if kubectl \ - --context "${KUBE_CONTEXT}" \ - --namespace flux-system \ - patch "${FLUX_KUSTOMIZATION_RESOURCE}" \ - "${IMAGE_VERIFICATION_FLUX_PARENT_KUSTOMIZATION}" \ - --type=json \ - --patch-file="${flux_policy_parent_patch_file}" \ - -o json \ - >"${flux_policy_parent_state_file}" \ - 2>"${flux_policy_parent_result_file}"; then - flux_policy_parent_acquired=true - else + + # The contended object is a Flux Kustomization whose status the controller rewrites + # continuously, so its resourceVersion moves on its own and this CAS is lost exactly + # when Flux is busiest — which, on a serialized merge queue, is the moment the next + # deploy starts. Losing it is contention, not a conflict, so it is retried; every + # other rejection, and every state that is no longer ours to fence, still fails + # closed on the first attempt (#3046). + attempt=1 + while :; do + resource_version="$(jq -er '.metadata.resourceVersion' \ + "${flux_policy_parent_state_file}")" + annotations_present="$(jq -r \ + '(.metadata.annotations? | type) == "object"' \ + "${flux_policy_parent_state_file}")" + jq -n \ + --arg resource_version "${resource_version}" \ + --arg uid "${flux_policy_parent_uid}" \ + --arg owner_path "${FLUX_POLICY_PARENT_OWNER_JSON_PATH}" \ + --arg owner "${flux_policy_parent_owner}" \ + --argjson annotations_present "${annotations_present}" ' + [ + {op: "test", path: "/metadata/resourceVersion", value: $resource_version}, + {op: "test", path: "/metadata/uid", value: $uid} + ] + + (if $annotations_present then [] else + [{op: "add", path: "/metadata/annotations", value: {}}] + end) + + [ + {op: "add", path: $owner_path, value: $owner}, + {op: "add", path: "/spec/suspend", value: true} + ] + ' >"${flux_policy_parent_patch_file}" + if kubectl \ + --context "${KUBE_CONTEXT}" \ + --namespace flux-system \ + patch "${FLUX_KUSTOMIZATION_RESOURCE}" \ + "${IMAGE_VERIFICATION_FLUX_PARENT_KUSTOMIZATION}" \ + --type=json \ + --patch-file="${flux_policy_parent_patch_file}" \ + -o json \ + >"${flux_policy_parent_state_file}" \ + 2>"${flux_policy_parent_result_file}"; then + flux_policy_parent_acquired=true + break + fi + # A lost patch response is ambiguous. Re-read and adopt only the exact # UID/owner/suspend tuple written by this transaction so EXIT cleanup owns # the durable fence even when kubectl reported failure. @@ -4757,14 +4801,63 @@ pause_flux_policy_parent() { --namespace flux-system \ get "${FLUX_KUSTOMIZATION_RESOURCE}" \ "${IMAGE_VERIFICATION_FLUX_PARENT_KUSTOMIZATION}" \ - -o json >"${flux_policy_parent_state_file}" || - ! flux_policy_parent_is_owned; then + -o json >"${flux_policy_parent_state_file}" \ + 2>"${reread_error_file}"; then + # A failed re-read is now the actionable cause, so it replaces the patch + # rejection in the emitted output. Two things can leave that output empty: + # the copy fails, or kubectl exits non-zero having written no stderr, so the + # diagnostic file is zero bytes and copying it SUCCEEDS while the redirection + # truncates the result file. emit_safe_operation_output skips an empty file + # entirely, which is the very silence this block exists to prevent, so an + # empty diagnostic counts as a failed copy and falls back to a deterministic + # non-empty line. + if [[ ! -s "${reread_error_file}" ]] || + ! cat "${reread_error_file}" \ + >"${flux_policy_parent_result_file}" 2>/dev/null; then + echo "parent re-read failed; its diagnostic could not be read" \ + >"${flux_policy_parent_result_file}" + fi + break + fi + if flux_policy_parent_is_owned; then + flux_policy_parent_acquired=true + break + fi + + # An owner that is not ours is a genuine foreign fence, never contention: refuse it + # with the same wording the pre-acquisition check uses, so the two are one thing to + # an operator rather than two. + if flux_policy_parent_has_any_owner; then + rm -f "${reread_error_file}" emit_safe_operation_output "flux-policy-parent-patch" \ "${flux_policy_parent_result_file}" - echo "::error::Could not atomically pause or adopt the parent Flux policy handoff." + refuse_owned_flux_policy_parent return 1 fi - flux_policy_parent_acquired=true + + # Only a rejection whose resourceVersion demonstrably MOVED is contention. A + # rejection at an unchanged resourceVersion is something else entirely -- a + # permission denial, a validation error -- and retrying it would just repeat a + # request the server has already refused on its merits. + reread_resource_version="$(jq -r '.metadata.resourceVersion // ""' \ + "${flux_policy_parent_state_file}")" + if [[ "${reread_resource_version}" == "${resource_version}" ]] || + ((attempt >= max_attempts)) || + ! flux_policy_parent_claim_preconditions_still_hold || + [[ -e "${sync_lease_lost_file}" ]]; then + break + fi + + attempt=$((attempt + 1)) + sleep "${SYNC_INTERVAL}" + done + + rm -f "${reread_error_file}" + if [[ "${flux_policy_parent_acquired}" != "true" ]]; then + emit_safe_operation_output "flux-policy-parent-patch" \ + "${flux_policy_parent_result_file}" + echo "::error::Could not atomically pause or adopt the parent Flux policy handoff." + return 1 fi # New parent reconciliations now stop at spec.suspend. After a mandatory diff --git a/scripts/tests/refresh-flux-ghcr-auth/fake_kubectl_test.go b/scripts/tests/refresh-flux-ghcr-auth/fake_kubectl_test.go index d47b2f569..5dff33137 100644 --- a/scripts/tests/refresh-flux-ghcr-auth/fake_kubectl_test.go +++ b/scripts/tests/refresh-flux-ghcr-auth/fake_kubectl_test.go @@ -390,6 +390,19 @@ func fakeKubectlPatchFluxPolicyKustomization(args []string, namespace, patchFile } func fakeKubectlGetFluxPolicyParent(args []string, namespace string) int { + // A re-read that fails after the patch response was lost. The silent variant + // leaves the caller's diagnostic file at zero bytes, so copying it succeeds + // while carrying nothing; the diagnostic variant writes real stderr, whose + // content must survive to the operator. + if markerExists("flux-policy-parent-patch-response-lost") { + diagnostic := os.Getenv("FAKE_FLUX_POLICY_PARENT_REREAD_FAILURE_DIAGNOSTIC") + if diagnostic != "" { + return commandFailure(92, "%s", diagnostic) + } + if os.Getenv("FAKE_FLUX_POLICY_PARENT_REREAD_SILENT_FAILURE") == "true" { + return 92 + } + } if namespace != "flux-system" || (!containsArg(args, "-o") && !containsArg(args, "--output")) { return commandFailure(91, "invalid parent Flux Kustomization lookup") @@ -508,6 +521,38 @@ func fakeKubectlPatchFluxPolicyParent(args []string, namespace, patchFile string appendEnvFile("OPERATION_LOG", "flux-policy-parent-patch-rejected\n") return commandFailure(56, "%s", rejection) } + // Ordinary Flux controller churn advances the parent's resourceVersion between + // the script's read and its patch, so the CAS test fails against an object that + // is still perfectly claimable. Model exactly that -- advance the stored version + // and reject -- for the first N acquisition attempts. Note this deliberately + // MOVES the version, which is what separates contention from the rejection knob + // above: that one rejects without moving anything, and must never be retried. + if budget := parseInt( + os.Getenv("FAKE_FLUX_POLICY_PARENT_CAS_CHURN_REJECTIONS"), 0, + ); budget > 0 { + fired := parseInt(markerContent("flux-policy-parent-cas-churn-count"), 0) + if fired < budget { + setMarkerContent( + "flux-policy-parent-cas-churn-count", + strconv.Itoa(fired+1), + ) + setMarkerContent( + "flux-policy-parent-resource-version", + incrementDecimal(currentResourceVersion), + ) + if os.Getenv("FAKE_FLUX_POLICY_PARENT_FOREIGN_OWNER_AFTER_CAS_CHURN") == "true" { + setMarkerContent( + "flux-policy-parent-owner", + "fixture-foreign-transaction", + ) + } + appendEnvFile("OPERATION_LOG", "flux-policy-parent-cas-churn:flux-system\n") + return commandFailure( + 56, + "Error from server (Invalid): the server rejected our request due to an error in our request", + ) + } + } owner := patchValueString(patch, "add", ownerPath) if !hasPatchOperation(patch, "test", "/metadata/resourceVersion", currentResourceVersion) || owner == "" || diff --git a/scripts/tests/refresh-flux-ghcr-auth/rollout_convergence_test.go b/scripts/tests/refresh-flux-ghcr-auth/rollout_convergence_test.go index 8bc3a7e1f..1ef42d28d 100644 --- a/scripts/tests/refresh-flux-ghcr-auth/rollout_convergence_test.go +++ b/scripts/tests/refresh-flux-ghcr-auth/rollout_convergence_test.go @@ -1330,6 +1330,113 @@ func TestRejectedFluxParentPatchPreservesSafeDiagnostic(t *testing.T) { } } +// The parent Flux Kustomization's status is rewritten continuously by its own +// controller, so the fence CAS is lost to nothing but ordinary churn -- and, on a +// serialized merge queue, it is lost precisely when the queue is busiest. Losing it +// used to kill the whole production deploy and evict the PR (#3046). +func TestFluxParentFenceRetriesWhenControllerChurnBreaksItsCAS(t *testing.T) { + t.Parallel() + f := newFixture(t) + result := f.runHelper(validConfig(), nil, map[string]string{ + "FAKE_FLUX_POLICY_PARENT_CAS_CHURN_REJECTIONS": "2", + }) + requireSuccessResult(t, result) + operations := readLines(f.operationLog) + // Assert the contention actually HAPPENED, not merely that the run passed: a + // fixture knob that silently stopped firing would make this test vacuous. + if got := strings.Count( + strings.Join(operations, "\n"), "flux-policy-parent-cas-churn:flux-system", + ); got != 2 { + t.Fatalf("CAS rejections = %d, want the 2 the fixture injected", got) + } + // ... and that the fence still landed, released, and let the transaction through. + requireLine(t, operations, "flux-policy-parent-pause:flux-system") + requireLine(t, operations, "root-patch") + requireLine(t, operations, "flux-policy-parent-resume:flux-system") + for _, marker := range []string{ + "flux-policy-parent-owner", "flux-policy-parent-suspended", + } { + if pathExists(filepath.Join(f.syncStateDir, marker)) { + t.Fatalf("converged fence left %s behind", marker) + } + } +} + +// Contention is retried; a foreign owner is not. The retry re-reads before deciding, +// so this is the case where that re-read must change the verdict -- and it must say +// which of the two it found, or an operator cannot tell a busy controller from a +// competing transaction. +func TestFluxParentFenceRefusesAForeignOwnerFoundByTheContentionReRead(t *testing.T) { + t.Parallel() + f := newFixture(t) + result := f.runHelper(validConfig(), nil, map[string]string{ + "FAKE_FLUX_POLICY_PARENT_CAS_CHURN_REJECTIONS": "5", + "FAKE_FLUX_POLICY_PARENT_FOREIGN_OWNER_AFTER_CAS_CHURN": "true", + }) + requireFailureResult(t, result) + output := result.stdout + result.stderr + requireContains(t, output, + "Another transaction already owns the parent Flux policy handoff") + requireNotContains(t, output, "Could not atomically pause or adopt") + operations := readLines(f.operationLog) + // Exactly one: a foreign fence is refused on sight rather than retried against, + // even though the fixture would happily reject four more times. + if got := strings.Count( + strings.Join(operations, "\n"), "flux-policy-parent-cas-churn:flux-system", + ); got != 1 { + t.Fatalf("CAS rejections = %d, want a single refusal on sight", got) + } + for _, unexpected := range []string{ + "flux-policy-parent-pause:flux-system", "flux-policy-pause:infrastructure", + "root-patch", + } { + requireNoLine(t, operations, unexpected) + } + // The foreign owner marker is the fixture's own, but the SUSPEND is not: refusing + // must never have paused anyone else's parent. + if pathExists(filepath.Join(f.syncStateDir, "flux-policy-parent-suspended")) { + t.Fatal("refused foreign fence suspended the parent anyway") + } +} + +func TestFluxParentFenceStillFailsClosedWhenCASRetriesAreExhausted(t *testing.T) { + t.Parallel() + f := newFixture(t) + result := f.runHelper(validConfig(), nil, map[string]string{ + "FAKE_FLUX_POLICY_PARENT_CAS_CHURN_REJECTIONS": "99", + }) + requireFailureResult(t, result) + output := result.stdout + result.stderr + requireContains(t, output, + "Could not atomically pause or adopt the parent Flux policy handoff") + // The last rejection's own text still reaches the operator through the bounded, + // printable helper -- a retry that swallowed the cause would be worse than none. + requireContains(t, output, + "flux-policy-parent-patch: Error from server (Invalid): "+ + "the server rejected our request due to an error in our request") + operations := readLines(f.operationLog) + // Bounded, and bounded at the budget: neither one attempt (no retry at all) nor + // unbounded (a deploy that never gives up is its own outage). + if got := strings.Count( + strings.Join(operations, "\n"), "flux-policy-parent-cas-churn:flux-system", + ); got != 5 { + t.Fatalf("CAS rejections = %d, want the bounded budget of 5", got) + } + for _, unexpected := range []string{ + "flux-policy-parent-pause:flux-system", "flux-policy-pause:infrastructure", + "root-patch", + } { + requireNoLine(t, operations, unexpected) + } + for _, marker := range []string{ + "flux-policy-parent-owner", "flux-policy-parent-suspended", + } { + if pathExists(filepath.Join(f.syncStateDir, marker)) { + t.Fatalf("exhausted CAS retries left %s", marker) + } + } +} + func TestAmbiguousFluxFenceAcquisitionIsAdoptedAndCleaned(t *testing.T) { t.Parallel() for _, test := range []struct { @@ -1978,3 +2085,44 @@ func TestRuntimeProbeKubeletRejectionFailsFastAndDistinctly(t *testing.T) { // Kubelet's raw message can carry node detail; it must not reach the log. requireNotContains(t, output, "Node didn't have enough resource") } + +// A parent re-read can fail without writing anything to stderr. The diagnostic +// file is then zero bytes rather than absent, so copying it succeeds while +// carrying nothing -- and emit_safe_operation_output skips an empty file. The +// operator must still be told why the fence could not be taken. +func TestSilentFluxParentRereadFailureStillEmitsADiagnostic(t *testing.T) { + t.Parallel() + + f := newFixture(t) + result := f.runHelper(validConfig(), nil, map[string]string{ + "FAKE_FLUX_POLICY_PARENT_PATCH_RESPONSE_LOST": "true", + "FAKE_FLUX_POLICY_PARENT_REREAD_SILENT_FAILURE": "true", + }) + requireFailureResult(t, result) + + if !pathExists(filepath.Join(f.syncStateDir, "flux-policy-parent-patch-response-lost")) { + t.Fatal("fixture did not lose the applied fence patch response") + } + requireContains(t, result.stdout+result.stderr, + "flux-policy-parent-patch: parent re-read failed; its diagnostic could not be read") +} + +// The empty-diagnostic guard must not swallow a real one: when the failed +// re-read does write stderr, that text is what the operator needs, not the +// deterministic fallback. +func TestFluxParentRereadFailureDiagnosticSurvives(t *testing.T) { + t.Parallel() + + const diagnostic = "Error from server (Forbidden): kustomizations.kustomize.toolkit.fluxcd.io is forbidden" + + f := newFixture(t) + result := f.runHelper(validConfig(), nil, map[string]string{ + "FAKE_FLUX_POLICY_PARENT_PATCH_RESPONSE_LOST": "true", + "FAKE_FLUX_POLICY_PARENT_REREAD_FAILURE_DIAGNOSTIC": diagnostic, + }) + requireFailureResult(t, result) + + output := result.stdout + result.stderr + requireContains(t, output, "flux-policy-parent-patch: "+diagnostic) + requireNotContains(t, output, "its diagnostic could not be read") +}