diff --git a/apis/kubefleet.dev/placement/v1alpha1/common.go b/apis/kubefleet.dev/placement/v1alpha1/common.go index e1b991dc1..d70d8404b 100644 --- a/apis/kubefleet.dev/placement/v1alpha1/common.go +++ b/apis/kubefleet.dev/placement/v1alpha1/common.go @@ -16,6 +16,64 @@ limitations under the License. package v1alpha1 +// The annotation and label keys that KubeFleet reserves for the placement APIs. +// +// Note that these keys carry the KubeFleet domain itself rather than the API group; they are +// set on arbitrary Kubernetes objects, not only on the objects of this group. +const ( + // KubeFleetPrefix is the domain that prefixes every reserved annotation and label key below, + // per the Kubernetes convention that reserves unprefixed keys for end users. + KubeFleetPrefix = "kubefleet.dev/" + + // ClusterSelectorsAnnotation is the annotation that requests annotation-based placement for the + // resource it is set on. Its value is a semicolon-separated list of cluster selectors, each of + // which is a comma-separated list of LABEL_KEY=LABEL_VALUE label matchers with an optional + // count=N|All directive, e.g.: + // + // kubefleet.dev/cluster-selectors: "env=staging,count=All;env=canary,region=eastus,count=1" + // + // KubeFleet keeps a PlacementPolicy (or ClusterPlacementPolicy) object in sync with the + // annotation for as long as it is present. + // + // The key names what the value holds rather than what KubeFleet does with it, matching the + // clusterSelectors field of the generated policy: the annotation is one way to write that field, + // and the two are read together often enough that they should not have to be translated. + // + // Within the annotation, `region` may be used in place of the well-known + // topology.kubernetes.io/region label key, and `alias` in place of ClusterAliasLabel. + ClusterSelectorsAnnotation = KubeFleetPrefix + "cluster-selectors" + + // ClusterAliasLabel is the label that KubeFleet reserves on member cluster objects for selecting + // clusters by their name (alias). + ClusterAliasLabel = KubeFleetPrefix + "cluster-alias" +) + +// The labels that record, on a placement policy KubeFleet generated from an annotation, the +// resource whose annotation caused it to exist. +// +// These keys carry the API group rather than the bare KubeFleet domain, since unlike the keys +// above they are set on objects of this group. +// +// The labels exist so that a policy can be found with a List when its generated name is not known; +// the owner reference on the policy remains the authoritative record of where it came from. Note +// that ParentNameLabel is lossy: the name of a resource can run to 253 bytes while a label value +// stops at 63, so for a longer name the label holds a prefix and a hash instead, and selecting on +// it with the resource's own name matches nothing. +const ( + // ParentAPIGroupLabel holds the API group of the resource a policy was generated from. It is + // the empty string for resources in the core API group, and is always present, so that + // core-group resources can be selected as readily as any other. + ParentAPIGroupLabel = "placement.kubefleet.dev/parent-api-group" + + // ParentKindLabel holds the kind of the resource a policy was generated from, spelled as the + // kind itself is (Deployment, not deployment). + ParentKindLabel = "placement.kubefleet.dev/parent-kind" + + // ParentNameLabel holds the name of the resource a policy was generated from, shortened if it + // does not fit in a label value. + ParentNameLabel = "placement.kubefleet.dev/parent-name" +) + type ObjectReference struct { // The namespace of the referenced object. // diff --git a/apis/kubefleet.dev/placement/v1alpha1/placementpolicy_types.go b/apis/kubefleet.dev/placement/v1alpha1/placementpolicy_types.go index f29abb1cf..e1f954e66 100644 --- a/apis/kubefleet.dev/placement/v1alpha1/placementpolicy_types.go +++ b/apis/kubefleet.dev/placement/v1alpha1/placementpolicy_types.go @@ -149,7 +149,17 @@ type PlacementPolicySpec struct { Tolerations []Toleration `json:"tolerations,omitempty"` } -// +kubebuilder:validation:XValidation:rule="!has(self.minCount) || !has(self.count) || (type(self.count) == string && self.count == 'All') || (type(self.count) == int && self.minCount <= self.count) || (type(self.count) == string && self.count.matches('^[0-9]+$') && self.minCount <= int(self.count))",message="minCount must be less than or equal to count when count is not All" +// A string form of count is compared against minCount only when it is a numeric one of at most the +// three digits that the count field's own pattern permits -- the {1,3} bound below must move with +// that pattern's ceiling. Any other string ("All", or junk the pattern rejects) passes this rule +// vacuously, so that the field's own validation reports the real problem alone. The previous, +// unbounded digit guard let a string longer than an int64 reach int(), turning a plain pattern +// violation into an opaque evaluation error beside it. The digit class is spelled [0-9]{1,3} +// rather than mirroring the pattern's [1-9][0-9]{0,2} verbatim because the CEL cost estimator +// prices a regex by its length against an unbounded string -- an int-or-string is opaque to the +// sibling MaxLength -- and the longer spelling does not fit the budget. + +// +kubebuilder:validation:XValidation:rule="!has(self.minCount) || !has(self.count) || (type(self.count) == int && self.minCount <= self.count) || (type(self.count) == string && (!self.count.matches('^[0-9]{1,3}$') || self.minCount <= int(self.count)))",message="minCount must be less than or equal to count" type ClusterSelector struct { // A list of terms that form the selector. The terms are ORed, i.e., a cluster would match the selector // if it matches any of the terms. @@ -160,6 +170,12 @@ type ClusterSelector struct { // +kubebuilder:validation:MaxItems=5 Terms []ClusterLabelAndPropertySelectorTerm `json:"terms,omitempty"` + // The two validation markers below split the work by form: a pattern constrains only the string + // form of an int-or-string ("All", and digits arriving as a quoted string), so the CEL rule is + // what bounds the integer form. Without it, an unquoted count outside 1-999 is accepted while + // the same number in quotes is rejected. This block is deliberately detached from the field's + // doc comment: it is rationale for maintainers, not schema documentation for users. + // The desired number of clusters that KubeFleet should select based on the given terms. // // The default value is 1. To select all clusters that match the given terms, use the value "All". @@ -167,7 +183,9 @@ type ClusterSelector struct { // +kubebuilder:validation:Optional // +kubebuilder:default=1 // +kubebuilder:validation:XIntOrString + // +kubebuilder:validation:MaxLength=3 // +kubebuilder:validation:Pattern="^([1-9][0-9]{0,2}|All)$" + // +kubebuilder:validation:XValidation:rule="type(self) == int ? self >= 1 && self <= 999 : true",message="count must be between 1 and 999, or \"All\"" Count *intstr.IntOrString `json:"count,omitempty"` // The minimum number of clusters that KubeFleet should select based on the given terms, when KubeFleet is not able diff --git a/apis/placement/v1/clusterresourceplacement_types.go b/apis/placement/v1/clusterresourceplacement_types.go index f86563a30..965f8da46 100644 --- a/apis/placement/v1/clusterresourceplacement_types.go +++ b/apis/placement/v1/clusterresourceplacement_types.go @@ -969,7 +969,8 @@ type RollingUpdateConfig struct { // Defaults to 25%. // +kubebuilder:default="25%" // +kubebuilder:validation:XIntOrString - // +kubebuilder:validation:Pattern="^((100|[0-9]{1,2})%|[0-9]+)$" + // +kubebuilder:validation:Pattern="^((100|[0-9]{1,2})%|[0-9]{1,9})$" + // +kubebuilder:validation:XValidation:rule="type(self) == int ? self >= 0 : true",message="maxUnavailable must be a non-negative integer or a percentage" // +kubebuilder:validation:Optional MaxUnavailable *intstr.IntOrString `json:"maxUnavailable,omitempty"` @@ -983,7 +984,8 @@ type RollingUpdateConfig struct { // Defaults to 25%. // +kubebuilder:default="25%" // +kubebuilder:validation:XIntOrString - // +kubebuilder:validation:Pattern="^((100|[0-9]{1,2})%|[0-9]+)$" + // +kubebuilder:validation:Pattern="^((100|[0-9]{1,2})%|[0-9]{1,9})$" + // +kubebuilder:validation:XValidation:rule="type(self) == int ? self >= 0 : true",message="maxSurge must be a non-negative integer or a percentage" // +kubebuilder:validation:Optional MaxSurge *intstr.IntOrString `json:"maxSurge,omitempty"` diff --git a/apis/placement/v1beta1/clusterresourceplacement_types.go b/apis/placement/v1beta1/clusterresourceplacement_types.go index ad3b8556c..1eb57a784 100644 --- a/apis/placement/v1beta1/clusterresourceplacement_types.go +++ b/apis/placement/v1beta1/clusterresourceplacement_types.go @@ -984,7 +984,8 @@ type RollingUpdateConfig struct { // Defaults to 25%. // +kubebuilder:default="25%" // +kubebuilder:validation:XIntOrString - // +kubebuilder:validation:Pattern="^((100|[0-9]{1,2})%|[0-9]+)$" + // +kubebuilder:validation:Pattern="^((100|[0-9]{1,2})%|[0-9]{1,9})$" + // +kubebuilder:validation:XValidation:rule="type(self) == int ? self >= 0 : true",message="maxUnavailable must be a non-negative integer or a percentage" // +kubebuilder:validation:Optional MaxUnavailable *intstr.IntOrString `json:"maxUnavailable,omitempty"` @@ -998,7 +999,8 @@ type RollingUpdateConfig struct { // Defaults to 25%. // +kubebuilder:default="25%" // +kubebuilder:validation:XIntOrString - // +kubebuilder:validation:Pattern="^((100|[0-9]{1,2})%|[0-9]+)$" + // +kubebuilder:validation:Pattern="^((100|[0-9]{1,2})%|[0-9]{1,9})$" + // +kubebuilder:validation:XValidation:rule="type(self) == int ? self >= 0 : true",message="maxSurge must be a non-negative integer or a percentage" // +kubebuilder:validation:Optional MaxSurge *intstr.IntOrString `json:"maxSurge,omitempty"` diff --git a/charts/hub-agent/README.md b/charts/hub-agent/README.md index 7cc93551b..ab10cd780 100644 --- a/charts/hub-agent/README.md +++ b/charts/hub-agent/README.md @@ -109,6 +109,7 @@ _See [helm install](https://helm.sh/docs/helm/helm_install/) for command documen | `enableClusterInventoryAPI` | Enable cluster inventory APIs | `true` | | `enableStagedUpdateRunAPIs` | Enable staged update run APIs | `true` | | `enableEvictionAPIs` | Enable eviction APIs | `true` | +| `enableAnnotationBasedPlacement` | Keep a placement policy in sync with the `kubefleet.dev/cluster-selectors` annotation on a resource. Requires the `placement.kubefleet.dev/v1alpha1` CRDs. No chart installs them yet; apply them from `config/crd/bases/` (the hub agent refuses to start with this flag on until they are present). | `false` | | `enablePprof` | Enable pprof endpoint | `true` | | `pprofPort` | pprof server port | `6065` | | `hubAPIQPS` | QPS for fleet-apiserver (not including events/node heartbeat) | `250` | diff --git a/charts/hub-agent/templates/deployment.yaml b/charts/hub-agent/templates/deployment.yaml index 57b857298..6fb5d2f98 100644 --- a/charts/hub-agent/templates/deployment.yaml +++ b/charts/hub-agent/templates/deployment.yaml @@ -50,6 +50,7 @@ spec: - --enable-cluster-inventory-apis={{ .Values.enableClusterInventoryAPI }} - --enable-staged-update-run-apis={{ .Values.enableStagedUpdateRunAPIs }} - --enable-eviction-apis={{ .Values.enableEvictionAPIs}} + - --enable-annotation-based-placement={{ .Values.enableAnnotationBasedPlacement }} - --enable-pprof={{ .Values.enablePprof }} - --pprof-port={{ .Values.pprofPort }} - --max-concurrent-cluster-placement={{ .Values.MaxConcurrentClusterPlacement }} diff --git a/charts/hub-agent/templates/rbac.yaml b/charts/hub-agent/templates/rbac.yaml index 11b58a658..989fabe63 100644 --- a/charts/hub-agent/templates/rbac.yaml +++ b/charts/hub-agent/templates/rbac.yaml @@ -67,6 +67,20 @@ rules: - approvalrequests/status verbs: ["get", "update"] + # Placement policies the hub-agent generates from the + # kubefleet.dev/cluster-selectors annotation on a resource. Unlike the + # user-created placement resources above, the hub-agent owns these outright: + # it creates one when the annotation appears, updates it when the annotation + # changes, and deletes it when the annotation is removed. No status + # subresource rule: the generating controller never writes status. + # + # The broad read rule further down does not cover these, being read-only. + - apiGroups: ["placement.kubefleet.dev"] + resources: + - placementpolicies + - clusterplacementpolicies + verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] + # Fleet cluster APIs. MemberCluster is user-created and user-deleted; the # hub-agent only adds/removes its finalizer (update) and writes status. # InternalMemberCluster is created by the hub-agent and cleaned up via diff --git a/charts/hub-agent/values.yaml b/charts/hub-agent/values.yaml index 0db937227..9bcb1cb28 100644 --- a/charts/hub-agent/values.yaml +++ b/charts/hub-agent/values.yaml @@ -48,6 +48,10 @@ affinity: {} enableClusterInventoryAPI: true enableStagedUpdateRunAPIs: true enableEvictionAPIs: true +# Keeps a placement policy in sync with the kubefleet.dev/cluster-selectors annotation on a resource. +# It requires the placement.kubefleet.dev/v1alpha1 custom resource definitions, which this chart does not +# install yet, so it is off by default. +enableAnnotationBasedPlacement: false enablePprof: true pprofPort: 6065 diff --git a/cmd/hubagent/main.go b/cmd/hubagent/main.go index ec14e40eb..c2ceff4dd 100644 --- a/cmd/hubagent/main.go +++ b/cmd/hubagent/main.go @@ -44,6 +44,7 @@ import ( fleetnetworkingv1alpha1 "go.goms.io/fleet-networking/api/v1alpha1" clusterv1beta1 "github.com/kubefleet-dev/kubefleet/apis/cluster/v1beta1" + kfplacementv1alpha1 "github.com/kubefleet-dev/kubefleet/apis/kubefleet.dev/placement/v1alpha1" placementv1alpha1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1alpha1" placementv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1" "github.com/kubefleet-dev/kubefleet/cmd/hubagent/options" @@ -80,6 +81,7 @@ func init() { utilruntime.Must(fleetnetworkingv1alpha1.AddToScheme(scheme)) utilruntime.Must(placementv1alpha1.AddToScheme(scheme)) utilruntime.Must(clusterinventory.AddToScheme(scheme)) + utilruntime.Must(kfplacementv1alpha1.AddToScheme(scheme)) // +kubebuilder:scaffold:scheme klog.InitFlags(nil) } @@ -171,6 +173,7 @@ func main() { NetworkingAgentsEnabled: opts.ClusterMgmtOpts.NetworkingAgentsEnabled, MaxConcurrentReconciles: int(math.Ceil(float64(opts.PlacementMgmtOpts.MaxFleetSize) / 100)), //one member cluster reconciler routine per 100 member clusters ForceDeleteWaitTime: opts.ClusterMgmtOpts.ForceDeleteWaitTime.Duration, + SeedClusterAliasLabel: opts.FeatureFlags.EnableAnnotationBasedPlacement, }).SetupWithManager(mgr, "membercluster-controller"); err != nil { klog.ErrorS(err, "unable to create v1beta1 controller", "controller", "MemberCluster") exitWithErrorFunc() diff --git a/cmd/hubagent/options/featureflags.go b/cmd/hubagent/options/featureflags.go index 00b0d6e6b..d39be56dd 100644 --- a/cmd/hubagent/options/featureflags.go +++ b/cmd/hubagent/options/featureflags.go @@ -51,6 +51,17 @@ type FeatureFlags struct { // ResourcePlacement APIs are a set of KubeFleet APIs for processing namespace scoped resource placements. // This flag does not concern the cluster-scoped placement APIs (`ClusterResourcePlacement` and its related APIs). EnableResourcePlacementAPIs bool + + // Enable annotation-based placement in the KubeFleet hub agent or not. + // + // With it enabled, the hub agent keeps a placement policy in sync with the + // kubefleet.dev/cluster-selectors annotation on any resource it watches, so that a placement can + // be expressed without authoring a placement policy by hand. + // + // This defaults to off. The feature reads and writes the placement.kubefleet.dev/v1alpha1 + // placement policy APIs, which are alpha and whose custom resource definitions a cluster does not + // necessarily have installed. + EnableAnnotationBasedPlacement bool } // AddFlags adds flags for FeatureFlags to the specified FlagSet. @@ -88,6 +99,13 @@ func (o *FeatureFlags) AddFlags(flags *flag.FlagSet) { true, "Enable the ResourcePlacement API support (for namespace-scoped placements) in the KubeFleet hub agent or not.", ) + + flags.BoolVar( + &o.EnableAnnotationBasedPlacement, + "enable-annotation-based-placement", + false, + "Enable annotation-based placement in the KubeFleet hub agent or not. It requires the placement.kubefleet.dev/v1alpha1 custom resource definitions to be installed.", + ) } // A list of flag variables that allow pluggable validation logic when parsing the input args. diff --git a/cmd/hubagent/options/options_test.go b/cmd/hubagent/options/options_test.go index bb3231ceb..9ce91dc6d 100644 --- a/cmd/hubagent/options/options_test.go +++ b/cmd/hubagent/options/options_test.go @@ -334,6 +334,9 @@ func TestFeatureFlags(t *testing.T) { EnableStagedUpdateRunAPIs: true, EnableEvictionAPIs: true, EnableResourcePlacementAPIs: true, + // Annotation-based placement is alpha and needs CRDs the chart does not install, + // so unlike its siblings it is off unless asked for. + EnableAnnotationBasedPlacement: false, }, }, { @@ -345,13 +348,15 @@ func TestFeatureFlags(t *testing.T) { "--enable-staged-update-run-apis=false", "--enable-eviction-apis=false", "--enable-resource-placement=false", + "--enable-annotation-based-placement=true", }, wantFeatureFlags: FeatureFlags{ - EnableV1Beta1APIs: true, - EnableClusterInventoryAPIs: false, - EnableStagedUpdateRunAPIs: false, - EnableEvictionAPIs: false, - EnableResourcePlacementAPIs: false, + EnableV1Beta1APIs: true, + EnableClusterInventoryAPIs: false, + EnableStagedUpdateRunAPIs: false, + EnableEvictionAPIs: false, + EnableResourcePlacementAPIs: false, + EnableAnnotationBasedPlacement: true, }, }, { diff --git a/cmd/hubagent/workload/setup.go b/cmd/hubagent/workload/setup.go index ec895061f..b16de10ce 100644 --- a/cmd/hubagent/workload/setup.go +++ b/cmd/hubagent/workload/setup.go @@ -22,6 +22,7 @@ import ( "strings" "sync" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/discovery" "k8s.io/client-go/dynamic" @@ -31,8 +32,10 @@ import ( ctrl "sigs.k8s.io/controller-runtime" clusterv1beta1 "github.com/kubefleet-dev/kubefleet/apis/cluster/v1beta1" + kfplacementv1alpha1 "github.com/kubefleet-dev/kubefleet/apis/kubefleet.dev/placement/v1alpha1" placementv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1" "github.com/kubefleet-dev/kubefleet/cmd/hubagent/options" + "github.com/kubefleet-dev/kubefleet/pkg/controllers/annotationplacement" "github.com/kubefleet-dev/kubefleet/pkg/controllers/bindingwatcher" "github.com/kubefleet-dev/kubefleet/pkg/controllers/clusterinventory/clusterprofile" "github.com/kubefleet-dev/kubefleet/pkg/controllers/clusterresourceplacementeviction" @@ -69,6 +72,8 @@ const ( resourceChangeControllerName = "resource-change-controller" + annotationPlacementControllerName = "annotation-placement-controller" + schedulerQueueName = "scheduler-queue" ) @@ -116,6 +121,14 @@ var ( placementv1beta1.GroupVersion.WithKind(placementv1beta1.ClusterResourcePlacementEvictionKind), placementv1beta1.GroupVersion.WithKind(placementv1beta1.ClusterResourcePlacementDisruptionBudgetKind), } + + // The kinds annotation-based placement generates, and so cannot run without. The kinds are + // spelled out here rather than taken from constants because the placement.kubefleet.dev API + // package does not declare any yet. + annotationBasedPlacementGVKs = []schema.GroupVersionKind{ + kfplacementv1alpha1.GroupVersion.WithKind("PlacementPolicy"), + kfplacementv1alpha1.GroupVersion.WithKind("ClusterPlacementPolicy"), + } ) // SetupControllers set up the customized controllers we developed @@ -504,6 +517,51 @@ func SetupControllers(ctx context.Context, wg *sync.WaitGroup, mgr ctrl.Manager, } resourceChangeController := controller.NewController(resourceChangeControllerName, controller.ClusterWideKeyFunc, rcr.Reconcile, rateLimiter) + // Set up the controller that keeps a placement policy in sync with the cluster-selectors + // annotation on a resource. It shares the resource change controller's informers, but is fed + // only the resources that carry the annotation. + var annotationPlacementController controller.Controller + if opts.FeatureFlags.EnableAnnotationBasedPlacement { + for _, gvk := range annotationBasedPlacementGVKs { + if err = utils.CheckCRDInstalled(discoverClient, gvk); err != nil { + klog.ErrorS(err, "unable to find the CRD that annotation based placement requires", "GVK", gvk) + return err + } + } + klog.Info("Setting up annotation based placement controller") + apr := &annotationplacement.Reconciler{ + Client: mgr.GetClient(), + UncachedReader: mgr.GetAPIReader(), + RestMapper: mgr.GetRESTMapper(), + InformerManager: dynamicInformerManager, + Recorder: mgr.GetEventRecorderFor(annotationPlacementControllerName), + // The same eligibility test the change detector applies to its events. The reconciler + // needs it again because it can be reached for a resource the detector filters out -- + // through the generated policy watch, or through the deletion-shaped event the detector + // reports when a resource stops passing the filter without being deleted. + ShouldPlace: func(source *unstructured.Unstructured) (bool, error) { + // The resource config decides which APIs are watched at all; a source of a disabled + // API is one the watcher would never have selected, so a policy generated for it + // before the API was excluded must stop being kept. The check belongs here because + // the generated policy watch can reach this controller for such a source even after + // its API is gone from the watch set. + if resourceConfig.IsResourceDisabled(source.GroupVersionKind()) { + return false, nil + } + if !utils.ShouldPropagateNamespace(annotationplacement.SourceNamespace(source), skippedNamespaces) { + return false, nil + } + return controller.ShouldPropagateObj(dynamicInformerManager, source.DeepCopy(), opts.WebhookAndAdmissionPolicyOpts.EnableWorkload) + }, + } + // A rate limiter of its own, rather than the one the controllers above share. An exponential + // failure limiter keys its backoff on the queued item alone, and this controller queues the + // very same cluster wide keys as the resource change controller: sharing one would let a + // success here clear the backoff that repeated failures there had earned. + annotationPlacementRateLimiter := options.DefaultControllerRateLimiter(opts.PlacementMgmtOpts.PlacementControllerWorkQueueRateLimiterOpts) + annotationPlacementController = controller.NewController(annotationPlacementControllerName, controller.ClusterWideKeyFunc, apr.Reconcile, annotationPlacementRateLimiter) + } + // Set up the InformerPopulator that runs on ALL pods (leader and followers) // This ensures all pods have synced informer caches for webhook validation klog.Info("Setting up informer populator") @@ -527,6 +585,7 @@ func SetupControllers(ctx context.Context, wg *sync.WaitGroup, mgr ctrl.Manager, ClusterResourcePlacementControllerV1Beta1: clusterResourcePlacementControllerV1Beta1, ResourcePlacementController: resourcePlacementController, ResourceChangeController: resourceChangeController, + AnnotationPlacementController: annotationPlacementController, InformerManager: dynamicInformerManager, ResourceConfig: resourceConfig, SkippedNamespaces: skippedNamespaces, diff --git a/config/crd/bases/placement.kubefleet.dev_clusterplacementpolicies.yaml b/config/crd/bases/placement.kubefleet.dev_clusterplacementpolicies.yaml index d528f88bd..329cfdd64 100644 --- a/config/crd/bases/placement.kubefleet.dev_clusterplacementpolicies.yaml +++ b/config/crd/bases/placement.kubefleet.dev_clusterplacementpolicies.yaml @@ -63,8 +63,12 @@ spec: The desired number of clusters that KubeFleet should select based on the given terms. The default value is 1. To select all clusters that match the given terms, use the value "All". + maxLength: 3 pattern: ^([1-9][0-9]{0,2}|All)$ x-kubernetes-int-or-string: true + x-kubernetes-validations: + - message: count must be between 1 and 999, or "All" + rule: 'type(self) == int ? self >= 1 && self <= 999 : true' minCount: description: |- The minimum number of clusters that KubeFleet should select based on the given terms, when KubeFleet is not able @@ -238,13 +242,11 @@ spec: type: string type: object x-kubernetes-validations: - - message: minCount must be less than or equal to count when count - is not All + - message: minCount must be less than or equal to count rule: '!has(self.minCount) || !has(self.count) || (type(self.count) - == string && self.count == ''All'') || (type(self.count) == - int && self.minCount <= self.count) || (type(self.count) == - string && self.count.matches(''^[0-9]+$'') && self.minCount - <= int(self.count))' + == int && self.minCount <= self.count) || (type(self.count) + == string && (!self.count.matches(''^[0-9]{1,3}$'') || self.minCount + <= int(self.count)))' maxItems: 10 minItems: 1 type: array diff --git a/config/crd/bases/placement.kubefleet.dev_placementpolicies.yaml b/config/crd/bases/placement.kubefleet.dev_placementpolicies.yaml index 578a6eb5f..bf1da5129 100644 --- a/config/crd/bases/placement.kubefleet.dev_placementpolicies.yaml +++ b/config/crd/bases/placement.kubefleet.dev_placementpolicies.yaml @@ -63,8 +63,12 @@ spec: The desired number of clusters that KubeFleet should select based on the given terms. The default value is 1. To select all clusters that match the given terms, use the value "All". + maxLength: 3 pattern: ^([1-9][0-9]{0,2}|All)$ x-kubernetes-int-or-string: true + x-kubernetes-validations: + - message: count must be between 1 and 999, or "All" + rule: 'type(self) == int ? self >= 1 && self <= 999 : true' minCount: description: |- The minimum number of clusters that KubeFleet should select based on the given terms, when KubeFleet is not able @@ -238,13 +242,11 @@ spec: type: string type: object x-kubernetes-validations: - - message: minCount must be less than or equal to count when count - is not All + - message: minCount must be less than or equal to count rule: '!has(self.minCount) || !has(self.count) || (type(self.count) - == string && self.count == ''All'') || (type(self.count) == - int && self.minCount <= self.count) || (type(self.count) == - string && self.count.matches(''^[0-9]+$'') && self.minCount - <= int(self.count))' + == int && self.minCount <= self.count) || (type(self.count) + == string && (!self.count.matches(''^[0-9]{1,3}$'') || self.minCount + <= int(self.count)))' maxItems: 10 minItems: 1 type: array diff --git a/config/crd/bases/placement.kubernetes-fleet.io_clusterresourceplacements.yaml b/config/crd/bases/placement.kubernetes-fleet.io_clusterresourceplacements.yaml index 7740c8c48..9aa1b1e41 100644 --- a/config/crd/bases/placement.kubernetes-fleet.io_clusterresourceplacements.yaml +++ b/config/crd/bases/placement.kubernetes-fleet.io_clusterresourceplacements.yaml @@ -952,8 +952,11 @@ spec: This does not apply to the case that we do in-place update of resources on the same cluster. This can not be 0 if MaxUnavailable is 0. Defaults to 25%. - pattern: ^((100|[0-9]{1,2})%|[0-9]+)$ + pattern: ^((100|[0-9]{1,2})%|[0-9]{1,9})$ x-kubernetes-int-or-string: true + x-kubernetes-validations: + - message: maxSurge must be a non-negative integer or a percentage + rule: 'type(self) == int ? self >= 0 : true' maxUnavailable: anyOf: - type: integer @@ -971,8 +974,12 @@ spec: The minimum of MaxUnavailable is 0 to allow no downtime moving a placement from one cluster to another. Please set it to be greater than 0 to avoid rolling out stuck during in-place resource update. Defaults to 25%. - pattern: ^((100|[0-9]{1,2})%|[0-9]+)$ + pattern: ^((100|[0-9]{1,2})%|[0-9]{1,9})$ x-kubernetes-int-or-string: true + x-kubernetes-validations: + - message: maxUnavailable must be a non-negative integer or + a percentage + rule: 'type(self) == int ? self >= 0 : true' unavailablePeriodSeconds: default: 60 description: |- @@ -2631,8 +2638,11 @@ spec: This does not apply to the case that we do in-place update of resources on the same cluster. This can not be 0 if MaxUnavailable is 0. Defaults to 25%. - pattern: ^((100|[0-9]{1,2})%|[0-9]+)$ + pattern: ^((100|[0-9]{1,2})%|[0-9]{1,9})$ x-kubernetes-int-or-string: true + x-kubernetes-validations: + - message: maxSurge must be a non-negative integer or a percentage + rule: 'type(self) == int ? self >= 0 : true' maxUnavailable: anyOf: - type: integer @@ -2650,8 +2660,12 @@ spec: The minimum of MaxUnavailable is 0 to allow no downtime moving a placement from one cluster to another. Please set it to be greater than 0 to avoid rolling out stuck during in-place resource update. Defaults to 25%. - pattern: ^((100|[0-9]{1,2})%|[0-9]+)$ + pattern: ^((100|[0-9]{1,2})%|[0-9]{1,9})$ x-kubernetes-int-or-string: true + x-kubernetes-validations: + - message: maxUnavailable must be a non-negative integer or + a percentage + rule: 'type(self) == int ? self >= 0 : true' unavailablePeriodSeconds: default: 60 description: |- diff --git a/config/crd/bases/placement.kubernetes-fleet.io_resourceplacements.yaml b/config/crd/bases/placement.kubernetes-fleet.io_resourceplacements.yaml index ee3855bce..df15dd56a 100644 --- a/config/crd/bases/placement.kubernetes-fleet.io_resourceplacements.yaml +++ b/config/crd/bases/placement.kubernetes-fleet.io_resourceplacements.yaml @@ -944,8 +944,11 @@ spec: This does not apply to the case that we do in-place update of resources on the same cluster. This can not be 0 if MaxUnavailable is 0. Defaults to 25%. - pattern: ^((100|[0-9]{1,2})%|[0-9]+)$ + pattern: ^((100|[0-9]{1,2})%|[0-9]{1,9})$ x-kubernetes-int-or-string: true + x-kubernetes-validations: + - message: maxSurge must be a non-negative integer or a percentage + rule: 'type(self) == int ? self >= 0 : true' maxUnavailable: anyOf: - type: integer @@ -963,8 +966,12 @@ spec: The minimum of MaxUnavailable is 0 to allow no downtime moving a placement from one cluster to another. Please set it to be greater than 0 to avoid rolling out stuck during in-place resource update. Defaults to 25%. - pattern: ^((100|[0-9]{1,2})%|[0-9]+)$ + pattern: ^((100|[0-9]{1,2})%|[0-9]{1,9})$ x-kubernetes-int-or-string: true + x-kubernetes-validations: + - message: maxUnavailable must be a non-negative integer or + a percentage + rule: 'type(self) == int ? self >= 0 : true' unavailablePeriodSeconds: default: 60 description: |- @@ -2608,8 +2615,11 @@ spec: This does not apply to the case that we do in-place update of resources on the same cluster. This can not be 0 if MaxUnavailable is 0. Defaults to 25%. - pattern: ^((100|[0-9]{1,2})%|[0-9]+)$ + pattern: ^((100|[0-9]{1,2})%|[0-9]{1,9})$ x-kubernetes-int-or-string: true + x-kubernetes-validations: + - message: maxSurge must be a non-negative integer or a percentage + rule: 'type(self) == int ? self >= 0 : true' maxUnavailable: anyOf: - type: integer @@ -2627,8 +2637,12 @@ spec: The minimum of MaxUnavailable is 0 to allow no downtime moving a placement from one cluster to another. Please set it to be greater than 0 to avoid rolling out stuck during in-place resource update. Defaults to 25%. - pattern: ^((100|[0-9]{1,2})%|[0-9]+)$ + pattern: ^((100|[0-9]{1,2})%|[0-9]{1,9})$ x-kubernetes-int-or-string: true + x-kubernetes-validations: + - message: maxUnavailable must be a non-negative integer or + a percentage + rule: 'type(self) == int ? self >= 0 : true' unavailablePeriodSeconds: default: 60 description: |- diff --git a/pkg/controllers/annotationplacement/controller.go b/pkg/controllers/annotationplacement/controller.go new file mode 100644 index 000000000..6a0c195c9 --- /dev/null +++ b/pkg/controllers/annotationplacement/controller.go @@ -0,0 +1,480 @@ +/* +Copyright 2026 The KubeFleet Authors. + +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 annotationplacement + +import ( + "context" + "fmt" + "slices" + "time" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/tools/record" + "k8s.io/klog/v2" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + + kfplacementv1alpha1 "github.com/kubefleet-dev/kubefleet/apis/kubefleet.dev/placement/v1alpha1" + "github.com/kubefleet-dev/kubefleet/pkg/utils/controller" + "github.com/kubefleet-dev/kubefleet/pkg/utils/informer" + "github.com/kubefleet-dev/kubefleet/pkg/utils/keys" +) + +// The reasons of the events this controller records on the annotated resource. Each reason is the +// same whether the generated object is a PlacementPolicy or a ClusterPlacementPolicy -- the reason +// names the action, and the event message names the concrete kind. +// +// The events are recorded on the resource the user annotated rather than on the policy generated +// from it, because the resource is where a user who has just run kubectl annotate is looking, and +// because a rejected annotation generates no policy for an event to be attached to. +const ( + // EventReasonPolicyCreated is recorded when a generated policy is created for a resource. + EventReasonPolicyCreated = "PlacementPolicyCreated" + // EventReasonPolicyUpdated is recorded when an annotation change reaches its generated policy. + EventReasonPolicyUpdated = "PlacementPolicyUpdated" + // EventReasonPolicyDeleted is recorded when the policy generated for a resource is deleted -- + // because the annotation was removed, or because the resource stopped being eligible for + // placement. The event's message names which. + EventReasonPolicyDeleted = "PlacementPolicyDeleted" + // EventReasonInvalidAnnotation is recorded when an annotation cannot be parsed. It is a warning + // rather than an error on the queue: no amount of retrying makes a malformed annotation parse, + // so the event is the only way the user learns that the placement they asked for is not running. + EventReasonInvalidAnnotation = "InvalidClusterSelectorsAnnotation" + // EventReasonPolicyConflict is recorded when a policy already exists at a resource's generated + // name but is not one this controller generated. It is a warning: the controller neither + // overwrites nor deletes the pre-existing policy, so the placement the annotation asks for is not + // running, and the event is how the user learns why. + EventReasonPolicyConflict = "PlacementPolicyConflict" +) + +// conflictRequeueAfter is how long the reconciler waits before re-examining a source whose generated +// name is occupied by a policy it did not create. +// +// The source has to be requeued, not merely left, because nothing else will bring it back: the +// blocking policy carries no owner reference to the source, so removing it enqueues its own owners +// and never this source, and the annotation the source carries does not change, so no source event +// fires either. Without a requeue the placement the annotation asks for would never be created once +// the conflict is cleared. The interval is a compromise -- short enough that resolving the conflict +// takes visible effect soon, long enough that a conflict left in place does not busy-poll. +const conflictRequeueAfter = time.Minute + +// Reconciler keeps the placement policy generated from a resource's cluster-selectors annotation in +// sync with that annotation. +// +// It is driven by the same dynamic informers as the resource change controller, so its queue holds +// keys for resources of any kind the hub agent watches, not for the generated policies themselves. +type Reconciler struct { + // Client writes the generated placement policies -- creating, updating, and deleting them. Reads + // go through UncachedReader, so this is used only for the mutations. + Client client.Client + + // UncachedReader reads the generated placement policies straight from the API server rather than + // from a cache. The policies are watched through a different informer than a manager-backed cache + // would read from, and the two can sit at different points: were a policy read from a cache, an + // edit or a deletion the policy watch delivered first could be met with a stale object, the pass + // would conclude nothing had changed, and the drift would stand until the next resync -- or, for a + // deletion, forever, since a deleted object is gone from the watch's own cache and no later event + // or resync re-delivers it. A read from the API server is current as of the moment the watch + // fired. The reconciler's queue holds only annotated resources and the owners of generated + // policies, so this read is not on the hot path of every watched resource in the cluster. + UncachedReader client.Reader + + // RestMapper converts the group kind of a queued key into the resource that the informer + // manager knows it by. + RestMapper meta.RESTMapper + + // InformerManager holds the informers the annotated resources are read from. + InformerManager informer.Manager + + // Recorder records the outcome of a reconciliation on the annotated resource. + Recorder record.EventRecorder + + // ShouldPlace reports whether a resource is one KubeFleet places at all, mirroring the filter + // the resource watcher applies to its events. For example, a Deployment in a user namespace + // should place (true); a ConfigMap in a skipped namespace like kube-system, or a ReplicaSet a + // Deployment already owns, should not (false). + // + // The reconciler applies it again because it can be reached for a resource the watcher would + // filter out: the watcher reports a resource that stops passing its filter as a deletion, and + // the generated policy watch enqueues whatever a policy names as its owner. A resource that + // fails the check has its generated policy deleted. + // + // Eligibility can change over a resource's life -- an owned ReplicaSet becomes eligible again + // once orphaned, a resource stays ineligible while in a skipped namespace. The transitions that + // matter are edits to the resource itself (its owner references, its labels), so each fires an + // event that re-runs this check; a resource that becomes eligible again and still carries the + // annotation has its policy regenerated on that event. + // + // Left nil, every resource is eligible. + ShouldPlace func(source *unstructured.Unstructured) (bool, error) +} + +// Reconcile brings the generated policy for one resource in line with that resource's annotation. +func (r *Reconciler) Reconcile(ctx context.Context, key controller.QueueKey) (ctrl.Result, error) { + startTime := time.Now() + clusterWideKey, ok := key.(keys.ClusterWideKey) + if !ok { + err := fmt.Errorf("got a resource key %+v not of type cluster wide key", key) + klog.ErrorS(err, "We have encountered a fatal error that can't be retried", "key", key) + return ctrl.Result{}, controller.NewUnexpectedBehaviorError(err) + } + klog.V(2).InfoS("Reconciling annotation-based placement", "obj", clusterWideKey) + defer func() { + klog.V(2).InfoS("Annotation-based placement reconciliation loop ends", "obj", clusterWideKey, "latency", time.Since(startTime).Milliseconds()) + }() + + source, err := r.sourceObject(clusterWideKey) + switch { + case apierrors.IsNotFound(err): + // The resource is gone, and the delete is issued from here rather than left to garbage + // collection. The generated policy does carry an owner reference back to the resource, but + // the collector removes a dependent only once every owner is gone, and the merge + // deliberately preserves owner references that other parties added -- any live one of which + // would keep the policy standing indefinitely. Deleting explicitly is idempotent, so at + // worst it beats the collector to an object that was doomed anyway. + _, deleted, err := r.deleteGeneratedPolicy(ctx, clusterWideKey.GroupVersionKind(), clusterWideKey.Namespace, clusterWideKey.Name) + switch { + case err != nil: + klog.ErrorS(err, "Failed to delete the policy generated for a resource that is gone", "obj", clusterWideKey) + case deleted: + klog.V(2).InfoS("Deleted the policy generated for a resource that is gone", "obj", clusterWideKey) + } + return ctrl.Result{}, err + case meta.IsNoMatchError(err): + // The kind is unknown to the API server even under its served version, so the source's own + // CRD has been removed. The generated policy watch enqueues a policy's generating owner, so + // this key is that stale policy's source: the policy is deleted here rather than left to + // garbage collection, which -- as in the resource-gone case above -- keeps a dependent alive + // as long as any owner reference the merge preserved still is. Retrying cannot make the kind + // exist, so the key is not requeued. + _, deleted, delErr := r.deleteGeneratedPolicy(ctx, clusterWideKey.GroupVersionKind(), clusterWideKey.Namespace, clusterWideKey.Name) + switch { + case delErr != nil: + klog.ErrorS(delErr, "Failed to delete the policy generated for a resource whose kind is gone", "obj", clusterWideKey) + case deleted: + klog.V(2).InfoS("Deleted the policy generated for a resource whose kind is gone", "obj", clusterWideKey) + default: + // deleted is false because nothing this controller generated was there to remove: either + // no policy exists at the name, or one does but belongs to someone else, in which case + // deleteGeneratedPolicy has already logged the decline distinctly. Either way there is + // nothing more to do and no retry can make the kind exist, so the key is dropped. + klog.V(2).InfoS("A key names a kind the API server does not know and this controller has no generated policy to clean up for it; dropping the key", "obj", clusterWideKey) + } + return ctrl.Result{}, delErr + case err != nil: + klog.ErrorS(err, "Failed to get the annotated resource", "obj", clusterWideKey) + return ctrl.Result{}, err + } + + if r.ShouldPlace != nil { + eligible, err := r.ShouldPlace(source) + if err != nil { + klog.ErrorS(err, "Failed to decide whether the resource is eligible for placement", "obj", clusterWideKey) + return ctrl.Result{}, err + } + if !eligible { + // A resource KubeFleet does not place cannot keep a generated policy either; without + // this, a resource that stops being eligible (for instance a ReplicaSet adopted by a + // Deployment) would leave its policy behind, invisible to the watcher from then on. + return ctrl.Result{}, r.deletePolicy(ctx, source, "the resource is not eligible for placement") + } + } + + value, annotated := source.GetAnnotations()[kfplacementv1alpha1.ClusterSelectorsAnnotation] + if !annotated { + return ctrl.Result{}, r.deletePolicy(ctx, source, "the "+kfplacementv1alpha1.ClusterSelectorsAnnotation+" annotation was removed") + } + + selectors, err := parseClusterSelectors(value) + if err != nil { + // The annotation is the user's to fix, so the failure is reported to the user and the key is + // dropped. Any policy generated from an earlier, valid annotation is deliberately left + // standing: the desired state is now unknown, and tearing down a running placement is a + // worse answer to a typo than leaving the last one the user did express. + klog.V(2).InfoS("The annotation cannot be parsed", "obj", clusterWideKey, "err", err) + r.Recorder.Eventf(source, corev1.EventTypeWarning, EventReasonInvalidAnnotation, + "The %s annotation is not valid and no placement policy was generated from it: %s", kfplacementv1alpha1.ClusterSelectorsAnnotation, err) + return ctrl.Result{}, nil + } + return r.syncPolicy(ctx, source, selectors) +} + +// sourceObject reads the annotated resource a queued key refers to. +func (r *Reconciler) sourceObject(key keys.ClusterWideKey) (*unstructured.Unstructured, error) { + restMapping, err := r.RestMapper.RESTMapping(key.GroupKind(), key.Version) + if meta.IsNoMatchError(err) && key.Version != "" { + // The version the key recorded may have been removed while the kind lives on under a newer + // served version -- a policy generated for it is still valid, so the served mapping is + // tried before concluding the kind is gone. + restMapping, err = r.RestMapper.RESTMapping(key.GroupKind()) + } + if err != nil { + if meta.IsNoMatchError(err) { + // Returned unwrapped: the caller distinguishes a kind that does not exist, which no + // retry can fix, and the wrapping below would flatten the error to a string. + return nil, err + } + return nil, controller.NewUnexpectedBehaviorError(fmt.Errorf("failed to get the resource of object %+v: %w", key, err)) + } + gvr := restMapping.Resource + if !r.InformerManager.IsInformerSynced(gvr) { + return nil, controller.NewExpectedBehaviorError(fmt.Errorf("informer cache for %+v is not synced yet", gvr)) + } + + // Scope is read from the mapping just resolved, not from the queued key's group-version-kind. The + // two can disagree: the key's version may be one the informer manager no longer indexes -- because + // it was retired, or because the fallback above resolved a different served version than the key + // named -- and the manager's scope lookup is keyed on the exact version, so it would report a + // cluster-scoped source as namespaced and read it from a namespace it does not live in, missing it + // and taking the resource for deleted. The mapping carries the authoritative scope for the gvr the + // object is actually read through. + var object runtime.Object + if restMapping.Scope.Name() == meta.RESTScopeNameRoot { + object, err = r.InformerManager.Lister(gvr).Get(key.Name) + } else { + object, err = r.InformerManager.Lister(gvr).ByNamespace(key.Namespace).Get(key.Name) + } + if err != nil { + // Wrapped rather than replaced: the caller distinguishes a resource that is gone from a + // cache that could not answer, and only the original error carries that. + return nil, fmt.Errorf("failed to get the object %+v: %w", key, err) + } + + source, ok := object.(*unstructured.Unstructured) + if !ok { + return nil, controller.NewUnexpectedBehaviorError(fmt.Errorf("object %+v read from the informer cache is not unstructured", key)) + } + return source, nil +} + +// syncPolicy creates or updates the policy generated from a resource's annotation, reporting whether +// the source needs to be looked at again later. +func (r *Reconciler) syncPolicy(ctx context.Context, source *unstructured.Unstructured, selectors []kfplacementv1alpha1.ClusterSelector) (ctrl.Result, error) { + desired := desiredPolicy(source, selectors) + actual := emptyPolicyForScope(source.GetNamespace()) + + err := r.UncachedReader.Get(ctx, client.ObjectKeyFromObject(desired), actual) + switch { + case apierrors.IsNotFound(err): + if err := r.Client.Create(ctx, desired); err != nil { + klog.ErrorS(err, "Failed to create the generated placement policy", "obj", klog.KObj(source), "policy", klog.KObj(desired)) + return ctrl.Result{}, controller.NewAPIServerError(false, err) + } + klog.V(2).InfoS("Created the generated placement policy", "obj", klog.KObj(source), "policy", klog.KObj(desired)) + r.Recorder.Eventf(source, corev1.EventTypeNormal, EventReasonPolicyCreated, + "Created the %s %s from the %s annotation", generatedPolicyKind(source.GetNamespace()), desired.GetName(), kfplacementv1alpha1.ClusterSelectorsAnnotation) + return ctrl.Result{}, nil + case err != nil: + klog.ErrorS(err, "Failed to get the generated placement policy", "obj", klog.KObj(source), "policy", klog.KObj(desired)) + return ctrl.Result{}, controller.NewAPIServerError(true, err) + } + + if !isGeneratedFor(actual, source.GroupVersionKind(), source.GetName()) { + // A policy already occupies this resource's generated name, but it is not one this controller + // produced -- a user or another tool authored a policy that happens to collide with the + // deterministic name. Overwriting its spec would silently commandeer it, so it is left exactly + // as found and the conflict is surfaced to the user instead. The name mixes in a hash of the + // resource's identity, so a genuine collision is near impossible and almost always means the + // name was chosen deliberately. + // + // The source is requeued rather than dropped: removing the blocking policy fires no event that + // would reach this source (the policy carries no owner reference to it), and the annotation + // does not change, so nothing else would ever create the requested policy once the conflict is + // cleared. See conflictRequeueAfter. + klog.V(2).InfoS("A policy at the generated name was not generated by this controller; leaving it untouched", "obj", klog.KObj(source), "policy", klog.KObj(actual)) + r.Recorder.Eventf(source, corev1.EventTypeWarning, EventReasonPolicyConflict, + "A %s named %s already exists and was not generated from the %s annotation; it was left unchanged", generatedPolicyKind(source.GetNamespace()), actual.GetName(), kfplacementv1alpha1.ClusterSelectorsAnnotation) + return ctrl.Result{RequeueAfter: conflictRequeueAfter}, nil + } + + if !applyDesiredPolicy(actual, desired) { + klog.V(3).InfoS("The generated placement policy is already up to date", "obj", klog.KObj(source), "policy", klog.KObj(actual)) + return ctrl.Result{}, nil + } + if err := r.Client.Update(ctx, actual); err != nil { + klog.ErrorS(err, "Failed to update the generated placement policy", "obj", klog.KObj(source), "policy", klog.KObj(actual)) + return ctrl.Result{}, controller.NewAPIServerError(false, err) + } + klog.V(2).InfoS("Updated the generated placement policy", "obj", klog.KObj(source), "policy", klog.KObj(actual)) + r.Recorder.Eventf(source, corev1.EventTypeNormal, EventReasonPolicyUpdated, + "Updated the %s %s from the %s annotation", generatedPolicyKind(source.GetNamespace()), actual.GetName(), kfplacementv1alpha1.ClusterSelectorsAnnotation) + return ctrl.Result{}, nil +} + +// deletePolicy removes the policy generated for a resource that should not have one -- because the +// annotation was removed, or because the resource is not eligible for placement -- and tells the +// user which through an event carrying the given cause. +// +// The cause is a plain string, never a format: keeping the only format string in the Eventf call +// below constant is what lets go vet check it. +func (r *Reconciler) deletePolicy(ctx context.Context, source *unstructured.Unstructured, cause string) error { + name, deleted, err := r.deleteGeneratedPolicy(ctx, source.GroupVersionKind(), source.GetNamespace(), source.GetName()) + if err != nil { + klog.ErrorS(err, "Failed to delete the generated placement policy", "obj", klog.KObj(source), "policy", klog.KRef(source.GetNamespace(), name)) + return err + } + if !deleted { + // The common case by far: a resource nobody annotated. + return nil + } + klog.V(2).InfoS("Deleted the generated placement policy", "obj", klog.KObj(source), "policy", klog.KRef(source.GetNamespace(), name)) + r.Recorder.Eventf(source, corev1.EventTypeNormal, EventReasonPolicyDeleted, "Deleted the %s %s because %s", generatedPolicyKind(source.GetNamespace()), name, cause) + return nil +} + +// deleteGeneratedPolicy deletes the policy generated for the given resource identity, reporting the +// policy's name and whether this pass performed the deletion. The name is returned so a caller that +// logs or records an event about the deletion need not derive it a second time. +// +// The policy is read through the uncached reader before it is deleted, so a deletion the policy watch +// delivered is seen even if a cache has yet to catch up; the read is confined to annotated resources, +// which are all that reach this controller's queue. +func (r *Reconciler) deleteGeneratedPolicy(ctx context.Context, gvk schema.GroupVersionKind, namespace, name string) (string, bool, error) { + actual := emptyPolicyForScope(namespace) + policyName := generatedPolicyName(gvk, namespace, name) + + err := r.UncachedReader.Get(ctx, client.ObjectKey{Namespace: namespace, Name: policyName}, actual) + switch { + case apierrors.IsNotFound(err): + return policyName, false, nil + case err != nil: + return policyName, false, controller.NewAPIServerError(true, err) + } + + if !isGeneratedFor(actual, gvk, name) { + // A policy occupies the generated name but this controller did not create it, so deleting it + // would destroy a user's or another tool's object. It is never this controller's to remove; + // the deletion is declined and reported as though nothing was there to delete. + klog.V(2).InfoS("A policy at the generated name was not generated by this controller; declining to delete it", "policy", klog.KRef(namespace, policyName)) + return policyName, false, nil + } + + // The delete carries the resource version the read returned as a precondition, so it removes only + // the exact object this pass read and confirmed was one it generated. Between that read and here + // the policy could be replaced -- deleted and a hand-authored one created at the same name, or + // overwritten in place with its provenance stripped -- and an unconditioned delete, which targets + // the name alone, would then remove whatever now sits there. The precondition turns that into a + // conflict instead; the conflict requeues, and the next pass reads the current object and declines + // it if it is no longer one this controller generated. + resourceVersion := actual.GetResourceVersion() + if err := r.Client.Delete(ctx, actual, client.Preconditions{ResourceVersion: &resourceVersion}); err != nil { + if apierrors.IsNotFound(err) { + // The read above raced a deletion that already happened -- typically this controller's + // own, re-entered through the generated policy watch moments later. Nothing was deleted + // here, and reporting otherwise would log, and on some paths announce to the user, a + // deletion that this pass did not perform. + return policyName, false, nil + } + return policyName, false, controller.NewAPIServerError(false, err) + } + return policyName, true, nil +} + +// applyDesiredPolicy brings a live generated policy in line with the desired one, reporting whether +// anything changed. +// +// Only what this controller generates is overwritten: the spec, the provenance labels, and the owner +// reference to the annotated resource. Labels and annotations that something else added are left +// alone, so that a generated policy can be labelled by an operator or a GitOps tool without this +// controller and that tool taking turns undoing each other. +func applyDesiredPolicy(actual, desired client.Object) bool { + changed := false + + actualSpec, desiredSpec := policySpec(actual), policySpec(desired) + if actualSpec == nil || desiredSpec == nil { + // Unreachable for the objects this package builds; treated as no change rather than a panic + // so that a future scope cannot take the reconcile loop down with it. + klog.ErrorS(fmt.Errorf("object of type %T is not a generated placement policy", actual), "Skipped updating an object that is not a placement policy") + return false + } + if !equality.Semantic.DeepEqual(actualSpec, desiredSpec) { + desiredSpec.DeepCopyInto(actualSpec) + changed = true + } + + labels := actual.GetLabels() + for key, value := range desired.GetLabels() { + if existing, found := labels[key]; found && existing == value { + continue + } + if labels == nil { + labels = make(map[string]string, len(desired.GetLabels())) + } + labels[key] = value + changed = true + } + if changed { + actual.SetLabels(labels) + } + + for _, owner := range desired.GetOwnerReferences() { + if ensureOwnerReference(actual, owner) { + changed = true + } + } + return changed +} + +// ensureOwnerReference adds the owner reference to the annotated resource if it is missing or has +// drifted, leaving any other owner reference in place, and reports whether it changed anything. +func ensureOwnerReference(policy client.Object, want metav1.OwnerReference) bool { + owners := policy.GetOwnerReferences() + for i := range owners { + if !sameOwnerIdentity(owners[i], want) { + continue + } + if equality.Semantic.DeepEqual(owners[i], want) { + return false + } + // Deep copied rather than assigned: an owner reference carries two pointer fields, and the + // live policy must not come away sharing them with the object the desired one was built from. + owners[i] = *want.DeepCopy() + policy.SetOwnerReferences(owners) + return true + } + policy.SetOwnerReferences(append(slices.Clone(owners), *want.DeepCopy())) + return true +} + +// sameOwnerIdentity reports whether two owner references name the same resource, matched on its +// group, kind, and name and deliberately not on its version or its UID. +// +// Both of those can change while the resource keeps its name: a source deleted and recreated comes +// back with a new UID, and a served version can be retired. Matching on UID would then treat the +// recreated source as a stranger and append a second owner reference to it every reconcile, leaving +// the original's dangling one behind; matching on version would do the same across a version change. +// The group, kind, and name are what stay fixed, and a generated policy carries at most one owner +// reference for its source, so matching on them collapses onto that one reference as intended. +func sameOwnerIdentity(a, b metav1.OwnerReference) bool { + return a.Kind == b.Kind && a.Name == b.Name && + schema.FromAPIVersionAndKind(a.APIVersion, a.Kind).Group == schema.FromAPIVersionAndKind(b.APIVersion, b.Kind).Group +} + +// HasClusterSelectorsAnnotation reports whether an object carries the annotation this controller +// acts on. It is what keeps every unrelated resource in the cluster out of this controller's queue, +// and so is called by the resource watcher on every event for every watched resource. +func HasClusterSelectorsAnnotation(object metav1.Object) bool { + _, found := object.GetAnnotations()[kfplacementv1alpha1.ClusterSelectorsAnnotation] + return found +} diff --git a/pkg/controllers/annotationplacement/controller_integration_test.go b/pkg/controllers/annotationplacement/controller_integration_test.go new file mode 100644 index 000000000..907c53510 --- /dev/null +++ b/pkg/controllers/annotationplacement/controller_integration_test.go @@ -0,0 +1,406 @@ +/* +Copyright 2026 The KubeFleet Authors. + +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 annotationplacement + +import ( + "fmt" + "strings" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + + kfplacementv1alpha1 "github.com/kubefleet-dev/kubefleet/apis/kubefleet.dev/placement/v1alpha1" +) + +const ( + eventuallyTimeout = "10s" + eventuallyInterval = "250ms" +) + +var configMapCount int + +// annotate sets, changes, or (with an empty value) removes the annotation on an object, and waits for +// the informer cache the reconciler reads from to catch up. +func annotate(object client.Object, value string) { + Expect(hubClient.Get(ctx, client.ObjectKeyFromObject(object), object)).Should(Succeed()) + annotations := object.GetAnnotations() + if annotations == nil { + annotations = map[string]string{} + } + if value == "" { + delete(annotations, kfplacementv1alpha1.ClusterSelectorsAnnotation) + } else { + annotations[kfplacementv1alpha1.ClusterSelectorsAnnotation] = value + } + object.SetAnnotations(annotations) + Expect(hubClient.Update(ctx, object)).Should(Succeed()) + + // The reconciler reads the annotated resource from the informer cache rather than from the API + // server, so a reconcile run before the cache catches up would act on the previous value. + gvr := configMapGVR + if object.GetNamespace() == "" { + gvr = namespaceGVR + } + Eventually(func() (string, error) { + cached, err := cachedObject(gvr, object) + if err != nil { + return "", err + } + return cached.GetAnnotations()[kfplacementv1alpha1.ClusterSelectorsAnnotation], nil + }, eventuallyTimeout, eventuallyInterval).Should(Equal(value), "the informer cache never caught up with the annotation") +} + +// waitForCache blocks until the informer cache the reconciler reads from has observed an object. +// +// Without it, a reconcile can run against a cache that has not caught up, where a resource that is +// merely not yet visible is indistinguishable from one that carries no annotation: both leave no +// generated policy behind, so an assertion that none exists would hold either way. +func waitForCache(gvr schema.GroupVersionResource, object client.Object) { + Eventually(func() error { + _, err := cachedObject(gvr, object) + return err + }, eventuallyTimeout, eventuallyInterval).Should(Succeed(), "the informer cache never observed the resource") +} + +// cachedObject reads an object out of the informer cache the reconciler uses. +func cachedObject(gvr schema.GroupVersionResource, object client.Object) (client.Object, error) { + lister := informerManager.Lister(gvr) + if object.GetNamespace() == "" { + cached, err := lister.Get(object.GetName()) + if err != nil { + return nil, err + } + return cached.(client.Object), nil + } + cached, err := lister.ByNamespace(object.GetNamespace()).Get(object.GetName()) + if err != nil { + return nil, err + } + return cached.(client.Object), nil +} + +// reconcile runs one pass of the reconciler over an object, as the resource watcher would. +func reconcile(gvk schema.GroupVersionKind, object client.Object) error { + _, err := reconciler.Reconcile(ctx, keyFor(gvk, object.GetNamespace(), object.GetName())) + return err +} + +// generatedPolicyFor reads back the policy generated for an object, whichever scope it has. +func generatedPolicyFor(gvk schema.GroupVersionKind, object client.Object) (client.Object, error) { + namespace := object.GetNamespace() + policy := emptyPolicyForScope(namespace) + name := generatedPolicyName(gvk, namespace, object.GetName()) + err := hubClient.Get(ctx, types.NamespacedName{Namespace: namespace, Name: name}, policy) + return policy, err +} + +// drainEvents returns the reasons of every event recorded so far, clearing the recorder. +func drainEvents() []string { + reasons := []string{} + for { + select { + case event := <-eventRecorder.Events: + fields := strings.SplitN(event, " ", 3) + if len(fields) >= 2 { + reasons = append(reasons, fields[1]) + } + default: + return reasons + } + } +} + +// The API server itself enforces the count bounds on both forms of the int-or-string: the Pattern +// marker covers the string form, and the CEL rule covers the integer form, which a pattern alone +// leaves unbounded. The parser mirrors the same bounds for annotations; these specs pin the API +// side against a real API server, where a plain schema reading cannot. +var _ = Describe("the count bounds of a hand-authored policy", func() { + newPolicy := func(count intstr.IntOrString) *kfplacementv1alpha1.PlacementPolicy { + configMapCount++ + return &kfplacementv1alpha1.PlacementPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("hand-authored-%d", configMapCount), + Namespace: "default", + }, + Spec: kfplacementv1alpha1.PlacementPolicySpec{ + ClusterSelectors: []kfplacementv1alpha1.ClusterSelector{{Count: &count}}, + ResourceSelectors: []kfplacementv1alpha1.ResourceSelector{{ + APIVersion: "v1", Kind: "ConfigMap", Name: "app", + }}, + }, + } + } + + DescribeTable("integer and string forms share the same bounds", + func(count intstr.IntOrString, wantAccepted bool) { + policy := newPolicy(count) + err := hubClient.Create(ctx, policy) + if err == nil { + // Whatever the verdict was meant to be, an object that made it in must not + // outlive the spec; a regression that admits an out-of-range count would + // otherwise leave its evidence lying around for the rest of the suite. + DeferCleanup(func() { + Expect(client.IgnoreNotFound(hubClient.Delete(ctx, policy))).Should(Succeed()) + }) + } + if wantAccepted { + Expect(err).Should(Succeed()) + return + } + Expect(apierrors.IsInvalid(err)).Should(BeTrue(), "got %v, want an invalid error", err) + }, + Entry("count 1 as an integer is accepted", intstr.FromInt32(1), true), + Entry("count 999 as an integer is accepted", intstr.FromInt32(999), true), + Entry("count 999 as a string is accepted", intstr.FromString("999"), true), + Entry("count All is accepted", intstr.FromString("All"), true), + // The integer entries below are the reason the CEL rule exists: before it, they were + // accepted while their quoted twins were rejected. + Entry("count 1000 as an integer is rejected", intstr.FromInt32(1000), false), + Entry("count 0 as an integer is rejected", intstr.FromInt32(0), false), + Entry("count -1 as an integer is rejected", intstr.FromInt32(-1), false), + Entry("count 1000 as a string is rejected", intstr.FromString("1000"), false), + Entry("count 0 as a string is rejected", intstr.FromString("0"), false), + ) + + // A digit string too long for an int64 used to reach the int() conversion inside the + // minCount<=count rule, adding an opaque evaluation error beside the length violation. The + // count field's own validation must be the whole story now: MaxLength rejects the string + // first, and the rule's digit guard keeps int() out of reach regardless. + It("should report an over-long count with the count field's own message alone", func() { + policy := newPolicy(intstr.FromString("99999999999999999999")) + policySpec(policy).ClusterSelectors[0].MinCount = ptr.To(int32(1)) + + err := hubClient.Create(ctx, policy) + Expect(apierrors.IsInvalid(err)).Should(BeTrue(), "got %v, want an invalid error", err) + Expect(err.Error()).Should(ContainSubstring("Too long"), "the count field's length bound must report the malformed count") + Expect(err.Error()).ShouldNot(ContainSubstring("minCount"), "the minCount rule must stay out of a problem that is not its own") + }) +}) + +var _ = Describe("annotation based placement", func() { + Context("a namespaced resource", Ordered, func() { + var configMap *corev1.ConfigMap + + BeforeAll(func() { + configMapCount++ + configMap = &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("app-%d", configMapCount), + Namespace: "default", + }, + } + Expect(hubClient.Create(ctx, configMap)).Should(Succeed()) + waitForCache(configMapGVR, configMap) + drainEvents() + }) + + AfterAll(func() { + Expect(client.IgnoreNotFound(hubClient.Delete(ctx, configMap))).Should(Succeed()) + }) + + It("should generate no policy while the resource carries no annotation", func() { + Expect(reconcile(configMapGVK, configMap)).Should(Succeed()) + _, err := generatedPolicyFor(configMapGVK, configMap) + Expect(apierrors.IsNotFound(err)).Should(BeTrue(), "got %v, want a not found error", err) + Expect(drainEvents()).Should(BeEmpty()) + }) + + It("should generate a placement policy when the annotation is set", func() { + annotate(configMap, "env=staging,count=All") + Expect(reconcile(configMapGVK, configMap)).Should(Succeed()) + + policy, err := generatedPolicyFor(configMapGVK, configMap) + Expect(err).Should(Succeed()) + + spec := policySpec(policy) + Expect(spec.ClusterSelectors).Should(HaveLen(1)) + Expect(spec.ClusterSelectors[0].Count).Should(Equal(ptrIntOrString(countAll))) + Expect(spec.ClusterSelectors[0].Terms[0].MatchLabels).Should(Equal(map[string]string{"env": "staging"})) + Expect(spec.ResourceSelectors).Should(Equal([]kfplacementv1alpha1.ResourceSelector{{ + APIGroup: "", + APIVersion: "v1", + Kind: "ConfigMap", + Name: configMap.Name, + }})) + + Expect(policy.GetNamespace()).Should(Equal(configMap.Namespace), "a namespaced resource must generate a policy in its own namespace") + Expect(policy.GetLabels()).Should(HaveKeyWithValue(kfplacementv1alpha1.ParentKindLabel, "ConfigMap")) + Expect(policy.GetLabels()).Should(HaveKeyWithValue(kfplacementv1alpha1.ParentNameLabel, configMap.Name)) + Expect(policy.GetLabels()).Should(HaveKeyWithValue(kfplacementv1alpha1.ParentAPIGroupLabel, "")) + + Expect(policy.GetOwnerReferences()).Should(HaveLen(1)) + owner := policy.GetOwnerReferences()[0] + Expect(owner.UID).Should(Equal(configMap.UID)) + Expect(owner.Controller).Should(BeNil(), "a generated policy must not claim controller ownership") + Expect(owner.BlockOwnerDeletion).Should(BeNil(), "a generated policy must not block deletion of the resource it came from") + + Expect(drainEvents()).Should(Equal([]string{EventReasonPolicyCreated})) + }) + + It("should not touch the policy when nothing changed", func() { + before, err := generatedPolicyFor(configMapGVK, configMap) + Expect(err).Should(Succeed()) + + Expect(reconcile(configMapGVK, configMap)).Should(Succeed()) + + after, err := generatedPolicyFor(configMapGVK, configMap) + Expect(err).Should(Succeed()) + // A resource version that moved means the reconciler wrote to the API server without + // anything having changed, which every pass would then repeat. + Expect(after.GetResourceVersion()).Should(Equal(before.GetResourceVersion())) + Expect(drainEvents()).Should(BeEmpty()) + }) + + It("should update the policy when the annotation changes", func() { + annotate(configMap, "env=canary,region=eastus,count=3") + Expect(reconcile(configMapGVK, configMap)).Should(Succeed()) + + policy, err := generatedPolicyFor(configMapGVK, configMap) + Expect(err).Should(Succeed()) + spec := policySpec(policy) + Expect(spec.ClusterSelectors).Should(HaveLen(1)) + Expect(spec.ClusterSelectors[0].Count).Should(Equal(ptrIntOrString("3"))) + Expect(spec.ClusterSelectors[0].Terms[0].MatchLabels).Should(Equal(map[string]string{ + "env": "canary", + corev1.LabelTopologyRegion: "eastus", + })) + Expect(drainEvents()).Should(Equal([]string{EventReasonPolicyUpdated})) + }) + + It("should restore the policy when someone edits it", func() { + policy, err := generatedPolicyFor(configMapGVK, configMap) + Expect(err).Should(Succeed()) + policySpec(policy).ClusterSelectors = nil + Expect(hubClient.Update(ctx, policy)).Should(Succeed()) + + // In the running agent this reconcile is triggered by the watch on the generated + // policies themselves; an edit produces no event on the ConfigMap. + Expect(reconcile(configMapGVK, configMap)).Should(Succeed()) + + restored, err := generatedPolicyFor(configMapGVK, configMap) + Expect(err).Should(Succeed()) + Expect(policySpec(restored).ClusterSelectors).Should(HaveLen(1)) + Expect(drainEvents()).Should(Equal([]string{EventReasonPolicyUpdated})) + }) + + It("should recreate the policy when someone deletes it", func() { + policy, err := generatedPolicyFor(configMapGVK, configMap) + Expect(err).Should(Succeed()) + Expect(hubClient.Delete(ctx, policy)).Should(Succeed()) + + Expect(reconcile(configMapGVK, configMap)).Should(Succeed()) + + _, err = generatedPolicyFor(configMapGVK, configMap) + Expect(err).Should(Succeed(), "the deleted policy must be generated again") + Expect(drainEvents()).Should(Equal([]string{EventReasonPolicyCreated})) + }) + + It("should keep the policy and warn when the annotation becomes invalid", func() { + annotate(configMap, "env") + Expect(reconcile(configMapGVK, configMap)).Should(Succeed(), "a malformed annotation must not be retried") + + _, err := generatedPolicyFor(configMapGVK, configMap) + Expect(err).Should(Succeed(), "the policy from the last valid annotation must be left in place") + Expect(drainEvents()).Should(Equal([]string{EventReasonInvalidAnnotation})) + }) + + It("should delete the policy when the annotation is removed", func() { + annotate(configMap, "") + Expect(reconcile(configMapGVK, configMap)).Should(Succeed()) + + _, err := generatedPolicyFor(configMapGVK, configMap) + Expect(apierrors.IsNotFound(err)).Should(BeTrue(), "got %v, want a not found error", err) + Expect(drainEvents()).Should(Equal([]string{EventReasonPolicyDeleted})) + }) + + It("should delete the policy once the resource itself is gone", func() { + // Deleting explicitly, rather than leaving the policy to garbage collection, is what + // keeps an owner reference some other party added from holding the policy up forever. + // It also means envtest, which runs no garbage collector, can observe the cleanup. + annotate(configMap, "env=staging") + Expect(reconcile(configMapGVK, configMap)).Should(Succeed()) + _, err := generatedPolicyFor(configMapGVK, configMap) + Expect(err).Should(Succeed()) + drainEvents() + + Expect(hubClient.Delete(ctx, configMap)).Should(Succeed()) + Eventually(func() error { + _, err := cachedObject(configMapGVR, configMap) + return err + }, eventuallyTimeout, eventuallyInterval).ShouldNot(Succeed(), "the informer cache never observed the deletion") + + Expect(reconcile(configMapGVK, configMap)).Should(Succeed()) + _, err = generatedPolicyFor(configMapGVK, configMap) + Expect(apierrors.IsNotFound(err)).Should(BeTrue(), "got %v, want a not found error", err) + // No event: the resource an event would be recorded on no longer exists. + Expect(drainEvents()).Should(BeEmpty()) + }) + }) + + Context("a cluster scoped resource", Ordered, func() { + var namespace *corev1.Namespace + + BeforeAll(func() { + configMapCount++ + namespace = &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("team-%d", configMapCount)}, + } + Expect(hubClient.Create(ctx, namespace)).Should(Succeed()) + waitForCache(namespaceGVR, namespace) + drainEvents() + }) + + AfterAll(func() { + Expect(client.IgnoreNotFound(hubClient.Delete(ctx, namespace))).Should(Succeed()) + }) + + It("should generate a cluster scoped policy", func() { + annotate(namespace, "env=staging") + Expect(reconcile(namespaceGVK, namespace)).Should(Succeed()) + + policy, err := generatedPolicyFor(namespaceGVK, namespace) + Expect(err).Should(Succeed()) + Expect(policy).Should(BeAssignableToTypeOf(&kfplacementv1alpha1.ClusterPlacementPolicy{}), + "a cluster scoped resource must generate a ClusterPlacementPolicy, since a namespaced policy owned by it would never be collected") + Expect(policy.GetNamespace()).Should(BeEmpty()) + Expect(policySpec(policy).ClusterSelectors).Should(HaveLen(1)) + Expect(drainEvents()).Should(Equal([]string{EventReasonPolicyCreated})) + }) + + It("should delete the cluster scoped policy when the annotation is removed", func() { + annotate(namespace, "") + Expect(reconcile(namespaceGVK, namespace)).Should(Succeed()) + + _, err := generatedPolicyFor(namespaceGVK, namespace) + Expect(apierrors.IsNotFound(err)).Should(BeTrue(), "got %v, want a not found error", err) + Expect(drainEvents()).Should(Equal([]string{EventReasonPolicyDeleted})) + }) + }) +}) + +func ptrIntOrString(value string) *intstr.IntOrString { + parsed := intstr.Parse(value) + return &parsed +} diff --git a/pkg/controllers/annotationplacement/controller_test.go b/pkg/controllers/annotationplacement/controller_test.go new file mode 100644 index 000000000..aa06f2b74 --- /dev/null +++ b/pkg/controllers/annotationplacement/controller_test.go @@ -0,0 +1,1189 @@ +/* +Copyright 2026 The KubeFleet Authors. + +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 annotationplacement + +import ( + "context" + "errors" + "slices" + "strings" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/tools/record" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + kfplacementv1alpha1 "github.com/kubefleet-dev/kubefleet/apis/kubefleet.dev/placement/v1alpha1" + placementv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1" + "github.com/kubefleet-dev/kubefleet/pkg/utils/controller" + "github.com/kubefleet-dev/kubefleet/pkg/utils/keys" + testinformer "github.com/kubefleet-dev/kubefleet/test/utils/informer" +) + +var ( + deploymentGVR = schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"} + namespaceGVR = schema.GroupVersionResource{Group: "", Version: "v1", Resource: "namespaces"} +) + +const ( + testNamespace = "prod" + testName = "web" + + oneSelector = "env=staging" + twoSelectors = "env=staging,count=All;env=canary,region=eastus" +) + +// newSource builds the annotated resource as the informer cache would hold it. +func newSource(gvk schema.GroupVersionKind, namespace, name string, annotations map[string]string) *unstructured.Unstructured { + source := sourceObject(gvk, namespace, name) + if annotations != nil { + source.SetAnnotations(annotations) + } + return source +} + +// keyFor builds the queue key the resource watcher would enqueue for a resource. +func keyFor(gvk schema.GroupVersionKind, namespace, name string) keys.ClusterWideKey { + return keys.ClusterWideKey{ + ResourceIdentifier: placementv1beta1.ResourceIdentifier{ + Group: gvk.Group, + Version: gvk.Version, + Kind: gvk.Kind, + Namespace: namespace, + Name: name, + }, + } +} + +func newScheme(t *testing.T) *runtime.Scheme { + t.Helper() + scheme := runtime.NewScheme() + if err := kfplacementv1alpha1.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme() = %v, want no error", err) + } + return scheme +} + +func newRESTMapper() meta.RESTMapper { + mapper := meta.NewDefaultRESTMapper([]schema.GroupVersion{deploymentGVK.GroupVersion(), namespaceGVK.GroupVersion()}) + mapper.AddSpecific(deploymentGVK, deploymentGVR, deploymentGVR, meta.RESTScopeNamespace) + mapper.AddSpecific(namespaceGVK, namespaceGVR, namespaceGVR, meta.RESTScopeRoot) + return mapper +} + +// newReconciler wires a reconciler whose informer cache holds the given resources and whose client +// holds the given policies. +func newReconciler(t *testing.T, sources map[schema.GroupVersionResource][]runtime.Object, clusterScoped []schema.GroupVersionKind, funcs interceptor.Funcs, policies ...client.Object) (*Reconciler, *record.FakeRecorder) { + t.Helper() + listers := make(map[schema.GroupVersionResource]*testinformer.FakeLister, len(sources)) + for gvr, objects := range sources { + listers[gvr] = &testinformer.FakeLister{Objects: objects} + } + apiResources := make(map[schema.GroupVersionKind]bool, len(clusterScoped)) + for _, gvk := range clusterScoped { + apiResources[gvk] = true + } + recorder := record.NewFakeRecorder(10) + // One fake client backs both the writer and the uncached reader: with a single store the two are + // always consistent, which is what every test that is not exercising cache staleness wants. A test + // that needs the reader to diverge from a stale cache builds its own reconciler with two stores. + c := fake.NewClientBuilder().WithScheme(newScheme(t)).WithObjects(policies...).WithInterceptorFuncs(funcs).Build() + return &Reconciler{ + Client: c, + UncachedReader: c, + RestMapper: newRESTMapper(), + InformerManager: &testinformer.FakeManager{ + APIResources: apiResources, + IsClusterScopedResource: true, + Listers: listers, + }, + Recorder: recorder, + }, recorder +} + +// recordedReasons drains the recorder and returns the reason of every event it holds. +func recordedReasons(recorder *record.FakeRecorder) []string { + reasons := make([]string, 0, len(recorder.Events)) + for { + select { + case event := <-recorder.Events: + // A recorded event reads "Normal ". + fields := strings.SplitN(event, " ", 3) + if len(fields) < 2 { + reasons = append(reasons, event) + continue + } + reasons = append(reasons, fields[1]) + default: + return reasons + } + } +} + +// policyFrom reads the generated policy for a resource back out of the client, or reports that none +// exists. +func policyFrom(ctx context.Context, t *testing.T, r *Reconciler, source *unstructured.Unstructured) (client.Object, bool) { + t.Helper() + namespace := source.GetNamespace() + policy := emptyPolicyForScope(namespace) + name := generatedPolicyName(source.GroupVersionKind(), namespace, source.GetName()) + err := r.Client.Get(ctx, client.ObjectKey{Namespace: namespace, Name: name}, policy) + switch { + case apierrors.IsNotFound(err): + return nil, false + case err != nil: + t.Fatalf("Get(%s) = %v, want no error", name, err) + } + return policy, true +} + +// policyIgnoreOpts drop the fields the API server owns, which no expectation here can predict. +var policyIgnoreOpts = cmp.Options{ + cmpopts.IgnoreFields(metav1.TypeMeta{}, "Kind", "APIVersion"), + cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion", "Generation", "CreationTimestamp", "ManagedFields"), +} + +func TestReconcile(t *testing.T) { + // The desired policy is built with desiredPolicy, which policy_test.go covers on its own; what + // is under test here is whether the reconciler puts that object in the cluster, takes it away + // again, and says so. + annotatedSource := newSource(deploymentGVK, testNamespace, testName, map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: oneSelector}) + bareSource := newSource(deploymentGVK, testNamespace, testName, nil) + clusterScopedSource := newSource(namespaceGVK, "", testName, map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: oneSelector}) + + testCases := []struct { + name string + // source is the resource the informer cache holds, and the resource the key points at. + source *unstructured.Unstructured + // existing is the annotation value a policy was already generated from, if any. + existing string + gvr schema.GroupVersionResource + wantPolicy bool + wantValue string + wantReasons []string + }{ + { + name: "an annotated resource with no policy yet gets one", + source: annotatedSource, + gvr: deploymentGVR, + wantPolicy: true, + wantValue: oneSelector, + wantReasons: []string{EventReasonPolicyCreated}, + }, + { + name: "a policy that already matches the annotation is left alone", + source: annotatedSource, + existing: oneSelector, + gvr: deploymentGVR, + wantPolicy: true, + wantValue: oneSelector, + wantReasons: nil, + }, + { + name: "a changed annotation reaches the policy", + source: newSource(deploymentGVK, testNamespace, testName, map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: twoSelectors}), + existing: oneSelector, + gvr: deploymentGVR, + wantPolicy: true, + wantValue: twoSelectors, + wantReasons: []string{EventReasonPolicyUpdated}, + }, + { + name: "removing the annotation deletes the policy", + source: bareSource, + existing: oneSelector, + gvr: deploymentGVR, + wantPolicy: false, + wantReasons: []string{EventReasonPolicyDeleted}, + }, + { + name: "a resource that was never annotated is left alone", + source: bareSource, + gvr: deploymentGVR, + wantPolicy: false, + wantReasons: nil, + }, + { + name: "a cluster scoped resource generates a cluster scoped policy", + source: clusterScopedSource, + gvr: namespaceGVR, + wantPolicy: true, + wantValue: oneSelector, + wantReasons: []string{EventReasonPolicyCreated}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + var policies []client.Object + if tc.existing != "" { + selectors, err := parseClusterSelectors(tc.existing) + if err != nil { + t.Fatalf("parseClusterSelectors(%q) = %v, want no error", tc.existing, err) + } + policies = append(policies, desiredPolicy(tc.source, selectors)) + } + clusterScoped := []schema.GroupVersionKind{namespaceGVK} + r, recorder := newReconciler(t, map[schema.GroupVersionResource][]runtime.Object{tc.gvr: {tc.source}}, clusterScoped, interceptor.Funcs{}, policies...) + + key := keyFor(tc.source.GroupVersionKind(), tc.source.GetNamespace(), tc.source.GetName()) + if _, err := r.Reconcile(ctx, key); err != nil { + t.Fatalf("Reconcile(%v) = %v, want no error", key, err) + } + + got, found := policyFrom(ctx, t, r, tc.source) + if found != tc.wantPolicy { + t.Fatalf("Reconcile(%v) left a generated policy = %v, want %v", key, found, tc.wantPolicy) + } + if tc.wantPolicy { + selectors, err := parseClusterSelectors(tc.wantValue) + if err != nil { + t.Fatalf("parseClusterSelectors(%q) = %v, want no error", tc.wantValue, err) + } + want := desiredPolicy(tc.source, selectors) + if diff := cmp.Diff(got, want, policyIgnoreOpts); diff != "" { + t.Errorf("Reconcile(%v) generated policy mismatch (-got, +want):\n%s", key, diff) + } + } + if diff := cmp.Diff(recordedReasons(recorder), tc.wantReasons, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("Reconcile(%v) recorded events mismatch (-got, +want):\n%s", key, diff) + } + }) + } +} + +// TestReconcileInvalidAnnotation covers the case the parser rejects: the user hears about it, the +// key is not retried, and a policy generated from an earlier valid annotation stays up. +func TestReconcileInvalidAnnotation(t *testing.T) { + ctx := context.Background() + valid := newSource(deploymentGVK, testNamespace, testName, map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: oneSelector}) + selectors, err := parseClusterSelectors(oneSelector) + if err != nil { + t.Fatalf("parseClusterSelectors(%q) = %v, want no error", oneSelector, err) + } + existing := desiredPolicy(valid, selectors) + + broken := newSource(deploymentGVK, testNamespace, testName, map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: "env"}) + r, recorder := newReconciler(t, map[schema.GroupVersionResource][]runtime.Object{deploymentGVR: {broken}}, nil, interceptor.Funcs{}, existing) + + key := keyFor(deploymentGVK, testNamespace, testName) + if _, err := r.Reconcile(ctx, key); err != nil { + t.Fatalf("Reconcile(%v) = %v, want no error, so that a malformed annotation is not retried", key, err) + } + + got, found := policyFrom(ctx, t, r, broken) + if !found { + t.Fatalf("Reconcile(%v) removed the policy generated from the previous annotation, want it left in place", key) + } + if diff := cmp.Diff(got, existing, policyIgnoreOpts); diff != "" { + t.Errorf("Reconcile(%v) changed the policy generated from the previous annotation (-got, +want):\n%s", key, diff) + } + if diff := cmp.Diff(recordedReasons(recorder), []string{EventReasonInvalidAnnotation}); diff != "" { + t.Errorf("Reconcile(%v) recorded events mismatch (-got, +want):\n%s", key, diff) + } +} + +// TestReconcileDeletedResource covers a key for a resource that is already gone. The reconciler +// deletes the generated policy itself rather than deferring to garbage collection, which removes a +// dependent only once every owner reference on it is gone -- and the merge deliberately preserves +// owner references that other parties added, any live one of which would keep the policy standing. +// No event is recorded, since the resource an event would be recorded on no longer exists. +func TestReconcileDeletedResource(t *testing.T) { + ctx := context.Background() + source := newSource(deploymentGVK, testNamespace, testName, map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: oneSelector}) + selectors, err := parseClusterSelectors(oneSelector) + if err != nil { + t.Fatalf("parseClusterSelectors(%q) = %v, want no error", oneSelector, err) + } + existing := desiredPolicy(source, selectors) + // The other party whose owner reference would hold the policy back from garbage collection. + otherOwner := metav1.OwnerReference{APIVersion: "example.com/v1", Kind: "Widget", Name: "unrelated", UID: "00000000-0000-0000-0000-0000000000ff"} + existing.SetOwnerReferences(append(existing.GetOwnerReferences(), otherOwner)) + + r, recorder := newReconciler(t, map[schema.GroupVersionResource][]runtime.Object{deploymentGVR: {}}, nil, interceptor.Funcs{}, existing) + + key := keyFor(deploymentGVK, testNamespace, testName) + if _, err := r.Reconcile(ctx, key); err != nil { + t.Fatalf("Reconcile(%v) = %v, want no error", key, err) + } + if _, found := policyFrom(ctx, t, r, source); found { + t.Errorf("Reconcile(%v) left the generated policy standing, want it deleted", key) + } + if got := recordedReasons(recorder); len(got) != 0 { + t.Errorf("Reconcile(%v) recorded events = %v, want none", key, got) + } +} + +// TestReconcileIneligibleResource covers a resource that exists but fails the placement eligibility +// test -- for instance one in a skipped namespace, or a ReplicaSet that a Deployment has adopted. +// Its generated policy is deleted: the resource watcher reports such a resource as deleted and then +// never again, so this reconciliation is the last chance to clean up. +func TestReconcileIneligibleResource(t *testing.T) { + ctx := context.Background() + source := newSource(deploymentGVK, testNamespace, testName, map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: oneSelector}) + selectors, err := parseClusterSelectors(oneSelector) + if err != nil { + t.Fatalf("parseClusterSelectors(%q) = %v, want no error", oneSelector, err) + } + existing := desiredPolicy(source, selectors) + + r, recorder := newReconciler(t, map[schema.GroupVersionResource][]runtime.Object{deploymentGVR: {source}}, nil, interceptor.Funcs{}, existing) + r.ShouldPlace = func(*unstructured.Unstructured) (bool, error) { return false, nil } + + key := keyFor(deploymentGVK, testNamespace, testName) + if _, err := r.Reconcile(ctx, key); err != nil { + t.Fatalf("Reconcile(%v) = %v, want no error", key, err) + } + if _, found := policyFrom(ctx, t, r, source); found { + t.Errorf("Reconcile(%v) left the generated policy standing, want it deleted", key) + } + if diff := cmp.Diff(recordedReasons(recorder), []string{EventReasonPolicyDeleted}); diff != "" { + t.Errorf("Reconcile(%v) recorded events mismatch (-got, +want):\n%s", key, diff) + } + + // A second pass over a resource that never generated anything must stay silent. + if _, err := r.Reconcile(ctx, key); err != nil { + t.Fatalf("Reconcile(%v) = %v, want no error", key, err) + } + if got := recordedReasons(recorder); len(got) != 0 { + t.Errorf("Reconcile(%v) recorded events = %v, want none on the second pass", key, got) + } +} + +// TestReconcileEligibilityError covers the eligibility test itself failing, which must surface as a +// retryable error rather than as either verdict. +func TestReconcileEligibilityError(t *testing.T) { + source := newSource(deploymentGVK, testNamespace, testName, map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: oneSelector}) + r, _ := newReconciler(t, map[schema.GroupVersionResource][]runtime.Object{deploymentGVR: {source}}, nil, interceptor.Funcs{}) + wantErr := errors.New("the eligibility test is unwell") + r.ShouldPlace = func(*unstructured.Unstructured) (bool, error) { return false, wantErr } + + key := keyFor(deploymentGVK, testNamespace, testName) + if _, err := r.Reconcile(context.Background(), key); !errors.Is(err, wantErr) { + t.Errorf("Reconcile(%v) = %v, want an error wrapping %v", key, err, wantErr) + } + if _, found := policyFrom(context.Background(), t, r, source); found { + t.Errorf("Reconcile(%v) acted on the policy despite the eligibility error", key) + } +} + +func TestReconcileErrors(t *testing.T) { + source := newSource(deploymentGVK, testNamespace, testName, map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: oneSelector}) + + testCases := []struct { + name string + key controller.QueueKey + synced *bool + wantErr error + wantKind string + }{ + { + name: "a key of the wrong type cannot be retried", + key: "apps/v1/Deployment/prod/web", + wantErr: controller.ErrUnexpectedBehavior, + }, + + { + name: "an unsynced informer is retried", + key: keyFor(deploymentGVK, testNamespace, testName), + synced: ptr.To(false), + wantErr: controller.ErrExpectedBehavior, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + r, _ := newReconciler(t, map[schema.GroupVersionResource][]runtime.Object{deploymentGVR: {source}}, nil, interceptor.Funcs{}) + if tc.synced != nil { + r.InformerManager.(*testinformer.FakeManager).InformerSynced = tc.synced + } + _, err := r.Reconcile(context.Background(), tc.key) + if err == nil { + t.Fatalf("Reconcile(%v) = nil, want an error", tc.key) + } + if !errors.Is(err, tc.wantErr) { + t.Errorf("Reconcile(%v) = %v, want an error of kind %v", tc.key, err, tc.wantErr) + } + }) + } +} + +// TestReconcileAPIServerErrors covers the paths where the write itself fails. Each has to come back +// as a retryable error rather than as a silent success, because the annotation and the cluster have +// disagreed at that point and only another pass can settle it. +func TestReconcileAPIServerErrors(t *testing.T) { + annotated := newSource(deploymentGVK, testNamespace, testName, map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: oneSelector}) + bare := newSource(deploymentGVK, testNamespace, testName, nil) + selectors, err := parseClusterSelectors(oneSelector) + if err != nil { + t.Fatalf("parseClusterSelectors(%q) = %v, want no error", oneSelector, err) + } + existing := desiredPolicy(annotated, selectors) + failure := apierrors.NewInternalError(errors.New("the api server is unwell")) + + testCases := []struct { + name string + source *unstructured.Unstructured + existing []client.Object + interceptor interceptor.Funcs + }{ + { + name: "the policy cannot be read", + source: annotated, + interceptor: interceptor.Funcs{ + Get: func(context.Context, client.WithWatch, client.ObjectKey, client.Object, ...client.GetOption) error { + return failure + }, + }, + }, + { + name: "the policy cannot be created", + source: annotated, + interceptor: interceptor.Funcs{ + Create: func(context.Context, client.WithWatch, client.Object, ...client.CreateOption) error { + return failure + }, + }, + }, + { + name: "the policy cannot be updated", + source: newSource(deploymentGVK, testNamespace, testName, map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: twoSelectors}), + existing: []client.Object{existing}, + interceptor: interceptor.Funcs{ + Update: func(context.Context, client.WithWatch, client.Object, ...client.UpdateOption) error { + return failure + }, + }, + }, + { + name: "the policy cannot be deleted", + source: bare, + existing: []client.Object{existing}, + interceptor: interceptor.Funcs{ + Delete: func(context.Context, client.WithWatch, client.Object, ...client.DeleteOption) error { + return failure + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + r, recorder := newReconciler(t, map[schema.GroupVersionResource][]runtime.Object{deploymentGVR: {tc.source}}, nil, tc.interceptor, tc.existing...) + + key := keyFor(deploymentGVK, testNamespace, testName) + _, err := r.Reconcile(context.Background(), key) + if err == nil { + t.Fatalf("Reconcile(%v) = nil, want an error", key) + } + if !errors.Is(err, controller.ErrAPIServerError) { + t.Errorf("Reconcile(%v) = %v, want an error of kind %v", key, err, controller.ErrAPIServerError) + } + // A write that failed must not be announced as though it had happened. + if got := recordedReasons(recorder); len(got) != 0 { + t.Errorf("Reconcile(%v) recorded events = %v, want none", key, got) + } + }) + } +} + +// TestDeleteRacesAnotherDeletion covers a cached read handing the reconciler a policy that is +// already gone by the time it deletes -- typically its own deletion re-entered through the +// generated policy watch. The pass must not claim, in logs or events, a deletion it did not do. +func TestDeleteRacesAnotherDeletion(t *testing.T) { + ctx := context.Background() + source := newSource(deploymentGVK, testNamespace, testName, nil) + annotated := newSource(deploymentGVK, testNamespace, testName, map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: oneSelector}) + selectors, err := parseClusterSelectors(oneSelector) + if err != nil { + t.Fatalf("parseClusterSelectors(%q) = %v, want no error", oneSelector, err) + } + existing := desiredPolicy(annotated, selectors) + + // The interceptor stands in for the stale cache: the Get finds the policy, the Delete + // discovers someone else got there first. + raced := interceptor.Funcs{ + Delete: func(context.Context, client.WithWatch, client.Object, ...client.DeleteOption) error { + return apierrors.NewNotFound(schema.GroupResource{Group: kfplacementv1alpha1.GroupVersion.Group, Resource: "placementpolicies"}, existing.GetName()) + }, + } + r, recorder := newReconciler(t, map[schema.GroupVersionResource][]runtime.Object{deploymentGVR: {source}}, nil, raced, existing) + + key := keyFor(deploymentGVK, testNamespace, testName) + if _, err := r.Reconcile(ctx, key); err != nil { + t.Fatalf("Reconcile(%v) = %v, want no error", key, err) + } + if got := recordedReasons(recorder); len(got) != 0 { + t.Errorf("Reconcile(%v) recorded events = %v, want none for a deletion this pass did not perform", key, got) + } +} + +// TestReconcileDoesNotDeleteAReplacementPolicy covers the window between the read that confirms a +// policy is this controller's and the delete that removes it. If the policy is replaced in that +// window -- overwritten with its provenance stripped, or deleted and a hand-authored one created at +// the same name -- an unconditioned delete would remove the replacement, since it targets the name +// alone. The resource-version precondition must turn that into a conflict, and the retry must then +// read the current object and decline it. +func TestReconcileDoesNotDeleteAReplacementPolicy(t *testing.T) { + ctx := context.Background() + bare := newSource(deploymentGVK, testNamespace, testName, nil) + annotated := newSource(deploymentGVK, testNamespace, testName, map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: oneSelector}) + selectors, err := parseClusterSelectors(oneSelector) + if err != nil { + t.Fatalf("parseClusterSelectors(%q) = %v, want no error", oneSelector, err) + } + ours := desiredPolicy(annotated, selectors) + + // A spec no generated policy would carry, marking the object that takes the name. + foreignSpec := kfplacementv1alpha1.PlacementPolicySpec{ + ResourceSelectors: []kfplacementv1alpha1.ResourceSelector{{APIGroup: "example.com", APIVersion: "v1", Kind: "Widget", Name: "hand-authored"}}, + } + + replaced := false + raced := interceptor.Funcs{ + Delete: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + if !replaced { + replaced = true + // Stand in for the replacement: the live object loses its provenance and its resource + // version moves on, so it is no longer the one the precondition names. + live := &kfplacementv1alpha1.PlacementPolicy{} + if err := c.Get(ctx, client.ObjectKeyFromObject(obj), live); err != nil { + return err + } + live.SetLabels(nil) + live.SetOwnerReferences(nil) + live.Spec = foreignSpec + if err := c.Update(ctx, live); err != nil { + return err + } + } + return c.Delete(ctx, obj, opts...) + }, + } + r, recorder := newReconciler(t, map[schema.GroupVersionResource][]runtime.Object{deploymentGVR: {bare}}, nil, raced, ours) + + key := keyFor(deploymentGVK, testNamespace, testName) + // First pass: the policy is replaced after it is read, so the guarded delete conflicts and the + // pass returns a retryable error rather than removing the replacement. + if _, err := r.Reconcile(ctx, key); err == nil { + t.Fatalf("Reconcile(%v) = nil, want a conflict error so the delete of a replaced policy is retried", key) + } + got, found := policyFrom(ctx, t, r, bare) + if !found { + t.Fatalf("Reconcile(%v) deleted the replacement policy, want it left in place", key) + } + if diff := cmp.Diff(got.(*kfplacementv1alpha1.PlacementPolicy).Spec, foreignSpec); diff != "" { + t.Errorf("Reconcile(%v) changed the replacement policy (-got, +want):\n%s", key, diff) + } + + // Second pass: the reconciler reads the current, now foreign object, declines it, and records no + // deletion it did not perform. + if _, err := r.Reconcile(ctx, key); err != nil { + t.Fatalf("Reconcile(%v) = %v, want no error once the replacement is recognized as foreign", key, err) + } + if _, found := policyFrom(ctx, t, r, bare); !found { + t.Errorf("Reconcile(%v) deleted the foreign replacement on retry, want it left in place", key) + } + if got := recordedReasons(recorder); slices.Contains(got, EventReasonPolicyDeleted) { + t.Errorf("Reconcile(%v) recorded events = %v, want no deletion of a policy it did not generate", key, got) + } +} + +func TestApplyDesiredPolicy(t *testing.T) { + source := newSource(deploymentGVK, testNamespace, testName, map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: oneSelector}) + selectors, err := parseClusterSelectors(oneSelector) + if err != nil { + t.Fatalf("parseClusterSelectors(%q) = %v, want no error", oneSelector, err) + } + desired := desiredPolicy(source, selectors) + + otherOwner := metav1.OwnerReference{ + APIVersion: "example.com/v1", + Kind: "Widget", + Name: "unrelated", + UID: "00000000-0000-0000-0000-0000000000ff", + } + + testCases := []struct { + name string + // mutate turns the desired policy into the live one the reconciler would read back. + mutate func(client.Object) + wantChanged bool + // check asserts on whatever the merge is supposed to preserve. + check func(*testing.T, client.Object) + }{ + { + name: "an untouched policy needs no update", + mutate: func(client.Object) {}, + wantChanged: false, + }, + { + name: "a drifted spec is restored", + mutate: func(policy client.Object) { + policySpec(policy).ClusterSelectors = nil + }, + wantChanged: true, + check: func(t *testing.T, policy client.Object) { + if diff := cmp.Diff(policySpec(policy), policySpec(desired)); diff != "" { + t.Errorf("applyDesiredPolicy() spec mismatch (-got, +want):\n%s", diff) + } + }, + }, + { + name: "a missing provenance label is restored", + mutate: func(policy client.Object) { + labels := policy.GetLabels() + delete(labels, kfplacementv1alpha1.ParentKindLabel) + policy.SetLabels(labels) + }, + wantChanged: true, + check: func(t *testing.T, policy client.Object) { + if diff := cmp.Diff(policy.GetLabels(), desired.GetLabels()); diff != "" { + t.Errorf("applyDesiredPolicy() labels mismatch (-got, +want):\n%s", diff) + } + }, + }, + { + name: "a policy stripped of every label gets the provenance labels back", + mutate: func(policy client.Object) { + policy.SetLabels(nil) + }, + wantChanged: true, + check: func(t *testing.T, policy client.Object) { + if diff := cmp.Diff(policy.GetLabels(), desired.GetLabels()); diff != "" { + t.Errorf("applyDesiredPolicy() labels mismatch (-got, +want):\n%s", diff) + } + }, + }, + { + name: "labels added by something else are kept", + mutate: func(policy client.Object) { + labels := policy.GetLabels() + labels["example.com/managed-by"] = "gitops" + delete(labels, kfplacementv1alpha1.ParentKindLabel) + policy.SetLabels(labels) + }, + wantChanged: true, + check: func(t *testing.T, policy client.Object) { + if got := policy.GetLabels()["example.com/managed-by"]; got != "gitops" { + t.Errorf("applyDesiredPolicy() label example.com/managed-by = %q, want %q", got, "gitops") + } + }, + }, + { + name: "an owner reference that drifted is corrected in place", + mutate: func(policy client.Object) { + drifted := parentOwnerReference(source) + drifted.APIVersion = "apps/v1beta1" + policy.SetOwnerReferences([]metav1.OwnerReference{drifted}) + }, + wantChanged: true, + check: func(t *testing.T, policy client.Object) { + want := []metav1.OwnerReference{parentOwnerReference(source)} + if diff := cmp.Diff(policy.GetOwnerReferences(), want); diff != "" { + t.Errorf("applyDesiredPolicy() owner references mismatch (-got, +want):\n%s", diff) + } + }, + }, + { + name: "a missing owner reference is restored alongside any other", + mutate: func(policy client.Object) { + policy.SetOwnerReferences([]metav1.OwnerReference{otherOwner}) + }, + wantChanged: true, + check: func(t *testing.T, policy client.Object) { + want := []metav1.OwnerReference{otherOwner, parentOwnerReference(source)} + if diff := cmp.Diff(policy.GetOwnerReferences(), want); diff != "" { + t.Errorf("applyDesiredPolicy() owner references mismatch (-got, +want):\n%s", diff) + } + }, + }, + { + // The source was deleted and recreated under the same name, so its reference carries the + // old UID. It must be updated in place, not left dangling while a second one is appended -- + // otherwise the list grows by one on every such cycle. + name: "an owner reference left by a recreated source is replaced, not appended", + mutate: func(policy client.Object) { + stale := parentOwnerReference(source) + stale.UID = "00000000-0000-0000-0000-00000000dead" + policy.SetOwnerReferences([]metav1.OwnerReference{stale}) + }, + wantChanged: true, + check: func(t *testing.T, policy client.Object) { + want := []metav1.OwnerReference{parentOwnerReference(source)} + if diff := cmp.Diff(policy.GetOwnerReferences(), want); diff != "" { + t.Errorf("applyDesiredPolicy() owner references mismatch (-got, +want):\n%s", diff) + } + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + actual := desired.DeepCopyObject().(client.Object) + tc.mutate(actual) + + if got := applyDesiredPolicy(actual, desired); got != tc.wantChanged { + t.Errorf("applyDesiredPolicy() = %v, want %v", got, tc.wantChanged) + } + if tc.check != nil { + tc.check(t, actual) + } + // Whatever the merge did, a second pass over its own output must find nothing left to + // do; otherwise the reconciler would issue an update on every single pass. + if got := applyDesiredPolicy(actual, desired); got { + t.Errorf("applyDesiredPolicy() = true on the second pass, want false") + } + }) + } +} + +// TestEventMessagesNameTheGeneratedKind pins that an event's message names the kind actually +// generated: the two scopes generate different kinds, and a message claiming a PlacementPolicy for +// what is really a ClusterPlacementPolicy sends the user's kubectl to a resource that is not there. +func TestEventMessagesNameTheGeneratedKind(t *testing.T) { + testCases := []struct { + name string + source *unstructured.Unstructured + gvr schema.GroupVersionResource + wantKind string + }{ + { + name: "a namespaced source names PlacementPolicy", + source: newSource(deploymentGVK, testNamespace, testName, map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: oneSelector}), + gvr: deploymentGVR, + wantKind: "PlacementPolicy", + }, + { + name: "a cluster scoped source names ClusterPlacementPolicy", + source: newSource(namespaceGVK, "", testName, map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: oneSelector}), + gvr: namespaceGVR, + wantKind: "ClusterPlacementPolicy", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + r, recorder := newReconciler(t, map[schema.GroupVersionResource][]runtime.Object{tc.gvr: {tc.source}}, []schema.GroupVersionKind{namespaceGVK}, interceptor.Funcs{}) + + key := keyFor(tc.source.GroupVersionKind(), tc.source.GetNamespace(), tc.source.GetName()) + if _, err := r.Reconcile(context.Background(), key); err != nil { + t.Fatalf("Reconcile(%v) = %v, want no error", key, err) + } + + select { + case event := <-recorder.Events: + // The kind is matched with its surrounding spaces: "ClusterPlacementPolicy" + // contains "PlacementPolicy" as a bare substring, so an unanchored check would + // pass the namespaced case even if the wrong kind were named. + if !strings.Contains(event, " the "+tc.wantKind+" ") { + t.Errorf("Reconcile(%v) recorded event %q, want it to name the kind %q", key, event, tc.wantKind) + } + default: + t.Fatalf("Reconcile(%v) recorded no event, want one naming the kind %q", key, tc.wantKind) + } + }) + } +} + +// TestReconcileUnknownKind covers a key whose kind the API server does not know, which the +// generated policy watch can produce by enqueuing whatever a policy names as its owner. The kind +// being gone means the source's own CRD was removed, so any policy generated from it is stale and is +// deleted; when none exists the key is simply dropped, since no retry can make the kind exist and an +// error would keep it backing off forever. +func TestReconcileUnknownKind(t *testing.T) { + widgetGVK := schema.GroupVersionKind{Group: "example.com", Version: "v1", Kind: "Widget"} + // The resource whose kind is gone. It is only used to derive the generated policy the reconciler + // must find and delete; the informer and the REST mapper deliberately do not know its kind. + widget := newSource(widgetGVK, testNamespace, testName, map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: oneSelector}) + selectors, err := parseClusterSelectors(oneSelector) + if err != nil { + t.Fatalf("parseClusterSelectors(%q) = %v, want no error", oneSelector, err) + } + stale := desiredPolicy(widget, selectors) + + testCases := []struct { + name string + existing []client.Object + wantPolicy bool + }{ + {name: "no policy to clean up, key is dropped", existing: nil, wantPolicy: false}, + {name: "a stale policy left by the gone kind is deleted", existing: []client.Object{stale}, wantPolicy: false}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + r, recorder := newReconciler(t, map[schema.GroupVersionResource][]runtime.Object{}, nil, interceptor.Funcs{}, tc.existing...) + key := keyFor(widgetGVK, testNamespace, testName) + if _, err := r.Reconcile(context.Background(), key); err != nil { + t.Errorf("Reconcile(%v) = %v, want no error, so that a kind that does not exist is not retried", key, err) + } + if _, found := policyFrom(context.Background(), t, r, widget); found != tc.wantPolicy { + t.Errorf("Reconcile(%v) left a generated policy = %v, want %v", key, found, tc.wantPolicy) + } + // No event is recorded either way: the resource an event would attach to is gone. + if got := recordedReasons(recorder); len(got) != 0 { + t.Errorf("Reconcile(%v) recorded events = %v, want none", key, got) + } + }) + } +} + +// TestReconcileResolvesRemovedVersion covers a key whose recorded version is no longer served while +// the kind lives on under another. The generated policy watch can enqueue such a key, and treating +// the removed version as a gone kind would wrongly delete a policy that is still wanted; the +// reconciler falls back to the served version and keeps the policy in sync instead. +func TestReconcileResolvesRemovedVersion(t *testing.T) { + ctx := context.Background() + // The source is served under apps/v1, the version the REST mapper knows; the key arrives naming + // apps/v2, a version that has since been removed. + source := newSource(deploymentGVK, testNamespace, testName, map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: oneSelector}) + r, recorder := newReconciler(t, map[schema.GroupVersionResource][]runtime.Object{deploymentGVR: {source}}, nil, interceptor.Funcs{}) + + key := keyFor(schema.GroupVersionKind{Group: "apps", Version: "v2", Kind: "Deployment"}, testNamespace, testName) + if _, err := r.Reconcile(ctx, key); err != nil { + t.Fatalf("Reconcile(%v) = %v, want no error", key, err) + } + + if _, found := policyFrom(ctx, t, r, source); !found { + t.Errorf("Reconcile(%v) generated no policy, want the removed version resolved to the served one", key) + } + if diff := cmp.Diff(recordedReasons(recorder), []string{EventReasonPolicyCreated}); diff != "" { + t.Errorf("Reconcile(%v) recorded events mismatch (-got, +want):\n%s", key, diff) + } +} + +// TestReconcileForeignPolicyAtGeneratedName covers a policy that already occupies a resource's +// generated name but was authored by someone else -- it carries none of this controller's provenance +// labels. The controller must neither overwrite it when the annotation asks for a policy nor delete +// it when the annotation is removed; a bare name match would do both. +func TestReconcileForeignPolicyAtGeneratedName(t *testing.T) { + // A policy sitting at the generated name, distinguishable by a spec this controller would never + // produce and by the absence of the provenance labels. + foreignSpec := func() kfplacementv1alpha1.PlacementPolicySpec { + return kfplacementv1alpha1.PlacementPolicySpec{ + ResourceSelectors: []kfplacementv1alpha1.ResourceSelector{{APIGroup: "example.com", APIVersion: "v1", Kind: "Widget", Name: "hand-authored"}}, + } + } + newForeign := func() *kfplacementv1alpha1.PlacementPolicy { + return &kfplacementv1alpha1.PlacementPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: generatedPolicyName(deploymentGVK, testNamespace, testName), + Namespace: testNamespace, + }, + Spec: foreignSpec(), + } + } + + testCases := []struct { + name string + source *unstructured.Unstructured + wantReasons []string + }{ + { + name: "the annotation asks for a policy, the foreign one is not overwritten", + source: newSource(deploymentGVK, testNamespace, testName, map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: oneSelector}), + // The user hears why the placement they asked for is not running. + wantReasons: []string{EventReasonPolicyConflict}, + }, + { + name: "the annotation is absent, the foreign one is not deleted", + // Nothing asked for a policy here, so declining to delete a policy that was never this + // controller's is silent -- the same as a resource that never generated anything. + source: newSource(deploymentGVK, testNamespace, testName, nil), + wantReasons: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + r, recorder := newReconciler(t, map[schema.GroupVersionResource][]runtime.Object{deploymentGVR: {tc.source}}, nil, interceptor.Funcs{}, newForeign()) + + key := keyFor(deploymentGVK, testNamespace, testName) + if _, err := r.Reconcile(ctx, key); err != nil { + t.Fatalf("Reconcile(%v) = %v, want no error", key, err) + } + + got, found := policyFrom(ctx, t, r, tc.source) + if !found { + t.Fatalf("Reconcile(%v) removed the foreign policy, want it left in place", key) + } + gotSpec := got.(*kfplacementv1alpha1.PlacementPolicy).Spec + if diff := cmp.Diff(gotSpec, foreignSpec()); diff != "" { + t.Errorf("Reconcile(%v) changed the foreign policy's spec (-got, +want):\n%s", key, diff) + } + if labels := got.GetLabels(); len(labels) != 0 { + t.Errorf("Reconcile(%v) added labels %v to the foreign policy, want it left untouched", key, labels) + } + if diff := cmp.Diff(recordedReasons(recorder), tc.wantReasons, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("Reconcile(%v) recorded events mismatch (-got, +want):\n%s", key, diff) + } + }) + } +} + +// TestReconcileRepairsDriftedProvenance covers a policy this controller generated whose provenance +// labels were later edited away. Its owner reference still identifies it as ours, so it must be +// repaired while the annotation stands and deleted once the annotation is removed -- never stranded +// as though it were foreign, which would leave its placement running with nothing able to reconcile +// or remove it. +func TestReconcileRepairsDriftedProvenance(t *testing.T) { + annotated := newSource(deploymentGVK, testNamespace, testName, map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: oneSelector}) + selectors, err := parseClusterSelectors(oneSelector) + if err != nil { + t.Fatalf("parseClusterSelectors(%q) = %v, want no error", oneSelector, err) + } + // A generated policy whose provenance labels have drifted -- one label is gone -- while the owner + // reference this controller set is untouched, so the policy is still recognizable as ours. + drifted := func() client.Object { + policy := desiredPolicy(annotated, selectors) + labels := policy.GetLabels() + delete(labels, kfplacementv1alpha1.ParentKindLabel) + policy.SetLabels(labels) + return policy + } + + testCases := []struct { + name string + source *unstructured.Unstructured + wantPolicy bool + wantReason string + }{ + { + name: "the annotation stands, so the drifted labels are repaired", + source: annotated, + wantPolicy: true, + wantReason: EventReasonPolicyUpdated, + }, + { + name: "the annotation is gone, so the policy is deleted", + source: newSource(deploymentGVK, testNamespace, testName, nil), + wantPolicy: false, + wantReason: EventReasonPolicyDeleted, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + r, recorder := newReconciler(t, map[schema.GroupVersionResource][]runtime.Object{deploymentGVR: {tc.source}}, nil, interceptor.Funcs{}, drifted()) + + key := keyFor(deploymentGVK, testNamespace, testName) + if _, err := r.Reconcile(ctx, key); err != nil { + t.Fatalf("Reconcile(%v) = %v, want no error", key, err) + } + + got, found := policyFrom(ctx, t, r, annotated) + if found != tc.wantPolicy { + t.Fatalf("Reconcile(%v) left a generated policy = %v, want %v", key, found, tc.wantPolicy) + } + if tc.wantPolicy { + if diff := cmp.Diff(got.GetLabels(), desiredPolicy(annotated, selectors).GetLabels()); diff != "" { + t.Errorf("Reconcile(%v) did not restore the drifted provenance labels (-got, +want):\n%s", key, diff) + } + } + if diff := cmp.Diff(recordedReasons(recorder), []string{tc.wantReason}); diff != "" { + t.Errorf("Reconcile(%v) recorded events mismatch (-got, +want):\n%s", key, diff) + } + }) + } +} + +// TestReconcileResolvesRemovedVersionForClusterScoped covers a cluster-scoped source reached through +// a key whose version is no longer served -- what the generated policy watch produces from an owner +// reference written under an old version. Scope must come from the resolved mapping: read from the +// stale queued version, which the informer manager no longer indexes as cluster-scoped, it would look +// the object up in a namespace it does not live in and take it for deleted. +func TestReconcileResolvesRemovedVersionForClusterScoped(t *testing.T) { + ctx := context.Background() + source := newSource(namespaceGVK, "", testName, map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: oneSelector}) + + // The informer holds the source as a cluster-scoped resource: it lives under no namespace, so a + // namespaced lookup finds nothing. That is what makes this a real regression pin -- determining + // scope from the stale queued GVK would misclassify the source as namespaced and read it through + // ByNamespace, which now misses it, taking the source for deleted; determining scope from the + // resolved mapping reads it through the cluster-scoped lister and finds it. + recorder := record.NewFakeRecorder(10) + c := fake.NewClientBuilder().WithScheme(newScheme(t)).Build() + r := &Reconciler{ + Client: c, + UncachedReader: c, + RestMapper: newRESTMapper(), + InformerManager: &testinformer.FakeManager{ + // The manager knows the served version's scope but not the retired one the key names. + APIResources: map[schema.GroupVersionKind]bool{namespaceGVK: true}, + IsClusterScopedResource: true, + Listers: map[schema.GroupVersionResource]*testinformer.FakeLister{ + namespaceGVR: {Objects: []runtime.Object{source}, ClusterScoped: true}, + }, + }, + Recorder: recorder, + } + + // The key names Namespace under a retired version; only the served v1 is registered in the mapper. + key := keyFor(schema.GroupVersionKind{Group: "", Version: "v2", Kind: "Namespace"}, "", testName) + if _, err := r.Reconcile(ctx, key); err != nil { + t.Fatalf("Reconcile(%v) = %v, want no error", key, err) + } + if _, found := policyFrom(ctx, t, r, source); !found { + t.Errorf("Reconcile(%v) generated no policy, want the cluster-scoped source resolved through the served version", key) + } + if diff := cmp.Diff(recordedReasons(recorder), []string{EventReasonPolicyCreated}); diff != "" { + t.Errorf("Reconcile(%v) recorded events mismatch (-got, +want):\n%s", key, diff) + } +} + +// TestReconcileReadsPolicyThroughUncachedReader covers the read that repairs drift. The generated +// policies are watched through a different informer than a manager cache reads from, so a read from +// such a cache can lag behind the watch that just fired. The reconciler must read the policy through +// the uncached reader, which is current as of the moment the watch fired, or it would see a stale +// object, decide nothing changed, and leave the drift -- or, for a deletion, leave it forever. +func TestReconcileReadsPolicyThroughUncachedReader(t *testing.T) { + ctx := context.Background() + source := newSource(deploymentGVK, testNamespace, testName, map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: oneSelector}) + correct, err := parseClusterSelectors(oneSelector) + if err != nil { + t.Fatalf("parseClusterSelectors(%q) = %v, want no error", oneSelector, err) + } + drifted, err := parseClusterSelectors(twoSelectors) + if err != nil { + t.Fatalf("parseClusterSelectors(%q) = %v, want no error", twoSelectors, err) + } + + // The writer's cache is stale: it holds the policy exactly as this controller last wrote it. The + // uncached reader is current, and the policy has drifted since. A reconciler reading the writer's + // cache would see no difference and never repair the drift. + writer := fake.NewClientBuilder().WithScheme(newScheme(t)).WithObjects(desiredPolicy(source, correct)).Build() + uncached := fake.NewClientBuilder().WithScheme(newScheme(t)).WithObjects(desiredPolicy(source, drifted)).Build() + + recorder := record.NewFakeRecorder(10) + r := &Reconciler{ + Client: writer, + UncachedReader: uncached, + RestMapper: newRESTMapper(), + InformerManager: &testinformer.FakeManager{ + Listers: map[schema.GroupVersionResource]*testinformer.FakeLister{ + deploymentGVR: {Objects: []runtime.Object{source}}, + }, + }, + Recorder: recorder, + } + + key := keyFor(deploymentGVK, testNamespace, testName) + if _, err := r.Reconcile(ctx, key); err != nil { + t.Fatalf("Reconcile(%v) = %v, want no error", key, err) + } + // The drift the uncached reader sees is repaired; read from the stale writer cache, no update + // would have been recorded. + if diff := cmp.Diff(recordedReasons(recorder), []string{EventReasonPolicyUpdated}); diff != "" { + t.Errorf("Reconcile(%v) recorded events mismatch (-got, +want):\n%s", key, diff) + } +} + +// TestReconcileRequeuesWhileForeignPolicyBlocks covers a source whose generated name is occupied by a +// foreign policy. Removing that policy fires no event that reaches the source -- it carries no owner +// reference to it -- and the annotation does not change, so the reconciler must requeue the source +// itself, or the requested policy would never be created once the conflict is cleared. +func TestReconcileRequeuesWhileForeignPolicyBlocks(t *testing.T) { + ctx := context.Background() + source := newSource(deploymentGVK, testNamespace, testName, map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: oneSelector}) + foreign := &kfplacementv1alpha1.PlacementPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: generatedPolicyName(deploymentGVK, testNamespace, testName), + Namespace: testNamespace, + }, + Spec: kfplacementv1alpha1.PlacementPolicySpec{ + ResourceSelectors: []kfplacementv1alpha1.ResourceSelector{{APIGroup: "example.com", APIVersion: "v1", Kind: "Widget", Name: "hand-authored"}}, + }, + } + r, recorder := newReconciler(t, map[schema.GroupVersionResource][]runtime.Object{deploymentGVR: {source}}, nil, interceptor.Funcs{}, foreign) + + key := keyFor(deploymentGVK, testNamespace, testName) + result, err := r.Reconcile(ctx, key) + if err != nil { + t.Fatalf("Reconcile(%v) = %v, want no error", key, err) + } + if result.RequeueAfter != conflictRequeueAfter { + t.Errorf("Reconcile(%v).RequeueAfter = %v, want %v so the blocked source is retried", key, result.RequeueAfter, conflictRequeueAfter) + } + if diff := cmp.Diff(recordedReasons(recorder), []string{EventReasonPolicyConflict}); diff != "" { + t.Errorf("Reconcile(%v) recorded events mismatch (-got, +want):\n%s", key, diff) + } + + // The user removes the blocking policy. The next pass creates the requested policy and stops + // requeueing, which is the resumption a bare return would never have reached. + if err := r.Client.Delete(ctx, foreign); err != nil { + t.Fatalf("Delete(foreign) = %v, want no error", err) + } + result, err = r.Reconcile(ctx, key) + if err != nil { + t.Fatalf("Reconcile(%v) = %v, want no error", key, err) + } + if result.RequeueAfter != 0 { + t.Errorf("Reconcile(%v).RequeueAfter = %v after the conflict cleared, want no requeue", key, result.RequeueAfter) + } + if _, found := policyFrom(ctx, t, r, source); !found { + t.Errorf("Reconcile(%v) did not create the policy after the conflict cleared", key) + } + if diff := cmp.Diff(recordedReasons(recorder), []string{EventReasonPolicyCreated}); diff != "" { + t.Errorf("Reconcile(%v) recorded events mismatch (-got, +want):\n%s", key, diff) + } +} + +// TestApplyDesiredPolicyRejectsForeignObject covers the branch that exists only so that a scope this +// package does not know about cannot take the reconcile loop down with it. +func TestApplyDesiredPolicyRejectsForeignObject(t *testing.T) { + if got := applyDesiredPolicy(&unstructured.Unstructured{}, &kfplacementv1alpha1.PlacementPolicy{}); got { + t.Errorf("applyDesiredPolicy(%T) = true, want false", &unstructured.Unstructured{}) + } +} + +func TestHasClusterSelectorsAnnotation(t *testing.T) { + testCases := []struct { + name string + annotations map[string]string + want bool + }{ + {name: "no annotations at all", annotations: nil, want: false}, + {name: "other annotations only", annotations: map[string]string{"example.com/other": "value"}, want: false}, + {name: "the annotation is present", annotations: map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: oneSelector}, want: true}, + // An empty value is still a request for annotation-based placement; the parser is what + // rejects it, and it can only do that if the resource reaches the queue at all. + {name: "the annotation is present but empty", annotations: map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: ""}, want: true}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + source := newSource(deploymentGVK, testNamespace, testName, tc.annotations) + if got := HasClusterSelectorsAnnotation(source); got != tc.want { + t.Errorf("HasClusterSelectorsAnnotation(%v) = %v, want %v", tc.annotations, got, tc.want) + } + }) + } +} diff --git a/pkg/controllers/annotationplacement/parser.go b/pkg/controllers/annotationplacement/parser.go new file mode 100644 index 000000000..f70b82f8c --- /dev/null +++ b/pkg/controllers/annotationplacement/parser.go @@ -0,0 +1,237 @@ +/* +Copyright 2026 The KubeFleet Authors. + +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 annotationplacement implements FEP-0001 annotation-based placement: it keeps a placement +// policy object in sync with the kubefleet.dev/cluster-selectors annotation set on a Kubernetes +// resource, so that simple placement scenarios can be expressed without authoring a placement +// policy by hand. +package annotationplacement + +import ( + "fmt" + "strconv" + "strings" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/apimachinery/pkg/util/validation" + + kfplacementv1alpha1 "github.com/kubefleet-dev/kubefleet/apis/kubefleet.dev/placement/v1alpha1" +) + +const ( + // selectorSeparator separates the cluster selectors within the annotation value. + selectorSeparator = ";" + // matcherSeparator separates the label matchers, and the count directive, within one selector. + matcherSeparator = "," + // keyValueSeparator separates a label matcher's key from its value. + keyValueSeparator = "=" + + // countKey is the reserved matcher key that sets the desired cluster count of a selector. + countKey = "count" + // countAll is the count value that selects every cluster matching the selector's terms. + countAll = "All" + + // regionShorthand and aliasShorthand are the label key shorthands FEP-0001 reserves. + regionShorthand = "region" + aliasShorthand = "alias" +) + +// The limits below mirror the validation markers on the placement policy API, so that a rejected +// annotation is reported against the annotation itself rather than surfacing later as an opaque +// failure to create the policy object. +const ( + // maxSelectors mirrors MaxItems on PlacementPolicySpec.ClusterSelectors. + maxSelectors = 10 + // maxMatchLabels mirrors MaxProperties on ClusterLabelAndPropertySelectorTerm.MatchLabels. + maxMatchLabels = 10 + // maxCount mirrors the ceiling on ClusterSelector.Count, which the API enforces once per form: + // the Pattern marker bounds the string form, and a CEL rule bounds the integer form, since a + // pattern does not constrain the integer side of an int-or-string. + maxCount = 999 +) + +// defaultCount is the desired cluster count of a selector that does not carry a count directive. It +// matches the API's own default for the field, and is set explicitly rather than left to the API +// server so that the policy object built from an annotation compares equal to the one the API +// server stores; otherwise every reconciliation would observe a difference and issue an update. +const defaultCount = 1 + +// shorthandLabelKeys maps the label key shorthands reserved by FEP-0001 to the keys they expand to. +var shorthandLabelKeys = map[string]string{ + regionShorthand: corev1.LabelTopologyRegion, + aliasShorthand: kfplacementv1alpha1.ClusterAliasLabel, +} + +// parseClusterSelectors converts the value of the kubefleet.dev/cluster-selectors annotation into +// the cluster selectors of a placement policy. +// +// Errors returned by this function are always caused by the annotation's own contents; they are +// never transient, and a caller should surface them to the user rather than retry. +func parseClusterSelectors(value string) ([]kfplacementv1alpha1.ClusterSelector, error) { + if strings.TrimSpace(value) == "" { + return nil, fmt.Errorf("the annotation value is empty; it must list at least one cluster selector") + } + + segments := strings.Split(value, selectorSeparator) + if len(segments) > maxSelectors { + return nil, fmt.Errorf("the annotation lists %d cluster selectors, more than the supported maximum of %d", len(segments), maxSelectors) + } + + selectors := make([]kfplacementv1alpha1.ClusterSelector, 0, len(segments)) + for idx, segment := range segments { + selector, err := parseSelector(segment) + if err != nil { + // The index is 1-based: it is read by whoever wrote the annotation, who counts the + // selectors in it by eye. + return nil, fmt.Errorf("cluster selector %d (%q) is invalid: %w", idx+1, strings.TrimSpace(segment), err) + } + selectors = append(selectors, selector) + } + return selectors, nil +} + +// parseSelector converts one semicolon-delimited segment of the annotation into a cluster selector. +func parseSelector(segment string) (kfplacementv1alpha1.ClusterSelector, error) { + var selector kfplacementv1alpha1.ClusterSelector + + matchLabels := make(map[string]string) + var count *intstr.IntOrString + + for _, matcher := range strings.Split(segment, matcherSeparator) { + matcher = strings.TrimSpace(matcher) + if matcher == "" { + return selector, fmt.Errorf("it has an empty label matcher") + } + + rawKey, rawValue, found := strings.Cut(matcher, keyValueSeparator) + if !found { + return selector, fmt.Errorf("the label matcher %q is not in the KEY=VALUE format", matcher) + } + rawKey, rawValue = strings.TrimSpace(rawKey), strings.TrimSpace(rawValue) + + if rawKey == countKey { + if count != nil { + return selector, fmt.Errorf("it sets %s more than once", countKey) + } + parsed, err := parseCount(rawValue) + if err != nil { + return selector, err + } + count = parsed + continue + } + + key, err := parseLabelKey(rawKey) + if err != nil { + return selector, err + } + if _, duplicate := matchLabels[key]; duplicate { + return selector, fmt.Errorf("it matches on the label key %q more than once", key) + } + if err := validateLabelValue(key, rawValue); err != nil { + return selector, err + } + matchLabels[key] = rawValue + } + + if len(matchLabels) > maxMatchLabels { + return selector, fmt.Errorf("it matches on %d label keys, more than the supported maximum of %d", len(matchLabels), maxMatchLabels) + } + + if count == nil { + fallback := intstr.FromInt32(defaultCount) + count = &fallback + } + selector.Count = count + // A selector that carries only a count directive has no terms, which the API reads as a match on + // every cluster; this is deliberate, as it lets `count=All` on its own select the whole fleet. + if len(matchLabels) > 0 { + selector.Terms = []kfplacementv1alpha1.ClusterLabelAndPropertySelectorTerm{ + {MatchLabels: matchLabels}, + } + } + return selector, nil +} + +// parseCount converts the value of a count directive into the desired cluster count of a selector. +func parseCount(value string) (*intstr.IntOrString, error) { + if value == countAll { + all := intstr.FromString(countAll) + return &all, nil + } + if strings.EqualFold(value, countAll) { + return nil, fmt.Errorf("the %s value %q is not recognized; it must be spelled exactly %q", countKey, value, countAll) + } + + if !isAllDigits(value) { + return nil, fmt.Errorf("the %s value %q is neither a positive integer nor %q", countKey, value, countAll) + } + parsed, err := strconv.ParseInt(value, 10, 32) + if err != nil { + return nil, fmt.Errorf("the %s value %q is out of the supported range (1-%d)", countKey, value, maxCount) + } + if parsed < 1 || parsed > maxCount { + return nil, fmt.Errorf("the %s value %d is out of the supported range (1-%d)", countKey, parsed, maxCount) + } + + count := intstr.FromInt32(int32(parsed)) + return &count, nil +} + +// parseLabelKey expands a label key shorthand, if one is used, and verifies that the result is a +// key that Kubernetes accepts. +func parseLabelKey(key string) (string, error) { + if expanded, isShorthand := shorthandLabelKeys[key]; isShorthand { + return expanded, nil + } + if key == "" { + return "", fmt.Errorf("it has a label matcher with an empty label key") + } + if errs := validation.IsQualifiedName(key); len(errs) > 0 { + return "", fmt.Errorf("the label key %q is not valid: %s", key, strings.Join(errs, "; ")) + } + return key, nil +} + +// validateLabelValue verifies that a label matcher's value is one that Kubernetes accepts. +// +// Kubernetes considers the empty string a valid label value, but this surface rejects it: an empty +// value in a shorthand annotation is far more often a typo than a deliberate match on a label set +// to the empty string, and the placement policy API remains available for the latter. +func validateLabelValue(key, value string) error { + if value == "" { + return fmt.Errorf("the label key %q is matched against an empty value; use a placement policy object to match on an empty label value", key) + } + if errs := validation.IsValidLabelValue(value); len(errs) > 0 { + return fmt.Errorf("the value %q of the label key %q is not valid: %s", value, key, strings.Join(errs, "; ")) + } + return nil +} + +// isAllDigits reports whether the string is a non-empty run of decimal digits, which excludes the +// signs and the whitespace that the strconv parsers would otherwise accept. +func isAllDigits(s string) bool { + if s == "" { + return false + } + for _, r := range s { + if r < '0' || r > '9' { + return false + } + } + return true +} diff --git a/pkg/controllers/annotationplacement/parser_test.go b/pkg/controllers/annotationplacement/parser_test.go new file mode 100644 index 000000000..72e38fc09 --- /dev/null +++ b/pkg/controllers/annotationplacement/parser_test.go @@ -0,0 +1,485 @@ +/* +Copyright 2026 The KubeFleet Authors. + +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 annotationplacement + +import ( + "fmt" + "os" + "regexp" + "strconv" + "strings" + "testing" + + "github.com/google/go-cmp/cmp" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/apimachinery/pkg/util/validation" + "sigs.k8s.io/yaml" + + kfplacementv1alpha1 "github.com/kubefleet-dev/kubefleet/apis/kubefleet.dev/placement/v1alpha1" +) + +const ( + regionLabel = "topology.kubernetes.io/region" + aliasLabel = "kubefleet.dev/cluster-alias" + + // qualifiedNameMaxLength is the longest name segment of a label key that Kubernetes accepts. + // It mirrors the unexported limit that validation.IsQualifiedName enforces. + qualifiedNameMaxLength = 63 + + // crdPath is the placement policy CRD, read by TestLimitsMatchAPIValidation. + crdPath = "../../../config/crd/bases/placement.kubefleet.dev_placementpolicies.yaml" +) + +// selectorOf builds the cluster selector that the parser is expected to produce for one segment of +// the annotation; a nil matchLabels stands for a selector that carries no terms. +func selectorOf(count intstr.IntOrString, matchLabels map[string]string) kfplacementv1alpha1.ClusterSelector { + selector := kfplacementv1alpha1.ClusterSelector{Count: &count} + if matchLabels != nil { + selector.Terms = []kfplacementv1alpha1.ClusterLabelAndPropertySelectorTerm{ + {MatchLabels: matchLabels}, + } + } + return selector +} + +func TestParseClusterSelectors(t *testing.T) { + one, three, all := intstr.FromInt32(1), intstr.FromInt32(3), intstr.FromString("All") + + testCases := []struct { + name string + value string + want []kfplacementv1alpha1.ClusterSelector + }{ + { + name: "single label matcher with the region shorthand", + value: "region=eastus", + want: []kfplacementv1alpha1.ClusterSelector{ + selectorOf(one, map[string]string{regionLabel: "eastus"}), + }, + }, + { + // The first example given in FEP-0001. + name: "one cluster per region", + value: "region=eastus;region=westus;region=centralus", + want: []kfplacementv1alpha1.ClusterSelector{ + selectorOf(one, map[string]string{regionLabel: "eastus"}), + selectorOf(one, map[string]string{regionLabel: "westus"}), + selectorOf(one, map[string]string{regionLabel: "centralus"}), + }, + }, + { + // The second example given in FEP-0001. + name: "named clusters through the alias shorthand", + value: "alias=bravelion;alias=smartfish", + want: []kfplacementv1alpha1.ClusterSelector{ + selectorOf(one, map[string]string{aliasLabel: "bravelion"}), + selectorOf(one, map[string]string{aliasLabel: "smartfish"}), + }, + }, + { + // The third example given in FEP-0001. + name: "mixed counts and multiple matchers per selector", + value: "env=staging,count=All;env=canary,region=eastus,count=1", + want: []kfplacementv1alpha1.ClusterSelector{ + selectorOf(all, map[string]string{"env": "staging"}), + selectorOf(one, map[string]string{"env": "canary", regionLabel: "eastus"}), + }, + }, + { + name: "count on its own selects the whole fleet", + value: "count=All", + want: []kfplacementv1alpha1.ClusterSelector{selectorOf(all, nil)}, + }, + { + name: "count directive may precede the label matchers", + value: "count=3,env=prod", + want: []kfplacementv1alpha1.ClusterSelector{ + selectorOf(three, map[string]string{"env": "prod"}), + }, + }, + { + name: "fully qualified label keys are passed through", + value: fmt.Sprintf("%s=eastus", regionLabel), + want: []kfplacementv1alpha1.ClusterSelector{ + selectorOf(one, map[string]string{regionLabel: "eastus"}), + }, + }, + { + // Shorthands are expanded on an exact match only; Kubernetes label keys are + // case-sensitive, so a differently cased key is a different label. + name: "shorthand expansion is case-sensitive", + value: "Region=eastus", + want: []kfplacementv1alpha1.ClusterSelector{ + selectorOf(one, map[string]string{"Region": "eastus"}), + }, + }, + { + name: "surrounding whitespace is ignored", + value: " region = eastus , count = 3 ; env=prod ", + want: []kfplacementv1alpha1.ClusterSelector{ + selectorOf(three, map[string]string{regionLabel: "eastus"}), + selectorOf(one, map[string]string{"env": "prod"}), + }, + }, + { + name: "count at the top of the supported range", + value: fmt.Sprintf("env=prod,count=%d", maxCount), + want: []kfplacementv1alpha1.ClusterSelector{ + selectorOf(intstr.FromInt32(maxCount), map[string]string{"env": "prod"}), + }, + }, + { + name: "label value at the longest length Kubernetes accepts", + value: "env=" + strings.Repeat("a", validation.LabelValueMaxLength), + want: []kfplacementv1alpha1.ClusterSelector{ + selectorOf(one, map[string]string{"env": strings.Repeat("a", validation.LabelValueMaxLength)}), + }, + }, + { + name: "label matchers at the supported maximum", + value: func() string { + matchers := make([]string, 0, maxMatchLabels) + for i := 0; i < maxMatchLabels; i++ { + matchers = append(matchers, fmt.Sprintf("key%d=value", i)) + } + return strings.Join(matchers, matcherSeparator) + }(), + want: func() []kfplacementv1alpha1.ClusterSelector { + matchLabels := make(map[string]string, maxMatchLabels) + for i := 0; i < maxMatchLabels; i++ { + matchLabels[fmt.Sprintf("key%d", i)] = "value" + } + return []kfplacementv1alpha1.ClusterSelector{selectorOf(one, matchLabels)} + }(), + }, + { + // The count directive is reserved on an exact match only, like the shorthands above. + name: "the count keyword is case-sensitive", + value: "Count=5", + want: []kfplacementv1alpha1.ClusterSelector{ + selectorOf(one, map[string]string{"Count": "5"}), + }, + }, + { + name: "selector count at the supported maximum", + value: strings.TrimSuffix(strings.Repeat("region=eastus;", maxSelectors), ";"), + want: func() []kfplacementv1alpha1.ClusterSelector { + selectors := make([]kfplacementv1alpha1.ClusterSelector, 0, maxSelectors) + for i := 0; i < maxSelectors; i++ { + selectors = append(selectors, selectorOf(one, map[string]string{regionLabel: "eastus"})) + } + return selectors + }(), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got, err := parseClusterSelectors(tc.value) + if err != nil { + t.Fatalf("parseClusterSelectors(%q) = %v, want no error", tc.value, err) + } + if diff := cmp.Diff(got, tc.want); diff != "" { + t.Errorf("parseClusterSelectors(%q) mismatch (-got, +want):\n%s", tc.value, diff) + } + }) + } +} + +// formatLimit renders a CRD size limit, which is absent as often as it is set. +func formatLimit(limit *int64) string { + if limit == nil { + return "no limit" + } + return strconv.FormatInt(*limit, 10) +} + +// TestLimitsMatchAPIValidation ties the parser's limits to the validation markers on the placement +// policy CRD. +// +// The parser turns an annotation that exceeds a limit into an error reported against the annotation +// itself. Were the API to tighten a limit on its own, the parser would instead keep building policy +// objects that the API server rejects, and the failure would surface as an endless retry far away +// from the annotation that caused it. +func TestLimitsMatchAPIValidation(t *testing.T) { + rawCRD, err := os.ReadFile(crdPath) + if err != nil { + t.Fatalf("ReadFile(%q) = %v, want no error", crdPath, err) + } + var crd apiextensionsv1.CustomResourceDefinition + if err := yaml.Unmarshal(rawCRD, &crd); err != nil { + t.Fatalf("Unmarshal(%q) = %v, want no error", crdPath, err) + } + + var schema *apiextensionsv1.JSONSchemaProps + for idx := range crd.Spec.Versions { + if crd.Spec.Versions[idx].Name == kfplacementv1alpha1.GroupVersion.Version { + schema = crd.Spec.Versions[idx].Schema.OpenAPIV3Schema + break + } + } + if schema == nil { + t.Fatalf("the %s CRD has no %s version", crd.Name, kfplacementv1alpha1.GroupVersion.Version) + } + + clusterSelectors := schema.Properties["spec"].Properties["clusterSelectors"] + if got := clusterSelectors.MaxItems; got == nil || *got != maxSelectors { + t.Errorf("the CRD caps clusterSelectors at %s, want maxSelectors (%d)", formatLimit(got), maxSelectors) + } + // Every step below walks into the schema, so a renamed or restructured field must stop the test + // with a message that names the missing step rather than panic on a nil pointer. + if clusterSelectors.Items == nil || clusterSelectors.Items.Schema == nil { + t.Fatalf("the CRD schema of clusterSelectors carries no item schema") + } + + selector := clusterSelectors.Items.Schema.Properties + terms := selector["terms"] + if terms.Items == nil || terms.Items.Schema == nil { + t.Fatalf("the CRD schema of the selector terms carries no item schema") + } + matchLabels := terms.Items.Schema.Properties["matchLabels"] + if got := matchLabels.MaxProperties; got == nil || *got != maxMatchLabels { + t.Errorf("the CRD caps matchLabels at %s, want maxMatchLabels (%d)", formatLimit(got), maxMatchLabels) + } + + // The count field is an int-or-string, and the API bounds each form separately: a pattern for + // the string form, checked here by exercising it, and a CEL rule for the integer form, checked + // here by asserting the rule spells out the same bounds the parser enforces. A pattern alone + // would leave the integer form unbounded. + count := selector["count"] + countPattern, err := regexp.Compile(count.Pattern) + if err != nil { + t.Fatalf("Compile(%q) = %v, want no error", count.Pattern, err) + } + if got := strconv.Itoa(maxCount); !countPattern.MatchString(got) { + t.Errorf("the CRD pattern %q rejects maxCount (%s), want it accepted", count.Pattern, got) + } + if got := strconv.Itoa(maxCount + 1); countPattern.MatchString(got) { + t.Errorf("the CRD pattern %q accepts %s, want maxCount (%d) to be the ceiling", count.Pattern, got, maxCount) + } + if !countPattern.MatchString(countAll) { + t.Errorf("the CRD pattern %q rejects %q, want it accepted", count.Pattern, countAll) + } + + // The rule's spelling is free to change; what must not drift is the ceiling it encodes, in + // both the rule and the message a user is shown. The behavior itself is pinned separately by + // the integration suite, which submits out-of-range counts to a real API server. + wantCeiling := fmt.Sprintf("%d", maxCount) + integerRuleFound := false + for _, validation := range count.XValidations { + if !strings.Contains(validation.Rule, wantCeiling) { + continue + } + integerRuleFound = true + if !strings.Contains(validation.Message, wantCeiling) { + t.Errorf("the CRD rejects an out-of-range count with the message %q, want it to name the ceiling (%s)", validation.Message, wantCeiling) + } + } + if !integerRuleFound { + rules := make([]string, 0, len(count.XValidations)) + for _, validation := range count.XValidations { + rules = append(rules, validation.Rule) + } + t.Errorf("the CRD validates count with the rules %q, want one to bound the integer form at maxCount (%s)", rules, wantCeiling) + } + + if count.Default == nil { + t.Fatalf("the CRD schema of the selector count carries no default") + } + wantDefault := strconv.Itoa(defaultCount) + if got := string(count.Default.Raw); got != wantDefault { + t.Errorf("the CRD defaults count to %s, want defaultCount (%s)", got, wantDefault) + } +} + +func TestParseClusterSelectorsInvalid(t *testing.T) { + testCases := []struct { + name string + // value is the annotation value under test. + value string + // wantErrContains is a fragment the error message must carry, so that the annotation's + // author can tell which part of the value was rejected and why. + wantErrContains string + }{ + { + name: "empty value", + value: "", + wantErrContains: "empty", + }, + { + name: "whitespace-only value", + value: " ", + wantErrContains: "empty", + }, + { + name: "more selectors than supported", + value: strings.TrimSuffix(strings.Repeat("region=eastus;", maxSelectors+1), ";"), + wantErrContains: "more than the supported maximum", + }, + { + // Selectors are counted from one in the error, since whoever wrote the annotation + // counts them by eye. + name: "empty selector between two others", + value: "region=eastus;;region=westus", + wantErrContains: "cluster selector 2", + }, + { + name: "empty selector reports the reason as well as the position", + value: "region=eastus;;region=westus", + wantErrContains: "empty label matcher", + }, + { + name: "trailing separator leaves an empty selector", + value: "region=eastus;", + wantErrContains: "empty label matcher", + }, + { + name: "trailing matcher separator", + value: "region=eastus,", + wantErrContains: "empty label matcher", + }, + { + name: "matcher without a value", + value: "region", + wantErrContains: "KEY=VALUE", + }, + { + name: "matcher with an empty key", + value: "=eastus", + wantErrContains: "empty label key", + }, + { + name: "label key that Kubernetes rejects", + value: "not a key=eastus", + wantErrContains: "is not valid", + }, + { + name: "label value that Kubernetes rejects", + value: "env=not a value", + wantErrContains: "is not valid", + }, + { + name: "label matched against an empty value", + value: "env=", + wantErrContains: "empty value", + }, + { + // A value one byte past what Kubernetes accepts. Left unchecked, this would become a + // policy object that the API server rejects, turning a typo into an endless retry. + name: "label value one byte too long", + value: "env=" + strings.Repeat("a", validation.LabelValueMaxLength+1), + wantErrContains: "is not valid", + }, + { + name: "label key one byte too long", + value: strings.Repeat("k", qualifiedNameMaxLength+1) + "=prod", + wantErrContains: "is not valid", + }, + { + // strings.Cut splits on the first separator only, so the value keeps the rest; it is + // then rejected, since a label value cannot contain a separator. + name: "value containing the key-value separator", + value: "env=prod=east", + wantErrContains: "is not valid", + }, + { + name: "same label key matched twice", + value: "env=staging,env=canary", + wantErrContains: "more than once", + }, + { + // The shorthand and the key it expands to are the same label, which would otherwise + // silently drop one of the two matchers. + name: "shorthand collides with its expanded key", + value: fmt.Sprintf("region=eastus,%s=westus", regionLabel), + wantErrContains: "more than once", + }, + { + name: "count set twice", + value: "env=prod,count=1,count=2", + wantErrContains: "more than once", + }, + { + name: "count of zero", + value: "env=prod,count=0", + wantErrContains: "out of the supported range", + }, + { + name: "count above the supported range", + value: fmt.Sprintf("env=prod,count=%d", maxCount+1), + wantErrContains: "out of the supported range", + }, + { + // All digits, but too large to hold; this must be reported like any other out-of-range + // count rather than escaping as a conversion failure. + name: "count too large to be represented", + value: "env=prod,count=99999999999999999999", + wantErrContains: "out of the supported range", + }, + { + name: "negative count", + value: "env=prod,count=-1", + wantErrContains: "neither a positive integer", + }, + { + name: "explicitly signed count", + value: "env=prod,count=+5", + wantErrContains: "neither a positive integer", + }, + { + name: "count that is not a number", + value: "env=prod,count=many", + wantErrContains: "neither a positive integer", + }, + { + // A near miss on the All sentinel is worth its own message; the API accepts the exact + // spelling only. + name: "lowercased All sentinel", + value: "env=prod,count=all", + wantErrContains: `spelled exactly "All"`, + }, + { + name: "uppercased All sentinel", + value: "env=prod,count=ALL", + wantErrContains: `spelled exactly "All"`, + }, + { + name: "more label matchers than supported in one selector", + value: func() string { + matchers := make([]string, 0, maxMatchLabels+1) + for i := 0; i <= maxMatchLabels; i++ { + matchers = append(matchers, fmt.Sprintf("key%d=value", i)) + } + return strings.Join(matchers, matcherSeparator) + }(), + wantErrContains: "more than the supported maximum", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got, err := parseClusterSelectors(tc.value) + if err == nil { + t.Fatalf("parseClusterSelectors(%q) = %v, want an error", tc.value, got) + } + if !strings.Contains(err.Error(), tc.wantErrContains) { + t.Errorf("parseClusterSelectors(%q) = %v, want an error containing %q", tc.value, err, tc.wantErrContains) + } + }) + } +} diff --git a/pkg/controllers/annotationplacement/policy.go b/pkg/controllers/annotationplacement/policy.go new file mode 100644 index 000000000..81040e68c --- /dev/null +++ b/pkg/controllers/annotationplacement/policy.go @@ -0,0 +1,268 @@ +/* +Copyright 2026 The KubeFleet Authors. + +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 annotationplacement + +import ( + "fmt" + "strings" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/validation" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + + kfplacementv1alpha1 "github.com/kubefleet-dev/kubefleet/apis/kubefleet.dev/placement/v1alpha1" + "github.com/kubefleet-dev/kubefleet/pkg/utils/naming" +) + +const ( + // maxKindSegmentLength caps how much of the kind appears in a generated name. Every kind a user + // is likely to annotate is far shorter; the cap exists so that a pathologically long custom + // kind cannot crowd out the part of the name that identifies the resource. + maxKindSegmentLength = 40 + + // separatorCount is the number of separators a generated name spends joining its three parts. + separatorCount = 2 +) + +// The values the placement policy API defaults these fields to. +// +// A generated policy sets them explicitly, rather than letting the API server fill them in, so +// that the object built here equals the one the API server stores. Left unset, every field the API +// defaults would read as a difference on every pass and provoke an update that changes nothing. +// +// whenUnfulfilled deliberately follows the API default of requesting a cluster: asking the platform +// for a cluster that does not exist yet is the point of these APIs, and the annotation is meant to +// be the lowest-friction way to do so. The grammar has no slot for the field, so guarding against +// a selector nothing can ever satisfy (a mistyped region, say) is the claim fulfillment layer's +// job -- quota, approval, expiry -- not a softer default here. +const ( + defaultResourceRevisionHistoryLimit int32 = 3 + + defaultWhenUnfulfilled = kfplacementv1alpha1.WhenUnfulfilledOptionAddClusterClaim +) + +// generatedPolicyName derives the name of the placement policy generated for a resource. +// +// The name is deterministic, so that reconciling the same resource twice converges on one policy +// instead of accumulating them, and it is readable, so that the policy can be recognized in a +// listing. Neither property may come at the cost of the third: the hash is taken over the +// resource's whole identity, including the parts that do not appear in the name at all, so two +// resources cannot generate one name even when truncation erases what distinguishes them. +func generatedPolicyName(gvk schema.GroupVersionKind, namespace, name string) string { + // The kind and the name are sanitized before they appear in the name: a resource legal for its + // own API can carry characters a DNS-1123 object name forbids (an RBAC name like + // system:aggregate-to-admin), and copying them through would produce a policy the API server + // rejects, hot-looping the reconciler. The hash below is taken over the unsanitized identity, so + // sanitizing the visible parts costs no uniqueness. + kindSegment := naming.Truncate(naming.Sanitize(gvk.Kind), maxKindSegmentLength) + + // Whatever the kind and the hash do not use is available to the resource's own name. + nameBudget := validation.DNS1123SubdomainMaxLength - len(kindSegment) - naming.HashLength - separatorCount + nameSegment := naming.Truncate(naming.Sanitize(name), nameBudget) + + // Empty segments are dropped rather than joined. An object always has a name, and one read + // from the API server always has a kind, but joining an empty leading segment would put a + // separator at the front of the name, where the API server does not allow one. + segments := make([]string, 0, 3) + for _, segment := range []string{kindSegment, nameSegment} { + if segment != "" { + segments = append(segments, segment) + } + } + identity := fmt.Sprintf("%s/%s/%s/%s", gvk.Group, gvk.Kind, namespace, name) + segments = append(segments, naming.Hash(identity)) + + return strings.Join(segments, "-") +} + +// parentLabels returns the labels recording which resource a generated policy came from. +// +// Every value is shortened to fit a label value, including the API group: a custom resource's +// group is validated as a DNS-1123 subdomain and so may run to 253 bytes, well past what a label +// value holds. A kind cannot exceed the limit today, but it is shortened alongside the others +// rather than relying on a bound that belongs to a different validation rule than this one. +func parentLabels(gvk schema.GroupVersionKind, name string) map[string]string { + return map[string]string{ + kfplacementv1alpha1.ParentAPIGroupLabel: naming.LabelValue(gvk.Group), + kfplacementv1alpha1.ParentKindLabel: naming.LabelValue(gvk.Kind), + kfplacementv1alpha1.ParentNameLabel: naming.LabelValue(name), + } +} + +// isGeneratedFor reports whether a live policy is one this controller generated for the given +// resource identity. It is what stops the controller from overwriting or deleting a policy a user or +// another tool authored at the deterministic name this resource happens to generate. +// +// Ownership is recognized from either marker this controller stamps -- the owner reference back to +// the source, or the provenance labels -- and does not require both. Both are things applyDesiredPolicy +// repairs, so demanding both be intact would let a single edited label or stripped owner reference +// classify one of this controller's own policies as foreign: it would then be denied the very repair +// that would restore the marker and, worse, denied deletion when its annotation is removed, leaving a +// placement running with no way to reconcile or clean it up. Recognizing either marker keeps the +// policy repairable as long as one survives; a foreign policy that merely collides with the name +// carries neither and is still left untouched. Both markers are derived from the resource's stable +// identity rather than its UID, so the judgment holds across a delete and recreate of the resource. +// +// The one residual gap is a policy that loses both markers at once, which is then indistinguishable +// from a foreign object and abandoned. It is reachable without malice -- a tool that rewrites the +// whole object, such as a plain kubectl apply of a hand-kept manifest or a non-server-side-apply +// Update, drops metadata.labels and metadata.ownerReferences together -- but a policy this controller +// generated is not one a user is expected to manage that way, and requiring both markers to survive +// is the accepted price of never touching a policy that is genuinely someone else's. +func isGeneratedFor(policy client.Object, gvk schema.GroupVersionKind, name string) bool { + sourceRef := metav1.OwnerReference{APIVersion: gvk.GroupVersion().String(), Kind: gvk.Kind, Name: name} + for _, ref := range policy.GetOwnerReferences() { + if sameOwnerIdentity(ref, sourceRef) { + return true + } + } + + labels := policy.GetLabels() + for key, want := range parentLabels(gvk, name) { + if labels[key] != want { + return false + } + } + return true +} + +// parentOwnerReference returns the owner reference that ties a generated policy to the resource it +// was generated from, so that deleting the resource collects the policy with it. +// +// The reference deliberately claims neither controller nor blocking ownership: setting either +// requires permission to update the finalizers subresource of the owner, and the owner here can be +// a resource of any kind at all. Cascading deletion, the only property this needs, works the same +// without them. +func parentOwnerReference(source *unstructured.Unstructured) metav1.OwnerReference { + gvk := source.GroupVersionKind() + return metav1.OwnerReference{ + APIVersion: gvk.GroupVersion().String(), + Kind: gvk.Kind, + Name: source.GetName(), + UID: source.GetUID(), + } +} + +// parentResourceSelector returns the selector that places the annotated resource itself. +// +// It is the only resource selector a generated policy has: an annotation asks for the resource it is +// written on to be placed, and nothing else. Should that ever stop being true, note that the +// reconciler replaces the whole spec of a generated policy, so a selector added from elsewhere would +// have to be reconciled here rather than merely appended to the live object. +func parentResourceSelector(source *unstructured.Unstructured) kfplacementv1alpha1.ResourceSelector { + gvk := source.GroupVersionKind() + // The namespace is deliberately left unset. A generated policy for a namespaced resource lives + // in that resource's own namespace, where the API requires the selector's namespace to be + // empty or identical; a generated policy for a cluster-scoped resource has no namespace to name. + return kfplacementv1alpha1.ResourceSelector{ + APIGroup: gvk.Group, + APIVersion: gvk.Version, + Kind: gvk.Kind, + Name: source.GetName(), + } +} + +// withAPIDefaults returns the selectors with every field the API defaults set explicitly, leaving +// the caller's own slice untouched. +func withAPIDefaults(selectors []kfplacementv1alpha1.ClusterSelector) []kfplacementv1alpha1.ClusterSelector { + if selectors == nil { + return nil + } + defaulted := make([]kfplacementv1alpha1.ClusterSelector, len(selectors)) + for i, selector := range selectors { + selector.DeepCopyInto(&defaulted[i]) + if defaulted[i].WhenUnfulfilled == "" { + defaulted[i].WhenUnfulfilled = defaultWhenUnfulfilled + } + } + return defaulted +} + +// desiredPolicy builds the placement policy that should exist for an annotated resource. +// +// The policy's scope follows the resource's own: a namespaced resource yields a PlacementPolicy in +// its namespace, and a cluster-scoped resource yields a ClusterPlacementPolicy. That is what keeps +// the owner reference valid, since a cluster-scoped object owned by a namespaced one is accepted +// on creation and then never collected at all. +// +// Scope is read from the resource's own namespace, which the API server has already reconciled +// with the scope its kind is registered under; the source must therefore be an object read from +// the API server rather than one assembled in memory, which is also what guarantees it carries the +// UID and the kind that the owner reference and the generated name are built from. Taking an +// unstructured object rather than a client.Object is deliberate for the same reason: a typed +// object routinely arrives with an empty kind, which would silently change the generated name. +func desiredPolicy(source *unstructured.Unstructured, selectors []kfplacementv1alpha1.ClusterSelector) client.Object { + gvk := source.GroupVersionKind() + namespace := source.GetNamespace() + + policy := emptyPolicyForScope(namespace) + policy.SetName(generatedPolicyName(gvk, namespace, source.GetName())) + policy.SetNamespace(namespace) + policy.SetLabels(parentLabels(gvk, source.GetName())) + policy.SetOwnerReferences([]metav1.OwnerReference{parentOwnerReference(source)}) + *policySpec(policy) = kfplacementv1alpha1.PlacementPolicySpec{ + ClusterSelectors: withAPIDefaults(selectors), + ResourceSelectors: []kfplacementv1alpha1.ResourceSelector{parentResourceSelector(source)}, + ResourceRevisionHistoryLimit: ptr.To(defaultResourceRevisionHistoryLimit), + } + return policy +} + +// emptyPolicyForScope returns an empty generated policy of the scope that a resource in the given +// namespace generates: a namespaced resource yields a PlacementPolicy in its own namespace, and a +// cluster-scoped resource, whose namespace is empty, yields a ClusterPlacementPolicy. +// +// This is the single place the scope is decided. The reconciler needs the same answer to read and to +// delete a generated policy as it does to build one, and a disagreement between those would leave a +// policy behind rather than fail. +func emptyPolicyForScope(namespace string) client.Object { + if namespace == "" { + return &kfplacementv1alpha1.ClusterPlacementPolicy{} + } + return &kfplacementv1alpha1.PlacementPolicy{} +} + +// generatedPolicyKind returns the kind of the policy that emptyPolicyForScope produces for the +// given namespace, spelled as the kind itself is, so that a message carrying it can be pasted +// straight into a kubectl command against the right resource. It asks emptyPolicyForScope rather +// than repeating its namespace check, which keeps the scope decision in the one place that +// function's contract promises. +func generatedPolicyKind(namespace string) string { + if _, clusterScoped := emptyPolicyForScope(namespace).(*kfplacementv1alpha1.ClusterPlacementPolicy); clusterScoped { + return "ClusterPlacementPolicy" + } + return "PlacementPolicy" +} + +// policySpec returns a pointer to the spec of a generated policy, whichever scope it has, so that +// callers can read and write the spec without repeating the scope distinction. +// +// It returns nil for any other type. Callers within this package only ever pass objects that +// emptyPolicyForScope produced, so a nil here means the two have fallen out of step. +func policySpec(policy client.Object) *kfplacementv1alpha1.PlacementPolicySpec { + switch typed := policy.(type) { + case *kfplacementv1alpha1.PlacementPolicy: + return &typed.Spec + case *kfplacementv1alpha1.ClusterPlacementPolicy: + return &typed.Spec + default: + return nil + } +} diff --git a/pkg/controllers/annotationplacement/policy_test.go b/pkg/controllers/annotationplacement/policy_test.go new file mode 100644 index 000000000..2d704e4ea --- /dev/null +++ b/pkg/controllers/annotationplacement/policy_test.go @@ -0,0 +1,466 @@ +/* +Copyright 2026 The KubeFleet Authors. + +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 annotationplacement + +import ( + "fmt" + "strings" + "testing" + + "github.com/google/go-cmp/cmp" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/apimachinery/pkg/util/validation" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + + kfplacementv1alpha1 "github.com/kubefleet-dev/kubefleet/apis/kubefleet.dev/placement/v1alpha1" + "github.com/kubefleet-dev/kubefleet/pkg/utils/naming" +) + +var ( + deploymentGVK = schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"} + namespaceGVK = schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Namespace"} +) + +// sourceObject builds the annotated resource a policy is generated from. +func sourceObject(gvk schema.GroupVersionKind, namespace, name string) *unstructured.Unstructured { + source := &unstructured.Unstructured{} + source.SetGroupVersionKind(gvk) + source.SetNamespace(namespace) + source.SetName(name) + source.SetUID(types.UID("uid-" + name)) + return source +} + +func TestGeneratedPolicyName(t *testing.T) { + testCases := []struct { + name string + gvk schema.GroupVersionKind + namespace string + objName string + want string + }{ + { + name: "namespaced resource", + gvk: deploymentGVK, + namespace: "prod", + objName: "app", + want: "deployment-app-" + hashOfIdentity("apps", "Deployment", "prod", "app"), + }, + { + name: "cluster-scoped resource", + gvk: namespaceGVK, + objName: "work", + want: "namespace-work-" + hashOfIdentity("", "Namespace", "", "work"), + }, + { + // The kind is lowercased so that the name is a legal DNS-1123 subdomain; the hash is + // taken over the kind as it is really spelled, so two kinds that differ only in case + // cannot generate the same name. + name: "kind is lowercased in the name but not in the identity", + gvk: schema.GroupVersionKind{Group: "example.com", Version: "v1", Kind: "MyWidget"}, + namespace: "prod", + objName: "gadget", + want: "mywidget-gadget-" + hashOfIdentity("example.com", "MyWidget", "prod", "gadget"), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got := generatedPolicyName(tc.gvk, tc.namespace, tc.objName) + if got != tc.want { + t.Errorf("generatedPolicyName(%v, %q, %q) = %v, want %v", tc.gvk, tc.namespace, tc.objName, got, tc.want) + } + }) + } +} + +func hashOfIdentity(group, kind, namespace, name string) string { + return naming.Hash(fmt.Sprintf("%s/%s/%s/%s", group, kind, namespace, name)) +} + +// TestGeneratedPolicyNameValidity covers the inputs where the name has to be shortened to fit, +// which is where a generated name has twice gone wrong in this project: the result must stay a +// legal object name, and it must still tell two resources apart after the part that distinguishes +// them has been cut away. +func TestGeneratedPolicyNameValidity(t *testing.T) { + longKind := "My" + strings.Repeat("Widget", 40) + longName := strings.Repeat("a", validation.DNS1123SubdomainMaxLength) + + testCases := []struct { + name string + gvk schema.GroupVersionKind + namespace string + objName string + }{ + {name: "short everything", gvk: deploymentGVK, namespace: "prod", objName: "app"}, + { + // An RBAC name carries a colon, legal for the resource but not for a Kubernetes object + // name; without sanitizing it the generated name is rejected and the reconciler hot-loops. + name: "rbac name with a colon", + gvk: schema.GroupVersionKind{Group: "rbac.authorization.k8s.io", Version: "v1", Kind: "ClusterRole"}, + objName: "system:aggregate-to-admin", + }, + { + // A dot beside an illegal character is the case that a naive sanitizer keeping dots gets + // wrong: it would leave a label ending in a dash, which the API server rejects. Resource + // names commonly carry dots (domain-like custom resource names), so this is reachable. + name: "name with a dot beside an illegal character", + gvk: deploymentGVK, + objName: "my.:app", + }, + {name: "name at the object limit", gvk: deploymentGVK, namespace: "prod", objName: longName}, + { + name: "name truncated onto a separator", + gvk: deploymentGVK, + namespace: "prod", + objName: strings.Repeat("a", 194) + "-" + strings.Repeat("b", 40), + }, + { + name: "name truncated onto a dot", + gvk: deploymentGVK, + namespace: "prod", + objName: strings.Repeat("a", 194) + "." + strings.Repeat("b", 40), + }, + { + name: "kind longer than its budget", + gvk: schema.GroupVersionKind{Group: "example.com", Version: "v1", Kind: longKind}, + namespace: "prod", + objName: longName, + }, + { + // An object read from the API server always carries its kind, but an empty one must + // not put a separator at the front of the generated name, where it would be rejected. + name: "empty kind", + gvk: schema.GroupVersionKind{Group: "apps", Version: "v1"}, + namespace: "prod", + objName: "app", + }, + { + name: "empty kind and empty name", + gvk: schema.GroupVersionKind{Group: "apps", Version: "v1"}, + namespace: "prod", + }, + } + + seen := make(map[string]string, len(testCases)) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got := generatedPolicyName(tc.gvk, tc.namespace, tc.objName) + if errs := validation.IsDNS1123Subdomain(got); len(errs) > 0 { + t.Errorf("generatedPolicyName(%v, %q, %q) = %v, want a valid object name: %s", + tc.gvk, tc.namespace, tc.objName, got, strings.Join(errs, "; ")) + } + if len(got) > validation.DNS1123SubdomainMaxLength { + t.Errorf("len(generatedPolicyName(%v, %q, %q)) = %d, want at most %d", + tc.gvk, tc.namespace, tc.objName, len(got), validation.DNS1123SubdomainMaxLength) + } + if previous, collided := seen[got]; collided { + t.Errorf("generatedPolicyName(%v, %q, %q) = %v, want a name distinct from that of %s", + tc.gvk, tc.namespace, tc.objName, got, previous) + } + seen[got] = tc.name + }) + } +} + +// TestGeneratedPolicyNameDistinguishesIdentities pins the property the hash exists for: resources +// that differ anywhere in their identity, including in parts the readable name never shows, must +// not generate the same policy name. +func TestGeneratedPolicyNameDistinguishesIdentities(t *testing.T) { + longName := strings.Repeat("a", 300) + + identities := []struct { + name string + gvk schema.GroupVersionKind + namespace string + objName string + }{ + {name: "baseline", gvk: deploymentGVK, namespace: "prod", objName: "app"}, + {name: "different namespace", gvk: deploymentGVK, namespace: "staging", objName: "app"}, + {name: "different api group, same kind", gvk: schema.GroupVersionKind{Group: "example.com", Version: "v1", Kind: "Deployment"}, namespace: "prod", objName: "app"}, + {name: "kind differing only in case", gvk: schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "DeploymenT"}, namespace: "prod", objName: "app"}, + // Two names that sanitize to the same visible segment -- a dot and a dash both render as a + // dash -- must still generate distinct policy names, since the hash is over the raw identity. + {name: "name with a dot", gvk: deploymentGVK, namespace: "prod", objName: "my.app"}, + {name: "name with a dash where the other has a dot", gvk: deploymentGVK, namespace: "prod", objName: "my-app"}, + // Two names identical up to well past the truncation point. + {name: "long name", gvk: deploymentGVK, namespace: "prod", objName: longName + "one"}, + {name: "long name sharing a prefix", gvk: deploymentGVK, namespace: "prod", objName: longName + "two"}, + } + + seen := make(map[string]string, len(identities)) + for _, identity := range identities { + got := generatedPolicyName(identity.gvk, identity.namespace, identity.objName) + if previous, collided := seen[got]; collided { + t.Errorf("generatedPolicyName for %s = %v, want a name distinct from that of %s", identity.name, got, previous) + } + seen[got] = identity.name + } +} + +// TestIsGeneratedFor pins the check that keeps this controller from commandeering or deleting a +// policy someone else authored at a resource's generated name, while still recognizing its own policy +// after either provenance marker has drifted so that the policy stays repairable and removable. +func TestIsGeneratedFor(t *testing.T) { + source := sourceObject(deploymentGVK, "prod", "app") + mine := desiredPolicy(source, nil) + + // The same policy with its provenance labels edited away; the owner reference still identifies it. + labelsStripped := mine.DeepCopyObject().(client.Object) + labelsStripped.SetLabels(nil) + + // The same policy with its owner reference removed; the provenance labels still identify it. + ownerStripped := mine.DeepCopyObject().(client.Object) + ownerStripped.SetOwnerReferences(nil) + + // The same policy stripped of both markers; nothing is left to tell it from a foreign object. + bothStripped := mine.DeepCopyObject().(client.Object) + bothStripped.SetLabels(nil) + bothStripped.SetOwnerReferences(nil) + + // A policy at the same name authored by someone else, carrying neither marker. + foreign := emptyPolicyForScope("prod") + foreign.SetName(mine.GetName()) + foreign.SetNamespace("prod") + + // A policy that carries another resource's provenance -- the collision a bare name match would + // miss -- and no owner reference to this source. + otherSource := sourceObject(deploymentGVK, "prod", "other") + otherLabelled := emptyPolicyForScope("prod") + otherLabelled.SetName(mine.GetName()) + otherLabelled.SetNamespace("prod") + otherLabelled.SetLabels(parentLabels(otherSource.GroupVersionKind(), otherSource.GetName())) + + testCases := []struct { + name string + policy client.Object + want bool + }{ + {name: "the policy this controller generated", policy: mine, want: true}, + {name: "our policy with its labels stripped is still recognized by its owner reference", policy: labelsStripped, want: true}, + {name: "our policy with its owner reference stripped is still recognized by its labels", policy: ownerStripped, want: true}, + {name: "our policy with both markers gone is indistinguishable from foreign", policy: bothStripped, want: false}, + {name: "a foreign policy with neither marker", policy: foreign, want: false}, + {name: "a policy carrying another resource's provenance", policy: otherLabelled, want: false}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if got := isGeneratedFor(tc.policy, source.GroupVersionKind(), source.GetName()); got != tc.want { + t.Errorf("isGeneratedFor(%q) = %v, want %v", tc.policy.GetName(), got, tc.want) + } + }) + } +} + +func TestDesiredPolicy(t *testing.T) { + // The parser leaves whenUnfulfilled unset, since the annotation grammar cannot express it; the + // generated policy carries the API's default explicitly so that it does not read as drift. + selectors := []kfplacementv1alpha1.ClusterSelector{ + { + Count: ptr.To(intstr.FromInt32(1)), + Terms: []kfplacementv1alpha1.ClusterLabelAndPropertySelectorTerm{ + {MatchLabels: map[string]string{"env": "prod"}}, + }, + }, + } + wantSelectors := []kfplacementv1alpha1.ClusterSelector{ + { + Count: ptr.To(intstr.FromInt32(1)), + Terms: []kfplacementv1alpha1.ClusterLabelAndPropertySelectorTerm{ + {MatchLabels: map[string]string{"env": "prod"}}, + }, + WhenUnfulfilled: kfplacementv1alpha1.WhenUnfulfilledOptionAddClusterClaim, + }, + } + + testCases := []struct { + name string + source *unstructured.Unstructured + want client.Object + }{ + { + name: "namespaced resource yields a namespaced policy", + source: sourceObject(deploymentGVK, "prod", "app"), + want: &kfplacementv1alpha1.PlacementPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "deployment-app-" + hashOfIdentity("apps", "Deployment", "prod", "app"), + Namespace: "prod", + Labels: map[string]string{ + kfplacementv1alpha1.ParentAPIGroupLabel: "apps", + kfplacementv1alpha1.ParentKindLabel: "Deployment", + kfplacementv1alpha1.ParentNameLabel: "app", + }, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "apps/v1", + Kind: "Deployment", + Name: "app", + UID: types.UID("uid-app"), + }}, + }, + Spec: kfplacementv1alpha1.PlacementPolicySpec{ + ClusterSelectors: wantSelectors, + ResourceSelectors: []kfplacementv1alpha1.ResourceSelector{{ + APIGroup: "apps", + APIVersion: "v1", + Kind: "Deployment", + Name: "app", + }}, + ResourceRevisionHistoryLimit: ptr.To(defaultResourceRevisionHistoryLimit), + }, + }, + }, + { + name: "cluster-scoped resource yields a cluster-scoped policy", + source: sourceObject(namespaceGVK, "", "work"), + want: &kfplacementv1alpha1.ClusterPlacementPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "namespace-work-" + hashOfIdentity("", "Namespace", "", "work"), + Labels: map[string]string{ + // The core API group is the empty string, and the label is present all the + // same, so that core-group resources can be selected like any other. + kfplacementv1alpha1.ParentAPIGroupLabel: "", + kfplacementv1alpha1.ParentKindLabel: "Namespace", + kfplacementv1alpha1.ParentNameLabel: "work", + }, + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "v1", + Kind: "Namespace", + Name: "work", + UID: types.UID("uid-work"), + }}, + }, + Spec: kfplacementv1alpha1.PlacementPolicySpec{ + ClusterSelectors: wantSelectors, + ResourceSelectors: []kfplacementv1alpha1.ResourceSelector{{ + APIVersion: "v1", + Kind: "Namespace", + Name: "work", + }}, + ResourceRevisionHistoryLimit: ptr.To(defaultResourceRevisionHistoryLimit), + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got := desiredPolicy(tc.source, selectors) + if diff := cmp.Diff(got, tc.want); diff != "" { + t.Errorf("desiredPolicy() mismatch (-got, +want):\n%s", diff) + } + }) + } +} + +// TestParentOwnerReferenceIsNotControlling pins the choice not to claim controller or blocking +// ownership: either would require permission to update the finalizers subresource of a resource of +// any kind the user cares to annotate. +func TestParentOwnerReferenceIsNotControlling(t *testing.T) { + got := parentOwnerReference(sourceObject(deploymentGVK, "prod", "app")) + if got.Controller != nil { + t.Errorf("parentOwnerReference().Controller = %v, want nil", *got.Controller) + } + if got.BlockOwnerDeletion != nil { + t.Errorf("parentOwnerReference().BlockOwnerDeletion = %v, want nil", *got.BlockOwnerDeletion) + } +} + +// TestParentNameLabelIsShortenedWhenTooLong covers a resource whose name cannot fit in a label +// value, where the label is knowingly lossy and the owner reference is what remains exact. +func TestParentNameLabelIsShortenedWhenTooLong(t *testing.T) { + longName := strings.Repeat("a", 200) + source := sourceObject(deploymentGVK, "prod", longName) + + policy := desiredPolicy(source, nil) + labels := policy.GetLabels() + got := labels[kfplacementv1alpha1.ParentNameLabel] + + if got == longName { + t.Errorf("parent name label = the full name, want it shortened to fit a label value") + } + if errs := validation.IsValidLabelValue(got); len(errs) > 0 { + t.Errorf("parent name label = %v, want a valid label value: %s", got, strings.Join(errs, "; ")) + } + if wantOwner := longName; policy.GetOwnerReferences()[0].Name != wantOwner { + t.Errorf("owner reference name = %v, want the unshortened %v", policy.GetOwnerReferences()[0].Name, wantOwner) + } +} + +// TestParentLabelsAreValid covers the label values that have to be shortened to fit. A custom +// resource's API group is validated as a DNS-1123 subdomain and so runs to 253 bytes, four times +// what a label value holds; left unshortened it would make every generated policy for that +// resource fail validation on creation, with nothing but a retrying reconciler to show for it. +func TestParentLabelsAreValid(t *testing.T) { + testCases := []struct { + name string + gvk schema.GroupVersionKind + objName string + }{ + {name: "ordinary resource", gvk: deploymentGVK, objName: "app"}, + {name: "core group resource", gvk: namespaceGVK, objName: "work"}, + { + name: "api group too long for a label value", + gvk: schema.GroupVersionKind{Group: strings.Repeat("g", 90) + ".example.com", Version: "v1", Kind: "Widget"}, + objName: "gadget", + }, + { + name: "kind at the longest a kind can be", + gvk: schema.GroupVersionKind{Group: "example.com", Version: "v1", Kind: strings.Repeat("W", 63)}, + objName: "gadget", + }, + {name: "name too long for a label value", gvk: deploymentGVK, objName: strings.Repeat("a", 200)}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + for key, value := range parentLabels(tc.gvk, tc.objName) { + if errs := validation.IsValidLabelValue(value); len(errs) > 0 { + t.Errorf("parentLabels()[%s] = %v, want a valid label value: %s", key, value, strings.Join(errs, "; ")) + } + } + }) + } +} + +// TestDesiredPolicySetsDefaultedFields pins that the generated policy carries a value for every +// field the API server would otherwise default. A field left unset is stored with the API's +// default and then reads as a difference on the next pass, which would have the reconciler issue +// an update that changes nothing, forever. +func TestDesiredPolicySetsDefaultedFields(t *testing.T) { + selectors := []kfplacementv1alpha1.ClusterSelector{{Count: ptr.To(intstr.FromInt32(1))}} + policy, ok := desiredPolicy(sourceObject(deploymentGVK, "prod", "app"), selectors).(*kfplacementv1alpha1.PlacementPolicy) + if !ok { + t.Fatalf("desiredPolicy() returned a %T, want a *PlacementPolicy", policy) + } + + if got := policy.Spec.ResourceRevisionHistoryLimit; got == nil || *got != defaultResourceRevisionHistoryLimit { + t.Errorf("resourceRevisionHistoryLimit = %v, want %d", got, defaultResourceRevisionHistoryLimit) + } + if got := policy.Spec.ClusterSelectors[0].WhenUnfulfilled; got != defaultWhenUnfulfilled { + t.Errorf("whenUnfulfilled = %v, want %v", got, defaultWhenUnfulfilled) + } + // The caller's own selectors must not have been defaulted in place. + if got := selectors[0].WhenUnfulfilled; got != "" { + t.Errorf("the caller's selector whenUnfulfilled = %v, want it left empty", got) + } +} diff --git a/pkg/controllers/annotationplacement/suite_test.go b/pkg/controllers/annotationplacement/suite_test.go new file mode 100644 index 000000000..4e740c4cf --- /dev/null +++ b/pkg/controllers/annotationplacement/suite_test.go @@ -0,0 +1,126 @@ +/* +Copyright 2026 The KubeFleet Authors. + +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 annotationplacement + +import ( + "context" + "path/filepath" + "testing" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/discovery" + "k8s.io/client-go/dynamic" + "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/restmapper" + "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" + + kfplacementv1alpha1 "github.com/kubefleet-dev/kubefleet/apis/kubefleet.dev/placement/v1alpha1" + "github.com/kubefleet-dev/kubefleet/pkg/utils/informer" +) + +var ( + configMapGVK = schema.GroupVersionKind{Group: "", Version: "v1", Kind: "ConfigMap"} + configMapGVR = schema.GroupVersionResource{Group: "", Version: "v1", Resource: "configmaps"} +) + +var ( + testEnv *envtest.Environment + hubClient client.Client + informerManager informer.Manager + reconciler *Reconciler + eventRecorder *record.FakeRecorder + ctx context.Context + cancel context.CancelFunc +) + +// The GVRs the informer manager is told to watch. A resource of a kind with no informer never +// reaches the reconciler at all, so these are exactly the kinds these tests may annotate. +var ( + configMapAPIResource = informer.APIResourceMeta{ + GroupVersionKind: configMapGVK, + GroupVersionResource: configMapGVR, + } + namespaceAPIResource = informer.APIResourceMeta{ + GroupVersionKind: namespaceGVK, + GroupVersionResource: namespaceGVR, + IsClusterScoped: true, + } +) + +func TestAPIs(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Annotation Based Placement Controller Suite") +} + +var _ = BeforeSuite(func() { + ctx, cancel = context.WithCancel(context.TODO()) + + By("bootstrapping the test environment") + testEnv = &envtest.Environment{ + CRDDirectoryPaths: []string{filepath.Join("../../../", "config", "crd", "bases")}, + ErrorIfCRDPathMissing: true, + } + cfg, err := testEnv.Start() + Expect(err).Should(Succeed()) + Expect(cfg).NotTo(BeNil()) + + Expect(kfplacementv1alpha1.AddToScheme(scheme.Scheme)).Should(Succeed()) + + hubClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme}) + Expect(err).Should(Succeed()) + + dynamicClient, err := dynamic.NewForConfig(cfg) + Expect(err).Should(Succeed()) + + discoveryClient, err := discovery.NewDiscoveryClientForConfig(cfg) + Expect(err).Should(Succeed()) + groupResources, err := restmapper.GetAPIGroupResources(discoveryClient) + Expect(err).Should(Succeed()) + restMapper := restmapper.NewDiscoveryRESTMapper(groupResources) + + By("starting the informers the reconciler reads annotated resources from") + informerManager = informer.NewInformerManager(dynamicClient, 5*time.Minute, ctx.Done()) + informerManager.CreateInformerForResource(configMapAPIResource) + informerManager.CreateInformerForResource(namespaceAPIResource) + informerManager.Start() + informerManager.WaitForCacheSync() + + // The recorder is buffered generously: an unread event blocks the reconciler that records it, + // which would surface as a timeout somewhere unrelated rather than as a failed expectation. + eventRecorder = record.NewFakeRecorder(100) + reconciler = &Reconciler{ + Client: hubClient, + // The envtest client reads straight from the API server, so it serves as the uncached reader + // too; there is no separate cache to fall behind here. + UncachedReader: hubClient, + RestMapper: restMapper, + InformerManager: informerManager, + Recorder: eventRecorder, + } +}) + +var _ = AfterSuite(func() { + defer func() { + Expect(testEnv.Stop()).Should(Succeed()) + }() + cancel() +}) diff --git a/pkg/controllers/annotationplacement/watch.go b/pkg/controllers/annotationplacement/watch.go new file mode 100644 index 000000000..60a6cb7bb --- /dev/null +++ b/pkg/controllers/annotationplacement/watch.go @@ -0,0 +1,142 @@ +/* +Copyright 2026 The KubeFleet Authors. + +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 annotationplacement + +import ( + "fmt" + + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/tools/cache" + "k8s.io/klog/v2" + + kfplacementv1alpha1 "github.com/kubefleet-dev/kubefleet/apis/kubefleet.dev/placement/v1alpha1" + "github.com/kubefleet-dev/kubefleet/pkg/utils/informer" +) + +// SourceNamespace returns the namespace whose skip-status governs whether a source resource is +// placed. For an ordinary namespaced resource that is its metadata.namespace; a core Namespace +// object carries none and is itself the namespace, so its own name is returned. The skip check +// would otherwise read an empty namespace for a Namespace source and place one KubeFleet excludes. +func SourceNamespace(source *unstructured.Unstructured) string { + if gvk := source.GroupVersionKind(); gvk.Group == "" && gvk.Kind == "Namespace" { + return source.GetName() + } + return source.GetNamespace() +} + +// GeneratedPolicyResources returns the resources that hold the policies this controller generates, +// for the resource watcher to add informers for. +// +// Watching the generated policies, and not only the resources they are generated from, is what +// makes drift repairable: without it, deleting a generated policy or editing its spec produces no +// event on the resource it came from, and for a resource that never changes again the policy would +// stay missing or wrong forever. A resync does not step in either, since the resource watcher +// drops updates whose resource version did not move. +func GeneratedPolicyResources() []informer.APIResourceMeta { + return []informer.APIResourceMeta{ + { + GroupVersionKind: kfplacementv1alpha1.GroupVersion.WithKind("PlacementPolicy"), + GroupVersionResource: kfplacementv1alpha1.GroupVersion.WithResource("placementpolicies"), + }, + { + GroupVersionKind: kfplacementv1alpha1.GroupVersion.WithKind("ClusterPlacementPolicy"), + GroupVersionResource: kfplacementv1alpha1.GroupVersion.WithResource("clusterplacementpolicies"), + IsClusterScoped: true, + }, + } +} + +// NewGeneratedPolicyEventHandler returns the event handler for the generated policy informers. For +// every policy event it enqueues the resources the policy was generated from, so that the next +// reconciliation of those resources restores whatever the event changed. +func NewGeneratedPolicyEventHandler(enqueue func(obj interface{})) cache.ResourceEventHandler { + return cache.ResourceEventHandlerFuncs{ + AddFunc: func(obj interface{}) { + enqueueGeneratingResources(obj, enqueue) + }, + UpdateFunc: func(oldObj, newObj interface{}) { + oldMeta, err := meta.Accessor(oldObj) + if err != nil { + klog.ErrorS(err, "Failed to handle a generated policy update event", "oldObj", oldObj) + return + } + newMeta, err := meta.Accessor(newObj) + if err != nil { + klog.ErrorS(err, "Failed to handle a generated policy update event", "newObj", newObj) + return + } + if oldMeta.GetResourceVersion() == newMeta.GetResourceVersion() { + // A resync, not a change. + return + } + // Both sides are enqueued: an owner reference present only on the old side belongs to a + // resource whose policy this update just took away from it. + enqueueGeneratingResources(oldObj, enqueue) + enqueueGeneratingResources(newObj, enqueue) + }, + DeleteFunc: func(obj interface{}) { + enqueueGeneratingResources(obj, enqueue) + }, + } +} + +// enqueueGeneratingResources enqueues the resource that generated a policy, identified from the +// policy's owner references. +// +// Only the owner whose identity reproduces this policy's own generated name is enqueued. A policy +// may carry more than one owner reference -- a foreign one that applyDesiredPolicy deliberately +// preserves, or any owner on a hand-authored policy that happens to share these informers -- and +// following those would enqueue a key for a kind the resource watcher does not track, which +// sourceObject would answer by lazily creating an informer outside the resource configuration. +// Matching the generated name is exact, so only the true source passes. +// +// The owner reference is used rather than the parent labels because the labels are lossy (a long +// name is shortened to a prefix and a hash) and, being labels, can be stripped -- which is itself +// drift this path exists to repair. +func enqueueGeneratingResources(obj interface{}, enqueue func(obj interface{})) { + if tombstone, ok := obj.(cache.DeletedFinalStateUnknown); ok { + obj = tombstone.Obj + } + accessor, err := meta.Accessor(obj) + if err != nil { + klog.ErrorS(fmt.Errorf("object %+v is not a policy: %w", obj, err), "Skipped a generated policy event") + return + } + // A generated policy always lives in the namespace of the resource it came from, and a + // cluster-scoped one has none, matching a cluster-scoped owner. + policyName, policyNamespace := accessor.GetName(), accessor.GetNamespace() + for _, owner := range accessor.GetOwnerReferences() { + gv, err := schema.ParseGroupVersion(owner.APIVersion) + if err != nil { + klog.ErrorS(err, "Skipped an owner with an unparsable API version", "policy", klog.KObj(accessor), "apiVersion", owner.APIVersion) + continue + } + gvk := gv.WithKind(owner.Kind) + if generatedPolicyName(gvk, policyNamespace, owner.Name) != policyName { + // Not the owner this policy was generated from; following it would reach a resource + // the watcher never selected. + continue + } + source := &unstructured.Unstructured{} + source.SetGroupVersionKind(gvk) + source.SetNamespace(policyNamespace) + source.SetName(owner.Name) + enqueue(source) + } +} diff --git a/pkg/controllers/annotationplacement/watch_test.go b/pkg/controllers/annotationplacement/watch_test.go new file mode 100644 index 000000000..f907afbec --- /dev/null +++ b/pkg/controllers/annotationplacement/watch_test.go @@ -0,0 +1,229 @@ +/* +Copyright 2026 The KubeFleet Authors. + +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 annotationplacement + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/tools/cache" + + kfplacementv1alpha1 "github.com/kubefleet-dev/kubefleet/apis/kubefleet.dev/placement/v1alpha1" +) + +var configMapSourceGVK = schema.GroupVersionKind{Group: "", Version: "v1", Kind: "ConfigMap"} + +// generatedPolicyOwnedBy builds a policy as the informer would deliver it: correctly named for the +// resource that generated it, owned by that resource, plus any extra owner references a caller +// wants to plant. A policy whose name does not derive from an owner is that owner's, which is what +// lets the handler tell the true source from a foreign owner or a hand-authored policy. +func generatedPolicyOwnedBy(source *unstructured.Unstructured, resourceVersion string, extraOwners ...metav1.OwnerReference) *unstructured.Unstructured { + namespace := source.GetNamespace() + policy := &unstructured.Unstructured{} + if namespace == "" { + policy.SetGroupVersionKind(kfplacementv1alpha1.GroupVersion.WithKind("ClusterPlacementPolicy")) + } else { + policy.SetGroupVersionKind(kfplacementv1alpha1.GroupVersion.WithKind("PlacementPolicy")) + } + policy.SetNamespace(namespace) + policy.SetName(generatedPolicyName(source.GroupVersionKind(), namespace, source.GetName())) + policy.SetResourceVersion(resourceVersion) + policy.SetOwnerReferences(append([]metav1.OwnerReference{parentOwnerReference(source)}, extraOwners...)) + return policy +} + +// namedPolicy builds a policy with an explicit name (not derived from any owner), for the cases a +// generating source is not supposed to be found -- a hand-authored policy, or one whose only owners +// are foreign. +func namedPolicy(namespace, name, resourceVersion string, owners ...metav1.OwnerReference) *unstructured.Unstructured { + policy := &unstructured.Unstructured{} + policy.SetGroupVersionKind(kfplacementv1alpha1.GroupVersion.WithKind("PlacementPolicy")) + policy.SetNamespace(namespace) + policy.SetName(name) + policy.SetResourceVersion(resourceVersion) + policy.SetOwnerReferences(owners) + return policy +} + +func ownerRef(apiVersion, kind, name string) metav1.OwnerReference { + return metav1.OwnerReference{APIVersion: apiVersion, Kind: kind, Name: name, UID: "00000000-0000-0000-0000-000000000001"} +} + +// enqueuedIdentity is what these tests compare: the full identity of an enqueued resource, since +// getting the namespace or the group wrong sends the reconciliation to the wrong object entirely. +type enqueuedIdentity struct { + APIVersion, Kind, Namespace, Name string +} + +func identityOf(obj interface{}) enqueuedIdentity { + source := obj.(*unstructured.Unstructured) + return enqueuedIdentity{ + APIVersion: source.GetAPIVersion(), + Kind: source.GetKind(), + Namespace: source.GetNamespace(), + Name: source.GetName(), + } +} + +func TestGeneratedPolicyEventHandler(t *testing.T) { + deploymentSource := sourceObject(deploymentGVK, "prod", "web") + configMapSource := sourceObject(configMapSourceGVK, "prod", "app") + namespaceSource := sourceObject(namespaceGVK, "", "team") + + deploymentIdentity := enqueuedIdentity{APIVersion: "apps/v1", Kind: "Deployment", Namespace: "prod", Name: "web"} + + testCases := []struct { + name string + // event drives the handler under test. + event func(cache.ResourceEventHandler) + want []enqueuedIdentity + }{ + { + name: "a policy created enqueues the resource that generated it", + event: func(h cache.ResourceEventHandler) { + h.OnAdd(generatedPolicyOwnedBy(deploymentSource, "1"), false) + }, + want: []enqueuedIdentity{deploymentIdentity}, + }, + { + name: "a core group generating resource keeps its empty group", + event: func(h cache.ResourceEventHandler) { + h.OnAdd(generatedPolicyOwnedBy(configMapSource, "1"), false) + }, + want: []enqueuedIdentity{{APIVersion: "v1", Kind: "ConfigMap", Namespace: "prod", Name: "app"}}, + }, + { + // The finding this filter fixes: a foreign owner reference -- one applyDesiredPolicy + // preserves, or any owner on a policy that merely shares these informers -- must not be + // followed, or the watcher would enqueue a key for a kind it never selected. + name: "a foreign owner reference is not followed", + event: func(h cache.ResourceEventHandler) { + h.OnAdd(generatedPolicyOwnedBy(deploymentSource, "1", ownerRef("v1", "Pod", "some-pod"), ownerRef("apps/v1", "Deployment", "unrelated")), false) + }, + want: []enqueuedIdentity{deploymentIdentity}, + }, + { + // The drift event this watch exists for: someone strips the generating owner reference. + // The old side still names the source whose policy just lost its reference. + name: "an owner reference removed on update is still enqueued from the old side", + event: func(h cache.ResourceEventHandler) { + withOwner := generatedPolicyOwnedBy(deploymentSource, "1") + stripped := generatedPolicyOwnedBy(deploymentSource, "2") + stripped.SetOwnerReferences(nil) + h.OnUpdate(withOwner, stripped) + }, + want: []enqueuedIdentity{deploymentIdentity}, + }, + { + name: "a resync is not a change", + event: func(h cache.ResourceEventHandler) { + h.OnUpdate(generatedPolicyOwnedBy(deploymentSource, "1"), generatedPolicyOwnedBy(deploymentSource, "1")) + }, + want: nil, + }, + { + name: "a deleted policy enqueues the resource that generated it", + event: func(h cache.ResourceEventHandler) { + h.OnDelete(generatedPolicyOwnedBy(deploymentSource, "1")) + }, + want: []enqueuedIdentity{deploymentIdentity}, + }, + { + name: "a deleted policy arriving as a tombstone still enqueues its source", + event: func(h cache.ResourceEventHandler) { + h.OnDelete(cache.DeletedFinalStateUnknown{Key: "prod/x", Obj: generatedPolicyOwnedBy(deploymentSource, "1")}) + }, + want: []enqueuedIdentity{deploymentIdentity}, + }, + { + // A hand-authored policy is owned by nothing this controller generated: its name does + // not derive from its owner, so no owner is followed. + name: "a policy whose name does not derive from its owner enqueues nothing", + event: func(h cache.ResourceEventHandler) { + h.OnAdd(namedPolicy("prod", "hand-authored", "1", ownerRef("apps/v1", "Deployment", "web")), false) + }, + want: nil, + }, + { + name: "a policy with no owners enqueues nothing", + event: func(h cache.ResourceEventHandler) { + h.OnAdd(namedPolicy("prod", "hand-authored", "1"), false) + }, + want: nil, + }, + { + name: "a cluster scoped policy enqueues a cluster scoped source", + event: func(h cache.ResourceEventHandler) { + h.OnAdd(generatedPolicyOwnedBy(namespaceSource, "1"), false) + }, + want: []enqueuedIdentity{{APIVersion: "v1", Kind: "Namespace", Namespace: "", Name: "team"}}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var got []enqueuedIdentity + handler := NewGeneratedPolicyEventHandler(func(obj interface{}) { + got = append(got, identityOf(obj)) + }) + + tc.event(handler) + + if diff := cmp.Diff(got, tc.want, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("enqueued resources mismatch (-got, +want):\n%s", diff) + } + }) + } +} + +// TestSourceNamespace covers the skip-check namespace: a namespaced resource uses its own +// namespace, but a Namespace object -- which carries none -- is itself the namespace. +func TestSourceNamespace(t *testing.T) { + testCases := []struct { + name string + source *unstructured.Unstructured + want string + }{ + { + name: "a namespaced resource uses its metadata namespace", + source: sourceObject(deploymentGVK, "prod", "web"), + want: "prod", + }, + { + name: "a namespace object uses its own name", + source: sourceObject(namespaceGVK, "", "kube-system"), + want: "kube-system", + }, + { + name: "a cluster scoped non-namespace resource has no namespace", + source: sourceObject(schema.GroupVersionKind{Group: "rbac.authorization.k8s.io", Version: "v1", Kind: "ClusterRole"}, "", "admin"), + want: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if got := SourceNamespace(tc.source); got != tc.want { + t.Errorf("SourceNamespace(%s) = %q, want %q", tc.source.GetName(), got, tc.want) + } + }) + } +} diff --git a/pkg/controllers/membercluster/v1beta1/membercluster_controller.go b/pkg/controllers/membercluster/v1beta1/membercluster_controller.go index 1b4cf8b0e..53c474de7 100644 --- a/pkg/controllers/membercluster/v1beta1/membercluster_controller.go +++ b/pkg/controllers/membercluster/v1beta1/membercluster_controller.go @@ -42,6 +42,7 @@ import ( "github.com/kubefleet-dev/kubefleet/apis" clusterv1beta1 "github.com/kubefleet-dev/kubefleet/apis/cluster/v1beta1" + kfplacementv1alpha1 "github.com/kubefleet-dev/kubefleet/apis/kubefleet.dev/placement/v1alpha1" placementv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1" sharedmetrics "github.com/kubefleet-dev/kubefleet/pkg/metrics/shared" "github.com/kubefleet-dev/kubefleet/pkg/utils" @@ -75,6 +76,12 @@ type Reconciler struct { MaxConcurrentReconciles int // the wait time in minutes before we force delete a member cluster. ForceDeleteWaitTime time.Duration + + // SeedClusterAliasLabel controls whether joining member clusters are given the cluster alias + // label, seeded from the cluster name. It follows the annotation-based placement feature flag: + // the alias exists for that feature's alias= shorthand, and seeding it on fleets that do not + // run the feature would relabel every member cluster for nothing. + SeedClusterAliasLabel bool // agents are used as hashset to query the expected agent type, so the value will be ignored. agents map[clusterv1beta1.AgentType]bool } @@ -283,16 +290,30 @@ func (r *Reconciler) ensureFinalizer(ctx context.Context, mc *clusterv1beta1.Mem // ensureMemberNameLabel makes sure that the member cluster has a label with its own name. // This enables selecting clusters by name in ResourceOverride and ClusterResourceOverride via labelSelector. func (r *Reconciler) ensureMemberNameLabel(ctx context.Context, mc *clusterv1beta1.MemberCluster) error { - if mc.Labels != nil && mc.Labels[placementv1beta1.MemberNameLabel] == mc.Name { - return nil - } - + changed := false if mc.Labels == nil { mc.Labels = make(map[string]string) } - mc.Labels[placementv1beta1.MemberNameLabel] = mc.Name - klog.InfoS("Ensured the member cluster name label", "memberCluster", klog.KObj(mc)) + if mc.Labels[placementv1beta1.MemberNameLabel] != mc.Name { + mc.Labels[placementv1beta1.MemberNameLabel] = mc.Name + changed = true + } + + // The alias label is seeded from the cluster name, but only when it is absent entirely. Unlike + // the name label above, which states a fact this controller owns and reasserts, the alias + // exists to be renamed: it is the level of indirection that lets an admin point a selector at + // "the cluster playing this role" rather than at a fixed name. Reasserting it here would + // silently revert an admin's alias on the next reconcile. + if _, found := mc.Labels[kfplacementv1alpha1.ClusterAliasLabel]; r.SeedClusterAliasLabel && !found { + mc.Labels[kfplacementv1alpha1.ClusterAliasLabel] = mc.Name + changed = true + } + + if !changed { + return nil + } + klog.InfoS("Ensured the member cluster name and alias labels", "memberCluster", klog.KObj(mc)) return r.Update(ctx, mc, client.FieldOwner(utils.MCControllerFieldManagerName)) } diff --git a/pkg/controllers/membercluster/v1beta1/membercluster_controller_test.go b/pkg/controllers/membercluster/v1beta1/membercluster_controller_test.go index 6fa477eaf..8d5c51075 100644 --- a/pkg/controllers/membercluster/v1beta1/membercluster_controller_test.go +++ b/pkg/controllers/membercluster/v1beta1/membercluster_controller_test.go @@ -39,6 +39,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" clusterv1beta1 "github.com/kubefleet-dev/kubefleet/apis/cluster/v1beta1" + kfplacementv1alpha1 "github.com/kubefleet-dev/kubefleet/apis/kubefleet.dev/placement/v1alpha1" placementv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1" "github.com/kubefleet-dev/kubefleet/pkg/utils" "github.com/kubefleet-dev/kubefleet/pkg/utils/controller" @@ -71,26 +72,54 @@ func TestEnsureMemberNameLabel(t *testing.T) { wantLabels map[string]string wantErr string }{ - "label already present with correct value": { + "name and alias labels already present with correct values": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ - MockUpdate: test.NewMockUpdateFn(fmt.Errorf("update should not be called when label is already correct")), + MockUpdate: test.NewMockUpdateFn(fmt.Errorf("update should not be called when the labels are already correct")), }, }, memberCluster: &clusterv1beta1.MemberCluster{ ObjectMeta: metav1.ObjectMeta{ Name: "mc1", Labels: map[string]string{ - placementv1beta1.MemberNameLabel: "mc1", + placementv1beta1.MemberNameLabel: "mc1", + kfplacementv1alpha1.ClusterAliasLabel: "mc1", }, }, }, wantLabels: map[string]string{ - placementv1beta1.MemberNameLabel: "mc1", + placementv1beta1.MemberNameLabel: "mc1", + kfplacementv1alpha1.ClusterAliasLabel: "mc1", + }, + }, + // The alias is the admin's to rename: unlike the name label, a different value is left + // alone rather than reasserted, since the alias exists precisely so that a selector can + // follow a role while the cluster behind it changes. + "an alias renamed by an admin is not reverted": { + r: &Reconciler{ + SeedClusterAliasLabel: true, + Client: &test.MockClient{ + MockUpdate: test.NewMockUpdateFn(fmt.Errorf("update should not be called when the alias was deliberately renamed")), + }, + }, + memberCluster: &clusterv1beta1.MemberCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mc1", + Labels: map[string]string{ + placementv1beta1.MemberNameLabel: "mc1", + kfplacementv1alpha1.ClusterAliasLabel: "bravelion", + }, + }, + }, + wantLabels: map[string]string{ + placementv1beta1.MemberNameLabel: "mc1", + kfplacementv1alpha1.ClusterAliasLabel: "bravelion", }, }, "no labels at all": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockUpdate: func(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { return nil @@ -103,11 +132,13 @@ func TestEnsureMemberNameLabel(t *testing.T) { }, }, wantLabels: map[string]string{ - placementv1beta1.MemberNameLabel: "mc1", + placementv1beta1.MemberNameLabel: "mc1", + kfplacementv1alpha1.ClusterAliasLabel: "mc1", }, }, "labels exist but name label is missing": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockUpdate: func(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { return nil @@ -123,12 +154,14 @@ func TestEnsureMemberNameLabel(t *testing.T) { }, }, wantLabels: map[string]string{ - "existing-label": "value", - placementv1beta1.MemberNameLabel: "mc1", + "existing-label": "value", + placementv1beta1.MemberNameLabel: "mc1", + kfplacementv1alpha1.ClusterAliasLabel: "mc1", }, }, "label present with wrong value": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockUpdate: func(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { return nil @@ -143,12 +176,56 @@ func TestEnsureMemberNameLabel(t *testing.T) { }, }, }, + wantLabels: map[string]string{ + placementv1beta1.MemberNameLabel: "mc1", + kfplacementv1alpha1.ClusterAliasLabel: "mc1", + }, + }, + // The day-2 scenario: a member cluster labeled by the controller before the alias existed. + // Only the alias branch has anything to do, and it alone must drive the update. + "name label correct, alias absent, alias alone drives the update": { + r: &Reconciler{ + SeedClusterAliasLabel: true, + Client: &test.MockClient{ + MockUpdate: func(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { + return nil + }, + }, + }, + memberCluster: &clusterv1beta1.MemberCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mc1", + Labels: map[string]string{ + placementv1beta1.MemberNameLabel: "mc1", + }, + }, + }, + wantLabels: map[string]string{ + placementv1beta1.MemberNameLabel: "mc1", + kfplacementv1alpha1.ClusterAliasLabel: "mc1", + }, + }, + "no alias is seeded while the feature is off": { + r: &Reconciler{ + Client: &test.MockClient{ + MockUpdate: test.NewMockUpdateFn(fmt.Errorf("update should not be called when the name label is correct and seeding is off")), + }, + }, + memberCluster: &clusterv1beta1.MemberCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mc1", + Labels: map[string]string{ + placementv1beta1.MemberNameLabel: "mc1", + }, + }, + }, wantLabels: map[string]string{ placementv1beta1.MemberNameLabel: "mc1", }, }, "update error": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockUpdate: func(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { return errors.New("update failed") @@ -225,6 +302,7 @@ func TestSyncNamespace(t *testing.T) { }{ "namespace doesn't exist": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error { return apierrors.NewNotFound(schema.GroupResource{}, "") @@ -242,6 +320,7 @@ func TestSyncNamespace(t *testing.T) { }, "namespace exists without label": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error { o := obj.(*corev1.Namespace) @@ -266,6 +345,7 @@ func TestSyncNamespace(t *testing.T) { }, "namespace exists with label": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error { o := obj.(*corev1.Namespace) @@ -285,6 +365,7 @@ func TestSyncNamespace(t *testing.T) { }, "namespace create error": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error { return apierrors.NewNotFound(schema.GroupResource{}, "") @@ -300,6 +381,7 @@ func TestSyncNamespace(t *testing.T) { }, "namespace get error": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error { return errors.New("namespace cannot be retrieved") @@ -312,6 +394,7 @@ func TestSyncNamespace(t *testing.T) { }, "namespace patch error": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error { o := obj.(*corev1.Namespace) @@ -368,6 +451,7 @@ func TestSyncRole(t *testing.T) { }{ "role exists but no diff": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error { o := obj.(*rbacv1.Role) @@ -393,6 +477,7 @@ func TestSyncRole(t *testing.T) { }, "role exists but with diff": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error { o := obj.(*rbacv1.Role) @@ -418,6 +503,7 @@ func TestSyncRole(t *testing.T) { }, "role doesn't exist": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error { return apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "Namespace"}, "namespace") @@ -436,6 +522,7 @@ func TestSyncRole(t *testing.T) { }, "role create error": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error { return apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "Namespace"}, "namespace") @@ -451,6 +538,7 @@ func TestSyncRole(t *testing.T) { }, "role get error": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error { return errors.New("role cannot be retrieved") @@ -464,6 +552,7 @@ func TestSyncRole(t *testing.T) { }, "role update error": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error { o := obj.(*rbacv1.Role) @@ -553,6 +642,7 @@ func TestSyncRoleBinding(t *testing.T) { }{ "role binding but no diff": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error { roleRef := rbacv1.RoleRef{ @@ -587,6 +677,7 @@ func TestSyncRoleBinding(t *testing.T) { }, "identity without APIGroup should not trigger roleBinding reconcile": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error { roleRef := rbacv1.RoleRef{ @@ -621,6 +712,7 @@ func TestSyncRoleBinding(t *testing.T) { }, "role binding but with diff": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error { roleRef := rbacv1.RoleRef{ @@ -654,6 +746,7 @@ func TestSyncRoleBinding(t *testing.T) { }, "role binding doesn't exist": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error { return apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "Namespace"}, "namespace") @@ -669,6 +762,7 @@ func TestSyncRoleBinding(t *testing.T) { }, "role binding create error": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error { return apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "Namespace"}, "namespace") @@ -685,6 +779,7 @@ func TestSyncRoleBinding(t *testing.T) { }, "role binding get error": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error { return errors.New("role binding cannot be retrieved") @@ -701,6 +796,7 @@ func TestSyncRoleBinding(t *testing.T) { }, "role binding update error": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error { return nil @@ -778,6 +874,7 @@ func TestSyncInternalMemberCluster(t *testing.T) { }{ "internal member cluster exists and spec is updated": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockUpdate: updateMock}, recorder: utils.NewFakeRecorder(1), @@ -794,6 +891,7 @@ func TestSyncInternalMemberCluster(t *testing.T) { }, "internal member cluster exists and spec is not updated ": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockUpdate: updateMock}, }, @@ -825,6 +923,7 @@ func TestSyncInternalMemberCluster(t *testing.T) { }, "internal member cluster gets created": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockCreate: createMock}, recorder: utils.NewFakeRecorder(1), @@ -838,6 +937,7 @@ func TestSyncInternalMemberCluster(t *testing.T) { }, "internal member cluster create error": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockCreate: createMock}, }, @@ -1900,6 +2000,7 @@ func TestHandleDelete(t *testing.T) { }, "Remove the namespace when the imc does not exist": { r: &Reconciler{ + SeedClusterAliasLabel: true, Client: &test.MockClient{ MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error { if key.Namespace == "" { diff --git a/pkg/resourcewatcher/change_detector.go b/pkg/resourcewatcher/change_detector.go index e2d82d2e7..4dfeefdce 100644 --- a/pkg/resourcewatcher/change_detector.go +++ b/pkg/resourcewatcher/change_detector.go @@ -29,6 +29,7 @@ import ( "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/manager" + "github.com/kubefleet-dev/kubefleet/pkg/controllers/annotationplacement" "github.com/kubefleet-dev/kubefleet/pkg/utils" "github.com/kubefleet-dev/kubefleet/pkg/utils/controller" "github.com/kubefleet-dev/kubefleet/pkg/utils/informer" @@ -67,6 +68,14 @@ type ChangeDetector struct { // This controller will be used by both v1alpha1 & v1beta1 ClusterResourcePlacementController. ResourceChangeController controller.Controller + // AnnotationPlacementController maintains a rate limited queue holding the cluster wide key of + // every resource whose cluster-selectors annotation may have changed, and a reconcile function + // that keeps the placement policy generated from that annotation in step with it. + // + // It is nil unless annotation-based placement is enabled, and unlike ResourceChangeController it + // is fed only the resources that carry the annotation. + AnnotationPlacementController controller.Controller + // InformerManager manages all the dynamic informers created by the discovery client InformerManager informer.Manager @@ -98,6 +107,8 @@ func (d *ChangeDetector) Start(ctx context.Context) error { // set up the dynamicResourceChangeEventHandler that enqueue an event to the resource change controller's queue. dynamicResourceChangeEventHandler := newFilteringHandlerOnAllEvents(d.dynamicResourceFilter, d.onResourceAdded, d.onResourceUpdated, d.onResourceDeleted) + + d.watchGeneratedPolicies() // run the resource type list once to start informers for the existing resources d.discoverResources(dynamicResourceChangeEventHandler) defer d.InformerManager.Stop() @@ -126,9 +137,32 @@ func (d *ChangeDetector) Start(ctx context.Context) error { errs.Go(func() error { return d.ResourceChangeController.Run(cctx, d.ConcurrentResourceChangeWorker) }) + if d.AnnotationPlacementController != nil { + errs.Go(func() error { + return d.AnnotationPlacementController.Run(cctx, d.ConcurrentResourceChangeWorker) + }) + } return errs.Wait() } +// watchGeneratedPolicies watches the policies that annotation-based placement generates, on top of +// the resources they are generated from: an edit or a delete of a generated policy produces no +// event on its resource, and would otherwise go unrepaired forever. It does nothing when +// annotation-based placement is not running. +// +// The policy informers are registered as static resources, and the resource config keeps the whole +// placement.kubefleet.dev group out of dynamic discovery, so the generated policy event handler is +// the only handler these informers ever get. +func (d *ChangeDetector) watchGeneratedPolicies() { + if d.AnnotationPlacementController == nil { + return + } + generatedPolicyEventHandler := annotationplacement.NewGeneratedPolicyEventHandler(d.AnnotationPlacementController.Enqueue) + for _, res := range annotationplacement.GeneratedPolicyResources() { + d.InformerManager.AddStaticResource(res, generatedPolicyEventHandler) + } +} + // discoverAPIResourcesLoop runs discoverResources periodically func (d *ChangeDetector) discoverAPIResourcesLoop(ctx context.Context, period time.Duration, dynamicResourceEventHandler cache.ResourceEventHandler) { wait.UntilWithContext(ctx, func(ctx context.Context) { @@ -154,6 +188,14 @@ func (d *ChangeDetector) discoverResources(dynamicResourceEventHandler cache.Res // dynamicResourceFilter filters out resources that we don't want to watch. func (d *ChangeDetector) dynamicResourceFilter(obj any) bool { + // A deletion the watch missed arrives as a tombstone wrapping the object's final state, not as + // the object itself. It has to be unwrapped before anything here inspects the object; filtering + // on the tombstone would silently drop every relist-detected deletion, since a tombstone is not + // a runtime object and fails the key derivation below. + if tombstone, ok := obj.(cache.DeletedFinalStateUnknown); ok { + obj = tombstone.Obj + } + key, err := controller.ClusterWideKeyFunc(obj) if err != nil { return false diff --git a/pkg/resourcewatcher/change_detector_test.go b/pkg/resourcewatcher/change_detector_test.go index cb666b7bc..19d32f594 100644 --- a/pkg/resourcewatcher/change_detector_test.go +++ b/pkg/resourcewatcher/change_detector_test.go @@ -211,10 +211,22 @@ func TestChangeDetector_dynamicResourceFilter(t *testing.T) { want: false, }, { - // Tombstones from informer cache deletions are not unwrapped by ClusterWideKeyFunc, - // so the filter rejects them. The downstream delete handler unwraps tombstones separately. - name: "tombstone object is filtered out", + // A relist-detected deletion arrives as a tombstone wrapping the object's final state. + // The filter must judge the wrapped object, not the tombstone: rejecting tombstones + // wholesale would silently drop every such deletion before the delete handler -- which + // is what unwraps them for use -- ever saw it. + name: "tombstone wrapping a watched object passes the filter", obj: cache.DeletedFinalStateUnknown{Key: "default/cm", Obj: unstructuredConfigMap("default", "cm")}, + want: true, + }, + { + name: "tombstone wrapping an object in a skipped namespace is filtered out", + obj: cache.DeletedFinalStateUnknown{Key: "kube-system/cm", Obj: unstructuredConfigMap("kube-system", "cm")}, + want: false, + }, + { + name: "tombstone wrapping garbage is filtered out", + obj: cache.DeletedFinalStateUnknown{Key: "default/cm", Obj: "not-a-runtime-object"}, want: false, }, { diff --git a/pkg/resourcewatcher/change_detector_watch_test.go b/pkg/resourcewatcher/change_detector_watch_test.go new file mode 100644 index 000000000..9e5c8f883 --- /dev/null +++ b/pkg/resourcewatcher/change_detector_watch_test.go @@ -0,0 +1,68 @@ +/* +Copyright 2026 The KubeFleet Authors. + +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 resourcewatcher + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + + "github.com/kubefleet-dev/kubefleet/pkg/controllers/annotationplacement" + "github.com/kubefleet-dev/kubefleet/pkg/utils/informer" + testinformer "github.com/kubefleet-dev/kubefleet/test/utils/informer" +) + +// TestWatchGeneratedPolicies pins the wiring that makes generated-policy drift repairable: with +// annotation-based placement running, the detector registers an informer for each generated policy +// resource, and with it off, it registers none. +func TestWatchGeneratedPolicies(t *testing.T) { + testCases := []struct { + name string + controller *recordingController + want []informer.APIResourceMeta + }{ + { + name: "the generated policy resources are watched when the feature runs", + controller: &recordingController{}, + want: annotationplacement.GeneratedPolicyResources(), + }, + { + name: "nothing is watched when the feature is off", + controller: nil, + want: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + manager := &testinformer.FakeManager{} + detector := &ChangeDetector{InformerManager: manager} + if tc.controller != nil { + detector.AnnotationPlacementController = tc.controller + } + + detector.watchGeneratedPolicies() + + // APIResourceMeta has unexported bookkeeping fields; comparing the comparable value as a + // whole covers the exported identity without reaching into them. + if diff := cmp.Diff(manager.StaticResources, tc.want, cmpopts.EquateEmpty(), cmpopts.EquateComparable(informer.APIResourceMeta{})); diff != "" { + t.Errorf("watchGeneratedPolicies() registered resources mismatch (-got, +want):\n%s", diff) + } + }) + } +} diff --git a/pkg/resourcewatcher/event_handlers.go b/pkg/resourcewatcher/event_handlers.go index db8ea1db6..05abecdf2 100644 --- a/pkg/resourcewatcher/event_handlers.go +++ b/pkg/resourcewatcher/event_handlers.go @@ -24,6 +24,8 @@ import ( "k8s.io/client-go/tools/cache" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/kubefleet-dev/kubefleet/pkg/controllers/annotationplacement" ) // handleTombStoneObj handles the case that the delete object is a tombStone instead of the real object @@ -60,6 +62,9 @@ func (d *ChangeDetector) onResourceAdded(obj interface{}) { klog.V(3).InfoS("A resource is added", "obj", klog.KObj(metaInfo), "gvk", runtimeObject.GetObjectKind().GroupVersionKind().String()) d.ResourceChangeController.Enqueue(obj) + if annotationplacement.HasClusterSelectorsAnnotation(metaInfo) { + d.enqueueForAnnotationPlacement(obj) + } } // onResourceUpdated handles object update event and push the updated object to the resource queue. @@ -83,6 +88,12 @@ func (d *ChangeDetector) onResourceUpdated(oldObj, newObj interface{}) { klog.V(3).InfoS("A resource is updated", "obj", oldObjMeta.GetName(), "namespace", oldObjMeta.GetNamespace(), "gvk", runtimeObject.GetObjectKind().GroupVersionKind().String()) d.ResourceChangeController.Enqueue(newObj) + // The old object is checked as well as the new one, because the update that matters most to + // annotation-based placement is the one that removes the annotation: looking only at the new + // object would filter that event out and leave the generated policy behind forever. + if annotationplacement.HasClusterSelectorsAnnotation(newObjMeta) || annotationplacement.HasClusterSelectorsAnnotation(oldObjMeta) { + d.enqueueForAnnotationPlacement(newObj) + } return } klog.V(4).InfoS("Received a resource updated event with no change", "obj", oldObjMeta.GetName(), @@ -98,4 +109,21 @@ func (d *ChangeDetector) onResourceDeleted(obj interface{}) { } klog.V(3).InfoS("A resource is deleted", "obj", klog.KObj(clientObj), "gvk", clientObj.GetObjectKind().GroupVersionKind().String()) d.ResourceChangeController.Enqueue(clientObj) + // The generated policy is deleted by the reconciler rather than left to garbage collection, + // which would keep the policy for as long as any other party's owner reference on it survives. + // This callback also stands in for more than deletion: a resource that stops passing the + // dynamic resource filter without being deleted is reported here too, and its generated policy + // has to go the same way, since no further event about the resource will ever be seen. + if annotationplacement.HasClusterSelectorsAnnotation(clientObj) { + d.enqueueForAnnotationPlacement(clientObj) + } +} + +// enqueueForAnnotationPlacement hands an object to the annotation-based placement controller, if +// that feature is running at all. +func (d *ChangeDetector) enqueueForAnnotationPlacement(obj interface{}) { + if d.AnnotationPlacementController == nil { + return + } + d.AnnotationPlacementController.Enqueue(obj) } diff --git a/pkg/resourcewatcher/event_handlers_test.go b/pkg/resourcewatcher/event_handlers_test.go index 5f01866a6..945d91e6f 100644 --- a/pkg/resourcewatcher/event_handlers_test.go +++ b/pkg/resourcewatcher/event_handlers_test.go @@ -19,15 +19,22 @@ package resourcewatcher import ( "context" "reflect" + "sync" "testing" - fleetv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/tools/cache" "sigs.k8s.io/controller-runtime/pkg/client" + kfplacementv1alpha1 "github.com/kubefleet-dev/kubefleet/apis/kubefleet.dev/placement/v1alpha1" + fleetv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1" "github.com/kubefleet-dev/kubefleet/pkg/utils/controller" ) @@ -136,3 +143,183 @@ func (t *fakeController) Enqueue(_ interface{}) { func (t *fakeController) Run(_ context.Context, _ int) error { return nil } + +var _ controller.Controller = &recordingController{} + +// recordingController stands in for a real controller queue, remembering what was handed to it. +type recordingController struct { + mu sync.Mutex + objects []interface{} +} + +func (c *recordingController) Enqueue(obj interface{}) { + c.mu.Lock() + defer c.mu.Unlock() + c.objects = append(c.objects, obj) +} + +func (c *recordingController) Run(context.Context, int) error { return nil } + +// names returns the name of every object enqueued, which is enough to tell the objects in these +// tests apart. +func (c *recordingController) names(t *testing.T) []string { + t.Helper() + c.mu.Lock() + defer c.mu.Unlock() + names := make([]string, 0, len(c.objects)) + for _, obj := range c.objects { + accessor, err := meta.Accessor(obj) + if err != nil { + t.Fatalf("meta.Accessor(%v) = %v, want no error", obj, err) + } + names = append(names, accessor.GetName()) + } + return names +} + +// watchedResource builds a resource of the kind the dynamic informers hand to the event handlers. +func watchedResource(name, resourceVersion string, annotated bool) *unstructured.Unstructured { + object := &unstructured.Unstructured{} + object.SetGroupVersionKind(deploymentGVK()) + object.SetNamespace("prod") + object.SetName(name) + object.SetResourceVersion(resourceVersion) + if annotated { + object.SetAnnotations(map[string]string{kfplacementv1alpha1.ClusterSelectorsAnnotation: "env=staging"}) + } + return object +} + +func deploymentGVK() schema.GroupVersionKind { + return schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"} +} + +// TestEventHandlersEnqueueForAnnotationPlacement covers which events reach the annotation-based +// placement controller. Getting this wrong is not visible in the controller itself: an event that is +// never enqueued leaves the generated policy exactly as it was, with nothing to show that anything +// was missed. +func TestEventHandlersEnqueueForAnnotationPlacement(t *testing.T) { + testCases := []struct { + name string + // event runs the handler under test against the detector. + event func(*ChangeDetector) + // wantResourceChange and wantAnnotationPlacement hold the names enqueued to each queue. + wantResourceChange []string + wantAnnotationPlacement []string + }{ + { + name: "an added resource with the annotation is enqueued to both", + event: func(d *ChangeDetector) { + d.onResourceAdded(watchedResource("annotated", "1", true)) + }, + wantResourceChange: []string{"annotated"}, + wantAnnotationPlacement: []string{"annotated"}, + }, + { + name: "an added resource without the annotation is kept out of the placement queue", + event: func(d *ChangeDetector) { + d.onResourceAdded(watchedResource("plain", "1", false)) + }, + wantResourceChange: []string{"plain"}, + wantAnnotationPlacement: nil, + }, + { + // The event this whole filter has to get right: looking only at the new object would + // drop it, and the policy generated from the old annotation would never be deleted. + name: "removing the annotation is still enqueued", + event: func(d *ChangeDetector) { + d.onResourceUpdated(watchedResource("annotated", "1", true), watchedResource("annotated", "2", false)) + }, + wantResourceChange: []string{"annotated"}, + wantAnnotationPlacement: []string{"annotated"}, + }, + { + name: "adding the annotation is enqueued", + event: func(d *ChangeDetector) { + d.onResourceUpdated(watchedResource("annotated", "1", false), watchedResource("annotated", "2", true)) + }, + wantResourceChange: []string{"annotated"}, + wantAnnotationPlacement: []string{"annotated"}, + }, + { + name: "an update to a resource that never carried the annotation is kept out", + event: func(d *ChangeDetector) { + d.onResourceUpdated(watchedResource("plain", "1", false), watchedResource("plain", "2", false)) + }, + wantResourceChange: []string{"plain"}, + wantAnnotationPlacement: nil, + }, + { + name: "an update that changed nothing is enqueued nowhere", + event: func(d *ChangeDetector) { + d.onResourceUpdated(watchedResource("annotated", "1", true), watchedResource("annotated", "1", true)) + }, + wantResourceChange: nil, + wantAnnotationPlacement: nil, + }, + { + // The reconciler deletes the generated policy itself: garbage collection would keep it + // for as long as any other party's owner reference on it survives. This callback also + // stands in for a resource that stops passing the dynamic resource filter without being + // deleted, after which no further event about it is ever seen. + name: "a deleted annotated resource is enqueued so its policy is cleaned up", + event: func(d *ChangeDetector) { + d.onResourceDeleted(watchedResource("annotated", "1", true)) + }, + wantResourceChange: []string{"annotated"}, + wantAnnotationPlacement: []string{"annotated"}, + }, + { + name: "a deleted resource without the annotation is kept out of the placement queue", + event: func(d *ChangeDetector) { + d.onResourceDeleted(watchedResource("plain", "1", false)) + }, + wantResourceChange: []string{"plain"}, + wantAnnotationPlacement: nil, + }, + { + name: "a deleted annotated resource arriving as a tombstone is enqueued too", + event: func(d *ChangeDetector) { + d.onResourceDeleted(cache.DeletedFinalStateUnknown{Key: "prod/annotated", Obj: watchedResource("annotated", "1", true)}) + }, + wantResourceChange: []string{"annotated"}, + wantAnnotationPlacement: []string{"annotated"}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + resourceChange, annotationPlacement := &recordingController{}, &recordingController{} + detector := &ChangeDetector{ + ResourceChangeController: resourceChange, + AnnotationPlacementController: annotationPlacement, + } + + tc.event(detector) + + if diff := cmp.Diff(resourceChange.names(t), tc.wantResourceChange, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("resource change queue mismatch (-got, +want):\n%s", diff) + } + if diff := cmp.Diff(annotationPlacement.names(t), tc.wantAnnotationPlacement, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("annotation placement queue mismatch (-got, +want):\n%s", diff) + } + }) + } +} + +// TestEventHandlersWithAnnotationPlacementDisabled covers the hub agent running without the feature, +// where the controller is nil. Every event still has to reach the resource change controller. +func TestEventHandlersWithAnnotationPlacementDisabled(t *testing.T) { + resourceChange := &recordingController{} + detector := &ChangeDetector{ResourceChangeController: resourceChange} + + annotated := watchedResource("annotated", "1", true) + detector.onResourceAdded(annotated) + detector.onResourceUpdated(annotated, watchedResource("annotated", "2", false)) + detector.onResourceDeleted(annotated) + + want := []string{"annotated", "annotated", "annotated"} + if diff := cmp.Diff(resourceChange.names(t), want); diff != "" { + t.Errorf("resource change queue mismatch (-got, +want):\n%s", diff) + } +} diff --git a/pkg/utils/apiresources.go b/pkg/utils/apiresources.go index d81141dd6..727be57a5 100644 --- a/pkg/utils/apiresources.go +++ b/pkg/utils/apiresources.go @@ -29,6 +29,7 @@ import ( metricsV1beta1 "k8s.io/metrics/pkg/apis/metrics/v1beta1" clusterv1beta1 "github.com/kubefleet-dev/kubefleet/apis/cluster/v1beta1" + kfplacementv1alpha1 "github.com/kubefleet-dev/kubefleet/apis/kubefleet.dev/placement/v1alpha1" placementv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1" ) @@ -207,6 +208,14 @@ func NewResourceConfig(isAllowList bool) *ResourceConfig { // disable cluster group by default r.AddGroup(clusterv1beta1.GroupVersion.Group) + // Disable the placement.kubefleet.dev group wholesale: everything in it is KubeFleet's own + // bookkeeping (placement policies, bindings, snapshots, works, claims), including the policies + // that annotation-based placement generates. Treating those as placeable resources would let a + // placement that selects a whole namespace propagate KubeFleet's own generated objects, and + // would put a second, filtered event handler on the informers that the generated-policy watch + // registers first. + r.AddGroup(kfplacementv1alpha1.GroupVersion.Group) + // disable some fleet networking resources r.AddGroupKind(serviceImportGK) r.AddGroupKind(trafficManagerProfileGK) diff --git a/pkg/utils/naming/naming.go b/pkg/utils/naming/naming.go new file mode 100644 index 000000000..5d0eea89c --- /dev/null +++ b/pkg/utils/naming/naming.go @@ -0,0 +1,149 @@ +/* +Copyright 2026 The KubeFleet Authors. + +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 naming derives the names and label values of objects that KubeFleet generates on a +// user's behalf. +// +// A controller that names an object after another one runs into the same two problems every time: +// a name legal for a Kubernetes object is not always legal where the controller puts it (object +// names run to 253 bytes, label values stop at 63), and shortening a name to fit can both produce +// a syntactically invalid result and collapse two distinct identities onto one string. Getting +// either wrong fails at the API server rather than at the call site, which surfaces as a +// reconciler that retries forever while the user sees nothing happen at all. +// +// The helpers here exist so that the answer is written once. They are deliberately primitive: +// callers compose the identity that is hashed, since only the caller knows what makes one of its +// objects distinct from another. +package naming + +import ( + "crypto/sha256" + "encoding/hex" + "fmt" + "strings" + + "k8s.io/apimachinery/pkg/util/validation" +) + +const ( + // HashLength is the number of hexadecimal characters of Hash's output, i.e. 64 bits of a + // SHA-256 sum. A narrower hash is tempting and wrong: these hashes are what keep two distinct + // objects from generating one name, and whichever of them loses the race is not merely + // misnamed, it silently never gets its object at all. + HashLength = 16 + + // labelValuePrefixMaxLength is how much of an over-long value survives in front of the hash + // that LabelValue appends, allowing for the separator between the two. + labelValuePrefixMaxLength = validation.LabelValueMaxLength - HashLength - 1 + + // separators are the characters a Kubernetes name may contain but may not begin or end with. + separators = "-_." +) + +// Hash returns a short, stable, collision-resistant digest of s. +// +// Callers should hash the whole identity of an object, never the shortened form of it that appears +// in a generated name: shortening is precisely what collapses two identities into one, so a hash +// taken afterwards would agree where the identities differ. +func Hash(s string) string { + sum := sha256.Sum256([]byte(s)) + return hex.EncodeToString(sum[:])[:HashLength] +} + +// TrimSeparators removes the separators that a name may not end with, which truncating an +// otherwise valid name can leave exposed. +// +// The result can be empty only if the fragment consisted entirely of separators; since Kubernetes +// object names must begin with an alphanumeric character, a fragment taken from the front of one +// never can. +func TrimSeparators(fragment string) string { + return strings.TrimRight(fragment, separators) +} + +// Truncate shortens s to at most maxLength bytes, leaving a result that is still legal at the end +// of a Kubernetes name. +// +// It does not make the result unique; a caller that truncates must append Hash of the full, +// untruncated identity for that. +// +// A budget of zero or less leaves nothing, which is returned as the empty string rather than as a +// panic: this package exists so that a caller's arithmetic mistake surfaces as a name it can +// inspect, not as a crash in a reconcile loop. +func Truncate(s string, maxLength int) string { + if maxLength <= 0 { + return "" + } + if len(s) <= maxLength { + return s + } + return TrimSeparators(s[:maxLength]) +} + +// Sanitize renders s legal as a single label of a generated DNS-1123 name: it lowercases the value +// and replaces every character outside [a-z0-9-] with a dash. A name legal for its own API but not +// as a Kubernetes object name -- an RBAC name like system:aggregate-to-admin, whose colon a DNS-1123 +// name forbids -- becomes usable this way. +// +// The dot is mapped to a dash along with everything else, rather than kept as a label separator: a +// dot is only legal between two alphanumerics, so preserving it would let an invalid character next +// to it (or another dot) produce an empty or dash-bounded label that the API server still rejects -- +// the very failure Sanitize exists to prevent. Its one caller joins the sanitized parts with dashes +// anyway, so no dot is needed to keep them apart. +// +// It is lossy: distinct inputs can sanitize to the same string, so a caller must still append Hash +// of the full, unsanitized identity for uniqueness. +func Sanitize(s string) string { + return TrimSeparators(strings.TrimLeft(mapInvalid(strings.ToLower(s), isDNS1123Char), separators)) +} + +// sanitizeLabelValue is Sanitize's counterpart for a label value, whose legal character set is +// wider (it keeps case and underscores). It exists so LabelValue never returns a value the API +// server would reject; like Sanitize it is lossy and relies on a hash for identity. +func sanitizeLabelValue(value string) string { + return TrimSeparators(strings.TrimLeft(mapInvalid(value, isLabelValueChar), separators)) +} + +func mapInvalid(s string, valid func(rune) bool) string { + return strings.Map(func(r rune) rune { + if valid(r) { + return r + } + return '-' + }, s) +} + +func isDNS1123Char(r rune) bool { + return (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') || r == '-' +} + +func isLabelValueChar(r rune) bool { + return (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || r == '-' || r == '_' || r == '.' +} + +// LabelValue renders value so that it fits in a label value, sanitizing characters the API server +// would reject and shortening it to a prefix and a hash of the whole when it is too long. +// +// The result is lossy by design -- both sanitization and shortening can collapse distinct values -- +// and callers must treat it that way: selecting on the label with the original value matches nothing +// once it has been rewritten, so anything that needs to resolve an exact identity has to read it +// from a field that has neither a character nor a length limit. +func LabelValue(value string) string { + sanitized := sanitizeLabelValue(value) + if len(sanitized) <= validation.LabelValueMaxLength { + return sanitized + } + return fmt.Sprintf("%s-%s", TrimSeparators(sanitized[:labelValuePrefixMaxLength]), Hash(value)) +} diff --git a/pkg/utils/naming/naming_test.go b/pkg/utils/naming/naming_test.go new file mode 100644 index 000000000..5c0ffae66 --- /dev/null +++ b/pkg/utils/naming/naming_test.go @@ -0,0 +1,249 @@ +/* +Copyright 2026 The KubeFleet Authors. + +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 naming + +import ( + "strings" + "testing" + + "k8s.io/apimachinery/pkg/util/validation" +) + +func TestHash(t *testing.T) { + testCases := []struct { + name string + a, b string + want bool + }{ + {name: "same input hashes the same", a: "apps/Deployment/prod/app", b: "apps/Deployment/prod/app", want: true}, + {name: "namespace is part of the identity", a: "apps/Deployment/prod/app", b: "apps/Deployment/staging/app", want: false}, + {name: "api group is part of the identity", a: "apps/Widget/prod/app", b: "example.com/Widget/prod/app", want: false}, + {name: "separator placement matters", a: "apps/Deployment/prod/app", b: "apps/Deployment/prod-app/", want: false}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + gotA, gotB := Hash(tc.a), Hash(tc.b) + if (gotA == gotB) != tc.want { + t.Errorf("Hash(%q) = %v, Hash(%q) = %v, want equal to be %v", tc.a, gotA, tc.b, gotB, tc.want) + } + }) + } +} + +// TestHashShape pins the properties every generated name depends on: a fixed width, and a value +// made only of characters legal anywhere in a Kubernetes name. +func TestHashShape(t *testing.T) { + for _, input := range []string{"", "app", strings.Repeat("x", 4096), "ünïcödé/名前"} { + got := Hash(input) + if len(got) != HashLength { + t.Errorf("len(Hash(%q)) = %d, want %d", input, len(got), HashLength) + } + if strings.Trim(got, "0123456789abcdef") != "" { + t.Errorf("Hash(%q) = %v, want only lowercase hexadecimal characters", input, got) + } + } +} + +func TestTrimSeparators(t *testing.T) { + testCases := []struct { + name string + fragment string + want string + }{ + {name: "nothing to trim", fragment: "app", want: "app"}, + {name: "trailing dash", fragment: "app-", want: "app"}, + {name: "trailing dot", fragment: "app.", want: "app"}, + {name: "trailing underscore", fragment: "app_", want: "app"}, + {name: "run of separators", fragment: "app-._-", want: "app"}, + {name: "leading separators are left alone", fragment: "-app", want: "-app"}, + {name: "separators in the middle are left alone", fragment: "my-app.v2", want: "my-app.v2"}, + {name: "entirely separators", fragment: "---", want: ""}, + {name: "empty", fragment: "", want: ""}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if got := TrimSeparators(tc.fragment); got != tc.want { + t.Errorf("TrimSeparators(%q) = %v, want %v", tc.fragment, got, tc.want) + } + }) + } +} + +func TestTruncate(t *testing.T) { + testCases := []struct { + name string + s string + maxLength int + want string + }{ + {name: "shorter than the limit", s: "app", maxLength: 10, want: "app"}, + {name: "exactly at the limit", s: "app", maxLength: 3, want: "app"}, + {name: "over the limit", s: "application", maxLength: 4, want: "appl"}, + // The cut landing on a separator is the case that produced an invalid generated name in + // practice: the character after the cut would begin a new label. + {name: "cut lands on a dash", s: "my-application", maxLength: 3, want: "my"}, + {name: "cut lands on a dot", s: "my.application", maxLength: 3, want: "my"}, + {name: "cut lands inside a run of separators", s: "my-._application", maxLength: 5, want: "my"}, + {name: "cut lands just past a run of separators", s: "my-._application", maxLength: 6, want: "my-._a"}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got := Truncate(tc.s, tc.maxLength) + if got != tc.want { + t.Errorf("Truncate(%q, %d) = %v, want %v", tc.s, tc.maxLength, got, tc.want) + } + if len(got) > tc.maxLength { + t.Errorf("len(Truncate(%q, %d)) = %d, want at most %d", tc.s, tc.maxLength, len(got), tc.maxLength) + } + }) + } +} + +func TestLabelValue(t *testing.T) { + testCases := []struct { + name string + value string + want string + }{ + {name: "short value is kept as is", value: "app", want: "app"}, + { + name: "value at the limit is kept as is", + value: strings.Repeat("a", validation.LabelValueMaxLength), + want: strings.Repeat("a", validation.LabelValueMaxLength), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if got := LabelValue(tc.value); got != tc.want { + t.Errorf("LabelValue(%q) = %v, want %v", tc.value, got, tc.want) + } + }) + } +} + +// TestLabelValueShortening covers the values that do not fit, where the result is not a value the +// test can spell out but a set of properties it must hold: it has to be a legal label value, it +// has to stay within the limit, and it has to distinguish inputs that share a prefix. +func TestLabelValueShortening(t *testing.T) { + long := strings.Repeat("a", validation.LabelValueMaxLength+1) + testCases := []string{ + long, + long + "b", + // A value whose truncation point falls on a separator, which must not survive into the + // shortened form. + strings.Repeat("a", labelValuePrefixMaxLength-1) + "-" + strings.Repeat("b", 40), + strings.Repeat("a", labelValuePrefixMaxLength-1) + "." + strings.Repeat("c", 40), + } + + seen := make(map[string]string, len(testCases)) + for _, value := range testCases { + got := LabelValue(value) + if len(got) > validation.LabelValueMaxLength { + t.Errorf("len(LabelValue(%q)) = %d, want at most %d", value, len(got), validation.LabelValueMaxLength) + } + if errs := validation.IsValidLabelValue(got); len(errs) > 0 { + t.Errorf("LabelValue(%q) = %v, want a valid label value: %s", value, got, strings.Join(errs, "; ")) + } + if previous, collided := seen[got]; collided { + t.Errorf("LabelValue(%q) = %v, want a value distinct from that of %q", value, got, previous) + } + seen[got] = value + } +} + +// TestSanitize pins the transform that lets a name legal for its own API but not as a Kubernetes +// object name pass through into a generated name: every result must be legal as a DNS-1123 name +// segment, and the characters a name already allows must survive unchanged. +func TestSanitize(t *testing.T) { + testCases := []struct { + name string + value string + want string + }{ + {name: "already legal", value: "my-app", want: "my-app"}, + {name: "uppercase is lowercased", value: "MyApp", want: "myapp"}, + // The RBAC name that motivated the sanitizer: a colon is legal in the name but not in a + // Kubernetes object name. + {name: "rbac name with a colon", value: "system:aggregate-to-admin", want: "system-aggregate-to-admin"}, + {name: "underscores become dashes", value: "my_app", want: "my-app"}, + // A dot becomes a dash like any other separator: kept as a dot, an invalid character beside it + // would leave a label bounded by a dash, which the API server rejects. + {name: "dot becomes a dash", value: "my.app", want: "my-app"}, + {name: "dot next to an illegal character does not leave a bad label", value: "my.:app", want: "my--app"}, + {name: "adjacent dots do not leave an empty label", value: "a..b", want: "a--b"}, + {name: "leading separators are trimmed", value: ":::app", want: "app"}, + {name: "trailing separators are trimmed", value: "app:::", want: "app"}, + {name: "entirely illegal collapses to empty", value: ":::", want: ""}, + {name: "empty", value: "", want: ""}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got := Sanitize(tc.value) + if got != tc.want { + t.Errorf("Sanitize(%q) = %v, want %v", tc.value, got, tc.want) + } + if got != "" { + if errs := validation.IsDNS1123Subdomain(got); len(errs) > 0 { + t.Errorf("Sanitize(%q) = %v, want a valid DNS-1123 subdomain: %s", tc.value, got, strings.Join(errs, "; ")) + } + } + }) + } +} + +// TestLabelValueSanitizes covers values that carry characters a label value forbids. The wider +// label-value character set keeps case and underscores that Sanitize would drop, but a colon still +// has to go, and the result must be a value the API server accepts. +func TestLabelValueSanitizes(t *testing.T) { + testCases := []struct { + name string + value string + want string + }{ + {name: "case and underscores are kept", value: "My_App", want: "My_App"}, + {name: "colon becomes a dash", value: "system:aggregate-to-admin", want: "system-aggregate-to-admin"}, + {name: "leading separators are trimmed", value: ":app", want: "app"}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got := LabelValue(tc.value) + if got != tc.want { + t.Errorf("LabelValue(%q) = %v, want %v", tc.value, got, tc.want) + } + if errs := validation.IsValidLabelValue(got); len(errs) > 0 { + t.Errorf("LabelValue(%q) = %v, want a valid label value: %s", tc.value, got, strings.Join(errs, "; ")) + } + }) + } +} + +// TestTruncateNonPositiveBudget covers a caller whose own arithmetic left nothing for the value. +// The package exists to keep budget mistakes from reaching the API server; it must not turn one +// into a panic inside a reconcile loop either. +func TestTruncateNonPositiveBudget(t *testing.T) { + for _, maxLength := range []int{0, -1, -64} { + if got := Truncate("application", maxLength); got != "" { + t.Errorf("Truncate(%q, %d) = %v, want the empty string", "application", maxLength, got) + } + } +} diff --git a/pkg/webhook/membercluster/membercluster_validating_webhook.go b/pkg/webhook/membercluster/membercluster_validating_webhook.go index a61a951f7..f5ae9b6bf 100644 --- a/pkg/webhook/membercluster/membercluster_validating_webhook.go +++ b/pkg/webhook/membercluster/membercluster_validating_webhook.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "net/http" + "strings" admissionv1 "k8s.io/api/admission/v1" "k8s.io/apimachinery/pkg/types" @@ -14,6 +15,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" clusterv1beta1 "github.com/kubefleet-dev/kubefleet/apis/cluster/v1beta1" + kfplacementv1alpha1 "github.com/kubefleet-dev/kubefleet/apis/kubefleet.dev/placement/v1alpha1" "github.com/kubefleet-dev/kubefleet/pkg/utils" "github.com/kubefleet-dev/kubefleet/pkg/utils/validator" @@ -87,5 +89,56 @@ func (v *memberClusterValidator) Handle(ctx context.Context, req admission.Reque klog.V(2).ErrorS(err, "Member cluster has invalid fields, request is denied", "operation", req.Operation, "memberCluster", mcObjectName) return admission.Denied(err.Error()) } - return admission.Allowed("Member cluster has valid fields") + + response := admission.Allowed("Member cluster has valid fields") + if warning := v.clusterAliasCollisionWarning(ctx, &mc); warning != "" { + response = response.WithWarnings(warning) + } + return response +} + +// clusterAliasCollisionWarning returns a warning message if another member cluster already carries +// the alias this one is being labelled with, or the empty string otherwise. +// +// The alias selects a cluster by a name of the admin's choosing, so it is meant to identify one +// cluster; two clusters sharing an alias makes an alias-based selector match both. It is only a +// warning, never a denial: labelling a replacement cluster with the outgoing one's alias before +// removing it from the outgoing one is exactly the handoff the alias exists to allow, and that +// handoff passes through a state where two clusters share the alias. For the same reason a failure +// to list the member clusters does not block the request -- an advisory check must not stand +// between an admin and the cluster they are registering. +func (v *memberClusterValidator) clusterAliasCollisionWarning(ctx context.Context, mc *clusterv1beta1.MemberCluster) string { + alias, ok := mc.Labels[kfplacementv1alpha1.ClusterAliasLabel] + if !ok || alias == "" { + return "" + } + + memberClusterList := &clusterv1beta1.MemberClusterList{} + if err := v.client.List(ctx, memberClusterList); err != nil { + klog.V(2).ErrorS(err, "Failed to list member clusters for the alias uniqueness check; admitting without a warning", "memberCluster", klog.KObj(mc)) + return "" + } + + holders := make([]string, 0, len(memberClusterList.Items)) + for i := range memberClusterList.Items { + other := &memberClusterList.Items[i] + if other.Name == mc.Name { + continue + } + if other.Labels[kfplacementv1alpha1.ClusterAliasLabel] == alias { + holders = append(holders, other.Name) + } + } + if len(holders) == 0 { + return "" + } + // The message leads with the alias value and lists at most a few holders: an admission warning + // is truncated by the API server past 256 bytes, and the admin already knows which label they + // set, so the actionable half -- the value and who else holds it -- must fit inside that budget. + const maxListedHolders = 3 + listed := holders + if len(listed) > maxListedHolders { + listed = append(listed[:maxListedHolders:maxListedHolders], fmt.Sprintf("and %d more", len(holders)-maxListedHolders)) + } + return fmt.Sprintf("cluster alias %q is already used by %s; an alias-based cluster selector will match more than one cluster while this is the case", alias, strings.Join(listed, ", ")) } diff --git a/pkg/webhook/membercluster/membercluster_validating_webhook_test.go b/pkg/webhook/membercluster/membercluster_validating_webhook_test.go index 7cc231e41..5619607fb 100644 --- a/pkg/webhook/membercluster/membercluster_validating_webhook_test.go +++ b/pkg/webhook/membercluster/membercluster_validating_webhook_test.go @@ -13,12 +13,14 @@ import ( "k8s.io/apimachinery/pkg/types" clusterv1beta1 "github.com/kubefleet-dev/kubefleet/apis/cluster/v1beta1" + kfplacementv1alpha1 "github.com/kubefleet-dev/kubefleet/apis/kubefleet.dev/placement/v1alpha1" "github.com/kubefleet-dev/kubefleet/pkg/utils" fleetnetworkingv1alpha1 "go.goms.io/fleet-networking/api/v1alpha1" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) @@ -139,3 +141,127 @@ func newInternalServiceExport(clusterID, namespace string) *fleetnetworkingv1alp }, } } + +func buildCreateRequestFromObject(t *testing.T, mc *clusterv1beta1.MemberCluster) admission.Request { + t.Helper() + + raw, err := json.Marshal(mc) + if err != nil { + t.Fatalf("failed to marshal member cluster: %v", err) + } + return admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Name: mc.Name, + Object: runtime.RawExtension{Raw: raw}, + }, + } +} + +func memberClusterWithAlias(name, alias string) *clusterv1beta1.MemberCluster { + mc := &clusterv1beta1.MemberCluster{ObjectMeta: metav1.ObjectMeta{Name: name}} + if alias != "" { + mc.Labels = map[string]string{kfplacementv1alpha1.ClusterAliasLabel: alias} + } + return mc +} + +// TestHandleClusterAliasCollision covers the alias uniqueness warning: a member cluster whose alias +// is already held by another is admitted with a warning, never denied, and the cases that must stay +// silent (no alias, a unique alias, and the alias's own holder) produce none. +func TestHandleClusterAliasCollision(t *testing.T) { + t.Parallel() + + testCases := map[string]struct { + incoming *clusterv1beta1.MemberCluster + wantWarning bool + }{ + "no alias label is silent": { + incoming: memberClusterWithAlias("cluster-two", ""), + wantWarning: false, + }, + "a unique alias is silent": { + incoming: memberClusterWithAlias("cluster-two", "api-primary"), + wantWarning: false, + }, + "an alias already held by another cluster warns": { + incoming: memberClusterWithAlias("cluster-two", "web-primary"), + wantWarning: true, + }, + "the alias's own holder does not warn about itself": { + incoming: memberClusterWithAlias("cluster-one", "web-primary"), + wantWarning: false, + }, + } + + for name, tc := range testCases { + tc := tc + t.Run(name, func(t *testing.T) { + t.Parallel() + + // A fresh object per subtest: the fake client stamps a resourceVersion onto the objects + // it is built with, so a shared pointer would be written concurrently under -race. + existing := memberClusterWithAlias("cluster-one", "web-primary") + validator := newMemberClusterValidatorForTest(t, false, existing) + resp := validator.Handle(context.Background(), buildCreateRequestFromObject(t, tc.incoming)) + + if !resp.Allowed { + t.Fatalf("Handle() = denied, want allowed regardless of alias collision: %+v", resp.Result) + } + if gotWarning := len(resp.Warnings) > 0; gotWarning != tc.wantWarning { + t.Errorf("Handle() produced a warning = %v (%v), want %v", gotWarning, resp.Warnings, tc.wantWarning) + } + }) + } +} + +// TestClusterAliasCollisionWarningTruncatesHolders covers the many-holders path: the message caps +// the listed clusters so it stays inside the API server's warning-length budget. +func TestClusterAliasCollisionWarningTruncatesHolders(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + if err := clusterv1beta1.AddToScheme(scheme); err != nil { + t.Fatalf("failed to add member cluster scheme: %v", err) + } + seed := make([]client.Object, 0, 6) + for i := 0; i < 6; i++ { + seed = append(seed, memberClusterWithAlias(fmt.Sprintf("holder-%d", i), "web-primary")) + } + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(seed...).Build() + v := &memberClusterValidator{client: c, decoder: admission.NewDecoder(scheme)} + + got := v.clusterAliasCollisionWarning(context.Background(), memberClusterWithAlias("newcomer", "web-primary")) + if !strings.Contains(got, "and 3 more") { + t.Errorf("clusterAliasCollisionWarning() = %q, want it to summarize the surplus holders as \"and 3 more\"", got) + } + if len(got) > 256 { + t.Errorf("clusterAliasCollisionWarning() message is %d bytes, want it within the API server's 256-byte warning budget", len(got)) + } +} + +// TestClusterAliasCollisionWarningListError covers the fail-open path: a member cluster list that +// errors must admit the request without a warning rather than block it, since the check is advisory. +func TestClusterAliasCollisionWarningListError(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + if err := clusterv1beta1.AddToScheme(scheme); err != nil { + t.Fatalf("failed to add member cluster scheme: %v", err) + } + // A real collision exists in the store, so a successful list WOULD warn; only the injected list + // error can produce the empty result this asserts, which is what makes it a fail-open test + // rather than a trivially-no-collisions one. + failingClient := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(memberClusterWithAlias("cluster-one", "web-primary")). + WithInterceptorFuncs(interceptor.Funcs{ + List: func(context.Context, client.WithWatch, client.ObjectList, ...client.ListOption) error { + return fmt.Errorf("the member cluster list is unwell") + }, + }).Build() + v := &memberClusterValidator{client: failingClient, decoder: admission.NewDecoder(scheme)} + + if got := v.clusterAliasCollisionWarning(context.Background(), memberClusterWithAlias("cluster-two", "web-primary")); got != "" { + t.Errorf("clusterAliasCollisionWarning() = %q, want empty on a list error (fail open)", got) + } +} diff --git a/pkg/webhook/validation/uservalidation.go b/pkg/webhook/validation/uservalidation.go index 923c7137f..c63c1b896 100644 --- a/pkg/webhook/validation/uservalidation.go +++ b/pkg/webhook/validation/uservalidation.go @@ -19,6 +19,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" clusterv1beta1 "github.com/kubefleet-dev/kubefleet/apis/cluster/v1beta1" + kfplacementv1alpha1 "github.com/kubefleet-dev/kubefleet/apis/kubefleet.dev/placement/v1alpha1" placementv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1" "github.com/kubefleet-dev/kubefleet/pkg/utils" ) @@ -94,7 +95,13 @@ func ValidateFleetMemberClusterUpdate(currentMC, oldMC clusterv1beta1.MemberClus } isLabelUpdated := isMapFieldUpdated(currentMC.GetLabels(), oldMC.GetLabels()) - if isLabelUpdated && !isUserInGroup(userInfo, mastersGroup) && shouldDenyLabelModification(currentMC.GetLabels(), oldMC.GetLabels(), denyModifyMemberClusterLabels) { + // A whitelisted identity (the hub agent seeds the alias) or a cluster admin may modify the + // reserved kubefleet.dev/ labels; ordinary users may not. The distinction matters because, + // unlike the member-name label the controller reasserts every reconcile, the cluster alias is + // left as it is set -- so a non-admin edit to it would persist and redirect alias-based + // placements. (system:masters is already exempted before this runs, at the guard below.) + isFleetController := isAdminGroupUserOrWhiteListedUser(whiteListedUsers, userInfo) + if isLabelUpdated && !isUserInGroup(userInfo, mastersGroup) && shouldDenyLabelModification(currentMC.GetLabels(), oldMC.GetLabels(), denyModifyMemberClusterLabels, isFleetController) { // allow any user to modify kubernetes-fleet.io/* labels, but restricts other label modifications given denyModifyMemberClusterLabels is true. klog.V(2).InfoS(DeniedModifyMemberClusterLabels, "user", userInfo.Username, "groups", userInfo.Groups, "operation", req.Operation, "GVK", req.RequestKind, "subResource", req.SubResource, "namespacedName", namespacedName) return admission.Denied(DeniedModifyMemberClusterLabels) @@ -160,22 +167,30 @@ func isUserInGroup(userInfo authenticationv1.UserInfo, groupName string) bool { return slices.Contains(userInfo.Groups, groupName) } -// shouldDenyLabelModification returns true if any labels (besides kubernetes-fleet.io/* labels) are being modified and denyModifyMemberClusterLabels is true. -func shouldDenyLabelModification(currentLabels, oldLabels map[string]string, denyModifyMemberClusterLabels bool) bool { +// shouldDenyLabelModification returns true if any labels the requester is not allowed to touch are +// being modified and denyModifyMemberClusterLabels is true. +// +// The kubernetes-fleet.io/ prefix is exempt for everyone, as it always has been: those labels +// (e.g. the member name) are reasserted by the controller, so a stray edit self-heals. The +// kubefleet.dev/ prefix is exempt only for the fleet controllers, because the cluster alias it +// carries is not reasserted -- a non-admin edit would persist and redirect alias-based placements. +// The hub agent is not in system:masters, so without an exemption for its own identity, denying +// that prefix would wedge alias seeding. +func shouldDenyLabelModification(currentLabels, oldLabels map[string]string, denyModifyMemberClusterLabels, isFleetController bool) bool { if !denyModifyMemberClusterLabels { return false } for k, v := range currentLabels { oldV, exists := oldLabels[k] if !exists || oldV != v { - if !strings.HasPrefix(k, placementv1beta1.FleetPrefix) { + if !isReservedLabelKey(k, isFleetController) { return true } } } for k := range oldLabels { if _, exists := currentLabels[k]; !exists { - if !strings.HasPrefix(k, placementv1beta1.FleetPrefix) { + if !isReservedLabelKey(k, isFleetController) { return true } } @@ -183,6 +198,15 @@ func shouldDenyLabelModification(currentLabels, oldLabels map[string]string, den return false } +// isReservedLabelKey reports whether a label key belongs to a prefix the requester may modify: +// kubernetes-fleet.io/ for anyone, and kubefleet.dev/ only for a fleet controller. +func isReservedLabelKey(key string, isFleetController bool) bool { + if strings.HasPrefix(key, placementv1beta1.FleetPrefix) { + return true + } + return isFleetController && strings.HasPrefix(key, kfplacementv1alpha1.KubeFleetPrefix) +} + // isMemberClusterMapFieldUpdated return true if member cluster label is updated. func isMapFieldUpdated(currentMap, oldMap map[string]string) bool { return !reflect.DeepEqual(currentMap, oldMap) diff --git a/pkg/webhook/validation/uservalidation_test.go b/pkg/webhook/validation/uservalidation_test.go index 5562c9482..aa8de89e6 100644 --- a/pkg/webhook/validation/uservalidation_test.go +++ b/pkg/webhook/validation/uservalidation_test.go @@ -363,6 +363,83 @@ func TestValidateFleetMemberClusterUpdate(t *testing.T) { wantResponse: admission.Allowed(fmt.Sprintf(ResourceAllowedFormat, "nonSystemMastersUser", utils.GenerateGroupString([]string{"someGroup"}), admissionv1.Update, &utils.MCMetaGVK, "", types.NamespacedName{Name: "test-mc"})), }, + // The kubefleet.dev/ prefix is reserved for the fleet controllers: the whitelisted hub agent + // seeds the cluster alias label under it, is not in system:masters, and would otherwise be + // denied its own update and wedge the member cluster's reconciliation. + "allow the whitelisted fleet controller to set kubefleet.dev/* labels": { + denyModifyMemberClusterLabels: true, + whiteListedUsers: []string{"system:serviceaccount:fleet-system:hub-agent-sa"}, + oldMC: &clusterv1beta1.MemberCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-mc", + Labels: map[string]string{"kubernetes-fleet.io/member-name": "test-mc"}, + Annotations: map[string]string{ + "fleet.azure.com/cluster-resource-id": "test-cluster-resource-id", + }, + }, + }, + newMC: &clusterv1beta1.MemberCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-mc", + Labels: map[string]string{ + "kubernetes-fleet.io/member-name": "test-mc", + "kubefleet.dev/cluster-alias": "test-mc", + }, + Annotations: map[string]string{ + "fleet.azure.com/cluster-resource-id": "test-cluster-resource-id", + }, + }, + }, + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-mc", + UserInfo: authenticationv1.UserInfo{ + Username: "system:serviceaccount:fleet-system:hub-agent-sa", + Groups: []string{"system:serviceaccounts"}, + }, + RequestKind: &utils.MCMetaGVK, + Operation: admissionv1.Update, + }, + }, + wantResponse: admission.Allowed(fmt.Sprintf(ResourceAllowedFormat, "system:serviceaccount:fleet-system:hub-agent-sa", utils.GenerateGroupString([]string{"system:serviceaccounts"}), + admissionv1.Update, &utils.MCMetaGVK, "", types.NamespacedName{Name: "test-mc"})), + }, + // Unlike kubernetes-fleet.io/ labels, kubefleet.dev/ ones are not exempt for ordinary users: + // the cluster alias is not reasserted by the controller, so a non-admin's edit would persist + // and redirect alias-based placements. A user who is neither admin nor whitelisted is denied. + "deny a non-whitelisted user modifying a kubefleet.dev/ label": { + denyModifyMemberClusterLabels: true, + oldMC: &clusterv1beta1.MemberCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-mc", + Labels: map[string]string{"kubefleet.dev/cluster-alias": "web-primary"}, + Annotations: map[string]string{ + "fleet.azure.com/cluster-resource-id": "test-cluster-resource-id", + }, + }, + }, + newMC: &clusterv1beta1.MemberCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-mc", + Labels: map[string]string{"kubefleet.dev/cluster-alias": "hijacked"}, + Annotations: map[string]string{ + "fleet.azure.com/cluster-resource-id": "test-cluster-resource-id", + }, + }, + }, + req: admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Name: "test-mc", + UserInfo: authenticationv1.UserInfo{ + Username: "some-user", + Groups: []string{"system:authenticated"}, + }, + RequestKind: &utils.MCMetaGVK, + Operation: admissionv1.Update, + }, + }, + wantResponse: admission.Denied(DeniedModifyMemberClusterLabels), + }, "allow label creation by any user for kubernetes-fleet.io/* labels": { denyModifyMemberClusterLabels: true, oldMC: &clusterv1beta1.MemberCluster{ diff --git a/test/apis/placement/v1beta1/api_validation_integration_test.go b/test/apis/placement/v1beta1/api_validation_integration_test.go index 3357999e8..8f084fb7e 100644 --- a/test/apis/placement/v1beta1/api_validation_integration_test.go +++ b/test/apis/placement/v1beta1/api_validation_integration_test.go @@ -30,6 +30,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" placementv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1" @@ -564,6 +565,112 @@ var _ = Describe("Test placement v1beta1 API validation", func() { Expect(errors.As(err, &statusErr)).To(BeTrue(), "The returned error is not a StatusError") Expect(statusErr.Status().Message).Should(ContainSubstring("operator must be Exists when key is empty")) }) + + // The rolling update bounds are an int-or-string whose pattern constrains the string form + // only; the CEL rules are what keep the integer form non-negative, so these cases exercise + // the integer form specifically, with the string entries pinning that the pattern still + // owns its side. + DescribeTable("the rolling update bounds of a ClusterResourcePlacement", + func(mutate func(*placementv1beta1.RollingUpdateConfig), wantMessage string) { + crpName := fmt.Sprintf(crpNameTemplate, GinkgoParallelProcess()) + rollingUpdate := &placementv1beta1.RollingUpdateConfig{} + mutate(rollingUpdate) + crp := &placementv1beta1.ClusterResourcePlacement{ + ObjectMeta: metav1.ObjectMeta{ + Name: crpName, + }, + Spec: placementv1beta1.PlacementSpec{ + ResourceSelectors: []placementv1beta1.ResourceSelectorTerm{ + { + Group: "", + Version: "v1", + Kind: "Namespace", + Name: nonExistentNSName, + }, + }, + Strategy: placementv1beta1.RolloutStrategy{ + Type: placementv1beta1.RollingUpdateRolloutStrategyType, + RollingUpdate: rollingUpdate, + }, + }, + } + + err := hubClient.Create(ctx, crp) + if wantMessage == "" { + Expect(err).To(Succeed(), "Expected the CRP to be accepted") + return + } + Expect(err).To(HaveOccurred(), "Expected error when creating CRP with an out-of-range rolling update bound") + var statusErr *k8sErrors.StatusError + Expect(errors.As(err, &statusErr)).To(BeTrue(), "The returned error is not a StatusError") + Expect(statusErr.Status().Message).Should(ContainSubstring(wantMessage)) + }, + Entry("maxUnavailable 0 as an integer is accepted", func(c *placementv1beta1.RollingUpdateConfig) { + c.MaxUnavailable = ptr.To(intstr.FromInt32(0)) + }, ""), + Entry("maxUnavailable 5 as an integer is accepted", func(c *placementv1beta1.RollingUpdateConfig) { + c.MaxUnavailable = ptr.To(intstr.FromInt32(5)) + }, ""), + Entry("maxSurge 0 as an integer is accepted", func(c *placementv1beta1.RollingUpdateConfig) { + c.MaxSurge = ptr.To(intstr.FromInt32(0)) + }, ""), + Entry("maxUnavailable 25% is accepted", func(c *placementv1beta1.RollingUpdateConfig) { + c.MaxUnavailable = ptr.To(intstr.FromString("25%")) + }, ""), + Entry("maxUnavailable 100% is accepted", func(c *placementv1beta1.RollingUpdateConfig) { + c.MaxUnavailable = ptr.To(intstr.FromString("100%")) + }, ""), + Entry("maxUnavailable -1 as an integer is rejected", func(c *placementv1beta1.RollingUpdateConfig) { + c.MaxUnavailable = ptr.To(intstr.FromInt32(-1)) + }, "maxUnavailable must be a non-negative integer or a percentage"), + Entry("maxSurge -1 as an integer is rejected", func(c *placementv1beta1.RollingUpdateConfig) { + c.MaxSurge = ptr.To(intstr.FromInt32(-1)) + }, "maxSurge must be a non-negative integer or a percentage"), + Entry("maxUnavailable -1 as a string is rejected by the pattern", func(c *placementv1beta1.RollingUpdateConfig) { + c.MaxUnavailable = ptr.To(intstr.FromString("-1")) + }, "spec.strategy.rollingUpdate.maxUnavailable in body should match"), + Entry("maxUnavailable 101% is rejected by the pattern", func(c *placementv1beta1.RollingUpdateConfig) { + c.MaxUnavailable = ptr.To(intstr.FromString("101%")) + }, "spec.strategy.rollingUpdate.maxUnavailable in body should match"), + // The digit branch of the pattern is bounded to nine digits, all of which fit an int32 + // comfortably; an unbounded digit string used to pass validation only to fail in + // whatever later consumed it. + Entry("maxUnavailable with nine digits is accepted", func(c *placementv1beta1.RollingUpdateConfig) { + c.MaxUnavailable = ptr.To(intstr.FromString("999999999")) + }, ""), + Entry("maxUnavailable with ten digits is rejected by the pattern", func(c *placementv1beta1.RollingUpdateConfig) { + c.MaxUnavailable = ptr.To(intstr.FromString("9999999999")) + }, "spec.strategy.rollingUpdate.maxUnavailable in body should match"), + ) + + It("does not re-litigate the rolling update bounds on an unrelated update", func() { + crpName := fmt.Sprintf(crpNameTemplate, GinkgoParallelProcess()) + crp := &placementv1beta1.ClusterResourcePlacement{ + ObjectMeta: metav1.ObjectMeta{ + Name: crpName, + }, + Spec: placementv1beta1.PlacementSpec{ + ResourceSelectors: []placementv1beta1.ResourceSelectorTerm{ + { + Group: "", + Version: "v1", + Kind: "Namespace", + Name: nonExistentNSName, + }, + }, + Strategy: placementv1beta1.RolloutStrategy{ + Type: placementv1beta1.RollingUpdateRolloutStrategyType, + RollingUpdate: &placementv1beta1.RollingUpdateConfig{ + MaxUnavailable: ptr.To(intstr.FromString("25%")), + }, + }, + }, + } + Expect(hubClient.Create(ctx, crp)).To(Succeed()) + + crp.Spec.RevisionHistoryLimit = ptr.To(int32(5)) + Expect(hubClient.Update(ctx, crp)).To(Succeed(), "Expected an update leaving the bounds untouched to pass their validation") + }) }) Context("Test ClusterResourcePlacement API validation - invalid update cases", func() { @@ -1826,6 +1933,79 @@ var _ = Describe("Test placement v1beta1 API validation", func() { }) }) + Context("Test ResourcePlacement rolling update bounds", func() { + rpNamespace := "default" + + AfterEach(func() { + rpName := fmt.Sprintf(rpNameTemplate, GinkgoParallelProcess()) + Eventually(func() error { + rp := &placementv1beta1.ResourcePlacement{ + ObjectMeta: metav1.ObjectMeta{ + Name: rpName, + Namespace: rpNamespace, + }, + } + if err := hubClient.Delete(ctx, rp); err != nil && !k8sErrors.IsNotFound(err) { + return fmt.Errorf("failed to delete RP: %w", err) + } + if err := hubClient.Get(ctx, client.ObjectKey{Name: rpName, Namespace: rpNamespace}, &placementv1beta1.ResourcePlacement{}); !k8sErrors.IsNotFound(err) { + return fmt.Errorf("RP still exists after deletion attempt (error: %w)", err) + } + return nil + }, eventuallyDuration, eventuallyInterval).Should(Succeed()) + }) + + // ResourcePlacement shares RollingUpdateConfig with ClusterResourcePlacement, but unlike + // the cluster-scoped placement it has no validating webhook behind it, so the CRD schema + // is the only line of defense here. + DescribeTable("the rolling update bounds of a ResourcePlacement", + func(mutate func(*placementv1beta1.RollingUpdateConfig), wantMessage string) { + rpName := fmt.Sprintf(rpNameTemplate, GinkgoParallelProcess()) + rollingUpdate := &placementv1beta1.RollingUpdateConfig{} + mutate(rollingUpdate) + rp := &placementv1beta1.ResourcePlacement{ + ObjectMeta: metav1.ObjectMeta{ + Name: rpName, + Namespace: rpNamespace, + }, + Spec: placementv1beta1.PlacementSpec{ + ResourceSelectors: []placementv1beta1.ResourceSelectorTerm{ + { + Group: "", + Version: "v1", + Kind: "ConfigMap", + Name: "app", + }, + }, + Strategy: placementv1beta1.RolloutStrategy{ + Type: placementv1beta1.RollingUpdateRolloutStrategyType, + RollingUpdate: rollingUpdate, + }, + }, + } + + err := hubClient.Create(ctx, rp) + if wantMessage == "" { + Expect(err).To(Succeed(), "Expected the RP to be accepted") + return + } + Expect(err).To(HaveOccurred(), "Expected error when creating RP with an out-of-range rolling update bound") + var statusErr *k8sErrors.StatusError + Expect(errors.As(err, &statusErr)).To(BeTrue(), "The returned error is not a StatusError") + Expect(statusErr.Status().Message).Should(ContainSubstring(wantMessage)) + }, + Entry("maxUnavailable 0 as an integer is accepted", func(c *placementv1beta1.RollingUpdateConfig) { + c.MaxUnavailable = ptr.To(intstr.FromInt32(0)) + }, ""), + Entry("maxUnavailable -1 as an integer is rejected", func(c *placementv1beta1.RollingUpdateConfig) { + c.MaxUnavailable = ptr.To(intstr.FromInt32(-1)) + }, "maxUnavailable must be a non-negative integer or a percentage"), + Entry("maxSurge -1 as an integer is rejected", func(c *placementv1beta1.RollingUpdateConfig) { + c.MaxSurge = ptr.To(intstr.FromInt32(-1)) + }, "maxSurge must be a non-negative integer or a percentage"), + ) + }) + Context("Test ResourcePlacement API validation - invalid update cases", func() { var rp placementv1beta1.ResourcePlacement rpName := fmt.Sprintf(rpNameTemplate, GinkgoParallelProcess()) diff --git a/test/e2e/annotation_placement_test.go b/test/e2e/annotation_placement_test.go new file mode 100644 index 000000000..2e1b4e791 --- /dev/null +++ b/test/e2e/annotation_placement_test.go @@ -0,0 +1,367 @@ +/* +Copyright 2026 The KubeFleet Authors. + +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 e2e + +import ( + "fmt" + + "github.com/google/go-cmp/cmp" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + + clusterv1beta1 "github.com/kubefleet-dev/kubefleet/apis/cluster/v1beta1" + kfplacementv1alpha1 "github.com/kubefleet-dev/kubefleet/apis/kubefleet.dev/placement/v1alpha1" +) + +// The event reason the hub agent records on a resource whose annotation cannot be parsed. It is +// spelled out here rather than imported so that a rename in the controller shows up as a failed +// spec, the same way it would show up for a user filtering events by reason. +const invalidClusterSelectorsAnnotationEventReason = "InvalidClusterSelectorsAnnotation" + +// generatedPolicyLabels returns the provenance labels the hub agent stamps on a policy it generates +// for the given resource; listing on them is how a user, and these specs, find the policy without +// knowing the generated name. +func generatedPolicyLabels(apiGroup, kind, name string) client.MatchingLabels { + return client.MatchingLabels{ + kfplacementv1alpha1.ParentAPIGroupLabel: apiGroup, + kfplacementv1alpha1.ParentKindLabel: kind, + kfplacementv1alpha1.ParentNameLabel: name, + } +} + +// setClusterSelectorsAnnotation sets, changes, or (with an empty value) removes the cluster +// selectors annotation on a hub resource, retrying on a conflict with the hub agent's own writes. +// Removing the annotation from a resource that is already gone counts as done, so that cleanup +// after a failed spec does not wait out the timeout on it. +func setClusterSelectorsAnnotation(object client.Object, value string) { + Eventually(func() error { + if err := hubClient.Get(ctx, client.ObjectKeyFromObject(object), object); err != nil { + if value == "" && apierrors.IsNotFound(err) { + return nil + } + return err + } + annotations := object.GetAnnotations() + if annotations == nil { + annotations = map[string]string{} + } + if value == "" { + delete(annotations, kfplacementv1alpha1.ClusterSelectorsAnnotation) + } else { + annotations[kfplacementv1alpha1.ClusterSelectorsAnnotation] = value + } + object.SetAnnotations(annotations) + return hubClient.Update(ctx, object) + }, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to set the cluster selectors annotation on %s", object.GetName()) +} + +// annotationClusterSelector builds the cluster selector the hub agent generates from one segment of +// the annotation, with the fields the API defaults filled in as the API server stores them. +func annotationClusterSelector(matchLabels map[string]string, count intstr.IntOrString) kfplacementv1alpha1.ClusterSelector { + selector := kfplacementv1alpha1.ClusterSelector{ + Count: ptr.To(count), + WhenUnfulfilled: kfplacementv1alpha1.WhenUnfulfilledOptionAddClusterClaim, + } + if len(matchLabels) > 0 { + selector.Terms = []kfplacementv1alpha1.ClusterLabelAndPropertySelectorTerm{{MatchLabels: matchLabels}} + } + return selector +} + +// The FEP-0001 annotation-based placement experience is alpha and runs behind the +// enable-annotation-based-placement hub agent flag, which test/e2e/setup.sh turns on. These specs +// drive the feature the way a user does, through the annotation alone, and check the whole path +// that the controller's own envtest suite cannot: the resource watcher noticing the annotation, the +// generated policy watch noticing edits to the policy, and the event recorded on the resource. +var _ = Describe("annotation based placement", Ordered, func() { + var ( + configMap corev1.ConfigMap + deployment appsv1.Deployment + namespace corev1.Namespace + configMapLabels client.MatchingLabels + ) + + BeforeAll(func() { + By("creating the work resources") + createWorkResources() + configMap = appConfigMap() + namespace = appNamespace() + configMapLabels = generatedPolicyLabels("", "ConfigMap", configMap.Name) + }) + + AfterAll(func() { + // A wait below that fails aborts the rest of this node; the work resources must go + // regardless, or they leak into every spec that follows in this process. + defer cleanupWorkResources() + + // The annotations come off first so that the hub agent withdraws the generated policies + // itself, rather than leaving them to the garbage collector once the namespace goes; a + // policy that outlived its annotation would be the feature failing, not cleanup succeeding. + // Each object is un-annotated only if the node that created it got as far as naming it; + // otherwise the call would wait out its timeout on an empty name. + if configMap.Name != "" { + setClusterSelectorsAnnotation(&configMap, "") + } + if deployment.Name != "" { + setClusterSelectorsAnnotation(&deployment, "") + } + if namespace.Name != "" { + setClusterSelectorsAnnotation(&namespace, "") + } + Eventually(func() error { + policies := &kfplacementv1alpha1.ClusterPlacementPolicyList{} + if err := hubClient.List(ctx, policies, generatedPolicyLabels("", "Namespace", namespace.Name)); err != nil { + return err + } + if len(policies.Items) != 0 { + return fmt.Errorf("%d cluster placement policies generated for namespace %s are still present", len(policies.Items), namespace.Name) + } + return nil + }, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to clean up the generated cluster placement policy") + }) + + // generatedPolicy reads back the one policy generated for the ConfigMap, failing the caller's + // Eventually until exactly one exists. + generatedPolicy := func() (*kfplacementv1alpha1.PlacementPolicy, error) { + policies := &kfplacementv1alpha1.PlacementPolicyList{} + if err := hubClient.List(ctx, policies, client.InNamespace(configMap.Namespace), configMapLabels); err != nil { + return nil, err + } + if len(policies.Items) != 1 { + return nil, fmt.Errorf("%d placement policies generated for the config map, want 1", len(policies.Items)) + } + return &policies.Items[0], nil + } + + noGeneratedPolicyActual := func() error { + policies := &kfplacementv1alpha1.PlacementPolicyList{} + if err := hubClient.List(ctx, policies, client.InNamespace(configMap.Namespace), configMapLabels); err != nil { + return err + } + if len(policies.Items) != 0 { + return fmt.Errorf("%d placement policies generated for the config map, want none", len(policies.Items)) + } + return nil + } + + generatedPolicySpecActual := func(wantSelectors ...kfplacementv1alpha1.ClusterSelector) func() error { + wantSpec := kfplacementv1alpha1.PlacementPolicySpec{ + ClusterSelectors: wantSelectors, + ResourceSelectors: []kfplacementv1alpha1.ResourceSelector{{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: configMap.Name, + }}, + ResourceRevisionHistoryLimit: ptr.To(int32(3)), + } + return func() error { + policy, err := generatedPolicy() + if err != nil { + return err + } + if diff := cmp.Diff(policy.Spec, wantSpec); diff != "" { + return fmt.Errorf("generated placement policy spec diff (-got, +want):\n%s", diff) + } + return nil + } + } + + It("should seed the cluster alias label on every member cluster", func() { + // The alias= shorthand of the annotation matches on this label; the hub agent seeds it + // from the cluster name when the feature is on, so that the shorthand works out of the box. + // + // The label is written by the member cluster reconciler, not at join time, so it is + // awaited rather than read once. + for _, name := range allMemberClusterNames { + Eventually(func() error { + mc := &clusterv1beta1.MemberCluster{} + if err := hubClient.Get(ctx, types.NamespacedName{Name: name}, mc); err != nil { + return err + } + if got := mc.Labels[kfplacementv1alpha1.ClusterAliasLabel]; got != name { + return fmt.Errorf("member cluster %s has alias label %q, want %q", name, got, name) + } + return nil + }, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Member cluster %s does not carry its alias label", name) + } + }) + + It("should generate no policy for a resource that carries no annotation", func() { + Consistently(noGeneratedPolicyActual, consistentlyDuration, consistentlyInterval).Should(Succeed(), "A policy was generated for a resource without the annotation") + }) + + It("should generate a placement policy when the annotation is set", func() { + setClusterSelectorsAnnotation(&configMap, fmt.Sprintf("%s=%s,count=All", envLabelName, envCanary)) + + wantSelector := annotationClusterSelector(map[string]string{envLabelName: envCanary}, intstr.FromString("All")) + Eventually(generatedPolicySpecActual(wantSelector), eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to generate the placement policy") + + policy, err := generatedPolicy() + Expect(err).Should(Succeed()) + Expect(policy.Namespace).Should(Equal(configMap.Namespace), "A namespaced resource must generate a policy in its own namespace") + + Expect(hubClient.Get(ctx, client.ObjectKeyFromObject(&configMap), &configMap)).Should(Succeed()) + wantOwnerReferences := []metav1.OwnerReference{{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: configMap.Name, + UID: configMap.UID, + }} + diff := cmp.Diff(policy.OwnerReferences, wantOwnerReferences) + Expect(diff).To(BeEmpty(), "generated placement policy owner references diff (-got, +want):\n%s", diff) + }) + + It("should update the policy when the annotation changes", func() { + // The region shorthand expands to the well-known topology label rather than being matched + // literally. + setClusterSelectorsAnnotation(&configMap, fmt.Sprintf("region=%s,count=2", regionEast)) + + wantSelector := annotationClusterSelector(map[string]string{corev1.LabelTopologyRegion: regionEast}, intstr.FromInt32(2)) + Eventually(generatedPolicySpecActual(wantSelector), eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update the placement policy") + }) + + It("should restore the policy when it is edited", func() { + policy, err := generatedPolicy() + Expect(err).Should(Succeed()) + policy.Spec.ClusterSelectors = nil + Expect(hubClient.Update(ctx, policy)).Should(Succeed(), "Failed to edit the generated placement policy") + + // Nothing happened to the ConfigMap; only the watch on generated policies can bring the + // hub agent back to this resource. + wantSelector := annotationClusterSelector(map[string]string{corev1.LabelTopologyRegion: regionEast}, intstr.FromInt32(2)) + Eventually(generatedPolicySpecActual(wantSelector), eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to restore the edited placement policy") + }) + + It("should recreate the policy when it is deleted", func() { + policy, err := generatedPolicy() + Expect(err).Should(Succeed()) + deletedUID := policy.UID + Expect(hubClient.Delete(ctx, policy)).Should(Succeed(), "Failed to delete the generated placement policy") + + Eventually(func() error { + policy, err := generatedPolicy() + if err != nil { + return err + } + if policy.UID == deletedUID { + return fmt.Errorf("the deleted placement policy is still present") + } + return nil + }, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to recreate the deleted placement policy") + wantSelector := annotationClusterSelector(map[string]string{corev1.LabelTopologyRegion: regionEast}, intstr.FromInt32(2)) + Eventually(generatedPolicySpecActual(wantSelector), eventuallyDuration, eventuallyInterval).Should(Succeed(), "The recreated placement policy does not match the annotation") + }) + + It("should keep the policy and record an event when the annotation becomes invalid", func() { + setClusterSelectorsAnnotation(&configMap, envLabelName) + + Eventually(func() error { + events := &corev1.EventList{} + if err := hubClient.List(ctx, events, client.InNamespace(configMap.Namespace), client.MatchingFields{ + "involvedObject.name": configMap.Name, + "reason": invalidClusterSelectorsAnnotationEventReason, + }); err != nil { + return err + } + if len(events.Items) == 0 { + return fmt.Errorf("no %s event recorded on the config map", invalidClusterSelectorsAnnotationEventReason) + } + return nil + }, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to record the invalid annotation event") + + wantSelector := annotationClusterSelector(map[string]string{corev1.LabelTopologyRegion: regionEast}, intstr.FromInt32(2)) + Consistently(generatedPolicySpecActual(wantSelector), consistentlyDuration, consistentlyInterval).Should(Succeed(), "The policy from the last valid annotation was not left in place") + }) + + It("should delete the policy when the annotation is removed", func() { + setClusterSelectorsAnnotation(&configMap, "") + Eventually(noGeneratedPolicyActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to delete the placement policy") + }) + + It("should carry the API group of a non-core resource into the generated policy", func() { + // The ConfigMap above sits in the core group, whose empty name would also match a policy + // that forgot the group altogether; a Deployment pins the field to a real value. + deployment = appDeployment() + Expect(hubClient.Create(ctx, &deployment)).Should(Succeed(), "Failed to create the deployment") + setClusterSelectorsAnnotation(&deployment, fmt.Sprintf("%s=%s", envLabelName, envProd)) + + deploymentLabels := generatedPolicyLabels("apps", "Deployment", deployment.Name) + wantSpec := kfplacementv1alpha1.PlacementPolicySpec{ + ClusterSelectors: []kfplacementv1alpha1.ClusterSelector{ + annotationClusterSelector(map[string]string{envLabelName: envProd}, intstr.FromInt32(1)), + }, + ResourceSelectors: []kfplacementv1alpha1.ResourceSelector{{ + APIGroup: "apps", + APIVersion: "v1", + Kind: "Deployment", + Name: deployment.Name, + }}, + ResourceRevisionHistoryLimit: ptr.To(int32(3)), + } + Eventually(func() error { + policies := &kfplacementv1alpha1.PlacementPolicyList{} + if err := hubClient.List(ctx, policies, client.InNamespace(deployment.Namespace), deploymentLabels); err != nil { + return err + } + if len(policies.Items) != 1 { + return fmt.Errorf("%d placement policies generated for the deployment, want 1", len(policies.Items)) + } + if diff := cmp.Diff(policies.Items[0].Spec, wantSpec); diff != "" { + return fmt.Errorf("generated placement policy spec diff (-got, +want):\n%s", diff) + } + return nil + }, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to generate the placement policy for the deployment") + }) + + It("should generate a cluster placement policy for a cluster scoped resource", func() { + // The alias shorthand expands to the cluster alias label the hub agent seeds above. + setClusterSelectorsAnnotation(&namespace, fmt.Sprintf("alias=%s", memberCluster1EastProdName)) + + wantSpec := kfplacementv1alpha1.PlacementPolicySpec{ + ClusterSelectors: []kfplacementv1alpha1.ClusterSelector{ + annotationClusterSelector(map[string]string{kfplacementv1alpha1.ClusterAliasLabel: memberCluster1EastProdName}, intstr.FromInt32(1)), + }, + ResourceSelectors: []kfplacementv1alpha1.ResourceSelector{{ + APIVersion: "v1", + Kind: "Namespace", + Name: namespace.Name, + }}, + ResourceRevisionHistoryLimit: ptr.To(int32(3)), + } + Eventually(func() error { + policies := &kfplacementv1alpha1.ClusterPlacementPolicyList{} + if err := hubClient.List(ctx, policies, generatedPolicyLabels("", "Namespace", namespace.Name)); err != nil { + return err + } + if len(policies.Items) != 1 { + return fmt.Errorf("%d cluster placement policies generated for the namespace, want 1", len(policies.Items)) + } + if diff := cmp.Diff(policies.Items[0].Spec, wantSpec); diff != "" { + return fmt.Errorf("generated cluster placement policy spec diff (-got, +want):\n%s", diff) + } + return nil + }, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to generate the cluster placement policy") + }) +}) diff --git a/test/e2e/setup.sh b/test/e2e/setup.sh index 0f297084a..c931ccf90 100755 --- a/test/e2e/setup.sh +++ b/test/e2e/setup.sh @@ -131,6 +131,19 @@ helm install cert-manager jetstack/cert-manager \ --wait \ --timeout=300s +# Install the placement policy CRDs that annotation based placement generates policies for. +# +# The hub agent chart does not ship the placement.kubefleet.dev CRDs yet, and the hub agent +# refuses to start with the feature on when they are absent, so they are applied straight from +# the source tree ahead of the chart. +kubectl apply -f ../../config/crd/bases/placement.kubefleet.dev_placementpolicies.yaml +kubectl apply -f ../../config/crd/bases/placement.kubefleet.dev_clusterplacementpolicies.yaml +# The hub agent checks for the CRDs at startup with a retry budget of about a second, so they +# must be established before the chart is installed rather than merely applied. +kubectl wait --for=condition=Established --timeout=60s \ + -f ../../config/crd/bases/placement.kubefleet.dev_placementpolicies.yaml \ + -f ../../config/crd/bases/placement.kubefleet.dev_clusterplacementpolicies.yaml + # Install the hub agent to the hub cluster helm install hub-agent ../../charts/hub-agent/ \ --namespace fleet-system \ @@ -153,6 +166,7 @@ helm install hub-agent ../../charts/hub-agent/ \ --set-file additionalConfigData.admissionPolicyManagerCfg=admission_policy_manager_cfg.yaml \ --set admissionPolicyManagerConfigName=admissionPolicyManagerCfg \ --set enableAdmissionPolicyManager=true \ + --set enableAnnotationBasedPlacement=true \ --set resourceSnapshotCreationMinimumInterval=$RESOURCE_SNAPSHOT_CREATION_MINIMUM_INTERVAL \ --set resourceChangesCollectionDuration=$RESOURCE_CHANGES_COLLECTION_DURATION \ --wait \ diff --git a/test/e2e/setup_test.go b/test/e2e/setup_test.go index 5786862e5..7c9c63144 100644 --- a/test/e2e/setup_test.go +++ b/test/e2e/setup_test.go @@ -47,6 +47,7 @@ import ( fleetnetworkingv1alpha1 "go.goms.io/fleet-networking/api/v1alpha1" clusterv1beta1 "github.com/kubefleet-dev/kubefleet/apis/cluster/v1beta1" + kfplacementv1alpha1 "github.com/kubefleet-dev/kubefleet/apis/kubefleet.dev/placement/v1alpha1" placementv1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1" placementv1alpha1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1alpha1" placementv1beta1 "github.com/kubefleet-dev/kubefleet/apis/placement/v1beta1" @@ -292,6 +293,9 @@ func TestMain(m *testing.M) { if err := placementv1alpha1.AddToScheme(scheme); err != nil { log.Fatalf("failed to add custom APIs (placement v1alpha1) to the runtime scheme: %v", err) } + if err := kfplacementv1alpha1.AddToScheme(scheme); err != nil { + log.Fatalf("failed to add custom APIs (kubefleet.dev placement v1alpha1) to the runtime scheme: %v", err) + } if err := placementv1beta1.AddToScheme(scheme); err != nil { log.Fatalf("failed to add custom APIs (placement) to the runtime scheme: %v", err) } diff --git a/test/utils/informer/manager.go b/test/utils/informer/manager.go index 004ef9364..5b05495ed 100644 --- a/test/utils/informer/manager.go +++ b/test/utils/informer/manager.go @@ -33,6 +33,11 @@ import ( type FakeLister struct { Objects []runtime.Object Err error + // ClusterScoped mirrors a cluster-scoped resource's cache, whose objects are indexed under a bare + // name and belong to no namespace. When set, a namespaced lookup through ByNamespace finds + // nothing, exactly as the real per-namespace indexer does -- so a caller that mistakes a + // cluster-scoped resource for a namespaced one and reads it through ByNamespace misses it. + ClusterScoped bool } func (f *FakeLister) List(selector labels.Selector) ([]runtime.Object, error) { @@ -68,7 +73,7 @@ func (f *FakeLister) Get(name string) (runtime.Object, error) { } func (f *FakeLister) ByNamespace(namespace string) cache.GenericNamespaceLister { - return &FakeNamespaceLister{Objects: f.Objects, Namespace: namespace, Err: f.Err} + return &FakeNamespaceLister{Objects: f.Objects, Namespace: namespace, Err: f.Err, clusterScoped: f.ClusterScoped} } // FakeNamespaceLister implements cache.GenericNamespaceLister. @@ -76,12 +81,18 @@ type FakeNamespaceLister struct { Objects []runtime.Object Namespace string Err error + // clusterScoped is propagated from the parent FakeLister; when set, the objects live under no + // namespace, so every namespaced lookup here finds nothing. + clusterScoped bool } func (f *FakeNamespaceLister) List(selector labels.Selector) ([]runtime.Object, error) { if f.Err != nil { return nil, f.Err } + if f.clusterScoped { + return nil, nil + } var filtered []runtime.Object for _, obj := range f.Objects { @@ -103,6 +114,9 @@ func (f *FakeNamespaceLister) Get(name string) (runtime.Object, error) { if f.Err != nil { return nil, f.Err } + if f.clusterScoped { + return nil, apierrors.NewNotFound(schema.GroupResource{Resource: "test"}, name) + } for _, obj := range f.Objects { if uObj := obj.(*unstructured.Unstructured); uObj.GetName() == name && uObj.GetNamespace() == f.Namespace { return obj, nil @@ -128,12 +142,15 @@ type FakeManager struct { // InformerSynced controls whether IsInformerSynced returns true or false. // If nil, defaults to true. If set, returns the value for all resources. InformerSynced *bool + // StaticResources records every resource registered through AddStaticResource. + StaticResources []informer.APIResourceMeta } func (m *FakeManager) AddDynamicResources(_ []informer.APIResourceMeta, _ cache.ResourceEventHandler, _ bool) { } -func (m *FakeManager) AddStaticResource(_ informer.APIResourceMeta, _ cache.ResourceEventHandler) { +func (m *FakeManager) AddStaticResource(resource informer.APIResourceMeta, _ cache.ResourceEventHandler) { + m.StaticResources = append(m.StaticResources, resource) } func (m *FakeManager) IsInformerSynced(_ schema.GroupVersionResource) bool {