feat: [FEP-0001] support annotation-based placement - #829
Draft
Yetkin Timocin (ytimocin) wants to merge 22 commits into
Draft
feat: [FEP-0001] support annotation-based placement#829Yetkin Timocin (ytimocin) wants to merge 22 commits into
Yetkin Timocin (ytimocin) wants to merge 22 commits into
Conversation
Yetkin Timocin (ytimocin)
force-pushed
the
feat/fep0001-annotation-based-placement
branch
from
August 18, 2026 23:20
1d41c64 to
30cd7e6
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Implements FEP-0001 annotation-based placement, including policy reconciliation, drift repair, feature wiring, alias support, and related API validation fixes.
Changes:
- Adds annotation parsing and generated placement-policy reconciliation.
- Wires policy watches, cleanup, feature flags, RBAC, and cluster aliases.
- Tightens count and rolling-update validation.
Reviewed changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
test/utils/informer/manager.go |
Records static informer registrations. |
test/apis/placement/v1beta1/api_validation_integration_test.go |
Tests rolling-update validation. |
pkg/webhook/validation/uservalidation.go |
Exempts reserved KubeFleet labels. |
pkg/webhook/validation/uservalidation_test.go |
Tests label exemption. |
pkg/utils/naming/naming.go |
Adds naming and hashing helpers. |
pkg/utils/naming/naming_test.go |
Tests naming helpers. |
pkg/utils/apiresources.go |
Excludes placement bookkeeping resources. |
pkg/resourcewatcher/event_handlers.go |
Enqueues annotated resources. |
pkg/resourcewatcher/event_handlers_test.go |
Tests annotation event routing. |
pkg/resourcewatcher/change_detector.go |
Adds policy watches and tombstone handling. |
pkg/resourcewatcher/change_detector_watch_test.go |
Tests policy watch registration. |
pkg/resourcewatcher/change_detector_test.go |
Tests tombstone filtering. |
pkg/controllers/membercluster/v1beta1/membercluster_controller.go |
Seeds cluster aliases. |
pkg/controllers/membercluster/v1beta1/membercluster_controller_test.go |
Tests alias behavior. |
pkg/controllers/annotationplacement/watch.go |
Maps policy events to source resources. |
pkg/controllers/annotationplacement/watch_test.go |
Tests policy event mapping. |
pkg/controllers/annotationplacement/suite_test.go |
Configures integration tests. |
pkg/controllers/annotationplacement/policy.go |
Derives generated policies. |
pkg/controllers/annotationplacement/policy_test.go |
Tests policy derivation. |
pkg/controllers/annotationplacement/parser.go |
Parses selector annotations. |
pkg/controllers/annotationplacement/parser_test.go |
Tests annotation grammar. |
pkg/controllers/annotationplacement/controller.go |
Reconciles generated policies. |
pkg/controllers/annotationplacement/controller_test.go |
Tests reconciliation behavior. |
pkg/controllers/annotationplacement/controller_integration_test.go |
Tests full policy lifecycle. |
config/crd/bases/placement.kubernetes-fleet.io_resourceplacements.yaml |
Tightens rollout bounds. |
config/crd/bases/placement.kubernetes-fleet.io_clusterresourceplacements.yaml |
Tightens CRP rollout bounds. |
config/crd/bases/placement.kubefleet.dev_placementpolicies.yaml |
Bounds selector counts. |
config/crd/bases/placement.kubefleet.dev_clusterplacementpolicies.yaml |
Bounds cluster-policy counts. |
cmd/hubagent/workload/setup.go |
Wires the new controller. |
cmd/hubagent/options/options_test.go |
Tests feature-flag parsing. |
cmd/hubagent/options/featureflags.go |
Adds the feature flag. |
cmd/hubagent/main.go |
Registers APIs and alias seeding. |
charts/hub-agent/values.yaml |
Adds chart configuration. |
charts/hub-agent/templates/rbac.yaml |
Grants policy permissions. |
charts/hub-agent/templates/deployment.yaml |
Passes the feature flag. |
charts/hub-agent/README.md |
Documents chart configuration. |
apis/placement/v1beta1/clusterresourceplacement_types.go |
Adds rollout validation markers. |
apis/placement/v1/clusterresourceplacement_types.go |
Adds v1 rollout validation. |
apis/kubefleet.dev/placement/v1alpha1/placementpolicy_types.go |
Strengthens count validation. |
apis/kubefleet.dev/placement/v1alpha1/common.go |
Defines annotation and label constants. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+103
to
+117
| 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 | ||
| } | ||
| // The owner's own namespace is the policy's: 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. | ||
| source := &unstructured.Unstructured{} | ||
| source.SetGroupVersionKind(gv.WithKind(owner.Kind)) | ||
| source.SetNamespace(accessor.GetNamespace()) | ||
| source.SetName(owner.Name) | ||
| enqueue(source) | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Fixed — enqueueGeneratingResources now enqueues only the owner whose generatedPolicyName reproduces this policy's own name, so a foreign owner (or an owner of a kind outside resource discovery) is skipped rather than lazily creating an informer. Added a foreign-owner-not-followed test (mutation-verified).
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Collaborator
Author
|
Ran this branch on real AKS to cover what Kind can't (live admission webhook, guard rail, multi-replica leader election). Setup: 3 AKS clusters (
Also verified startup fail-fast: with the flag on and the CRDs absent, the agent exits with |
Adds the parser behind annotation-based placement: it turns the value of the kubefleet.dev/place-to annotation into the cluster selectors of a placement policy. The controller that keeps a policy object in sync with the annotation follows separately. The grammar is the one FEP-0001 specifies: a semicolon-separated list of selectors, each a comma-separated list of LABEL_KEY=LABEL_VALUE matchers with an optional count=N|All directive that defaults to 1, and with `region` and `alias` accepted as shorthands for the well-known region label and the reserved cluster alias label. Every key and value is validated against the rules Kubernetes itself enforces, and the parser's own limits are held to the ones the placement policy CRD declares, so a mistaken annotation is reported against the annotation rather than surfacing later as a policy object the API server rejects and a reconciler that retries it forever. Where the FEP is silent the parser errs toward rejecting the input: duplicate label keys (including a shorthand colliding with the key it expands to), a repeated count directive, and an empty label value are all errors rather than silently resolved. A selector carrying only a count directive is allowed, so `count=All` on its own selects the whole fleet. The count ceiling is enforced client-side only. The API's pattern marker constrains the string form of the field alone, so the API server today accepts an integer count both above the ceiling and below one; this is noted on the constant and belongs with the other CEL gaps in the placement policy API. Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
…tion Computes what the placement policy for an annotated resource should look like: its name, the labels recording where it came from, its owner reference, and the resource selector that places the annotated resource itself. The reconciler that creates, updates and deletes it follows separately. The policy's scope follows the annotated resource's own, so the owner reference is always same-scope: a cluster-scoped object owned by a namespaced one is accepted on creation and then never collected at all. The reference claims neither controller nor blocking ownership, since setting either requires permission to update the finalizers subresource of the owner, and the owner here can be a resource of any kind. Names and label values are derived through a new pkg/utils/naming, added rather than written twice: the placement policy controller of #786 has its own private copies of the same helpers, with the same semantics, and can move onto this package when it merges. Two bugs have already been fixed once in those copies, and a second copy is how they come back. Every part of an identity that can outgrow where it is put is shortened through that package, including the API group, which is validated as a DNS-1123 subdomain and so runs to four times what a label value holds. The generated name is shortened the same way and finished with a hash of the whole, untruncated identity, so that resources which differ only past the truncation point, or only in the case of their kind, cannot converge on one policy. Fields the API server defaults are set explicitly. Left unset they are stored with the default and then read as a difference on the next pass, which would have the reconciler issue an update that changes nothing for as long as the policy exists. Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
The annotation key was introduced as a placeholder. Rename it to kubefleet.dev/cluster-selectors, which names the value rather than the action taken on it and matches the clusterSelectors field of the policy generated from it, so that the annotation and the field it stands in for do not have to be translated between when read together. Renames the constant to ClusterSelectorsAnnotation and the parser entry point to parseClusterSelectors to follow. No behavior changes. Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
…otation Adds the reconciler that keeps a generated placement policy in step with the cluster-selectors annotation on a resource: it creates the policy when the annotation appears, updates it when the annotation changes, and deletes it when the annotation is removed. A resource that is itself deleted is deliberately left alone. The generated policy carries an owner reference back to it, so garbage collection removes the policy without this controller racing the collector to delete an object that is already doomed. An annotation the parser rejects is reported to the user as an event on the annotated resource and the key is dropped rather than retried: no number of retries makes a malformed annotation parse. A policy generated from an earlier, valid annotation is left standing, since tearing down a running placement is a worse answer to a typo than keeping the last placement the user did express. Only what the controller generates is overwritten on an existing policy -- the spec, the provenance labels, and its own owner reference. Labels and owner references from elsewhere are preserved so that a generated policy can be labelled by an operator or a GitOps tool without the two taking turns undoing each other. Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
Wires the annotation placement controller into the resource watcher, behind --enable-annotation-based-placement. The flag defaults to off: the feature reads and writes the placement.kubefleet.dev/v1alpha1 policies, whose definitions this chart does not install yet, so the hub agent checks they exist and refuses to start without them rather than retrying against a kind the cluster does not have. The controller shares the resource watcher's informers but is fed only the resources that carry the annotation, which keeps every unrelated object in the cluster out of its queue. An update is enqueued when either the old or the new object carries the annotation: looking at the new one alone would filter out the removal of an annotation, which is precisely the event that has to delete the generated policy. A deleted resource is not enqueued at all, since the policy is garbage collected through its owner reference. The controller gets a rate limiter of its own rather than the one the placement controllers share. An exponential failure limiter keys its backoff on the queued item alone, and this controller queues the same cluster wide keys as the resource change controller, so one limiter between them would let a success here clear the backoff that repeated failures there had earned. Grants the hub agent write access to the generated policies; the broad rule it otherwise relies on is read-only. No status subresource rule: the controller never writes status. Adds an envtest suite covering the round trip against a real API server -- annotation set, changed, made invalid, and removed -- for both a namespaced and a cluster-scoped resource. Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
The controller synchronized in one direction only: it acted on events about the annotated resources, so anything that happened to a generated policy directly went unseen. Deleting a policy, or editing its spec, produced no event on the resource it came from, and for a resource that never changes again the policy stayed missing or wrong forever -- a resync does not step in, since updates whose resource version did not move are dropped. The resource watcher now watches the generated policies themselves and, for every policy event, enqueues the resources the policy names as its owners, so the next reconciliation restores whatever the event changed. The owner references are used rather than the parent labels, which are lossy and, being labels, can be stripped -- itself drift this path exists to repair. The placement.kubefleet.dev group joins the resource config's default skip list along the way: everything in it is KubeFleet's own bookkeeping, and treating the generated policies as placeable resources would let a placement that selects a namespace propagate them. A deleted resource now has its generated policy deleted explicitly rather than left to garbage collection. The collector removes a dependent only once every owner reference on it is gone, and the merge deliberately preserves owner references that other parties add -- any live one of which would have kept the policy standing indefinitely. A resource that stops passing the resource watcher's filter without being deleted -- a ReplicaSet adopted by a Deployment, for instance -- is reported by the watcher as a deletion and then never seen again. The reconciler therefore applies the same eligibility test itself and deletes the generated policy of a live but ineligible resource, that being its last chance to clean up. A key whose kind the API server does not know, which the policy watch can produce by enqueuing whatever a policy names as its owner, is dropped rather than retried: no retry makes the kind exist, and the key would otherwise back off on the queue forever. Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
Deleting a generated policy re-enters the reconciler through the generated policy watch, and the second pass's cached read can still see the object the first pass just deleted. The delete it then issues comes back not found, which was being swallowed into a success: one real deletion produced two "deleted" log lines, and on the annotation-removal path would have produced two events. Surfaced by exercising the controller on a kind cluster rather than by any test. A delete that finds nothing now reports that this pass deleted nothing, matching what the read-side already did when the policy was gone before the pass began. Also settles the whenUnfulfilled question the policy derivation had flagged as open: generated selectors keep the API's RequestCluster default deliberately, since 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. Guarding against a selector that nothing can ever satisfy belongs to the claim fulfillment layer. Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
The count of a cluster selector is an int-or-string whose only bound was a pattern, and a pattern constrains the string form alone: the API server rejected count: "1000" while accepting count: 1000, along with zero and negative integers the field's own documentation rules out. A CEL rule now bounds the integer form to the same 1-999 range the pattern holds the string form to. The parser had been holding this line by itself for annotation-derived counts; hand-authored policies had no line at all. The bounds are pinned twice: envtest submits both forms of both verdicts to a real API server, and the parser's limits test asserts the CRD's rule and message name the same ceiling the parser enforces, without binding either to an exact spelling. Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
…ts own
The minCount<=count rule guarded its string comparison with an
unbounded digit match, so a count too long for an int64 reached the
int() conversion and turned a plain validation failure into an opaque
evaluation error beside it. The guard now recognizes only the numeric
strings of at most the three digits the count field itself permits;
anything else -- "All", or junk the field's own validation rejects --
passes the rule vacuously, so the real problem is reported alone, and
the message drops a qualifier about a case the rule no longer treats
specially.
Count additionally gains a MaxLength matching its longest legal values,
which rejects an over-long string outright. The digit guard is spelled
[0-9]{1,3} rather than mirroring the count pattern's own numeric branch
verbatim: the CEL cost estimator prices a regex by its length against
an unbounded string, an int-or-string being opaque to the sibling
MaxLength, and the longer spelling does not fit the budget.
Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
The maxUnavailable and maxSurge fields of a rolling update strategy are int-or-strings whose only bound was a pattern, and a pattern constrains the string form alone: the API server rejected maxUnavailable: "-1" while accepting maxUnavailable: -1, a value the field's documentation rules out and the rollout controller cannot act on. A CEL rule now keeps the integer form non-negative in both the v1 and v1beta1 APIs. For ClusterResourcePlacement the validating webhook already rejected negative values, so this closes a gap only where the webhook is not running. ResourcePlacement has no working webhook behind it at all, so there the schema is the only line of defense; the new tests cover both kinds for that reason. Note that these are served API versions: a manifest carrying a negative integer bound, previously accepted and stored, is rejected on its next update that changes the value. Unchanged stored values are ratcheted through, since the field sits on a plain struct path that validation correlates across updates. Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
A deletion the watch missed arrives as a tombstone wrapping the object's final state, and the resource watcher's filter derived its key from the tombstone itself, which is not a runtime object. Every relist-detected deletion was therefore dropped before any handler saw it, even though the delete handler has always known how to unwrap one. The filter now unwraps the tombstone before judging the object, and the test that pinned the dropped-deletion behavior as intended now pins the opposite. Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
FEP-0001 reserves the kubefleet.dev/cluster-alias label for selecting member clusters by name, and its own examples select by alias -- yet nothing has ever set the label, so every alias selector matched no clusters at all. With annotation-based placement enabled, the member cluster controller now seeds the alias from the cluster name on join, and only when the label is absent entirely. Unlike the member name label beside it, which states a fact the controller owns and reasserts, the alias exists to be renamed: it is the indirection that lets a selector follow the cluster playing a role rather than a fixed name, so a value an admin changed is never reverted. Fleets that do not run the feature are left untouched. The guard rail that optionally denies member cluster label changes exempted only the kubernetes-fleet.io prefix; it now exempts the kubefleet.dev prefix as well. The hub agent is not in system:masters, and denying it this label would have failed the same update that maintains the member name label, wedging the cluster's reconciliation. Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
The digit branch of the rolling update pattern accepted a string of any length, which passed validation only to fail in whatever consumed it later. It is now bounded to nine digits, all of which fit an int32 comfortably, in both the v1 and v1beta1 APIs; the same ratcheting considerations apply as for the sign bound before it. Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
An event about a ClusterPlacementPolicy called it a placement policy, so pasting the name from the event into kubectl against that kind found nothing. The created, updated, and deleted events now name the kind verbatim. The kind is asked of the same function that decides the scope, keeping that decision in the one place its contract promises, and the deleted event's doc comment now covers both of its causes. Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
The cluster alias label is seeded on member cluster join and 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. The member cluster validating webhook now warns when the alias being set is already held by another cluster. It is 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 state must stay legal while transient. For the same reason a failure to list the member clusters admits the request without a warning rather than blocking it -- the webhook fails closed, so an advisory check that errored on a listing hiccup would wedge every member cluster write. Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
Yetkin Timocin (ytimocin)
force-pushed
the
feat/fep0001-annotation-based-placement
branch
from
August 19, 2026 16:20
7f38e9b to
8eb13bd
Compare
| | `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, which this chart does not install. | `false` | |
Collaborator
Author
There was a problem hiding this comment.
We should mention which chart installs the necessary CRDs.
Collaborator
Author
There was a problem hiding this comment.
Updated — no chart installs the placement.kubefleet.dev CRDs yet, so the row now points at config/crd/bases/ and notes the hub agent fails fast if the flag is on without them.
| 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, |
Collaborator
Author
There was a problem hiding this comment.
Is this name aligned with the repository patterns?
Collaborator
Author
There was a problem hiding this comment.
SeedClusterAliasLabel follows the existing boolean-option field naming on the reconciler; the "membercluster-controller" registration string is pre-existing and unchanged. Happy to rename if you had a specific convention in mind.
Comment on lines
+50
to
+61
| // 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" |
Collaborator
Author
There was a problem hiding this comment.
Do these apply for both PlacementPolicy and ClusterPlacementPolicy?
Collaborator
Author
There was a problem hiding this comment.
Yes — the reasons are scope-neutral and apply to both PlacementPolicy and ClusterPlacementPolicy; the event message names the concrete kind. Clarified in the doc comment.
Comment on lines
+85
to
+92
| // ShouldPlace reports whether a resource is one KubeFleet places at all, mirroring the filter | ||
| // the resource watcher applies to its events. 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. | ||
| // | ||
| // Left nil, every resource is eligible. | ||
| ShouldPlace func(source *unstructured.Unstructured) (bool, error) |
Collaborator
Author
There was a problem hiding this comment.
We should be more clear here. A simple example would help for both cases: should place or should not place.
Collaborator
Author
There was a problem hiding this comment.
Added examples to the ShouldPlace doc: a Deployment in a user namespace places; a ConfigMap in a skipped namespace like kube-system, or a ReplicaSet a Deployment already owns, does not.
|
|
||
| source, err := r.sourceObject(clusterWideKey) | ||
| switch { | ||
| case apierrors.IsNotFound(err): |
Collaborator
Author
There was a problem hiding this comment.
Could there be a race condition here where the resource could be manually deleted but would be recreated by another controller?
Collaborator
Author
There was a problem hiding this comment.
No corruption. A delete-then-recreate by another controller yields a distinct object (new UID): the delete reconcile removes the policy, the recreate fires a fresh event and generates a policy under the same deterministic, name-derived name via create-or-update, carrying the new owner reference. The name is keyed on identity, not UID, so it converges.
Comment on lines
+118
to
+125
| 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 |
Collaborator
Author
There was a problem hiding this comment.
The error messages must be aligned with the repository standards.
Collaborator
Author
There was a problem hiding this comment.
These follow the repo convention (lowercase, no trailing punctuation, %w-wrapped, %+v for keys) — same as rollout/workgenerator. If you meant a specific message, point me at it and I'll align.
| klog.ErrorS(err, "Failed to decide whether the resource is eligible for placement", "obj", clusterWideKey) | ||
| return ctrl.Result{}, err | ||
| } | ||
| if !eligible { |
Collaborator
Author
There was a problem hiding this comment.
Could there be a scenario where an object could be ineligible at time x but then eligible at time y?
Collaborator
Author
There was a problem hiding this comment.
Yes — e.g. a ReplicaSet becomes eligible again once orphaned. It is handled: the transitions that matter (owner references, labels) are edits to the resource, so each fires an event that re-runs ShouldPlace, and a resource that becomes eligible again while still annotated has its policy regenerated on that event. Noted in the ShouldPlace doc.
| // | ||
| // 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 { |
Collaborator
Author
There was a problem hiding this comment.
Why do we pass the cause to this function? This function deletes the policy and prints a message. That is not aligned with single responsibility principle.
Collaborator
Author
There was a problem hiding this comment.
The cause is the event's human-readable reason. deletePolicy deletes the generated policy and records why in one place — the single spot that already derives the policy's generated name. Moving the event emission to the callers would spread that name derivation and the event format across each call site. I kept it cohesive; happy to refactor to caller-emitted events if you'd prefer.
…mespaces Two correctness gaps found in review of the generated-policy watch and the placement eligibility check. The watch re-enqueued every owner reference a policy carried, so a foreign owner -- one the merge preserves, or any owner on a policy that merely shares these informers -- was treated as a generating source. If its kind was one the resource watcher does not track, reading it lazily created an informer outside the resource configuration. Only the owner whose identity reproduces the policy's own generated name is followed now; matching a 64-bit identity hash admits nothing else. The eligibility check read metadata.namespace to decide whether a resource's namespace is skipped, but a Namespace object carries none and is itself the namespace, so an annotated "default" or "kube-system" generated a policy for a namespace KubeFleet excludes. The namespace is now taken from the object's own name for a Namespace, matching how the resource change controller already special-cases the kind. Also clarifies, per review, that the event reasons are the same for both generated kinds, that eligibility can change over a resource's life and is re-checked on the events that change it, and where the placement CRDs come from until a chart installs them. Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
…controllers The guard rail that optionally denies member cluster label changes exempted the whole kubefleet.dev/ prefix for every user. Unlike the member-name label the controller reasserts each reconcile, the cluster alias is left as it is set, so a non-admin's edit to it would persist and redirect alias-based placements to another cluster. The prefix is now exempt only for a whitelisted identity (the hub agent, which seeds the alias) or a cluster admin; ordinary users are denied. The kubernetes-fleet.io/ prefix stays exempt for everyone as before, and admins can still rename an alias for a cluster handoff, since they are exempted before this check runs. Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
…lete path deletePolicy derived the generated policy name and deleteGeneratedPolicy derived it again for the same policy -- two SHA-256 hashes of the same identity per deletion. deleteGeneratedPolicy now returns the name it computed, and deletePolicy uses it. No behavior change. Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
…feedback Address review findings on the annotation-based placement controller: - Sanitize characters legal in a resource's own name but not in a Kubernetes object name (an RBAC name like system:aggregate-to-admin, or a dot beside another separator) before they reach a generated policy's name, which the API server would otherwise reject and hot-loop the reconciler. The uniqueness hash is still taken over the full, unsanitized identity. - Leave a policy this controller did not generate untouched: before updating or deleting the policy at a resource's deterministic name, verify it carries this controller's provenance labels, so a hand-authored policy colliding with that name is neither overwritten nor deleted. The conflict is surfaced to the user through an event. - Clean up a stale generated policy when its source's API is gone: a source of a config-excluded API stops being placed, and a key whose kind the API server no longer knows has its generated policy deleted rather than left behind. A key naming a version retired while the kind lives on falls back to the served version instead of being treated as a gone kind. - Converge the owner reference across a source deleted and recreated under the same name: match the existing reference on the source's group, kind, and name rather than its UID, so the one reference is updated in place instead of a stale one accumulating every cycle. Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
Two further review findings on the annotation-based placement controller: - A generated policy was recognized as this controller's own only when all of its provenance labels matched, and that check gates both the repair path, which restores drifted labels, and the delete path. Editing one label on a generated policy therefore made it neither repairable nor removable, so its placement could keep running after the annotation was removed. Recognize ownership from either marker this controller stamps -- the owner reference to the source or the provenance labels -- so a single drifted marker no longer strands the policy, while a foreign policy that merely collides with the name still carries neither and is left untouched. - The source's scope was read from the queued key's group-version-kind, which the informer manager keys on the exact version. When a served version is retired, or the version-agnostic mapping fallback resolves a different one than the key named, a cluster-scoped source was read through a namespaced lookup, missed, and taken for deleted, deleting and recreating its policy. Read the scope from the resolved REST mapping instead. Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
…conflict Two further review findings on the annotation-based placement controller: - Generated policies are watched through a different informer than the controller's cache read from, so an edit or deletion the watch delivered first could be met with a stale object: the pass saw no change and returned, leaving drift until the next resync and, for a deletion -- which a later event or resync never re-delivers, the object being gone from the watch's own cache -- leaving the placement removed indefinitely. Read the policy through an uncached reader, current as of the moment the watch fired; writes still go through the cached client. The queue holds only annotated resources and generated-policy owners, so the read is not on a hot path. - When a policy occupies a resource's generated name but was authored by someone else, the reconciler left it untouched and returned without asking to be run again. Removing that policy fires no event that reaches the source, since it carries no owner reference to it, and the annotation does not change, so the requested policy would never be created once the conflict was cleared. Requeue the source while the conflict stands so the placement is created as soon as the name is free. Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
…nt race deleteGeneratedPolicy read the policy through the uncached reader and confirmed it was one this controller generated, then deleted it by name with no precondition. A delete targeting the name alone removes whatever occupies it, so a policy replaced between the read and the delete -- deleted and a hand-authored one created at the same name, or overwritten in place with its provenance stripped -- would be deleted even though it was no longer this controller's. Carry the resource version the read returned as a delete precondition, so the delete removes only the exact object this pass verified. A replacement moves the resource version, turning the delete into a conflict that requeues; the next pass reads the current object and, finding it foreign, declines it. The resource version guards both a delete-and-recreate and an in-place overwrite, where a UID precondition would miss the latter. Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
Collaborator
Author
|
Pushed a round of fixes addressing review feedback. Each ships with a regression test that fails without the fix:
Also ran a local end-to-end smoke pass (kind hub, feature flag enabled) across 13 scenarios covering all of the above — basic generation, cluster-scoped sources, RBAC-name sanitization, provenance-drift repair, direct policy deletion/recreation, the name-conflict flow, config-excluded and skipped-namespace sources, and source delete/recreate — all green. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of your changes
Implements FEP-0001 annotation-based placement (#785): setting
on any resource the hub agent watches keeps a
PlacementPolicy(namespaced source) orClusterPlacementPolicy(cluster-scoped source) in sync with the annotation for as long as it is present. The annotation is named after thespec.clusterSelectorsfield it compiles into, per the discussion on #785;regionexpands totopology.kubernetes.io/regionandaliastokubefleet.dev/cluster-alias.How it works
pkg/controllers/annotationplacement/parser.go): pure translation of the annotation grammar into[]ClusterSelector, mirroring the API's own limits so a bad annotation is reported against the annotation rather than surfacing later as a failed policy write.policy.go): deterministic name<kind>-<name>-<16-hex hash of apiGroup/Kind/namespace/name>, provenance labels (placement.kubefleet.dev/parent-api-group|parent-kind|parent-name), and a non-controller owner reference to the annotated resource (settingcontroller/blockOwnerDeletionwould needupdateon the owner'sfinalizerssubresource for arbitrary kinds). All API-defaulted fields are set explicitly so reconciliation converges instead of issuing no-op updates forever.controller.go): creates/updates/deletes the generated policy; recordsPlacementPolicyCreated/Updated/DeletedandInvalidClusterSelectorsAnnotationevents on the annotated resource. An invalid annotation is not retried (no retry makes it parse) and the policy from the last valid annotation is left standing. The merge overwrites only what this controller generates — foreign labels and owner references are preserved.--enable-annotation-based-placement(default off), with a fail-fastCheckCRDInstalledfor theplacement.kubefleet.devCRDs, a dedicated workqueue rate limiter (this controller shares the resource change controller's key space, and an exponential failure limiter keys backoff on the item alone), RBAC for the generated policies, and the flag plumbed through the hub-agent chart. Theplacement.kubefleet.devgroup joins the resource config's default skip list so KubeFleet's own bookkeeping objects are never treated as placeable resources.kubefleet.dev/cluster-aliasfrom the cluster name on join — set-if-absent only, so an alias an admin renamed is never reverted. Without this, FEP-0001's ownalias=bravelionexample selects zero clusters. The optional deny-member-cluster-label-changes guard rail now exempts the reservedkubefleet.dev/prefix alongsidekubernetes-fleet.io/, since the hub agent's own update would otherwise be denied and wedge reconciliation.pkg/utils/naming: shared truncate/hash/label-value helpers extracted so feat: [FEP-0001] implement the placement policy controller #820 can drop its private copies on its next rebase (verified behaviour-identical).Adjacent fixes that surfaced while testing
countaccepted out-of-range bare integers:Patternconstrains only the string form of an int-or-string, socount: 1000was accepted whilecount: "1000"was rejected. A CEL rule now bounds the integer form to the same 1–999; proven against a real API server in envtest.minCount<=countproduced an opaque evaluation error beside a plain pattern violation for over-long digit strings; the rule now leaves malformed strings to the field's own validation, andcountgainsMaxLength=3(its longest legal values are exactly 3 characters).maxUnavailable/maxSurge(v1 and v1beta1) accepted negative bare integers (-1unquoted passed;"-1"was rejected) and digit strings of unbounded length. CEL bounds the integer form at ≥0 and the pattern's digit branch is bounded to nine digits. These touch served APIs — kept as separate commits for release-note labeling; stored out-of-range values are ratcheted through on unrelated updates since the fields sit on a correlatable struct path. Note: the ResourcePlacement validating webhook is currently never invoked (handler registered but noValidatingWebhookConfigurationrule), so for RP the schema is the only line of defense — a separate issue will track that.Deliberately out of scope
placement.kubefleet.devCRDs are not added to the hub-agent chart (crd-verifyexcludes them until the implementation completes, per interface: [FEP-0001] add API definition for the Placement Policy and Cluster Request API objects #781); the flag defaults to off and the agent refuses to start if it is on without them.whenUnfulfilledon generated selectors keeps the API'sRequestClusterdefault deliberately: requesting 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 ask. Guarding against unsatisfiable selectors belongs to the claim fulfillment layer.Fixes #785
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
annotationplacement98%+,naming100%), including mutation-tested assertions (e.g. removing the old-object annotation check fails the removal test).denyModifyMemberClusterLabels=true): the same lifecycle matrix, plus alias seeding on MemberCluster join under the live guard rail, an admin's alias rename surviving reconciles, the guard rail still denying non-reserved label changes, thealias=shorthand selecting the renamed alias, the count CEL bound rejecting an out-of-range integer at the API server, and leader failover with reconciliation resuming under the new leader.Special notes for your reviewer
maxUnavailable/maxSurgecommits ("bound the integer form of the rolling update settings", "bound the digit form of the rolling update settings") tighten validation on served v1/v1beta1 APIs and can be cherry-picked into their own PR with arelease-note/breakinglabel if preferred — perVERSIONING.mdthat class of change warrants a minor bump.PlacementPolicyobjects; the controllers that consume them are gated by--enable-placement-policy-apison feat: [FEP-0001] implement the placement policy controller #820's branch. With this PR's flag on and that one off, generated policies sit inert with no error anywhere — the worst kind of misconfiguration, since every component looks healthy. Neither branch can check a flag the other defines, so whichever PR merges second should add a startup guard: refuse to start if--enable-annotation-based-placementis set without--enable-placement-policy-apis, same fail-fast shape as this PR's CRD check. (If this PR lands first, that guard belongs in feat: [FEP-0001] implement the placement policy controller #820's rebase, which also owes the migration of its private naming helpers topkg/utils/namingintroduced here.)whenUnfulfilleddefault noted above.