From 6c4f3891099d6635d31589b23196aaf418981710 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Sat, 24 Oct 2026 21:10:53 +0000 Subject: [PATCH 1/5] feat: reconcile only the namespaces --watch-namespaces names Two instances can now share one config-plane cluster. --watch-namespaces (Helm: controllerManager.watchNamespaces) confines the manager's cache to the listed namespaces, so an instance reconciles the namespaced kinds there only. A ClusterWatchRule is cluster-scoped, so every instance sees it: the one serving its GitTarget's namespace reconciles it, and the others write nothing to it, status included. A rule re-pointed out of the served namespaces is dropped from the store. The Namespace informer is narrowed by its name label, so a label change elsewhere enqueues nothing, and ConfigMaps are also cached in the controller's own namespace for the install-level known_hosts ConfigMap. The chart scopes the admission webhook to the same namespaces. Empty, the default, serves every namespace as before. Co-Authored-By: Claude Opus 5.5 (1M context) --- charts/gitops-reverser/README.md | 1 + .../gitops-reverser/templates/deployment.yaml | 3 + .../validate-operator-types-webhook.yaml | 10 +++ charts/gitops-reverser/values.schema.json | 6 ++ charts/gitops-reverser/values.yaml | 7 ++ cmd/main.go | 60 ++++++++++++++++- cmd/main_chart_args_test.go | 9 +++ cmd/main_watch_namespaces_test.go | 56 ++++++++++++++++ docs/configuration.md | 35 ++++++++++ .../clusterwatchrule_admission_test.go | 52 ++++++++++++++ .../controller/clusterwatchrule_controller.go | 25 +++++++ internal/rulestore/store.go | 8 +++ internal/watch/bootstrap.go | 3 + internal/watch/bootstrap_admission_test.go | 29 ++++++++ internal/watch/manager.go | 3 + internal/watch/served_namespaces.go | 67 +++++++++++++++++++ internal/watch/served_namespaces_test.go | 39 +++++++++++ 17 files changed, 412 insertions(+), 1 deletion(-) create mode 100644 cmd/main_watch_namespaces_test.go create mode 100644 internal/watch/served_namespaces.go create mode 100644 internal/watch/served_namespaces_test.go diff --git a/charts/gitops-reverser/README.md b/charts/gitops-reverser/README.md index 89cb01f4a..b097b2aa2 100644 --- a/charts/gitops-reverser/README.md +++ b/charts/gitops-reverser/README.md @@ -179,6 +179,7 @@ nodeSelector: | `servers.receiver.port` | Push receiver container port; must match the port in `bindAddress` | `9292` | | `controllerManager.additionalSensitiveResources` | Extra Secret-shaped resource types encrypted as `resource` or `group/resource` | `[]` | | `controllerManager.inClusterKubeProviders` | `KubeProvider`s, as `namespace/name`, that may use `spec.inCluster`, the operator's own identity (`--in-cluster-kube-providers`). Exact names, no wildcards. The provider the chart renders is added on its own when it uses `inCluster`, so list only the others. One that sets `inCluster` and is not listed reports `InClusterNotAuthorized` and is never used | `[]` | +| `controllerManager.watchNamespaces` | Config-plane namespaces this release reconciles (`--watch-namespaces`), so two releases can share one cluster. A release reconciles the `GitTarget`s, rules, providers and branches in these namespaces only, a `ClusterWatchRule` only when its `GitTarget` is in one of them, and scopes its admission webhook to them. List every namespace whose objects reference each other. See [two instances in one cluster](../../docs/configuration.md#two-instances-in-one-cluster) | `[]` (every namespace) | | `controllerManager.insecureAllowGitHTTP` | Permit credentials with `http://` GitProvider URLs for trusted in-cluster development Git servers | `false` | | `controllerManager.branchWorkerQueueDepth` | Event queue depth per branch worker. A full queue refuses the write (`git_queue_refusals_total`) instead of stalling the watch path; the watch delivers it again from its cursor, and only a cursor that expires first loses that write's own commit (and, under `prune.mode: OnEvent`, a delete's removal). Costs queue depth × payload of pod memory, which `branchBufferMaxSize` does **not** cover | `1000` | | `controllerManager.branchBufferMaxSize` | Maximum in-memory event buffer per branch worker. Bounds the open commit window plus the writes retained for replay until a push succeeds. While the remote cannot be reached, retained writes that reach it close the worker to new writes, saves and resyncs until they are published; the watch delivers a refused event again | `8Mi` | diff --git a/charts/gitops-reverser/templates/deployment.yaml b/charts/gitops-reverser/templates/deployment.yaml index 4f74ba4ff..5c54882e5 100644 --- a/charts/gitops-reverser/templates/deployment.yaml +++ b/charts/gitops-reverser/templates/deployment.yaml @@ -102,6 +102,9 @@ spec: {{- with .Values.controllerManager.additionalSensitiveResources }} - {{ printf "--additional-sensitive-resources=%s" (join "," .) | quote }} {{- end }} + {{- with .Values.controllerManager.watchNamespaces }} + - {{ printf "--watch-namespaces=%s" (join "," .) | quote }} + {{- end }} {{- with include "gitops-reverser.inClusterKubeProviders" . }} - {{ printf "--in-cluster-kube-providers=%s" . | quote }} {{- end }} diff --git a/charts/gitops-reverser/templates/validate-operator-types-webhook.yaml b/charts/gitops-reverser/templates/validate-operator-types-webhook.yaml index 3d382ebf6..868434ed1 100644 --- a/charts/gitops-reverser/templates/validate-operator-types-webhook.yaml +++ b/charts/gitops-reverser/templates/validate-operator-types-webhook.yaml @@ -58,6 +58,16 @@ webhooks: resources: - watchrules scope: Namespaced + {{- with .Values.controllerManager.watchNamespaces }} + # Only the namespaces this release reconciles: another release sharing the cluster judges + # its own, against the objects its own cache holds. + namespaceSelector: + matchExpressions: + - key: kubernetes.io/metadata.name + operator: In + values: + {{- toYaml . | nindent 12 }} + {{- end }} # Redis write on real requests; nothing on dry-run (the handler honors this). sideEffects: NoneOnDryRun timeoutSeconds: {{ .Values.servers.admission.timeoutSeconds }} diff --git a/charts/gitops-reverser/values.schema.json b/charts/gitops-reverser/values.schema.json index 4eb69f9d7..609d6cde4 100644 --- a/charts/gitops-reverser/values.schema.json +++ b/charts/gitops-reverser/values.schema.json @@ -105,6 +105,12 @@ "items": { "type": "string", "pattern": "^[a-z0-9]([-a-z0-9]*[a-z0-9])?/[a-z0-9]([-a-z0-9.]*[a-z0-9])?$" }, "description": "KubeProviders (namespace/name) besides the one the chart renders that may use spec.inCluster, the operator's own identity. Exact names, no wildcards." }, + "watchNamespaces": { + "type": "array", + "uniqueItems": true, + "items": { "type": "string", "pattern": "^[a-z0-9]([-a-z0-9]*[a-z0-9])?$", "maxLength": 63 }, + "description": "Config-plane namespaces this release reconciles (--watch-namespaces). Empty: every namespace." + }, "insecureAllowGitHTTP": { "type": "boolean", "description": "Permit credentials with http:// GitProvider URLs for trusted in-cluster development Git servers." diff --git a/charts/gitops-reverser/values.yaml b/charts/gitops-reverser/values.yaml index 5def3889f..95ba5627c 100644 --- a/charts/gitops-reverser/values.yaml +++ b/charts/gitops-reverser/values.yaml @@ -68,6 +68,13 @@ controllerManager: # KubeProvider that sets inCluster and is not listed reports InClusterNotAuthorized and is never # used. inClusterKubeProviders: [] + # Config-plane namespaces this release reconciles (--watch-namespaces). Empty: every namespace. + # Two releases sharing one cluster each list their own: a release reconciles the GitTargets, + # rules, providers and branches in these namespaces only, and a ClusterWatchRule only when its + # GitTarget is in one of them. Its admission webhook is scoped to them too. List every namespace + # whose objects reference each other, and this release's own namespace when the chart renders + # objects there (kubeProvider, below). The ClusterRole stays cluster-wide. + watchNamespaces: [] # Permit credentials with http:// GitProvider URLs. This is only for trusted # in-cluster development Git servers; production remotes should use HTTPS or SSH. insecureAllowGitHTTP: false diff --git a/cmd/main.go b/cmd/main.go index 0c964e41f..b2d94ea5b 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -25,10 +25,13 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/selection" utilruntime "k8s.io/apimachinery/pkg/util/runtime" clientgoscheme "k8s.io/client-go/kubernetes/scheme" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/certwatcher" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log/zap" @@ -142,6 +145,7 @@ func main() { RuleStore: ruleStore, EventRouter: nil, // Will be set below SensitiveResources: cfg.sensitiveResources, + ServedNamespaces: cfg.watchNamespaces, // Resolve a source cluster (named by a GitTarget.spec.kubeProviderRef) into a rest.Config: // read the KubeProvider, then its kubeconfig Secret from the provider's own namespace, or // the operator's own identity where --in-cluster-kube-providers allows it. The manager @@ -214,6 +218,8 @@ func main() { RuleStore: ruleStore, WatchManager: watchMgr, Recorder: mgr.GetEventRecorderFor("clusterwatchrule"), + // A ClusterWatchRule is cluster-scoped, so the cache does not confine it. + ServedNamespaces: cfg.watchNamespaces, }).SetupWithManager(mgr), "unable to create controller", "controller", "ClusterWatchRule") // Valkey/Redis is optional. When configured it holds each GitTarget's watch resume cursor so work @@ -515,7 +521,10 @@ type appConfig struct { // operator-supplied kubeconfig is attacker-adjacent input, so unsafe kubeconfigs are REJECTED // (a legible Validated=False), diverging from Flux's silent strip. kubeConfigSafety kubeconfig.SafetyPolicy - zapOpts zap.Options + // watchNamespaces is the config-plane namespaces this instance reconciles; the zero value is + // every namespace. + watchNamespaces watch.ServedNamespaces + zapOpts zap.Options } // parseFlags parses CLI flags and returns the application configuration. @@ -722,6 +731,12 @@ func parseFlagsWithArgs(fs *flag.FlagSet, args []string) (appConfig, error) { "known_hosts at all. A present-but-unparseable known_hosts is always a hard error.") fs.BoolVar(&cfg.credentialPolicy.AllowInsecureGitHTTP, "insecure-allow-git-http", false, "INSECURE, dev/throwaway clusters only: permit credentials with http:// GitProvider URLs.") + var watchNamespaces string + fs.StringVar(&watchNamespaces, "watch-namespaces", "", + "Comma-separated config-plane namespaces this instance reconciles. Empty (the default): every "+ + "namespace. Lets two instances share one cluster: each reconciles only the GitTargets, rules, "+ + "providers and branches in its own namespaces, and a ClusterWatchRule only when its GitTarget "+ + "is in one of them.") bindReceiverFlags(fs, &cfg) cfg.zapOpts = zap.Options{ // Production mode defaults to JSON encoding, which is easier for log processors to parse. @@ -786,6 +801,10 @@ func parseFlagsWithArgs(fs *flag.FlagSet, args []string) (appConfig, error) { if err != nil { return appConfig{}, err } + cfg.watchNamespaces, err = watch.ParseServedNamespaces(watchNamespaces) + if err != nil { + return appConfig{}, err + } // The install-level default known-hosts ConfigMap lives in the controller's own namespace, // supplied via the downward API. Without it, that resolution layer is simply unavailable. @@ -1273,6 +1292,7 @@ func newManager( DisableFor: []client.Object{&corev1.Secret{}}, }, }, + Cache: servedNamespacesCacheOptions(cfg.watchNamespaces, cfg.sshHostKeys.ControllerNamespace), }) if err != nil { setupLog.Error(err, "unable to start manager") @@ -1281,6 +1301,44 @@ func newManager( return mgr } +// servedNamespacesCacheOptions confines the manager's cache to the served namespaces, so this +// instance lists, watches and reconciles the namespaced kinds there only. Every namespace is +// served when none are named. +// +// Two kinds need more than DefaultNamespaces. A Namespace is cluster-scoped and so unaffected by +// it; it is narrowed by its immutable name label, so a label change elsewhere enqueues nothing and +// a grant never reads a namespace this instance does not serve. ConfigMaps are cached in the +// controller's own namespace as well, for the install-level known_hosts ConfigMap. +func servedNamespacesCacheOptions(served watch.ServedNamespaces, controllerNamespace string) cache.Options { + if served.All() { + return cache.Options{} + } + names := served.Names() + defaults := make(map[string]cache.Config, len(names)) + for _, name := range names { + defaults[name] = cache.Config{} + } + configMapNamespaces := make(map[string]cache.Config, len(names)+1) + for name := range defaults { + configMapNamespaces[name] = cache.Config{} + } + if controllerNamespace != "" { + configMapNamespaces[controllerNamespace] = cache.Config{} + } + byName, err := labels.NewRequirement(corev1.LabelMetadataName, selection.In, names) + if err != nil { + // The names passed ParseServedNamespaces, which is stricter than a label value. + panic(fmt.Sprintf("served namespaces are not label values: %v", err)) + } + return cache.Options{ + DefaultNamespaces: defaults, + ByObject: map[client.Object]cache.ByObject{ + &corev1.Namespace{}: {Label: labels.NewSelector().Add(*byName)}, + &corev1.ConfigMap{}: {Namespaces: configMapNamespaces}, + }, + } +} + // setupAdmissionWebhooks registers both handlers on the one admission server: the // always-allow observer (a future-policy extension point) and the validate-operator-types // handler, which captures the submitter of our own command kinds into commandAuthorStore and diff --git a/cmd/main_chart_args_test.go b/cmd/main_chart_args_test.go index 1fc6600b6..47f7988c4 100644 --- a/cmd/main_chart_args_test.go +++ b/cmd/main_chart_args_test.go @@ -73,6 +73,7 @@ func TestChartRendersArgsTheBinaryAccepts(t *testing.T) { wantGitHTTP bool wantQueueDepth int wantBufferBytes int64 + wantWatchNS []string }{ "chart defaults": { wantKeyPrefix: "gitops-reverser", @@ -142,6 +143,12 @@ func TestChartRendersArgsTheBinaryAccepts(t *testing.T) { wantQueueDepth: 4000, wantBufferBytes: 32 * 1024 * 1024, }, + // A list joined into one quoted flag: quoting it wrong would make every name invalid. + "watch namespaces": { + setValues: []string{"controllerManager.watchNamespaces={team-b,team-a}"}, + wantKeyPrefix: "gitops-reverser", + wantWatchNS: []string{"team-a", "team-b"}, + }, } for name, tc := range tests { @@ -174,6 +181,8 @@ func TestChartRendersArgsTheBinaryAccepts(t *testing.T) { require.Equal(t, tc.wantBufferBytes, cfg.branchWorkerLimits.MaxBufferBytes, "controllerManager.branchBufferMaxSize must reach the binary") } + require.Equal(t, tc.wantWatchNS, cfg.watchNamespaces.Names(), + "controllerManager.watchNamespaces must reach the binary, and default to every namespace") }) } } diff --git a/cmd/main_watch_namespaces_test.go b/cmd/main_watch_namespaces_test.go new file mode 100644 index 000000000..66a492240 --- /dev/null +++ b/cmd/main_watch_namespaces_test.go @@ -0,0 +1,56 @@ +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/labels" + "sigs.k8s.io/controller-runtime/pkg/cache" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/ConfigButler/gitops-reverser/internal/watch" +) + +func TestServedNamespacesCacheOptions_EveryNamespaceLeavesTheCacheAlone(t *testing.T) { + assert.Equal(t, cache.Options{}, servedNamespacesCacheOptions(watch.ServedNamespaces{}, "gitops-reverser")) +} + +func TestServedNamespacesCacheOptions_ConfinesTheCacheToTheServedNamespaces(t *testing.T) { + served, err := watch.ParseServedNamespaces("team-a,team-b") + require.NoError(t, err) + + opts := servedNamespacesCacheOptions(served, "gitops-reverser") + + assert.Equal(t, map[string]cache.Config{"team-a": {}, "team-b": {}}, opts.DefaultNamespaces) + + byObject := map[string]cache.ByObject{} + for obj, cfg := range opts.ByObject { + byObject[kindOf(obj)] = cfg + } + assert.Equal(t, + map[string]cache.Config{"team-a": {}, "team-b": {}, "gitops-reverser": {}}, + byObject["ConfigMap"].Namespaces, + "the install-level known_hosts ConfigMap lives in the controller's own namespace") + + namespaces := byObject["Namespace"].Label + require.NotNil(t, namespaces, "a cluster-scoped Namespace is not confined by DefaultNamespaces") + for name, want := range map[string]bool{"team-a": true, "team-b": true, "team-c": false} { + got := namespaces.Matches(labels.Set{corev1.LabelMetadataName: name}) + assert.Equal(t, want, got, name) + } +} + +func kindOf(obj client.Object) string { + switch obj.(type) { + case *corev1.ConfigMap: + return "ConfigMap" + case *corev1.Namespace: + return "Namespace" + default: + return "unexpected" + } +} diff --git a/docs/configuration.md b/docs/configuration.md index 6b021fc0e..1d44910e3 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -588,6 +588,41 @@ which cluster a folder means, so every `GitTarget` names its `GitBranch` and its Nothing falls back to the provider the chart renders, and that provider admits only the release namespace's own objects until the installer grants more. +### Two instances in one cluster + +Two installations can share one config-plane cluster when each reconciles its own namespaces. Set +`--watch-namespaces` (Helm: `controllerManager.watchNamespaces`) on each to a disjoint list: + +```yaml +# The platform's release +controllerManager: + watchNamespaces: [platform, team-a, team-b] +--- +# The customer's release, installed next to it +controllerManager: + watchNamespaces: [customer-mirror] +``` + +An instance then reconciles the `GitProvider`s, `GitBranch`es, `GitReceiver`s, `KubeProvider`s, +`GitTarget`s, rules, applier claims and `CommitRequest`s in its namespaces only. A +`ClusterWatchRule` is cluster-scoped, so both instances see it; the one serving the namespace of +the `GitTarget` it names reconciles it, and the other writes nothing to it, status included. The +chart scopes each release's admission webhook to the same namespaces. + +- **List every namespace whose objects reference each other.** A `GitBranch` naming a + `GitProvider` in another namespace, or a `GitTarget` naming a `KubeProvider` there, needs both + namespaces in the list; one outside it reads as an error, not as a missing object. Include the + release's own namespace when the chart renders a `KubeProvider` there. +- **Grants reach the served namespaces only.** A grant that admits a namespace another instance + serves admits nothing there: this instance never reconciles that namespace's objects. +- **Each instance needs its own Redis key prefix** (`queue.redis.keyPrefix`) when two share one + Redis. +- **The ClusterRole stays cluster-wide.** The namespaced kinds are listed and watched per + namespace, but the chart's permissions are not narrowed with them. +- **High availability is a different thing.** Two instances serving disjoint namespaces never + write the same `GitTarget`; two replicas of one instance would, which is why the chart still + refuses `replicaCount` above one. + ## `GitProvider` `GitProvider` defines the Git remote, credentials, who may claim which branches, and commit behavior. A diff --git a/internal/controller/clusterwatchrule_admission_test.go b/internal/controller/clusterwatchrule_admission_test.go index 1bc5eccfc..92f1dd86e 100644 --- a/internal/controller/clusterwatchrule_admission_test.go +++ b/internal/controller/clusterwatchrule_admission_test.go @@ -421,6 +421,58 @@ func TestReconcile_RevocationRemovesCompiledRule(t *testing.T) { assertTerminalRefusal(t, f.reloadRule(ctx, t)) } +// TestReconcile_AClusterWatchRuleAnotherInstanceServesIsLeftAlone: under --watch-namespaces a rule +// naming a GitTarget outside the served namespaces belongs to another instance. It is not compiled +// and its status is not written, so two instances never fight over its conditions. +func TestReconcile_AClusterWatchRuleAnotherInstanceServesIsLeftAlone(t *testing.T) { + ctx := context.Background() + f := newCWAFixture(t, []client.Object{ + cwaClusterWatchRule(), cwaGitTarget(), cwaGitProvider(), + cwaKubeProvider(cwaAdmitting()), + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: cwaTargetNS}}, + }, interceptor.Funcs{}) + served, err := watch.ParseServedNamespaces("another-team") + require.NoError(t, err) + f.reconciler.ServedNamespaces = served + + _, err = f.reconcile(ctx) + require.NoError(t, err) + + assert.Empty(t, f.compiledNames()) + assert.Empty(t, f.reloadRule(ctx, t).Status.Conditions, "another instance's rule gets no status from this one") + assert.Zero(t, f.wm.replans, "a rule this instance never held needs no replan") +} + +// TestReconcile_AClusterWatchRuleRepointedOutOfTheServedNamespacesIsDropped: a rule this instance +// compiled, re-pointed at a GitTarget in a namespace it does not serve, leaves the store, and its +// streams are replanned away. +func TestReconcile_AClusterWatchRuleRepointedOutOfTheServedNamespacesIsDropped(t *testing.T) { + ctx := context.Background() + f := newCWAFixture(t, []client.Object{ + cwaClusterWatchRule(), cwaGitTarget(), cwaGitProvider(), + cwaKubeProvider(cwaAdmitting()), + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: cwaTargetNS}}, + }, interceptor.Funcs{}) + served, err := watch.ParseServedNamespaces(cwaTargetNS + "," + cwaProviderNS) + require.NoError(t, err) + f.reconciler.ServedNamespaces = served + + _, err = f.reconcile(ctx) + require.NoError(t, err) + require.Equal(t, []string{cwaRuleName}, f.compiledNames(), "precondition: the rule is running") + replansWhileServed := f.wm.replans + + rule := f.reloadRule(ctx, t) + rule.Spec.GitTargetRef.Namespace = "another-team" + require.NoError(t, f.client.Update(ctx, rule)) + + _, err = f.reconcile(ctx) + require.NoError(t, err) + + assert.Empty(t, f.compiledNames()) + assert.Greater(t, f.wm.replans, replansWhileServed) +} + // TestReconcile_AdmissionReadErrorRequeuesAndKeepsCompiledRule pins the fail-safe direction. A // transient apiserver failure is NOT a denial: tearing the stream down on it would turn every // blip into an outage, so the rule stays compiled and the error requeues. diff --git a/internal/controller/clusterwatchrule_controller.go b/internal/controller/clusterwatchrule_controller.go index 90eed0af6..5f171724f 100644 --- a/internal/controller/clusterwatchrule_controller.go +++ b/internal/controller/clusterwatchrule_controller.go @@ -56,6 +56,10 @@ type ClusterWatchRuleReconciler struct { WatchManager WatchManagerInterface // Recorder emits a Kubernetes Event on every persisted Ready transition; nil disables Events. Recorder record.EventRecorder + // ServedNamespaces is the config-plane namespaces this instance reconciles. A ClusterWatchRule + // naming a GitTarget outside them belongs to another instance and is left alone. The zero value + // serves every namespace. + ServedNamespaces watch.ServedNamespaces } // +kubebuilder:rbac:groups=configbutler.ai,resources=clusterwatchrules,verbs=get;list;watch;create;update;patch;delete @@ -107,6 +111,11 @@ func (r *ClusterWatchRuleReconciler) Reconcile(ctx context.Context, req ctrl.Req return ctrl.Result{}, err } + if !r.ServedNamespaces.Serves(clusterRule.Spec.GitTargetRef.Namespace) { + r.forgetUnservedClusterWatchRule(req.NamespacedName, log) + return ctrl.Result{}, nil + } + log.Info("Starting ClusterWatchRule validation", "name", clusterRule.Name, "target", clusterRule.Spec.GitTargetRef, @@ -133,6 +142,22 @@ func (r *ClusterWatchRuleReconciler) Reconcile(ctx context.Context, req ctrl.Req return r.reconcileClusterWatchRuleViaTarget(ctx, st, &clusterRule) } +// forgetUnservedClusterWatchRule leaves a rule another instance serves alone: no status write, so +// two instances never fight over its conditions. A rule re-pointed out of the served namespaces is +// dropped from the store, and the plan that ran its streams is redone. +func (r *ClusterWatchRuleReconciler) forgetUnservedClusterWatchRule(key types.NamespacedName, log logr.Logger) { + if !r.RuleStore.HasClusterWatchRule(key) { + return + } + log.Info("ClusterWatchRule now names a GitTarget in a namespace this instance does not serve; dropping it", + "name", key.Name) + r.RuleStore.DeleteClusterWatchRule(key) + if r.WatchManager != nil { + r.WatchManager.TriggerAllRuleChange() + } + telemetry.ForgetResourceConditions(conditionKindClusterWatchRule, key.Namespace, key.Name) +} + // reconcileClusterWatchRuleViaTarget validates and stores a ClusterWatchRule that references a GitTarget. func (r *ClusterWatchRuleReconciler) reconcileClusterWatchRuleViaTarget( ctx context.Context, diff --git a/internal/rulestore/store.go b/internal/rulestore/store.go index 9941b918b..a66fa500a 100644 --- a/internal/rulestore/store.go +++ b/internal/rulestore/store.go @@ -331,6 +331,14 @@ func (s *RuleStore) DeleteClusterWatchRule(key types.NamespacedName) { delete(s.clusterRules, key) } +// HasClusterWatchRule reports whether the store holds a compiled ClusterWatchRule under key. +func (s *RuleStore) HasClusterWatchRule(key types.NamespacedName) bool { + s.mu.RLock() + defer s.mu.RUnlock() + _, ok := s.clusterRules[key] + return ok +} + // MarkReady records that initial WatchRule and ClusterWatchRule bootstrap has completed. func (s *RuleStore) MarkReady() { s.mu.Lock() diff --git a/internal/watch/bootstrap.go b/internal/watch/bootstrap.go index 7e57338fc..734263d8e 100644 --- a/internal/watch/bootstrap.go +++ b/internal/watch/bootstrap.go @@ -37,6 +37,9 @@ func (m *Manager) bootstrapRuleStore(ctx context.Context, log logr.Logger) error } for i := range clusterWatchRules.Items { rule := clusterWatchRules.Items[i] + if !m.ServedNamespaces.Serves(rule.Spec.GitTargetRef.Namespace) { + continue + } if err := m.bootstrapClusterWatchRule(ctx, rule); err != nil { log.Error(err, "Failed to seed ClusterWatchRule into store", "name", rule.Name) } diff --git a/internal/watch/bootstrap_admission_test.go b/internal/watch/bootstrap_admission_test.go index 55805952b..a8da8ea4b 100644 --- a/internal/watch/bootstrap_admission_test.go +++ b/internal/watch/bootstrap_admission_test.go @@ -191,6 +191,35 @@ func TestBootstrapRuleStore_SkipsUnauthorizedRuleButStillReady(t *testing.T) { "the store must still be marked ready: a refused rule is a refusal, not a startup failure") } +// TestBootstrapRuleStore_SeedsNoClusterWatchRuleAnotherInstanceServes: under --watch-namespaces a +// ClusterWatchRule naming a GitTarget outside the served namespaces belongs to another instance, +// and is not seeded even though its provider would admit it. +func TestBootstrapRuleStore_SeedsNoClusterWatchRuleAnotherInstanceServes(t *testing.T) { + servedTarget := bootGitTarget() + servedTarget.Name, servedTarget.Namespace = "ok-mirror", "team-ok" + servedRule := bootClusterWatchRule() + servedRule.Name = "served-rule" + servedRule.Spec.GitTargetRef = meta.NamespacedObjectReference{Name: "ok-mirror", Namespace: "team-ok"} + + m := bootManager(t, + bootGitTarget(), bootGitProvider(), bootClusterWatchRule(), + servedTarget, + &configv1alpha3.GitProvider{ObjectMeta: metav1.ObjectMeta{Name: "git", Namespace: "team-ok"}}, + servedRule, + bootKubeProvider(mirrorGrants(bootTargetNS, "team-ok")), + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: bootTargetNS}}, + &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "team-ok"}}, + ) + var err error + m.ServedNamespaces, err = ParseServedNamespaces("team-ok," + bootProviderNS) + require.NoError(t, err) + + require.NoError(t, m.bootstrapRuleStore(context.Background(), logr.Discard())) + + assert.Equal(t, []string{"served-rule"}, bootCompiledNames(m)) + assert.True(t, m.RuleStore.IsReady()) +} + // TestBootstrapClusterWatchRule_WildcardStillResolvesItsClusterScopedTypes pins what a wildcard // selector means on the cluster kind. Which scope a rule watches is decided by which KIND you // write, never by a field, so `resources: ["*"]` on a ClusterWatchRule resolves the cluster-scoped diff --git a/internal/watch/manager.go b/internal/watch/manager.go index fe836ea3c..5196b56f2 100644 --- a/internal/watch/manager.go +++ b/internal/watch/manager.go @@ -81,6 +81,9 @@ type Manager struct { // builds its observations, so each TypeRecord carries the right Sensitive fact. The // zero value still treats core Secrets as sensitive. SensitiveResources types.SensitiveResourcePolicy + // ServedNamespaces is the config-plane namespaces this instance reconciles. Bootstrap seeds no + // ClusterWatchRule naming a GitTarget outside it. The zero value serves every namespace. + ServedNamespaces ServedNamespaces // dynamicClient overrides the config-built dynamic client when non-nil. // Used in tests to inject a fake client without a real REST config. diff --git a/internal/watch/served_namespaces.go b/internal/watch/served_namespaces.go new file mode 100644 index 000000000..070f0f814 --- /dev/null +++ b/internal/watch/served_namespaces.go @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: Apache-2.0 + +package watch + +import ( + "fmt" + "sort" + "strings" + + "k8s.io/apimachinery/pkg/util/validation" +) + +// ServedNamespaces is the set of config-plane namespaces one operator instance reconciles, from +// --watch-namespaces. The zero value serves every namespace. +// +// The manager's cache already confines the namespaced kinds to the set. A ClusterWatchRule is +// cluster-scoped, so every instance sees every one; it belongs to the instance serving the +// namespace of the GitTarget it names, and the others leave it alone, status included. +type ServedNamespaces struct { + names map[string]struct{} +} + +// ParseServedNamespaces reads a comma-separated list of namespace names. Empty serves every +// namespace. A name that is not a valid namespace name, or an empty entry, is an error. +func ParseServedNamespaces(raw string) (ServedNamespaces, error) { + if strings.TrimSpace(raw) == "" { + return ServedNamespaces{}, nil + } + names := make(map[string]struct{}) + for entry := range strings.SplitSeq(raw, ",") { + name := strings.TrimSpace(entry) + if name == "" { + return ServedNamespaces{}, fmt.Errorf("--watch-namespaces %q has an empty entry", raw) + } + if errs := validation.IsDNS1123Label(name); len(errs) > 0 { + return ServedNamespaces{}, fmt.Errorf("--watch-namespaces: %q is not a namespace name: %s", + name, strings.Join(errs, "; ")) + } + names[name] = struct{}{} + } + return ServedNamespaces{names: names}, nil +} + +// All reports whether every namespace is served. +func (s ServedNamespaces) All() bool { return len(s.names) == 0 } + +// Serves reports whether namespace is one this instance reconciles. +func (s ServedNamespaces) Serves(namespace string) bool { + if s.All() { + return true + } + _, ok := s.names[namespace] + return ok +} + +// Names returns the served namespaces, sorted; nil when every namespace is served. +func (s ServedNamespaces) Names() []string { + if s.All() { + return nil + } + out := make([]string, 0, len(s.names)) + for name := range s.names { + out = append(out, name) + } + sort.Strings(out) + return out +} diff --git a/internal/watch/served_namespaces_test.go b/internal/watch/served_namespaces_test.go new file mode 100644 index 000000000..ae1943d69 --- /dev/null +++ b/internal/watch/served_namespaces_test.go @@ -0,0 +1,39 @@ +// SPDX-License-Identifier: Apache-2.0 + +package watch + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestParseServedNamespaces_EmptyServesEveryNamespace(t *testing.T) { + for _, raw := range []string{"", " "} { + served, err := ParseServedNamespaces(raw) + require.NoError(t, err) + assert.True(t, served.All()) + assert.True(t, served.Serves("anything")) + assert.Nil(t, served.Names()) + } +} + +func TestParseServedNamespaces_ServesOnlyTheNamedNamespaces(t *testing.T) { + served, err := ParseServedNamespaces(" team-b,team-a ,team-b") + require.NoError(t, err) + + assert.False(t, served.All()) + assert.True(t, served.Serves("team-a")) + assert.True(t, served.Serves("team-b")) + assert.False(t, served.Serves("team-c")) + assert.False(t, served.Serves(""), "a cluster-scoped object's empty namespace is not served") + assert.Equal(t, []string{"team-a", "team-b"}, served.Names()) +} + +func TestParseServedNamespaces_RefusesWhatIsNotANamespaceName(t *testing.T) { + for _, raw := range []string{"team-a,", ",team-a", "team-a,,team-b", "Team-A", "team_a", "team-a/x"} { + _, err := ParseServedNamespaces(raw) + assert.Error(t, err, raw) + } +} From 9c1fe7fe91cac53ede9d0ce8ee15918bd0eb378e Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Sat, 24 Oct 2026 21:10:54 +0000 Subject: [PATCH 2/5] feat(applier): count each apply an applier claim did not start claimed_resource_reconciled_total{applier_kind, claim_namespace, claim_name} counts a held Kustomization resumed, or a held Application's automated sync switched on, by somebody else. The ClaimedResourceReconciled Event said so already, but the API server folds identical Events and expires them, so it made a poor alarm. The Flux guide's troubleshooting table gains the common cause of a recurring one: a controller that writes the Kustomization's whole spec without spec.suspend, which SuspendOwnedElsewhere cannot report. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/bi-directional-argocd.md | 4 +-- docs/bi-directional-flux.md | 6 ++-- docs/interpreting-metrics.md | 5 ++++ internal/controller/argocdclaim_hold.go | 1 + internal/controller/argocdclaim_hold_test.go | 7 +++++ internal/controller/fluxclaim_hold.go | 29 ++++++++++++++++++-- internal/controller/fluxclaim_hold_test.go | 16 +++++++++++ internal/telemetry/apply_requests.go | 11 ++++++++ internal/telemetry/exporter.go | 1 + 9 files changed, 74 insertions(+), 6 deletions(-) diff --git a/docs/bi-directional-argocd.md b/docs/bi-directional-argocd.md index fd1ae1f31..fa0c7941a 100644 --- a/docs/bi-directional-argocd.md +++ b/docs/bi-directional-argocd.md @@ -39,8 +39,8 @@ for each of these, and for the settings that look like a fix and are not. server-side apply under its own field manager, `gitops-reverser`, and touches nothing else of `automated`, so `prune` and `selfHeal` stay as they are. Switching automated sync on again (Argo CD's *Enable Auto-Sync*, `kubectl edit`) is undone within a minute, with a -`ClaimedResourceReconciled` Event; a sync Argo CD started in that minute runs to its end, -unchecked against the branch. +`ClaimedResourceReconciled` Event, and counted in `claimed_resource_reconciled_total`; a sync Argo CD +started in that minute runs to its end, unchecked against the branch. Deleting the applier claim, or no longer naming it from any `GitTarget`, hands the switch back as it was: syncing on its own if it did before, manual if it was. The applier claim records which in `status.previousSuspendState` when it first switches it off: `NotSuspended` for automated sync on, diff --git a/docs/bi-directional-flux.md b/docs/bi-directional-flux.md index ff529e1d2..6e436f0c2 100644 --- a/docs/bi-directional-flux.md +++ b/docs/bi-directional-flux.md @@ -477,8 +477,9 @@ tip, the `Kustomization`s GitOps Reverser holds ignore the new artifact, and one ## Overriding by hand - **`flux resume ks checkout`** applies the `Kustomization` at once, unchecked. Within a minute the - applier claim suspends it again, records a `ClaimedResourceReconciled` Event, and shows the apply as an - `Unresolved` operation that completes once nothing it started is running. It does so every time. + applier claim suspends it again, records a `ClaimedResourceReconciled` Event, counts it in + `claimed_resource_reconciled_total`, and shows the apply as an `Unresolved` operation that completes + once nothing it started is running. It does so every time. - **`flux reconcile ks checkout`** does nothing visible while the `Kustomization` is suspended: Flux marks the request handled without applying. - **`flux suspend ks checkout`** is taken back with a token of GitOps Reverser's. @@ -513,6 +514,7 @@ flux get ks checkout -n flux-system | --- | --- | --- | | `FluxClaim` `Configured=False`, `ApplierNotFound` | No `Kustomization` with that namespace and name in the applier claim's cluster, or Flux is not installed there | Create it, or recreate the applier claim with the right `kustomizationRef`; an omitted namespace is the applier claim's | | `FluxClaim` `Configured=False`, `SuspendOwnedElsewhere` | The manifest that applies the `Kustomization` sets `spec.suspend` | Remove the field from that manifest | +| Repeated `ClaimedResourceReconciled` Events (a rising `claimed_resource_reconciled_total`), each followed by applier claim `OperationUnresolved` | A controller writes the `Kustomization` with an Update of its whole spec that omits `spec.suspend`, such as `controllerutil.CreateOrUpdate` with a spec it builds from scratch. An Update takes no ownership of a field it removes, so `SuspendOwnedElsewhere` cannot report it, and Flux applies unchecked on each of that controller's writes until the applier claim suspends it again | Make that controller copy `spec.suspend` from the object it read into the spec it writes, or apply with server-side apply and leave the field out | | `ApplierClaimReady=False`, `SourceMismatch`, or applier claim `UnsupportedSource` | The message names the broken [`GitRepository` rule](#the-gitrepository) | Fix the `GitRepository`; the applier claim then adopts the `Kustomization`, and the apply that was waiting is asked for | | `FluxClaim` `Configured=False`, `ApplierNamespaceNotAllowed` | The `Kustomization` is in a namespace the grants do not give, such as `flux-system` | Add it to `applierNamespaces` in the `KubeProvider`'s `spec.allowedApplierClaims` (Helm: `kubeProvider.allowedApplierClaims`) | | `FluxClaim` `Configured=Unknown`, `NotBound` | A `GitTarget` names the applier claim, but cannot be judged yet: its `GitProvider` or `KubeProvider` does not exist | Create the missing object; the message names it | diff --git a/docs/interpreting-metrics.md b/docs/interpreting-metrics.md index 3cd870ec7..5099d583f 100644 --- a/docs/interpreting-metrics.md +++ b/docs/interpreting-metrics.md @@ -837,6 +837,7 @@ else pushed. See [who applies this folder](configuration.md#who-applies-this-fol | --- | --- | --- | --- | | `apply_requests_total` | counter | `applier_kind`, `reason`, `outcome` | One per attempt to one applier. `applier_kind` is `kustomization` (a Flux `Kustomization`) or `application` (an Argo CD `Application`); `reason` is `ForeignCommit` / `Refusal` / `Repair` / `CatchUp`, for either kind (`CatchUp` adopts an object the API created in the folder, or converges once when the worker starts and once when a `GitTarget` first names the applier claim); `outcome` is `sent` (the applier was asked and the ask landed; it says nothing about how the apply went. What needs no apply, a converge of the commit the applier last applied or a branch the remote does not carry, the branch worker never sends, so it is not counted), `failed` (could not ask, including an applier that applies another repository or branch, an `Application` with automated sync on, or one whose running sync did not end within 5 minutes; the branch worker keeps the reason pending and decides the retry after a flush, so a failure that persists keeps counting at its backoff) or `dropped` (no `GitTarget` may still ask for that reason: each was deleted or recreated, moved to another branch or applier, is no longer admitted by its `KubeProvider`, or withdrew its consent to a `Refusal`) or `stale` (asked nothing because the branch tip moved before the ask landed; the worker decides again at once, at the new tip, and frequent stale attempts mean the branch keeps moving within an attempt's lifetime) | | `apply_revision_mismatches_total` | counter | `applier_kind` | One per apply operation that succeeded on another revision than the commit GitOps Reverser checked. A `Kustomization` applies whatever its `GitRepository` artifact holds when it reconciles; the artifact is read again just before the resume, so this is somebody fetching the source in between (a person, or the source's own interval). The apply window still withholds a late write over a newer edit; a steady rate means something else keeps fetching the source | +| `claimed_resource_reconciled_total` | counter | `applier_kind`, `claim_namespace`, `claim_name` | One per apply an applier claim did not start: a held `Kustomization` resumed (`flux resume`, or a controller writing its spec without `spec.suspend`), or a held `Application`'s automated sync switched on, by somebody else. From then until the claim takes it back, on its next look within a minute, the applier applies whatever its source holds, unchecked against the branch. The claim records a `ClaimedResourceReconciled` Event for each as well, but the API server folds identical Events into one and expires them, so alert on this counter. A `Kustomization` resumed so also shows the apply as an `Unresolved` operation on the claim | | `apply_window_collisions_total` | counter | `provider_namespace`, `provider_name`, `branch`, `outcome` | One per write by the applier, while an apply it was asked for runs, to an object the API side edited after the flush before that request: the apply arriving late over a newer edit. `outcome` is `withheld` (proven to be the applier's write, by its field manager alone, with an entry as new as the edit or newer: `kustomize-controller`, or `argocd-controller` / `argocd-application-controller`; so not mirrored: Git keeps the API's value, and a `Repair` of the applier follows, which restores it on the cluster) or `ambiguous` (the applier and another field manager wrote in the same second, so it was mirrored as ever; if it was the apply, the revert is in Git as the applier's commit). An applier entry older than the edit is not counted: it is the API's undo of an edit, mirrored | | `apply_echoes_total` | counter | `provider_namespace`, `provider_name`, `branch`, `outcome` | One per write by the applier's own field managers alone, judged by the apply window: an apply GitOps Reverser asked for landing, object by object. `outcome` is `not_mirrored` (the applier wrote the object last, at or after the earliest send of the apply windows still open, and nobody edited it in the window: the apply itself, whose content is the commit Git already holds, so it is not mirrored) or `late` (the applier's managers alone wrote the object last with no apply window open: an echo arriving more than 10 seconds after its operation completed, or an apply somebody else started; it is mirrored as an edit). `not_mirrored` rises with every apply and needs no action. A steady `late` rate means applies keep landing outside the windows, and such a write can revert a newer Git-side change to the object: look for a person or a tool applying the folder by hand (`flux reconcile`, a manual sync), or for watch events reaching the worker long after the apply | | `apply_ownership_conflicts_total` | counter | `provider_namespace`, `provider_name`, `branch` | One per object another applier owns that the branch worker kept out of a coordinated folder: a live write not mirrored, or a snapshot object neither upserted nor swept. The owner is read from Flux's owner labels (`kustomize.toolkit.fluxcd.io/name` and `/namespace`) or Argo CD's tracking annotation (`argocd.argoproj.io/tracking-id`). Any non-zero rate is a misconfiguration: another `Kustomization` or `Application` applies objects into a namespace the folder mirrors, and two appliers applying one object fight each other. The worker logs each object once, naming both appliers; narrow the `WatchRule`, or move the object to one applier | @@ -866,6 +867,9 @@ sum by (receiver_namespace, receiver_name) ( # Repair that follows; ambiguous ones may have put the applier's revert in Git. sum by (provider_namespace, provider_name, branch, outcome) ( increase(gitopsreverser_apply_window_collisions_total[1h])) > 0 +# An applier that somebody else made apply, by claim: each was unchecked against the branch. +sum by (applier_kind, claim_namespace, claim_name) ( + increase(gitopsreverser_claimed_resource_reconciled_total[1h])) > 0 # An applier stuck mid-apply: nothing more is asked of it until its operation completes. max by (provider_namespace, provider_name, branch) (gitopsreverser_apply_operations_unresolved) > 0 # Applier writes mirrored as edits because no apply window was open. A steady rate can revert @@ -1350,6 +1354,7 @@ sum by (outcome) (rate(gitopsreverser_secret_encryptions_total[5m])) | `(time() - …_fact_follower_last_success_timestamp_seconds > 600) or (…_transport_info == 1 unless on() …_fact_follower_last_success_timestamp_seconds)`, `for: 10m` | The fact follower is wedged; attribution is degrading to committer-authored cluster-wide. Both arms are needed — see below. | | `rate(gitopsreverser_git_resync_failures_total[15m]) > 0` sustained | Background resyncs are not committing; the folder relies on steady-state events to catch up. | | `increase(gitopsreverser_apply_ownership_conflicts_total[1h]) > 0` | Another `Kustomization` or `Application` applies objects into a coordinated folder; they are kept out of Git, and the log names both appliers. | +| `increase(gitopsreverser_claimed_resource_reconciled_total[1h]) > 0` | Somebody else made a held applier apply, unchecked against the branch; the labels name the applier claim. A steady rate on one claim is a controller that rewrites the `Kustomization` without `spec.suspend`. | | `increase(gitopsreverser_stale_snapshots_total{outcome="used"}[1h]) > 0` | An applier has been failing for at least 5 minutes, so a resync decided a snapshot taken while Git was ahead and reverted part of the unapplied commit. | | `gitopsreverser_api_catalog_group_versions{state="degraded"} > 0` | Part of the API surface is hidden behind a broken APIService. | | `time() - gitopsreverser_watch_plan_oldest_dirty_since_timestamp_seconds > 120`, `for: 5m` | A GitTarget cannot be planned — its source cluster or catalog is unreachable — and its mirror is not converging. The `time() -` is not optional: the gauge is a Unix timestamp, so a bare `> 120` is true of every instant since 1970 and would fire on ordinary planning. | diff --git a/internal/controller/argocdclaim_hold.go b/internal/controller/argocdclaim_hold.go index 909d37d27..ed6923ce3 100644 --- a/internal/controller/argocdclaim_hold.go +++ b/internal/controller/argocdclaim_hold.go @@ -163,6 +163,7 @@ func (r *ApplierClaimReconciler) switchOff( r.event(claim, corev1.EventTypeNormal, EventReasonAdopted, "switched automated sync of Application %s off: from now on GitOps Reverser starts every sync", ref) case state.On: + recordClaimedResourceReconciled(ctx, applierKindApplication, claim) r.event(claim, corev1.EventTypeNormal, EventReasonClaimedResourceReconciled, "automated sync of Application %s was switched on outside GitOps Reverser, which switched it off "+ "again; a sync Argo CD started meanwhile was not checked against the branch", ref) diff --git a/internal/controller/argocdclaim_hold_test.go b/internal/controller/argocdclaim_hold_test.go index 83e52d2b3..1474526bd 100644 --- a/internal/controller/argocdclaim_hold_test.go +++ b/internal/controller/argocdclaim_hold_test.go @@ -24,6 +24,7 @@ import ( "github.com/ConfigButler/gitops-reverser/internal/applier" "github.com/ConfigButler/gitops-reverser/internal/applier/argosim" "github.com/ConfigButler/gitops-reverser/internal/applier/fluxsim" + "github.com/ConfigButler/gitops-reverser/internal/telemetry" ) var checkoutApplication = k8stypes.NamespacedName{Namespace: "argocd", Name: "checkout"} @@ -147,9 +148,13 @@ func TestArgoCDClaim_SwitchesAutomatedSyncOff(t *testing.T) { // switched on outside GitOps Reverser is switched off again, every time, with a Normal // ClaimedResourceReconciled Event each time. func TestArgoCDClaim_AutomatedSyncSwitchedOnIsSwitchedOffAgain(t *testing.T) { + reader, err := telemetry.InitTestExporter() + require.NoError(t, err) f := newArgoHoldFixture(t, nil, applierArgoApplication(nil)) reconcileClaim(t, f.r) f.events() + _, counted := claimedResourceReconciledCount(reader, "application") + assert.False(t, counted, "an adoption is no sync somebody else started") for round := range 2 { f.switchOn(t) @@ -163,6 +168,8 @@ func TestArgoCDClaim_AutomatedSyncSwitchedOnIsSwitchedOffAgain(t *testing.T) { require.Len(t, events, 1, "an Event every time") assert.True(t, strings.HasPrefix(events[0], "Normal ClaimedResourceReconciled automated sync of Application "+ "argocd/checkout was switched on outside GitOps Reverser, which switched it off again"), events[0]) + count, _ := claimedResourceReconciledCount(reader, "application") + assert.Equal(t, int64(round+1), count, "counted every time, where the Event is aggregated") } } diff --git a/internal/controller/fluxclaim_hold.go b/internal/controller/fluxclaim_hold.go index a17adee38..2499e45f8 100644 --- a/internal/controller/fluxclaim_hold.go +++ b/internal/controller/fluxclaim_hold.go @@ -8,6 +8,8 @@ import ( "sync" "time" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/metric" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" apimeta "k8s.io/apimachinery/pkg/api/meta" @@ -22,6 +24,7 @@ import ( configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" "github.com/ConfigButler/gitops-reverser/internal/applier" "github.com/ConfigButler/gitops-reverser/internal/git" + "github.com/ConfigButler/gitops-reverser/internal/telemetry" ) // A FluxClaim holds its Kustomization suspended while idle; the branch workers resume it for each @@ -274,7 +277,7 @@ func (r *ApplierClaimReconciler) hold( if err != nil { return keptCondition(claim.Status.Conditions, ApplierClaimConditionInControl, err.Error()) } - r.suspended(claim, token, wasInControl(claim) || held.inFlight()) + r.suspended(ctx, claim, token, wasInControl(claim) || held.inFlight()) return suspending(fmt.Sprintf("Kustomization %s is being suspended", ref)) case !held.suspension.Held || adopt: return r.takeOverSuspend(ctx, st, claim, held) @@ -415,13 +418,16 @@ func suspendState(suspended bool) configbutleraiv1alpha3.SuspendState { // suspended records a Kustomization just suspended: an adoption, or, for one held before, an // external resume, recorded as an Unresolved operation that completes once token is handled. -func (r *ApplierClaimReconciler) suspended(claim *configbutleraiv1alpha3.FluxClaim, token string, resumed bool) { +func (r *ApplierClaimReconciler) suspended( + ctx context.Context, claim *configbutleraiv1alpha3.FluxClaim, token string, resumed bool, +) { ref := claim.Applier() if !resumed { r.event(claim, corev1.EventTypeNormal, EventReasonAdopted, "suspended Kustomization %s: from now on GitOps Reverser starts every apply it makes", ref) return } + recordClaimedResourceReconciled(ctx, applierKindKustomization, claim) r.event(claim, corev1.EventTypeNormal, EventReasonClaimedResourceReconciled, "Kustomization %s was resumed outside GitOps Reverser, which suspended it again; the apply "+ "that resume started was not checked against the branch", ref) @@ -905,6 +911,25 @@ func (r *ApplierClaimReconciler) event(object client.Object, eventType, reason, } } +// applier_kind label values of claimed_resource_reconciled_total, as apply_requests_total spells them. +const ( + applierKindKustomization = "kustomization" + applierKindApplication = "application" +) + +// recordClaimedResourceReconciled counts an apply the applier claim did not start, beside its +// ClaimedResourceReconciled Event, which the API server aggregates and expires. +func recordClaimedResourceReconciled(ctx context.Context, applierKind string, claim client.Object) { + if telemetry.ClaimedResourceReconciledTotal == nil { + return + } + telemetry.ClaimedResourceReconciledTotal.Add(ctx, 1, metric.WithAttributes( + attribute.String("applier_kind", applierKind), + attribute.String("claim_namespace", claim.GetNamespace()), + attribute.String("claim_name", claim.GetName()), + )) +} + // publishAlsoAppliedBy sets the applier claim's status.source.alsoAppliedBy: the objects applying its // GitRepository in the applier's cluster, other than its own Kustomization, that no FluxClaim on the // same KubeProvider holds. A listing that fails publishes none. Information only (decision 2): it never diff --git a/internal/controller/fluxclaim_hold_test.go b/internal/controller/fluxclaim_hold_test.go index 9641e7184..e11b74340 100644 --- a/internal/controller/fluxclaim_hold_test.go +++ b/internal/controller/fluxclaim_hold_test.go @@ -13,6 +13,7 @@ import ( "github.com/fluxcd/pkg/apis/meta" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + sdkmetric "go.opentelemetry.io/otel/sdk/metric" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -29,6 +30,7 @@ import ( "github.com/ConfigButler/gitops-reverser/internal/applier" "github.com/ConfigButler/gitops-reverser/internal/applier/fluxsim" "github.com/ConfigButler/gitops-reverser/internal/git" + "github.com/ConfigButler/gitops-reverser/internal/telemetry" ) var checkoutKustomization = k8stypes.NamespacedName{Namespace: "flux-system", Name: "checkout"} @@ -115,9 +117,13 @@ func TestFluxClaim_AdoptsItsKustomization(t *testing.T) { // Kustomization is suspended again with a token, every time, with a Normal Event each time, and // recorded as an Unresolved operation that completes once nothing that resume started is running. func TestFluxClaim_AnExternalResumeIsSuspendedAgain(t *testing.T) { + reader, err := telemetry.InitTestExporter() + require.NoError(t, err) f := newHoldFixture(t, testFluxClaim(claimLocalProvider)) reconcileClaim(t, f.r) f.events() + _, counted := claimedResourceReconciledCount(reader, "kustomization") + assert.False(t, counted, "an adoption is no apply somebody else started") for round := range 2 { patch := []byte(`{"spec":{"suspend":false}}`) @@ -139,6 +145,8 @@ func TestFluxClaim_AnExternalResumeIsSuspendedAgain(t *testing.T) { require.Len(t, events, 1, "an Event every time") assert.True(t, strings.HasPrefix(events[0], "Normal ClaimedResourceReconciled "+ "Kustomization flux-system/checkout was resumed outside GitOps Reverser"), events[0]) + count, _ := claimedResourceReconciledCount(reader, "kustomization") + assert.Equal(t, int64(round+1), count, "counted every time, where the Event is aggregated") got := reconcileClaim(t, f.r).(*configbutleraiv1alpha3.FluxClaim) require.NotNil(t, got.Status.Operation) @@ -151,6 +159,14 @@ func TestFluxClaim_AnExternalResumeIsSuspendedAgain(t *testing.T) { assert.Len(t, f.sim.Applied(), 2, "each resume applied once, unchecked; that is what the record says") } +// claimedResourceReconciledCount reads claimed_resource_reconciled_total for the test claim. +func claimedResourceReconciledCount(reader *sdkmetric.ManualReader, applierKind string) (int64, bool) { + return telemetry.CollectInt64Sum(reader, "gitopsreverser_claimed_resource_reconciled_total", + map[string]string{ + "applier_kind": applierKind, "claim_namespace": claimKey.Namespace, "claim_name": claimKey.Name, + }) +} + // TestFluxClaim_LeavesABranchWorkersOperationAlone: a worker records its operation before it // resumes the Kustomization, so a resumed Kustomization with an operation of the worker's in flight // is that operation, not an external resume; the worker suspends it again itself. diff --git a/internal/telemetry/apply_requests.go b/internal/telemetry/apply_requests.go index ccda08aed..a759cbe0d 100644 --- a/internal/telemetry/apply_requests.go +++ b/internal/telemetry/apply_requests.go @@ -36,6 +36,17 @@ import "go.opentelemetry.io/otel/metric" // each GitTarget carry that identity instead. var ApplyRequestsTotal metric.Int64Counter +// ClaimedResourceReconciledTotal counts the applies an applier claim did not start, labelled by +// {applier_kind, claim_namespace, claim_name}: a held Kustomization resumed, or a held Application's +// automated sync switched on, by somebody else. The applier may apply unchecked from then until the +// claim takes it back, which it does on its next look, within a minute. The ClaimedResourceReconciled +// Event on the claim says the same, but the API server aggregates identical Events and expires them, +// so alert on this instead. A Kustomization resumed so also shows its apply as an Unresolved operation. +// +// The claim is named: it holds exactly one applier, so the label is its identity and stays bounded +// by the claims that exist. +var ClaimedResourceReconciledTotal metric.Int64Counter + // ApplyRevisionMismatchesTotal counts apply operations that succeeded on another revision than the // commit GitOps Reverser checked, labelled by {applier_kind}. A Flux Kustomization applies whatever // its GitRepository's artifact holds when it reconciles; the artifact is read again just before the diff --git a/internal/telemetry/exporter.go b/internal/telemetry/exporter.go index 18351c636..1e4963938 100644 --- a/internal/telemetry/exporter.go +++ b/internal/telemetry/exporter.go @@ -608,6 +608,7 @@ func registerCounters() error { {"gitopsreverser_watch_plan_triggers_total", &WatchPlanTriggersTotal}, {"gitopsreverser_apply_requests_total", &ApplyRequestsTotal}, {"gitopsreverser_apply_revision_mismatches_total", &ApplyRevisionMismatchesTotal}, + {"gitopsreverser_claimed_resource_reconciled_total", &ClaimedResourceReconciledTotal}, {"gitopsreverser_apply_window_collisions_total", &ApplyWindowCollisionsTotal}, {"gitopsreverser_apply_echoes_total", &ApplyEchoesTotal}, {"gitopsreverser_stale_snapshots_total", &StaleSnapshotsTotal}, From 75bae8530429031de582ea02f5a49205bd18930f Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Sat, 24 Oct 2026 21:10:54 +0000 Subject: [PATCH 3/5] test(attribution): name the deleter when a controller clears the finalizer A terminating object's removal is credited to its deletion, never to the write at its version. That write can be a finalizer cleared by an UPDATE as well as a PATCH, and its fact can be filed before or after the deletion's; pin all four. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../queue/fact_index_removal_cause_test.go | 57 +++++++++++++++---- 1 file changed, 47 insertions(+), 10 deletions(-) diff --git a/internal/queue/fact_index_removal_cause_test.go b/internal/queue/fact_index_removal_cause_test.go index fa3e163a4..adbd26629 100644 --- a/internal/queue/fact_index_removal_cause_test.go +++ b/internal/queue/fact_index_removal_cause_test.go @@ -124,19 +124,56 @@ func TestFactIndex_RemovalNamesAnRVOnlyWriteAtItsOwnVersion(t *testing.T) { require.Equal(t, "bob", resolution.Fact.Author) } -// For a terminating object the fact at its version can be the finalizer PATCH, which says who cleared -// a finalizer rather than who asked for the deletion. Only a deletion counts. +// For a terminating object the fact at its version can be the finalizer PATCH or UPDATE, which says +// who cleared a finalizer rather than who asked for the deletion. Only a deletion counts. An operator +// clearing its finalizer with a whole-object Update (controllerutil.RemoveFinalizer, then Update) is +// the common case. func TestFactIndex_RemovalOfATerminatingObjectNeedsTheDeletion(t *testing.T) { - const route = "removal-terminating" - h := newFactIndexHarness(t, FactIndexConfig{}) - h.publish(factIndexTestStream(route), verbFact("finalizer-controller", "11", "patch")) - h.waitForFacts(1) + for _, verb := range []string{"patch", "update"} { + t.Run(verb, func(t *testing.T) { + route := "removal-terminating-" + verb + h := newFactIndexHarness(t, FactIndexConfig{}) + h.publish(factIndexTestStream(route), verbFact("finalizer-controller", "11", verb)) + h.waitForFacts(1) - h.absent(removalQuery(route, true)) + h.absent(removalQuery(route, true)) - h.publish(factIndexTestStream(route), verbFact("carol", "11", "delete")) - resolution := h.resolve(removalQuery(route, true)) - require.Equal(t, "carol", resolution.Fact.Author, "the deletion names the initiator") + h.publish(factIndexTestStream(route), verbFact("carol", "11", "delete")) + resolution := h.resolve(removalQuery(route, true)) + require.Equal(t, "carol", resolution.Fact.Author, "the deletion names the initiator") + }) + } +} + +// The reported case, in both orders: Carol deletes a resource her operator finalizes, and the +// operator's service account clears the finalizer, the write at the removal's own version. Whichever +// fact the index files first, the removal names Carol, at once. +func TestFactIndex_AFinalizerClearedByAControllerNamesTheDeleter(t *testing.T) { + for _, verb := range []string{"patch", "update"} { + for name, controllerFirst := range map[string]bool{"controller first": true, "deletion first": false} { + t.Run(verb+" "+name, func(t *testing.T) { + route := "removal-finalizer-" + verb + if controllerFirst { + route += "-controller-first" + } + h := newFactIndexHarness(t, FactIndexConfig{}) + deletion := verbFact("carol", "10", "delete") + clearing := verbFact("system:serviceaccount:team-a:operator", "11", verb) + facts := []AuthorFact{deletion, clearing} + if controllerFirst { + facts = []AuthorFact{clearing, deletion} + } + h.publish(factIndexTestStream(route), facts...) + h.waitForFacts(len(facts)) + + started := time.Now() + resolution := h.resolve(removalQuery(route, true)) + require.Equal(t, AttributionDeleteSticky, resolution.Result) + require.Equal(t, "carol", resolution.Fact.Author) + require.Less(t, time.Since(started), time.Second) + }) + } + } } // Bob relabels the object out and Carol deletes it, and both facts arrive in ONE audit batch, Carol's From 5d9a918583d7eec54bf6d032862152192bd530af Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Sat, 24 Oct 2026 21:10:55 +0000 Subject: [PATCH 4/5] docs: close the gitops-api asks this branch answers #32, #31, #23 and the first step of #28 are answered here; #11 shipped in 0.41.0. #34's two side notes are answered as well. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/TODO.md | 14 ++--- docs/future/gitops-api-open-asks.md | 84 +++++++---------------------- 2 files changed, 24 insertions(+), 74 deletions(-) diff --git a/docs/TODO.md b/docs/TODO.md index 765141dcf..f41e3e100 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -20,7 +20,8 @@ Nothing open. The 0.54.0 release PR's changelog was checked against what ships o discard or race an open commit window or pending live edits ([render-fidelity.md](design/support-boundary/render-fidelity.md) §6b). - [ ] **Watch the held applier**, so a switch-on or a `flux resume` is undone at once instead of - within the one-minute `heldClaimSteadyInterval`. Also makes ask #31 below exact. + within the one-minute `heldClaimSteadyInterval`. Also makes `claimed_resource_reconciled_total` + (ask #31) count at the moment of the resume. [applier-claim-held-applier-watch.md](design/applier-claim-held-applier-watch.md). - [ ] **F3: a Flux fetch is made before the hold is checked**, so a `Kustomization` resumed by hand applies the fetched artifact at once (`internal/applier/flux.go`, `Request`). @@ -51,17 +52,10 @@ Nothing open. The 0.54.0 release PR's changelog was checked against what ships o Their argument for each is in [gitops-api-open-asks.md](future/gitops-api-open-asks.md). -- [ ] **#32**: a troubleshooting row in [bi-directional-flux.md](bi-directional-flux.md) for a - controller that replaces the `Kustomization`'s whole spec with an Update. Docs only. -- [ ] **#31**: a metric for an apply the claim did not start; today it is only the - `ClaimedResourceReconciled` Event, which the API server aggregates and expires. - [ ] **#34**: an e2e with an in-cluster applier and a remote source cluster, provoking one apply window collision. -- [ ] **#23**: verify with their reproduction that #431 fixed deletion-as-intent naming the - finalizing controller instead of the deleter. -- [ ] **#11**: one YAML encoder for `[CREATE]` and `[UPDATE]` bodies (indent churn). -- [ ] **#28**: `--watch-namespaces`, then an owner selector, so two instances can share one - config-plane cluster. It is the same mechanism HA needs. +- [ ] **#28, step 2**: an owner selector, for two instances sharing a namespace. Step 1, + `--watch-namespaces`, shipped; a chart-rendered namespaced Role would let its ClusterRole shrink. - [ ] **#10**: consider the namespace when placement follows siblings. Cosmetic. ## Attribution diff --git a/docs/future/gitops-api-open-asks.md b/docs/future/gitops-api-open-asks.md index bbc76734e..2e1604a15 100644 --- a/docs/future/gitops-api-open-asks.md +++ b/docs/future/gitops-api-open-asks.md @@ -5,39 +5,19 @@ Status: **open asks from a production user**, condensed on 2026-10-24 from their left out: #30 shipped as `GitReceiver` (#432), #33 went with the prefetch in #425, #22 is fixed (`pkg/manifestanalyzer/repo.go`), #26 and #27 shipped (`git_branch_targets`, `commit_requests_total`), #25 shipped in 0.44.0 (#343), and #6 is answered by immutability that -fails at admission with "delete and recreate". Kind and field names are updated to `main` +fails at admission with "delete and recreate". Since revision 15: #32 is a row in +[the Flux guide](../bi-directional-flux.md#troubleshooting); #31 is +`claimed_resource_reconciled_total`, labeled by applier kind and claim; #28's first step is +`--watch-namespaces` ([two instances in one cluster](../configuration.md#two-instances-in-one-cluster)); +ask #23 is pinned at the fact index, for a finalizer cleared by a PATCH or an UPDATE, in either order +(`TestFactIndex_AFinalizerClearedByAControllerNamesTheDeleter`); and #11 shipped in 0.41.0 (#290): +create and update write through one encoder, `internal/yamlstyle`, and a test fails on a second +one. A file GitOps Reverser did not write is re-indented once, on its first edit. Kind and field names are updated to `main` (`FluxClaim` for `FluxTrigger`, `KubeProvider` for `ClusterProvider`, `ClaimedResourceReconciled` for `ResumedElsewhere`). -Their suggested order, as of revision 15: #32, then #31, then #34 when the e2e budget allows; -then #23, #11 and #10; #28 when two owners share a cluster. - -## 32. A troubleshooting row: something replaces the `Kustomization`'s whole spec - -**Ask.** One row in [the Flux guide](../bi-directional-flux.md)'s troubleshooting table: repeated -`ClaimedResourceReconciled` Events mean a controller writes the `Kustomization` with a full-spec -Update that omits `spec.suspend`; make it carry the field across. - -**Why.** The guide covers a manifest that *sets* `spec.suspend` (`SuspendOwnedElsewhere`). The more -common case in practice is a controller that never sets the field and replaces the whole spec -(`controllerutil.CreateOrUpdate` with a spec it builds from scratch). An Update that removes a field -another manager owns takes no ownership of it, so `SuspendOwnedElsewhere` can never fire: the -symptom is a resume Event plus an `OperationUnresolved` on every reconcile of that controller, and -Flux applying unchecked until the claim suspends it again. They reproduced it on purpose. The fix on -their side was three lines; finding it from the guide alone would not have been obvious. - -## 31. Count "resumed elsewhere" in a metric, not only in an `Event` - -**Ask.** A counter for an apply the claim did not start (the resume, and the `OperationUnresolved` -that follows it), labeled like `apply_requests_total`. Possibly also a timestamp in the claim's -status. - -**Why.** A resume behind the operator's back is the one way left for Flux to apply unchecked, so it -is what they most want to alarm on. Today it is only an Event, and an Event is a poor alarm source: -the API server aggregates identical ones (three resumes in three minutes showed as one Event with -`count: 3`) and drops them after an hour. They also saw the detection time vary from 2s to about -75s, which is the claim's steady poll; a watch on the held applier would close that window. That -watch is designed in [watching the held applier](../design/applier-claim-held-applier-watch.md). +Their suggested order, as of revision 15: #34 when the e2e budget allows; then #10; #28's owner +selector when two owners share a cluster. ## 34. An e2e for an in-cluster applier with a remote source cluster @@ -51,20 +31,23 @@ in it, but never saw the apply window withhold anything (`apply_window_collision zero), so the part that reads `managedFields` on the remote object is untested in this topology. An e2e that provokes one collision would settle it. -Two smaller things they noted: `OperationUnresolved` is recorded three times for one operation (on -the claim, the `GitTarget` and the `WatchRule`), and `apply_requests_total{outcome="sent"}` was -said to count both "the applier was asked" and "nothing needed asking". (On `main` an apply that is -not needed is never sent, so it is not counted; check whether that answers them.) +Two smaller things they noted, both answered. `OperationUnresolved` on the `GitTarget` and the +`WatchRule` is not recorded again: each object's readiness takes the reason of the one it depends +on (`ApplierClaimReady` on the `GitTarget`, `GitTargetReady` on the rule), so one operation shows on +three objects by design. And `apply_requests_total{outcome="sent"}` counts only an applier that was +asked: an apply that is not needed is never sent, so it is not counted. ## 28. Two operator instances cannot share one config-plane cluster **Ask.** A way for two instances sharing one config-plane cluster to partition the CR set, so that exactly one instance reconciles a given `GitTarget`. Smallest useful first, and they stack: -1. **`--watch-namespaces`**, a `Cache.DefaultNamespaces` pass-through. It partitions the namespaced - kinds, which since #430 includes `KubeProvider`; only `ClusterWatchRule` stays cluster-scoped. +1. **`--watch-namespaces`**, a `Cache.DefaultNamespaces` pass-through. **Shipped**: it partitions + the namespaced kinds, and a `ClusterWatchRule` goes to the instance serving its `GitTarget`'s + namespace. The ClusterRole stays cluster-wide (a namespaced cache lists per namespace, so a + narrowed role is possible, but the chart does not render one yet). 2. **An explicit owner selector**, a label selector or a `spec.instance`-style field paired with an - `--instance-name` flag, which covers the cluster-scoped kinds too. + `--instance-name` flag. Open, and only needed if two instances must share a namespace. **Why.** It is the same mechanism the chart's replica guard (`charts/gitops-reverser/templates/validate-replica-count.yaml`) says is missing for HA: "Standby @@ -80,33 +63,6 @@ restart for one owner's reasons costs the other's users their commit message, be `RenderMatchesLive` starts `Unknown` after a restart and an open window is dropped. They are not blocked: one shared instance works. -## 23. Deletion-as-intent picks the wrong actor when a controller clears the finalizer - -**Probably fixed by #431; verify with their reproduction.** #431 credits a terminating object's -removal to its deletion evidence first. - -**Ask.** Attribute a deletion-as-intent `DELETE` to the actor who **set** `deletionTimestamp`, not -to whoever makes the next modification while it is set. - -**Why.** Measured on `v0.40.1`: a human deletes a CR that their operator finalizes, and the commit -names the operator's service account. The operator's finalizer-removal `UPDATE` is itself a -modification of a terminating object, so it renders as the `DELETE` and wins the commit; the human's -own delete arrives a moment later and folds to a no-op because the file is already gone. The more -finalizers an object has, the more likely a controller's cleanup write is observed first. - -**Reproduction** (about 90 seconds): create a namespaced CR with a finalizer, mirror it with a -`ClusterWatchRule`, delete it as a human, let the controller clear the finalizer, read the commit. - -## 11. Serialize `[CREATE]` and `[UPDATE]` bodies identically - -**Ask.** One encoder for both paths. - -**Why.** A create rendered list items at 2-space indent, an update at 4, so every install churned -all the lines of a list to carry one field. Unverified since `v0.35.0`, but the two paths still -reach different encoders (`pkg/manifestanalyzer` renders via JSON to YAML; `internal/git/manifestedit` -uses `yaml.v3` with `SetIndent(2)`). Cosmetic, but it makes `git diff` on the mirror useless for -seeing what changed. - ## 10. Sibling inference can place an object in another namespace's directory **Ask.** Consider the namespace when inferring where a *new* document goes. Cosmetic. From f2146bed3556b30019b82f9e6b70350340c52c8c Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Sun, 25 Oct 2026 05:40:18 +0000 Subject: [PATCH 5/5] revert: drop --watch-namespaces; defer ask #28 Reverts 6c4f3891. Partitioning one cluster between two instances by namespace removes only two of the four costs the ask lists: the CRDs and the ClusterRole stay shared, so the instances keep one upgrade cadence and one RBAC posture. It is not the HA mechanism either, and it adds a second tenancy model beside one instance serving many namespaces through grants. The team is not blocked: one shared instance works. The asks doc records #28 as deferred and why; the restart cost behind it belongs to the durable pending writes. Co-Authored-By: Claude Opus 5.5 (1M context) --- charts/gitops-reverser/README.md | 1 - .../gitops-reverser/templates/deployment.yaml | 3 - .../validate-operator-types-webhook.yaml | 10 --- charts/gitops-reverser/values.schema.json | 6 -- charts/gitops-reverser/values.yaml | 7 -- cmd/main.go | 60 +---------------- cmd/main_chart_args_test.go | 9 --- cmd/main_watch_namespaces_test.go | 56 ---------------- docs/TODO.md | 4 +- docs/configuration.md | 35 ---------- docs/future/gitops-api-open-asks.md | 54 +++++++++------ .../clusterwatchrule_admission_test.go | 52 -------------- .../controller/clusterwatchrule_controller.go | 25 ------- internal/rulestore/store.go | 8 --- internal/watch/bootstrap.go | 3 - internal/watch/bootstrap_admission_test.go | 29 -------- internal/watch/manager.go | 3 - internal/watch/served_namespaces.go | 67 ------------------- internal/watch/served_namespaces_test.go | 39 ----------- 19 files changed, 37 insertions(+), 434 deletions(-) delete mode 100644 cmd/main_watch_namespaces_test.go delete mode 100644 internal/watch/served_namespaces.go delete mode 100644 internal/watch/served_namespaces_test.go diff --git a/charts/gitops-reverser/README.md b/charts/gitops-reverser/README.md index b097b2aa2..89cb01f4a 100644 --- a/charts/gitops-reverser/README.md +++ b/charts/gitops-reverser/README.md @@ -179,7 +179,6 @@ nodeSelector: | `servers.receiver.port` | Push receiver container port; must match the port in `bindAddress` | `9292` | | `controllerManager.additionalSensitiveResources` | Extra Secret-shaped resource types encrypted as `resource` or `group/resource` | `[]` | | `controllerManager.inClusterKubeProviders` | `KubeProvider`s, as `namespace/name`, that may use `spec.inCluster`, the operator's own identity (`--in-cluster-kube-providers`). Exact names, no wildcards. The provider the chart renders is added on its own when it uses `inCluster`, so list only the others. One that sets `inCluster` and is not listed reports `InClusterNotAuthorized` and is never used | `[]` | -| `controllerManager.watchNamespaces` | Config-plane namespaces this release reconciles (`--watch-namespaces`), so two releases can share one cluster. A release reconciles the `GitTarget`s, rules, providers and branches in these namespaces only, a `ClusterWatchRule` only when its `GitTarget` is in one of them, and scopes its admission webhook to them. List every namespace whose objects reference each other. See [two instances in one cluster](../../docs/configuration.md#two-instances-in-one-cluster) | `[]` (every namespace) | | `controllerManager.insecureAllowGitHTTP` | Permit credentials with `http://` GitProvider URLs for trusted in-cluster development Git servers | `false` | | `controllerManager.branchWorkerQueueDepth` | Event queue depth per branch worker. A full queue refuses the write (`git_queue_refusals_total`) instead of stalling the watch path; the watch delivers it again from its cursor, and only a cursor that expires first loses that write's own commit (and, under `prune.mode: OnEvent`, a delete's removal). Costs queue depth × payload of pod memory, which `branchBufferMaxSize` does **not** cover | `1000` | | `controllerManager.branchBufferMaxSize` | Maximum in-memory event buffer per branch worker. Bounds the open commit window plus the writes retained for replay until a push succeeds. While the remote cannot be reached, retained writes that reach it close the worker to new writes, saves and resyncs until they are published; the watch delivers a refused event again | `8Mi` | diff --git a/charts/gitops-reverser/templates/deployment.yaml b/charts/gitops-reverser/templates/deployment.yaml index 5c54882e5..4f74ba4ff 100644 --- a/charts/gitops-reverser/templates/deployment.yaml +++ b/charts/gitops-reverser/templates/deployment.yaml @@ -102,9 +102,6 @@ spec: {{- with .Values.controllerManager.additionalSensitiveResources }} - {{ printf "--additional-sensitive-resources=%s" (join "," .) | quote }} {{- end }} - {{- with .Values.controllerManager.watchNamespaces }} - - {{ printf "--watch-namespaces=%s" (join "," .) | quote }} - {{- end }} {{- with include "gitops-reverser.inClusterKubeProviders" . }} - {{ printf "--in-cluster-kube-providers=%s" . | quote }} {{- end }} diff --git a/charts/gitops-reverser/templates/validate-operator-types-webhook.yaml b/charts/gitops-reverser/templates/validate-operator-types-webhook.yaml index 868434ed1..3d382ebf6 100644 --- a/charts/gitops-reverser/templates/validate-operator-types-webhook.yaml +++ b/charts/gitops-reverser/templates/validate-operator-types-webhook.yaml @@ -58,16 +58,6 @@ webhooks: resources: - watchrules scope: Namespaced - {{- with .Values.controllerManager.watchNamespaces }} - # Only the namespaces this release reconciles: another release sharing the cluster judges - # its own, against the objects its own cache holds. - namespaceSelector: - matchExpressions: - - key: kubernetes.io/metadata.name - operator: In - values: - {{- toYaml . | nindent 12 }} - {{- end }} # Redis write on real requests; nothing on dry-run (the handler honors this). sideEffects: NoneOnDryRun timeoutSeconds: {{ .Values.servers.admission.timeoutSeconds }} diff --git a/charts/gitops-reverser/values.schema.json b/charts/gitops-reverser/values.schema.json index 609d6cde4..4eb69f9d7 100644 --- a/charts/gitops-reverser/values.schema.json +++ b/charts/gitops-reverser/values.schema.json @@ -105,12 +105,6 @@ "items": { "type": "string", "pattern": "^[a-z0-9]([-a-z0-9]*[a-z0-9])?/[a-z0-9]([-a-z0-9.]*[a-z0-9])?$" }, "description": "KubeProviders (namespace/name) besides the one the chart renders that may use spec.inCluster, the operator's own identity. Exact names, no wildcards." }, - "watchNamespaces": { - "type": "array", - "uniqueItems": true, - "items": { "type": "string", "pattern": "^[a-z0-9]([-a-z0-9]*[a-z0-9])?$", "maxLength": 63 }, - "description": "Config-plane namespaces this release reconciles (--watch-namespaces). Empty: every namespace." - }, "insecureAllowGitHTTP": { "type": "boolean", "description": "Permit credentials with http:// GitProvider URLs for trusted in-cluster development Git servers." diff --git a/charts/gitops-reverser/values.yaml b/charts/gitops-reverser/values.yaml index 95ba5627c..5def3889f 100644 --- a/charts/gitops-reverser/values.yaml +++ b/charts/gitops-reverser/values.yaml @@ -68,13 +68,6 @@ controllerManager: # KubeProvider that sets inCluster and is not listed reports InClusterNotAuthorized and is never # used. inClusterKubeProviders: [] - # Config-plane namespaces this release reconciles (--watch-namespaces). Empty: every namespace. - # Two releases sharing one cluster each list their own: a release reconciles the GitTargets, - # rules, providers and branches in these namespaces only, and a ClusterWatchRule only when its - # GitTarget is in one of them. Its admission webhook is scoped to them too. List every namespace - # whose objects reference each other, and this release's own namespace when the chart renders - # objects there (kubeProvider, below). The ClusterRole stays cluster-wide. - watchNamespaces: [] # Permit credentials with http:// GitProvider URLs. This is only for trusted # in-cluster development Git servers; production remotes should use HTTPS or SSH. insecureAllowGitHTTP: false diff --git a/cmd/main.go b/cmd/main.go index b2d94ea5b..0c964e41f 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -25,13 +25,10 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" - "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/selection" utilruntime "k8s.io/apimachinery/pkg/util/runtime" clientgoscheme "k8s.io/client-go/kubernetes/scheme" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/certwatcher" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log/zap" @@ -145,7 +142,6 @@ func main() { RuleStore: ruleStore, EventRouter: nil, // Will be set below SensitiveResources: cfg.sensitiveResources, - ServedNamespaces: cfg.watchNamespaces, // Resolve a source cluster (named by a GitTarget.spec.kubeProviderRef) into a rest.Config: // read the KubeProvider, then its kubeconfig Secret from the provider's own namespace, or // the operator's own identity where --in-cluster-kube-providers allows it. The manager @@ -218,8 +214,6 @@ func main() { RuleStore: ruleStore, WatchManager: watchMgr, Recorder: mgr.GetEventRecorderFor("clusterwatchrule"), - // A ClusterWatchRule is cluster-scoped, so the cache does not confine it. - ServedNamespaces: cfg.watchNamespaces, }).SetupWithManager(mgr), "unable to create controller", "controller", "ClusterWatchRule") // Valkey/Redis is optional. When configured it holds each GitTarget's watch resume cursor so work @@ -521,10 +515,7 @@ type appConfig struct { // operator-supplied kubeconfig is attacker-adjacent input, so unsafe kubeconfigs are REJECTED // (a legible Validated=False), diverging from Flux's silent strip. kubeConfigSafety kubeconfig.SafetyPolicy - // watchNamespaces is the config-plane namespaces this instance reconciles; the zero value is - // every namespace. - watchNamespaces watch.ServedNamespaces - zapOpts zap.Options + zapOpts zap.Options } // parseFlags parses CLI flags and returns the application configuration. @@ -731,12 +722,6 @@ func parseFlagsWithArgs(fs *flag.FlagSet, args []string) (appConfig, error) { "known_hosts at all. A present-but-unparseable known_hosts is always a hard error.") fs.BoolVar(&cfg.credentialPolicy.AllowInsecureGitHTTP, "insecure-allow-git-http", false, "INSECURE, dev/throwaway clusters only: permit credentials with http:// GitProvider URLs.") - var watchNamespaces string - fs.StringVar(&watchNamespaces, "watch-namespaces", "", - "Comma-separated config-plane namespaces this instance reconciles. Empty (the default): every "+ - "namespace. Lets two instances share one cluster: each reconciles only the GitTargets, rules, "+ - "providers and branches in its own namespaces, and a ClusterWatchRule only when its GitTarget "+ - "is in one of them.") bindReceiverFlags(fs, &cfg) cfg.zapOpts = zap.Options{ // Production mode defaults to JSON encoding, which is easier for log processors to parse. @@ -801,10 +786,6 @@ func parseFlagsWithArgs(fs *flag.FlagSet, args []string) (appConfig, error) { if err != nil { return appConfig{}, err } - cfg.watchNamespaces, err = watch.ParseServedNamespaces(watchNamespaces) - if err != nil { - return appConfig{}, err - } // The install-level default known-hosts ConfigMap lives in the controller's own namespace, // supplied via the downward API. Without it, that resolution layer is simply unavailable. @@ -1292,7 +1273,6 @@ func newManager( DisableFor: []client.Object{&corev1.Secret{}}, }, }, - Cache: servedNamespacesCacheOptions(cfg.watchNamespaces, cfg.sshHostKeys.ControllerNamespace), }) if err != nil { setupLog.Error(err, "unable to start manager") @@ -1301,44 +1281,6 @@ func newManager( return mgr } -// servedNamespacesCacheOptions confines the manager's cache to the served namespaces, so this -// instance lists, watches and reconciles the namespaced kinds there only. Every namespace is -// served when none are named. -// -// Two kinds need more than DefaultNamespaces. A Namespace is cluster-scoped and so unaffected by -// it; it is narrowed by its immutable name label, so a label change elsewhere enqueues nothing and -// a grant never reads a namespace this instance does not serve. ConfigMaps are cached in the -// controller's own namespace as well, for the install-level known_hosts ConfigMap. -func servedNamespacesCacheOptions(served watch.ServedNamespaces, controllerNamespace string) cache.Options { - if served.All() { - return cache.Options{} - } - names := served.Names() - defaults := make(map[string]cache.Config, len(names)) - for _, name := range names { - defaults[name] = cache.Config{} - } - configMapNamespaces := make(map[string]cache.Config, len(names)+1) - for name := range defaults { - configMapNamespaces[name] = cache.Config{} - } - if controllerNamespace != "" { - configMapNamespaces[controllerNamespace] = cache.Config{} - } - byName, err := labels.NewRequirement(corev1.LabelMetadataName, selection.In, names) - if err != nil { - // The names passed ParseServedNamespaces, which is stricter than a label value. - panic(fmt.Sprintf("served namespaces are not label values: %v", err)) - } - return cache.Options{ - DefaultNamespaces: defaults, - ByObject: map[client.Object]cache.ByObject{ - &corev1.Namespace{}: {Label: labels.NewSelector().Add(*byName)}, - &corev1.ConfigMap{}: {Namespaces: configMapNamespaces}, - }, - } -} - // setupAdmissionWebhooks registers both handlers on the one admission server: the // always-allow observer (a future-policy extension point) and the validate-operator-types // handler, which captures the submitter of our own command kinds into commandAuthorStore and diff --git a/cmd/main_chart_args_test.go b/cmd/main_chart_args_test.go index 47f7988c4..1fc6600b6 100644 --- a/cmd/main_chart_args_test.go +++ b/cmd/main_chart_args_test.go @@ -73,7 +73,6 @@ func TestChartRendersArgsTheBinaryAccepts(t *testing.T) { wantGitHTTP bool wantQueueDepth int wantBufferBytes int64 - wantWatchNS []string }{ "chart defaults": { wantKeyPrefix: "gitops-reverser", @@ -143,12 +142,6 @@ func TestChartRendersArgsTheBinaryAccepts(t *testing.T) { wantQueueDepth: 4000, wantBufferBytes: 32 * 1024 * 1024, }, - // A list joined into one quoted flag: quoting it wrong would make every name invalid. - "watch namespaces": { - setValues: []string{"controllerManager.watchNamespaces={team-b,team-a}"}, - wantKeyPrefix: "gitops-reverser", - wantWatchNS: []string{"team-a", "team-b"}, - }, } for name, tc := range tests { @@ -181,8 +174,6 @@ func TestChartRendersArgsTheBinaryAccepts(t *testing.T) { require.Equal(t, tc.wantBufferBytes, cfg.branchWorkerLimits.MaxBufferBytes, "controllerManager.branchBufferMaxSize must reach the binary") } - require.Equal(t, tc.wantWatchNS, cfg.watchNamespaces.Names(), - "controllerManager.watchNamespaces must reach the binary, and default to every namespace") }) } } diff --git a/cmd/main_watch_namespaces_test.go b/cmd/main_watch_namespaces_test.go deleted file mode 100644 index 66a492240..000000000 --- a/cmd/main_watch_namespaces_test.go +++ /dev/null @@ -1,56 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 - -package main - -import ( - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/labels" - "sigs.k8s.io/controller-runtime/pkg/cache" - "sigs.k8s.io/controller-runtime/pkg/client" - - "github.com/ConfigButler/gitops-reverser/internal/watch" -) - -func TestServedNamespacesCacheOptions_EveryNamespaceLeavesTheCacheAlone(t *testing.T) { - assert.Equal(t, cache.Options{}, servedNamespacesCacheOptions(watch.ServedNamespaces{}, "gitops-reverser")) -} - -func TestServedNamespacesCacheOptions_ConfinesTheCacheToTheServedNamespaces(t *testing.T) { - served, err := watch.ParseServedNamespaces("team-a,team-b") - require.NoError(t, err) - - opts := servedNamespacesCacheOptions(served, "gitops-reverser") - - assert.Equal(t, map[string]cache.Config{"team-a": {}, "team-b": {}}, opts.DefaultNamespaces) - - byObject := map[string]cache.ByObject{} - for obj, cfg := range opts.ByObject { - byObject[kindOf(obj)] = cfg - } - assert.Equal(t, - map[string]cache.Config{"team-a": {}, "team-b": {}, "gitops-reverser": {}}, - byObject["ConfigMap"].Namespaces, - "the install-level known_hosts ConfigMap lives in the controller's own namespace") - - namespaces := byObject["Namespace"].Label - require.NotNil(t, namespaces, "a cluster-scoped Namespace is not confined by DefaultNamespaces") - for name, want := range map[string]bool{"team-a": true, "team-b": true, "team-c": false} { - got := namespaces.Matches(labels.Set{corev1.LabelMetadataName: name}) - assert.Equal(t, want, got, name) - } -} - -func kindOf(obj client.Object) string { - switch obj.(type) { - case *corev1.ConfigMap: - return "ConfigMap" - case *corev1.Namespace: - return "Namespace" - default: - return "unexpected" - } -} diff --git a/docs/TODO.md b/docs/TODO.md index f41e3e100..761e989ff 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -54,9 +54,9 @@ Their argument for each is in [gitops-api-open-asks.md](future/gitops-api-open-a - [ ] **#34**: an e2e with an in-cluster applier and a remote source cluster, provoking one apply window collision. -- [ ] **#28, step 2**: an owner selector, for two instances sharing a namespace. Step 1, - `--watch-namespaces`, shipped; a chart-rendered namespaced Role would let its ClusterRole shrink. - [ ] **#10**: consider the namespace when placement follows siblings. Cosmetic. +- **#28** (two instances sharing one cluster) is deferred, not open work: one instance serving many + namespaces is the tenancy model, and the restart cost behind the ask is the durable queue's. ## Attribution diff --git a/docs/configuration.md b/docs/configuration.md index 1d44910e3..6b021fc0e 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -588,41 +588,6 @@ which cluster a folder means, so every `GitTarget` names its `GitBranch` and its Nothing falls back to the provider the chart renders, and that provider admits only the release namespace's own objects until the installer grants more. -### Two instances in one cluster - -Two installations can share one config-plane cluster when each reconciles its own namespaces. Set -`--watch-namespaces` (Helm: `controllerManager.watchNamespaces`) on each to a disjoint list: - -```yaml -# The platform's release -controllerManager: - watchNamespaces: [platform, team-a, team-b] ---- -# The customer's release, installed next to it -controllerManager: - watchNamespaces: [customer-mirror] -``` - -An instance then reconciles the `GitProvider`s, `GitBranch`es, `GitReceiver`s, `KubeProvider`s, -`GitTarget`s, rules, applier claims and `CommitRequest`s in its namespaces only. A -`ClusterWatchRule` is cluster-scoped, so both instances see it; the one serving the namespace of -the `GitTarget` it names reconciles it, and the other writes nothing to it, status included. The -chart scopes each release's admission webhook to the same namespaces. - -- **List every namespace whose objects reference each other.** A `GitBranch` naming a - `GitProvider` in another namespace, or a `GitTarget` naming a `KubeProvider` there, needs both - namespaces in the list; one outside it reads as an error, not as a missing object. Include the - release's own namespace when the chart renders a `KubeProvider` there. -- **Grants reach the served namespaces only.** A grant that admits a namespace another instance - serves admits nothing there: this instance never reconciles that namespace's objects. -- **Each instance needs its own Redis key prefix** (`queue.redis.keyPrefix`) when two share one - Redis. -- **The ClusterRole stays cluster-wide.** The namespaced kinds are listed and watched per - namespace, but the chart's permissions are not narrowed with them. -- **High availability is a different thing.** Two instances serving disjoint namespaces never - write the same `GitTarget`; two replicas of one instance would, which is why the chart still - refuses `replicaCount` above one. - ## `GitProvider` `GitProvider` defines the Git remote, credentials, who may claim which branches, and commit behavior. A diff --git a/docs/future/gitops-api-open-asks.md b/docs/future/gitops-api-open-asks.md index 2e1604a15..a6eb4671b 100644 --- a/docs/future/gitops-api-open-asks.md +++ b/docs/future/gitops-api-open-asks.md @@ -7,17 +7,16 @@ left out: #30 shipped as `GitReceiver` (#432), #33 went with the prefetch in #42 `commit_requests_total`), #25 shipped in 0.44.0 (#343), and #6 is answered by immutability that fails at admission with "delete and recreate". Since revision 15: #32 is a row in [the Flux guide](../bi-directional-flux.md#troubleshooting); #31 is -`claimed_resource_reconciled_total`, labeled by applier kind and claim; #28's first step is -`--watch-namespaces` ([two instances in one cluster](../configuration.md#two-instances-in-one-cluster)); -ask #23 is pinned at the fact index, for a finalizer cleared by a PATCH or an UPDATE, in either order +`claimed_resource_reconciled_total`, labeled by applier kind and claim; ask #23 is pinned at the +fact index, for a finalizer cleared by a PATCH or an UPDATE, in either order (`TestFactIndex_AFinalizerClearedByAControllerNamesTheDeleter`); and #11 shipped in 0.41.0 (#290): create and update write through one encoder, `internal/yamlstyle`, and a test fails on a second -one. A file GitOps Reverser did not write is re-indented once, on its first edit. Kind and field names are updated to `main` -(`FluxClaim` for `FluxTrigger`, `KubeProvider` for `ClusterProvider`, `ClaimedResourceReconciled` -for `ResumedElsewhere`). +one. A file GitOps Reverser did not write is re-indented once, on its first edit. Kind and field +names are updated to `main` (`FluxClaim` for `FluxTrigger`, `KubeProvider` for `ClusterProvider`, +`ClaimedResourceReconciled` for `ResumedElsewhere`). -Their suggested order, as of revision 15: #34 when the e2e budget allows; then #10; #28's owner -selector when two owners share a cluster. +Their suggested order, as of revision 15: #34 when the e2e budget allows; then #10; #28 when two +owners share a cluster. #28 is deferred: see below for why. ## 34. An e2e for an in-cluster applier with a remote source cluster @@ -42,20 +41,15 @@ asked: an apply that is not needed is never sent, so it is not counted. **Ask.** A way for two instances sharing one config-plane cluster to partition the CR set, so that exactly one instance reconciles a given `GitTarget`. Smallest useful first, and they stack: -1. **`--watch-namespaces`**, a `Cache.DefaultNamespaces` pass-through. **Shipped**: it partitions - the namespaced kinds, and a `ClusterWatchRule` goes to the instance serving its `GitTarget`'s - namespace. The ClusterRole stays cluster-wide (a namespaced cache lists per namespace, so a - narrowed role is possible, but the chart does not render one yet). +1. **`--watch-namespaces`**, a `Cache.DefaultNamespaces` pass-through. It partitions the namespaced + kinds, which since #430 includes `KubeProvider`; only `ClusterWatchRule` stays cluster-scoped. 2. **An explicit owner selector**, a label selector or a `spec.instance`-style field paired with an - `--instance-name` flag. Open, and only needed if two instances must share a namespace. + `--instance-name` flag, which covers the cluster-scoped kinds too. -**Why.** It is the same mechanism the chart's replica guard -(`charts/gitops-reverser/templates/validate-replica-count.yaml`) says is missing for HA: "Standby -replicas need ownership coordination first, so that two instances never write the same GitTarget." -They hit it moving onto a cluster that already runs an instance for its owner's own workloads: two -populations, different source clusters, providers, credentials and namespaces, and still both -instances would reconcile both sets. RBAC is no workaround: controller-runtime issues a cluster-wide -List, so a narrowed ClusterRole fails the cache sync and the manager exits. +**Why.** They hit it moving onto a cluster that already runs an instance for its owner's own +workloads: two populations, different source clusters, providers, credentials and namespaces, and +still both instances would reconcile both sets. RBAC is no workaround: controller-runtime issues a +cluster-wide List, so a narrowed ClusterRole fails the cache sync and the manager exits. **What sharing costs them**, for calibration: one Valkey holding both populations' attribution facts; one upgrade cadence; one RBAC posture that is the union of both; and, customer-visible, a @@ -63,6 +57,26 @@ restart for one owner's reasons costs the other's users their commit message, be `RenderMatchesLive` starts `Unknown` after a restart and an open window is dropped. They are not blocked: one shared instance works. +**Deferred, not built.** Step 1 was built and dropped before merge (#445). It removed two of the +four costs above, the shared Valkey and the restart reaching the other owner's users. It could not +remove the other two: + +- **One upgrade cadence stays.** The CRDs are cluster-scoped, so both instances run against one + schema, and an API break (we make them without migrations) breaks whichever instance lags. +- **One RBAC posture stays.** The ClusterRole is still cluster-wide. + +It is also not the HA mechanism the chart's replica guard +(`charts/gitops-reverser/templates/validate-replica-count.yaml`) asks for. Two instances serving +disjoint namespaces never contend for a `GitTarget`; two replicas of one instance would, and need +leader or ownership coordination instead. Against that, it adds a lasting cost: a second tenancy +model beside one instance serving many namespaces through grants (`consumerNamespaces`, +cross-namespace provider references). Every cross-namespace reference gains a "namespace this +instance does not serve" failure, and every cluster-scoped kind needs its own ownership check. + +The cost their users see is the restart dropping an open window. That is better fixed inside one +instance, for every installation: the durable pending writes in [the TODO](../TODO.md). A second +owner who needs its own upgrade cadence or RBAC needs its own cluster; #28 would give it neither. + ## 10. Sibling inference can place an object in another namespace's directory **Ask.** Consider the namespace when inferring where a *new* document goes. Cosmetic. diff --git a/internal/controller/clusterwatchrule_admission_test.go b/internal/controller/clusterwatchrule_admission_test.go index 92f1dd86e..1bc5eccfc 100644 --- a/internal/controller/clusterwatchrule_admission_test.go +++ b/internal/controller/clusterwatchrule_admission_test.go @@ -421,58 +421,6 @@ func TestReconcile_RevocationRemovesCompiledRule(t *testing.T) { assertTerminalRefusal(t, f.reloadRule(ctx, t)) } -// TestReconcile_AClusterWatchRuleAnotherInstanceServesIsLeftAlone: under --watch-namespaces a rule -// naming a GitTarget outside the served namespaces belongs to another instance. It is not compiled -// and its status is not written, so two instances never fight over its conditions. -func TestReconcile_AClusterWatchRuleAnotherInstanceServesIsLeftAlone(t *testing.T) { - ctx := context.Background() - f := newCWAFixture(t, []client.Object{ - cwaClusterWatchRule(), cwaGitTarget(), cwaGitProvider(), - cwaKubeProvider(cwaAdmitting()), - &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: cwaTargetNS}}, - }, interceptor.Funcs{}) - served, err := watch.ParseServedNamespaces("another-team") - require.NoError(t, err) - f.reconciler.ServedNamespaces = served - - _, err = f.reconcile(ctx) - require.NoError(t, err) - - assert.Empty(t, f.compiledNames()) - assert.Empty(t, f.reloadRule(ctx, t).Status.Conditions, "another instance's rule gets no status from this one") - assert.Zero(t, f.wm.replans, "a rule this instance never held needs no replan") -} - -// TestReconcile_AClusterWatchRuleRepointedOutOfTheServedNamespacesIsDropped: a rule this instance -// compiled, re-pointed at a GitTarget in a namespace it does not serve, leaves the store, and its -// streams are replanned away. -func TestReconcile_AClusterWatchRuleRepointedOutOfTheServedNamespacesIsDropped(t *testing.T) { - ctx := context.Background() - f := newCWAFixture(t, []client.Object{ - cwaClusterWatchRule(), cwaGitTarget(), cwaGitProvider(), - cwaKubeProvider(cwaAdmitting()), - &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: cwaTargetNS}}, - }, interceptor.Funcs{}) - served, err := watch.ParseServedNamespaces(cwaTargetNS + "," + cwaProviderNS) - require.NoError(t, err) - f.reconciler.ServedNamespaces = served - - _, err = f.reconcile(ctx) - require.NoError(t, err) - require.Equal(t, []string{cwaRuleName}, f.compiledNames(), "precondition: the rule is running") - replansWhileServed := f.wm.replans - - rule := f.reloadRule(ctx, t) - rule.Spec.GitTargetRef.Namespace = "another-team" - require.NoError(t, f.client.Update(ctx, rule)) - - _, err = f.reconcile(ctx) - require.NoError(t, err) - - assert.Empty(t, f.compiledNames()) - assert.Greater(t, f.wm.replans, replansWhileServed) -} - // TestReconcile_AdmissionReadErrorRequeuesAndKeepsCompiledRule pins the fail-safe direction. A // transient apiserver failure is NOT a denial: tearing the stream down on it would turn every // blip into an outage, so the rule stays compiled and the error requeues. diff --git a/internal/controller/clusterwatchrule_controller.go b/internal/controller/clusterwatchrule_controller.go index 5f171724f..90eed0af6 100644 --- a/internal/controller/clusterwatchrule_controller.go +++ b/internal/controller/clusterwatchrule_controller.go @@ -56,10 +56,6 @@ type ClusterWatchRuleReconciler struct { WatchManager WatchManagerInterface // Recorder emits a Kubernetes Event on every persisted Ready transition; nil disables Events. Recorder record.EventRecorder - // ServedNamespaces is the config-plane namespaces this instance reconciles. A ClusterWatchRule - // naming a GitTarget outside them belongs to another instance and is left alone. The zero value - // serves every namespace. - ServedNamespaces watch.ServedNamespaces } // +kubebuilder:rbac:groups=configbutler.ai,resources=clusterwatchrules,verbs=get;list;watch;create;update;patch;delete @@ -111,11 +107,6 @@ func (r *ClusterWatchRuleReconciler) Reconcile(ctx context.Context, req ctrl.Req return ctrl.Result{}, err } - if !r.ServedNamespaces.Serves(clusterRule.Spec.GitTargetRef.Namespace) { - r.forgetUnservedClusterWatchRule(req.NamespacedName, log) - return ctrl.Result{}, nil - } - log.Info("Starting ClusterWatchRule validation", "name", clusterRule.Name, "target", clusterRule.Spec.GitTargetRef, @@ -142,22 +133,6 @@ func (r *ClusterWatchRuleReconciler) Reconcile(ctx context.Context, req ctrl.Req return r.reconcileClusterWatchRuleViaTarget(ctx, st, &clusterRule) } -// forgetUnservedClusterWatchRule leaves a rule another instance serves alone: no status write, so -// two instances never fight over its conditions. A rule re-pointed out of the served namespaces is -// dropped from the store, and the plan that ran its streams is redone. -func (r *ClusterWatchRuleReconciler) forgetUnservedClusterWatchRule(key types.NamespacedName, log logr.Logger) { - if !r.RuleStore.HasClusterWatchRule(key) { - return - } - log.Info("ClusterWatchRule now names a GitTarget in a namespace this instance does not serve; dropping it", - "name", key.Name) - r.RuleStore.DeleteClusterWatchRule(key) - if r.WatchManager != nil { - r.WatchManager.TriggerAllRuleChange() - } - telemetry.ForgetResourceConditions(conditionKindClusterWatchRule, key.Namespace, key.Name) -} - // reconcileClusterWatchRuleViaTarget validates and stores a ClusterWatchRule that references a GitTarget. func (r *ClusterWatchRuleReconciler) reconcileClusterWatchRuleViaTarget( ctx context.Context, diff --git a/internal/rulestore/store.go b/internal/rulestore/store.go index a66fa500a..9941b918b 100644 --- a/internal/rulestore/store.go +++ b/internal/rulestore/store.go @@ -331,14 +331,6 @@ func (s *RuleStore) DeleteClusterWatchRule(key types.NamespacedName) { delete(s.clusterRules, key) } -// HasClusterWatchRule reports whether the store holds a compiled ClusterWatchRule under key. -func (s *RuleStore) HasClusterWatchRule(key types.NamespacedName) bool { - s.mu.RLock() - defer s.mu.RUnlock() - _, ok := s.clusterRules[key] - return ok -} - // MarkReady records that initial WatchRule and ClusterWatchRule bootstrap has completed. func (s *RuleStore) MarkReady() { s.mu.Lock() diff --git a/internal/watch/bootstrap.go b/internal/watch/bootstrap.go index 734263d8e..7e57338fc 100644 --- a/internal/watch/bootstrap.go +++ b/internal/watch/bootstrap.go @@ -37,9 +37,6 @@ func (m *Manager) bootstrapRuleStore(ctx context.Context, log logr.Logger) error } for i := range clusterWatchRules.Items { rule := clusterWatchRules.Items[i] - if !m.ServedNamespaces.Serves(rule.Spec.GitTargetRef.Namespace) { - continue - } if err := m.bootstrapClusterWatchRule(ctx, rule); err != nil { log.Error(err, "Failed to seed ClusterWatchRule into store", "name", rule.Name) } diff --git a/internal/watch/bootstrap_admission_test.go b/internal/watch/bootstrap_admission_test.go index a8da8ea4b..55805952b 100644 --- a/internal/watch/bootstrap_admission_test.go +++ b/internal/watch/bootstrap_admission_test.go @@ -191,35 +191,6 @@ func TestBootstrapRuleStore_SkipsUnauthorizedRuleButStillReady(t *testing.T) { "the store must still be marked ready: a refused rule is a refusal, not a startup failure") } -// TestBootstrapRuleStore_SeedsNoClusterWatchRuleAnotherInstanceServes: under --watch-namespaces a -// ClusterWatchRule naming a GitTarget outside the served namespaces belongs to another instance, -// and is not seeded even though its provider would admit it. -func TestBootstrapRuleStore_SeedsNoClusterWatchRuleAnotherInstanceServes(t *testing.T) { - servedTarget := bootGitTarget() - servedTarget.Name, servedTarget.Namespace = "ok-mirror", "team-ok" - servedRule := bootClusterWatchRule() - servedRule.Name = "served-rule" - servedRule.Spec.GitTargetRef = meta.NamespacedObjectReference{Name: "ok-mirror", Namespace: "team-ok"} - - m := bootManager(t, - bootGitTarget(), bootGitProvider(), bootClusterWatchRule(), - servedTarget, - &configv1alpha3.GitProvider{ObjectMeta: metav1.ObjectMeta{Name: "git", Namespace: "team-ok"}}, - servedRule, - bootKubeProvider(mirrorGrants(bootTargetNS, "team-ok")), - &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: bootTargetNS}}, - &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "team-ok"}}, - ) - var err error - m.ServedNamespaces, err = ParseServedNamespaces("team-ok," + bootProviderNS) - require.NoError(t, err) - - require.NoError(t, m.bootstrapRuleStore(context.Background(), logr.Discard())) - - assert.Equal(t, []string{"served-rule"}, bootCompiledNames(m)) - assert.True(t, m.RuleStore.IsReady()) -} - // TestBootstrapClusterWatchRule_WildcardStillResolvesItsClusterScopedTypes pins what a wildcard // selector means on the cluster kind. Which scope a rule watches is decided by which KIND you // write, never by a field, so `resources: ["*"]` on a ClusterWatchRule resolves the cluster-scoped diff --git a/internal/watch/manager.go b/internal/watch/manager.go index 5196b56f2..fe836ea3c 100644 --- a/internal/watch/manager.go +++ b/internal/watch/manager.go @@ -81,9 +81,6 @@ type Manager struct { // builds its observations, so each TypeRecord carries the right Sensitive fact. The // zero value still treats core Secrets as sensitive. SensitiveResources types.SensitiveResourcePolicy - // ServedNamespaces is the config-plane namespaces this instance reconciles. Bootstrap seeds no - // ClusterWatchRule naming a GitTarget outside it. The zero value serves every namespace. - ServedNamespaces ServedNamespaces // dynamicClient overrides the config-built dynamic client when non-nil. // Used in tests to inject a fake client without a real REST config. diff --git a/internal/watch/served_namespaces.go b/internal/watch/served_namespaces.go deleted file mode 100644 index 070f0f814..000000000 --- a/internal/watch/served_namespaces.go +++ /dev/null @@ -1,67 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 - -package watch - -import ( - "fmt" - "sort" - "strings" - - "k8s.io/apimachinery/pkg/util/validation" -) - -// ServedNamespaces is the set of config-plane namespaces one operator instance reconciles, from -// --watch-namespaces. The zero value serves every namespace. -// -// The manager's cache already confines the namespaced kinds to the set. A ClusterWatchRule is -// cluster-scoped, so every instance sees every one; it belongs to the instance serving the -// namespace of the GitTarget it names, and the others leave it alone, status included. -type ServedNamespaces struct { - names map[string]struct{} -} - -// ParseServedNamespaces reads a comma-separated list of namespace names. Empty serves every -// namespace. A name that is not a valid namespace name, or an empty entry, is an error. -func ParseServedNamespaces(raw string) (ServedNamespaces, error) { - if strings.TrimSpace(raw) == "" { - return ServedNamespaces{}, nil - } - names := make(map[string]struct{}) - for entry := range strings.SplitSeq(raw, ",") { - name := strings.TrimSpace(entry) - if name == "" { - return ServedNamespaces{}, fmt.Errorf("--watch-namespaces %q has an empty entry", raw) - } - if errs := validation.IsDNS1123Label(name); len(errs) > 0 { - return ServedNamespaces{}, fmt.Errorf("--watch-namespaces: %q is not a namespace name: %s", - name, strings.Join(errs, "; ")) - } - names[name] = struct{}{} - } - return ServedNamespaces{names: names}, nil -} - -// All reports whether every namespace is served. -func (s ServedNamespaces) All() bool { return len(s.names) == 0 } - -// Serves reports whether namespace is one this instance reconciles. -func (s ServedNamespaces) Serves(namespace string) bool { - if s.All() { - return true - } - _, ok := s.names[namespace] - return ok -} - -// Names returns the served namespaces, sorted; nil when every namespace is served. -func (s ServedNamespaces) Names() []string { - if s.All() { - return nil - } - out := make([]string, 0, len(s.names)) - for name := range s.names { - out = append(out, name) - } - sort.Strings(out) - return out -} diff --git a/internal/watch/served_namespaces_test.go b/internal/watch/served_namespaces_test.go deleted file mode 100644 index ae1943d69..000000000 --- a/internal/watch/served_namespaces_test.go +++ /dev/null @@ -1,39 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 - -package watch - -import ( - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestParseServedNamespaces_EmptyServesEveryNamespace(t *testing.T) { - for _, raw := range []string{"", " "} { - served, err := ParseServedNamespaces(raw) - require.NoError(t, err) - assert.True(t, served.All()) - assert.True(t, served.Serves("anything")) - assert.Nil(t, served.Names()) - } -} - -func TestParseServedNamespaces_ServesOnlyTheNamedNamespaces(t *testing.T) { - served, err := ParseServedNamespaces(" team-b,team-a ,team-b") - require.NoError(t, err) - - assert.False(t, served.All()) - assert.True(t, served.Serves("team-a")) - assert.True(t, served.Serves("team-b")) - assert.False(t, served.Serves("team-c")) - assert.False(t, served.Serves(""), "a cluster-scoped object's empty namespace is not served") - assert.Equal(t, []string{"team-a", "team-b"}, served.Names()) -} - -func TestParseServedNamespaces_RefusesWhatIsNotANamespaceName(t *testing.T) { - for _, raw := range []string{"team-a,", ",team-a", "team-a,,team-b", "Team-A", "team_a", "team-a/x"} { - _, err := ParseServedNamespaces(raw) - assert.Error(t, err, raw) - } -}