Repository: crossplane/crossplane

Issue:
Revision controller race results in broken ValidatingAdmissionWebhook
<!--
Thank you for helping to improve Crossplane!

Please be sure to search for open issues before raising a new one. We use issues
for bug reports and feature requests. Please find us at https://slack.crossplane.io
for questions, support, and discussion.
-->

### What happened?
<!--
Please let us know what behaviour you expected and how Crossplane diverged from
that behaviour.
-->

Quoting @phisco from Slack:

> spotted a possible cause for the webhook errors [...], looks like it might be related to GetObservedTLSServerSecretName introduced [here](https://github.com/crossplane/crossplane/pull/6567/files#diff-d39afb49908dda5cf456b7e8e173d85a4eb337de12915a01c3af5ed5272aafd5), there are two reconcilers, but only [the runtime one is setting the secret name in the status](https://github.com/crossplane/crossplane/blob/e7a00ed4043ccead1f221196ef2b9a18f0e228f2/internal/controller/pkg/runtime/runtime_provider.go#L66-L72), but the other one is expecting it to be set to [do its job properly and establish validatingwebhooks.](https://github.com/crossplane/crossplane/blob/c84c8ad42b2632aaee6bc4185e095699b7607880/internal/controller/pkg/revision/establisher.go#L403-L407) So if the two don’t run in the right order, the validatingwebhook is created unpatched.

### How can we reproduce it?
<!--
Help us to reproduce your bug as succinctly and precisely as possible. Artifacts
such as example manifests or a script that triggers the issue are highly
appreciated!
-->

The `TestCrossplaneLifecycle` E2E hits this very often - it's one of the causes of https://github.com/crossplane/crossplane/issues/6451.

Discussion:

Solution patch:
diff --git a/Earthfile b/Earthfile
index eedf39c98..0489f0276 100644
--- a/Earthfile
+++ b/Earthfile
@@ -331,7 +331,7 @@ ko-init:
 
 # kind-setup is used by other targets to setup kind.
 kind-setup:
-  ARG KIND_VERSION=v0.25.0
+  ARG KIND_VERSION=v0.29.0
   ARG NATIVEPLATFORM
   ARG TARGETOS
   ARG TARGETARCH
diff --git a/internal/controller/pkg/revision/establisher.go b/internal/controller/pkg/revision/establisher.go
index dbf54cd53..4a93f2a83 100644
--- a/internal/controller/pkg/revision/establisher.go
+++ b/internal/controller/pkg/revision/establisher.go
@@ -46,6 +46,7 @@ const (
 	errAssertClientObj              = "cannot assert object to client.Object"
 	errConversionWithNoWebhookCA    = "cannot deploy a CRD with webhook conversion strategy without having a TLS bundle"
 	errGetWebhookTLSSecret          = "cannot get webhook tls secret"
+	errWebhookSecretNotPresent      = "waiting for package runtime controller to set revision's webhook TLS secret"
 	errWebhookSecretWithoutCABundle = "the value for the key tls.crt cannot be empty"
 	errFmtGetOwnedObject            = "cannot get owned object: %s/%s"
 	errFmtUpdateOwnedObject         = "cannot update owned object: %s/%s"
@@ -403,7 +404,7 @@ func (e *APIEstablisher) enrichControlledResource(res runtime.Object, webhookTLS
 func (e *APIEstablisher) getWebhookTLSCert(ctx context.Context, parentWithRuntime v1.PackageRevisionWithRuntime) (webhookTLSCert []byte, err error) {
 	tlsServerSecretName := parentWithRuntime.GetObservedTLSServerSecretName()
 	if tlsServerSecretName == nil {
-		return nil, nil
+		return nil, errors.New(errWebhookSecretNotPresent)
 	}
 
 	s := &corev1.Secret{}



Test patch:
diff --git a/internal/controller/pkg/revision/establisher_test.go b/internal/controller/pkg/revision/establisher_test.go
index 220a9e34d..f9058c297 100644
--- a/internal/controller/pkg/revision/establisher_test.go
+++ b/internal/controller/pkg/revision/establisher_test.go
@@ -70,7 +70,17 @@ func TestAPIEstablisherEstablish(t *testing.T) {
 			reason: "Establishment should be successful if we can establish control for a parent of existing objects.",
 			args: args{
 				est: newAPIEstablisher(&test.MockClient{
-					MockGet:    test.NewMockGetFn(nil),
+					MockGet: func(_ context.Context, _ client.ObjectKey, obj client.Object) error {
+						if s, ok := obj.(*corev1.Secret); ok {
+							(&corev1.Secret{
+								Data: map[string][]byte{
+									"tls.crt": caBundle,
+								},
+							}).DeepCopyInto(s)
+							return nil
+						}
+						return nil
+					},
 					MockUpdate: test.NewMockUpdateFn(nil),
 				}),
 				objs: []runtime.Object{
@@ -92,6 +102,11 @@ func TestAPIEstablisherEstablish(t *testing.T) {
 							v1.LabelParentPackage: "provider-name",
 						},
 					},
+					Status: v1.ProviderRevisionStatus{
+						PackageRevisionRuntimeStatus: v1.PackageRevisionRuntimeStatus{
+							TLSServerSecretName: &tlsServerSecretName,
+						},
+					},
 				},
 				control: true,
 			},
@@ -103,7 +118,17 @@ func TestAPIEstablisherEstablish(t *testing.T) {
 			reason: "Establishment should be successful if we can establish control for a parent of new objects.",
 			args: args{
 				est: newAPIEstablisher(&test.MockClient{
-					MockGet:    test.NewMockGetFn(kerrors.NewNotFound(schema.GroupResource{}, "")),
+					MockGet: func(_ context.Context, _ client.ObjectKey, obj client.Object) error {
+						if s, ok := obj.(*corev1.Secret); ok {
+							(&corev1.Secret{
+								Data: map[string][]byte{
+									"tls.crt": caBundle,
+								},
+							}).DeepCopyInto(s)
+							return nil
+						}
+						return kerrors.NewNotFound(schema.GroupResource{}, "")
+					},
 					MockCreate: test.NewMockCreateFn(nil),
 				}),
 				objs: []runtime.Object{
@@ -125,6 +150,11 @@ func TestAPIEstablisherEstablish(t *testing.T) {
 							v1.LabelParentPackage: "provider-name",
 						},
 					},
+					Status: v1.ProviderRevisionStatus{
+						PackageRevisionRuntimeStatus: v1.PackageRevisionRuntimeStatus{
+							TLSServerSecretName: &tlsServerSecretName,
+						},
+					},
 				},
 				control: true,
 			},
@@ -256,11 +286,51 @@ func TestAPIEstablisherEstablish(t *testing.T) {
 				refs: []xpv1.TypedReference{{Name: "ref-me"}},
 			},
 		},
+		"FailedTLSSecretNotPresent": {
+			reason: "Establishment should fail if TLS server secret is not present when trying to establish control.",
+			args: args{
+				est: newAPIEstablisher(&test.MockClient{
+					MockGet:    test.NewMockGetFn(kerrors.NewNotFound(schema.GroupResource{}, "")),
+					MockCreate: test.NewMockCreateFn(nil),
+				}),
+				objs: []runtime.Object{
+					&extv1.CustomResourceDefinition{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "ref-me",
+						},
+					},
+				},
+				parent: &v1.ProviderRevision{
+					ObjectMeta: metav1.ObjectMeta{
+						OwnerReferences: []metav1.OwnerReference{
+							{
+								Name: "provider-name",
+								UID:  "some-unique-uid-2312",
+							},
+						},
+						Labels: map[string]string{
+							v1.LabelParentPackage: "provider-name",
+						},
+					},
+				},
+				control: true,
+			},
+			want: want{
+				err: errors.New(errWebhookSecretNotPresent),
+			},
+		},
 		"FailedCreationWebhookDisabledConversionRequested": {
 			reason: "Establishment should fail if the CRD requires conversion webhook and Crossplane does not have the webhooks enabled.",
 			args: args{
 				est: newAPIEstablisher(&test.MockClient{
-					MockGet:    test.NewMockGetFn(kerrors.NewNotFound(schema.GroupResource{}, "")),
+					MockGet: func(_ context.Context, _ client.ObjectKey, obj client.Object) error {
+						if s, ok := obj.(*corev1.Secret); ok {
+							// Return empty secret (no CA bundle)
+							s.Data = map[string][]byte{}
+							return nil
+						}
+						return kerrors.NewNotFound(schema.GroupResource{}, "")
+					},
 					MockCreate: test.NewMockCreateFn(nil),
 				}),
 				objs: []runtime.Object{
@@ -287,11 +357,16 @@ func TestAPIEstablisherEstablish(t *testing.T) {
 							v1.LabelParentPackage: "provider-name",
 						},
 					},
+					Status: v1.ProviderRevisionStatus{
+						PackageRevisionRuntimeStatus: v1.PackageRevisionRuntimeStatus{
+							TLSServerSecretName: &tlsServerSecretName,
+						},
+					},
 				},
 				control: true,
 			},
 			want: want{
-				err: errors.New(errConversionWithNoWebhookCA),
+				err: errors.New(errWebhookSecretWithoutCABundle),
 			},
 		},
 		"FailedGettingWebhookTLSSecretControl": {
@@ -382,7 +457,17 @@ func TestAPIEstablisherEstablish(t *testing.T) {
 			reason: "Cannot establish control of object if we cannot create it.",
 			args: args{
 				est: newAPIEstablisher(&test.MockClient{
-					MockGet:    test.NewMockGetFn(kerrors.NewNotFound(schema.GroupResource{}, "")),
+					MockGet: func(_ context.Context, _ client.ObjectKey, obj client.Object) error {
+						if s, ok := obj.(*corev1.Secret); ok {
+							(&corev1.Secret{
+								Data: map[string][]byte{
+									"tls.crt": caBundle,
+								},
+							}).DeepCopyInto(s)
+							return nil
+						}
+						return kerrors.NewNotFound(schema.GroupResource{}, "")
+					},
 					MockCreate: test.NewMockCreateFn(errBoom),
 				}),
 				objs: []runtime.Object{
@@ -396,6 +481,11 @@ func TestAPIEstablisherEstablish(t *testing.T) {
 					ObjectMeta: metav1.ObjectMeta{
 						Name: "test",
 					},
+					Status: v1.ProviderRevisionStatus{
+						PackageRevisionRuntimeStatus: v1.PackageRevisionRuntimeStatus{
+							TLSServerSecretName: &tlsServerSecretName,
+						},
+					},
 				},
 				control: true,
 			},
@@ -407,7 +497,17 @@ func TestAPIEstablisherEstablish(t *testing.T) {
 			reason: "Cannot establish control of object if we cannot update it.",
 			args: args{
 				est: newAPIEstablisher(&test.MockClient{
-					MockGet:    test.NewMockGetFn(nil),
+					MockGet: func(_ context.Context, _ client.ObjectKey, obj client.Object) error {
+						if s, ok := obj.(*corev1.Secret); ok {
+							(&corev1.Secret{
+								Data: map[string][]byte{
+									"tls.crt": caBundle,
+								},
+							}).DeepCopyInto(s)
+							return nil
+						}
+						return nil
+					},
 					MockUpdate: test.NewMockUpdateFn(errBoom),
 				}),
 				objs: []runtime.Object{
@@ -421,6 +521,11 @@ func TestAPIEstablisherEstablish(t *testing.T) {
 					ObjectMeta: metav1.ObjectMeta{
 						Name: "test",
 					},
+					Status: v1.ProviderRevisionStatus{
+						PackageRevisionRuntimeStatus: v1.PackageRevisionRuntimeStatus{
+							TLSServerSecretName: &tlsServerSecretName,
+						},
+					},
 				},
 				control: true,
 			},
diff --git a/test/e2e/funcs/collect.go b/test/e2e/funcs/collect.go
index b46c34820..83e2113cf 100644
--- a/test/e2e/funcs/collect.go
+++ b/test/e2e/funcs/collect.go
@@ -32,6 +32,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
 
 	"github.com/crossplane/crossplane-runtime/pkg/errors"
+	"github.com/crossplane/crossplane-runtime/pkg/resource/unstructured/claim"
 	"github.com/crossplane/crossplane-runtime/pkg/resource/unstructured/composite"
 )
 
@@ -51,6 +52,8 @@ type coordinate struct {
 //
 // Note: this is a pretty expensive operation only suited for e2e tests with
 // small clusters.
+//
+//nolint:gocognit // Only a little over, mostly a loop.
 func buildRelatedObjectGraph(ctx context.Context, t *testing.T, discoveryClient discovery.DiscoveryInterface, client dynamic.Interface, mapper meta.RESTMapper) (map[coordinate][]coordinate, error) {
 	t.Helper()
 
@@ -86,22 +89,24 @@ func buildRelatedObjectGraph(ctx context.Context, t *testing.T, discoveryClient
 					Name:                 obj.GetName(),
 				}
 
-				// collect owner refenerces
 				var refs []corev1.ObjectReference
-				for _, ownerRef := range obj.GetOwnerReferences() {
+
+				// If this is a claim with a reference to an XR,
+				// the XR is probably the most important thing
+				// we care about. Include it.
+				cm := claim.Unstructured{Unstructured: obj}
+				if ref := cm.GetResourceReference(); ref != nil {
 					refs = append(refs, corev1.ObjectReference{
-						APIVersion: ownerRef.APIVersion,
-						Kind:       ownerRef.Kind,
-						Namespace:  obj.GetNamespace(),
-						Name:       ownerRef.Name,
+						APIVersion: ref.APIVersion,
+						Kind:       ref.Kind,
+						Name:       ref.Name,
 					})
 				}
 
-				// maybe this is an XR with resource reference to the claim? Fake owner refs.
-				comp := composite.Unstructured{Unstructured: obj}
-
-				refs = append(refs, comp.GetResourceReferences()...)
-				if ref := comp.GetClaimReference(); ref != nil {
+				// If this is an XR, include its reference to a
+				// claim and any composed resources.
+				xr := composite.Unstructured{Schema: composite.SchemaLegacy, Unstructured: obj}
+				if ref := xr.GetClaimReference(); ref != nil {
 					refs = append(refs, corev1.ObjectReference{
 						APIVersion: ref.APIVersion,
 						Kind:       ref.Kind,
@@ -109,6 +114,22 @@ func buildRelatedObjectGraph(ctx context.Context, t *testing.T, discoveryClient
 						Namespace:  ref.Namespace,
 					})
 				}
+				refs = append(refs, xr.GetResourceReferences()...)
+
+				// Handle modern v2 style XRs too, with
+				// spec.crossplane.resourceRefs.
+				xr = composite.Unstructured{Schema: composite.SchemaModern, Unstructured: obj}
+				refs = append(refs, xr.GetResourceReferences()...)
+
+				// Finally include any owners.
+				for _, ownerRef := range obj.GetOwnerReferences() {
+					refs = append(refs, corev1.ObjectReference{
+						APIVersion: ownerRef.APIVersion,
+						Kind:       ownerRef.Kind,
+						Namespace:  obj.GetNamespace(),
+						Name:       ownerRef.Name,
+					})
+				}
 
 				for _, ref := range refs {
 					group, version := parseAPIVersion(ref.APIVersion)
diff --git a/test/e2e/funcs/feature.go b/test/e2e/funcs/feature.go
index bef3045ad..64acd4fe9 100644
--- a/test/e2e/funcs/feature.go
+++ b/test/e2e/funcs/feature.go
@@ -1339,8 +1339,28 @@ func FilterByGK(gk schema.GroupKind) func(o k8s.Object) bool {
 
 func toYAML(objs ...client.Object) string {
 	docs := make([]string, 0, len(objs))
-	for _, o := range objs {
-		oy, _ := yaml.Marshal(o)
+	for _, obj := range objs {
+		// First round-trip the object through JSON to make it
+		// unstructured.
+		j, _ := json.Marshal(obj)
+		u := &unstructured.Unstructured{}
+		_ = json.Unmarshal(j, u)
+
+		p := fieldpath.Pave(u.Object)
+
+		// Remove managed fields. They're seldom useful when
+		// troubleshooting a broken test, and distract from the rest of
+		// the object.
+		_ = p.DeleteField("metadata.managedFields")
+
+		// For resources like XRDs and CRDs with huge specs, filter it
+		// out. Otherwise it usually overflows the backscroll when a
+		// test fails.
+		if u.GetKind() == "CustomResourceDefinition" || u.GetKind() == "CompositeResourceDefinition" {
+			_ = p.DeleteField("spec")
+		}
+
+		oy, _ := yaml.Marshal(u)
 		docs = append(docs, string(oy))
 	}
 
diff --git a/test/e2e/install_test.go b/test/e2e/install_test.go
index 4530b3b90..0fdbc391f 100644
--- a/test/e2e/install_test.go
+++ b/test/e2e/install_test.go
@@ -31,6 +31,7 @@ import (
 	"sigs.k8s.io/e2e-framework/third_party/helm"
 
 	xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1"
+	"github.com/crossplane/crossplane-runtime/pkg/resource/unstructured/composed"
 
 	apiextensionsv1 "github.com/crossplane/crossplane/apis/apiextensions/v1"
 	pkgv1 "github.com/crossplane/crossplane/apis/pkg/v1"
@@ -68,10 +69,29 @@ func TestCrossplaneLifecycle(t *testing.T) {
 			Assess("DeleteClaim", funcs.AllOf(
 				funcs.DeleteResourcesWithPropagationPolicy(manifests, "claim.yaml", metav1.DeletePropagationForeground),
 				funcs.ResourcesDeletedWithin(30*time.Second, manifests, "claim.yaml"),
+
+				// TODO(negz): We're seeing composed resources
+				// sticking around after the claim is deleted,
+				// even though the claim is deleting the XR (and
+				// thus its composed resources) with foreground
+				// deletion. How is that possible?
+				funcs.ListedResourcesDeletedWithin(1*time.Minute, composed.NewList(composed.FromReferenceToList(corev1.ObjectReference{
+					APIVersion: "nop.crossplane.io/v1alpha1",
+					Kind:       "NopResource",
+				}))),
 			)).
 			Assess("DeletePrerequisites", funcs.AllOf(
 				funcs.DeleteResourcesWithPropagationPolicy(manifests, "setup/*.yaml", metav1.DeletePropagationForeground),
 				funcs.ResourcesDeletedWithin(3*time.Minute, manifests, "setup/*.yaml"),
+
+				// TODO(negz): We're seeing revisions sticking
+				// around after Crossplane is uninstalled, which
+				// blocks deleting their CRDs. It's unclear how
+				// that's possible given that we're deleting the
+				// packages with foreground deletion, and
+				// waiting for the packages to be gone.
+				funcs.ListedResourcesDeletedWithin(1*time.Minute, &pkgv1.ProviderRevisionList{}),
+				funcs.ListedResourcesDeletedWithin(1*time.Minute, &pkgv1.FunctionRevisionList{}),
 			)).
 			Assess("UninstallCrossplane", funcs.AllOf(
 				funcs.AsFeaturesFunc(funcs.HelmUninstall(
@@ -152,10 +172,29 @@ func TestCrossplaneLifecycle(t *testing.T) {
 			Assess("DeleteClaim", funcs.AllOf(
 				funcs.DeleteResourcesWithPropagationPolicy(manifests, "claim.yaml", metav1.DeletePropagationForeground),
 				funcs.ResourcesDeletedWithin(30*time.Second, manifests, "claim.yaml"),
+
+				// TODO(negz): We're seeing composed resources
+				// sticking around after the claim is deleted,
+				// even though the claim is deleting the XR (and
+				// thus its composed resources) with foreground
+				// deletion. How is that possible?
+				funcs.ListedResourcesDeletedWithin(1*time.Minute, composed.NewList(composed.FromReferenceToList(corev1.ObjectReference{
+					APIVersion: "nop.crossplane.io/v1alpha1",
+					Kind:       "NopResource",
+				}))),
 			)).
 			WithTeardown("DeletePrerequisites", funcs.AllOf(
 				funcs.DeleteResourcesWithPropagationPolicy(manifests, "setup/*.yaml", metav1.DeletePropagationForeground),
 				funcs.ResourcesDeletedWithin(3*time.Minute, manifests, "setup/*.yaml"),
+
+				// TODO(negz): We're seeing revisions sticking
+				// around after Crossplane is uninstalled, which
+				// blocks deleting their CRDs. It's unclear how
+				// that's possible given that we're deleting the
+				// packages with foreground deletion, and
+				// waiting for the packages to be gone.
+				funcs.ListedResourcesDeletedWithin(1*time.Minute, &pkgv1.ProviderRevisionList{}),
+				funcs.ListedResourcesDeletedWithin(1*time.Minute, &pkgv1.FunctionRevisionList{}),
 			)).
 			// Uninstalling the Crossplane Helm chart doesn't remove its CRDs. We
 			// want to make sure they can be deleted cleanly. If they can't, it's a
@@ -208,6 +247,16 @@ func TestCrossplaneLifecycle(t *testing.T) {
 			Assess("DeleteClaim", funcs.AllOf(
 				funcs.DeleteResourcesWithPropagationPolicy(manifests, "claim.yaml", metav1.DeletePropagationForeground),
 				funcs.ResourcesDeletedWithin(30*time.Second, manifests, "claim.yaml"),
+
+				// TODO(negz): We're seeing composed resources
+				// sticking around after the claim is deleted,
+				// even though the claim is deleting the XR (and
+				// thus its composed resources) with foreground
+				// deletion. How is that possible?
+				funcs.ListedResourcesDeletedWithin(1*time.Minute, composed.NewList(composed.FromReferenceToList(corev1.ObjectReference{
+					APIVersion: "nop.crossplane.io/v1alpha1",
+					Kind:       "NopResource",
+				}))),
 			)).
 			WithTeardown("DeletePrerequisites", funcs.AllOf(
 				funcs.DeleteResourcesWithPropagationPolicy(manifests, "setup/*.yaml", metav1.DeletePropagationForeground),