Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Leave deletion of package service account to garbage collector #5039

Merged
merged 1 commit into from
Nov 20, 2023
Merged

Leave deletion of package service account to garbage collector #5039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 7 additions & 5 deletions internal/controller/pkg/revision/runtime_function.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
23 changes: 1 addition & 22 deletions internal/controller/pkg/revision/runtime_function_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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")
Expand Down
12 changes: 7 additions & 5 deletions internal/controller/pkg/revision/runtime_provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
23 changes: 1 addition & 22 deletions internal/controller/pkg/revision/runtime_provider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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")
Expand Down