From 223a2cfa4ec467c063308610d314a12b71acd308 Mon Sep 17 00:00:00 2001 From: Philippe Scorsolini Date: Thu, 16 Nov 2023 16:05:23 +0100 Subject: [PATCH 1/7] fix: use right service name for conversion webhooks in CRDs Signed-off-by: Philippe Scorsolini (cherry picked from commit f87b6da1d1ba4282a14ca1c91cf0d54c3b9cb82a) --- internal/controller/pkg/revision/establisher.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/pkg/revision/establisher.go b/internal/controller/pkg/revision/establisher.go index 4d903233a..f9cd1a291 100644 --- a/internal/controller/pkg/revision/establisher.go +++ b/internal/controller/pkg/revision/establisher.go @@ -276,7 +276,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) } From c5fca9c4e766a2c9b21570540f26a109724d5dae Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Thu, 16 Nov 2023 15:50:11 +0000 Subject: [PATCH 2/7] fix(deps): update module github.com/docker/docker to v24.0.7+incompatible [security] --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 7093e2819..84f7e1513 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index af1601ecd..89491da6d 100644 --- a/go.sum +++ b/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= From a53d20c88b9e517eb9d981a31698d24c0f957b3a Mon Sep 17 00:00:00 2001 From: Hasan Turken Date: Mon, 20 Nov 2023 09:30:13 +0300 Subject: [PATCH 3/7] Leave deletion of package SA to garbage collector Signed-off-by: Hasan Turken (cherry picked from commit f0ad2a26fcce70fa3c7abfd8f1d92415e7c9ad4e) --- .../pkg/revision/runtime_function.go | 12 ++++++---- .../pkg/revision/runtime_function_test.go | 23 +------------------ .../pkg/revision/runtime_provider.go | 12 ++++++---- .../pkg/revision/runtime_provider_test.go | 23 +------------------ 4 files changed, 16 insertions(+), 54 deletions(-) diff --git a/internal/controller/pkg/revision/runtime_function.go b/internal/controller/pkg/revision/runtime_function.go index 4776e9361..cf4f92673 100644 --- a/internal/controller/pkg/revision/runtime_function.go +++ b/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 diff --git a/internal/controller/pkg/revision/runtime_function_test.go b/internal/controller/pkg/revision/runtime_function_test.go index 58a84e1f4..53439df72 100644 --- a/internal/controller/pkg/revision/runtime_function_test.go +++ b/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") diff --git a/internal/controller/pkg/revision/runtime_provider.go b/internal/controller/pkg/revision/runtime_provider.go index 15ea238bc..02633da1c 100644 --- a/internal/controller/pkg/revision/runtime_provider.go +++ b/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 diff --git a/internal/controller/pkg/revision/runtime_provider_test.go b/internal/controller/pkg/revision/runtime_provider_test.go index 334866dba..2e4205f13 100644 --- a/internal/controller/pkg/revision/runtime_provider_test.go +++ b/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") From f70be543bac31f97d3eb48f0c08d6c18f4aed22d Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Fri, 17 Nov 2023 14:40:38 +0100 Subject: [PATCH 4/7] apiextensions/definition: don't implicitly wait for MR informer, we do that ourselves Signed-off-by: Dr. Stefan Schimanski (cherry picked from commit c6a9babd01fa543e75b412758193281f66c89fe0) --- internal/controller/apiextensions/definition/composed.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/apiextensions/definition/composed.go b/internal/controller/apiextensions/definition/composed.go index 4578ea53c..1fb643eee 100644 --- a/internal/controller/apiextensions/definition/composed.go +++ b/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) From 744a38317275661b5a810367de98be6e565a9a7c Mon Sep 17 00:00:00 2001 From: Hasan Turken Date: Mon, 20 Nov 2023 17:30:35 +0300 Subject: [PATCH 5/7] Release objects in parallel Signed-off-by: Hasan Turken (cherry picked from commit 6b8a425ca65d4b1f4ec8a82a5b22b1919792f468) --- .../controller/pkg/revision/establisher.go | 87 ++++++++++++------- .../pkg/revision/establisher_test.go | 58 ++++++++++++- 2 files changed, 110 insertions(+), 35 deletions(-) diff --git a/internal/controller/pkg/revision/establisher.go b/internal/controller/pkg/revision/establisher.go index f9cd1a291..496f6c0f0 100644 --- a/internal/controller/pkg/revision/establisher.go +++ b/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,39 +137,64 @@ 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 { + 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()) } - 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 + 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. } - // 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()) - } + if changed { + u.SetOwnerReferences(ors) + if err := e.client.Update(ctx, &u); err != nil { + return errors.Wrapf(err, errFmtUpdateOwnedObject, u.GetKind(), u.GetName()) + } + } + select { + case <-ctx.Done(): + return ctx.Err() + default: + return nil + } + }) + } + + if err := g.Wait(); err != nil { + return err } + return nil } diff --git a/internal/controller/pkg/revision/establisher_test.go b/internal/controller/pkg/revision/establisher_test.go index de85c1d11..45378aec7 100644 --- a/internal/controller/pkg/revision/establisher_test.go +++ b/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{ From e90e891b7788760502963f453a83033b86be3498 Mon Sep 17 00:00:00 2001 From: Hasan Turken Date: Tue, 21 Nov 2023 13:08:34 +0300 Subject: [PATCH 6/7] Stop the goroutine immediately if context is done while releasing objects Signed-off-by: Hasan Turken (cherry picked from commit 6c0f7b42ccf38033c24a8856b5cd1b6b7abf5ef7) --- internal/controller/pkg/revision/establisher.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/internal/controller/pkg/revision/establisher.go b/internal/controller/pkg/revision/establisher.go index 496f6c0f0..6f2dcf6e1 100644 --- a/internal/controller/pkg/revision/establisher.go +++ b/internal/controller/pkg/revision/establisher.go @@ -147,6 +147,12 @@ func (e *APIEstablisher) ReleaseObjects(ctx context.Context, parent v1.PackageRe for _, ref := range allObjs { ref := ref // Pin the loop variable. g.Go(func() error { + select { + case <-ctx.Done(): + return ctx.Err() + default: + } + u := unstructured.Unstructured{} u.SetAPIVersion(ref.APIVersion) u.SetKind(ref.Kind) @@ -182,12 +188,7 @@ func (e *APIEstablisher) ReleaseObjects(ctx context.Context, parent v1.PackageRe return errors.Wrapf(err, errFmtUpdateOwnedObject, u.GetKind(), u.GetName()) } } - select { - case <-ctx.Done(): - return ctx.Err() - default: - return nil - } + return nil }) } From 6ff74532f8d5bcb8b892e437ad5687809da59314 Mon Sep 17 00:00:00 2001 From: Hasan Turken Date: Tue, 21 Nov 2023 14:42:47 +0300 Subject: [PATCH 7/7] Fix linter in release object by removing redundant if Signed-off-by: Hasan Turken (cherry picked from commit 404cc5d465e5df425c7e6c9209e87f604fe46206) --- internal/controller/pkg/revision/establisher.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/internal/controller/pkg/revision/establisher.go b/internal/controller/pkg/revision/establisher.go index 6f2dcf6e1..975586f3e 100644 --- a/internal/controller/pkg/revision/establisher.go +++ b/internal/controller/pkg/revision/establisher.go @@ -192,11 +192,7 @@ func (e *APIEstablisher) ReleaseObjects(ctx context.Context, parent v1.PackageRe }) } - if err := g.Wait(); err != nil { - return err - } - - return nil + return g.Wait() } func (e *APIEstablisher) addLabels(objs []runtime.Object, parent v1.PackageRevision) error {