Repository navigation
Conversation
Propagate retained disable to delegated Connectivity and wait for its direct absence before credential revocation. Drain Connectivity before other Stack modules, validate authoritative readiness and parent liveness, and preserve foreign finalizers with optimistic installation. BREAKING CHANGE: disable retains the delegated CR; deletion requires compatible Connectivity cascading cleanup before credentials and other modules are removed. Upgrade and qualify the paired Connectivity CRDs and controller first.
NumaryBot
added
risk: medium
Dav-14
changed the title
|
🟡 [minor] Document the mandatory disable controller instead of an optional handler The updated reconciliation guide tells module authors to register Suggestion: Replace step 4 with the mandatory disable-controller contract: pass an explicit controller to WithModuleReconciler; modules retaining runtime-removal behavior pass DisableModuleOwnedObjects, while Connectivity supplies its delegated lifecycle controller. |
NumaryBot
added
risk: high
and removed
risk: medium
NumaryBot
added
risk: medium
|
🟡 [minor] Remove guarantees from the reverted core hardening The new reconciliation guide promises optimistic-lock finalizer installation and direct API-reader parent liveness checks. Current core code installs finalizers with plain client.MergeFrom (reconciler.go:355) and loads the Stack through GetClient (controllers.go:68). The Stack guide repeats the direct-reader guarantee at line 40. These statements incorrectly tell module authors that stale-parent and concurrent-finalizer protections exist. Suggestion: Update both guides to describe the existing cached parent read and finalizer behavior, without claiming the excluded hardening. |
|
Addressed this finding in d7561b9. Both guides now describe the cached parent Stack read and existing finalizer behavior: unregistered finalizers are removed, and missing registered finalizers use a plain Validation: repository |
NumaryBot
added
risk: high
and removed
risk: medium
Dav-14
changed the title
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Reviewed the reversible-suspension contract at ec4eb86. ModuleProperties.Disabled, Stack-or-module evaluation and Connectivity's dedicated retained-disable path are covered. Three retention gaps remain below.
A complementary local delta based on this exact head reproduces the Cluster/signing-key deletion before correction and passes 18 race-enabled suites, including 162 controller specs, after correction. That delta is not on this PR. These local results do not qualify the specialized Ledger operator or the paired fctl/Membership runtime matrix.
| return err | ||
| } | ||
| } | ||
| return core.ForModule(core.NoRequirements(), func(ctx core.Context, stack *v1beta1.Stack, _ *core.ReconcilerOptions[*v1beta1.Ledger], ledger *v1beta1.Ledger, version string) error { |
There was a problem hiding this comment.
[P1] Preserve the delegated Cluster during suspension, rather than only its clusterID.
This still enters ForModule's owned-object deletion path when either disabled flag is true. The discovered ledger.formance.com/Cluster, Issuer and Certificate are registered in Owns, so they are deleted; the integration test explicitly expects a new Cluster UID on resume. Remembering clusterID does not satisfy the agreed retention of the CR, configuration and TLS ownership chain.
Forward Stack.spec.disabled || Ledger.spec.disabled to the existing primary/preview Cluster before active gates, keep its durable resources, and explicitly clear the delegated flag on resume. Keep real deletion separate. Test unchanged UID/spec/TLS/PVCs and require a compatible specialized CRD/operator; do not assume older CRDs persist an unknown disabled field.
| } | ||
|
|
||
| if stack.Spec.Disabled { | ||
| if stack.Spec.Disabled || t.IsDisabled() { |
There was a problem hiding this comment.
[P1] Limit suspension cleanup to disposable execution resources.
removeAllModulesOwnedObjects excludes only Database/BrokerTopic Resource implementations. Auth registers Secret ownership, so this deletes auth-signing-key; ensureSigningKeySecret generates a different RSA key on resume. Wallets, Orchestration, Reconciliation and TransactionPlane similarly own AuthClient CRs whose missing IDs/secrets are randomly regenerated. Removing ResourceReferences also exposes their copied-resource ownership chain to GC.
Use a suspension-specific policy that retains these identities and required configuration while stopping workloads/exposure. Keep DeleteOwnedObjects and explicit finalizers unchanged. Add pause/resume assertions for the signing-key bytes/UID and AuthClient ID/secret/UID, beyond checking that a new Deployment appears.
| // state or turn it into a missing dependency for lifecycle handlers. | ||
| if module, ok := dependency.(v1beta1.Module); ok && module.IsDisabled() { | ||
| return requirementsEvaluation{ | ||
| status: metav1.ConditionFalse, |
There was a problem hiding this comment.
[P1] Keep DependencyDisabled out of destructive topology cleanup.
ForModule invokes UnsatisfiedRequirementsHandle for this False result. Connectivity handles the reason explicitly, but CleanupLegacyModuleOnV3 only checks whether a primary v3 Cluster exists before deleting consumer AuthClients/BrokerConsumers and workloads. A disabled Ledger can still have that Cluster while reconcilers run asynchronously; retaining the Cluster for reversible suspension makes this state persistent.
Make the legacy handler distinguish DependencyDisabled and stop only disposable execution resources for that reason. Preserve identity/subscription configuration, keep actual incompatibility cleanup separate, and cover the disabled-dependency + retained-primary-Cluster case for the affected consumers.
Membership exposes module enable/disable, but the Agent previously deleted omitted module CRs. For Connectivity this deleted delegated resources and Ledger credentials, turning a disable into cleanup and leaving instances unable to finish removal.
This change adds the shared optional
spec.disabledfield to disable-capable modules and applies Stack disable inheritance. The companion Agent change retains existing modules and toggles this field; never-enabled modules remain absent. Existing runtime cleanup stays unchanged for other modules. Connectivity delegates disable while retaining its configuration, instances and credentials, and requests delegated deletion without waiting in the Stack Operator. Disabled dependencies cannot satisfy enabled-module requirements.Ledger v3 keeps its existing shutdown behavior and records the owned Cluster identity on the durable Ledger before cleanup. Re-enable restores that identity against retained volumes. Failed persistence, conflicting identities, foreign ownership or an unassigned generated identity prevent unsafe cleanup/recreation. Primary and preview use the same child identity.
Validation: repository pre-commit (three linters, generated CRDs/Helm/docs), Ledger unit tests, nine focused race tests, command build and full envtest integration 160/160 PASS. The integration disable test simulates the Ledger Operator assigning its ID and verifies re-enable creates a new Cluster UID with the same identity. Independent Principal Engineer, Product Engineer and SRE source review passed. Exact-head CI and image builds are separate gates.
Delivery: EN-3217, Agent #225, Connectivity #485. Corrected CRDs/controller must precede the Agent. The requested live
fctlmodule toggle, Stack disable, soft-delete/restore and forced-delete matrix remains unqualified. The existing unintended Connectivity deletion requires a separate approved recovery; no finalizer bypass is used. No merge or release.