From db1984c7cc98c2fb0750206fcd0af4098b54ab35 Mon Sep 17 00:00:00 2001 From: Thomas Krampl Date: Fri, 2 Oct 2026 10:19:12 +0200 Subject: [PATCH 1/2] Fix omitted fields persisting when applying Kubernetes resources MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace server-side apply with create/update operations so submitted manifests define the entire desired object. Server-side apply retains omitted fields when another field manager owns them. This caused removed fields such as  spec.resources.limits.cpu  to remain on existing resources. Existing resources are now replaced using their current  resourceVersion  to detect concurrent updates. Strict field validation and before/after change reporting are preserved. This intentionally introduces replacement semantics for all supported resource types, including omitted metadata and data fields. Kubernetes defaults and validation still apply, and writes require create/update permissions. Adds regression coverage for field removal across ownership histories, full-object replacement, resource versions, error propagation, and idempotent reapplication. --- integration_tests/apply_replace.lua | 137 +++++++++ internal/apply/client.go | 39 +-- internal/apply/client_test.go | 446 ++++++++++++++++++++++++++++ 3 files changed, 599 insertions(+), 23 deletions(-) create mode 100644 integration_tests/apply_replace.lua create mode 100644 internal/apply/client_test.go diff --git a/integration_tests/apply_replace.lua b/integration_tests/apply_replace.lua new file mode 100644 index 000000000..6bbbb41c2 --- /dev/null +++ b/integration_tests/apply_replace.lua @@ -0,0 +1,137 @@ +local user = User.new() +local team = Team.new("apply-replace-team", "Full-object apply testing", "#apply-replace-team") +team:addMember(user) + +Test.rest("create resources with fields that will be omitted", function(t) + t.addHeader("x-user-email", user:email()) + t.send("POST", "/api/v1/teams/apply-replace-team/environments/dev/apply", [[ + {"resources": [ + { + "apiVersion": "nais.io/v1alpha1", + "kind": "Application", + "metadata": { + "name": "my-app", + "labels": {"kept": "same", "removed": "old"}, + "annotations": {"removed": "old"} + }, + "spec": { + "image": "example.com/my-app:v1", + "resources": { + "limits": {"cpu": "1", "memory": "512Mi"}, + "requests": {"cpu": "100m", "memory": "256Mi"} + } + } + }, + { + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": {"name": "my-config"}, + "data": {"kept": "old", "removed": "old"}, + "binaryData": {"removed": "b2xk"} + } + ]} + ]]) + t.check(200, { + results = { + { resource = "Application/my-app", environmentName = "dev", status = "created" }, + { resource = "ConfigMap/my-config", environmentName = "dev", status = "created" }, + }, + }) +end) + +local replacement = [[ + {"resources": [ + { + "apiVersion": "nais.io/v1alpha1", + "kind": "Application", + "metadata": { + "name": "my-app", + "labels": {"kept": "same"} + }, + "spec": { + "image": "example.com/my-app:v1", + "resources": { + "limits": {"memory": "512Mi"}, + "requests": {"cpu": "100m", "memory": "256Mi"} + } + } + }, + { + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": {"name": "my-config"}, + "data": {"kept": "new"} + } + ]} +]] + +Test.rest("apply replaces the entire object and reports removed fields", function(t) + t.addHeader("x-user-email", user:email()) + t.send("POST", "/api/v1/teams/apply-replace-team/environments/dev/apply", replacement) + t.check(200, { + results = { + { + resource = "Application/my-app", + environmentName = "dev", + status = "applied", + changedFields = { + { field = "metadata.annotations.removed", oldValue = "old" }, + { field = "metadata.labels.removed", oldValue = "old" }, + { field = "spec.resources.limits.cpu", oldValue = "1" }, + }, + }, + { + resource = "ConfigMap/my-config", + environmentName = "dev", + status = "applied", + changedFields = { + { field = "binaryData.removed", oldValue = "b2xk" }, + { field = "data.kept", oldValue = "old", newValue = "new" }, + { field = "data.removed", oldValue = "old" }, + }, + }, + }, + }) +end) + +Test.k8s("omitted CPU limit and metadata are removed from the stored Application", function(t) + t.check("nais.io/v1alpha1", "applications", "dev", team:slug(), "my-app", { + apiVersion = "nais.io/v1alpha1", + kind = "Application", + metadata = { + name = "my-app", + namespace = team:slug(), + labels = { kept = "same" }, + }, + spec = { + image = "example.com/my-app:v1", + resources = { + limits = { memory = "512Mi" }, + requests = { cpu = "100m", memory = "256Mi" }, + }, + }, + }) +end) + +Test.k8s("omitted data keys and top-level fields are removed from the stored ConfigMap", function(t) + t.check("v1", "configmaps", "dev", team:slug(), "my-config", { + apiVersion = "v1", + kind = "ConfigMap", + metadata = { + name = "my-config", + namespace = team:slug(), + }, + data = { kept = "new" }, + }) +end) + +Test.rest("reapplying the replacement has no changes", function(t) + t.addHeader("x-user-email", user:email()) + t.send("POST", "/api/v1/teams/apply-replace-team/environments/dev/apply", replacement) + t.check(200, { + results = { + { resource = "Application/my-app", environmentName = "dev", status = "applied" }, + { resource = "ConfigMap/my-config", environmentName = "dev", status = "applied" }, + }, + }) +end) diff --git a/internal/apply/client.go b/internal/apply/client.go index 13add645b..d48884cba 100644 --- a/internal/apply/client.go +++ b/internal/apply/client.go @@ -2,20 +2,18 @@ package apply import ( "context" - "encoding/json" "fmt" apierrors "k8s.io/apimachinery/pkg/api/errors" 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/client-go/dynamic" ) const fieldManager = "nais-api" -// ApplyResult holds the before and after states of a server-side apply operation. +// ApplyResult holds the before and after states of an apply operation. type ApplyResult struct { // Before is the state of the object before the apply. Nil if the object did not exist. Before *unstructured.Unstructured @@ -25,9 +23,9 @@ type ApplyResult struct { Created bool } -// ApplyResource performs a Kubernetes server-side apply for a single resource. -// It fetches the current state (before), applies the resource, and returns both -// before and after states so the caller can diff them. +// ApplyResource creates or replaces a Kubernetes resource with the submitted object. +// Existing resources are updated using their current resourceVersion to detect conflicts. +// It returns both before and after states so the caller can diff them. func ApplyResource( ctx context.Context, client dynamic.Interface, @@ -46,7 +44,6 @@ func ApplyResource( resourceClient := client.Resource(gvr).Namespace(namespace) - // Step 1: Get the current state of the object (before-state). before, err := resourceClient.Get(ctx, name, metav1.GetOptions{}) if err != nil { if !apierrors.IsNotFound(err) { @@ -55,24 +52,20 @@ func ApplyResource( before = nil } - // Step 2: Marshal the object to JSON for the apply patch. - data, err := json.Marshal(obj.Object) - if err != nil { - return nil, fmt.Errorf("marshaling resource to JSON: %w", err) - } - - // Step 3: Server-side apply using PATCH with ApplyPatchType. - after, err := resourceClient.Patch( - ctx, - name, - types.ApplyPatchType, - data, - metav1.PatchOptions{ + desired := obj.DeepCopy() + var after *unstructured.Unstructured + if before == nil { + after, err = resourceClient.Create(ctx, desired, metav1.CreateOptions{ FieldManager: fieldManager, - Force: new(true), FieldValidation: metav1.FieldValidationStrict, - }, - ) + }) + } else { + desired.SetResourceVersion(before.GetResourceVersion()) + after, err = resourceClient.Update(ctx, desired, metav1.UpdateOptions{ + FieldManager: fieldManager, + FieldValidation: metav1.FieldValidationStrict, + }) + } if err != nil { return nil, fmt.Errorf("applying %s/%s: %w", namespace, name, err) } diff --git a/internal/apply/client_test.go b/internal/apply/client_test.go new file mode 100644 index 000000000..61ee52740 --- /dev/null +++ b/internal/apply/client_test.go @@ -0,0 +1,446 @@ +package apply + +import ( + "encoding/json" + "errors" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/nais/api/internal/activitylog" + apierrors "k8s.io/apimachinery/pkg/api/errors" + 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/apimachinery/pkg/runtime/serializer" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/managedfields" + dynfake "k8s.io/client-go/dynamic/fake" + k8stesting "k8s.io/client-go/testing" +) + +func TestApplyResource_RemoveCPULimit(t *testing.T) { + for _, test := range []struct { + name string + previousManager string + applyBeforeRemoval bool + changeCPU bool + }{ + { + name: "removes a field owned only by nais-api", + }, + { + name: "removes a field owned by another manager", + previousManager: "previous-deployer", + }, + { + name: "removes a field shared with another manager", + previousManager: "previous-deployer", + applyBeforeRemoval: true, + }, + { + name: "removes a field after forcefully taking ownership", + previousManager: "previous-deployer", + applyBeforeRemoval: true, + changeCPU: true, + }, + } { + t.Run(test.name, func(t *testing.T) { + ctx := t.Context() + gvr := schema.GroupVersionResource{Group: "nais.io", Version: "v1alpha1", Resource: "applications"} + client := newFieldManagedDynamicClient(gvr.GroupVersion().WithKind("Application")) + resourceClient := client.Resource(gvr).Namespace("my-team") + application := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "nais.io/v1alpha1", + "kind": "Application", + "metadata": map[string]any{ + "name": "my-app", + "namespace": "my-team", + }, + "spec": map[string]any{ + "image": "example.com/my-app:v1", + "resources": map[string]any{ + "limits": map[string]any{ + "cpu": "1", + "memory": "512Mi", + }, + "requests": map[string]any{ + "cpu": "100m", + "memory": "256Mi", + }, + }, + }, + }} + + if test.previousManager == "" { + result, err := ApplyResource(ctx, client, gvr, application) + if err != nil { + t.Fatal(err) + } + if !result.Created || result.Before != nil { + t.Fatal("expected application to be created without a before-state") + } + if diff := cmp.Diff(application.Object["spec"], result.After.Object["spec"]); diff != "" { + t.Fatalf("created spec mismatch (-want +got):\n%s", diff) + } + } else { + data, err := json.Marshal(application.Object) + if err != nil { + t.Fatal(err) + } + _, err = resourceClient.Patch(ctx, application.GetName(), types.ApplyPatchType, data, metav1.PatchOptions{ + FieldManager: test.previousManager, + }) + if err != nil { + t.Fatal(err) + } + } + + oldCPU := "1" + if test.applyBeforeRemoval { + if test.changeCPU { + oldCPU = "2" + if err := unstructured.SetNestedField(application.Object, oldCPU, "spec", "resources", "limits", "cpu"); err != nil { + t.Fatal(err) + } + } + data, err := json.Marshal(application.Object) + if err != nil { + t.Fatal(err) + } + applied, err := resourceClient.Patch(ctx, application.GetName(), types.ApplyPatchType, data, metav1.PatchOptions{ + FieldManager: fieldManager, + Force: new(true), + }) + if err != nil { + t.Fatal(err) + } + if diff := cmp.Diff(application.Object["spec"], applied.Object["spec"]); diff != "" { + t.Fatalf("applied spec mismatch (-want +got):\n%s", diff) + } + } + + desired := application.DeepCopy() + unstructured.RemoveNestedField(desired.Object, "spec", "resources", "limits", "cpu") + + result, err := ApplyResource(ctx, client, gvr, desired) + if err != nil { + t.Fatal(err) + } + if result.Created || result.Before == nil { + t.Fatal("expected existing application to be applied with a before-state") + } + beforeCPU, found, err := unstructured.NestedString(result.Before.Object, "spec", "resources", "limits", "cpu") + if err != nil { + t.Fatal(err) + } + if !found || beforeCPU != oldCPU { + t.Fatalf("CPU limit before apply = %q (found: %t), want %q", beforeCPU, found, oldCPU) + } + afterCPU, found, err := unstructured.NestedString(result.After.Object, "spec", "resources", "limits", "cpu") + if err != nil { + t.Fatal(err) + } + if found { + t.Fatalf("spec.resources.limits.cpu is still present after apply: %q", afterCPU) + } + if diff := cmp.Diff(desired.Object["spec"], result.After.Object["spec"]); diff != "" { + t.Errorf("applied spec mismatch (-want +got):\n%s", diff) + } + + changes := Diff(result.Before, result.After) + wantChanges := []activitylog.ResourceChangedField{{ + Field: "spec.resources.limits.cpu", + OldValue: &oldCPU, + }} + if diff := cmp.Diff(wantChanges, changes); diff != "" { + t.Errorf("changed fields mismatch (-want +got):\n%s", diff) + } + + stored, err := resourceClient.Get(ctx, application.GetName(), metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + if diff := cmp.Diff(result.After.Object, stored.Object); diff != "" { + t.Errorf("stored application mismatch (-want +got):\n%s", diff) + } + + reapplied, err := ApplyResource(ctx, client, gvr, desired) + if err != nil { + t.Fatal(err) + } + if reapplied.Created { + t.Error("expected existing application to be reapplied, not created") + } + if diff := cmp.Diff(desired.Object["spec"], reapplied.After.Object["spec"]); diff != "" { + t.Errorf("reapplied spec mismatch (-want +got):\n%s", diff) + } + if changes := Diff(reapplied.Before, reapplied.After); len(changes) != 0 { + t.Errorf("expected no changes when reapplying, got %+v", changes) + } + }) + } +} + +func TestApplyResource_ReplacesEntireObject(t *testing.T) { + for _, test := range []struct { + kind string + gvr schema.GroupVersionResource + beforeFields map[string]any + desiredFields map[string]any + }{ + { + kind: "Application", + gvr: schema.GroupVersionResource{Group: "nais.io", Version: "v1alpha1", Resource: "applications"}, + beforeFields: map[string]any{ + "spec": map[string]any{ + "image": "example.com/my-app:v1", + "resources": map[string]any{ + "limits": map[string]any{"cpu": "1"}, + }, + }, + }, + desiredFields: map[string]any{ + "spec": map[string]any{"image": "example.com/my-app:v2"}, + }, + }, + { + kind: "ConfigMap", + gvr: schema.GroupVersionResource{Version: "v1", Resource: "configmaps"}, + beforeFields: map[string]any{ + "data": map[string]any{"kept": "old", "removed": "old"}, + "binaryData": map[string]any{"removed": "b2xk"}, + }, + desiredFields: map[string]any{ + "data": map[string]any{"kept": "new"}, + }, + }, + { + kind: "Secret", + gvr: schema.GroupVersionResource{Version: "v1", Resource: "secrets"}, + beforeFields: map[string]any{ + "data": map[string]any{"kept": "b2xk", "removed": "b2xk"}, + "type": "Opaque", + }, + desiredFields: map[string]any{ + "data": map[string]any{"kept": "bmV3"}, + }, + }, + } { + t.Run(test.kind, func(t *testing.T) { + ctx := t.Context() + gvk := test.gvr.GroupVersion().WithKind(test.kind) + client := newFieldManagedDynamicClient(gvk) + resourceClient := client.Resource(test.gvr).Namespace("my-team") + existing := (&unstructured.Unstructured{Object: test.beforeFields}).DeepCopy() + existing.SetGroupVersionKind(gvk) + existing.SetName("my-resource") + existing.SetNamespace("my-team") + existing.SetLabels(map[string]string{"kept": "same", "removed": "old"}) + existing.SetAnnotations(map[string]string{"removed": "old"}) + + data, err := json.Marshal(existing.Object) + if err != nil { + t.Fatal(err) + } + _, err = resourceClient.Patch(ctx, existing.GetName(), types.ApplyPatchType, data, metav1.PatchOptions{ + FieldManager: "previous-deployer", + }) + if err != nil { + t.Fatal(err) + } + + desired := (&unstructured.Unstructured{Object: test.desiredFields}).DeepCopy() + desired.SetGroupVersionKind(gvk) + desired.SetName(existing.GetName()) + desired.SetNamespace(existing.GetNamespace()) + desired.SetLabels(map[string]string{"kept": "same", "added": "new"}) + input := desired.DeepCopy() + client.ClearActions() + + result, err := ApplyResource(ctx, client, test.gvr, desired) + if err != nil { + t.Fatal(err) + } + if result.Created || result.Before == nil { + t.Fatal("expected replacement of an existing resource") + } + if changes := Diff(existing, result.Before); len(changes) != 0 { + t.Fatalf("before-state differs from existing resource: %+v", changes) + } + + actual := result.After.DeepCopy() + actual.SetManagedFields(nil) + actual.SetResourceVersion("") + if diff := cmp.Diff(input.Object, actual.Object); diff != "" { + t.Errorf("replacement mismatch (-want +got):\n%s", diff) + } + if diff := cmp.Diff(input.Object, desired.Object); diff != "" { + t.Errorf("input was mutated (-want +got):\n%s", diff) + } + + actions := client.Actions() + if len(actions) != 2 || actions[0].GetVerb() != "get" || actions[1].GetVerb() != "update" { + t.Fatalf("expected GET followed by UPDATE, got %v", actions) + } + update, ok := actions[1].(k8stesting.UpdateActionImpl) + if !ok { + t.Fatalf("expected UpdateActionImpl, got %T", actions[1]) + } + options := update.GetUpdateOptions() + if options.FieldManager != fieldManager || options.FieldValidation != metav1.FieldValidationStrict { + t.Errorf("unexpected update options: %+v", options) + } + stored, err := resourceClient.Get(ctx, desired.GetName(), metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + if diff := cmp.Diff(result.After.Object, stored.Object); diff != "" { + t.Errorf("stored replacement mismatch (-want +got):\n%s", diff) + } + }) + } +} + +func TestApplyResource_UsesCurrentResourceVersion(t *testing.T) { + gvr := schema.GroupVersionResource{Group: "nais.io", Version: "v1alpha1", Resource: "applications"} + client := dynfake.NewSimpleDynamicClient(runtime.NewScheme()) + existing := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "nais.io/v1alpha1", + "kind": "Application", + "metadata": map[string]any{ + "name": "my-app", + "namespace": "my-team", + "resourceVersion": "42", + }, + "spec": map[string]any{"image": "example.com/my-app:v1"}, + }} + desired := existing.DeepCopy() + desired.SetResourceVersion("") + if err := unstructured.SetNestedField(desired.Object, "example.com/my-app:v2", "spec", "image"); err != nil { + t.Fatal(err) + } + input := desired.DeepCopy() + + client.PrependReactor("get", "*", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, existing.DeepCopy(), nil + }) + client.PrependReactor("update", "*", func(action k8stesting.Action) (bool, runtime.Object, error) { + update, ok := action.(k8stesting.UpdateAction) + if !ok { + t.Fatalf("expected UpdateAction, got %T", action) + } + object, ok := update.GetObject().(*unstructured.Unstructured) + if !ok { + t.Fatalf("expected unstructured object, got %T", update.GetObject()) + } + expected := desired.DeepCopy() + expected.SetResourceVersion("42") + if diff := cmp.Diff(expected.Object, object.Object); diff != "" { + t.Errorf("update mismatch (-want +got):\n%s", diff) + } + return true, object.DeepCopy(), nil + }) + + result, err := ApplyResource(t.Context(), client, gvr, desired) + if err != nil { + t.Fatal(err) + } + if result.Created { + t.Error("expected an update, not a creation") + } + if diff := cmp.Diff(existing.Object, result.Before.Object); diff != "" { + t.Errorf("before-state mismatch (-want +got):\n%s", diff) + } + if result.After.GetResourceVersion() != "42" { + t.Errorf("resourceVersion = %q, want 42", result.After.GetResourceVersion()) + } + if diff := cmp.Diff(input.Object, desired.Object); diff != "" { + t.Errorf("input was mutated (-want +got):\n%s", diff) + } +} + +func TestApplyResource_KubernetesErrors(t *testing.T) { + gvr := schema.GroupVersionResource{Group: "nais.io", Version: "v1alpha1", Resource: "applications"} + for _, test := range []struct { + name string + verb string + err error + }{ + { + name: "get forbidden", + verb: "get", + err: apierrors.NewForbidden(gvr.GroupResource(), "my-app", errors.New("denied")), + }, + { + name: "create rejected", + verb: "create", + err: apierrors.NewBadRequest("invalid resource"), + }, + { + name: "created concurrently", + verb: "create", + err: apierrors.NewAlreadyExists(gvr.GroupResource(), "my-app"), + }, + { + name: "update rejected", + verb: "update", + err: apierrors.NewBadRequest("immutable field changed"), + }, + { + name: "updated concurrently", + verb: "update", + err: apierrors.NewConflict(gvr.GroupResource(), "my-app", errors.New("resourceVersion changed")), + }, + } { + t.Run(test.name, func(t *testing.T) { + client := dynfake.NewSimpleDynamicClient(runtime.NewScheme()) + desired := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "nais.io/v1alpha1", + "kind": "Application", + "metadata": map[string]any{ + "name": "my-app", + "namespace": "my-team", + }, + }} + if test.verb == "update" { + existing := desired.DeepCopy() + existing.SetResourceVersion("42") + client.PrependReactor("get", "*", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, existing.DeepCopy(), nil + }) + } + client.PrependReactor(test.verb, "*", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, test.err + }) + + result, err := ApplyResource(t.Context(), client, gvr, desired) + if !errors.Is(err, test.err) { + t.Fatalf("error = %v, want wrapped %v", err, test.err) + } + if result != nil { + t.Errorf("expected no result on failure, got %+v", result) + } + wantActions := 2 + if test.verb == "get" { + wantActions = 1 + } + if actions := client.Actions(); len(actions) != wantActions || actions[len(actions)-1].GetVerb() != test.verb { + t.Errorf("unexpected actions after failure: %v", actions) + } + }) + } +} + +func newFieldManagedDynamicClient(gvk schema.GroupVersionKind) *dynfake.FakeDynamicClient { + scheme := runtime.NewScheme() + scheme.AddKnownTypeWithName(gvk, &unstructured.Unstructured{}) + scheme.AddKnownTypeWithName(gvk.GroupVersion().WithKind(gvk.Kind+"List"), &unstructured.UnstructuredList{}) + + // The integration fake replaces spec wholesale, masking field ownership. + // Use Kubernetes' field manager to include existing field ownership. + tracker := k8stesting.NewFieldManagedObjectTracker(scheme, serializer.NewCodecFactory(scheme).UniversalDecoder(), managedfields.NewDeducedTypeConverter()) + client := dynfake.NewSimpleDynamicClient(scheme) + client.PrependReactor("*", "*", k8stesting.ObjectReaction(tracker)) + return client +} From b73ee784cb9d28be240d4ca3d146a6bae815e751 Mon Sep 17 00:00:00 2001 From: Thomas Krampl Date: Tue, 6 Oct 2026 13:53:07 +0200 Subject: [PATCH 2/2] Migrate legacy field ownership before server-side apply Transfer Update ownership from kubectl-create, kubectl-client-side-apply, and deployd to nais-api so omitted fields can be removed while preserving other managers' ownership. Reject foreign-field conflicts and unsafe cross-version migrations, and retain caller resourceVersion preconditions across migration. Make the Kubernetes fake ownership-aware and cover omission, migration, controller ownership, and concurrency regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- integration_tests/apply.lua | 2 + integration_tests/apply_replace.lua | 6 +- integration_tests/apply_secret.lua | 1 + internal/apply/client.go | 91 +++- internal/apply/client_test.go | 592 +++++++++++++++---------- internal/apply/migration_test.go | 276 ++++++++++++ internal/kubernetes/fake/apply_test.go | 181 ++++++++ internal/kubernetes/fake/fake.go | 67 +-- 8 files changed, 941 insertions(+), 275 deletions(-) create mode 100644 internal/apply/migration_test.go create mode 100644 internal/kubernetes/fake/apply_test.go diff --git a/integration_tests/apply.lua b/integration_tests/apply.lua index 99a1d2603..720c30fb4 100644 --- a/integration_tests/apply.lua +++ b/integration_tests/apply.lua @@ -47,6 +47,7 @@ Test.k8s("verify resource was created in fake environment", function(t) metadata = { name = "my-app", namespace = "apply-team", + managedFields = Ignore(), }, spec = { image = "example.com/my-app:v1", @@ -118,6 +119,7 @@ Test.k8s("verify resource was updated in fake environment", function(t) metadata = { name = "my-app", namespace = "apply-team", + managedFields = Ignore(), }, spec = { image = "example.com/my-app:v2", diff --git a/integration_tests/apply_replace.lua b/integration_tests/apply_replace.lua index 6bbbb41c2..666bfd0e4 100644 --- a/integration_tests/apply_replace.lua +++ b/integration_tests/apply_replace.lua @@ -1,5 +1,5 @@ local user = User.new() -local team = Team.new("apply-replace-team", "Full-object apply testing", "#apply-replace-team") +local team = Team.new("apply-replace-team", "Apply field removal testing", "#apply-replace-team") team:addMember(user) Test.rest("create resources with fields that will be omitted", function(t) @@ -65,7 +65,7 @@ local replacement = [[ ]} ]] -Test.rest("apply replaces the entire object and reports removed fields", function(t) +Test.rest("apply removes omitted owned fields and reports changes", function(t) t.addHeader("x-user-email", user:email()) t.send("POST", "/api/v1/teams/apply-replace-team/environments/dev/apply", replacement) t.check(200, { @@ -101,6 +101,7 @@ Test.k8s("omitted CPU limit and metadata are removed from the stored Application metadata = { name = "my-app", namespace = team:slug(), + managedFields = Ignore(), labels = { kept = "same" }, }, spec = { @@ -120,6 +121,7 @@ Test.k8s("omitted data keys and top-level fields are removed from the stored Con metadata = { name = "my-config", namespace = team:slug(), + managedFields = Ignore(), }, data = { kept = "new" }, }) diff --git a/integration_tests/apply_secret.lua b/integration_tests/apply_secret.lua index ee230e607..54a66fd01 100644 --- a/integration_tests/apply_secret.lua +++ b/integration_tests/apply_secret.lua @@ -59,6 +59,7 @@ Test.k8s("Secret apply still updates the Kubernetes object", function(t) metadata = { name = "apply-secret", namespace = team:slug(), + managedFields = Ignore(), annotations = { snapshot = "NEW_SNAPSHOT" }, }, }) diff --git a/internal/apply/client.go b/internal/apply/client.go index d48884cba..141f66a1e 100644 --- a/internal/apply/client.go +++ b/internal/apply/client.go @@ -2,13 +2,17 @@ package apply import ( "context" + "encoding/json" "fmt" apierrors "k8s.io/apimachinery/pkg/api/errors" 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/sets" "k8s.io/client-go/dynamic" + "k8s.io/client-go/util/csaupgrade" ) const fieldManager = "nais-api" @@ -23,8 +27,8 @@ type ApplyResult struct { Created bool } -// ApplyResource creates or replaces a Kubernetes resource with the submitted object. -// Existing resources are updated using their current resourceVersion to detect conflicts. +// ApplyResource uses server-side apply after migrating ownership from legacy deployers. +// Other field managers retain their ownership, and conflicting changes are rejected. // It returns both before and after states so the caller can diff them. func ApplyResource( ctx context.Context, @@ -52,20 +56,56 @@ func ApplyResource( before = nil } + submittedVersion := obj.GetResourceVersion() + if submittedVersion != "" { + if before == nil { + return nil, apierrors.NewConflict(gvr.GroupResource(), name, fmt.Errorf("resourceVersion %q refers to a resource that no longer exists", submittedVersion)) + } + if submittedVersion != before.GetResourceVersion() { + return nil, apierrors.NewConflict(gvr.GroupResource(), name, fmt.Errorf("requested resourceVersion %q does not match current version %q", submittedVersion, before.GetResourceVersion())) + } + } desired := obj.DeepCopy() - var after *unstructured.Unstructured - if before == nil { - after, err = resourceClient.Create(ctx, desired, metav1.CreateOptions{ - FieldManager: fieldManager, - FieldValidation: metav1.FieldValidationStrict, - }) - } else { - desired.SetResourceVersion(before.GetResourceVersion()) - after, err = resourceClient.Update(ctx, desired, metav1.UpdateOptions{ - FieldManager: fieldManager, - FieldValidation: metav1.FieldValidationStrict, - }) + if before != nil { + legacyManagers := sets.New("kubectl-create", "kubectl-client-side-apply", "deployd") + if err := validateMigrationVersions(before.GetManagedFields(), legacyManagers); err != nil { + return nil, fmt.Errorf("preparing ownership migration for %s/%s: %w", namespace, name, err) + } + // Transfer legacy Update ownership to SSA so omitted fields can be removed + // without taking ownership from other managers. + patch, err := csaupgrade.UpgradeManagedFieldsPatch( + before, + legacyManagers, + fieldManager, + ) + if err != nil { + return nil, fmt.Errorf("preparing ownership migration for %s/%s: %w", namespace, name, err) + } + if patch != nil { + migrated, err := resourceClient.Patch(ctx, name, types.JSONPatchType, patch, metav1.PatchOptions{ + FieldManager: fieldManager, + }) + if err != nil { + return nil, fmt.Errorf("migrating field ownership for %s/%s: %w", namespace, name, err) + } + if submittedVersion != "" { + if migrated == nil || migrated.GetResourceVersion() == "" { + return nil, fmt.Errorf("ownership migration for %s/%s returned no resourceVersion", namespace, name) + } + desired.SetResourceVersion(migrated.GetResourceVersion()) + } + } + } + + data, err := json.Marshal(desired.Object) + if err != nil { + return nil, fmt.Errorf("marshaling resource to JSON: %w", err) } + after, err := resourceClient.Patch(ctx, name, types.ApplyPatchType, data, metav1.PatchOptions{ + FieldManager: fieldManager, + Force: new(false), + FieldValidation: metav1.FieldValidationStrict, + }) if err != nil { return nil, fmt.Errorf("applying %s/%s: %w", namespace, name, err) } @@ -76,3 +116,26 @@ func ApplyResource( Created: before == nil, }, nil } + +func validateMigrationVersions(entries []metav1.ManagedFieldsEntry, legacyManagers sets.Set[string]) error { + var targetVersion string + for _, entry := range entries { + if entry.Manager == fieldManager && entry.Operation == metav1.ManagedFieldsOperationApply && entry.Subresource == "" { + targetVersion = entry.APIVersion + break + } + } + // csaupgrade drops legacy entries in other API versions without merging them. + for _, entry := range entries { + if !legacyManagers.Has(entry.Manager) || entry.Operation != metav1.ManagedFieldsOperationUpdate || entry.Subresource != "" { + continue + } + if targetVersion == "" { + targetVersion = entry.APIVersion + } + if entry.APIVersion != targetVersion { + return fmt.Errorf("cannot safely migrate %q ownership from API version %q into %q ownership for API version %q", entry.Manager, entry.APIVersion, fieldManager, targetVersion) + } + } + return nil +} diff --git a/internal/apply/client_test.go b/internal/apply/client_test.go index 61ee52740..742ead73c 100644 --- a/internal/apply/client_test.go +++ b/internal/apply/client_test.go @@ -15,6 +15,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/serializer" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/managedfields" + "k8s.io/client-go/dynamic" dynfake "k8s.io/client-go/dynamic/fake" k8stesting "k8s.io/client-go/testing" ) @@ -23,140 +24,110 @@ func TestApplyResource_RemoveCPULimit(t *testing.T) { for _, test := range []struct { name string previousManager string + previousApply bool applyBeforeRemoval bool - changeCPU bool + controllerOwnsCPU bool + wantCPU bool }{ - { - name: "removes a field owned only by nais-api", - }, - { - name: "removes a field owned by another manager", - previousManager: "previous-deployer", - }, - { - name: "removes a field shared with another manager", - previousManager: "previous-deployer", - applyBeforeRemoval: true, - }, - { - name: "removes a field after forcefully taking ownership", - previousManager: "previous-deployer", - applyBeforeRemoval: true, - changeCPU: true, - }, + {name: "owned only by nais-api"}, + {name: "created using kubectl create", previousManager: "kubectl-create"}, + {name: "managed by client-side kubectl apply", previousManager: "kubectl-client-side-apply"}, + {name: "managed by deployd", previousManager: "deployd"}, + {name: "unchanged kubectl-create ownership is migrated", previousManager: "kubectl-create", applyBeforeRemoval: true}, + {name: "unchanged kubectl apply ownership is migrated", previousManager: "kubectl-client-side-apply", applyBeforeRemoval: true}, + {name: "unchanged deployd ownership is migrated", previousManager: "deployd", applyBeforeRemoval: true}, + {name: "controller-owned field is retained", previousManager: "cpu-controller", wantCPU: true}, + {name: "field shared with a controller is retained", previousManager: "cpu-controller", applyBeforeRemoval: true, wantCPU: true}, + {name: "migrated field shared with a controller is retained", previousManager: "deployd", controllerOwnsCPU: true, wantCPU: true}, + {name: "legacy Apply entries are not migrated", previousManager: "deployd", previousApply: true, wantCPU: true}, + {name: "server-side kubectl ownership is retained", previousManager: "kubectl", previousApply: true, wantCPU: true}, } { t.Run(test.name, func(t *testing.T) { ctx := t.Context() gvr := schema.GroupVersionResource{Group: "nais.io", Version: "v1alpha1", Resource: "applications"} client := newFieldManagedDynamicClient(gvr.GroupVersion().WithKind("Application")) resourceClient := client.Resource(gvr).Namespace("my-team") - application := &unstructured.Unstructured{Object: map[string]any{ - "apiVersion": "nais.io/v1alpha1", - "kind": "Application", - "metadata": map[string]any{ - "name": "my-app", - "namespace": "my-team", - }, - "spec": map[string]any{ - "image": "example.com/my-app:v1", - "resources": map[string]any{ - "limits": map[string]any{ - "cpu": "1", - "memory": "512Mi", - }, - "requests": map[string]any{ - "cpu": "100m", - "memory": "256Mi", - }, - }, - }, - }} - + application := applyTestApplication() if test.previousManager == "" { result, err := ApplyResource(ctx, client, gvr, application) if err != nil { t.Fatal(err) } if !result.Created || result.Before != nil { - t.Fatal("expected application to be created without a before-state") - } - if diff := cmp.Diff(application.Object["spec"], result.After.Object["spec"]); diff != "" { - t.Fatalf("created spec mismatch (-want +got):\n%s", diff) + t.Fatal("expected creation without a before-state") } } else { - data, err := json.Marshal(application.Object) - if err != nil { - t.Fatal(err) - } - _, err = resourceClient.Patch(ctx, application.GetName(), types.ApplyPatchType, data, metav1.PatchOptions{ + snapshot := application.DeepCopy() + // The tracker does not assign resourceVersions like a real API server. + snapshot.SetResourceVersion("1") + if test.previousApply { + applyAsManager(t, resourceClient, snapshot, test.previousManager) + } else if _, err := resourceClient.Create(ctx, snapshot, metav1.CreateOptions{ FieldManager: test.previousManager, - }) - if err != nil { + }); err != nil { t.Fatal(err) } } - - oldCPU := "1" - if test.applyBeforeRemoval { - if test.changeCPU { - oldCPU = "2" - if err := unstructured.SetNestedField(application.Object, oldCPU, "spec", "resources", "limits", "cpu"); err != nil { - t.Fatal(err) - } + if test.controllerOwnsCPU { + controllerIntent := application.DeepCopy() + controllerIntent.Object["spec"] = map[string]any{ + "resources": map[string]any{"limits": map[string]any{"cpu": "1"}}, } - data, err := json.Marshal(application.Object) + applyAsManager(t, resourceClient, controllerIntent, "cpu-controller") + } + if test.applyBeforeRemoval { + result, err := ApplyResource(ctx, client, gvr, application) if err != nil { t.Fatal(err) } - applied, err := resourceClient.Patch(ctx, application.GetName(), types.ApplyPatchType, data, metav1.PatchOptions{ - FieldManager: fieldManager, - Force: new(true), - }) - if err != nil { - t.Fatal(err) + if result.Created || len(Diff(result.Before, result.After)) != 0 { + t.Fatal("expected unchanged application to be applied without configuration changes") } - if diff := cmp.Diff(application.Object["spec"], applied.Object["spec"]); diff != "" { - t.Fatalf("applied spec mismatch (-want +got):\n%s", diff) + if !test.wantCPU { + for _, entry := range result.After.GetManagedFields() { + if entry.Manager == test.previousManager { + t.Fatalf("legacy ownership was not transferred: %+v", entry) + } + } } } - desired := application.DeepCopy() unstructured.RemoveNestedField(desired.Object, "spec", "resources", "limits", "cpu") + input := desired.DeepCopy() result, err := ApplyResource(ctx, client, gvr, desired) if err != nil { t.Fatal(err) } if result.Created || result.Before == nil { - t.Fatal("expected existing application to be applied with a before-state") + t.Fatal("expected an existing application with a before-state") } beforeCPU, found, err := unstructured.NestedString(result.Before.Object, "spec", "resources", "limits", "cpu") - if err != nil { - t.Fatal(err) + if err != nil || !found || beforeCPU != "1" { + t.Fatalf("CPU before apply = %q (found=%t), err=%v", beforeCPU, found, err) } - if !found || beforeCPU != oldCPU { - t.Fatalf("CPU limit before apply = %q (found: %t), want %q", beforeCPU, found, oldCPU) - } - afterCPU, found, err := unstructured.NestedString(result.After.Object, "spec", "resources", "limits", "cpu") - if err != nil { - t.Fatal(err) + expected := desired + if test.wantCPU { + expected = application } - if found { - t.Fatalf("spec.resources.limits.cpu is still present after apply: %q", afterCPU) - } - if diff := cmp.Diff(desired.Object["spec"], result.After.Object["spec"]); diff != "" { + if diff := cmp.Diff(expected.Object["spec"], result.After.Object["spec"]); diff != "" { t.Errorf("applied spec mismatch (-want +got):\n%s", diff) } - changes := Diff(result.Before, result.After) - wantChanges := []activitylog.ResourceChangedField{{ - Field: "spec.resources.limits.cpu", - OldValue: &oldCPU, - }} - if diff := cmp.Diff(wantChanges, changes); diff != "" { - t.Errorf("changed fields mismatch (-want +got):\n%s", diff) + if test.wantCPU { + if len(changes) != 0 { + t.Errorf("controller-owned field changed: %+v", changes) + } + } else { + oldCPU := "1" + wantChanges := []activitylog.ResourceChangedField{{Field: "spec.resources.limits.cpu", OldValue: &oldCPU}} + if diff := cmp.Diff(wantChanges, changes); diff != "" { + t.Errorf("changed fields mismatch (-want +got):\n%s", diff) + } + } + if diff := cmp.Diff(input.Object, desired.Object); diff != "" { + t.Errorf("input was mutated (-want +got):\n%s", diff) } - stored, err := resourceClient.Get(ctx, application.GetName(), metav1.GetOptions{}) if err != nil { t.Fatal(err) @@ -164,25 +135,21 @@ func TestApplyResource_RemoveCPULimit(t *testing.T) { if diff := cmp.Diff(result.After.Object, stored.Object); diff != "" { t.Errorf("stored application mismatch (-want +got):\n%s", diff) } - reapplied, err := ApplyResource(ctx, client, gvr, desired) if err != nil { t.Fatal(err) } - if reapplied.Created { - t.Error("expected existing application to be reapplied, not created") + if reapplied.Created || len(Diff(reapplied.Before, reapplied.After)) != 0 { + t.Error("expected reapplication without configuration changes") } - if diff := cmp.Diff(desired.Object["spec"], reapplied.After.Object["spec"]); diff != "" { + if diff := cmp.Diff(expected.Object["spec"], reapplied.After.Object["spec"]); diff != "" { t.Errorf("reapplied spec mismatch (-want +got):\n%s", diff) } - if changes := Diff(reapplied.Before, reapplied.After); len(changes) != 0 { - t.Errorf("expected no changes when reapplying, got %+v", changes) - } }) } } -func TestApplyResource_ReplacesEntireObject(t *testing.T) { +func TestApplyResource_RemovesOmittedLegacyFields(t *testing.T) { for _, test := range []struct { kind string gvr schema.GroupVersionResource @@ -193,16 +160,9 @@ func TestApplyResource_ReplacesEntireObject(t *testing.T) { kind: "Application", gvr: schema.GroupVersionResource{Group: "nais.io", Version: "v1alpha1", Resource: "applications"}, beforeFields: map[string]any{ - "spec": map[string]any{ - "image": "example.com/my-app:v1", - "resources": map[string]any{ - "limits": map[string]any{"cpu": "1"}, - }, - }, - }, - desiredFields: map[string]any{ - "spec": map[string]any{"image": "example.com/my-app:v2"}, + "spec": map[string]any{"image": "example.com/my-app:v1", "resources": map[string]any{"limits": map[string]any{"cpu": "1"}}}, }, + desiredFields: map[string]any{"spec": map[string]any{"image": "example.com/my-app:v2"}}, }, { kind: "ConfigMap", @@ -211,20 +171,13 @@ func TestApplyResource_ReplacesEntireObject(t *testing.T) { "data": map[string]any{"kept": "old", "removed": "old"}, "binaryData": map[string]any{"removed": "b2xk"}, }, - desiredFields: map[string]any{ - "data": map[string]any{"kept": "new"}, - }, + desiredFields: map[string]any{"data": map[string]any{"kept": "new"}}, }, { - kind: "Secret", - gvr: schema.GroupVersionResource{Version: "v1", Resource: "secrets"}, - beforeFields: map[string]any{ - "data": map[string]any{"kept": "b2xk", "removed": "b2xk"}, - "type": "Opaque", - }, - desiredFields: map[string]any{ - "data": map[string]any{"kept": "bmV3"}, - }, + kind: "Secret", + gvr: schema.GroupVersionResource{Version: "v1", Resource: "secrets"}, + beforeFields: map[string]any{"data": map[string]any{"kept": "b2xk", "removed": "b2xk"}}, + desiredFields: map[string]any{"data": map[string]any{"kept": "bmV3"}}, }, } { t.Run(test.kind, func(t *testing.T) { @@ -236,20 +189,12 @@ func TestApplyResource_ReplacesEntireObject(t *testing.T) { existing.SetGroupVersionKind(gvk) existing.SetName("my-resource") existing.SetNamespace("my-team") + existing.SetResourceVersion("42") existing.SetLabels(map[string]string{"kept": "same", "removed": "old"}) existing.SetAnnotations(map[string]string{"removed": "old"}) - - data, err := json.Marshal(existing.Object) - if err != nil { + if _, err := resourceClient.Create(ctx, existing, metav1.CreateOptions{FieldManager: "deployd"}); err != nil { t.Fatal(err) } - _, err = resourceClient.Patch(ctx, existing.GetName(), types.ApplyPatchType, data, metav1.PatchOptions{ - FieldManager: "previous-deployer", - }) - if err != nil { - t.Fatal(err) - } - desired := (&unstructured.Unstructured{Object: test.desiredFields}).DeepCopy() desired.SetGroupVersionKind(gvk) desired.SetName(existing.GetName()) @@ -263,157 +208,314 @@ func TestApplyResource_ReplacesEntireObject(t *testing.T) { t.Fatal(err) } if result.Created || result.Before == nil { - t.Fatal("expected replacement of an existing resource") + t.Fatal("expected an existing resource") } if changes := Diff(existing, result.Before); len(changes) != 0 { t.Fatalf("before-state differs from existing resource: %+v", changes) } - actual := result.After.DeepCopy() actual.SetManagedFields(nil) actual.SetResourceVersion("") if diff := cmp.Diff(input.Object, actual.Object); diff != "" { - t.Errorf("replacement mismatch (-want +got):\n%s", diff) + t.Errorf("applied object mismatch (-want +got):\n%s", diff) } if diff := cmp.Diff(input.Object, desired.Object); diff != "" { t.Errorf("input was mutated (-want +got):\n%s", diff) } - actions := client.Actions() - if len(actions) != 2 || actions[0].GetVerb() != "get" || actions[1].GetVerb() != "update" { - t.Fatalf("expected GET followed by UPDATE, got %v", actions) + if len(actions) != 3 || actions[0].GetVerb() != "get" { + t.Fatalf("expected GET, migration PATCH, and SSA PATCH, got %v", actions) } - update, ok := actions[1].(k8stesting.UpdateActionImpl) - if !ok { - t.Fatalf("expected UpdateActionImpl, got %T", actions[1]) + for i, patchType := range []types.PatchType{types.JSONPatchType, types.ApplyPatchType} { + action, ok := actions[i+1].(k8stesting.PatchActionImpl) + if !ok || action.GetPatchType() != patchType { + t.Fatalf("action %d is not %s: %v", i+1, patchType, actions[i+1]) + } + options := action.GetPatchOptions() + if options.FieldManager != fieldManager { + t.Errorf("unexpected field manager: %q", options.FieldManager) + } + if patchType == types.ApplyPatchType && + (options.Force == nil || *options.Force || options.FieldValidation != metav1.FieldValidationStrict) { + t.Errorf("unexpected SSA options: %+v", options) + } + } + }) + } +} + +func TestApplyResource_PreservesControllerFields(t *testing.T) { + gvr := schema.GroupVersionResource{Group: "nais.io", Version: "v1alpha1", Resource: "applications"} + client := newFieldManagedDynamicClient(gvr.GroupVersion().WithKind("Application")) + resourceClient := client.Resource(gvr).Namespace("my-team") + application := applyTestApplication() + snapshot := application.DeepCopy() + snapshot.SetResourceVersion("1") + if _, err := resourceClient.Create(t.Context(), snapshot, metav1.CreateOptions{FieldManager: "deployd"}); err != nil { + t.Fatal(err) + } + controllerIntent := application.DeepCopy() + delete(controllerIntent.Object, "spec") + controllerIntent.SetAnnotations(map[string]string{"controller": "kept"}) + controllerIntent.SetLabels(map[string]string{"controller": "kept"}) + controllerIntent.SetFinalizers([]string{"controller.example.com/cleanup"}) + controllerIntent.SetOwnerReferences([]metav1.OwnerReference{{ + APIVersion: "nais.io/v1alpha1", Kind: "Application", Name: "owner", UID: "owner-uid", + Controller: new(true), BlockOwnerDeletion: new(true), + }}) + applyAsManager(t, resourceClient, controllerIntent, "resource-controller") + desired := application.DeepCopy() + unstructured.RemoveNestedField(desired.Object, "spec", "resources", "limits", "cpu") + + result, err := ApplyResource(t.Context(), client, gvr, desired) + if err != nil { + t.Fatal(err) + } + if diff := cmp.Diff(controllerIntent.Object["metadata"], metadataWithoutBookkeeping(result.After)); diff != "" { + t.Errorf("controller metadata mismatch (-want +got):\n%s", diff) + } + if diff := cmp.Diff(desired.Object["spec"], result.After.Object["spec"]); diff != "" { + t.Errorf("legacy field removal mismatch (-want +got):\n%s", diff) + } + for _, entry := range result.Before.GetManagedFields() { + if entry.Manager == "resource-controller" { + found := false + for _, after := range result.After.GetManagedFields() { + if after.Manager == entry.Manager { + found = true + if diff := cmp.Diff(entry, after); diff != "" { + t.Errorf("controller ownership changed (-want +got):\n%s", diff) + } + } } - options := update.GetUpdateOptions() - if options.FieldManager != fieldManager || options.FieldValidation != metav1.FieldValidationStrict { - t.Errorf("unexpected update options: %+v", options) + if !found { + t.Error("controller ownership was removed") } - stored, err := resourceClient.Get(ctx, desired.GetName(), metav1.GetOptions{}) + } + } +} + +func TestApplyResource_RejectsControllerConflicts(t *testing.T) { + gvr := schema.GroupVersionResource{Group: "nais.io", Version: "v1alpha1", Resource: "applications"} + client := newFieldManagedDynamicClient(gvr.GroupVersion().WithKind("Application")) + resourceClient := client.Resource(gvr).Namespace("my-team") + existing := applyTestApplication() + before := applyAsManager(t, resourceClient, existing, "resource-controller") + desired := existing.DeepCopy() + if err := unstructured.SetNestedField(desired.Object, "2", "spec", "resources", "limits", "cpu"); err != nil { + t.Fatal(err) + } + result, err := ApplyResource(t.Context(), client, gvr, desired) + if !apierrors.IsConflict(err) || result != nil { + t.Fatalf("expected an ownership conflict, got result=%+v, err=%v", result, err) + } + stored, err := resourceClient.Get(t.Context(), existing.GetName(), metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + if diff := cmp.Diff(before.Object, stored.Object); diff != "" { + t.Errorf("conflicting apply modified the resource (-want +got):\n%s", diff) + } +} + +func TestApplyResource_PreservesServerAssignedFields(t *testing.T) { + for _, test := range []struct { + kind string + gvr schema.GroupVersionResource + manifest map[string]any + controller map[string]any + }{ + { + kind: "Service", + gvr: schema.GroupVersionResource{Version: "v1", Resource: "services"}, + manifest: map[string]any{"spec": map[string]any{ + "selector": map[string]any{"app": "my-app"}, + "ports": []any{map[string]any{"port": int64(80)}}, + }}, + controller: map[string]any{"spec": map[string]any{ + "clusterIP": "10.0.0.42", "clusterIPs": []any{"10.0.0.42"}, + }}, + }, + { + kind: "Job", + gvr: schema.GroupVersionResource{Group: "batch", Version: "v1", Resource: "jobs"}, + manifest: map[string]any{"spec": map[string]any{"template": map[string]any{ + "spec": map[string]any{ + "restartPolicy": "Never", + "containers": []any{map[string]any{"name": "app", "image": "example.com/app:v1"}}, + }, + }}}, + controller: map[string]any{"spec": map[string]any{ + "selector": map[string]any{"matchLabels": map[string]any{"batch.kubernetes.io/controller-uid": "job-uid"}}, + "template": map[string]any{"metadata": map[string]any{"labels": map[string]any{ + "batch.kubernetes.io/controller-uid": "job-uid", "batch.kubernetes.io/job-name": "my-resource", + }}}, + }}, + }, + } { + t.Run(test.kind, func(t *testing.T) { + gvk := test.gvr.GroupVersion().WithKind(test.kind) + client := newFieldManagedDynamicClient(gvk) + resourceClient := client.Resource(test.gvr).Namespace("my-team") + manifest := (&unstructured.Unstructured{Object: test.manifest}).DeepCopy() + manifest.SetGroupVersionKind(gvk) + manifest.SetName("my-resource") + manifest.SetNamespace("my-team") + snapshot := manifest.DeepCopy() + snapshot.SetResourceVersion("1") + if _, err := resourceClient.Create(t.Context(), snapshot, metav1.CreateOptions{FieldManager: "kubectl-create"}); err != nil { + t.Fatal(err) + } + controller := (&unstructured.Unstructured{Object: test.controller}).DeepCopy() + controller.SetGroupVersionKind(gvk) + controller.SetName(manifest.GetName()) + controller.SetNamespace(manifest.GetNamespace()) + live := applyAsManager(t, resourceClient, controller, "kube-controller-manager") + + result, err := ApplyResource(t.Context(), client, test.gvr, manifest) if err != nil { t.Fatal(err) } - if diff := cmp.Diff(result.After.Object, stored.Object); diff != "" { - t.Errorf("stored replacement mismatch (-want +got):\n%s", diff) + if changes := Diff(result.Before, result.After); len(changes) != 0 { + t.Errorf("unchanged manifest removed server-assigned fields: %+v", changes) + } + if diff := cmp.Diff(live.Object["spec"], result.After.Object["spec"]); diff != "" { + t.Errorf("server-assigned fields changed (-want +got):\n%s", diff) } }) } } -func TestApplyResource_UsesCurrentResourceVersion(t *testing.T) { +func TestApplyResource_MigrationResourceVersionConflict(t *testing.T) { gvr := schema.GroupVersionResource{Group: "nais.io", Version: "v1alpha1", Resource: "applications"} - client := dynfake.NewSimpleDynamicClient(runtime.NewScheme()) - existing := &unstructured.Unstructured{Object: map[string]any{ - "apiVersion": "nais.io/v1alpha1", - "kind": "Application", - "metadata": map[string]any{ - "name": "my-app", - "namespace": "my-team", - "resourceVersion": "42", - }, - "spec": map[string]any{"image": "example.com/my-app:v1"}, - }} - desired := existing.DeepCopy() - desired.SetResourceVersion("") - if err := unstructured.SetNestedField(desired.Object, "example.com/my-app:v2", "spec", "image"); err != nil { + client := newFieldManagedDynamicClient(gvr.GroupVersion().WithKind("Application")) + resourceClient := client.Resource(gvr).Namespace("my-team") + desired := applyTestApplication() + snapshot := desired.DeepCopy() + snapshot.SetResourceVersion("42") + if _, err := resourceClient.Create(t.Context(), snapshot, metav1.CreateOptions{FieldManager: "deployd"}); err != nil { t.Fatal(err) } input := desired.DeepCopy() - - client.PrependReactor("get", "*", func(k8stesting.Action) (bool, runtime.Object, error) { - return true, existing.DeepCopy(), nil - }) - client.PrependReactor("update", "*", func(action k8stesting.Action) (bool, runtime.Object, error) { - update, ok := action.(k8stesting.UpdateAction) - if !ok { - t.Fatalf("expected UpdateAction, got %T", action) + client.ClearActions() + conflict := apierrors.NewConflict(gvr.GroupResource(), desired.GetName(), errors.New("resourceVersion changed")) + client.PrependReactor("patch", "*", func(action k8stesting.Action) (bool, runtime.Object, error) { + patch, ok := action.(k8stesting.PatchAction) + if !ok || patch.GetPatchType() != types.JSONPatchType { + t.Fatalf("expected migration PATCH before SSA, got %v", action) } - object, ok := update.GetObject().(*unstructured.Unstructured) - if !ok { - t.Fatalf("expected unstructured object, got %T", update.GetObject()) + var operations []struct { + Op string + Path string + Value json.RawMessage } - expected := desired.DeepCopy() - expected.SetResourceVersion("42") - if diff := cmp.Diff(expected.Object, object.Object); diff != "" { - t.Errorf("update mismatch (-want +got):\n%s", diff) + if err := json.Unmarshal(patch.GetPatch(), &operations); err != nil { + t.Fatal(err) } - return true, object.DeepCopy(), nil + guarded := false + for _, operation := range operations { + if operation.Op == "replace" && operation.Path == "/metadata/resourceVersion" { + guarded = true + if string(operation.Value) != `"42"` { + t.Errorf("migration resourceVersion = %s, want 42", operation.Value) + } + } + } + if !guarded { + t.Error("ownership migration has no resourceVersion guard") + } + return true, nil, conflict }) + result, err := ApplyResource(t.Context(), client, gvr, desired) + if !errors.Is(err, conflict) || result != nil { + t.Fatalf("expected wrapped migration conflict, got result=%+v, err=%v", result, err) + } + if actions := client.Actions(); len(actions) != 2 { + t.Errorf("expected no SSA after migration failure, got %v", actions) + } + if diff := cmp.Diff(input.Object, desired.Object); diff != "" { + t.Errorf("input was mutated (-want +got):\n%s", diff) + } +} +func TestApplyResource_DoesNotMigrateStatusOwnership(t *testing.T) { + gvr := schema.GroupVersionResource{Group: "nais.io", Version: "v1alpha1", Resource: "applications"} + client := dynfake.NewSimpleDynamicClient(runtime.NewScheme()) + desired := applyTestApplication() + before := desired.DeepCopy() + before.SetResourceVersion("42") + statusOwner := metav1.ManagedFieldsEntry{ + Manager: "deployd", Operation: metav1.ManagedFieldsOperationUpdate, + APIVersion: desired.GetAPIVersion(), Subresource: "status", FieldsType: "FieldsV1", + FieldsV1: &metav1.FieldsV1{Raw: []byte(`{"f:status":{"f:state":{}}}`)}, + } + before.SetManagedFields([]metav1.ManagedFieldsEntry{statusOwner}) + client.PrependReactor("get", "*", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, before.DeepCopy(), nil + }) + client.PrependReactor("patch", "*", func(action k8stesting.Action) (bool, runtime.Object, error) { + patch, ok := action.(k8stesting.PatchAction) + if !ok || patch.GetPatchType() != types.ApplyPatchType { + t.Fatalf("status ownership must not generate a migration patch: %v", action) + } + return true, before.DeepCopy(), nil + }) result, err := ApplyResource(t.Context(), client, gvr, desired) if err != nil { t.Fatal(err) } - if result.Created { - t.Error("expected an update, not a creation") - } - if diff := cmp.Diff(existing.Object, result.Before.Object); diff != "" { - t.Errorf("before-state mismatch (-want +got):\n%s", diff) + if diff := cmp.Diff([]metav1.ManagedFieldsEntry{statusOwner}, result.After.GetManagedFields()); diff != "" { + t.Errorf("status ownership changed (-want +got):\n%s", diff) } - if result.After.GetResourceVersion() != "42" { - t.Errorf("resourceVersion = %q, want 42", result.After.GetResourceVersion()) +} + +func TestApplyResource_RejectsMalformedLegacyOwnership(t *testing.T) { + gvr := schema.GroupVersionResource{Group: "nais.io", Version: "v1alpha1", Resource: "applications"} + client := dynfake.NewSimpleDynamicClient(runtime.NewScheme()) + desired := applyTestApplication() + before := desired.DeepCopy() + before.SetResourceVersion("42") + before.SetManagedFields([]metav1.ManagedFieldsEntry{{ + Manager: "deployd", Operation: metav1.ManagedFieldsOperationUpdate, + APIVersion: desired.GetAPIVersion(), FieldsType: "FieldsV1", + FieldsV1: &metav1.FieldsV1{Raw: []byte(`{"f:spec":123}`)}, + }}) + client.PrependReactor("get", "*", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, before.DeepCopy(), nil + }) + result, err := ApplyResource(t.Context(), client, gvr, desired) + if err == nil || result != nil { + t.Fatalf("expected invalid ownership to fail, got result=%+v, err=%v", result, err) } - if diff := cmp.Diff(input.Object, desired.Object); diff != "" { - t.Errorf("input was mutated (-want +got):\n%s", diff) + if actions := client.Actions(); len(actions) != 1 { + t.Errorf("invalid ownership must not cause a write: %v", actions) } } func TestApplyResource_KubernetesErrors(t *testing.T) { gvr := schema.GroupVersionResource{Group: "nais.io", Version: "v1alpha1", Resource: "applications"} for _, test := range []struct { - name string - verb string - err error + name string + verb string + existing bool + err error }{ - { - name: "get forbidden", - verb: "get", - err: apierrors.NewForbidden(gvr.GroupResource(), "my-app", errors.New("denied")), - }, - { - name: "create rejected", - verb: "create", - err: apierrors.NewBadRequest("invalid resource"), - }, - { - name: "created concurrently", - verb: "create", - err: apierrors.NewAlreadyExists(gvr.GroupResource(), "my-app"), - }, - { - name: "update rejected", - verb: "update", - err: apierrors.NewBadRequest("immutable field changed"), - }, - { - name: "updated concurrently", - verb: "update", - err: apierrors.NewConflict(gvr.GroupResource(), "my-app", errors.New("resourceVersion changed")), - }, + {name: "get forbidden", verb: "get", err: apierrors.NewForbidden(gvr.GroupResource(), "my-app", errors.New("denied"))}, + {name: "creation rejected", verb: "patch", err: apierrors.NewBadRequest("invalid resource")}, + {name: "existing apply rejected", verb: "patch", existing: true, err: apierrors.NewBadRequest("immutable field changed")}, + {name: "apply conflict", verb: "patch", existing: true, err: apierrors.NewConflict(gvr.GroupResource(), "my-app", errors.New("ownership conflict"))}, } { t.Run(test.name, func(t *testing.T) { client := dynfake.NewSimpleDynamicClient(runtime.NewScheme()) - desired := &unstructured.Unstructured{Object: map[string]any{ - "apiVersion": "nais.io/v1alpha1", - "kind": "Application", - "metadata": map[string]any{ - "name": "my-app", - "namespace": "my-team", - }, - }} - if test.verb == "update" { - existing := desired.DeepCopy() - existing.SetResourceVersion("42") + desired := applyTestApplication() + if test.existing { client.PrependReactor("get", "*", func(k8stesting.Action) (bool, runtime.Object, error) { - return true, existing.DeepCopy(), nil + return true, desired.DeepCopy(), nil }) } client.PrependReactor(test.verb, "*", func(k8stesting.Action) (bool, runtime.Object, error) { return true, nil, test.err }) - result, err := ApplyResource(t.Context(), client, gvr, desired) if !errors.Is(err, test.err) { t.Fatalf("error = %v, want wrapped %v", err, test.err) @@ -432,13 +534,45 @@ func TestApplyResource_KubernetesErrors(t *testing.T) { } } +func applyTestApplication() *unstructured.Unstructured { + return &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "nais.io/v1alpha1", + "kind": "Application", + "metadata": map[string]any{"name": "my-app", "namespace": "my-team"}, + "spec": map[string]any{ + "image": "example.com/my-app:v1", + "resources": map[string]any{ + "limits": map[string]any{"cpu": "1", "memory": "512Mi"}, + "requests": map[string]any{"cpu": "100m", "memory": "256Mi"}, + }, + }, + }} +} + +func applyAsManager(t *testing.T, client dynamic.ResourceInterface, object *unstructured.Unstructured, manager string) *unstructured.Unstructured { + t.Helper() + data, err := json.Marshal(object.Object) + if err != nil { + t.Fatal(err) + } + result, err := client.Patch(t.Context(), object.GetName(), types.ApplyPatchType, data, metav1.PatchOptions{FieldManager: manager}) + if err != nil { + t.Fatal(err) + } + return result +} + +func metadataWithoutBookkeeping(object *unstructured.Unstructured) any { + copy := object.DeepCopy() + copy.SetManagedFields(nil) + copy.SetResourceVersion("") + return copy.Object["metadata"] +} + func newFieldManagedDynamicClient(gvk schema.GroupVersionKind) *dynfake.FakeDynamicClient { scheme := runtime.NewScheme() scheme.AddKnownTypeWithName(gvk, &unstructured.Unstructured{}) scheme.AddKnownTypeWithName(gvk.GroupVersion().WithKind(gvk.Kind+"List"), &unstructured.UnstructuredList{}) - - // The integration fake replaces spec wholesale, masking field ownership. - // Use Kubernetes' field manager to include existing field ownership. tracker := k8stesting.NewFieldManagedObjectTracker(scheme, serializer.NewCodecFactory(scheme).UniversalDecoder(), managedfields.NewDeducedTypeConverter()) client := dynfake.NewSimpleDynamicClient(scheme) client.PrependReactor("*", "*", k8stesting.ObjectReaction(tracker)) diff --git a/internal/apply/migration_test.go b/internal/apply/migration_test.go new file mode 100644 index 000000000..ce6dfecb7 --- /dev/null +++ b/internal/apply/migration_test.go @@ -0,0 +1,276 @@ +package apply + +import ( + "encoding/json" + "testing" + + "github.com/google/go-cmp/cmp" + apierrors "k8s.io/apimachinery/pkg/api/errors" + 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/apimachinery/pkg/types" + dynfake "k8s.io/client-go/dynamic/fake" + k8stesting "k8s.io/client-go/testing" +) + +func TestApplyResource_MigrationAPIVersions(t *testing.T) { + for _, test := range []struct { + name string + owners []metav1.ManagedFieldsEntry + wantError bool + wantMigrate bool + }{ + { + name: "existing SSA version differs from legacy version", + owners: []metav1.ManagedFieldsEntry{ + migrationTestOwner(fieldManager, metav1.ManagedFieldsOperationApply, "nais.io/v1", ""), + migrationTestOwner("deployd", metav1.ManagedFieldsOperationUpdate, "nais.io/v1alpha1", ""), + }, + wantError: true, + }, + { + name: "one legacy manager owns fields in multiple API versions", + owners: []metav1.ManagedFieldsEntry{ + migrationTestOwner("deployd", metav1.ManagedFieldsOperationUpdate, "nais.io/v1", ""), + migrationTestOwner("deployd", metav1.ManagedFieldsOperationUpdate, "nais.io/v1alpha1", ""), + }, + wantError: true, + }, + { + name: "different legacy managers own different API versions", + owners: []metav1.ManagedFieldsEntry{ + migrationTestOwner("kubectl-create", metav1.ManagedFieldsOperationUpdate, "nais.io/v1", ""), + migrationTestOwner("deployd", metav1.ManagedFieldsOperationUpdate, "nais.io/v1alpha1", ""), + }, + wantError: true, + }, + { + name: "matching SSA and legacy versions are migrated", + owners: []metav1.ManagedFieldsEntry{ + migrationTestOwner(fieldManager, metav1.ManagedFieldsOperationApply, "nais.io/v1", ""), + migrationTestOwner("deployd", metav1.ManagedFieldsOperationUpdate, "nais.io/v1", ""), + }, + wantMigrate: true, + }, + { + name: "a single older version can be migrated before SSA version conversion", + owners: []metav1.ManagedFieldsEntry{ + migrationTestOwner("deployd", metav1.ManagedFieldsOperationUpdate, "nais.io/v1alpha1", ""), + }, + wantMigrate: true, + }, + { + name: "foreign manager and status versions do not prevent migration", + owners: []metav1.ManagedFieldsEntry{ + migrationTestOwner(fieldManager, metav1.ManagedFieldsOperationApply, "nais.io/v1", ""), + migrationTestOwner("deployd", metav1.ManagedFieldsOperationUpdate, "nais.io/v1", ""), + migrationTestOwner("deployd", metav1.ManagedFieldsOperationUpdate, "nais.io/v1alpha1", "status"), + migrationTestOwner("controller", metav1.ManagedFieldsOperationUpdate, "nais.io/v1alpha1", ""), + }, + wantMigrate: true, + }, + { + name: "legacy Apply entries are not migrated regardless of their version", + owners: []metav1.ManagedFieldsEntry{ + migrationTestOwner(fieldManager, metav1.ManagedFieldsOperationApply, "nais.io/v1", ""), + migrationTestOwner("deployd", metav1.ManagedFieldsOperationApply, "nais.io/v1alpha1", ""), + }, + }, + } { + t.Run(test.name, func(t *testing.T) { + gvr := schema.GroupVersionResource{Group: "nais.io", Version: "v1", Resource: "naisjobs"} + client := dynfake.NewSimpleDynamicClient(runtime.NewScheme()) + desired := applyTestApplication() + desired.SetAPIVersion("nais.io/v1") + desired.SetKind("Naisjob") + before := desired.DeepCopy() + before.SetResourceVersion("42") + before.SetManagedFields(test.owners) + snapshot := before.DeepCopy() + migrations := 0 + applies := 0 + client.PrependReactor("get", "*", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, before.DeepCopy(), nil + }) + client.PrependReactor("patch", "*", func(action k8stesting.Action) (bool, runtime.Object, error) { + patch, ok := action.(k8stesting.PatchAction) + if !ok { + t.Fatalf("expected PatchAction, got %T", action) + } + if patch.GetPatchType() == types.JSONPatchType { + migrations++ + migrated := before.DeepCopy() + migrated.SetResourceVersion("43") + return true, migrated, nil + } + if patch.GetPatchType() != types.ApplyPatchType { + t.Fatalf("unexpected patch type: %s", patch.GetPatchType()) + } + applies++ + return true, desired.DeepCopy(), nil + }) + + result, err := ApplyResource(t.Context(), client, gvr, desired) + if test.wantError { + if err == nil || result != nil { + t.Fatalf("expected unsupported migration to fail, got result=%+v, err=%v", result, err) + } + if actions := client.Actions(); len(actions) != 1 || migrations != 0 || applies != 0 { + t.Errorf("unsupported migration performed a write: %v", actions) + } + } else { + if err != nil { + t.Fatal(err) + } + wantMigrations := 0 + if test.wantMigrate { + wantMigrations = 1 + } + if migrations != wantMigrations || applies != 1 { + t.Errorf("migration/apply counts = %d/%d, want %d/1", migrations, applies, wantMigrations) + } + } + if diff := cmp.Diff(snapshot.Object, before.Object); diff != "" { + t.Errorf("migration mutated the before-state (-want +got):\n%s", diff) + } + }) + } +} + +func TestApplyResource_CallerResourceVersion(t *testing.T) { + for _, test := range []struct { + name string + submittedVersion string + migrate bool + missing bool + concurrentUpdate bool + wantConflict bool + wantApplyVersion string + wantMigrationCall bool + }{ + { + name: "current version is advanced after migration", submittedVersion: "42", migrate: true, + wantApplyVersion: "43", wantMigrationCall: true, + }, + { + name: "current version is retained without migration", submittedVersion: "42", + wantApplyVersion: "42", + }, + { + name: "absent version stays absent after migration", migrate: true, wantMigrationCall: true, + }, + { + name: "stale version cannot migrate ownership", submittedVersion: "41", migrate: true, wantConflict: true, + }, + { + name: "stale version cannot apply without migration", submittedVersion: "41", wantConflict: true, + }, + { + name: "a deleted versioned object is not recreated", submittedVersion: "42", missing: true, wantConflict: true, + }, + { + name: "unversioned creation remains supported", missing: true, + }, + { + name: "concurrent update after migration is still rejected", submittedVersion: "42", migrate: true, + concurrentUpdate: true, wantConflict: true, wantApplyVersion: "43", wantMigrationCall: true, + }, + } { + t.Run(test.name, func(t *testing.T) { + gvr := schema.GroupVersionResource{Group: "nais.io", Version: "v1alpha1", Resource: "applications"} + client := dynfake.NewSimpleDynamicClient(runtime.NewScheme()) + desired := applyTestApplication() + desired.SetResourceVersion(test.submittedVersion) + input := desired.DeepCopy() + before := applyTestApplication() + before.SetResourceVersion("42") + if test.migrate { + before.SetManagedFields([]metav1.ManagedFieldsEntry{ + migrationTestOwner("deployd", metav1.ManagedFieldsOperationUpdate, before.GetAPIVersion(), ""), + }) + } + currentVersion := "42" + migrations := 0 + applies := 0 + client.PrependReactor("get", "*", func(k8stesting.Action) (bool, runtime.Object, error) { + if test.missing { + return true, nil, apierrors.NewNotFound(gvr.GroupResource(), desired.GetName()) + } + return true, before.DeepCopy(), nil + }) + client.PrependReactor("patch", "*", func(action k8stesting.Action) (bool, runtime.Object, error) { + patch, ok := action.(k8stesting.PatchAction) + if !ok { + t.Fatalf("expected PatchAction, got %T", action) + } + if patch.GetPatchType() == types.JSONPatchType { + migrations++ + currentVersion = "43" + migrated := before.DeepCopy() + migrated.SetResourceVersion(currentVersion) + if test.concurrentUpdate { + currentVersion = "44" + } + return true, migrated, nil + } + if patch.GetPatchType() != types.ApplyPatchType { + t.Fatalf("unexpected patch type: %s", patch.GetPatchType()) + } + applies++ + var submitted unstructured.Unstructured + if err := json.Unmarshal(patch.GetPatch(), &submitted); err != nil { + t.Fatal(err) + } + if submitted.GetResourceVersion() != test.wantApplyVersion { + t.Errorf("SSA resourceVersion = %q, want %q", submitted.GetResourceVersion(), test.wantApplyVersion) + } + if version := submitted.GetResourceVersion(); version != "" && version != currentVersion { + return true, nil, apierrors.NewConflict(gvr.GroupResource(), desired.GetName(), nil) + } + submitted.SetResourceVersion("45") + return true, &submitted, nil + }) + + result, err := ApplyResource(t.Context(), client, gvr, desired) + if test.wantConflict { + if !apierrors.IsConflict(err) || result != nil { + t.Fatalf("expected concurrency conflict, got result=%+v, err=%v", result, err) + } + if !test.concurrentUpdate && applies != 0 { + t.Error("stale request reached SSA") + } + } else { + if err != nil { + t.Fatal(err) + } + if result.Created != test.missing || applies != 1 { + t.Errorf("unexpected create/apply result: created=%t, applies=%d", result.Created, applies) + } + if !test.missing { + if diff := cmp.Diff(before.Object, result.Before.Object); diff != "" { + t.Errorf("before-state mismatch (-want +got):\n%s", diff) + } + } + } + wantMigrations := 0 + if test.wantMigrationCall { + wantMigrations = 1 + } + if migrations != wantMigrations { + t.Errorf("migration calls = %d, want %d", migrations, wantMigrations) + } + if diff := cmp.Diff(input.Object, desired.Object); diff != "" { + t.Errorf("caller input was mutated (-want +got):\n%s", diff) + } + }) + } +} + +func migrationTestOwner(manager string, operation metav1.ManagedFieldsOperationType, apiVersion, subresource string) metav1.ManagedFieldsEntry { + return metav1.ManagedFieldsEntry{ + Manager: manager, Operation: operation, APIVersion: apiVersion, Subresource: subresource, FieldsType: "FieldsV1", + FieldsV1: &metav1.FieldsV1{Raw: []byte(`{"f:spec":{"f:resources":{"f:limits":{"f:cpu":{}}}}}`)}, + } +} diff --git a/internal/kubernetes/fake/apply_test.go b/internal/kubernetes/fake/apply_test.go new file mode 100644 index 000000000..56fd2e5d2 --- /dev/null +++ b/internal/kubernetes/fake/apply_test.go @@ -0,0 +1,181 @@ +package fake_test + +import ( + "encoding/json" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/nais/api/internal/apply" + "github.com/nais/api/internal/kubernetes" + "github.com/nais/api/internal/kubernetes/fake" + apierrors "k8s.io/apimachinery/pkg/api/errors" + 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" +) + +func TestApplyRemovesOwnedFieldsInFakeClient(t *testing.T) { + for _, test := range []struct { + kind string + gvr schema.GroupVersionResource + existing map[string]any + desired map[string]any + }{ + { + kind: "Application", + gvr: schema.GroupVersionResource{Group: "nais.io", Version: "v1alpha1", Resource: "applications"}, + existing: map[string]any{"spec": map[string]any{ + "image": "example.com/app:v1", "resources": map[string]any{"limits": map[string]any{"cpu": "1", "memory": "512Mi"}}, + }}, + desired: map[string]any{"spec": map[string]any{ + "image": "example.com/app:v1", "resources": map[string]any{"limits": map[string]any{"memory": "512Mi"}}, + }}, + }, + { + kind: "ConfigMap", + gvr: schema.GroupVersionResource{Version: "v1", Resource: "configmaps"}, + existing: map[string]any{"data": map[string]any{"kept": "old", "removed": "old"}, "binaryData": map[string]any{"removed": "b2xk"}}, + desired: map[string]any{"data": map[string]any{"kept": "new"}}, + }, + { + kind: "Secret", + gvr: schema.GroupVersionResource{Version: "v1", Resource: "secrets"}, + existing: map[string]any{"data": map[string]any{"kept": "b2xk", "removed": "b2xk"}}, + desired: map[string]any{"data": map[string]any{"kept": "bmV3"}}, + }, + } { + t.Run(test.kind, func(t *testing.T) { + scheme, err := kubernetes.NewScheme() + if err != nil { + t.Fatal(err) + } + client := fake.NewDynamicClient(scheme) + existing := (&unstructured.Unstructured{Object: test.existing}).DeepCopy() + existing.SetGroupVersionKind(test.gvr.GroupVersion().WithKind(test.kind)) + existing.SetName("my-resource") + existing.SetNamespace("my-team") + existing.SetLabels(map[string]string{"kept": "same", "removed": "old"}) + existing.SetAnnotations(map[string]string{"removed": "old"}) + if _, err := apply.ApplyResource(t.Context(), client, test.gvr, existing); err != nil { + t.Fatal(err) + } + desired := (&unstructured.Unstructured{Object: test.desired}).DeepCopy() + desired.SetGroupVersionKind(existing.GroupVersionKind()) + desired.SetName(existing.GetName()) + desired.SetNamespace(existing.GetNamespace()) + desired.SetLabels(map[string]string{"kept": "same"}) + + result, err := apply.ApplyResource(t.Context(), client, test.gvr, desired) + if err != nil { + t.Fatal(err) + } + actual := result.After.DeepCopy() + actual.SetManagedFields(nil) + if diff := cmp.Diff(desired.Object, actual.Object); diff != "" { + t.Errorf("fake SSA mismatch (-want +got):\n%s", diff) + } + reapplied, err := apply.ApplyResource(t.Context(), client, test.gvr, desired) + if err != nil { + t.Fatal(err) + } + if changes := apply.Diff(reapplied.Before, reapplied.After); len(changes) != 0 { + t.Errorf("reapplying changed the fake resource: %+v", changes) + } + }) + } +} + +func TestFakeClientPreservesOwnershipAndConflicts(t *testing.T) { + scheme, err := kubernetes.NewScheme() + if err != nil { + t.Fatal(err) + } + client := fake.NewDynamicClient(scheme) + gvr := schema.GroupVersionResource{Group: "nais.io", Version: "v1alpha1", Resource: "applications"} + resourceClient := client.Resource(gvr).Namespace("my-team") + application := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "nais.io/v1alpha1", "kind": "Application", + "metadata": map[string]any{"name": "my-app", "namespace": "my-team"}, + "spec": map[string]any{"image": "example.com/app:v1", "resources": map[string]any{"limits": map[string]any{"cpu": "1"}}}, + }} + if _, err := apply.ApplyResource(t.Context(), client, gvr, application); err != nil { + t.Fatal(err) + } + controller := application.DeepCopy() + controller.Object["spec"] = map[string]any{"resources": map[string]any{"limits": map[string]any{"cpu": "1"}}} + controller.SetFinalizers([]string{"controller.example.com/cleanup"}) + data, err := json.Marshal(controller.Object) + if err != nil { + t.Fatal(err) + } + if _, err := resourceClient.Patch(t.Context(), controller.GetName(), types.ApplyPatchType, data, metav1.PatchOptions{ + FieldManager: "resource-controller", + }); err != nil { + t.Fatal(err) + } + desired := application.DeepCopy() + unstructured.RemoveNestedField(desired.Object, "spec", "resources") + result, err := apply.ApplyResource(t.Context(), client, gvr, desired) + if err != nil { + t.Fatal(err) + } + if diff := cmp.Diff(application.Object["spec"], result.After.Object["spec"]); diff != "" { + t.Errorf("controller-owned CPU was removed (-want +got):\n%s", diff) + } + if diff := cmp.Diff(controller.GetFinalizers(), result.After.GetFinalizers()); diff != "" { + t.Errorf("controller finalizers changed (-want +got):\n%s", diff) + } + before := result.After.DeepCopy() + conflicting := application.DeepCopy() + if err := unstructured.SetNestedField(conflicting.Object, "2", "spec", "resources", "limits", "cpu"); err != nil { + t.Fatal(err) + } + if result, err := apply.ApplyResource(t.Context(), client, gvr, conflicting); result != nil || !apierrors.IsConflict(err) { + t.Fatalf("expected ownership conflict, got result=%+v, err=%v", result, err) + } + stored, err := resourceClient.Get(t.Context(), application.GetName(), metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + if diff := cmp.Diff(before.Object, stored.Object); diff != "" { + t.Errorf("conflicting apply modified the resource (-want +got):\n%s", diff) + } +} + +func TestFakeClientMigratesLegacyOwnership(t *testing.T) { + scheme, err := kubernetes.NewScheme() + if err != nil { + t.Fatal(err) + } + client := fake.NewDynamicClient(scheme) + gvr := schema.GroupVersionResource{Group: "nais.io", Version: "v1alpha1", Resource: "applications"} + application := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "nais.io/v1alpha1", "kind": "Application", + "metadata": map[string]any{"name": "my-app", "namespace": "my-team", "resourceVersion": "1"}, + "spec": map[string]any{"image": "example.com/app:v1", "resources": map[string]any{"limits": map[string]any{"cpu": "1", "memory": "512Mi"}}}, + }} + application.SetManagedFields([]metav1.ManagedFieldsEntry{{ + Manager: "deployd", Operation: metav1.ManagedFieldsOperationUpdate, APIVersion: application.GetAPIVersion(), FieldsType: "FieldsV1", + FieldsV1: &metav1.FieldsV1{Raw: []byte(`{"f:spec":{"f:resources":{"f:limits":{"f:cpu":{}}}}}`)}, + }}) + if _, err := client.Resource(gvr).Namespace("my-team").Create(t.Context(), application, metav1.CreateOptions{}); err != nil { + t.Fatal(err) + } + desired := application.DeepCopy() + desired.SetManagedFields(nil) + desired.SetResourceVersion("") + unstructured.RemoveNestedField(desired.Object, "spec", "resources", "limits", "cpu") + result, err := apply.ApplyResource(t.Context(), client, gvr, desired) + if err != nil { + t.Fatal(err) + } + if diff := cmp.Diff(desired.Object["spec"], result.After.Object["spec"]); diff != "" { + t.Errorf("legacy-owned CPU was retained (-want +got):\n%s", diff) + } + for _, entry := range result.After.GetManagedFields() { + if entry.Manager == "deployd" { + t.Errorf("legacy ownership was not migrated: %+v", entry) + } + } +} diff --git a/internal/kubernetes/fake/fake.go b/internal/kubernetes/fake/fake.go index c1eae2182..819d54458 100644 --- a/internal/kubernetes/fake/fake.go +++ b/internal/kubernetes/fake/fake.go @@ -5,7 +5,6 @@ import ( "encoding/json" "fmt" "io/fs" - "maps" "os" "path/filepath" "strings" @@ -17,11 +16,13 @@ import ( nais_io_v1alpha1 "github.com/nais/liberator/pkg/apis/nais.io/v1alpha1" mapperatorv1 "github.com/nais/pgrator/pkg/api/v1" unleash_nais_io_v1 "github.com/nais/unleasherator/api/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/managedfields" "k8s.io/client-go/dynamic" dynfake "k8s.io/client-go/dynamic/fake" "k8s.io/client-go/rest" @@ -291,51 +292,57 @@ func NewDynamicClient(scheme *runtime.Scheme) *dynfake.FakeDynamicClient { if patchAction.GetPatchType() != types.ApplyPatchType { return false, nil, nil } + patchWithOptions, ok := action.(k8stesting.PatchActionImpl) + if !ok { + return true, nil, fmt.Errorf("expected PatchActionImpl, got %T", action) + } gvr := patchAction.GetResource() ns := patchAction.GetNamespace() name := patchAction.GetName() desired := &unstructured.Unstructured{} - if err := json.Unmarshal(patchAction.GetPatch(), &desired.Object); err != nil { + if err := json.Unmarshal(patchAction.GetPatch(), desired); err != nil { return true, nil, fmt.Errorf("unmarshaling apply patch: %w", err) } existing, err := client.Tracker().Get(gvr, ns, name) + created := apierrors.IsNotFound(err) if err != nil { - if err := client.Tracker().Create(gvr, desired, ns); err != nil { - return true, nil, fmt.Errorf("creating object via apply: %w", err) + if !created { + return true, nil, err } - return true, desired, nil + existing = &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": desired.GetAPIVersion(), + "kind": desired.GetKind(), + }} + } + gvk := desired.GroupVersionKind() + // A per-kind scheme avoids converting to another kind registered with + // the same unstructured Go type and group/version. + applyScheme := runtime.NewScheme() + applyScheme.AddKnownTypeWithName(gvk, &unstructured.Unstructured{}) + manager, err := managedfields.NewDefaultCRDFieldManager( + managedfields.NewDeducedTypeConverter(), + applyScheme, applyScheme, applyScheme, gvk, gvk.GroupVersion(), "", nil, + ) + if err != nil { + return true, nil, fmt.Errorf("creating field manager: %w", err) } - - existingUnstr, ok := existing.(*unstructured.Unstructured) - if !ok { - return true, nil, fmt.Errorf("expected *unstructured.Unstructured, got %T", existing) - } - - merged := existingUnstr.DeepCopy() - for k, v := range desired.Object { - if k == "metadata" { - // Merge metadata rather than replacing it wholesale. - desiredMeta, ok1 := v.(map[string]any) - existingMeta, ok2 := merged.Object["metadata"].(map[string]any) - if ok1 && ok2 { - maps.Copy(existingMeta, desiredMeta) - merged.Object["metadata"] = existingMeta - } else { - merged.Object[k] = v - } - } else { - merged.Object[k] = v - } + options := patchWithOptions.GetPatchOptions() + force := options.Force != nil && *options.Force + applied, err := manager.Apply(existing, desired, options.FieldManager, force) + if err != nil { + return true, nil, err } - - if err := client.Tracker().Update(gvr, merged, ns); err != nil { + if created { + if err := client.Tracker().Create(gvr, applied, ns); err != nil { + return true, nil, fmt.Errorf("creating object via apply: %w", err) + } + } else if err := client.Tracker().Update(gvr, applied, ns); err != nil { return true, nil, fmt.Errorf("updating object via apply: %w", err) } - - return true, merged, nil + return true, applied, nil }) return client