diff --git a/api/v1alpha3/audit_route_test.go b/api/v1alpha3/audit_route_test.go new file mode 100644 index 00000000..80a2bca0 --- /dev/null +++ b/api/v1alpha3/audit_route_test.go @@ -0,0 +1,81 @@ +// SPDX-License-Identifier: Apache-2.0 + +package v1alpha3 + +import ( + "testing" + + "github.com/stretchr/testify/assert" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// TestClusterProvider_AuditRoute covers the defaulting rule the whole feature rests on: an unset +// auditRoute resolves to the provider's own name, which is what every install already partitions its +// facts by, so adding the field changes nothing until someone sets it. +func TestClusterProvider_AuditRoute(t *testing.T) { + tests := []struct { + name string + provider ClusterProvider + want string + }{ + { + name: "no attribution block falls back to the object name", + provider: ClusterProvider{ObjectMeta: metav1.ObjectMeta{Name: "prod-eu-1"}}, + want: "prod-eu-1", + }, + { + name: "an empty auditRoute falls back to the object name", + provider: ClusterProvider{ + ObjectMeta: metav1.ObjectMeta{Name: "prod-eu-1"}, + Spec: ClusterProviderSpec{Attribution: &ClusterProviderAttribution{}}, + }, + want: "prod-eu-1", + }, + { + name: "a declared route wins over the object name", + provider: ClusterProvider{ + ObjectMeta: metav1.ObjectMeta{Name: "tenant-acme-delegating"}, + Spec: ClusterProviderSpec{ + Attribution: &ClusterProviderAttribution{AuditRoute: "prod-eu-1"}, + }, + }, + want: "prod-eu-1", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, tt.provider.AuditRoute()) + }) + } +} + +// TestClusterProvider_AuditRoute_SharedBySeveralProviders is the reported bug, stated as an API +// invariant. An API server has one audit webhook backend and posts under one route, so two +// ClusterProviders naming one physical cluster can only both resolve authors if they agree on that +// route. Before this field they could not: each read under its own name, and every commit through +// the provider the API server did NOT post under was authored attribution-unresolved. +// +// Locality is deliberately not involved. Both providers here omit kubeConfig, which is what makes +// them in-cluster, and it is the declared route rather than that fact which joins them. +func TestClusterProvider_AuditRoute_SharedBySeveralProviders(t *testing.T) { + fedByTheAPIServer := ClusterProvider{ObjectMeta: metav1.ObjectMeta{Name: "default"}} + delegating := ClusterProvider{ + ObjectMeta: metav1.ObjectMeta{Name: "srcns-delegating"}, + Spec: ClusterProviderSpec{ + AllowSourceNamespaceOverride: true, + Attribution: &ClusterProviderAttribution{AuditRoute: "default"}, + }, + } + + assert.True(t, fedByTheAPIServer.IsInCluster()) + assert.True(t, delegating.IsInCluster()) + assert.Equal(t, fedByTheAPIServer.AuditRoute(), delegating.AuditRoute(), + "two providers on one cluster must read one partition, or only the routed one is attributed") + + // A provider on a genuinely different cluster keeps its own partition, so the join can never + // cross-credit. This is what a declared route buys that keying by object UID alone would not: + // an etcd-snapshot clone reproduces object UIDs exactly, and only a human-chosen name separates + // the copy from its origin. + clone := ClusterProvider{ObjectMeta: metav1.ObjectMeta{Name: "prod-restored"}} + assert.NotEqual(t, fedByTheAPIServer.AuditRoute(), clone.AuditRoute()) +} diff --git a/api/v1alpha3/clusterprovider_types.go b/api/v1alpha3/clusterprovider_types.go index 495867aa..5ff7ee3b 100644 --- a/api/v1alpha3/clusterprovider_types.go +++ b/api/v1alpha3/clusterprovider_types.go @@ -10,8 +10,8 @@ import ( // DefaultClusterProviderName is the conventionally opinionated ClusterProvider name that an // omitted GitTarget.spec.clusterProviderRef points at. That defaulting is its ONLY special // behavior: it is an ordinary user-created object that may omit kubeConfig (the operator's own -// in-cluster config) or set it to mirror a remote cluster, it is existence-gated on its -// /audit-webhook/default route like every other name, and it is never created by the operator. +// in-cluster config) or set it to mirror a remote cluster, its audit route defaults to its name like +// every other provider's, and it is never created by the operator. const DefaultClusterProviderName = "default" // ClusterProviderReference references the cluster-scoped ClusterProvider a GitTarget sources @@ -118,6 +118,35 @@ type ClusterProviderSpec struct { // +optional // +kubebuilder:validation:Minimum=1 Burst *int32 `json:"burst,omitempty"` + + // Attribution groups this cluster's author-attribution settings. The block is spelled + // "attribution" rather than "authorAttribution" even though the operator flags are + // --author-attribution-*: the prefix groups a flat flag namespace, and a block on a + // source-cluster object already supplies that scope. + // +optional + Attribution *ClusterProviderAttribution `json:"attribution,omitempty"` +} + +// ClusterProviderAttribution holds the per-cluster author-attribution settings. It exists as a +// block so later per-cluster knobs (grace, mode) have a home beside auditRoute. +type ClusterProviderAttribution struct { + // AuditRoute is the route this cluster's audit events arrive on. The sender is the API server's + // webhook backend (https://kubernetes.io/docs/tasks/debug/debug-cluster/audit/#webhook-backend), + // and this is the segment its configured URL ends in: /audit-webhook/. When several + // logical clusters share one backend it is instead the value of the audit-event annotation named + // by --author-attribution-audit-route-annotation-key. + // + // It partitions the attribution facts, so two ClusterProviders carrying the same route read one + // cluster's facts, and two carrying different routes can never cross-credit an author. + // + // Empty means metadata.name. Set it when several ClusterProviders name one cluster, since an API + // server has one webhook backend and so posts under one route: every other provider for that + // cluster must be pointed at the same route or it resolves no authors at all. Set it also when + // the events are labelled for something other than this object, such as a kcp logical cluster. + // +optional + // +kubebuilder:validation:MaxLength=253 + // +kubebuilder:validation:Pattern=`^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$` + AuditRoute string `json:"auditRoute,omitempty"` } // ClusterProviderStatus defines the observed state of ClusterProvider. @@ -150,8 +179,10 @@ type ClusterProviderStatus struct { // ClusterProvider is the cluster-scoped, read-side peer of GitProvider: it names a SOURCE cluster a // GitTarget mirrors FROM, and owns that cluster's connectivity credential (spec.kubeConfig), // namespace-access authorization (spec.allowedNamespaces), and per-cluster status. Its NAME is the -// cluster's identity everywhere — the /audit-webhook/ ingress route and the attribution -// fact-index key. No name is special: "default" is merely what an omitted +// cluster's identity for the watch data plane, and the DEFAULT for its audit route: attribution +// facts are partitioned by spec.attribution.auditRoute, which falls back to this name. Several +// providers may name one cluster by declaring one route, which is what an API server with a single +// audit webhook backend requires. No name is special: "default" is merely what an omitted // GitTarget.spec.clusterProviderRef points at, and it may just as well name a remote cluster, since // in-cluster-ness follows from spec.kubeConfig (omitted = in-cluster) rather than from the name. // @@ -191,6 +222,23 @@ func (p *ClusterProvider) IsInCluster() bool { return p.Spec.KubeConfig == nil } +// AuditRoute is the identity this provider's attribution facts are keyed under: the route its +// cluster's audit events arrive on, defaulting to the provider's own name. It is resolved through +// this method rather than read off the field so no caller ever handles the empty case, the same +// shape as (api/v1alpha3).GitTarget.SourceCluster(). +// +// The default is what makes this change invisible to an existing install: one ClusterProvider per +// cluster already partitions its facts by name, so an unset field resolves exactly what it always +// resolved. Deliberately NOT conditional on locality: defaulting an in-cluster provider to the +// literal "default" would make that name reserved for the local cluster again, a rule this project +// enforced with CEL and then reversed before shipping (docs/finished/multi-cluster-author-attribution.md). +func (p *ClusterProvider) AuditRoute() string { + if p.Spec.Attribution == nil || p.Spec.Attribution.AuditRoute == "" { + return p.Name + } + return p.Spec.Attribution.AuditRoute +} + // AllowsNamespace reports whether a namespace (by name and labels) may reference this provider // from a GitTarget, per spec.allowedNamespaces. It is DENY-BY-DEFAULT: a provider with no // allowedNamespaces policy (neither names nor selector) admits no namespace. Names and selector diff --git a/api/v1alpha3/gittarget_types.go b/api/v1alpha3/gittarget_types.go index 676552a3..a48b4974 100644 --- a/api/v1alpha3/gittarget_types.go +++ b/api/v1alpha3/gittarget_types.go @@ -318,9 +318,10 @@ type GitTarget struct { // SourceCluster is the identity the watch data plane keys a GitTarget's source cluster on: the // referenced ClusterProvider's NAME. It defaults to "default" when clusterProviderRef is unset — // so a source-cluster-unaware caller still gets a concrete, non-empty name, and there is no "" -// sentinel. That name is a convention, not a claim about which physical cluster it is. The name is -// the cluster's identity everywhere: the fact-index key, the GVK→GVR registry key, and the -// /audit-webhook/ route. +// sentinel. That name is a convention, not a claim about which physical cluster it is. The name +// keys the GVK→GVR registry and the watch context. It is NOT the attribution partition: facts are +// keyed by the referenced ClusterProvider's AuditRoute(), which defaults to this name but may +// differ when several providers share one cluster's single audit stream. func (g *GitTarget) SourceCluster() string { if g.Spec.ClusterProviderRef == nil || g.Spec.ClusterProviderRef.Name == "" { return DefaultClusterProviderName diff --git a/api/v1alpha3/zz_generated.deepcopy.go b/api/v1alpha3/zz_generated.deepcopy.go index 21592aec..790807ea 100644 --- a/api/v1alpha3/zz_generated.deepcopy.go +++ b/api/v1alpha3/zz_generated.deepcopy.go @@ -75,6 +75,21 @@ func (in *ClusterProvider) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ClusterProviderAttribution) DeepCopyInto(out *ClusterProviderAttribution) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ClusterProviderAttribution. +func (in *ClusterProviderAttribution) DeepCopy() *ClusterProviderAttribution { + if in == nil { + return nil + } + out := new(ClusterProviderAttribution) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ClusterProviderList) DeepCopyInto(out *ClusterProviderList) { *out = *in @@ -145,6 +160,11 @@ func (in *ClusterProviderSpec) DeepCopyInto(out *ClusterProviderSpec) { *out = new(int32) **out = **in } + if in.Attribution != nil { + in, out := &in.Attribution, &out.Attribution + *out = new(ClusterProviderAttribution) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ClusterProviderSpec. diff --git a/charts/gitops-reverser/README.md b/charts/gitops-reverser/README.md index 21ea71ea..fc3cb63a 100644 --- a/charts/gitops-reverser/README.md +++ b/charts/gitops-reverser/README.md @@ -194,7 +194,7 @@ nodeSelector: | `attribution.enabled` | Run audit ingress and name mirrored-resource commit authors from matching kube-apiserver audit facts | `false` | | `attribution.ttl` | How long an attribution fact is retained waiting for the matching watch event to join it | `10m` | | `attribution.grace` | Bounded per-event wait for a matching audit fact before a watch event ships as the committer | `3s` | -| `attribution.clusterAnnotationKey` | Audit-event annotation naming the `ClusterProvider` each event belongs to. Empty keeps audit routes named (`/audit-webhook/`). Set it only for a control plane emitting **one shared audit stream** for several logical clusters: it enables the bare `/audit-webhook`, which resolves the source cluster per event. An event with no annotation, or naming a provider that does not exist, is rejected (counted and logged) and never credited to a fallback | `""` | +| `attribution.auditRouteAnnotationKey` | Audit-event annotation naming the **audit route** each event belongs to. Empty keeps audit routes named (`/audit-webhook/`). Set it only for a control plane emitting **one shared audit stream** for several logical clusters: it enables the bare `/audit-webhook`, which reads the route per event. A `ClusterProvider` joins a route via `spec.attribution.auditRoute` (default: its own name). An event with no annotation is rejected (counted and logged) and never credited to a fallback | `""` | | `clusterProvider.createDefault` | Render and own a `ClusterProvider` named `default` — the source cluster a `GitTarget` mirrors from when it omits `spec.clusterProviderRef`. The **operator never creates one**, so without this you commit the object yourself. Chart-owned: turning it off makes Helm delete the provider it created, and a `GitTarget` referencing a missing provider is held unready (`ClusterProviderNotFound`). The `quickstart` values never create one | `true` | | `clusterProvider.default.kubeConfig.secretRef.name` | Secret (release namespace) holding a kubeconfig for the rendered `default` provider. Empty means the operator's **own in-cluster** cluster; a name points `default` at a **remote** cluster instead — the name is a convention, not a claim about which cluster it is | `""` | | `clusterProvider.default.kubeConfig.secretRef.key` | Key within that Secret. Empty reads `value` then `value.yaml` | `""` | @@ -238,7 +238,7 @@ See [`values.yaml`](values.yaml) for complete configuration options. When `attribution.enabled=true`, `https://:9444/audit-webhook/` receives audit events from kube-apiserver — audit routes are **named**, including `/audit-webhook/default`. The bare `/audit-webhook` is rejected with **400** unless -`attribution.clusterAnnotationKey` is set, which turns it into the shared-stream endpoint that +`attribution.auditRouteAnnotationKey` is set, which turns it into the shared-stream endpoint that resolves each event's source cluster from that annotation. The operator extracts a minimal attribution fact from each (auditID, user, verb, resourceVersion, GVR, namespace, name, UID, status, timestamps) into the Redis attribution index (populated only when audit attribution is enabled). When a Redis endpoint is configured it also stores diff --git a/charts/gitops-reverser/templates/deployment.yaml b/charts/gitops-reverser/templates/deployment.yaml index 3d8a43a5..a771f75e 100644 --- a/charts/gitops-reverser/templates/deployment.yaml +++ b/charts/gitops-reverser/templates/deployment.yaml @@ -83,8 +83,8 @@ spec: - --author-attribution={{ .Values.attribution.enabled }} - --author-attribution-ttl={{ .Values.attribution.ttl }} - --author-attribution-grace={{ .Values.attribution.grace }} - {{- if .Values.attribution.clusterAnnotationKey }} - - --author-attribution-cluster-annotation-key={{ .Values.attribution.clusterAnnotationKey }} + {{- if .Values.attribution.auditRouteAnnotationKey }} + - --author-attribution-audit-route-annotation-key={{ .Values.attribution.auditRouteAnnotationKey }} {{- end }} {{- if .Values.servers.admission.enabled }} - --admission-webhook diff --git a/charts/gitops-reverser/values.schema.json b/charts/gitops-reverser/values.schema.json index 9d0d64ce..0e611482 100644 --- a/charts/gitops-reverser/values.schema.json +++ b/charts/gitops-reverser/values.schema.json @@ -275,7 +275,7 @@ "enabled": { "type": "boolean" }, "ttl": { "$ref": "#/$defs/duration" }, "grace": { "$ref": "#/$defs/duration" }, - "clusterAnnotationKey": { + "auditRouteAnnotationKey": { "type": "string", "description": "Audit-event annotation naming the ClusterProvider each event belongs to. Empty (default) keeps audit routes named (/audit-webhook/); set it only for one shared audit stream carrying several logical clusters, which enables the bare /audit-webhook endpoint." } diff --git a/charts/gitops-reverser/values.yaml b/charts/gitops-reverser/values.yaml index e8bab7e6..d5153741 100644 --- a/charts/gitops-reverser/values.yaml +++ b/charts/gitops-reverser/values.yaml @@ -213,14 +213,15 @@ attribution: # Bounded per-event wait for a matching audit fact before a watch event ships as the committer. # Larger values raise attribution hit-rate at the cost of commit latency. grace: "3s" - # Audit-event annotation naming the ClusterProvider each event belongs to. Audit routes are - # normally NAMED (/audit-webhook/, including /audit-webhook/default), and this is empty. + # Audit-event annotation naming the AUDIT ROUTE each event belongs to. Audit routes are normally + # NAMED (/audit-webhook/, including /audit-webhook/default), and this is empty. # Set it only for a control plane that emits ONE shared audit stream for several logical # clusters: it enables the bare /audit-webhook endpoint, which reads this annotation from every - # event, so one batch may fan out to several source clusters. An event with no annotation, or - # naming a ClusterProvider that does not exist, is rejected (counted and logged) and never - # credited to a fallback. See docs/configuration.md. - clusterAnnotationKey: "" + # event, so one batch may fan out to several routes. A ClusterProvider joins a route by setting + # spec.attribution.auditRoute to the same value; it defaults to the provider's own name. An event + # with no annotation is rejected (counted and logged) and never credited to a fallback. + # See docs/configuration.md. + auditRouteAnnotationKey: "" # The source cluster a GitTarget mirrors FROM is a ClusterProvider, and a GitTarget that omits # spec.clusterProviderRef references one named "default". The operator NEVER creates a diff --git a/cmd/main.go b/cmd/main.go index de2b5fb7..891d91f4 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -24,7 +24,6 @@ import ( _ "k8s.io/client-go/plugin/pkg/client/auth" corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" @@ -240,13 +239,9 @@ func main() { auditHandler, err := webhookhandler.NewAuditHandler(webhookhandler.AuditHandlerConfig{ MaxRequestBodyBytes: cfg.auditMaxRequestBodyBytes, FactRecorder: attributionIndex, - // Gate every /audit-webhook/ route on the ClusterProvider existing. The audit - // server already requires a CA-signed client cert (RequireAndVerifyClientCert), so this - // only decides which named source clusters an authenticated apiserver may post for. - ProviderResolver: clusterProviderExistence{reader: mgr.GetClient()}, // Empty leaves the bare /audit-webhook endpoint disabled (400); set, it demultiplexes a // shared stream per event by this annotation. - ClusterAnnotationKey: cfg.clusterAnnotationKey, + AuditRouteAnnotationKey: cfg.auditRouteAnnotationKey, }) fatalIfErr(err, "unable to build audit handler") @@ -262,13 +257,13 @@ func main() { ) setupLog.Info("author attribution enabled: matched audit facts name the commit author", "redisAddr", cfg.redisAddr, "grace", cfg.attributionGrace.String(), - "clusterAnnotationKey", cfg.clusterAnnotationKey) - if cfg.clusterAnnotationKey == "" { + "auditRouteAnnotationKey", cfg.auditRouteAnnotationKey) + if cfg.auditRouteAnnotationKey == "" { setupLog.Info("audit routes are named: post to /audit-webhook/; " + "the bare /audit-webhook endpoint is disabled") } else { setupLog.Info("shared audit stream enabled on the bare /audit-webhook endpoint: each event's "+ - "ClusterProvider is read from its annotation", "annotationKey", cfg.clusterAnnotationKey) + "ClusterProvider is read from its annotation", "annotationKey", cfg.auditRouteAnnotationKey) } case cfg.redisAddr != "": setupLog.Info("configured-author mode: author attribution disabled; commits use the configured "+ @@ -397,7 +392,7 @@ type appConfig struct { authorAttribution bool attributionFactTTL time.Duration attributionGrace time.Duration - clusterAnnotationKey string + auditRouteAnnotationKey string branchBufferMaxBytes int64 sensitiveResources types.SensitiveResourcePolicy sshHostKeys git.SSHHostKeyConfig @@ -508,13 +503,14 @@ func parseFlagsWithArgs(fs *flag.FlagSet, args []string) (appConfig, error) { "Bounded per-event wait for a matching audit fact to arrive before a watch event ships as the "+ "configured committer (duration string; default 3s). Larger values raise attribution hit-rate "+ "at the cost of commit latency.") - fs.StringVar(&cfg.clusterAnnotationKey, "author-attribution-cluster-annotation-key", "", - "Audit-event annotation naming the ClusterProvider each event belongs to. Setting it enables the "+ + fs.StringVar(&cfg.auditRouteAnnotationKey, "author-attribution-audit-route-annotation-key", "", + "Audit-event annotation naming the AUDIT ROUTE each event belongs to. Setting it enables the "+ "bare /audit-webhook endpoint for a SHARED audit stream carrying several logical clusters: the "+ - "source cluster is resolved per event, so one batch may fan out to several providers. An event "+ - "with no annotation, or naming a provider that does not exist, is rejected (counted and logged) "+ - "rather than credited to a fallback. Empty (the default) leaves the bare endpoint disabled, and "+ - "every producer must post to /audit-webhook/.") + "route is read per event, so one batch may fan out to several routes. A ClusterProvider joins "+ + "a route by setting spec.attribution.auditRoute to the same value (it defaults to the "+ + "provider's own name). An event carrying no annotation is rejected (counted and logged) rather "+ + "than credited to a fallback. Empty (the default) leaves the bare endpoint disabled, and every "+ + "producer must post to /audit-webhook/.") branchBufferMaxSizeStr := os.Getenv("BRANCH_BUFFER_MAX_SIZE") if branchBufferMaxSizeStr == "" { branchBufferMaxSizeStr = defaultBranchBufferMaxSizeStr @@ -618,9 +614,9 @@ func validateAuditConfig(cfg appConfig) error { } // The annotation key only has a receiver to configure when the audit ingress is running at all; // silently ignoring it would look like annotation routing was enabled when nothing serves it. - if strings.TrimSpace(cfg.clusterAnnotationKey) != "" && !cfg.authorAttribution { + if strings.TrimSpace(cfg.auditRouteAnnotationKey) != "" && !cfg.authorAttribution { return errors.New( - "author-attribution-cluster-annotation-key requires author-attribution to be enabled; " + + "author-attribution-audit-route-annotation-key requires author-attribution to be enabled; " + "without it there is no audit ingress to route") } if strings.TrimSpace(cfg.redisAddr) == "" { @@ -857,22 +853,6 @@ func buildServerTLSConfig(tlsOpts []func(*tls.Config)) *tls.Config { return serverTLS } -// clusterProviderExistence adapts the manager client to the audit handler's AuditProviderResolver: -// it reports whether a ClusterProvider (a named source cluster) exists, gating remote -// /audit-webhook/ routes. The read is cached; during startup it blocks until the cache syncs. -type clusterProviderExistence struct{ reader client.Reader } - -func (c clusterProviderExistence) ProviderExists(ctx context.Context, name string) (bool, error) { - var cp configbutleraiv1alpha3.ClusterProvider - if err := c.reader.Get(ctx, client.ObjectKey{Name: name}, &cp); err != nil { - if apierrors.IsNotFound(err) { - return false, nil - } - return false, err - } - return true, nil -} - func buildAuditServerTLSConfig(cfg appConfig, tlsOpts []func(*tls.Config)) (*tls.Config, error) { serverTLS := buildServerTLSConfig(tlsOpts) diff --git a/config/crd/bases/configbutler.ai_clusterproviders.yaml b/config/crd/bases/configbutler.ai_clusterproviders.yaml index 71a8385e..5df1a4d9 100644 --- a/config/crd/bases/configbutler.ai_clusterproviders.yaml +++ b/config/crd/bases/configbutler.ai_clusterproviders.yaml @@ -39,8 +39,10 @@ spec: ClusterProvider is the cluster-scoped, read-side peer of GitProvider: it names a SOURCE cluster a GitTarget mirrors FROM, and owns that cluster's connectivity credential (spec.kubeConfig), namespace-access authorization (spec.allowedNamespaces), and per-cluster status. Its NAME is the - cluster's identity everywhere — the /audit-webhook/ ingress route and the attribution - fact-index key. No name is special: "default" is merely what an omitted + cluster's identity for the watch data plane, and the DEFAULT for its audit route: attribution + facts are partitioned by spec.attribution.auditRoute, which falls back to this name. Several + providers may name one cluster by declaring one route, which is what an API server with a single + audit webhook backend requires. No name is special: "default" is merely what an omitted GitTarget.spec.clusterProviderRef points at, and it may just as well name a remote cluster, since in-cluster-ness follows from spec.kubeConfig (omitted = in-cluster) rather than from the name. @@ -149,6 +151,32 @@ spec: type: object x-kubernetes-map-type: atomic type: object + attribution: + description: |- + Attribution groups this cluster's author-attribution settings. The block is spelled + "attribution" rather than "authorAttribution" even though the operator flags are + --author-attribution-*: the prefix groups a flat flag namespace, and a block on a + source-cluster object already supplies that scope. + properties: + auditRoute: + description: |- + AuditRoute is the route this cluster's audit events arrive on. The sender is the API server's + webhook backend (https://kubernetes.io/docs/tasks/debug/debug-cluster/audit/#webhook-backend), + and this is the segment its configured URL ends in: /audit-webhook/. When several + logical clusters share one backend it is instead the value of the audit-event annotation named + by --author-attribution-audit-route-annotation-key. + + It partitions the attribution facts, so two ClusterProviders carrying the same route read one + cluster's facts, and two carrying different routes can never cross-credit an author. + + Empty means metadata.name. Set it when several ClusterProviders name one cluster, since an API + server has one webhook backend and so posts under one route: every other provider for that + cluster must be pointed at the same route or it resolves no authors at all. Set it also when + the events are labelled for something other than this object, such as a kcp logical cluster. + maxLength: 253 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ + type: string + type: object burst: description: |- Burst overrides the operator's outgoing kube-client burst for this cluster. Omitted, the diff --git a/docs/INDEX.md b/docs/INDEX.md index 0ecf433b..53c79080 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -65,10 +65,12 @@ says what we support and refuse** — and then its kustomize field taxonomy, the write boundary, the orchestrator/expansion line, and how secrets are handled. -Eleven other open items: +Thirteen other open items: | Doc | Open question | |---|---| +| [`docs-linting.md`](design/docs-linting.md) | how to mechanise [`style-guide.md`](style-guide.md) with markdownlint-cli2 and Vale. Both tools are installed and measured but wired into nothing: the structural backlog is 138 fixes after `--fix`, the prose backlog is ~3,000 em dashes, so they need different gates. Open: the `MD013` limit, and whether the prose gate stays changed-files-only | +| [`attribution-fact-identity.md`](design/attribution-fact-identity.md) | several `ClusterProvider`s may name one physical cluster, but a kube-apiserver posts audit to one route, so only one of those names is ever fed and every other one authors `unknown (attribution unresolved)`. Proposes a declared `spec.attribution.auditRoute` that partitions the facts instead of `metadata.name`, so several providers can share one cluster's facts while cloned clusters stay separate, ingestion loses its last Kubernetes read, and a misrouted provider becomes loud. Renames the key infix and the annotation-key flag to the same word | | [`watch-and-catalog-architecture.md`](design/watch-and-catalog-architecture.md) | the target three-layer watch model — **needs a human call before building** | | [`metrics-observability-plan.md`](design/metrics-observability-plan.md) | the watch-stage metrics do not exist yet | | [`reconcile-triggering.md`](design/reconcile-triggering.md) | which controllers still fail to wake up | diff --git a/docs/architecture.md b/docs/architecture.md index b65dd245..8eba58ae 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -607,7 +607,7 @@ per-mutation change log. Attribution runs only when `--author-attribution=true`; Redis is then its required state store. A normal source posts audit `EventList` payloads to `/audit-webhook/`. The bare -`/audit-webhook` endpoint is enabled only with `--author-attribution-cluster-annotation-key`, for a +`/audit-webhook` endpoint is enabled only with `--author-attribution-audit-route-annotation-key`, for a trusted control plane that puts a provider name in each event. There is no supplementary body endpoint or body joiner, because watch — not audit — carries the object body. The handler applies an intrinsic accept gate (StageResponseComplete, a mutating verb, success, non-dry-run, a changed resourceVersion, and the diff --git a/docs/attribution-setup-guide.md b/docs/attribution-setup-guide.md index 404bddc3..2761d2d3 100644 --- a/docs/attribution-setup-guide.md +++ b/docs/attribution-setup-guide.md @@ -59,9 +59,12 @@ for you with `clusterProvider.createDefault: true`. `default` is only the name a points at: that provider may omit `spec.kubeConfig` (the operator's own cluster) or set it to mirror a remote one. -The provider's **name is the cluster's identity everywhere** — the attribution fact-index key and -the `/audit-webhook/` audit route — so a fact from one cluster can never name -the author of an object watched on another. A provider also carries a deny-by-default +The provider's **name is the cluster's identity** for the watch data plane, and the default for its +**audit route**. Attribution facts are partitioned by `spec.attribution.auditRoute`, which falls back +to the provider's name, so a fact from one cluster can never name the author of an object watched on +another. Set it explicitly when several providers name one cluster: an API server has a single audit +webhook backend and posts under one route, so the others must declare that route to see its facts. +A provider also carries a deny-by-default `spec.allowedNamespaces` policy: a `GitTarget` may reference it only from an admitted namespace (enforced on every reconcile, before that target's watches start — so tightening the policy also stops a `GitTarget` that already exists). diff --git a/docs/bug-report.md b/docs/bug-report.md new file mode 100644 index 00000000..3fd6ea7e --- /dev/null +++ b/docs/bug-report.md @@ -0,0 +1,180 @@ +# Bug report — attribution is silently `unresolved` for objects mirrored through a non-`default` ClusterProvider + +**For:** the gitops-reverser team +**Found by:** gitops-api, while spiking the one-reverser consolidation against branch +`feat/gittarget-prune-mode-pr5` +**Date:** 2026-07-21 +**Reverser version:** branch `feat/gittarget-prune-mode-pr5` @ `b9c2e495` (past v0.38.0), built locally +as `gitops-reverser:e2e-local`, run in configured **attribution mode** (`--author-attribution`, +`--author-attribution-grace=10s`, `--redis-addr` set). +**Severity:** medium — mirroring is correct; **author attribution is silently lost**. It is only +visible at all because of the new `unknown (attribution unresolved)` author on this branch — which is +exactly what surfaced it. + +## Summary + +Every object mirrored through a GitTarget whose `spec.clusterProviderRef` names a **dedicated +in-cluster ClusterProvider** (one created with `kubeConfig` omitted, so it points at the operator's +own cluster, but with a **name other than `default`**) is committed as +`unknown (attribution unresolved) `, even though the +same actor's writes through the **`default`** ClusterProvider attribute correctly. + +The audit facts *are* being written; they are simply never matched for objects whose GitTarget +resolves attribution under a non-`default` cluster identity. Our leading hypothesis is that the +attribution fact index is keyed per source-cluster (ClusterProvider), and a ClusterProvider with no +audit route delivering facts under **its own** identity — which a dedicated in-cluster provider is, +because the local apiserver's audit is filed under `default` — has an empty fact index, so every +lookup misses. + +## Evidence (two back-to-back runs, reverser restarted between them to zero the counters) + +Workload: the branch's own `manager`-labelled specs, focused to two Describe blocks — +`WatchRule source namespace` (`test/e2e/source_namespace_e2e_test.go`) and +`Manager GitTarget prune policy` (`test/e2e/prune_mode_e2e_test.go`). Both create `configmaps` on the +same in-cluster apiserver, as the same e2e identity, under the same audit policy. + +**`gitopsreverser_attribution_resolutions_total`, `sum by (result,resource)` — identical across runs:** + +| result | resource | Run A | Run B | +|---|---|---|---| +| `exact_user` | configmaps | 9 | 9 | +| `weak` | configmaps | 3 | 3 | +| `absent` | configmaps | **5** | **5** | + +**The 5 `absent` are, in both runs, exactly the objects the source-namespace spec mirrors** — and +*only* those (the prune spec's objects all resolved): + +``` +[CREATE] v1/configmaps/srcns-mirrored → unknown (attribution unresolved) +[CREATE] v1/configmaps/srcns-wildcard-admitted → unknown (attribution unresolved) +[CREATE] v1/configmaps/srcns-two-cm-a → unknown (attribution unresolved) +[CREATE] v1/configmaps/srcns-two-cm-b → unknown (attribution unresolved) +[CREATE] v1/configmaps/srcns-refused-cm → unknown (attribution unresolved) +``` + +**The `absent` resolutions waited the FULL grace and no fact ever arrived** +(`gitopsreverser_attribution_resolution_wait_seconds_bucket{result="absent"}`): + +``` +wait <= 10.0s : 0 +wait <= +Inf : 5 ← all 5 exceeded the 10s grace; a fact was never matched +``` + +**Facts are being written — they are just not matched for these objects** +(`gitopsreverser_attribution_fact_events_total`, Run B): + +``` +written = 66 +matched = 12 ← == exact_user(9) + weak(3); the resolved population only +deletecollection_expanded = 18 +(no expired_unmatched, no late) +``` + +`written=66 / matched=12` with **zero `expired_unmatched`** means the resolver is looking up a key +under which no fact was ever filed — not a fact that was written and then aged out. + +## The one structural difference between resolved and unresolved objects + +| | resolves (`exact_user`/`weak`) | unresolved (`absent`) | +|---|---|---| +| spec | `Manager GitTarget prune policy` | `WatchRule source namespace` | +| `GitTarget.spec.clusterProviderRef` | omitted → defaults to `{name: default}` | a **dedicated** in-cluster provider `srcns-delegating` (created via `applyInClusterClusterProvider`, `kubeConfig` omitted) | +| `WatchRule` source namespace | own namespace | overridden (`rules[].sourceNamespace`, incl. `"*"`) | + +The split is **100% clean and reproducible**: every `default`-provider object resolved, every +dedicated-provider object was `absent`. That rules out a stochastic grace-window/load effect (which +would be flaky and would not respect the provider boundary). + +The source-namespace override and the dedicated ClusterProvider are both present on the srcns objects, +but the code path below shows the **override is irrelevant** and the **ClusterProvider identity is the +whole cause**. + +## Root cause (confirmed in code + config, not just correlation) + +The attribution fact index is keyed by **provider name**, and the write side and read side derive that +name from two different places that do not agree for a non-`default` in-cluster ClusterProvider: + +1. **Facts are written under the provider name of the audit ROUTE.** `AuditHandler.resolveRoute` + (`internal/webhook/audit_handler.go`) maps `/audit-webhook/` → provider ``, or resolves + it per-event from `--audit-cluster-annotation-key` on the bare `/audit-webhook`. It then calls + `RecordFact(ctx, providerName, event)`, which stores `factKeyExact/Last/RV(providerName, …)` + (`internal/queue/attribution_index.go:129,182,385-395`). + - In this environment the apiserver posts to **`/audit-webhook/default`** (its + `webhook-config.yaml`) and the reverser has **no** `--audit-cluster-annotation-key`, so **every** + local fact is filed under provider `default`. +2. **Facts are read under the GitTarget's ClusterProvider name.** The resolver calls + `LookupAuthorResolution(ctx, providerName, …)` with `providerName = GitTarget.SourceCluster()` — the + `spec.clusterProviderRef` name. For the srcns targets that is **`srcns-delegating`**. + +`default` (write) ≠ `srcns-delegating` (read) → every lookup misses → `absent`, full grace, `written` +but never `matched`. The object's namespace matches on both sides, which is why the `sourceNamespace` +override plays no part. + +**In short:** a ClusterProvider that attribution *reads* under its own name, but under which no audit +route ever *writes* a fact, yields silent `unresolved` for everything mirrored through it. A dedicated +in-cluster ClusterProvider (`kubeConfig` omitted, name ≠ `default`) is exactly that, because the local +apiserver's audit is filed under `default`. + +## Why this matters to us (gitops-api) + +Our one-reverser consolidation gives **every tenant workspace its own (non-`default`) +ClusterProvider** and relies on per-actor attribution as its core value. In our real topology each +workspace is *remote* and posts to the bare `/audit-webhook` with `--audit-cluster-annotation-key= +kcp.io/cluster`, so facts *are* filed under the provider name a GitTarget resolves to — provided the +provider is named exactly the annotation value and the flag is set. This finding is therefore not +(we believe) a blocker for the remote path, but it is a **loud warning about how silent the failure +mode is**: the moment the audit-write name and the GitTarget-read name diverge — a missing +annotation-key flag, a provider named differently from its `kcp.io/cluster` hash, or an in-cluster +provider whose audit still routes to `default` — **every commit becomes `unresolved` with no error, +no condition, and no failed reconcile**. Attribution is our product's core value, so a +misconfiguration that silently drops it (rather than failing loudly) is high-consequence for us. + +## What is NOT affected + +- **Mirroring is correct.** All five objects are mirrored to Git under the right folders; only the + commit *author* is wrong. +- The `default` provider path is unaffected. + +## Reproduction + +1. Bring up the e2e harness on the branch (`task _cluster-ready && task prepare-e2e`; needs + `controller-gen` on PATH and `HOST_PROJECT_PATH` set — see this repo's `hack/spikes/README.md`). +2. `kubectl -n gitops-reverser rollout restart deploy/gitops-reverser` to zero the counters. +3. Run the two specs: + ```sh + go run github.com/onsi/ginkgo/v2/ginkgo --label-filter='manager' \ + --focus='WatchRule source namespace|Manager GitTarget prune policy' ./test/e2e/ + ``` +4. Query Prometheus: `sum by (result,resource) (gitopsreverser_attribution_resolutions_total)` and + `sum by (op) (gitopsreverser_attribution_fact_events_total)`; and `git log --author` the mirrored + repos under `.stamps/repos/*/` for `attribution unresolved`. + +## What we'd most like fixed + +**Make this failure loud.** A GitTarget (or ClusterProvider) with attribution enabled whose provider +has **received zero audit facts under its own name** should surface a condition / warning, instead of +silently authoring every commit as `unresolved`. The `unknown (attribution unresolved)` author on this +branch already makes it *visible in git*, which is how we found it — a status condition would make it +*actionable* before a single commit lands wrong. + +## Open questions for the team + +1. Is a **dedicated in-cluster ClusterProvider** (`kubeConfig` omitted, name ≠ `default`) intended to + be a supported attribution configuration at all? If yes, where should its facts come from, given the + local apiserver's audit is filed under `default`? If no, admission could reject/warn on it. +2. Confirmation that the **remote path we depend on** — bare `/audit-webhook` + + `--audit-cluster-annotation-key`, provider named exactly the annotation value — files facts under + the same name the GitTarget resolves to. (Our reading of the code says yes; we'd value a confirmation + and, ideally, an e2e that asserts a *remote-CP* commit is attributed, since the current + `source-cluster` spec only exercises an unreachable kubeconfig.) +3. Would you accept a small e2e that pins this: a GitTarget on the `default` provider **with** a + `sourceNamespace` override attributes correctly (isolating the override as innocent), while a + dedicated-provider GitTarget does not? We have the harness set up and can contribute it. + +## Root-cause code references + +- `internal/queue/attribution_index.go` — `RecordFact(providerName,…)` (:129), `writeFactKeys` (:182), + `matchFactKey` under `factKeyExact/Last/RV(providerName,…)` (:385–395), `LookupAuthorResolution(providerName,…)` (:360). +- `internal/webhook/audit_handler.go` — `resolveRoute` / `providerRouteForPath` (:155–204): route → provider name. +- This environment: apiserver `webhook-config.yaml` server `…/audit-webhook/default`; reverser args carry + no `--audit-cluster-annotation-key`; srcns GitTargets `clusterProviderRef: srcns-delegating`. diff --git a/docs/config-flag-conventions.md b/docs/config-flag-conventions.md index 21e89467..50f45ecb 100644 --- a/docs/config-flag-conventions.md +++ b/docs/config-flag-conventions.md @@ -87,6 +87,15 @@ All flags for one component share a prefix and don't drift. If the toggle is `--author-attribution-grace` — not a bare `--attribution-ttl` next to a prefixed `--audit-attribution-enabled`. Pick the prefix once; apply it to the whole group. +**The prefix is a flat-namespace device, so nested configuration drops it.** Chart values and CRD +fields are already scoped by their block, and repeating the prefix inside one stutters: +`attribution.ttl` in `values.yaml` maps to `--author-attribution-ttl`, and +`ClusterProvider.spec.attribution` needs no `author` because a source-cluster object has no other +kind of attribution. Flags have no such scope, which is why they keep the qualifier: this product +has two attribution concepts, author attribution and +[render attribution](design/support-boundary/render-attribution.md), and only the flat namespace has +to tell them apart. + ### 6. Positive phrasing; reserve `--no-` for genuine on-by-default toggles Avoid double negatives in names and in help text. Flux controllers use diff --git a/docs/configuration.md b/docs/configuration.md index 1f0d6ec3..dccd6243 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1048,25 +1048,31 @@ annotation key is set: ```yaml attribution: - clusterAnnotationKey: example.io/source-cluster + auditRouteAnnotationKey: example.io/source-cluster ``` When this option is set, the receiver reads `example.io/source-cluster` from each event. Its value is -the name of the `ClusterProvider` that owns the event, so events in the same batch may route to -different source clusters. +the **audit route** the event belongs to, so events in the same batch may route to different +partitions. A `ClusterProvider` joins a route by setting `spec.attribution.auditRoute` to the same +value; it defaults to the provider's own name. -**The bare endpoint never guesses a source cluster.** Rejection happens at two levels: +**The bare endpoint never guesses a route.** Rejection happens at two levels: | Situation | Result | |---|---| -| A request reaches `/audit-webhook` while `clusterAnnotationKey` is unset | The whole request is rejected with **400**. The bare endpoint is not enabled, so a producer posting to it is misconfigured. | -| An event carries no annotation, or names a `ClusterProvider` that does not exist | That **event** is rejected: it produces no attribution fact and is never credited to a fallback provider. The request still returns 200, so correctly-annotated events in the same batch are kept. | +| A request reaches `/audit-webhook` while `auditRouteAnnotationKey` is unset | The whole request is rejected with **400**. The bare endpoint is not enabled, so a producer posting to it is misconfigured. | +| An event carries no annotation | That **event** is rejected: it produces no attribution fact and is never credited to a fallback route. The request still returns 200, so correctly-annotated events in the same batch are kept. | + +An annotation naming a route no `ClusterProvider` has declared is **not** rejected. The route is a +partition name rather than a claim about an object, so the fact is stored and expires unread if +nothing joins it. That keeps ingestion free of Kubernetes reads, and lets a provider created after +its events started flowing pick them up. The second row is a per-event rejection rather than a per-request one on purpose. A shared stream is heterogeneous by definition, so failing the whole batch would discard events that routed correctly and leave the apiserver retrying a batch that can never succeed. Rejected events are counted and logged, so a producer that is not stamping the annotation is visible rather than silent — if that count rises, -point the producer at `/audit-webhook/` instead. +point the producer at `/audit-webhook/` instead. Use an annotation that the producing control plane sets consistently as source metadata. This is routing metadata only: it keeps the audit fact and the watch event in the same source-cluster diff --git a/docs/design/attribution-fact-identity.md b/docs/design/attribution-fact-identity.md new file mode 100644 index 00000000..a4b79988 --- /dev/null +++ b/docs/design/attribution-fact-identity.md @@ -0,0 +1,574 @@ +# Attribution when several ClusterProviders name one cluster + +> **design**: implemented. Index: [`../INDEX.md`](../INDEX.md) +> +> Status: the field, the two renames, the dropped ingestion gate, and the warning are built and +> covered by unit and e2e tests. Two deviations from the text below are marked in place: the value +> is validated as a DNS-1123 **subdomain** rather than a label, and the `lastAuditEventTime` status +> field is NOT built (the warning is). +> +> Prompted by a bug report from the gitops-api team (`docs/bug-report.md`): every object mirrored +> through a dedicated in-cluster `ClusterProvider` was committed as +> `unknown (attribution unresolved)`. The report's diagnosis is correct and is reproduced in code +> below. Its recommended fix (make the failure loud) is adopted here as one half of the answer. + +## The decision + +A `ClusterProvider` declares the audit route its cluster's events arrive on, and that declared +value rather than the object's name is what partitions the attribution facts. Several providers may +name one physical cluster by carrying the same route. The field defaults to the object's own name, +so every install that works today keeps working unchanged. + +```yaml +apiVersion: configbutler.ai/v1alpha3 +kind: ClusterProvider +metadata: + name: tenant-acme-delegating # the name humans read and GitTargets reference +spec: + attribution: + auditRoute: prod-eu-1 # the route this cluster's audit events arrive on +``` + +Ingestion does not change, and it loses its only Kubernetes read: an audit event arrives, and its +fact is stored under the route it arrived on, with no lookup, no existence check, and no fan out. +The key keeps its structure, and the rest of the system is renamed to speak the same word. + +## What happens today + +The write side files a fact under the name in the audit route. The read side looks one up under the +name on the `GitTarget`. Both are the provider name, exactly as designed, and they diverge whenever +those two names are not the same object. + +| Side | Code | Key input | +|---|---|---| +| write | [`audit_handler.go`](../../internal/webhook/audit_handler.go) `resolveRoute` then `RecordFact` | the `` in `/audit-webhook/` | +| read | [`target_watch.go`](../../internal/watch/target_watch.go) `attachAuthor` then `LookupAuthorResolution` | `GitTarget.SourceCluster()`, the `spec.clusterProviderRef` name | + +A kube-apiserver takes one `--audit-webhook-config-file`, and that file +[uses the kubeconfig format to specify *the remote address of the service*](https://kubernetes.io/docs/tasks/debug/debug-cluster/audit/#webhook-backend), +singular. So one physical cluster feeds exactly one provider name through its +[webhook backend](https://kubernetes.io/docs/tasks/debug/debug-cluster/audit/#webhook-backend), and +that is the constraint everything below follows from. Every other `ClusterProvider` naming that same +cluster reads a partition nothing writes, and each of its commits is authored +`unknown (attribution unresolved)` after waiting out the full grace window. Nothing fails, no +condition changes, and the mirrored content is correct, so the loss is visible only in the commit +author. + +Two providers, one remote cluster, one audit route. Both targets see the same object on their own +watch, and both look for the same write, under two different keys: + +```mermaid +flowchart LR + subgraph SOURCE["One source cluster · one kube-apiserver"] + API["Kubernetes API
ConfigMap web · uid U · rv 101"] + AUD["Webhook backend
one audit webhook config
names one server URL"] + end + + subgraph CONTROL["Control plane · operator cluster"] + CPA["ClusterProvider prod-a
allowSourceNamespaceOverride: true"] + CPB["ClusterProvider prod-b
allowSourceNamespaceOverride: false"] + GTA["GitTarget alpha"] + GTB["GitTarget beta"] + end + + subgraph REDIS["Attribution index"] + FA[("route:prod-a:configmaps:object:U:101
author alice")] + FB[("route:prod-b:configmaps:object:U:101
nothing ever writes this key")] + end + + GTA -->|clusterProviderRef| CPA + GTB -->|clusterProviderRef| CPB + CPA -. "kubeConfig" .-> API + CPB -. "the same kubeConfig, the same cluster" .-> API + AUD ==>|"POST /audit-webhook/prod-a"| FA + API -->|"watch event · uid U · rv 101"| GTA + API -->|"watch event · uid U · rv 101"| GTB + GTA -->|"read under prod-a"| FA + GTB -->|"read under prod-b"| FB + FA -.->|hit| C1["commit author
alice"] + FB -.->|"absent after the full grace"| C2["commit author
unknown (attribution unresolved)"] + + classDef ok fill:#e6f7e6,stroke:#33aa33,color:#000; + classDef miss fill:#ffe6e6,stroke:#cc3333,color:#000; + class FA,C1 ok; + class FB,C2 miss; +``` + +The reported case is this picture with `kubeConfig` omitted from both providers and the route named +`default`: the local apiserver posts to `/audit-webhook/default`, `prod-b` becomes +`srcns-delegating`, and the red path is the one the gitops-api team saw. Nothing about the diagram +depends on the cluster being remote. It depends only on two provider names sharing one audit route. + +The reported run is the clean form of this: five objects mirrored through `srcns-delegating` were +`absent`, the objects mirrored through `default` in the same run resolved, and the fact counters +showed `written=66 / matched=12` with zero expiries. Facts were being written the whole time, under +a key nobody read. + +## Why several providers name one cluster + +`ClusterProvider` carries two jobs on one object. It is the cluster's **identity** (the audit route, +the fact partition, the GVK to GVR registry key), and it is a **policy** holder: +`spec.allowedNamespaces` decides which namespaces may reference it, and +`spec.allowSourceNamespaceOverride` delegates source-namespace selection to admitted `GitTarget`s. + +The identity job wants one object per cluster. The policy job wants one object per delegation +stance, because both fields are provider-wide. A platform administrator who grants the override to +one tenant and withholds it from another has no choice but to create two providers on the same +cluster. That is what [`watchrule-source-namespace/`](watchrule-source-namespace/README.md) shipped, +and its own e2e creates `srcns-delegating` and `srcns-non-delegating` side by side. The +configuration in the bug report is one the product encourages. + +Splitting `metadata.name` from the audit route resolves that tension without splitting the object: +the name serves the policy job (one object per stance, named for the stance), and the route serves +the identity job (one value per cluster, shared by every object that carries it). + +## The constraint on any fix + +Ingestion stays low level and trackable. An audit event arrives, it is reduced to a fact, and the +fact is stored under the route it arrived on. Ingestion does not read a `ClusterProvider`, +does not resolve locality, does not write a fact more than once, and does not refuse an event +because something is missing. A provider may be created later, deleted and recreated, or renamed +around a running audit stream, and none of that is ingestion's problem: the facts carry a short TTL +and a stored fact nobody reads costs one expiring Redis key. + +Whether a fact may be *used* is not a keyspace question. A `GitTarget` mirrors an object only when +`allowedNamespaces`, `allowedSourceNamespaces`, and the `WatchRule` gate all admit it. By the time a +watch event reaches the resolver, the decision to mirror that object has been taken. Attribution +answers a narrower question: who last wrote the object this event carries. + +## The proposal + +### The field + +A new optional field, on the provider that owns the cluster identity: + +```go +// AuditRoute is the route this cluster's audit events arrive on. The sender is the API server's +// webhook backend (https://kubernetes.io/docs/tasks/debug/debug-cluster/audit/#webhook-backend), +// and this is the segment its configured URL ends in: /audit-webhook/. When several +// logical clusters share one backend it is instead the value of the audit-event annotation named +// by --author-attribution-audit-route-annotation-key. +// +// It partitions the attribution facts, so two ClusterProviders carrying the same route read one +// cluster's facts, and two carrying different routes can never cross-credit an author. +// +// Empty means metadata.name, which is the single-provider-per-cluster convention and what every +// existing install already does. Set it when several ClusterProviders name one cluster (only one +// of them can be what the API server posts under), or when the audit events are labelled for +// something other than this object, such as a kcp logical cluster. +// +optional +AuditRoute string `json:"auditRoute,omitempty"` +``` + +Resolved through a method, the same shape as +[`GitTarget.SourceCluster()`](../../api/v1alpha3/gittarget_types.go), so no caller ever sees the +empty case: + +```go +func (p *ClusterProvider) AuditRoute() string { + if p.Spec.Attribution == nil || p.Spec.Attribution.AuditRoute == "" { + return p.Name + } + return p.Spec.Attribution.AuditRoute +} +``` + +It is **mutable**, unlike `spec.kubeConfig`. Repointing it changes which partition is read, not +which cluster is mirrored, so it is not the silent-retarget hazard immutability exists to prevent. +Correcting a typo should not require deleting the object. + +### The key keeps its shape and renames its infix + +```text +:author:v1:audit:route:::object:: +:author:v1:audit:route:::object::last +:author:v1:audit:route:::rv: +``` + +The infix stays on all three families, including the rv-only hatch where it is +[correctness work](../finished/multi-cluster-author-attribution.md) rather than partitioning, since +resourceVersions are small cluster-scoped integers that collide freely. Two things change: the +value, which becomes a declared route rather than an object name, and the label, which becomes +`route:` where it was `cluster:`. + +The label is spelled `route:` rather than `auditRoute:` because the key already says `audit` one +segment earlier (`author:v1:audit:`), so the longer form would stutter. It reads as one path: +*author, from audit, on route `prod-eu-1`*. + +The same two providers, now agreeing on one identifier: + +```mermaid +flowchart LR + subgraph SOURCE["One source cluster · one kube-apiserver"] + API["Kubernetes API
ConfigMap web · uid U · rv 101"] + AUD["Webhook backend
names one server URL"] + end + + subgraph CONTROL["Control plane · operator cluster"] + CPA["ClusterProvider prod-a
auditRoute: prod-eu-1"] + CPB["ClusterProvider prod-b
auditRoute: prod-eu-1"] + GTA["GitTarget alpha"] + GTB["GitTarget beta"] + end + + subgraph REDIS["Attribution index"] + F[("route:prod-eu-1:configmaps:object:U:101
author alice")] + end + + GTA -->|clusterProviderRef| CPA + GTB -->|clusterProviderRef| CPB + CPA -. "kubeConfig" .-> API + CPB -. "the same kubeConfig, the same cluster" .-> API + AUD ==>|"POST /audit-webhook/prod-eu-1"| F + API -->|"watch event · uid U · rv 101"| GTA + API -->|"watch event · uid U · rv 101"| GTB + GTA -->|"read under prod-eu-1"| F + GTB -->|"read under prod-eu-1"| F + F -.->|hit| C1["commit author
alice"] + F -.->|hit| C2["commit author
alice"] + + classDef ok fill:#e6f7e6,stroke:#33aa33,color:#000; + class F,C1,C2 ok; +``` + +### Ingestion gets simpler, not harder + +Today a named route 404s when the `ClusterProvider` does not exist, and the annotation-routed bare +endpoint rejects an event whose annotation names no provider. Both checks go away, and with them the +`AuditProviderResolver` interface, its `clusterProviderExistence` implementation in `cmd/main.go`, +and the startup rule that annotation routing requires a resolver. Ingestion becomes: parse the route +or the annotation, record the fact under it. It makes no Kubernetes API call at all. + +That is correct under this design because the route is no longer a claim about an object that must +exist. It is the partition name. An audit batch in flight while a provider is being created or +recreated is currently dropped (and the apiserver does not retry a 404); now it is stored, and the +provider joins it when it arrives. + +The authentication boundary does not move. The audit server still requires a client certificate +signed by the audit CA (`RequireAndVerifyClientCert`), which is what stops an unauthenticated +producer. The existence check was never an authorization check: +[`multi-source-audit-ingress-hardening.md`](multi-source-audit-ingress-hardening.md) already records +that "a holder of a shared, CA-signed client certificate can therefore submit facts for any existing +provider route". Removing it widens that from any existing route to any string, which changes the +blast radius of an already-accepted trust assumption rather than opening a new one. + +> **This touches an open design.** +> [`multi-source-audit-ingress-hardening.md`](multi-source-audit-ingress-hardening.md) lists "an +> event with a missing, unknown, or untrusted annotation records no fact" as an acceptance criterion +> for annotation routing. That criterion needs restating in terms of routes rather than providers. +> Its option A (provider-bound mTLS) becomes route-bound mTLS and gets *stronger* here, because the +> certificate would bind to the route, which is now the real identity. + +### What this does for kcp and shared streams + +Annotation routing becomes coherent rather than coincidental. With +`--author-attribution-audit-route-annotation-key=kcp.io/cluster`, the annotation value on each +event is the logical cluster it came from, and that value is now matched against `auditRoute` +rather than against `metadata.name`. A workspace provider can be called `tenant-acme` and carry +`auditRoute: `. + +Today those installs have to name the Kubernetes object after the hash for attribution to resolve, +which answers open question 2 in the bug report: the remote path works, but only if you accept +hash-named objects. This removes that constraint. + +### Cloned and restored clusters stay separate + +Keeping the partition on every key family is what makes this topology supportable, and it is the +main reason this design was chosen over dropping the partition in favour of the object UID. + +An etcd snapshot restored into a second cluster reproduces every `metadata.uid` exactly, so the +original and the copy contain the same objects with the same identities. Mirroring both from one +operator is then only safe if something separates their facts, and under this design that something +is a value a human sets: + +```yaml +metadata: {name: prod} # the original +spec: {attribution: {auditRoute: prod-eu-1}} +--- +metadata: {name: prod-restored} # an etcd snapshot of it, stood up for a rehearsal +spec: {attribution: {auditRoute: prod-eu-1-restored}} +``` + +Object `U` exists in both with the same UID. Its facts land under +`route:prod-eu-1:…:object:U:…` and `route:prod-eu-1-restored:…:object:U:…`, which never +join. A design keying facts by object identity alone could not make that distinction, because +every identity readable from inside a cloned cluster was cloned along with it. + +The same applies to the narrower case of one provider name being repointed at a different physical +cluster. `spec.kubeConfig` is immutable precisely so that repointing means delete and recreate, and +giving the recreated provider a fresh `auditRoute` also gives it a fresh partition, without the +purge mechanism that was considered and rejected earlier. + +That rejection also settles the fate of the dormant `PurgeClusterFacts`, which had been left exported +and tested in case purge-on-adopt was ever wanted. It is deleted as part of this change, along with +the decision record that kept it alive. A shared route makes it unsafe rather than merely unused: +purging on one provider's deletion would drop the facts of every other provider reading that route, +including the operator's own cluster, which was never torn down. The reasoning that still binds (no +finalizer, because `helm uninstall` would strand the object in `Terminating`) now lives on +`LegacyClusterProviderFinalizer` in +[`clusterprovider_controller.go`](../../internal/controller/clusterprovider_controller.go). + +## The naming of the field + +`auditRoute` is chosen, and the rest of the system is renamed to speak the same word. That second +half is the point: the concept is currently spelled three different ways, and adding a fourth name +for it would be the actual mistake. + +| Where | Today | Becomes | +|---|---|---| +| the `ClusterProvider` field | does not exist | `spec.attribution.auditRoute` | +| the fact key infix | `cluster:` | `route:` | +| the shared-stream flag | `--author-attribution-cluster-annotation-key` | `--author-attribution-audit-route-annotation-key` | + +The word earns it on two grounds. It is already the internal name: +[`audit_handler.go`](../../internal/webhook/audit_handler.go) defines an `auditRoute` type whose job +is mapping one request's events to a source cluster, and it deliberately spans both delivery forms +(a named path sets `provider`, the shared endpoint sets `annotationKey`). And it says *audit*, which +is the half that distinguishes this from every other cluster identity in the system: a +`ClusterProvider` also has a name, a kubeconfig, and a watch context, and none of those are this. + +The objection to it is that a reader takes "route" to mean the URL, while the value also arrives as +an annotation with no URL involved +([`audit_handler.go`](../../internal/webhook/audit_handler.go) does +`name := event.Annotations[route.annotationKey]`, which is the kcp form). Renaming the flag to +`--author-attribution-audit-route-annotation-key` answers that directly: the annotation is stated, +in the flag's own name, to carry an audit route. The word then means "the label these events arrive +under", in both forms, everywhere. + +| Rejected | Why | +|---|---| +| `clusterID` | matches the current key infix and `internal/watch`'s 99 uses of `clusterID`, but says nothing about audit, and a `ClusterProvider` has several identities that are not this one. Renaming the key and the flag removes its one advantage | +| `auditWebhookBackendRoute` | uses the official upstream term, and is the closest miss. A [webhook backend](https://kubernetes.io/docs/tasks/debug/debug-cluster/audit/#webhook-backend) is one per API server, but one backend carries many routes under annotation routing (a kcp shard posts every logical cluster through a single backend), so the name implies a 1:1 exactly where the interesting case is N:1. It also names the sender's configuration rather than our partition | +| `auditPathSegmentName`, `auditSegmentName` | describe one of the two delivery forms, and the wrong one for kcp. A `...Name` suffix also works against the point, which is separating a technical identifier from the friendly `metadata.name` | +| `auditClusterID` | `auditRoute` with a redundant noun, once the flag and the key both say route | +| `clusterIngestionID` | reads well standing alone, but "ingestion" is ambiguous: [watch-first ingestion](../finished/watch-first-ingestion-architecture.md) is the object-state path, and this is the audit path | +| `auditIngressID` | "audit ingress" is established repo vocabulary, but it names the operator's own receiving server, so it reads as the wrong end of the connection | +| `auditID` | taken: `event.AuditID` is the per-event UUID on every Kubernetes audit event | +| `auditStreamID` | "stream" means watch streams (`StreamsRunning`, `status.streams.summary`) | +| `auditSourceID` | "source" means the watched cluster (`SourceCluster()`, `sourceNamespace`) | +| `uploadID` | implies a per-batch handle, as in S3 multipart uploads | + +The name still carries less weight than the field's description, which is what a reader meets in +`kubectl explain`. That description states both delivery forms literally, so the concrete question +("what do I type here?") is answered where it is asked rather than compressed into an identifier. + +The block is `spec.attribution`, not `spec.authorAttribution`, even though every flag in the group +is spelled `--author-attribution-*`. The prefix is a +[flat-namespace device](../config-flag-conventions.md): flags need it because the product has two +attribution concepts (author attribution and +[render attribution](support-boundary/render-attribution.md)) sharing one undifferentiated list, +while a block on a source-cluster object cannot mean the rendering one. The chart already made this +call in four places, mapping `attribution.ttl` to `--author-attribution-ttl`, and the metrics agree +with `gitopsreverser_attribution_*`. Only the annotation-key flag is renamed here, and it keeps the +prefix, so the flag group stays internally consistent. + +The field sits under `spec.attribution` rather than at the top level, so later attribution settings +(per-provider grace, per-provider mode, both already contemplated in the `ClusterProvider` decision +record) have somewhere to go, and so `auditRoute` is unambiguous within its block. + +## What this costs + +One field, and one new way to be silently wrong. An `auditRoute` that names a route no apiserver +posts to produces exactly the failure in the bug report, and so does forgetting to set it on the +second provider for a cluster. That is why the warning below is part of this change rather than a +follow-up: without it, this design moves the silence one field to the left. + +The cost is bounded by being **explicit and inspectable**. `kubectl get clusterprovider -o yaml` +shows which route a provider reads, `kubectl get` can print it as a column, and two providers +sharing a cluster are visible as two objects carrying one value. None of that is true today, where +the coupling is implied by a name and discoverable only by reading a Redis key. + +### What it does not cost + +The semantics do not move, which is worth stating because the alternatives below all move something: + +- No change to the key's **structure**. The infix is relabelled and its value is sourced + differently, but no family gains or loses a dimension, so every join keeps its meaning. +- No test inversion. `TestAttributionIndex_CrossClusterIsolation` and + `TestAttributionIndex_RVOnlyHatchIsClusterScoped` both stay true as written, and only their key + fixtures move. +- No behaviour change for any existing install, because the field defaults to `metadata.name`. +- No resolve step in ingestion. The read side gains one field access on an object it already holds. + +The vocabulary rename costs a rollout window and a hard flag break, both taken deliberately and +without compatibility shims. See [migration](#no-backward-compatibility-is-implemented). + +## Could the cluster identify itself instead? + +This was the main alternative and it does not survive contact with the clone case. It is recorded +because the reasoning is what makes the declared route the right answer rather than a resigned one. + +| Candidate | Where it comes from | Distinct per cluster | Survives an etcd clone | +|---|---|---|---| +| `kube-system` namespace UID | one GET on the source client | yes | no, it is copied | +| `default/kubernetes` Service UID | one GET on the source client | yes | no, it is copied | +| kubeconfig `server` URL | the kubeconfig itself | no | not applicable | +| API server CA bundle | the kubeconfig itself | mostly | no, it is copied | +| API server `/version` | discovery | no | not applicable | +| `kcp.io/cluster` annotation | the audit event | yes, per logical cluster | not applicable | + +The `kube-system` namespace UID is the usual answer and the best of these. It is minted when the +cluster is created, it never changes, and the operator can already read it: `namespaces: get, list, +watch` is in [`config/rbac/role.yaml`](../../config/rbac/role.yaml) and applies to every source +client, so it would cost one cached GET and no new permission. + +The server URL cannot work, for two independent reasons. One cluster is reachable at several +addresses (an internal service address, an external load balancer, a bastion), so the same cluster +yields different strings. And the in-cluster config's server is `https://kubernetes.default.svc` for +every cluster viewed from inside itself, so the string that would identify the local cluster is the +one string that is identical everywhere. + +### A fingerprint cannot separate a clone from its origin + +An etcd snapshot restore copies the keyspace byte for byte, including every `metadata.uid`. A +cluster restored from a snapshot of another therefore carries the same `kube-system` UID as its +origin, exactly as it carries the same object UIDs. A fingerprint read from inside the cluster +cannot tell a clone from its origin, because the fingerprint is one of the things that got cloned. + +A restore that replays objects through the API instead (the Velero shape) has the API server mint +fresh UIDs for everything, so that topology collides on nothing and needs no protection either way. + +What separates two clones is not an identity that can be read. It is a human deciding these are two +clusters and saying so. A declared `auditRoute` is that decision, written down in the one place the +audit pipeline can act on it. + +### The options in full + +| # | Scheme | Fixes the reported bug | Separates clones | Cost | +|---|---|---|---|---| +| A | provider name in the key (today) | no | yes | none, it is what ships | +| B | object UID, dropping the cluster partition | yes | no | inverts a unit test and a documented invariant | +| C | cluster fingerprint as the key | yes | no | a resolve step on both sides, and the layer ingestion is meant not to have | +| **D** | **declared `auditRoute`, chosen** | **yes** | **yes** | **one field, and a new way to misconfigure** | +| E | B plus the fingerprint as a diagnostic | yes | no | one cached GET per provider | + +B is the tempting one, because object UIDs are globally unique and the +earlier fact-purge decision already leaned on exactly that to justify not purging. It was rejected here for one reason: it makes the cluster boundary an emergent +property of UUID uniqueness instead of a stated fact, and that property is the one thing an etcd +clone breaks. C is B with more machinery and no more safety, since its identity is copied by the +same clone that copies the object UIDs. + +One property worth stating because it is easy to miss: under any of these, **a cluster sends its +audit events once**, however many `ClusterProvider`s name it. Under A that is also true, but only +one of those providers can use them. + +## Making the misconfiguration loud + +Part of this change, not a follow-up, because `auditRoute` introduces a value that can be wrong. + +`lastAuditEventTime` was specified in the `ClusterProvider` decision record and never implemented. +The `ClusterProviderStatus` doc comment still records it as deferred, and there is no such field in +`api/v1alpha3` today. It remains the right shape, because a quiet cluster is not an unhealthy one +and a timestamp says that where a condition cannot: + +**Built.** The resolver tracks, per route, whether attribution has ever resolved and how many events +have gone unresolved since. A route that has never resolved and produces +`attributionUnresolvedWarnThreshold` (5) unresolved events in a row logs once, naming the fix rather +than the symptom. It is one line per route per process: the condition is a configuration mistake, so +repeating it per event would bury it. + +The threshold is not 1 because a lone miss is ordinary (an audit batch can land after the grace +window under load). A run of them with nothing ever matched is the signature of a route nobody +writes to. + +**Not built.** The two status bullets originally specified here: + +- Record the time of the last fact stored per audit route. +- Surface it on `ClusterProvider.status`, alongside the resolved route. + +`lastAuditEventTime` remains what the `ClusterProviderStatus` doc comment already calls it: +deferred. The warning delivers the loudness this design owes the bug report; the timestamp is a +separate, smaller piece of observability work, listed here so its absence is a decision rather than +an oversight. + +## Migration + +No data migration, and no API version bump. `auditRoute` defaults to `metadata.name`, which is the +value every install already uses, so a provider that sets nothing resolves exactly what it resolved +before. Adopting the fix is per-provider and opt-in: set `auditRoute` on the extra providers for a +cluster, and their targets start resolving. The chart's `default` provider needs no change. + +### No backward compatibility is implemented + +Both renames are hard renames. There is **no deprecated flag alias, no dual-read of the old key +prefix, and no migration shim**, because the shared-stream audit path has no users to protect. That +is a deliberate decision recorded here so the absence does not read as an oversight, and so a +reviewer does not add compatibility code back on the assumption that it was forgotten. + +- `--author-attribution-cluster-annotation-key` becomes + `--author-attribution-audit-route-annotation-key`, and the old spelling is removed outright. An + operator that still passes it fails to start on an unknown flag, which is the loud outcome, and + the chart's `values.yaml` key is renamed in the same change. +- The fact key infix becomes `route:` with no dual-read. Facts written by an old pod under + `cluster:…` are never read again, and they expire on their own. + +The one consequence that survives is a rollout window: while old and new pods overlap, facts written +by one are not read by the other, so those events find no fact and commit as the configured +committer. That is the ordinary degradation, it self-clears at the fact TTL of minutes, and it is +the same rollout behaviour [`redis-key-schema-v3.md`](../finished/redis-key-schema-v3.md) §10 +documented for the `v2` to `v3` bump. Attribution is a freshness SLO rather than a correctness one, +so a few minutes of committer-authored commits is the accepted price for one vocabulary. + +## Tests + +Unit: + +- `ClusterProvider.AuditRoute()` returns the name when unset and the field when set. +- Two providers with different names and one shared `auditRoute` resolve one recorded fact. +- Two providers with distinct routes do not cross-credit. This is + `TestAttributionIndex_CrossClusterIsolation` unchanged in meaning, with its key fixtures moved. +- The audit handler records a fact for a route with no matching `ClusterProvider`, replacing the + test that asserts a 404. + +An e2e pins the reported behaviour so it cannot return. It belongs with the existing attribution +specs and mirrors their shape (`commit_author_attribution_e2e_test.go` already proves the OIDC claim +chain through the `default` provider): + +1. Create a dedicated in-cluster `ClusterProvider` whose name is not `default` and which sets + `spec.attribution.auditRoute: default`, plus a `GitProvider` with a zero commit window and a + `GitTarget` referencing that provider. +2. Create a `ConfigMap` while impersonating a user carrying OIDC display-name and email claims, + using the same helper the existing spec uses. +3. Assert `git log --pretty=%an <%ae>` for that object's own path is the impersonated identity, and + specifically not `unknown (attribution unresolved)`. + +Step 3 fails on `main` today and passes after this change, which is the property that makes the spec +worth having. A second case should mirror an object reached through a `rules[].sourceNamespace` +override on that provider, because that is the exact shape the bug report hit, and it also records +that the override itself was never implicated. + +A third case is worth its cost: a provider that sets no `auditRoute`, and whose name is not what the +apiserver posts under, still commits `unknown (attribution unresolved)` and fires the warning. That +pins the loud path, which is the half of this design that stops the next occurrence being +invisible. + +## Open questions + +1. **Should an in-cluster provider default differently?** Two distinct defaults are in play, and + only the second is open. + + The **baseline default is settled**: an empty `auditRoute` resolves to `metadata.name`, for every + provider. That is what keeps existing installs byte-identical, since the object name is already + what partitions their facts. + + The open one is a **conditional override on top of it**: a provider with no `kubeConfig` could + resolve an empty `auditRoute` to the literal `default` instead of to its own name, which would + fix the reported case with no configuration at all and save the srcns e2e providers a line each. + + The recommendation is no, and the reason is stronger than "it infers something". It would make + `default` a reserved name for the local cluster again, and + [that exact rule was proposed, enforced with CEL, and reversed](../finished/multi-cluster-author-attribution.md) + before shipping: local-versus-remote follows from `spec.kubeConfig` alone, for any name, and + pinning a name to a physical cluster is *"the same silent-retarget hazard that `kubeConfig` + immutability exists to prevent"*. A conditional default would smuggle that coupling back in + through the audit path, and it would also assume a provider named `default` is the local audit + route, which is exactly what an operator who renamed their route has not got. One explicit line + per extra provider, plus a warning that names the fix, is the cheaper trade. +2. **Value format validation.** RESOLVED, as a DNS-1123 **subdomain** (max 253) rather than the + label this section first proposed. A label would have been more restrictive than the default: + `metadata.name` on a cluster-scoped object is a subdomain and may contain dots, so an explicit + `auditRoute` could not have expressed the value it defaults to. Subdomain characters are all safe + in a URL path segment and in a Redis key, where `escapeKeyField` already handles `:` and `%`. +3. **Does the hardening doc's option A become identifier-bound mTLS?** Binding a certificate to the + `auditRoute` is strictly more useful than binding it to a provider object now that the identifier + is what partitions facts, but that is that document's call. diff --git a/docs/design/multi-source-audit-ingress-hardening.md b/docs/design/multi-source-audit-ingress-hardening.md index 9c6fd428..1ab36958 100644 --- a/docs/design/multi-source-audit-ingress-hardening.md +++ b/docs/design/multi-source-audit-ingress-hardening.md @@ -21,7 +21,7 @@ administered-source boundary. The current user-facing limit is documented in [`../../SECURITY.md`](../../SECURITY.md#shared-audit-ingress-trust-model). The bare `/audit-webhook` route is different: it is enabled only when -`--author-attribution-cluster-annotation-key` is set, then resolves every event to a provider from that +`--author-attribution-audit-route-annotation-key` is set, then resolves every event to an audit route from that annotation. It is suitable only if the upstream control plane stamps the annotation and an untrusted writer cannot forge it. diff --git a/docs/facts/audit-webhook-api-server-connectivity.md b/docs/facts/audit-webhook-api-server-connectivity.md index 48872822..c0e86ab7 100644 --- a/docs/facts/audit-webhook-api-server-connectivity.md +++ b/docs/facts/audit-webhook-api-server-connectivity.md @@ -4,7 +4,7 @@ GitOps Reverser relies on the Kubernetes [audit webhook backend](https://kubernetes.io/docs/tasks/debug/debug-cluster/audit/#webhook-backend): the kube-apiserver POSTs audit events to an HTTPS endpoint using a kubeconfig file written on the control-plane node(s). -> **The path names the source cluster.** Audit routes are `/audit-webhook/` — the `ClusterProvider` whose facts this stream carries. The examples below use `/audit-webhook/default`, matching a `GitTarget` that omits `spec.clusterProviderRef`; substitute your provider's name. The bare `/audit-webhook` is the shared-stream endpoint for one audit feed carrying several logical clusters, and it is rejected with **400** unless `attribution.clusterAnnotationKey` is set — see [configuration.md](../configuration.md). This works best on clusters you fully control — k3s, k3d, Talos, Kamaji. Managed platforms (EKS, GKE, AKS) restrict access to apiserver configuration; running on them requires switching to a self-managed control plane or a platform that does expose it. +> **The path names the source cluster.** Audit routes are `/audit-webhook/` — the `ClusterProvider` whose facts this stream carries. The examples below use `/audit-webhook/default`, matching a `GitTarget` that omits `spec.clusterProviderRef`; substitute your provider's name. The bare `/audit-webhook` is the shared-stream endpoint for one audit feed carrying several logical clusters, and it is rejected with **400** unless `attribution.auditRouteAnnotationKey` is set — see [configuration.md](../configuration.md). This works best on clusters you fully control — k3s, k3d, Talos, Kamaji. Managed platforms (EKS, GKE, AKS) restrict access to apiserver configuration; running on them requires switching to a self-managed control plane or a platform that does expose it. ## The problem diff --git a/docs/finished/clusterprovider-fact-purge.md b/docs/finished/clusterprovider-fact-purge.md deleted file mode 100644 index 577e9b58..00000000 --- a/docs/finished/clusterprovider-fact-purge.md +++ /dev/null @@ -1,181 +0,0 @@ -# Fact purge for `ClusterProvider`: decision record - -> **finished** — shipped or closed. Kept for context only; **nothing here binds**. For current -> behaviour see [`../architecture.md`](../architecture.md). Index: [`../INDEX.md`](../INDEX.md) -> -> Prompted by a real failure: `helm uninstall` stranded the `default` `ClusterProvider` in -> `Terminating` forever, which then blocked reinstalling. This document asked whether -> purge-on-delete was the right mechanism. **Outcome: option D — no finalizer, and no purge.** - -## The decision - -**No finalizer, and no purge.** The `ClusterProvider` controller takes no finalizer; it only sheds -the retired one so objects created by older operators can still be deleted. - -Two steps got here. First, a finalizer is the wrong *mechanism*: purge-on-delete needs a living -operator, which `helm uninstall` does not provide. Second — and this is what settled it — the purge -itself is not earning its keep. The exact-key join is -`(cluster, group/resource, object UID, resourceVersion)`, and a re-provisioned cluster mints fresh -object UIDs, so a stale fact cannot match on that key at all. Combined with a TTL that clears -everything within minutes, and with the fact that repointing a provider name is an operator/automation -action rather than a routine one, the residual risk does not justify a mechanism. - -The rest of this document is the analysis that led there, kept because the reasoning matters more -than the conclusion. - -### The one caveat worth knowing - -The UID argument covers the exact key and the uid-keyed `:last` pointer. It does **not** cover the -**rv-only escape hatch** (`factKeyRV`), which is written when an audit fact carries no UID at all and -read as a last resort. resourceVersions are cluster-scoped integers, so a fresh cluster restarts low -and *can* collide with a stale fact from a previous incarnation. - -Reaching a wrong author through it needs all of: a no-UID fact recorded under the old incarnation, a -new object whose uid-keyed lookups miss, an RV collision between the two clusters, and all of it -inside the TTL. The result is already classed `AttributionWeak`, affects one commit's author (never -Git content), and self-clears. Accepted knowingly rather than overlooked. - -## What is being protected - -When author attribution is enabled, each mutating audit event is reduced to a small **fact** in Redis -(who did it, to which object, at which resourceVersion). A live watch event later looks the fact up -and the commit is authored by that user instead of the configured committer. - -Facts are keyed by **source cluster**, and the source cluster is a `ClusterProvider` **name** -([`attribution_index.go`](../../internal/queue/attribution_index.go)): - -``` -:author:v1:audit:cluster:::object:: -``` - -That name partitioning is what stops a fact from one cluster authoring a commit for a matching object -in another — an invariant covered by `TestAttributionIndex_CrossClusterIsolation` and -`TestAttributionIndex_RVOnlyHatchIsClusterScoped`. - -Two properties matter for this decision: - -- **Facts expire on their own.** They carry a TTL and nothing else deletes them. They are never object - state; a miss just means "absent". -- **A provider name can be reused for a different physical cluster.** `spec.kubeConfig` is immutable, - and the CEL rejection message tells you so explicitly: *"delete and recreate the ClusterProvider to - point a name at a different cluster"*. Delete-and-recreate is therefore the **supported** way to - repoint a name — not an exotic edge case. In practice it is an operator/automation action, not - something that happens on its own. - -## The actual risk - -Put those together and the hazard is precise: - -> A provider name is deleted and recreated against a **different cluster**, and a watch event from -> the new cluster joins a fact left over from the old one — crediting a user who never touched it. - -The window is bounded by the fact TTL. Outside that window there is no hazard at all, because the -facts are gone by themselves. - -This is a **misattribution** bug, not a data-loss one: Git content is unaffected, only the commit -author is. It is nonetheless the thing this whole feature exists to get right, so it is worth -addressing — but it is worth addressing *proportionately*. - -> **Note — the documented TTL is wrong.** `DefaultAttributionFactTTL` is **15 minutes**, but the -> `--author-attribution-ttl` flag help and [`configuration.md`](../configuration.md) both say -> "default 10m". Chart installs are unaffected in practice because `values.yaml` sets `ttl: "10m"` -> explicitly, but a non-chart install gets 15m while the docs promise 10m. Worth fixing regardless of -> which option below is chosen. - -## What the finalizer costs - -The finalizer (`configbutler.ai/clusterprovider-fact-purge`, now retired to -`LegacyClusterProviderFinalizer` and only ever removed) held the object until the controller had -scanned and deleted every fact under that provider name. - -The cost is that **a finalizer is a promise only a living operator can keep**: - -- **`helm uninstall` removes the operator and the `ClusterProvider` together.** Nothing is left to - detach the finalizer. The object strands in `Terminating` permanently, and the next - `helm install --wait` then fails on it: `resource ClusterProvider//default not ready. status: - Terminating`. Recovering needs a manual `kubectl patch` of the finalizer. Verified on a live - cluster, and the reason the narrow fix shipped. -- **If the operator is simply down when someone deletes a provider**, the finalizer blocks the delete - until it comes back — and if the object is force-removed to unblock, the purge never happens and the - facts leak anyway. So the finalizer does not even reliably deliver the guarantee it exists for. - -In the default configuration the trade was worse still: with no Redis there are no facts, so the -finalizer guarded *nothing* while still stranding the object. That is what the shipped fix removes. - -## Options - -### A. Unconditional finalizer (the old behaviour) - -Take the finalizer always; purge on delete. - -- **Pro** — simple, one code path. -- **Con** — strands the object on uninstall *even when there is nothing to purge*. This is the bug - that was hit. **Rejected.** - -### B. Conditional finalizer — *shipped, then superseded* - -Take the finalizer only when a `FactPurger` is wired (i.e. attribution is on). - -- **Pro** — small, safe, and removes the hazard for the chart default and the quickstart, which is - where it actually bit. No semantic change to the attribution path. -- **Con** — only moves the problem. With attribution **enabled**, `helm uninstall` can still strand - the provider, and the operator-is-down case is untouched. A partial fix. - -### C. Purge on adopt — *the right way to keep a purge, if we wanted one* - -Drop the finalizer. Instead, when a provider name is observed for the **first time**, purge anything -left under it. "First time" is detectable precisely as `status.observedGeneration == 0`: empty on a -freshly created object, and never reset by an operator restart, so a running provider is never -re-purged. - -- **Pro** — same guarantee ("a recreated name starts clean"), and it is the guarantee that actually - matters, because stale facts are only dangerous once the name is *in use again*. -- **Pro** — works when the operator was **down** during the delete, which the finalizer cannot do at - all. Strictly more robust for the stated goal. -- **Pro** — no finalizer, so no liveness hazard: uninstall, reinstall and force-delete all behave. -- **Con** — facts for a name that is never recreated linger until their TTL rather than being deleted - promptly. This is storage hygiene, not correctness, and the TTL already bounds it. -- **Con** — a bug in the "is this newly adopted?" test would purge **live** facts and silently degrade - attribution to committer-authored commits. This is the one real risk and it needs a test that pins - "a steady-state reconcile never purges". -- **Con** — needs a migration step: existing objects already carry the finalizer, so the controller - must actively strip it, or they will strand exactly as before. - -### D. No purge at all — rely on the TTL — **CHOSEN** - -Delete the mechanism entirely and accept the window. - -- **Pro** — simplest possible; no finalizer, no adopt hook, nothing to get wrong. -- **Con** — leaves a misattribution window on a supported workflow (repointing a name). Accepted: - the exact key includes the object UID, which a re-provisioned cluster does not reproduce, so the - window only exists for the narrow rv-only case described at the top. - -### E. Purge on adopt **and** keep the finalizer as best-effort - -Both: purge on adopt for correctness, finalizer for prompt cleanup. - -- **Pro** — facts also disappear promptly in the common case. -- **Con** — reintroduces the whole liveness hazard for a benefit the TTL already provides. The - finalizer is the expensive half and the least reliable half. **Not recommended.** - -## Outcome — option D - -Chosen over C on the grounds that the purge protects against something the key shape already -prevents. Option C's adopt-purge would have been the right way to *keep* a purge; it just turned out -not to be worth keeping one. - -What was implemented: - -1. **No finalizer is ever taken.** `AddFinalizer` is gone. -2. **The retired finalizer is shed**, including on an object already stuck in `Terminating` — that is - the only way an object stranded by an older operator becomes deletable again, so the shed runs - *before* the deletion check rather than after it. -3. `ClusterFactPurger` and its wiring are removed. `AttributionIndex.PurgeClusterFacts` remains - (exported and tested) so a future purge has something to call. - -`PurgeClusterFacts` was deliberately left in place. If the rv-only caveat above ever shows up in -practice, purge-on-adopt (option C) is the way to reintroduce it, and the mechanism is still there. - -## Status - -Implemented. The rv-only collision is a known, accepted residual risk. diff --git a/docs/finished/multi-cluster-author-attribution.md b/docs/finished/multi-cluster-author-attribution.md index aa887065..a9a86fe5 100644 --- a/docs/finished/multi-cluster-author-attribution.md +++ b/docs/finished/multi-cluster-author-attribution.md @@ -8,8 +8,10 @@ > provider-name-partitioned attribution facts, reconcile-time namespace authorization, and named > audit routes. Remaining ingress work: > [`../design/multi-source-audit-ingress-hardening.md`](../design/multi-source-audit-ingress-hardening.md). -> Purge-on-delete was later answered separately: -> [`clusterprovider-fact-purge.md`](clusterprovider-fact-purge.md). +> Purge-on-delete was answered separately and rejected, and its record has since been removed. The +> reasoning that still binds lives on `LegacyClusterProviderFinalizer` in +> [`clusterprovider_controller.go`](../../internal/controller/clusterprovider_controller.go): no +> finalizer, because `helm uninstall` would strand the object in `Terminating`. ## The problem @@ -170,7 +172,7 @@ no recent mutations must not read as unready. | Admission attribution | Cut entirely (above) — needs a source-side agent, not a webhook. | | Mutable `kubeConfig` / endpoint repointing | v1 promised it; a mutable *endpoint* silently retargets and misattributes. `kubeConfig` is immutable; Secret **contents** rotation stays transparent. | | Workload identity (`configMapRef`), ServiceAccount impersonation | Present in the embedded Flux type; rejected by CEL for now. | -| Fact purge on delete | Answered separately and **rejected**: [`clusterprovider-fact-purge.md`](clusterprovider-fact-purge.md). | +| Fact purge on delete | Answered separately and **rejected**. No finalizer: `helm uninstall` would strand the object in `Terminating`. See `LegacyClusterProviderFinalizer` in [`clusterprovider_controller.go`](../../internal/controller/clusterprovider_controller.go). | | Managed control planes (EKS/GKE/AKS) | Needs the source-side agent. | | `ReferenceGrant`-style per-namespace consent | `allowedNamespaces` is one object for the admin; revisit if self-service consent is wanted. | diff --git a/internal/audit/outcome/outcome.go b/internal/audit/outcome/outcome.go index e0d18d31..7bf38ea2 100644 --- a/internal/audit/outcome/outcome.go +++ b/internal/audit/outcome/outcome.go @@ -84,10 +84,6 @@ const ( // that carries no source-cluster annotation, so it names no ClusterProvider. Never credited to a // fallback; a rising rate means a producer is not stamping the annotation. MissingClusterAnnotation Outcome = "missing_cluster_annotation" - // UnknownClusterProvider — the source-cluster annotation names a ClusterProvider that does not - // exist. Dropped rather than guessed: a wrong source cluster would let a user from one logical - // cluster author a matching object in another. - UnknownClusterProvider Outcome = "unknown_cluster_provider" // WriteError — a redis/enqueue failure; the event never reached the log. WriteError Outcome = "write_error" @@ -104,7 +100,7 @@ func (o Outcome) Category() Category { case NotNeeded, NilEvent, Stage, ReadOnlyOrUnknownVerb, FailedRequest, DryRun, UnchangedResourceVersion, MalformedAdditional, NonScaleSubresource, ShallowDropped, RVLessEmptyHighWater, OlderThanHighWater, NonNumericRV, - MissingClusterAnnotation, UnknownClusterProvider: + MissingClusterAnnotation: return Dropped case WriteError: return Error diff --git a/internal/audit/outcome/outcome_test.go b/internal/audit/outcome/outcome_test.go index bb1faa12..f8d70e9c 100644 --- a/internal/audit/outcome/outcome_test.go +++ b/internal/audit/outcome/outcome_test.go @@ -33,7 +33,6 @@ func TestOutcomeCategory(t *testing.T) { OlderThanHighWater: Dropped, NonNumericRV: Dropped, MissingClusterAnnotation: Dropped, - UnknownClusterProvider: Dropped, WriteError: Error, } for o, want := range cases { diff --git a/internal/controller/clusterprovider_controller.go b/internal/controller/clusterprovider_controller.go index 9e50c92c..ef548e6b 100644 --- a/internal/controller/clusterprovider_controller.go +++ b/internal/controller/clusterprovider_controller.go @@ -31,14 +31,21 @@ import ( // no longer added; it is only ever REMOVED, so an object created by an older operator can still be // deleted after an upgrade instead of stranding in Terminating. // -// It was dropped because purge-on-delete is a promise only a living operator can keep, and the two -// cases that matter break it: `helm uninstall` removes the manager and the ClusterProvider -// together, so nothing detaches the finalizer and the object strands forever (which then blocks -// reinstalling); and an operator that is simply down blocks the delete, then loses the purge anyway -// if the object is force-removed. Nor is the purge needed: facts are keyed by -// (cluster, group/resource, uid, resourceVersion) and expire on their own, so a re-provisioned -// cluster reusing a provider name produces different object UIDs and cannot join a stale fact on -// the exact key. See docs/finished/clusterprovider-fact-purge.md. +// DO NOT REINTRODUCE A FINALIZER HERE. It was dropped because purge-on-delete is a promise only a +// living operator can keep, and the two cases that matter break it. `helm uninstall` removes the +// manager and the ClusterProvider together, so nothing detaches the finalizer, the object strands in +// Terminating forever, and the next `helm install --wait` then fails on it — recovering needs a +// manual kubectl patch. That was hit on a live cluster and is why this shipped. An operator that is +// simply down blocks the delete instead, and then loses the purge anyway if the object is +// force-removed, so the finalizer does not even reliably deliver the guarantee it exists for. +// +// The purge it protected is not needed either, and is now unsafe. Facts are keyed by +// (audit route, group/resource, uid, resourceVersion) and expire on their own, so a re-provisioned +// cluster produces different object UIDs and cannot join a stale fact on the exact key. And since +// spec.attribution.auditRoute is what partitions them, SEVERAL ClusterProviders may share one route +// (an API server posts audit under exactly one), so purging on a single provider's deletion would +// drop the facts of every other provider on that route, and of the operator's own cluster, which was +// never torn down. A purge would have to be re-derived against the route, not the object. const LegacyClusterProviderFinalizer = "configbutler.ai/clusterprovider-fact-purge" // ClusterProviderReconciler reconciles a ClusterProvider object. It is the read-side peer of the diff --git a/internal/controller/clusterprovider_controller_unit_test.go b/internal/controller/clusterprovider_controller_unit_test.go index 22eaf4d6..e76cf13c 100644 --- a/internal/controller/clusterprovider_controller_unit_test.go +++ b/internal/controller/clusterprovider_controller_unit_test.go @@ -145,9 +145,10 @@ func TestClusterProviderReconcile_InClusterDefault(t *testing.T) { // TestClusterProviderReconcile_TakesNoFinalizer pins the contract: this controller never makes a // ClusterProvider undeletable. Nothing has to happen before one goes away — attribution facts are -// keyed by (cluster, group/resource, uid, resourceVersion) and expire on their own, so a +// keyed by (audit route, group/resource, uid, resourceVersion) and expire on their own, so a // re-provisioned cluster reusing a provider name mints different object UIDs and cannot join a -// stale fact. See docs/finished/clusterprovider-fact-purge.md. +// stale fact. The reason a finalizer must never come back is on LegacyClusterProviderFinalizer: +// helm uninstall would strand the object in Terminating. func TestClusterProviderReconcile_TakesNoFinalizer(t *testing.T) { provider := clusterProviderWithKubeConfig("default", "", "") cl := fake.NewClientBuilder(). diff --git a/internal/controller/gittarget_controller.go b/internal/controller/gittarget_controller.go index f8e7c7c5..3596ad91 100644 --- a/internal/controller/gittarget_controller.go +++ b/internal/controller/gittarget_controller.go @@ -219,6 +219,7 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( ctx, gitDest, target.SourceCluster(), + r.auditRouteFor(ctx, &target), target.EffectivePruneMode(), gitPathWasRefused, ); declareErr != nil { diff --git a/internal/controller/gittarget_source_cluster.go b/internal/controller/gittarget_source_cluster.go index 78dc1ca6..9fef6ee8 100644 --- a/internal/controller/gittarget_source_cluster.go +++ b/internal/controller/gittarget_source_cluster.go @@ -81,6 +81,26 @@ const ( GitTargetReasonClusterProviderReady = "ClusterProviderReady" ) +// auditRouteFor resolves the audit route this GitTarget's attribution facts are keyed under, from +// the referenced ClusterProvider's spec.attribution.auditRoute. It is read here, on the control +// plane, so the watch data plane can capture it on Declare and never needs a Kubernetes client of +// its own to attribute an event. +// +// An unreadable provider falls back to the provider NAME, which is exactly what AuditRoute() +// defaults to. So a transient read failure resolves the same route a provider that sets nothing +// would, and never a route that silently matches no facts. +func (r *GitTargetReconciler) auditRouteFor( + ctx context.Context, + target *configbutleraiv1alpha3.GitTarget, +) string { + name := target.SourceCluster() + var cp configbutleraiv1alpha3.ClusterProvider + if err := r.Get(ctx, k8stypes.NamespacedName{Name: name}, &cp); err != nil { + return name + } + return cp.AuditRoute() +} + // clusterProviderReadiness reads the referenced ClusterProvider's Ready condition and projects it, // mirroring gitProviderReadiness. It returns Unknown — which does NOT downgrade Ready — when the // provider cannot be observed (not found, or no Ready condition yet), so a not-yet-installed diff --git a/internal/controller/gittarget_source_cluster_test.go b/internal/controller/gittarget_source_cluster_test.go index aa6ec1c6..45b1af2e 100644 --- a/internal/controller/gittarget_source_cluster_test.go +++ b/internal/controller/gittarget_source_cluster_test.go @@ -384,7 +384,7 @@ func TestReconcile_UnauthorizedNamespaceStartsNoWatch(t *testing.T) { // client is wired) — which is exactly the capture a refused GitTarget must not produce. other := types.NewResourceReference("authorized", ns).WithUID("other-uid") _ = watchManager.DeclareForGitTarget( - context.Background(), other, providerName, configbutleraiv1alpha3.PruneOnEvent) + context.Background(), other, providerName, providerName, configbutleraiv1alpha3.PruneOnEvent) id, declaredOther := watchManager.DeclaredSourceCluster(other) require.True(t, declaredOther, "the positive control must declare, or the assertion above proves nothing") assert.Equal(t, providerName, id) @@ -493,3 +493,65 @@ func TestGitProviderReadiness_AllScenarios(t *testing.T) { }) } } + +// TestAuditRouteFor covers how a GitTarget learns the audit route its author facts are keyed under. +// The controller reads it here, on the control plane, so the watch data plane needs no Kubernetes +// client of its own to attribute an event. +// +// The fallback is the load-bearing case: an unreadable provider must resolve the provider NAME, +// which is exactly what AuditRoute() defaults to. Resolving anything else would key the read to a +// partition the audit handler never writes, which is the failure this whole change removes. +func TestAuditRouteFor(t *testing.T) { + const providerName = "srcns-delegating" + + target := func() *configbutleraiv1alpha3.GitTarget { + return &configbutleraiv1alpha3.GitTarget{ + ObjectMeta: metav1.ObjectMeta{Name: "gt", Namespace: "tenant"}, + Spec: configbutleraiv1alpha3.GitTargetSpec{ + ClusterProviderRef: &configbutleraiv1alpha3.ClusterProviderReference{Name: providerName}, + }, + } + } + provider := func(attribution *configbutleraiv1alpha3.ClusterProviderAttribution) client.Object { + return &configbutleraiv1alpha3.ClusterProvider{ + ObjectMeta: metav1.ObjectMeta{Name: providerName}, + Spec: configbutleraiv1alpha3.ClusterProviderSpec{Attribution: attribution}, + } + } + + tests := []struct { + name string + objects []client.Object + want string + }{ + { + name: "a declared route is used", + objects: []client.Object{ + provider(&configbutleraiv1alpha3.ClusterProviderAttribution{AuditRoute: "default"}), + }, + want: "default", + }, + { + name: "an empty attribution block falls back to the provider name", + objects: []client.Object{provider(&configbutleraiv1alpha3.ClusterProviderAttribution{})}, + want: providerName, + }, + { + name: "no attribution block falls back to the provider name", + objects: []client.Object{provider(nil)}, + want: providerName, + }, + { + name: "an unreadable provider falls back to the provider name", + objects: nil, + want: providerName, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cl := fake.NewClientBuilder().WithScheme(scScheme(t)).WithObjects(tc.objects...).Build() + r := &GitTargetReconciler{Client: cl} + assert.Equal(t, tc.want, r.auditRouteFor(context.Background(), target())) + }) + } +} diff --git a/internal/queue/attribution_index.go b/internal/queue/attribution_index.go index 85137222..8f9d4108 100644 --- a/internal/queue/attribution_index.go +++ b/internal/queue/attribution_index.go @@ -36,14 +36,17 @@ const DefaultKeyPrefix = "gitops-reverser" const ( // attributionKeySuffix namespaces audit-sourced resource author facts under the // top-level author domain, e.g. - // "gitops-reverser:author:v1:audit:cluster:::...". + // "gitops-reverser:author:v1:audit:route:::...". attributionKeySuffix = ":author:v1:audit:" - // clusterKeyInfix carries the SOURCE CLUSTER dimension (a ClusterProvider name, "default" - // for the in-cluster provider) so a fact from cluster A never joins a watch event from - // cluster B — the rv-only hatch especially, since RV is not globally unique. It sits right - // after attributionKeySuffix so a provider's facts share a single glob prefix - // "…:author:v1:audit:cluster::*", which the delete-on-provider purge scans. - clusterKeyInfix = "cluster:" + // routeKeyInfix carries the AUDIT ROUTE dimension so a fact from cluster A never joins a watch + // event from cluster B — the rv-only hatch especially, since RV is not globally unique. The + // route is what the audit events arrived under, NOT the ClusterProvider's name: an API server + // has one webhook backend and posts under one route, so several providers naming one cluster + // all declare that route and share its facts (ClusterProvider.AuditRoute()). It is spelled + // "route" rather than "auditRoute" because the key already says audit one segment earlier. It + // sits right after attributionKeySuffix so a route's facts share a single glob prefix + // "…:author:v1:audit:route::*", so one route's facts are inspectable with a single SCAN. + routeKeyInfix = "route:" // factObjectInfix groups every fact for one object under one prefix, so a SCAN of // ":object::*" shows the whole history-in-flight for that object. factObjectInfix = ":object:" @@ -126,7 +129,7 @@ type AttributionIndex struct { // It is a no-op for events without an objectRef, a resolvable name, or a user — those // can never name an author. The caller (the audit handler) has already rejected reads, // failures, dry-runs, and non-ResponseComplete stages. -func (a *AttributionIndex) RecordFact(ctx context.Context, providerName string, event auditv1.Event) error { +func (a *AttributionIndex) RecordFact(ctx context.Context, auditRoute string, event auditv1.Event) error { if event.ObjectRef == nil { return nil } @@ -139,7 +142,7 @@ func (a *AttributionIndex) RecordFact(ctx context.Context, providerName string, // A deletecollection is name-less, so it can never name a single object. When the // API server returns the deleted set, expand it into one fact per object instead. if strings.EqualFold(event.Verb, "deletecollection") { - return a.RecordDeleteCollectionFacts(ctx, providerName, event) + return a.RecordDeleteCollectionFacts(ctx, auditRoute, event) } op, _ := auditutil.VerbToOperation(event.Verb) @@ -179,7 +182,7 @@ func (a *AttributionIndex) RecordFact(ctx context.Context, providerName string, return fmt.Errorf("marshal attribution fact: %w", err) } - wrote, err := a.writeFactKeys(ctx, providerName, gr, uid, rv, raw) + wrote, err := a.writeFactKeys(ctx, auditRoute, gr, uid, rv, raw) if err != nil { return err } @@ -195,15 +198,19 @@ func (a *AttributionIndex) RecordFact(ctx context.Context, providerName string, // known, or the type-scoped rv-only escape hatch when the fact has an RV but no UID. // The exact key is written once per (uid, rv) and never contended, so there is no // conflict marking. It reports whether any key was written. -func (a *AttributionIndex) writeFactKeys(ctx context.Context, cluster, gr, uid, rv string, raw []byte) (bool, error) { +func (a *AttributionIndex) writeFactKeys( + ctx context.Context, + auditRoute, gr, uid, rv string, + raw []byte, +) (bool, error) { switch { case uid != "": if rv != "" { - if err := a.setFact(ctx, a.factKeyExact(cluster, gr, uid, rv), raw); err != nil { + if err := a.setFact(ctx, a.factKeyExact(auditRoute, gr, uid, rv), raw); err != nil { return false, fmt.Errorf("store exact attribution fact: %w", err) } } - if err := a.setFact(ctx, a.factKeyLast(cluster, gr, uid), raw); err != nil { + if err := a.setFact(ctx, a.factKeyLast(auditRoute, gr, uid), raw); err != nil { return false, fmt.Errorf("store last attribution fact: %w", err) } return true, nil @@ -211,7 +218,7 @@ func (a *AttributionIndex) writeFactKeys(ctx context.Context, cluster, gr, uid, // The §5 escape hatch: a UID-bearing fact's rv-only key would be dead (the watch // side always carries a UID and resolves via object::… first), so it is // written only when there is no UID. - if err := a.setFact(ctx, a.factKeyRV(cluster, gr, rv), raw); err != nil { + if err := a.setFact(ctx, a.factKeyRV(auditRoute, gr, rv), raw); err != nil { return false, fmt.Errorf("store rv-only attribution fact: %w", err) } return true, nil @@ -234,7 +241,7 @@ func (a *AttributionIndex) writeFactKeys(ctx context.Context, cluster, gr, uid, // See docs/spec/deletecollection-attribution-expander.md. func (a *AttributionIndex) RecordDeleteCollectionFacts( ctx context.Context, - providerName string, + auditRoute string, event auditv1.Event, ) error { if !strings.EqualFold(event.Verb, "deletecollection") || event.ObjectRef == nil || event.ObjectRef.Resource == "" { @@ -262,7 +269,7 @@ func (a *AttributionIndex) RecordDeleteCollectionFacts( } return a.storeDeleteCollectionFacts( ctx, - providerName, + auditRoute, event.ObjectRef.APIGroup, event.ObjectRef.Resource, items, @@ -275,7 +282,7 @@ func (a *AttributionIndex) RecordDeleteCollectionFacts( // one item was written. func (a *AttributionIndex) storeDeleteCollectionFacts( ctx context.Context, - cluster string, + auditRoute string, group, resource string, items []deleteCollectionItem, base AuthorFact, @@ -295,7 +302,7 @@ func (a *AttributionIndex) storeDeleteCollectionFacts( if err != nil { return fmt.Errorf("marshal deletecollection fact: %w", err) } - if err := a.setFact(ctx, a.factKeyLast(cluster, gr, string(item.UID)), raw); err != nil { + if err := a.setFact(ctx, a.factKeyLast(auditRoute, gr, string(item.UID)), raw); err != nil { return fmt.Errorf("store deletecollection fact %q: %w", item.Name, err) } expanded = true @@ -351,13 +358,13 @@ func deleteCollectionItems(obj *runtime.Unknown) []deleteCollectionItem { // policy: see LookupAuthorResolution. func (a *AttributionIndex) LookupAuthor( ctx context.Context, - providerName string, + auditRoute string, gvr schema.GroupVersionResource, uid types.UID, rv string, exactCapable bool, ) (AuthorFact, bool) { - resolution := a.LookupAuthorResolution(ctx, providerName, gvr, uid, rv, exactCapable) + resolution := a.LookupAuthorResolution(ctx, auditRoute, gvr, uid, rv, exactCapable) return resolution.Fact, resolution.Result != AttributionAbsent } @@ -374,7 +381,7 @@ func (a *AttributionIndex) LookupAuthor( // A miss returns AttributionAbsent; there is no tombstone and so no expired outcome. func (a *AttributionIndex) LookupAuthorResolution( ctx context.Context, - providerName string, + auditRoute string, gvr schema.GroupVersionResource, uid types.UID, rv string, @@ -382,17 +389,17 @@ func (a *AttributionIndex) LookupAuthorResolution( ) AuthorResolution { gr := groupResourceKey(gvr.Group, gvr.Resource) if uid != "" && rv != "" { - if res, ok := a.matchFactKey(ctx, a.factKeyExact(providerName, gr, string(uid), rv), false); ok { + if res, ok := a.matchFactKey(ctx, a.factKeyExact(auditRoute, gr, string(uid), rv), false); ok { return res } } if !exactCapable && uid != "" { - if res, ok := a.matchFactKey(ctx, a.factKeyLast(providerName, gr, string(uid)), true); ok { + if res, ok := a.matchFactKey(ctx, a.factKeyLast(auditRoute, gr, string(uid)), true); ok { return res } } if rv != "" { - if res, ok := a.matchFactKey(ctx, a.factKeyRV(providerName, gr, rv), true); ok { + if res, ok := a.matchFactKey(ctx, a.factKeyRV(auditRoute, gr, rv), true); ok { return res } } @@ -430,37 +437,36 @@ func attributionResultForFact(fact AuthorFact, weak bool) AttributionResult { return AttributionExactUser } -// clusterFactPrefix is the per-cluster glob prefix under which every fact for one source cluster -// lives, e.g. "gitops-reverser:author:v1:audit:cluster:prod-eu-1:". The delete-on-provider purge -// SCANs "*". -func (a *AttributionIndex) clusterFactPrefix(cluster string) string { - return resolveKeyPrefix(a.keyPrefix) + attributionKeySuffix + clusterKeyInfix + escapeKeyField(cluster) + ":" +// routeFactPrefix is the per-route glob prefix under which every fact for one audit route lives, +// e.g. "gitops-reverser:author:v1:audit:route:prod-eu-1:". +func (a *AttributionIndex) routeFactPrefix(auditRoute string) string { + return resolveKeyPrefix(a.keyPrefix) + attributionKeySuffix + routeKeyInfix + escapeKeyField(auditRoute) + ":" } // factKeyBase is the per-(cluster,type) prefix shared by every fact key, e.g. -// "gitops-reverser:author:v1:audit:cluster:default:apps/deployments". -func (a *AttributionIndex) factKeyBase(cluster, gr string) string { - return a.clusterFactPrefix(cluster) + gr +// "gitops-reverser:author:v1:audit:route:default:apps/deployments". +func (a *AttributionIndex) factKeyBase(auditRoute, gr string) string { + return a.routeFactPrefix(auditRoute) + gr } // factKeyExact is the immutable per-write fact key, e.g. -// "gitops-reverser:author:v1:audit:cluster:default:apps/deployments:object::101". -func (a *AttributionIndex) factKeyExact(cluster, gr, uid, rv string) string { - return a.factKeyBase(cluster, gr) + factObjectInfix + escapeKeyField(uid) + ":" + escapeKeyField(rv) +// "gitops-reverser:author:v1:audit:route:default:apps/deployments:object::101". +func (a *AttributionIndex) factKeyExact(auditRoute, gr, uid, rv string) string { + return a.factKeyBase(auditRoute, gr) + factObjectInfix + escapeKeyField(uid) + ":" + escapeKeyField(rv) } // factKeyLast is the latest-writer-wins pointer for an object, e.g. -// "gitops-reverser:author:v1:audit:cluster:default:apps/deployments:object::last". -func (a *AttributionIndex) factKeyLast(cluster, gr, uid string) string { - return a.factKeyBase(cluster, gr) + factObjectInfix + escapeKeyField(uid) + ":" + factLastLeaf +// "gitops-reverser:author:v1:audit:route:default:apps/deployments:object::last". +func (a *AttributionIndex) factKeyLast(auditRoute, gr, uid string) string { + return a.factKeyBase(auditRoute, gr) + factObjectInfix + escapeKeyField(uid) + ":" + factLastLeaf } // factKeyRV is the (cluster, type)-scoped rv-only escape hatch, e.g. -// "gitops-reverser:author:v1:audit:cluster:default:apps/deployments:rv:101". RV is opaque per the +// "gitops-reverser:author:v1:audit:route:default:apps/deployments:rv:101". RV is opaque per the // Kubernetes API contract and not globally unique — not even within a cluster's type, and // certainly not across clusters — so this key always includes both the cluster and the type. -func (a *AttributionIndex) factKeyRV(cluster, gr, rv string) string { - return a.factKeyBase(cluster, gr) + factRVInfix + escapeKeyField(rv) +func (a *AttributionIndex) factKeyRV(auditRoute, gr, rv string) string { + return a.factKeyBase(auditRoute, gr) + factRVInfix + escapeKeyField(rv) } // setFact writes one fact value under its key with the bounded fact TTL. No sibling @@ -469,38 +475,6 @@ func (a *AttributionIndex) setFact(ctx context.Context, key string, raw []byte) return a.client.Set(ctx, key, raw, a.factTTL).Err() } -// PurgeClusterFacts deletes every attribution fact keyed to one source cluster (a ClusterProvider -// name). It is run by the ClusterProvider delete finalizer so a recreated name — a cluster torn -// down and stood up again under the same provider name — starts with a clean keyspace and can -// never join a stale predecessor's facts. It SCANs the per-cluster glob prefix and deletes in -// batches (facts also self-expire on the TTL, so this is the belt to that suspenders). It returns -// the number of keys removed. -func (a *AttributionIndex) PurgeClusterFacts(ctx context.Context, providerName string) (int, error) { - pattern := a.clusterFactPrefix(providerName) + "*" - var cursor uint64 - var removed int - for { - keys, next, err := a.client.Scan(ctx, cursor, pattern, attributionFactScanBatchSize).Result() - if err != nil { - return removed, fmt.Errorf("scan cluster facts %q: %w", pattern, err) - } - if len(keys) > 0 { - if err := a.client.Del(ctx, keys...).Err(); err != nil { - return removed, fmt.Errorf("delete cluster facts %q: %w", pattern, err) - } - removed += len(keys) - } - cursor = next - if cursor == 0 { - break - } - } - if removed > 0 { - a.recordFactEvent(ctx, "cluster_purged") - } - return removed, nil -} - func (a *AttributionIndex) recordFactEvent(ctx context.Context, op string) { if telemetry.AttributionFactEventsTotal == nil { return diff --git a/internal/queue/attribution_index_test.go b/internal/queue/attribution_index_test.go index 95b2966a..ddab34c8 100644 --- a/internal/queue/attribution_index_test.go +++ b/internal/queue/attribution_index_test.go @@ -568,26 +568,6 @@ func TestAttributionIndex_RVOnlyHatchIsClusterScoped(t *testing.T) { require.Equal(t, "bob", us.Fact.Author, "same RV in another cluster must not leak across") } -// TestAttributionIndex_PurgeClusterFacts checks the delete-on-provider purge removes exactly one -// cluster's facts and leaves the others intact. -func TestAttributionIndex_PurgeClusterFacts(t *testing.T) { - idx := newTestAttributionIndex(t) - ctx := context.Background() - - require.NoError(t, idx.RecordFact(ctx, "prod-eu-1", mutationEvent("update", "uid-1", "101", "alice"))) - require.NoError(t, idx.RecordFact(ctx, "default", mutationEvent("update", "uid-2", "1", "bob"))) - - removed, err := idx.PurgeClusterFacts(ctx, "prod-eu-1") - require.NoError(t, err) - require.Positive(t, removed) - - _, ok := idx.LookupAuthor(ctx, "prod-eu-1", appsDeploymentGVR(), "uid-1", "101", true) - require.False(t, ok, "purged cluster's facts are gone") - other, ok := idx.LookupAuthor(ctx, "default", appsDeploymentGVR(), "uid-2", "1", true) - require.True(t, ok, "another cluster's facts are untouched") - require.Equal(t, "bob", other.Author) -} - // TestAttributionIndex_SingleProviderMatchesBareInstall proves a single-(default)-provider install // round-trips correctly: what RecordFact writes under "default" is exactly what a "default" read // joins, so a bare single-cluster install behaves as before the cluster dimension existed. @@ -603,19 +583,19 @@ func TestAttributionIndex_SingleProviderMatchesBareInstall(t *testing.T) { func TestAttributionIndex_FactKeyReadableFormat(t *testing.T) { idx := newTestAttributionIndex(t) - require.Equal(t, "gitops-reverser:author:v1:audit:cluster:default:apps/deployments:object:uid-1:101", + require.Equal(t, "gitops-reverser:author:v1:audit:route:default:apps/deployments:object:uid-1:101", idx.factKeyExact("default", "apps/deployments", "uid-1", "101")) - require.Equal(t, "gitops-reverser:author:v1:audit:cluster:default:apps/deployments:object:uid-1:last", + require.Equal(t, "gitops-reverser:author:v1:audit:route:default:apps/deployments:object:uid-1:last", idx.factKeyLast("default", "apps/deployments", "uid-1")) - require.Equal(t, "gitops-reverser:author:v1:audit:cluster:default:apps/deployments:rv:101", + require.Equal(t, "gitops-reverser:author:v1:audit:route:default:apps/deployments:rv:101", idx.factKeyRV("default", "apps/deployments", "101")) // A remote provider keys under its own name, so its facts never collide with the local ones. - require.Equal(t, "gitops-reverser:author:v1:audit:cluster:prod-eu-1:apps/deployments:object:uid-1:101", + require.Equal(t, "gitops-reverser:author:v1:audit:route:prod-eu-1:apps/deployments:object:uid-1:101", idx.factKeyExact("prod-eu-1", "apps/deployments", "uid-1", "101")) // The core group drops the group segment. - require.Equal(t, "gitops-reverser:author:v1:audit:cluster:default:configmaps:object:uid-2:last", + require.Equal(t, "gitops-reverser:author:v1:audit:route:default:configmaps:object:uid-2:last", idx.factKeyLast("default", "configmaps", "uid-2")) } @@ -636,3 +616,85 @@ func TestNewRedisStore_RequiresAddr(t *testing.T) { _, err := NewRedisStore(RedisStoreConfig{}) require.Error(t, err) } + +// TestAttributionIndex_SharedAuditRouteJoinsAcrossProviders is the reported bug at the keyspace +// layer. An API server has one audit webhook backend and posts under ONE route, so a fact recorded +// on that route must be readable by every ClusterProvider that declares it, whatever those +// providers are named. Before the route existed, each provider read under its own name and only the +// routed one ever matched. +func TestAttributionIndex_SharedAuditRouteJoinsAcrossProviders(t *testing.T) { + idx := newTestAttributionIndex(t) + ctx := context.Background() + + // The local API server posts to /audit-webhook/default, so the fact lands on that route only. + require.NoError(t, idx.RecordFact(ctx, "default", mutationEvent("update", "uid-1", "101", "alice"))) + + // A dedicated in-cluster provider named srcns-delegating declares auditRoute: default, so its + // GitTargets read the same partition and resolve the same author. + fact, ok := idx.LookupAuthor(ctx, "default", appsDeploymentGVR(), "uid-1", "101", true) + require.True(t, ok) + require.Equal(t, "alice", fact.Author) + + // A provider that did NOT declare the route reads its own name and finds nothing. This is the + // exact failure the bug report measured, kept as a test so the fix cannot silently regress into + // "every route resolves everything". + _, ok = idx.LookupAuthor(ctx, "srcns-delegating", appsDeploymentGVR(), "uid-1", "101", true) + require.False(t, ok, "a route nobody wrote to must miss, or the partition means nothing") +} + +// TestAttributionIndex_WriteFactKeysByFactShape pins which keys each fact shape writes, which is +// where the v3 schema's §5 escape hatch lives. The rules are not symmetric and the asymmetry is +// deliberate: a UID-bearing fact's rv-only key would be DEAD, because the watch side always carries +// a UID and resolves via object::… first, so writing one would be a key nobody ever reads. +func TestAttributionIndex_WriteFactKeysByFactShape(t *testing.T) { + const route, gr = "prod-eu-1", "apps/deployments" + raw := []byte(`{"author":"alice"}`) + + tests := []struct { + name string + uid, rv string + wantWrote bool + }{ + { + name: "uid and rv write the immutable exact key and the last pointer", + uid: "uid-1", rv: "101", wantWrote: true, + }, + { + name: "uid without rv writes only the last pointer", + uid: "uid-1", wantWrote: true, + }, + { + name: "rv without uid writes only the rv-only hatch", + rv: "101", wantWrote: true, + }, + { + name: "neither uid nor rv writes nothing at all", + // An event that carries no joinable identity cannot ever be matched, so recording it + // would leave a key that only expires. Reporting wrote=false also keeps the "written" + // fact-event counter honest. + wantWrote: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + idx, mr := newTestAttributionIndexWithRedis(t) + ctx := context.Background() + + wrote, err := idx.writeFactKeys(ctx, route, gr, tt.uid, tt.rv, raw) + require.NoError(t, err) + require.Equal(t, tt.wantWrote, wrote) + + exists := func(key string) bool { + _, getErr := mr.Get(key) + return getErr == nil + } + require.Equal(t, tt.uid != "" && tt.rv != "", exists(idx.factKeyExact(route, gr, tt.uid, tt.rv)), + "the exact key needs both a uid and the rv that write produced") + require.Equal(t, tt.uid != "", exists(idx.factKeyLast(route, gr, tt.uid)), + "the last-writer pointer is keyed by uid alone") + require.Equal(t, tt.uid == "" && tt.rv != "", exists(idx.factKeyRV(route, gr, tt.rv)), + "the rv-only hatch exists only for a fact that has no uid") + }) + } +} diff --git a/internal/queue/key_prefix_test.go b/internal/queue/key_prefix_test.go index e1c72e2a..e4de21c4 100644 --- a/internal/queue/key_prefix_test.go +++ b/internal/queue/key_prefix_test.go @@ -87,11 +87,11 @@ func TestRedisStore_KeyPrefixReachesEveryKeyFamily(t *testing.T) { store.watchCursorKey("gtuid-3", gvr, "team-a")) idx := store.AttributionIndex(0) - require.Equal(t, "cell-a:tenant-7:author:v1:audit:cluster:default:apps/deployments:object:uid-1:101", + require.Equal(t, "cell-a:tenant-7:author:v1:audit:route:default:apps/deployments:object:uid-1:101", idx.factKeyExact("default", "apps/deployments", "uid-1", "101")) - require.Equal(t, "cell-a:tenant-7:author:v1:audit:cluster:default:apps/deployments:object:uid-1:last", + require.Equal(t, "cell-a:tenant-7:author:v1:audit:route:default:apps/deployments:object:uid-1:last", idx.factKeyLast("default", "apps/deployments", "uid-1")) - require.Equal(t, "cell-a:tenant-7:author:v1:audit:cluster:default:apps/deployments:rv:101", + require.Equal(t, "cell-a:tenant-7:author:v1:audit:route:default:apps/deployments:rv:101", idx.factKeyRV("default", "apps/deployments", "101")) require.Equal(t, "cell-a:tenant-7:author:v1:command:cr-uid", store.CommandAuthorStore().key("cr-uid")) @@ -119,8 +119,8 @@ func TestRedisStore_ZeroValueStoreStillWritesPrefixedKeys(t *testing.T) { require.Equal(t, "gitops-reverser:watch:v1:target:gtuid-3:configmaps:cluster:last-rv", store.watchCursorKey("gtuid-3", coreConfigmapsGVR(), "")) require.Equal(t, "gitops-reverser:author:v1:command:cr-uid", store.CommandAuthorStore().key("cr-uid")) - require.Equal(t, "gitops-reverser:author:v1:audit:cluster:default:", - store.AttributionIndex(0).clusterFactPrefix("default")) + require.Equal(t, "gitops-reverser:author:v1:audit:route:default:", + store.AttributionIndex(0).routeFactPrefix("default")) } // Two reversers sharing one Redis/Valkey and one logical database must not read each @@ -154,7 +154,7 @@ func TestRedisStore_DistinctPrefixesIsolateCursors(t *testing.T) { } // The attribution telemetry gauge SCANs ":author:v1:audit:*" and the per-provider purge -// SCANs ":author:v1:audit:cluster::*". A prefix that contained a glob metacharacter +// SCANs ":author:v1:audit:route::*". A prefix that contained a glob metacharacter // would make either count/delete the wrong keyspace; validation rejects those, so the pattern is // always a literal prefix plus one trailing star. func TestAttributionIndex_ScanPatternIsPrefixed(t *testing.T) { @@ -162,5 +162,5 @@ func TestAttributionIndex_ScanPatternIsPrefixed(t *testing.T) { store := newPrefixedRedisStore(t, "tenant-a") idx := store.AttributionIndex(0) - require.Equal(t, "tenant-a:author:v1:audit:cluster:prod-eu-1:", idx.clusterFactPrefix("prod-eu-1")) + require.Equal(t, "tenant-a:author:v1:audit:route:prod-eu-1:", idx.routeFactPrefix("prod-eu-1")) } diff --git a/internal/watch/audit_route_test.go b/internal/watch/audit_route_test.go new file mode 100644 index 00000000..086ba00e --- /dev/null +++ b/internal/watch/audit_route_test.go @@ -0,0 +1,102 @@ +// SPDX-License-Identifier: Apache-2.0 + +package watch + +import ( + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// TestManager_AuditRouteForCluster covers the read side of the Declare-time capture. The watch +// manager holds no Kubernetes client for ClusterProviders, so the controller hands it the route and +// this is where an event's author lookup picks it up. +func TestManager_AuditRouteForCluster(t *testing.T) { + m := &Manager{} + + // Nothing captured yet: a lookup racing the first Declare must read the cluster id, which is the + // provider's own name and exactly what AuditRoute() defaults to. Anything else would key the + // read to a partition the handler never writes. + assert.Equal(t, "srcns-delegating", m.auditRouteForCluster("srcns-delegating")) + + m.rememberClusterAuditRoute("srcns-delegating", "default") + assert.Equal(t, "default", m.auditRouteForCluster("srcns-delegating")) + + // An empty route never overwrites a captured one: a Declare that could not resolve the provider + // passes the fallback, and that must not clobber a route already learned. + m.rememberClusterAuditRoute("srcns-delegating", "") + assert.Equal(t, "default", m.auditRouteForCluster("srcns-delegating")) + + // A different cluster keeps its own route, so the partition still separates clusters. + m.rememberClusterAuditRoute("prod-eu-1", "prod-eu-1") + assert.Equal(t, "prod-eu-1", m.auditRouteForCluster("prod-eu-1")) +} + +// TestManager_AuditRouteDiesWithItsCluster pins the teardown: a route captured for a cluster is +// dropped when the last GitTarget mirroring from it goes away, so a provider recreated against a +// different route cannot inherit its predecessor's partition. +func TestManager_AuditRouteDiesWithItsCluster(t *testing.T) { + m := &Manager{Log: logr.Discard()} + gitDest := types.NewResourceReference("target", "ns").WithUID("uid-1") + + m.rememberGitTargetCluster(gitDest, "prod-eu-1") + m.rememberClusterAuditRoute("prod-eu-1", "shared-route") + require.Equal(t, "shared-route", m.auditRouteForCluster("prod-eu-1")) + + m.forgetGitTargetCluster(gitDest) + assert.Equal(t, "prod-eu-1", m.auditRouteForCluster("prod-eu-1"), + "the captured route must die with its cluster, leaving the provider-name default") +} + +// TestRouteAttributionHealth_WarnsOnceForARouteThatNeverResolves pins the loud half of the design. +// A route no API server posts under produces a run of unresolved events and never a resolution, +// which is the one signature worth interrupting an operator for. Everything else stays quiet: a +// single late fact is ordinary, and a route that has ever resolved is merely missing one. +func TestRouteAttributionHealth_WarnsOnceForARouteThatNeverResolves(t *testing.T) { + t.Run("a run of misses on a never-resolved route warns exactly once", func(t *testing.T) { + var health routeAttributionHealth + for i := 1; i < attributionUnresolvedWarnThreshold; i++ { + warn, _ := health.observe("srcns-delegating", false) + assert.False(t, warn, "a short run of misses is ordinary under audit-batch delay") + } + warn, streak := health.observe("srcns-delegating", false) + assert.True(t, warn) + assert.Equal(t, attributionUnresolvedWarnThreshold, streak) + + warn, _ = health.observe("srcns-delegating", false) + assert.False(t, warn, "a configuration mistake is worth saying once, not once per event") + }) + + t.Run("a route that has resolved never warns", func(t *testing.T) { + var health routeAttributionHealth + _, _ = health.observe("default", true) + for range attributionUnresolvedWarnThreshold * 3 { + warn, _ := health.observe("default", false) + assert.False(t, warn, "a working route missing some facts is a freshness problem, not a misconfiguration") + } + }) + + t.Run("routes are tracked independently", func(t *testing.T) { + var health routeAttributionHealth + for range attributionUnresolvedWarnThreshold { + _, _ = health.observe("broken", false) + } + warn, _ := health.observe("other", false) + assert.False(t, warn, "one broken route must not implicate another") + }) + + t.Run("a resolution clears the streak", func(t *testing.T) { + var health routeAttributionHealth + for i := 1; i < attributionUnresolvedWarnThreshold; i++ { + _, _ = health.observe("flaky", false) + } + _, _ = health.observe("flaky", true) + warn, streak := health.observe("flaky", false) + assert.False(t, warn) + assert.Equal(t, 1, streak, "the run restarts, so an intermittent miss never accumulates into a warning") + }) +} diff --git a/internal/watch/author_resolver.go b/internal/watch/author_resolver.go index 25d8533b..7b5cbaaf 100644 --- a/internal/watch/author_resolver.go +++ b/internal/watch/author_resolver.go @@ -4,6 +4,7 @@ package watch import ( "context" + "sync" "time" "github.com/go-logr/logr" @@ -36,7 +37,7 @@ type AttributionLookup interface { // (also consult the last-writer-wins /last pointer). LookupAuthorResolution( ctx context.Context, - providerName string, + auditRoute string, gvr schema.GroupVersionResource, uid k8stypes.UID, rv string, @@ -84,7 +85,7 @@ type AuthorResolver interface { // lookup. cmd/main.go:258 only builds one with a non-nil index. ResolveAuthor( ctx context.Context, - providerName string, + auditRoute string, gvr schema.GroupVersionResource, uid k8stypes.UID, rv string, @@ -92,10 +93,56 @@ type AuthorResolver interface { ) (git.UserInfo, git.AttributionOutcome) } +// attributionUnresolvedWarnThreshold is how many consecutive unresolved events one audit route may +// produce, having never resolved a single one, before the resolver says so. It is not 1 because a +// lone miss is ordinary: an audit batch can arrive after the grace window under load. A run of them +// with nothing ever matched is the signature of a route nobody writes to, which is exactly the +// misconfiguration that used to be silent. +const attributionUnresolvedWarnThreshold = 5 + +// routeAttributionHealth tracks, per audit route, whether attribution has ever resolved and how many +// events have gone unresolved since. It exists to make one specific misconfiguration loud: a +// ClusterProvider whose spec.attribution.auditRoute names a route no API server posts under reads a +// partition nothing writes, so every commit is authored "unresolved" with no error, no condition, +// and no failed reconcile. +type routeAttributionHealth struct { + mu sync.Mutex + resolved map[string]bool + absent map[string]int + warned map[string]bool +} + +// observe records one resolution outcome for a route and reports whether this is the moment to warn, +// plus the current unresolved run length. It warns at most once per route per process: the condition +// is a configuration mistake, so repeating it every event would bury the log without telling anyone +// anything new. +func (h *routeAttributionHealth) observe(route string, resolved bool) (bool, int) { + h.mu.Lock() + defer h.mu.Unlock() + if h.resolved == nil { + h.resolved = map[string]bool{} + h.absent = map[string]int{} + h.warned = map[string]bool{} + } + if resolved { + h.resolved[route] = true + delete(h.absent, route) + return false, 0 + } + h.absent[route]++ + streak := h.absent[route] + if h.resolved[route] || h.warned[route] || streak < attributionUnresolvedWarnThreshold { + return false, streak + } + h.warned[route] = true + return true, streak +} + type attributionResolver struct { lookup AttributionLookup grace time.Duration log logr.Logger + health routeAttributionHealth } // NewAuthorResolver builds the conservative author resolver over the attribution @@ -112,7 +159,7 @@ func NewAuthorResolver( func (r *attributionResolver) ResolveAuthor( ctx context.Context, - providerName string, + auditRoute string, gvr schema.GroupVersionResource, uid k8stypes.UID, rv string, @@ -129,18 +176,16 @@ func (r *attributionResolver) ResolveAuthor( } deadline := time.Now().Add(r.grace) for { - resolution := r.lookup.LookupAuthorResolution(ctx, providerName, gvr, uid, rv, exactCapable) + resolution := r.lookup.LookupAuthorResolution(ctx, auditRoute, gvr, uid, rv, exactCapable) if resolution.Result != queue.AttributionAbsent { ui, outcome, result := r.userInfoForResolution(resolution) recordAttributionResolution(ctx, gvr, result, time.Since(start)) + r.health.observe(auditRoute, outcome == git.AttributionResolved) return ui, outcome } - if !time.Now().Before(deadline) { - recordAttributionResolution(ctx, gvr, queue.AttributionAbsent, time.Since(start)) - return git.UserInfo{}, git.AttributionUnresolved - } - if !sleepOrDone(ctx, attributionPollInterval) { + if !time.Now().Before(deadline) || !sleepOrDone(ctx, attributionPollInterval) { recordAttributionResolution(ctx, gvr, queue.AttributionAbsent, time.Since(start)) + r.warnIfRouteNeverResolves(auditRoute, gvr) return git.UserInfo{}, git.AttributionUnresolved } } @@ -184,3 +229,19 @@ func recordAttributionResolution( telemetry.AttributionResolutionWaitSeconds.Record(ctx, wait.Seconds(), attrs) } } + +// warnIfRouteNeverResolves says, once per audit route, that a route has produced a run of +// unresolved events and has never resolved one. That is the shape of a ClusterProvider pointed at a +// route no API server posts under, which is otherwise invisible: mirroring stays correct and only +// the commit author is lost. The message names the fix rather than the symptom. +func (r *attributionResolver) warnIfRouteNeverResolves(auditRoute string, gvr schema.GroupVersionResource) { + warn, streak := r.health.observe(auditRoute, false) + if !warn { + return + } + r.log.Info("no audit facts have ever arrived on this audit route; every commit through it is "+ + "authored as attribution-unresolved. An API server posts audit under ONE route, so a second "+ + "ClusterProvider naming the same cluster must set spec.attribution.auditRoute to the route "+ + "that cluster actually posts to (it defaults to the provider's own name)", + "auditRoute", auditRoute, "unresolvedInARow", streak, "gvr", gvr.String()) +} diff --git a/internal/watch/author_resolver_test.go b/internal/watch/author_resolver_test.go index bf2cb732..52b36537 100644 --- a/internal/watch/author_resolver_test.go +++ b/internal/watch/author_resolver_test.go @@ -165,3 +165,40 @@ func TestAuthorResolver_AuthorlessFactIsUnresolved(t *testing.T) { assert.Equal(t, git.AttributionUnresolved, outcome) } + +// TestAuthorResolver_WarnsOnceForARouteThatNeverResolves drives the whole resolver, not just the +// health counter, so the warning path is exercised the way production reaches it: repeated events +// on one audit route that never match a fact. +// +// This is the loud half of the audit-route fix. A ClusterProvider pointed at a route no API server +// posts under mirrors correctly and loses only the commit author, which is why the original bug went +// unnoticed until an explicit unresolved-author placeholder made it visible in Git. +func TestAuthorResolver_WarnsOnceForARouteThatNeverResolves(t *testing.T) { + // hitAfter is beyond any call this test makes, so every lookup misses. + lookup := &fakeLookup{hitAfter: 1 << 30} + resolver := NewAuthorResolver(lookup, 0, logr.Discard()) + concrete, ok := resolver.(*attributionResolver) + require.True(t, ok) + + const route = "srcns-delegating" + for range attributionUnresolvedWarnThreshold { + _, outcome := resolver.ResolveAuthor( + context.Background(), route, resolverGVR, "uid-1", "101", true) + require.Equal(t, git.AttributionUnresolved, outcome) + } + + // The threshold has been reached, so the route is marked warned and never warns again. + warn, _ := concrete.health.observe(route, false) + assert.False(t, warn, "a configuration mistake is worth saying once, not once per event") + + // A route that resolves is never implicated, even after the other one has warned. + other := &fakeLookup{ + resolution: queue.AuthorResolution{ + Fact: queue.AuthorFact{Author: "alice"}, + Result: queue.AttributionExactUser, + }, + } + healthy := NewAuthorResolver(other, 0, logr.Discard()) + _, outcome := healthy.ResolveAuthor(context.Background(), "default", resolverGVR, "uid-2", "1", true) + assert.Equal(t, git.AttributionResolved, outcome) +} diff --git a/internal/watch/cluster_context.go b/internal/watch/cluster_context.go index e5762ca9..41471e01 100644 --- a/internal/watch/cluster_context.go +++ b/internal/watch/cluster_context.go @@ -275,6 +275,39 @@ func (m *Manager) rememberGitTargetCluster(gitDest types.ResourceReference, clus m.gitTargetClusters[gitDest.Key()] = clusterID } +// rememberClusterAuditRoute captures the audit route a source cluster's attribution facts are keyed +// under, from (api/v1alpha3).ClusterProvider.AuditRoute(). It is keyed by CLUSTER, not by GitTarget: +// the route is a property of the provider, so every GitTarget mirroring from it writes the same +// value and the last Declare wins harmlessly. +// +// Unlike the cluster id, this is MUTABLE — spec.attribution.auditRoute may be edited to correct a +// misconfiguration. The GitTarget controller re-declares on a ClusterProvider spec change +// (clusterProviderReadyOrSpecChanged), so an edit lands here without a restart. +func (m *Manager) rememberClusterAuditRoute(clusterID, auditRoute string) { + if auditRoute == "" { + return + } + m.gitTargetClustersMu.Lock() + defer m.gitTargetClustersMu.Unlock() + if m.clusterAuditRoutes == nil { + m.clusterAuditRoutes = map[string]string{} + } + m.clusterAuditRoutes[clusterID] = auditRoute +} + +// auditRouteForCluster resolves the audit route a source cluster's facts are keyed under. An +// uncaptured cluster falls back to the cluster id, which is the ClusterProvider's own name and +// exactly what AuditRoute() defaults to, so a lookup racing the first Declare reads the same key a +// provider with no auditRoute set would. +func (m *Manager) auditRouteForCluster(clusterID string) string { + m.gitTargetClustersMu.Lock() + defer m.gitTargetClustersMu.Unlock() + if route := m.clusterAuditRoutes[clusterID]; route != "" { + return route + } + return clusterID +} + // DeclaredSourceCluster reports the source cluster captured for a GitTarget at Declare time and // whether that GitTarget has declared at all. It is the observable form of the capture-on-Declare // contract: a GitTarget the controller's Validated gate refused never reaches DeclareForGitTarget, @@ -317,6 +350,11 @@ func (m *Manager) forgetGitTargetCluster(gitDest types.ResourceReference) { break } } + // The captured route dies with the cluster it belongs to, so a provider recreated against a + // different audit route cannot inherit its predecessor's key. + if had && !stillReferenced { + delete(m.clusterAuditRoutes, clusterID) + } m.gitTargetClustersMu.Unlock() if !had || clusterID == configPlaneClusterID || stillReferenced { diff --git a/internal/watch/manager.go b/internal/watch/manager.go index f93f7040..b124a063 100644 --- a/internal/watch/manager.go +++ b/internal/watch/manager.go @@ -128,6 +128,12 @@ type Manager struct { gitTargetClustersMu sync.Mutex gitTargetClusters map[string]string + // clusterAuditRoutes maps a source-cluster id to the audit route its attribution facts are + // keyed under (ClusterProvider.AuditRoute()), also captured on Declare. It is keyed by CLUSTER + // rather than by GitTarget because the route belongs to the provider, and unlike the cluster id + // it is mutable. Guarded by gitTargetClustersMu, alongside the map it is captured with. + clusterAuditRoutes map[string]string + // resourceCatalogMu guards every clusterContext's catalog/registry edge-triggered // logging state (catalogDegradedLogged, typeRefusalsLogged). resourceCatalogMu sync.Mutex diff --git a/internal/watch/materialization.go b/internal/watch/materialization.go index 0523d3c8..a9b68e23 100644 --- a/internal/watch/materialization.go +++ b/internal/watch/materialization.go @@ -23,14 +23,17 @@ func (m *Manager) DeclareForGitTarget( ctx context.Context, gitDest types.ResourceReference, clusterID string, + auditRoute string, pruneMode v1alpha3.PruneMode, forceRecheck ...bool, ) error { - // Capture the UID and the source cluster before starting watches: the data plane keys its - // resume cursors by GitTarget UID, and resolves rules/opens watches against the captured - // cluster's context — neither of which the rule-derived watch tables carry. + // Capture the UID, the source cluster, and that cluster's audit route before starting watches: + // the data plane keys its resume cursors by GitTarget UID, resolves rules/opens watches against + // the captured cluster's context, and reads author facts under the captured route — none of + // which the rule-derived watch tables carry. m.rememberGitTargetUID(gitDest) m.rememberGitTargetCluster(gitDest, clusterID) + m.rememberClusterAuditRoute(clusterID, auditRoute) force := (len(forceRecheck) > 0 && forceRecheck[0]) || m.pruneModeRequiresReplay(gitDest, pruneMode) if err := m.EnsureGitTargetWatches(ctx, gitDest, force); err != nil { m.Log.Info("watch-first declare skipped; surface not observable", diff --git a/internal/watch/target_watch.go b/internal/watch/target_watch.go index ea5d54bb..d495e0c6 100644 --- a/internal/watch/target_watch.go +++ b/internal/watch/target_watch.go @@ -764,12 +764,15 @@ func (m *Manager) attachAuthor( // author fact's post-write RV, so it may consult the /last pointer; a create/update is // exact-capable and must not fall through to /last. exactCapable := event.Operation != string(configv1alpha3.OperationDelete) - // event.SourceCluster (stamped just above, before this call) is the SOURCE CLUSTER — the - // ClusterProvider name — this event was watched on. It keys the author read against exactly - // the facts the audit handler recorded for that cluster, so a fact from cluster A can never - // name the author of an object watched on cluster B. + // event.SourceCluster (stamped just above, before this call) is the ClusterProvider NAME this + // event was watched on; auditRouteForCluster turns it into the AUDIT ROUTE the handler filed + // facts under. The two differ whenever several providers name one cluster: an API server has one + // webhook backend and posts under one route, so every other provider for that cluster declares + // that same route and joins the same facts. Keying the read on the provider name instead was + // the bug this indirection exists to prevent, and a fact from cluster A still cannot name the + // author of an object watched on cluster B, because their routes differ. userInfo, outcome := m.AuthorResolver.ResolveAuthor( - ctx, event.SourceCluster, gvr, u.GetUID(), u.GetResourceVersion(), exactCapable, + ctx, m.auditRouteForCluster(event.SourceCluster), gvr, u.GetUID(), u.GetResourceVersion(), exactCapable, ) // Stamp the outcome even when no actor was named: an unresolved attribution is a fact the // writer, the author_kind metric, and CommitRequest matching all need. Leaving it at the diff --git a/internal/webhook/audit_handler.go b/internal/webhook/audit_handler.go index e24b428e..386377f5 100644 --- a/internal/webhook/audit_handler.go +++ b/internal/webhook/audit_handler.go @@ -43,23 +43,13 @@ type auditHandlerFirsts struct { unroutableEvent sync.Once } -// AuditFactRecorder stores the minimal author-attribution fact for one accepted, -// mutating audit event under a SOURCE CLUSTER (a ClusterProvider name), so a fact from -// one cluster never joins a watch event from another. It is the only thing the audit -// webhook does now: watch carries the object body, so audit is a pure attribution lookup -// table. A nil recorder means configured-author mode — the handler is not wired at all. +// AuditFactRecorder stores the minimal author-attribution fact for one accepted, mutating audit +// event under the AUDIT ROUTE it arrived on, so a fact from one cluster never joins a watch event +// from another. It is the only thing the audit webhook does now: watch carries the object body, so +// audit is a pure attribution lookup table. A nil recorder means configured-author mode — the +// handler is not wired at all. type AuditFactRecorder interface { - RecordFact(ctx context.Context, providerName string, event auditv1.Event) error -} - -// AuditProviderResolver reports whether a named source cluster (a ClusterProvider) exists, so an -// /audit-webhook/ route is accepted only for a configured cluster. It is the gate behind the -// connection's mTLS: the audit server already requires a CA-signed client cert -// (RequireAndVerifyClientCert), so an unauthenticated apiserver never reaches here; this then -// refuses a route for a provider that does not exist, rather than accumulating orphan facts. Every -// name is gated the same way, "default" included. A nil resolver means no route can be served. -type AuditProviderResolver interface { - ProviderExists(ctx context.Context, name string) (bool, error) + RecordFact(ctx context.Context, auditRoute string, event auditv1.Event) error } // AuditHandlerConfig contains configuration for the audit handler. @@ -70,16 +60,12 @@ type AuditHandlerConfig struct { // A write failure returns an audit-request error so the API server retries // delivery; mirrored-resource author attribution depends on these facts. FactRecorder AuditFactRecorder - // ProviderResolver gates every /audit-webhook/ route on the ClusterProvider existing. - // Nil means named routes are all 404. - ProviderResolver AuditProviderResolver - // ClusterAnnotationKey enables the bare /audit-webhook endpoint for a SHARED audit stream that - // carries several logical clusters: the owning ClusterProvider is read PER EVENT from this - // audit-event annotation, so one batch may fan out to several source clusters. Empty (the - // default) means the bare endpoint is NOT enabled and every producer must post to a named - // /audit-webhook/. Setting it requires a ProviderResolver — an annotation naming an - // unknown provider must be rejected, never guessed. - ClusterAnnotationKey string + // AuditRouteAnnotationKey enables the bare /audit-webhook endpoint for a SHARED audit stream + // that carries several logical clusters: the AUDIT ROUTE is read PER EVENT from this + // audit-event annotation, so one batch may fan out to several routes. Empty (the default) means + // the bare endpoint is NOT enabled and every producer must post to a named + // /audit-webhook/. + AuditRouteAnnotationKey string } // AuditHandler receives kube-apiserver audit events on /audit-webhook and records @@ -97,12 +83,6 @@ func NewAuditHandler(config AuditHandlerConfig) (*AuditHandler, error) { if config.MaxRequestBodyBytes <= 0 { config.MaxRequestBodyBytes = DefaultAuditMaxRequestBodyBytes } - // Annotation routing must be able to reject an unknown provider, so it cannot run without a - // resolver. Fail at startup rather than 400-ing every bare request at runtime. - if config.ClusterAnnotationKey != "" && config.ProviderResolver == nil { - return nil, errors.New("cluster annotation routing requires a ProviderResolver") - } - scheme := runtime.NewScheme() if err := audit.AddToScheme(scheme); err != nil { return nil, fmt.Errorf("failed to initialize scheme: %w", err) @@ -142,7 +122,7 @@ func (h *AuditHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - route, ok := h.resolveRoute(ctx, w, r) + route, ok := h.resolveRoute(w, r) if !ok { return } @@ -155,55 +135,45 @@ func (h *AuditHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { // resolveRoute maps an already-syntactically-valid path to the route its events belong to. It // writes the rejection itself and reports ok=false when the request cannot be served at all — as // opposed to a per-event rejection, which still returns 200 for the rest of the batch. -func (h *AuditHandler) resolveRoute(ctx context.Context, w http.ResponseWriter, r *http.Request) (auditRoute, bool) { - providerName, named := providerRouteForPath(r.URL.Path) +func (h *AuditHandler) resolveRoute(w http.ResponseWriter, r *http.Request) (auditRoute, bool) { + route, named := auditRouteForPath(r.URL.Path) if !named { // The bare endpoint never guesses a source cluster. It exists only to demultiplex a SHARED // stream by annotation, so without a configured key there is nothing it could mean and the // producer is simply misconfigured: reject the whole request rather than silently dropping // every event in it. - if h.config.ClusterAnnotationKey == "" { + if h.config.AuditRouteAnnotationKey == "" { http.Error(w, "the bare /audit-webhook endpoint is not enabled; post to "+ - "/audit-webhook/", http.StatusBadRequest) + "/audit-webhook/", http.StatusBadRequest) return auditRoute{}, false } - return auditRoute{annotationKey: h.config.ClusterAnnotationKey}, true + return auditRoute{annotationKey: h.config.AuditRouteAnnotationKey}, true } - // The connection is already CA-authenticated (the audit server requires a client cert), so gate - // on the named ClusterProvider existing rather than accepting facts for an unknown cluster. - // Every name is gated, "default" included — it is an ordinary provider. - if h.config.ProviderResolver == nil { - http.NotFound(w, r) - return auditRoute{}, false - } - exists, err := h.config.ProviderResolver.ProviderExists(ctx, providerName) - if err != nil { - http.Error(w, "resolve source cluster", http.StatusServiceUnavailable) - return auditRoute{}, false - } - if !exists { - http.NotFound(w, r) - return auditRoute{}, false - } - return auditRoute{provider: providerName}, true + // A named route is accepted as-is. Ingestion does NOT check that a ClusterProvider carries this + // route: the route is a partition name, not a claim about an object, and a fact filed under a + // route nobody reads costs one key that expires on the fact TTL. Refusing here instead dropped + // audit batches in flight while a provider was being created or recreated, which the API server + // does not retry after a 404. The connection is already CA-authenticated (the audit server + // requires a client cert), which is the boundary that keeps an unauthenticated producer out. + return auditRoute{route: route}, true } -// auditRoute is how one accepted request maps its events to SOURCE CLUSTERS. Exactly one field is -// set: a named /audit-webhook/ route fixes `provider` for the whole batch, while the bare -// /audit-webhook route carries `annotationKey` and resolves a provider PER EVENT, so a single batch -// from a shared stream may fan out to several source clusters. +// auditRoute is how one accepted request maps its events to AUDIT ROUTES, the dimension the +// attribution facts are partitioned by. Exactly one field is set: a named /audit-webhook/ +// path fixes `route` for the whole batch, while the bare /audit-webhook carries `annotationKey` and +// reads the route PER EVENT, so a single batch from a shared stream may fan out to several routes. type auditRoute struct { - provider string + route string annotationKey string } -// providerRouteForPath maps an accepted audit route to the SOURCE CLUSTER its facts belong to and -// whether it is a NAMED route. Audit routes are named: /audit-webhook/ carries the provider -// (named=true), including /audit-webhook/default. The bare /audit-webhook names no provider — -// it is the shared, annotation-routed endpoint, so it returns ("", false) and the caller resolves -// each event separately. validateAuditWebhookPath has already accepted the path syntactically. -func providerRouteForPath(path string) (string, bool) { +// auditRouteForPath maps an accepted request path to the AUDIT ROUTE its facts belong to and +// whether the path named one. /audit-webhook/ carries the route (named=true). The bare +// /audit-webhook names none — it is the shared, annotation-routed endpoint, so it returns +// ("", false) and the caller reads each event's route separately. validateAuditWebhookPath has +// already accepted the path syntactically. +func auditRouteForPath(path string) (string, bool) { segment := strings.TrimPrefix(path, "/audit-webhook/") if segment == path || segment == "" { return "", false @@ -333,16 +303,13 @@ func (h *AuditHandler) processEvent(ctx context.Context, route auditRoute, event return nil } - providerName, routed, err := h.resolveEventProvider(ctx, route, &event) - if err != nil { - return err - } + eventRoute, routed := h.resolveEventRoute(ctx, route, &event) if !routed { return nil } if h.config.FactRecorder != nil { - if err := h.config.FactRecorder.RecordFact(ctx, providerName, event); err != nil { + if err := h.config.FactRecorder.RecordFact(ctx, eventRoute, event); err != nil { outcome.Record(ctx, &event, outcome.WriteError) return fmt.Errorf("record attribution fact %q: %w", event.AuditID, err) } @@ -358,39 +325,32 @@ func (h *AuditHandler) processEvent(ctx context.Context, route auditRoute, event return nil } -// resolveEventProvider returns the ClusterProvider that owns one accepted event, and whether it -// routed at all. On a named route that is the route's provider, unconditionally. On the shared, -// annotation-routed bare endpoint it is read from the event's own annotations and existence-checked: -// an event carrying no annotation, or naming a ClusterProvider that does not exist, is REJECTED — -// it produces no fact and is never credited to a fallback provider, because a wrong source cluster -// would let a user from one logical cluster author a matching object in another. The rejection is -// per EVENT so correctly-annotated events in the same batch still land. A resolver failure is -// transient rather than a verdict, so it is returned as an error and the API server retries the -// whole batch. -func (h *AuditHandler) resolveEventProvider( +// resolveEventRoute returns the AUDIT ROUTE one accepted event's fact is filed under, and whether it +// routed at all. On a named route that is the route's own value, unconditionally. On the shared, +// annotation-routed bare endpoint it is read from the event's own annotations, and an event carrying +// no annotation is REJECTED: it produces no fact and is never credited to a fallback route, because +// a wrong route would let a user from one logical cluster author a matching object in another. The +// rejection is per EVENT so correctly-annotated events in the same batch still land. +// +// An annotation naming a route no ClusterProvider has declared is NOT rejected. The route is a +// partition name rather than a claim about an object, so the fact is stored and expires unread if +// nothing ever joins it. That keeps ingestion free of Kubernetes reads on the hot path, and keeps a +// provider that is created (or recreated) after its events have started flowing from losing them. +func (h *AuditHandler) resolveEventRoute( ctx context.Context, route auditRoute, event *auditv1.Event, -) (string, bool, error) { +) (string, bool) { if route.annotationKey == "" { - return route.provider, true, nil + return route.route, true } name := event.Annotations[route.annotationKey] if name == "" { h.rejectUnroutableEvent(ctx, event, outcome.MissingClusterAnnotation, route.annotationKey, name) - return "", false, nil - } - - exists, err := h.config.ProviderResolver.ProviderExists(ctx, name) - if err != nil { - return "", false, fmt.Errorf("resolve source cluster %q: %w", name, err) - } - if !exists { - h.rejectUnroutableEvent(ctx, event, outcome.UnknownClusterProvider, route.annotationKey, name) - return "", false, nil + return "", false } - return name, true, nil + return name, true } // rejectUnroutableEvent counts and logs one event the shared endpoint could not route. Counting it diff --git a/internal/webhook/audit_handler_test.go b/internal/webhook/audit_handler_test.go index 352467b8..fcda981b 100644 --- a/internal/webhook/audit_handler_test.go +++ b/internal/webhook/audit_handler_test.go @@ -108,21 +108,16 @@ func eventListFixtureBody(t *testing.T, path string) string { return string(body) } -// defaultRoute is the named audit route for a ClusterProvider called "default". Audit routes are -// NAMED, and the bare /audit-webhook is the shared, annotation-routed endpoint that is off unless -// ClusterAnnotationKey is set — so every test about event classification (rather than routing) +// defaultRoute is the named audit route a single-cluster install posts to. Audit routes are NAMED, +// and the bare /audit-webhook is the shared, annotation-routed endpoint that is off unless +// AuditRouteAnnotationKey is set — so every test about event classification (rather than routing) // posts here, exactly as a single-cluster apiserver would. const defaultRoute = "/audit-webhook/default" -// routedConfig fills in the ProviderResolver that defaultRoute is existence-gated on, so a -// classification test can keep stating only what it is actually about. A test that cares about the -// gate supplies its own resolver, which is left untouched. -func routedConfig(config AuditHandlerConfig) AuditHandlerConfig { - if config.ProviderResolver == nil { - config.ProviderResolver = fakeProviderResolver{existing: map[string]bool{"default": true}} - } - return config -} +// routedConfig used to fill in the ProviderResolver that defaultRoute was existence-gated on. +// Ingestion no longer reads Kubernetes at all, so it is now identity, kept so the classification +// tests keep reading as "post to a normal route" rather than growing an explanation each. +func routedConfig(config AuditHandlerConfig) AuditHandlerConfig { return config } // serveBody runs one POST request through the handler and returns the recorder. func serveBody(t *testing.T, handler *AuditHandler, method, path, body string) *httptest.ResponseRecorder { @@ -156,69 +151,40 @@ func TestAuditHandler_NamedDefaultRouteThreadsItsProvider(t *testing.T) { assert.Equal(t, "default", recorder.lastProvider(), "the named default route keys facts by its own name") } -// fakeProviderResolver answers ProviderExists from a fixed set, or with an injectable error. -type fakeProviderResolver struct { - existing map[string]bool - err error -} - -func (f fakeProviderResolver) ProviderExists(_ context.Context, name string) (bool, error) { - if f.err != nil { - return false, f.err - } - return f.existing[name], nil -} - -// TestAuditHandler_NamedRouting checks the /audit-webhook/ gate: an existing provider is -// served and its facts keyed by name; a missing provider is 404 (for "default" too); a resolver -// error is 503; and the bare endpoint is not a fallback for any of them. +// TestAuditHandler_NamedRouting checks the /audit-webhook/ mapping: the route is taken from +// the path verbatim and keys the facts, with NO check that a ClusterProvider declares it. Ingestion +// is a partition write, not a claim about an object, so a route nobody reads costs one expiring key +// and a provider created later still joins its facts. func TestAuditHandler_NamedRouting(t *testing.T) { body := eventListBody(acceptedCreateEvent) - t.Run("existing provider records under its name", func(t *testing.T) { + t.Run("a named route records under its own name", func(t *testing.T) { recorder := &fakeFactRecorder{} - handler, err := NewAuditHandler(AuditHandlerConfig{ - FactRecorder: recorder, - ProviderResolver: fakeProviderResolver{existing: map[string]bool{"prod-eu-1": true}}, - }) + handler, err := NewAuditHandler(AuditHandlerConfig{FactRecorder: recorder}) require.NoError(t, err) w := serveBody(t, handler, http.MethodPost, "/audit-webhook/prod-eu-1", body) require.Equal(t, http.StatusOK, w.Code) assert.Equal(t, "prod-eu-1", recorder.lastProvider()) }) - t.Run("missing provider is 404, records nothing", func(t *testing.T) { + t.Run("a route no ClusterProvider declares is stored, not refused", func(t *testing.T) { recorder := &fakeFactRecorder{} - handler, err := NewAuditHandler(AuditHandlerConfig{ - FactRecorder: recorder, - ProviderResolver: fakeProviderResolver{existing: map[string]bool{}}, - }) + handler, err := NewAuditHandler(AuditHandlerConfig{FactRecorder: recorder}) require.NoError(t, err) - w := serveBody(t, handler, http.MethodPost, "/audit-webhook/gone", body) - assert.Equal(t, http.StatusNotFound, w.Code) - assert.Zero(t, recorder.len()) + w := serveBody(t, handler, http.MethodPost, "/audit-webhook/not-declared-yet", body) + require.Equal(t, http.StatusOK, w.Code, + "a 404 here dropped batches in flight while a provider was being created; "+ + "the API server does not retry one") + assert.Equal(t, "not-declared-yet", recorder.lastProvider()) }) - t.Run("resolver error is 503", func(t *testing.T) { - handler, err := NewAuditHandler(AuditHandlerConfig{ - FactRecorder: &fakeFactRecorder{}, - ProviderResolver: fakeProviderResolver{err: assert.AnError}, - }) - require.NoError(t, err) - w := serveBody(t, handler, http.MethodPost, "/audit-webhook/prod-eu-1", body) - assert.Equal(t, http.StatusServiceUnavailable, w.Code) - }) - - t.Run("default is existence-gated like any other name", func(t *testing.T) { + t.Run("default is an ordinary route", func(t *testing.T) { recorder := &fakeFactRecorder{} - handler, err := NewAuditHandler(AuditHandlerConfig{ - FactRecorder: recorder, - ProviderResolver: fakeProviderResolver{existing: map[string]bool{}}, // default absent - }) + handler, err := NewAuditHandler(AuditHandlerConfig{FactRecorder: recorder}) require.NoError(t, err) w := serveBody(t, handler, http.MethodPost, defaultRoute, body) - assert.Equal(t, http.StatusNotFound, w.Code, "default has no privileged route") - assert.Zero(t, recorder.len()) + require.Equal(t, http.StatusOK, w.Code) + assert.Equal(t, "default", recorder.lastProvider(), "default has no privileged route") }) t.Run("bare endpoint is 400 while no annotation key is configured", func(t *testing.T) { @@ -227,12 +193,13 @@ func TestAuditHandler_NamedRouting(t *testing.T) { require.NoError(t, err) w := serveBody(t, handler, http.MethodPost, "/audit-webhook", body) assert.Equal(t, http.StatusBadRequest, w.Code, - "the bare endpoint resolves no provider of its own, so a producer posting to it is misconfigured") + "the bare endpoint reads no route of its own, so a producer posting to it is misconfigured") assert.Zero(t, recorder.len()) }) } -// clusterAnnotation is the annotation key a shared audit stream stamps its source cluster into. +// clusterAnnotation is the audit-event annotation a shared stream stamps with each event's audit +// route, matching --author-attribution-audit-route-annotation-key. const clusterAnnotation = "example.io/source-cluster" // annotatedEvent builds an otherwise-acceptable create event carrying an audit-event annotation @@ -252,25 +219,23 @@ func annotatedEvent(auditID string, annotations map[string]string) string { } // TestAuditHandler_AnnotationRouting pins the shared-stream contract: with an annotation key -// configured the bare endpoint resolves the ClusterProvider PER EVENT, so one batch fans out to -// several source clusters, and an event that names none — or names one that does not exist — is -// rejected by itself, never credited to a fallback, while the rest of the batch still lands. +// configured the bare endpoint reads the AUDIT ROUTE per event, so one batch fans out to several +// routes, and an event that names none is rejected by itself, never credited to a fallback, while +// the rest of the batch still lands. A route no ClusterProvider has declared is NOT a rejection. func TestAuditHandler_AnnotationRouting(t *testing.T) { - newHandler := func(t *testing.T, recorder *fakeFactRecorder, resolver AuditProviderResolver) *AuditHandler { + newHandler := func(t *testing.T, recorder *fakeFactRecorder) *AuditHandler { t.Helper() handler, err := NewAuditHandler(AuditHandlerConfig{ - FactRecorder: recorder, - ProviderResolver: resolver, - ClusterAnnotationKey: clusterAnnotation, + FactRecorder: recorder, + AuditRouteAnnotationKey: clusterAnnotation, }) require.NoError(t, err) return handler } - known := fakeProviderResolver{existing: map[string]bool{"prod-eu-1": true, "prod-us-1": true}} t.Run("one batch fans out to several source clusters", func(t *testing.T) { recorder := &fakeFactRecorder{} - handler := newHandler(t, recorder, known) + handler := newHandler(t, recorder) w := serveBody(t, handler, http.MethodPost, "/audit-webhook", eventListBody( annotatedEvent("eu", map[string]string{clusterAnnotation: "prod-eu-1"}), @@ -289,11 +254,10 @@ func TestAuditHandler_AnnotationRouting(t *testing.T) { {"no annotation at all", nil}, {"the key present but empty", map[string]string{clusterAnnotation: ""}}, {"a different key", map[string]string{"other.io/cluster": "prod-eu-1"}}, - {"an unknown provider", map[string]string{clusterAnnotation: "never-created"}}, } { t.Run(tt.name, func(t *testing.T) { recorder := &fakeFactRecorder{} - handler := newHandler(t, recorder, known) + handler := newHandler(t, recorder) w := serveBody(t, handler, http.MethodPost, "/audit-webhook", eventListBody( annotatedEvent("rejected", tt.annotations), @@ -308,21 +272,21 @@ func TestAuditHandler_AnnotationRouting(t *testing.T) { } }) - t.Run("a resolver failure retries the whole batch", func(t *testing.T) { + t.Run("a route no ClusterProvider declares is stored like any other", func(t *testing.T) { recorder := &fakeFactRecorder{} - handler := newHandler(t, recorder, fakeProviderResolver{err: assert.AnError}) + handler := newHandler(t, recorder) w := serveBody(t, handler, http.MethodPost, "/audit-webhook", eventListBody( - annotatedEvent("eu", map[string]string{clusterAnnotation: "prod-eu-1"}), + annotatedEvent("eu", map[string]string{clusterAnnotation: "never-declared"}), )) - assert.Equal(t, http.StatusInternalServerError, w.Code, - "a lookup failure is transient, not a verdict that the provider is absent") - assert.Zero(t, recorder.len()) + require.Equal(t, http.StatusOK, w.Code) + assert.Equal(t, []string{"never-declared"}, recorder.recordedProviders(), + "the annotation names a partition, not an object that must already exist") }) t.Run("named routes ignore the annotation", func(t *testing.T) { recorder := &fakeFactRecorder{} - handler := newHandler(t, recorder, known) + handler := newHandler(t, recorder) w := serveBody(t, handler, http.MethodPost, "/audit-webhook/prod-us-1", eventListBody( annotatedEvent("eu", map[string]string{clusterAnnotation: "prod-eu-1"}), @@ -333,26 +297,18 @@ func TestAuditHandler_AnnotationRouting(t *testing.T) { }) } -// TestNewAuditHandler_AnnotationRoutingRequiresResolver pins the startup guard: annotation routing -// must be able to reject an unknown provider, so it cannot be configured without a resolver. -func TestNewAuditHandler_AnnotationRoutingRequiresResolver(t *testing.T) { - _, err := NewAuditHandler(AuditHandlerConfig{ClusterAnnotationKey: clusterAnnotation}) - require.Error(t, err) - assert.Contains(t, err.Error(), "ProviderResolver") -} - -// TestProviderRouteForPath covers the path -> (provider, named) mapping directly. The bare path +// TestAuditRouteForPath covers the path -> (provider, named) mapping directly. The bare path // names no provider at all: it is the shared, annotation-routed endpoint. -func TestProviderRouteForPath(t *testing.T) { - name, named := providerRouteForPath("/audit-webhook") +func TestAuditRouteForPath(t *testing.T) { + name, named := auditRouteForPath("/audit-webhook") assert.Empty(t, name, "the bare path resolves no provider by itself") assert.False(t, named) - name, named = providerRouteForPath("/audit-webhook/prod-eu-1") + name, named = auditRouteForPath("/audit-webhook/prod-eu-1") assert.Equal(t, "prod-eu-1", name) assert.True(t, named, "a segment names a provider") - name, named = providerRouteForPath(defaultRoute) + name, named = auditRouteForPath(defaultRoute) assert.Equal(t, "default", name, "default is an ordinary named route") assert.True(t, named) } @@ -390,7 +346,7 @@ func TestAuditHandler_MethodAndPathValidation(t *testing.T) { // none, so a producer posting there is misconfigured rather than routed to a default. {"bare endpoint rejected without an annotation key", http.MethodPost, "/audit-webhook", http.StatusBadRequest}, // A name with no ClusterProvider behind it is 404 — the gate applies to every name. - {"unknown named route is 404", http.MethodPost, "/audit-webhook/prod-eu-1", http.StatusNotFound}, + {"any named route is served", http.MethodPost, "/audit-webhook/prod-eu-1", http.StatusOK}, } for _, tt := range tests { diff --git a/internal/webhook/audit_metrics_test.go b/internal/webhook/audit_metrics_test.go index 5e7ba52e..9f8abf62 100644 --- a/internal/webhook/audit_metrics_test.go +++ b/internal/webhook/audit_metrics_test.go @@ -149,7 +149,7 @@ func TestServeHTTP_UnroutableEventOutcomes(t *testing.T) { wantOutcome string }{ {"unstamped event", nil, "missing_cluster_annotation"}, - {"unknown provider", map[string]string{clusterAnnotation: "never-created"}, "unknown_cluster_provider"}, + {"the key present but empty", map[string]string{clusterAnnotation: ""}, "missing_cluster_annotation"}, } for _, tt := range tests { @@ -159,9 +159,8 @@ func TestServeHTTP_UnroutableEventOutcomes(t *testing.T) { recorder := &fakeFactRecorder{} handler, err := NewAuditHandler(AuditHandlerConfig{ - FactRecorder: recorder, - ProviderResolver: fakeProviderResolver{existing: map[string]bool{"prod-eu-1": true}}, - ClusterAnnotationKey: clusterAnnotation, + FactRecorder: recorder, + AuditRouteAnnotationKey: clusterAnnotation, }) require.NoError(t, err) diff --git a/test/e2e/audit_route_attribution_e2e_test.go b/test/e2e/audit_route_attribution_e2e_test.go new file mode 100644 index 00000000..0dca4676 --- /dev/null +++ b/test/e2e/audit_route_attribution_e2e_test.go @@ -0,0 +1,187 @@ +// SPDX-License-Identifier: Apache-2.0 + +package e2e + +import ( + "fmt" + "os" + "path" + "path/filepath" + "strings" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +// This spec pins the bug the gitops-api team reported: every object mirrored through a DEDICATED +// in-cluster ClusterProvider committed as "unknown (attribution unresolved)", while the same actor's +// writes through "default" attributed correctly. +// +// The cause is structural rather than stochastic. A kube-apiserver takes one +// --audit-webhook-config-file naming one server URL, so it posts audit under exactly ONE route (here +// /audit-webhook/default). Attribution facts are partitioned by that route, so a second +// ClusterProvider on the same cluster reads a partition nothing writes unless it declares the same +// route via spec.attribution.auditRoute. +// +// Nothing else in the suite catches this. commit_author_attribution_e2e_test.go proves the OIDC +// claim chain, but only through the default provider, which is the one route the apiserver feeds. +// source_namespace_e2e_test.go creates dedicated in-cluster providers but asserts Git paths, not +// commit authors. The gap between them is exactly where the loss lived. +// +// See docs/design/attribution-fact-identity.md. +var _ = Describe("Audit route attribution", Label("manager"), Ordered, func() { + const ( + // The apiserver's webhook-config.yaml posts to /audit-webhook/default, so this is the only + // route that carries facts on this cluster (test/e2e/cluster/audit/webhook-config.yaml). + fedRoute = "default" + basePath = "e2e/audit-route-attribution" + claimName = "configbutler.ai/claims/display-name" + claimMail = "configbutler.ai/claims/email" + ) + + var ( + testNs string + sourceNs string + repo *RepoArtifacts + gitProvName string + gitTargetName string + watchRuleName string + clusterProv string + ) + + BeforeAll(func() { + if configuredAuthorModeEnabled() { + Skip("watch-first configured-author mode has no audit facts for author attribution") + } + + seed := GinkgoRandomSeed() + testNs = testNamespaceFor("audit-route") + sourceNs = testNamespaceFor("audit-route-src") + _, _ = kubectlRun("create", "namespace", testNs) + _, _ = kubectlRun("create", "namespace", sourceNs) + + By("setting up the Gitea repo and credentials") + repo = SetupRepo(resolveE2EContext(), testNs, fmt.Sprintf("e2e-audit-route-%d", seed)) + _, err := kubectlRunInNamespace(testNs, "apply", "-f", repo.SecretsYAML) + Expect(err).NotTo(HaveOccurred(), "failed to apply git secrets") + applySOPSAgeKeyToNamespace(testNs) + + gitProvName = fmt.Sprintf("audit-route-gitprovider-%d", seed) + gitTargetName = fmt.Sprintf("audit-route-gittarget-%d", seed) + watchRuleName = fmt.Sprintf("audit-route-watchrule-%d", seed) + clusterProv = fmt.Sprintf("audit-route-cp-%d", seed) + + By("declaring a DEDICATED in-cluster ClusterProvider that declares the fed audit route") + // kubeConfig is omitted, so this names the operator's own cluster, exactly as the reported + // srcns-delegating provider did. Its NAME is not the route the apiserver posts to, which is + // the whole point: without spec.attribution.auditRoute it would read a partition nothing + // writes. It also delegates source-namespace selection, so the second case below can watch + // a namespace other than the rule's own, reproducing the reported shape exactly. + Expect(applyInClusterClusterProviderWithAuditRoute( + clusterProv, testNs, sourceNs, true, fedRoute)).Error(). + NotTo(HaveOccurred(), "failed to apply the dedicated ClusterProvider") + + // A 0s commit window makes every watched event its own commit, so each assertion reads an + // author scoped to its own file path and concurrent audit traffic cannot change it. + createReadyGitProvider(gitProvName, testNs, repo.GitSecretHTTP, repo.RepoURLHTTP) + + By("creating a GitTarget that mirrors through the dedicated provider") + Expect(applyGitTargetWithSourceNamespaces( + testNs, gitTargetName, gitProvName, basePath, clusterProv, testNs, sourceNs)).Error(). + NotTo(HaveOccurred(), "failed to apply the GitTarget") + verifyResourceCondition("gittarget", gitTargetName, testNs, "Validated", "True", "OK", "") + }) + + AfterAll(func() { + deleteClusterProvider(clusterProv) + cleanupPipeline(testNs, gitProvName, gitTargetName, watchRuleName) + cleanupNamespace(testNs) + cleanupNamespace(sourceNs) + }) + + SetDefaultEventuallyTimeout(3 * time.Minute) + SetDefaultEventuallyPollingInterval(3 * time.Second) + + // assertCommitAuthor creates a ConfigMap in ns while impersonating an identity carrying OIDC + // claims, then asserts the commit for that object's own path is authored by it. On main before + // this change the author is "unknown (attribution unresolved)", because the read went to the + // provider's name rather than to the route the apiserver posts under. + assertCommitAuthor := func(ns, cmName, asUser, displayName, email string) { + GinkgoHelper() + + By(fmt.Sprintf("creating ConfigMap %q in %q as %q", cmName, ns, asUser)) + err := createConfigMapAsImpersonatedUser(ns, cmName, asUser, + []string{"system:masters"}, + map[string][]string{claimName: {displayName}, claimMail: {email}}, + ) + Expect(err).NotTo(HaveOccurred(), "failed to create the impersonated ConfigMap") + + repoPath := path.Join(basePath, fmt.Sprintf("%s/configmaps/%s.yaml", ns, cmName)) + Eventually(func(g Gomega) { + pullLatestRepoState(g, repo.CheckoutDir) + + info, statErr := os.Stat(filepath.Join(repo.CheckoutDir, repoPath)) + g.Expect(statErr).NotTo(HaveOccurred(), "mirrored file should exist at %s", repoPath) + g.Expect(info.Size()).To(BeNumerically(">", 0)) + + out, logErr := gitRun(repo.CheckoutDir, "log", "-1", "--pretty=%an <%ae>", "--", repoPath) + g.Expect(logErr).NotTo(HaveOccurred(), "git log author failed: %s", out) + g.Expect(strings.TrimSpace(out)).To(Equal(fmt.Sprintf("%s <%s>", displayName, email)), + "a commit through a dedicated ClusterProvider must carry the real actor, not the "+ + "unresolved-attribution placeholder") + }).Should(Succeed()) + + _, _ = kubectlRunInNamespace(ns, "delete", "configmap", cmName, "--ignore-not-found=true") + } + + It("attributes a commit mirrored through a dedicated in-cluster ClusterProvider", func() { + By("creating a WatchRule in the GitTarget's own namespace") + data := struct{ Name, Namespace, DestinationName string }{ + Name: watchRuleName, Namespace: testNs, DestinationName: gitTargetName, + } + Expect(applyFromTemplate("test/e2e/templates/manager/watchrule-configmap.tmpl", data, testNs)). + To(Succeed(), "failed to apply the WatchRule") + verifyResourceStatus("watchrule", watchRuleName, testNs, "True", "Ready", "") + waitForStreamsRunning(gitTargetName, testNs) + + assertCommitAuthor(testNs, fmt.Sprintf("audit-route-cm-%d", GinkgoRandomSeed()), + "oidc-route-user", "Route User", "route-user@configbutler.ai") + }) + + It("attributes a commit reached through a rules[].sourceNamespace override", func() { + // The reported objects were mirrored through BOTH a dedicated provider and a sourceNamespace + // override, and the report argued the override was innocent. This asserts that directly: the + // override changes which namespace is watched and nothing about attribution. + overrideRule := watchRuleName + "-override" + Expect(applyWatchRuleWithSourceNamespace(overrideRule, testNs, gitTargetName, sourceNs)).Error(). + NotTo(HaveOccurred(), "failed to apply the overriding WatchRule") + verifyResourceCondition("watchrule", overrideRule, testNs, + "SourceNamespaceAuthorized", "True", "SourceNamespaceAllowed", "") + waitForStreamsRunning(gitTargetName, testNs) + DeferCleanup(func() { cleanupWatchRule(overrideRule, testNs) }) + + assertCommitAuthor(sourceNs, fmt.Sprintf("audit-route-src-cm-%d", GinkgoRandomSeed()), + "oidc-override-user", "Override User", "override-user@configbutler.ai") + }) +}) + +// applyInClusterClusterProviderWithAuditRoute applies a ClusterProvider that OMITS kubeConfig (so it +// names the operator's own cluster) and declares the audit route its facts arrive on. The route is +// what lets several providers on one cluster share the single stream that cluster's apiserver posts. +func applyInClusterClusterProviderWithAuditRoute( + name, allowedNS, extraNS string, delegate bool, auditRoute string, +) (string, error) { + manifest := fmt.Sprintf(`apiVersion: configbutler.ai/v1alpha3 +kind: ClusterProvider +metadata: + name: %s +spec: + allowedNamespaces: + names: [%s, %s] + allowSourceNamespaceOverride: %t + attribution: + auditRoute: %s +`, name, allowedNS, extraNS, delegate, auditRoute) + return kubectlRunWithStdin("", manifest, "apply", "-f", "-") +} diff --git a/test/e2e/cluster/audit/webhook-config.yaml b/test/e2e/cluster/audit/webhook-config.yaml index e06e0998..e89c2d8d 100644 --- a/test/e2e/cluster/audit/webhook-config.yaml +++ b/test/e2e/cluster/audit/webhook-config.yaml @@ -11,7 +11,7 @@ clusters: cluster: # Audit routes are NAMED: this cluster's source identity is the "default" ClusterProvider the # install creates. The bare /audit-webhook is the shared, annotation-routed endpoint and is - # rejected with 400 unless --author-attribution-cluster-annotation-key is set. + # rejected with 400 unless --author-attribution-audit-route-annotation-key is set. server: https://127.0.0.1:30444/audit-webhook/default insecure-skip-tls-verify: true contexts: