From a3cc22858f1070197501d6d032341c1c233760ce Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Wed, 22 Jul 2026 17:30:49 +0000 Subject: [PATCH 1/4] refactor(status): one writer for the kstatus trio, one status write per reconcile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Acts on the Flux-maintainer review in docs/future/flux-maintainer-review-status-and-config-model.md, taking its own ordering: the "Before the next release" and "Next" blocks (F1, F2, F3, F5, F7, F8, F11) plus the cheap F12 nits. The API-surface block (F6 suspend/interval, F9, F10) is left for a separate change. Two pieces of shared machinery carry most of it, and both replace code that had been copied five or six times: internal/controller/readiness.go — gates no longer set Ready/Reconciling/Stalled, they contribute a verdict, and the trio is derived once at the end from a precedence stated in one place. This fixes F1: a GitTarget with a refused Git path (terminal) was handed to the source/provider projection, which stamped Stalled=False, Reconciling=True over the refusal, flipping the object from kstatus Failed to InProgress so `kubectl wait` and every CI gate on it hung to timeout. It replaces eight trio-setting helpers, downgradeReady among them. internal/controller/status.go — a per-reconcile status session that captures the object as read and writes the difference exactly once, as a status patch with optimistic concurrency. It replaces five near-identical updateStatusWithRetry copies and six setCondition wrappers, and with them: the unconditional write that made every reconcile cost roughly two (F3), and the read-modify-write that could stamp an observedGeneration never actually observed (F11). Also: - F2: zero resolved types is Ready=True. A GitTarget with no WatchRules is step 3 of the documented setup flow, and it sat at Reconciling=True forever with nothing that could ever resolve, re-reconciling every 10s for its whole life. status.streams.summary still reads 0/0 and the reason is still NoResolvedTypes. - F3: GenerationChangedPredicate on the For() of GitTarget, WatchRule and ClusterWatchRule, which had none; status.lastReconcileTime and status.streams.observedTime removed rather than excluded from the comparison. - F5: upsertCondition delegates to apimeta.SetStatusCondition, so a touched condition keeps its position instead of migrating to the tail every pass. - F7: every reconciler takes an EventRecorder; the shared writer emits one Event per persisted Ready transition, after the patch rather than beside each write. - F8: generic reasons alias github.com/fluxcd/pkg/apis/meta, already a dependency. "OK" and "Ready" as reasons become "Succeeded"; domain reasons stay. - F12: GitTarget printer columns cut from seven default to four; GitProvider conditions gain the patch-strategy markers; ObjectMeta json tags unified. F4 is resolved the other way, deliberately: the abnormal-true pair keeps being written when False, and docs/spec/status-conditions-guide.md now records that as the single knowing deviation from the API conventions, with the reasons. Co-Authored-By: Claude Opus 4.8 (1M context) --- api/v1alpha3/clusterwatchrule_types.go | 2 +- api/v1alpha3/gitprovider_types.go | 4 +- api/v1alpha3/gittarget_types.go | 22 +- api/v1alpha3/watchrule_types.go | 6 +- api/v1alpha3/zz_generated.deepcopy.go | 11 +- cmd/main.go | 5 + .../configbutler.ai_clusterwatchrules.yaml | 4 - .../crd/bases/configbutler.ai_gittargets.yaml | 37 +- .../crd/bases/configbutler.ai_watchrules.yaml | 4 - config/rbac/role.yaml | 7 + docs/UPGRADING.md | 45 ++ docs/design/reconcile-triggering.md | 25 +- docs/facts/subresources.md | 3 +- ...intainer-review-status-and-config-model.md | 67 +- docs/spec/status-conditions-guide.md | 74 +- .../controller/clusterprovider_controller.go | 126 +-- .../clusterprovider_controller_unit_test.go | 19 +- .../clusterwatchrule_admission_test.go | 6 +- .../controller/clusterwatchrule_controller.go | 309 ++------ .../controller/commitrequest_controller.go | 4 +- internal/controller/condition_helper.go | 53 +- internal/controller/condition_helper_test.go | 18 +- internal/controller/constants.go | 22 +- internal/controller/gitprovider_controller.go | 335 ++------ .../controller/gitprovider_controller_test.go | 52 +- internal/controller/gittarget_controller.go | 728 ++++++++---------- .../controller/gittarget_controller_test.go | 8 +- .../gittarget_controller_unit_test.go | 2 +- .../controller/gittarget_dependency_status.go | 24 +- .../gittarget_placement_validation_test.go | 3 +- .../controller/gittarget_source_cluster.go | 139 +--- .../gittarget_source_cluster_test.go | 16 +- internal/controller/gittarget_status_test.go | 249 +++--- internal/controller/readiness.go | 170 ++++ internal/controller/ssh_test.go | 7 +- internal/controller/status.go | 184 +++++ internal/controller/stream_status.go | 133 ++-- internal/controller/stream_status_test.go | 31 +- internal/controller/watchrule_controller.go | 303 ++------ internal/controller/watchrule_kstatus_test.go | 17 +- .../controller/watchrule_source_namespace.go | 38 +- internal/watch/stream_readiness.go | 4 +- test/e2e/aggregated_apiserver_e2e_test.go | 2 +- test/e2e/argocd_bi_directional_e2e_test.go | 6 +- test/e2e/audit_route_attribution_e2e_test.go | 4 +- .../e2e/commit_author_attribution_e2e_test.go | 2 +- test/e2e/commit_request_e2e_test.go | 4 +- test/e2e/commit_window_batching_e2e_test.go | 2 +- test/e2e/crd_lifecycle_e2e_test.go | 10 +- test/e2e/deletecollection_intent_e2e_test.go | 2 +- test/e2e/demo_e2e_test.go | 6 +- ...yment_scale_author_attribution_e2e_test.go | 2 +- .../deployment_scale_subresource_e2e_test.go | 2 +- test/e2e/e2e_test.go | 14 +- test/e2e/flux_bi_directional_e2e_test.go | 9 +- test/e2e/foreign_content_e2e_test.go | 2 +- test/e2e/gittarget_isolation_e2e_test.go | 8 +- test/e2e/gittarget_overlap_e2e_test.go | 6 +- test/e2e/helmrelease_mirror_edit_e2e_test.go | 6 +- test/e2e/inplace_edit_e2e_test.go | 12 +- test/e2e/new_file_placement_e2e_test.go | 6 +- test/e2e/prune_mode_e2e_test.go | 4 +- test/e2e/quickstart_framework_e2e_test.go | 12 +- test/e2e/restart_reconcile_e2e_test.go | 4 +- test/e2e/signing_e2e_test.go | 24 +- test/e2e/source_cluster_e2e_test.go | 2 +- test/e2e/source_namespace_e2e_test.go | 12 +- test/e2e/tilt_playground_e2e_test.go | 6 +- test/e2e/unsupported_folder_e2e_test.go | 2 +- test/e2e/values_file_acceptance_e2e_test.go | 6 +- .../watchrule_configmap_secret_e2e_test.go | 22 +- 71 files changed, 1618 insertions(+), 1897 deletions(-) create mode 100644 internal/controller/readiness.go create mode 100644 internal/controller/status.go diff --git a/api/v1alpha3/clusterwatchrule_types.go b/api/v1alpha3/clusterwatchrule_types.go index 50a759ac..06355f91 100644 --- a/api/v1alpha3/clusterwatchrule_types.go +++ b/api/v1alpha3/clusterwatchrule_types.go @@ -200,7 +200,7 @@ type ClusterWatchRule struct { // metadata is a standard object metadata. // +optional - metav1.ObjectMeta `json:"metadata,omitempty"` + metav1.ObjectMeta `json:"metadata,omitempty,omitzero"` // spec defines the desired state of ClusterWatchRule. // +required diff --git a/api/v1alpha3/gitprovider_types.go b/api/v1alpha3/gitprovider_types.go index 8fe01f6b..58f38e92 100644 --- a/api/v1alpha3/gitprovider_types.go +++ b/api/v1alpha3/gitprovider_types.go @@ -143,7 +143,9 @@ type GitProviderStatus struct { // +optional // +listType=map // +listMapKey=type - Conditions []metav1.Condition `json:"conditions,omitempty"` + // +patchMergeKey=type + // +patchStrategy=merge + Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"` // SigningPublicKey is the operator's SSH signing public key in authorized_keys format. // Register this as a signing key on your git platform. diff --git a/api/v1alpha3/gittarget_types.go b/api/v1alpha3/gittarget_types.go index a48b4974..6564aa86 100644 --- a/api/v1alpha3/gittarget_types.go +++ b/api/v1alpha3/gittarget_types.go @@ -195,10 +195,6 @@ type GitTargetStatus struct { // +patchStrategy=merge Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"` - // LastReconcileTime is the timestamp of the most recent reconcile attempt. - // +optional - LastReconcileTime metav1.Time `json:"lastReconcileTime,omitempty"` - // LastPushTime is the timestamp of the last successful push. // +optional LastPushTime *metav1.Time `json:"lastPushTime,omitempty"` @@ -228,7 +224,11 @@ type GitTargetStatus struct { // GitTargetStreamsStatus is a bounded roll-up of the stream readiness state for the // types this GitTarget tracks. type GitTargetStreamsStatus struct { - // Summary is the display-only ready/total ratio. + // Summary is the display-only ready/total ratio, e.g. "3/4". + // + // It restates Ready and Total, which the API conventions would normally rule out. It exists + // solely to feed the Streams printer column: a column can read one JSONPath, not format two. + // Do not compute anything from it — read ready and total. // +optional Summary string `json:"summary,omitempty"` @@ -243,10 +243,6 @@ type GitTargetStreamsStatus struct { // Blocked is how many tracked types cannot currently be watched. Blocked int32 `json:"blocked"` - - // ObservedTime is when this roll-up was last computed. - // +optional - ObservedTime *metav1.Time `json:"observedTime,omitempty"` } // Design rationale, kept out of the generated CRD description by the blank line below. @@ -282,9 +278,8 @@ type GitTargetRetentionStatus struct { // +kubebuilder:object:root=true // +kubebuilder:subresource:status -// +kubebuilder:printcolumn:name="Provider",type=string,JSONPath=`.spec.providerRef.name` -// +kubebuilder:printcolumn:name="Branch",type=string,JSONPath=`.spec.branch` -// +kubebuilder:printcolumn:name="Path",type=string,JSONPath=`.spec.path` +// Seven default-priority columns wrapped `kubectl get gittargets` on any normal terminal. +// Flux ships three or four; the identity fields stay one `-o wide` away. // +kubebuilder:printcolumn:name="Ready",type=string,JSONPath=`.status.conditions[?(@.type=="Ready")].status` // +kubebuilder:printcolumn:name="Reason",type=string,JSONPath=`.status.conditions[?(@.type=="Ready")].reason` // +kubebuilder:printcolumn:name="Streams",type=string,JSONPath=`.status.streams.summary` @@ -296,6 +291,9 @@ type GitTargetRetentionStatus struct { // +kubebuilder:printcolumn:name="ClusterProviderReady",type=string,JSONPath=`.status.conditions[?(@.type=="ClusterProviderReady")].status`,priority=1 // +kubebuilder:printcolumn:name="Status",type=string,JSONPath=`.status.conditions[?(@.type=="Ready")].message`,priority=1 // +kubebuilder:printcolumn:name="Encryption",type=string,JSONPath=`.spec.encryption.provider`,priority=1 +// +kubebuilder:printcolumn:name="Provider",type=string,JSONPath=`.spec.providerRef.name`,priority=1 +// +kubebuilder:printcolumn:name="Branch",type=string,JSONPath=`.spec.branch`,priority=1 +// +kubebuilder:printcolumn:name="Path",type=string,JSONPath=`.spec.path`,priority=1 // +kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp` // GitTarget is the Schema for the gittargets API. diff --git a/api/v1alpha3/watchrule_types.go b/api/v1alpha3/watchrule_types.go index 05fae6f6..e6ae0769 100644 --- a/api/v1alpha3/watchrule_types.go +++ b/api/v1alpha3/watchrule_types.go @@ -235,10 +235,6 @@ type WatchRuleStreamsStatus struct { // +optional // +kubebuilder:validation:MaxItems=5 PendingSample []string `json:"pendingSample,omitempty"` - - // ObservedTime is when this roll-up was last computed. - // +optional - ObservedTime *metav1.Time `json:"observedTime,omitempty"` } // Design rationale, kept out of the generated CRD description by the blank line below. @@ -274,7 +270,7 @@ type WatchRule struct { // metadata is a standard object metadata // +optional - metav1.ObjectMeta `json:"metadata,omitempty"` + metav1.ObjectMeta `json:"metadata,omitempty,omitzero"` // spec defines the desired state of WatchRule // +required diff --git a/api/v1alpha3/zz_generated.deepcopy.go b/api/v1alpha3/zz_generated.deepcopy.go index 790807ea..057a273d 100644 --- a/api/v1alpha3/zz_generated.deepcopy.go +++ b/api/v1alpha3/zz_generated.deepcopy.go @@ -824,7 +824,6 @@ func (in *GitTargetStatus) DeepCopyInto(out *GitTargetStatus) { (*in)[i].DeepCopyInto(&(*out)[i]) } } - in.LastReconcileTime.DeepCopyInto(&out.LastReconcileTime) if in.LastPushTime != nil { in, out := &in.LastPushTime, &out.LastPushTime *out = (*in).DeepCopy() @@ -832,7 +831,7 @@ func (in *GitTargetStatus) DeepCopyInto(out *GitTargetStatus) { if in.Streams != nil { in, out := &in.Streams, &out.Streams *out = new(GitTargetStreamsStatus) - (*in).DeepCopyInto(*out) + **out = **in } if in.Retention != nil { in, out := &in.Retention, &out.Retention @@ -854,10 +853,6 @@ func (in *GitTargetStatus) DeepCopy() *GitTargetStatus { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *GitTargetStreamsStatus) DeepCopyInto(out *GitTargetStreamsStatus) { *out = *in - if in.ObservedTime != nil { - in, out := &in.ObservedTime, &out.ObservedTime - *out = (*in).DeepCopy() - } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GitTargetStreamsStatus. @@ -1142,10 +1137,6 @@ func (in *WatchRuleStreamsStatus) DeepCopyInto(out *WatchRuleStreamsStatus) { *out = make([]string, len(*in)) copy(*out, *in) } - if in.ObservedTime != nil { - in, out := &in.ObservedTime, &out.ObservedTime - *out = (*in).DeepCopy() - } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new WatchRuleStreamsStatus. diff --git a/cmd/main.go b/cmd/main.go index ec5044c6..33095c5d 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -187,6 +187,7 @@ func main() { Scheme: mgr.GetScheme(), RuleStore: ruleStore, WatchManager: watchMgr, + Recorder: mgr.GetEventRecorderFor("watchrule"), }).SetupWithManager(mgr), "unable to create controller", "controller", "WatchRule") // ClusterWatchRule controller (with WatchManager reference for dynamic reconciliation) @@ -195,6 +196,7 @@ func main() { Scheme: mgr.GetScheme(), RuleStore: ruleStore, WatchManager: watchMgr, + Recorder: mgr.GetEventRecorderFor("clusterwatchrule"), }).SetupWithManager(mgr), "unable to create controller", "controller", "ClusterWatchRule") // Valkey/Redis is optional. When configured it holds each GitTarget's watch resume cursor so work @@ -282,6 +284,7 @@ func main() { Client: mgr.GetClient(), Scheme: mgr.GetScheme(), SSHHostKeys: cfg.sshHostKeys, + Recorder: mgr.GetEventRecorderFor("gitprovider"), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "GitProvider") os.Exit(1) @@ -291,6 +294,7 @@ func main() { Scheme: mgr.GetScheme(), OperatorNamespace: os.Getenv("POD_NAMESPACE"), KubeConfigSafety: cfg.kubeConfigSafety, + Recorder: mgr.GetEventRecorderFor("clusterprovider"), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "ClusterProvider") os.Exit(1) @@ -300,6 +304,7 @@ func main() { Scheme: mgr.GetScheme(), WorkerManager: workerManager, EventRouter: eventRouter, + Recorder: mgr.GetEventRecorderFor("gittarget"), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "GitTarget") os.Exit(1) diff --git a/config/crd/bases/configbutler.ai_clusterwatchrules.yaml b/config/crd/bases/configbutler.ai_clusterwatchrules.yaml index 549b38bd..e40a5e1e 100644 --- a/config/crd/bases/configbutler.ai_clusterwatchrules.yaml +++ b/config/crd/bases/configbutler.ai_clusterwatchrules.yaml @@ -275,10 +275,6 @@ spec: be watched. format: int32 type: integer - observedTime: - description: ObservedTime is when this roll-up was last computed. - format: date-time - type: string pendingSample: description: PendingSample is a bounded sample of types not yet ready. diff --git a/config/crd/bases/configbutler.ai_gittargets.yaml b/config/crd/bases/configbutler.ai_gittargets.yaml index 25f1f875..07864f8f 100644 --- a/config/crd/bases/configbutler.ai_gittargets.yaml +++ b/config/crd/bases/configbutler.ai_gittargets.yaml @@ -15,15 +15,6 @@ spec: scope: Namespaced versions: - additionalPrinterColumns: - - jsonPath: .spec.providerRef.name - name: Provider - type: string - - jsonPath: .spec.branch - name: Branch - type: string - - jsonPath: .spec.path - name: Path - type: string - jsonPath: .status.conditions[?(@.type=="Ready")].status name: Ready type: string @@ -65,6 +56,18 @@ spec: name: Encryption priority: 1 type: string + - jsonPath: .spec.providerRef.name + name: Provider + priority: 1 + type: string + - jsonPath: .spec.branch + name: Branch + priority: 1 + type: string + - jsonPath: .spec.path + name: Path + priority: 1 + type: string - jsonPath: .metadata.creationTimestamp name: Age type: date @@ -444,11 +447,6 @@ spec: push. format: date-time type: string - lastReconcileTime: - description: LastReconcileTime is the timestamp of the most recent - reconcile attempt. - format: date-time - type: string observedGeneration: description: ObservedGeneration is the latest generation observed by the controller. @@ -494,10 +492,6 @@ spec: be watched. format: int32 type: integer - observedTime: - description: ObservedTime is when this roll-up was last computed. - format: date-time - type: string ready: description: Ready is how many tracked types are Streaming. format: int32 @@ -508,7 +502,12 @@ spec: format: int32 type: integer summary: - description: Summary is the display-only ready/total ratio. + description: |- + Summary is the display-only ready/total ratio, e.g. "3/4". + + It restates Ready and Total, which the API conventions would normally rule out. It exists + solely to feed the Streams printer column: a column can read one JSONPath, not format two. + Do not compute anything from it — read ready and total. type: string total: description: Total is how many types this target tracks. diff --git a/config/crd/bases/configbutler.ai_watchrules.yaml b/config/crd/bases/configbutler.ai_watchrules.yaml index af458b92..6e4f2dc7 100644 --- a/config/crd/bases/configbutler.ai_watchrules.yaml +++ b/config/crd/bases/configbutler.ai_watchrules.yaml @@ -287,10 +287,6 @@ spec: be watched. format: int32 type: integer - observedTime: - description: ObservedTime is when this roll-up was last computed. - format: date-time - type: string pendingSample: description: PendingSample is a bounded sample of types not yet ready. diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 5091f084..bcb1e628 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -4,6 +4,13 @@ kind: ClusterRole metadata: name: gitops-reverser rules: +- apiGroups: + - "" + resources: + - events + verbs: + - create + - patch - apiGroups: - "" resources: diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 49c3c111..a2a0098d 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,51 @@ guidance that the changelog's breaking-change entries link to. We are pre-1.0, so breaking changes bump the **minor** version (release-please is configured with `bump-minor-pre-major`) rather than the major. Read the relevant entry before upgrading across it. +## Unreleased: status vocabulary and two removed status fields (next minor; status-only change) + +Nothing in `spec` changed and no manifest you wrote needs editing. Three things a **reader of +status** may notice: + +**1. The generic condition reason is now `Succeeded`, not `OK` or `Ready`.** A reason that restates +the condition type (`Ready=True, reason=Ready`) answers nothing. The generic reasons are now aliases +of [`github.com/fluxcd/pkg/apis/meta`](https://pkg.go.dev/github.com/fluxcd/pkg/apis/meta) +(`Succeeded`, `Failed`, `Progressing`, `DependencyNotReady`), so one alerting rule works across every +kind here and across every Flux kind in the same cluster. Domain reasons (`UnsupportedContent`, +`WriteBoundaryRefused`, `NoAdmittedSourceNamespaces`, …) are unchanged. + +If you match on a reason string, update: + +| Was | Now | +|---|---| +| `Ready=True`, reason `OK` (GitTarget, `Validated` too) | reason `Succeeded` | +| `Ready=True`, reason `Ready` (GitProvider, ClusterProvider, WatchRule, ClusterWatchRule) | reason `Succeeded` | + +Matching on `status` rather than `reason` (the usual case, and what +`kubectl wait --for=condition=Ready` does) needs no change. + +**2. `GitTarget.status.lastReconcileTime` and `status.streams.observedTime` were removed.** Both were +stamped with the current time on every pass, which made every reconcile a status write, and every +status write a fresh watch event that re-queued the object. A condition's `lastTransitionTime` plus +the `controller_runtime_reconcile_total` metric answer the same question without making every object +mutable on read. `status.lastPushTime` (a real event) and `status.retention.observedTime` (the +data plane's own observation time, which you need to tell a stale zero from a live one) both stay. + +**3. A GitTarget with no WatchRules now reports `Ready=True`.** It used to sit at +`Ready=False`/`Reconciling=True`/`NoResolvedTypes` forever, a state the documented setup flow passes +through, since you create the GitTarget before its rules. Nothing was pending and nothing ever would +be, so `kubectl wait --for=condition=Ready` never returned and the object re-reconciled every 10 +seconds for its whole life. "Nothing to mirror" is a converged state. `status.streams.summary` still +reads `0/0` and `StreamsRunning` still carries reason `NoResolvedTypes`, so the zero stays visible. + +If you have automation that treats "GitTarget Ready" as "GitTarget is mirroring something", assert +`status.streams.total > 0` as well. + +**Also in this change, with no action needed:** GitTarget's `kubectl get` output drops to four +columns (`Ready`, `Reason`, `Streams`, `Age`); `Provider`, `Branch` and `Path` moved to +`-o wide`. And every controller now emits a Kubernetes Event on each persisted `Ready` transition, +visible in `kubectl describe` and routable by anything that consumes Events. The operator's +ServiceAccount gained `create`/`patch` on `events` for it, which `helm upgrade` applies. + ## Unreleased — a resync no longer deletes Git documents by default (next minor; behavior change) **`GitTarget` gained `spec.prune.mode`, and its effective default changes what a resync does.** diff --git a/docs/design/reconcile-triggering.md b/docs/design/reconcile-triggering.md index 55977c2c..025045b4 100644 --- a/docs/design/reconcile-triggering.md +++ b/docs/design/reconcile-triggering.md @@ -45,11 +45,19 @@ Facts first, then the fix, then the gaps, then the toolbox. | Controller | Watches (besides `For`) | Predicate | Happy-path requeue | Data-plane edge | |---|---|---|---|---| | **GitProvider** | *(none)* | `GenerationChanged` on self | 5 min | ❌ | -| **GitTarget** | GitProvider, WatchRule, ClusterWatchRule | `GenerationChanged` on deps | 5 min | ✅ GitPath channel (new) | -| **WatchRule** | GitTarget, GitProvider | `GenerationChanged` | 5 min | ❌ | -| **ClusterWatchRule** | GitTarget, GitProvider | `GenerationChanged` | 5 min | ❌ | +| **GitTarget** | GitProvider, ClusterProvider, Namespace, WatchRule, ClusterWatchRule | `GenerationChanged` on **self** and on deps | 5 min converged / 10 s settling | ✅ GitPath channel | +| **WatchRule** | GitTarget, GitProvider, ClusterProvider | `GenerationChanged` on **self** and on deps | 5 min converged / 10 s settling | ✅ source-namespace channel | +| **ClusterWatchRule** | GitTarget, GitProvider | `GenerationChanged` on **self** and on deps | 5 min converged / 10 s settling | ❌ | | **CommitRequest** | *(none)* | — | polls every 2 s | polls worker, no push | +The `For()` predicate on GitTarget, WatchRule and ClusterWatchRule is not an +optimisation, it closes a self-triggering edge: a status write bumps `resourceVersion` +and fires an Update watch event that `EnqueueRequestForObject` turns straight back into +a queued request, un-rate-limited, so every reconcile cost roughly two. The shared status +writer already suppresses no-op writes (see +[`../spec/status-conditions-guide.md`](../spec/status-conditions-guide.md)); the predicate +makes it structural, and matches what GitProvider and ClusterProvider already did. + The GitTarget no longer watches Secrets: the encryption-Secret watch was removed so the process stops retaining every Secret value in the cluster; generated-age-Secret recovery and out-of-band age-key updates ride the 5-minute reconcile instead. @@ -221,13 +229,22 @@ status heartbeat. The fix for B1 is the same shape: `predicate.Or( GenerationChangedPredicate{}, gitTargetReadinessChangedPredicate{})` rather than removing the predicate (which reintroduces the storm). -### F4. Conditions **and** Events, every loop (fixes Gap A surfacing) +### F4. Conditions **and** Events, every loop (fixes Gap A surfacing) — SHIPPED Flux always patches kstatus conditions (`fluxcd/pkg/runtime/conditions` + `patch`) **and** emits a Kubernetes Warning Event (`fluxcd/pkg/runtime/events`, which also forwards to notification-controller), then requeues at a shorter failure interval. A failure is immediately visible and alertable, not buried in a metric. +**Built.** Every reconciler takes an `EventRecorder` and the shared status writer +(`internal/controller/status.go`) emits one Event per **persisted** `Ready` +transition: `Normal` when `Ready=True`, `Warning` otherwise, carrying the condition's +reason and message. Emitting after the patch rather than beside each condition write +is deliberate — a reconcile may write `Ready` several times and only the last is +stored, so intermediate values never reach `kubectl describe`. That also means the +suppressed no-op status write emits nothing, which is the intended behavior: nothing +changed, so nothing happened. + ### F5. Generalize the data-plane nudge (the §2 recipe, reused) Extend the GitPath channel into a small shared "status nudge": the data plane diff --git a/docs/facts/subresources.md b/docs/facts/subresources.md index c7f8dcf8..49ebe131 100644 --- a/docs/facts/subresources.md +++ b/docs/facts/subresources.md @@ -200,11 +200,10 @@ spec: path: clusters/dev status: observedGeneration: 4 - lastReconcileTime: "2026-06-08T10:00:00Z" conditions: - type: Ready status: "True" - reason: OK + reason: Succeeded ``` The user owns `spec`; the controller owns `status`. `observedGeneration` is what diff --git a/docs/future/flux-maintainer-review-status-and-config-model.md b/docs/future/flux-maintainer-review-status-and-config-model.md index 4b570b95..6d87e0f7 100644 --- a/docs/future/flux-maintainer-review-status-and-config-model.md +++ b/docs/future/flux-maintainer-review-status-and-config-model.md @@ -4,9 +4,16 @@ > Date: 2026-07-21 > Reviewed at: branch `feat/gittarget-prune-mode-pr5`, commit `f37a7ba`. > -> **Scheduled and done so far:** F12's *enum casing* only — `PruneMode` is now -> `Never`/`OnEvent`/`Always`, taken before the release because it was the last moment it was free. -> Every other finding, including the rest of F12, is still open. +> **Done so far.** F12's *enum casing* (`PruneMode` is now `Never`/`OnEvent`/`Always`, taken before +> the release because it was the last moment it was free), and then the whole of §4's +> **"Before the next release"** and **"Next"** blocks: **F1, F2, F3, F5, F7, F8, F11** and the rest +> of **F12**, shipped together on `feat/flux-status-contract`. See §6 for exactly what landed and +> what each fix looks like now. +> +> **Still open:** **F4** (abnormal-true polarity — resolved the other way, deliberately; see §6), +> **F6** (`spec.suspend`, `spec.interval`, reconcile-request annotation), **F9** (the stored +> `scope: Namespaced` status-write question), **F10** (CommitRequest lifecycle). Those are §4's +> "Then (API surface)" block and are a separate change. > Stance: reviewed as if this API were proposed for the GitOps Toolkit, with Flux's own > source (`external-sources/flux/`) and kstatus (`sigs.k8s.io/cli-utils/pkg/kstatus`) as ground > truth rather than recollection. @@ -650,3 +657,57 @@ object *operable* by the reflexes a platform team already has from Flux, and rig those reflexes return anything here. Most of it is already designed in `docs/design/reconcile-triggering.md`; it needs building. `spec.suspend` first, because this controller writes to Git and there is currently no way to make it stop. + +--- + +## 6. What was built (2026-07-22, branch `feat/flux-status-contract`) + +The review's own ordering was followed: §4's "Before the next release" and "Next" blocks, plus the +cheap F12 nits. The API-surface block (F6, F9, F10) is deliberately left for a separate change, +because each of those adds or changes a spec field and deserves its own review. + +Two pieces of shared machinery carry most of it, and both replace code that had been copied five or +six times: + +**`internal/controller/readiness.go` — the accumulator that owns the trio.** Gates no longer set +`Ready`/`Reconciling`/`Stalled`; they *contribute* a verdict at one of three levels (converged, +progressing, stalled) and the trio is derived once at the end. Within a level the first contributor +wins, so the precedence is the order of a handful of adjacent calls in `gitTargetReadinessGates` / +`ruleReadiness` — readable in one place rather than emergent from which gate ran last. This is the +`summarize`-once shape the review pointed at, without taking `fluxcd/pkg/runtime` as a dependency. + +**`internal/controller/status.go` — the per-reconcile status session.** `beginStatus` captures the +object as read, `set`/`applyReadiness` collect what the reconcile wants to say, and `commit` writes +the difference exactly once. It replaced five near-identical `updateStatusWithRetry` copies and, with +them, three shared defects, plus six `setCondition`/`setTypedCondition` wrappers and eight +trio-setting helpers (`setStalledConditions` ×3, `setProgressingConditions`, `setReadyConditions`, +`setRuleStalled` ×2, `setRuleProgressing`, `downgradeReady`). + +| Finding | What landed | +|---|---| +| **F1** — `downgradeReady` erased a terminal `Stalled` | The accumulator. `downgradeReady` is gone; the source/provider projection now only *publishes* its three conditions and contributes progressing verdicts. The regression cell ("refused path **and** unready provider") is a table row in `gittarget_status_test.go` that runs the real `kstatus.Compute` over reconciler output. | +| **F2** — a GitTarget with no WatchRules never reached `Current` | `streamsAxis` reports `True` when `Ready == Total`, which is vacuously true at zero. `status.streams.summary` still reads `0/0` and the reason is still `NoResolvedTypes`, so the zero stays visible. "Data plane not wired" is now a *different* state (reason `Progressing`) so the two can never be confused. | +| **F3** — unconditional writes, moving timestamps, no self-predicate | All three legs. `commit` sends nothing when the status is unchanged; `status.lastReconcileTime` and `status.streams.observedTime` were removed rather than excluded from the comparison (they existed only to be overwritten); `GenerationChangedPredicate` added to the `For()` of GitTarget, WatchRule and ClusterWatchRule. | +| **F5** — `upsertCondition` reordered the list on every touch | Delegates to `apimeta.SetStatusCondition`, which updates in place. The test now pins position stability rather than de-duplication of an input `listType=map` makes impossible. | +| **F7** — zero Kubernetes Events | Every reconciler takes an `EventRecorder`; the shared writer emits one Event per **persisted** `Ready` transition (`Normal` on True, `Warning` otherwise). After the patch, not beside each `set`, so intermediate values a reconcile never stored are never announced. | +| **F8** — ad-hoc reasons, `Reason == Type` | Generic reasons alias `fluxcd/pkg/apis/meta`. `OK` and `Ready` as *reasons* became `Succeeded` across all six kinds and the e2e suite. Domain reasons kept. | +| **F11** — `observedGeneration` for a generation never observed | Comes free with the patch: the write uses optimistic concurrency, so a spec that moved under the reconcile loses the write instead of being mislabelled. The conflict is dropped rather than retried — the write that beat us already enqueued a fresh pass. | +| **F12** — API-conventions nits | GitTarget's default printer columns cut from seven to four (`Provider`/`Branch`/`Path` moved to `priority=1`); `patchStrategy`/`patchMergeKey` added to `GitProviderStatus.Conditions`; `ObjectMeta` json tags unified on `omitempty,omitzero`; `status.streams.summary`'s field doc now says *why* it duplicates the counts beside it. | + +### F4 was resolved the other way, on purpose + +The review is right that the code and `docs/spec/status-conditions-guide.md` disagreed, and right +that "pick one" was the answer. The one picked was **keep writing the pair when False**, and the +guide now says so and says why. + +kstatus tolerates it — it tests for `== True` and ignores everything else — so nothing downstream +misreads it. Against that: `kubectl wait --for=condition=Stalled=false` reads the explicit `False`, +this repo's e2e suite asserts it in several specs, and a condition that vanishes is harder for a +human to reason about than one that reads `False`. The cost of the deviation is three extra condition +entries in `-o yaml`; the cost of removing it is a real capability plus a suite-wide rewrite. What +*was* wrong and is now fixed is the incoherent message the review quoted +(`Reconciling=False, message="Reconciliation is stalled"` is at least true of the object, and it is +now written from one place rather than as leftover state from a gate that lost). + +The deviation is recorded in the guide as the single knowing departure from the API conventions, so +the two documents cannot silently drift again. diff --git a/docs/spec/status-conditions-guide.md b/docs/spec/status-conditions-guide.md index 48dc4d0e..650488d7 100644 --- a/docs/spec/status-conditions-guide.md +++ b/docs/spec/status-conditions-guide.md @@ -20,7 +20,61 @@ A list treated as a map keyed by `type`. Don't append duplicates — update the `kubectl wait --for=condition=Ready=true` works by watching this array until the matching type hits `"True"`. -Only update `lastTransitionTime` when `status` actually changes — not on every reconcile. +Only update `lastTransitionTime` when `status` actually changes — not on every reconcile. Update the +existing entry **in place**: a setter that removes and re-appends reorders the list on every touch, +which is a diff in `kubectl get -o yaml` and, for a cluster that mirrors its own config objects into +Git through this operator, a stream of commits that reorder conditions and change nothing. + +### One writer for the trio + +`Ready`, `Reconciling` and `Stalled` are derived **together, once, at the end of a reconcile**, from +a precedence stated in one place. Gates do not set them; gates *contribute* to an accumulator +(`internal/controller/readiness.go`) and the trio falls out of the worst contribution. + +This is not a style preference. When each gate wrote the trio itself, the trio said whatever the last +gate said — so a GitTarget whose Git path had been refused (terminal) was then handed to the +source/provider projection, which stamped `Stalled=False, Reconciling=True` over the refusal because +a provider happened to be mid-check. To a human reading `Ready` little changed. To kstatus — which +never reads `Ready`, only the abnormal-true pair — the object flipped from `Failed` to `InProgress`, +so `kubectl wait` and every CI gate built on it waited out its timeout on an object that was never +going to converge. + +### Status writes are suppressed when nothing changed + +`reconcileStatus.commit` computes the difference between the status as read and the status as +written, and sends **nothing** when they are equal. A status write bumps `resourceVersion`, which +fires an Update watch event, which the controller's own `For()` turns straight back into a queued +request — so an unconditional write makes every reconcile cost roughly two. Status fields that move +on every pass (a "last reconcile attempt" timestamp) defeat this by construction and were removed; +`lastTransitionTime` plus `controller_runtime_reconcile_total` answer the same question without +making every object mutable on read. + +The write is a status **patch with optimistic concurrency**, and a conflict is dropped rather than +retried: a conflict means the object moved under this reconcile, so the status just computed +describes a generation that is no longer current, and the write that beat us has already enqueued a +fresh pass. + +### Reason vocabulary + +Generic reasons are aliases of [`github.com/fluxcd/pkg/apis/meta`](https://pkg.go.dev/github.com/fluxcd/pkg/apis/meta) +— `Succeeded`, `Failed`, `Progressing`, `DependencyNotReady` — a module this project already depends +on. Sharing the vocabulary means one alerting rule works across every kind here *and* across every +Flux kind in the same cluster. A reason that restates the condition type (`Ready=True, reason=Ready`) +answers nothing and is not used. + +Domain reasons stay this project's own — `UnsupportedContent`, `WriteBoundaryRefused`, +`IgnoreShadowsManagedPath`, `NoAdmittedSourceNamespaces` — because they carry information a generic +reason cannot. Declaring domain reasons is exactly what the upstream vocabulary asks projects to do. + +### One deliberate deviation: the abnormal-true pair is written when False + +The Kubernetes API conventions say an abnormal-true condition SHOULD only be present when `True`, and +Flux deletes `Reconciling`/`Stalled` rather than writing them `False`. This project writes them +either way. kstatus tolerates it (it tests for `== True` and ignores everything else), and both +`kubectl wait --for=condition=Stalled=false` and this repo's e2e suite read the explicit `False`. A +condition that vanishes is harder to reason about than one that reads `False`. This is the only place +the project knowingly departs from the conventions, and it is recorded here so the code and this +document cannot silently disagree. ## Best practices @@ -35,7 +89,16 @@ Only update `lastTransitionTime` when `status` actually changes — not on every success, `False` = failed, `Unknown` = in progress — all unambiguous. 4. **Don't duplicate between conditions and status fields.** A string field that mirrors a - condition is redundant noise. Pick one representation. + condition is redundant noise. Pick one representation. The one exception in this project is + `status.streams.summary` (`"3/4"`), which restates `ready` and `total` beside it: a printer + column can read one JSONPath, not format two. Its field doc says so, so the next reader does not + "clean it up". + +5. **Emit an Event on every persisted `Ready` transition.** Conditions say what is true now; Events + say what happened. A transient failure that clears before anyone looks is invisible without them, + and an Event-driven alerting pipeline has nothing to route. They are emitted after the status + patch lands, not beside each condition write, so a reconcile that writes `Ready` twice (a + placeholder, then the real outcome) announces only the value that was actually stored. ## Applied to this project @@ -66,7 +129,12 @@ const ( Canonical reads: -* fully mirrored: `Ready=True`, `Reconciling=False`, `Stalled=False` +* fully mirrored: `Ready=True`, `Reconciling=False`, `Stalled=False`, reason `Succeeded` +* **nothing to mirror** — no WatchRule has claimed the GitTarget yet, or its rules were deleted: + `Ready=True`, `Reconciling=False`, `Stalled=False`. "I have nothing to mirror" is a *converged* + state, not a pending one; `status.streams.summary` keeps showing `0/0` and `StreamsRunning` keeps + reason `NoResolvedTypes`, so the zero stays visible without being reported as a failure to + converge. (Flux's Kustomization with an empty path reports the same.) * initial replay or recheck: `Ready=False`, `Reconciling=True`, `Stalled=False` * refused Git path, invalid provider, RBAC denial, or broken encryption: `Ready=False`, `Reconciling=False`, `Stalled=True` diff --git a/internal/controller/clusterprovider_controller.go b/internal/controller/clusterprovider_controller.go index ef548e6b..878b58d7 100644 --- a/internal/controller/clusterprovider_controller.go +++ b/internal/controller/clusterprovider_controller.go @@ -12,7 +12,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" k8stypes "k8s.io/apimachinery/pkg/types" - "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" @@ -67,6 +67,9 @@ type ClusterProviderReconciler struct { KubeConfigSafety kubeconfig.SafetyPolicy firsts clusterProviderLogFirsts + + // Recorder emits a Kubernetes Event on every persisted Ready transition; nil disables Events. + Recorder record.EventRecorder } // clusterProviderLogFirsts keeps startup progress visible without turning every routine @@ -79,6 +82,7 @@ type clusterProviderLogFirsts struct { // It never creates or deletes one, so it takes neither verb. // +kubebuilder:rbac:groups=configbutler.ai,resources=clusterproviders,verbs=get;list;watch;update;patch // +kubebuilder:rbac:groups=configbutler.ai,resources=clusterproviders/status,verbs=get;update;patch +// +kubebuilder:rbac:groups="",resources=events,verbs=create;patch // Reconcile validates a ClusterProvider's inputs and updates its status. func (r *ClusterProviderReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { @@ -149,32 +153,36 @@ func (r *ClusterProviderReconciler) reconcileClusterProvider( "inCluster", provider.IsInCluster(), "generation", provider.Generation) - r.setProgressingConditions(provider, ReasonChecking, "Validating cluster provider inputs...") + st := beginStatus(r.Client, r.Recorder, provider, &provider.Status.Conditions) + provider.Status.ObservedGeneration = provider.Generation valid, reason, message, err := r.validateProviderKubeConfig(ctx, provider) if err != nil { log.Error(err, "failed to read ClusterProvider kubeconfig Secret", "name", provider.Name) return ctrl.Result{}, err } + + validated := metav1.ConditionTrue + rd := newReadiness(message, "ClusterProvider is not stalled") if !valid { - r.setCondition(provider, ClusterProviderConditionValidated, metav1.ConditionFalse, reason, message) - r.setStalledConditions(provider, reason, message) - // A failed status write is a real failure, not a verdict: propagate it so the invalid - // provider is retried rather than left reporting a stale status for a whole steady interval. - return r.updateStatusAndRequeue(ctx, provider) + validated = metav1.ConditionFalse + rd.stalled(reason, message) } + st.set(ClusterProviderConditionValidated, validated, reason, message) + st.applyReadiness(rd) - r.setCondition(provider, ClusterProviderConditionValidated, metav1.ConditionTrue, reason, message) - r.setReadyConditions(provider, message) - - if err := r.updateStatusWithRetry(ctx, provider); err != nil { + // A failed status write is a real failure, not a verdict: propagate it so the provider is + // retried rather than left reporting a stale status for a whole steady interval. + if err := st.commit(ctx); err != nil { log.Error(err, "failed to update ClusterProvider status", "name", provider.Name) return ctrl.Result{}, err } - r.firsts.validationSuccess.Do(func() { - log.Info("First ClusterProvider validation completed successfully", "name", provider.Name) - }) + if valid { + r.firsts.validationSuccess.Do(func() { + log.Info("First ClusterProvider validation completed successfully", "name", provider.Name) + }) + } return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil } @@ -226,96 +234,6 @@ func (r *ClusterProviderReconciler) validateProviderKubeConfig( return true, ReasonValidated, fmt.Sprintf("kubeconfig Secret %s validated", secretKey), nil } -func (r *ClusterProviderReconciler) setReadyConditions( - provider *configbutleraiv1alpha3.ClusterProvider, - message string, -) { - r.setCondition(provider, ConditionTypeReady, metav1.ConditionTrue, ConditionTypeReady, message) - r.setCondition(provider, ConditionTypeReconciling, metav1.ConditionFalse, ConditionTypeReady, - "Reconciliation complete") - r.setCondition(provider, ConditionTypeStalled, metav1.ConditionFalse, ConditionTypeReady, - "ClusterProvider is not stalled") -} - -func (r *ClusterProviderReconciler) setProgressingConditions( - provider *configbutleraiv1alpha3.ClusterProvider, - reason, message string, -) { - r.setCondition(provider, ConditionTypeReady, metav1.ConditionFalse, reason, message) - r.setCondition(provider, ConditionTypeReconciling, metav1.ConditionTrue, reason, message) - r.setCondition(provider, ConditionTypeStalled, metav1.ConditionFalse, reason, - "Reconciliation is making progress") -} - -func (r *ClusterProviderReconciler) setStalledConditions( - provider *configbutleraiv1alpha3.ClusterProvider, - reason, message string, -) { - r.setCondition(provider, ConditionTypeReady, metav1.ConditionFalse, reason, message) - r.setCondition(provider, ConditionTypeReconciling, metav1.ConditionFalse, reason, - "Reconciliation is stalled") - r.setCondition(provider, ConditionTypeStalled, metav1.ConditionTrue, reason, message) -} - -// setCondition sets or updates one condition by type and pins observedGeneration. -func (r *ClusterProviderReconciler) setCondition( - provider *configbutleraiv1alpha3.ClusterProvider, - conditionType string, - status metav1.ConditionStatus, - reason, message string, -) { - provider.Status.ObservedGeneration = provider.Generation - provider.Status.Conditions = upsertCondition( - provider.Status.Conditions, - conditionType, - status, - reason, - message, - provider.Generation, - ) -} - -// updateStatusAndRequeue updates the status and requeues on the steady interval. -func (r *ClusterProviderReconciler) updateStatusAndRequeue( - ctx context.Context, - provider *configbutleraiv1alpha3.ClusterProvider, -) (ctrl.Result, error) { - if err := r.updateStatusWithRetry(ctx, provider); err != nil { - return ctrl.Result{}, err - } - return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil -} - -// updateStatusWithRetry updates the status, re-reading the latest object on conflict. -func (r *ClusterProviderReconciler) updateStatusWithRetry( - ctx context.Context, - provider *configbutleraiv1alpha3.ClusterProvider, -) error { - return wait.ExponentialBackoff(wait.Backoff{ - Duration: RetryInitialDuration, - Factor: RetryBackoffFactor, - Jitter: RetryBackoffJitter, - Steps: RetryMaxSteps, - }, func() (bool, error) { - latest := &configbutleraiv1alpha3.ClusterProvider{} - key := client.ObjectKeyFromObject(provider) - if err := r.Get(ctx, key, latest); err != nil { - if apierrors.IsNotFound(err) { - return true, nil - } - return false, err - } - latest.Status = provider.Status - if err := r.Status().Update(ctx, latest); err != nil { - if apierrors.IsConflict(err) { - return false, nil - } - return false, err - } - return true, nil - }) -} - // clusterProviderReconcilePredicate admits spec changes and the start of deletion. A deletion // transition is needed only for the upgrade path: an older object can still carry the retired // fact-purge finalizer, and this controller must shed it promptly rather than waiting for periodic diff --git a/internal/controller/clusterprovider_controller_unit_test.go b/internal/controller/clusterprovider_controller_unit_test.go index e76cf13c..7ea88e28 100644 --- a/internal/controller/clusterprovider_controller_unit_test.go +++ b/internal/controller/clusterprovider_controller_unit_test.go @@ -269,12 +269,13 @@ func TestClusterProviderReconcile_InvalidStatusWriteFails(t *testing.T) { WithObjects(provider). WithStatusSubresource(&configbutleraiv1alpha3.ClusterProvider{}). WithInterceptorFuncs(interceptor.Funcs{ - SubResourceUpdate: func( + SubResourcePatch: func( _ context.Context, _ client.Client, _ string, _ client.Object, - _ ...client.SubResourceUpdateOption, + _ client.Patch, + _ ...client.SubResourcePatchOption, ) error { return errors.New("status write boom") }, @@ -317,17 +318,19 @@ func TestValidateProviderKubeConfig_NilSecretRef(t *testing.T) { assert.NotEmpty(t, msg) } -// TestClusterProviderUpdateStatus_DeletedObject checks the status writer treats a vanished object -// as done (the NotFound branch of the retry loop) rather than erroring. +// TestClusterProviderUpdateStatus_DeletedObject checks the shared status writer treats a vanished +// object as done rather than erroring: a reconcile that raced a delete must not fail the workqueue. func TestClusterProviderUpdateStatus_DeletedObject(t *testing.T) { cl := fake.NewClientBuilder(). WithScheme(scScheme(t)). WithStatusSubresource(&configbutleraiv1alpha3.ClusterProvider{}). Build() - r := &ClusterProviderReconciler{Client: cl, OperatorNamespace: cpOperatorNS} - // The object was never created, so the retry loop's Get returns NotFound -> success, no error. - err := r.updateStatusWithRetry(context.Background(), clusterProviderWithKubeConfig("gone", "", "")) - require.NoError(t, err) + provider := clusterProviderWithKubeConfig("gone", "", "") + st := beginStatus(cl, nil, provider, &provider.Status.Conditions) + st.set(ConditionTypeReady, metav1.ConditionTrue, ReasonSucceeded, "gone but written") + + // The object was never created, so the patch returns NotFound -> treated as done, no error. + require.NoError(t, st.commit(context.Background())) } // TestClusterProviderReconcile_RemoteInvalidKubeConfig checks a remote provider whose kubeconfig diff --git a/internal/controller/clusterwatchrule_admission_test.go b/internal/controller/clusterwatchrule_admission_test.go index 15f3b3be..2ecf9de6 100644 --- a/internal/controller/clusterwatchrule_admission_test.go +++ b/internal/controller/clusterwatchrule_admission_test.go @@ -289,14 +289,14 @@ func TestReconcile_RefusalStopsDataPlaneBeforePublishingStatus(t *testing.T) { cwaClusterProvider(cwaDenying()), &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: cwaTargetNS}}, }, interceptor.Funcs{ - SubResourceUpdate: func( + SubResourcePatch: func( ctx context.Context, c client.Client, _ string, - obj client.Object, opts ...client.SubResourceUpdateOption, + obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption, ) error { seq++ statusWrittenAt = seq compiledAtStatusWrite = len(f.store.SnapshotClusterWatchRules()) - return c.Status().Update(ctx, obj, opts...) + return c.Status().Patch(ctx, obj, patch, opts...) }, }) diff --git a/internal/controller/clusterwatchrule_controller.go b/internal/controller/clusterwatchrule_controller.go index ff878340..40aba8de 100644 --- a/internal/controller/clusterwatchrule_controller.go +++ b/internal/controller/clusterwatchrule_controller.go @@ -8,11 +8,10 @@ import ( "github.com/go-logr/logr" corev1 "k8s.io/api/core/v1" - k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" - "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" @@ -34,7 +33,7 @@ const ( ClusterWatchRuleReasonAccessDenied = "AccessDenied" ClusterWatchRuleReasonGitTargetNotFound = "GitTargetNotFound" ClusterWatchRuleReasonGitDestinationInvalid = "GitDestinationInvalid" - ClusterWatchRuleReasonReady = "Ready" + ClusterWatchRuleReasonReady = ReasonSucceeded ClusterWatchRuleReasonResourcesResolved = "Resolved" ClusterWatchRuleReasonUnresolvedResources = "UnresolvedResources" @@ -56,6 +55,8 @@ type ClusterWatchRuleReconciler struct { Scheme *runtime.Scheme RuleStore *rulestore.RuleStore WatchManager WatchManagerInterface + // Recorder emits a Kubernetes Event on every persisted Ready transition; nil disables Events. + Recorder record.EventRecorder } // +kubebuilder:rbac:groups=configbutler.ai,resources=clusterwatchrules,verbs=get;list;watch;create;update;patch;delete @@ -101,73 +102,59 @@ func (r *ClusterWatchRuleReconciler) Reconcile(ctx context.Context, req ctrl.Req "target", clusterRule.Spec.TargetRef, "generation", clusterRule.Generation, "resourceVersion", clusterRule.ResourceVersion) + st := beginStatus(r.Client, r.Recorder, &clusterRule, &clusterRule.Status.Conditions) clusterRule.Status.ObservedGeneration = clusterRule.Generation - // Set initial validating status - log.Info("Setting initial validating status") - r.setCondition(&clusterRule, metav1.ConditionUnknown, - ClusterWatchRuleReasonValidating, "Validating ClusterWatchRule configuration...") - r.setTypedCondition( - &clusterRule, + // Seed the axis conditions as not-yet-evaluated. There is deliberately no placeholder write of + // the Ready/Reconciling/Stalled trio here: every path below ends in exactly one applyReadiness. + st.set( ConditionTypeStreamsRunning, metav1.ConditionUnknown, GitTargetStreamsRunningReasonNotReady, "Blocked by validation; streams not evaluated", ) - r.setTypedCondition( - &clusterRule, + st.set( ConditionTypeGitTargetReady, metav1.ConditionUnknown, ReasonProgressing, "Blocked by validation; GitTarget not evaluated", ) - r.setTypedCondition(&clusterRule, ConditionTypeReconciling, metav1.ConditionTrue, ReasonChecking, - "Validating ClusterWatchRule") - r.setTypedCondition(&clusterRule, ConditionTypeStalled, metav1.ConditionFalse, ReasonChecking, - "ClusterWatchRule is not stalled") // Delegate to target-based reconciliation - return r.reconcileClusterWatchRuleViaTarget(ctx, &clusterRule) + return r.reconcileClusterWatchRuleViaTarget(ctx, st, &clusterRule) } // reconcileClusterWatchRuleViaTarget validates and stores a ClusterWatchRule that references a GitTarget. func (r *ClusterWatchRuleReconciler) reconcileClusterWatchRuleViaTarget( ctx context.Context, + st *reconcileStatus, clusterRule *configbutleraiv1alpha3.ClusterWatchRule, ) (ctrl.Result, error) { log := logf.FromContext(ctx).WithName("reconcileClusterWatchRuleViaTarget") // Target is required if clusterRule.Spec.TargetRef.Name == "" { - r.setCondition(clusterRule, metav1.ConditionFalse, ClusterWatchRuleReasonGitDestinationInvalid, - "Target.name must be specified for ClusterWatchRule") - r.setTypedCondition( - clusterRule, + st.set( ConditionTypeGitTargetReady, metav1.ConditionFalse, ClusterWatchRuleReasonGitDestinationInvalid, "Target.name must be specified for ClusterWatchRule", ) - r.setRuleStalled(clusterRule, ClusterWatchRuleReasonGitDestinationInvalid, + return r.stallRule(ctx, st, ClusterWatchRuleReasonGitDestinationInvalid, "Target.name must be specified for ClusterWatchRule") - return r.updateStatusAndRequeue(ctx, clusterRule) } // For ClusterWatchRule, target namespace must be specified targetNS := clusterRule.Spec.TargetRef.Namespace if targetNS == "" { - r.setCondition(clusterRule, metav1.ConditionFalse, ClusterWatchRuleReasonGitDestinationInvalid, - "Target.namespace must be specified for ClusterWatchRule") - r.setTypedCondition( - clusterRule, + st.set( ConditionTypeGitTargetReady, metav1.ConditionFalse, ClusterWatchRuleReasonGitDestinationInvalid, "Target.namespace must be specified for ClusterWatchRule", ) - r.setRuleStalled(clusterRule, ClusterWatchRuleReasonGitDestinationInvalid, + return r.stallRule(ctx, st, ClusterWatchRuleReasonGitDestinationInvalid, "Target.namespace must be specified for ClusterWatchRule") - return r.updateStatusAndRequeue(ctx, clusterRule) } // Fetch GitTarget @@ -177,10 +164,16 @@ func (r *ClusterWatchRuleReconciler) reconcileClusterWatchRuleViaTarget( log.Error(err, "Failed to get referenced GitTarget", "gitTargetName", clusterRule.Spec.TargetRef.Name, "gitTargetNamespace", targetNS) - r.setGitTargetNotFound(clusterRule, targetNS, err) - return r.updateStatusAndRequeue(ctx, clusterRule) + st.set( + ConditionTypeGitTargetReady, + metav1.ConditionFalse, + ClusterWatchRuleReasonGitTargetNotFound, + fmt.Sprintf("Referenced GitTarget '%s/%s' not found: %v", + targetNS, clusterRule.Spec.TargetRef.Name, err), + ) + return r.stallRule(ctx, st, ClusterWatchRuleReasonGitTargetNotFound, "Referenced GitTarget not found") } - r.setGitTargetReadyCondition(clusterRule, target) + r.setGitTargetReadyCondition(st, target) // Resolve GitProvider from target providerName := target.Spec.ProviderRef.Name @@ -191,22 +184,14 @@ func (r *ClusterWatchRuleReconciler) reconcileClusterWatchRuleViaTarget( if err := r.Get(ctx, providerKey, &provider); err != nil { log.Error(err, "Failed to resolve GitProvider from GitTarget", "gitProviderName", providerName, "gitProviderNamespace", providerNS) - r.setCondition( - clusterRule, + st.set( + ConditionTypeGitTargetReady, metav1.ConditionFalse, ClusterWatchRuleReasonGitProviderNotFound, fmt.Sprintf("GitProvider '%s/%s' (from GitTarget) not found: %v", providerNS, providerName, err), ) - r.setTypedCondition( - clusterRule, - ConditionTypeGitTargetReady, - metav1.ConditionFalse, - ClusterWatchRuleReasonGitProviderNotFound, - "Referenced GitProvider not found", - ) - r.setRuleStalled(clusterRule, ClusterWatchRuleReasonGitProviderNotFound, "Referenced GitProvider not found") - return r.updateStatusAndRequeue(ctx, clusterRule) + return r.stallRule(ctx, st, ClusterWatchRuleReasonGitProviderNotFound, "Referenced GitProvider not found") } // Ready check @@ -216,7 +201,7 @@ func (r *ClusterWatchRuleReconciler) reconcileClusterWatchRuleViaTarget( // deliberately no AddOrUpdateClusterWatchRule here: routing every compilation through // watch.CompileClusterWatchRule is what stops the startup bootstrap from being a second, // ungated path into the store. - if handled, result, err := r.gateClusterWatchRule(ctx, clusterRule, target, provider, log); handled { + if handled, result, err := r.gateClusterWatchRule(ctx, st, clusterRule, target, provider, log); handled { return result, err } @@ -226,14 +211,20 @@ func (r *ClusterWatchRuleReconciler) reconcileClusterWatchRuleViaTarget( log.Error(err, "Failed to reconcile watch manager after cluster rule update") // Don't fail the reconciliation - the rule is valid, just log the watch manager issue } - r.setResourceResolutionCondition(ctx, clusterRule) - r.setStreamsReadyCondition(clusterRule, r.WatchManager.StreamSummaryForClusterWatchRule(*clusterRule)) + r.setResourceResolutionCondition(ctx, st, clusterRule) + r.setStreamsReadyCondition(st, clusterRule, r.WatchManager.StreamSummaryForClusterWatchRule(*clusterRule)) } else { - r.setStreamsReadyCondition(clusterRule, noResolvedStreamsSummary()) + r.setStreamsReadyCondition(st, clusterRule, noResolvedStreamsSummary()) } log.Info("ClusterWatchRule reconciliation via GitTarget successful", "name", clusterRule.Name) - return r.setReadyAndUpdateStatusWithTarget(ctx, clusterRule) + + msg := fmt.Sprintf( + "ClusterWatchRule is ready and monitoring resources via GitTarget '%s/%s'", + clusterRule.Spec.TargetRef.Namespace, + clusterRule.Spec.TargetRef.Name, + ) + return r.commitRule(ctx, st, ruleReadiness(clusterRule.Status.Conditions, "ClusterWatchRule", msg)) } // gateClusterWatchRule is the ClusterWatchRule gate and the ONE place this controller compiles a @@ -255,6 +246,7 @@ func (r *ClusterWatchRuleReconciler) reconcileClusterWatchRuleViaTarget( // unchanged. func (r *ClusterWatchRuleReconciler) gateClusterWatchRule( ctx context.Context, + st *reconcileStatus, clusterRule *configbutleraiv1alpha3.ClusterWatchRule, target configbutleraiv1alpha3.GitTarget, provider configbutleraiv1alpha3.GitProvider, @@ -273,7 +265,7 @@ func (r *ClusterWatchRuleReconciler) gateClusterWatchRule( return false, ctrl.Result{}, nil } - result, refuseErr := r.refuseClusterWatchRule(ctx, clusterRule, decision, log) + result, refuseErr := r.refuseClusterWatchRule(ctx, st, clusterRule, decision, log) return true, result, refuseErr } @@ -291,6 +283,7 @@ func (r *ClusterWatchRuleReconciler) gateClusterWatchRule( // edit converting the rule to the cluster-only model. func (r *ClusterWatchRuleReconciler) refuseClusterWatchRule( ctx context.Context, + st *reconcileStatus, clusterRule *configbutleraiv1alpha3.ClusterWatchRule, decision watch.ClusterWatchRuleDecision, log logr.Logger, @@ -311,91 +304,58 @@ func (r *ClusterWatchRuleReconciler) refuseClusterWatchRule( } } - r.setTypedCondition( - clusterRule, + st.set( ConditionTypeGitTargetReady, metav1.ConditionFalse, decision.Reason, decision.Message, ) - r.setTypedCondition( - clusterRule, + st.set( ConditionTypeStreamsRunning, metav1.ConditionFalse, decision.Reason, "No streams: the ClusterWatchRule was refused", ) - r.setRuleStalled(clusterRule, decision.Reason, decision.Message) - return r.updateStatusAndRequeue(ctx, clusterRule) + return r.stallRule(ctx, st, decision.Reason, decision.Message) } -// setReadyAndUpdateStatusWithTarget sets Ready with target message and updates status with retry. -func (r *ClusterWatchRuleReconciler) setReadyAndUpdateStatusWithTarget( +// stallRule publishes a terminal ClusterWatchRule outcome and ends the reconcile. +func (r *ClusterWatchRuleReconciler) stallRule( ctx context.Context, - clusterRule *configbutleraiv1alpha3.ClusterWatchRule, -) (ctrl.Result, error) { - msg := fmt.Sprintf( - "ClusterWatchRule is ready and monitoring resources via GitTarget '%s/%s'", - clusterRule.Spec.TargetRef.Namespace, - clusterRule.Spec.TargetRef.Name, - ) - r.setRuleKstatus(clusterRule, msg) - if err := r.updateStatusWithRetry(ctx, clusterRule); err != nil { - return ctrl.Result{}, err - } - if conditionIsFalse(clusterRule.Status.Conditions, ConditionTypeResourcesResolved) { - return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil - } - if !conditionIsTrue(clusterRule.Status.Conditions, ConditionTypeGitTargetReady) { - return ctrl.Result{RequeueAfter: RequeueStreamSettleInterval}, nil - } - if !conditionIsTrue(clusterRule.Status.Conditions, ConditionTypeStreamsRunning) { - return ctrl.Result{RequeueAfter: RequeueStreamSettleInterval}, nil - } - return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil -} - -// setCondition sets or updates the Ready condition. -func (r *ClusterWatchRuleReconciler) setCondition( - clusterRule *configbutleraiv1alpha3.ClusterWatchRule, - status metav1.ConditionStatus, + st *reconcileStatus, reason, message string, -) { - r.setTypedCondition(clusterRule, ConditionTypeReady, status, reason, message) +) (ctrl.Result, error) { + rd := newRuleReadiness("ClusterWatchRule", "") + rd.stalled(reason, message) + return r.commitRule(ctx, st, rd) } -func (r *ClusterWatchRuleReconciler) setGitTargetNotFound( - clusterRule *configbutleraiv1alpha3.ClusterWatchRule, - targetNS string, - err error, -) { - r.setCondition( - clusterRule, - metav1.ConditionFalse, - ClusterWatchRuleReasonGitTargetNotFound, - fmt.Sprintf("Referenced GitTarget '%s/%s' not found: %v", targetNS, clusterRule.Spec.TargetRef.Name, err), - ) - r.setTypedCondition( - clusterRule, - ConditionTypeGitTargetReady, - metav1.ConditionFalse, - ClusterWatchRuleReasonGitTargetNotFound, - "Referenced GitTarget not found", - ) - r.setRuleStalled(clusterRule, ClusterWatchRuleReasonGitTargetNotFound, "Referenced GitTarget not found") +// commitRule writes the trio, persists the status, and picks the requeue cadence from the same +// verdict — so the cadence can never disagree with what status says. +func (r *ClusterWatchRuleReconciler) commitRule( + ctx context.Context, + st *reconcileStatus, + rd *readiness, +) (ctrl.Result, error) { + st.applyReadiness(rd) + if err := st.commit(ctx); err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{RequeueAfter: requeueFor(rd)}, nil } func (r *ClusterWatchRuleReconciler) setGitTargetReadyCondition( - clusterRule *configbutleraiv1alpha3.ClusterWatchRule, + st *reconcileStatus, target configbutleraiv1alpha3.GitTarget, ) { ready := gitTargetReadyCondition(target) - r.setTypedCondition(clusterRule, ConditionTypeGitTargetReady, ready.Status, ready.Reason, ready.Message) + st.setValue(ConditionTypeGitTargetReady, ready) } func (r *ClusterWatchRuleReconciler) setResourceResolutionCondition( ctx context.Context, + st *reconcileStatus, clusterRule *configbutleraiv1alpha3.ClusterWatchRule, ) { resolved, message := r.WatchManager.ResolveClusterWatchRuleResources(ctx, *clusterRule) @@ -405,146 +365,27 @@ func (r *ClusterWatchRuleReconciler) setResourceResolutionCondition( status = metav1.ConditionTrue reason = ClusterWatchRuleReasonResourcesResolved } - r.setTypedCondition(clusterRule, ConditionTypeResourcesResolved, status, reason, message) + st.set(ConditionTypeResourcesResolved, status, reason, message) } func (r *ClusterWatchRuleReconciler) setStreamsReadyCondition( + st *reconcileStatus, clusterRule *configbutleraiv1alpha3.ClusterWatchRule, streams watch.StreamSummary, ) { clusterRule.Status.Streams = watchRuleStreamsStatus(streams) - r.setTypedCondition( - clusterRule, - ConditionTypeStreamsRunning, - streamConditionStatus(streams), - streams.Reason, - streams.Message, - ) -} - -func (r *ClusterWatchRuleReconciler) setRuleStalled( - clusterRule *configbutleraiv1alpha3.ClusterWatchRule, - reason string, - message string, -) { - r.setTypedCondition(clusterRule, ConditionTypeReady, metav1.ConditionFalse, reason, message) - r.setTypedCondition( - clusterRule, - ConditionTypeReconciling, - metav1.ConditionFalse, - reason, - "Reconciliation is stalled", - ) - r.setTypedCondition(clusterRule, ConditionTypeStalled, metav1.ConditionTrue, reason, message) -} - -func (r *ClusterWatchRuleReconciler) setRuleKstatus( - clusterRule *configbutleraiv1alpha3.ClusterWatchRule, - readyMessage string, -) { - applyRuleKstatus( - clusterRule.Status.Conditions, - readyMessage, - "ClusterWatchRule is not stalled", - func(conditionType string, status metav1.ConditionStatus, reason, message string) { - r.setTypedCondition(clusterRule, conditionType, status, reason, message) - }, - func(reason, message string) { - r.setRuleStalled(clusterRule, reason, message) - }, - ) -} - -func (r *ClusterWatchRuleReconciler) setTypedCondition( - clusterRule *configbutleraiv1alpha3.ClusterWatchRule, - conditionType string, - status metav1.ConditionStatus, - reason string, - message string, -) { - clusterRule.Status.Conditions = upsertCondition( - clusterRule.Status.Conditions, - conditionType, - status, - reason, - message, - clusterRule.Generation, - ) -} - -// updateStatusAndRequeue updates the status and returns requeue result. -func (r *ClusterWatchRuleReconciler) updateStatusAndRequeue( - ctx context.Context, - clusterRule *configbutleraiv1alpha3.ClusterWatchRule, -) (ctrl.Result, error) { - if err := r.updateStatusWithRetry(ctx, clusterRule); err != nil { - return ctrl.Result{}, err - } - return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil -} - -// updateStatusWithRetry updates the status with retry logic to handle race conditions. -// -//nolint:dupl // Similar retry logic pattern used across controllers -func (r *ClusterWatchRuleReconciler) updateStatusWithRetry( - ctx context.Context, - clusterRule *configbutleraiv1alpha3.ClusterWatchRule, -) error { - log := logf.FromContext(ctx).WithName("updateStatusWithRetry") - - log.Info("Starting status update with retry", - "name", clusterRule.Name, - "conditionsCount", len(clusterRule.Status.Conditions)) - - return wait.ExponentialBackoff(wait.Backoff{ - Duration: RetryInitialDuration, - Factor: RetryBackoffFactor, - Jitter: RetryBackoffJitter, - Steps: RetryMaxSteps, - }, func() (bool, error) { - log.Info("Attempting status update") - - // Get the latest version of the resource - latest := &configbutleraiv1alpha3.ClusterWatchRule{} - key := client.ObjectKeyFromObject(clusterRule) - if err := r.Get(ctx, key, latest); err != nil { - if k8serrors.IsNotFound(err) { - log.Info("Resource was deleted, nothing to update") - return true, nil - } - log.Error(err, "Failed to get latest resource version") - return false, err - } - - log.Info("Got latest resource version", - "generation", latest.Generation, - "resourceVersion", latest.ResourceVersion) - - // Copy our status to the latest version - latest.Status = clusterRule.Status - - log.Info("Attempting to update status", - "conditionsCount", len(latest.Status.Conditions)) - - // Attempt to update - if err := r.Status().Update(ctx, latest); err != nil { - if k8serrors.IsConflict(err) { - log.Info("Resource version conflict, retrying") - return false, nil - } - log.Error(err, "Failed to update status") - return false, err - } - - log.Info("Status update successful") - return true, nil - }) + st.set(ConditionTypeStreamsRunning, streamConditionStatus(streams), streams.Reason, streams.Message) } // SetupWithManager sets up the controller with the Manager. func (r *ClusterWatchRuleReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). - For(&configbutleraiv1alpha3.ClusterWatchRule{}). + // A For() predicate is not an optimisation here, it closes a self-triggering edge: a status + // write bumps resourceVersion and fires an Update watch event that EnqueueRequestForObject + // turns straight back into a queued request, un-rate-limited. reconcileStatus.commit() + // already suppresses no-op writes, so the loop has no fuel; this makes it structural, and + // matches what GitProvider and ClusterProvider already do. + For(&configbutleraiv1alpha3.ClusterWatchRule{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})). // GenerationChangedPredicate keeps these watches reacting to a freshly // applied or spec-changed dependency while ignoring the status-only // updates the controllers write themselves — without it every GitTarget diff --git a/internal/controller/commitrequest_controller.go b/internal/controller/commitrequest_controller.go index 37a643da..d46f6986 100644 --- a/internal/controller/commitrequest_controller.go +++ b/internal/controller/commitrequest_controller.go @@ -134,7 +134,7 @@ func (r *CommitRequestReconciler) Reconcile(ctx context.Context, req ctrl.Reques // First sight: stamp the still-running conditions so the object reports its // progress (kstatus InProgress) and AuthorAttributed is settled immediately. A // disabled controller returns above, so it never stamps. - if conditionByType(commitRequest.Status.Conditions, ConditionTypeReady) == nil { + if findCondition(commitRequest.Status.Conditions, ConditionTypeReady) == nil { markCommitRequestWaitingForCloseDelay(commitRequest, attribution) if err := r.Status().Update(ctx, commitRequest); err != nil { return ctrl.Result{}, err @@ -223,7 +223,7 @@ func (r *CommitRequestReconciler) recordCloseDelayWait( commitRequest *configbutleraiv1alpha3.CommitRequest, attribution commitRequestAttribution, ) error { - if c := conditionByType(commitRequest.Status.Conditions, ConditionTypeReconciling); c != nil && + if c := findCondition(commitRequest.Status.Conditions, ConditionTypeReconciling); c != nil && c.Reason == crReasonWaitingForCloseDelay { return nil } diff --git a/internal/controller/condition_helper.go b/internal/controller/condition_helper.go index 6d9a3025..e523eb31 100644 --- a/internal/controller/condition_helper.go +++ b/internal/controller/condition_helper.go @@ -3,9 +3,18 @@ package controller import ( + apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +// upsertCondition sets one condition by type, preserving LastTransitionTime while Status is +// unchanged. +// +// It delegates to apimachinery's own setter rather than rebuilding the slice. The hand-rolled +// version filtered the target type out and re-appended it at the tail, so touching Ready migrated +// it to the end and every other condition shuffled up — a diff on every reconcile in +// `kubectl get -o yaml`, and, for a cluster whose own config objects are mirrored into Git by this +// operator, a stream of commits that reorder conditions and change nothing. func upsertCondition( conditions []metav1.Condition, conditionType string, @@ -13,47 +22,25 @@ func upsertCondition( reason, message string, observedGeneration int64, ) []metav1.Condition { - now := metav1.Now() - next := metav1.Condition{ + apimeta.SetStatusCondition(&conditions, metav1.Condition{ Type: conditionType, Status: status, Reason: reason, Message: message, ObservedGeneration: observedGeneration, - LastTransitionTime: now, - } - - var existing *metav1.Condition - result := make([]metav1.Condition, 0, len(conditions)) - for i := range conditions { - cond := conditions[i] - if cond.Type == conditionType { - if existing == nil { - existing = &cond - } - continue - } - result = append(result, cond) - } - - if existing != nil && existing.Status == next.Status && !existing.LastTransitionTime.IsZero() { - next.LastTransitionTime = existing.LastTransitionTime - } - - result = append(result, next) - return result + }) + return conditions } -func conditionByType(conditions []metav1.Condition, conditionType string) *metav1.Condition { - for i := range conditions { - if conditions[i].Type == conditionType { - return &conditions[i] - } - } - return nil +// findCondition returns the named condition, or nil. +func findCondition(conditions []metav1.Condition, conditionType string) *metav1.Condition { + return apimeta.FindStatusCondition(conditions, conditionType) } func conditionIsTrue(conditions []metav1.Condition, conditionType string) bool { - condition := conditionByType(conditions, conditionType) - return condition != nil && condition.Status == metav1.ConditionTrue + return apimeta.IsStatusConditionTrue(conditions, conditionType) +} + +func conditionIsFalse(conditions []metav1.Condition, conditionType string) bool { + return apimeta.IsStatusConditionFalse(conditions, conditionType) } diff --git a/internal/controller/condition_helper_test.go b/internal/controller/condition_helper_test.go index 15ea3c7e..e6800eae 100644 --- a/internal/controller/condition_helper_test.go +++ b/internal/controller/condition_helper_test.go @@ -10,7 +10,12 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func TestUpsertCondition_DeduplicatesAndUpdatesFields(t *testing.T) { +// TestUpsertCondition_UpdatesInPlaceAndKeepsOrder pins the property the hand-rolled setter did not +// have: a touched condition keeps its POSITION. The old implementation filtered the target type out +// and re-appended it, so every reconcile that touched Ready shuffled the list — a diff in +// `kubectl get -o yaml` and, for a cluster mirroring its own config objects into Git, a stream of +// commits that reorder conditions and change nothing. +func TestUpsertCondition_UpdatesInPlaceAndKeepsOrder(t *testing.T) { oldTransition := metav1.NewTime(time.Now().Add(-5 * time.Minute)) conditions := []metav1.Condition{ { @@ -22,10 +27,10 @@ func TestUpsertCondition_DeduplicatesAndUpdatesFields(t *testing.T) { LastTransitionTime: oldTransition, }, { - Type: GitTargetConditionReady, + Type: GitTargetConditionStreamsRunning, Status: metav1.ConditionTrue, - Reason: "Duplicate", - Message: "Duplicate", + Reason: "AllStreamsReady", + Message: "1/1 streams running", ObservedGeneration: 1, LastTransitionTime: oldTransition, }, @@ -40,8 +45,9 @@ func TestUpsertCondition_DeduplicatesAndUpdatesFields(t *testing.T) { 9, ) - require.Len(t, conditions, 1) - require.Equal(t, GitTargetConditionReady, conditions[0].Type) + require.Len(t, conditions, 2) + require.Equal(t, GitTargetConditionReady, conditions[0].Type, "the touched condition must not migrate") + require.Equal(t, GitTargetConditionStreamsRunning, conditions[1].Type) require.Equal(t, metav1.ConditionTrue, conditions[0].Status) require.Equal(t, GitTargetReasonOK, conditions[0].Reason) require.Equal(t, "Updated message", conditions[0].Message) diff --git a/internal/controller/constants.go b/internal/controller/constants.go index 48d29d4a..8e239dfa 100644 --- a/internal/controller/constants.go +++ b/internal/controller/constants.go @@ -7,6 +7,8 @@ import ( "context" "time" + fluxmeta "github.com/fluxcd/pkg/apis/meta" + "sigs.k8s.io/controller-runtime/pkg/event" configv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" @@ -125,14 +127,30 @@ const ( // RetryMaxSteps is the maximum number of retry attempts. RetryMaxSteps = 5 + // The generic condition reasons below are ALIASES of github.com/fluxcd/pkg/apis/meta, a module + // this project already depends on. Sharing the vocabulary is the point: one alerting rule + // written against reason=Failed or reason=Succeeded works across every kind here AND across + // every Flux kind in the same cluster, which is not true of a per-project spelling. Domain + // reasons (UnsupportedContent, WriteBoundaryRefused, NoAdmittedSourceNamespaces, ...) stay ours + // — those carry information a generic reason cannot, and declaring them is exactly what the + // upstream vocabulary asks projects to do. + + // ReasonSucceeded is the reason on a healthy, fully reconciled object. It replaces the former + // "OK"/"Ready" spellings, which restated the condition type instead of answering "why". + ReasonSucceeded = fluxmeta.SucceededReason + // ReasonFailed indicates an observed failure: declared state does not match actual state. + ReasonFailed = fluxmeta.FailedReason + // ReasonProgressing indicates that a stream or control-plane gate is still converging. + ReasonProgressing = fluxmeta.ProgressingReason + // ReasonDependencyNotReady indicates that a referenced object is not ready yet. + ReasonDependencyNotReady = fluxmeta.DependencyNotReadyReason + // ReasonChecking indicates that the controller is checking the resource status. ReasonChecking = "Checking" // ReasonReconciling indicates that reconciliation is still making progress. ReasonReconciling = "Reconciling" // ReasonStalled indicates that reconciliation is blocked until a human fixes the object or dependency. ReasonStalled = "Stalled" - // ReasonProgressing indicates that a stream or control-plane gate is still converging. - ReasonProgressing = "Progressing" // ReasonSecretNotFound indicates that the referenced secret was not found. ReasonSecretNotFound = "SecretNotFound" // ReasonSecretMalformed indicates that the referenced secret is invalid. diff --git a/internal/controller/gitprovider_controller.go b/internal/controller/gitprovider_controller.go index c5ca618e..792cb83d 100644 --- a/internal/controller/gitprovider_controller.go +++ b/internal/controller/gitprovider_controller.go @@ -10,11 +10,9 @@ import ( "sync" corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" - "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" @@ -35,6 +33,9 @@ type GitProviderReconciler struct { Scheme *runtime.Scheme firsts gitProviderLogFirsts + // Recorder emits a Kubernetes Event on every persisted Ready transition; nil disables Events. + Recorder record.EventRecorder + // SSHHostKeys configures SSH host-key resolution (install-level default ConfigMap and the // dev-only missing-key opt-out) for the connectivity check's credential read, so it matches // what the write path uses. @@ -55,6 +56,7 @@ type gitProviderLogFirsts struct { // +kubebuilder:rbac:groups=configbutler.ai,resources=gitproviders/status,verbs=get;update;patch // +kubebuilder:rbac:groups=configbutler.ai,resources=gitproviders/finalizers,verbs=update // +kubebuilder:rbac:groups="",resources=secrets,verbs=get;create;update +// +kubebuilder:rbac:groups="",resources=events,verbs=create;patch // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state. @@ -77,6 +79,9 @@ func (r *GitProviderReconciler) Reconcile(ctx context.Context, req ctrl.Request) } // reconcileGitProvider performs the main reconciliation logic. +// +// Every gate below contributes to ONE readiness accumulator and every exit goes through +// commitProvider, so the Ready/Reconciling/Stalled trio is written exactly once per reconcile. func (r *GitProviderReconciler) reconcileGitProvider( ctx context.Context, log logr.Logger, @@ -90,43 +95,88 @@ func (r *GitProviderReconciler) reconcileGitProvider( "generation", gitProvider.Generation, "resourceVersion", gitProvider.ResourceVersion) - r.setProgressingConditions(gitProvider, ReasonChecking, "Validating repository connectivity...") + st := beginStatus(r.Client, r.Recorder, gitProvider, &gitProvider.Status.Conditions) + gitProvider.Status.ObservedGeneration = gitProvider.Generation + rd := newReadiness( + fmt.Sprintf("Repository connectivity validated for %s", gitProvider.Spec.URL), + "GitProvider is not stalled", + ) + if err := r.validateCommitConfiguration(gitProvider); err != nil { - r.setStalledConditions(gitProvider, ReasonCommitConfigInvalid, err.Error()) - result, _ := r.updateStatusAndRequeue(ctx, gitProvider) - return result, nil + rd.stalled(ReasonCommitConfigInvalid, err.Error()) + return r.commitProvider(ctx, st, rd) } if err := r.ensureSigningKey(ctx, gitProvider); err != nil { - reason := ReasonSecretMalformed - if strings.Contains(err.Error(), "secretRef.name") { - reason = ReasonCommitConfigInvalid - } - if strings.Contains(err.Error(), "not found") || - strings.Contains(err.Error(), "generateWhenMissing is disabled") { - reason = ReasonSecretNotFound - } - - r.setStalledConditions(gitProvider, reason, err.Error()) - result, _ := r.updateStatusAndRequeue(ctx, gitProvider) - return result, nil + rd.stalled(signingKeyFailureReason(err), err.Error()) + return r.commitProvider(ctx, st, rd) } - // Fetch and validate secret - secret, shouldReturn := r.fetchAndValidateSecret(ctx, log, gitProvider) + secret, shouldReturn := r.fetchAndValidateSecret(ctx, log, rd, gitProvider) if shouldReturn { - result, _ := r.updateStatusAndRequeue(ctx, gitProvider) - return result, nil + return r.commitProvider(ctx, st, rd) } - // Extract credentials - auth, result, shouldReturn := r.getAuthFromSecret(ctx, log, gitProvider, secret) - if shouldReturn { - return result, nil + auth, err := r.extractCredentials(ctx, gitProvider, secret) + if err != nil { + log.Error(err, "Failed to extract credentials from secret") + rd.stalled(ReasonSecretMalformed, + fmt.Sprintf("Secret '%s' malformed: %v", gitProvider.Spec.SecretRef.Name, err)) + return r.commitProvider(ctx, st, rd) + } + // secret is nil on the anonymous-access path (no secretRef); in that case there is no secret to + // report and gitProvider.Spec.SecretRef is nil, so guard the dereference below. + if secret != nil { + r.firsts.credentialsLoaded.Do(func() { + log.Info("GitProvider credentials loaded from secret", + "secretName", gitProvider.Spec.SecretRef.Name, + "namespace", gitProvider.Namespace) + }) + } + + log.V(1).Info("Validating repository connectivity", "url", gitProvider.Spec.URL) + branchCount, err := r.checkRemoteConnectivity(ctx, gitProvider.Spec.URL, auth) + if err != nil { + log.Error(err, "Repository connectivity check failed", "url", gitProvider.Spec.URL) + rd.stalled(ReasonConnectionFailed, fmt.Sprintf("Failed to connect to repository: %v", err)) + return r.commitProvider(ctx, st, rd) } - // Validate repository connectivity - return r.validateAndUpdateStatus(ctx, log, gitProvider, auth) + r.firsts.validationSuccess.Do(func() { + log.Info("First GitProvider validation completed successfully", + "name", gitProvider.Name, + "namespace", gitProvider.Namespace, + "branchCount", branchCount) + }) + return r.commitProvider(ctx, st, rd) +} + +// commitProvider writes the trio and persists the status. Every outcome requeues on the steady +// interval: a GitProvider's failures are all "the remote or the credential changed", which nothing +// this controller does will fix sooner. +func (r *GitProviderReconciler) commitProvider( + ctx context.Context, + st *reconcileStatus, + rd *readiness, +) (ctrl.Result, error) { + st.applyReadiness(rd) + if err := st.commit(ctx); err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil +} + +// signingKeyFailureReason classifies an ensureSigningKey failure for the Stalled reason. +func signingKeyFailureReason(err error) string { + switch { + case strings.Contains(err.Error(), "not found"), + strings.Contains(err.Error(), "generateWhenMissing is disabled"): + return ReasonSecretNotFound + case strings.Contains(err.Error(), "secretRef.name"): + return ReasonCommitConfigInvalid + default: + return ReasonSecretMalformed + } } // fetchAndValidateSecret fetches the secret if specified. @@ -134,6 +184,7 @@ func (r *GitProviderReconciler) reconcileGitProvider( func (r *GitProviderReconciler) fetchAndValidateSecret( ctx context.Context, log logr.Logger, + rd *readiness, gitProvider *configbutleraiv1alpha3.GitProvider, ) (*corev1.Secret, bool) { if gitProvider.Spec.SecretRef == nil { @@ -157,16 +208,9 @@ func (r *GitProviderReconciler) fetchAndValidateSecret( log.Error(err, "Failed to fetch secret", "secretName", gitProvider.Spec.SecretRef.Name, "namespace", gitProvider.Namespace) - r.setStalledConditions( - gitProvider, - ReasonSecretNotFound, - fmt.Sprintf( - "Secret '%s' not found in namespace '%s': %v", - gitProvider.Spec.SecretRef.Name, - gitProvider.Namespace, - err, - ), - ) + rd.stalled(ReasonSecretNotFound, fmt.Sprintf( + "Secret '%s' not found in namespace '%s': %v", + gitProvider.Spec.SecretRef.Name, gitProvider.Namespace, err)) return nil, true } @@ -174,81 +218,6 @@ func (r *GitProviderReconciler) fetchAndValidateSecret( return secret, false } -// getAuthFromSecret extracts authentication from the secret. -// Returns (auth, result, shouldReturn). If shouldReturn is true, caller should return the result immediately. -func (r *GitProviderReconciler) getAuthFromSecret( - ctx context.Context, - log logr.Logger, - gitProvider *configbutleraiv1alpha3.GitProvider, - secret *corev1.Secret, -) (transport.AuthMethod, ctrl.Result, bool) { - log.V(1).Info("Extracting credentials from secret") - auth, err := r.extractCredentials(ctx, gitProvider, secret) - if err != nil { - log.Error(err, "Failed to extract credentials from secret") - secretName := gitProvider.Spec.SecretRef.Name - r.setStalledConditions(gitProvider, ReasonSecretMalformed, - fmt.Sprintf("Secret '%s' malformed: %v", secretName, err)) - result, _ := r.updateStatusAndRequeue(ctx, gitProvider) - return nil, result, true - } - - log.V(1).Info("Successfully extracted credentials", "hasAuth", auth != nil) - // secret is nil on the anonymous-access path (no secretRef); in that case - // there is no secret to report and gitProvider.Spec.SecretRef is nil, so - // guard the dereference below. - if secret != nil { - r.firsts.credentialsLoaded.Do(func() { - log.Info("GitProvider credentials loaded from secret", - "secretName", gitProvider.Spec.SecretRef.Name, - "namespace", gitProvider.Namespace) - }) - } - return auth, ctrl.Result{}, false -} - -// validateAndUpdateStatus validates repository connectivity and updates the status. -func (r *GitProviderReconciler) validateAndUpdateStatus( - ctx context.Context, - log logr.Logger, - gitProvider *configbutleraiv1alpha3.GitProvider, - auth transport.AuthMethod, -) (ctrl.Result, error) { - log.V(1).Info("Validating repository connectivity", - "url", gitProvider.Spec.URL) - - // Check repository connectivity and get branch count - branchCount, err := r.checkRemoteConnectivity(ctx, gitProvider.Spec.URL, auth) - if err != nil { - log.Error(err, "Repository connectivity check failed", - "url", gitProvider.Spec.URL) - r.setStalledConditions(gitProvider, ReasonConnectionFailed, - fmt.Sprintf("Failed to connect to repository: %v", err)) - return r.updateStatusAndRequeue(ctx, gitProvider) - } - - log.V(1).Info("Repository connectivity validated successfully", "branchCount", branchCount) - message := fmt.Sprintf("Repository connectivity validated for %s", gitProvider.Spec.URL) - r.setReadyConditions(gitProvider, message) - - log.V(1).Info("GitProvider validation successful", "name", gitProvider.Name) - log.V(1).Info("Updating status with success condition") - - if err := r.updateStatusWithRetry(ctx, gitProvider); err != nil { - log.Error(err, "Failed to update GitProvider status") - return ctrl.Result{}, err - } - - r.firsts.validationSuccess.Do(func() { - log.Info("First GitProvider validation completed successfully", - "name", gitProvider.Name, - "namespace", gitProvider.Namespace, - "branchCount", branchCount) - }) - log.V(1).Info("Status update completed successfully, scheduling requeue", "requeueAfter", RequeueSteadyInterval) - return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil -} - // fetchSecret retrieves the secret containing Git credentials. func (r *GitProviderReconciler) fetchSecret( ctx context.Context, secretName, secretNamespace string) (*corev1.Secret, error) { @@ -318,144 +287,6 @@ func (r *GitProviderReconciler) validateCommitConfiguration( return nil } -func (r *GitProviderReconciler) setReadyConditions( - gitProvider *configbutleraiv1alpha3.GitProvider, - message string, -) { - r.setCondition(gitProvider, ConditionTypeReady, metav1.ConditionTrue, ConditionTypeReady, message) - r.setCondition( - gitProvider, - ConditionTypeReconciling, - metav1.ConditionFalse, - ConditionTypeReady, - "Reconciliation complete", - ) - r.setCondition( - gitProvider, - ConditionTypeStalled, - metav1.ConditionFalse, - ConditionTypeReady, - "GitProvider is not stalled", - ) -} - -func (r *GitProviderReconciler) setProgressingConditions( - gitProvider *configbutleraiv1alpha3.GitProvider, - reason string, - message string, -) { - r.setCondition(gitProvider, ConditionTypeReady, metav1.ConditionFalse, reason, message) - r.setCondition(gitProvider, ConditionTypeReconciling, metav1.ConditionTrue, reason, message) - r.setCondition( - gitProvider, - ConditionTypeStalled, - metav1.ConditionFalse, - reason, - "Reconciliation is making progress", - ) -} - -func (r *GitProviderReconciler) setStalledConditions( - gitProvider *configbutleraiv1alpha3.GitProvider, - reason string, - message string, -) { - r.setCondition(gitProvider, ConditionTypeReady, metav1.ConditionFalse, reason, message) - r.setCondition(gitProvider, ConditionTypeReconciling, metav1.ConditionFalse, reason, "Reconciliation is stalled") - r.setCondition(gitProvider, ConditionTypeStalled, metav1.ConditionTrue, reason, message) -} - -// setCondition sets or updates one condition by type. -func (r *GitProviderReconciler) setCondition( - gitProvider *configbutleraiv1alpha3.GitProvider, - conditionType string, - status metav1.ConditionStatus, - reason, - message string, -) { - gitProvider.Status.ObservedGeneration = gitProvider.Generation - gitProvider.Status.Conditions = upsertCondition( - gitProvider.Status.Conditions, - conditionType, - status, - reason, - message, - gitProvider.Generation, - ) -} - -// updateStatusAndRequeue updates the status and requeues on the unified control-plane steady -// interval. The control plane no longer watches Secrets, so every status outcome falls back to -// this single cadence; see docs/rbac.md. -func (r *GitProviderReconciler) updateStatusAndRequeue( - ctx context.Context, - gitProvider *configbutleraiv1alpha3.GitProvider, -) (ctrl.Result, error) { - if err := r.updateStatusWithRetry(ctx, gitProvider); err != nil { - return ctrl.Result{}, err - } - return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil -} - -// updateStatusWithRetry updates the status with retry logic to handle race conditions. -// - -func (r *GitProviderReconciler) updateStatusWithRetry( - ctx context.Context, - gitProvider *configbutleraiv1alpha3.GitProvider, -) error { - log := logf.FromContext(ctx).WithName("updateStatusWithRetry") - - log.V(1).Info("Starting status update with retry", - "name", gitProvider.Name, - "namespace", gitProvider.Namespace, - "conditionsCount", len(gitProvider.Status.Conditions)) - - return wait.ExponentialBackoff(wait.Backoff{ - Duration: RetryInitialDuration, - Factor: RetryBackoffFactor, - Jitter: RetryBackoffJitter, - Steps: RetryMaxSteps, - }, func() (bool, error) { - log.V(1).Info("Attempting status update") - - // Get the latest version of the resource - latest := &configbutleraiv1alpha3.GitProvider{} - key := client.ObjectKeyFromObject(gitProvider) - if err := r.Get(ctx, key, latest); err != nil { - if apierrors.IsNotFound(err) { - log.Info("Resource was deleted, nothing to update") - return true, nil - } - log.Error(err, "Failed to get latest resource version") - return false, err - } - - log.V(1).Info("Got latest resource version", - "generation", latest.Generation, - "resourceVersion", latest.ResourceVersion) - - // Copy our status to the latest version - latest.Status = gitProvider.Status - - log.V(1).Info("Attempting to update status", - "conditionsCount", len(latest.Status.Conditions)) - - // Attempt to update - if err := r.Status().Update(ctx, latest); err != nil { - if apierrors.IsConflict(err) { - log.V(1).Info("Resource version conflict, retrying") - return false, nil - } - log.Error(err, "Failed to update status") - return false, err - } - - log.V(1).Info("Status update successful") - return true, nil - }) -} - // SetupWithManager sets up the controller with the Manager. func (r *GitProviderReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). diff --git a/internal/controller/gitprovider_controller_test.go b/internal/controller/gitprovider_controller_test.go index 2e370900..f1e6f041 100644 --- a/internal/controller/gitprovider_controller_test.go +++ b/internal/controller/gitprovider_controller_test.go @@ -196,14 +196,10 @@ var _ = Describe("GitProvider Controller", func() { }) Context("Status Condition Management", func() { - var reconciler *GitProviderReconciler var gitProvider *configbutleraiv1alpha3.GitProvider + var st *reconcileStatus BeforeEach(func() { - reconciler = &GitProviderReconciler{ - Client: k8sClient, - Scheme: k8sClient.Scheme(), - } gitProvider = &configbutleraiv1alpha3.GitProvider{ ObjectMeta: metav1.ObjectMeta{ Name: "test-provider", @@ -213,16 +209,11 @@ var _ = Describe("GitProvider Controller", func() { Conditions: []metav1.Condition{}, }, } + st = beginStatus(k8sClient, nil, gitProvider, &gitProvider.Status.Conditions) }) It("should set initial checking condition", func() { - reconciler.setCondition( - gitProvider, - ConditionTypeReady, - metav1.ConditionFalse, - ReasonChecking, - "Validating...", - ) + st.set(ConditionTypeReady, metav1.ConditionFalse, ReasonChecking, "Validating...") Expect(gitProvider.Status.Conditions).To(HaveLen(1)) condition := gitProvider.Status.Conditions[0] @@ -232,23 +223,14 @@ var _ = Describe("GitProvider Controller", func() { Expect(condition.Message).To(Equal("Validating...")) }) - It("should update existing condition", func() { - // Set initial condition - reconciler.setCondition( - gitProvider, - ConditionTypeReady, - metav1.ConditionFalse, - ReasonChecking, - "Checking...", - ) - - // Update condition - reconciler.setCondition(gitProvider, ConditionTypeReady, metav1.ConditionTrue, "Ready", "Success!") + It("should update an existing condition in place", func() { + st.set(ConditionTypeReady, metav1.ConditionFalse, ReasonChecking, "Checking...") + st.set(ConditionTypeReady, metav1.ConditionTrue, ReasonSucceeded, "Success!") Expect(gitProvider.Status.Conditions).To(HaveLen(1)) condition := gitProvider.Status.Conditions[0] Expect(condition.Status).To(Equal(metav1.ConditionTrue)) - Expect(condition.Reason).To(Equal("Ready")) + Expect(condition.Reason).To(Equal(ReasonSucceeded)) Expect(condition.Message).To(Equal("Success!")) }) @@ -263,7 +245,7 @@ var _ = Describe("GitProvider Controller", func() { } for _, tc := range testCases { - reconciler.setCondition(gitProvider, ConditionTypeReady, metav1.ConditionFalse, tc.reason, tc.message) + st.set(ConditionTypeReady, metav1.ConditionFalse, tc.reason, tc.message) Expect(gitProvider.Status.Conditions).To(HaveLen(1)) condition := gitProvider.Status.Conditions[0] @@ -351,15 +333,15 @@ var _ = Describe("GitProvider Controller", func() { Expect(err).NotTo(HaveOccurred()) Expect(updatedProvider.Status.Conditions).To(HaveLen(3)) - condition := conditionByType(updatedProvider.Status.Conditions, ConditionTypeReady) + condition := findCondition(updatedProvider.Status.Conditions, ConditionTypeReady) Expect(condition).NotTo(BeNil()) Expect(condition.Type).To(Equal("Ready")) Expect(condition.Status).To(Equal(metav1.ConditionFalse)) Expect(condition.Reason).To(Equal(ReasonSecretNotFound)) Expect(condition.Message).To(ContainSubstring("Secret 'nonexistent-secret' not found")) - Expect(conditionByType(updatedProvider.Status.Conditions, ConditionTypeReconciling).Status). + Expect(findCondition(updatedProvider.Status.Conditions, ConditionTypeReconciling).Status). To(Equal(metav1.ConditionFalse)) - Expect(conditionByType(updatedProvider.Status.Conditions, ConditionTypeStalled).Status). + Expect(findCondition(updatedProvider.Status.Conditions, ConditionTypeStalled).Status). To(Equal(metav1.ConditionTrue)) }) @@ -414,12 +396,12 @@ var _ = Describe("GitProvider Controller", func() { Expect(err).NotTo(HaveOccurred()) Expect(updatedProvider.Status.Conditions).To(HaveLen(3)) - condition := conditionByType(updatedProvider.Status.Conditions, ConditionTypeReady) + condition := findCondition(updatedProvider.Status.Conditions, ConditionTypeReady) Expect(condition).NotTo(BeNil()) Expect(condition.Status).To(Equal(metav1.ConditionFalse)) Expect(condition.Reason).To(Equal(ReasonSecretMalformed)) Expect(condition.Message).To(ContainSubstring("Secret 'malformed-secret' malformed")) - Expect(conditionByType(updatedProvider.Status.Conditions, ConditionTypeStalled).Status). + Expect(findCondition(updatedProvider.Status.Conditions, ConditionTypeStalled).Status). To(Equal(metav1.ConditionTrue)) }) @@ -473,12 +455,12 @@ var _ = Describe("GitProvider Controller", func() { Expect(err).NotTo(HaveOccurred()) Expect(updatedProvider.Status.Conditions).To(HaveLen(3)) - condition := conditionByType(updatedProvider.Status.Conditions, ConditionTypeReady) + condition := findCondition(updatedProvider.Status.Conditions, ConditionTypeReady) Expect(condition).NotTo(BeNil()) Expect(condition.Status).To(Equal(metav1.ConditionFalse)) Expect(condition.Reason).To(Equal(ReasonCommitConfigInvalid)) Expect(condition.Message).To(ContainSubstring("invalid commit configuration")) - Expect(conditionByType(updatedProvider.Status.Conditions, ConditionTypeStalled).Status). + Expect(findCondition(updatedProvider.Status.Conditions, ConditionTypeStalled).Status). To(Equal(metav1.ConditionTrue)) Expect(updatedProvider.Status.SigningPublicKey).To(BeEmpty()) }) @@ -522,12 +504,12 @@ var _ = Describe("GitProvider Controller", func() { Expect(err).NotTo(HaveOccurred()) Expect(updatedProvider.Status.Conditions).To(HaveLen(3)) - condition := conditionByType(updatedProvider.Status.Conditions, ConditionTypeReady) + condition := findCondition(updatedProvider.Status.Conditions, ConditionTypeReady) Expect(condition).NotTo(BeNil()) Expect(condition.Status).To(Equal(metav1.ConditionFalse)) Expect(condition.Reason).To(Equal(ReasonSecretNotFound)) Expect(condition.Message).To(ContainSubstring("signing secret")) - Expect(conditionByType(updatedProvider.Status.Conditions, ConditionTypeStalled).Status). + Expect(findCondition(updatedProvider.Status.Conditions, ConditionTypeStalled).Status). To(Equal(metav1.ConditionTrue)) Expect(updatedProvider.Status.SigningPublicKey).To(BeEmpty()) }) diff --git a/internal/controller/gittarget_controller.go b/internal/controller/gittarget_controller.go index 3596ad91..878e3137 100644 --- a/internal/controller/gittarget_controller.go +++ b/internal/controller/gittarget_controller.go @@ -17,7 +17,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" k8stypes "k8s.io/apimachinery/pkg/types" - "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" @@ -55,7 +55,9 @@ const GitTargetConditionStreamsReady = GitTargetConditionStreamsRunning const GitTargetStreamsReadyReasonNotReady = GitTargetStreamsRunningReasonNotReady const ( - GitTargetReasonOK = "OK" + // GitTargetReasonOK is the healthy reason. It is the shared Succeeded vocabulary rather than + // a per-kind spelling; the name is kept for call-site stability. + GitTargetReasonOK = ReasonSucceeded GitTargetReasonProviderNotFound = "ProviderNotFound" GitTargetReasonBranchNotAllowed = "BranchNotAllowed" GitTargetReasonTargetConflict = "TargetConflict" @@ -107,16 +109,24 @@ type GitTargetReconciler struct { Scheme *runtime.Scheme WorkerManager *git.WorkerManager EventRouter *watch.EventRouter + // Recorder emits a Kubernetes Event on every persisted Ready transition. It may be nil in + // tests, in which case no Event is recorded and nothing else changes. + Recorder record.EventRecorder } // +kubebuilder:rbac:groups=configbutler.ai,resources=gittargets,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=configbutler.ai,resources=gittargets/status,verbs=get;update;patch // +kubebuilder:rbac:groups=configbutler.ai,resources=gitproviders,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=secrets,verbs=get;create;update +// +kubebuilder:rbac:groups="",resources=events,verbs=create;patch + +// newGitTargetReadiness starts the accumulator that owns this kind's kstatus trio. Its converged +// outcome is what Ready reports when no gate objected. +func newGitTargetReadiness() *readiness { + return newReadiness("GitTarget is fully reconciled", "GitTarget is not stalled") +} // Reconcile validates GitTarget references and drives startup lifecycle gates. -// -//nolint:gocognit,cyclop,funlen // Gate pipeline is intentionally explicit to keep status transitions obvious. func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { log := logf.FromContext(ctx).WithName("GitTargetReconciler") @@ -125,12 +135,12 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return r.handleFetchError(err, log, req.NamespacedName) } + st := beginStatus(r.Client, r.Recorder, &target, &target.Status.Conditions) target.Status.ObservedGeneration = target.Generation - target.Status.LastReconcileTime = metav1.Now() gitPathWasRefused := conditionIsFalse(target.Status.Conditions, GitTargetConditionGitPathAccepted) providerNS := target.Namespace - validated, validationMsg, validationResult, validationErr := r.evaluateValidatedGate(ctx, &target, providerNS) + validated, validationMsg, validationResult, validationErr := r.evaluateValidatedGate(ctx, st, &target, providerNS) if validationErr != nil { return ctrl.Result{}, validationErr } @@ -142,45 +152,28 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // releases the source-cluster context; a later recovery re-declares and re-snapshots. A // local GitTarget names no source cluster, so its streams are left untouched. r.stopSourceClusterMirror(&target) - r.setCondition( - &target, + st.set( GitTargetConditionEncryptionConfigured, metav1.ConditionUnknown, GitTargetReasonBlocked, "Blocked by Validated=False", ) - r.setBlockedDataPlane(&target) - r.setGitPathAcceptedUnknown(&target, "Blocked by Validated=False") - r.setStalledConditions( - &target, - GitTargetReadyReasonValidationFailed, - validationMsg, - ) - if validationResult != nil { - if err := r.updateStatusWithRetry(ctx, &target); err != nil { - return ctrl.Result{}, err - } - return *validationResult, nil - } - if err := r.updateStatusWithRetry(ctx, &target); err != nil { - return ctrl.Result{}, err - } - return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil + return r.stall(ctx, st, blockedGate{ + reason: GitTargetReadyReasonValidationFailed, + message: validationMsg, + blocked: "Blocked by Validated=False", + result: validationResult, + }) } - encryptionReady, encryptionMessage, encryptionRequeueAfter := r.evaluateEncryptionGate(ctx, &target, log) + encryptionReady, encryptionMessage, encryptionRequeueAfter := r.evaluateEncryptionGate(ctx, st, &target, log) if !encryptionReady { - r.setBlockedDataPlane(&target) - r.setGitPathAcceptedUnknown(&target, "Blocked by EncryptionConfigured=False") - r.setStalledConditions( - &target, - GitTargetReadyReasonEncryptionNotConfigured, - encryptionMessage, - ) - if err := r.updateStatusWithRetry(ctx, &target); err != nil { - return ctrl.Result{}, err - } - return ctrl.Result{RequeueAfter: encryptionRequeueAfter}, nil + return r.stall(ctx, st, blockedGate{ + reason: GitTargetReadyReasonEncryptionNotConfigured, + message: encryptionMessage, + blocked: "Blocked by EncryptionConfigured=False", + result: &ctrl.Result{RequeueAfter: encryptionRequeueAfter}, + }) } // Ensure the branch worker exists and register the GitTarget's event stream before declaring @@ -188,96 +181,113 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // destination worker must already be wired. wired, wiringMessage := r.evaluateWorkerWiringGate(&target, providerNS, log) if !wired { - r.setBlockedDataPlane(&target) - r.setGitPathAcceptedUnknown(&target, "Blocked by worker wiring failure") - r.setStalledConditions( - &target, - GitTargetReadyReasonWorkerUnavailable, - wiringMessage, - ) - if err := r.updateStatusWithRetry(ctx, &target); err != nil { - return ctrl.Result{}, err - } - return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil + return r.stall(ctx, st, blockedGate{ + reason: GitTargetReadyReasonWorkerUnavailable, + message: wiringMessage, + blocked: "Blocked by worker wiring failure", + }) } - // Ensure watch-first data-plane streams exist, then project source and target readiness - // into the kstatus trio. - streamsSettling := false - var streams watch.StreamSummary - gitPath := watch.GitPathAcceptanceStatus{ - Accepted: true, - Reason: GitTargetReasonGitPathAccepted, - Message: "GitTarget path accepted", - } - renderFidelity := watch.RenderFidelityStatus{ - State: "True", Reason: GitTargetReasonRenderMatchesLive, Message: "Every rendered token matches live", - } - if r.EventRouter != nil && r.EventRouter.WatchManager != nil { - gitDest := types.NewResourceReference(target.Name, target.Namespace).WithUID(string(target.UID)) - if declareErr := r.EventRouter.WatchManager.DeclareForGitTarget( - ctx, - gitDest, - target.SourceCluster(), - r.auditRouteFor(ctx, &target), - target.EffectivePruneMode(), - gitPathWasRefused, - ); declareErr != nil { - log.V(1).Info("stream declaration skipped; surface not observable", - "gitDest", gitDest.String(), "err", declareErr.Error()) - streamsSettling = true - } - streams = r.EventRouter.WatchManager.StreamSummaryForGitTarget(gitDest) - gitPath = r.EventRouter.WatchManager.GitPathAcceptanceForGitTarget(gitDest) - renderFidelity = r.EventRouter.WatchManager.RenderFidelityForGitTarget(gitDest) - target.Status.Streams = gitTargetStreamsStatus(streams) - // Retention is read beside the others and projected the same way, but it feeds NO - // condition: a document kept by policy is the configured outcome, not a degraded target. - target.Status.Retention = gitTargetRetentionStatus( - r.EventRouter.WatchManager.RetentionForGitTarget(gitDest)) - streamsSettling = streamsSettling || !streams.StreamsRunning() || !gitPath.Accepted || - renderFidelity.State == "Unknown" - } else { - streams = noResolvedStreamsSummary() - target.Status.Streams = gitTargetStreamsStatus(streams) - streamsSettling = true - } + observed := r.observeDataPlane(ctx, &target, gitPathWasRefused, log) + st.setValue(GitTargetConditionStreamsRunning, observed.axes.Streams) + st.setValue(GitTargetConditionGitPathAccepted, observed.axes.GitPath) + st.setValue(GitTargetConditionRenderMatchesLive, observed.axes.Render) - r.applyDataPlaneConditions(&target, streams, gitPath, renderFidelity) + // Source-cluster reachability (runtime), GitProvider readiness (destination-side) and + // ClusterProvider readiness (source-config side) are published as conditions of their own AND + // contributed to the trio below. Publishing is all this does: nothing here writes Ready. + sourceReach := r.observeSourceReachable(&target) + provider := r.gitProviderReadiness(ctx, &target, providerNS) + clusterProvider := r.clusterProviderReadiness(ctx, &target) + st.setValue(GitTargetConditionSourceClusterReachable, sourceReach) + st.setValue(GitTargetConditionGitProviderReady, provider) + st.setValue(GitTargetConditionClusterProviderReady, clusterProvider) - // Project source-cluster reachability (runtime) and GitProvider readiness (destination-side) - // and fold both into Ready. This runs AFTER applyDataPlaneConditions so it can only downgrade - // Ready — a source/provider problem holds the target below Ready, but a healthy pair never - // overrides a still-replaying stream. - // A GitTarget with no source cluster mirrors the cluster the operator runs in, which is - // reachable by definition — so the default (used when no watch manager is wired, e.g. in - // tests) is True/LocalCluster for a local target and Unknown only for a remote one. - sourceReach := watch.SourceClusterReachableStatus{State: "True", Reason: "LocalCluster"} - if !target.IsLocalSource() { - sourceReach = watch.SourceClusterReachableStatus{State: "Unknown", Reason: "AwaitingDiscovery"} + rd := newGitTargetReadiness() + gitTargetReadinessGates(rd, observed, provider, clusterProvider, sourceReach) + st.applyReadiness(rd) + + if err := st.commit(ctx); err != nil { + return ctrl.Result{}, err } - if r.EventRouter != nil && r.EventRouter.WatchManager != nil { - sourceReach = r.EventRouter.WatchManager.SourceClusterReachable(target.SourceCluster()) + return ctrl.Result{RequeueAfter: requeueFor(rd)}, nil +} + +// requeueFor picks the periodic cadence. Only a converged object earns the steady interval: a +// target that is still settling — or one that is stalled and waiting for the world to change under +// it — is re-checked on the fast loop so it converges (or recovers) promptly. +func requeueFor(rd *readiness) time.Duration { + if rd.converged() { + return RequeueSteadyInterval } - providerStatus, providerReason, providerMessage := r.gitProviderReadiness(ctx, &target, providerNS) - cpStatus, cpReason, cpMessage := r.clusterProviderReadiness(ctx, &target) - r.projectSourceAndProvider(&target, sourceReach, providerStatus, providerReason, providerMessage, - cpStatus, cpReason, cpMessage) - streamsSettling = streamsSettling || sourceReach.State != "True" || - providerStatus != metav1.ConditionTrue || cpStatus == metav1.ConditionFalse + return RequeueStreamSettleInterval +} + +// blockedGate describes a control-plane gate that failed before the data plane could be evaluated. +type blockedGate struct { + // reason and message are the terminal outcome published on the trio. + reason, message string + // blocked explains, on each data-plane condition, why it was not evaluated at all. + blocked string + // result overrides the requeue this gate would otherwise get. + result *ctrl.Result +} - if err := r.updateStatusWithRetry(ctx, &target); err != nil { +// stall publishes a terminal control-plane outcome and ends the reconcile. +// +// The four early-return gates used to repeat this block verbatim: mark the data plane +// not-evaluated, stamp the trio, write status with retry, choose a requeue. They differ only in the +// four values blockedGate carries. +func (r *GitTargetReconciler) stall( + ctx context.Context, + st *reconcileStatus, + gate blockedGate, +) (ctrl.Result, error) { + st.set(GitTargetConditionStreamsRunning, metav1.ConditionUnknown, + GitTargetStreamsRunningReasonNotReady, gate.blocked+"; streams not evaluated") + st.set(GitTargetConditionRenderMatchesLive, metav1.ConditionUnknown, + GitTargetReasonRenderRechecking, gate.blocked+"; render fidelity not evaluated") + st.set(GitTargetConditionGitPathAccepted, metav1.ConditionUnknown, + GitTargetReasonNotChecked, gate.blocked) + + rd := newGitTargetReadiness() + rd.stalled(gate.reason, gate.message) + st.applyReadiness(rd) + + if err := st.commit(ctx); err != nil { return ctrl.Result{}, err } - - if streamsSettling { - return ctrl.Result{RequeueAfter: RequeueStreamSettleInterval}, nil + if gate.result != nil { + return *gate.result, nil } return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil } +// observeSourceReachable projects the source cluster's runtime reachability. +// +// A GitTarget with no source cluster mirrors the cluster the operator runs in, which is reachable +// by definition — so the default (used when no watch manager is wired, e.g. in tests) is +// True/LocalCluster for a local target and Unknown only for a remote one. +func (r *GitTargetReconciler) observeSourceReachable( + target *configbutleraiv1alpha3.GitTarget, +) conditionValue { + reach := watch.SourceClusterReachableStatus{State: "True", Reason: "LocalCluster"} + if !target.IsLocalSource() { + reach = watch.SourceClusterReachableStatus{State: "Unknown", Reason: "AwaitingDiscovery"} + } + if r.EventRouter != nil && r.EventRouter.WatchManager != nil { + reach = r.EventRouter.WatchManager.SourceClusterReachable(target.SourceCluster()) + } + return conditionValue{ + Status: conditionStatusFromString(reach.State), + Reason: reach.Reason, + Message: reach.Message, + } +} + func (r *GitTargetReconciler) evaluateValidatedGate( ctx context.Context, + st *reconcileStatus, target *configbutleraiv1alpha3.GitTarget, providerNS string, ) (bool, string, *ctrl.Result, error) { @@ -286,7 +296,7 @@ func (r *GitTargetReconciler) evaluateValidatedGate( return false, "", nil, err } if !validated { - r.setCondition(target, GitTargetConditionValidated, metav1.ConditionFalse, reason, message) + st.set(GitTargetConditionValidated, metav1.ConditionFalse, reason, message) return false, fmt.Sprintf("Validated gate failed: %s", reason), result, nil } @@ -295,14 +305,12 @@ func (r *GitTargetReconciler) evaluateValidatedGate( return false, "", nil, conflictErr } if conflict { - r.setCondition(target, GitTargetConditionValidated, metav1.ConditionFalse, conflictReason, conflictMsg) + st.set(GitTargetConditionValidated, metav1.ConditionFalse, conflictReason, conflictMsg) return false, fmt.Sprintf("Validated gate failed: %s", conflictReason), &conflictResult, nil } if placementOK, placementMsg := validatePlacementPolicy(target.Spec.Placement); !placementOK { - r.setCondition( - target, - GitTargetConditionValidated, + st.set(GitTargetConditionValidated, metav1.ConditionFalse, GitTargetReasonInvalidConfig, placementMsg, @@ -323,14 +331,12 @@ func (r *GitTargetReconciler) evaluateValidatedGate( return false, "", nil, authErr } if !authorized { - r.setCondition(target, GitTargetConditionValidated, metav1.ConditionFalse, authReason, authMsg) + st.set(GitTargetConditionValidated, metav1.ConditionFalse, authReason, authMsg) result := ctrl.Result{RequeueAfter: RequeueSteadyInterval} return false, fmt.Sprintf("Validated gate failed: %s", authReason), &result, nil } - r.setCondition( - target, - GitTargetConditionValidated, + st.set(GitTargetConditionValidated, metav1.ConditionTrue, GitTargetReasonOK, "Provider, branch, and placement validation passed", @@ -340,13 +346,12 @@ func (r *GitTargetReconciler) evaluateValidatedGate( func (r *GitTargetReconciler) evaluateEncryptionGate( ctx context.Context, + st *reconcileStatus, target *configbutleraiv1alpha3.GitTarget, log logr.Logger, ) (bool, string, time.Duration) { if !isTargetAgeEncryptionEnabled(target) { - r.setCondition( - target, - GitTargetConditionEncryptionConfigured, + st.set(GitTargetConditionEncryptionConfigured, metav1.ConditionTrue, GitTargetReasonNotRequired, "SOPS age encryption is not enabled for this GitTarget", @@ -362,7 +367,7 @@ func (r *GitTargetReconciler) evaluateEncryptionGate( if strings.Contains(err.Error(), "failed to fetch encryption secret") { reason = GitTargetReasonMissingSecret } - r.setCondition(target, GitTargetConditionEncryptionConfigured, metav1.ConditionFalse, reason, err.Error()) + st.set(GitTargetConditionEncryptionConfigured, metav1.ConditionFalse, reason, err.Error()) return false, fmt.Sprintf("EncryptionConfigured gate failed: %s", reason), RequeueSteadyInterval } if _, err := git.ResolveTargetEncryption(ctx, r.Client, target); err != nil { @@ -370,13 +375,11 @@ func (r *GitTargetReconciler) evaluateEncryptionGate( if strings.Contains(err.Error(), "failed to fetch encryption secret") { reason = GitTargetReasonMissingSecret } - r.setCondition(target, GitTargetConditionEncryptionConfigured, metav1.ConditionFalse, reason, err.Error()) + st.set(GitTargetConditionEncryptionConfigured, metav1.ConditionFalse, reason, err.Error()) return false, fmt.Sprintf("EncryptionConfigured gate failed: %s", reason), RequeueSteadyInterval } - r.setCondition( - target, - GitTargetConditionEncryptionConfigured, + st.set(GitTargetConditionEncryptionConfigured, metav1.ConditionTrue, GitTargetReasonOK, "Encryption configuration is valid", @@ -423,244 +426,202 @@ func (r *GitTargetReconciler) stopSourceClusterMirror(target *configbutleraiv1al r.EventRouter.WatchManager.ForgetGitTargetDeclaration(gitDest) } -func (r *GitTargetReconciler) setBlockedDataPlane(target *configbutleraiv1alpha3.GitTarget) { - r.setCondition( - target, - GitTargetConditionStreamsRunning, - metav1.ConditionUnknown, - GitTargetStreamsRunningReasonNotReady, - "Blocked by control-plane gate; streams not evaluated", - ) - r.setCondition( - target, - GitTargetConditionRenderMatchesLive, - metav1.ConditionUnknown, - GitTargetReasonRenderRechecking, - "Blocked by control-plane gate; render fidelity not evaluated", - ) +// gitTargetAxes are the three data-plane observations a GitTarget publishes as conditions in their +// own right: what its watch streams are doing, whether its Git folder is safe to materialize, and +// whether what was rendered still matches live. +// +// They are OBSERVATIONS. None of them writes Ready, Reconciling or Stalled — that is +// gitTargetReadinessGates' job, and splitting the two is what stopped a later gate from silently +// overwriting an earlier gate's terminal verdict. +type gitTargetAxes struct { + Streams conditionValue + GitPath conditionValue + Render conditionValue } -func (r *GitTargetReconciler) setGitPathAcceptedUnknown( - target *configbutleraiv1alpha3.GitTarget, - message string, -) { - r.setCondition( - target, - GitTargetConditionGitPathAccepted, - metav1.ConditionUnknown, - GitTargetReasonNotChecked, - message, - ) +// dataPlaneObservation is everything one reconcile learned from the watch manager. +type dataPlaneObservation struct { + axes gitTargetAxes + streams watch.StreamSummary + // declareFailed records that the stream declaration did not land, so the axes describe a + // surface that is not yet observable rather than a converged one. + declareFailed bool } -func (r *GitTargetReconciler) setStalledConditions( +// observeDataPlane declares this GitTarget's streams, reads back what the watch manager knows, and +// projects the three data-plane axes. It mutates only the roll-up fields of status. +func (r *GitTargetReconciler) observeDataPlane( + ctx context.Context, target *configbutleraiv1alpha3.GitTarget, - reason, message string, -) { - r.setCondition(target, GitTargetConditionReady, metav1.ConditionFalse, reason, message) - r.setCondition(target, GitTargetConditionReconciling, metav1.ConditionFalse, reason, "Reconciliation is stalled") - r.setCondition(target, GitTargetConditionStalled, metav1.ConditionTrue, reason, message) -} + gitPathWasRefused bool, + log logr.Logger, +) dataPlaneObservation { + if r.EventRouter == nil || r.EventRouter.WatchManager == nil { + // No data plane is wired (tests, standalone). Nothing has been OBSERVED, which is not the + // same as "observed to be empty": the streams axis stays False and holds the target below + // Ready, and it says Progressing rather than NoResolvedTypes so the two are never confused. + streams := noResolvedStreamsSummary() + target.Status.Streams = gitTargetStreamsStatus(streams) + return dataPlaneObservation{ + axes: gitTargetAxes{ + Streams: conditionValue{ + Status: metav1.ConditionFalse, + Reason: ReasonProgressing, + Message: "Data plane is not wired; streams have not been evaluated", + }, + GitPath: gitPathAxis(watch.GitPathAcceptanceStatus{Accepted: true}), + Render: renderAxis(watch.RenderFidelityStatus{State: git.RenderFidelityTrue}), + }, + streams: streams, + } + } -func (r *GitTargetReconciler) applyDataPlaneConditions( - target *configbutleraiv1alpha3.GitTarget, - streams watch.StreamSummary, - gitPath watch.GitPathAcceptanceStatus, - renderFidelity watch.RenderFidelityStatus, -) { - d := deriveGitTargetDataPlaneStatusWithRenderFidelity(streams, gitPath, renderFidelity) - r.setCondition(target, GitTargetConditionStreamsRunning, d.StreamsStatus, d.StreamsReason, d.StreamsMessage) - r.setCondition(target, GitTargetConditionGitPathAccepted, d.GitPathStatus, d.GitPathReason, d.GitPathMessage) - r.setCondition(target, GitTargetConditionRenderMatchesLive, d.RenderFidelityStatus, - d.RenderFidelityReason, d.RenderFidelityMessage) - r.setCondition(target, GitTargetConditionReady, d.ReadyStatus, d.ReadyReason, d.ReadyMessage) - r.setCondition( - target, - GitTargetConditionReconciling, - d.ReconcilingStatus, - d.ReconcilingReason, - d.ReconcilingMessage, - ) - r.setCondition(target, GitTargetConditionStalled, d.StalledStatus, d.StalledReason, d.StalledMessage) + manager := r.EventRouter.WatchManager + gitDest := types.NewResourceReference(target.Name, target.Namespace).WithUID(string(target.UID)) + + observation := dataPlaneObservation{} + if declareErr := manager.DeclareForGitTarget( + ctx, + gitDest, + target.SourceCluster(), + r.auditRouteFor(ctx, target), + target.EffectivePruneMode(), + gitPathWasRefused, + ); declareErr != nil { + log.V(1).Info("stream declaration skipped; surface not observable", + "gitDest", gitDest.String(), "err", declareErr.Error()) + observation.declareFailed = true + } + + observation.streams = manager.StreamSummaryForGitTarget(gitDest) + observation.axes = gitTargetAxes{ + Streams: streamsAxis(observation.streams), + GitPath: gitPathAxis(manager.GitPathAcceptanceForGitTarget(gitDest)), + Render: renderAxis(manager.RenderFidelityForGitTarget(gitDest)), + } + + target.Status.Streams = gitTargetStreamsStatus(observation.streams) + // Retention is read beside the others and projected the same way, but it feeds NO condition: a + // document kept by policy is the configured outcome, not a degraded target. + target.Status.Retention = gitTargetRetentionStatus(manager.RetentionForGitTarget(gitDest)) + return observation } -type gitTargetDataPlaneDecision struct { - StreamsStatus metav1.ConditionStatus - StreamsReason string - StreamsMessage string - GitPathStatus metav1.ConditionStatus - GitPathReason string - GitPathMessage string - RenderFidelityStatus metav1.ConditionStatus - RenderFidelityReason string - RenderFidelityMessage string - ReadyStatus metav1.ConditionStatus - ReadyReason string - ReadyMessage string - ReconcilingStatus metav1.ConditionStatus - ReconcilingReason string - ReconcilingMessage string - StalledStatus metav1.ConditionStatus - StalledReason string - StalledMessage string +// streamsAxis renders StreamsRunning. +// +// Zero resolved types reports TRUE — vacuously, and deliberately. It used to report +// False/NoResolvedTypes, which pinned the GitTarget at Reconciling=True with nothing left that +// could ever resolve. That is not an exotic state: it is step 3 of the documented setup flow +// (create the GitTarget, THEN the WatchRules) and the steady state of any target whose rules were +// deleted. kstatus never reached Current, `kubectl wait --for=condition=Ready` never returned, and +// the reconcile burned a pass plus a status write every 10 seconds for the life of the object. +// "I have nothing to mirror" is converged — Flux's Kustomization with an empty path reports the +// same. The count stays visible: status.streams.summary still reads "0/0" and the condition's +// reason is still NoResolvedTypes, so the zero is legible without being reported as a failure. +func streamsAxis(streams watch.StreamSummary) conditionValue { + status := metav1.ConditionFalse + if streams.Ready == streams.Total { + status = metav1.ConditionTrue + } + return conditionValue{Status: status, Reason: streams.Reason, Message: streams.Message} } -func deriveGitTargetDataPlaneStatus( - streams watch.StreamSummary, - gitPath watch.GitPathAcceptanceStatus, -) gitTargetDataPlaneDecision { - streamsStatus := metav1.ConditionFalse - if streams.StreamsRunning() { - streamsStatus = metav1.ConditionTrue - } - gitPathStatus := metav1.ConditionTrue - gitPathReason := GitTargetReasonGitPathAccepted - gitPathMessage := "GitTarget path accepted" - if !gitPath.Accepted { - gitPathStatus = metav1.ConditionFalse - gitPathReason = gitPath.Reason - if gitPathReason == "" { - gitPathReason = GitTargetReasonUnsupportedContent +// gitPathAxis renders GitPathAccepted from the data plane's write-plan verdict. +func gitPathAxis(gitPath watch.GitPathAcceptanceStatus) conditionValue { + if gitPath.Accepted { + message := gitPath.Message + if message == "" { + message = "GitTarget path accepted" } - gitPathMessage = gitPath.Message - } - if gitPathMessage == "" { - gitPathMessage = "GitTarget path accepted" - } - - switch { - case !gitPath.Accepted: - return gitTargetDataPlaneDecision{ - StreamsStatus: streamsStatus, - StreamsReason: streams.Reason, - StreamsMessage: streams.Message, - GitPathStatus: gitPathStatus, - GitPathReason: gitPathReason, - GitPathMessage: gitPathMessage, - ReadyStatus: metav1.ConditionFalse, - ReadyReason: gitPathReason, - ReadyMessage: gitPathMessage, - ReconcilingStatus: metav1.ConditionFalse, - ReconcilingReason: gitPathReason, - ReconcilingMessage: "Reconciliation is stalled", - StalledStatus: metav1.ConditionTrue, - StalledReason: gitPathReason, - StalledMessage: gitPathMessage, - } - case streams.Blocked > 0: - return gitTargetDataPlaneDecision{ - StreamsStatus: metav1.ConditionFalse, - StreamsReason: streams.Reason, - StreamsMessage: streams.Message, - GitPathStatus: gitPathStatus, - GitPathReason: gitPathReason, - GitPathMessage: gitPathMessage, - ReadyStatus: metav1.ConditionFalse, - ReadyReason: streams.Reason, - ReadyMessage: streams.Message, - ReconcilingStatus: metav1.ConditionFalse, - ReconcilingReason: streams.Reason, - ReconcilingMessage: "Reconciliation is stalled", - StalledStatus: metav1.ConditionTrue, - StalledReason: streams.Reason, - StalledMessage: streams.Message, - } - case !streams.StreamsRunning(): - return gitTargetDataPlaneDecision{ - StreamsStatus: metav1.ConditionFalse, - StreamsReason: streams.Reason, - StreamsMessage: streams.Message, - GitPathStatus: gitPathStatus, - GitPathReason: gitPathReason, - GitPathMessage: gitPathMessage, - ReadyStatus: metav1.ConditionFalse, - ReadyReason: ReasonProgressing, - ReadyMessage: streams.Message, - ReconcilingStatus: metav1.ConditionTrue, - ReconcilingReason: streams.Reason, - ReconcilingMessage: streams.Message, - StalledStatus: metav1.ConditionFalse, - StalledReason: ReasonProgressing, - StalledMessage: "Reconciliation is making progress", + return conditionValue{ + Status: metav1.ConditionTrue, + Reason: GitTargetReasonGitPathAccepted, + Message: message, } } - return gitTargetDataPlaneDecision{ - StreamsStatus: metav1.ConditionTrue, - StreamsReason: streams.Reason, - StreamsMessage: streams.Message, - GitPathStatus: gitPathStatus, - GitPathReason: gitPathReason, - GitPathMessage: gitPathMessage, - ReadyStatus: metav1.ConditionTrue, - ReadyReason: GitTargetReasonOK, - ReadyMessage: "GitTarget is fully reconciled", - ReconcilingStatus: metav1.ConditionFalse, - ReconcilingReason: GitTargetReasonOK, - ReconcilingMessage: "Reconciliation complete", - StalledStatus: metav1.ConditionFalse, - StalledReason: GitTargetReasonOK, - StalledMessage: "GitTarget is not stalled", + + value := conditionValue{Status: metav1.ConditionFalse, Reason: gitPath.Reason, Message: gitPath.Message} + if value.Reason == "" { + value.Reason = GitTargetReasonUnsupportedContent + } + if value.Message == "" { + value.Message = "GitTarget path refused" } + return value } -// deriveGitTargetDataPlaneStatusWithRenderFidelity layers the independent render-vs-live gate -// over the existing source-stream and Git-path decisions. A divergence stalls the target without -// changing GitPathAccepted; an incomplete epoch is progress, not failure. -func deriveGitTargetDataPlaneStatusWithRenderFidelity( - streams watch.StreamSummary, - gitPath watch.GitPathAcceptanceStatus, - renderFidelity watch.RenderFidelityStatus, -) gitTargetDataPlaneDecision { - decision := deriveGitTargetDataPlaneStatus(streams, gitPath) - decision.RenderFidelityStatus = metav1.ConditionTrue - decision.RenderFidelityReason = GitTargetReasonRenderMatchesLive - decision.RenderFidelityMessage = "Every rendered token matches live" - +// renderAxis renders RenderMatchesLive. A divergence is terminal; an incomplete epoch is progress. +func renderAxis(renderFidelity watch.RenderFidelityStatus) conditionValue { + value := conditionValue{ + Status: metav1.ConditionTrue, + Reason: GitTargetReasonRenderMatchesLive, + Message: "Every rendered token matches live", + } switch renderFidelity.State { case git.RenderFidelityFalse: - decision.RenderFidelityStatus = metav1.ConditionFalse - decision.RenderFidelityReason = GitTargetReasonRenderDoesNotMatchLive - decision.RenderFidelityMessage = renderFidelity.Message + value = conditionValue{ + Status: metav1.ConditionFalse, + Reason: GitTargetReasonRenderDoesNotMatchLive, + Message: renderFidelity.Message, + } case git.RenderFidelityUnknown: - decision.RenderFidelityStatus = metav1.ConditionUnknown - decision.RenderFidelityReason = GitTargetReasonRenderRechecking - decision.RenderFidelityMessage = renderFidelity.Message + value = conditionValue{ + Status: metav1.ConditionUnknown, + Reason: GitTargetReasonRenderRechecking, + Message: renderFidelity.Message, + } case git.RenderFidelityTrue: } if renderFidelity.Reason != "" { - decision.RenderFidelityReason = renderFidelity.Reason + value.Reason = renderFidelity.Reason } - if decision.RenderFidelityMessage == "" { - decision.RenderFidelityMessage = "Waiting for render-vs-live verification" + if value.Message == "" { + value.Message = "Waiting for render-vs-live verification" } + return value +} - if !gitPath.Accepted || streams.Blocked > 0 || !streams.StreamsRunning() { - return decision - } - switch renderFidelity.State { - case git.RenderFidelityFalse: - decision.ReadyStatus = metav1.ConditionFalse - decision.ReadyReason = decision.RenderFidelityReason - decision.ReadyMessage = decision.RenderFidelityMessage - decision.ReconcilingStatus = metav1.ConditionFalse - decision.ReconcilingReason = decision.RenderFidelityReason - decision.ReconcilingMessage = "Reconciliation is stalled" - decision.StalledStatus = metav1.ConditionTrue - decision.StalledReason = decision.RenderFidelityReason - decision.StalledMessage = decision.RenderFidelityMessage - case git.RenderFidelityUnknown: - decision.ReadyStatus = metav1.ConditionFalse - decision.ReadyReason = decision.RenderFidelityReason - decision.ReadyMessage = decision.RenderFidelityMessage - decision.ReconcilingStatus = metav1.ConditionTrue - decision.ReconcilingReason = decision.RenderFidelityReason - decision.ReconcilingMessage = decision.RenderFidelityMessage - decision.StalledStatus = metav1.ConditionFalse - decision.StalledReason = ReasonProgressing - decision.StalledMessage = "Reconciliation is making progress" - case git.RenderFidelityTrue: - } - return decision +// gitTargetReadinessGates contributes every GitTarget gate to the accumulator that owns the trio. +// +// THIS FUNCTION IS THE PRECEDENCE. Read it top to bottom: a stall always beats a progressing gate +// whatever the order, and within each group the first contributor wins, so the order of these calls +// is the order in which competing explanations are preferred. Adding a gate means adding a line +// here, in the position where its answer should outrank the ones below it — not calling a setter +// from wherever the gate happens to be evaluated. +func gitTargetReadinessGates( + rd *readiness, + observed dataPlaneObservation, + provider, clusterProvider, sourceReach conditionValue, +) { + // Terminal, most specific first. Each of these needs a human: the folder holds content the + // operator will not manage, a watch is refused, or what was written no longer matches live. + rd.stalledIf(observed.axes.GitPath.Status == metav1.ConditionFalse, + observed.axes.GitPath.Reason, observed.axes.GitPath.Message) + rd.stalledIf(observed.streams.Blocked > 0, observed.streams.Reason, observed.streams.Message) + rd.stalledIf(observed.axes.Render.Status == metav1.ConditionFalse, + observed.axes.Render.Reason, observed.axes.Render.Message) + + // Transient. Each of these clears on its own and each has a Watches() edge that re-runs this + // reconcile when it does, so they are progress, never a stall. Dependencies before this + // object's own data plane: "your GitProvider is down" is a more actionable answer than + // "streams are still replaying", and it is usually the cause of the latter. + rd.progressingIf(provider.Status == metav1.ConditionFalse, metav1.ConditionFalse, + provider.Reason, provider.Message) + rd.progressingIf(clusterProvider.Status == metav1.ConditionFalse, metav1.ConditionFalse, + clusterProvider.Reason, clusterProvider.Message) + rd.progressingIf(sourceReach.Status == metav1.ConditionFalse, metav1.ConditionFalse, + sourceReach.Reason, sourceReach.Message) + // An unconfirmed source has not been established at all, so Ready is Unknown rather than False. + rd.progressingIf(sourceReach.Status == metav1.ConditionUnknown, metav1.ConditionUnknown, + sourceReach.Reason, sourceReach.Message) + rd.progressingIf(observed.axes.Render.Status == metav1.ConditionUnknown, metav1.ConditionFalse, + observed.axes.Render.Reason, observed.axes.Render.Message) + rd.progressingIf(observed.axes.Streams.Status != metav1.ConditionTrue, metav1.ConditionFalse, + observed.axes.Streams.Reason, observed.axes.Streams.Message) + // Last: the declaration itself did not land, so every axis above describes a surface that was + // never observed. Nothing else objected, but this target is not converged either. + rd.progressingIf(observed.declareFailed, metav1.ConditionFalse, ReasonProgressing, + "Stream declaration has not landed yet; the data-plane surface is not observable") } func (r *GitTargetReconciler) ensureEventStream( @@ -715,32 +676,6 @@ func (r *GitTargetReconciler) ensureEventStream( return stream, nil } -// isConditionTrue returns true if the named condition is present with Status=True. -func isConditionTrue(conditions []metav1.Condition, conditionType string) bool { - for _, c := range conditions { - if c.Type == conditionType { - return c.Status == metav1.ConditionTrue - } - } - return false -} - -func (r *GitTargetReconciler) setCondition( - target *configbutleraiv1alpha3.GitTarget, - conditionType string, - status metav1.ConditionStatus, - reason, message string, -) { - target.Status.Conditions = upsertCondition( - target.Status.Conditions, - conditionType, - status, - reason, - message, - target.Generation, - ) -} - func (r *GitTargetReconciler) validateProviderAndBranch( ctx context.Context, target *configbutleraiv1alpha3.GitTarget, @@ -1069,17 +1004,12 @@ func clampIntToInt32(value int) int32 { } func gitTargetStreamsStatus(streams watch.StreamSummary) *configbutleraiv1alpha3.GitTargetStreamsStatus { - observed := streams.ObservedTime - if observed.IsZero() { - observed = metav1.Now() - } return &configbutleraiv1alpha3.GitTargetStreamsStatus{ - Summary: streams.Summary(), - Total: clampIntToInt32(streams.Total), - Ready: clampIntToInt32(streams.Ready), - Replaying: clampIntToInt32(streams.Replaying), - Blocked: clampIntToInt32(streams.Blocked), - ObservedTime: &observed, + Summary: streams.Summary(), + Total: clampIntToInt32(streams.Total), + Ready: clampIntToInt32(streams.Ready), + Replaying: clampIntToInt32(streams.Replaying), + Blocked: clampIntToInt32(streams.Blocked), } } @@ -1105,45 +1035,15 @@ func gitTargetRetentionStatus(summary watch.RetentionSummary) *configbutleraiv1a } } -// updateStatusWithRetry updates the status with retry logic to handle race conditions. -func (r *GitTargetReconciler) updateStatusWithRetry( - ctx context.Context, - target *configbutleraiv1alpha3.GitTarget, -) error { - log := logf.FromContext(ctx).WithName("updateStatusWithRetry") - - return wait.ExponentialBackoff(wait.Backoff{ - Duration: RetryInitialDuration, - Factor: RetryBackoffFactor, - Jitter: RetryBackoffJitter, - Steps: RetryMaxSteps, - }, func() (bool, error) { - latest := &configbutleraiv1alpha3.GitTarget{} - key := client.ObjectKeyFromObject(target) - if err := r.Get(ctx, key, latest); err != nil { - if apierrors.IsNotFound(err) { - return true, nil - } - return false, err - } - - latest.Status = target.Status - if err := r.Status().Update(ctx, latest); err != nil { - if apierrors.IsConflict(err) { - log.V(1).Info("Status conflict, retrying") - return false, nil - } - return false, err - } - - return true, nil - }) -} - // SetupWithManager sets up the controller with the Manager. func (r *GitTargetReconciler) SetupWithManager(mgr ctrl.Manager) error { b := ctrl.NewControllerManagedBy(mgr). - For(&configbutleraiv1alpha3.GitTarget{}). + // A For() predicate is not an optimisation here, it closes a self-triggering edge: a status + // write bumps resourceVersion and fires an Update watch event that EnqueueRequestForObject + // turns straight back into a queued request, un-rate-limited. reconcileStatus.commit() + // already suppresses no-op writes, so the loop has no fuel; this makes it structural, and + // matches what GitProvider and ClusterProvider already do. + For(&configbutleraiv1alpha3.GitTarget{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})). // No control-plane Secret watch. Reacting to age-key Secret changes with a // full-object Secret watch made the process retain every Secret value in the // cluster. Generated-age-Secret recovery and out-of-band age-key updates are diff --git a/internal/controller/gittarget_controller_test.go b/internal/controller/gittarget_controller_test.go index b1b4db69..89dce766 100644 --- a/internal/controller/gittarget_controller_test.go +++ b/internal/controller/gittarget_controller_test.go @@ -305,10 +305,10 @@ var _ = Describe("GitTarget Controller Security", func() { Eventually(func(g Gomega) { var got configbutleraiv1alpha3.GitTarget g.Expect(k8sClient.Get(ctx, key, &got)).To(Succeed()) - g.Expect(isConditionTrue(got.Status.Conditions, GitTargetConditionReady)).To(BeFalse()) - g.Expect(isConditionTrue(got.Status.Conditions, GitTargetConditionReconciling)).To(BeTrue()) - g.Expect(isConditionTrue(got.Status.Conditions, GitTargetConditionStalled)).To(BeFalse()) - g.Expect(isConditionTrue(got.Status.Conditions, GitTargetConditionStreamsRunning)).To(BeFalse()) + g.Expect(conditionIsTrue(got.Status.Conditions, GitTargetConditionReady)).To(BeFalse()) + g.Expect(conditionIsTrue(got.Status.Conditions, GitTargetConditionReconciling)).To(BeTrue()) + g.Expect(conditionIsTrue(got.Status.Conditions, GitTargetConditionStalled)).To(BeFalse()) + g.Expect(conditionIsTrue(got.Status.Conditions, GitTargetConditionStreamsRunning)).To(BeFalse()) for _, c := range got.Status.Conditions { g.Expect(c.Type).NotTo(Equal("EventStreamLive"), "EventStreamLive condition was removed") } diff --git a/internal/controller/gittarget_controller_unit_test.go b/internal/controller/gittarget_controller_unit_test.go index 01b4d1c4..8da4bd28 100644 --- a/internal/controller/gittarget_controller_unit_test.go +++ b/internal/controller/gittarget_controller_unit_test.go @@ -78,7 +78,7 @@ func TestIsConditionTrue(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, isConditionTrue(tt.conditions, tt.conditionType)) + assert.Equal(t, tt.want, conditionIsTrue(tt.conditions, tt.conditionType)) }) } } diff --git a/internal/controller/gittarget_dependency_status.go b/internal/controller/gittarget_dependency_status.go index bbbe2f0a..f257cc53 100644 --- a/internal/controller/gittarget_dependency_status.go +++ b/internal/controller/gittarget_dependency_status.go @@ -10,35 +10,29 @@ import ( configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" ) -type gitTargetReadyStatus struct { - Status metav1.ConditionStatus - Reason string - Message string -} - -func gitTargetReadyCondition(target configbutleraiv1alpha3.GitTarget) gitTargetReadyStatus { - if ready := conditionByType(target.Status.Conditions, GitTargetConditionReady); ready != nil { +func gitTargetReadyCondition(target configbutleraiv1alpha3.GitTarget) conditionValue { + if ready := findCondition(target.Status.Conditions, GitTargetConditionReady); ready != nil { if ready.Status == metav1.ConditionTrue { - return gitTargetReadyStatus{ + return conditionValue{ Status: metav1.ConditionTrue, Reason: GitTargetReasonOK, Message: fmt.Sprintf("GitTarget %s/%s is ready", target.Namespace, target.Name), } } - if stalled := conditionByType(target.Status.Conditions, GitTargetConditionStalled); stalled != nil && + if stalled := findCondition(target.Status.Conditions, GitTargetConditionStalled); stalled != nil && stalled.Status == metav1.ConditionTrue { reason, message := conditionReasonMessage(stalled, ReasonStalled, "GitTarget is stalled") - return gitTargetReadyStatus{Status: metav1.ConditionFalse, Reason: reason, Message: message} + return conditionValue{Status: metav1.ConditionFalse, Reason: reason, Message: message} } reason, message := conditionReasonMessage(ready, ReasonProgressing, "GitTarget is not ready yet") - return gitTargetReadyStatus{Status: metav1.ConditionFalse, Reason: reason, Message: message} + return conditionValue{Status: metav1.ConditionFalse, Reason: reason, Message: message} } - if reconciling := conditionByType(target.Status.Conditions, GitTargetConditionReconciling); reconciling != nil && + if reconciling := findCondition(target.Status.Conditions, GitTargetConditionReconciling); reconciling != nil && reconciling.Status == metav1.ConditionTrue { reason, message := conditionReasonMessage(reconciling, ReasonProgressing, "GitTarget is reconciling") - return gitTargetReadyStatus{Status: metav1.ConditionFalse, Reason: reason, Message: message} + return conditionValue{Status: metav1.ConditionFalse, Reason: reason, Message: message} } - return gitTargetReadyStatus{ + return conditionValue{ Status: metav1.ConditionUnknown, Reason: ReasonProgressing, Message: fmt.Sprintf("Waiting for GitTarget %s/%s to publish Ready", target.Namespace, target.Name), diff --git a/internal/controller/gittarget_placement_validation_test.go b/internal/controller/gittarget_placement_validation_test.go index bb20282d..a5ab6abb 100644 --- a/internal/controller/gittarget_placement_validation_test.go +++ b/internal/controller/gittarget_placement_validation_test.go @@ -175,8 +175,9 @@ func TestEvaluateValidatedGate_InvalidPlacementPolicy(t *testing.T) { client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(provider, target).Build() reconciler := &GitTargetReconciler{Client: client} + st := beginStatus(client, nil, target, &target.Status.Conditions) - validated, msg, result, err := reconciler.evaluateValidatedGate(context.Background(), target, ns) + validated, msg, result, err := reconciler.evaluateValidatedGate(context.Background(), st, target, ns) require.NoError(t, err) assert.Nil(t, result) diff --git a/internal/controller/gittarget_source_cluster.go b/internal/controller/gittarget_source_cluster.go index 9fef6ee8..cce44686 100644 --- a/internal/controller/gittarget_source_cluster.go +++ b/internal/controller/gittarget_source_cluster.go @@ -11,7 +11,6 @@ import ( configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" "github.com/ConfigButler/gitops-reverser/internal/authz" - "github.com/ConfigButler/gitops-reverser/internal/watch" ) // GitTargetReasonNamespaceNotAuthorized is the Validated=False reason when a GitTarget's namespace @@ -108,33 +107,41 @@ func (r *GitTargetReconciler) auditRouteFor( func (r *GitTargetReconciler) clusterProviderReadiness( ctx context.Context, target *configbutleraiv1alpha3.GitTarget, -) (metav1.ConditionStatus, string, string) { +) conditionValue { name := target.SourceCluster() + notReady := func(status metav1.ConditionStatus, format string, args ...any) conditionValue { + return conditionValue{ + Status: status, + Reason: GitTargetReasonClusterProviderNotReady, + Message: fmt.Sprintf(format, args...), + } + } + var cp configbutleraiv1alpha3.ClusterProvider if err := r.Get(ctx, k8stypes.NamespacedName{Name: name}, &cp); err != nil { - return metav1.ConditionUnknown, GitTargetReasonClusterProviderNotReady, - fmt.Sprintf("referenced ClusterProvider %q readiness not observed: %v", name, err) + return notReady(metav1.ConditionUnknown, + "referenced ClusterProvider %q readiness not observed: %v", name, err) } c := findCondition(cp.Status.Conditions, ConditionTypeReady) switch { case c == nil: - return metav1.ConditionUnknown, GitTargetReasonClusterProviderNotReady, - fmt.Sprintf("referenced ClusterProvider %q has not reported readiness yet", name) + return notReady(metav1.ConditionUnknown, "referenced ClusterProvider %q has not reported readiness yet", name) case c.Status == metav1.ConditionTrue: - return metav1.ConditionTrue, GitTargetReasonClusterProviderReady, - fmt.Sprintf("referenced ClusterProvider %q is Ready", name) + return conditionValue{ + Status: metav1.ConditionTrue, + Reason: GitTargetReasonClusterProviderReady, + Message: fmt.Sprintf("referenced ClusterProvider %q is Ready", name), + } case c.Status == metav1.ConditionFalse: - msg := fmt.Sprintf("referenced ClusterProvider %q is not Ready", name) if c.Message != "" { - msg = fmt.Sprintf("referenced ClusterProvider %q is not Ready: %s", name, c.Message) + return notReady(metav1.ConditionFalse, "referenced ClusterProvider %q is not Ready: %s", name, c.Message) } - return metav1.ConditionFalse, GitTargetReasonClusterProviderNotReady, msg + return notReady(metav1.ConditionFalse, "referenced ClusterProvider %q is not Ready", name) default: // Ready=Unknown is the provider saying it does not know yet, which is exactly the case the // contract above refuses to downgrade on. Collapsing it into False would hold a GitTarget // down on a provider mid-reconcile. - return metav1.ConditionUnknown, GitTargetReasonClusterProviderNotReady, - fmt.Sprintf("referenced ClusterProvider %q readiness is unknown", name) + return notReady(metav1.ConditionUnknown, "referenced ClusterProvider %q readiness is unknown", name) } } @@ -155,103 +162,37 @@ func (r *GitTargetReconciler) gitProviderReadiness( ctx context.Context, target *configbutleraiv1alpha3.GitTarget, providerNS string, -) (metav1.ConditionStatus, string, string) { - var gp configbutleraiv1alpha3.GitProvider +) conditionValue { key := k8stypes.NamespacedName{Name: target.Spec.ProviderRef.Name, Namespace: providerNS} + notReady := func(status metav1.ConditionStatus, format string, args ...any) conditionValue { + return conditionValue{ + Status: status, + Reason: GitTargetReasonGitProviderNotReady, + Message: fmt.Sprintf(format, args...), + } + } + + var gp configbutleraiv1alpha3.GitProvider if err := r.Get(ctx, key, &gp); err != nil { - return metav1.ConditionUnknown, GitTargetReasonGitProviderNotReady, - fmt.Sprintf("referenced GitProvider %s readiness not observed: %v", key, err) + return notReady(metav1.ConditionUnknown, "referenced GitProvider %s readiness not observed: %v", key, err) } c := findCondition(gp.Status.Conditions, ConditionTypeReady) switch { case c == nil: - return metav1.ConditionUnknown, GitTargetReasonGitProviderNotReady, - fmt.Sprintf("referenced GitProvider %s has not reported readiness yet", key) + return notReady(metav1.ConditionUnknown, "referenced GitProvider %s has not reported readiness yet", key) case c.Status == metav1.ConditionTrue: - return metav1.ConditionTrue, GitTargetReasonGitProviderReady, - fmt.Sprintf("referenced GitProvider %s is Ready", key) - default: - msg := fmt.Sprintf("referenced GitProvider %s is not Ready", key) - if c.Message != "" { - msg = fmt.Sprintf("referenced GitProvider %s is not Ready: %s", key, c.Message) + return conditionValue{ + Status: metav1.ConditionTrue, + Reason: GitTargetReasonGitProviderReady, + Message: fmt.Sprintf("referenced GitProvider %s is Ready", key), } - return metav1.ConditionFalse, GitTargetReasonGitProviderNotReady, msg - } -} - -// findCondition returns the named condition, or nil. -func findCondition(conditions []metav1.Condition, conditionType string) *metav1.Condition { - for i := range conditions { - if conditions[i].Type == conditionType { - return &conditions[i] - } - } - return nil -} - -// projectSourceAndProvider sets the SourceClusterReachable and GitProviderReady conditions and -// folds them into the aggregate. It only ever DOWNGRADES Ready — a source-side or -// destination-side problem holds the target below Ready, but a healthy pair never overrides a -// data-plane stall (e.g. a still-replaying stream). Precedence: destination first -// (GitProviderReady, a stall the operator must fix), then source reachability (a transient the -// data plane retries — False is progressing, Unknown holds Ready at Unknown). -func (r *GitTargetReconciler) projectSourceAndProvider( - target *configbutleraiv1alpha3.GitTarget, - sourceReach watch.SourceClusterReachableStatus, - providerStatus metav1.ConditionStatus, - providerReason, providerMessage string, - clusterProviderStatus metav1.ConditionStatus, - clusterProviderReason, clusterProviderMessage string, -) { - reachStatus := conditionStatusFromString(sourceReach.State) - r.setCondition( - target, - GitTargetConditionSourceClusterReachable, - reachStatus, - sourceReach.Reason, - sourceReach.Message, - ) - r.setCondition(target, GitTargetConditionGitProviderReady, providerStatus, providerReason, providerMessage) - r.setCondition(target, GitTargetConditionClusterProviderReady, clusterProviderStatus, - clusterProviderReason, clusterProviderMessage) - - switch { - case providerStatus == metav1.ConditionFalse: - // Destination-side: the provider's own periodic check is failing. Wait for it to - // recover (progressing), which the Watches(&GitProvider{}) trigger promptly re-runs. - r.downgradeReady(target, metav1.ConditionFalse, providerReason, providerMessage) - case clusterProviderStatus == metav1.ConditionFalse: - // Source-config side: the ClusterProvider explicitly reports itself not Ready (e.g. its - // kubeconfig stopped validating). Progressing — the Watches(&ClusterProvider{}) trigger - // re-runs this as the provider recovers. - r.downgradeReady(target, metav1.ConditionFalse, clusterProviderReason, clusterProviderMessage) - case reachStatus == metav1.ConditionFalse: - // Source-side: an otherwise-valid kubeconfig whose API server cannot be contacted. A - // transient the data plane retries, so this is progressing, not stalled. - r.downgradeReady(target, metav1.ConditionFalse, sourceReach.Reason, sourceReach.Message) - case reachStatus == metav1.ConditionUnknown: - // An unconfirmed source is not yet Ready; hold Ready at Unknown until first discovery. - r.downgradeReady(target, metav1.ConditionUnknown, sourceReach.Reason, sourceReach.Message) + case c.Message != "": + return notReady(metav1.ConditionFalse, "referenced GitProvider %s is not Ready: %s", key, c.Message) + default: + return notReady(metav1.ConditionFalse, "referenced GitProvider %s is not Ready", key) } } -// downgradeReady lowers the aggregate below Ready without ever raising it: it is called only on a -// source/provider problem, and rewrites Ready plus the kstatus Reconciling/Stalled pair to match. -// Every such problem here is expected to clear on its own (a provider recovers, a source becomes -// reachable), so it is always PROGRESSING (Reconciling=True, Stalled=False) — a re-check via the -// Watches triggers converges it. A blocking, needs-a-human problem is handled by the setStalled* -// path in Reconcile instead, never here. -func (r *GitTargetReconciler) downgradeReady( - target *configbutleraiv1alpha3.GitTarget, - readyStatus metav1.ConditionStatus, - reason, message string, -) { - r.setCondition(target, GitTargetConditionReady, readyStatus, reason, message) - r.setCondition(target, GitTargetConditionReconciling, metav1.ConditionTrue, reason, message) - r.setCondition(target, GitTargetConditionStalled, metav1.ConditionFalse, ReasonProgressing, - "Reconciliation is making progress") -} - // conditionStatusFromString maps the watch layer's "True"/"False"/"Unknown" onto the API type. func conditionStatusFromString(state string) metav1.ConditionStatus { switch state { diff --git a/internal/controller/gittarget_source_cluster_test.go b/internal/controller/gittarget_source_cluster_test.go index 45b1af2e..8490f0e2 100644 --- a/internal/controller/gittarget_source_cluster_test.go +++ b/internal/controller/gittarget_source_cluster_test.go @@ -397,7 +397,7 @@ func TestClusterProviderReadiness_AllScenarios(t *testing.T) { Status: configbutleraiv1alpha3.ClusterProviderStatus{Conditions: conds}, } } - ready := metav1.Condition{Type: ConditionTypeReady, Status: metav1.ConditionTrue, Reason: "OK"} + ready := metav1.Condition{Type: ConditionTypeReady, Status: metav1.ConditionTrue, Reason: ReasonSucceeded} notReady := metav1.Condition{ Type: ConditionTypeReady, Status: metav1.ConditionFalse, @@ -442,9 +442,9 @@ func TestClusterProviderReadiness_AllScenarios(t *testing.T) { ClusterProviderRef: &configbutleraiv1alpha3.ClusterProviderReference{Name: "prod-eu-1"}, }, } - status, _, msg := r.clusterProviderReadiness(context.Background(), target) - assert.Equal(t, tc.want, status) - assert.NotEmpty(t, msg) + got := r.clusterProviderReadiness(context.Background(), target) + assert.Equal(t, tc.want, got.Status) + assert.NotEmpty(t, got.Message) }) } } @@ -456,7 +456,7 @@ func TestGitProviderReadiness_AllScenarios(t *testing.T) { Status: configbutleraiv1alpha3.GitProviderStatus{Conditions: conds}, } } - ready := metav1.Condition{Type: ConditionTypeReady, Status: metav1.ConditionTrue, Reason: "OK"} + ready := metav1.Condition{Type: ConditionTypeReady, Status: metav1.ConditionTrue, Reason: ReasonSucceeded} notReady := metav1.Condition{ Type: ConditionTypeReady, Status: metav1.ConditionFalse, @@ -487,9 +487,9 @@ func TestGitProviderReadiness_AllScenarios(t *testing.T) { ProviderRef: configbutleraiv1alpha3.GitProviderReference{Name: "prov"}, }, } - status, _, msg := r.gitProviderReadiness(context.Background(), target, "team-a") - assert.Equal(t, tc.want, status) - assert.NotEmpty(t, msg) + got := r.gitProviderReadiness(context.Background(), target, "team-a") + assert.Equal(t, tc.want, got.Status) + assert.NotEmpty(t, got.Message) }) } } diff --git a/internal/controller/gittarget_status_test.go b/internal/controller/gittarget_status_test.go index a168e91b..94a6fc92 100644 --- a/internal/controller/gittarget_status_test.go +++ b/internal/controller/gittarget_status_test.go @@ -9,167 +9,192 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + kstatus "sigs.k8s.io/cli-utils/pkg/kstatus/status" configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/git" "github.com/ConfigButler/gitops-reverser/internal/watch" ) -func TestDeriveGitTargetDataPlaneStatus(t *testing.T) { +// TestGitTargetReadinessGates walks the gate table and asserts what a kstatus CONSUMER would read, +// not what the conditions happen to say. Every row goes through the real kstatus library. +func TestGitTargetReadinessGates(t *testing.T) { + running := watch.StreamSummary{ + Total: 3, Ready: 3, Reason: watch.StreamReasonAllStreamsReady, Message: "3/3 streams running", + } + accepted := watch.GitPathAcceptanceStatus{Accepted: true} + matches := watch.RenderFidelityStatus{State: git.RenderFidelityTrue} + healthy := conditionValue{Status: metav1.ConditionTrue, Reason: ReasonSucceeded, Message: "healthy"} + tests := []struct { - name string - streams watch.StreamSummary - gitPath watch.GitPathAcceptanceStatus - wantReady metav1.ConditionStatus - wantReconciling metav1.ConditionStatus - wantStalled metav1.ConditionStatus - wantGitPath metav1.ConditionStatus - wantStreams metav1.ConditionStatus - wantStalledReason string + name string + streams watch.StreamSummary + gitPath watch.GitPathAcceptanceStatus + render watch.RenderFidelityStatus + provider conditionValue + clusterProvider conditionValue + sourceReach conditionValue + want kstatus.Status + wantReadyReason string }{ { - name: "all streams running and Git path accepted", - streams: watch.StreamSummary{ - Total: 3, Ready: 3, Reason: watch.StreamReasonAllStreamsReady, Message: "3/3 streams running", - }, - gitPath: watch.GitPathAcceptanceStatus{Accepted: true}, - wantReady: metav1.ConditionTrue, - wantReconciling: metav1.ConditionFalse, - wantStalled: metav1.ConditionFalse, - wantGitPath: metav1.ConditionTrue, - wantStreams: metav1.ConditionTrue, + name: "everything healthy", streams: running, gitPath: accepted, render: matches, + provider: healthy, clusterProvider: healthy, sourceReach: healthy, + want: kstatus.CurrentStatus, wantReadyReason: ReasonSucceeded, }, { - name: "stream replaying", + // F2. No WatchRule has claimed this target yet — step 3 of the documented setup flow. + // Nothing is pending, so nothing may report as in progress. + name: "no resolved types is converged, not perpetually reconciling", + streams: watch.StreamSummary{Reason: watch.StreamReasonNoResolvedTypes, Message: "0/0 streams running"}, + gitPath: accepted, render: matches, + provider: healthy, clusterProvider: healthy, sourceReach: healthy, + want: kstatus.CurrentStatus, wantReadyReason: ReasonSucceeded, + }, + { + name: "stream replaying", gitPath: accepted, render: matches, streams: watch.StreamSummary{ Total: 2, Ready: 1, Replaying: 1, Reason: watch.StreamReasonReplaying, Message: "1/2 streams running; 1 replaying", }, - gitPath: watch.GitPathAcceptanceStatus{Accepted: true}, - wantReady: metav1.ConditionFalse, - wantReconciling: metav1.ConditionTrue, - wantStalled: metav1.ConditionFalse, - wantGitPath: metav1.ConditionTrue, - wantStreams: metav1.ConditionFalse, + provider: healthy, clusterProvider: healthy, sourceReach: healthy, + want: kstatus.InProgressStatus, wantReadyReason: watch.StreamReasonReplaying, }, { - name: "stream blocked", + name: "stream blocked", gitPath: accepted, render: matches, streams: watch.StreamSummary{ Total: 2, Ready: 1, Blocked: 1, Reason: watch.StreamReasonWatchError, Message: "1/2 streams running; 1 blocked", }, - gitPath: watch.GitPathAcceptanceStatus{Accepted: true}, - wantReady: metav1.ConditionFalse, - wantReconciling: metav1.ConditionFalse, - wantStalled: metav1.ConditionTrue, - wantGitPath: metav1.ConditionTrue, - wantStreams: metav1.ConditionFalse, - wantStalledReason: watch.StreamReasonWatchError, + provider: healthy, clusterProvider: healthy, sourceReach: healthy, + want: kstatus.FailedStatus, wantReadyReason: watch.StreamReasonWatchError, }, { - name: "Git path refused", - streams: watch.StreamSummary{ - Total: 1, Ready: 1, Reason: watch.StreamReasonAllStreamsReady, Message: "1/1 streams running", - }, + name: "Git path refused", streams: running, render: matches, gitPath: watch.GitPathAcceptanceStatus{ Accepted: false, Reason: GitTargetReasonUnsupportedContent, Message: "Git path refused at kustomization.yaml: uses patches", }, - wantReady: metav1.ConditionFalse, - wantReconciling: metav1.ConditionFalse, - wantStalled: metav1.ConditionTrue, - wantGitPath: metav1.ConditionFalse, - wantStreams: metav1.ConditionTrue, - wantStalledReason: GitTargetReasonUnsupportedContent, + provider: healthy, clusterProvider: healthy, sourceReach: healthy, + want: kstatus.FailedStatus, wantReadyReason: GitTargetReasonUnsupportedContent, }, { - // A write-boundary refusal is not "this folder holds content we cannot manage"; - // it is "this edit had nowhere safe to land". It must carry its own reason all - // the way through to Stalled, so an operator can tell the two apart. - name: "write boundary refused", - streams: watch.StreamSummary{ - Total: 1, Ready: 1, Reason: watch.StreamReasonAllStreamsReady, Message: "1/1 streams running", - }, + // A write-boundary refusal is not "this folder holds content we cannot manage"; it is + // "this edit had nowhere safe to land". It must carry its own reason through to Stalled. + name: "write boundary refused", streams: running, render: matches, gitPath: watch.GitPathAcceptanceStatus{ Accepted: false, Reason: GitTargetReasonWriteBoundaryRefused, Message: "Git path refused at base/deployment.yaml: write-fan-in must be 1", }, - wantReady: metav1.ConditionFalse, - wantReconciling: metav1.ConditionFalse, - wantStalled: metav1.ConditionTrue, - wantGitPath: metav1.ConditionFalse, - wantStreams: metav1.ConditionTrue, - wantStalledReason: GitTargetReasonWriteBoundaryRefused, + provider: healthy, clusterProvider: healthy, sourceReach: healthy, + want: kstatus.FailedStatus, wantReadyReason: GitTargetReasonWriteBoundaryRefused, + }, + { + name: "render diverged is terminal", streams: running, gitPath: accepted, + render: watch.RenderFidelityStatus{ + State: git.RenderFidelityFalse, Reason: GitTargetReasonRenderDoesNotMatchLive, Message: "${REGION}", + }, + provider: healthy, clusterProvider: healthy, sourceReach: healthy, + want: kstatus.FailedStatus, wantReadyReason: GitTargetReasonRenderDoesNotMatchLive, + }, + { + name: "render rechecking is progress", streams: running, gitPath: accepted, + render: watch.RenderFidelityStatus{ + State: git.RenderFidelityUnknown, Reason: GitTargetReasonRenderRechecking, Message: "waiting", + }, + provider: healthy, clusterProvider: healthy, sourceReach: healthy, + want: kstatus.InProgressStatus, wantReadyReason: GitTargetReasonRenderRechecking, + }, + { + name: "unready GitProvider holds the target below Ready", streams: running, + gitPath: accepted, render: matches, + provider: conditionValue{ + Status: metav1.ConditionFalse, Reason: GitTargetReasonGitProviderNotReady, Message: "no repo", + }, + clusterProvider: healthy, sourceReach: healthy, + want: kstatus.InProgressStatus, wantReadyReason: GitTargetReasonGitProviderNotReady, + }, + { + // THE F1 REGRESSION. A refused Git path is terminal, and an unready provider is + // transient. The provider gate used to run last and stamp Reconciling=True / + // Stalled=False over the refusal, flipping the object from kstatus Failed to + // InProgress — so `kubectl wait` and every CI gate built on kstatus waited out its + // timeout on a target that was never going to converge. + name: "refused path AND unready provider stays Failed", streams: running, render: matches, + gitPath: watch.GitPathAcceptanceStatus{ + Accepted: false, + Reason: GitTargetReasonUnsupportedContent, + Message: "Git path refused at kustomization.yaml: uses patches", + }, + provider: conditionValue{ + Status: metav1.ConditionFalse, Reason: GitTargetReasonGitProviderNotReady, Message: "no repo", + }, + clusterProvider: healthy, + sourceReach: conditionValue{ + Status: metav1.ConditionUnknown, Reason: "AwaitingDiscovery", Message: "not discovered yet", + }, + want: kstatus.FailedStatus, wantReadyReason: GitTargetReasonUnsupportedContent, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - d := deriveGitTargetDataPlaneStatus(tt.streams, tt.gitPath) - assert.Equal(t, tt.wantReady, d.ReadyStatus) - assert.Equal(t, tt.wantReconciling, d.ReconcilingStatus) - assert.Equal(t, tt.wantStalled, d.StalledStatus) - assert.Equal(t, tt.wantGitPath, d.GitPathStatus) - assert.Equal(t, tt.wantStreams, d.StreamsStatus) - if tt.wantStalledReason != "" { - assert.Equal(t, tt.wantStalledReason, d.StalledReason) + target := &configbutleraiv1alpha3.GitTarget{} + st := beginStatus(nil, nil, target, &target.Status.Conditions) + observed := dataPlaneObservation{ + streams: tt.streams, + axes: gitTargetAxes{ + Streams: streamsAxis(tt.streams), + GitPath: gitPathAxis(tt.gitPath), + Render: renderAxis(tt.render), + }, } - assert.NotEmpty(t, d.ReadyMessage) + st.setValue(GitTargetConditionStreamsRunning, observed.axes.Streams) + st.setValue(GitTargetConditionGitPathAccepted, observed.axes.GitPath) + st.setValue(GitTargetConditionRenderMatchesLive, observed.axes.Render) + + rd := newGitTargetReadiness() + gitTargetReadinessGates(rd, observed, tt.provider, tt.clusterProvider, tt.sourceReach) + st.applyReadiness(rd) + + ready := findCondition(target.Status.Conditions, GitTargetConditionReady) + require.NotNil(t, ready) + assert.Equal(t, tt.wantReadyReason, ready.Reason) + assert.NotEmpty(t, ready.Message) + + computed, err := kstatus.Compute(gitTargetStatusObject(unstructuredConditions(target.Status.Conditions))) + require.NoError(t, err) + assert.Equal(t, tt.want, computed.Status, + "a kstatus consumer must read %s here", tt.want) }) } } -func TestApplyDataPlaneConditions_SetsKstatusTrio(t *testing.T) { - r := &GitTargetReconciler{} - target := &configbutleraiv1alpha3.GitTarget{} - - r.applyDataPlaneConditions(target, watch.StreamSummary{ - Total: 1, Ready: 1, Reason: watch.StreamReasonAllStreamsReady, Message: "1/1 streams running", - }, watch.GitPathAcceptanceStatus{Accepted: true}, watch.RenderFidelityStatus{State: "True"}) - - require.True(t, isConditionTrue(target.Status.Conditions, GitTargetConditionReady)) - require.True(t, isConditionTrue(target.Status.Conditions, GitTargetConditionStreamsRunning)) - require.True(t, isConditionTrue(target.Status.Conditions, GitTargetConditionGitPathAccepted)) - require.True(t, isConditionTrue(target.Status.Conditions, GitTargetConditionRenderMatchesLive)) - require.False(t, isConditionTrue(target.Status.Conditions, GitTargetConditionReconciling)) - require.False(t, isConditionTrue(target.Status.Conditions, GitTargetConditionStalled)) +// unstructuredConditions renders a real condition set the way kstatus.Compute consumes it, so the +// conformance assertions above run against reconciler OUTPUT rather than a hand-built fixture. +func unstructuredConditions(conditions []metav1.Condition) []map[string]interface{} { + out := make([]map[string]interface{}, 0, len(conditions)) + for _, c := range conditions { + out = append(out, conditionMap(c.Type, string(c.Status), c.Reason, c.Message)) + } + return out } -func TestSetBlockedDataPlane_MarksUnknownAndPending(t *testing.T) { - r := &GitTargetReconciler{} +// TestGitTargetStall_PublishesTerminalTrio covers the control-plane gates that return before the +// data plane is ever evaluated. +func TestGitTargetStall_PublishesTerminalTrio(t *testing.T) { target := &configbutleraiv1alpha3.GitTarget{} + st := beginStatus(nil, nil, target, &target.Status.Conditions) - r.setBlockedDataPlane(target) - - streamsRunning := conditionByType(target.Status.Conditions, GitTargetConditionStreamsRunning) - require.NotNil(t, streamsRunning) - assert.Equal(t, metav1.ConditionUnknown, streamsRunning.Status) - assert.Equal(t, GitTargetStreamsRunningReasonNotReady, streamsRunning.Reason) - renderMatchesLive := conditionByType(target.Status.Conditions, GitTargetConditionRenderMatchesLive) - require.NotNil(t, renderMatchesLive) - assert.Equal(t, metav1.ConditionUnknown, renderMatchesLive.Status) -} + rd := newGitTargetReadiness() + rd.stalled(GitTargetReadyReasonValidationFailed, "Validated gate failed: ProviderNotFound") + st.applyReadiness(rd) -func TestDeriveGitTargetDataPlaneStatusWithRenderFidelity(t *testing.T) { - streams := watch.StreamSummary{ - Total: 1, Ready: 1, Reason: watch.StreamReasonAllStreamsReady, Message: "1/1 streams running", - } - gitPath := watch.GitPathAcceptanceStatus{Accepted: true} - - unknown := deriveGitTargetDataPlaneStatusWithRenderFidelity( - streams, gitPath, watch.RenderFidelityStatus{State: "Unknown", Reason: "Rechecking", Message: "waiting"}) - assert.Equal(t, metav1.ConditionUnknown, unknown.RenderFidelityStatus) - assert.Equal(t, metav1.ConditionTrue, unknown.ReconcilingStatus) - assert.Equal(t, metav1.ConditionFalse, unknown.StalledStatus) - - diverged := deriveGitTargetDataPlaneStatusWithRenderFidelity( - streams, gitPath, - watch.RenderFidelityStatus{State: "False", Reason: "RenderDoesNotMatchLive", Message: "${REGION}"}) - assert.Equal(t, metav1.ConditionFalse, diverged.RenderFidelityStatus) - assert.Equal(t, metav1.ConditionFalse, diverged.ReadyStatus) - assert.Equal(t, metav1.ConditionTrue, diverged.StalledStatus) - assert.Equal(t, GitTargetReasonRenderDoesNotMatchLive, diverged.StalledReason) + computed, err := kstatus.Compute(gitTargetStatusObject(unstructuredConditions(target.Status.Conditions))) + require.NoError(t, err) + assert.Equal(t, kstatus.FailedStatus, computed.Status) } // TestGitTargetRetentionStatus_AbsentAndZeroMeanDifferentThings is the whole point of the field diff --git a/internal/controller/readiness.go b/internal/controller/readiness.go new file mode 100644 index 00000000..da9b7559 --- /dev/null +++ b/internal/controller/readiness.go @@ -0,0 +1,170 @@ +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// readinessLevel ranks how a gate's outcome must be reported to kstatus. The order is the +// precedence: a terminal gate outranks a progressing one, which outranks convergence. +type readinessLevel int + +const ( + // readinessConverged: nothing is pending and nothing is wrong. kstatus Current. + readinessConverged readinessLevel = iota + // readinessProgressing: the gate will clear on its own — a dependency is still coming up, a + // stream is still replaying. kstatus InProgress. + readinessProgressing + // readinessStalled: the gate needs a human. Waiting changes nothing. kstatus Failed. + readinessStalled +) + +// readiness is the accumulator that makes ONE function the writer of the Ready/Reconciling/Stalled +// trio. +// +// The bug it exists to prevent: when every gate sets the trio itself, the trio says whatever the +// LAST gate said. A GitTarget whose Git path had just been refused (terminal: Ready=False, +// Stalled=True) was then handed to the source/provider projection, which stamped +// Stalled=False, Reconciling=True over it because the provider happened to be mid-check. For a +// human reading `Ready` nothing much changed. For kstatus — which never reads Ready, only the +// abnormal-true pair — the object flipped from Failed to InProgress, so `kubectl wait` and every +// CI gate built on it waited out its timeout on a target that was never going to converge. +// +// So gates no longer set: they CONTRIBUTE. Each one reports its verdict, the worst verdict wins, +// and the trio is derived from that single answer. This is the shape Flux settled on for the same +// reason — summarize once at the end from a declared precedence order, rather than by successive +// mutation (`fluxcd/pkg/runtime/patch`, `WithOwnedConditions`). +// +// Ties within a level go to the FIRST contributor, so callers must contribute in precedence order. +// That is a deliberate constraint: it forces the precedence to be written down as the order of a +// handful of adjacent calls, where it can be read, instead of emerging from which gate happened to +// run last. +type readiness struct { + // whenConverged is what Ready reports when no gate objected. + whenConverged conditionValue + // notStalledMessage is the message on Stalled=False. It names the kind ("GitTarget is not + // stalled"), which is why it is per-caller rather than a constant. + notStalledMessage string + + level readinessLevel + verdict conditionValue +} + +// newReadiness starts an accumulator whose converged outcome is Ready=True with the shared +// Succeeded reason and the given message. A readiness with no contributions reports exactly that. +func newReadiness(message, notStalledMessage string) *readiness { + return &readiness{ + whenConverged: conditionValue{ + Status: metav1.ConditionTrue, + Reason: ReasonSucceeded, + Message: message, + }, + notStalledMessage: notStalledMessage, + } +} + +// stalled contributes a terminal gate: this object is not converging and will not, until someone +// changes something. +func (r *readiness) stalled(reason, message string) { + r.contribute(readinessStalled, conditionValue{Status: metav1.ConditionFalse, Reason: reason, Message: message}) +} + +// progressing contributes a gate that is expected to clear on its own. +// +// readyStatus distinguishes the two honest ways to not-be-ready-yet: False, when the answer is +// known to be "not yet" (a dependency reported itself unready), and Unknown, when the answer has +// not been established at all (nothing has been observed since startup). kstatus treats both as +// InProgress; the difference is for the human and for `kubectl wait`. +func (r *readiness) progressing(readyStatus metav1.ConditionStatus, reason, message string) { + r.contribute(readinessProgressing, conditionValue{Status: readyStatus, Reason: reason, Message: message}) +} + +// stalledIf and progressingIf contribute only when cond holds, so a gate list reads as a list. +func (r *readiness) stalledIf(cond bool, reason, message string) { + if cond { + r.stalled(reason, message) + } +} + +func (r *readiness) progressingIf(cond bool, readyStatus metav1.ConditionStatus, reason, message string) { + if cond { + r.progressing(readyStatus, reason, message) + } +} + +func (r *readiness) contribute(level readinessLevel, verdict conditionValue) { + if level <= r.level { + return + } + r.level = level + r.verdict = verdict +} + +// converged reports whether no gate objected — the signal a reconcile uses to pick the steady +// requeue cadence over the fast one. +func (r *readiness) converged() bool { return r.level == readinessConverged } + +// kstatusTrio is the Ready/Reconciling/Stalled triple, derived together so they cannot disagree. +type kstatusTrio struct { + Ready conditionValue + Reconciling conditionValue + Stalled conditionValue +} + +// trio derives Ready, Reconciling and Stalled from the accumulated verdict. +// +// The abnormal-true pair is written even when False. The Kubernetes API conventions say such a +// condition SHOULD only be present when True, and kstatus tolerates either (it tests for == True +// and ignores everything else) — but `kubectl wait --for=condition=Stalled=false` and this repo's +// e2e suite both read the explicit False, and a condition that vanishes is harder to reason about +// than one that reads False. The deviation is deliberate and documented in +// docs/spec/status-conditions-guide.md. +func (r *readiness) trio() kstatusTrio { + switch r.level { + case readinessStalled: + return kstatusTrio{ + Ready: r.verdict, + Reconciling: conditionValue{ + Status: metav1.ConditionFalse, + Reason: r.verdict.Reason, + Message: "Reconciliation is stalled", + }, + Stalled: conditionValue{ + Status: metav1.ConditionTrue, + Reason: r.verdict.Reason, + Message: r.verdict.Message, + }, + } + + case readinessProgressing: + return kstatusTrio{ + Ready: r.verdict, + Reconciling: conditionValue{ + Status: metav1.ConditionTrue, + Reason: r.verdict.Reason, + Message: r.verdict.Message, + }, + Stalled: conditionValue{ + Status: metav1.ConditionFalse, + Reason: ReasonProgressing, + Message: "Reconciliation is making progress", + }, + } + + case readinessConverged: + } + return kstatusTrio{ + Ready: r.whenConverged, + Reconciling: conditionValue{ + Status: metav1.ConditionFalse, + Reason: r.whenConverged.Reason, + Message: "Reconciliation complete", + }, + Stalled: conditionValue{ + Status: metav1.ConditionFalse, + Reason: r.whenConverged.Reason, + Message: r.notStalledMessage, + }, + } +} diff --git a/internal/controller/ssh_test.go b/internal/controller/ssh_test.go index f2cf0a4d..301611ce 100644 --- a/internal/controller/ssh_test.go +++ b/internal/controller/ssh_test.go @@ -410,8 +410,6 @@ func TestCheckRemoteConnectivity(t *testing.T) { // TestGitProviderConditions tests the condition setting logic. func TestGitProviderConditions(t *testing.T) { - reconciler := &GitProviderReconciler{} - // Mock GitProvider gitProvider := &configbutleraiv1alpha3.GitProvider{} @@ -421,7 +419,7 @@ func TestGitProviderConditions(t *testing.T) { reason string message string }{ - {metav1.ConditionTrue, "Ready", "Repository connectivity validated"}, + {metav1.ConditionTrue, ReasonSucceeded, "Repository connectivity validated"}, {metav1.ConditionFalse, ReasonConnectionFailed, "Failed to connect"}, {metav1.ConditionFalse, ReasonSecretNotFound, "Secret not found"}, {metav1.ConditionFalse, ReasonChecking, "Checking repository"}, @@ -429,7 +427,8 @@ func TestGitProviderConditions(t *testing.T) { for _, tc := range testCases { t.Run(tc.reason, func(t *testing.T) { - reconciler.setCondition(gitProvider, ConditionTypeReady, tc.status, tc.reason, tc.message) + st := beginStatus(nil, nil, gitProvider, &gitProvider.Status.Conditions) + st.set(ConditionTypeReady, tc.status, tc.reason, tc.message) if len(gitProvider.Status.Conditions) == 0 { t.Error("Expected at least one condition") diff --git a/internal/controller/status.go b/internal/controller/status.go new file mode 100644 index 00000000..2a093e21 --- /dev/null +++ b/internal/controller/status.go @@ -0,0 +1,184 @@ +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + "context" + "fmt" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/client" + logf "sigs.k8s.io/controller-runtime/pkg/log" +) + +// conditionValue is a condition's payload without its type: the (status, reason, message) triple +// that every gate produces and every writer consumes. +// +// It exists so a gate can RETURN its verdict instead of applying it. Passing the triple around as +// three positional strings is what made "who writes Ready?" unanswerable — every gate had to call a +// setter, so the answer was always "whoever ran last". See readiness. +type conditionValue struct { + Status metav1.ConditionStatus + Reason string + Message string +} + +// conditionReasonMessage reads a condition's reason and message, substituting the given defaults +// for whichever of the two the condition leaves empty. +func conditionReasonMessage(condition *metav1.Condition, defaultReason, defaultMessage string) (string, string) { + if condition == nil { + return defaultReason, defaultMessage + } + reason, message := condition.Reason, condition.Message + if reason == "" { + reason = defaultReason + } + if message == "" { + message = defaultMessage + } + return reason, message +} + +// reasonUnspecified is the placeholder for a condition written with an empty reason. The CRD schema +// requires a reason, so an empty one is rejected with a 422 that fails the whole status write and +// hides whatever the condition was trying to report. A visibly wrong reason is strictly better. +const reasonUnspecified = "Unspecified" + +// reconcileStatus is the per-reconcile status session: it captures the object's status as read, +// collects every condition the reconcile writes, and commits the difference exactly once. +// +// It replaces the five near-identical `updateStatusWithRetry` implementations that each controller +// used to carry, and with them three defects those copies shared: +// +// - They wrote unconditionally. A full-object `Status().Update` on every pass bumps +// resourceVersion, which fires an Update watch event, which re-enqueues the object — every +// reconcile cost roughly two. commit() sends nothing at all when nothing changed. +// - They did read-modify-write: re-Get the object, overwrite its whole status, Update. That +// stamps the freshly-read object with an observedGeneration computed from the generation this +// reconcile actually saw, which may already be stale — kstatus then reads Current for a spec +// nobody looked at. commit() patches with optimistic concurrency instead, so a spec that moved +// under us loses the write rather than mislabelling it. +// - They retried on conflict with an exponential backoff. A conflict means somebody else wrote +// the object, and that write already enqueued us; re-running the reconcile on fresh data is +// both simpler and more correct than replaying a status computed from stale data. +type reconcileStatus struct { + client client.Client + recorder record.EventRecorder + + object client.Object + // before is the object as read at the top of the reconcile; the patch is computed against it. + before client.Object + // beforeConditions is the persisted condition set, kept for the Event on a Ready transition. + beforeConditions []metav1.Condition + // conditions points at the live status condition slice of object, which set() rewrites. + conditions *[]metav1.Condition +} + +// beginStatus opens the status session. Call it once, immediately after the object is read and +// before any condition is written. +func beginStatus( + c client.Client, + recorder record.EventRecorder, + object client.Object, + conditions *[]metav1.Condition, +) *reconcileStatus { + before, _ := object.DeepCopyObject().(client.Object) + return &reconcileStatus{ + client: c, + recorder: recorder, + object: object, + before: before, + beforeConditions: append([]metav1.Condition(nil), *conditions...), + conditions: conditions, + } +} + +// set writes one condition, pinned to the generation this reconcile observed. +func (s *reconcileStatus) set(conditionType string, status metav1.ConditionStatus, reason, message string) { + if reason == "" { + reason = reasonUnspecified + } + *s.conditions = upsertCondition( + *s.conditions, conditionType, status, reason, message, s.object.GetGeneration()) +} + +// setValue writes one condition from a gate's verdict. +func (s *reconcileStatus) setValue(conditionType string, value conditionValue) { + s.set(conditionType, value.Status, value.Reason, value.Message) +} + +// applyReadiness writes the kstatus trio — and is the ONLY thing that writes it. Every gate +// contributes to the readiness accumulator; the trio is derived from those contributions once, +// here, from a precedence stated in one place. +func (s *reconcileStatus) applyReadiness(r *readiness) { + trio := r.trio() + s.setValue(ConditionTypeReady, trio.Ready) + s.setValue(ConditionTypeReconciling, trio.Reconciling) + s.setValue(ConditionTypeStalled, trio.Stalled) +} + +// commit writes the status subresource — or nothing, when the status is identical to what was read. +// +// Suppressing the no-op write is what breaks the self-triggering reconcile edge: a status write +// bumps resourceVersion and fires the Update watch event that `For()` turns straight back into a +// queued request. Nothing to say, nothing to write, no wake-up. +func (s *reconcileStatus) commit(ctx context.Context) error { + log := logf.FromContext(ctx).WithName("status") + + data, err := client.MergeFrom(s.before).Data(s.object) + if err != nil { + return fmt.Errorf("compute status patch for %s: %w", client.ObjectKeyFromObject(s.object), err) + } + if string(data) == "{}" { + return nil + } + + // Optimistic concurrency, deliberately WITHOUT a retry loop: a conflict means the object moved + // under this reconcile, so the status just computed describes a generation that is no longer + // current. The write that beat us enqueued us again, so dropping this one converges on fresh + // data instead of publishing a stale observation. + patch := client.MergeFromWithOptions(s.before, client.MergeFromWithOptimisticLock{}) + switch err := s.client.Status().Patch(ctx, s.object, patch); { + case err == nil: + s.recordReadyTransition() + return nil + case apierrors.IsNotFound(err): + return nil + case apierrors.IsConflict(err): + log.V(1).Info("status write skipped; object changed during reconcile", + "object", client.ObjectKeyFromObject(s.object)) + return nil + default: + return fmt.Errorf("write status for %s: %w", client.ObjectKeyFromObject(s.object), err) + } +} + +// recordReadyTransition emits one Kubernetes Event per PERSISTED change of Ready. +// +// It runs after the patch rather than beside set() on purpose: a reconcile may write Ready several +// times (an early "Validating" placeholder, then the real outcome) and only the last one is ever +// stored. Announcing intermediate values would fill `kubectl describe` with states that never +// existed. Events are how a transient failure that clears before anyone looks stays visible at all, +// and they are the only thing an Event-driven alerting pipeline can route. +func (s *reconcileStatus) recordReadyTransition() { + if s.recorder == nil { + return + } + after := findCondition(*s.conditions, ConditionTypeReady) + if after == nil { + return + } + before := findCondition(s.beforeConditions, ConditionTypeReady) + if before != nil && before.Status == after.Status && before.Reason == after.Reason { + return + } + + eventType := corev1.EventTypeWarning + if after.Status == metav1.ConditionTrue { + eventType = corev1.EventTypeNormal + } + s.recorder.Event(s.object, eventType, after.Reason, after.Message) +} diff --git a/internal/controller/stream_status.go b/internal/controller/stream_status.go index bd08123f..232a5f30 100644 --- a/internal/controller/stream_status.go +++ b/internal/controller/stream_status.go @@ -24,10 +24,6 @@ func noResolvedStreamsSummary() watch.StreamSummary { } func watchRuleStreamsStatus(streams watch.StreamSummary) *configbutleraiv1alpha3.WatchRuleStreamsStatus { - observed := streams.ObservedTime - if observed.IsZero() { - observed = metav1.Now() - } return &configbutleraiv1alpha3.WatchRuleStreamsStatus{ Summary: streams.Summary(), Total: clampIntToInt32(streams.Total), @@ -35,106 +31,75 @@ func watchRuleStreamsStatus(streams watch.StreamSummary) *configbutleraiv1alpha3 Replaying: clampIntToInt32(streams.Replaying), Blocked: clampIntToInt32(streams.Blocked), PendingSample: append([]string(nil), streams.PendingSample...), - ObservedTime: &observed, } } -// ruleReadyReason is the reason every rule kind stamps on its Ready/Reconciling/Stalled trio once -// it is healthy. WatchRule and ClusterWatchRule both spell it "Ready", so it is a constant here -// rather than a parameter each caller passes identically. -const ruleReadyReason = "Ready" +// newRuleReadiness starts the accumulator that owns a rule kind's kstatus trio. +func newRuleReadiness(kind, readyMessage string) *readiness { + return newReadiness(readyMessage, kind+" is not stalled") +} + +// ruleReadiness derives a rule's trio from the axis conditions the reconcile already published. +// +// Both rule kinds share it because they share the axes: resources resolved, the referenced +// GitTarget's health, whether streams are running, and — WatchRule only — whether the effective +// source namespace is authorized. ClusterWatchRule simply carries no SourceNamespaceAuthorized +// condition, so that gate is absent rather than special-cased. +// +// The order of the contributions below IS the precedence; see readiness. +func ruleReadiness(conditions []metav1.Condition, kind, readyMessage string) *readiness { + rd := newRuleReadiness(kind, readyMessage) -func applyRuleKstatus( - conditions []metav1.Condition, - readyMessage string, - notStalledMessage string, - setCondition func(string, metav1.ConditionStatus, string, string), - setStalled func(string, string), -) { - resources := conditionByType(conditions, ConditionTypeResourcesResolved) - gitTarget := conditionByType(conditions, ConditionTypeGitTargetReady) - streams := conditionByType(conditions, ConditionTypeStreamsRunning) + resources := findCondition(conditions, ConditionTypeResourcesResolved) + gitTarget := findCondition(conditions, ConditionTypeGitTargetReady) + streams := findCondition(conditions, ConditionTypeStreamsRunning) // Absent on ClusterWatchRule, which does not carry this condition; nil simply never gates. - sourceNS := conditionByType(conditions, ConditionTypeSourceNamespaceAuthorized) + sourceNS := findCondition(conditions, ConditionTypeSourceNamespaceAuthorized) - if stalled := stalledRuleCondition(resources, gitTarget, streams, sourceNS); stalled != nil { - setStalled(stalled.Reason, stalled.Message) - return + // A source-authorization REFUSAL outranks every other cause: the rule is not running and no + // amount of waiting changes that, so it must not be reported as merely progressing behind an + // unresolved type or an unready target. Only False is terminal here — Unknown is the + // deliberately non-terminal "cannot say yet", handled among the progressing gates below. + contributeStalled(rd, sourceNS) + contributeStalled(rd, resources) + if gitTarget != nil && gitTargetReadyReasonIsStalled(gitTarget.Reason) { + contributeStalled(rd, gitTarget) + } + if streams != nil && streamReasonIsStalled(streams.Reason) { + contributeStalled(rd, streams) } + // Source authorization is an ADDITIONAL prerequisite of Ready, so Ready=True means the source // namespace is authorized AND the GitTarget is ready AND resources resolved AND streams are // running — never merely that the gate passed. Unknown yields InProgress here rather than a // separate status path, which is what makes the retained-scope and cache-syncing cases // representable without inventing a second readiness model. - if sourceNS != nil && sourceNS.Status != metav1.ConditionTrue { - reason, message := conditionReasonMessage( - sourceNS, ReasonProgressing, "Establishing source-namespace authorization") - setRuleProgressing(reason, message, notStalledMessage, setCondition) - return - } - if gitTarget != nil && gitTarget.Status != metav1.ConditionTrue { - reason, message := conditionReasonMessage(gitTarget, ReasonProgressing, "Waiting for GitTarget to become ready") - setRuleProgressing(reason, message, notStalledMessage, setCondition) - return - } - - switch { - case streams != nil && streams.Status == metav1.ConditionTrue: - setCondition(ConditionTypeReady, metav1.ConditionTrue, ruleReadyReason, readyMessage) - setCondition(ConditionTypeReconciling, metav1.ConditionFalse, ruleReadyReason, "Reconciliation complete") - setCondition(ConditionTypeStalled, metav1.ConditionFalse, ruleReadyReason, notStalledMessage) - default: - reason, message := ReasonProgressing, "Waiting for streams to run" - if streams != nil { - reason, message = streams.Reason, streams.Message - } - setRuleProgressing(reason, message, notStalledMessage, setCondition) - } + contributeProgressing(rd, sourceNS, ReasonProgressing, "Establishing source-namespace authorization") + contributeProgressing(rd, gitTarget, ReasonProgressing, "Waiting for GitTarget to become ready") + contributeProgressing(rd, streams, ReasonProgressing, "Waiting for streams to run") + return rd } -func stalledRuleCondition(resources, gitTarget, streams, sourceNS *metav1.Condition) *metav1.Condition { - switch { - // A source-authorization REFUSAL outranks every other cause: the rule is not running and no - // amount of waiting changes that, so it must not be reported as merely progressing behind an - // unresolved type or an unready target. Only False is terminal here — Unknown is the - // deliberately non-terminal "cannot say yet", handled by the caller as progressing. - case sourceNS != nil && sourceNS.Status == metav1.ConditionFalse: - return sourceNS - case resources != nil && resources.Status == metav1.ConditionFalse: - return resources - case gitTarget != nil && gitTarget.Status == metav1.ConditionFalse && gitTargetReadyReasonIsStalled(gitTarget.Reason): - return gitTarget - case streams != nil && streams.Status == metav1.ConditionFalse && streamReasonIsStalled(streams.Reason): - return streams - default: - return nil +// contributeStalled contributes a present-and-False condition as a terminal gate. +func contributeStalled(rd *readiness, condition *metav1.Condition) { + if condition != nil && condition.Status == metav1.ConditionFalse { + rd.stalled(condition.Reason, condition.Message) } } -func setRuleProgressing( - reason string, - message string, - notStalledMessage string, - setCondition func(string, metav1.ConditionStatus, string, string), -) { - setCondition(ConditionTypeReady, metav1.ConditionFalse, reason, message) - setCondition(ConditionTypeReconciling, metav1.ConditionTrue, reason, message) - setCondition(ConditionTypeStalled, metav1.ConditionFalse, reason, notStalledMessage) -} - -func conditionReasonMessage(condition *metav1.Condition, defaultReason, defaultMessage string) (string, string) { - if condition == nil { - return defaultReason, defaultMessage +// contributeProgressing contributes any condition that is not yet True as a transient gate, falling +// back to the given reason and message when the condition itself carries neither. +func contributeProgressing(rd *readiness, condition *metav1.Condition, reason, message string) { + if condition == nil || condition.Status == metav1.ConditionTrue { + return } - reason := condition.Reason - if reason == "" { - reason = defaultReason + if condition.Reason != "" { + reason = condition.Reason } - message := condition.Message - if message == "" { - message = defaultMessage + if condition.Message != "" { + message = condition.Message } - return reason, message + rd.progressing(metav1.ConditionFalse, reason, message) } func streamReasonIsStalled(reason string) bool { diff --git a/internal/controller/stream_status_test.go b/internal/controller/stream_status_test.go index ff96d85e..dd73adf1 100644 --- a/internal/controller/stream_status_test.go +++ b/internal/controller/stream_status_test.go @@ -11,7 +11,7 @@ import ( "github.com/ConfigButler/gitops-reverser/internal/watch" ) -func TestApplyRuleKstatus_GitTargetReadyStalledBlocksRule(t *testing.T) { +func TestRuleReadiness_GitTargetReadyStalledBlocksRule(t *testing.T) { conditions := []metav1.Condition{ {Type: ConditionTypeResourcesResolved, Status: metav1.ConditionTrue, Reason: "Resolved", Message: "resolved"}, { @@ -27,29 +27,10 @@ func TestApplyRuleKstatus_GitTargetReadyStalledBlocksRule(t *testing.T) { Message: "1/1 streams running", }, } - got := map[string]metav1.ConditionStatus{} - reasons := map[string]string{} + trio := ruleReadiness(conditions, "WatchRule", "rule ready").trio() - applyRuleKstatus( - conditions, - "rule ready", - "rule is not stalled", - func(conditionType string, status metav1.ConditionStatus, reason, _ string) { - got[conditionType] = status - reasons[conditionType] = reason - }, - func(reason, _ string) { - got[ConditionTypeReady] = metav1.ConditionFalse - got[ConditionTypeReconciling] = metav1.ConditionFalse - got[ConditionTypeStalled] = metav1.ConditionTrue - reasons[ConditionTypeReady] = reason - reasons[ConditionTypeReconciling] = reason - reasons[ConditionTypeStalled] = reason - }, - ) - - assert.Equal(t, metav1.ConditionFalse, got[ConditionTypeReady]) - assert.Equal(t, metav1.ConditionFalse, got[ConditionTypeReconciling]) - assert.Equal(t, metav1.ConditionTrue, got[ConditionTypeStalled]) - assert.Equal(t, GitTargetReasonUnsupportedContent, reasons[ConditionTypeStalled]) + assert.Equal(t, metav1.ConditionFalse, trio.Ready.Status) + assert.Equal(t, metav1.ConditionFalse, trio.Reconciling.Status) + assert.Equal(t, metav1.ConditionTrue, trio.Stalled.Status) + assert.Equal(t, GitTargetReasonUnsupportedContent, trio.Stalled.Reason) } diff --git a/internal/controller/watchrule_controller.go b/internal/controller/watchrule_controller.go index fc662f87..5fb14229 100644 --- a/internal/controller/watchrule_controller.go +++ b/internal/controller/watchrule_controller.go @@ -6,11 +6,10 @@ import ( "context" "fmt" - apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" - "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" @@ -33,7 +32,7 @@ const ( WatchRuleReasonAccessDenied = "AccessDenied" WatchRuleReasonGitTargetNotFound = "GitTargetNotFound" WatchRuleReasonGitDestinationInvalid = "GitDestinationInvalid" - WatchRuleReasonReady = "Ready" + WatchRuleReasonReady = ReasonSucceeded WatchRuleReasonResourcesResolved = "Resolved" WatchRuleReasonUnresolvedResources = "UnresolvedResources" ) @@ -45,6 +44,8 @@ type WatchRuleReconciler struct { Scheme *runtime.Scheme RuleStore *rulestore.RuleStore WatchManager WatchManagerInterface + // Recorder emits a Kubernetes Event on every persisted Ready transition; nil disables Events. + Recorder record.EventRecorder } // +kubebuilder:rbac:groups=configbutler.ai,resources=watchrules,verbs=get;list;watch;create;update;patch;delete @@ -53,6 +54,7 @@ type WatchRuleReconciler struct { // +kubebuilder:rbac:groups=configbutler.ai,resources=gitproviders,verbs=get;list;watch // +kubebuilder:rbac:groups=configbutler.ai,resources=clusterproviders,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=namespaces,verbs=get;list;watch +// +kubebuilder:rbac:groups="",resources=events,verbs=create;patch // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state. @@ -103,72 +105,48 @@ func (r *WatchRuleReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( "target", watchRule.Spec.TargetRef, "generation", watchRule.Generation, "resourceVersion", watchRule.ResourceVersion) + st := beginStatus(r.Client, r.Recorder, &watchRule, &watchRule.Status.Conditions) watchRule.Status.ObservedGeneration = watchRule.Generation - // Set initial validating status - log.Info("Setting initial validating status") - r.setCondition(&watchRule, metav1.ConditionUnknown, //nolint:lll // Descriptive message - WatchRuleReasonValidating, "Validating WatchRule configuration...") - r.setTypedCondition( - &watchRule, + // Seed the axis conditions as not-yet-evaluated. There is deliberately no placeholder write of + // the Ready/Reconciling/Stalled trio here: every path below ends in exactly one applyReadiness, + // and a placeholder trio would be a second writer of the thing that must have only one. + st.set( ConditionTypeStreamsRunning, metav1.ConditionUnknown, GitTargetStreamsRunningReasonNotReady, "Blocked by validation; streams not evaluated", ) - r.setTypedCondition( - &watchRule, + st.set( ConditionTypeGitTargetReady, metav1.ConditionUnknown, ReasonProgressing, "Blocked by validation; GitTarget not evaluated", ) - r.setTypedCondition( - &watchRule, + st.set( ConditionTypeSourceNamespaceAuthorized, metav1.ConditionUnknown, WatchRuleReasonCheckingSourceNamespacePolicy, "Blocked by validation; source namespace not evaluated", ) - r.setTypedCondition( - &watchRule, - ConditionTypeReconciling, - metav1.ConditionTrue, - ReasonChecking, - "Validating WatchRule", - ) - r.setTypedCondition( - &watchRule, - ConditionTypeStalled, - metav1.ConditionFalse, - ReasonChecking, - "WatchRule is not stalled", - ) // Route by configuration surface (Target is required now) if watchRule.Spec.TargetRef.Name == "" { - r.setCondition( - &watchRule, - metav1.ConditionFalse, - WatchRuleReasonGitDestinationInvalid, - "Target.name must be specified", - ) - r.setTypedCondition( - &watchRule, + st.set( ConditionTypeGitTargetReady, metav1.ConditionFalse, WatchRuleReasonGitDestinationInvalid, "Target.name must be specified", ) - r.setRuleStalled(&watchRule, WatchRuleReasonGitDestinationInvalid, "Target.name must be specified") - return r.updateStatusAndRequeue(ctx, &watchRule) + return r.stallRule(ctx, st, WatchRuleReasonGitDestinationInvalid, "Target.name must be specified") } - return r.reconcileWatchRuleViaTarget(ctx, &watchRule) + return r.reconcileWatchRuleViaTarget(ctx, st, &watchRule) } // reconcileWatchRuleViaTarget validates and stores a WatchRule that references a GitTarget. func (r *WatchRuleReconciler) reconcileWatchRuleViaTarget( ctx context.Context, + st *reconcileStatus, watchRule *configbutleraiv1alpha3.WatchRule, ) (ctrl.Result, error) { log := logf.FromContext(ctx).WithName("reconcileWatchRuleViaTarget") @@ -183,28 +161,16 @@ func (r *WatchRuleReconciler) reconcileWatchRuleViaTarget( log.Error(err, "Failed to get referenced GitTarget", "gitTargetName", watchRule.Spec.TargetRef.Name, "gitTargetNamespace", targetNS) - r.setCondition( - watchRule, - metav1.ConditionFalse, - WatchRuleReasonGitTargetNotFound, - fmt.Sprintf( - "Referenced GitTarget '%s/%s' not found: %v", - targetNS, - watchRule.Spec.TargetRef.Name, - err, - ), - ) - r.setTypedCondition( - watchRule, + st.set( ConditionTypeGitTargetReady, metav1.ConditionFalse, WatchRuleReasonGitTargetNotFound, - "Referenced GitTarget not found", + fmt.Sprintf("Referenced GitTarget '%s/%s' not found: %v", targetNS, watchRule.Spec.TargetRef.Name, err), ) - r.setRuleStalled(watchRule, WatchRuleReasonGitTargetNotFound, "Referenced GitTarget not found") - return r.updateStatusAndRequeue(ctx, watchRule) + return r.stallRule(ctx, st, WatchRuleReasonGitTargetNotFound, "Referenced GitTarget not found") } - r.setGitTargetReadyCondition(watchRule, target) + ready := gitTargetReadyCondition(target) + st.set(ConditionTypeGitTargetReady, ready.Status, ready.Reason, ready.Message) // Resolve the GitProvider named by the target. A GitProviderReference is a // name-only reference to a GitProvider in the GitTarget's own namespace. @@ -216,37 +182,20 @@ func (r *WatchRuleReconciler) reconcileWatchRuleViaTarget( if err := r.Get(ctx, providerKey, &provider); err != nil { log.Error(err, "Failed to resolve GitProvider from GitTarget", "gitProviderName", providerName, "gitProviderNamespace", providerNS) - r.setCondition( - watchRule, - metav1.ConditionFalse, - WatchRuleReasonGitProviderNotFound, // Reuse reason for now - fmt.Sprintf( - "GitProvider '%s/%s' (from GitTarget) not found: %v", - providerNS, - providerName, - err, - ), - ) - r.setTypedCondition( - watchRule, + st.set( ConditionTypeGitTargetReady, metav1.ConditionFalse, WatchRuleReasonGitProviderNotFound, - "Referenced GitProvider not found", + fmt.Sprintf("GitProvider '%s/%s' (from GitTarget) not found: %v", providerNS, providerName, err), ) - r.setRuleStalled(watchRule, WatchRuleReasonGitProviderNotFound, "Referenced GitProvider not found") - return r.updateStatusAndRequeue(ctx, watchRule) + return r.stallRule(ctx, st, WatchRuleReasonGitProviderNotFound, "Referenced GitProvider not found") } - // Ready check (GitProvider doesn't have status conditions yet in my implementation? I added them) - // I added GitProviderStatus with Conditions. - // TODO: Check GitProvider readiness. For now assume ready if found. - // Source-namespace gate AND compilation, in that order and in one call — see // gateSourceNamespace. There is deliberately no AddOrUpdateWatchRule here: routing every // compilation through watch.CompileWatchRule is what stops the startup bootstrap from being a // second, ungated path into the store. - if handled, result, err := r.gateSourceNamespace(ctx, watchRule, target, provider, log); handled { + if handled, result, err := r.gateSourceNamespace(ctx, st, watchRule, target, provider, log); handled { return result, err } @@ -256,59 +205,50 @@ func (r *WatchRuleReconciler) reconcileWatchRuleViaTarget( log.Error(err, "Failed to reconcile watch manager after rule update") // Don't fail the reconciliation - the rule is valid, just log the watch manager issue } - r.setResourceResolutionCondition(ctx, watchRule) - r.setStreamsReadyCondition(watchRule, r.WatchManager.StreamSummaryForWatchRule(*watchRule)) + r.setResourceResolutionCondition(ctx, st, watchRule) + r.setStreamsReadyCondition(st, watchRule, r.WatchManager.StreamSummaryForWatchRule(*watchRule)) } else { - r.setStreamsReadyCondition(watchRule, noResolvedStreamsSummary()) + r.setStreamsReadyCondition(st, watchRule, noResolvedStreamsSummary()) } log.Info("WatchRule reconciliation via GitTarget successful", "name", watchRule.Name) - return r.setReadyAndUpdateStatusWithTarget(ctx, watchRule, targetNS) -} -// setReadyAndUpdateStatusWithTarget sets Ready with target message and updates status with retry. -func (r *WatchRuleReconciler) setReadyAndUpdateStatusWithTarget( - ctx context.Context, - watchRule *configbutleraiv1alpha3.WatchRule, - targetNS string, -) (ctrl.Result, error) { msg := fmt.Sprintf( "WatchRule is ready and monitoring resources via GitTarget '%s/%s'", targetNS, watchRule.Spec.TargetRef.Name, ) - r.setRuleKstatus(watchRule, msg) - if err := r.updateStatusWithRetry(ctx, watchRule); err != nil { - return ctrl.Result{}, err - } - if conditionIsFalse(watchRule.Status.Conditions, ConditionTypeResourcesResolved) { - return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil - } - if !conditionIsTrue(watchRule.Status.Conditions, ConditionTypeGitTargetReady) { - return ctrl.Result{RequeueAfter: RequeueStreamSettleInterval}, nil - } - if !conditionIsTrue(watchRule.Status.Conditions, ConditionTypeStreamsRunning) { - return ctrl.Result{RequeueAfter: RequeueStreamSettleInterval}, nil - } - return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil + return r.commitRule(ctx, st, ruleReadiness(watchRule.Status.Conditions, "WatchRule", msg)) } -// setCondition sets or updates the Ready condition. -func (r *WatchRuleReconciler) setCondition( //nolint:lll // Function signature - watchRule *configbutleraiv1alpha3.WatchRule, status metav1.ConditionStatus, reason, message string) { - r.setTypedCondition(watchRule, ConditionTypeReady, status, reason, message) +// stallRule publishes a terminal WatchRule outcome and ends the reconcile. +func (r *WatchRuleReconciler) stallRule( + ctx context.Context, + st *reconcileStatus, + reason, message string, +) (ctrl.Result, error) { + rd := newRuleReadiness("WatchRule", "") + rd.stalled(reason, message) + return r.commitRule(ctx, st, rd) } -func (r *WatchRuleReconciler) setGitTargetReadyCondition( - watchRule *configbutleraiv1alpha3.WatchRule, - target configbutleraiv1alpha3.GitTarget, -) { - ready := gitTargetReadyCondition(target) - r.setTypedCondition(watchRule, ConditionTypeGitTargetReady, ready.Status, ready.Reason, ready.Message) +// commitRule writes the trio, persists the status, and picks the requeue cadence from the same +// verdict — so the cadence can never disagree with what status says. +func (r *WatchRuleReconciler) commitRule( + ctx context.Context, + st *reconcileStatus, + rd *readiness, +) (ctrl.Result, error) { + st.applyReadiness(rd) + if err := st.commit(ctx); err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{RequeueAfter: requeueFor(rd)}, nil } func (r *WatchRuleReconciler) setResourceResolutionCondition( ctx context.Context, + st *reconcileStatus, watchRule *configbutleraiv1alpha3.WatchRule, ) { resolved, message := r.WatchManager.ResolveWatchRuleResources(ctx, *watchRule) @@ -318,150 +258,27 @@ func (r *WatchRuleReconciler) setResourceResolutionCondition( status = metav1.ConditionTrue reason = WatchRuleReasonResourcesResolved } - r.setTypedCondition(watchRule, ConditionTypeResourcesResolved, status, reason, message) + st.set(ConditionTypeResourcesResolved, status, reason, message) } func (r *WatchRuleReconciler) setStreamsReadyCondition( + st *reconcileStatus, watchRule *configbutleraiv1alpha3.WatchRule, streams watch.StreamSummary, ) { watchRule.Status.Streams = watchRuleStreamsStatus(streams) - r.setTypedCondition( - watchRule, - ConditionTypeStreamsRunning, - streamConditionStatus(streams), - streams.Reason, - streams.Message, - ) -} - -func (r *WatchRuleReconciler) setRuleStalled( - watchRule *configbutleraiv1alpha3.WatchRule, - reason string, - message string, -) { - r.setTypedCondition(watchRule, ConditionTypeReady, metav1.ConditionFalse, reason, message) - r.setTypedCondition(watchRule, ConditionTypeReconciling, metav1.ConditionFalse, reason, "Reconciliation is stalled") - r.setTypedCondition(watchRule, ConditionTypeStalled, metav1.ConditionTrue, reason, message) -} - -func (r *WatchRuleReconciler) setRuleKstatus( - watchRule *configbutleraiv1alpha3.WatchRule, - readyMessage string, -) { - applyRuleKstatus( - watchRule.Status.Conditions, - readyMessage, - "WatchRule is not stalled", - func(conditionType string, status metav1.ConditionStatus, reason, message string) { - r.setTypedCondition(watchRule, conditionType, status, reason, message) - }, - func(reason, message string) { - r.setRuleStalled(watchRule, reason, message) - }, - ) -} - -func (r *WatchRuleReconciler) setTypedCondition( - watchRule *configbutleraiv1alpha3.WatchRule, - conditionType string, - status metav1.ConditionStatus, - reason string, - message string, -) { - watchRule.Status.Conditions = upsertCondition( - watchRule.Status.Conditions, - conditionType, - status, - reason, - message, - watchRule.Generation, - ) -} - -func conditionIsFalse(conditions []metav1.Condition, conditionType string) bool { - for _, condition := range conditions { - if condition.Type == conditionType { - return condition.Status == metav1.ConditionFalse - } - } - return false -} - -// updateStatusAndRequeue updates the status and requeues on the unified control-plane steady -// interval. The control plane no longer watches Secrets, so every status outcome falls back to -// this single cadence; see docs/rbac.md. -func (r *WatchRuleReconciler) updateStatusAndRequeue( - ctx context.Context, watchRule *configbutleraiv1alpha3.WatchRule) (ctrl.Result, error) { - if err := r.updateStatusWithRetry(ctx, watchRule); err != nil { - return ctrl.Result{}, err - } - return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil -} - -// updateStatusWithRetry updates the status with retry logic to handle race conditions -// -//nolint:dupl // Similar retry logic pattern used across controllers -func (r *WatchRuleReconciler) updateStatusWithRetry( - ctx context.Context, - watchRule *configbutleraiv1alpha3.WatchRule, -) error { - log := logf.FromContext(ctx).WithName("updateStatusWithRetry") - - log.Info("Starting status update with retry", - "name", watchRule.Name, - "namespace", watchRule.Namespace, - "conditionsCount", len(watchRule.Status.Conditions)) - - return wait.ExponentialBackoff(wait.Backoff{ - Duration: RetryInitialDuration, - Factor: RetryBackoffFactor, - Jitter: RetryBackoffJitter, - Steps: RetryMaxSteps, - }, func() (bool, error) { - log.Info("Attempting status update") - - // Get the latest version of the resource - latest := &configbutleraiv1alpha3.WatchRule{} - key := client.ObjectKeyFromObject(watchRule) - if err := r.Get(ctx, key, latest); err != nil { - if apierrors.IsNotFound(err) { - log.Info("Resource was deleted, nothing to update") - return true, nil - } - log.Error(err, "Failed to get latest resource version") - return false, err - } - - log.Info("Got latest resource version", - "generation", latest.Generation, - "resourceVersion", latest.ResourceVersion) - - // Copy our status to the latest version - latest.Status = watchRule.Status - - log.Info("Attempting to update status", - "conditionsCount", len(latest.Status.Conditions)) - - // Attempt to update - if err := r.Status().Update(ctx, latest); err != nil { - if apierrors.IsConflict(err) { - log.Info("Resource version conflict, retrying") - return false, nil - } - log.Error(err, "Failed to update status") - return false, err - } - - log.Info("Status update successful") - return true, nil - }) + st.set(ConditionTypeStreamsRunning, streamConditionStatus(streams), streams.Reason, streams.Message) } // SetupWithManager sets up the controller with the Manager. func (r *WatchRuleReconciler) SetupWithManager(mgr ctrl.Manager) error { b := ctrl.NewControllerManagedBy(mgr). - For(&configbutleraiv1alpha3.WatchRule{}). + // A For() predicate is not an optimisation here, it closes a self-triggering edge: a status + // write bumps resourceVersion and fires an Update watch event that EnqueueRequestForObject + // turns straight back into a queued request, un-rate-limited. reconcileStatus.commit() + // already suppresses no-op writes, so the loop has no fuel; this makes it structural, and + // matches what GitProvider and ClusterProvider already do. + For(&configbutleraiv1alpha3.WatchRule{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})). // GenerationChangedPredicate keeps these watches reacting to a freshly // applied or spec-changed dependency while ignoring the status-only // updates the controllers write themselves — without it every GitTarget diff --git a/internal/controller/watchrule_kstatus_test.go b/internal/controller/watchrule_kstatus_test.go index 3d2936d4..7399f50a 100644 --- a/internal/controller/watchrule_kstatus_test.go +++ b/internal/controller/watchrule_kstatus_test.go @@ -172,21 +172,10 @@ func TestApplyRuleKstatus_SourceAuthorizationIsAPrerequisite(t *testing.T) { conditions = append(conditions, *tt.sourceNS) } - got := map[string]metav1.ConditionStatus{} - applyRuleKstatus( - conditions, "ready", "not stalled", - func(conditionType string, status metav1.ConditionStatus, _, _ string) { - got[conditionType] = status - }, - func(string, string) { - got[ConditionTypeReady] = metav1.ConditionFalse - got[ConditionTypeReconciling] = metav1.ConditionFalse - got[ConditionTypeStalled] = metav1.ConditionTrue - }, - ) + trio := ruleReadiness(conditions, "WatchRule", "ready").trio() - assert.Equal(t, tt.wantReady, got[ConditionTypeReady], "Ready") - assert.Equal(t, tt.wantStalled, got[ConditionTypeStalled], "Stalled") + assert.Equal(t, tt.wantReady, trio.Ready.Status, "Ready") + assert.Equal(t, tt.wantStalled, trio.Stalled.Status, "Stalled") }) } } diff --git a/internal/controller/watchrule_source_namespace.go b/internal/controller/watchrule_source_namespace.go index 410dc3bd..8069c9be 100644 --- a/internal/controller/watchrule_source_namespace.go +++ b/internal/controller/watchrule_source_namespace.go @@ -57,6 +57,7 @@ const ( // unchanged. func (r *WatchRuleReconciler) gateSourceNamespace( ctx context.Context, + st *reconcileStatus, watchRule *configbutleraiv1alpha3.WatchRule, target configbutleraiv1alpha3.GitTarget, provider configbutleraiv1alpha3.GitProvider, @@ -74,8 +75,7 @@ func (r *WatchRuleReconciler) gateSourceNamespace( switch { case resolved.Admitted(): - r.setTypedCondition( - watchRule, + st.set( ConditionTypeSourceNamespaceAuthorized, metav1.ConditionTrue, resolved.Reason, @@ -84,7 +84,7 @@ func (r *WatchRuleReconciler) gateSourceNamespace( return false, ctrl.Result{}, nil case resolved.Terminal(): - result, refuseErr := r.refuseSourceNamespace(ctx, watchRule, resolved, log) + result, refuseErr := r.refuseSourceNamespace(ctx, st, watchRule, resolved, log) return true, result, refuseErr default: @@ -92,7 +92,7 @@ func (r *WatchRuleReconciler) gateSourceNamespace( // rule with an already-resolved scope is retaining it through an unevaluatable policy. In // every case this is PROGRESSING, not failed: turning a temporary connection problem into // a terminal Stalled=True would stop a stream over an outage nobody chose. - result, updateErr := r.holdSourceNamespaceUnknown(ctx, watchRule, resolved) + result, updateErr := r.holdSourceNamespaceUnknown(ctx, st, watchRule, resolved) return true, result, updateErr } } @@ -110,6 +110,7 @@ func (r *WatchRuleReconciler) gateSourceNamespace( // the mappers and channel registered in SetupWithManager. func (r *WatchRuleReconciler) refuseSourceNamespace( ctx context.Context, + st *reconcileStatus, watchRule *configbutleraiv1alpha3.WatchRule, resolved authz.ResolvedSourceScope, log logr.Logger, @@ -131,23 +132,20 @@ func (r *WatchRuleReconciler) refuseSourceNamespace( } } - r.setTypedCondition( - watchRule, + st.set( ConditionTypeSourceNamespaceAuthorized, metav1.ConditionFalse, resolved.Reason, resolved.Message, ) - r.setTypedCondition( - watchRule, + st.set( ConditionTypeStreamsRunning, metav1.ConditionFalse, resolved.Reason, "No streams: the rule's source-namespace scope is not authorized", ) - r.setRuleStalled(watchRule, resolved.Reason, resolved.Message) - return r.updateStatusAndRequeue(ctx, watchRule) + return r.stallRule(ctx, st, resolved.Reason, resolved.Message) } // holdSourceNamespaceUnknown publishes the "cannot say yet" state: SourceNamespaceAuthorized is @@ -160,31 +158,29 @@ func (r *WatchRuleReconciler) refuseSourceNamespace( // outage. func (r *WatchRuleReconciler) holdSourceNamespaceUnknown( ctx context.Context, + st *reconcileStatus, watchRule *configbutleraiv1alpha3.WatchRule, resolved authz.ResolvedSourceScope, ) (ctrl.Result, error) { - r.setTypedCondition( - watchRule, + st.set( ConditionTypeSourceNamespaceAuthorized, metav1.ConditionUnknown, resolved.Reason, resolved.Message, ) - r.setTypedCondition( - watchRule, + st.set( ConditionTypeStreamsRunning, metav1.ConditionUnknown, resolved.Reason, "Streams not re-evaluated while source-namespace authorization is unsettled", ) - r.setRuleKstatus(watchRule, "WatchRule source-namespace authorization is unsettled") - if err := r.updateStatusWithRetry(ctx, watchRule); err != nil { - return ctrl.Result{}, err - } - // Retry on the fast settle cadence: the answer usually arrives with the next source-cluster - // refresh, and the enqueue edge may not fire when nothing observably changed. - return ctrl.Result{RequeueAfter: RequeueStreamSettleInterval}, nil + // Progressing, never stalled — and on the fast settle cadence, because the answer usually + // arrives with the next source-cluster refresh and the enqueue edge may not fire when nothing + // observably changed. requeueFor gives that cadence to any non-converged verdict. + rd := ruleReadiness(watchRule.Status.Conditions, "WatchRule", + "WatchRule source-namespace authorization is unsettled") + return r.commitRule(ctx, st, rd) } // sourceScope returns the source-scope service, or nil when the data plane is not wired. A nil diff --git a/internal/watch/stream_readiness.go b/internal/watch/stream_readiness.go index d0564f4c..0336e10e 100644 --- a/internal/watch/stream_readiness.go +++ b/internal/watch/stream_readiness.go @@ -8,7 +8,6 @@ import ( "strings" "time" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" k8stypes "k8s.io/apimachinery/pkg/types" @@ -63,7 +62,6 @@ type StreamSummary struct { Reason string Message string PendingSample []string - ObservedTime metav1.Time } // Summary returns the display ratio stored in status.streams.summary. @@ -241,7 +239,7 @@ func streamSummaryCounts( byGVR map[schema.GroupVersionResource]targetStreamStatus, displayNames map[schema.GroupVersionResource]string, ) (StreamSummary, []string, []string) { - out := StreamSummary{Total: len(byGVR), ObservedTime: metav1.Now()} + out := StreamSummary{Total: len(byGVR)} var blockedNames, replayingNames []string for gvr, status := range byGVR { name := displayNames[gvr] diff --git a/test/e2e/aggregated_apiserver_e2e_test.go b/test/e2e/aggregated_apiserver_e2e_test.go index 66d5c881..352b56e2 100644 --- a/test/e2e/aggregated_apiserver_e2e_test.go +++ b/test/e2e/aggregated_apiserver_e2e_test.go @@ -72,7 +72,7 @@ var _ = Describe("Aggregated API server", Label("aggregated-api"), Ordered, func Expect(err).NotTo(HaveOccurred(), "failed to apply aggregated-api WatchRule") By("verifying the WatchRule reaches Ready=True") - verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Ready", "") + verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Succeeded", "") By("waiting for the flunder stream to be live before asserting per-event commits") waitForStreamsRunning(targetName, testNs) diff --git a/test/e2e/argocd_bi_directional_e2e_test.go b/test/e2e/argocd_bi_directional_e2e_test.go index fb8ce2d7..8937a763 100644 --- a/test/e2e/argocd_bi_directional_e2e_test.go +++ b/test/e2e/argocd_bi_directional_e2e_test.go @@ -570,7 +570,7 @@ func (r argoBiDirectionalRun) startReverserPipeline() { GinkgoHelper() createGitProviderWithURLInNamespace( r.gitProviderName, r.testNs, argoBiDirectionalRepo.GitSecretHTTP, r.repoURL) - verifyResourceStatus("gitprovider", r.gitProviderName, r.testNs, "True", "Ready", "") + verifyResourceStatus("gitprovider", r.gitProviderName, r.testNs, "True", "Succeeded", "") createGitTarget(r.gitTargetName, r.testNs, r.gitProviderName, r.livePath, "main") @@ -586,8 +586,8 @@ func (r argoBiDirectionalRun) startReverserPipeline() { Group: crdGroupArgoBiDirectional, }, r.testNs)).To(Succeed(), "failed to apply the IceCreamOrder WatchRule") - verifyResourceStatus("gittarget", r.gitTargetName, r.testNs, "True", "Ready", "") - verifyResourceStatus("watchrule", r.watchRuleName, r.testNs, "True", "Ready", "") + verifyResourceStatus("gittarget", r.gitTargetName, r.testNs, "True", "Succeeded", "") + verifyResourceStatus("watchrule", r.watchRuleName, r.testNs, "True", "Succeeded", "") // Gate on StreamsRunning so the IceCreamOrder watch is live before Argo CD // applies anything; otherwise the first live object could be folded into an diff --git a/test/e2e/audit_route_attribution_e2e_test.go b/test/e2e/audit_route_attribution_e2e_test.go index 0dca4676..e9f8949b 100644 --- a/test/e2e/audit_route_attribution_e2e_test.go +++ b/test/e2e/audit_route_attribution_e2e_test.go @@ -90,7 +90,7 @@ var _ = Describe("Audit route attribution", Label("manager"), Ordered, func() { Expect(applyGitTargetWithSourceNamespaces( testNs, gitTargetName, gitProvName, basePath, clusterProv, testNs, sourceNs)).Error(). NotTo(HaveOccurred(), "failed to apply the GitTarget") - verifyResourceCondition("gittarget", gitTargetName, testNs, "Validated", "True", "OK", "") + verifyResourceCondition("gittarget", gitTargetName, testNs, "Validated", "True", "Succeeded", "") }) AfterAll(func() { @@ -142,7 +142,7 @@ var _ = Describe("Audit route attribution", Label("manager"), Ordered, func() { } Expect(applyFromTemplate("test/e2e/templates/manager/watchrule-configmap.tmpl", data, testNs)). To(Succeed(), "failed to apply the WatchRule") - verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Ready", "") + verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Succeeded", "") waitForStreamsRunning(gitTargetName, testNs) assertCommitAuthor(testNs, fmt.Sprintf("audit-route-cm-%d", GinkgoRandomSeed()), diff --git a/test/e2e/commit_author_attribution_e2e_test.go b/test/e2e/commit_author_attribution_e2e_test.go index 0789f416..bb232c18 100644 --- a/test/e2e/commit_author_attribution_e2e_test.go +++ b/test/e2e/commit_author_attribution_e2e_test.go @@ -82,7 +82,7 @@ var _ = Describe("Commit Author Attribution", Label("manager"), Ordered, func() } err = applyFromTemplate("test/e2e/templates/manager/watchrule-configmap.tmpl", data, testNs) Expect(err).NotTo(HaveOccurred(), "failed to apply WatchRule") - verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Ready", "") + verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Succeeded", "") // Authorship only flows through the per-event audit tail. A ConfigMap created // while the configmaps type is still building its first checkpoint would land in diff --git a/test/e2e/commit_request_e2e_test.go b/test/e2e/commit_request_e2e_test.go index 5823fbde..32833aa0 100644 --- a/test/e2e/commit_request_e2e_test.go +++ b/test/e2e/commit_request_e2e_test.go @@ -67,7 +67,7 @@ var _ = Describe("Commit Request", Label("commit-request", "audit-consumer"), Or // would match — its initial reconcile would establish main before the spec's own edit, hiding // the pure "branch not even created until something is finalized" behaviour this suite tests. applyDeploymentWatchRule(testNs, watchRuleName, gitTargetName) - verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Ready", "") + verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Succeeded", "") // Gate on StreamsRunning=True before any spec creates a Deployment: under watch-first, "Ready" only // means the watch set is reconciled, not that each watch has opened and finished its @@ -317,7 +317,7 @@ var _ = Describe("Commit Request Bundle (UC2)", Label("commit-request", "audit-c // main stays absent until the bundle is finalized — unlike a ConfigMap rule, // which would mirror the pre-existing kube-root-ca.crt and establish main early. applyDeploymentWatchRule(testNs, watchRuleName, gitTargetName) - verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Ready", "") + verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Succeeded", "") // Gate on StreamsRunning=True before any spec creates a Deployment: under watch-first, "Ready" only // means the watch set is reconciled, not that each watch has opened and finished its diff --git a/test/e2e/commit_window_batching_e2e_test.go b/test/e2e/commit_window_batching_e2e_test.go index 036029fb..6360610e 100644 --- a/test/e2e/commit_window_batching_e2e_test.go +++ b/test/e2e/commit_window_batching_e2e_test.go @@ -79,7 +79,7 @@ var _ = Describe("Commit Window Batching", } err = applyFromTemplate("test/e2e/templates/manager/watchrule-configmap.tmpl", watchRuleData, testNs) Expect(err).NotTo(HaveOccurred(), "failed to apply WatchRule") - verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Ready", "") + verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Succeeded", "") By("waiting for the configmaps stream to be live so the seed and burst are live events") waitForStreamsRunning(gitTargetName, testNs) diff --git a/test/e2e/crd_lifecycle_e2e_test.go b/test/e2e/crd_lifecycle_e2e_test.go index d575f8df..f4f958e8 100644 --- a/test/e2e/crd_lifecycle_e2e_test.go +++ b/test/e2e/crd_lifecycle_e2e_test.go @@ -109,8 +109,8 @@ var _ = Describe("Manager CRD Lifecycle", Label("manager"), Serial, Ordered, fun Expect(err).NotTo(HaveOccurred(), "Failed to apply ClusterWatchRule for CRDs") By("verifying ClusterWatchRule is ready") - verifyResourceStatus("clusterwatchrule", clusterWatchRuleName, "", "True", "Ready", "") - verifyResourceCondition("gittarget", destName, testNs, "Validated", "True", "OK", "") + verifyResourceStatus("clusterwatchrule", clusterWatchRuleName, "", "True", "Succeeded", "") + verifyResourceCondition("gittarget", destName, testNs, "Validated", "True", "Succeeded", "") By("installing the IceCreamOrder CRD to trigger Git commit") err = applyIceCreamCRD(crdGroupCRDLifecycle) @@ -218,8 +218,8 @@ var _ = Describe("Manager CRD Lifecycle", Label("manager"), Serial, Ordered, fun Expect(err2).NotTo(HaveOccurred(), "Failed to apply WatchRule for CRDs") By("verifying WatchRule is ready") - verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Ready", "") - verifyResourceCondition("gittarget", destName, testNs, "Validated", "True", "OK", "") + verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Succeeded", "") + verifyResourceCondition("gittarget", destName, testNs, "Validated", "True", "Succeeded", "") // Gate on StreamsRunning so this and the later modify/delete specs (which share this Ordered // GitTarget) act on a live stream — the [DELETE] commit assertion downstream depends on it. @@ -618,7 +618,7 @@ var _ = Describe("Manager CRD Lifecycle", Label("manager"), Serial, Ordered, fun By("verifying ClusterWatchRule is ready") - verifyResourceStatus("clusterwatchrule", clusterWatchRuleName, "", "True", "Ready", "") + verifyResourceStatus("clusterwatchrule", clusterWatchRuleName, "", "True", "Succeeded", "") By("waiting for the CRD stream to be live so the deletion is a live [DELETE] event") waitForStreamsRunning(destName, testNs) diff --git a/test/e2e/deletecollection_intent_e2e_test.go b/test/e2e/deletecollection_intent_e2e_test.go index b8d915c1..a3a2eecb 100644 --- a/test/e2e/deletecollection_intent_e2e_test.go +++ b/test/e2e/deletecollection_intent_e2e_test.go @@ -90,7 +90,7 @@ var _ = Describe("DeleteCollection intent & attribution", Label("manager"), Orde }{Name: watchRuleName, Namespace: testNs, DestinationName: gitTargetName} err = applyFromTemplate("test/e2e/templates/manager/watchrule-configmap.tmpl", data, testNs) Expect(err).NotTo(HaveOccurred(), "failed to apply WatchRule") - verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Ready", "") + verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Succeeded", "") // Attribution flows only once the configmaps type is live; a write made while // the type is still building its first checkpoint would land unattributed. diff --git a/test/e2e/demo_e2e_test.go b/test/e2e/demo_e2e_test.go index 9eaf0a05..acb57ece 100644 --- a/test/e2e/demo_e2e_test.go +++ b/test/e2e/demo_e2e_test.go @@ -279,10 +279,10 @@ func (r *demoRun) removeLegacyDemoResources() { } func (r *demoRun) verifyCoffeeConfigReverseResourcesReady() { - verifyResourceStatus("gitprovider", demoCoffeeConfigGitProviderName, voterTestNamespace, "True", "Ready", "") + verifyResourceStatus("gitprovider", demoCoffeeConfigGitProviderName, voterTestNamespace, "True", "Succeeded", "") verifyResourceCondition( - "gittarget", demoCoffeeConfigGitProviderName, voterTestNamespace, "Validated", "True", "OK", "") - verifyResourceStatus("watchrule", demoCoffeeConfigGitProviderName, voterTestNamespace, "True", "Ready", "") + "gittarget", demoCoffeeConfigGitProviderName, voterTestNamespace, "Validated", "True", "Succeeded", "") + verifyResourceStatus("watchrule", demoCoffeeConfigGitProviderName, voterTestNamespace, "True", "Succeeded", "") } func (r *demoRun) touchCoffeeConfig() { diff --git a/test/e2e/deployment_scale_author_attribution_e2e_test.go b/test/e2e/deployment_scale_author_attribution_e2e_test.go index b6947395..d1b657ef 100644 --- a/test/e2e/deployment_scale_author_attribution_e2e_test.go +++ b/test/e2e/deployment_scale_author_attribution_e2e_test.go @@ -68,7 +68,7 @@ var _ = Describe("Deployment scale author attribution", Label("manager", "subres createReadyGitProvider(providerName, testNs, repo.GitSecretHTTP, repo.RepoURLHTTP) createValidatedGitTarget(targetName, testNs, providerName, targetPath) applyDeploymentWatchRule(testNs, watchRuleName, targetName) - verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Ready", "") + verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Succeeded", "") // Authorship only flows through the per-event audit tail. A scale issued // before the live stream is running would land in the unattributed baseline diff --git a/test/e2e/deployment_scale_subresource_e2e_test.go b/test/e2e/deployment_scale_subresource_e2e_test.go index 979cf85b..ba24ee25 100644 --- a/test/e2e/deployment_scale_subresource_e2e_test.go +++ b/test/e2e/deployment_scale_subresource_e2e_test.go @@ -40,7 +40,7 @@ var _ = Describe("Deployment scale subresource", Label("manager", "subresource") createReadyGitProvider(providerName, testNs, repo.GitSecretHTTP, repo.RepoURLHTTP) createValidatedGitTarget(targetName, testNs, providerName, targetPath) applyDeploymentWatchRule(testNs, watchRuleName, targetName) - verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Ready", "") + verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Succeeded", "") By("creating a Deployment with replicas=1") applyScaleTestDeployment(testNs, deploymentName, 1) diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index a694163e..015bc7d2 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -232,12 +232,8 @@ func verifyResourceCondition( } g.Expect(conditionStatus).To(Equal(expectedStatus)) - switch { - case expectedReason == "": - // Status-only gate: caller does not assert a reason. - case resourceType == "gittarget" && conditionType == "Ready" && expectedReason == "Ready": - g.Expect([]string{"Ready", "OK"}).To(ContainElement(conditionReason)) - default: + // An empty expectedReason is a status-only gate: the caller does not assert a reason. + if expectedReason != "" { g.Expect(conditionReason).To(Equal(expectedReason)) } if expectedMessageContains != "" { @@ -265,14 +261,14 @@ func waitForWatchRuleStreamsRunning(name, ns string) { //nolint:unused // Helper func createReadyGitProvider(name, ns, secretName, repoURL string) { GinkgoHelper() createGitProviderWithURLInNamespace(name, ns, secretName, repoURL) - verifyResourceStatus("gitprovider", name, ns, "True", "Ready", "") + verifyResourceStatus("gitprovider", name, ns, "True", "Succeeded", "") } // createReadyGitProviderWithCommitWindow is createReadyGitProvider with an explicit commit window. func createReadyGitProviderWithCommitWindow(name, ns, secretName, repoURL, commitWindow string) { GinkgoHelper() createGitProviderWithCommitWindow(name, ns, secretName, repoURL, commitWindow) - verifyResourceStatus("gitprovider", name, ns, "True", "Ready", "") + verifyResourceStatus("gitprovider", name, ns, "True", "Succeeded", "") } // createValidatedGitTarget creates a GitTarget on the "main" branch and blocks until it accepts @@ -282,7 +278,7 @@ func createReadyGitProviderWithCommitWindow(name, ns, secretName, repoURL, commi func createValidatedGitTarget(name, ns, providerName, path string) { GinkgoHelper() createGitTarget(name, ns, providerName, path, "main") - verifyResourceCondition("gittarget", name, ns, "Validated", "True", "OK", "") + verifyResourceCondition("gittarget", name, ns, "Validated", "True", "Succeeded", "") } // cleanupPipeline deletes the three namespaced pipeline resources in dependency order (rule, then diff --git a/test/e2e/flux_bi_directional_e2e_test.go b/test/e2e/flux_bi_directional_e2e_test.go index 3140a3ad..1cbe0f96 100644 --- a/test/e2e/flux_bi_directional_e2e_test.go +++ b/test/e2e/flux_bi_directional_e2e_test.go @@ -175,12 +175,12 @@ var _ = Describe("Bi Directional (Flux)", Label("bi-directional", "flux"), Order Group: crdGroupBiDirectional, }, testNs) Expect(err).NotTo(HaveOccurred(), "failed to apply bi-directional WatchRule") - verifyResourceStatus("gitprovider", run.gitProviderName, testNs, "True", "Ready", "") + verifyResourceStatus("gitprovider", run.gitProviderName, testNs, "True", "Succeeded", "") run.waitForControllerEncryptionSecret() run.applyFluxDecryptionSecretFromControllerSecret() run.applyFluxLiveKustomization() run.waitForGitTargetReady() - verifyResourceStatus("watchrule", run.watchRuleName, testNs, "True", "Ready", "") + verifyResourceStatus("watchrule", run.watchRuleName, testNs, "True", "Succeeded", "") // Gate on StreamsRunning so the IceCreamOrder watch is live before the API-driven patch below: // the exact commit-count assertions depend on that change arriving as one live per-event @@ -263,7 +263,7 @@ var _ = Describe("Bi Directional (Flux)", Label("bi-directional", "flux"), Order DestinationName: run.gitTargetName, }, testNs) Expect(err).NotTo(HaveOccurred(), "failed to apply Secret WatchRule") - verifyResourceStatus("watchrule", run.secretWatchRuleName, testNs, "True", "Ready", "") + verifyResourceStatus("watchrule", run.secretWatchRuleName, testNs, "True", "Succeeded", "") _, _ = kubectlRunInNamespace(testNs, "delete", "secret", run.reverseSecretName, "--ignore-not-found=true") _, err = kubectlRunInNamespace( @@ -709,8 +709,7 @@ func (r biDirectionalRun) waitForOrderSpec(name, container, flavor, topping stri func (r biDirectionalRun) waitForGitTargetReady() { GinkgoHelper() - // The gittarget Ready special-case in verifyResourceCondition accepts reason "Ready" or "OK". - verifyResourceStatus("gittarget", r.gitTargetName, r.testNs, "True", "Ready", "") + verifyResourceStatus("gittarget", r.gitTargetName, r.testNs, "True", "Succeeded", "") } func (r biDirectionalRun) waitForControllerEncryptionSecret() { diff --git a/test/e2e/foreign_content_e2e_test.go b/test/e2e/foreign_content_e2e_test.go index 27c4e267..7ead1b77 100644 --- a/test/e2e/foreign_content_e2e_test.go +++ b/test/e2e/foreign_content_e2e_test.go @@ -51,7 +51,7 @@ var _ = Describe("Manager Foreign Content Refusal", Label("manager", "foreign-co By("creating the GitProvider") createGitProviderWithURLInNamespace(providerName, testNs, repo.GitSecretHTTP, repo.RepoURLHTTP) - verifyResourceStatus("gitprovider", providerName, testNs, "True", "Ready", "") + verifyResourceStatus("gitprovider", providerName, testNs, "True", "Succeeded", "") }) AfterAll(func() { diff --git a/test/e2e/gittarget_isolation_e2e_test.go b/test/e2e/gittarget_isolation_e2e_test.go index 5e19515c..94426060 100644 --- a/test/e2e/gittarget_isolation_e2e_test.go +++ b/test/e2e/gittarget_isolation_e2e_test.go @@ -69,14 +69,14 @@ var _ = Describe("Manager GitTarget Isolation", Label("manager"), Ordered, func( By("creating two independent GitTargets writing to separate paths in the same repo") createGitTarget(targetA, testNs, providerName, pathA, "main") createGitTarget(targetB, testNs, providerName, pathB, "main") - verifyResourceCondition("gittarget", targetA, testNs, "Validated", "True", "OK", "") - verifyResourceCondition("gittarget", targetB, testNs, "Validated", "True", "OK", "") + verifyResourceCondition("gittarget", targetA, testNs, "Validated", "True", "Succeeded", "") + verifyResourceCondition("gittarget", targetB, testNs, "Validated", "True", "Succeeded", "") By("target A and target B both watch ConfigMaps (baseline steady state)") applyIsolationWatchRule(ruleA, testNs, targetA, `"configmaps"`) applyIsolationWatchRule(ruleB, testNs, targetB, `"configmaps"`) - verifyResourceStatus("watchrule", ruleA, testNs, "True", "Ready", "") - verifyResourceStatus("watchrule", ruleB, testNs, "True", "Ready", "") + verifyResourceStatus("watchrule", ruleA, testNs, "True", "Succeeded", "") + verifyResourceStatus("watchrule", ruleB, testNs, "True", "Succeeded", "") By("waiting for both targets' configmaps streams to be live before any event is created") waitForStreamsRunning(targetA, testNs) diff --git a/test/e2e/gittarget_overlap_e2e_test.go b/test/e2e/gittarget_overlap_e2e_test.go index 3bc52be0..5515b519 100644 --- a/test/e2e/gittarget_overlap_e2e_test.go +++ b/test/e2e/gittarget_overlap_e2e_test.go @@ -67,8 +67,8 @@ var _ = Describe("Manager GitTarget Overlap Guard", Label("manager"), Ordered, f createGitTarget("overlap-sibling-a", testNs, providerName, "overlap/team-a", "main") createGitTarget("overlap-sibling-b", testNs, providerName, "overlap/team-b", "main") - verifyResourceCondition("gittarget", "overlap-sibling-a", testNs, "Validated", "True", "OK", "") - verifyResourceCondition("gittarget", "overlap-sibling-b", testNs, "Validated", "True", "OK", "") + verifyResourceCondition("gittarget", "overlap-sibling-a", testNs, "Validated", "True", "Succeeded", "") + verifyResourceCondition("gittarget", "overlap-sibling-b", testNs, "Validated", "True", "Succeeded", "") }) It("rejects a path nested inside an existing target's path", func() { @@ -78,7 +78,7 @@ var _ = Describe("Manager GitTarget Overlap Guard", Label("manager"), Ordered, f // same-second tie. That keeps this spec from flaking. By("creating the parent target first so it wins the overlap election") createGitTarget("overlap-parent", testNs, providerName, "overlap/nested", "main") - verifyResourceCondition("gittarget", "overlap-parent", testNs, "Validated", "True", "OK", "") + verifyResourceCondition("gittarget", "overlap-parent", testNs, "Validated", "True", "Succeeded", "") By("creating a target nested under the parent (created later, must lose)") createGitTarget("overlap-parent-child", testNs, providerName, "overlap/nested/child", "main") diff --git a/test/e2e/helmrelease_mirror_edit_e2e_test.go b/test/e2e/helmrelease_mirror_edit_e2e_test.go index eef00fff..637ada11 100644 --- a/test/e2e/helmrelease_mirror_edit_e2e_test.go +++ b/test/e2e/helmrelease_mirror_edit_e2e_test.go @@ -61,7 +61,7 @@ var _ = Describe("Manager Higher-Level KRM (HelmRelease)", By("creating the GitProvider") createGitProviderWithURLInNamespace(providerName, testNs, repo.GitSecretHTTP, repo.RepoURLHTTP) - verifyResourceStatus("gitprovider", providerName, testNs, "True", "Ready", "") + verifyResourceStatus("gitprovider", providerName, testNs, "True", "Succeeded", "") }) AfterAll(func() { @@ -81,8 +81,8 @@ var _ = Describe("Manager Higher-Level KRM (HelmRelease)", DestinationName string }{Name: ruleName, Namespace: testNs, DestinationName: destName}, testNs) Expect(err).NotTo(HaveOccurred(), "failed to apply HelmRelease WatchRule") - verifyResourceCondition("gittarget", destName, testNs, "Validated", "True", "OK", "") - verifyResourceStatus("watchrule", ruleName, testNs, "True", "Ready", "") + verifyResourceCondition("gittarget", destName, testNs, "Validated", "True", "Succeeded", "") + verifyResourceStatus("watchrule", ruleName, testNs, "True", "Succeeded", "") waitForStreamsRunning(destName, testNs) By(fmt.Sprintf("creating the HelmRelease with chart version %s", initialVersion)) diff --git a/test/e2e/inplace_edit_e2e_test.go b/test/e2e/inplace_edit_e2e_test.go index 1e9e8274..8637f454 100644 --- a/test/e2e/inplace_edit_e2e_test.go +++ b/test/e2e/inplace_edit_e2e_test.go @@ -55,7 +55,7 @@ var _ = Describe("Manager In-Place Manifest Editing", Label("manager", "inplace- By("creating the GitProvider") createGitProviderWithURLInNamespace("inplace-edit-provider", testNs, repo.GitSecretHTTP, repo.RepoURLHTTP) - verifyResourceStatus("gitprovider", "inplace-edit-provider", testNs, "True", "Ready", "") + verifyResourceStatus("gitprovider", "inplace-edit-provider", testNs, "True", "Succeeded", "") }) AfterAll(func() { @@ -74,8 +74,8 @@ var _ = Describe("Manager In-Place Manifest Editing", Label("manager", "inplace- DestinationName string }{Name: ruleName, Namespace: testNs, DestinationName: destName}, testNs) Expect(err).NotTo(HaveOccurred(), "failed to apply ConfigMap WatchRule") - verifyResourceCondition("gittarget", destName, testNs, "Validated", "True", "OK", "") - verifyResourceStatus("watchrule", ruleName, testNs, "True", "Ready", "") + verifyResourceCondition("gittarget", destName, testNs, "Validated", "True", "Succeeded", "") + verifyResourceStatus("watchrule", ruleName, testNs, "True", "Succeeded", "") waitForStreamsRunning(destName, testNs) By("creating the ConfigMap with color=blue") @@ -179,7 +179,7 @@ var _ = Describe( By("creating the GitProvider") createGitProviderWithURLInNamespace(providerName, testNs, repo.GitSecretHTTP, repo.RepoURLHTTP) - verifyResourceStatus("gitprovider", providerName, testNs, "True", "Ready", "") + verifyResourceStatus("gitprovider", providerName, testNs, "True", "Succeeded", "") }) AfterAll(func() { @@ -211,8 +211,8 @@ var _ = Describe( DestinationName string }{Name: ruleName, Namespace: testNs, DestinationName: destName}, testNs) Expect(err).NotTo(HaveOccurred(), "failed to apply ConfigMap WatchRule") - verifyResourceCondition("gittarget", destName, testNs, "Validated", "True", "OK", "") - verifyResourceStatus("watchrule", ruleName, testNs, "True", "Ready", "") + verifyResourceCondition("gittarget", destName, testNs, "Validated", "True", "Succeeded", "") + verifyResourceStatus("watchrule", ruleName, testNs, "True", "Succeeded", "") waitForStreamsRunning(destName, testNs) By("patching ConfigMaps that live in a multi-document file and a nested folder") diff --git a/test/e2e/new_file_placement_e2e_test.go b/test/e2e/new_file_placement_e2e_test.go index 3c745fbe..a2683fa1 100644 --- a/test/e2e/new_file_placement_e2e_test.go +++ b/test/e2e/new_file_placement_e2e_test.go @@ -51,7 +51,7 @@ var _ = Describe("Manager New-File Placement", Label("manager", "new-file-placem By("creating the GitProvider") createGitProviderWithURLInNamespace(providerName, testNs, repo.GitSecretHTTP, repo.RepoURLHTTP) - verifyResourceStatus("gitprovider", providerName, testNs, "True", "Ready", "") + verifyResourceStatus("gitprovider", providerName, testNs, "True", "Succeeded", "") }) AfterAll(func() { @@ -81,8 +81,8 @@ var _ = Describe("Manager New-File Placement", Label("manager", "new-file-placem DestinationName string }{Name: ruleName, Namespace: testNs, DestinationName: destName}, testNs) Expect(err).NotTo(HaveOccurred(), "failed to apply ConfigMap WatchRule") - verifyResourceCondition("gittarget", destName, testNs, "Validated", "True", "OK", "") - verifyResourceStatus("watchrule", ruleName, testNs, "True", "Ready", "") + verifyResourceCondition("gittarget", destName, testNs, "Validated", "True", "Succeeded", "") + verifyResourceStatus("watchrule", ruleName, testNs, "True", "Succeeded", "") waitForStreamsRunning(destName, testNs) By("creating a brand-new ConfigMap with no existing document in Git") diff --git a/test/e2e/prune_mode_e2e_test.go b/test/e2e/prune_mode_e2e_test.go index aa223b40..c1a7ea2d 100644 --- a/test/e2e/prune_mode_e2e_test.go +++ b/test/e2e/prune_mode_e2e_test.go @@ -76,7 +76,7 @@ var _ = Describe("Manager GitTarget prune policy", Label("manager"), Ordered, fu applyPruneGitTarget(neverTarget, testNs, providerName, neverPath, "Never") applyPruneGitTarget(alwaysTarget, testNs, providerName, alwaysPath, "Always") for _, name := range []string{defaultTarget, neverTarget, alwaysTarget} { - verifyResourceCondition("gittarget", name, testNs, "Validated", "True", "OK", "") + verifyResourceCondition("gittarget", name, testNs, "Validated", "True", "Succeeded", "") } By("each target watches ConfigMaps in this namespace") @@ -84,7 +84,7 @@ var _ = Describe("Manager GitTarget prune policy", Label("manager"), Ordered, fu applyIsolationWatchRule(neverRule, testNs, neverTarget, `"configmaps"`) applyIsolationWatchRule(alwaysRule, testNs, alwaysTarget, `"configmaps"`) for _, name := range []string{defaultRule, neverRule, alwaysRule} { - verifyResourceStatus("watchrule", name, testNs, "True", "Ready", "") + verifyResourceStatus("watchrule", name, testNs, "True", "Succeeded", "") } By("waiting for every target's ConfigMap stream to be live before any event is created") diff --git a/test/e2e/quickstart_framework_e2e_test.go b/test/e2e/quickstart_framework_e2e_test.go index e0bf895d..1561867f 100644 --- a/test/e2e/quickstart_framework_e2e_test.go +++ b/test/e2e/quickstart_framework_e2e_test.go @@ -111,9 +111,9 @@ func (r *quickstartFrameworkRun) runReadmeQuickstartFlow() { r.verifyNoRedisConfiguredAuthor() By("verifying the chart's starter resources become Ready") - verifyResourceStatus("gitprovider", "example-provider", r.namespace, "True", "Ready", "") - verifyResourceCondition("gittarget", "example-target", r.namespace, "Validated", "True", "OK", "") - verifyResourceStatus("watchrule", "example-watchrule", r.namespace, "True", "Ready", "") + verifyResourceStatus("gitprovider", "example-provider", r.namespace, "True", "Succeeded", "") + verifyResourceCondition("gittarget", "example-target", r.namespace, "Validated", "True", "Succeeded", "") + verifyResourceStatus("watchrule", "example-watchrule", r.namespace, "True", "Succeeded", "") waitForStreamsRunning("example-target", r.namespace) By("verifying the starter GitTarget generated its SOPS age key") @@ -349,9 +349,9 @@ func (r *quickstartFrameworkRun) applyQuickstartResources() { func (r *quickstartFrameworkRun) verifyQuickstartResourcesReady() { ns := r.namespace - verifyResourceStatus("gitprovider", r.providerName, ns, "True", "Ready", "") - verifyResourceCondition("gittarget", r.targetName, ns, "Validated", "True", "OK", "") - verifyResourceStatus("watchrule", r.watchRuleName, ns, "True", "Ready", "") + verifyResourceStatus("gitprovider", r.providerName, ns, "True", "Succeeded", "") + verifyResourceCondition("gittarget", r.targetName, ns, "Validated", "True", "Succeeded", "") + verifyResourceStatus("watchrule", r.watchRuleName, ns, "True", "Succeeded", "") waitForStreamsRunning(r.targetName, ns) } diff --git a/test/e2e/restart_reconcile_e2e_test.go b/test/e2e/restart_reconcile_e2e_test.go index f7992bc8..f8c53230 100644 --- a/test/e2e/restart_reconcile_e2e_test.go +++ b/test/e2e/restart_reconcile_e2e_test.go @@ -112,8 +112,8 @@ var _ = Describe("Restart Reconcile Safety", Label("restart-reconcile"), Serial, "test/e2e/templates/restart/watchrule-wildcard.tmpl", wrData, testNs, )).To(Succeed(), "failed to apply wildcard WatchRule") - verifyResourceCondition("gittarget", gitTargetName, testNs, "Validated", "True", "OK", "") - verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Ready", "") + verifyResourceCondition("gittarget", gitTargetName, testNs, "Validated", "True", "Succeeded", "") + verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Succeeded", "") By("creating quiet IceCreamOrder resources to build up the git mirror") for _, name := range orderNames { diff --git a/test/e2e/signing_e2e_test.go b/test/e2e/signing_e2e_test.go index c71aba78..e4c31e8f 100644 --- a/test/e2e/signing_e2e_test.go +++ b/test/e2e/signing_e2e_test.go @@ -118,7 +118,7 @@ var _ = Describe("Commit Signing", Label("signing"), Ordered, func() { g.Expect(strings.TrimSpace(output)).To(HavePrefix("ssh-"), "signingPublicKey should be populated") signingPublicKey = strings.TrimSpace(output) }).Should(Succeed()) - verifyResourceStatus("gitprovider", providerName, testNs, "True", "Ready", "") + verifyResourceStatus("gitprovider", providerName, testNs, "True", "Succeeded", "") By("registering the generated signing public key with Gitea") registered, err := gitea.RegisterSigningPublicKey(signingRepo.User, signingPublicKey, @@ -146,7 +146,7 @@ var _ = Describe("Commit Signing", Label("signing"), Ordered, func() { DestinationName string }{watchRuleName, testNs, destName} Expect(applyFromTemplate("test/e2e/templates/watchrule.tmpl", watchRuleData, testNs)).To(Succeed()) - verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Ready", "") + verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Succeeded", "") waitForStreamsRunning(destName, testNs) By("triggering a per-event commit") @@ -246,7 +246,7 @@ var _ = Describe("Commit Signing", Label("signing"), Ordered, func() { Expect(applyFromTemplate("test/e2e/templates/gitprovider-signing.tmpl", data, testNs)).To(Succeed()) By("waiting for GitProvider to become Ready and to surface the BYOK public key") - verifyResourceStatus("gitprovider", providerName, testNs, "True", "Ready", "") + verifyResourceStatus("gitprovider", providerName, testNs, "True", "Succeeded", "") var statusKey string Eventually(func(g Gomega) { output, err := kubectlRunInNamespace(testNs, "get", "gitprovider", providerName, @@ -269,7 +269,7 @@ var _ = Describe("Commit Signing", Label("signing"), Ordered, func() { DestinationName string }{watchRuleName, testNs, destName} Expect(applyFromTemplate("test/e2e/templates/watchrule.tmpl", watchRuleData, testNs)).To(Succeed()) - verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Ready", "") + verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Succeeded", "") waitForStreamsRunning(destName, testNs) By("triggering a per-event commit") @@ -340,7 +340,7 @@ var _ = Describe("Commit Signing", Label("signing"), Ordered, func() { GenerateWhenMissing: true, } Expect(applyFromTemplate("test/e2e/templates/gitprovider-signing.tmpl", data, testNs)).To(Succeed()) - verifyResourceStatus("gitprovider", providerName, testNs, "True", "Ready", "") + verifyResourceStatus("gitprovider", providerName, testNs, "True", "Succeeded", "") createValidatedGitTarget(destName, testNs, providerName, commitPath) @@ -350,7 +350,7 @@ var _ = Describe("Commit Signing", Label("signing"), Ordered, func() { DestinationName string }{watchRuleName, testNs, destName} Expect(applyFromTemplate("test/e2e/templates/watchrule.tmpl", watchRuleData, testNs)).To(Succeed()) - verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Ready", "") + verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Succeeded", "") waitForStreamsRunning(destName, testNs) _, err := kubectlRunInNamespace(testNs, "create", "configmap", cmName, "--from-literal=key=template-test") @@ -445,7 +445,7 @@ var _ = Describe("Commit Signing", Label("signing"), Ordered, func() { GenerateWhenMissing: true, } Expect(applyFromTemplate("test/e2e/templates/gitprovider-signing.tmpl", data, testNs)).To(Succeed()) - verifyResourceStatus("gitprovider", providerName, testNs, "True", "Ready", "") + verifyResourceStatus("gitprovider", providerName, testNs, "True", "Succeeded", "") createValidatedGitTarget(destName, testNs, providerName, commitPath) @@ -455,7 +455,7 @@ var _ = Describe("Commit Signing", Label("signing"), Ordered, func() { DestinationName string }{watchRuleName, testNs, destName} Expect(applyFromTemplate("test/e2e/templates/watchrule.tmpl", watchRuleData, testNs)).To(Succeed()) - verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Ready", "") + verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Succeeded", "") By("recreating the GitTarget now that the WatchRule is active to force a fresh reconcile batch") cleanupGitTarget(destName, testNs) @@ -558,14 +558,14 @@ var _ = Describe("Commit Signing", Label("signing"), Ordered, func() { SigningSecretName: "signing-key-overlap", GenerateWhenMissing: true, }, testNs)).To(Succeed()) - verifyResourceStatus("gitprovider", providerName, testNs, "True", "Ready", "") + verifyResourceStatus("gitprovider", providerName, testNs, "True", "Succeeded", "") By("creating target A and its WatchRule, then waiting for A to reconcile the seed band") createValidatedGitTarget(destNameA, testNs, providerName, commitPathA) Expect(applyFromTemplate("test/e2e/templates/watchrule.tmpl", struct { Name, Namespace, DestinationName string }{watchRuleNameA, testNs, destNameA}, testNs)).To(Succeed()) - verifyResourceStatus("watchrule", watchRuleNameA, testNs, "True", "Ready", "") + verifyResourceStatus("watchrule", watchRuleNameA, testNs, "True", "Succeeded", "") waitForStreamsRunning(destNameA, testNs) By("proving the shared ConfigMap tail is live for A via a probe per-event commit") @@ -590,8 +590,8 @@ var _ = Describe("Commit Signing", Label("signing"), Ordered, func() { Expect(applyFromTemplate("test/e2e/templates/watchrule.tmpl", struct { Name, Namespace, DestinationName string }{watchRuleNameB, testNs, destNameB}, testNs)).To(Succeed()) - verifyResourceCondition("gittarget", destNameB, testNs, "Validated", "True", "OK", "") - verifyResourceStatus("watchrule", watchRuleNameB, testNs, "True", "Ready", "") + verifyResourceCondition("gittarget", destNameB, testNs, "Validated", "True", "Succeeded", "") + verifyResourceStatus("watchrule", watchRuleNameB, testNs, "True", "Succeeded", "") waitForStreamsRunning(destNameB, testNs) By("asserting seed + overlap content converges under path B with no seed leaked as a per-event commit") diff --git a/test/e2e/source_cluster_e2e_test.go b/test/e2e/source_cluster_e2e_test.go index c3fb7aa9..235722bd 100644 --- a/test/e2e/source_cluster_e2e_test.go +++ b/test/e2e/source_cluster_e2e_test.go @@ -343,7 +343,7 @@ var _ = Describe("Manager source cluster / ClusterProvider attribution", Label(" target := "sc-unreachable-target" _, err = applyGitTargetWithClusterProvider(testNs, target, providerName, "clusters/unreachable", cpName) Expect(err).NotTo(HaveOccurred()) - verifyResourceCondition("gittarget", target, testNs, "Validated", "True", "OK", "") + verifyResourceCondition("gittarget", target, testNs, "Validated", "True", "Succeeded", "") verifyResourceCondition("gittarget", target, testNs, "SourceClusterReachable", "False", "SourceClusterUnreachable", "", "150s") }) diff --git a/test/e2e/source_namespace_e2e_test.go b/test/e2e/source_namespace_e2e_test.go index c346b282..422ce4a5 100644 --- a/test/e2e/source_namespace_e2e_test.go +++ b/test/e2e/source_namespace_e2e_test.go @@ -103,8 +103,8 @@ var _ = Describe("WatchRule source namespace", Label("manager"), Ordered, func() configNS, refusedTarget, providerName, refusedPath, nonDelegatingCP, sourceNS)).Error(). NotTo(HaveOccurred(), "failed to apply refused GitTarget") - verifyResourceCondition("gittarget", grantedTarget, configNS, "Validated", "True", "OK", "") - verifyResourceCondition("gittarget", refusedTarget, configNS, "Validated", "True", "OK", "") + verifyResourceCondition("gittarget", grantedTarget, configNS, "Validated", "True", "Succeeded", "") + verifyResourceCondition("gittarget", refusedTarget, configNS, "Validated", "True", "Succeeded", "") }) AfterAll(func() { @@ -128,7 +128,7 @@ var _ = Describe("WatchRule source namespace", Label("manager"), Ordered, func() By("asserting the override is authorized") verifyResourceCondition("watchrule", grantedRule, configNS, "SourceNamespaceAuthorized", "True", "SourceNamespaceAllowed", "") - verifyResourceStatus("watchrule", grantedRule, configNS, "True", "Ready", "") + verifyResourceStatus("watchrule", grantedRule, configNS, "True", "Succeeded", "") waitForStreamsRunning(grantedTarget, configNS) By("creating a ConfigMap in the SOURCE namespace") @@ -165,7 +165,7 @@ var _ = Describe("WatchRule source namespace", Label("manager"), Ordered, func() By("asserting the wildcard is authorized") verifyResourceCondition("watchrule", wildcardRule, configNS, "SourceNamespaceAuthorized", "True", "SourceNamespaceAllowed", "") - verifyResourceStatus("watchrule", wildcardRule, configNS, "True", "Ready", "") + verifyResourceStatus("watchrule", wildcardRule, configNS, "True", "Succeeded", "") By("creating one ConfigMap in the wildcard-only namespace and one in an unadmitted namespace") // wildcardNS is admitted by the target's policy but named by NO rule item, so anything @@ -223,8 +223,8 @@ var _ = Describe("WatchRule source namespace", Label("manager"), Ordered, func() NotTo(HaveOccurred(), "failed to apply the first explicit rule") Expect(applyWatchRuleWithSourceNamespace(ruleB, configNS, grantedTarget, wildcardNS)).Error(). NotTo(HaveOccurred(), "failed to apply the second explicit rule") - verifyResourceStatus("watchrule", ruleA, configNS, "True", "Ready", "") - verifyResourceStatus("watchrule", ruleB, configNS, "True", "Ready", "") + verifyResourceStatus("watchrule", ruleA, configNS, "True", "Succeeded", "") + verifyResourceStatus("watchrule", ruleB, configNS, "True", "Succeeded", "") By("creating one ConfigMap in each watched namespace") _, err := kubectlRunInNamespace(sourceNS, "create", "configmap", cmA, "--from-literal=k=v") diff --git a/test/e2e/tilt_playground_e2e_test.go b/test/e2e/tilt_playground_e2e_test.go index e1a5846a..2f39d79f 100644 --- a/test/e2e/tilt_playground_e2e_test.go +++ b/test/e2e/tilt_playground_e2e_test.go @@ -100,9 +100,9 @@ func (r playgroundRun) applyResources() { } func (r playgroundRun) verifyResourcesReady() { - verifyResourceStatus("gitprovider", r.providerName, r.namespace, "True", "Ready", "") - verifyResourceCondition("gittarget", r.targetName, r.namespace, "Validated", "True", "OK", "") - verifyResourceStatus("watchrule", r.watchRuleName, r.namespace, "True", "Ready", "") + verifyResourceStatus("gitprovider", r.providerName, r.namespace, "True", "Succeeded", "") + verifyResourceCondition("gittarget", r.targetName, r.namespace, "Validated", "True", "Succeeded", "") + verifyResourceStatus("watchrule", r.watchRuleName, r.namespace, "True", "Succeeded", "") } func (r playgroundRun) logArtifacts() { diff --git a/test/e2e/unsupported_folder_e2e_test.go b/test/e2e/unsupported_folder_e2e_test.go index 93dadcfb..4c9a7b3f 100644 --- a/test/e2e/unsupported_folder_e2e_test.go +++ b/test/e2e/unsupported_folder_e2e_test.go @@ -50,7 +50,7 @@ var _ = Describe("Manager Unsupported Folder Refusal", Label("manager", "unsuppo By("creating the GitProvider") createGitProviderWithURLInNamespace(providerName, testNs, repo.GitSecretHTTP, repo.RepoURLHTTP) - verifyResourceStatus("gitprovider", providerName, testNs, "True", "Ready", "") + verifyResourceStatus("gitprovider", providerName, testNs, "True", "Succeeded", "") }) AfterAll(func() { diff --git a/test/e2e/values_file_acceptance_e2e_test.go b/test/e2e/values_file_acceptance_e2e_test.go index a7644ec7..ad0fee59 100644 --- a/test/e2e/values_file_acceptance_e2e_test.go +++ b/test/e2e/values_file_acceptance_e2e_test.go @@ -97,7 +97,7 @@ func provisionValuesFileEnv(caseName string) valuesFileEnv { providerName := caseName + "-values-provider" By("creating the GitProvider") createGitProviderWithURLInNamespace(providerName, testNs, repo.GitSecretHTTP, repo.RepoURLHTTP) - verifyResourceStatus("gitprovider", providerName, testNs, "True", "Ready", "") + verifyResourceStatus("gitprovider", providerName, testNs, "True", "Succeeded", "") DeferCleanup(func() { _, _ = kubectlRunInNamespace(testNs, "delete", "gitprovider", providerName, "--ignore-not-found=true") @@ -144,8 +144,8 @@ func assertReferencedValuesFileAdopted(env valuesFileEnv, tc referencedValuesCas DeferCleanup(func() { cleanupWatchRule(ruleName, env.testNs) }) waitForGitTargetGitPathAccepted(destName, env.testNs) - verifyResourceCondition("gittarget", destName, env.testNs, "Validated", "True", "OK", "") - verifyResourceStatus("watchrule", ruleName, env.testNs, "True", "Ready", "") + verifyResourceCondition("gittarget", destName, env.testNs, "Validated", "True", "Succeeded", "") + verifyResourceStatus("watchrule", ruleName, env.testNs, "True", "Succeeded", "") waitForStreamsRunning(destName, env.testNs) By("mirroring a live ConfigMap into the adopted folder — a refused folder writes nothing") diff --git a/test/e2e/watchrule_configmap_secret_e2e_test.go b/test/e2e/watchrule_configmap_secret_e2e_test.go index 8f746991..2a5cc927 100644 --- a/test/e2e/watchrule_configmap_secret_e2e_test.go +++ b/test/e2e/watchrule_configmap_secret_e2e_test.go @@ -92,7 +92,7 @@ var _ = Describe("Manager WatchRule ConfigMap and Secret", Label("manager"), Ord Expect(err).NotTo(HaveOccurred(), "Failed to apply WatchRule") By("verifying the WatchRule is reconciled") - verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Ready", "") + verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Succeeded", "") By("cleaning up test resources") cleanupWatchRule(watchRuleName, testNs) @@ -149,7 +149,7 @@ spec: _, err = kubectlRunWithStdin(testNs, watchRuleManifest, "apply", "-f", "-") Expect(err).NotTo(HaveOccurred(), "Failed to apply wildcard WatchRule") - verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Ready", "") + verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Succeeded", "") Eventually(func(g Gomega) { output, getErr := kubectlRunInNamespace( testNs, @@ -219,8 +219,8 @@ spec: err := applyFromTemplate("test/e2e/templates/watchrule-secret.tmpl", data, testNs) Expect(err).NotTo(HaveOccurred(), "Failed to apply WatchRule") - verifyResourceCondition("gittarget", destName, testNs, "Validated", "True", "OK", "") - verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Ready", "") + verifyResourceCondition("gittarget", destName, testNs, "Validated", "True", "Succeeded", "") + verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Succeeded", "") waitForStreamsRunning(destName, testNs) By("creating Secret in watched namespace") @@ -323,7 +323,7 @@ spec: err := applyFromTemplate("test/e2e/templates/watchrule-secret.tmpl", data, testNs) Expect(err).NotTo(HaveOccurred(), "Failed to apply WatchRule") - verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Ready", "") + verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Succeeded", "") waitForStreamsRunning(destName, testNs) By("validating generated encryption secret has recipient and warning annotations") @@ -467,8 +467,8 @@ spec: Expect(err2).NotTo(HaveOccurred(), "Failed to apply WatchRule") By("verifying WatchRule is ready") - verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Ready", "") - verifyResourceCondition("gittarget", destName, testNs, "Validated", "True", "OK", "") + verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Succeeded", "") + verifyResourceCondition("gittarget", destName, testNs, "Validated", "True", "Succeeded", "") // Gate on StreamsRunning so the configmaps watch is past its replay watermark before we create // the ConfigMap: a create observed mid-replay folds into the unattributed baseline instead of @@ -610,7 +610,7 @@ spec: By("creating GitTarget but no WatchRule yet") destName := watchRuleName + "-dest" createGitTarget(destName, testNs, gitProviderName, "e2e/backfill-rule-add", "main") - verifyResourceCondition("gittarget", destName, testNs, "Validated", "True", "OK", "") + verifyResourceCondition("gittarget", destName, testNs, "Validated", "True", "Succeeded", "") By("creating the ConfigMap BEFORE the rule that should select it") configMapData := struct { @@ -648,7 +648,7 @@ spec: } err = applyFromTemplate("test/e2e/templates/manager/watchrule-configmap.tmpl", watchRuleData, testNs) Expect(err).NotTo(HaveOccurred(), "Failed to apply WatchRule") - verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Ready", "") + verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Succeeded", "") By("waiting for the pre-existing ConfigMap to be backfilled into git") verifyBackfill := func(g Gomega) { @@ -702,8 +702,8 @@ spec: Expect(err2).NotTo(HaveOccurred(), "Failed to apply WatchRule") By("verifying WatchRule is ready") - verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Ready", "") - verifyResourceCondition("gittarget", destName, testNs, "Validated", "True", "OK", "") + verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Succeeded", "") + verifyResourceCondition("gittarget", destName, testNs, "Validated", "True", "Succeeded", "") // Gate on StreamsRunning so both the create and the delete below are live per-event commits // rather than baseline folds, making the [DELETE] commit on the file's path deterministic. From c6cc006233850947c06f95da760df3a5e831b4e8 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Wed, 22 Jul 2026 17:44:36 +0000 Subject: [PATCH 2/4] test(e2e): finish the Succeeded reason sweep and split the requeue cadence by kind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six e2e specs still expected reason "Ready" on a GitProvider's Ready condition; the first sweep only caught the argument shape without a message. Reason expectations across the suite are now uniformly "Succeeded". Also splits the requeue cadence rather than sharing one rule, because the two kinds recover differently. A stalled rule waits for an event — a policy change, a Namespace label change, an edit — and every one of those has a watch edge, so it takes the steady interval as before. A stalled GitTarget waits for the Git FOLDER to change, which produces no Kubernetes event at all, so it keeps the fast loop it already had. Drops two reason aliases that nothing used. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../controller/clusterwatchrule_controller.go | 11 +++++++++-- internal/controller/constants.go | 12 ++++-------- internal/controller/gittarget_controller.go | 18 +++++++++++------- internal/controller/readiness.go | 8 ++++++-- internal/controller/watchrule_controller.go | 11 +++++++++-- test/e2e/crd_lifecycle_e2e_test.go | 2 +- test/e2e/gitprovider_validation_e2e_test.go | 6 +++--- test/e2e/gittarget_isolation_e2e_test.go | 2 +- test/e2e/gittarget_overlap_e2e_test.go | 2 +- test/e2e/source_namespace_e2e_test.go | 2 +- .../e2e/watchrule_configmap_secret_e2e_test.go | 4 ++-- 11 files changed, 48 insertions(+), 30 deletions(-) diff --git a/internal/controller/clusterwatchrule_controller.go b/internal/controller/clusterwatchrule_controller.go index 40aba8de..a8e826ef 100644 --- a/internal/controller/clusterwatchrule_controller.go +++ b/internal/controller/clusterwatchrule_controller.go @@ -332,7 +332,11 @@ func (r *ClusterWatchRuleReconciler) stallRule( } // commitRule writes the trio, persists the status, and picks the requeue cadence from the same -// verdict — so the cadence can never disagree with what status says. +// verdict, so the cadence can never disagree with what status says. +// +// Only a CONVERGING rule takes the fast loop. A stalled one waits for an event — a ClusterProvider +// policy change, a source-cluster Namespace label change, an edit to the rule — and every one of +// those has a watch edge registered in SetupWithManager, so polling it would find nothing. func (r *ClusterWatchRuleReconciler) commitRule( ctx context.Context, st *reconcileStatus, @@ -342,7 +346,10 @@ func (r *ClusterWatchRuleReconciler) commitRule( if err := st.commit(ctx); err != nil { return ctrl.Result{}, err } - return ctrl.Result{RequeueAfter: requeueFor(rd)}, nil + if rd.converging() { + return ctrl.Result{RequeueAfter: RequeueStreamSettleInterval}, nil + } + return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil } func (r *ClusterWatchRuleReconciler) setGitTargetReadyCondition( diff --git a/internal/controller/constants.go b/internal/controller/constants.go index 8e239dfa..a80272be 100644 --- a/internal/controller/constants.go +++ b/internal/controller/constants.go @@ -129,21 +129,17 @@ const ( // The generic condition reasons below are ALIASES of github.com/fluxcd/pkg/apis/meta, a module // this project already depends on. Sharing the vocabulary is the point: one alerting rule - // written against reason=Failed or reason=Succeeded works across every kind here AND across - // every Flux kind in the same cluster, which is not true of a per-project spelling. Domain - // reasons (UnsupportedContent, WriteBoundaryRefused, NoAdmittedSourceNamespaces, ...) stay ours - // — those carry information a generic reason cannot, and declaring them is exactly what the + // written against reason=Succeeded works across every kind here AND across every Flux kind in + // the same cluster, which is not true of a per-project spelling. Domain + // reasons (UnsupportedContent, WriteBoundaryRefused, NoAdmittedSourceNamespaces, ...) stay ours: + // those carry information a generic reason cannot, and declaring them is exactly what the // upstream vocabulary asks projects to do. // ReasonSucceeded is the reason on a healthy, fully reconciled object. It replaces the former // "OK"/"Ready" spellings, which restated the condition type instead of answering "why". ReasonSucceeded = fluxmeta.SucceededReason - // ReasonFailed indicates an observed failure: declared state does not match actual state. - ReasonFailed = fluxmeta.FailedReason // ReasonProgressing indicates that a stream or control-plane gate is still converging. ReasonProgressing = fluxmeta.ProgressingReason - // ReasonDependencyNotReady indicates that a referenced object is not ready yet. - ReasonDependencyNotReady = fluxmeta.DependencyNotReadyReason // ReasonChecking indicates that the controller is checking the resource status. ReasonChecking = "Checking" diff --git a/internal/controller/gittarget_controller.go b/internal/controller/gittarget_controller.go index 878e3137..40ebf56b 100644 --- a/internal/controller/gittarget_controller.go +++ b/internal/controller/gittarget_controller.go @@ -210,13 +210,17 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( if err := st.commit(ctx); err != nil { return ctrl.Result{}, err } - return ctrl.Result{RequeueAfter: requeueFor(rd)}, nil + return ctrl.Result{RequeueAfter: gitTargetRequeue(rd)}, nil } -// requeueFor picks the periodic cadence. Only a converged object earns the steady interval: a -// target that is still settling — or one that is stalled and waiting for the world to change under -// it — is re-checked on the fast loop so it converges (or recovers) promptly. -func requeueFor(rd *readiness) time.Duration { +// gitTargetRequeue picks the periodic cadence. Only a converged GitTarget earns the steady interval. +// +// A STALLED target gets the fast loop too, which is deliberate and specific to this kind: its +// terminal states clear when the Git FOLDER changes (someone removes the unsupported file, someone +// fixes the diverged value), and that produces no Kubernetes event at all. The data-plane +// acceptance channel is best-effort, so the periodic re-check is what actually recovers a refused +// target. It is cheap now that a re-check which finds nothing new writes nothing. +func gitTargetRequeue(rd *readiness) time.Duration { if rd.converged() { return RequeueSteadyInterval } @@ -235,9 +239,9 @@ type blockedGate struct { // stall publishes a terminal control-plane outcome and ends the reconcile. // -// The four early-return gates used to repeat this block verbatim: mark the data plane +// The three early-return gates used to repeat this block verbatim: mark the data plane // not-evaluated, stamp the trio, write status with retry, choose a requeue. They differ only in the -// four values blockedGate carries. +// values blockedGate carries. func (r *GitTargetReconciler) stall( ctx context.Context, st *reconcileStatus, diff --git a/internal/controller/readiness.go b/internal/controller/readiness.go index da9b7559..75fed6a8 100644 --- a/internal/controller/readiness.go +++ b/internal/controller/readiness.go @@ -101,10 +101,14 @@ func (r *readiness) contribute(level readinessLevel, verdict conditionValue) { r.verdict = verdict } -// converged reports whether no gate objected — the signal a reconcile uses to pick the steady -// requeue cadence over the fast one. +// converged reports whether no gate objected. func (r *readiness) converged() bool { return r.level == readinessConverged } +// converging reports whether something is pending that is expected to clear on its own. It is +// distinct from !converged() because a STALLED object is neither: it is waiting for a human, or for +// the world outside Kubernetes to change. +func (r *readiness) converging() bool { return r.level == readinessProgressing } + // kstatusTrio is the Ready/Reconciling/Stalled triple, derived together so they cannot disagree. type kstatusTrio struct { Ready conditionValue diff --git a/internal/controller/watchrule_controller.go b/internal/controller/watchrule_controller.go index 5fb14229..ee5ef4a9 100644 --- a/internal/controller/watchrule_controller.go +++ b/internal/controller/watchrule_controller.go @@ -233,7 +233,11 @@ func (r *WatchRuleReconciler) stallRule( } // commitRule writes the trio, persists the status, and picks the requeue cadence from the same -// verdict — so the cadence can never disagree with what status says. +// verdict, so the cadence can never disagree with what status says. +// +// Only a CONVERGING rule takes the fast loop. A stalled one waits for an event — a ClusterProvider +// policy change, a source-cluster Namespace label change, an edit to the rule — and every one of +// those has a watch edge registered in SetupWithManager, so polling it would find nothing. func (r *WatchRuleReconciler) commitRule( ctx context.Context, st *reconcileStatus, @@ -243,7 +247,10 @@ func (r *WatchRuleReconciler) commitRule( if err := st.commit(ctx); err != nil { return ctrl.Result{}, err } - return ctrl.Result{RequeueAfter: requeueFor(rd)}, nil + if rd.converging() { + return ctrl.Result{RequeueAfter: RequeueStreamSettleInterval}, nil + } + return ctrl.Result{RequeueAfter: RequeueSteadyInterval}, nil } func (r *WatchRuleReconciler) setResourceResolutionCondition( diff --git a/test/e2e/crd_lifecycle_e2e_test.go b/test/e2e/crd_lifecycle_e2e_test.go index f4f958e8..5f3eb063 100644 --- a/test/e2e/crd_lifecycle_e2e_test.go +++ b/test/e2e/crd_lifecycle_e2e_test.go @@ -53,7 +53,7 @@ var _ = Describe("Manager CRD Lifecycle", Label("manager"), Serial, Ordered, fun ) verifyResourceStatus( "gitprovider", "gitprovider-normal", testNs, - "True", "Ready", "Repository connectivity validated", + "True", "Succeeded", "Repository connectivity validated", ) }) diff --git a/test/e2e/gitprovider_validation_e2e_test.go b/test/e2e/gitprovider_validation_e2e_test.go index a195749d..11263870 100644 --- a/test/e2e/gitprovider_validation_e2e_test.go +++ b/test/e2e/gitprovider_validation_e2e_test.go @@ -59,7 +59,7 @@ var _ = Describe("Manager GitProvider Validation", Label("manager"), Ordered, fu verifyResourceStatus( "gitprovider", gitProviderName, testNs, - "True", "Ready", "Repository connectivity validated", + "True", "Succeeded", "Repository connectivity validated", ) By("showing final controller logs") @@ -91,7 +91,7 @@ var _ = Describe("Manager GitProvider Validation", Label("manager"), Ordered, fu validationRepo.RepoURLHTTP, ) verifyResourceStatus( - "gitprovider", gitProviderName, testNs, "True", "Ready", "Repository connectivity validated", + "gitprovider", gitProviderName, testNs, "True", "Succeeded", "Repository connectivity validated", ) // GitTarget with branch not matching any pattern should fail @@ -135,7 +135,7 @@ var _ = Describe("Manager GitProvider Validation", Label("manager"), Ordered, fu showControllerLogs("after SSH GitProvider creation") verifyResourceStatus( - "gitprovider", gitProviderName, testNs, "True", "Ready", "Repository connectivity validated", + "gitprovider", gitProviderName, testNs, "True", "Succeeded", "Repository connectivity validated", ) By("✅ Final SSH test logs") diff --git a/test/e2e/gittarget_isolation_e2e_test.go b/test/e2e/gittarget_isolation_e2e_test.go index 94426060..19f9a37c 100644 --- a/test/e2e/gittarget_isolation_e2e_test.go +++ b/test/e2e/gittarget_isolation_e2e_test.go @@ -63,7 +63,7 @@ var _ = Describe("Manager GitTarget Isolation", Label("manager"), Ordered, func( ) verifyResourceStatus( "gitprovider", providerName, testNs, - "True", "Ready", "Repository connectivity validated", + "True", "Succeeded", "Repository connectivity validated", ) By("creating two independent GitTargets writing to separate paths in the same repo") diff --git a/test/e2e/gittarget_overlap_e2e_test.go b/test/e2e/gittarget_overlap_e2e_test.go index 5515b519..358e7874 100644 --- a/test/e2e/gittarget_overlap_e2e_test.go +++ b/test/e2e/gittarget_overlap_e2e_test.go @@ -52,7 +52,7 @@ var _ = Describe("Manager GitTarget Overlap Guard", Label("manager"), Ordered, f ) verifyResourceStatus( "gitprovider", providerName, testNs, - "True", "Ready", "Repository connectivity validated", + "True", "Succeeded", "Repository connectivity validated", ) }) diff --git a/test/e2e/source_namespace_e2e_test.go b/test/e2e/source_namespace_e2e_test.go index 422ce4a5..04002177 100644 --- a/test/e2e/source_namespace_e2e_test.go +++ b/test/e2e/source_namespace_e2e_test.go @@ -81,7 +81,7 @@ var _ = Describe("WatchRule source namespace", Label("manager"), Ordered, func() createGitProviderWithURLInNamespace( providerName, configNS, srcnsRepo.GitSecretHTTP, srcnsRepo.RepoURLHTTP) verifyResourceStatus("gitprovider", providerName, configNS, - "True", "Ready", "Repository connectivity validated") + "True", "Succeeded", "Repository connectivity validated") By("declaring two in-cluster ClusterProviders that differ ONLY in the delegation flag") // Both omit kubeConfig, so both name the operator's own cluster: this is deliberately the diff --git a/test/e2e/watchrule_configmap_secret_e2e_test.go b/test/e2e/watchrule_configmap_secret_e2e_test.go index 2a5cc927..15397f5a 100644 --- a/test/e2e/watchrule_configmap_secret_e2e_test.go +++ b/test/e2e/watchrule_configmap_secret_e2e_test.go @@ -50,7 +50,7 @@ var _ = Describe("Manager WatchRule ConfigMap and Secret", Label("manager"), Ord ) verifyResourceStatus( "gitprovider", "gitprovider-normal", testNs, - "True", "Ready", "Repository connectivity validated", + "True", "Succeeded", "Repository connectivity validated", ) }) @@ -66,7 +66,7 @@ var _ = Describe("Manager WatchRule ConfigMap and Secret", Label("manager"), Ord // gitprovider-normal is created and verified in BeforeAll; this spec // re-asserts it stays Ready without re-creating it. verifyResourceStatus( - "gitprovider", "gitprovider-normal", testNs, "True", "Ready", "Repository connectivity validated", + "gitprovider", "gitprovider-normal", testNs, "True", "Succeeded", "Repository connectivity validated", ) }) From 729903eac066fe9dcae13a80192081fc4f5ff408 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Fri, 24 Jul 2026 20:24:48 +0000 Subject: [PATCH 3/4] ci(lint): allow the deprecated GetEventRecorderFor in cmd/main.go Newer staticcheck (CI's bundled version, ahead of the pinned golangci-lint 2.12.2) flags mgr.GetEventRecorderFor as deprecated in favour of the events-API GetEventRecorder. GetEventRecorderFor returns the record.EventRecorder the shared status writer emits on, is the ubiquitous kubebuilder idiom, and the replacement's Eventf demands an action verb and a note that do not fit a one-line status Event. Scope the allowance to cmd/main.go, mirroring the existing scheme.Builder SA1019 exclusion. Co-Authored-By: Claude Opus 4.8 (1M context) --- .golangci.yml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.golangci.yml b/.golangci.yml index 190cd549..09da6053 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -363,6 +363,14 @@ linters: - text: "SA1019: scheme.Builder is deprecated" path: 'api/v1alpha3/groupversion_info\.go' linters: [staticcheck] + # mgr.GetEventRecorderFor returns the record.EventRecorder our shared status writer emits on + # (a single Event per persisted Ready transition). It is the ubiquitous kubebuilder idiom; the + # non-deprecated GetEventRecorder returns the newer events.EventRecorder, whose Eventf demands + # an action verb and a note and does not fit a one-line status Event. Newer staticcheck + # flags the deprecation; scope the allowance to where the manager is wired. + - text: "SA1019: mgr.GetEventRecorderFor is deprecated" + path: 'cmd/main\.go' + linters: [staticcheck] # Allow fmt.Print* in test/e2e (debugging outputs) - path: 'test/e2e/.*\.go' linters: [forbidigo] From 683a29606c88ca45d0c3e6d51e151147e1dba710 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Fri, 24 Jul 2026 20:53:51 +0000 Subject: [PATCH 4/4] fix(status): keep a GitProvider mid-reconcile from downgrading its GitTarget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gitProviderReadiness documented the same contract clusterProviderReadiness implements — only an EXPLICIT Ready=False downgrades — but its switch had no ConditionFalse arm, so a provider reporting Ready=Unknown fell through to the message/default arms and was published as False. Since gitTargetReadinessGates only gates on ConditionFalse, that pinned every dependent GitTarget at Ready=False for as long as the provider was still validating. Also documents what a dropped status conflict actually costs: every For() in this project carries GenerationChangedPredicate, so a conflict from a metadata- or status-only write does NOT re-enqueue, and the requeue cadence is the backstop. Renames two tests onto the functions they now exercise. Co-Authored-By: Claude Opus 5 (1M context) --- docs/spec/status-conditions-guide.md | 8 ++++++-- .../clusterprovider_controller_unit_test.go | 4 ++-- internal/controller/gittarget_source_cluster.go | 11 ++++++++--- internal/controller/gittarget_source_cluster_test.go | 9 +++++++++ internal/controller/watchrule_kstatus_test.go | 4 ++-- 5 files changed, 27 insertions(+), 9 deletions(-) diff --git a/docs/spec/status-conditions-guide.md b/docs/spec/status-conditions-guide.md index 20665c41..d3ee6917 100644 --- a/docs/spec/status-conditions-guide.md +++ b/docs/spec/status-conditions-guide.md @@ -51,8 +51,12 @@ making every object mutable on read. The write is a status **patch with optimistic concurrency**, and a conflict is dropped rather than retried: a conflict means the object moved under this reconcile, so the status just computed -describes a generation that is no longer current, and the write that beat us has already enqueued a -fresh pass. +describes a generation that is no longer current, and publishing it would publish a stale +observation. Convergence is deferred, not guaranteed to be immediate. A conflict caused by a *spec* +edit re-enqueues at once; one caused by a metadata- or status-only write does **not**, because every +`For()` in this project carries `GenerationChangedPredicate`. That case converges on the next +dependency event or the requeue cadence `commitRule`/`requeueFor` already picked from the same +verdict — which is why that cadence, and not a retry loop here, is the backstop. ### Reason vocabulary diff --git a/internal/controller/clusterprovider_controller_unit_test.go b/internal/controller/clusterprovider_controller_unit_test.go index 7ea88e28..cd2b04db 100644 --- a/internal/controller/clusterprovider_controller_unit_test.go +++ b/internal/controller/clusterprovider_controller_unit_test.go @@ -318,9 +318,9 @@ func TestValidateProviderKubeConfig_NilSecretRef(t *testing.T) { assert.NotEmpty(t, msg) } -// TestClusterProviderUpdateStatus_DeletedObject checks the shared status writer treats a vanished +// TestReconcileStatusCommit_DeletedObject checks the shared status writer treats a vanished // object as done rather than erroring: a reconcile that raced a delete must not fail the workqueue. -func TestClusterProviderUpdateStatus_DeletedObject(t *testing.T) { +func TestReconcileStatusCommit_DeletedObject(t *testing.T) { cl := fake.NewClientBuilder(). WithScheme(scScheme(t)). WithStatusSubresource(&configbutleraiv1alpha3.ClusterProvider{}). diff --git a/internal/controller/gittarget_source_cluster.go b/internal/controller/gittarget_source_cluster.go index 561e5e94..aab345e9 100644 --- a/internal/controller/gittarget_source_cluster.go +++ b/internal/controller/gittarget_source_cluster.go @@ -185,10 +185,15 @@ func (r *GitTargetReconciler) gitProviderReadiness( Reason: GitTargetReasonGitProviderReady, Message: fmt.Sprintf("referenced GitProvider %s is Ready", key), } - case c.Message != "": - return notReady(metav1.ConditionFalse, "referenced GitProvider %s is not Ready: %s", key, c.Message) - default: + case c.Status == metav1.ConditionFalse: + if c.Message != "" { + return notReady(metav1.ConditionFalse, "referenced GitProvider %s is not Ready: %s", key, c.Message) + } return notReady(metav1.ConditionFalse, "referenced GitProvider %s is not Ready", key) + default: + // Ready=Unknown is the provider mid-reconcile saying it does not know yet, which is the + // case the contract above refuses to downgrade on — same rule as clusterProviderReadiness. + return notReady(metav1.ConditionUnknown, "referenced GitProvider %s readiness is unknown", key) } } diff --git a/internal/controller/gittarget_source_cluster_test.go b/internal/controller/gittarget_source_cluster_test.go index 7ff4854f..a1518d0d 100644 --- a/internal/controller/gittarget_source_cluster_test.go +++ b/internal/controller/gittarget_source_cluster_test.go @@ -463,6 +463,15 @@ func TestGitProviderReadiness_AllScenarios(t *testing.T) { {"ready", provider([]metav1.Condition{ready}), metav1.ConditionTrue}, {"not ready -> False (downgrades)", provider([]metav1.Condition{notReady}), metav1.ConditionFalse}, {"no condition -> Unknown (does not downgrade)", provider(nil), metav1.ConditionUnknown}, + { + "Ready=Unknown -> Unknown (provider mid-reconcile does not downgrade)", + provider([]metav1.Condition{{ + Type: ConditionTypeReady, + Status: metav1.ConditionUnknown, + Reason: ReasonProgressing, + }}), + metav1.ConditionUnknown, + }, {"absent provider -> Unknown", nil, metav1.ConditionUnknown}, } for _, tc := range tests { diff --git a/internal/controller/watchrule_kstatus_test.go b/internal/controller/watchrule_kstatus_test.go index 7399f50a..af42a579 100644 --- a/internal/controller/watchrule_kstatus_test.go +++ b/internal/controller/watchrule_kstatus_test.go @@ -119,10 +119,10 @@ func TestWatchRuleSourceNamespaceKstatusContract(t *testing.T) { } } -// TestApplyRuleKstatus_SourceAuthorizationIsAPrerequisite asserts the aggregation itself, not just +// TestRuleReadiness_SourceAuthorizationIsAPrerequisite asserts the aggregation itself, not just // its inputs: Ready=True must require SourceNamespaceAuthorized=True even when every pre-existing // prerequisite is healthy. Without this, a rule could report Ready while its gate said otherwise. -func TestApplyRuleKstatus_SourceAuthorizationIsAPrerequisite(t *testing.T) { +func TestRuleReadiness_SourceAuthorizationIsAPrerequisite(t *testing.T) { healthy := []metav1.Condition{ {Type: ConditionTypeResourcesResolved, Status: metav1.ConditionTrue, Reason: "Resolved"}, {Type: ConditionTypeGitTargetReady, Status: metav1.ConditionTrue, Reason: "Ready"},