diff --git a/cmd/crossplane/project/integration_plan_output_test.go b/cmd/crossplane/project/integration_plan_output_test.go index dbed8e77..2f7de251 100644 --- a/cmd/crossplane/project/integration_plan_output_test.go +++ b/cmd/crossplane/project/integration_plan_output_test.go @@ -458,6 +458,118 @@ func TestSimulatePinnedChangedFunction(t *testing.T) { } } +// TestSimulatePinnedClaimChangedFunction is TestSimulatePinnedChangedFunction +// for a Manual claim of a legacy cluster-scoped XR: the pin lives on the +// claim's backing XR, and the claim is rendered against it (or left deferred) +// just as that XR would be. +func TestSimulatePinnedClaimChangedFunction(t *testing.T) { + t.Parallel() + + const pinned = "testdata/pinned-claim/" + + cases := map[string]struct { + reason string + // composition is the cluster's copy of the project's Composition, + // whose current revision the backing XR is pinned to. + composition string + // wantDeferred is whether the claim stays deferred; otherwise it is + // rendered against its backing XR's pinned revision. + wantDeferred bool + }{ + "PinnedClaimWithChangedFunctionRendersPinnedRevision": { + reason: "A Manual claim whose backing XR's pinned revision calls a changed function is rendered against that revision, not deferred.", + composition: "testdata/project/embedded-claim/apis/composition.yaml", + }, + "PinnedClaimWithUnchangedFunctionDeferred": { + reason: "A Manual claim whose backing XR's pinned revision calls no changed function stays deferred.", + composition: pinned + "unchanged-composition.yaml", + wantDeferred: true, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + t.Parallel() + + if reason := integrationEnvGap(); reason != "" { + t.Skip(reason) + } + + cluster := integration.Start(t, integration.Setup{ + CRDDirs: []string{"../../../internal/testutils/integration/testdata/comp/crds"}, + SetupFiles: []string{ + pinned + "xrd.yaml", + tc.composition, + "../../../internal/testutils/integration/testdata/comp/resources/functions.yaml", + "testdata/project/embedded-claim/cluster-function.yaml", + pinned + "claim.yaml", + }, + CrossplaneManagedResources: []integration.HierarchicalOwnershipRelation{ + integration.Composes(pinned+"xr.yaml", pinned+"downstream.yaml"), + }, + XRDAPIVersion: integration.XRDV1, // the XRD is v1, so Crossplane writes the XR's resourceRefs at spec.resourceRefs + }) + cfg := cluster.Config + + // The harness pins the backing XR to its Composition's current revision. + xr := &un.Unstructured{} + xr.SetAPIVersion("pinned.diff.example.org/v1alpha1") + xr.SetKind("XPinned") + if err := cluster.Client.Get(cluster.Context, client.ObjectKey{Name: "manual-claim-x1y2z"}, xr); err != nil { + t.Fatalf("cannot get the pinned backing XR: %v", err) + } + rev, _, _ := un.NestedString(xr.Object, "spec", "compositionRevisionRef", "name") + + base := simharness.WithRenderBinary([]string{ + "project", "simulate", "--timeout=" + simharness.DefaultTimeout.String(), + "-f", copyProjectFixture(t, "embedded-claim"), + "--cache-dir=" + t.TempDir(), "--skip-plan", "-n", "default", + }, simharness.LocalCrossplaneBinary(t)) + + var stdout, stderr bytes.Buffer + code, err := simharness.RunSimulateArgvWith(t, cfg, append(slices.Clip(base), "-o", "json"), &simulate.Hooks{}, &stdout, &stderr) + if err != nil { + t.Fatalf("\n%s\nsimulate: unexpected error: %v\nstderr:\n%s", tc.reason, err, stderr.String()) + } + var res simulate.Result + if err := json.Unmarshal(stdout.Bytes(), &res); err != nil { + t.Fatalf("\n%s\nsimulate -o json: output is not a simulate.Result: %v\n%s", tc.reason, err, stdout.String()) + } + if len(res.Composites) != 1 { + t.Fatalf("\n%s\nsimulate -o json: want 1 composite, got %+v", tc.reason, res.Composites) + } + c := res.Composites[0] + + if tc.wantDeferred { + if c.Status != simulate.CompositeDeferred || strings.HasPrefix(c.Detail, "rendered against") { + t.Errorf("\n%s\nsimulate -o json: want a deferred pinned claim, got %+v", tc.reason, c) + } + return + } + + if code != dp.ExitCodeDiffDetected { + t.Errorf("\n%s\nsimulate: want exit code %d, got %d", tc.reason, dp.ExitCodeDiffDetected, code) + } + if c.Status != simulate.CompositeChanged || !c.Pinned || c.Detail != "rendered against pinned revision "+rev+" with the changed functions" { + t.Errorf("\n%s\nsimulate -o json: want a changed pinned claim rendered against %s, got %+v", tc.reason, rev, c) + } + // The plan is rendered by the project's build of the function the + // pinned revision calls, which prefixes configData with "embedded-". + var found bool + for _, p := range res.Plans { + for _, ch := range p.Changes { + if ch.Path == "spec.forProvider.configData" && ch.Planned != nil && ch.Planned.Value == "embedded-existing-value" { + found = true + } + } + } + if !found { + t.Errorf("\n%s\nsimulate -o json: want a configData change to the embedded function's output, got plans %+v", tc.reason, res.Plans) + } + }) + } +} + // TestSimulateNameRejected runs the --name scopes simulate refuses: one // naming a composite no supplied Composition reaches, and a namespace/name // combined with --namespace. diff --git a/cmd/crossplane/project/testdata/pinned-claim/claim.yaml b/cmd/crossplane/project/testdata/pinned-claim/claim.yaml new file mode 100644 index 00000000..15b0a59c --- /dev/null +++ b/cmd/crossplane/project/testdata/pinned-claim/claim.yaml @@ -0,0 +1,15 @@ +# A Manual claim bound to xr.yaml. The pin lives on the backing XR alone. +apiVersion: pinned.diff.example.org/v1alpha1 +kind: Pinned +metadata: + name: manual-claim + namespace: default +spec: + coolField: existing-value + compositionUpdatePolicy: Manual + compositionRef: + name: xpinneds.pinned.diff.example.org + resourceRef: + apiVersion: pinned.diff.example.org/v1alpha1 + kind: XPinned + name: manual-claim-x1y2z diff --git a/cmd/crossplane/project/testdata/pinned-claim/downstream.yaml b/cmd/crossplane/project/testdata/pinned-claim/downstream.yaml new file mode 100644 index 00000000..3dd73eea --- /dev/null +++ b/cmd/crossplane/project/testdata/pinned-claim/downstream.yaml @@ -0,0 +1,10 @@ +# The composed resource of xr.yaml. +apiVersion: nop.example.org/v1alpha1 +kind: XDownstreamResource +metadata: + name: manual-claim-x1y2z + annotations: + crossplane.io/composition-resource-name: nop-resource +spec: + forProvider: + configData: existing-value diff --git a/cmd/crossplane/project/testdata/pinned-claim/unchanged-composition.yaml b/cmd/crossplane/project/testdata/pinned-claim/unchanged-composition.yaml new file mode 100644 index 00000000..3903a6fd --- /dev/null +++ b/cmd/crossplane/project/testdata/pinned-claim/unchanged-composition.yaml @@ -0,0 +1,32 @@ +# The cluster's Composition when its pipeline calls no function the project changed. +apiVersion: apiextensions.crossplane.io/v1 +kind: Composition +metadata: + name: xpinneds.pinned.diff.example.org +spec: + compositeTypeRef: + apiVersion: pinned.diff.example.org/v1alpha1 + kind: XPinned + mode: Pipeline + pipeline: + - step: generate-resources + functionRef: + name: function-go-templating + input: + apiVersion: template.fn.crossplane.io/v1beta1 + kind: GoTemplate + source: Inline + inline: + template: | + apiVersion: nop.example.org/v1alpha1 + kind: XDownstreamResource + metadata: + name: {{ .observed.composite.resource.metadata.name }} + annotations: + gotemplating.fn.crossplane.io/composition-resource-name: nop-resource + spec: + forProvider: + configData: {{ .observed.composite.resource.spec.coolField }} + - step: automatically-detect-ready-composed-resources + functionRef: + name: function-auto-ready diff --git a/cmd/crossplane/project/testdata/pinned-claim/xr.yaml b/cmd/crossplane/project/testdata/pinned-claim/xr.yaml new file mode 100644 index 00000000..3cac95ea --- /dev/null +++ b/cmd/crossplane/project/testdata/pinned-claim/xr.yaml @@ -0,0 +1,15 @@ +# The backing XR of claim.yaml. The harness pins it to the Composition's current revision. +apiVersion: pinned.diff.example.org/v1alpha1 +kind: XPinned +metadata: + name: manual-claim-x1y2z +spec: + coolField: existing-value + compositionUpdatePolicy: Manual + compositionRef: + name: xpinneds.pinned.diff.example.org + claimRef: + apiVersion: pinned.diff.example.org/v1alpha1 + kind: Pinned + name: manual-claim + namespace: default diff --git a/cmd/crossplane/project/testdata/pinned-claim/xrd.yaml b/cmd/crossplane/project/testdata/pinned-claim/xrd.yaml new file mode 100644 index 00000000..57de4e01 --- /dev/null +++ b/cmd/crossplane/project/testdata/pinned-claim/xrd.yaml @@ -0,0 +1,26 @@ +# A legacy cluster-scoped XRD (v1, with claimNames) for the pinned-claim project simulate cases. +apiVersion: apiextensions.crossplane.io/v1 +kind: CompositeResourceDefinition +metadata: + name: xpinneds.pinned.diff.example.org +spec: + group: pinned.diff.example.org + names: + kind: XPinned + plural: xpinneds + claimNames: + kind: Pinned + plural: pinneds + versions: + - name: v1alpha1 + served: true + referenceable: true + schema: + openAPIV3Schema: + type: object + properties: + spec: + type: object + properties: + coolField: + type: string diff --git a/cmd/crossplane/project/testdata/project/embedded-claim/apis/composition.yaml b/cmd/crossplane/project/testdata/project/embedded-claim/apis/composition.yaml new file mode 100644 index 00000000..4c24f7ca --- /dev/null +++ b/cmd/crossplane/project/testdata/project/embedded-claim/apis/composition.yaml @@ -0,0 +1,16 @@ +apiVersion: apiextensions.crossplane.io/v1 +kind: Composition +metadata: + name: xpinneds.pinned.diff.example.org +spec: + compositeTypeRef: + apiVersion: pinned.diff.example.org/v1alpha1 + kind: XPinned + mode: Pipeline + pipeline: + - step: generate-resources + functionRef: + name: example-itprojectrender + - step: automatically-detect-ready-composed-resources + functionRef: + name: function-auto-ready diff --git a/cmd/crossplane/project/testdata/project/embedded-claim/cluster-function.yaml b/cmd/crossplane/project/testdata/project/embedded-claim/cluster-function.yaml new file mode 100644 index 00000000..42b38287 --- /dev/null +++ b/cmd/crossplane/project/testdata/project/embedded-claim/cluster-function.yaml @@ -0,0 +1,9 @@ +--- +# A cluster Function with the same name as the project's embedded function. The +# project's function must win; this one has no step input, so go-templating errors if it is used. +apiVersion: pkg.crossplane.io/v1beta1 +kind: Function +metadata: + name: example-itprojectrender +spec: + package: xpkg.crossplane.io/crossplane-contrib/function-go-templating:v0.11.0 diff --git a/cmd/crossplane/project/testdata/project/embedded-claim/crossplane-project.yaml b/cmd/crossplane/project/testdata/project/embedded-claim/crossplane-project.yaml new file mode 100644 index 00000000..b227feb7 --- /dev/null +++ b/cmd/crossplane/project/testdata/project/embedded-claim/crossplane-project.yaml @@ -0,0 +1,6 @@ +apiVersion: dev.crossplane.io/v1alpha1 +kind: Project +metadata: + name: it-project +spec: + repository: xpkg.crossplane.io/example/itproject diff --git a/cmd/crossplane/project/testdata/project/embedded-claim/functions/render/template.gotmpl b/cmd/crossplane/project/testdata/project/embedded-claim/functions/render/template.gotmpl new file mode 100644 index 00000000..81120499 --- /dev/null +++ b/cmd/crossplane/project/testdata/project/embedded-claim/functions/render/template.gotmpl @@ -0,0 +1,9 @@ +apiVersion: nop.example.org/v1alpha1 +kind: XDownstreamResource +metadata: + name: {{ .observed.composite.resource.metadata.name }} + annotations: + gotemplating.fn.crossplane.io/composition-resource-name: nop-resource +spec: + forProvider: + configData: embedded-{{ .observed.composite.resource.spec.coolField }} diff --git a/internal/diff/diffprocessor/comp_processor.go b/internal/diff/diffprocessor/comp_processor.go index 53c2970f..2cd08c07 100644 --- a/internal/diff/diffprocessor/comp_processor.go +++ b/internal/diff/diffprocessor/comp_processor.go @@ -501,7 +501,8 @@ func (p *DefaultCompDiffProcessor) processSingleComposition(ctx context.Context, // A dropped backing XR is reported under its claim, so it does not re-point on its own. Nor does its // pin apply to the claim's render, which is handed the backing XR (see collectXRDiffs): the claim's - // spec, policy included, is what Crossplane syncs into that XR. + // spec, policy included, is what Crossplane syncs into that XR, so only the claim's own pin, resolved + // for that synced XR, does. maps.DeleteFunc(repointingXRs, func(key string, _ bool) bool { return !slices.ContainsFunc(keptXRs, isKey(key)) }) maps.DeleteFunc(pinnedXRs, func(key string, _ pinnedResolution) bool { return !slices.ContainsFunc(keptXRs, isKey(key)) }) @@ -655,8 +656,8 @@ func objectRef(res *un.Unstructured) corev1.ObjectReference { // another kind) in scope, read from the binding Crossplane records on either side (see managedBy), so the // change is reported once, under the claim. A filtered XR is removed whenever its claim is in scope. A // kept XR is removed only when its claim is kept too: one whose claim is filtered, such as a Manual claim -// whose backing XR is pinned to a revision calling a changed function, is still rendered, since nothing -// else reports it. +// naming a revision that calls no changed function while its backing XR is still pinned to one that +// does, is still rendered, since nothing else reports it. func withoutClaimedXRs(kept []*un.Unstructured, dropped []filteredXR, xrKind string) ([]*un.Unstructured, []filteredXR) { boundIn := func(claims []*un.Unstructured) func(*un.Unstructured) bool { return func(xr *un.Unstructured) bool { @@ -707,7 +708,8 @@ func overlapGroups(composites []renderedComposite) []dt.XRDiffGroup { // cliCompositionRevision for which composites are rendered with that name. // // pinned, keyed by dt.MakeDiffKeyFromResource, holds the composites rendered against their own pinned -// revision instead of cliComp; see pinnedFunctionChangeXRs. +// revision instead of cliComp, each with the key of the composite its render resolves the composition +// for; see pinnedFunctionChangeXRs. func (p *DefaultCompDiffProcessor) collectXRDiffs(ctx context.Context, xrs []*un.Unstructured, cliComp *apiextensionsv1.Composition, revisionName string, revisionLabels map[string]string, pinned map[string]pinnedResolution) map[string]*XRDiffResult { // Extract the target GVK from the CLI composition's compositeTypeRef cliCompTargetAPIVersion := cliComp.Spec.CompositeTypeRef.APIVersion @@ -729,6 +731,15 @@ func (p *DefaultCompDiffProcessor) collectXRDiffs(ctx context.Context, xrs []*un rootResourceKeys[key] = true } + // The provider is handed the composite the pin was resolved for, which for a claim is its backing + // XR, so it looks pins up under that composite's key. + pinnedComposites := make(map[string]dtypes.ResolvedComposition, len(pinned)) + for _, pin := range pinned { + if pin.err == nil { + pinnedComposites[pin.composite] = pin.resolved + } + } + // Composition provider that returns CLI composition for: // 1. Root-level resources (XRs and Claims that use this composition) // 2. XRs whose type matches the CLI composition's compositeTypeRef @@ -746,12 +757,12 @@ func (p *DefaultCompDiffProcessor) collectXRDiffs(ctx context.Context, xrs []*un // Check 1: Is this a root-level resource (XR or Claim supplied as affectedXRs to this composition)? // Root-level resources always use the CLI composition, even claims whose GVK differs from the XR type. key := dt.MakeDiffKeyFromResource(res) - if pin, ok := pinned[key]; ok { + if pin, ok := pinnedComposites[key]; ok { p.config.Logger.Debug("Resource is pinned to a revision calling a changed function, using that revision", "resource", resourceID, - "revision", pin.resolved.RevisionName) + "revision", pin.RevisionName) - return pin.resolved, nil + return pin, nil } if rootResourceKeys[key] { @@ -825,6 +836,10 @@ func (p *DefaultCompDiffProcessor) collectXRDiffs(ctx context.Context, xrs []*un type pinnedResolution struct { resolved dtypes.ResolvedComposition err error + + // composite is the dt.MakeDiffKeyFromResource of the composite the render resolves the composition + // for: the XR itself or, for a claim, its backing XR (see DiffProcessor.ResolveComposite). + composite string } // anyResolved reports whether any pinned lookup succeeded. @@ -844,6 +859,9 @@ func anyResolved(m map[string]pinnedResolution) bool { // revision instead of the diffed composition. It returns those XRs' resolutions (keyed by // dt.MakeDiffKeyFromResource), kept with the moved XRs appended, and dropped without them. // +// The revision is looked up for the composite the render resolves the composition for: the XR itself +// or, for a Manual claim, its backing XR with the claim synced in, whose pin is the claim's. +// // It does nothing when no function changed or under IncludeManual, which keeps Manual XRs and renders // them against the diffed composition. A Manual XR whose revision calls no changed function stays dropped. // A failed lookup is that XR's own error (a kept XR whose result is the error), not the run's. So with @@ -865,8 +883,14 @@ func (p *DefaultCompDiffProcessor) pinnedFunctionChangeXRs(ctx context.Context, continue } - resolved, err := p.compositionClient.FindMatchingComposition(ctx, d.xr) - if err == nil && !pinnedRevisionCallsChangedFunction(d.xr, resolved, p.config.ChangedFunctions) { + var resolved dtypes.ResolvedComposition + + composite, err := p.xrProc.ResolveComposite(ctx, d.xr) + if err == nil { + resolved, err = p.compositionClient.FindMatchingComposition(ctx, composite) + } + + if err == nil && !pinnedRevisionCallsChangedFunction(composite, resolved, p.config.ChangedFunctions) { remaining = append(remaining, d) continue } @@ -875,19 +899,23 @@ func (p *DefaultCompDiffProcessor) pinnedFunctionChangeXRs(ctx context.Context, pinned = make(map[string]pinnedResolution) } - pinned[dt.MakeDiffKeyFromResource(d.xr)] = pinnedResolution{resolved: resolved, err: errors.Wrapf(err, "cannot resolve the revision %s is pinned to", d.xr.GetName())} + pin := pinnedResolution{resolved: resolved, err: errors.Wrapf(err, "cannot resolve the revision %s is pinned to", d.xr.GetName())} + if err == nil { + pin.composite = dt.MakeDiffKeyFromResource(composite) + } + + pinned[dt.MakeDiffKeyFromResource(d.xr)] = pin kept = append(kept, d.xr) } return pinned, kept, remaining } -// pinnedRevisionCallsChangedFunction reports whether resolved is the pinned revision of xr and calls a -// changed function. A resolution with no revision name (the composition itself) or for another type -// (a claim, whose backing XR the render looks up under a different key) is not. -// TODO(jcogilvie): a pinned claim stays deferred, so a legacy cluster-scoped XR's claim pinned to a -// revision that calls a changed function is missed; route the backing XR's key to support it. -func pinnedRevisionCallsChangedFunction(xr *un.Unstructured, resolved dtypes.ResolvedComposition, changed sets.Set[string]) bool { +// pinnedRevisionCallsChangedFunction reports whether resolved is the pinned revision of composite and +// calls a changed function. composite is the one the render resolves the composition for, so for a +// claim it is the backing XR, whose type the revision's compositeTypeRef names. A resolution with no +// revision name (the composition itself) or for another type is not. +func pinnedRevisionCallsChangedFunction(composite *un.Unstructured, resolved dtypes.ResolvedComposition, changed sets.Set[string]) bool { if resolved.Composition == nil || resolved.RevisionName == "" { return false } @@ -896,7 +924,7 @@ func pinnedRevisionCallsChangedFunction(xr *un.Unstructured, resolved dtypes.Res ref := resolved.Composition.Spec.CompositeTypeRef refGV, err := schema.ParseGroupVersion(ref.APIVersion) - if err != nil || refGV.Group != xr.GroupVersionKind().Group || ref.Kind != xr.GetKind() { + if err != nil || refGV.Group != composite.GroupVersionKind().Group || ref.Kind != composite.GetKind() { return false } diff --git a/internal/diff/diffprocessor/comp_processor_test.go b/internal/diff/diffprocessor/comp_processor_test.go index ce35cf26..ef87f1b5 100644 --- a/internal/diff/diffprocessor/comp_processor_test.go +++ b/internal/diff/diffprocessor/comp_processor_test.go @@ -2355,6 +2355,22 @@ func TestDefaultCompDiffProcessor_processSingleComposition_PinnedXRWithChangedFu Build() } + // A Manual claim, whose render resolves the composition for its backing XR (see + // DiffProcessor.ResolveComposite): the revision's compositeTypeRef names the XR's kind, not the claim's. + claim := tu.NewResource("example.org/v1", "Claim", "pinned").InNamespace("ns"). + WithSpecField("crossplane", map[string]any{ + "compositionUpdatePolicy": "Manual", + "compositionRevisionRef": map[string]any{"name": "test-comp-rev1"}, + }). + Build() + resolveComposite := func(_ context.Context, res *un.Unstructured) (*un.Unstructured, error) { + if res.GetKind() == "Claim" { + return pinnedXR("pinned-xyz"), nil + } + + return res, nil + } + // The pinned revision's pipeline has a step the CLI composition lacks, so a render that saw the // CLI composition instead is detectable. revisionWith := func(fn string) *apiextensionsv1.Composition { @@ -2380,6 +2396,7 @@ func TestDefaultCompDiffProcessor_processSingleComposition_PinnedXRWithChangedFu lookupErr error includeManual bool unrelated bool + claim bool want want }{ "PinnedXRWithChangedFunctionRendersPinnedRevision": { @@ -2390,6 +2407,18 @@ func TestDefaultCompDiffProcessor_processSingleComposition_PinnedXRWithChangedFu revision: revisionWith("function-other"), want: want{Status: dt.XRStatusFiltered, FilterReason: dt.FilterReasonManualPolicy}, }, + // Issue #516: the revision is checked against the backing XR's type, and the render, handed the + // backing XR, finds the pin under its key. + "PinnedClaimWithChangedFunctionRendersPinnedRevision": { + revision: revisionWith("function-a"), + claim: true, + want: want{Status: dt.XRStatusUnchanged, Pinned: true, RenderedRevision: "test-comp-rev1", Pipeline: []string{"function-a"}}, + }, + "PinnedClaimWithUnrelatedFunctionChangeStaysDeferred": { + revision: revisionWith("function-other"), + claim: true, + want: want{Status: dt.XRStatusFiltered, FilterReason: dt.FilterReasonManualPolicy}, + }, "PinnedXRWithChangedFunctionRendersWhenCompositionCallsNone": { revision: revisionWith("function-a"), unrelated: true, @@ -2444,8 +2473,15 @@ func TestDefaultCompDiffProcessor_processSingleComposition_PinnedXRWithChangedFu ) proc.xrProc = &tu.MockDiffProcessor{ + ResolveCompositeFn: resolveComposite, + // Like the real one, the provider is handed the effective composite. DiffSingleResourceFn: func(ctx context.Context, xr *un.Unstructured, provider types.CompositionProvider) (map[string]*dt.ResourceDiff, map[string]*dt.ResourceDiff, error) { - resolved, err := provider(ctx, xr) + composite, err := resolveComposite(ctx, xr) + if err != nil { + return nil, nil, err + } + + resolved, err := provider(ctx, composite) if err != nil { return nil, nil, err } @@ -2459,7 +2495,12 @@ func TestDefaultCompDiffProcessor_processSingleComposition_PinnedXRWithChangedFu }, } - got, _, err := proc.processSingleComposition(t.Context(), cliComp.BuildAsUnstructured(), []*un.Unstructured{pinnedXR("pinned")}, false) + input := pinnedXR("pinned") + if tt.claim { + input = claim + } + + got, _, err := proc.processSingleComposition(t.Context(), cliComp.BuildAsUnstructured(), []*un.Unstructured{input}, false) if err != nil { t.Fatalf("processSingleComposition: %v", err) } @@ -2491,9 +2532,9 @@ func TestDefaultCompDiffProcessor_processSingleComposition_PinnedXRWithChangedFu } // TestDefaultCompDiffProcessor_processSingleComposition_PinnedBackingXROfFilteredClaim pins that the -// backing-XR drop runs after the pinned step: a Manual claim stays filtered (a pinned claim is -// deferred), so its backing XR, pinned to a revision calling a changed function, is the only entry that -// reports that change and must still be rendered against its pinned revision. +// backing-XR drop runs after the pinned step: a Manual claim naming a revision that calls no changed +// function stays filtered, so its backing XR, still pinned in the cluster to a revision calling a changed +// function, is the only entry that reports that change and must still be rendered against that revision. func TestDefaultCompDiffProcessor_processSingleComposition_PinnedBackingXROfFilteredClaim(t *testing.T) { cliComp := tu.NewComposition("test-comp"). WithCompositeTypeRef("example.org/v1", "XR"). @@ -2502,20 +2543,26 @@ func TestDefaultCompDiffProcessor_processSingleComposition_PinnedBackingXROfFilt manual := map[string]any{"compositionUpdatePolicy": "Manual", "compositionRevisionRef": map[string]any{"name": "test-comp-rev1"}} claim := tu.NewResource("example.org/v1", "Claim", "my-claim").InNamespace("ns"). - WithSpecField("crossplane", manual).Build() + WithSpecField("crossplane", map[string]any{"compositionUpdatePolicy": "Manual", "compositionRevisionRef": map[string]any{"name": "test-comp-rev0"}}).Build() backing := tu.NewResource("example.org/v1", "XR", "my-claim-xyz"). WithSpecField("compositionRef", map[string]any{"name": "test-comp"}). WithSpecField("crossplane", manual). WithSpecField("claimRef", map[string]any{"apiVersion": "example.org/v1", "kind": "Claim", "name": "my-claim", "namespace": "ns"}). Build() - revision := tu.NewComposition("test-comp").WithCompositeTypeRef("example.org/v1", "XR").WithPipelineMode(). - WithPipelineStep("step-old", "function-a", nil).Build() + // Revision 1 calls the changed function; revision 0, which the claim names, does not. + revisions := map[string]*apiextensionsv1.Composition{ + "test-comp-rev0": tu.NewComposition("test-comp").WithCompositeTypeRef("example.org/v1", "XR").WithPipelineMode(). + WithPipelineStep("step-old", "function-other", nil).Build(), + "test-comp-rev1": tu.NewComposition("test-comp").WithCompositeTypeRef("example.org/v1", "XR").WithPipelineMode(). + WithPipelineStep("step-old", "function-a", nil).Build(), + } client := tu.NewMockCompositionClient(). WithSuccessfulCompositionFetch(cliComp.Build()). - WithFindMatchingComposition(func(context.Context, *un.Unstructured) (types.ResolvedComposition, error) { - return types.ResolvedComposition{Composition: revision, RevisionName: "test-comp-rev1"}, nil + WithFindMatchingComposition(func(_ context.Context, res *un.Unstructured) (types.ResolvedComposition, error) { + name, _, _ := un.NestedString(res.Object, "spec", "crossplane", "compositionRevisionRef", "name") + return types.ResolvedComposition{Composition: revisions[name], RevisionName: name}, nil }). Build() @@ -2524,6 +2571,17 @@ func TestDefaultCompDiffProcessor_processSingleComposition_PinnedBackingXROfFilt rendered := map[string]string{} proc.xrProc = &tu.MockDiffProcessor{ + // The claim's effective composite is its backing XR with the claim's revision synced in. + ResolveCompositeFn: func(_ context.Context, res *un.Unstructured) (*un.Unstructured, error) { + if res.GetKind() != "Claim" { + return res, nil + } + + synced := backing.DeepCopy() + _ = un.SetNestedField(synced.Object, "test-comp-rev0", "spec", "crossplane", "compositionRevisionRef", "name") + + return synced, nil + }, DiffSingleResourceFn: func(ctx context.Context, xr *un.Unstructured, provider types.CompositionProvider) (map[string]*dt.ResourceDiff, map[string]*dt.ResourceDiff, error) { resolved, err := provider(ctx, xr) if err != nil { diff --git a/internal/diff/diffprocessor/diff_processor.go b/internal/diff/diffprocessor/diff_processor.go index 5ff00b9e..f52482bc 100644 --- a/internal/diff/diffprocessor/diff_processor.go +++ b/internal/diff/diffprocessor/diff_processor.go @@ -56,6 +56,10 @@ type DiffProcessor interface { // which only the render-overlap check consumes. DiffSingleResource(ctx context.Context, res *un.Unstructured, compositionProvider types.CompositionProvider) (map[string]*dt.ResourceDiff, map[string]*dt.ResourceDiff, error) + // ResolveComposite returns the composite DiffSingleResource hands its composition provider for res: + // the defaulted effective composite (see CompositeResolver), which for a claim is its backing XR. + ResolveComposite(ctx context.Context, res *un.Unstructured) (*un.Unstructured, error) + // Initialize prepares the processor's providers before the first diff. Initialize(ctx context.Context) error @@ -365,6 +369,46 @@ func (p *DefaultDiffProcessor) DiffSingleResource(ctx context.Context, res *un.U return diffs, rendered.Unchanged(), err } +// ResolveComposite returns the defaulted effective composite of res; see DiffProcessor. +func (p *DefaultDiffProcessor) ResolveComposite(ctx context.Context, res *un.Unstructured) (*un.Unstructured, error) { + resourceID := fmt.Sprintf("%s/%s", res.GetKind(), res.GetName()) + + authoredXR, _, err := p.SanitizeXR(res, resourceID) + if err != nil { + return nil, err + } + + _, defaultedXR, err := p.resolveComposite(ctx, authoredXR, nil, resourceID) + + return defaultedXR, err +} + +// resolveComposite returns the effective composite of authoredXR and its defaulted effective XR, from +// which the composition is resolved and which render consumes. existing is as for Resolve. +func (p *DefaultDiffProcessor) resolveComposite(ctx context.Context, authoredXR *cmp.Unstructured, existing *un.Unstructured, resourceID string) (EffectiveComposite, *un.Unstructured, error) { + // What Crossplane would actually reconcile: the input over its cluster copy or, for a claim, its + // backing XR with the claim synced in. Computed before anything reads the composite's composition, + // revision or update policy, so that resolving the composition and rendering agree on them. + composite, err := p.compositeResolver.Resolve(ctx, authoredXR, existing) + if err != nil { + p.config.Logger.Debug("Failed to resolve the effective composite", "resource", resourceID, "error", err) + return EffectiveComposite{}, nil, errors.Wrap(err, "cannot resolve the effective composite") + } + + // Default the effective XR the way the apiserver would, so that composition resolution and render + // see the spec Crossplane would see. This follows inheriting the cluster's fields, so that a default + // never overrides a value the cluster already holds (a Manual policy, say). The policy is strict: + // rendering without those defaults could produce a wrong diff. The storage version it reports is + // only for describing a predicted addition (see predictLocally), so it is not wanted here. + defaultedXR, _, err := p.renderDefaulter.Default(ctx, composite.Effective) + if err != nil { + p.config.Logger.Debug("Failed to apply XRD defaults", "resource", resourceID, "error", err) + return EffectiveComposite{}, nil, errors.Wrap(err, "cannot apply XRD defaults") + } + + return composite, defaultedXR, nil +} + // diffSingleResourceInternal is the internal implementation that allows control over removal detection. // parentXR should be nil for root XRs, and the parent XR for nested XRs. // existing is res's cluster copy when the caller already has it (a nested XR found among its parent's @@ -386,28 +430,13 @@ func (p *DefaultDiffProcessor) diffSingleResourceInternal(ctx context.Context, r return nil, types.RenderedResources{}, err } - // What Crossplane would actually reconcile: the input over its cluster copy or, for a claim, its - // backing XR with the claim synced in. Computed before anything reads the composite's composition, - // revision or update policy, so that resolving the composition and rendering agree on them. - composite, err := p.compositeResolver.Resolve(ctx, authoredXR, existing) + composite, defaultedXR, err := p.resolveComposite(ctx, authoredXR, existing, resourceID) if err != nil { - p.config.Logger.Debug("Failed to resolve the effective composite", "resource", resourceID, "error", err) - return nil, types.RenderedResources{}, errors.Wrap(err, "cannot resolve the effective composite") + return nil, types.RenderedResources{}, err } p.warnIfDeleting(composite.Cluster, parentXR, resourceID) - // Default the effective XR the way the apiserver would, so that composition resolution and render - // see the spec Crossplane would see. This follows inheriting the cluster's fields, so that a default - // never overrides a value the cluster already holds (a Manual policy, say). The policy is strict: - // rendering without those defaults could produce a wrong diff. The storage version it reports is - // only for describing a predicted addition (see predictLocally), so it is not wanted here. - defaultedXR, _, err := p.renderDefaulter.Default(ctx, composite.Effective) - if err != nil { - p.config.Logger.Debug("Failed to apply XRD defaults", "resource", resourceID, "error", err) - return nil, types.RenderedResources{}, errors.Wrap(err, "cannot apply XRD defaults") - } - // Get the composition using the provided function. It is resolved from the defaulted effective XR, // so that it sees the revision a composite is pinned to and the update policy an XRD defaults. resolved, err := compositionProvider(ctx, defaultedXR) diff --git a/internal/diff/testutils/mocks.go b/internal/diff/testutils/mocks.go index 761055d5..6bcd364b 100644 --- a/internal/diff/testutils/mocks.go +++ b/internal/diff/testutils/mocks.go @@ -259,9 +259,20 @@ type MockDiffProcessor struct { InitializeFn func(ctx context.Context) error PerformDiffFn func(ctx context.Context, resources []*un.Unstructured, compositionProvider types.CompositionProvider) (*dt.XRReport, error) DiffSingleResourceFn func(ctx context.Context, res *un.Unstructured, compositionProvider types.CompositionProvider) (map[string]*dt.ResourceDiff, map[string]*dt.ResourceDiff, error) + ResolveCompositeFn func(ctx context.Context, res *un.Unstructured) (*un.Unstructured, error) CleanupFn func(ctx context.Context) error } +// ResolveComposite implements the DiffProcessor.ResolveComposite method. Unset, res is its own +// effective composite. +func (m *MockDiffProcessor) ResolveComposite(ctx context.Context, res *un.Unstructured) (*un.Unstructured, error) { + if m.ResolveCompositeFn != nil { + return m.ResolveCompositeFn(ctx, res) + } + + return res, nil +} + // Initialize implements the DiffProcessor interface. func (m *MockDiffProcessor) Initialize(ctx context.Context) error { if m.InitializeFn != nil {