From e2556fb1591ab0bf3ca45bbed2968a2bdfc7fa3d Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Thu, 18 Sep 2025 16:31:21 -0400 Subject: [PATCH 1/2] Fix claim derived-resource lookups by label; fixes #26 Signed-off-by: Jonathan Ogilvie --- cmd/diff/diff_integration_test.go | 37 ++---- .../diffprocessor/diff_calculator_test.go | 30 ++--- cmd/diff/diffprocessor/diff_processor.go | 2 +- cmd/diff/diffprocessor/processor_config.go | 4 +- cmd/diff/diffprocessor/resource_manager.go | 107 ++++++++++++++---- .../diffprocessor/resource_manager_test.go | 4 +- .../existing-claim-downstream-resource.yaml | 8 +- 7 files changed, 121 insertions(+), 71 deletions(-) diff --git a/cmd/diff/diff_integration_test.go b/cmd/diff/diff_integration_test.go index 69e4af1..fff44d2 100644 --- a/cmd/diff/diff_integration_test.go +++ b/cmd/diff/diff_integration_test.go @@ -61,31 +61,6 @@ func TestDiffIntegration(t *testing.T) { _ = pkgv1.AddToScheme(scheme) _ = extv1.AddToScheme(scheme) - // TODO: is there a reason to even run this against v1 if everything is backwards compatible? - // claims are still here (for now). we obviously need to keep tests for those. - // we've already removed the deprecated environmentconfig version. - // I can see running the ITs against v2 since we can test against an old image. important to IT against v1 image - // given changes to move xp specific stuff into spec.crossplane, which will not be reflected in running these tests - - // TODO: add a test to cover v2 CompositeResourceDefinition (XRD) if running against Crossplane v2 - // TODO: add a test to cover namespaced xrds against v2 - // update: these ITs don't run against a version of xp besides what they are compiled against. that'll matter - // in the e2es. - // we'll want to rig up some way to specify xrd-v1 or xrd-v2 or both in the test cases - // but the rub is that the cluster directory containing the crds is pulled from either v1 or v2, so we can't just - // run both. - // thinking we should just grab the cluster directory for v1 and check it in, since it won't advance anymore once - // v2 is out. v2 we can update at build time. every test spins up its own envtest with the crd path, so we can - // definitely toggle there. - - // the CRDs that support the XRDs will vary based on the Crossplane version, though (namespaced vs cluster scoped), - // so we need to bifurcate /testdata/diff/crds accordingly. although each XRD that we define will have a version - // specified inside it which will lead to the generation of that crd. so maybe it's test specific actually. - - // TODO: namespaced XRDs cannot compose cluster-scoped resources, so we need to ensure XDownstreamResource definitions - // account for that. maybe we just need to add parallel CRDs for namespace scoped and cluster scoped XRDs that can - // coexist. - // Test cases tests := map[string]struct { setupFiles []string @@ -1042,16 +1017,20 @@ Summary: 2 added`, + coolField: modified-value --- -~~~ XDownstreamResource/test-claim +~~~ XDownstreamResource/test-claim-82crv apiVersion: nop.example.org/v1alpha1 kind: XDownstreamResource metadata: annotations: crossplane.io/composition-resource-name: nop-resource - generateName: test-claim- +- generateName: test-claim-82crv- labels: - crossplane.io/composite: test-claim - name: test-claim + crossplane.io/claim-name: test-claim + crossplane.io/claim-namespace: existing-namespace +- crossplane.io/composite: test-claim-82crv +- name: test-claim-82crv ++ crossplane.io/composite: test-claim ++ name: test-claim spec: forProvider: - configData: existing-value diff --git a/cmd/diff/diffprocessor/diff_calculator_test.go b/cmd/diff/diffprocessor/diff_calculator_test.go index 81f6c85..ce89714 100644 --- a/cmd/diff/diffprocessor/diff_calculator_test.go +++ b/cmd/diff/diffprocessor/diff_calculator_test.go @@ -83,7 +83,7 @@ func TestDefaultDiffCalculator_CalculateDiff(t *testing.T) { Build() // Create resource manager - resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -113,7 +113,7 @@ func TestDefaultDiffCalculator_CalculateDiff(t *testing.T) { Build() // Create resource manager - resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -144,7 +144,7 @@ func TestDefaultDiffCalculator_CalculateDiff(t *testing.T) { Build() // Create resource manager - resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -182,7 +182,7 @@ func TestDefaultDiffCalculator_CalculateDiff(t *testing.T) { Build() // Create resource manager - resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -212,7 +212,7 @@ func TestDefaultDiffCalculator_CalculateDiff(t *testing.T) { Build() // Create resource manager - resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -238,7 +238,7 @@ func TestDefaultDiffCalculator_CalculateDiff(t *testing.T) { Build() // Create resource manager - resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -309,7 +309,7 @@ func TestDefaultDiffCalculator_CalculateDiff(t *testing.T) { Build() // Create resource manager - resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -455,7 +455,7 @@ func TestDefaultDiffCalculator_CalculateDiffs(t *testing.T) { Build() // Create resource manager - resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -491,7 +491,7 @@ func TestDefaultDiffCalculator_CalculateDiffs(t *testing.T) { Build() // Create resource manager - resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -529,7 +529,7 @@ func TestDefaultDiffCalculator_CalculateDiffs(t *testing.T) { Build() // Create resource manager - resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -572,7 +572,7 @@ func TestDefaultDiffCalculator_CalculateDiffs(t *testing.T) { Build() // Create resource manager - resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -629,7 +629,7 @@ func TestDefaultDiffCalculator_CalculateDiffs(t *testing.T) { Build() // Create resource manager - resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -762,7 +762,7 @@ func TestDefaultDiffCalculator_CalculateRemovedResourceDiffs(t *testing.T) { // Create a resource manager (not directly used in this test) resourceClient := tu.NewMockResourceClient().Build() - resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -791,7 +791,7 @@ func TestDefaultDiffCalculator_CalculateRemovedResourceDiffs(t *testing.T) { // Create a resource manager (not directly used in this test) resourceClient := tu.NewMockResourceClient().Build() - resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -817,7 +817,7 @@ func TestDefaultDiffCalculator_CalculateRemovedResourceDiffs(t *testing.T) { // Create a resource manager (not directly used in this test) resourceClient := tu.NewMockResourceClient().Build() - resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, diff --git a/cmd/diff/diffprocessor/diff_processor.go b/cmd/diff/diffprocessor/diff_processor.go index 5c9f5db..885ce83 100644 --- a/cmd/diff/diffprocessor/diff_processor.go +++ b/cmd/diff/diffprocessor/diff_processor.go @@ -71,7 +71,7 @@ func NewDiffProcessor(k8cs k8.Clients, xpcs xp.Clients, opts ...ProcessorOption) diffOpts := config.GetDiffOptions() // Create components using factories - resourceManager := config.Factories.ResourceManager(k8cs.Resource, config.Logger) + resourceManager := config.Factories.ResourceManager(k8cs.Resource, xpcs.Definition, config.Logger) schemaValidator := config.Factories.SchemaValidator(k8cs.Schema, xpcs.Definition, config.Logger) requirementsProvider := config.Factories.RequirementsProvider(k8cs.Resource, xpcs.Environment, config.RenderFunc, config.Logger) diffCalculator := config.Factories.DiffCalculator(k8cs.Apply, xpcs.ResourceTree, resourceManager, config.Logger, diffOpts) diff --git a/cmd/diff/diffprocessor/processor_config.go b/cmd/diff/diffprocessor/processor_config.go index 1b2ea48..ce57c4d 100644 --- a/cmd/diff/diffprocessor/processor_config.go +++ b/cmd/diff/diffprocessor/processor_config.go @@ -32,7 +32,7 @@ type ProcessorConfig struct { // ComponentFactories contains factory functions for creating processor components. type ComponentFactories struct { // ResourceManager creates a ResourceManager - ResourceManager func(client k8.ResourceClient, logger logging.Logger) ResourceManager + ResourceManager func(client k8.ResourceClient, defClient xp.DefinitionClient, logger logging.Logger) ResourceManager // SchemaValidator creates a SchemaValidator SchemaValidator func(schema k8.SchemaClient, def xp.DefinitionClient, logger logging.Logger) SchemaValidator @@ -86,7 +86,7 @@ func WithRenderFunc(renderFn RenderFunc) ProcessorOption { } // WithResourceManagerFactory sets the ResourceManager factory function. -func WithResourceManagerFactory(factory func(k8.ResourceClient, logging.Logger) ResourceManager) ProcessorOption { +func WithResourceManagerFactory(factory func(k8.ResourceClient, xp.DefinitionClient, logging.Logger) ResourceManager) ProcessorOption { return func(config *ProcessorConfig) { config.Factories.ResourceManager = factory } diff --git a/cmd/diff/diffprocessor/resource_manager.go b/cmd/diff/diffprocessor/resource_manager.go index 97d7d87..8a73641 100644 --- a/cmd/diff/diffprocessor/resource_manager.go +++ b/cmd/diff/diffprocessor/resource_manager.go @@ -5,6 +5,7 @@ import ( "fmt" "strings" + xp "github.com/crossplane-contrib/crossplane-diff/cmd/diff/client/crossplane" k8 "github.com/crossplane-contrib/crossplane-diff/cmd/diff/client/kubernetes" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -28,15 +29,17 @@ type ResourceManager interface { // DefaultResourceManager implements ResourceManager interface. type DefaultResourceManager struct { - client k8.ResourceClient - logger logging.Logger + client k8.ResourceClient + defClient xp.DefinitionClient + logger logging.Logger } // NewResourceManager creates a new DefaultResourceManager. -func NewResourceManager(client k8.ResourceClient, logger logging.Logger) ResourceManager { +func NewResourceManager(client k8.ResourceClient, defClient xp.DefinitionClient, logger logging.Logger) ResourceManager { return &DefaultResourceManager{ - client: client, - logger: logger, + client: client, + defClient: defClient, + logger: logger, } } @@ -186,22 +189,48 @@ func (m *DefaultResourceManager) lookupByComposite(ctx context.Context, composit return nil, false, nil } - // Create a label selector to find resources managed by this composite - labelSelector := metav1.LabelSelector{ - MatchLabels: map[string]string{ - "crossplane.io/composite": composite.GetName(), - }, + // Determine the appropriate label selector based on whether the composite is a claim + var labelSelector metav1.LabelSelector + var lookupName string + isCompositeAClaim := m.isClaimResource(ctx, composite) + + if isCompositeAClaim { + // For claims, we need to find the XR that was created from this claim + // The downstream resources will have labels pointing to that XR + // We'll use the claim labels to find downstream resources + labelSelector = metav1.LabelSelector{ + MatchLabels: map[string]string{ + "crossplane.io/claim-name": composite.GetName(), + "crossplane.io/claim-namespace": composite.GetNamespace(), + }, + } + lookupName = composite.GetName() + m.logger.Debug("Using claim labels for resource lookup", + "claim", composite.GetName(), + "namespace", composite.GetNamespace()) + } else { + // For XRs, use the composite label + labelSelector = metav1.LabelSelector{ + MatchLabels: map[string]string{ + "crossplane.io/composite": composite.GetName(), + }, + } + lookupName = composite.GetName() + m.logger.Debug("Using composite label for resource lookup", + "composite", composite.GetName()) } - // Look up resources with the composite label + // Look up resources with the appropriate label selector resources, err := m.client.GetResourcesByLabel(ctx, gvk, namespace, labelSelector) if err != nil { - return nil, false, errors.Wrapf(err, "cannot list resources for composite %s", composite.GetName()) + return nil, false, errors.Wrapf(err, "cannot list resources for %s %s", + map[bool]string{true: "claim", false: "composite"}[isCompositeAClaim], lookupName) } if len(resources) == 0 { - m.logger.Debug("No resources found with composite owner label", - "composite", composite.GetName()) + m.logger.Debug("No resources found with owner labels", + "lookupName", lookupName, + "isClaimLookup", isCompositeAClaim) return nil, false, nil } @@ -287,6 +316,26 @@ func (m *DefaultResourceManager) hasMatchingResourceName(annotations map[string] return false } +// isClaimResource checks if the resource is a claim type by attempting to find an XRD that defines it as a claim. +func (m *DefaultResourceManager) isClaimResource(ctx context.Context, resource *un.Unstructured) bool { + if m.defClient == nil { + // If no definition client is available, assume it's not a claim + return false + } + + gvk := resource.GroupVersionKind() + + // Try to find an XRD that defines this resource type as a claim + _, err := m.defClient.GetXRDForClaim(ctx, gvk) + if err != nil { + m.logger.Debug("Resource is not a claim type", "gvk", gvk.String(), "error", err) + return false + } + + m.logger.Debug("Resource is a claim type", "gvk", gvk.String()) + return true +} + // UpdateOwnerRefs ensures all OwnerReferences have valid UIDs. func (m *DefaultResourceManager) UpdateOwnerRefs(parent *un.Unstructured, child *un.Unstructured) { // if there's no parent, we are the parent. @@ -349,7 +398,9 @@ func (m *DefaultResourceManager) UpdateOwnerRefs(parent *un.Unstructured, child "newCount", len(updatedRefs)) } -// updateCompositeOwnerLabel updates the crossplane.io/composite label on the child. +// updateCompositeOwnerLabel updates the ownership labels on the child resource. +// For Claims, it sets claim-name and claim-namespace labels. +// For XRs, it sets the composite label. func (m *DefaultResourceManager) updateCompositeOwnerLabel(parent, child *un.Unstructured) { if parent == nil { return @@ -361,18 +412,36 @@ func (m *DefaultResourceManager) updateCompositeOwnerLabel(parent, child *un.Uns labels = make(map[string]string) } - // Set the composite owner label parentName := parent.GetName() if parentName == "" && parent.GetGenerateName() != "" { // For XRs with only generateName, use the generateName prefix parentName = parent.GetGenerateName() } - if parentName != "" { + if parentName == "" { + return + } + + // Check if the parent is a claim + ctx := context.Background() + isParentAClaim := m.isClaimResource(ctx, parent) + + switch { + case isParentAClaim: + // For claims, set claim-specific labels (all claims are namespaced) + labels["crossplane.io/claim-name"] = parentName + labels["crossplane.io/claim-namespace"] = parent.GetNamespace() + m.logger.Debug("Updated claim owner labels", + "claimName", parentName, + "claimNamespace", parent.GetNamespace(), + "child", child.GetName()) + default: + // For XRs, set the composite label labels["crossplane.io/composite"] = parentName - child.SetLabels(labels) m.logger.Debug("Updated composite owner label", - "label", parentName, + "composite", parentName, "child", child.GetName()) } + + child.SetLabels(labels) } diff --git a/cmd/diff/diffprocessor/resource_manager_test.go b/cmd/diff/diffprocessor/resource_manager_test.go index efb9770..723180f 100644 --- a/cmd/diff/diffprocessor/resource_manager_test.go +++ b/cmd/diff/diffprocessor/resource_manager_test.go @@ -265,7 +265,7 @@ func TestDefaultResourceManager_FetchCurrentObject(t *testing.T) { t.Run(name, func(t *testing.T) { // Create the resource manager resourceClient := tt.setupResourceClient() - rm := NewResourceManager(resourceClient, tu.TestLogger(t, false)) + rm := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) // Call the method under test current, isNew, err := rm.FetchCurrentObject(ctx, tt.composite, tt.desired) @@ -435,7 +435,7 @@ func TestDefaultResourceManager_UpdateOwnerRefs(t *testing.T) { for name, tt := range tests { t.Run(name, func(t *testing.T) { // Create the resource manager - rm := NewResourceManager(tu.NewMockResourceClient().Build(), tu.TestLogger(t, false)) + rm := NewResourceManager(tu.NewMockResourceClient().Build(), nil, tu.TestLogger(t, false)) // Need to create a copy of the child to avoid modifying test data child := tt.child.DeepCopy() diff --git a/cmd/diff/testdata/diff/resources/existing-claim-downstream-resource.yaml b/cmd/diff/testdata/diff/resources/existing-claim-downstream-resource.yaml index 0e07989..47a0544 100644 --- a/cmd/diff/testdata/diff/resources/existing-claim-downstream-resource.yaml +++ b/cmd/diff/testdata/diff/resources/existing-claim-downstream-resource.yaml @@ -3,10 +3,12 @@ kind: XDownstreamResource metadata: annotations: crossplane.io/composition-resource-name: nop-resource - generateName: test-claim- + generateName: test-claim-82crv- labels: - crossplane.io/composite: test-claim - name: test-claim + crossplane.io/composite: test-claim-82crv + crossplane.io/claim-name: test-claim + crossplane.io/claim-namespace: existing-namespace + name: test-claim-82crv spec: forProvider: configData: existing-value From 146670c80ea607a1c207a6ef126f4ddba7220b5d Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Thu, 18 Sep 2025 17:33:54 -0400 Subject: [PATCH 2/2] Fix claim derived-resource lookups by label; fixes #26 Signed-off-by: Jonathan Ogilvie --- .../client/crossplane/definition_client.go | 19 +++ .../crossplane/definition_client_test.go | 116 +++++++++++++++ .../diffprocessor/diff_calculator_test.go | 30 ++-- cmd/diff/diffprocessor/diff_processor.go | 4 +- cmd/diff/diffprocessor/resource_manager.go | 24 +--- .../diffprocessor/resource_manager_test.go | 134 +++++++++++++++++- cmd/diff/diffprocessor/schema_validator.go | 20 +-- .../diffprocessor/schema_validator_test.go | 85 ----------- cmd/diff/testutils/mock_builder.go | 6 + cmd/diff/testutils/mocks.go | 26 ++-- 10 files changed, 304 insertions(+), 160 deletions(-) diff --git a/cmd/diff/client/crossplane/definition_client.go b/cmd/diff/client/crossplane/definition_client.go index 8cbce13..529e108 100644 --- a/cmd/diff/client/crossplane/definition_client.go +++ b/cmd/diff/client/crossplane/definition_client.go @@ -28,6 +28,9 @@ type DefinitionClient interface { // GetXRDForXR finds the XRD that defines the given XR type GetXRDForXR(ctx context.Context, gvk schema.GroupVersionKind) (*un.Unstructured, error) + + // IsClaimResource checks if the given resource is a claim type + IsClaimResource(ctx context.Context, resource *un.Unstructured) bool } // DefaultDefinitionClient implements DefinitionClient. @@ -210,3 +213,19 @@ func (c *DefaultDefinitionClient) GetXRDForXR(ctx context.Context, gvk schema.Gr return nil, errors.Errorf("no XRD found that defines XR type %s", gvk.String()) } + +// IsClaimResource checks if the given resource is a claim type by attempting +// to find an XRD that defines it as a claim. +func (c *DefaultDefinitionClient) IsClaimResource(ctx context.Context, resource *un.Unstructured) bool { + gvk := resource.GroupVersionKind() + + // Try to find an XRD that defines this resource type as a claim + _, err := c.GetXRDForClaim(ctx, gvk) + if err != nil { + c.logger.Debug("Resource is not a claim type", "gvk", gvk.String(), "error", err) + return false + } + + c.logger.Debug("Resource is a claim type", "gvk", gvk.String()) + return true +} diff --git a/cmd/diff/client/crossplane/definition_client_test.go b/cmd/diff/client/crossplane/definition_client_test.go index cfbfbbf..ce8798c 100644 --- a/cmd/diff/client/crossplane/definition_client_test.go +++ b/cmd/diff/client/crossplane/definition_client_test.go @@ -626,3 +626,119 @@ func TestDefaultDefinitionClient_Initialize(t *testing.T) { }) } } + +func TestDefaultDefinitionClient_IsClaimResource(t *testing.T) { + ctx := t.Context() + + tests := map[string]struct { + reason string + mockResource tu.MockResourceClient + cachedXRDs []*un.Unstructured + resource *un.Unstructured + expected bool + }{ + "ResourceIsClaim": { + reason: "Should return true when resource is a claim type", + mockResource: *tu.NewMockResourceClient(). + WithSuccessfulInitialize(). + Build(), + cachedXRDs: []*un.Unstructured{ + // Create a mock XRD that defines this as a claim + tu.NewResource("apiextensions.crossplane.io/v1", "CompositeResourceDefinition", "testclaims.example.org"). + WithSpecField("group", "example.org"). + WithSpecField("names", map[string]interface{}{ + "kind": "XTestResource", + "plural": "xtestresources", + "singular": "xtestresource", + }). + WithSpecField("claimNames", map[string]interface{}{ + "kind": "TestClaim", + "plural": "testclaims", + "singular": "testclaim", + }). + Build(), + }, + resource: tu.NewResource("example.org/v1", "TestClaim", "test-claim"). + InNamespace("default"). + Build(), + expected: true, + }, + "ResourceIsNotClaim": { + reason: "Should return false when resource is not a claim type", + mockResource: *tu.NewMockResourceClient(). + WithSuccessfulInitialize(). + Build(), + cachedXRDs: []*un.Unstructured{ + // Create a mock XRD that defines this as an XR (no claimNames) + tu.NewResource("apiextensions.crossplane.io/v1", "CompositeResourceDefinition", "testxrs.example.org"). + WithSpecField("group", "example.org"). + WithSpecField("names", map[string]interface{}{ + "kind": "TestXR", + "plural": "testxrs", + "singular": "testxr", + }). + // No claimNames field + Build(), + }, + resource: tu.NewResource("example.org/v1", "TestXR", "test-xr"). + InNamespace("default"). + Build(), + expected: false, + }, + "GetXRDForClaimError": { + reason: "Should return false when GetXRDForClaim fails", + mockResource: *tu.NewMockResourceClient(). + WithSuccessfulInitialize(). + WithListResourcesFailure("list error"). + Build(), + cachedXRDs: nil, // Force GetXRDs to fail + resource: tu.NewResource("example.org/v1", "TestResource", "test-resource"). + Build(), + expected: false, // Should return false on error + }, + "NoMatchingXRD": { + reason: "Should return false when no matching XRD exists", + mockResource: *tu.NewMockResourceClient(). + WithSuccessfulInitialize(). + Build(), + cachedXRDs: []*un.Unstructured{ + // Create an XRD that doesn't match our resource + tu.NewResource("apiextensions.crossplane.io/v1", "CompositeResourceDefinition", "otherclaims.example.org"). + WithSpecField("group", "example.org"). + WithSpecField("names", map[string]interface{}{ + "kind": "XOtherResource", + "plural": "xotherresources", + "singular": "xotherresource", + }). + WithSpecField("claimNames", map[string]interface{}{ + "kind": "OtherClaim", + "plural": "otherclaims", + "singular": "otherclaim", + }). + Build(), + }, + resource: tu.NewResource("example.org/v1", "TestClaim", "test-claim"). + Build(), + expected: false, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + c := &DefaultDefinitionClient{ + resourceClient: &tt.mockResource, + logger: tu.TestLogger(t, false), + xrds: tt.cachedXRDs, + xrdsLoaded: tt.cachedXRDs != nil, // Only mark as loaded if we have cached XRDs + } + + // Call the function under test + result := c.IsClaimResource(ctx, tt.resource) + + // Check the result + if result != tt.expected { + t.Errorf("\n%s\nIsClaimResource() = %v, want %v", tt.reason, result, tt.expected) + } + }) + } +} diff --git a/cmd/diff/diffprocessor/diff_calculator_test.go b/cmd/diff/diffprocessor/diff_calculator_test.go index ce89714..0bd6074 100644 --- a/cmd/diff/diffprocessor/diff_calculator_test.go +++ b/cmd/diff/diffprocessor/diff_calculator_test.go @@ -83,7 +83,7 @@ func TestDefaultDiffCalculator_CalculateDiff(t *testing.T) { Build() // Create resource manager - resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -113,7 +113,7 @@ func TestDefaultDiffCalculator_CalculateDiff(t *testing.T) { Build() // Create resource manager - resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -144,7 +144,7 @@ func TestDefaultDiffCalculator_CalculateDiff(t *testing.T) { Build() // Create resource manager - resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -182,7 +182,7 @@ func TestDefaultDiffCalculator_CalculateDiff(t *testing.T) { Build() // Create resource manager - resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -212,7 +212,7 @@ func TestDefaultDiffCalculator_CalculateDiff(t *testing.T) { Build() // Create resource manager - resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -238,7 +238,7 @@ func TestDefaultDiffCalculator_CalculateDiff(t *testing.T) { Build() // Create resource manager - resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -309,7 +309,7 @@ func TestDefaultDiffCalculator_CalculateDiff(t *testing.T) { Build() // Create resource manager - resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -455,7 +455,7 @@ func TestDefaultDiffCalculator_CalculateDiffs(t *testing.T) { Build() // Create resource manager - resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -491,7 +491,7 @@ func TestDefaultDiffCalculator_CalculateDiffs(t *testing.T) { Build() // Create resource manager - resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -529,7 +529,7 @@ func TestDefaultDiffCalculator_CalculateDiffs(t *testing.T) { Build() // Create resource manager - resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -572,7 +572,7 @@ func TestDefaultDiffCalculator_CalculateDiffs(t *testing.T) { Build() // Create resource manager - resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -629,7 +629,7 @@ func TestDefaultDiffCalculator_CalculateDiffs(t *testing.T) { Build() // Create resource manager - resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -762,7 +762,7 @@ func TestDefaultDiffCalculator_CalculateRemovedResourceDiffs(t *testing.T) { // Create a resource manager (not directly used in this test) resourceClient := tu.NewMockResourceClient().Build() - resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -791,7 +791,7 @@ func TestDefaultDiffCalculator_CalculateRemovedResourceDiffs(t *testing.T) { // Create a resource manager (not directly used in this test) resourceClient := tu.NewMockResourceClient().Build() - resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, @@ -817,7 +817,7 @@ func TestDefaultDiffCalculator_CalculateRemovedResourceDiffs(t *testing.T) { // Create a resource manager (not directly used in this test) resourceClient := tu.NewMockResourceClient().Build() - resourceManager := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) + resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false)) return applyClient, resourceTreeClient, resourceManager }, diff --git a/cmd/diff/diffprocessor/diff_processor.go b/cmd/diff/diffprocessor/diff_processor.go index 885ce83..bc904c2 100644 --- a/cmd/diff/diffprocessor/diff_processor.go +++ b/cmd/diff/diffprocessor/diff_processor.go @@ -41,6 +41,7 @@ type DiffProcessor interface { type DefaultDiffProcessor struct { fnClient xp.FunctionClient compClient xp.CompositionClient + defClient xp.DefinitionClient config ProcessorConfig schemaValidator SchemaValidator diffCalculator DiffCalculator @@ -80,6 +81,7 @@ func NewDiffProcessor(k8cs k8.Clients, xpcs xp.Clients, opts ...ProcessorOption) processor := &DefaultDiffProcessor{ fnClient: xpcs.Function, compClient: xpcs.Composition, + defClient: xpcs.Definition, config: config, schemaValidator: schemaValidator, diffCalculator: diffCalculator, @@ -222,7 +224,7 @@ func (p *DefaultDiffProcessor) DiffSingleResource(ctx context.Context, res *un.U // WORKAROUND for https://github.com/crossplane/crossplane/issues/6782 // Propagate namespace from XR to namespaced managed resources // For claims (v1 compatibility), skip namespace propagation entirely as all managed resources are cluster-scoped - isClaimRoot := p.schemaValidator.IsClaimResource(ctx, xrUnstructured) + isClaimRoot := p.defClient.IsClaimResource(ctx, xrUnstructured) if !isClaimRoot { p.propagateNamespacesToManagedResources(ctx, xrUnstructured, desired.ComposedResources) } else { diff --git a/cmd/diff/diffprocessor/resource_manager.go b/cmd/diff/diffprocessor/resource_manager.go index 8a73641..1f8e29b 100644 --- a/cmd/diff/diffprocessor/resource_manager.go +++ b/cmd/diff/diffprocessor/resource_manager.go @@ -192,7 +192,7 @@ func (m *DefaultResourceManager) lookupByComposite(ctx context.Context, composit // Determine the appropriate label selector based on whether the composite is a claim var labelSelector metav1.LabelSelector var lookupName string - isCompositeAClaim := m.isClaimResource(ctx, composite) + isCompositeAClaim := m.defClient.IsClaimResource(ctx, composite) if isCompositeAClaim { // For claims, we need to find the XR that was created from this claim @@ -316,26 +316,6 @@ func (m *DefaultResourceManager) hasMatchingResourceName(annotations map[string] return false } -// isClaimResource checks if the resource is a claim type by attempting to find an XRD that defines it as a claim. -func (m *DefaultResourceManager) isClaimResource(ctx context.Context, resource *un.Unstructured) bool { - if m.defClient == nil { - // If no definition client is available, assume it's not a claim - return false - } - - gvk := resource.GroupVersionKind() - - // Try to find an XRD that defines this resource type as a claim - _, err := m.defClient.GetXRDForClaim(ctx, gvk) - if err != nil { - m.logger.Debug("Resource is not a claim type", "gvk", gvk.String(), "error", err) - return false - } - - m.logger.Debug("Resource is a claim type", "gvk", gvk.String()) - return true -} - // UpdateOwnerRefs ensures all OwnerReferences have valid UIDs. func (m *DefaultResourceManager) UpdateOwnerRefs(parent *un.Unstructured, child *un.Unstructured) { // if there's no parent, we are the parent. @@ -424,7 +404,7 @@ func (m *DefaultResourceManager) updateCompositeOwnerLabel(parent, child *un.Uns // Check if the parent is a claim ctx := context.Background() - isParentAClaim := m.isClaimResource(ctx, parent) + isParentAClaim := m.defClient.IsClaimResource(ctx, parent) switch { case isParentAClaim: diff --git a/cmd/diff/diffprocessor/resource_manager_test.go b/cmd/diff/diffprocessor/resource_manager_test.go index 723180f..75d6da5 100644 --- a/cmd/diff/diffprocessor/resource_manager_test.go +++ b/cmd/diff/diffprocessor/resource_manager_test.go @@ -68,6 +68,7 @@ func TestDefaultResourceManager_FetchCurrentObject(t *testing.T) { tests := map[string]struct { setupResourceClient func() *tu.MockResourceClient + defClient *tu.MockDefinitionClient composite *un.Unstructured desired *un.Unstructured wantIsNew bool @@ -80,6 +81,7 @@ func TestDefaultResourceManager_FetchCurrentObject(t *testing.T) { WithResourcesExist(existingResource). Build() }, + defClient: tu.NewMockDefinitionClient().Build(), composite: nil, desired: existingResource.DeepCopy(), wantIsNew: false, @@ -92,6 +94,7 @@ func TestDefaultResourceManager_FetchCurrentObject(t *testing.T) { WithResourceNotFound(). Build() }, + defClient: tu.NewMockDefinitionClient().Build(), composite: nil, desired: tu.NewResource("example.org/v1", "TestResource", "non-existent").Build(), wantIsNew: true, @@ -104,6 +107,7 @@ func TestDefaultResourceManager_FetchCurrentObject(t *testing.T) { WithResourceNotFound(). Build() }, + defClient: tu.NewMockDefinitionClient().Build(), composite: nil, desired: tu.NewResource("example.org/v1", "XR", "new-xr").Build(), wantIsNew: true, @@ -116,6 +120,7 @@ func TestDefaultResourceManager_FetchCurrentObject(t *testing.T) { WithResourceNotFound(). Build() }, + defClient: tu.NewMockDefinitionClient().Build(), composite: nil, desired: resourceWithGenerateName, wantIsNew: true, @@ -144,6 +149,7 @@ func TestDefaultResourceManager_FetchCurrentObject(t *testing.T) { }). Build() }, + defClient: tu.NewMockDefinitionClient().Build(), composite: parentXR, desired: tu.NewResource("example.org/v1", "TestResource", ""). WithLabels(map[string]string{ @@ -172,6 +178,7 @@ func TestDefaultResourceManager_FetchCurrentObject(t *testing.T) { }). Build() }, + defClient: tu.NewMockDefinitionClient().Build(), composite: parentXR, desired: tu.NewResource("example.org/v1", "ComposedResource", "composed-resource"). WithLabels(map[string]string{ @@ -191,6 +198,7 @@ func TestDefaultResourceManager_FetchCurrentObject(t *testing.T) { WithResourceNotFound(). Build() }, + defClient: tu.NewMockDefinitionClient().Build(), composite: parentXR, desired: tu.NewResource("example.org/v1", "Resource", "resource-name"). WithLabels(map[string]string{ @@ -223,6 +231,7 @@ func TestDefaultResourceManager_FetchCurrentObject(t *testing.T) { }). Build() }, + defClient: tu.NewMockDefinitionClient().Build(), composite: parentXR, desired: tu.NewResource("example.org/v1", "TestResource", ""). WithLabels(map[string]string{ @@ -246,6 +255,7 @@ func TestDefaultResourceManager_FetchCurrentObject(t *testing.T) { }). Build() }, + defClient: tu.NewMockDefinitionClient().Build(), composite: parentXR, desired: tu.NewResource("example.org/v1", "ComposedResource", ""). WithLabels(map[string]string{ @@ -259,13 +269,62 @@ func TestDefaultResourceManager_FetchCurrentObject(t *testing.T) { wantIsNew: true, // Fall back to creating a new resource wantErr: false, // We handle the error gracefully }, + "ClaimResource_FoundByClaimLabels": { + setupResourceClient: func() *tu.MockResourceClient { + // Create an existing resource with claim labels + existingClaimResource := tu.NewResource("example.org/v1", "ComposedResource", "claim-managed-resource"). + WithSpecField("field", "value"). + WithLabels(map[string]string{ + "crossplane.io/claim-name": "test-claim", + "crossplane.io/claim-namespace": "test-namespace", + }). + WithAnnotations(map[string]string{ + "crossplane.io/composition-resource-name": "resource-a", + }). + Build() + + return tu.NewMockResourceClient(). + WithResourceNotFound(). // Direct lookup fails + WithGetResourcesByLabel(func(_ context.Context, _ schema.GroupVersionKind, _ string, sel metav1.LabelSelector) ([]*un.Unstructured, error) { + // Check if looking up by claim labels + if claimName, exists := sel.MatchLabels["crossplane.io/claim-name"]; exists && claimName == "test-claim" { + if claimNS, exists := sel.MatchLabels["crossplane.io/claim-namespace"]; exists && claimNS == "test-namespace" { + return []*un.Unstructured{existingClaimResource}, nil + } + } + return []*un.Unstructured{}, nil + }). + Build() + }, + defClient: tu.NewMockDefinitionClient(). + WithIsClaimResource(func(_ context.Context, resource *un.Unstructured) bool { + return resource.GetKind() == "TestClaim" + }). + Build(), + composite: tu.NewResource("example.org/v1", "TestClaim", "test-claim"). + InNamespace("test-namespace"). + Build(), + desired: tu.NewResource("example.org/v1", "ComposedResource", "claim-managed-resource"). + WithLabels(map[string]string{ + "crossplane.io/claim-name": "test-claim", + "crossplane.io/claim-namespace": "test-namespace", + }). + WithAnnotations(map[string]string{ + "crossplane.io/composition-resource-name": "resource-a", + }). + Build(), + wantIsNew: false, + wantResourceID: "claim-managed-resource", + wantErr: false, + }, } for name, tt := range tests { t.Run(name, func(t *testing.T) { // Create the resource manager resourceClient := tt.setupResourceClient() - rm := NewResourceManager(resourceClient, nil, tu.TestLogger(t, false)) + + rm := NewResourceManager(resourceClient, tt.defClient, tu.TestLogger(t, false)) // Call the method under test current, isNew, err := rm.FetchCurrentObject(ctx, tt.composite, tt.desired) @@ -312,15 +371,17 @@ func TestDefaultResourceManager_UpdateOwnerRefs(t *testing.T) { parentXR.SetUID(ParentUID) tests := map[string]struct { - parent *un.Unstructured - child *un.Unstructured - validate func(t *testing.T, child *un.Unstructured) + parent *un.Unstructured + child *un.Unstructured + defClient *tu.MockDefinitionClient + validate func(t *testing.T, child *un.Unstructured) }{ "NilParent_NoChange": { parent: nil, child: tu.NewResource("example.org/v1", "Child", "child-resource"). WithOwnerReference("some-api-version", "SomeKind", "some-name", "foobar"). Build(), + defClient: tu.NewMockDefinitionClient().Build(), validate: func(t *testing.T, child *un.Unstructured) { t.Helper() // Owner refs should be unchanged @@ -342,6 +403,7 @@ func TestDefaultResourceManager_UpdateOwnerRefs(t *testing.T) { child: tu.NewResource("example.org/v1", "Child", "child-resource"). WithOwnerReference("XR", "parent-xr", "example.org/v1", ""). Build(), + defClient: tu.NewMockDefinitionClient().Build(), validate: func(t *testing.T, child *un.Unstructured) { t.Helper() // Owner reference should be updated with parent's UID @@ -359,6 +421,7 @@ func TestDefaultResourceManager_UpdateOwnerRefs(t *testing.T) { child: tu.NewResource("example.org/v1", "Child", "child-resource"). WithOwnerReference("other-api-version", "OtherKind", "other-name", ""). Build(), + defClient: tu.NewMockDefinitionClient().Build(), validate: func(t *testing.T, child *un.Unstructured) { t.Helper() // Owner reference should have a UID, but not parent's UID @@ -403,6 +466,7 @@ func TestDefaultResourceManager_UpdateOwnerRefs(t *testing.T) { return child }(), + defClient: tu.NewMockDefinitionClient().Build(), validate: func(t *testing.T, child *un.Unstructured) { t.Helper() ownerRefs := child.GetOwnerReferences() @@ -430,12 +494,72 @@ func TestDefaultResourceManager_UpdateOwnerRefs(t *testing.T) { } }, }, + "ClaimParent_SetsClaimLabels": { + parent: tu.NewResource("example.org/v1", "TestClaim", "test-claim"). + InNamespace("test-namespace"). + Build(), + child: tu.NewResource("example.org/v1", "Child", "child-resource").Build(), + defClient: tu.NewMockDefinitionClient(). + WithIsClaimResource(func(_ context.Context, resource *un.Unstructured) bool { + return resource.GetKind() == "TestClaim" + }). + Build(), + validate: func(t *testing.T, child *un.Unstructured) { + t.Helper() + labels := child.GetLabels() + if labels == nil { + t.Fatal("Expected labels to be set") + } + + // Check claim-specific labels + if claimName, exists := labels["crossplane.io/claim-name"]; !exists || claimName != "test-claim" { + t.Errorf("Expected crossplane.io/claim-name=test-claim, got %s", claimName) + } + if claimNS, exists := labels["crossplane.io/claim-namespace"]; !exists || claimNS != "test-namespace" { + t.Errorf("Expected crossplane.io/claim-namespace=test-namespace, got %s", claimNS) + } + + // Should not have composite label for claims + if composite, exists := labels["crossplane.io/composite"]; exists { + t.Errorf("Should not have crossplane.io/composite label for claim, but got %s", composite) + } + }, + }, + "XRParent_SetsCompositeLabel": { + parent: tu.NewResource("example.org/v1", "XR", "test-xr").Build(), + child: tu.NewResource("example.org/v1", "Child", "child-resource").Build(), + defClient: tu.NewMockDefinitionClient(). + WithIsClaimResource(func(_ context.Context, resource *un.Unstructured) bool { + return resource.GetKind() == "TestClaim" + }). + Build(), + validate: func(t *testing.T, child *un.Unstructured) { + t.Helper() + labels := child.GetLabels() + if labels == nil { + t.Fatal("Expected labels to be set") + } + + // Check composite label + if composite, exists := labels["crossplane.io/composite"]; !exists || composite != "test-xr" { + t.Errorf("Expected crossplane.io/composite=test-xr, got %s", composite) + } + + // Should not have claim-specific labels for XRs + if claimName, exists := labels["crossplane.io/claim-name"]; exists { + t.Errorf("Should not have crossplane.io/claim-name label for XR, but got %s", claimName) + } + if claimNS, exists := labels["crossplane.io/claim-namespace"]; exists { + t.Errorf("Should not have crossplane.io/claim-namespace label for XR, but got %s", claimNS) + } + }, + }, } for name, tt := range tests { t.Run(name, func(t *testing.T) { // Create the resource manager - rm := NewResourceManager(tu.NewMockResourceClient().Build(), nil, tu.TestLogger(t, false)) + rm := NewResourceManager(tu.NewMockResourceClient().Build(), tt.defClient, tu.TestLogger(t, false)) // Need to create a copy of the child to avoid modifying test data child := tt.child.DeepCopy() diff --git a/cmd/diff/diffprocessor/schema_validator.go b/cmd/diff/diffprocessor/schema_validator.go index 1b35f27..7d79924 100644 --- a/cmd/diff/diffprocessor/schema_validator.go +++ b/cmd/diff/diffprocessor/schema_validator.go @@ -31,9 +31,6 @@ type SchemaValidator interface { // ValidateScopeConstraints validates that a resource has the appropriate namespace for its scope // and that namespaced resources match the expected namespace (no cross-namespace refs) ValidateScopeConstraints(ctx context.Context, resource *un.Unstructured, expectedNamespace string, isClaimRoot bool) error - - // IsClaimResource checks if the root resource is a claim type - IsClaimResource(ctx context.Context, resource *un.Unstructured) bool } // DefaultSchemaValidator implements SchemaValidator interface. @@ -126,7 +123,7 @@ func (v *DefaultSchemaValidator) ValidateResources(ctx context.Context, xr *un.U // Additionally validate resource scope constraints (namespace requirements and cross-namespace refs) expectedNamespace := xr.GetNamespace() - isClaimRoot := v.IsClaimResource(ctx, xr) + isClaimRoot := v.defClient.IsClaimResource(ctx, xr) v.logger.Debug("Performing resource scope validation", "resourceCount", len(resources), "expectedNamespace", expectedNamespace, "isClaimRoot", isClaimRoot) for _, resource := range resources { if err := v.ValidateScopeConstraints(ctx, resource, expectedNamespace, isClaimRoot); err != nil { @@ -288,18 +285,3 @@ func (v *DefaultSchemaValidator) ValidateScopeConstraints(ctx context.Context, r return nil } - -// IsClaimResource checks if the resource is a claim type by attempting to find an XRD that defines it as a claim. -func (v *DefaultSchemaValidator) IsClaimResource(ctx context.Context, resource *un.Unstructured) bool { - gvk := resource.GroupVersionKind() - - // Try to find an XRD that defines this resource type as a claim - _, err := v.defClient.GetXRDForClaim(ctx, gvk) - if err != nil { - v.logger.Debug("Resource is not a claim type", "gvk", gvk.String(), "error", err) - return false - } - - v.logger.Debug("Resource is a claim type", "gvk", gvk.String()) - return true -} diff --git a/cmd/diff/diffprocessor/schema_validator_test.go b/cmd/diff/diffprocessor/schema_validator_test.go index e07d169..0b7b018 100644 --- a/cmd/diff/diffprocessor/schema_validator_test.go +++ b/cmd/diff/diffprocessor/schema_validator_test.go @@ -608,88 +608,3 @@ func TestDefaultSchemaValidator_ValidateScopeConstraints(t *testing.T) { }) } } - -func TestDefaultSchemaValidator_IsClaimResource(t *testing.T) { - ctx := t.Context() - - tests := map[string]struct { - setupClient func() *tu.MockDefinitionClient - resource *un.Unstructured - expected bool - }{ - "ResourceIsClaim": { - setupClient: func() *tu.MockDefinitionClient { - // Create a mock XRD that defines this as a claim - claimXRD := tu.NewResource("apiextensions.crossplane.io/v1", "CompositeResourceDefinition", "testclaims.example.org"). - WithSpecField("group", "example.org"). - WithSpecField("names", map[string]interface{}{ - "kind": "TestClaim", - "plural": "testclaims", - "singular": "testclaim", - }). - WithSpecField("claimNames", map[string]interface{}{ - "kind": "TestClaim", - "plural": "testclaims", - "singular": "testclaim", - }). - Build() - - return tu.NewMockDefinitionClient(). - WithGetXRDForClaim(func(_ context.Context, gvk schema.GroupVersionKind) (*un.Unstructured, error) { - if gvk.Group == "example.org" && gvk.Kind == "TestClaim" { - return claimXRD, nil - } - return nil, errors.New("no XRD found that defines claim type") - }). - Build() - }, - resource: tu.NewResource("example.org/v1", "TestClaim", "test-claim"). - InNamespace("default"). - Build(), - expected: true, - }, - "ResourceIsNotClaim": { - setupClient: func() *tu.MockDefinitionClient { - return tu.NewMockDefinitionClient(). - WithGetXRDForClaim(func(_ context.Context, _ schema.GroupVersionKind) (*un.Unstructured, error) { - return nil, errors.New("no XRD found that defines claim type") - }). - Build() - }, - resource: tu.NewResource("example.org/v1", "TestXR", "test-xr"). - InNamespace("default"). - Build(), - expected: false, - }, - "GetXRDForClaimError": { - setupClient: func() *tu.MockDefinitionClient { - return tu.NewMockDefinitionClient(). - WithGetXRDForClaim(func(_ context.Context, _ schema.GroupVersionKind) (*un.Unstructured, error) { - return nil, errors.New("failed to fetch XRDs") - }). - Build() - }, - resource: tu.NewResource("example.org/v1", "TestResource", "test-resource"). - Build(), - expected: false, // Should return false on error - }, - } - - for name, tt := range tests { - t.Run(name, func(t *testing.T) { - defClient := tt.setupClient() - logger := tu.TestLogger(t, false) - - // Create the schema validator - validator := NewSchemaValidator(tu.NewMockSchemaClient().Build(), defClient, logger) - - // Call the function under test - result := validator.IsClaimResource(ctx, tt.resource) - - // Check the result - if result != tt.expected { - t.Errorf("IsClaimResource() = %v, want %v", result, tt.expected) - } - }) - } -} diff --git a/cmd/diff/testutils/mock_builder.go b/cmd/diff/testutils/mock_builder.go index 6831b8e..9959c39 100644 --- a/cmd/diff/testutils/mock_builder.go +++ b/cmd/diff/testutils/mock_builder.go @@ -693,6 +693,12 @@ func (b *MockDefinitionClientBuilder) WithXRDForClaim(unstructured *un.Unstructu }) } +// WithIsClaimResource sets the IsClaimResource behavior. +func (b *MockDefinitionClientBuilder) WithIsClaimResource(fn func(context.Context, *un.Unstructured) bool) *MockDefinitionClientBuilder { + b.mock.IsClaimResourceFn = fn + return b +} + // Build returns the built mock. func (b *MockDefinitionClientBuilder) Build() *MockDefinitionClient { return b.mock diff --git a/cmd/diff/testutils/mocks.go b/cmd/diff/testutils/mocks.go index 64fdfaa..ce6a93f 100644 --- a/cmd/diff/testutils/mocks.go +++ b/cmd/diff/testutils/mocks.go @@ -268,7 +268,6 @@ func (m *MockDiffProcessor) PerformDiff(ctx context.Context, stdout io.Writer, r type MockSchemaValidator struct { ValidateResourcesFn func(ctx context.Context, xr *un.Unstructured, composed []cpd.Unstructured) error ValidateScopeConstraintsFn func(ctx context.Context, resource *un.Unstructured, expectedNamespace string, isClaimRoot bool) error - IsClaimResourceFn func(ctx context.Context, resource *un.Unstructured) bool } // ValidateResources validates a set of resources against schemas from the cluster. @@ -292,14 +291,6 @@ func (m *MockSchemaValidator) ValidateScopeConstraints(ctx context.Context, reso return nil } -// IsClaimResource checks if the root resource is a claim type. -func (m *MockSchemaValidator) IsClaimResource(ctx context.Context, resource *un.Unstructured) bool { - if m.IsClaimResourceFn != nil { - return m.IsClaimResourceFn(ctx, resource) - } - return false -} - // endregion // region Kubernetes client mocks @@ -580,10 +571,11 @@ func (m *MockEnvironmentClient) GetEnvironmentConfig(ctx context.Context, name s // MockDefinitionClient implements the crossplane.DefinitionClient interface. type MockDefinitionClient struct { - InitializeFn func(ctx context.Context) error - GetXRDsFn func(ctx context.Context) ([]*un.Unstructured, error) - GetXRDForClaimFn func(ctx context.Context, gvk schema.GroupVersionKind) (*un.Unstructured, error) - GetXRDForXRFn func(ctx context.Context, gvk schema.GroupVersionKind) (*un.Unstructured, error) + InitializeFn func(ctx context.Context) error + GetXRDsFn func(ctx context.Context) ([]*un.Unstructured, error) + GetXRDForClaimFn func(ctx context.Context, gvk schema.GroupVersionKind) (*un.Unstructured, error) + GetXRDForXRFn func(ctx context.Context, gvk schema.GroupVersionKind) (*un.Unstructured, error) + IsClaimResourceFn func(ctx context.Context, resource *un.Unstructured) bool } // Initialize implements crossplane.DefinitionClient. @@ -618,6 +610,14 @@ func (m *MockDefinitionClient) GetXRDForXR(ctx context.Context, gvk schema.Group return nil, errors.New("GetXRDForXR not implemented") } +// IsClaimResource implements crossplane.DefinitionClient. +func (m *MockDefinitionClient) IsClaimResource(ctx context.Context, resource *un.Unstructured) bool { + if m.IsClaimResourceFn != nil { + return m.IsClaimResourceFn(ctx, resource) + } + return false +} + // MockResourceTreeClient implements the crossplane.ResourceTreeClient interface. type MockResourceTreeClient struct { InitializeFn func(ctx context.Context) error