From a2f74ef94773eda71b2234c72e518b5ec0ccbc99 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Tue, 27 May 2025 19:03:48 +0200 Subject: [PATCH 1/4] remove the bundled NSTemplateTiers that are no longer used --- cmd/main.go | 5 +- .../nstemplatetier_controller.go | 57 +++++++++- .../nstemplatetier_controller_test.go | 107 ++++++++++++++++-- go.mod | 2 + go.sum | 4 +- .../nstemplatetier_generator.go | 7 +- pkg/templates/resources.go | 5 +- 7 files changed, 166 insertions(+), 21 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index 75a2adfbe..f5b9c5112 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -275,8 +275,9 @@ func main() { // nolint:gocyclo os.Exit(1) } if err := (&nstemplatetier.Reconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Namespace: namespace, }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "NSTemplateTier") os.Exit(1) diff --git a/controllers/nstemplatetier/nstemplatetier_controller.go b/controllers/nstemplatetier/nstemplatetier_controller.go index f31e193c5..d0337aab5 100644 --- a/controllers/nstemplatetier/nstemplatetier_controller.go +++ b/controllers/nstemplatetier/nstemplatetier_controller.go @@ -4,12 +4,17 @@ import ( "context" "fmt" "reflect" + "slices" "time" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/host-operator/controllers/toolchainconfig" + "github.com/codeready-toolchain/host-operator/deploy" + "github.com/codeready-toolchain/host-operator/pkg/templates" "github.com/codeready-toolchain/host-operator/pkg/templates/nstemplatetiers" "github.com/codeready-toolchain/toolchain-common/pkg/condition" + "github.com/codeready-toolchain/toolchain-common/pkg/hash" + commonnstemplatetiers "github.com/codeready-toolchain/toolchain-common/pkg/template/nstemplatetiers" "github.com/redhat-cop/operator-utils/pkg/util" corev1 "k8s.io/api/core/v1" @@ -29,6 +34,10 @@ import ( // SetupWithManager sets up the controller with the Manager. func (r *Reconciler) SetupWithManager(mgr manager.Manager) error { + if err := r.initBundledNsTemplateTiersKeys(); err != nil { + return err + } + return ctrl.NewControllerManagedBy(mgr). For(&toolchainv1alpha1.NSTemplateTier{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})). Watches(&toolchainv1alpha1.TierTemplate{}, @@ -38,8 +47,31 @@ func (r *Reconciler) SetupWithManager(mgr manager.Manager) error { // Reconciler reconciles a NSTemplateTier object (only when this latter's specs were updated) type Reconciler struct { - Client runtimeclient.Client - Scheme *runtime.Scheme + Client runtimeclient.Client + Scheme *runtime.Scheme + Namespace string + // BundledNsTemplateTierKeys is the list of the object keys of the bundled ns template tiers. This should be initialized + // using SetupWithManager but the field is made public so that it can be set freely during the tests. + BundledNsTemplateTierKeys []runtimeclient.ObjectKey +} + +// initBundledNsTemplateTiersKeys initializes the internal list of the NSTemplateTiers bundled with the binary. This method +// is only public because it is also used in the tests. +func (r *Reconciler) initBundledNsTemplateTiersKeys() error { + metadata, files, err := nstemplatetiers.LoadFiles(deploy.NSTemplateTiersFS, nstemplatetiers.NsTemplateTierRootDir) + if err != nil { + return err + } + + return commonnstemplatetiers.GenerateTiers(r.Scheme, func(obj runtimeclient.Object, _ bool, _ string) (bool, error) { + // the bundled annotation is ensured on the template tiers only when they are created. It may not exist in the files on the embedded + // filesystem. Therefore, we're adding all the tiers from the filesystem unconditionally here and only check for the annotation + // in the reconcile method. + if _, ok := obj.(*toolchainv1alpha1.NSTemplateTier); ok { + r.BundledNsTemplateTierKeys = append(r.BundledNsTemplateTierKeys, runtimeclient.ObjectKeyFromObject(obj)) + } + return true, nil + }, r.Namespace, metadata, files) } //+kubebuilder:rbac:groups=toolchain.dev.openshift.com,resources=nstemplatetiers,verbs=get;list;watch;create;update;patch;delete @@ -67,6 +99,18 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. return reconcile.Result{}, nil } + if tier.Annotations[toolchainv1alpha1.BundledAnnotationKey] == templates.BundledWithHostOperatorAnnotationValue { + if !slices.Contains(r.BundledNsTemplateTierKeys, request.NamespacedName) { + // this tier used to be bundled with the operator but it is not anymore. + // if it is not used by anything, we are safe to delete it. + if used, err := r.isTierUsed(ctx, tier); err != nil { + return reconcile.Result{}, err + } else if !used { + return reconcile.Result{}, r.Client.Delete(ctx, tier) + } + } + } + _, err := toolchainconfig.GetToolchainConfig(r.Client) if err != nil { return reconcile.Result{}, fmt.Errorf("unable to get ToolchainConfig: %w", err) @@ -91,6 +135,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. return reconcile.Result{}, err } +func (r *Reconciler) isTierUsed(ctx context.Context, tier *toolchainv1alpha1.NSTemplateTier) (bool, error) { + list := &toolchainv1alpha1.SpaceList{} + if err := r.Client.List(ctx, list, runtimeclient.InNamespace(r.Namespace), runtimeclient.HasLabels{hash.TemplateTierHashLabelKey(tier.Name)}); err != nil { + return false, err + } + return len(list.Items) > 0, nil +} + // ensureRevision ensures that there is a TierTemplateRevision CR for each of the TierTemplate. // returns `true` if a new TierTemplateRevision CR was created, `err` if something wrong happened func (r *Reconciler) ensureRevision(ctx context.Context, nsTmplTier *toolchainv1alpha1.NSTemplateTier) (bool, error) { @@ -183,7 +235,6 @@ func (r *Reconciler) ensureTTRforTemplate(ctx context.Context, nsTmplTier *toolc return false, "", err } return true, ttrName, nil - } func (r *Reconciler) createNewTierTemplateRevision(ctx context.Context, nsTmplTier *toolchainv1alpha1.NSTemplateTier, tierTemplate *toolchainv1alpha1.TierTemplate) (string, error) { diff --git a/controllers/nstemplatetier/nstemplatetier_controller_test.go b/controllers/nstemplatetier/nstemplatetier_controller_test.go index 23aa79499..16f51c2f7 100644 --- a/controllers/nstemplatetier/nstemplatetier_controller_test.go +++ b/controllers/nstemplatetier/nstemplatetier_controller_test.go @@ -11,8 +11,10 @@ import ( toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/host-operator/controllers/nstemplatetier" "github.com/codeready-toolchain/host-operator/pkg/apis" + "github.com/codeready-toolchain/host-operator/pkg/templates" tiertest "github.com/codeready-toolchain/host-operator/test/nstemplatetier" "github.com/codeready-toolchain/host-operator/test/tiertemplaterevision" + "github.com/codeready-toolchain/toolchain-common/pkg/hash" "github.com/codeready-toolchain/toolchain-common/pkg/test" templatev1 "github.com/openshift/api/template/v1" "github.com/stretchr/testify/assert" @@ -37,9 +39,7 @@ const ( func TestReconcile(t *testing.T) { // given t.Run("failures", func(t *testing.T) { - t.Run("unable to get NSTemplateTier", func(t *testing.T) { - t.Run("tier not found", func(t *testing.T) { // given base1nsTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates) @@ -75,7 +75,6 @@ func TestReconcile(t *testing.T) { assert.Equal(t, reconcile.Result{}, res) // no explicit requeue }) }) - }) t.Run("revisions management", func(t *testing.T) { @@ -194,7 +193,6 @@ func TestReconcile(t *testing.T) { require.Len(t, ttrs.Items, len(tierTemplatesRefs)) // it's one TTR per each tiertemplate in the NSTemplateTier }) }) - }) t.Run("revision field is set but TierTemplateRevision CRs are missing, they should be created", func(t *testing.T) { @@ -264,7 +262,6 @@ func TestReconcile(t *testing.T) { }) t.Run("errors", func(t *testing.T) { - t.Run("error when TierTemplate is missing ", func(t *testing.T) { // given // make sure revisions field is nill before starting the test @@ -285,9 +282,7 @@ func TestReconcile(t *testing.T) { // and the TierTemplateRevision CRs are not created tiertemplaterevision.AssertThatTTRs(t, cl, base1nsTier.GetNamespace()).DoNotExist() }) - }) - }) t.Run("if being deleted, then do nothing", func(t *testing.T) { @@ -320,6 +315,94 @@ func TestReconcile(t *testing.T) { }) }) + t.Run("bundled tiers", func(t *testing.T) { + for _, setup := range []struct { + bundled bool + annotatedBundled bool + used bool + }{ + { + bundled: false, + annotatedBundled: false, + used: false, + }, + { + bundled: false, + annotatedBundled: false, + used: true, + }, + { + bundled: false, + annotatedBundled: true, + used: false, + }, + { + bundled: false, + annotatedBundled: true, + used: true, + }, + { + bundled: true, + annotatedBundled: false, + used: false, + }, + { + bundled: true, + annotatedBundled: false, + used: true, + }, + { + bundled: true, + annotatedBundled: true, + used: false, + }, + { + bundled: true, + annotatedBundled: true, + used: true, + }, + } { + t.Run(fmt.Sprintf("tier: bundled=%v, annotatedBundled=%v, used=%v", setup.bundled, setup.annotatedBundled, setup.used), func(t *testing.T) { + // given + base1nsTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithParameter("DEPLOYMENT_QUOTA", "60")) + objs := initTierTemplates(t, withTemplateObjects(newTestCRQ("600")), base1nsTier.Name) + objs = append(objs, base1nsTier) + + if setup.annotatedBundled { + if base1nsTier.Annotations == nil { + base1nsTier.Annotations = map[string]string{} + } + base1nsTier.Annotations[toolchainv1alpha1.BundledAnnotationKey] = templates.BundledWithHostOperatorAnnotationValue + } + + if setup.used { + space := &toolchainv1alpha1.Space{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-space", + Namespace: base1nsTier.Namespace, + Labels: map[string]string{hash.TemplateTierHashLabelKey(base1nsTier.Name): "1234"}, + }, + } + objs = append(objs, space) + } + + r, req, cl := prepareReconcile(t, base1nsTier.Name, objs...) + + if setup.bundled { + r.BundledNsTemplateTierKeys = append(r.BundledNsTemplateTierKeys, runtimeclient.ObjectKeyFromObject(base1nsTier)) + } + + // when + _, err := r.Reconcile(context.TODO(), req) + gerr := cl.Get(context.TODO(), runtimeclient.ObjectKeyFromObject(base1nsTier), &toolchainv1alpha1.NSTemplateTier{}) + + // then + shouldBeDeleted := !setup.bundled && setup.annotatedBundled && !setup.used + require.NoError(t, err) + assert.Equal(t, shouldBeDeleted, errors.IsNotFound(gerr)) + }) + } + }) } func TestUpdateNSTemplateTier(t *testing.T) { @@ -454,13 +537,11 @@ func TestUpdateNSTemplateTier(t *testing.T) { HasStatusTierTemplateRevisions(tierTemplatesRefs).Tier() require.NotEqual(t, tierBeingUpdated.Status.Revisions, newNSTmplTier.Status.Revisions) }) - }) - } func newTestCRQ(podsCount string) unstructured.Unstructured { - var crq = unstructured.Unstructured{Object: map[string]interface{}{ + crq := unstructured.Unstructured{Object: map[string]interface{}{ "kind": "ClusterResourceQuota", "metadata": map[string]interface{}{ "name": "for-{{.SPACE_NAME}}-deployments", @@ -509,9 +590,11 @@ func prepareReconcile(t *testing.T, name string, initObjs ...runtimeclient.Objec require.NoError(t, err) cl := test.NewFakeClient(t, initObjs...) r := &nstemplatetier.Reconciler{ - Client: cl, - Scheme: s, + Client: cl, + Scheme: s, + Namespace: test.HostOperatorNs, } + return r, reconcile.Request{ NamespacedName: types.NamespacedName{ Name: name, diff --git a/go.mod b/go.mod index 19bf00e84..c6371d374 100644 --- a/go.mod +++ b/go.mod @@ -133,3 +133,5 @@ require ( go 1.22.0 toolchain go1.22.12 + +replace github.com/codeready-toolchain/api => github.com/metlos/api v0.0.0-20250604131500-1d064aa4a843 diff --git a/go.sum b/go.sum index a25058bf8..52e34d6f9 100644 --- a/go.sum +++ b/go.sum @@ -38,8 +38,6 @@ github.com/cloudflare/circl v1.1.0/go.mod h1:prBCrKB9DV4poKZY1l9zBXg2QJY7mvgRvtM github.com/cloudflare/circl v1.3.7 h1:qlCDlTPz2n9fu58M0Nh1J/JzcFpfgkFHHX3O35r5vcU= github.com/cloudflare/circl v1.3.7/go.mod h1:sRTcRWXGLrKw6yIGJ+l7amYJFfAXbZG0kBSc8r4zxgA= github.com/cncf/udpa/go v0.0.0-20191209042840-269d4d468f6f/go.mod h1:M8M6+tZqaGXZJjfX53e64911xZQV5JYwmTeXPW+k8Sc= -github.com/codeready-toolchain/api v0.0.0-20250506092100-39b4862e1271 h1:AHrFr/aPuJ4+0zHw4sFXcfMA92kChy12JAPS5bLODlw= -github.com/codeready-toolchain/api v0.0.0-20250506092100-39b4862e1271/go.mod h1:20258od6i5+jP406Z76YaI2ow/vc7URwsDU2bokpkRE= github.com/codeready-toolchain/toolchain-common v0.0.0-20250506093954-2b65ad3a2e12 h1:w54sojJJ8PsHZzK1mC+/EUBrQ9F2sC/k7JUVc8LSqK4= github.com/codeready-toolchain/toolchain-common v0.0.0-20250506093954-2b65ad3a2e12/go.mod h1:TrMvD0sP69wI6Rouzfs7OsOUSj4CGn/ZiIdiDBAFQjk= github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= @@ -181,6 +179,8 @@ github.com/mailgun/mailgun-go/v4 v4.8.1 h1:1+MdKakJuXnW2JJDbyPdO1ngAANOyHyVPxQvF github.com/mailgun/mailgun-go/v4 v4.8.1/go.mod h1:FJlF9rI5cQT+mrwujtJjPMbIVy3Ebor9bKTVsJ0QU40= github.com/mailru/easyjson v0.7.7 h1:UGYAvKxe3sBsEDzO8ZeWOSlIQfWFlxbzLZe7hwFURr0= github.com/mailru/easyjson v0.7.7/go.mod h1:xzfreul335JAWq5oZzymOObrkdz5UnU4kGfJJLY9Nlc= +github.com/metlos/api v0.0.0-20250604131500-1d064aa4a843 h1:ayHY/PrRsI4NnRhX3e3OxW6o1/oGC03WQo8AwFQiqWQ= +github.com/metlos/api v0.0.0-20250604131500-1d064aa4a843/go.mod h1:shTTQ+bYWinbdF/oK+ly4DO5jdXtIASZ+uPZElsmsKg= github.com/migueleliasweb/go-github-mock v0.0.18 h1:0lWt9MYmZQGnQE2rFtjlft/YtD6hzxuN6JJRFpujzEI= github.com/migueleliasweb/go-github-mock v0.0.18/go.mod h1:CcgXcbMoRnf3rRVHqGssuBquZDIcaplxL2W6G+xs7kM= github.com/mitchellh/copystructure v1.0.0 h1:Laisrj+bAB6b/yJwB5Bt3ITZhGJdqmxquMKeZ+mmkFQ= diff --git a/pkg/templates/nstemplatetiers/nstemplatetier_generator.go b/pkg/templates/nstemplatetiers/nstemplatetier_generator.go index 56665e4ce..bd869538f 100644 --- a/pkg/templates/nstemplatetiers/nstemplatetier_generator.go +++ b/pkg/templates/nstemplatetiers/nstemplatetier_generator.go @@ -7,6 +7,7 @@ import ( "path/filepath" "strings" + toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/host-operator/deploy" "github.com/codeready-toolchain/host-operator/pkg/templates" commonclient "github.com/codeready-toolchain/toolchain-common/pkg/client" @@ -20,10 +21,13 @@ import ( const NsTemplateTierRootDir = "templates/nstemplatetiers" +var bundledAnnotation = map[string]string{ + toolchainv1alpha1.BundledAnnotationKey: templates.BundledWithHostOperatorAnnotationValue, +} + // CreateOrUpdateResources generates the NSTemplateTier resources from the cluster resource template and namespace templates, // then uses the manager's client to create or update the resources on the cluster. func CreateOrUpdateResources(ctx context.Context, s *runtime.Scheme, client runtimeclient.Client, namespace string) error { - metadata, files, err := LoadFiles(deploy.NSTemplateTiersFS, NsTemplateTierRootDir) if err != nil { return err @@ -31,6 +35,7 @@ func CreateOrUpdateResources(ctx context.Context, s *runtime.Scheme, client runt // initialize tier generator, loads templates from assets return nstemplatetiers.GenerateTiers(s, func(toEnsure runtimeclient.Object, canUpdate bool, _ string) (bool, error) { + commonclient.MergeAnnotations(toEnsure, bundledAnnotation) if !canUpdate { if err := client.Create(ctx, toEnsure); err != nil && !apierrors.IsAlreadyExists(err) { return false, err diff --git a/pkg/templates/resources.go b/pkg/templates/resources.go index a01ce0825..658e4c11f 100644 --- a/pkg/templates/resources.go +++ b/pkg/templates/resources.go @@ -5,8 +5,11 @@ import ( "io/fs" ) -func GetAllFileNames(TemplateTierFS *embed.FS, root string) (files []string, err error) { +// BundledWithHostOperatorAnnotationValue is meant to be the value of the toolchainv1alpha1.BundledLabelKey that marks +// the objects as bundled with the host operator and therefore managed by it. +const BundledWithHostOperatorAnnotationValue = "host-operator" +func GetAllFileNames(TemplateTierFS *embed.FS, root string) (files []string, err error) { if err := fs.WalkDir(TemplateTierFS, root, func(path string, d fs.DirEntry, err error) error { if d.IsDir() { return nil From 05bb8c9e9bf760c2910baad2ee87db7431e90a15 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Thu, 5 Jun 2025 17:23:57 +0200 Subject: [PATCH 2/4] use the latest api --- go.mod | 4 +--- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index c6371d374..21ee68b7b 100644 --- a/go.mod +++ b/go.mod @@ -2,7 +2,7 @@ module github.com/codeready-toolchain/host-operator require ( cloud.google.com/go/recaptchaenterprise/v2 v2.13.0 - github.com/codeready-toolchain/api v0.0.0-20250506092100-39b4862e1271 + github.com/codeready-toolchain/api v0.0.0-20250605152105-383ffe6cac27 github.com/codeready-toolchain/toolchain-common v0.0.0-20250506093954-2b65ad3a2e12 github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect github.com/go-logr/logr v1.4.2 @@ -133,5 +133,3 @@ require ( go 1.22.0 toolchain go1.22.12 - -replace github.com/codeready-toolchain/api => github.com/metlos/api v0.0.0-20250604131500-1d064aa4a843 diff --git a/go.sum b/go.sum index 52e34d6f9..a6f888ab1 100644 --- a/go.sum +++ b/go.sum @@ -38,6 +38,8 @@ github.com/cloudflare/circl v1.1.0/go.mod h1:prBCrKB9DV4poKZY1l9zBXg2QJY7mvgRvtM github.com/cloudflare/circl v1.3.7 h1:qlCDlTPz2n9fu58M0Nh1J/JzcFpfgkFHHX3O35r5vcU= github.com/cloudflare/circl v1.3.7/go.mod h1:sRTcRWXGLrKw6yIGJ+l7amYJFfAXbZG0kBSc8r4zxgA= github.com/cncf/udpa/go v0.0.0-20191209042840-269d4d468f6f/go.mod h1:M8M6+tZqaGXZJjfX53e64911xZQV5JYwmTeXPW+k8Sc= +github.com/codeready-toolchain/api v0.0.0-20250605152105-383ffe6cac27 h1:g1ivSPPHTC96RHp8S/gRmqODWgoHyivq+/d5kSI0pEs= +github.com/codeready-toolchain/api v0.0.0-20250605152105-383ffe6cac27/go.mod h1:20258od6i5+jP406Z76YaI2ow/vc7URwsDU2bokpkRE= github.com/codeready-toolchain/toolchain-common v0.0.0-20250506093954-2b65ad3a2e12 h1:w54sojJJ8PsHZzK1mC+/EUBrQ9F2sC/k7JUVc8LSqK4= github.com/codeready-toolchain/toolchain-common v0.0.0-20250506093954-2b65ad3a2e12/go.mod h1:TrMvD0sP69wI6Rouzfs7OsOUSj4CGn/ZiIdiDBAFQjk= github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= @@ -179,8 +181,6 @@ github.com/mailgun/mailgun-go/v4 v4.8.1 h1:1+MdKakJuXnW2JJDbyPdO1ngAANOyHyVPxQvF github.com/mailgun/mailgun-go/v4 v4.8.1/go.mod h1:FJlF9rI5cQT+mrwujtJjPMbIVy3Ebor9bKTVsJ0QU40= github.com/mailru/easyjson v0.7.7 h1:UGYAvKxe3sBsEDzO8ZeWOSlIQfWFlxbzLZe7hwFURr0= github.com/mailru/easyjson v0.7.7/go.mod h1:xzfreul335JAWq5oZzymOObrkdz5UnU4kGfJJLY9Nlc= -github.com/metlos/api v0.0.0-20250604131500-1d064aa4a843 h1:ayHY/PrRsI4NnRhX3e3OxW6o1/oGC03WQo8AwFQiqWQ= -github.com/metlos/api v0.0.0-20250604131500-1d064aa4a843/go.mod h1:shTTQ+bYWinbdF/oK+ly4DO5jdXtIASZ+uPZElsmsKg= github.com/migueleliasweb/go-github-mock v0.0.18 h1:0lWt9MYmZQGnQE2rFtjlft/YtD6hzxuN6JJRFpujzEI= github.com/migueleliasweb/go-github-mock v0.0.18/go.mod h1:CcgXcbMoRnf3rRVHqGssuBquZDIcaplxL2W6G+xs7kM= github.com/mitchellh/copystructure v1.0.0 h1:Laisrj+bAB6b/yJwB5Bt3ITZhGJdqmxquMKeZ+mmkFQ= From cd13c5d32e7095d0e2e8f4352887f9d2424c8c46 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Fri, 6 Jun 2025 17:11:27 +0200 Subject: [PATCH 3/4] bundled tiers are created and deleted in the generator, controller has a finalizer to control the deletion --- cmd/main.go | 7 +- .../nstemplatetier_controller.go | 87 ++++--- .../nstemplatetier_controller_test.go | 215 +++++++++--------- pkg/constants/constants.go | 13 ++ pkg/constants/field_manager.go | 5 - .../nstemplatetier_generator.go | 80 ++++++- .../nstemplatetier_generator_test.go | 146 +++++++++++- pkg/templates/resources.go | 4 - test/nstemplatetier/nstemplatetier.go | 12 + 9 files changed, 382 insertions(+), 187 deletions(-) create mode 100644 pkg/constants/constants.go delete mode 100644 pkg/constants/field_manager.go diff --git a/cmd/main.go b/cmd/main.go index f5b9c5112..a2416422a 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -275,9 +275,8 @@ func main() { // nolint:gocyclo os.Exit(1) } if err := (&nstemplatetier.Reconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - Namespace: namespace, + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "NSTemplateTier") os.Exit(1) @@ -426,7 +425,7 @@ func main() { // nolint:gocyclo // create or update all NSTemplateTiers on the cluster at startup setupLog.Info("Creating/updating the NSTemplateTier resources") - if err := nstemplatetiers.CreateOrUpdateResources(ctx, mgr.GetScheme(), mgr.GetClient(), namespace); err != nil { + if err := nstemplatetiers.SyncResources(ctx, mgr.GetScheme(), mgr.GetClient(), namespace); err != nil { setupLog.Error(err, "") os.Exit(1) } diff --git a/controllers/nstemplatetier/nstemplatetier_controller.go b/controllers/nstemplatetier/nstemplatetier_controller.go index d0337aab5..8a25a478a 100644 --- a/controllers/nstemplatetier/nstemplatetier_controller.go +++ b/controllers/nstemplatetier/nstemplatetier_controller.go @@ -4,17 +4,14 @@ import ( "context" "fmt" "reflect" - "slices" "time" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/host-operator/controllers/toolchainconfig" - "github.com/codeready-toolchain/host-operator/deploy" - "github.com/codeready-toolchain/host-operator/pkg/templates" + "github.com/codeready-toolchain/host-operator/pkg/constants" "github.com/codeready-toolchain/host-operator/pkg/templates/nstemplatetiers" "github.com/codeready-toolchain/toolchain-common/pkg/condition" "github.com/codeready-toolchain/toolchain-common/pkg/hash" - commonnstemplatetiers "github.com/codeready-toolchain/toolchain-common/pkg/template/nstemplatetiers" "github.com/redhat-cop/operator-utils/pkg/util" corev1 "k8s.io/api/core/v1" @@ -34,10 +31,6 @@ import ( // SetupWithManager sets up the controller with the Manager. func (r *Reconciler) SetupWithManager(mgr manager.Manager) error { - if err := r.initBundledNsTemplateTiersKeys(); err != nil { - return err - } - return ctrl.NewControllerManagedBy(mgr). For(&toolchainv1alpha1.NSTemplateTier{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})). Watches(&toolchainv1alpha1.TierTemplate{}, @@ -47,31 +40,8 @@ func (r *Reconciler) SetupWithManager(mgr manager.Manager) error { // Reconciler reconciles a NSTemplateTier object (only when this latter's specs were updated) type Reconciler struct { - Client runtimeclient.Client - Scheme *runtime.Scheme - Namespace string - // BundledNsTemplateTierKeys is the list of the object keys of the bundled ns template tiers. This should be initialized - // using SetupWithManager but the field is made public so that it can be set freely during the tests. - BundledNsTemplateTierKeys []runtimeclient.ObjectKey -} - -// initBundledNsTemplateTiersKeys initializes the internal list of the NSTemplateTiers bundled with the binary. This method -// is only public because it is also used in the tests. -func (r *Reconciler) initBundledNsTemplateTiersKeys() error { - metadata, files, err := nstemplatetiers.LoadFiles(deploy.NSTemplateTiersFS, nstemplatetiers.NsTemplateTierRootDir) - if err != nil { - return err - } - - return commonnstemplatetiers.GenerateTiers(r.Scheme, func(obj runtimeclient.Object, _ bool, _ string) (bool, error) { - // the bundled annotation is ensured on the template tiers only when they are created. It may not exist in the files on the embedded - // filesystem. Therefore, we're adding all the tiers from the filesystem unconditionally here and only check for the annotation - // in the reconcile method. - if _, ok := obj.(*toolchainv1alpha1.NSTemplateTier); ok { - r.BundledNsTemplateTierKeys = append(r.BundledNsTemplateTierKeys, runtimeclient.ObjectKeyFromObject(obj)) - } - return true, nil - }, r.Namespace, metadata, files) + Client runtimeclient.Client + Scheme *runtime.Scheme } //+kubebuilder:rbac:groups=toolchain.dev.openshift.com,resources=nstemplatetiers,verbs=get;list;watch;create;update;patch;delete @@ -96,19 +66,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. } // if the NSTemplateTier is being deleted, then do nothing if util.IsBeingDeleted(tier) { - return reconcile.Result{}, nil - } - - if tier.Annotations[toolchainv1alpha1.BundledAnnotationKey] == templates.BundledWithHostOperatorAnnotationValue { - if !slices.Contains(r.BundledNsTemplateTierKeys, request.NamespacedName) { - // this tier used to be bundled with the operator but it is not anymore. - // if it is not used by anything, we are safe to delete it. - if used, err := r.isTierUsed(ctx, tier); err != nil { - return reconcile.Result{}, err - } else if !used { - return reconcile.Result{}, r.Client.Delete(ctx, tier) - } - } + return reconcile.Result{}, r.handleDeletion(ctx, tier) } _, err := toolchainconfig.GetToolchainConfig(r.Client) @@ -135,9 +93,42 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. return reconcile.Result{}, err } -func (r *Reconciler) isTierUsed(ctx context.Context, tier *toolchainv1alpha1.NSTemplateTier) (bool, error) { +func (r *Reconciler) handleDeletion(ctx context.Context, tier *toolchainv1alpha1.NSTemplateTier) error { + if !util.HasFinalizer(tier, constants.BundledNSTemplateTierFinalizerName) { + // no special handling required for "ordinary" nstemplatetiers + return nil + } + + bundled, err := nstemplatetiers.IsBundled(runtimeclient.ObjectKeyFromObject(tier)) + if err != nil { + return fmt.Errorf("failed to find if a tier is bundled or not (this shouldn't happen): %w", err) + } + + if bundled { + // let's just keep the bundled tier around even if someone wants to delete it. It'd be recreated at + // the next start of the operator anyway. + return nil + } + + // We have a tier with a "bundled" finalizer that is not bundled - i.e. a tier that used to be bundled + // but is not anymore and that is being deleted. We allow such deletion only when the tier is not used + // by any space. + used, err := isTierUsed(ctx, r.Client, tier) + if err != nil { + return err + } + + if !used { + util.RemoveFinalizer(tier, constants.BundledNSTemplateTierFinalizerName) + err = r.Client.Update(ctx, tier) + } + + return err +} + +func isTierUsed(ctx context.Context, client runtimeclient.Client, tier *toolchainv1alpha1.NSTemplateTier) (bool, error) { list := &toolchainv1alpha1.SpaceList{} - if err := r.Client.List(ctx, list, runtimeclient.InNamespace(r.Namespace), runtimeclient.HasLabels{hash.TemplateTierHashLabelKey(tier.Name)}); err != nil { + if err := client.List(ctx, list, runtimeclient.InNamespace(tier.Namespace), runtimeclient.HasLabels{hash.TemplateTierHashLabelKey(tier.Name)}); err != nil { return false, err } return len(list.Items) > 0, nil diff --git a/controllers/nstemplatetier/nstemplatetier_controller_test.go b/controllers/nstemplatetier/nstemplatetier_controller_test.go index 16f51c2f7..1eb016d09 100644 --- a/controllers/nstemplatetier/nstemplatetier_controller_test.go +++ b/controllers/nstemplatetier/nstemplatetier_controller_test.go @@ -11,12 +11,14 @@ import ( toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/host-operator/controllers/nstemplatetier" "github.com/codeready-toolchain/host-operator/pkg/apis" - "github.com/codeready-toolchain/host-operator/pkg/templates" + "github.com/codeready-toolchain/host-operator/pkg/constants" + "github.com/codeready-toolchain/host-operator/pkg/templates/nstemplatetiers" tiertest "github.com/codeready-toolchain/host-operator/test/nstemplatetier" "github.com/codeready-toolchain/host-operator/test/tiertemplaterevision" "github.com/codeready-toolchain/toolchain-common/pkg/hash" "github.com/codeready-toolchain/toolchain-common/pkg/test" templatev1 "github.com/openshift/api/template/v1" + "github.com/redhat-cop/operator-utils/pkg/util" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/api/errors" @@ -29,6 +31,7 @@ import ( "k8s.io/apimachinery/pkg/util/validation" "k8s.io/client-go/kubernetes/scheme" runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) @@ -285,123 +288,126 @@ func TestReconcile(t *testing.T) { }) }) - t.Run("if being deleted, then do nothing", func(t *testing.T) { - // given - tierBeingDeleted := base1nsTier.DeepCopy() - tierBeingDeleted.DeletionTimestamp = &metav1.Time{Time: time.Now()} - tierBeingDeleted.Finalizers = []string{"dummy"} - r, req, cl := prepareReconcile(t, tierBeingDeleted.Name, tierBeingDeleted) - called := false - cl.MockGet = func(ctx context.Context, key runtimeclient.ObjectKey, obj runtimeclient.Object, opts ...runtimeclient.GetOption) error { - if called { - return fmt.Errorf("should not call Get more than once") + t.Run("if being deleted", func(t *testing.T) { + t.Run("do nothing on non-bundled tiers", func(t *testing.T) { + // given + tierBeingDeleted := base1nsTier.DeepCopy() + tierBeingDeleted.DeletionTimestamp = &metav1.Time{Time: time.Now()} + controllerutil.AddFinalizer(tierBeingDeleted, "dummy") // so that the fake client allows us to create an object with non-nil deletion timestamp + r, req, cl := prepareReconcile(t, tierBeingDeleted.Name, tierBeingDeleted) + + // when + res, err := r.Reconcile(context.TODO(), req) + inCluster := &toolchainv1alpha1.NSTemplateTier{} + require.NoError(t, cl.Get(context.TODO(), runtimeclient.ObjectKeyFromObject(tierBeingDeleted), inCluster)) + + // then + require.NoError(t, err) + assert.Empty(t, res.Requeue) + assert.Len(t, inCluster.Finalizers, 1) + assert.Contains(t, inCluster.Finalizers, "dummy") + }) + t.Run("keep the finalizer if no-longer-bundled tier is still used", func(t *testing.T) { + // given + tierBeingDeleted := base1nsTier.DeepCopy() + tierBeingDeleted.DeletionTimestamp = &metav1.Time{Time: time.Now()} + tierBeingDeleted.Annotations = map[string]string{toolchainv1alpha1.BundledAnnotationKey: constants.BundledWithHostOperatorAnnotationValue} + util.AddFinalizer(tierBeingDeleted, constants.BundledNSTemplateTierFinalizerName) + nstemplatetiers.BundledTierKeys = []runtimeclient.ObjectKey{} + space := &toolchainv1alpha1.Space{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-space", + Namespace: tierBeingDeleted.Namespace, + Labels: map[string]string{hash.TemplateTierHashLabelKey(tierBeingDeleted.Name): "1234"}, + }, } - called = true - return cl.Client.Get(ctx, key, obj, opts...) - } - cl.MockCreate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.CreateOption) error { - return fmt.Errorf("should not call Create") - } - cl.MockStatusUpdate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.SubResourceUpdateOption) error { - return fmt.Errorf("should not call StatusUpdate") - } - // when - res, err := r.Reconcile(context.TODO(), req) + r, req, cl := prepareReconcile(t, tierBeingDeleted.Name, tierBeingDeleted, space) - // then - require.NoError(t, err) - assert.Empty(t, res.Requeue) - }) - }) + // when + res, err := r.Reconcile(context.TODO(), req) + inCluster := &toolchainv1alpha1.NSTemplateTier{} + gerr := cl.Get(context.TODO(), runtimeclient.ObjectKeyFromObject(tierBeingDeleted), inCluster) - t.Run("bundled tiers", func(t *testing.T) { - for _, setup := range []struct { - bundled bool - annotatedBundled bool - used bool - }{ - { - bundled: false, - annotatedBundled: false, - used: false, - }, - { - bundled: false, - annotatedBundled: false, - used: true, - }, - { - bundled: false, - annotatedBundled: true, - used: false, - }, - { - bundled: false, - annotatedBundled: true, - used: true, - }, - { - bundled: true, - annotatedBundled: false, - used: false, - }, - { - bundled: true, - annotatedBundled: false, - used: true, - }, - { - bundled: true, - annotatedBundled: true, - used: false, - }, - { - bundled: true, - annotatedBundled: true, - used: true, - }, - } { - t.Run(fmt.Sprintf("tier: bundled=%v, annotatedBundled=%v, used=%v", setup.bundled, setup.annotatedBundled, setup.used), func(t *testing.T) { + // then + require.NoError(t, err) + require.Empty(t, res) + require.NoError(t, gerr) + require.Contains(t, inCluster.Finalizers, constants.BundledNSTemplateTierFinalizerName) + }) + t.Run("remove the finalizer if no-longer-bundled tier is not used anymore", func(t *testing.T) { // given - base1nsTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithParameter("DEPLOYMENT_QUOTA", "60")) - objs := initTierTemplates(t, withTemplateObjects(newTestCRQ("600")), base1nsTier.Name) - objs = append(objs, base1nsTier) + tierBeingDeleted := base1nsTier.DeepCopy() + tierBeingDeleted.DeletionTimestamp = &metav1.Time{Time: time.Now()} + tierBeingDeleted.Annotations = map[string]string{toolchainv1alpha1.BundledAnnotationKey: constants.BundledWithHostOperatorAnnotationValue} + util.AddFinalizer(tierBeingDeleted, constants.BundledNSTemplateTierFinalizerName) + nstemplatetiers.BundledTierKeys = []runtimeclient.ObjectKey{} - if setup.annotatedBundled { - if base1nsTier.Annotations == nil { - base1nsTier.Annotations = map[string]string{} - } - base1nsTier.Annotations[toolchainv1alpha1.BundledAnnotationKey] = templates.BundledWithHostOperatorAnnotationValue - } + r, req, cl := prepareReconcile(t, tierBeingDeleted.Name, tierBeingDeleted) - if setup.used { - space := &toolchainv1alpha1.Space{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-space", - Namespace: base1nsTier.Namespace, - Labels: map[string]string{hash.TemplateTierHashLabelKey(base1nsTier.Name): "1234"}, - }, - } - objs = append(objs, space) + // when + res, err := r.Reconcile(context.TODO(), req) + inCluster := &toolchainv1alpha1.NSTemplateTier{} + gerr := cl.Get(context.TODO(), runtimeclient.ObjectKeyFromObject(tierBeingDeleted), inCluster) + + // then + require.NoError(t, err) + require.Empty(t, res) + require.True(t, errors.IsNotFound(gerr)) + require.Empty(t, inCluster.Finalizers) + }) + t.Run("keep the finalizer if a bundled finalizer is still used", func(t *testing.T) { + // given + tierBeingDeleted := base1nsTier.DeepCopy() + tierBeingDeleted.DeletionTimestamp = &metav1.Time{Time: time.Now()} + tierBeingDeleted.Annotations = map[string]string{toolchainv1alpha1.BundledAnnotationKey: constants.BundledWithHostOperatorAnnotationValue} + util.AddFinalizer(tierBeingDeleted, constants.BundledNSTemplateTierFinalizerName) + nstemplatetiers.BundledTierKeys = []runtimeclient.ObjectKey{runtimeclient.ObjectKeyFromObject(tierBeingDeleted)} + space := &toolchainv1alpha1.Space{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-space", + Namespace: tierBeingDeleted.Namespace, + Labels: map[string]string{hash.TemplateTierHashLabelKey(tierBeingDeleted.Name): "1234"}, + }, } - r, req, cl := prepareReconcile(t, base1nsTier.Name, objs...) + r, req, cl := prepareReconcile(t, tierBeingDeleted.Name, tierBeingDeleted, space) - if setup.bundled { - r.BundledNsTemplateTierKeys = append(r.BundledNsTemplateTierKeys, runtimeclient.ObjectKeyFromObject(base1nsTier)) - } + // when + res, err := r.Reconcile(context.TODO(), req) + inCluster := &toolchainv1alpha1.NSTemplateTier{} + gerr := cl.Get(context.TODO(), runtimeclient.ObjectKeyFromObject(tierBeingDeleted), inCluster) + + // then + require.NoError(t, err) + require.Empty(t, res) + require.NoError(t, gerr) + require.Len(t, inCluster.Finalizers, 1) + require.Contains(t, inCluster.Finalizers, constants.BundledNSTemplateTierFinalizerName) + }) + t.Run("keep the finalizer if a bundled tier is not used", func(t *testing.T) { + // given + tierBeingDeleted := base1nsTier.DeepCopy() + tierBeingDeleted.DeletionTimestamp = &metav1.Time{Time: time.Now()} + tierBeingDeleted.Annotations = map[string]string{toolchainv1alpha1.BundledAnnotationKey: constants.BundledWithHostOperatorAnnotationValue} + util.AddFinalizer(tierBeingDeleted, constants.BundledNSTemplateTierFinalizerName) + nstemplatetiers.BundledTierKeys = []runtimeclient.ObjectKey{runtimeclient.ObjectKeyFromObject(tierBeingDeleted)} + + r, req, cl := prepareReconcile(t, tierBeingDeleted.Name, tierBeingDeleted) // when - _, err := r.Reconcile(context.TODO(), req) - gerr := cl.Get(context.TODO(), runtimeclient.ObjectKeyFromObject(base1nsTier), &toolchainv1alpha1.NSTemplateTier{}) + res, err := r.Reconcile(context.TODO(), req) + inCluster := &toolchainv1alpha1.NSTemplateTier{} + gerr := cl.Get(context.TODO(), runtimeclient.ObjectKeyFromObject(tierBeingDeleted), inCluster) // then - shouldBeDeleted := !setup.bundled && setup.annotatedBundled && !setup.used require.NoError(t, err) - assert.Equal(t, shouldBeDeleted, errors.IsNotFound(gerr)) + require.Empty(t, res) + require.NoError(t, gerr) + require.Len(t, inCluster.Finalizers, 1) + require.Contains(t, inCluster.Finalizers, constants.BundledNSTemplateTierFinalizerName) }) - } + }) }) } @@ -590,9 +596,8 @@ func prepareReconcile(t *testing.T, name string, initObjs ...runtimeclient.Objec require.NoError(t, err) cl := test.NewFakeClient(t, initObjs...) r := &nstemplatetier.Reconciler{ - Client: cl, - Scheme: s, - Namespace: test.HostOperatorNs, + Client: cl, + Scheme: s, } return r, reconcile.Request{ diff --git a/pkg/constants/constants.go b/pkg/constants/constants.go new file mode 100644 index 000000000..f040c94a3 --- /dev/null +++ b/pkg/constants/constants.go @@ -0,0 +1,13 @@ +package constants + +import toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" + +// HostOperatorFieldManager is the field manager we want to use in the managed fields +// of objects deployed by the host operator. +const HostOperatorFieldManager = "kubesaw-host-operator" + +// BundledWithHostOperatorAnnotationValue is meant to be the value of the toolchainv1alpha1.BundledLabelKey that marks +// the objects as bundled with the host operator and therefore managed by it. +const BundledWithHostOperatorAnnotationValue = "host-operator" + +const BundledNSTemplateTierFinalizerName = toolchainv1alpha1.LabelKeyPrefix + "bundled-tier" diff --git a/pkg/constants/field_manager.go b/pkg/constants/field_manager.go deleted file mode 100644 index 797bb2180..000000000 --- a/pkg/constants/field_manager.go +++ /dev/null @@ -1,5 +0,0 @@ -package constants - -// HostOperatorFieldManager is the field manager we want to use in the managed fields -// of objects deployed by the host operator. -const HostOperatorFieldManager = "kubesaw-host-operator" diff --git a/pkg/templates/nstemplatetiers/nstemplatetier_generator.go b/pkg/templates/nstemplatetiers/nstemplatetier_generator.go index bd869538f..a5d373938 100644 --- a/pkg/templates/nstemplatetiers/nstemplatetier_generator.go +++ b/pkg/templates/nstemplatetiers/nstemplatetier_generator.go @@ -3,16 +3,19 @@ package nstemplatetiers import ( "context" "embed" + "errors" "fmt" "path/filepath" + "slices" "strings" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/host-operator/deploy" + "github.com/codeready-toolchain/host-operator/pkg/constants" "github.com/codeready-toolchain/host-operator/pkg/templates" commonclient "github.com/codeready-toolchain/toolchain-common/pkg/client" "github.com/codeready-toolchain/toolchain-common/pkg/template/nstemplatetiers" - "github.com/pkg/errors" + "github.com/redhat-cop/operator-utils/pkg/util" "gopkg.in/yaml.v2" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" @@ -21,21 +24,37 @@ import ( const NsTemplateTierRootDir = "templates/nstemplatetiers" -var bundledAnnotation = map[string]string{ - toolchainv1alpha1.BundledAnnotationKey: templates.BundledWithHostOperatorAnnotationValue, -} +var ( + bundledAnnotation = map[string]string{ + toolchainv1alpha1.BundledAnnotationKey: constants.BundledWithHostOperatorAnnotationValue, + } + + // BundledTierKeys is the list of NSTemplateTiers that are bundled with the operator binary. + // This list is initialized during the SyncResources call. DO NOT MODIFY it unless you're in + // tests and need to setup a custom list of bundled tiers. + BundledTierKeys []runtimeclient.ObjectKey +) -// CreateOrUpdateResources generates the NSTemplateTier resources from the cluster resource template and namespace templates, -// then uses the manager's client to create or update the resources on the cluster. -func CreateOrUpdateResources(ctx context.Context, s *runtime.Scheme, client runtimeclient.Client, namespace string) error { +// SyncResources generates the NSTemplateTier resources from the cluster resource template and namespace templates, +// then uses the manager's client to create or update the resources on the cluster. It also deletes all the tiers +// that used to be bundled but are not anymore. Note that these tiers have finalizers ensuring that the deletion +// actually concludes only when such tiers are not used by any space. +func SyncResources(ctx context.Context, s *runtime.Scheme, client runtimeclient.Client, namespace string) error { metadata, files, err := LoadFiles(deploy.NSTemplateTiersFS, NsTemplateTierRootDir) if err != nil { return err } + // re-initialize in case this function got called multiple times, even though it really shouldn't + BundledTierKeys = nil + // initialize tier generator, loads templates from assets - return nstemplatetiers.GenerateTiers(s, func(toEnsure runtimeclient.Object, canUpdate bool, _ string) (bool, error) { + err = nstemplatetiers.GenerateTiers(s, func(toEnsure runtimeclient.Object, canUpdate bool, _ string) (bool, error) { commonclient.MergeAnnotations(toEnsure, bundledAnnotation) + util.AddFinalizer(toEnsure, constants.BundledNSTemplateTierFinalizerName) + + BundledTierKeys = append(BundledTierKeys, runtimeclient.ObjectKeyFromObject(toEnsure)) + if !canUpdate { if err := client.Create(ctx, toEnsure); err != nil && !apierrors.IsAlreadyExists(err) { return false, err @@ -45,6 +64,45 @@ func CreateOrUpdateResources(ctx context.Context, s *runtime.Scheme, client runt applyCl := commonclient.NewApplyClient(client) return applyCl.ApplyObject(ctx, toEnsure, commonclient.ForceUpdate(true)) }, namespace, metadata, files) + if err != nil { + return err + } + + return removeNoLongerBundledTiers(ctx, client, namespace, BundledTierKeys) +} + +// IsBundled tells whether the provided tier is bundled with the binary or not. +// Returns an error if called before SyncResources. +func IsBundled(tier runtimeclient.ObjectKey) (bool, error) { + if BundledTierKeys == nil { + return false, fmt.Errorf("SyncResources not called yet") + } + + return slices.Contains(BundledTierKeys, tier), nil +} + +func removeNoLongerBundledTiers(ctx context.Context, client runtimeclient.Client, namespace string, bundledTierKeys []runtimeclient.ObjectKey) error { + allTiers := &toolchainv1alpha1.NSTemplateTierList{} + if err := client.List(ctx, allTiers, runtimeclient.InNamespace(namespace)); err != nil { + return err + } + + var allErrors []error + for _, tier := range allTiers.Items { + if tier.Annotations[toolchainv1alpha1.BundledAnnotationKey] == constants.BundledWithHostOperatorAnnotationValue && + !slices.Contains(bundledTierKeys, runtimeclient.ObjectKeyFromObject(&tier)) { + if err := client.Delete(ctx, &tier); err != nil { + allErrors = append(allErrors, err) + } + } + } + + err := errors.Join(allErrors...) + if err != nil { + err = fmt.Errorf("failed to delete some of the no-longer-bundled NSTemplateTiers: %w", err) + } + + return err } // LoadFiles takes the file from deploy/nstemplatetiers// . the folder structure should be 4 steps . @@ -53,13 +111,13 @@ func LoadFiles(nsTemplateTiers embed.FS, root string) (metadata map[string]strin // load templates from assets metadataContent, err := nsTemplateTiers.ReadFile(filepath.Join(root, "metadata.yaml")) if err != nil { - return nil, nil, errors.Wrapf(err, "unable to load templates") + return nil, nil, fmt.Errorf("unable to load templates: %w", err) } metadata = make(map[string]string) err = yaml.Unmarshal(metadataContent, &metadata) if err != nil { - return nil, nil, errors.Wrapf(err, "unable to load templates") + return nil, nil, fmt.Errorf("unable to load templates: %w", err) } paths, err := templates.GetAllFileNames(&nsTemplateTiers, root) @@ -84,7 +142,7 @@ func LoadFiles(nsTemplateTiers embed.FS, root string) (metadata map[string]strin fileName := filepath.Join(parts[2], parts[3]) content, err := nsTemplateTiers.ReadFile(name) if err != nil { - return nil, nil, errors.Wrapf(err, "unable to load templates") + return nil, nil, fmt.Errorf("unable to load templates: %w", err) } files[fileName] = content } diff --git a/pkg/templates/nstemplatetiers/nstemplatetier_generator_test.go b/pkg/templates/nstemplatetiers/nstemplatetier_generator_test.go index c22adc13b..c681dd3c4 100644 --- a/pkg/templates/nstemplatetiers/nstemplatetier_generator_test.go +++ b/pkg/templates/nstemplatetiers/nstemplatetier_generator_test.go @@ -9,10 +9,13 @@ import ( toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/host-operator/deploy" "github.com/codeready-toolchain/host-operator/pkg/apis" + "github.com/codeready-toolchain/host-operator/pkg/constants" "github.com/codeready-toolchain/host-operator/pkg/templates/nstemplatetiers" + "github.com/codeready-toolchain/toolchain-common/pkg/hash" commontest "github.com/codeready-toolchain/toolchain-common/pkg/test" "github.com/gofrs/uuid" "github.com/pkg/errors" + "github.com/redhat-cop/operator-utils/pkg/util" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -50,8 +53,7 @@ func roles(_ string) []string { return []string{"admin"} } -func TestCreateOrUpdateResourcesWitProdAssets(t *testing.T) { - +func TestSyncResourcesWitProdAssets(t *testing.T) { s := scheme.Scheme err := apis.AddToScheme(s) require.NoError(t, err) @@ -60,7 +62,7 @@ func TestCreateOrUpdateResourcesWitProdAssets(t *testing.T) { cl := commontest.NewFakeClient(t) // when - err = nstemplatetiers.CreateOrUpdateResources(context.TODO(), s, cl, namespace) + err = nstemplatetiers.SyncResources(context.TODO(), s, cl, namespace) // then require.NoError(t, err) @@ -109,10 +111,8 @@ func TestCreateOrUpdateResourcesWitProdAssets(t *testing.T) { } t.Run("failures", func(t *testing.T) { - namespace := "host-operator" + uuid.Must(uuid.NewV4()).String()[:7] t.Run("nstemplatetiers", func(t *testing.T) { - t.Run("failed to create nstemplatetiers", func(t *testing.T) { // given clt := commontest.NewFakeClient(t) @@ -124,7 +124,7 @@ func TestCreateOrUpdateResourcesWitProdAssets(t *testing.T) { return clt.Client.Create(ctx, obj, opts...) } // when - err := nstemplatetiers.CreateOrUpdateResources(context.TODO(), s, clt, namespace) + err := nstemplatetiers.SyncResources(context.TODO(), s, clt, namespace) // then require.Error(t, err) assert.Regexp(t, "unable to create NSTemplateTiers: unable to create or update the 'base' NSTemplateTier: unable to create resource of kind: NSTemplateTier, version: v1alpha1: an error", err.Error()) @@ -148,7 +148,7 @@ func TestCreateOrUpdateResourcesWitProdAssets(t *testing.T) { } // when - err := nstemplatetiers.CreateOrUpdateResources(context.TODO(), s, clt, namespace) + err := nstemplatetiers.SyncResources(context.TODO(), s, clt, namespace) // then require.Error(t, err) assert.Contains(t, err.Error(), "unable to create NSTemplateTiers: unable to create or update the 'advanced' NSTemplateTier: unable to create resource of kind: NSTemplateTier, version: v1alpha1: unable to update the resource") @@ -156,7 +156,6 @@ func TestCreateOrUpdateResourcesWitProdAssets(t *testing.T) { }) t.Run("tiertemplates", func(t *testing.T) { - t.Run("failed to create nstemplatetiers", func(t *testing.T) { // given clt := commontest.NewFakeClient(t) @@ -168,13 +167,12 @@ func TestCreateOrUpdateResourcesWitProdAssets(t *testing.T) { return clt.Client.Create(ctx, obj, opts...) } // when - err := nstemplatetiers.CreateOrUpdateResources(context.TODO(), s, clt, namespace) + err := nstemplatetiers.SyncResources(context.TODO(), s, clt, namespace) // then require.Error(t, err) assert.Regexp(t, fmt.Sprintf("unable to create TierTemplates: unable to create the 'base1ns-dev-\\w+-\\w+' TierTemplate in namespace '%s'", namespace), err.Error()) // we can't tell for sure which namespace will fail first, but the error should match the given regex }) }) - }) t.Run("failed to load assets", func(t *testing.T) { // when @@ -183,7 +181,135 @@ func TestCreateOrUpdateResourcesWitProdAssets(t *testing.T) { require.Error(t, err) assert.Equal(t, "unable to load templates: open /templates/nstemplatetiers/metadata.yaml: file does not exist", err.Error()) // error occurred while creating TierTemplate resources }) + t.Run("bundled tiers", func(t *testing.T) { + for _, setup := range []struct { + bundled bool + annotatedBundled bool + used bool + }{ + { + bundled: false, + annotatedBundled: false, + used: false, + }, + { + bundled: false, + annotatedBundled: false, + used: true, + }, + { + bundled: false, + annotatedBundled: true, + used: false, + }, + { + bundled: false, + annotatedBundled: true, + used: true, + }, + { + bundled: true, + annotatedBundled: false, + used: false, + }, + { + bundled: true, + annotatedBundled: false, + used: true, + }, + { + bundled: true, + annotatedBundled: true, + used: false, + }, + { + bundled: true, + annotatedBundled: true, + used: true, + }, + } { + t.Run(fmt.Sprintf("tier: bundled=%v, annotatedBundled=%v, used=%v", setup.bundled, setup.annotatedBundled, setup.used), func(t *testing.T) { + // given + testTier := &toolchainv1alpha1.NSTemplateTier{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testTier", + Namespace: namespace, + }, + } + objs := []runtimeclient.Object{testTier} + + if setup.annotatedBundled { + testTier.Annotations = map[string]string{toolchainv1alpha1.BundledAnnotationKey: constants.BundledWithHostOperatorAnnotationValue} + util.AddFinalizer(testTier, constants.BundledNSTemplateTierFinalizerName) + } + + if setup.bundled { + testTier.Name = "base" // use the name of the tier that we know is bundled + } + + if setup.used { + space := &toolchainv1alpha1.Space{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-space", + Namespace: testTier.Namespace, + Labels: map[string]string{hash.TemplateTierHashLabelKey(testTier.Name): "1234"}, + }, + } + objs = append(objs, space) + } + + clt := commontest.NewFakeClient(t, objs...) + + // when + err := nstemplatetiers.SyncResources(context.TODO(), clt.Scheme(), clt, namespace) + inCluster := &toolchainv1alpha1.NSTemplateTier{} + gerr := clt.Get(context.TODO(), runtimeclient.ObjectKeyFromObject(testTier), inCluster) + + // then + shouldBeDeleted := !setup.bundled && setup.annotatedBundled && !setup.used + shouldBeAnnotated := setup.bundled || setup.annotatedBundled + require.NoError(t, err) + require.NoError(t, gerr) + if shouldBeDeleted { + assert.NotNil(t, inCluster.DeletionTimestamp) + } + if shouldBeAnnotated { + assert.Equal(t, constants.BundledWithHostOperatorAnnotationValue, inCluster.Annotations[toolchainv1alpha1.BundledAnnotationKey]) + assert.Contains(t, inCluster.Finalizers, constants.BundledNSTemplateTierFinalizerName) + } else { + assert.Empty(t, inCluster.Annotations) + assert.Empty(t, inCluster.Finalizers) + } + }) + } + }) +} + +func TestIsBundled(t *testing.T) { + t.Run("errors if BundledTiers not initialized", func(t *testing.T) { + // given + nstemplatetiers.BundledTierKeys = nil + // when + _, err := nstemplatetiers.IsBundled(runtimeclient.ObjectKey{}) + + // then + assert.Error(t, err) + }) + t.Run("checks if tier is in BundledTiers", func(t *testing.T) { + // given + nstemplatetiers.BundledTierKeys = []runtimeclient.ObjectKey{{Name: "a", Namespace: "ns"}} + + // when + resA, errA := nstemplatetiers.IsBundled(runtimeclient.ObjectKey{Name: "a", Namespace: "ns"}) + resB, errB := nstemplatetiers.IsBundled(runtimeclient.ObjectKey{Name: "b", Namespace: "ns"}) + + // then + require.NoError(t, errA) + require.NoError(t, errB) + assert.True(t, resA) + assert.False(t, resB) + }) } func verifyTierTemplate(t *testing.T, cl *commontest.FakeClient, namespace, tierName, typeName string) string { diff --git a/pkg/templates/resources.go b/pkg/templates/resources.go index 658e4c11f..b909b0238 100644 --- a/pkg/templates/resources.go +++ b/pkg/templates/resources.go @@ -5,10 +5,6 @@ import ( "io/fs" ) -// BundledWithHostOperatorAnnotationValue is meant to be the value of the toolchainv1alpha1.BundledLabelKey that marks -// the objects as bundled with the host operator and therefore managed by it. -const BundledWithHostOperatorAnnotationValue = "host-operator" - func GetAllFileNames(TemplateTierFS *embed.FS, root string) (files []string, err error) { if err := fs.WalkDir(TemplateTierFS, root, func(path string, d fs.DirEntry, err error) error { if d.IsDir() { diff --git a/test/nstemplatetier/nstemplatetier.go b/test/nstemplatetier/nstemplatetier.go index 2033744d7..d8db79465 100644 --- a/test/nstemplatetier/nstemplatetier.go +++ b/test/nstemplatetier/nstemplatetier.go @@ -5,9 +5,11 @@ import ( "testing" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" + "github.com/codeready-toolchain/host-operator/pkg/constants" "github.com/codeready-toolchain/toolchain-common/pkg/hash" "github.com/codeready-toolchain/toolchain-common/pkg/test" "github.com/codeready-toolchain/toolchain-common/pkg/test/nstemplateset" + "github.com/redhat-cop/operator-utils/pkg/util" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -245,6 +247,16 @@ func WithParameter(name, value string) TierOption { } } +func MarkedBundled() TierOption { + return func(tier *toolchainv1alpha1.NSTemplateTier) { + if tier.Annotations == nil { + tier.Annotations = map[string]string{} + } + tier.Annotations[toolchainv1alpha1.BundledAnnotationKey] = constants.BundledWithHostOperatorAnnotationValue + util.AddFinalizer(tier, constants.BundledNSTemplateTierFinalizerName) + } +} + // OtherTier returns an "other" NSTemplateTier func OtherTier(t *testing.T, options ...TierOption) *toolchainv1alpha1.NSTemplateTier { return Tier(t, "other", toolchainv1alpha1.NSTemplateTierSpec{ From 4303502c6db9bf705cb1dde22c93fd6b83d2013d Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Tue, 10 Jun 2025 14:40:29 +0200 Subject: [PATCH 4/4] use just the presence of the finalizer as the "witness" for a bundled tier --- .../nstemplatetier_controller.go | 7 +- .../nstemplatetier_generator.go | 21 ++---- .../nstemplatetier_generator_test.go | 71 +++++++++---------- 3 files changed, 45 insertions(+), 54 deletions(-) diff --git a/controllers/nstemplatetier/nstemplatetier_controller.go b/controllers/nstemplatetier/nstemplatetier_controller.go index 8a25a478a..dcd76a4b8 100644 --- a/controllers/nstemplatetier/nstemplatetier_controller.go +++ b/controllers/nstemplatetier/nstemplatetier_controller.go @@ -22,6 +22,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/manager" @@ -119,11 +120,11 @@ func (r *Reconciler) handleDeletion(ctx context.Context, tier *toolchainv1alpha1 } if !used { - util.RemoveFinalizer(tier, constants.BundledNSTemplateTierFinalizerName) - err = r.Client.Update(ctx, tier) + controllerutil.RemoveFinalizer(tier, constants.BundledNSTemplateTierFinalizerName) + return r.Client.Update(ctx, tier) } - return err + return nil } func isTierUsed(ctx context.Context, client runtimeclient.Client, tier *toolchainv1alpha1.NSTemplateTier) (bool, error) { diff --git a/pkg/templates/nstemplatetiers/nstemplatetier_generator.go b/pkg/templates/nstemplatetiers/nstemplatetier_generator.go index a5d373938..812651667 100644 --- a/pkg/templates/nstemplatetiers/nstemplatetier_generator.go +++ b/pkg/templates/nstemplatetiers/nstemplatetier_generator.go @@ -15,25 +15,19 @@ import ( "github.com/codeready-toolchain/host-operator/pkg/templates" commonclient "github.com/codeready-toolchain/toolchain-common/pkg/client" "github.com/codeready-toolchain/toolchain-common/pkg/template/nstemplatetiers" - "github.com/redhat-cop/operator-utils/pkg/util" "gopkg.in/yaml.v2" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) const NsTemplateTierRootDir = "templates/nstemplatetiers" -var ( - bundledAnnotation = map[string]string{ - toolchainv1alpha1.BundledAnnotationKey: constants.BundledWithHostOperatorAnnotationValue, - } - - // BundledTierKeys is the list of NSTemplateTiers that are bundled with the operator binary. - // This list is initialized during the SyncResources call. DO NOT MODIFY it unless you're in - // tests and need to setup a custom list of bundled tiers. - BundledTierKeys []runtimeclient.ObjectKey -) +// BundledTierKeys is the list of NSTemplateTiers that are bundled with the operator binary. +// This list is initialized during the SyncResources call. DO NOT MODIFY it unless you're in +// tests and need to setup a custom list of bundled tiers. +var BundledTierKeys []runtimeclient.ObjectKey // SyncResources generates the NSTemplateTier resources from the cluster resource template and namespace templates, // then uses the manager's client to create or update the resources on the cluster. It also deletes all the tiers @@ -50,8 +44,7 @@ func SyncResources(ctx context.Context, s *runtime.Scheme, client runtimeclient. // initialize tier generator, loads templates from assets err = nstemplatetiers.GenerateTiers(s, func(toEnsure runtimeclient.Object, canUpdate bool, _ string) (bool, error) { - commonclient.MergeAnnotations(toEnsure, bundledAnnotation) - util.AddFinalizer(toEnsure, constants.BundledNSTemplateTierFinalizerName) + controllerutil.AddFinalizer(toEnsure, constants.BundledNSTemplateTierFinalizerName) BundledTierKeys = append(BundledTierKeys, runtimeclient.ObjectKeyFromObject(toEnsure)) @@ -89,7 +82,7 @@ func removeNoLongerBundledTiers(ctx context.Context, client runtimeclient.Client var allErrors []error for _, tier := range allTiers.Items { - if tier.Annotations[toolchainv1alpha1.BundledAnnotationKey] == constants.BundledWithHostOperatorAnnotationValue && + if controllerutil.ContainsFinalizer(&tier, constants.BundledNSTemplateTierFinalizerName) && !slices.Contains(bundledTierKeys, runtimeclient.ObjectKeyFromObject(&tier)) { if err := client.Delete(ctx, &tier); err != nil { allErrors = append(allErrors, err) diff --git a/pkg/templates/nstemplatetiers/nstemplatetier_generator_test.go b/pkg/templates/nstemplatetiers/nstemplatetier_generator_test.go index c681dd3c4..922953813 100644 --- a/pkg/templates/nstemplatetiers/nstemplatetier_generator_test.go +++ b/pkg/templates/nstemplatetiers/nstemplatetier_generator_test.go @@ -15,13 +15,13 @@ import ( commontest "github.com/codeready-toolchain/toolchain-common/pkg/test" "github.com/gofrs/uuid" "github.com/pkg/errors" - "github.com/redhat-cop/operator-utils/pkg/util" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/scheme" runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" ) @@ -183,52 +183,52 @@ func TestSyncResourcesWitProdAssets(t *testing.T) { }) t.Run("bundled tiers", func(t *testing.T) { for _, setup := range []struct { - bundled bool - annotatedBundled bool - used bool + bundled bool + markBundled bool + used bool }{ { - bundled: false, - annotatedBundled: false, - used: false, + bundled: false, + markBundled: false, + used: false, }, { - bundled: false, - annotatedBundled: false, - used: true, + bundled: false, + markBundled: false, + used: true, }, { - bundled: false, - annotatedBundled: true, - used: false, + bundled: false, + markBundled: true, + used: false, }, { - bundled: false, - annotatedBundled: true, - used: true, + bundled: false, + markBundled: true, + used: true, }, { - bundled: true, - annotatedBundled: false, - used: false, + bundled: true, + markBundled: false, + used: false, }, { - bundled: true, - annotatedBundled: false, - used: true, + bundled: true, + markBundled: false, + used: true, }, { - bundled: true, - annotatedBundled: true, - used: false, + bundled: true, + markBundled: true, + used: false, }, { - bundled: true, - annotatedBundled: true, - used: true, + bundled: true, + markBundled: true, + used: true, }, } { - t.Run(fmt.Sprintf("tier: bundled=%v, annotatedBundled=%v, used=%v", setup.bundled, setup.annotatedBundled, setup.used), func(t *testing.T) { + t.Run(fmt.Sprintf("tier: bundled=%v, annotatedBundled=%v, used=%v", setup.bundled, setup.markBundled, setup.used), func(t *testing.T) { // given testTier := &toolchainv1alpha1.NSTemplateTier{ ObjectMeta: metav1.ObjectMeta{ @@ -238,9 +238,8 @@ func TestSyncResourcesWitProdAssets(t *testing.T) { } objs := []runtimeclient.Object{testTier} - if setup.annotatedBundled { - testTier.Annotations = map[string]string{toolchainv1alpha1.BundledAnnotationKey: constants.BundledWithHostOperatorAnnotationValue} - util.AddFinalizer(testTier, constants.BundledNSTemplateTierFinalizerName) + if setup.markBundled { + controllerutil.AddFinalizer(testTier, constants.BundledNSTemplateTierFinalizerName) } if setup.bundled { @@ -266,18 +265,16 @@ func TestSyncResourcesWitProdAssets(t *testing.T) { gerr := clt.Get(context.TODO(), runtimeclient.ObjectKeyFromObject(testTier), inCluster) // then - shouldBeDeleted := !setup.bundled && setup.annotatedBundled && !setup.used - shouldBeAnnotated := setup.bundled || setup.annotatedBundled + shouldBeDeleted := !setup.bundled && setup.markBundled && !setup.used + shouldBeMarked := setup.bundled || setup.markBundled require.NoError(t, err) require.NoError(t, gerr) if shouldBeDeleted { assert.NotNil(t, inCluster.DeletionTimestamp) } - if shouldBeAnnotated { - assert.Equal(t, constants.BundledWithHostOperatorAnnotationValue, inCluster.Annotations[toolchainv1alpha1.BundledAnnotationKey]) + if shouldBeMarked { assert.Contains(t, inCluster.Finalizers, constants.BundledNSTemplateTierFinalizerName) } else { - assert.Empty(t, inCluster.Annotations) assert.Empty(t, inCluster.Finalizers) } })