Merge pull request #111 from turkenh/sync-upstream-release-1.14

Sync upstream release 1.14 up to v1.14.2

go.mod

@@ -9,7 +9,7 @@ require (
 	github.com/alecthomas/kong v0.8.1
 	github.com/bufbuild/buf v1.27.1
 	github.com/crossplane/crossplane-runtime v1.14.2
-	github.com/docker/docker v24.0.6+incompatible
+	github.com/docker/docker v24.0.7+incompatible
 	github.com/docker/go-connections v0.4.0
 	github.com/emicklei/dot v1.6.0
 	github.com/go-git/go-billy/v5 v5.5.0

go.sum

@@ -183,8 +183,8 @@ github.com/docker/cli v24.0.6+incompatible h1:fF+XCQCgJjjQNIMjzaSmiKJSCcfcXb3TWT
 github.com/docker/cli v24.0.6+incompatible/go.mod h1:JLrzqnKDaYBop7H2jaqPtU4hHvMKP+vjCwu2uszcLI8=
 github.com/docker/distribution v2.8.3+incompatible h1:AtKxIZ36LoNK51+Z6RpzLpddBirtxJnzDrHLEKxTAYk=
 github.com/docker/distribution v2.8.3+incompatible/go.mod h1:J2gT2udsDAN96Uj4KfcMRqY0/ypR+oyYUYmja8H+y+w=
-github.com/docker/docker v24.0.6+incompatible h1:hceabKCtUgDqPu+qm0NgsaXf28Ljf4/pWFL7xjWWDgE=
-github.com/docker/docker v24.0.6+incompatible/go.mod h1:eEKB0N0r5NX/I1kEveEz05bcu8tLC/8azJZsviup8Sk=
+github.com/docker/docker v24.0.7+incompatible h1:Wo6l37AuwP3JaMnZa226lzVXGA3F9Ig1seQen0cKYlM=
+github.com/docker/docker v24.0.7+incompatible/go.mod h1:eEKB0N0r5NX/I1kEveEz05bcu8tLC/8azJZsviup8Sk=
 github.com/docker/docker-credential-helpers v0.7.0/go.mod h1:rETQfLdHNT3foU5kuNkFR1R1V12OJRRO5lzt2D1b5X0=
 github.com/docker/docker-credential-helpers v0.8.0 h1:YQFtbBQb4VrpoPxhFuzEBPQ9E16qz5SpHLS+uswaCp8=
 github.com/docker/docker-credential-helpers v0.8.0/go.mod h1:UGFXcuoQ5TxPiB54nHOZ32AWRqQdECoh/Mg0AlEYb40=

internal/controller/apiextensions/definition/composed.go

@@ -156,7 +156,7 @@ func (i *composedResourceInformers) WatchComposedResources(gvks ...schema.GroupV
 
 		u := kunstructured.Unstructured{}
 		u.SetGroupVersionKind(gvk)
-		inf, err := ca.GetInformer(ctx, &u)
+		inf, err := ca.GetInformer(ctx, &u, cache.BlockUntilSynced(false)) // don't block. We wait in the go routine below.
 		if err != nil {
 			cancelFn()
 			log.Debug("failed getting informer", "error", err)

internal/controller/pkg/revision/establisher.go

@@ -128,7 +128,7 @@ func (e *APIEstablisher) Establish(ctx context.Context, objs []runtime.Object, p
 
 // ReleaseObjects removes control of owned resources in the API server for a
 // package revision.
-func (e *APIEstablisher) ReleaseObjects(ctx context.Context, parent v1.PackageRevision) error {
+func (e *APIEstablisher) ReleaseObjects(ctx context.Context, parent v1.PackageRevision) error { //nolint:gocyclo // complexity coming from parallelism.
 	// Note(turkenh): We rely on status.objectRefs to get the list of objects
 	// that are controlled by the package revision. Relying on the status is
 	// not ideal as it might get lost (e.g. if the status subresource is
@@ -137,40 +137,62 @@ func (e *APIEstablisher) ReleaseObjects(ctx context.Context, parent v1.PackageRe
 	// referenced resources available and rebuilding the status.
 	// In the next reconciliation loop, and we will be able to remove the
 	// control/ownership of the objects using the new status.
-	objRefs := parent.GetObjects()
-	// Stop controlling objects.
-	for _, ref := range objRefs {
-		u := unstructured.Unstructured{}
-		u.SetAPIVersion(ref.APIVersion)
-		u.SetKind(ref.Kind)
-		u.SetName(ref.Name)
-
-		if err := e.client.Get(ctx, types.NamespacedName{Name: u.GetName()}, &u); err != nil {
-			if kerrors.IsNotFound(err) {
-				// This is not expected, but still not an error for relinquishing.
-				continue
+	allObjs := parent.GetObjects()
+	if len(allObjs) == 0 {
+		return nil
+	}
+
+	g, ctx := errgroup.WithContext(ctx)
+	g.SetLimit(maxConcurrentEstablishers)
+	for _, ref := range allObjs {
+		ref := ref // Pin the loop variable.
+		g.Go(func() error {
+			select {
+			case <-ctx.Done():
+				return ctx.Err()
+			default:
 			}
-			return errors.Wrapf(err, errFmtGetOwnedObject, u.GetKind(), u.GetName())
-		}
-		ors := u.GetOwnerReferences()
-		for i := range ors {
-			if ors[i].UID == parent.GetUID() {
-				ors[i].Controller = ptr.To(false)
-				break
+
+			u := unstructured.Unstructured{}
+			u.SetAPIVersion(ref.APIVersion)
+			u.SetKind(ref.Kind)
+			u.SetName(ref.Name)
+
+			if err := e.client.Get(ctx, types.NamespacedName{Name: u.GetName()}, &u); err != nil {
+				if kerrors.IsNotFound(err) {
+					// This is not expected, but still not an error for releasing objects.
+					return nil
+				}
+				return errors.Wrapf(err, errFmtGetOwnedObject, u.GetKind(), u.GetName())
 			}
-			// Note(turkenh): What if we cannot find our UID in the owner
-			// references? This is not expected unless another party stripped
-			// out ownerRefs. I believe this is a fairly unlikely scenario,
-			// and we can ignore it for now especially considering that if that
-			// happens active revision or the package itself will still take
-			// over the ownership of such resources.
-		}
-		u.SetOwnerReferences(ors)
-		if err := e.client.Update(ctx, &u); err != nil {
-			return errors.Wrapf(err, errFmtUpdateOwnedObject, u.GetKind(), u.GetName())
-		}
+			ors := u.GetOwnerReferences()
+			changed := false
+			for i := range ors {
+				if ors[i].UID == parent.GetUID() {
+					if ors[i].Controller != nil && *ors[i].Controller {
+						ors[i].Controller = ptr.To(false)
+						changed = true
+					}
+					break
+				}
+				// Note(turkenh): What if we cannot find our UID in the owner
+				// references? This is not expected unless another party stripped
+				// out ownerRefs. I believe this is a fairly unlikely scenario,
+				// and we can ignore it for now especially considering that if that
+				// happens active revision or the package itself will still take
+				// over the ownership of such resources.
+			}
+			if changed {
+				u.SetOwnerReferences(ors)
+				if err := e.client.Update(ctx, &u); err != nil {
+					return errors.Wrapf(err, errFmtUpdateOwnedObject, u.GetKind(), u.GetName())
+				}
+			}
+			return nil
+		})
 	}
-	return nil
+
+	return g.Wait()
 }
 
 func (e *APIEstablisher) addLabels(objs []runtime.Object, parent v1.PackageRevision) error {
@@ -276,7 +298,7 @@ func (e *APIEstablisher) validate(ctx context.Context, objs []runtime.Object, pa
 						conf.Spec.Conversion.Webhook.ClientConfig.Service = &extv1.ServiceReference{}
 					}
 					conf.Spec.Conversion.Webhook.ClientConfig.CABundle = webhookTLSCert
-					conf.Spec.Conversion.Webhook.ClientConfig.Service.Name = parent.GetName()
+					conf.Spec.Conversion.Webhook.ClientConfig.Service.Name = parent.GetLabels()[v1.LabelParentPackage]
 					conf.Spec.Conversion.Webhook.ClientConfig.Service.Namespace = e.namespace
 					conf.Spec.Conversion.Webhook.ClientConfig.Service.Port = ptr.To[int32](servicePort)
 				}

internal/controller/pkg/revision/establisher_test.go

@@ -422,7 +422,7 @@ func TestAPIEstablisherEstablish(t *testing.T) {
 	}
 }
 
-func TestAPIEstablisherRelinquish(t *testing.T) {
+func TestAPIEstablisherReleaseObjects(t *testing.T) {
 	errBoom := errors.New("boom")
 	controls := true
 	noControl := false
@@ -499,7 +499,7 @@ func TestAPIEstablisherRelinquish(t *testing.T) {
 				err: nil,
 			},
 		},
-		"CannotGetUpdate": {
+		"CannotUpdate": {
 			reason: "Should return an error if we cannot update the owned object.",
 			args: args{
 				est: &APIEstablisher{
@@ -571,8 +571,58 @@ func TestAPIEstablisherRelinquish(t *testing.T) {
 				err: nil,
 			},
 		},
-		"SuccessfulRelinquish": {
-			reason: "ReleaseObjects should be successful if we can relinquish control of existing objects",
+		"AlreadyReleased": {
+			reason: "ReleaseObjects should make no updates if the object is already released.",
+			args: args{
+				est: &APIEstablisher{
+					client: &test.MockClient{
+						MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error {
+							o := obj.(*unstructured.Unstructured)
+							o.SetOwnerReferences([]metav1.OwnerReference{
+								{
+									APIVersion: "pkg.crossplane.io/v1",
+									Kind:       "Provider",
+									Name:       "provider-helm",
+									UID:        "some-other-uid-1234",
+									Controller: &noControl,
+								},
+								{
+									APIVersion: "pkg.crossplane.io/v1",
+									Kind:       "ProviderRevision",
+									Name:       "provider-helm-ce18dd03e6e4",
+									UID:        "some-unique-uid-2312",
+									Controller: &noControl,
+								},
+							})
+							return nil
+						},
+						MockUpdate: func(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error {
+							t.Errorf("should not have called update")
+							return nil
+						},
+					},
+				},
+				parent: &v1.ProviderRevision{
+					ObjectMeta: metav1.ObjectMeta{
+						UID: "some-unique-uid-2312",
+					},
+					Status: v1.PackageRevisionStatus{
+						ObjectRefs: []xpv1.TypedReference{
+							{
+								APIVersion: "apiextensions.k8s.io/v1",
+								Kind:       "CustomResourceDefinition",
+								Name:       "releases.helm.crossplane.io",
+							},
+						},
+					},
+				},
+			},
+			want: want{
+				err: nil,
+			},
+		},
+		"SuccessfulRelease": {
+			reason: "ReleaseObjects should be successful if we can release control of existing objects",
 			args: args{
 				est: &APIEstablisher{
 					client: &test.MockClient{

internal/controller/pkg/revision/runtime_function.go

@@ -145,11 +145,6 @@ func (h *FunctionHooks) Post(ctx context.Context, pkg runtime.Object, pr v1.Pack
 // Deactivate performs operations meant to happen before deactivating a revision.
 func (h *FunctionHooks) Deactivate(ctx context.Context, _ v1.PackageRevisionWithRuntime, build ManifestBuilder) error {
 	sa := build.ServiceAccount()
-	// Delete the service account if it exists.
-	if err := h.client.Delete(ctx, sa); resource.IgnoreNotFound(err) != nil {
-		return errors.Wrap(err, errDeleteFunctionSA)
-	}
-
 	// Delete the deployment if it exists.
 	// Different from the Post runtimeHook, we don't need to pass the
 	// "functionDeploymentOverrides()" here, because we're only interested
@@ -158,6 +153,13 @@ func (h *FunctionHooks) Deactivate(ctx context.Context, _ v1.PackageRevisionWith
 		return errors.Wrap(err, errDeleteFunctionDeployment)
 	}
 
+	// NOTE(turkenh): We don't delete the service account here because it might
+	// be used by other package revisions, e.g. user might have specified a
+	// service account name in the runtime config. This should not be a problem
+	// because we add the owner reference to the service account when we create
+	// them, and they will be garbage collected when the package revision is
+	// deleted if they are not used by any other package revisions.
+
 	// NOTE(ezgidemirel): Service and secret are created per package. Therefore,
 	// we're not deleting them here.
 	return nil

internal/controller/pkg/revision/runtime_function_test.go

@@ -484,24 +484,6 @@ func TestFunctionDeactivateHook(t *testing.T) {
 		args   args
 		want   want
 	}{
-		"ErrDeleteSA": {
-			reason: "Should return error if we fail to delete service account.",
-			args: args{
-				manifests: &MockManifestBuilder{
-					ServiceAccountFn: func(overrides ...ServiceAccountOverride) *corev1.ServiceAccount {
-						return &corev1.ServiceAccount{}
-					},
-				},
-				client: &test.MockClient{
-					MockDelete: func(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error {
-						return errBoom
-					},
-				},
-			},
-			want: want{
-				err: errors.Wrap(errBoom, errDeleteFunctionSA),
-			},
-		},
 		"ErrDeleteDeployment": {
 			reason: "Should return error if we fail to delete deployment.",
 			args: args{
@@ -560,10 +542,7 @@ func TestFunctionDeactivateHook(t *testing.T) {
 					MockDelete: func(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error {
 						switch obj.(type) {
 						case *corev1.ServiceAccount:
-							if obj.GetName() != "some-sa" {
-								return errors.New("unexpected service account name")
-							}
-							return nil
+							return errors.New("service account should not be deleted during deactivation")
 						case *appsv1.Deployment:
 							if obj.GetName() != "some-deployment" {
 								return errors.New("unexpected deployment name")

internal/controller/pkg/revision/runtime_provider.go

@@ -165,11 +165,6 @@ func (h *ProviderHooks) Post(ctx context.Context, pkg runtime.Object, pr v1.Pack
 // Deactivate performs operations meant to happen before deactivating a revision.
 func (h *ProviderHooks) Deactivate(ctx context.Context, pr v1.PackageRevisionWithRuntime, build ManifestBuilder) error {
 	sa := build.ServiceAccount()
-	// Delete the service account if it exists.
-	if err := h.client.Delete(ctx, sa); resource.IgnoreNotFound(err) != nil {
-		return errors.Wrap(err, errDeleteProviderSA)
-	}
-
 	// Delete the deployment if it exists.
 	// Different from the Post runtimeHook, we don't need to pass the
 	// "providerDeploymentOverrides()" here, because we're only interested
@@ -185,6 +180,13 @@ func (h *ProviderHooks) Deactivate(ctx context.Context, pr v1.PackageRevisionWit
 		return errors.Wrap(err, errDeleteProviderService)
 	}
 
+	// NOTE(turkenh): We don't delete the service account here because it might
+	// be used by other package revisions, e.g. user might have specified a
+	// service account name in the runtime config. This should not be a problem
+	// because we add the owner reference to the service account when we create
+	// them, and they will be garbage collected when the package revision is
+	// deleted if they are not used by any other package revisions.
+
 	// NOTE(phisco): Service and TLS secrets are created per package. Therefore,
 	// we're not deleting them here.
 	return nil

internal/controller/pkg/revision/runtime_provider_test.go

@@ -568,24 +568,6 @@ func TestProviderDeactivateHook(t *testing.T) {
 		args   args
 		want   want
 	}{
-		"ErrDeleteSA": {
-			reason: "Should return error if we fail to delete service account.",
-			args: args{
-				manifests: &MockManifestBuilder{
-					ServiceAccountFn: func(overrides ...ServiceAccountOverride) *corev1.ServiceAccount {
-						return &corev1.ServiceAccount{}
-					},
-				},
-				client: &test.MockClient{
-					MockDelete: func(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error {
-						return errBoom
-					},
-				},
-			},
-			want: want{
-				err: errors.Wrap(errBoom, errDeleteProviderSA),
-			},
-		},
 		"ErrDeleteDeployment": {
 			reason: "Should return error if we fail to delete deployment.",
 			args: args{
@@ -649,10 +631,7 @@ func TestProviderDeactivateHook(t *testing.T) {
 					MockDelete: func(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error {
 						switch obj.(type) {
 						case *corev1.ServiceAccount:
-							if obj.GetName() != "some-sa" {
-								return errors.New("unexpected service account name")
-							}
-							return nil
+							return errors.New("service account should not be deleted during deactivation")
 						case *appsv1.Deployment:
 							if obj.GetName() != "some-deployment" {
 								return errors.New("unexpected deployment name")