Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #4564 by switching Fleet’s namespace label/annotation reconciliation from a read-modify-write update to server-side apply (SSA), so Fleet only owns and prunes the specific metadata keys it declares while preserving metadata set by other actors (e.g., Rancher project assignment annotations).
Changes:
- Replace namespace metadata updates with SSA using a dedicated, stable field manager.
- Add a one-time managedFields migration to transfer legacy Update-based ownership of namespace labels/annotations into the SSA manager so dropped keys can be pruned after upgrades.
- Add unit tests for managedFields migration logic and an envtest-backed integration test to validate SSA ownership/pruning behavior against a real API server.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/cmd/agent/deployer/namespace_managed_fields.go | Adds managedFields migration to move legacy Update ownership of labels/annotations into the SSA field manager. |
| internal/cmd/agent/deployer/namespace_managed_fields_test.go | Unit tests for managedFields migration patch generation and scoping behavior. |
| internal/cmd/agent/deployer/deployer.go | Switches namespace labels/annotations reconciliation to SSA, adds legacy managedFields migration hook, and updates RBAC error messaging. |
| internal/cmd/agent/deployer/deployer_test.go | Updates unit tests to reflect SSA behavior (preserve foreign metadata, idempotency, Apply interception). |
| internal/cmd/agent/deployer/deployer_ssa_envtest_test.go | Envtest-backed test validating SSA field ownership, pruning, and legacy managedFields migration behavior. |
| go.mod | Promotes structured-merge-diff to a direct dependency for managedFields/fieldpath operations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1 task
p-se
force-pushed
the
issue-4564
branch
2 times, most recently
from
September 7, 2026 07:30
9be4513 to
aa8aa7a
Compare
1 task
d3flex
approved these changes
Sep 14, 2026
0xavi0
reviewed
Sep 14, 2026
|
|
||
| legacyIdx := -1 | ||
| for i, e := range entries { | ||
| if e.Manager == legacyNamespaceFieldManager && e.Operation == metav1.ManagedFieldsOperationUpdate && e.Subresource == "" { |
Contributor
There was a problem hiding this comment.
What happens with fields owned by Helm?
If my understanding is correct, two things write as manager fleetagent, operation Update:
- the old c.Update(ctx, ns) in setNamespaceLabelsAndAnnotations — the intended target
- Helm, when it creates the release namespace with labels: {name: }, because getManagedFieldsManager() returns filepath.Base(os.Args[0]) and Fleet never overrides it
I think that name: owned by Helm is gone, right?
| // It is skipped unless KUBEBUILDER_ASSETS points at envtest binaries (e.g. via | ||
| // `setup-envtest use -p env`). | ||
| func TestSetNamespaceLabelsAndAnnotations_ServerSideApply(t *testing.T) { | ||
| if os.Getenv("KUBEBUILDER_ASSETS") == "" { |
Contributor
There was a problem hiding this comment.
Is this value going to be valid here? (if it's not the test self skips)
Shouldn't this whole file be moved to integrationtests instead?
Comment on lines
-379
to
-389
| func addLabelsFromOptions(nsLabels map[string]string, optLabels map[string]string) { | ||
| maps.Copy(nsLabels, optLabels) | ||
|
|
||
| // Delete labels not defined in the options. | ||
| // Keep the `kubernetes.io/metadata.name` label as it is added by kubernetes when creating the namespace. | ||
| for k := range nsLabels { | ||
| if _, ok := optLabels[k]; k != corev1.LabelMetadataName && !ok { | ||
| delete(nsLabels, k) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Is this logic from PR #5687 missing in the new code? (I can't find it)
Manage the release namespace's labels and annotations with server-side apply instead of a read-modify-write update. The previous approach fetched the namespace, merged in the configured labels/annotations, and then deleted every key not present in the options, preserving only a hardcoded allowlist (kubernetes.io/metadata.name and pod-security.kubernetes.io/*). That clobbered metadata set by other actors -- most notably field.cattle.io/projectId, which Rancher adds when a namespace is moved into a Project -- and the allowlist could never be made complete. See issue rancher#4564. Fleet now applies with a dedicated field manager ("fleet-agent-namespace-metadata"), so it owns exactly the keys it declares: keys owned by other managers are left untouched, and a key Fleet stops declaring is pruned by the apiserver. ForceOwnership adopts keys previously written by the old update, so the first apply after upgrade does not conflict. Labels and annotations are applied independently. A bundle that sets only namespaceLabels never asserts ownership of annotations (and so never prunes them), and vice versa. pod-security.kubernetes.io/* labels are still stripped from the options, but existing ones on the namespace are now preserved implicitly: Fleet never declares them, so the apply does not touch them. The deployment's service account now needs 'patch' on the namespace instead of 'update'; the RBAC error messages are updated accordingly. Namespace creation remains Helm's responsibility -- ensureNamespaceExists only verifies the namespace is present and surfaces a clear RBAC error before the apply. Adds an envtest covering the pruning and cross-manager preservation behavior.
Namespaces that predate the switch to server-side apply still carry a "fleetagent" Update field-manager entry that owns their labels and annotations. ForceOwnership on the first post-upgrade apply makes Fleet's SSA manager co-own those keys but does not strip the stale Update entry, and a field is only pruned once no manager owns it. As a result, a key dropped from a bundle stays behind forever on any such namespace. Add a self-healing, idempotent migration that moves ownership of just the metadata.labels/metadata.annotations fields from the legacy Update entry into the SSA apply entry, leaving any other fields (e.g. finalizers) the general-purpose "fleetagent" manager owns untouched. This is scoped deliberately rather than absorbing the whole entry via csaupgrade. Runs before the namespace metadata apply and is a no-op once migrated.
Helm labels the release namespace with `name: <namespace>` when it creates it. The previous read-modify-write update removed every label that was not declared in the bundle options, so that label never survived and the e2e expectations encoded its absence. With server-side apply Fleet owns only the keys it declares, so labels written by other field managers are left alone. Update the two namespace label expectations accordingly.
The previous read-modify-write update deleted every namespace label and annotation that was not listed in the bundle options, regardless of who had written it. The only e2e coverage of that behaviour was incidental: an exact-match assertion in the target customization specs that happened to require Helm's `name` label to be gone. Updating those expectations for server-side apply removed it, and nothing asserted the new contract. Add a spec that pins both halves of it. A bundle declares two namespace labels and two annotations, a third label and annotation are written directly with kubectl so the API server records a different field manager, and one key is then dropped from the bundle options. Fleet must prune exactly that key and leave everything else in place. Also correct the comment added with the previous expectation update: the `name` label is written by Helm through the agent's own "fleetagent" field manager, not by an unrelated actor. Ownership is per field manager, not per writer.
The legacy managed-fields migration moved every metadata.labels and metadata.annotations path owned by the "fleetagent" Update entry into Fleet's server-side apply manager. That entry is not exclusively the old read-modify-write update it was meant to catch. Fleet does not create the release namespace, Helm does, and Helm labels it "name: <namespace>". Both client-go and Helm derive the field manager from the binary name (getManagedFieldsManager falls back to filepath.Base(os.Args[0])), so Helm's create is recorded under the same manager, with the same Update operation, and the API server merges the two into a single entry. The migration therefore ran on every fresh install, absorbed Helm's label, and the very next apply pruned it because Fleet never declares it. Take ownership only of the keys the bundle declares right now. A key Fleet declares is one it is about to own anyway, so the migration no longer asserts a claim to metadata it has no business managing: Helm's label stays put, and on a freshly created namespace the migration computes no patch at all. Keys the bundle still declares are migrated as before, so dropping one later prunes it. The map itself, the "." member of metadata.labels, is deliberately left with the legacy manager; owning the map is not owning its entries. The envtest expectation is adjusted accordingly: the legacy entry may survive, what must be gone is its ownership of the declared keys. Trade-off: a key dropped from the bundle in the same change that upgraded Fleet is never absorbed and stays on the namespace. Re-adding and dropping it again migrates and prunes it.
The envtest-backed namespace metadata test lived in the deployer package, where CI never ran it: ci.yml filters /integrationtests out of the unit test package list and sets no KUBEBUILDER_ASSETS, so the test skipped itself on every run. It was also the only envtest user outside integrationtests/. Rewrite it as four Ginkgo specs in integrationtests/agent, which already wires a real helmdeployer and Deployer against envtest and runs in CI. The specs drive real BundleDeployment reconciles instead of calling unexported deployer methods. An options-only spec change is enough to trigger a second sync, since DeployBundle runs on every reconcile and setNamespaceLabelsAndAnnotations runs unconditionally after helmdeploy. The managedFields ownership assertion is dropped as redundant: pruning is the observable consequence of a successful migration, and the field set shapes are covered by the pure-function tests in namespace_managed_fields_test.go. Two notes on the new Helm "name" label spec. It pins the literal "fleetagent" rather than importing the unexported constant, because the point is that this exact wire value is shared between client-go and Helm. And it sets createNamespace: false, because Helm 4 creates the release namespace with server-side apply, which would claim the label for an Apply entry of its own and hide the ownership situation under test. The case that bites in practice is a namespace that predates the SSA switch, where Helm's label sits in the same legacy Update entry as the metadata Fleet wrote.
The comment explaining why pod-security.kubernetes.io/* labels are not filtered out of namespaceLabels lived on addLabelsFromOptions, which was deleted when namespace metadata moved to server-side apply. The behavior from rancher#5687 was kept, but the explanation went with the function, and a reviewer reading the diff reasonably asked whether the logic had been lost too. Restate it on applyNamespaceMetadata, the function that now applies the options: the namespace is patched as the deployment's service account, so which labels a bundle may set is gated by that account's downstream RBAC, and deployments without a service account still run as the agent. Also note that a pod-security label the bundle does not declare is left alone, since Fleet never asserts ownership of it.
This branch has not been deployed
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.
SSA for namespaces
Manage the release namespace's labels and annotations with server-side apply instead of a read-modify-write update.
The previous approach fetched the namespace, merged in the configured labels/annotations, and then deleted every key not present in the options, preserving only a hardcoded allowlist (kubernetes.io/metadata.name). That clobbered metadata set by other actors -- most notably field.cattle.io/projectId, which Rancher adds when a namespace is moved into a Project -- and the allowlist could never be made complete. See issue #4564.
Fleet now applies with a dedicated field manager ("fleet-agent-namespace-metadata"), so it owns exactly the keys it declares: keys owned by other managers are left untouched, and a key Fleet stops declaring is pruned by the apiserver. ForceOwnership adopts keys previously written by the old update, so the first apply after upgrade does not conflict.
Labels and annotations are applied independently. A bundle that sets only namespaceLabels never asserts ownership of annotations (and so never prunes them), and vice versa.
Security-sensitive labels such as pod-security.kubernetes.io/* are not filtered out of the options; #5687 removed that filter. A bundle that declares one applies it like any other label, gated by the downstream RBAC of the deployment's service account, since the namespace is patched as that account. Deployments that resolve to no service account still run as the agent, so restricting them requires pinning a service account, either in the bundle or through a Policy. A pod-security label already on the namespace that the bundle does not declare is now preserved implicitly: Fleet never declares it, so the apply does not touch it.
The deployment's service account now needs 'patch' on the namespace instead of 'update'; the RBAC error messages are updated accordingly. Namespace creation remains Helm's responsibility -- fetchExistingNamespace only verifies the namespace is present and surfaces a clear RBAC error before the apply.
Migrate legacy namespace managed fields to enable SSA pruning
Namespaces that predate the switch to server-side apply still carry a "fleetagent" Update field-manager entry that owns their labels and annotations. ForceOwnership on the first post-upgrade apply makes Fleet's SSA manager co-own those keys but does not strip the stale Update entry, and a field is only pruned once no manager owns it. As a result, a key dropped from a bundle stays behind forever on any such namespace.
Add a self-healing, idempotent migration that moves ownership of the metadata.labels/metadata.annotations keys the bundle currently declares from the legacy Update entry into the SSA apply entry, leaving every other field the general-purpose "fleetagent" manager owns untouched, including finalizers and any label or annotation the bundle does not declare. This is scoped deliberately rather than absorbing the whole entry via csaupgrade. It runs before the namespace metadata apply and is a no-op once migrated.
Scoping the migration to the declared keys is what keeps it from taking metadata Fleet has no claim to. The legacy manager cannot be identified any more precisely than by name, and that name is shared: client-go derives it from the binary, and so does Helm. The entry Helm leaves behind when it creates the release namespace, carrying its "name: " label, is therefore indistinguishable from the entry the old read-modify-write update left. Absorbing it wholesale would make Fleet own Helm's label and prune it on the next apply. A key Fleet declares, by contrast, is one it is about to own anyway. With Helm 4 the collision is limited to namespaces that predate the SSA switch and to bundles using takeOwnership or force, because Helm 4 otherwise creates the namespace with server-side apply and records the label under an Apply entry of its own.
Known trade-off: a key dropped from the bundle in the same change that upgraded Fleet is never absorbed and stays on the namespace. It self-heals if the key is re-added and then dropped again.
Tests
Unit tests for the managed-fields migration in internal/cmd/agent/deployer, integration tests in ./integrationtests/agent that drive real BundleDeployment reconciles against envtest, and e2e specs in ./e2e/single-cluster.
Refers to #4564
Additional Information
Checklist