Skip to content

Commit 0a142d9

Browse files
jcogilviejcogilvie
authored
Fix claim derived-resource lookups by label; fixes #26 (#27)
* Fix claim derived-resource lookups by label; fixes #26 Signed-off-by: Jonathan Ogilvie <[email protected]> * Fix claim derived-resource lookups by label; fixes #26 Signed-off-by: Jonathan Ogilvie <[email protected]> --------- Signed-off-by: Jonathan Ogilvie <[email protected]>
1 parent c59d664 commit 0a142d9

File tree

13 files changed

+386
-192
lines changed

13 files changed

+386
-192
lines changed

‎cmd/diff/client/crossplane/definition_client.go‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,9 @@ type DefinitionClient interface {
2828

2929
// GetXRDForXR finds the XRD that defines the given XR type
3030
GetXRDForXR(ctx context.Context, gvk schema.GroupVersionKind) (*un.Unstructured, error)
31+
32+
// IsClaimResource checks if the given resource is a claim type
33+
IsClaimResource(ctx context.Context, resource *un.Unstructured) bool
3134
}
3235

3336
// DefaultDefinitionClient implements DefinitionClient.
@@ -210,3 +213,19 @@ func (c *DefaultDefinitionClient) GetXRDForXR(ctx context.Context, gvk schema.Gr
210213

211214
return nil, errors.Errorf("no XRD found that defines XR type %s", gvk.String())
212215
}
216+
217+
// IsClaimResource checks if the given resource is a claim type by attempting
218+
// to find an XRD that defines it as a claim.
219+
func (c *DefaultDefinitionClient) IsClaimResource(ctx context.Context, resource *un.Unstructured) bool {
220+
gvk := resource.GroupVersionKind()
221+
222+
// Try to find an XRD that defines this resource type as a claim
223+
_, err := c.GetXRDForClaim(ctx, gvk)
224+
if err != nil {
225+
c.logger.Debug("Resource is not a claim type", "gvk", gvk.String(), "error", err)
226+
return false
227+
}
228+
229+
c.logger.Debug("Resource is a claim type", "gvk", gvk.String())
230+
return true
231+
}

‎cmd/diff/client/crossplane/definition_client_test.go‎

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -626,3 +626,119 @@ func TestDefaultDefinitionClient_Initialize(t *testing.T) {
626626
})
627627
}
628628
}
629+
630+
func TestDefaultDefinitionClient_IsClaimResource(t *testing.T) {
631+
ctx := t.Context()
632+
633+
tests := map[string]struct {
634+
reason string
635+
mockResource tu.MockResourceClient
636+
cachedXRDs []*un.Unstructured
637+
resource *un.Unstructured
638+
expected bool
639+
}{
640+
"ResourceIsClaim": {
641+
reason: "Should return true when resource is a claim type",
642+
mockResource: *tu.NewMockResourceClient().
643+
WithSuccessfulInitialize().
644+
Build(),
645+
cachedXRDs: []*un.Unstructured{
646+
// Create a mock XRD that defines this as a claim
647+
tu.NewResource("apiextensions.crossplane.io/v1", "CompositeResourceDefinition", "testclaims.example.org").
648+
WithSpecField("group", "example.org").
649+
WithSpecField("names", map[string]interface{}{
650+
"kind": "XTestResource",
651+
"plural": "xtestresources",
652+
"singular": "xtestresource",
653+
}).
654+
WithSpecField("claimNames", map[string]interface{}{
655+
"kind": "TestClaim",
656+
"plural": "testclaims",
657+
"singular": "testclaim",
658+
}).
659+
Build(),
660+
},
661+
resource: tu.NewResource("example.org/v1", "TestClaim", "test-claim").
662+
InNamespace("default").
663+
Build(),
664+
expected: true,
665+
},
666+
"ResourceIsNotClaim": {
667+
reason: "Should return false when resource is not a claim type",
668+
mockResource: *tu.NewMockResourceClient().
669+
WithSuccessfulInitialize().
670+
Build(),
671+
cachedXRDs: []*un.Unstructured{
672+
// Create a mock XRD that defines this as an XR (no claimNames)
673+
tu.NewResource("apiextensions.crossplane.io/v1", "CompositeResourceDefinition", "testxrs.example.org").
674+
WithSpecField("group", "example.org").
675+
WithSpecField("names", map[string]interface{}{
676+
"kind": "TestXR",
677+
"plural": "testxrs",
678+
"singular": "testxr",
679+
}).
680+
// No claimNames field
681+
Build(),
682+
},
683+
resource: tu.NewResource("example.org/v1", "TestXR", "test-xr").
684+
InNamespace("default").
685+
Build(),
686+
expected: false,
687+
},
688+
"GetXRDForClaimError": {
689+
reason: "Should return false when GetXRDForClaim fails",
690+
mockResource: *tu.NewMockResourceClient().
691+
WithSuccessfulInitialize().
692+
WithListResourcesFailure("list error").
693+
Build(),
694+
cachedXRDs: nil, // Force GetXRDs to fail
695+
resource: tu.NewResource("example.org/v1", "TestResource", "test-resource").
696+
Build(),
697+
expected: false, // Should return false on error
698+
},
699+
"NoMatchingXRD": {
700+
reason: "Should return false when no matching XRD exists",
701+
mockResource: *tu.NewMockResourceClient().
702+
WithSuccessfulInitialize().
703+
Build(),
704+
cachedXRDs: []*un.Unstructured{
705+
// Create an XRD that doesn't match our resource
706+
tu.NewResource("apiextensions.crossplane.io/v1", "CompositeResourceDefinition", "otherclaims.example.org").
707+
WithSpecField("group", "example.org").
708+
WithSpecField("names", map[string]interface{}{
709+
"kind": "XOtherResource",
710+
"plural": "xotherresources",
711+
"singular": "xotherresource",
712+
}).
713+
WithSpecField("claimNames", map[string]interface{}{
714+
"kind": "OtherClaim",
715+
"plural": "otherclaims",
716+
"singular": "otherclaim",
717+
}).
718+
Build(),
719+
},
720+
resource: tu.NewResource("example.org/v1", "TestClaim", "test-claim").
721+
Build(),
722+
expected: false,
723+
},
724+
}
725+
726+
for name, tt := range tests {
727+
t.Run(name, func(t *testing.T) {
728+
c := &DefaultDefinitionClient{
729+
resourceClient: &tt.mockResource,
730+
logger: tu.TestLogger(t, false),
731+
xrds: tt.cachedXRDs,
732+
xrdsLoaded: tt.cachedXRDs != nil, // Only mark as loaded if we have cached XRDs
733+
}
734+
735+
// Call the function under test
736+
result := c.IsClaimResource(ctx, tt.resource)
737+
738+
// Check the result
739+
if result != tt.expected {
740+
t.Errorf("\n%s\nIsClaimResource() = %v, want %v", tt.reason, result, tt.expected)
741+
}
742+
})
743+
}
744+
}

