diff --git a/docs/primitives.md b/docs/primitives.md index 971bac1f..87f0e672 100644 --- a/docs/primitives.md +++ b/docs/primitives.md @@ -129,8 +129,38 @@ are sent; server-managed defaults, fields set by other controllers (HPAs, sideca and values written by webhooks are left untouched. The API server tracks field ownership automatically. The field manager name is derived from the owner and component as -`"{Owner.GetKind()}/{componentName}"`. The framework applies with forced ownership, so it takes control of conflicting -fields from other managers, while fields it does not include stay with their current owners. +`"{Owner.GetKind()}/{componentName}/{Owner.GetUID()}"`, for example +`ExampleApp/web-interface/3d8a9d5e-1c2b-4f6e-9a7d-0b1c2d3e4f5a`. The framework applies with forced ownership, so it +takes control of conflicting fields from other managers, while fields it does not include stay with their current +owners. + +The API server rejects a field manager longer than 128 characters, and neither the kind nor the component name has a +bounded length. When the readable form would exceed the limit, the manager is the hex-encoded SHA-256 of that readable +form instead: 64 characters, deterministic for the owner and component, and still distinct per owner. Such a manager +shows in `managedFields` as a hash rather than a name, so keep component names short if you rely on reading them there. + +The owner's UID is part of the manager name so that every owner is a distinct manager. Two custom resources of one kind +whose components render the same object therefore do not share a manager. When the framework sets a controller reference +on the object (the default), the second owner's apply carries a second controller reference, which the API server +rejects, so that owner's component reports an error instead of silently taking the object's fields from the first. With +a manager shared across owners, each forced apply would relinquish the other owner's fields wholesale, both would report +converged, and neither would ever see a conflict. Naming the owner also makes +`kubectl get -o yaml --show-managed-fields` say which custom resource wrote a field. + +The rejection depends on the controller reference. For a resource registered with `Unowned()`, or one whose owner +reference cannot be set because of a scope mismatch, nothing stops the second owner's forced apply from taking the +fields it declares, and the fields move between the two owners' managers on every reconcile. `managedFields` then names +the owner that wrote each field, but the framework does not detect the contention. A shared name between two owners is +the operator's responsibility in that case; a [guard](component.md#guards) that reads the live object and blocks when +another owner controls it is the way to make it explicit. + +!!! note "Upgrading from a release without the UID in the manager name" + + Releases before the UID was added used `"{Owner.GetKind()}/{componentName}"`. On the first reconcile after + upgrading, the new manager takes over every field it declares from the old one through forced ownership, so + managed objects converge without intervention. Fields the old manager owned that are no longer part of the desired + state stay in `managedFields` under the old manager name and are not pruned; they were already stale before the + upgrade. This removes the perpetual-update problem that arises when an operator strips server defaults every cycle, and it lets primitives coexist with other controllers that touch the same resources. diff --git a/pkg/component/create.go b/pkg/component/create.go index 7a5899c5..706d065d 100644 --- a/pkg/component/create.go +++ b/pkg/component/create.go @@ -2,6 +2,8 @@ package component import ( "context" + "crypto/sha256" + "encoding/hex" "fmt" "reflect" "strings" @@ -191,6 +193,35 @@ func applyFailed(rec ReconcileContext, labels ResourceMetricLabels, err error) e return err } +// applyFieldOwner returns the Server-Side Apply field manager for one component +// of one owner: "//", for example +// "ExampleApp/web-interface/3d8a9d5e-1c2b-4f6e-9a7d-0b1c2d3e4f5a". The owner's +// UID is part of the name so that two owners of the same kind whose components +// render the same object are distinct managers to the API server. With a manager +// shared across owners, each owner's forced apply would relinquish the other's +// fields wholesale and neither would ever see a conflict. The UID rather than +// the owner's name keeps the manager independent of the length of user-chosen +// names and reads the same for cluster-scoped and namespaced owners. +// +// The API server rejects a field manager longer than fieldManagerMaxLength. +// Neither the kind nor the component name has a bounded length, so when the +// readable form would exceed the limit the manager is the hex-encoded SHA-256 +// of that readable form instead: 64 characters, still deterministic for the +// owner and component, and still distinct per owner. +func applyFieldOwner(owner OperatorCRD, componentName string) client.FieldOwner { + manager := fmt.Sprintf("%s/%s/%s", owner.GetKind(), componentName, owner.GetUID()) + if len(manager) > fieldManagerMaxLength { + sum := sha256.Sum256([]byte(manager)) + manager = hex.EncodeToString(sum[:]) + } + return client.FieldOwner(manager) +} + +// fieldManagerMaxLength is the API server's limit on a field manager name, +// mirroring k8s.io/apimachinery/pkg/apis/meta/v1/validation.FieldManagerMaxLength. +// Longer managers are rejected. +const fieldManagerMaxLength = 128 + // applyResources ensures that all registered "creation" resources exist and match // the desired state in the Kubernetes cluster using Server-Side Apply. // @@ -209,16 +240,15 @@ func applyFailed(rec ReconcileContext, labels ResourceMetricLabels, err error) e // cluster with forced field ownership. Only operator-managed fields are sent; server-defaulted // fields (e.g., imagePullPolicy, strategy) are untouched. This prevents perpetual updates // that occur with CreateOrUpdate when the API server re-adds defaults every reconcile. -// - Field ownership is derived from the owner's Kind and the component name -// (e.g., "ExampleApp/web-interface"). Forced ownership means the framework takes control -// of any conflicting fields from other managers for fields it explicitly declares. +// - Field ownership is derived from the owner's Kind, the component name and the +// owner's UID (see applyFieldOwner). Forced ownership means the framework takes +// control of any conflicting fields from other managers for fields it explicitly +// declares. func applyResources( ctx context.Context, rec ReconcileContext, entries []reconcileEntry, componentName string, mapper meta.RESTMapper, ) ([]reconcileResult, error) { - fieldOwner := client.FieldOwner( - fmt.Sprintf("%s/%s", rec.Owner.GetKind(), componentName), - ) + fieldOwner := applyFieldOwner(rec.Owner, componentName) var results []reconcileResult @@ -259,9 +289,7 @@ func reconcileResources( ctx context.Context, rec ReconcileContext, entries []reconcileEntry, componentName string, mapper meta.RESTMapper, ) ([]reconcileResult, error) { - fieldOwner := client.FieldOwner( - fmt.Sprintf("%s/%s", rec.Owner.GetKind(), componentName), - ) + fieldOwner := applyFieldOwner(rec.Owner, componentName) var results []reconcileResult diff --git a/pkg/component/create_field_manager_test.go b/pkg/component/create_field_manager_test.go new file mode 100644 index 00000000..819bbc8c --- /dev/null +++ b/pkg/component/create_field_manager_test.go @@ -0,0 +1,110 @@ +package component + +import ( + "context" + "strings" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/stretchr/testify/mock" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// Two owners of one kind whose components render the same object must not +// share a field manager. With a shared manager, each owner's forced apply +// silently relinquishes the other's fields and both report converged +// (sourcehawk/operator-component-framework#197). With a per-owner manager the +// API server sees two managers and refuses the second owner's controller +// reference, so the second owner fails instead of stealing the object. +var _ = Describe("Apply field manager", func() { + var ( + ctx = context.Background() + namespace string + ownerA *MockOperatorCRD + ownerB *MockOperatorCRD + ) + + const componentName = "shared" + + // sharedConfigMapComponent renders the ConfigMap "shared-cm" with data + // tagged by the owner that rendered it, exactly as two custom resources + // naming one config object would. + sharedConfigMapComponent := func(owner *MockOperatorCRD) *Component { + cm := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: "shared-cm", Namespace: namespace}, + Data: map[string]string{"owner": owner.Name}, + } + res := &MockResource{} + res.On("Object").Return(cm, nil) + res.On("Identity").Return("ConfigMap/shared-cm") + res.On("Mutate", mock.Anything).Return(nil) + return &Component{ + name: componentName, + conditionType: "SharedReady", + reconcileResources: []reconcileEntry{{Resource: res}}, + } + } + + BeforeEach(func() { + namespace = createNamespace(ctx, "field-manager-test-") + ownerA = &MockOperatorCRD{ObjectMeta: metav1.ObjectMeta{Name: "owner-a", Namespace: namespace}} + ownerB = &MockOperatorCRD{ObjectMeta: metav1.ObjectMeta{Name: "owner-b", Namespace: namespace}} + Expect(k8sClient.Create(ctx, ownerA)).To(Succeed()) + Expect(k8sClient.Create(ctx, ownerB)).To(Succeed()) + }) + + AfterEach(func() { + Expect(k8sClient.Delete(ctx, ownerA)).To(Succeed()) + Expect(k8sClient.Delete(ctx, ownerB)).To(Succeed()) + }) + + applyManagers := func(cm *corev1.ConfigMap) []string { + var managers []string + for _, entry := range cm.ManagedFields { + if entry.Operation == metav1.ManagedFieldsOperationApply { + managers = append(managers, entry.Manager) + } + } + return managers + } + + It("names the owner in the field manager so a second owner cannot take the object", func() { + Expect(sharedConfigMapComponent(ownerA).Reconcile(ctx, newTestReconcileContext(ownerA))).To(Succeed()) + + err := sharedConfigMapComponent(ownerB).Reconcile(ctx, newTestReconcileContext(ownerB)) + Expect(err).To(HaveOccurred(), "the second owner's apply must be refused, not silently take over") + + cm := &corev1.ConfigMap{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: "shared-cm", Namespace: namespace}, cm)).To(Succeed()) + + Expect(cm.Data).To(HaveKeyWithValue("owner", ownerA.Name)) + Expect(cm.OwnerReferences).To(HaveLen(1)) + Expect(cm.OwnerReferences[0].UID).To(Equal(ownerA.UID)) + Expect(applyManagers(cm)).To(ConsistOf("MockOperatorCRD/" + componentName + "/" + string(ownerA.UID))) + }) + + It("applies with a hashed manager when the readable name exceeds the API server limit", func() { + comp := sharedConfigMapComponent(ownerA) + comp.name = strings.Repeat("c", 128) + Expect(comp.Reconcile(ctx, newTestReconcileContext(ownerA))).To(Succeed()) + + cm := &corev1.ConfigMap{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: "shared-cm", Namespace: namespace}, cm)).To(Succeed()) + Expect(cm.Data).To(HaveKeyWithValue("owner", ownerA.Name)) + Expect(applyManagers(cm)).To(ConsistOf(string(applyFieldOwner(ownerA, comp.name)))) + Expect(applyManagers(cm)[0]).To(HaveLen(64)) + }) + + It("keeps one manager per owner across repeated reconciles", func() { + comp := sharedConfigMapComponent(ownerA) + rec := newTestReconcileContext(ownerA) + Expect(comp.Reconcile(ctx, rec)).To(Succeed()) + Expect(comp.Reconcile(ctx, rec)).To(Succeed()) + + cm := &corev1.ConfigMap{} + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: "shared-cm", Namespace: namespace}, cm)).To(Succeed()) + Expect(applyManagers(cm)).To(HaveLen(1)) + }) +}) diff --git a/pkg/component/create_test.go b/pkg/component/create_test.go index e290bd74..c46365ca 100644 --- a/pkg/component/create_test.go +++ b/pkg/component/create_test.go @@ -1,7 +1,10 @@ package component import ( + "crypto/sha256" + "encoding/hex" "fmt" + "strings" "testing" "github.com/sourcehawk/operator-component-framework/pkg/component/concepts" @@ -1093,3 +1096,47 @@ func TestReconcileResources_IgnoreIfAbsent(t *testing.T) { require.Error(t, err) }) } + +func TestApplyFieldOwner(t *testing.T) { + const ( + uid = "3d8a9d5e-1c2b-4f6e-9a7d-0b1c2d3e4f5a" + prefix = "MockOperatorCRD/" + suffix = "/" + uid + ) + newOwner := func() *MockOperatorCRD { + return &MockOperatorCRD{ObjectMeta: metav1.ObjectMeta{Name: "owner", UID: uid}} + } + + t.Run("names the kind, component and owner UID when it fits", func(t *testing.T) { + got := applyFieldOwner(newOwner(), "web-interface") + + assert.Equal(t, client.FieldOwner(prefix+"web-interface"+suffix), got) + }) + + t.Run("keeps a manager of exactly the limit readable", func(t *testing.T) { + component := strings.Repeat("c", 128-len(prefix)-len(suffix)) + + got := applyFieldOwner(newOwner(), component) + + assert.Len(t, string(got), 128) + assert.Equal(t, prefix+component+suffix, string(got)) + }) + + t.Run("hashes the whole name when it would exceed the limit", func(t *testing.T) { + component := strings.Repeat("c", 128-len(prefix)-len(suffix)+1) + sum := sha256.Sum256([]byte(prefix + component + suffix)) + + got := applyFieldOwner(newOwner(), component) + + assert.Equal(t, client.FieldOwner(hex.EncodeToString(sum[:])), got) + assert.Len(t, string(got), 64) + }) + + t.Run("hashed managers still differ per owner", func(t *testing.T) { + component := strings.Repeat("c", 100) + other := newOwner() + other.UID = "00000000-0000-0000-0000-000000000000" + + assert.NotEqual(t, applyFieldOwner(newOwner(), component), applyFieldOwner(other, component)) + }) +} diff --git a/plugin/skills/using-primitives/SKILL.md b/plugin/skills/using-primitives/SKILL.md index 4dfe3bb6..99b838d2 100644 --- a/plugin/skills/using-primitives/SKILL.md +++ b/plugin/skills/using-primitives/SKILL.md @@ -171,10 +171,16 @@ kind's exact `ExtractInto` signature with `go doc` on its package. The framework reconciles with Server-Side Apply: each primitive builds its desired state (baseline plus all active mutations) and patches it with `client.Apply`, sending only the fields the operator declares. Server-managed defaults and fields set by other controllers or webhooks are left untouched. The field manager name is derived as -`"{Owner.GetKind()}/{componentName}"`, and the framework applies with forced ownership, taking control of conflicting -fields from other managers while leaving fields it does not include with their current owners. This is what lets -primitives coexist with other controllers touching the same resource without a perpetual-update fight over stripped -server defaults. +`"{Owner.GetKind()}/{componentName}/{Owner.GetUID()}"`, and the framework applies with forced ownership, taking control +of conflicting fields from other managers while leaving fields it does not include with their current owners. This is +what lets primitives coexist with other controllers touching the same resource without a perpetual-update fight over +stripped server defaults. The owner's UID makes each owner a distinct manager: when the framework sets a controller +reference (the default), two owners of one kind rendering the same object do not silently take each other's fields; the +second owner's apply is rejected because of its second controller reference. For `Unowned()` resources, or where a scope +mismatch prevents the owner reference, the second owner's forced apply still takes the fields it declares; +`managedFields` names each owner, but the framework does not detect the contention. When the readable manager would +exceed the API server's 128-character limit, the framework uses the hex-encoded SHA-256 of it instead (64 characters, +deterministic, still distinct per owner), so long kinds or component names show up in `managedFields` as a hash. **A Go type that overstates the CRD schema breaks Apply.** The API server's field manager types the patch against the target's OpenAPI schema before merging anything, so an undeclared field fails the whole apply and the server returns: diff --git a/plugin/skills/using-primitives/references/primitives.md b/plugin/skills/using-primitives/references/primitives.md index 971bac1f..87f0e672 100644 --- a/plugin/skills/using-primitives/references/primitives.md +++ b/plugin/skills/using-primitives/references/primitives.md @@ -129,8 +129,38 @@ are sent; server-managed defaults, fields set by other controllers (HPAs, sideca and values written by webhooks are left untouched. The API server tracks field ownership automatically. The field manager name is derived from the owner and component as -`"{Owner.GetKind()}/{componentName}"`. The framework applies with forced ownership, so it takes control of conflicting -fields from other managers, while fields it does not include stay with their current owners. +`"{Owner.GetKind()}/{componentName}/{Owner.GetUID()}"`, for example +`ExampleApp/web-interface/3d8a9d5e-1c2b-4f6e-9a7d-0b1c2d3e4f5a`. The framework applies with forced ownership, so it +takes control of conflicting fields from other managers, while fields it does not include stay with their current +owners. + +The API server rejects a field manager longer than 128 characters, and neither the kind nor the component name has a +bounded length. When the readable form would exceed the limit, the manager is the hex-encoded SHA-256 of that readable +form instead: 64 characters, deterministic for the owner and component, and still distinct per owner. Such a manager +shows in `managedFields` as a hash rather than a name, so keep component names short if you rely on reading them there. + +The owner's UID is part of the manager name so that every owner is a distinct manager. Two custom resources of one kind +whose components render the same object therefore do not share a manager. When the framework sets a controller reference +on the object (the default), the second owner's apply carries a second controller reference, which the API server +rejects, so that owner's component reports an error instead of silently taking the object's fields from the first. With +a manager shared across owners, each forced apply would relinquish the other owner's fields wholesale, both would report +converged, and neither would ever see a conflict. Naming the owner also makes +`kubectl get -o yaml --show-managed-fields` say which custom resource wrote a field. + +The rejection depends on the controller reference. For a resource registered with `Unowned()`, or one whose owner +reference cannot be set because of a scope mismatch, nothing stops the second owner's forced apply from taking the +fields it declares, and the fields move between the two owners' managers on every reconcile. `managedFields` then names +the owner that wrote each field, but the framework does not detect the contention. A shared name between two owners is +the operator's responsibility in that case; a [guard](component.md#guards) that reads the live object and blocks when +another owner controls it is the way to make it explicit. + +!!! note "Upgrading from a release without the UID in the manager name" + + Releases before the UID was added used `"{Owner.GetKind()}/{componentName}"`. On the first reconcile after + upgrading, the new manager takes over every field it declares from the old one through forced ownership, so + managed objects converge without intervention. Fields the old manager owned that are no longer part of the desired + state stay in `managedFields` under the old manager name and are not pruned; they were already stale before the + upgrade. This removes the perpetual-update problem that arises when an operator strips server defaults every cycle, and it lets primitives coexist with other controllers that touch the same resources.