Simplify nstemplatetiers.GenerateTiers (#486)

* simplify nstemplatetiers.GenerateTiers to not distiniguish between create/update in the EnsureObject func. This is in preparation for using the SSA client that doesn't do that either.

* be clever about detecting the generation change and try to detect possible test problems with the test templates

Author: metlos

pkg/template/nstemplatetiers/nstemplatetier_generator.go

@@ -21,7 +21,7 @@ import (
 
 var log = logf.Log.WithName("templates")
 
-type EnsureObject func(toEnsure runtimeclient.Object, canUpdate bool, tierName string) (bool, error)
+type EnsureObject func(toEnsure runtimeclient.Object, tierName string) error
 
 type TierGenerator struct {
 	ensureObject    EnsureObject
@@ -76,7 +76,6 @@ func GenerateTiers(s *runtime.Scheme, ensureObject EnsureObject, namespace strin
 
 // newNSTemplateTierGenerator loads templates from the provided assets and processes the tierTemplates and NSTemplateTiers
 func newNSTemplateTierGenerator(s *runtime.Scheme, ensureObject EnsureObject, namespace string, metadata map[string]string, files map[string][]byte) (*TierGenerator, error) {
-
 	templatesByTier, err := loadTemplatesByTiers(metadata, files)
 	if err != nil {
 		return nil, err
@@ -141,7 +140,6 @@ type BasedOnTier struct {
 // an optional `template` for the cluster resources (`clusterTemplate`) and the NSTemplateTier resource object.
 // Each `template` object contains a `revision` (`string`) and the `content` of the template to apply (`[]byte`)
 func loadTemplatesByTiers(metadata map[string]string, files map[string][]byte) (map[string]*tierData, error) {
-
 	results := make(map[string]*tierData)
 	for name, content := range files {
 		// split the name using the `/` separator
@@ -203,7 +201,6 @@ func loadTemplatesByTiers(metadata map[string]string, files map[string][]byte) (
 
 // initTierTemplates generates all TierTemplate resources, and adds them to the tier map indexed by tier name
 func (t *TierGenerator) initTierTemplates() error {
-
 	// process tiers in alphabetical order
 	tiers := make([]string, 0, len(t.templatesByTier))
 	for tier := range t.templatesByTier {
@@ -278,8 +275,7 @@ func (t *TierGenerator) createTierTemplates() error {
 	for tierName, tierTmpls := range t.templatesByTier {
 		for _, tierTmpl := range tierTmpls.tierTemplates {
 			log.Info("creating TierTemplate", "namespace", tierTmpl.Namespace, "name", tierTmpl.Name)
-			// using the "standard" client since we don't need to support updates on such resources, they should be immutable
-			if _, err := t.ensureObject(tierTmpl, false, tierName); err != nil {
+			if err := t.ensureObject(tierTmpl, tierName); err != nil {
 				return errors.Wrapf(err, "unable to create the '%s' TierTemplate in namespace '%s'", tierTmpl.Name, tierTmpl.Namespace)
 			}
 			log.Info("TierTemplate resource created", "namespace", tierTmpl.Namespace, "name", tierTmpl.Name)
@@ -336,7 +332,6 @@ func newTierTemplateName(tier, kind, revision string) string {
 
 // newNSTemplateTiers generates all NSTemplateTier resources and adds them to the tier map
 func (t *TierGenerator) initNSTemplateTiers() error {
-
 	for tierName, tierData := range t.templatesByTier {
 		nsTemplateTier := tierData.rawTemplates.nsTemplateTier
 		tierTemplates := tierData.tierTemplates
@@ -360,7 +355,6 @@ func (t *TierGenerator) initNSTemplateTiers() error {
 
 // createNSTemplateTiers creates the NSTemplateTier resources from the tier map
 func (t *TierGenerator) createNSTemplateTiers() error {
-
 	for tierName, tierData := range t.templatesByTier {
 		if len(tierData.objects) != 1 {
 			return fmt.Errorf("there is an unexpected number of NSTemplateTier object to be applied for tier name '%s'; expected: 1; actual: %d", tierName, len(tierData.objects))
@@ -380,7 +374,7 @@ func (t *TierGenerator) createNSTemplateTiers() error {
 			labels = make(map[string]string)
 		}
 		labels[toolchainv1alpha1.ProviderLabelKey] = toolchainv1alpha1.ProviderLabelValue
-		updated, err := t.ensureObject(tier, true, tierName)
+		err := t.ensureObject(tier, tierName)
 		if err != nil {
 			return errors.Wrapf(err, "unable to create or update the '%s' NSTemplateTier", tierName)
 		}
@@ -394,11 +388,7 @@ func (t *TierGenerator) createNSTemplateTiers() error {
 		for role, nsTemplate := range tier.Spec.SpaceRoles {
 			tierLog = tierLog.WithValues(fmt.Sprintf("spaceRoleTemplate-%s", role), nsTemplate.TemplateRef)
 		}
-		if updated {
-			tierLog.Info("NSTemplateTier was either updated or created")
-		} else {
-			tierLog.Info("NSTemplateTier wasn't updated nor created: the spec was already set as expected")
-		}
+		tierLog.Info("NSTemplateTier was patched")
 	}
 	return nil
 }

pkg/template/nstemplatetiers/nstemplatetier_generator_test.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"io/fs"
 	"path/filepath"
+	"reflect"
 	"regexp"
 	"testing"
 	texttemplate "text/template"
@@ -17,8 +18,6 @@ import (
 	commonclient "github.com/codeready-toolchain/toolchain-common/pkg/client"
 	"github.com/codeready-toolchain/toolchain-common/pkg/test"
 	"github.com/pkg/errors"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	runtimeclient "sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -141,12 +140,10 @@ func getTestTemplates(t *testing.T) map[string][]byte {
 }
 
 func TestGenerateTiers(t *testing.T) {
-
 	s := addToScheme(t)
 	logf.SetLogger(zap.New(zap.UseDevMode(true)))
 
 	t.Run("ok", func(t *testing.T) {
-
 		expectedTemplateRefs := map[string]map[string]interface{}{
 			"advanced": {
 				"clusterresources": "advanced-clusterresources-abcd123-654321a",
@@ -280,22 +277,35 @@ func TestGenerateTiers(t *testing.T) {
 			// when
 			err := GenerateTiers(s, ensureObjectFuncForClient(clt), namespace, getTestMetadata(), getTestTemplates(t))
 			require.NoError(t, err)
+			// here we're just capturing the state after the first call so that we can compare with the state after the second call
+			tierTmpls := toolchainv1alpha1.TierTemplateList{}
+			err = clt.List(context.TODO(), &tierTmpls, runtimeclient.InNamespace(namespace))
+			require.NoError(t, err)
+			require.Len(t, tierTmpls.Items, 16) // 4 items for advanced and base tiers + 3 for nocluster tier + 4 for appstudio
+			origTemplates := map[string]*toolchainv1alpha1.TierTemplate{}
+			for _, tmpl := range tierTmpls.Items {
+				origTemplates[tmpl.Name] = tmpl.DeepCopy()
+			}
 
 			// when calling CreateOrUpdateResources a second time
 			err = GenerateTiers(s, ensureObjectFuncForClient(clt), namespace, getTestMetadata(), getTestTemplates(t))
 
 			// then
 			require.NoError(t, err)
 			// verify that all TierTemplate CRs were updated
-			tierTmpls := toolchainv1alpha1.TierTemplateList{}
+			tierTmpls = toolchainv1alpha1.TierTemplateList{}
 			err = clt.List(context.TODO(), &tierTmpls, runtimeclient.InNamespace(namespace))
 			require.NoError(t, err)
 			require.Len(t, tierTmpls.Items, 16) // 4 items for advanced and base tiers + 3 for nocluster tier + 4 for appstudio
+			// check that the tier templates are unchanged
 			for _, tierTmpl := range tierTmpls.Items {
-				assert.Equal(t, int64(1), tierTmpl.Generation) // unchanged
-			}
-
-			// verify that 4 NSTemplateTier CRs were created:
+				// these two should always mean the same thing. If they don't, it means there's an issue with serde of
+				// the templates where template json -> object in cluster -> template json doesn't yield the same thing.
+				// (the json reprentation is used to detect generation change in our test.Client).
+				// This is usually caused by e.g explicitly using a zero value of some property in the template file.
+				assert.True(t, reflect.DeepEqual(origTemplates[tierTmpl.Name].Spec, tierTmpl.Spec), "deep equal different on %T %s", tierTmpl, tierTmpl.Name)
+				assert.Equal(t, int64(1), tierTmpl.Generation, "generation different on %T %s", tierTmpl, tierTmpl.Name) // unchanged
+			} // verify that 4 NSTemplateTier CRs were created:
 			for _, tierName := range []string{"advanced", "base", "nocluster", "appstudio"} {
 				tier := toolchainv1alpha1.NSTemplateTier{}
 				err = clt.Get(context.TODO(), types.NamespacedName{Namespace: namespace, Name: tierName}, &tier)
@@ -446,27 +456,25 @@ func TestGenerateTiers(t *testing.T) {
 	})
 
 	t.Run("failures", func(t *testing.T) {
-
 		namespace := "host-operator" + uuid.NewString()[:7]
 
 		t.Run("nstemplatetiers", func(t *testing.T) {
-
-			t.Run("failed to create nstemplatetiers", func(t *testing.T) {
+			t.Run("failed to patch nstemplatetiers", func(t *testing.T) {
 				// given
 				clt := test.NewFakeClient(t)
-				clt.MockCreate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.CreateOption) error {
-					if obj.GetObjectKind().GroupVersionKind().Kind == "NSTemplateTier" {
+				clt.MockPatch = func(ctx context.Context, obj runtimeclient.Object, patch runtimeclient.Patch, opts ...runtimeclient.PatchOption) error {
+					if _, ok := obj.(*toolchainv1alpha1.NSTemplateTier); ok {
 						// simulate a client/server error
 						return errors.Errorf("an error")
 					}
-					return clt.Client.Create(ctx, obj, opts...)
+					return test.Patch(ctx, clt, obj, patch, opts...)
 				}
 
 				// when
 				err := GenerateTiers(s, ensureObjectFuncForClient(clt), namespace, getTestMetadata(), getTestTemplates(t))
 				// then
 				require.Error(t, err)
-				assert.Regexp(t, "unable to create or update the '\\w+' NSTemplateTier: unable to create resource of kind: NSTemplateTier, version: v1alpha1: an error", err.Error())
+				assert.Regexp(t, "unable to create NSTemplateTiers: unable to create or update the '\\w+' NSTemplateTier: unable to patch 'toolchain.dev.openshift.com/v1alpha1, Kind=NSTemplateTier' called '\\w+' in namespace '[a-zA-Z0-9-]+': an error", err.Error())
 			})
 
 			t.Run("missing tier.yaml file", func(t *testing.T) {
@@ -481,40 +489,15 @@ func TestGenerateTiers(t *testing.T) {
 				require.EqualError(t, err, "unable to init NSTemplateTier generator: tier appstudio is missing a tier.yaml file")
 			})
 
-			t.Run("failed to update nstemplatetiers", func(t *testing.T) {
-				// given
-				// initialize the client with an existing `advanced` NSTemplatetier
-				clt := test.NewFakeClient(t, &toolchainv1alpha1.NSTemplateTier{
-					ObjectMeta: metav1.ObjectMeta{
-						Namespace: namespace,
-						Name:      "advanced",
-					},
-				})
-				clt.MockUpdate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.UpdateOption) error {
-					if obj.GetObjectKind().GroupVersionKind().Kind == "NSTemplateTier" {
-						// simulate a client/server error
-						return errors.Errorf("an error")
-					}
-					return clt.Client.Update(ctx, obj, opts...)
-				}
-
-				// when
-				err := GenerateTiers(s, ensureObjectFuncForClient(clt), namespace, getTestMetadata(), getTestTemplates(t))
-
-				// 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")
-			})
-
-			t.Run("failed to create nstemplatetiers", func(t *testing.T) {
+			t.Run("failed to patch nstemplatetiers", func(t *testing.T) {
 				// given
 				clt := test.NewFakeClient(t)
-				clt.MockCreate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.CreateOption) error {
+				clt.MockPatch = func(ctx context.Context, obj runtimeclient.Object, patch runtimeclient.Patch, opts ...runtimeclient.PatchOption) error {
 					if _, ok := obj.(*toolchainv1alpha1.TierTemplate); ok {
 						// simulate a client/server error
 						return errors.Errorf("an error")
 					}
-					return clt.Client.Create(ctx, obj, opts...)
+					return test.Patch(ctx, clt, obj, patch, opts...)
 				}
 
 				// when
@@ -529,7 +512,6 @@ func TestGenerateTiers(t *testing.T) {
 }
 
 func TestLoadTemplatesByTiers(t *testing.T) {
-
 	logf.SetLogger(zap.New(zap.UseDevMode(true)))
 
 	t.Run("ok", func(t *testing.T) {
@@ -591,7 +573,6 @@ func TestLoadTemplatesByTiers(t *testing.T) {
 	})
 
 	t.Run("failures", func(t *testing.T) {
-
 		t.Run("unparseable content", func(t *testing.T) {
 			// given
 			testTemplates := getTestTemplates(t)
@@ -663,13 +644,11 @@ func TestLoadTemplatesByTiers(t *testing.T) {
 }
 
 func TestNewNSTemplateTier(t *testing.T) {
-
 	s := scheme.Scheme
 	err := toolchainv1alpha1.AddToScheme(s)
 	require.NoError(t, err)
 
 	t.Run("ok", func(t *testing.T) {
-
 		t.Run("with test assets", func(t *testing.T) {
 			// given
 			namespace := "host-operator-" + uuid.NewString()[:7]
@@ -740,7 +719,6 @@ func TestNewTierTemplate(t *testing.T) {
 	namespace := "host-operator-" + uuid.NewString()[:7]
 
 	t.Run("ok", func(t *testing.T) {
-
 		t.Run("with test assets", func(t *testing.T) {
 			// given
 
@@ -780,7 +758,6 @@ func TestNewTierTemplate(t *testing.T) {
 	})
 
 	t.Run("failures", func(t *testing.T) {
-
 		t.Run("invalid template", func(t *testing.T) {
 			// given
 			testTemplates := getTestTemplates(t)
@@ -796,15 +773,9 @@ func TestNewTierTemplate(t *testing.T) {
 }
 
 func ensureObjectFuncForClient(cl runtimeclient.Client) EnsureObject {
-	return func(toEnsure runtimeclient.Object, canUpdate bool, _ string) (bool, error) {
-		if !canUpdate {
-			if err := cl.Create(context.TODO(), toEnsure); err != nil && !apierrors.IsAlreadyExists(err) {
-				return false, err
-			}
-			return true, nil
-		}
-		applyCl := commonclient.NewApplyClient(cl)
-		return applyCl.ApplyObject(context.TODO(), toEnsure, commonclient.ForceUpdate(true))
+	return func(toEnsure runtimeclient.Object, _ string) error {
+		applyCl := commonclient.NewSSAApplyClient(cl, "testFieldManager")
+		return applyCl.ApplyObject(context.TODO(), toEnsure)
 	}
 }
 
@@ -862,7 +833,6 @@ func expectedTemplateFromBasedOnTierConfig(t *testing.T, tier, templateFileName
 }
 
 func TestNewNSTemplateTiers(t *testing.T) {
-
 	// given
 	s := addToScheme(t)
 

pkg/template/nstemplatetiers/testdata/nstemplatetiers/appstudio/cluster.yaml

@@ -19,7 +19,6 @@ objects:
         requests.ephemeral-storage: 7Gi
         count/persistentvolumeclaims: "5"
     selector:
-      annotations: null
       labels:
         matchLabels:
           toolchain.dev.openshift.com/space: ${SPACE_NAME}

pkg/template/nstemplatetiers/testdata/nstemplatetiers/base/cluster.yaml

@@ -17,12 +17,11 @@ objects:
         requests.storage: 7Gi
         persistentvolumeclaims: "5"
     selector:
-      annotations: null
       labels:
         matchLabels:
           toolchain.dev.openshift.com/space: ${SPACE_NAME}
 parameters:
 - name: SPACE_NAME
   required: true
 - name: CPU_LIMIT
-  value: 4000m
+  value: 4000m