‎cmd/diff/diff_integration_test.go‎

Lines changed: 8 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -61,31 +61,6 @@ func TestDiffIntegration(t *testing.T) {
6161
_ = pkgv1.AddToScheme(scheme)
6262
_ = extv1.AddToScheme(scheme)
6363

64-
// TODO: is there a reason to even run this against v1 if everything is backwards compatible?
65-
// claims are still here (for now). we obviously need to keep tests for those.
66-
// we've already removed the deprecated environmentconfig version.
67-
// I can see running the ITs against v2 since we can test against an old image. important to IT against v1 image
68-
// given changes to move xp specific stuff into spec.crossplane, which will not be reflected in running these tests
69-
70-
// TODO: add a test to cover v2 CompositeResourceDefinition (XRD) if running against Crossplane v2
71-
// TODO: add a test to cover namespaced xrds against v2
72-
// update: these ITs don't run against a version of xp besides what they are compiled against. that'll matter
73-
// in the e2es.
74-
// we'll want to rig up some way to specify xrd-v1 or xrd-v2 or both in the test cases
75-
// but the rub is that the cluster directory containing the crds is pulled from either v1 or v2, so we can't just
76-
// run both.
77-
// thinking we should just grab the cluster directory for v1 and check it in, since it won't advance anymore once
78-
// v2 is out. v2 we can update at build time. every test spins up its own envtest with the crd path, so we can
79-
// definitely toggle there.
80-
81-
// the CRDs that support the XRDs will vary based on the Crossplane version, though (namespaced vs cluster scoped),
82-
// so we need to bifurcate /testdata/diff/crds accordingly. although each XRD that we define will have a version
83-
// specified inside it which will lead to the generation of that crd. so maybe it's test specific actually.
84-
85-
// TODO: namespaced XRDs cannot compose cluster-scoped resources, so we need to ensure XDownstreamResource definitions
86-
// account for that. maybe we just need to add parallel CRDs for namespace scoped and cluster scoped XRDs that can
87-
// coexist.
88-
8964
// Test cases
9065
tests := map[string]struct {
9166
setupFiles []string
@@ -1042,16 +1017,20 @@ Summary: 2 added`,
10421017
+ coolField: modified-value
10431018
10441019
---
1045-
~~~ XDownstreamResource/test-claim
1020+
~~~ XDownstreamResource/test-claim-82crv
10461021
apiVersion: nop.example.org/v1alpha1
10471022
kind: XDownstreamResource
10481023
metadata:
10491024
annotations:
10501025
crossplane.io/composition-resource-name: nop-resource
1051-
generateName: test-claim-
1026+
- generateName: test-claim-82crv-
10521027
labels:
1053-
crossplane.io/composite: test-claim
1054-
name: test-claim
1028+
crossplane.io/claim-name: test-claim
1029+
crossplane.io/claim-namespace: existing-namespace
1030+
- crossplane.io/composite: test-claim-82crv
1031+
- name: test-claim-82crv
1032+
+ crossplane.io/composite: test-claim
1033+
+ name: test-claim
10551034
spec:
10561035
forProvider:
10571036
- configData: existing-value

‎cmd/diff/diffprocessor/diff_calculator_test.go‎

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ func TestDefaultDiffCalculator_CalculateDiff(t *testing.T) {
8383
Build()
8484

8585
// Create resource manager
86-
resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false))
86+
resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false))
8787

8888
return applyClient, resourceTreeClient, resourceManager
8989
},
@@ -113,7 +113,7 @@ func TestDefaultDiffCalculator_CalculateDiff(t *testing.T) {
113113
Build()
114114

115115
// Create resource manager
116-
resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false))
116+
resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false))
117117

118118
return applyClient, resourceTreeClient, resourceManager
119119
},
@@ -144,7 +144,7 @@ func TestDefaultDiffCalculator_CalculateDiff(t *testing.T) {
144144
Build()
145145

146146
// Create resource manager
147-
resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false))
147+
resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false))
148148

149149
return applyClient, resourceTreeClient, resourceManager
150150
},
@@ -182,7 +182,7 @@ func TestDefaultDiffCalculator_CalculateDiff(t *testing.T) {
182182
Build()
183183

184184
// Create resource manager
185-
resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false))
185+
resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false))
186186

187187
return applyClient, resourceTreeClient, resourceManager
188188
},
@@ -212,7 +212,7 @@ func TestDefaultDiffCalculator_CalculateDiff(t *testing.T) {
212212
Build()
213213

214214
// Create resource manager
215-
resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false))
215+
resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false))
216216

217217
return applyClient, resourceTreeClient, resourceManager
218218
},
@@ -238,7 +238,7 @@ func TestDefaultDiffCalculator_CalculateDiff(t *testing.T) {
238238
Build()
239239

240240
// Create resource manager
241-
resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false))
241+
resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false))
242242

243243
return applyClient, resourceTreeClient, resourceManager
244244
},
@@ -309,7 +309,7 @@ func TestDefaultDiffCalculator_CalculateDiff(t *testing.T) {
309309
Build()
310310

311311
// Create resource manager
312-
resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false))
312+
resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false))
313313

314314
return applyClient, resourceTreeClient, resourceManager
315315
},
@@ -455,7 +455,7 @@ func TestDefaultDiffCalculator_CalculateDiffs(t *testing.T) {
455455
Build()
456456

457457
// Create resource manager
458-
resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false))
458+
resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false))
459459

460460
return applyClient, resourceTreeClient, resourceManager
461461
},
@@ -491,7 +491,7 @@ func TestDefaultDiffCalculator_CalculateDiffs(t *testing.T) {
491491
Build()
492492

493493
// Create resource manager
494-
resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false))
494+
resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false))
495495

496496
return applyClient, resourceTreeClient, resourceManager
497497
},
@@ -529,7 +529,7 @@ func TestDefaultDiffCalculator_CalculateDiffs(t *testing.T) {
529529
Build()
530530

531531
// Create resource manager
532-
resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false))
532+
resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false))
533533

534534
return applyClient, resourceTreeClient, resourceManager
535535
},
@@ -572,7 +572,7 @@ func TestDefaultDiffCalculator_CalculateDiffs(t *testing.T) {
572572
Build()
573573

574574
// Create resource manager
575-
resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false))
575+
resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false))
576576

577577
return applyClient, resourceTreeClient, resourceManager
578578
},
@@ -629,7 +629,7 @@ func TestDefaultDiffCalculator_CalculateDiffs(t *testing.T) {
629629
Build()
630630

631631
// Create resource manager
632-
resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false))
632+
resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false))
633633

634634
return applyClient, resourceTreeClient, resourceManager
635635
},
@@ -762,7 +762,7 @@ func TestDefaultDiffCalculator_CalculateRemovedResourceDiffs(t *testing.T) {
762762

763763
// Create a resource manager (not directly used in this test)
764764
resourceClient := tu.NewMockResourceClient().Build()
765-
resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false))
765+
resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false))
766766

767767
return applyClient, resourceTreeClient, resourceManager
768768
},
@@ -791,7 +791,7 @@ func TestDefaultDiffCalculator_CalculateRemovedResourceDiffs(t *testing.T) {
791791

792792
// Create a resource manager (not directly used in this test)
793793
resourceClient := tu.NewMockResourceClient().Build()
794-
resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false))
794+
resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false))
795795

796796
return applyClient, resourceTreeClient, resourceManager
797797
},
@@ -817,7 +817,7 @@ func TestDefaultDiffCalculator_CalculateRemovedResourceDiffs(t *testing.T) {
817817

818818
// Create a resource manager (not directly used in this test)
819819
resourceClient := tu.NewMockResourceClient().Build()
820-
resourceManager := NewResourceManager(resourceClient, tu.TestLogger(t, false))
820+
resourceManager := NewResourceManager(resourceClient, tu.NewMockDefinitionClient().Build(), tu.TestLogger(t, false))
821821

822822
return applyClient, resourceTreeClient, resourceManager
823823
},

‎cmd/diff/diffprocessor/diff_processor.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ type DiffProcessor interface {
4141
type DefaultDiffProcessor struct {
4242
fnClient xp.FunctionClient
4343
compClient xp.CompositionClient
44+
defClient xp.DefinitionClient
4445
config ProcessorConfig
4546
schemaValidator SchemaValidator
4647
diffCalculator DiffCalculator
@@ -71,7 +72,7 @@ func NewDiffProcessor(k8cs k8.Clients, xpcs xp.Clients, opts ...ProcessorOption)
7172
diffOpts := config.GetDiffOptions()
7273

7374
// Create components using factories
74-
resourceManager := config.Factories.ResourceManager(k8cs.Resource, config.Logger)
75+
resourceManager := config.Factories.ResourceManager(k8cs.Resource, xpcs.Definition, config.Logger)
7576
schemaValidator := config.Factories.SchemaValidator(k8cs.Schema, xpcs.Definition, config.Logger)
7677
requirementsProvider := config.Factories.RequirementsProvider(k8cs.Resource, xpcs.Environment, config.RenderFunc, config.Logger)
7778
diffCalculator := config.Factories.DiffCalculator(k8cs.Apply, xpcs.ResourceTree, resourceManager, config.Logger, diffOpts)
@@ -80,6 +81,7 @@ func NewDiffProcessor(k8cs k8.Clients, xpcs xp.Clients, opts ...ProcessorOption)
8081
processor := &DefaultDiffProcessor{
8182
fnClient: xpcs.Function,
8283
compClient: xpcs.Composition,
84+
defClient: xpcs.Definition,
8385
config: config,
8486
schemaValidator: schemaValidator,
8587
diffCalculator: diffCalculator,
@@ -222,7 +224,7 @@ func (p *DefaultDiffProcessor) DiffSingleResource(ctx context.Context, res *un.U
222224
// WORKAROUND for https://github.com/crossplane/crossplane/issues/6782
223225
// Propagate namespace from XR to namespaced managed resources
224226
// For claims (v1 compatibility), skip namespace propagation entirely as all managed resources are cluster-scoped
225-
isClaimRoot := p.schemaValidator.IsClaimResource(ctx, xrUnstructured)
227+
isClaimRoot := p.defClient.IsClaimResource(ctx, xrUnstructured)
226228
if !isClaimRoot {
227229
p.propagateNamespacesToManagedResources(ctx, xrUnstructured, desired.ComposedResources)
228230
} else {

‎cmd/diff/diffprocessor/processor_config.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ type ProcessorConfig struct {
3232
// ComponentFactories contains factory functions for creating processor components.
3333
type ComponentFactories struct {
3434
// ResourceManager creates a ResourceManager
35-
ResourceManager func(client k8.ResourceClient, logger logging.Logger) ResourceManager
35+
ResourceManager func(client k8.ResourceClient, defClient xp.DefinitionClient, logger logging.Logger) ResourceManager
3636

3737
// SchemaValidator creates a SchemaValidator
3838
SchemaValidator func(schema k8.SchemaClient, def xp.DefinitionClient, logger logging.Logger) SchemaValidator
@@ -86,7 +86,7 @@ func WithRenderFunc(renderFn RenderFunc) ProcessorOption {
8686
}
8787

8888
// WithResourceManagerFactory sets the ResourceManager factory function.
89-
func WithResourceManagerFactory(factory func(k8.ResourceClient, logging.Logger) ResourceManager) ProcessorOption {
89+
func WithResourceManagerFactory(factory func(k8.ResourceClient, xp.DefinitionClient, logging.Logger) ResourceManager) ProcessorOption {
9090
return func(config *ProcessorConfig) {
9191
config.Factories.ResourceManager = factory
9292
}

0 commit comments

Comments
 (0)