diff --git a/config/ha/README.md b/config/ha/README.md index eb067ac2..064d8465 100644 --- a/config/ha/README.md +++ b/config/ha/README.md @@ -3,10 +3,11 @@ `active-cluster-clusterrole.yaml` is a least-privilege grant for the identity behind a Standby's `--ha-active-kubeconfig` flag: read-only (`get`/`list`/`watch`) access to `Namespace` plus every resource type in -`pkg/ha.CRDMirrorSet` (`RemoteSyncer` never writes to the Active cluster — -only reads), plus the Active's own `coordination.k8s.io/v1` `Lease` — -the same kubeconfig is also used by #294's `WatchRemoteLease` to read the -Active's Lease directly, not just by `RemoteSyncer` to mirror CRDs. +`pkg/ha.CRDMirrorSet` and `pkg/ha.CredentialMirrorSet` (`RemoteSyncer` +never writes to the Active cluster — only reads), plus the Active's own +`coordination.k8s.io/v1` `Lease` — the same kubeconfig is also used by +#294's `WatchRemoteLease` to read the Active's Lease directly, not just by +`RemoteSyncer` to mirror resources. ## This is not applied by this repo's own deploy flow @@ -36,13 +37,23 @@ Nothing in this repo automates applying this to a real Active cluster — that's cross-cluster provisioning, out of scope for a single controller repo. Logged as a follow-up, not built. -## Credential mirroring (a later PR) - -If/when `pkg/ha.CredentialMirrorSet` (Secrets, ServiceAccounts, Roles, -RoleBindings — for #297's post-promotion use) is wired in, this -`ClusterRole` will need `secrets`/`serviceaccounts`/`roles`/`rolebindings` -appended. Worth knowing ahead of time: RBAC cannot scope `Secret` access by -`.type`, so that addition grants read access to **every** Secret in the -project namespaces on the Active cluster, not just the credential ones -`RemoteSyncer` actually mirrors — a real credential-exposure tradeoff to -weigh when that lands, not just an implementation detail. +## Credential mirroring and the Secret-read tradeoff + +`pkg/ha.CredentialMirrorSet` (Secrets with the SA-token type filtered out, +ServiceAccounts, Roles, RoleBindings — for #297's post-promotion use) is +wired in, and this `ClusterRole` grants the reads it needs. Weigh the +Secret rule before applying it: RBAC cannot scope `Secret` access by +`.type` or by namespace *label*, and a `ClusterRole` + +`ClusterRoleBinding` is cluster-wide — so the Standby's identity can read +**every** Secret on the Active hub, not just the gateway-certificate +Secrets `RemoteSyncer` actually mirrors. The syncer itself only *copies* +credential objects whose namespace it also mirrors (the label-scoped +project-namespace boundary — notably excluding the controller's own +namespace, whose name can match the project-namespace prefix) and +excludes SA-token Secrets from the watch entirely, but none of that +narrows what the identity *could* read if the kubeconfig leaked — +protect it like the credential it is. The narrower alternative — per-namespace `RoleBinding`s in each +project namespace instead of the cluster-wide binding — works with the +same `ClusterRole`, at the cost of maintaining those bindings as projects +come and go (cross-cluster provisioning tooling this repo deliberately +does not ship). diff --git a/config/ha/active-cluster-clusterrole.yaml b/config/ha/active-cluster-clusterrole.yaml index e41b86a1..3f208a10 100644 --- a/config/ha/active-cluster-clusterrole.yaml +++ b/config/ha/active-cluster-clusterrole.yaml @@ -9,12 +9,12 @@ # See config/ha/README.md for how and where to apply this. # # Scope: read-only (get/list/watch) on Namespace plus every type in -# pkg/ha.CRDMirrorSet, plus the Active's own HA Lease — the same -# --ha-active-kubeconfig identity is also used by #294's WatchRemoteLease to -# read the Active's coordination.k8s.io/v1 Lease (checkRemoteLeaseOnce), -# not just by RemoteSyncer. Confirmed live: without this, the Standby can -# mirror CRDs fine but permanently fails to read the Active's Lease, -# so it can never observe staleness in the first place. +# pkg/ha.CRDMirrorSet and pkg/ha.CredentialMirrorSet, plus the Active's own +# HA Lease — the same --ha-active-kubeconfig identity is also used by #294's +# WatchRemoteLease to read the Active's coordination.k8s.io/v1 Lease +# (checkRemoteLeaseOnce), not just by RemoteSyncer. Confirmed live: without +# this, the Standby can mirror CRDs fine but permanently fails to read the +# Active's Lease, so it can never observe staleness in the first place. apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: @@ -59,6 +59,36 @@ rules: - get - list - watch + # Credential mirroring (pkg/ha.CredentialMirrorSet). SECURITY TRADEOFF, + # named rather than buried: RBAC cannot scope Secret access by .type or by + # namespace *label*, and a ClusterRole+ClusterRoleBinding grants reads + # CLUSTER-WIDE — so this rule lets the Standby's identity read every + # Secret on the Active hub, not just the gateway certificates RemoteSyncer + # actually mirrors (the syncer's own field selector and mirrored-namespace + # gate narrow what is COPIED, but not what this identity COULD read). + # Treat the --ha-active-kubeconfig credential accordingly. To narrow the + # grant itself, replace this rule with per-namespace RoleBindings in each + # kubeslice project namespace — at the cost of maintaining them as + # projects come and go, which is cross-cluster provisioning tooling this + # repo deliberately does not ship. + - apiGroups: + - "" + resources: + - secrets + - serviceaccounts + verbs: + - get + - list + - watch + - apiGroups: + - rbac.authorization.k8s.io + resources: + - roles + - rolebindings + verbs: + - get + - list + - watch --- # Template only — the subject is deployment-specific (a ServiceAccount if the # Standby dials the Active in-cluster, a cert CN if it uses client-cert auth diff --git a/main.go b/main.go index 23849691..50d6f240 100644 --- a/main.go +++ b/main.go @@ -353,6 +353,7 @@ func initialize(services *service.Services) { // the same remote config and local client the elector above already // built rather than loading the kubeconfig twice. remoteSyncer, err := ha.NewRemoteSyncer(localHAClient, remoteHACfg, scheme, haRunMode, ha.RemoteSyncerOptions{ + Resources: ha.FullMirrorSet(), Workers: haSyncWorkers, PruneInterval: haSyncInterval, EventRecorder: eventRecorder, diff --git a/pkg/ha/credential_set_test.go b/pkg/ha/credential_set_test.go new file mode 100644 index 00000000..a888d9fb --- /dev/null +++ b/pkg/ha/credential_set_test.go @@ -0,0 +1,253 @@ +/* + * Copyright (c) 2022 Avesha, Inc. All rights reserved. # # SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package ha + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/fields" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/util/workqueue" + "sigs.k8s.io/controller-runtime/pkg/cache" + + "github.com/kubeslice/kubeslice-controller/util" +) + +var ( + nsGVK = schema.GroupVersionKind{Version: "v1", Kind: "Namespace"} + secretGVK = schema.GroupVersionKind{Version: "v1", Kind: "Secret"} + saGVK = schema.GroupVersionKind{Version: "v1", Kind: "ServiceAccount"} + roleGVK = schema.GroupVersionKind{Group: groupRBAC, Version: "v1", Kind: "Role"} + rbGVK = schema.GroupVersionKind{Group: groupRBAC, Version: "v1", Kind: "RoleBinding"} +) + +// buildCredentialSyncer is buildSyncer's sibling for the credential set. +func buildCredentialSyncer(t *testing.T, remote *stubRemote) *RemoteSyncer { + t.Helper() + byGVK := make(map[schema.GroupVersionKind]MirroredResource, len(CredentialMirrorSet)) + for _, res := range CredentialMirrorSet { + byGVK[res.GVK] = res + } + return &RemoteSyncer{ + mode: ModeStandby, + localClient: fakeClient(t), + remoteGet: remote.get, + resources: CredentialMirrorSet, + byGVK: byGVK, + workers: 1, + queue: workqueue.NewTypedRateLimitingQueue[syncKey]( + workqueue.NewTypedItemExponentialFailureRateLimiter[syncKey](time.Millisecond, time.Second), + ), + handlerRegistered: map[schema.GroupVersionKind]bool{}, + enqueuedAt: map[syncKey]time.Time{}, + log: testLog(), + } +} + +// registerMirroredNamespace makes ns visible in the stub's Namespace view, +// standing in for a project namespace the label-scoped remote cache mirrors. +func registerMirroredNamespace(remote *stubRemote, ns string) { + key := syncKey{GVK: nsGVK, Name: ns} + remote.objects[key] = newTestUnstructured(nsGVK, "", ns) +} + +func TestCredentialMirrorSet_ShapeAndDefenses(t *testing.T) { + var gvks []schema.GroupVersionKind + for _, res := range CredentialMirrorSet { + gvks = append(gvks, res.GVK) + assert.True(t, res.StripOwnerRefs, + "%s: UID-based ownerReferences never survive a cross-cluster copy, and credential objects are written by actors outside this repo — every row must strip them", res.GVK.Kind) + assert.True(t, res.RequireMirroredNamespace, + "%s: core types exist cluster-wide; every row must be gated on the mirrored-namespace boundary", res.GVK.Kind) + } + assert.ElementsMatch(t, []schema.GroupVersionKind{secretGVK, saGVK, roleGVK, rbGVK}, gvks, + "credential set is Secret/ServiceAccount/Role/RoleBinding only — access_control_service never creates ClusterRole/ClusterRoleBinding, despite the ADR's broader wording") +} + +func TestSkipServiceAccountTokenSecret(t *testing.T) { + saToken := newTestUnstructured(secretGVK, "proj", "sa-token") + require.NoError(t, unstructured.SetNestedField(saToken.Object, string(corev1.SecretTypeServiceAccountToken), "type")) + assert.True(t, skipServiceAccountTokenSecret(saToken), + "SA tokens are signed by the issuing cluster's key and are invalid on the Standby") + + opaque := newTestUnstructured(secretGVK, "proj", "gateway-cert") + require.NoError(t, unstructured.SetNestedField(opaque.Object, string(corev1.SecretTypeOpaque), "type")) + assert.False(t, skipServiceAccountTokenSecret(opaque)) + + untyped := newTestUnstructured(secretGVK, "proj", "no-type-field") + assert.False(t, skipServiceAccountTokenSecret(untyped)) +} + +func TestFullMirrorSet_CombinesBothSetsWithoutCollisions(t *testing.T) { + full := FullMirrorSet() + assert.Len(t, full, len(CRDMirrorSet)+len(CredentialMirrorSet)) + + seen := map[schema.GroupVersionKind]bool{} + for _, res := range full { + assert.False(t, seen[res.GVK], "duplicate mirror row for %s — byGVK keying would silently drop one", res.GVK) + seen[res.GVK] = true + } + assert.True(t, seen[nsGVK]) + assert.True(t, seen[secretGVK]) +} + +func TestReconcileKey_MirrorsOpaqueSecretWithData(t *testing.T) { + ctx := context.Background() + key := syncKey{GVK: secretGVK, Namespace: "kubeslice-avesha", Name: "gateway-cert"} + + src := newTestUnstructured(secretGVK, key.Namespace, key.Name) + require.NoError(t, unstructured.SetNestedField(src.Object, string(corev1.SecretTypeOpaque), "type")) + require.NoError(t, unstructured.SetNestedField(src.Object, "Y2VydC1kYXRh", "data", "ovpn.crt")) + + remote := newStubRemote() + remote.objects[key] = src + registerMirroredNamespace(remote, key.Namespace) + s := buildCredentialSyncer(t, remote) + + op, _, err := s.reconcileKey(ctx, key) + require.NoError(t, err) + assert.Equal(t, opCreate, op) + + got := getUnstructured(t, s.localClient, key) + assert.Equal(t, LabelValueActive, got.GetLabels()[LabelSyncedFromActive]) + data, found, err := unstructured.NestedString(got.Object, "data", "ovpn.crt") + require.NoError(t, err) + require.True(t, found) + assert.Equal(t, "Y2VydC1kYXRh", data, "the mirrored Secret must carry the source's data through unchanged") +} + +func TestReconcileKey_SkipsServiceAccountTokenSecret(t *testing.T) { + ctx := context.Background() + key := syncKey{GVK: secretGVK, Namespace: "kubeslice-avesha", Name: "kubeslice-rbac-worker-w1"} + + src := newTestUnstructured(secretGVK, key.Namespace, key.Name) + require.NoError(t, unstructured.SetNestedField(src.Object, string(corev1.SecretTypeServiceAccountToken), "type")) + + remote := newStubRemote() + remote.objects[key] = src + registerMirroredNamespace(remote, key.Namespace) + s := buildCredentialSyncer(t, remote) + + op, _, err := s.reconcileKey(ctx, key) + require.NoError(t, err) + assert.Equal(t, mirrorOp(""), op) + + existing := &unstructured.Unstructured{} + existing.SetGroupVersionKind(secretGVK) + err = s.localClient.Get(ctx, types.NamespacedName{Namespace: key.Namespace, Name: key.Name}, existing) + assert.Error(t, err, "an SA-token Secret must never be written onto the Standby") +} + +// TestReconcileKey_SkipsCredentialsInUnmirroredNamespaces pins the boundary +// that matters most: a namespace that is not label-mirrored is out of bounds +// no matter what it is named. The concrete case that motivated this (found +// live against a Helm-installed Active hub): under the chart's +// --project-namespace-prefix ("kubeslice-"), the controller's own +// kubeslice-controller namespace looks like a project namespace by name, and +// a name-based rule would have mirrored its webhook TLS key and image-pull +// Secrets onto the Standby. +func TestReconcileKey_SkipsCredentialsInUnmirroredNamespaces(t *testing.T) { + ctx := context.Background() + for _, tc := range []struct { + gvk schema.GroupVersionKind + ns string + name string + }{ + {secretGVK, "kubeslice-controller", "webhook-server-cert-secret"}, + {secretGVK, "kube-system", "bootstrap-token"}, + {saGVK, "kube-system", "hand-labeled-sa"}, + {roleGVK, "default", "some-role"}, + {rbGVK, "default", "some-rolebinding"}, + } { + key := syncKey{GVK: tc.gvk, Namespace: tc.ns, Name: tc.name} + src := newTestUnstructured(tc.gvk, tc.ns, tc.name) + if tc.gvk == secretGVK { + require.NoError(t, unstructured.SetNestedField(src.Object, string(corev1.SecretTypeOpaque), "type")) + } + + remote := newStubRemote() + remote.objects[key] = src // object visible, namespace deliberately NOT mirrored + s := buildCredentialSyncer(t, remote) + + op, _, err := s.reconcileKey(ctx, key) + require.NoError(t, err) + assert.Equal(t, mirrorOp(""), op, "%s %s/%s: unmirrored namespace must mean skip", tc.gvk.Kind, tc.ns, tc.name) + + existing := &unstructured.Unstructured{} + existing.SetGroupVersionKind(tc.gvk) + err = s.localClient.Get(ctx, types.NamespacedName{Namespace: tc.ns, Name: tc.name}, existing) + assert.Error(t, err, "%s %s/%s must not exist on the Standby", tc.gvk.Kind, tc.ns, tc.name) + } +} + +func TestReconcileKey_NamespaceCheckErrorIsRetryable(t *testing.T) { + ctx := context.Background() + key := syncKey{GVK: secretGVK, Namespace: "kubeslice-avesha", Name: "gateway-cert"} + + src := newTestUnstructured(secretGVK, key.Namespace, key.Name) + require.NoError(t, unstructured.SetNestedField(src.Object, string(corev1.SecretTypeOpaque), "type")) + + remote := newStubRemote() + remote.objects[key] = src + remote.errs[syncKey{GVK: nsGVK, Name: key.Namespace}] = fmt.Errorf("simulated transient cache failure") + s := buildCredentialSyncer(t, remote) + + _, _, err := s.reconcileKey(ctx, key) + assert.Error(t, err, + "a transient failure reading the namespace must surface as an error (workqueue retry), not as a silent skip") +} + +func TestMirrorCacheByObject_ScopesCredentialInformers(t *testing.T) { + byObject := mirrorCacheByObject() + + // The label-scoped types must match exactly what the controller stamps + // (via ReconcileProjectNamespace and util.GetOwnerLabel) and nothing else. + labeled := labels.Set(util.LabelsKubeSliceController) + for obj, cfg := range byObject { + if _, isSecret := obj.(*corev1.Secret); isSecret { + continue + } + require.NotNil(t, cfg.Label, "%T informer must be label-scoped", obj) + assert.True(t, cfg.Label.Matches(labeled), "%T: selector must match controller-stamped labels", obj) + assert.False(t, cfg.Label.Matches(labels.Set{}), "%T: selector must not match unlabeled objects", obj) + } + + // Secret can't be label-scoped (cert Secrets come from the external + // cert-generator job, unlabeled) — it must be field-scoped to exclude + // SA-token Secrets at the watch itself. + var secretCfg cache.ByObject + ok := false + for obj, cfg := range byObject { + if _, isSecret := obj.(*corev1.Secret); isSecret { + secretCfg, ok = cfg, true + } + } + require.True(t, ok, "Secret informer must have a ByObject entry") + require.NotNil(t, secretCfg.Field) + assert.False(t, secretCfg.Field.Matches(fields.Set{"type": string(corev1.SecretTypeServiceAccountToken)}), + "the Secret watch itself must exclude SA-token Secrets") + assert.True(t, secretCfg.Field.Matches(fields.Set{"type": string(corev1.SecretTypeOpaque)})) +} diff --git a/pkg/ha/mirror_set.go b/pkg/ha/mirror_set.go index 6cffa254..337a23b2 100644 --- a/pkg/ha/mirror_set.go +++ b/pkg/ha/mirror_set.go @@ -17,6 +17,7 @@ package ha import ( + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" ) @@ -49,11 +50,24 @@ type MirroredResource struct { // e.g. filtering out kubernetes.io/service-account-token Secrets in the // credential mirror set. Skip func(u *unstructured.Unstructured) bool + // RequireMirroredNamespace restricts mirroring to objects whose namespace + // the syncer itself mirrors — i.e. the namespace is present in the remote + // cache's label-scoped Namespace view (see namespaceMirrorSelector). Set + // on every credential row: core types exist in every namespace, and no + // name-based rule can draw this boundary safely — under the Helm chart's + // real-world --project-namespace-prefix ("kubeslice-"), the controller's + // own kubeslice-controller namespace matches the project-namespace naming + // pattern, and a prefix test would have mirrored its webhook TLS key, + // image-pull credentials, and Helm release Secrets onto the Standby + // (found live against a Helm-installed Active hub). The label boundary is + // the one ReconcileProjectNamespace actually maintains. + RequireMirroredNamespace bool } const ( groupController = "controller.kubeslice.io" groupWorker = "worker.kubeslice.io" + groupRBAC = "rbac.authorization.k8s.io" ) func gvk(group, kind string) schema.GroupVersionKind { @@ -79,3 +93,60 @@ var CRDMirrorSet = []MirroredResource{ {GVK: gvk(groupWorker, "WorkerSliceGateway")}, {GVK: gvk(groupWorker, "WorkerServiceImport")}, } + +// skipServiceAccountTokenSecret excludes kubernetes.io/service-account-token +// Secrets from mirroring. SA tokens are signed by the issuing cluster's own +// service-account key, so an Active-minted token is cryptographically invalid +// on the Standby; mirroring the ServiceAccount itself is what matters — the +// Standby's token controller mints its own, locally-valid token for it. +func skipServiceAccountTokenSecret(u *unstructured.Unstructured) bool { + secretType, _, _ := unstructured.NestedString(u.Object, "type") + return secretType == string(corev1.SecretTypeServiceAccountToken) +} + +// FullMirrorSet is everything a production Standby mirrors: CRDMirrorSet +// plus CredentialMirrorSet. Returned as a fresh slice so callers cannot +// mutate the package-level tables through it. +func FullMirrorSet() []MirroredResource { + full := make([]MirroredResource, 0, len(CRDMirrorSet)+len(CredentialMirrorSet)) + full = append(full, CRDMirrorSet...) + return append(full, CredentialMirrorSet...) +} + +// CredentialMirrorSet is the set of credential resources mirrored Active -> +// Standby so a promoted Standby can serve its worker clusters without manual +// re-provisioning: worker-identity RBAC (Role/RoleBinding/ServiceAccount, the +// only RBAC kinds access_control_service.go ever creates — no ClusterRole or +// ClusterRoleBinding, despite ADR Decision 6's broader wording) and Secrets +// such as the gateway certificates the ovpn job generates. +// +// Every row sets RequireMirroredNamespace — see that field's doc comment for +// why the boundary is the mirrored-namespace set and not a name pattern — +// and StripOwnerRefs: ownerReferences resolve by UID, which never survives +// the cross-cluster copy, and unlike the CRD set (audited — only +// VpnKeyRotation ever gets a reference, from this repo's own code) +// credential objects are also written by actors outside this repo (the +// token controller, the cert-generator job), so no such audit can hold here. +var CredentialMirrorSet = []MirroredResource{ + { + GVK: schema.GroupVersionKind{Version: "v1", Kind: "Secret"}, + StripOwnerRefs: true, + Skip: skipServiceAccountTokenSecret, + RequireMirroredNamespace: true, + }, + { + GVK: schema.GroupVersionKind{Version: "v1", Kind: "ServiceAccount"}, + StripOwnerRefs: true, + RequireMirroredNamespace: true, + }, + { + GVK: schema.GroupVersionKind{Group: groupRBAC, Version: "v1", Kind: "Role"}, + StripOwnerRefs: true, + RequireMirroredNamespace: true, + }, + { + GVK: schema.GroupVersionKind{Group: groupRBAC, Version: "v1", Kind: "RoleBinding"}, + StripOwnerRefs: true, + RequireMirroredNamespace: true, + }, +} diff --git a/pkg/ha/prune.go b/pkg/ha/prune.go index 5b02ace6..7774b7f3 100644 --- a/pkg/ha/prune.go +++ b/pkg/ha/prune.go @@ -41,13 +41,15 @@ const opPrune mirrorOp = "prune" // call. Overridable in tests, the same seam pattern as remoteGetFunc. type remoteListFunc func(ctx context.Context, gvk schema.GroupVersionKind) (map[syncKey]struct{}, error) -// runPrune periodically removes Standby-side drift the informers never -// reported: a mirrored object whose Active-side original was deleted while -// this process wasn't watching (e.g. between two Standby runs) never gets a -// Delete event — cold-start informers only deliver what currently exists — -// so its mirror would otherwise survive as an orphan forever. The workqueue -// owns retry-on-transient-failure and the informers' periodic resync -// self-heals missed updates; pruning orphans is this loop's only job. +// runPrune periodically reconciles drift between the Standby's mirrors and +// the Active hub that the informers never reported. Forward direction: a +// mirrored object whose Active-side original was deleted while this process +// wasn't watching (e.g. between two Standby runs) never gets a Delete event +// — cold-start informers only deliver what currently exists — so its mirror +// would otherwise survive as an orphan forever. Reverse direction: an +// Active-side object with no Standby mirror is re-enqueued (see pruneOnce +// for the cases that produces). The workqueue still owns +// retry-on-transient-failure; this loop only feeds it. // // It blocks until the remote cache has synced before the first pass: an // unsynced cache lists empty, and an empty "Active" view would read as @@ -99,9 +101,11 @@ func (s *RemoteSyncer) pruneOnce(ctx context.Context) { continue } + localKeys := make(map[syncKey]struct{}, len(local.Items)) for i := range local.Items { item := &local.Items[i] key := syncKey{GVK: res.GVK, Namespace: item.GetNamespace(), Name: item.GetName()} + localKeys[key] = struct{}{} if _, onActive := activeKeys[key]; onActive { continue } @@ -109,6 +113,20 @@ func (s *RemoteSyncer) pruneOnce(ctx context.Context) { "kind", key.GVK.Kind, "namespace", key.Namespace, "name", key.Name) s.enqueue(key) } + + // Reverse diff: an Active-side object with no mirror on the Standby. + // Usually a create this loop's forward pass can't see — a mirror + // someone deleted directly on the Standby, an object whose skip + // verdict was decided before its namespace had synced (see + // namespaceIsMirrored), or a key stuck deep in retry backoff. + // Re-enqueueing is always safe: the worker re-reads Active and runs + // the full Skip/namespace/conflict-guard chain, so objects that + // should not mirror simply no-op again. + for key := range activeKeys { + if _, mirrored := localKeys[key]; !mirrored { + s.enqueue(key) + } + } } } diff --git a/pkg/ha/prune_test.go b/pkg/ha/prune_test.go index 11a2c190..2ec6e088 100644 --- a/pkg/ha/prune_test.go +++ b/pkg/ha/prune_test.go @@ -119,6 +119,49 @@ func TestPruneOnce_SkipsKindWhenRemoteListFails(t *testing.T) { assert.Equal(t, 0, s.queue.Len(), "a failed list must not be read as \"everything was deleted on Active\"") } +func TestPruneOnce_ReverseDiffEnqueuesActiveObjectsMissingLocally(t *testing.T) { + ctx := context.Background() + missing := syncKey{GVK: testGVK, Namespace: "proj-a", Name: "sc-missing"} + + // Active has an object the Standby has no mirror of — a create the + // forward (orphan) pass can't see: a mirror deleted directly on the + // Standby, a skip decided before the namespace informer synced, or a key + // stuck deep in retry backoff. + s := buildSyncer(t, newStubRemote()) + s.remoteList = stubRemoteList([]syncKey{missing}, nil) + + s.pruneOnce(ctx) + + require.Equal(t, 1, s.queue.Len()) + got, _ := s.queue.Get() + assert.Equal(t, missing, got) +} + +func TestPruneOnce_ReverseDiffCannotOverrideConflictGuard(t *testing.T) { + ctx := context.Background() + key := syncKey{GVK: testGVK, Namespace: "proj-a", Name: "hand-created"} + + // The object exists on both sides, but the Standby's copy is not + // syncer-owned (no sync label) — so it is absent from the forward pass's + // labeled listing and the reverse diff re-enqueues it every round. That + // must stay harmless: the worker's conflict guard refuses the write. + remote := newStubRemote() + remote.objects[key] = newTestUnstructured(testGVK, key.Namespace, key.Name) + s := buildSyncer(t, remote) + require.NoError(t, s.localClient.Create(ctx, newTestUnstructured(testGVK, key.Namespace, key.Name))) + s.remoteList = stubRemoteList([]syncKey{key}, nil) + + s.pruneOnce(ctx) + k, shutdown := s.queue.Get() + require.False(t, shutdown) + require.Equal(t, key, k) + s.processOnce(ctx, k) + + got := getUnstructured(t, s.localClient, key) + assert.NotEqual(t, LabelValueActive, got.GetLabels()[LabelSyncedFromActive], + "a hand-created Standby object must never be adopted by the mirror, even via the prune loop") +} + func TestRunPrune_DoesNotPruneBeforeCacheSync(t *testing.T) { s := buildSyncer(t, newStubRemote()) s.pruneInterval = time.Millisecond diff --git a/pkg/ha/remote_syncer.go b/pkg/ha/remote_syncer.go index 9f5fe3fa..088876fd 100644 --- a/pkg/ha/remote_syncer.go +++ b/pkg/ha/remote_syncer.go @@ -25,8 +25,10 @@ import ( "github.com/kubeslice/kubeslice-monitoring/pkg/events" "go.uber.org/zap" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -65,6 +67,36 @@ func namespaceMirrorSelector() labels.Selector { return labels.SelectorFromSet(util.LabelsKubeSliceController) } +// mirrorCacheByObject scopes the remote cache's informers server-side, so +// unrelated Active-hub objects never reach this process at all — the mirror +// set's Skip predicates enforce the same boundaries client-side, but for +// core types that exist cluster-wide (Secrets above all) not watching them +// in the first place is both the cheaper and the safer layer. +// +// - Namespace, ServiceAccount, Role, RoleBinding: label-scoped to +// util.LabelsKubeSliceController — stamped on every project namespace by +// ReconcileProjectNamespace and on every credential object the +// controller creates via util.GetOwnerLabel (which embeds the same +// key/value pair). +// - Secret: cannot be label-scoped — gateway certificate Secrets are +// created by the external cert-generator job, unlabeled. Scoped instead +// with a field selector excluding SA-token Secrets (the API server +// supports "type" as a Secret field selector), the highest-volume +// class; the project-namespace boundary stays client-side in +// CredentialMirrorSet's Skip predicate. +func mirrorCacheByObject() map[client.Object]cache.ByObject { + controllerManaged := namespaceMirrorSelector() + return map[client.Object]cache.ByObject{ + &corev1.Namespace{}: {Label: controllerManaged}, + &corev1.ServiceAccount{}: {Label: controllerManaged}, + &rbacv1.Role{}: {Label: controllerManaged}, + &rbacv1.RoleBinding{}: {Label: controllerManaged}, + &corev1.Secret{}: { + Field: fields.OneTermNotEqualSelector("type", string(corev1.SecretTypeServiceAccountToken)), + }, + } +} + // opDelete extends mirror.go's opCreate/opUpdate for use in this file's // metrics/logging; mirrorDelete itself has no ambiguity about which // operation it performed, so it doesn't need to return one. @@ -196,12 +228,8 @@ func NewRemoteSyncer(localClient client.Client, remoteCfg *rest.Config, scheme * return nil, fmt.Errorf("standby mode requires a remote config for the active hub") } remoteCache, err := cache.New(remoteCfg, cache.Options{ - Scheme: scheme, - ByObject: map[client.Object]cache.ByObject{ - // Namespace is cluster-scoped and otherwise unfiltered; see - // namespaceMirrorSelector's doc comment for why this matters. - &corev1.Namespace{}: {Label: namespaceMirrorSelector()}, - }, + Scheme: scheme, + ByObject: mirrorCacheByObject(), }) if err != nil { return nil, fmt.Errorf("building remote cache: %w", err) @@ -426,6 +454,15 @@ func (s *RemoteSyncer) reconcileKey(ctx context.Context, key syncKey) (mirrorOp, if res.Skip != nil && res.Skip(src) { return "", 0, nil } + if res.RequireMirroredNamespace { + mirrored, err := s.namespaceIsMirrored(ctx, src.GetNamespace()) + if err != nil { + return "", 0, fmt.Errorf("checking namespace of %s %s/%s: %w", key.GVK.Kind, key.Namespace, key.Name, err) + } + if !mirrored { + return "", 0, nil + } + } op, err := mirrorCreateOrUpdate(ctx, s.localClient, key, res, src) if err != nil { @@ -440,6 +477,32 @@ func (s *RemoteSyncer) reconcileKey(ctx context.Context, key syncKey) (mirrorOp, return op, time.Since(s.enqueuedTime(key)).Seconds(), nil } +// namespaceIsMirrored reports whether ns is one of the namespaces the syncer +// itself mirrors, by reading the remote cache's Namespace view — which is +// label-scoped to controller-managed project namespaces (see +// namespaceMirrorSelector), so any namespace outside that boundary reads as +// NotFound here no matter what it is named. This is the namespace gate +// behind MirroredResource.RequireMirroredNamespace. +// +// A skip verdict is terminal for this queue item (no retry), so an object +// racing its own namespace's informer delivery on cold start can be skipped +// once — the prune loop's reverse diff re-enqueues it within one +// --ha-sync-interval (see pruneOnce), rather than waiting for the informer's +// much longer resync period. +func (s *RemoteSyncer) namespaceIsMirrored(ctx context.Context, ns string) (bool, error) { + if ns == "" { + return true, nil // cluster-scoped objects have no namespace to gate on + } + nsKey := syncKey{GVK: schema.GroupVersionKind{Version: "v1", Kind: "Namespace"}, Name: ns} + if _, err := s.remoteGet(ctx, nsKey); err != nil { + if apierrors.IsNotFound(err) { + return false, nil + } + return false, err + } + return true, nil +} + // getFromRemoteCache is remoteGetFunc's real implementation: a read from // controller-runtime's cache, which serves Get from the informer's local // indexer rather than the network, so it stays cheap even when a retry