From cf062f90f9678ea6c4599d5b427b512300be5bda Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Thu, 26 Jun 2025 15:47:56 +0200 Subject: [PATCH 1/2] 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. --- .../nstemplatetier_generator.go | 17 +--- .../nstemplatetier_generator_test.go | 81 ++++++------------- 2 files changed, 28 insertions(+), 70 deletions(-) diff --git a/pkg/template/nstemplatetiers/nstemplatetier_generator.go b/pkg/template/nstemplatetiers/nstemplatetier_generator.go index a997e2e8..15683311 100644 --- a/pkg/template/nstemplatetiers/nstemplatetier_generator.go +++ b/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 { @@ -279,7 +276,7 @@ func (t *TierGenerator) createTierTemplates() error { 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 +333,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 +356,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 +375,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 +389,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 } diff --git a/pkg/template/nstemplatetiers/nstemplatetier_generator_test.go b/pkg/template/nstemplatetiers/nstemplatetier_generator_test.go index 6df44322..0c72b0fd 100644 --- a/pkg/template/nstemplatetiers/nstemplatetier_generator_test.go +++ b/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,6 +277,15 @@ 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 + } // when calling CreateOrUpdateResources a second time err = GenerateTiers(s, ensureObjectFuncForClient(clt), namespace, getTestMetadata(), getTestTemplates(t)) @@ -287,12 +293,13 @@ func TestGenerateTiers(t *testing.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 + assert.True(t, reflect.DeepEqual(origTemplates[tierTmpl.Name].Spec, tierTmpl.Spec)) } // verify that 4 NSTemplateTier CRs were created: @@ -446,27 +453,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 +486,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 +509,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 +570,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 +641,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 +716,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 +755,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 +770,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 +830,6 @@ func expectedTemplateFromBasedOnTierConfig(t *testing.T, tier, templateFileName } func TestNewNSTemplateTiers(t *testing.T) { - // given s := addToScheme(t) From 3c1e49a3ae219ef70a8f87b2dd5cf923789481f2 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Thu, 3 Jul 2025 18:24:34 +0200 Subject: [PATCH 2/2] be clever about detecting the generation change and try to detect possible test problems with the test templates --- .../nstemplatetiers/nstemplatetier_generator.go | 1 - .../nstemplatetier_generator_test.go | 13 ++++++++----- .../testdata/nstemplatetiers/appstudio/cluster.yaml | 1 - .../testdata/nstemplatetiers/base/cluster.yaml | 3 +-- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/pkg/template/nstemplatetiers/nstemplatetier_generator.go b/pkg/template/nstemplatetiers/nstemplatetier_generator.go index 15683311..41889a6e 100644 --- a/pkg/template/nstemplatetiers/nstemplatetier_generator.go +++ b/pkg/template/nstemplatetiers/nstemplatetier_generator.go @@ -275,7 +275,6 @@ 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, tierName); err != nil { return errors.Wrapf(err, "unable to create the '%s' TierTemplate in namespace '%s'", tierTmpl.Name, tierTmpl.Namespace) } diff --git a/pkg/template/nstemplatetiers/nstemplatetier_generator_test.go b/pkg/template/nstemplatetiers/nstemplatetier_generator_test.go index 0c72b0fd..978bc840 100644 --- a/pkg/template/nstemplatetiers/nstemplatetier_generator_test.go +++ b/pkg/template/nstemplatetiers/nstemplatetier_generator_test.go @@ -284,7 +284,7 @@ func TestGenerateTiers(t *testing.T) { 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 + origTemplates[tmpl.Name] = tmpl.DeepCopy() } // when calling CreateOrUpdateResources a second time @@ -299,10 +299,13 @@ func TestGenerateTiers(t *testing.T) { 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.True(t, reflect.DeepEqual(origTemplates[tierTmpl.Name].Spec, tierTmpl.Spec)) - } - - // 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) diff --git a/pkg/template/nstemplatetiers/testdata/nstemplatetiers/appstudio/cluster.yaml b/pkg/template/nstemplatetiers/testdata/nstemplatetiers/appstudio/cluster.yaml index 35a65562..66301f7b 100644 --- a/pkg/template/nstemplatetiers/testdata/nstemplatetiers/appstudio/cluster.yaml +++ b/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} diff --git a/pkg/template/nstemplatetiers/testdata/nstemplatetiers/base/cluster.yaml b/pkg/template/nstemplatetiers/testdata/nstemplatetiers/base/cluster.yaml index c7d0f405..47c76793 100644 --- a/pkg/template/nstemplatetiers/testdata/nstemplatetiers/base/cluster.yaml +++ b/pkg/template/nstemplatetiers/testdata/nstemplatetiers/base/cluster.yaml @@ -17,7 +17,6 @@ objects: requests.storage: 7Gi persistentvolumeclaims: "5" selector: - annotations: null labels: matchLabels: toolchain.dev.openshift.com/space: ${SPACE_NAME} @@ -25,4 +24,4 @@ parameters: - name: SPACE_NAME required: true - name: CPU_LIMIT - value: 4000m \ No newline at end of file + value: 4000m