[release-0.1] Improve the heuristics we use to identify resource types

internal/graph/model/common.go

@@ -363,8 +363,6 @@ func GetKubernetesResource(u *kunstructured.Unstructured) (KubernetesResource, e
 	// This isn't _really_ that complex; it's a long but simple switch.
 
 	switch {
-	case unstructured.ProbablyManaged(u):
-		return GetManagedResource(u), nil
 
 	case unstructured.ProbablyProviderConfig(u):
 		return GetProviderConfig(u), nil
@@ -375,6 +373,14 @@ func GetKubernetesResource(u *kunstructured.Unstructured) (KubernetesResource, e
 	case unstructured.ProbablyClaim(u):
 		return GetCompositeResourceClaim(u), nil
 
+	// Note that order is important here. We want to check whether the resource
+	// seems to be a managed resource _after_ checking whether it seems to be a
+	// composite resource because it's possible to define a composite resource
+	// that would pass the ProbablyManaged check. Such a composite resource
+	// would very likely pass the ProbablyComposite check and never reach this.
+	case unstructured.ProbablyManaged(u):
+		return GetManagedResource(u), nil
+
 	case u.GroupVersionKind() == pkgv1.ProviderGroupVersionKind:
 		p := &pkgv1.Provider{}
 		if err := convert(u, p); err != nil {

internal/graph/model/common_test.go

@@ -499,15 +499,22 @@ func TestGetKubernetesResource(t *testing.T) {
 			u: func() *kunstructured.Unstructured {
 				// Set resource refs to convince unstructured.ProbablyClaim
 				xrc := &unstructured.Claim{}
+				xrc.SetNamespace("default")
 				xrc.SetName("cool")
 				xrc.SetCompositionReference(&corev1.ObjectReference{Name: "cmp"})
 				xrc.SetResourceReference(&corev1.ObjectReference{Name: "xr"})
 				return xrc.GetUnstructured()
 			}(),
 			want: want{
 				kr: CompositeResourceClaim{
-					ID:       ReferenceID{Name: "cool"},
-					Metadata: &ObjectMeta{Name: "cool"},
+					ID: ReferenceID{
+						Namespace: "default",
+						Name:      "cool",
+					},
+					Metadata: &ObjectMeta{
+						Namespace: pointer.StringPtr("default"),
+						Name:      "cool",
+					},
 					Spec: &CompositeResourceClaimSpec{
 						CompositionReference: &corev1.ObjectReference{Name: "cmp"},
 						ResourceReference:    &corev1.ObjectReference{Name: "xr"},

internal/unstructured/claim.go

@@ -13,14 +13,29 @@ import (
 // github.com/crossplane-runtime/pkg/resource/unstructured/composite.
 
 // ProbablyClaim returns true if the supplied *Unstructured is probably a
-// composite resource claim. It considers any resource with object references
-// at spec.resourceRefs and spec.compositionRef to probably be a composite
-// resource claim.
+// composite resource claim. It considers any namespaced resource with at least
+// one of the fields we inject into the OpenAPI schema of resource claims set.
+// Note that it is possible for this to produce a false negative. All of these
+// injected fields are optional so it's possible that a claim has none of them
+// set. Such a claim would not be functional, as indicated by not having a
+// (composite) resource ref set.
 func ProbablyClaim(u *unstructured.Unstructured) bool {
-	// TODO(negz): Require spec.compositionRef be set too once
-	// https://github.com/crossplane/crossplane/issues/2263 is addressed.
-	err := fieldpath.Pave(u.Object).GetValueInto("spec.resourceRef", &corev1.ObjectReference{})
-	return err == nil
+
+	p := fieldpath.Pave(u.Object)
+	switch {
+	case u.GetNamespace() == "":
+		return false
+	case p.GetValueInto("spec.compositionRef", &corev1.ObjectReference{}) == nil:
+		return true
+	case p.GetValueInto("spec.compositionSelector", &metav1.LabelSelector{}) == nil:
+		return true
+	case p.GetValueInto("spec.resourceRef", &corev1.ObjectReference{}) == nil:
+		return true
+	case p.GetValueInto("spec.writeConnectionSecretToRef", &xpv1.LocalSecretReference{}) == nil:
+		return true
+	}
+
+	return false
 }
 
 // An Claim composite resource claim.

internal/unstructured/claim_test.go

@@ -24,32 +24,86 @@ func emptyXRC() *Claim {
 }
 func TestProbablyClaim(t *testing.T) {
 	cases := map[string]struct {
-		u    *unstructured.Unstructured
-		want bool
+		reason string
+		u      *unstructured.Unstructured
+		want   bool
 	}{
-		"Probably": {
+		"HasCompositionRef": {
+			reason: "A namespaced resource with a composition ref is probably a claim.",
 			u: func() *unstructured.Unstructured {
 				o := map[string]interface{}{}
 				fieldpath.Pave(o).SetValue("spec.compositionRef", &corev1.ObjectReference{
 					Name: "coolcomposition",
 				})
+				u := &unstructured.Unstructured{Object: o}
+				u.SetNamespace("default")
+				return u
+			}(),
+			want: true,
+		},
+		"HasCompositionSelector": {
+			reason: "A namespaced resource with a composition selector is probably a claim.",
+			u: func() *unstructured.Unstructured {
+				o := map[string]interface{}{}
+				fieldpath.Pave(o).SetValue("spec.compositionSelector", &metav1.LabelSelector{
+					MatchLabels: map[string]string{"cool": "true"},
+				})
+				u := &unstructured.Unstructured{Object: o}
+				u.SetNamespace("default")
+				return u
+			}(),
+			want: true,
+		},
+		"HasResourceRef": {
+			reason: "A namespaced resource with a resource ref is probably a claim.",
+			u: func() *unstructured.Unstructured {
+				o := map[string]interface{}{}
 				fieldpath.Pave(o).SetValue("spec.resourceRef", &corev1.ObjectReference{
 					APIVersion: "example.org/v1",
 					Kind:       "Example",
 					Name:       "coolexample",
 				})
-				return &unstructured.Unstructured{Object: o}
+				u := &unstructured.Unstructured{Object: o}
+				u.SetNamespace("default")
+				return u
 			}(),
 			want: true,
 		},
-		"ProbablyNot": {
+		"HasSecretRef": {
+			reason: "A namespaced resource with a connection secret ref is probably a claim.",
 			u: func() *unstructured.Unstructured {
 				o := map[string]interface{}{}
-				fieldpath.Pave(o).SetValue("spec.resourceRefs", []string{"wat"}) // Not object refs.
+				fieldpath.Pave(o).SetValue("spec.writeConnectionSecretToRef", &xpv1.LocalSecretReference{
+					Name: "coolsecret",
+				})
+				u := &unstructured.Unstructured{Object: o}
+				u.SetNamespace("default")
+				return u
+			}(),
+			want: true,
+		},
+		"ClusterScoped": {
+			reason: "A cluster scoped resource with a composition ref is not a claim.",
+			u: func() *unstructured.Unstructured {
+				o := map[string]interface{}{}
+				fieldpath.Pave(o).SetValue("spec.compositionRef", &corev1.ObjectReference{
+					Name: "coolcomposition",
+				})
 				return &unstructured.Unstructured{Object: o}
 			}(),
 			want: false,
 		},
+		"HasResourceRefs": {
+			reason: "A namespaced resource with a resourceRefs array is not a claim.",
+			u: func() *unstructured.Unstructured {
+				o := map[string]interface{}{}
+				fieldpath.Pave(o).SetValue("spec.resourceRefs", []string{"wat"}) // Not object refs.
+				u := &unstructured.Unstructured{Object: o}
+				u.SetNamespace("default")
+				return u
+			}(),
+			want: false,
+		},
 	}
 
 	for name, tc := range cases {

internal/unstructured/composite.go

@@ -29,14 +29,27 @@ import (
 // github.com/crossplane-runtime/pkg/resource/unstructured/composite.
 
 // ProbablyComposite returns true if the supplied *Unstructured is probably a
-// composite resource. It considers any resource with an object reference at
-// spec.compositionRef and an array of object refs at spec.resourceRefs to
-// probably be a composite resource.
+// composite resource. It considers any cluster scoped resource with at least
+// one of the fields we inject into the OpenAPI schema of composite resources set.
+// Note that it is possible for this to produce a false negative. All of these
+// injected fields are optional so it's possible that an XR has none of them
+// set. Such an XR would not be functional, as indicated by not having an array
+// of composed resource refs set.
 func ProbablyComposite(u *unstructured.Unstructured) bool {
-	cerr := fieldpath.Pave(u.Object).GetValueInto("spec.compositionRef", &corev1.ObjectReference{})
+	p := fieldpath.Pave(u.Object)
 	r := []corev1.ObjectReference{}
-	rerr := fieldpath.Pave(u.Object).GetValueInto("spec.resourceRefs", &r)
-	return cerr == nil && rerr == nil
+	switch {
+	case u.GetNamespace() != "":
+		return false
+	case p.GetValueInto("spec.compositionRef", &corev1.ObjectReference{}) == nil:
+		return true
+	case p.GetValueInto("spec.compositionSelector", &metav1.LabelSelector{}) == nil:
+		return true
+	case p.GetValueInto("spec.resourceRefs", &r) == nil:
+		return true
+	}
+
+	return false
 }
 
 // A Composite resource.

internal/unstructured/composite_test.go

@@ -22,27 +22,64 @@ var _ resource.Composite = &Composite{}
 func emptyXR() *Composite {
 	return &Composite{Unstructured: unstructured.Unstructured{Object: map[string]interface{}{}}}
 }
+
 func TestProbablyComposite(t *testing.T) {
 	cases := map[string]struct {
-		u    *unstructured.Unstructured
-		want bool
+		reason string
+		u      *unstructured.Unstructured
+		want   bool
 	}{
-		"Probably": {
+		"HasCompositionRef": {
+			reason: "A cluster scoped resource with a composition ref is probably an XR.",
 			u: func() *unstructured.Unstructured {
 				o := map[string]interface{}{}
 				fieldpath.Pave(o).SetValue("spec.compositionRef", &corev1.ObjectReference{
 					Name: "coolcomposition",
 				})
-				fieldpath.Pave(o).SetValue("spec.resourceRefs", []corev1.ObjectReference{{
+				return &unstructured.Unstructured{Object: o}
+			}(),
+			want: true,
+		},
+		"HasCompositionSelector": {
+			reason: "A cluster scoped resource with a composition selector is probably an XR.",
+			u: func() *unstructured.Unstructured {
+				o := map[string]interface{}{}
+				fieldpath.Pave(o).SetValue("spec.compositionSelector", &metav1.LabelSelector{
+					MatchLabels: map[string]string{"cool": "true"},
+				})
+				return &unstructured.Unstructured{Object: o}
+			}(),
+			want: true,
+		},
+		"HasResourceRefs": {
+			reason: "A cluster scoped resource with an array of resource refs is probably an XR.",
+			u: func() *unstructured.Unstructured {
+				o := map[string]interface{}{}
+				r := []corev1.ObjectReference{{
 					APIVersion: "example.org/v1",
 					Kind:       "Example",
 					Name:       "coolexample",
-				}})
+				}}
+				fieldpath.Pave(o).SetValue("spec.resourceRefs", &r)
 				return &unstructured.Unstructured{Object: o}
 			}(),
 			want: true,
 		},
-		"ProbablyNot": {
+		"Namespaced": {
+			reason: "A namespaced resource with a composition ref is not an XR.",
+			u: func() *unstructured.Unstructured {
+				o := map[string]interface{}{}
+				fieldpath.Pave(o).SetValue("spec.compositionRef", &corev1.ObjectReference{
+					Name: "coolcomposition",
+				})
+				u := &unstructured.Unstructured{Object: o}
+				u.SetNamespace("default")
+				return u
+			}(),
+			want: false,
+		},
+		"WeirdResourceRefs": {
+			reason: "A cluster scoped resource with a non-objectref resourceRefs array is not an XR.",
 			u: func() *unstructured.Unstructured {
 				o := map[string]interface{}{}
 				fieldpath.Pave(o).SetValue("spec.resourceRefs", []string{"wat"}) // Not object refs.

internal/unstructured/managed.go

@@ -8,9 +8,15 @@ import (
 )
 
 // ProbablyManaged returns true if the supplied *Unstructured is probably a
-// managed resource. It considers any resource with a string value at field path
-// spec.providerConfigRef.name to probably be a managed resource.
+// managed resource. It considers any cluster scoped resource with a string
+// value at field path spec.providerConfigRef.name to probably be a managed
+// resource. spec.providerConfigRef is technically optional, but is defaulted at
+// create time by the CRD's OpenAPI schema.
 func ProbablyManaged(u *unstructured.Unstructured) bool {
+	if u.GetNamespace() != "" {
+		return false
+	}
+
 	_, err := fieldpath.Pave(u.Object).GetString("spec.providerConfigRef.name")
 	return err == nil
 }

internal/unstructured/managed_test.go

@@ -21,32 +21,46 @@ func emptyMR() *Managed {
 
 func TestProbablyManaged(t *testing.T) {
 	cases := map[string]struct {
-		u    *unstructured.Unstructured
-		want bool
+		reason string
+		u      *unstructured.Unstructured
+		want   bool
 	}{
 		"Probably": {
+			reason: "A cluster scoped resource with a string providerConfigRef.name is most likely a managed resource",
 			u: func() *unstructured.Unstructured {
 				o := map[string]interface{}{}
 				fieldpath.Pave(o).SetString("spec.providerConfigRef.name", "coolprovider")
 				return &unstructured.Unstructured{Object: o}
 			}(),
 			want: true,
 		},
-		"ProbablyNot": {
+		"WeirdProviderConfigRefType": {
+			reason: "A cluster scoped resource with an int providerConfigRef.name is not a managed resource",
 			u: func() *unstructured.Unstructured {
 				o := map[string]interface{}{}
 				fieldpath.Pave(o).SetValue("spec.providerConfigRef.name", 42) // Not a string.
 				return &unstructured.Unstructured{Object: o}
 			}(),
 			want: false,
 		},
+		"Namespaced": {
+			reason: "A namespaced resource with a string providerConfigRef.name is not a managed resource",
+			u: func() *unstructured.Unstructured {
+				o := map[string]interface{}{}
+				fieldpath.Pave(o).SetString("spec.providerConfigRef.name", "coolprovider")
+				u := &unstructured.Unstructured{Object: o}
+				u.SetNamespace("default")
+				return u
+			}(),
+			want: false,
+		},
 	}
 
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
 			got := ProbablyManaged(tc.u)
 			if diff := cmp.Diff(tc.want, got); diff != "" {
-				t.Errorf("\nProbablyManaged(...): -want, +got:\n%s", diff)
+				t.Errorf("\n%s\nProbablyManaged(...): -want, +got:\n%s", tc.reason, diff)
 			}
 		})
 	}

internal/unstructured/providerconfig.go

@@ -8,10 +8,10 @@ import (
 )
 
 // ProbablyProviderConfig returns true if the supplied *Unstructured is probably
-// a provider config. It considers any resource of kind: ProviderConfig to
-// probably be a provider config.
+// a provider config. It considers any cluster scoped resource of kind:
+// ProviderConfig to probably be a provider config.
 func ProbablyProviderConfig(u *unstructured.Unstructured) bool {
-	return u.GetKind() == "ProviderConfig"
+	return u.GetNamespace() == "" && u.GetKind() == "ProviderConfig"
 }
 
 // A ProviderConfig resource.