fix(connectivity): accept the ledger v3 preview as satisfying the v3 gate - #532
fix(connectivity): accept the ledger v3 preview as satisfying the v3 gate#532sylr wants to merge 4 commits into
Conversation
✅ Approve — automated reviewNo actionable defects were identified. The current code addresses the previously raised gate reachability, readiness, version binding, teardown, and retry concerns. No findings. |
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #532 (comment)
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Inline review focused on compatibility with the current main branch and reconciliation behavior.
Address the review findings on #532, on top of the module version requirements introduced by #527: - Declare the Ledger requirement presence-only. The previous VersionAtLeast(LedgerV3Version) constraint was evaluated by ForModule before Reconcile, so a v2 ledger running the v3 preview failed with DependencyVersionMismatch and the preview gate was unreachable. The effective v3 capability (module version or active preview, including the hard teardown) is decided by the stackLedgerHasV3 gate inside Reconcile. - Require reconciled preview readiness before provisioning. The Ledger's aggregate status.ready can be stale-true from a v2-only reconcile that predates the preview Setting, and the god-mode Credentials turns Ready from additionalNamespaces alone (no matched Cluster needed), so neither gate stops initial provisioning against a v3 service that does not exist yet. When the gate is opened by the preview, additionally require the Ledger's LedgerV3PreviewReady condition to be True: pending without teardown otherwise, so initial provisioning is blocked while existing resources survive transient unavailability. - Bound the unresolved-preview retry. A Settings read failure produces no watch event on recovery, and a pending error without WithRequeueAfter is returned as a success with no requeue, leaving Connectivity pending indefinitely; poll with a 5s delay instead. Constraint: the requirements framework only supports static version bounds, so preview awareness cannot be expressed in the Require declaration Rejected: extending core with a custom requirement predicate | invasive on a freshly merged framework; presence-only + module gate achieves the same with less machinery Rejected: trusting the credentials-Ready gate to block early provisioning | the ledger operator marks Credentials Ready from additionalNamespaces alone, without a matched Cluster Confidence: high Scope-risk: narrow Not-tested: end-to-end preview + connectivity on a live cluster
aa3f73e to
6be6ad5
Compare
…d retry unresolved gates Address the second review round on #532: - ledgers.V3PreviewReady no longer reads the Ledger's LedgerV3PreviewReady condition, which survives a rapid Setting remove/re-add until the ledger reconcile persists its cleanup and does not identify which preview version it reflects. It now reads the preview Cluster itself: it must exist, carry the preview label, not be deleting, be annotated with the currently resolved ledger.v3.preview-version (a new annotation stamped by createOrUpdateV3Cluster), and be Running at its observed generation. - ledgerGateClosed surfaces lookup errors instead of collapsing them into "gate open". On the capability-unavailable path a possibly-due teardown was otherwise skipped with a pending error carrying no requeue — and recovery from a Settings read failure emits no watch event — so that path now polls with ledgerGateRetryDelay; the unsatisfied-requirements handler returns the error so the framework retries. Constraint: Ledger CR status (ready flag or conditions) is stale across Setting changes; only the Cluster itself reflects the currently reconciled preview Rejected: stamping the preview version into the condition message | free-text parsing; the Cluster annotation is structured and updated atomically with the spec Rejected: comparing the Setting version to the Cluster's spec.image.tag | image override Settings can legitimately pin a different tag Confidence: high Scope-risk: narrow Not-tested: end-to-end preview remove/re-add on a live cluster
…gate Enabling connectivity on a stack running the ledger.v3.preview-version preview was impossible: the connectivity module gated on the ledger's resolved module version alone, which stays on v2 in preview mode by design, so the reconciler reported "connectivity requires a Ledger v3" and tore down the delegated resources on every reconcile — even though the v3 gRPC endpoint connectivity binds to is exactly what the preview provisions (observed on stack jdxmvkvwlyiy-pepj, eks-acme-dev-euw1-01). Add ledgers.HasV3, which accepts a v3 module version or an active v3 preview, mirroring the ledger reconciler's own decision (the Setting is ignored when the Ledger Operator CRD is unavailable, and values at or below v3.0.0-alpha are rejected), and use it for the connectivity gate and the capability-unavailable teardown decision. An error resolving the preview Setting is transient (LedgerV3PreviewUnresolved): pending without teardown, like an unresolvable module version. The readiness gate needs no change: a preview stack's Ledger only reports Ready once the preview Cluster is ready. Constraint: preview mode keeps the ledger module version on v2 by design Rejected: gating on the LedgerV3PreviewReady condition | status-derived and racy; the Setting is the source of truth the ledger reconciler itself uses Rejected: reading the Setting directly in the connectivity module | would disagree with the ledger reconciler when the Ledger Operator CRD is absent Confidence: high Scope-risk: narrow Directive: ledgerHasV3 is a package var only so connectivity tests can stub the preview branch, which depends on the ledger controller's startup capability discovery — keep production code assigning it exactly once Not-tested: end-to-end preview + connectivity on a live cluster
Address the review findings on #532, on top of the module version requirements introduced by #527: - Declare the Ledger requirement presence-only. The previous VersionAtLeast(LedgerV3Version) constraint was evaluated by ForModule before Reconcile, so a v2 ledger running the v3 preview failed with DependencyVersionMismatch and the preview gate was unreachable. The effective v3 capability (module version or active preview, including the hard teardown) is decided by the stackLedgerHasV3 gate inside Reconcile. - Require reconciled preview readiness before provisioning. The Ledger's aggregate status.ready can be stale-true from a v2-only reconcile that predates the preview Setting, and the god-mode Credentials turns Ready from additionalNamespaces alone (no matched Cluster needed), so neither gate stops initial provisioning against a v3 service that does not exist yet. When the gate is opened by the preview, additionally require the Ledger's LedgerV3PreviewReady condition to be True: pending without teardown otherwise, so initial provisioning is blocked while existing resources survive transient unavailability. - Bound the unresolved-preview retry. A Settings read failure produces no watch event on recovery, and a pending error without WithRequeueAfter is returned as a success with no requeue, leaving Connectivity pending indefinitely; poll with a 5s delay instead. Constraint: the requirements framework only supports static version bounds, so preview awareness cannot be expressed in the Require declaration Rejected: extending core with a custom requirement predicate | invasive on a freshly merged framework; presence-only + module gate achieves the same with less machinery Rejected: trusting the credentials-Ready gate to block early provisioning | the ledger operator marks Credentials Ready from additionalNamespaces alone, without a matched Cluster Confidence: high Scope-risk: narrow Not-tested: end-to-end preview + connectivity on a live cluster
…d retry unresolved gates Address the second review round on #532: - ledgers.V3PreviewReady no longer reads the Ledger's LedgerV3PreviewReady condition, which survives a rapid Setting remove/re-add until the ledger reconcile persists its cleanup and does not identify which preview version it reflects. It now reads the preview Cluster itself: it must exist, carry the preview label, not be deleting, be annotated with the currently resolved ledger.v3.preview-version (a new annotation stamped by createOrUpdateV3Cluster), and be Running at its observed generation. - ledgerGateClosed surfaces lookup errors instead of collapsing them into "gate open". On the capability-unavailable path a possibly-due teardown was otherwise skipped with a pending error carrying no requeue — and recovery from a Settings read failure emits no watch event — so that path now polls with ledgerGateRetryDelay; the unsatisfied-requirements handler returns the error so the framework retries. Constraint: Ledger CR status (ready flag or conditions) is stale across Setting changes; only the Cluster itself reflects the currently reconciled preview Rejected: stamping the preview version into the condition message | free-text parsing; the Cluster annotation is structured and updated atomically with the spec Rejected: comparing the Setting version to the Cluster's spec.image.tag | image override Settings can legitimately pin a different tag Confidence: high Scope-risk: narrow Not-tested: end-to-end preview remove/re-add on a live cluster
92804a5 to
69026af
Compare
|
This PR correctly implements the core ask: a stack running Ledger v2 with the Standards1. No other confirmed material standards finding: the duplication/ Spec1. PR-description bullet 5 ("The readiness gate needs no change") is contradicted by the shipped code. Commits 4d2f693 and 466600e add an entirely new readiness gate: 2. The promised No other confirmed material spec finding: the implementation matches the authoritative commit messages, the Reviewed independently by GLM (glm-5.3-flash) and DeepSeek (deepseek-v4-pro-0813) via Shipfox; verified and synthesized by GLM. |
4 participants
Summary
Enabling the Connectivity module on a stack running the Ledger v3 preview (
ledger.v3.preview-versionSetting) was impossible: the connectivity reconciler gated on the ledger's resolved module version alone, which stays on v2 in preview mode by design. The gate therefore always reportedconnectivity requires a Ledger v3and — being a hard gate — tore down the delegated Connectivity, the GatewayHTTPAPI route, and the god-mode ledger Credentials on every reconcile, even though the v3 gRPC endpoint connectivity binds to is exactly what the preview provisions. Observed on stackjdxmvkvwlyiy-pepj(eks-acme-dev-euw1-01).ledgers.HasV3(ctx, stack, ledgerVersion): true for a v3 module version or an active v3 preview. It reuses the ledger reconciler's own preview resolution, so both modules always agree — the Setting is ignored when the Ledger Operator CRD is unavailable, and values at or belowv3.0.0-alphaare rejected.ledgerGateClosed(the teardown decision on the capability-unavailable path).LedgerV3PreviewUnresolvedcondition, pending without teardown, mirroringLedgerVersionUnresolved.ledgers.V3GRPCBackendRef) already resolves identically for preview and full v3.LedgerNotV3condition message now readsconnectivity requires a Ledger v3 (found "v2.x" and no v3 preview).Test plan
TestHasV3(ledgers): v3 module version; v2 + preview Setting; v2 without Setting; Setting ignored when the Ledger Operator CRD is unavailable; invalid Setting surfaces an errorTestConnectivityReconcilePassesGateWhenLedgerV3PreviewActive: gate passes on a v2 ledger with the preview active, proceeds to credentials provisioning, no teardownTestConnectivityReconcileKeepsDelegatedWhenPreviewGateUnresolved: preview resolution error stays pending without flapping the delegated resourcesgo build ./...,make lint,go test ./internal/...(incl. envtest suite) all greenjdxmvkvwlyiy-pepjonce released