diff --git a/cmd/crossplane/simulate/defaults.go b/cmd/crossplane/simulate/defaults.go new file mode 100644 index 00000000..0c69f6ed --- /dev/null +++ b/cmd/crossplane/simulate/defaults.go @@ -0,0 +1,88 @@ +/* +Copyright 2026 The Crossplane Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package simulate + +import ( + "context" + + extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "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" + "sigs.k8s.io/controller-runtime/pkg/client" + + xrpkg "github.com/crossplane/cli/v2/pkg/xr" +) + +// crdDefaults caches the cluster's CRDs by group and kind, so the approximate +// fallback can apply the API server's schema defaults to a desired manifest +// before diffing it against the live resource. The live resource was defaulted +// by the API server on admission, and applying the manifest would default it +// again, so fields a manifest omits but the CRD defaults (deletionPolicy, +// managementPolicies, ...) are not removals and must not plan as ones. Like +// gather, this is best effort: a kind without a fetchable CRD simply diffs +// undefaulted, and the miss is cached so a run over many resources of that +// kind probes the cluster once. +type crdDefaults struct { + cache map[schema.GroupKind]*extv1.CustomResourceDefinition +} + +func newCRDDefaults() *crdDefaults { + return &crdDefaults{cache: map[schema.GroupKind]*extv1.CustomResourceDefinition{}} +} + +// apply returns a copy of desired with its CRD's defaults applied, as the API +// server would admit it, or desired itself when the cluster does not serve the +// kind or its CRD yields no usable schema. +func (c *crdDefaults) apply(ctx context.Context, kubeClient client.Client, rmapper meta.RESTMapper, desired *unstructured.Unstructured) *unstructured.Unstructured { + gvk := desired.GroupVersionKind() + crd, ok := c.cache[gvk.GroupKind()] + if !ok { + crd = fetchCRD(ctx, kubeClient, rmapper, gvk) + c.cache[gvk.GroupKind()] = crd + } + if crd == nil { + return desired + } + + defaulted := desired.DeepCopy() + if err := xrpkg.ApplyCRDDefaults(defaulted.Object, desired.GetAPIVersion(), *crd); err != nil { + return desired + } + return defaulted +} + +// fetchCRD fetches the CRD serving gvk. nil means no defaulting is possible: +// the kind is not served or its CRD is unreadable. +func fetchCRD(ctx context.Context, kubeClient client.Client, rmapper meta.RESTMapper, gvk schema.GroupVersionKind) *extv1.CustomResourceDefinition { + mapping, err := rmapper.RESTMapping(gvk.GroupKind(), gvk.Version) + if err != nil { + return nil + } + + u := &unstructured.Unstructured{} + u.SetGroupVersionKind(schema.GroupVersionKind{Group: "apiextensions.k8s.io", Version: "v1", Kind: "CustomResourceDefinition"}) + if err := kubeClient.Get(ctx, client.ObjectKey{Name: mapping.Resource.Resource + "." + gvk.Group}, u); err != nil { + return nil + } + crd := &extv1.CustomResourceDefinition{} + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, crd); err != nil { + return nil + } + return crd +} diff --git a/cmd/crossplane/simulate/simulate.go b/cmd/crossplane/simulate/simulate.go index bfdea6a3..7752a440 100644 --- a/cmd/crossplane/simulate/simulate.go +++ b/cmd/crossplane/simulate/simulate.go @@ -203,8 +203,9 @@ func (c *Cmd) Run(ctx context.Context, k *kong.Context, logger logging.Logger, s plans := make([]*plan.ResourcePlan, 0, len(resources)) stores := newObjectStoreCache() + crds := newCRDDefaults() for _, desired := range resources { - plans = append(plans, c.simulateOne(ctx, kubeClient, rmapper, defaultNamespace, mgr, stores, desired)) + plans = append(plans, c.simulateOne(ctx, kubeClient, rmapper, defaultNamespace, mgr, stores, crds, desired)) } return renderer.Render(k.Stdout, plans) @@ -325,7 +326,7 @@ type router interface { // to a diff server, and fall back to the approximate client-side diff when no // server can plan it. Failures become per-resource errors so one resource // never fails the whole run. -func (c *Cmd) simulateOne(ctx context.Context, kubeClient client.Client, rmapper meta.RESTMapper, defaultNamespace string, mgr router, stores *objectStoreCache, desired *unstructured.Unstructured) *plan.ResourcePlan { +func (c *Cmd) simulateOne(ctx context.Context, kubeClient client.Client, rmapper meta.RESTMapper, defaultNamespace string, mgr router, stores *objectStoreCache, crds *crdDefaults, desired *unstructured.Unstructured) *plan.ResourcePlan { if desired.GetName() == "" { p := plan.NewResourcePlan(desired) p.Error = "resource has no name" @@ -347,7 +348,11 @@ func (c *Cmd) simulateOne(ctx context.Context, kubeClient client.Client, rmapper gk := desired.GroupVersionKind().GroupKind() planClient, ok := mgr.Route(gk) if !ok { - return plan.Fallback(desired, live, fmt.Sprintf("no diff server for %s", gk)) + // The fallback diffs desired against live verbatim, but live was + // defaulted by the API server on admission and applying the manifest + // would default it again. Apply the CRD's defaults to desired first, + // so defaulted fields the manifest omits do not plan as removals. + return plan.Fallback(crds.apply(ctx, kubeClient, rmapper, desired), live, fmt.Sprintf("no diff server for %s", gk)) } pctx, cancel := context.WithTimeout(ctx, c.Timeout) @@ -358,7 +363,7 @@ func (c *Cmd) simulateOne(ctx context.Context, kubeClient client.Client, rmapper p, err := plan.Plan(pctx, planClient, desired, live, store) if err != nil { if plan.IsNotSupported(err) { - return plan.Fallback(desired, live, "the provider cannot compute a diff for this resource") + return plan.Fallback(crds.apply(ctx, kubeClient, rmapper, desired), live, "the provider cannot compute a diff for this resource") } p := plan.NewResourcePlan(desired) p.Error = fmt.Sprintf("diff server cannot plan: %s", err) diff --git a/cmd/crossplane/simulate/simulate_test.go b/cmd/crossplane/simulate/simulate_test.go index b5f9f7f0..e9780f1c 100644 --- a/cmd/crossplane/simulate/simulate_test.go +++ b/cmd/crossplane/simulate/simulate_test.go @@ -191,7 +191,7 @@ func TestSimulateOne(t *testing.T) { }}, }} - got := cmd.simulateOne(ctx, kube, rm, "default", &fakeRouter{client: pc}, newObjectStoreCache(), vpc("my-vpc", "10.1.0.0/16")) + got := cmd.simulateOne(ctx, kube, rm, "default", &fakeRouter{client: pc}, newObjectStoreCache(), newCRDDefaults(), vpc("my-vpc", "10.1.0.0/16")) if got.Action != plan.ActionUpdate || got.Approximate || got.Error != "" { t.Errorf("simulateOne(...): want a provider-computed update, got %+v", got) @@ -205,7 +205,7 @@ func TestSimulateOne(t *testing.T) { kube := fake.NewClientBuilder().WithScheme(s).Build() pc := &fakePlanClient{rsp: &diffv1alpha1.PlanResponse{Action: diffv1alpha1.Action_ACTION_CREATE}} - got := cmd.simulateOne(ctx, kube, rm, "default", &fakeRouter{client: pc}, newObjectStoreCache(), vpc("new-vpc", "10.0.0.0/16")) + got := cmd.simulateOne(ctx, kube, rm, "default", &fakeRouter{client: pc}, newObjectStoreCache(), newCRDDefaults(), vpc("new-vpc", "10.0.0.0/16")) if got.Action != plan.ActionCreate || got.Error != "" { t.Errorf("simulateOne(...): want a create, got %+v", got) @@ -219,7 +219,7 @@ func TestSimulateOne(t *testing.T) { live := vpc("my-vpc", "10.0.0.0/16") kube := fake.NewClientBuilder().WithScheme(s).WithObjects(live).Build() - got := cmd.simulateOne(ctx, kube, rm, "default", &fakeRouter{}, newObjectStoreCache(), vpc("my-vpc", "10.1.0.0/16")) + got := cmd.simulateOne(ctx, kube, rm, "default", &fakeRouter{}, newObjectStoreCache(), newCRDDefaults(), vpc("my-vpc", "10.1.0.0/16")) if !got.Approximate || got.Action != plan.ActionUpdate { t.Errorf("simulateOne(...): want an approximate update fallback, got %+v", got) @@ -231,18 +231,97 @@ func TestSimulateOne(t *testing.T) { kube := fake.NewClientBuilder().WithScheme(s).WithObjects(live).Build() pc := &fakePlanClient{err: notSupportedErr(t)} - got := cmd.simulateOne(ctx, kube, rm, "default", &fakeRouter{client: pc}, newObjectStoreCache(), vpc("my-vpc", "10.1.0.0/16")) + got := cmd.simulateOne(ctx, kube, rm, "default", &fakeRouter{client: pc}, newObjectStoreCache(), newCRDDefaults(), vpc("my-vpc", "10.1.0.0/16")) if !got.Approximate || got.Action != plan.ActionUpdate { t.Errorf("simulateOne(...): want an approximate fallback on DIFF_COMPUTATION_NOT_SUPPORTED, got %+v", got) } }) + t.Run("FallbackAppliesCRDDefaults", func(t *testing.T) { + // The live resource carries fields the API server defaulted on + // admission (deletionPolicy, managementPolicies) that the manifest + // omits. Applying the manifest would default them right back, so the + // fallback must not plan them as removals. + crdGVK := schema.GroupVersionKind{Group: "apiextensions.k8s.io", Version: "v1", Kind: "CustomResourceDefinition"} + + defScheme := runtime.NewScheme() + defScheme.AddKnownTypeWithName(vpcGVK, &unstructured.Unstructured{}) + defScheme.AddKnownTypeWithName(vpcGVK.GroupVersion().WithKind("VPCList"), &unstructured.UnstructuredList{}) + defScheme.AddKnownTypeWithName(crdGVK, &unstructured.Unstructured{}) + defScheme.AddKnownTypeWithName(crdGVK.GroupVersion().WithKind("CustomResourceDefinitionList"), &unstructured.UnstructuredList{}) + + crd := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "apiextensions.k8s.io/v1", + "kind": "CustomResourceDefinition", + "metadata": map[string]any{"name": "vpcs.ec2.aws.upbound.io"}, + "spec": map[string]any{ + "group": "ec2.aws.upbound.io", + "names": map[string]any{"kind": "VPC", "listKind": "VPCList", "plural": "vpcs", "singular": "vpc"}, + "scope": "Cluster", + "versions": []any{map[string]any{ + "name": "v1beta1", + "served": true, "storage": true, + "schema": map[string]any{"openAPIV3Schema": map[string]any{ + "type": "object", + "properties": map[string]any{ + "spec": map[string]any{ + "type": "object", + "properties": map[string]any{ + "deletionPolicy": map[string]any{"type": "string", "default": "Delete"}, + "managementPolicies": map[string]any{"type": "array", "items": map[string]any{"type": "string"}, "default": []any{"*"}}, + "forProvider": map[string]any{"type": "object", "properties": map[string]any{"cidrBlock": map[string]any{"type": "string"}}}, + }, + }, + }, + }}, + }}, + }, + }} + + live := vpc("my-vpc", "10.0.0.0/16") + _ = unstructured.SetNestedField(live.Object, "Delete", "spec", "deletionPolicy") + _ = unstructured.SetNestedStringSlice(live.Object, []string{"*"}, "spec", "managementPolicies") + + crdGets := 0 + kube := fake.NewClientBuilder().WithScheme(defScheme).WithObjects(live, crd). + WithInterceptorFuncs(interceptor.Funcs{Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if obj.GetObjectKind().GroupVersionKind().Kind == "CustomResourceDefinition" { + crdGets++ + } + return c.Get(ctx, key, obj, opts...) + }}).Build() + + crds := newCRDDefaults() + got := cmd.simulateOne(ctx, kube, rm, "default", &fakeRouter{}, newObjectStoreCache(), crds, vpc("my-vpc", "10.1.0.0/16")) + + if !got.Approximate || got.Action != plan.ActionUpdate { + t.Fatalf("simulateOne(...): want an approximate update fallback, got %+v", got) + } + want := []plan.FieldChange{{ + Path: "spec.forProvider.cidrBlock", + Actual: &plan.Value{Value: "10.0.0.0/16"}, + Planned: &plan.Value{Value: "10.1.0.0/16"}, + }} + if diff := cmp.Diff(want, got.Changes); diff != "" { + t.Errorf("simulateOne(...): want only the real change, no removals of defaulted fields, -want, +got:\n%s", diff) + } + + // An identical manifest plans as a no-op, and the CRD is served from + // the cache rather than fetched again. + if again := cmd.simulateOne(ctx, kube, rm, "default", &fakeRouter{}, newObjectStoreCache(), crds, vpc("my-vpc", "10.0.0.0/16")); again.Action != plan.ActionNoOp { + t.Errorf("simulateOne(...): want a no-op for a manifest omitting only defaulted fields, got %+v", again) + } + if crdGets != 1 { + t.Errorf("simulateOne(...): want 1 CRD fetch for 2 resources of the same kind, got %d", crdGets) + } + }) + t.Run("ServerErrorBecomesResourceError", func(t *testing.T) { kube := fake.NewClientBuilder().WithScheme(s).Build() pc := &fakePlanClient{err: status.Error(codes.Internal, "boom")} - got := cmd.simulateOne(ctx, kube, rm, "default", &fakeRouter{client: pc}, newObjectStoreCache(), vpc("my-vpc", "10.1.0.0/16")) + got := cmd.simulateOne(ctx, kube, rm, "default", &fakeRouter{client: pc}, newObjectStoreCache(), newCRDDefaults(), vpc("my-vpc", "10.1.0.0/16")) if got.Error == "" { t.Errorf("simulateOne(...): want a per-resource error, got %+v", got) @@ -283,7 +362,7 @@ func TestSimulateOne(t *testing.T) { kube := fake.NewClientBuilder().WithScheme(seedScheme).WithObjects(pc, secret).Build() pc2 := &fakePlanClient{rsp: &diffv1alpha1.PlanResponse{Action: diffv1alpha1.Action_ACTION_CREATE}} - got := cmd.simulateOne(ctx, kube, seedRM, "default", &fakeRouter{client: pc2}, newObjectStoreCache(), vpc("my-vpc", "10.0.0.0/16")) + got := cmd.simulateOne(ctx, kube, seedRM, "default", &fakeRouter{client: pc2}, newObjectStoreCache(), newCRDDefaults(), vpc("my-vpc", "10.0.0.0/16")) if got.Error != "" { t.Fatalf("simulateOne(...): unexpected error: %s", got.Error) @@ -411,7 +490,7 @@ func TestSimulateOne(t *testing.T) { emptyRM := meta.NewDefaultRESTMapper(nil) pc := &fakePlanClient{rsp: &diffv1alpha1.PlanResponse{Action: diffv1alpha1.Action_ACTION_CREATE}} - got := cmd.simulateOne(ctx, kube, emptyRM, "default", &fakeRouter{client: pc}, newObjectStoreCache(), vpc("my-vpc", "10.0.0.0/16")) + got := cmd.simulateOne(ctx, kube, emptyRM, "default", &fakeRouter{client: pc}, newObjectStoreCache(), newCRDDefaults(), vpc("my-vpc", "10.0.0.0/16")) if got.Action != plan.ActionCreate || got.Error != "" { t.Errorf("simulateOne(...): want a create for an unserved kind, got %+v", got) diff --git a/go.mod b/go.mod index fbd3db8b..504796fb 100644 --- a/go.mod +++ b/go.mod @@ -57,6 +57,7 @@ require ( k8s.io/utils v0.0.0-20260319190234-28399d86e0b5 sigs.k8s.io/controller-runtime v0.23.3 sigs.k8s.io/kind v0.30.0 + sigs.k8s.io/structured-merge-diff/v6 v6.3.2-0.20260122202528-d9cc6641c482 sigs.k8s.io/yaml v1.6.0 ) @@ -325,5 +326,4 @@ require ( sigs.k8s.io/kustomize/api v0.20.1 // indirect sigs.k8s.io/kustomize/kyaml v0.20.1 // indirect sigs.k8s.io/randfill v1.0.0 // indirect - sigs.k8s.io/structured-merge-diff/v6 v6.3.2-0.20260122202528-d9cc6641c482 // indirect ) diff --git a/internal/plan/fallback.go b/internal/plan/fallback.go index f3baa652..5827a53e 100644 --- a/internal/plan/fallback.go +++ b/internal/plan/fallback.go @@ -24,13 +24,19 @@ import ( "slices" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" ) // Fallback computes an approximate client-side plan for desired by comparing // its spec against the live resource's spec. It is used when no diff server // can plan the resource, and is honest about its limits: no replacement -// detection, no computed values, no provider defaults. live nil means the -// resource does not exist and plans as a create. +// detection, no computed values, no provider defaults. It diffs desired +// verbatim, so callers should apply the API server's CRD defaults to desired +// first when they can. Live-only fields plan as removals only when live's +// managedFields say a declarative applier owns them: anything else - fields +// the provider late-initialized, or API server defaults owned by nobody - +// survives an apply of the manifest. live nil means the resource does not +// exist and plans as a create. func Fallback(desired, live *unstructured.Unstructured, reason string) *ResourcePlan { p := NewResourcePlan(desired) p.Approximate = true @@ -44,8 +50,16 @@ func Fallback(desired, live *unstructured.Unstructured, reason string) *Resource return p } + // Ownership decisions compare against the set's leaves: a leaf is either a + // plainly owned field or a whole atomic subtree, while a partially owned + // parent map keeps its children and never matches. + owned := applierOwned(live) + if owned != nil { + owned = owned.Leaves() + } + liveSpec, _, _ := unstructured.NestedMap(live.Object, "spec") - p.Changes = diffMaps("spec", liveSpec, desiredSpec) + p.Changes = diffMaps(newCursor("spec"), liveSpec, desiredSpec, owned) if len(p.Changes) == 0 { p.Action = ActionNoOp return p @@ -54,46 +68,82 @@ func Fallback(desired, live *unstructured.Unstructured, reason string) *Resource return p } +// cursor tracks a position in the diffed object: the rendered path for the +// plan's output, and the structured field path for ownership lookups in a +// managedFields set. The two diverge on purpose: the rendered path is lossy +// (a map key may itself contain dots or brackets) and indexes list elements, +// while FieldsV1 sets never index lists positionally, so below a list the +// field path freezes at the list itself and ownership is decided there. +type cursor struct { + str string + fp fieldpath.Path + inList bool +} + +func newCursor(root string) cursor { + return cursor{str: root, fp: fieldpath.MakePathOrDie(root)} +} + +func (c cursor) field(k string) cursor { + next := cursor{str: c.str + "." + k, fp: c.fp, inList: c.inList} + if !c.inList { + next.fp = append(slices.Clip(c.fp), fieldpath.PathElement{FieldName: &k}) + } + return next +} + +func (c cursor) index(i int) cursor { + return cursor{str: fmt.Sprintf("%s[%d]", c.str, i), fp: c.fp, inList: true} +} + +// removable reports whether a removal at the cursor would really happen on +// apply: with no ownership information every removal counts, otherwise only +// applier-owned fields do. +func (c cursor) removable(owned *fieldpath.Set) bool { + return owned == nil || ownedLeaf(owned, c.fp) +} + // createChanges flattens a desired map into planned-only field changes, so a // create lists what it would set. func createChanges(path string, m map[string]any) []FieldChange { out := make([]FieldChange, 0, len(m)) for _, k := range slices.Sorted(maps.Keys(m)) { - out = append(out, flattenValue(path+"."+k, m[k], false)...) + out = append(out, flattenValue(newCursor(path).field(k), m[k], false, nil)...) } return out } // flattenValue emits one planned-only (or actual-only, for removals) change -// per leaf under path. -func flattenValue(path string, v any, removed bool) []FieldChange { +// per leaf under the cursor. Removals outside the applier-owned set stay +// silent: applying the manifest would not remove them. +func flattenValue(c cursor, v any, removed bool, owned *fieldpath.Set) []FieldChange { switch t := v.(type) { case map[string]any: out := make([]FieldChange, 0, len(t)) for _, k := range slices.Sorted(maps.Keys(t)) { - out = append(out, flattenValue(path+"."+k, t[k], removed)...) + out = append(out, flattenValue(c.field(k), t[k], removed, owned)...) } return out case []any: out := make([]FieldChange, 0, len(t)) for i, e := range t { - out = append(out, flattenValue(fmt.Sprintf("%s[%d]", path, i), e, removed)...) + out = append(out, flattenValue(c.index(i), e, removed, owned)...) } return out default: - fc := FieldChange{Path: path} if removed { - fc.Actual = &Value{Value: v} - } else { - fc.Planned = &Value{Value: v} + if !c.removable(owned) { + return nil + } + return []FieldChange{{Path: c.str, Actual: &Value{Value: v}}} } - return []FieldChange{fc} + return []FieldChange{{Path: c.str, Planned: &Value{Value: v}}} } } // diffMaps compares two maps recursively and returns the changes that turn -// actual into planned, with paths rooted at path. -func diffMaps(path string, actual, planned map[string]any) []FieldChange { +// actual into planned, with paths rooted at the cursor. +func diffMaps(c cursor, actual, planned map[string]any, owned *fieldpath.Set) []FieldChange { var out []FieldChange keys := map[string]struct{}{} for k := range actual { @@ -103,26 +153,26 @@ func diffMaps(path string, actual, planned map[string]any) []FieldChange { keys[k] = struct{}{} } for _, k := range slices.Sorted(maps.Keys(keys)) { - kp := path + "." + k + kc := c.field(k) av, aok := actual[k] pv, pok := planned[k] switch { case !aok: - out = append(out, flattenValue(kp, pv, false)...) + out = append(out, flattenValue(kc, pv, false, owned)...) case !pok: - out = append(out, flattenValue(kp, av, true)...) + out = append(out, flattenValue(kc, av, true, owned)...) default: - out = append(out, diffValues(kp, av, pv)...) + out = append(out, diffValues(kc, av, pv, owned)...) } } return out } -func diffValues(path string, actual, planned any) []FieldChange { +func diffValues(c cursor, actual, planned any, owned *fieldpath.Set) []FieldChange { am, aok := actual.(map[string]any) pm, pok := planned.(map[string]any) if aok && pok { - return diffMaps(path, am, pm) + return diffMaps(c, am, pm, owned) } as, aok := actual.([]any) @@ -130,7 +180,7 @@ func diffValues(path string, actual, planned any) []FieldChange { if aok && pok && len(as) == len(ps) { var out []FieldChange for i := range as { - out = append(out, diffValues(fmt.Sprintf("%s[%d]", path, i), as[i], ps[i])...) + out = append(out, diffValues(c.index(i), as[i], ps[i], owned)...) } return out } @@ -139,7 +189,7 @@ func diffValues(path string, actual, planned any) []FieldChange { return nil } return []FieldChange{{ - Path: path, + Path: c.str, Actual: &Value{Value: actual}, Planned: &Value{Value: planned}, }} diff --git a/internal/plan/fallback_test.go b/internal/plan/fallback_test.go index 8382c680..530d5854 100644 --- a/internal/plan/fallback_test.go +++ b/internal/plan/fallback_test.go @@ -21,6 +21,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) @@ -33,10 +34,20 @@ func mr(spec map[string]any) *unstructured.Unstructured { }} } +func managedBy(manager string, op metav1.ManagedFieldsOperationType, fields string) metav1.ManagedFieldsEntry { + return metav1.ManagedFieldsEntry{ + Manager: manager, + Operation: op, + FieldsType: "FieldsV1", + FieldsV1: &metav1.FieldsV1{Raw: []byte(fields)}, + } +} + func TestFallback(t *testing.T) { type args struct { - desired *unstructured.Unstructured - live *unstructured.Unstructured + desired *unstructured.Unstructured + live *unstructured.Unstructured + managedFields []metav1.ManagedFieldsEntry } type want struct { action Action @@ -149,6 +160,85 @@ func TestFallback(t *testing.T) { }}, }, }, + "LateInitializedFieldsAreNotRemovals": { + reason: "Live-only fields owned by the provider (late-initialized) or by nobody (API server defaults) survive an apply and must not plan as removals.", + args: args{ + desired: mr(map[string]any{"forProvider": map[string]any{"cidrBlock": "10.0.0.0/16"}}), + live: mr(map[string]any{ + "forProvider": map[string]any{"cidrBlock": "10.0.0.0/16", "instanceTenancy": "default"}, + "deletionPolicy": "Delete", + }), + managedFields: []metav1.ManagedFieldsEntry{ + managedBy("composed", metav1.ManagedFieldsOperationApply, `{"f:spec":{"f:forProvider":{"f:cidrBlock":{}}}}`), + managedBy("provider", metav1.ManagedFieldsOperationUpdate, `{"f:spec":{"f:forProvider":{"f:instanceTenancy":{}}}}`), + }, + }, + want: want{action: ActionNoOp}, + }, + "CoOwnedFieldsAreNotRemovals": { + reason: "A field the applier and another manager both own survives the apply - Kubernetes retains it while any owner remains - so omitting it is not a removal.", + args: args{ + desired: mr(map[string]any{"forProvider": map[string]any{"cidrBlock": "10.0.0.0/16"}}), + live: mr(map[string]any{"forProvider": map[string]any{ + "cidrBlock": "10.0.0.0/16", + "instanceTenancy": "default", + }}), + managedFields: []metav1.ManagedFieldsEntry{ + managedBy("composed", metav1.ManagedFieldsOperationApply, `{"f:spec":{"f:forProvider":{"f:cidrBlock":{},"f:instanceTenancy":{}}}}`), + managedBy("provider", metav1.ManagedFieldsOperationUpdate, `{"f:spec":{"f:forProvider":{"f:instanceTenancy":{}}}}`), + }, + }, + want: want{action: ActionNoOp}, + }, + "ApplierOwnedFieldsAreRemovals": { + reason: "A live-only field the applier owns really is removed when the manifest omits it.", + args: args{ + desired: mr(map[string]any{"forProvider": map[string]any{"cidrBlock": "10.0.0.0/16"}}), + live: mr(map[string]any{"forProvider": map[string]any{ + "cidrBlock": "10.0.0.0/16", + "enableDnsHostnames": true, + }}), + managedFields: []metav1.ManagedFieldsEntry{ + managedBy("composed", metav1.ManagedFieldsOperationApply, `{"f:spec":{"f:forProvider":{"f:cidrBlock":{},"f:enableDnsHostnames":{}}}}`), + }, + }, + want: want{ + action: ActionUpdate, + changes: []FieldChange{{Path: "spec.forProvider.enableDnsHostnames", Actual: &Value{Value: true}}}, + }, + }, + "ClientSideApplyCountsAsApplier": { + reason: "kubectl's client-side apply records operation Update, but its fields are declarative intent and their removal is real.", + args: args{ + desired: mr(map[string]any{"forProvider": map[string]any{}}), + live: mr(map[string]any{"forProvider": map[string]any{"cidrBlock": "10.0.0.0/16"}}), + managedFields: []metav1.ManagedFieldsEntry{ + managedBy("kubectl-client-side-apply", metav1.ManagedFieldsOperationUpdate, `{"f:spec":{"f:forProvider":{"f:cidrBlock":{}}}}`), + }, + }, + want: want{ + action: ActionUpdate, + changes: []FieldChange{{Path: "spec.forProvider.cidrBlock", Actual: &Value{Value: "10.0.0.0/16"}}}, + }, + }, + "AtomicListOwnershipCoversElements": { + reason: "FieldsV1 owns atomic lists as a whole, so element removals below an applier-owned list are real, and ones below a provider-owned list are not.", + args: args{ + desired: mr(map[string]any{"forProvider": map[string]any{}}), + live: mr(map[string]any{ + "forProvider": map[string]any{"securityGroupIds": []any{"sg-1"}}, + "managementPolicies": []any{"Observe"}, + }), + managedFields: []metav1.ManagedFieldsEntry{ + managedBy("composed", metav1.ManagedFieldsOperationApply, `{"f:spec":{"f:managementPolicies":{}}}`), + managedBy("provider", metav1.ManagedFieldsOperationUpdate, `{"f:spec":{"f:forProvider":{"f:securityGroupIds":{}}}}`), + }, + }, + want: want{ + action: ActionUpdate, + changes: []FieldChange{{Path: "spec.managementPolicies[0]", Actual: &Value{Value: "Observe"}}}, + }, + }, "ListLengthChange": { reason: "Lists of different lengths diff as one whole-list change.", args: args{ @@ -168,6 +258,10 @@ func TestFallback(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { + if len(tc.args.managedFields) > 0 { + tc.args.live.SetManagedFields(tc.args.managedFields) + } + got := Fallback(tc.args.desired, tc.args.live, "test") if !got.Approximate { diff --git a/internal/plan/managedfields.go b/internal/plan/managedfields.go new file mode 100644 index 00000000..7916ae5b --- /dev/null +++ b/internal/plan/managedfields.go @@ -0,0 +1,75 @@ +/* +Copyright 2026 The Crossplane Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package plan + +import ( + "bytes" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "sigs.k8s.io/structured-merge-diff/v6/fieldpath" +) + +// applierOwned returns the union of live's field sets owned by declarative +// appliers - server-side apply managers, plus kubectl's client-side apply - +// minus the fields other managers own too. Applying a manifest only removes +// fields its applier owns exclusively: a live-only field outside this set - +// late-initialized by the provider, defaulted by the API server and owned by +// nobody, or co-owned by another manager that keeps it alive - survives the +// apply and must not plan as a removal. nil means ownership is unknown (no +// managedFields, or ones that cannot be parsed) and removals should be taken +// at face value. +func applierOwned(live *unstructured.Unstructured) *fieldpath.Set { + if live == nil { + return nil + } + entries := live.GetManagedFields() + if len(entries) == 0 { + return nil + } + + owned := fieldpath.NewSet() + others := fieldpath.NewSet() + for _, e := range entries { + if e.Subresource != "" || e.FieldsV1 == nil { + continue + } + s := fieldpath.NewSet() + if err := s.FromJSON(bytes.NewReader(e.FieldsV1.Raw)); err != nil { + return nil + } + if e.Operation == metav1.ManagedFieldsOperationApply || e.Manager == "kubectl-client-side-apply" { + owned = owned.Union(s) + } else { + others = others.Union(s) + } + } + return owned.Difference(others) +} + +// ownedLeaf reports whether leaves - the leaf members of an applier-owned +// field set - contains fp or a prefix of it. An exact leaf is a plainly owned +// field; a prefix leaf means the applier owns that whole subtree, the way +// FieldsV1 records atomic lists and maps. +func ownedLeaf(leaves *fieldpath.Set, fp fieldpath.Path) bool { + for i := 1; i <= len(fp); i++ { + if leaves.Has(fp[:i]) { + return true + } + } + return false +} diff --git a/internal/plan/output.go b/internal/plan/output.go index 3e91b386..8391645c 100644 --- a/internal/plan/output.go +++ b/internal/plan/output.go @@ -214,9 +214,11 @@ func symbol(p *ResourcePlan) string { func renderChangeTree(w io.Writer, changes []FieldChange) { var prev []string for _, c := range sortedChanges(changes) { - if c.Actual == nil && c.Planned == nil { + if c.Actual == nil && c.Planned == nil && !c.RequiresReplace { // No value on either side renders no leaf line, so printing its - // parent keys would leave them dangling with nothing beneath. + // parent keys would leave them dangling with nothing beneath. A + // change that forces replacement is the exception: hiding it would + // leave a replace with nothing that explains it. continue } segs := strings.Split(c.Path, ".") @@ -247,6 +249,8 @@ func renderChangeTree(w io.Writer, changes []FieldChange) { _, _ = fmt.Fprintf(w, "%s+ %s: %s%s\n", indent, leaf, formatValue(c.Planned), note) case c.Actual != nil: _, _ = fmt.Fprintf(w, "%s- %s: %s%s\n", indent, leaf, formatValue(c.Actual), note) + default: + _, _ = fmt.Fprintf(w, "%s! %s: (value not reported)%s\n", indent, leaf, note) } } } @@ -265,9 +269,24 @@ func renderSummary(w io.Writer, plans []*ResourcePlan) { _, _ = fmt.Fprintln(w, line+".") if s.Replace > 0 { - fields := replaceFields(plans) + named := 0 + for _, p := range plans { + if p.Error == "" && p.Action == ActionReplace && len(p.ReplacePaths) > 0 { + named++ + } + } _, _ = fmt.Fprintln(w) - _, _ = fmt.Fprintf(w, "Warning: %s force replacement: %s.\n", pluralize(s.Replace, "resource has fields that", "resources have fields that"), strings.Join(fields, ", ")) + if named > 0 { + _, _ = fmt.Fprintf(w, "Warning: %s force replacement: %s.\n", pluralize(named, "resource has fields that", "resources have fields that"), strings.Join(replaceFields(plans), ", ")) + } + if unnamed := s.Replace - named; unnamed > 0 { + // The provider decided on a replacement without flagging any + // reported change as the cause. That happens when the forcing + // change is one its plan does not report as a field: a collection + // growing or shrinking, or a parameter behind an unresolved + // Secret reference. + _, _ = fmt.Fprintf(w, "Warning: %s replacement, but the provider did not report which field forces it.\n", pluralize(unnamed, "resource requires", "resources require")) + } _, _ = fmt.Fprintln(w, "Crossplane providers never destroy and recreate to apply a change.") _, _ = fmt.Fprintln(w, "Applying this leaves the resource unsynced until it is replaced manually.") } diff --git a/internal/plan/output_test.go b/internal/plan/output_test.go index 64ad4f6f..1ece5f76 100644 --- a/internal/plan/output_test.go +++ b/internal/plan/output_test.go @@ -117,6 +117,72 @@ Warning: 1 resource could not be planned. } } +func TestRenderPlanReplaceWithoutReportedCause(t *testing.T) { + // A diff server can decide on a replacement without flagging any reported + // change as the cause (the forcing change can be a collection size or an + // unresolved Secret-backed parameter, which it does not report as fields). + // The warning must say so instead of listing nothing. A forcing change + // with no value on either side still renders, so the cause is never + // silently dropped from the tree. + plans := []*ResourcePlan{{ + APIVersion: "containerservice.azure.upbound.io/v1beta2", Kind: "KubernetesCluster", Name: "my-aks", + Action: ActionReplace, + Changes: []FieldChange{ + {Path: "spec.forProvider.windowsProfile.adminPassword", Actual: &Value{Sensitive: true}, Planned: &Value{Unresolved: true}}, + {Path: "spec.forProvider.defaultNodePool.kubeletConfig", RequiresReplace: true}, + }, + ReplacePaths: []string{"spec.forProvider.defaultNodePool.kubeletConfig"}, + }, { + APIVersion: "containerservice.azure.upbound.io/v1beta2", Kind: "KubernetesCluster", Name: "other-aks", + Action: ActionReplace, + Changes: []FieldChange{ + {Path: "spec.forProvider.windowsProfile.adminPassword", Actual: &Value{Sensitive: true}, Planned: &Value{Unresolved: true}}, + }, + }} + + want := `[-/+] KubernetesCluster.containerservice.azure.upbound.io my-aks (replace) + spec: + forProvider: + defaultNodePool: + ! kubeletConfig: (value not reported) # forces replacement + windowsProfile: + ~ adminPassword: (sensitive value) -> (unresolved reference) + +[-/+] KubernetesCluster.containerservice.azure.upbound.io other-aks (replace) + spec: + forProvider: + windowsProfile: + ~ adminPassword: (sensitive value) -> (unresolved reference) + +Plan: 0 to add, 0 to change, 0 to destroy, 2 require replacement. + +Warning: 1 resource has fields that force replacement: kubeletConfig. +Warning: 1 resource requires replacement, but the provider did not report which field forces it. +Crossplane providers never destroy and recreate to apply a change. +Applying this leaves the resource unsynced until it is replaced manually. +` + + var buf bytes.Buffer + r, _ := NewRenderer(FormatPlan) + if err := r.Render(&buf, plans); err != nil { + t.Fatalf("Render(plan): unexpected error: %v", err) + } + if diff := cmp.Diff(want, buf.String()); diff != "" { + t.Errorf("Render(plan): -want, +got:\n%s", diff) + } + + // With no replace path reported anywhere, the warning says the provider + // did not name the forcing field rather than listing an empty set. + var buf2 bytes.Buffer + if err := r.Render(&buf2, plans[1:]); err != nil { + t.Fatalf("Render(plan): unexpected error: %v", err) + } + wantWarning := "Warning: 1 resource requires replacement, but the provider did not report which field forces it.\n" + if !bytes.Contains(buf2.Bytes(), []byte(wantWarning)) { + t.Errorf("Render(plan): want output to contain %q, got:\n%s", wantWarning, buf2.String()) + } +} + func TestNewRendererUnknownFormat(t *testing.T) { if _, err := NewRenderer("diff"); err == nil { t.Errorf("NewRenderer(diff): want an error for an unsupported format") diff --git a/pkg/xr/xrd.go b/pkg/xr/xrd.go index 64c6a407..e8657ca1 100644 --- a/pkg/xr/xrd.go +++ b/pkg/xr/xrd.go @@ -64,6 +64,10 @@ func ApplyCRDDefaults(xr map[string]any, apiVersion string, crd extv1.CustomReso return errors.Errorf("the specified API version '%s' does not exist in the XRD", apiVersion) } + if version.Schema == nil || version.Schema.OpenAPIV3Schema == nil { + return errors.Errorf("the specified API version '%s' has no schema", apiVersion) + } + if err := extv1.Convert_v1_JSONSchemaProps_To_apiextensions_JSONSchemaProps(version.Schema.OpenAPIV3Schema, &k, nil); err != nil { return err }