From 623751f8f383d710e7e0caa0b46ec6eb47a09419 Mon Sep 17 00:00:00 2001 From: Jithu M Kunjuraman Date: Mon, 11 Nov 2024 15:20:22 +0100 Subject: [PATCH] fix: Cleanup tags when component is removed (#28) When we receive a deletion request for the resource, we will also try to clean the TagBindings associated, which helps clean up unused tagValues/tagkeys residing on the GCP project Tested locally the following scenarios: ---------------------------------------- Multiple Resources created with multiple tags and deleting the resources only deletes the unused tags leaving behind the used tags(Since it is binding to other resources) Tested change in tag key-values which creates the new ones and deletes the old ones if unused. Creation and deletion of tags for single resources. --------- Co-authored-by: jijinp --- hack/verify-git-clean.sh | 4 +- internal/controller/taggable_controller.go | 36 ++++++++++++++++ internal/gcp/tags.go | 50 ++++++++++++++++++++++ 3 files changed, 88 insertions(+), 2 deletions(-) diff --git a/hack/verify-git-clean.sh b/hack/verify-git-clean.sh index 03d1814..78ca20b 100755 --- a/hack/verify-git-clean.sh +++ b/hack/verify-git-clean.sh @@ -1,7 +1,7 @@ #!/usr/bin/env bash set -euo pipefail - +go mod tidy if [ -n "$(git status --porcelain)" ]; then - echo "Working directory is not clean. Please run make generate helm, stash your changes or remove untracked files." + echo "Working directory is not clean. Please run make generate helm or run go mod tidy, stash your changes or remove untracked files." exit 1 fi diff --git a/internal/controller/taggable_controller.go b/internal/controller/taggable_controller.go index efb417c..7115806 100644 --- a/internal/controller/taggable_controller.go +++ b/internal/controller/taggable_controller.go @@ -98,6 +98,23 @@ func (r *TaggableResourceReconciler[T, P, PT]) Reconcile(ctx context.Context, re if err := r.Update(ctx, resource); err != nil { return ctrl.Result{}, err } + log.Info("resource deletion request received trying to delete associated tagValue/tagKey if unused") + projectID := r.determineProjectID(ctx, resource) + labels := resource.GetLabels() + for k, v := range r.LabelMatcher(labels) { + // return tagValue.Name, tagKey.Name, nil + valueID, keyID, err := r.getValueAndKeyID(ctx, projectID, k, v) + if err != nil { + return ctrl.Result{Requeue: true, RequeueAfter: 10 * time.Second}, err + } + if err := r.TagsManager.DeleteValueIfUnused(ctx, projectID, keyID, valueID); err != nil { + return ctrl.Result{Requeue: true, RequeueAfter: 10 * time.Second}, err + } + if err := r.TagsManager.DeleteKeyIfUnused(ctx, projectID, keyID); err != nil { + return ctrl.Result{Requeue: true, RequeueAfter: 10 * time.Second}, err + } + } + } // Stop reconciliation as the resource is being deleted return ctrl.Result{}, nil @@ -320,11 +337,30 @@ func (r *TaggableResourceReconciler[T, P, PT]) handleTagBindingsDeletion(ctx con return fmt.Errorf("error deleting tag binding %s: %w", tagBinding.Name, err) } } + // Cleanup finalizer + controllerutil.RemoveFinalizer(&tagBinding, "cnrm.cloud.google.com/finalizer") + if err := r.Update(ctx, &tagBinding); err != nil { + return fmt.Errorf("error removing finalizer from resource %s: %w", tagBinding.Name, err) + } } return nil } +func (r *TaggableResourceReconciler[T, P, PT]) getValueAndKeyID(ctx context.Context, projectID, key, value string) (string, string, error) { + tagValue, err := r.TagsManager.LookupValue(ctx, projectID, key, value) + if err != nil { + return "", "", fmt.Errorf("failed to lookup tag value: %w", err) + } + + tagKey, err := r.TagsManager.LookupKey(ctx, projectID, key) + if err != nil { + return "", "", fmt.Errorf("failed to lookup tag key: %w", err) + } + + return tagValue.Name, tagKey.Name, nil +} + func CreateTaggableResourceController[T any, P ResourceMetadataProvider[T], PT ResourcePointer[T]](mgr ctrl.Manager, tagsManager gcp.TagsManager, provider P, labelMatcher func(map[string]string) map[string]string) { if err := (&TaggableResourceReconciler[T, P, PT]{ Client: mgr.GetClient(), diff --git a/internal/gcp/tags.go b/internal/gcp/tags.go index d6db063..d45b337 100644 --- a/internal/gcp/tags.go +++ b/internal/gcp/tags.go @@ -39,6 +39,8 @@ type TagsManager interface { LookupValue(ctx context.Context, projectID string, key string, value string) (*resourcemanagerpb.TagValue, error) CreateValue(ctx context.Context, projectID string, key string, value string) (*resourcemanagerpb.TagValue, error) GetProjectInfo(ctx context.Context, projectID string) (*resourcemanagerpb.Project, error) + DeleteValueIfUnused(ctx context.Context, projectID string, key string, value string) error + DeleteKeyIfUnused(ctx context.Context, projectID string, key string) error } type tagsManager struct { @@ -178,3 +180,51 @@ func (m *tagsManager) GetProjectInfo(ctx context.Context, projectID string) (*re m.cache.Set(cacheKey, project, tagCacheDuration) return project, nil } + +func (m *tagsManager) DeleteValueIfUnused(ctx context.Context, projectID string, key string, value string) error { + + req := &resourcemanagerpb.DeleteTagValueRequest{ + Name: value, + } + + op, err := m.valuesClient.DeleteTagValue(ctx, req) + if err != nil { + var ae *apierror.APIError + if errors.As(err, &ae) && (ae.GRPCStatus().Code() == codes.FailedPrecondition || ae.GRPCStatus().Code() == codes.NotFound) { + // tag value is in use + return nil + } + return fmt.Errorf("failed to call tagValue deletion request: %w", err) + } + + _, err = op.Wait(ctx) + if err != nil { + return fmt.Errorf("failed to delete the tagValue %w", err) + } + + m.cache.Delete(cacheKeyTagValue(key, value)) + return nil +} + +func (m *tagsManager) DeleteKeyIfUnused(ctx context.Context, projectID string, key string) error { + + // Attempt to delete the tag key + req := &resourcemanagerpb.DeleteTagKeyRequest{ + Name: key, + } + op, err := m.keysClient.DeleteTagKey(ctx, req) + if err != nil { + var ae *apierror.APIError + if errors.As(err, &ae) && (ae.GRPCStatus().Code() == codes.FailedPrecondition || ae.GRPCStatus().Code() == codes.NotFound) { + return nil + } + return fmt.Errorf("failed to call tagKey deletion request: %w", err) + } + + _, err = op.Wait(ctx) + if err != nil { + return fmt.Errorf("failed to delete the tagKey %w", err) + } + m.cache.Delete(cacheKeyTagKey(key)) + return nil +}