From 8874945b67d936523b0ffe2ea07b537c492135be Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Fri, 5 Sep 2025 17:48:45 +0200 Subject: [PATCH 01/16] apply all cluster resources at once and read the prior state from the nstemplateset status instead of labels --- .../nstemplateset/cluster_resources.go | 404 ++---- .../nstemplateset/cluster_resources_test.go | 1093 +++++------------ .../nstemplateset/nstemplateset_controller.go | 15 +- .../nstemplateset_controller_test.go | 16 + test/cluster_assertion.go | 7 + test/nstemplateset_assertion.go | 10 +- 6 files changed, 448 insertions(+), 1097 deletions(-) diff --git a/controllers/nstemplateset/cluster_resources.go b/controllers/nstemplateset/cluster_resources.go index 79a5b60b0..f6158ce02 100644 --- a/controllers/nstemplateset/cluster_resources.go +++ b/controllers/nstemplateset/cluster_resources.go @@ -2,19 +2,14 @@ package nstemplateset import ( "context" + "fmt" + "slices" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" - applycl "github.com/codeready-toolchain/toolchain-common/pkg/client" - "github.com/codeready-toolchain/toolchain-common/pkg/template" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - quotav1 "github.com/openshift/api/quota/v1" errs "github.com/pkg/errors" "github.com/redhat-cop/operator-utils/pkg/util" - rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" @@ -24,157 +19,131 @@ type clusterResourcesManager struct { *statusManager } -// listExistingResourcesIfAvailable returns a list of comparable Objects representing existing resources in the cluster -type listExistingResources func(ctx context.Context, cl runtimeclient.Client, spacename string) ([]runtimeclient.Object, error) - -// listExistingResourcesIfAvailable checks if the API group is available in the cluster and returns a list of comparable Objects representing existing resources in the cluster -type listExistingResourcesIfAvailable func(ctx context.Context, cl runtimeclient.Client, spacename string, availableAPIGroups []metav1.APIGroup) ([]runtimeclient.Object, error) - -// toolchainObjectKind represents a resource kind that should be present in templates containing cluster resources. -// Such a kind should be watched by NSTempalateSet controller which means that every change of the -// resources of that kind triggers a new reconcile. -// It is expected that the template contains only the kinds that are being watched and there is an instance of -// toolchainObjectKind type created in clusterResourceKinds list -type toolchainObjectKind struct { - gvk schema.GroupVersionKind - object runtimeclient.Object - listExistingResourcesIfAvailable listExistingResourcesIfAvailable -} - -func newToolchainObjectKind(gvk schema.GroupVersionKind, emptyObject runtimeclient.Object, listExistingResources listExistingResources) toolchainObjectKind { - return toolchainObjectKind{ - gvk: gvk, - object: emptyObject, - listExistingResourcesIfAvailable: func(ctx context.Context, cl runtimeclient.Client, spacename string, availableAPIGroups []metav1.APIGroup) (objects []runtimeclient.Object, e error) { - if !apiGroupIsPresent(availableAPIGroups, gvk) { - return []runtimeclient.Object{}, nil - } - return listExistingResources(ctx, cl, spacename) - }, - } -} - -// clusterResourceKinds is a list that contains definitions for all cluster-scoped toolchainObjectKinds -var clusterResourceKinds = []toolchainObjectKind{ - newToolchainObjectKind( - quotav1.GroupVersion.WithKind("ClusterResourceQuota"), - "av1.ClusterResourceQuota{}, - func(ctx context.Context, cl runtimeclient.Client, spacename string) ([]runtimeclient.Object, error) { - itemList := "av1.ClusterResourceQuotaList{} - if err := cl.List(ctx, itemList, listBySpaceLabel(spacename)); err != nil { - return nil, err - } - list := make([]runtimeclient.Object, len(itemList.Items)) - for index := range itemList.Items { - list[index] = &itemList.Items[index] - } - return applycl.SortObjectsByName(list), nil - }), - - newToolchainObjectKind( - rbacv1.SchemeGroupVersion.WithKind("ClusterRoleBinding"), - &rbacv1.ClusterRoleBinding{}, - func(ctx context.Context, cl runtimeclient.Client, spacename string) ([]runtimeclient.Object, error) { - itemList := &rbacv1.ClusterRoleBindingList{} - if err := cl.List(ctx, itemList, listBySpaceLabel(spacename)); err != nil { - return nil, err - } - list := make([]runtimeclient.Object, len(itemList.Items)) - for index := range itemList.Items { - list[index] = &itemList.Items[index] - } - return applycl.SortObjectsByName(list), nil - }), - - newToolchainObjectKind( - toolchainv1alpha1.GroupVersion.WithKind("Idler"), - &toolchainv1alpha1.Idler{}, - func(ctx context.Context, cl runtimeclient.Client, spacename string) ([]runtimeclient.Object, error) { - itemList := &toolchainv1alpha1.IdlerList{} - if err := cl.List(ctx, itemList, listBySpaceLabel(spacename)); err != nil { - return nil, err - } - list := make([]runtimeclient.Object, len(itemList.Items)) - for index := range itemList.Items { - list[index] = &itemList.Items[index] - } - return applycl.SortObjectsByName(list), nil - }), -} - // ensure ensures that the cluster resources exist. // Returns `true, nil` if something was changed, `false, nil` if nothing changed, `false, err` if an error occurred -func (r *clusterResourcesManager) ensure(ctx context.Context, nsTmplSet *toolchainv1alpha1.NSTemplateSet) (bool, error) { +// +// NOTE: This method makes 1 important assumption: all the cluster-scoped resources of the NSTemplateSet are uniquely +// associated with it. I.e. the logic doesn't work correctly if 2 NSTemplateSets declare the very same cluster-scoped +// resource. It is assumed that if a cluster-scoped resource should be common to more than 1 nstemplateset, it should +// be deployed externally from Devsandbox, e.g. using ArgoCD. +func (r *clusterResourcesManager) ensure(ctx context.Context, nsTmplSet *toolchainv1alpha1.NSTemplateSet) error { logger := log.FromContext(ctx) userTierLogger := logger.WithValues("spacename", nsTmplSet.GetName(), "tier", nsTmplSet.Spec.TierName) userTierCtx := log.IntoContext(ctx, userTierLogger) userTierLogger.Info("ensuring cluster resources") spacename := nsTmplSet.GetName() - var tierTemplate *tierTemplate - var err error + + var oldTemplateRef, newTemplateRef string if nsTmplSet.Spec.ClusterResources != nil { - tierTemplate, err = getTierTemplate(ctx, r.GetHostClusterClient, nsTmplSet.Spec.ClusterResources.TemplateRef) + newTemplateRef = nsTmplSet.Spec.ClusterResources.TemplateRef + } + if nsTmplSet.Status.ClusterResources != nil { + oldTemplateRef = nsTmplSet.Status.ClusterResources.TemplateRef + } + + if oldTemplateRef == newTemplateRef { + return nil + } + + var newTierTemplate *tierTemplate + var oldTierTemplate *tierTemplate + var err error + if newTemplateRef != "" { + newTierTemplate, err = getTierTemplate(ctx, r.GetHostClusterClient, newTemplateRef) + if err != nil { + return r.wrapErrorWithStatusUpdateForClusterResourceFailure(userTierCtx, nsTmplSet, err, + "failed to retrieve the TierTemplate for the to-be-applied cluster resources with the name '%s'", newTemplateRef) + } + } + if oldTemplateRef != "" { + oldTierTemplate, err = getTierTemplate(ctx, r.GetHostClusterClient, oldTemplateRef) + if err != nil { + return r.wrapErrorWithStatusUpdateForClusterResourceFailure(userTierCtx, nsTmplSet, err, + "failed to retrieve TierTemplate for the last-applied cluster resources with the name '%s'", oldTemplateRef) + } + } + + var newObjs, currentObjects []runtimeclient.Object + if newTierTemplate != nil { + newObjs, err = newTierTemplate.process(r.Scheme, map[string]string{SpaceName: spacename}) + if err != nil { + return r.wrapErrorWithStatusUpdateForClusterResourceFailure(ctx, nsTmplSet, err, + "failed to process template for the to-be-applied cluster resources with the name '%s'", newTemplateRef) + } + } + if oldTierTemplate != nil { + currentObjects, err = oldTierTemplate.process(r.Scheme, map[string]string{SpaceName: spacename}) if err != nil { - return false, r.wrapErrorWithStatusUpdateForClusterResourceFailure(userTierCtx, nsTmplSet, err, - "failed to retrieve TierTemplate for the cluster resources with the name '%s'", nsTmplSet.Spec.ClusterResources.TemplateRef) + return r.wrapErrorWithStatusUpdateForClusterResourceFailure(ctx, nsTmplSet, err, + "failed to process template for the last-applied cluster resources with the name '%s'", oldTemplateRef) } } - // go through all cluster resource kinds - for _, clusterResourceKind := range clusterResourceKinds { - gvkLogger := userTierLogger.WithValues("gvk", clusterResourceKind.gvk) - gvkCtx := log.IntoContext(ctx, gvkLogger) - gvkLogger.Info("ensuring cluster resources") - newObjs := make([]runtimeclient.Object, 0) + statusChanged := false + firstDeployment := nsTmplSet.Status.ClusterResources == nil || nsTmplSet.Status.ClusterResources.TemplateRef == "" + var failureStatusReason statusUpdater + if firstDeployment { + failureStatusReason = r.setStatusClusterResourcesProvisionFailed + } else { + failureStatusReason = r.setStatusUpdateFailed + } - // get all objects of the resource kind from the template (if the template is specified) - if tierTemplate != nil { - newObjs, err = tierTemplate.process(r.Scheme, map[string]string{ - SpaceName: spacename, - }, retainObjectsOfSameGVK(clusterResourceKind.gvk)) + changeStatusIfNeeded := func() error { + if !statusChanged { + if firstDeployment { + err = r.setStatusProvisioningIfNotUpdating(ctx, nsTmplSet) + } else { + err = r.setStatusUpdatingIfNotProvisioning(ctx, nsTmplSet) + } if err != nil { - return false, r.wrapErrorWithStatusUpdateForClusterResourceFailure(gvkCtx, nsTmplSet, err, - "failed to process template for the cluster resources with the name '%s'", nsTmplSet.Spec.ClusterResources.TemplateRef) + return err } + statusChanged = true } + return nil + } - // list all existing objects of the cluster resource kind - currentObjects, err := clusterResourceKind.listExistingResourcesIfAvailable(ctx, r.Client, spacename, r.AvailableAPIGroups) - if err != nil { - return false, r.wrapErrorWithStatusUpdateForClusterResourceFailure(gvkCtx, nsTmplSet, err, - "failed to list existing cluster resources of GVK '%v'", clusterResourceKind.gvk) + for _, newObj := range newObjs { + if !shouldCreate(newObj, nsTmplSet) { + continue } - // if there are more than one existing, then check if there is any that should be updated or deleted - if len(currentObjects) > 0 { - updatedOrDeleted, err := r.updateOrDeleteRedundant(gvkCtx, currentObjects, newObjs, tierTemplate, nsTmplSet) - if err != nil { - return false, r.wrapErrorWithStatusUpdate(gvkCtx, nsTmplSet, r.setStatusUpdateFailed, - err, "failed to update/delete existing cluster resources of GVK '%v'", clusterResourceKind.gvk) - } - if updatedOrDeleted { - return true, err + // by removing the new objects from the current objects, we are going to be left with the objects + // that should be removed from the cluster after we're done with this loop + for oldIdx, curObj := range currentObjects { + if curObj.GetName() == newObj.GetName() && curObj.GetNamespace() == newObj.GetNamespace() && curObj.GetObjectKind().GroupVersionKind() == newObj.GetObjectKind().GroupVersionKind() { + currentObjects = slices.Delete(currentObjects, oldIdx, oldIdx+1) + break } } - // if none was found to be either updated or deleted or if there is no existing object available, - // then check if there is any object to be created - if len(newObjs) > 0 { - anyCreated, err := r.createMissing(gvkCtx, currentObjects, newObjs, tierTemplate, nsTmplSet) - if err != nil { - return false, r.wrapErrorWithStatusUpdate(gvkCtx, nsTmplSet, r.setStatusClusterResourcesProvisionFailed, - err, "failed to create missing cluster resource of GVK '%v'", clusterResourceKind.gvk) - } - if anyCreated { - return true, nil + + if err := changeStatusIfNeeded(); err != nil { + return err + } + + // create or update the resource + if _, err = r.apply(ctx, nsTmplSet, newTierTemplate, newObj); err != nil { + err := fmt.Errorf("failed to apply changes to the cluster resource %s, %s: %w", newObj.GetName(), newObj.GetObjectKind().GroupVersionKind().String(), err) + return r.wrapErrorWithStatusUpdate(ctx, nsTmplSet, failureStatusReason, err, "failure while syncing cluster resources") + } + } + + // what we're left with here is the list of currently existing objects that are no longer present in the template. + // we need to delete them + for _, obj := range currentObjects { + if err := changeStatusIfNeeded(); err != nil { + return err + } + if err := r.Client.Delete(ctx, obj); err != nil { + if !errors.IsNotFound(err) { + err := fmt.Errorf("failed to delete the cluster resource %s, %s: %w", obj.GetName(), obj.GetObjectKind().GroupVersionKind().String(), err) + return r.wrapErrorWithStatusUpdate(ctx, nsTmplSet, failureStatusReason, err, "failure while syncing cluster resources") } - } else { - gvkLogger.Info("no new cluster resources to create") } } - userTierLogger.Info("cluster resources already provisioned") - return false, nil + return nil } // apply creates or updates the given object with the set of toolchain labels. If the apply operation was successful, then it returns 'true, nil', @@ -195,161 +164,46 @@ func (r *clusterResourcesManager) apply(ctx context.Context, nsTmplSet *toolchai log.FromContext(ctx).Info("applying cluster resource", "object_name", object.GetObjectKind().GroupVersionKind().Kind+"/"+object.GetName()) createdOrModified, err := r.ApplyToolchainObjects(ctx, []runtimeclient.Object{object}, labels) if err != nil { - return false, errs.Wrapf(err, "failed to apply cluster resource of type '%v'", object.GetObjectKind().GroupVersionKind()) + return false, errs.Wrapf(err, "failed to apply cluster resource") } return createdOrModified, nil } -// updateOrDeleteRedundant takes the given currentObjs and newObjs and compares them. -// -// If there is any existing redundant resource (exist in the currentObjs, but not in the newObjs), then it deletes the resource and returns 'true, nil'. -// -// If there is any resource that is outdated (exists in both currentObjs and newObjs but its templateref is not matching), -// then it updates the resource and returns 'true, nil' -// -// If no resource to be updated or deleted was found then it returns 'false, nil'. In case of any errors 'false, error' -func (r *clusterResourcesManager) updateOrDeleteRedundant(ctx context.Context, currentObjs []runtimeclient.Object, newObjs []runtimeclient.Object, tierTemplate *tierTemplate, nsTmplSet *toolchainv1alpha1.NSTemplateSet) (bool, error) { - // go through all current objects, so we can compare then with the set of the requested and thus update the obsolete ones or delete redundant ones - logger := log.FromContext(ctx) - logger.Info("updating or deleting cluster resources") -CurrentObjects: - for _, currentObject := range currentObjs { - - // if the template is not specified, then delete all cluster resources one by one - if nsTmplSet.Spec.ClusterResources == nil || tierTemplate == nil { - return r.deleteClusterResource(ctx, nsTmplSet, currentObject) - } - - // check if the object should still exist and should be updated - for _, newObject := range newObjs { - if newObject.GetName() == currentObject.GetName() { - // is found then let's check if the object represents a feature and if yes then the feature is still enabled - if !shouldCreate(newObject, nsTmplSet) { - break // proceed to deleting the object - } - // Either it's not a featured object or the feature is still enabled - // Do we need to update it? - if !isUpToDate(currentObject, newObject, tierTemplate) { - logger.Info("updating cluster resource") - // let's update it - if err := r.setStatusUpdatingIfNotProvisioning(ctx, nsTmplSet); err != nil { - return false, err - } - return r.apply(ctx, nsTmplSet, tierTemplate, newObject) - } - continue CurrentObjects - } - } - // is not found then let's delete it - return r.deleteClusterResource(ctx, nsTmplSet, currentObject) +// delete deletes all cluster-scoped resources referenced by the nstemplateset. +func (r *clusterResourcesManager) delete(ctx context.Context, nsTmplSet *toolchainv1alpha1.NSTemplateSet) error { + if nsTmplSet.Status.ClusterResources == nil { + return nil } - return false, nil -} -// deleteClusterResource sets status to updating, deletes the given resource and returns 'true, nil'. In case of any errors 'false, error'. -func (r *clusterResourcesManager) deleteClusterResource(ctx context.Context, nsTmplSet *toolchainv1alpha1.NSTemplateSet, toDelete runtimeclient.Object) (bool, error) { - log.FromContext(ctx).Info("deleting cluster resource") - if err := r.setStatusUpdatingIfNotProvisioning(ctx, nsTmplSet); err != nil { - return false, err + currentTierTemplate, err := getTierTemplate(ctx, r.GetHostClusterClient, nsTmplSet.Status.ClusterResources.TemplateRef) + if err != nil { + return r.wrapErrorWithStatusUpdateForClusterResourceFailure(ctx, nsTmplSet, err, + "failed to read the existing cluster resources") } - if err := r.Client.Delete(ctx, toDelete); err != nil { - return false, errs.Wrapf(err, "failed to delete an existing redundant cluster resource of name '%s' and gvk '%v'", - toDelete.GetName(), toDelete.GetObjectKind().GroupVersionKind()) + currentObjects, err := currentTierTemplate.process(r.Scheme, map[string]string{SpaceName: nsTmplSet.Name}) + if err != nil { + return r.wrapErrorWithStatusUpdateForClusterResourceFailure(ctx, nsTmplSet, err, + "failed to list existing cluster resources") } - return true, nil -} -// createMissing takes the given currentObjs and newObjs and compares them if there is any that should be created. -// If such a object is found, then it creates it and returns 'true, nil'. If no missing resource was found then returns 'false, nil'. -// In case of any error 'false, error' -func (r *clusterResourcesManager) createMissing(ctx context.Context, currentObjs []runtimeclient.Object, newObjs []runtimeclient.Object, tierTemplate *tierTemplate, nsTmplSet *toolchainv1alpha1.NSTemplateSet) (bool, error) { - // go through all new (expected) objects to check if all of them already exist or not -NewObjects: - for _, newObject := range newObjs { - // Check if the new object is associated with a feature toggle. - // If yes then ignore this object if it represents a feature which is not enabled for this NSTemplateSet - if !shouldCreate(newObject, nsTmplSet) { - continue NewObjects + for _, toDelete := range currentObjects { + if err := r.Client.Get(ctx, types.NamespacedName{Name: toDelete.GetName()}, toDelete); err != nil && !errors.IsNotFound(err) { + return r.wrapErrorWithStatusUpdate(ctx, nsTmplSet, r.setStatusTerminatingFailed, err, + "failed to get the cluster resource '%s' (GVK '%s') while deleting cluster resources", toDelete.GetName(), toDelete.GetObjectKind().GroupVersionKind()) } - - // go through current objects to check if is one of the new (expected) - for _, currentObject := range currentObjs { - // if the name is the same, then it means that it already exist so just continue with the next new object - if newObject.GetName() == currentObject.GetName() { - continue NewObjects - } - } - // if there was no existing object found that would match with the new one, then set the status appropriately - namespaces, err := fetchNamespacesByOwner(ctx, r.Client, nsTmplSet.Name) - if err != nil { - return false, errs.Wrapf(err, "unable to fetch user's namespaces") - } - // if there is any existing namespace, then set the status to updating - if len(namespaces) == 0 { - if err := r.setStatusProvisioningIfNotUpdating(ctx, nsTmplSet); err != nil { - return false, err - } - } else { - // otherwise, to provisioning - if err := r.setStatusUpdatingIfNotProvisioning(ctx, nsTmplSet); err != nil { - return false, err - } + // ignore cluster resource that are already flagged for deletion + if errors.IsNotFound(err) || util.IsBeingDeleted(toDelete) { + continue } - // and create the object - return r.apply(ctx, nsTmplSet, tierTemplate, newObject) - } - return false, nil -} -// delete deletes one cluster scoped resource owned by the user and returns 'true, nil'. If no cluster-scoped resource owned -// by the user is found, then it returns 'false, nil'. In case of any errors 'false, error' -func (r *clusterResourcesManager) delete(ctx context.Context, nsTmplSet *toolchainv1alpha1.NSTemplateSet) (bool, error) { - if nsTmplSet.Spec.ClusterResources == nil { - return false, nil - } - for _, clusterResourceKind := range clusterResourceKinds { - // list all existing objects of the cluster resource kind - currentObjects, err := clusterResourceKind.listExistingResourcesIfAvailable(ctx, r.Client, nsTmplSet.Name, r.AvailableAPIGroups) - if err != nil { - return false, r.wrapErrorWithStatusUpdateForClusterResourceFailure(ctx, nsTmplSet, err, - "failed to list existing cluster resources of GVK '%v'", clusterResourceKind.gvk) + log.FromContext(ctx).Info("deleting cluster resource", "name", toDelete.GetName(), "kind", toDelete.GetObjectKind().GroupVersionKind().Kind) + if err := r.Client.Delete(ctx, toDelete); err != nil && errors.IsNotFound(err) { + // ignore case where the resource did not exist anymore, move to the next one to delete + continue + } else if err != nil { + // report an error only if the resource could not be deleted (but ignore if the resource did not exist anymore) + return r.wrapErrorWithStatusUpdate(ctx, nsTmplSet, r.setStatusTerminatingFailed, err, "failed to delete cluster resource '%s'", toDelete.GetName()) } - - for _, toDelete := range currentObjects { - if err := r.Client.Get(ctx, types.NamespacedName{Name: toDelete.GetName()}, toDelete); err != nil && !errors.IsNotFound(err) { - return false, r.wrapErrorWithStatusUpdate(ctx, nsTmplSet, r.setStatusTerminatingFailed, err, - "failed to get current object '%s' while deleting cluster resource of GVK '%s'", toDelete.GetName(), toDelete.GetObjectKind().GroupVersionKind()) - } - // ignore cluster resource that are already flagged for deletion - if errors.IsNotFound(err) || util.IsBeingDeleted(toDelete) { - continue - } - - log.FromContext(ctx).Info("deleting cluster resource", "name", toDelete.GetName(), "kind", toDelete.GetObjectKind().GroupVersionKind().Kind) - if err := r.Client.Delete(ctx, toDelete); err != nil && errors.IsNotFound(err) { - // ignore case where the resource did not exist anymore, move to the next one to delete - continue - } else if err != nil { - // report an error only if the resource could not be deleted (but ignore if the resource did not exist anymore) - return false, r.wrapErrorWithStatusUpdate(ctx, nsTmplSet, r.setStatusTerminatingFailed, err, "failed to delete cluster resource '%s'", toDelete.GetName()) - } - // stop there for now. Will reconcile again for the next cluster resource (if any exists) - return true, nil - } - } - return false, nil -} - -// isUpToDate returns true if the currentObject uses the corresponding templateRef and tier labels -func isUpToDate(currentObject, _ runtimeclient.Object, tierTemplate *tierTemplate) bool { - return currentObject.GetLabels() != nil && - currentObject.GetLabels()[toolchainv1alpha1.TemplateRefLabelKey] == tierTemplate.templateRef && - currentObject.GetLabels()[toolchainv1alpha1.TierLabelKey] == tierTemplate.tierName - // && currentObject.IsSame(newObject) <-- TODO Uncomment when IsSame is implemented for all ToolchainObjects! -} - -func retainObjectsOfSameGVK(gvk schema.GroupVersionKind) template.FilterFunc { - return func(obj runtime.RawExtension) bool { - return obj.Object.GetObjectKind().GroupVersionKind() == gvk } + return nil } diff --git a/controllers/nstemplateset/cluster_resources_test.go b/controllers/nstemplateset/cluster_resources_test.go index d62d2b073..006126366 100644 --- a/controllers/nstemplateset/cluster_resources_test.go +++ b/controllers/nstemplateset/cluster_resources_test.go @@ -2,15 +2,15 @@ package nstemplateset import ( "context" - "errors" "fmt" - "k8s.io/utils/strings/slices" "strings" "testing" "time" + "k8s.io/client-go/kubernetes/scheme" + "k8s.io/utils/strings/slices" + toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" - "github.com/codeready-toolchain/member-operator/pkg/apis" . "github.com/codeready-toolchain/member-operator/test" commonconfig "github.com/codeready-toolchain/toolchain-common/pkg/configuration" "github.com/codeready-toolchain/toolchain-common/pkg/test" @@ -21,153 +21,11 @@ import ( "github.com/stretchr/testify/require" rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" ) -func TestClusterResourceKinds(t *testing.T) { - // given - s := scheme.Scheme - err := apis.AddToScheme(s) - require.NoError(t, err) - ctx := context.TODO() - - for _, clusterResourceKind := range clusterResourceKinds { - johnyRuntimeObject := clusterResourceKind.object.DeepCopyObject() - johnyObject, ok := johnyRuntimeObject.(client.Object) - require.True(t, ok) - johnyObjectLabels := map[string]string{toolchainv1alpha1.SpaceLabelKey: "johny"} - johnyObject.SetLabels(johnyObjectLabels) - johnyObject.SetName("johny-object") - - johnyRuntimeObject2 := clusterResourceKind.object.DeepCopyObject() - johnyObject2, ok := johnyRuntimeObject2.(client.Object) - require.True(t, ok) - johnyObject2.SetLabels(johnyObjectLabels) - johnyObject2.SetName("johny-object-2") - - anotherRuntimeObject := clusterResourceKind.object.DeepCopyObject() - anotherObject, ok := anotherRuntimeObject.(client.Object) - require.True(t, ok) - anotherObject.SetLabels(map[string]string{toolchainv1alpha1.SpaceLabelKey: "another"}) - anotherObject.SetName("another-object") - namespace := newNamespace("basic", "johny", "code") - - apiGroups := newAPIGroups(newAPIGroup("apps", "v1"), newAPIGroup("", "v1"), newAPIGroup(clusterResourceKind.gvk.Group, clusterResourceKind.gvk.Version)) - - t.Run("listExistingResourcesIfAvailable should return one resource of gvk "+clusterResourceKind.gvk.String(), func(t *testing.T) { - // given - fakeClient := test.NewFakeClient(t, anotherObject, johnyObject, namespace) - - // when - existingResources, err := clusterResourceKind.listExistingResourcesIfAvailable(ctx, fakeClient, "johny", apiGroups) - - // then - require.NoError(t, err) - require.Len(t, existingResources, 1) - assert.Equal(t, johnyObject, existingResources[0]) - }) - - t.Run("listExistingResourcesIfAvailable should return two resources of gvk "+clusterResourceKind.gvk.String(), func(t *testing.T) { - // given - fakeClient := test.NewFakeClient(t, anotherObject, johnyObject, johnyObject2, namespace) - - // when - existingResources, err := clusterResourceKind.listExistingResourcesIfAvailable(ctx, fakeClient, "johny", apiGroups) - - // then - require.NoError(t, err) - require.Len(t, existingResources, 2) - assert.Equal(t, johnyObject, existingResources[0]) - assert.Equal(t, johnyObject2, existingResources[1]) - }) - - t.Run("listExistingResourcesIfAvailable should return not return any resource of gvk "+clusterResourceKind.gvk.String(), func(t *testing.T) { - // given - fakeClient := test.NewFakeClient(t, anotherObject, namespace) - - // when - existingResources, err := clusterResourceKind.listExistingResourcesIfAvailable(ctx, fakeClient, "johny", apiGroups) - - // then - require.NoError(t, err) - assert.Empty(t, existingResources) - }) - - t.Run("listExistingResourcesIfAvailable should return an error when listing resources of gvk "+clusterResourceKind.gvk.String(), func(t *testing.T) { - // given - fakeClient := test.NewFakeClient(t, anotherObject, johnyObject) - fakeClient.MockList = func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { - return fmt.Errorf("some error") - } - - // when - existingResources, err := clusterResourceKind.listExistingResourcesIfAvailable(ctx, fakeClient, "johny", apiGroups) - - // then - require.Error(t, err) - assert.Empty(t, existingResources) - }) - - t.Run("listExistingResourcesIfAvailable should not return any resource when APIGroup is missing for gvk "+clusterResourceKind.gvk.String(), func(t *testing.T) { - // given - fakeClient := test.NewFakeClient(t, anotherObject, johnyObject, johnyObject2, namespace) - - // when - existingResources, err := clusterResourceKind.listExistingResourcesIfAvailable(ctx, fakeClient, "johny", - newAPIGroups(newAPIGroup("apps", "v1"), newAPIGroup("", "v1"))) - - // then - require.NoError(t, err) - assert.Empty(t, existingResources) - }) - - t.Run("listExistingResourcesIfAvailable should not return any resource when APIGroup is present but is missing the version "+clusterResourceKind.gvk.String(), func(t *testing.T) { - // given - fakeClient := test.NewFakeClient(t, anotherObject, johnyObject, johnyObject2, namespace) - - // when - existingResources, err := clusterResourceKind.listExistingResourcesIfAvailable(ctx, fakeClient, "johny", - newAPIGroups(newAPIGroup("apps", "v1"), newAPIGroup("", "v1"), newAPIGroup(clusterResourceKind.gvk.Group, "old"))) - - // then - require.NoError(t, err) - assert.Empty(t, existingResources) - }) - } - - t.Run("verify ClusterResourceQuota is in clusterResourceKinds", func(t *testing.T) { - // given - clusterResource := clusterResourceKinds[0] - - // then - assert.Equal(t, "av1.ClusterResourceQuota{}, clusterResource.object) - assert.Equal(t, quotav1.GroupVersion.WithKind("ClusterResourceQuota"), clusterResource.gvk) - }) - - t.Run("verify ClusterRoleBinding is in clusterResourceKinds", func(t *testing.T) { - // given - clusterResource := clusterResourceKinds[1] - - // then - assert.Equal(t, &rbacv1.ClusterRoleBinding{}, clusterResource.object) - assert.Equal(t, rbacv1.SchemeGroupVersion.WithKind("ClusterRoleBinding"), clusterResource.gvk) - }) - - t.Run("verify Idler is in clusterResourceKinds", func(t *testing.T) { - // given - clusterResource := clusterResourceKinds[2] - - // then - assert.Equal(t, &toolchainv1alpha1.Idler{}, clusterResource.object) - assert.Equal(t, toolchainv1alpha1.GroupVersion.WithKind("Idler"), clusterResource.gvk) - }) -} - func TestEnsureClusterResourcesOK(t *testing.T) { // given logger := zap.New(zap.UseDevMode(true)) @@ -180,6 +38,23 @@ func TestEnsureClusterResourcesOK(t *testing.T) { restore := test.SetEnvVarAndRestore(t, commonconfig.WatchNamespaceEnvVar, "my-member-operator-namespace") t.Cleanup(restore) + t.Run("should set status to provisioning", func(t *testing.T) { + // given + manager, failingClient := prepareClusterResourcesManager(t, nsTmplSet) + + err := manager.ensure(ctx, nsTmplSet) + + require.NoError(t, err) + AssertThatNSTemplateSet(t, namespaceName, spacename, failingClient). + HasFinalizer(). + HasConditions(Provisioning()) + AssertThatCluster(t, failingClient). + HasResource("for-"+spacename, "av1.ClusterResourceQuota{}). + HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}). + HasResource(spacename+"-dev", &toolchainv1alpha1.Idler{}). + HasResource(spacename+"-stage", &toolchainv1alpha1.Idler{}) + }) + t.Run("should create only CRQs and set status to provisioning", func(t *testing.T) { tests := []struct { name string @@ -225,69 +100,31 @@ func TestEnsureClusterResourcesOK(t *testing.T) { } manager, fakeClient := prepareClusterResourcesManager(t, nsTmplSet) - t.Run("first iteration - create first resource with no feature", func(t *testing.T) { - // when - - // Each iteration of ensure() applies a single object only (if any) and exists after that. - // Assuming that the controller would watch the created resources and would trigger another reconcile/ensure() - // to apply all the objects one by one. - // So the first iteration always creates the first ClusterResourceQuota with no feature only. - // Even if the features are enabled. - createdOrUpdated, err := manager.ensure(ctx, nsTmplSet) - - // then - require.NoError(t, err) - assert.True(t, createdOrUpdated) - AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). - HasFinalizer(). - HasConditions(Provisioning()) - AssertThatCluster(t, fakeClient). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}). - HasNoResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}). - HasNoResource("feature-1-for-"+spacename, "av1.ClusterResourceQuota{}). - HasNoResource("feature-2-for-"+spacename, "av1.ClusterResourceQuota{}). - HasNoResource("feature-3-for-"+spacename, "av1.ClusterResourceQuota{}) - - // Now, do another iteration and make sure the corresponding objects are created for each enabled feature - enabledFeatures := strings.Split(testRun.enabledFeatures, ",") - expectedFeatureToBeAlreadyCreated := make([]string, 0) - for _, feature := range enabledFeatures { - expectedFeatureToBeAlreadyCreated = append(expectedFeatureToBeAlreadyCreated, feature) - t.Run(fmt.Sprintf("next iteration - create resource for feature %s if enabled", feature), func(t *testing.T) { - // when - createdOrUpdated, err := manager.ensure(ctx, nsTmplSet) - - // then - require.NoError(t, err) - assert.True(t, createdOrUpdated) - AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). - HasFinalizer(). - HasConditions(Provisioning()) - if testRun.enabledFeatures == "" { - AssertThatCluster(t, fakeClient). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}). - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}). - HasNoResource("feature-1-for-"+spacename, "av1.ClusterResourceQuota{}). - HasNoResource("feature-2-for-"+spacename, "av1.ClusterResourceQuota{}). - HasNoResource("feature-3-for-"+spacename, "av1.ClusterResourceQuota{}) - } else { - clusterAssertion := AssertThatCluster(t, fakeClient). - HasNoResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}) - // Check all expected features which should be already created by this and previous iterations - for _, expectedFeature := range expectedFeatureToBeAlreadyCreated { - clusterAssertion.HasResource(fmt.Sprintf("%s-for-%s", expectedFeature, spacename), "av1.ClusterResourceQuota{}) - } - // Check that the rest of the features are not (yet) created - for _, tierFeature := range allTierFeatures { - if !slices.Contains(expectedFeatureToBeAlreadyCreated, tierFeature) { - clusterAssertion.HasNoResource(fmt.Sprintf("%s-for-%s", tierFeature, spacename), "av1.ClusterResourceQuota{}) - } - } - } - }) + // when + err := manager.ensure(ctx, nsTmplSet) + + // then + require.NoError(t, err) + AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). + HasFinalizer(). + HasConditions(Provisioning()) + + // check that the resources not guarded by a feature gate are there + clusterAssertion := AssertThatCluster(t, fakeClient). + HasResource("for-"+spacename, "av1.ClusterResourceQuota{}). + HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}). + HasResource(spacename+"-dev", &toolchainv1alpha1.Idler{}). + HasResource(spacename+"-stage", &toolchainv1alpha1.Idler{}) + + // check that only the resources for the enabled features were deployed + enabledFeatures := strings.Split(testRun.enabledFeatures, ",") + for _, f := range allTierFeatures { + if slices.Contains(enabledFeatures, f) { + clusterAssertion.HasResource(fmt.Sprintf("%s-for-%s", f, spacename), "av1.ClusterResourceQuota{}) + } else { + clusterAssertion.HasNoResource(fmt.Sprintf("%s-for-%s", f, spacename), "av1.ClusterResourceQuota{}) } - }) + } }) } }) @@ -298,72 +135,23 @@ func TestEnsureClusterResourcesOK(t *testing.T) { manager, fakeClient := prepareClusterResourcesManager(t, nsTmplSet) // when - createdOrUpdated, err := manager.ensure(ctx, nsTmplSet) + err := manager.ensure(ctx, nsTmplSet) // then require.NoError(t, err) - assert.False(t, createdOrUpdated) AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). HasFinalizer(). HasSpecNamespaces("dev"). HasNoConditions() }) - t.Run("should create only one CRQ when the template contains two CRQs", func(t *testing.T) { - // given - nsTmplSet := newNSTmplSet(namespaceName, spacename, "withemptycrq", withNamespaces("abcde11", "dev"), withClusterResources("abcde11")) - manager, fakeClient := prepareClusterResourcesManager(t, nsTmplSet) - - // when - createdOrUpdated, err := manager.ensure(ctx, nsTmplSet) - - // then - require.NoError(t, err) - assert.True(t, createdOrUpdated) - AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). - HasFinalizer(). - HasConditions(Provisioning()) - AssertThatCluster(t, fakeClient). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}). - HasNoResource("for-empty", "av1.ClusterResourceQuota{}). - HasNoResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) - - t.Run("should create the second CRQ when the first one is already created but still not ClusterRoleBinding", func(t *testing.T) { - // when - createdOrUpdated, err := manager.ensure(ctx, nsTmplSet) - - // then - require.NoError(t, err) - assert.True(t, createdOrUpdated) - AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). - HasFinalizer(). - HasConditions(Provisioning()) - AssertThatCluster(t, fakeClient). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}). - HasResource("for-empty", "av1.ClusterResourceQuota{}). - HasNoResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) - - t.Run("should create ClusterRoleBinding when both CRQs are created", func(t *testing.T) { - // when - createdOrUpdated, err := manager.ensure(ctx, nsTmplSet) - - // then - require.NoError(t, err) - assert.True(t, createdOrUpdated) - AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). - HasFinalizer(). - HasConditions(Provisioning()) - AssertThatCluster(t, fakeClient). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}). - HasResource("for-empty", "av1.ClusterResourceQuota{}). - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) - }) - }) - }) - t.Run("should not do anything when all cluster resources are already created", func(t *testing.T) { // given - nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withNamespaces("abcde11", "dev"), withClusterResources("abcde11"), withConditions(Provisioned())) + nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", + withNamespaces("abcde11", "dev"), + withClusterResources("abcde11"), + withPreviouslyAppliedClusterResources("abcde11"), + withConditions(Provisioned())) crq := newClusterResourceQuota(spacename, "advanced") crb := newTektonClusterRoleBinding(spacename, "advanced") idlerDev := newIdler(spacename, spacename+"-dev", "advanced") @@ -371,11 +159,10 @@ func TestEnsureClusterResourcesOK(t *testing.T) { manager, fakeClient := prepareClusterResourcesManager(t, nsTmplSet, crq, crb, idlerDev, idlerStage) // when - createdOrUpdated, err := manager.ensure(ctx, nsTmplSet) + err := manager.ensure(ctx, nsTmplSet) // then require.NoError(t, err) - assert.False(t, createdOrUpdated) AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). HasFinalizer(). HasConditions(Provisioned()) @@ -388,7 +175,6 @@ func TestEnsureClusterResourcesOK(t *testing.T) { } func TestEnsureClusterResourcesFail(t *testing.T) { - // given logger := zap.New(zap.UseDevMode(true)) log.SetLogger(logger) @@ -400,35 +186,17 @@ func TestEnsureClusterResourcesFail(t *testing.T) { restore := test.SetEnvVarAndRestore(t, commonconfig.WatchNamespaceEnvVar, "my-member-operator-namespace") t.Cleanup(restore) - t.Run("fail to list cluster resources", func(t *testing.T) { - // given - manager, fakeClient := prepareClusterResourcesManager(t, nsTmplSet) - fakeClient.MockList = func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { - return errors.New("unable to list cluster resources") - } - - // when - _, err := manager.ensure(ctx, nsTmplSet) - - // then - require.Error(t, err) - assert.Contains(t, err.Error(), "unable to list cluster resources") - AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). - HasFinalizer(). - HasConditions(UnableToProvisionClusterResources("unable to list cluster resources")) - }) - t.Run("fail to get template containing cluster resources", func(t *testing.T) { // given nsTmplSet := newNSTmplSet(namespaceName, spacename, "fail", withNamespaces("abcde11", "dev"), withClusterResources("abcde11")) manager, fakeClient := prepareClusterResourcesManager(t, nsTmplSet) // when - _, err := manager.ensure(ctx, nsTmplSet) + err := manager.ensure(ctx, nsTmplSet) // then require.Error(t, err) - assert.Contains(t, err.Error(), "failed to retrieve TierTemplate for the cluster resources with the name 'fail-clusterresources-abcde11'") + assert.Contains(t, err.Error(), "failed to retrieve the TierTemplate for the to-be-applied cluster resources with the name 'fail-clusterresources-abcde11'") AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). HasFinalizer(). HasConditions(UnableToProvisionClusterResources( @@ -443,93 +211,48 @@ func TestEnsureClusterResourcesFail(t *testing.T) { } // when - _, err := manager.ensure(ctx, nsTmplSet) + err := manager.ensure(ctx, nsTmplSet) // then require.Error(t, err) - assert.Contains(t, err.Error(), "failed to create missing cluster resource of GVK 'quota.openshift.io/v1, Kind=ClusterResourceQuota'") + assert.Contains(t, err.Error(), "some error") AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). HasFinalizer(). HasConditions(UnableToProvisionClusterResources( - "failed to apply cluster resource of type 'quota.openshift.io/v1, Kind=ClusterResourceQuota': unable to create resource of kind: ClusterResourceQuota, version: v1: unable to create resource of kind: ClusterResourceQuota, version: v1: some error")) + "failed to apply changes to the cluster resource for-johnsmith-space, quota.openshift.io/v1, Kind=ClusterResourceQuota: failed to apply cluster resource: unable to create resource of kind: ClusterResourceQuota, version: v1: unable to create resource of kind: ClusterResourceQuota, version: v1: some error")) }) } func TestDeleteClusterResources(t *testing.T) { - // given logger := zap.New(zap.UseDevMode(true)) log.SetLogger(logger) ctx := log.IntoContext(context.TODO(), logger) + restore := test.SetEnvVarAndRestore(t, commonconfig.WatchNamespaceEnvVar, "my-member-operator-namespace") + t.Cleanup(restore) spacename := "johnsmith" namespaceName := "toolchain-member" crq := newClusterResourceQuota(spacename, "advanced") crb := newTektonClusterRoleBinding(spacename, "advanced") - nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withNamespaces("abcde11", "dev", "code"), withDeletionTs(), withClusterResources("abcde11")) + nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withNamespaces("abcde11", "dev", "code"), withDeletionTs(), withClusterResources("abcde11"), withPreviouslyAppliedClusterResources("abcde11")) - t.Run("delete only ClusterResourceQuota", func(t *testing.T) { + t.Run("deletes all cluster resources", func(t *testing.T) { // given manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq, crb) // when - deleted, err := manager.delete(ctx, nsTmplSet) + err := manager.delete(ctx, nsTmplSet) // then require.NoError(t, err) - assert.True(t, deleted) AssertThatCluster(t, cl). HasNoResource("for-"+spacename, "av1.ClusterResourceQuota{}). - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) - - t.Run("delete ClusterRoleBinding since CRQ is already deleted", func(t *testing.T) { - // when - deleted, err := manager.delete(ctx, nsTmplSet) - - // then - require.NoError(t, err) - assert.True(t, deleted) - AssertThatCluster(t, cl). - HasNoResource("for-"+spacename, "av1.ClusterResourceQuota{}). - HasNoResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) - }) - }) - - t.Run("should delete only one ClusterResourceQuota even when tier contains more ", func(t *testing.T) { - // given - nsTmplSet := newNSTmplSet(namespaceName, spacename, "withemptycrq", withNamespaces("abcde11", "dev"), withClusterResources("abcde11")) - crq := newClusterResourceQuota(spacename, "withemptycrq") - emptyCrq := newClusterResourceQuota("empty", "withemptycrq") - emptyCrq.Labels[toolchainv1alpha1.SpaceLabelKey] = spacename - manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq, emptyCrq, crb) - - // when - deleted, err := manager.delete(ctx, nsTmplSet) - - // then - require.NoError(t, err) - assert.True(t, deleted) - AssertThatCluster(t, cl). - HasNoResource("for-empty", "av1.ClusterResourceQuota{}). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}). - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) - - t.Run("delete the for-empty CRQ since it's the last one to be deleted", func(t *testing.T) { - // when - deleted, err := manager.delete(ctx, nsTmplSet) - - // then - require.NoError(t, err) - assert.True(t, deleted) - AssertThatCluster(t, cl). - HasNoResource("for-"+spacename, "av1.ClusterResourceQuota{}). - HasNoResource("for-empty", "av1.ClusterResourceQuota{}). - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) - }) + HasNoResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) }) t.Run("delete the second ClusterResourceQuota since the first one has deletion timestamp set", func(t *testing.T) { // given - nsTmplSet := newNSTmplSet(namespaceName, spacename, "withemptycrq", withNamespaces("abcde11", "dev"), withClusterResources("abcde11")) + nsTmplSet := newNSTmplSet(namespaceName, spacename, "withemptycrq", withNamespaces("abcde11", "dev"), withClusterResources("abcde11"), withPreviouslyAppliedClusterResources("abcde11")) crq := newClusterResourceQuota(spacename, "withemptycrq", withFinalizer()) deletionTS := metav1.NewTime(time.Now()) crq.SetDeletionTimestamp(&deletionTS) @@ -538,11 +261,10 @@ func TestDeleteClusterResources(t *testing.T) { manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq, emptyCrq) // when - deleted, err := manager.delete(ctx, nsTmplSet) + err := manager.delete(ctx, nsTmplSet) // then require.NoError(t, err) - assert.True(t, deleted) AssertThatCluster(t, cl). HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, HasDeletionTimestamp()). HasNoResource("for-empty", "av1.ClusterResourceQuota{}) @@ -556,17 +278,17 @@ func TestDeleteClusterResources(t *testing.T) { withNamespaces("abcde11", "dev", "code"), withDeletionTs(), withClusterResources("abcde11"), + withPreviouslyAppliedClusterResources("abcde11"), withNSTemplateSetFeatureAnnotation("feature-2")) crq := newClusterResourceQuota(spacename, "advanced", withFeatureAnnotation("feature-2"), withName("feature-2-for-"+spacename)) manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq) // when - deleted, err := manager.delete(ctx, nsTmplSet) + err := manager.delete(ctx, nsTmplSet) // then require.NoError(t, err) - assert.True(t, deleted) AssertThatCluster(t, cl). HasNoResource(crq.Name, "av1.ClusterResourceQuota{}) }) @@ -576,11 +298,10 @@ func TestDeleteClusterResources(t *testing.T) { manager, cl := prepareClusterResourcesManager(t, nsTmplSet) // when - deleted, err := manager.delete(ctx, nsTmplSet) + err := manager.delete(ctx, nsTmplSet) // then require.NoError(t, err) - assert.False(t, deleted) AssertThatCluster(t, cl). HasNoResource("for-"+spacename, "av1.ClusterResourceQuota{}) }) @@ -593,11 +314,10 @@ func TestDeleteClusterResources(t *testing.T) { } // when - deleted, err := manager.delete(ctx, nsTmplSet) + err := manager.delete(ctx, nsTmplSet) // then require.Error(t, err) - assert.False(t, deleted) assert.Equal(t, "failed to delete cluster resource 'for-johnsmith': mock error", err.Error()) AssertThatNSTemplateSet(t, namespaceName, spacename, cl). HasFinalizer(). // finalizer was not added and nothing else was done @@ -606,7 +326,6 @@ func TestDeleteClusterResources(t *testing.T) { } func TestPromoteClusterResources(t *testing.T) { - restore := test.SetEnvVarAndRestore(t, commonconfig.WatchNamespaceEnvVar, "my-member-operator-namespace") t.Cleanup(restore) @@ -614,289 +333,163 @@ func TestPromoteClusterResources(t *testing.T) { logger := zap.New(zap.UseDevMode(true)) log.SetLogger(logger) ctx := log.IntoContext(context.TODO(), logger) - spacename := "johnsmith" + spaceName := "johnsmith" namespaceName := "toolchain-member" - crb := newTektonClusterRoleBinding(spacename, "advanced") + crb := newTektonClusterRoleBinding(spaceName, "advanced") t.Run("success", func(t *testing.T) { - t.Run("upgrade from advanced to team tier by changing only the CRQ", func(t *testing.T) { // given - nsTmplSet := newNSTmplSet(namespaceName, spacename, "team", withNamespaces("abcde11", "dev"), withClusterResources("abcde11")) - codeNs := newNamespace("advanced", spacename, "code") - crq := newClusterResourceQuota(spacename, "advanced") - crb := newTektonClusterRoleBinding(spacename, "advanced") - manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq, crb, codeNs) - - // when - updated, err := manager.ensure(ctx, nsTmplSet) - - // then + previousTierTemplate, err := createTierTemplate(scheme.Codecs.UniversalDeserializer(), + test.CreateTemplate( + test.WithObjects(emptyCrq), + test.WithParams(spacename), + ), "advanced", "clusterresources", "previousrevision") require.NoError(t, err) - assert.True(t, updated) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl). - HasFinalizer(). - HasConditions(Updating()) - AssertThatCluster(t, cl). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, - WithLabel("toolchain.dev.openshift.com/templateref", "team-clusterresources-abcde11"), - WithLabel("toolchain.dev.openshift.com/tier", "team"), - Containing(`"limits.cpu":"4","limits.memory":"15Gi"`)). - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}, - WithLabel("toolchain.dev.openshift.com/tier", "advanced")) - - t.Run("upgrade from advanced to team tier by changing only the CRB since CRQ is already changed", func(t *testing.T) { - // when - updated, err := manager.ensure(ctx, nsTmplSet) - // then - require.NoError(t, err) - assert.True(t, updated) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl). - HasFinalizer(). - HasConditions(Updating()) - AssertThatCluster(t, cl). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, - WithLabel("toolchain.dev.openshift.com/tier", "team"), - Containing(`"limits.cpu":"4","limits.memory":"15Gi"`)). - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}, - WithLabel("toolchain.dev.openshift.com/tier", "team")) - }) - }) - - t.Run("upgrade from base to advanced tier by changing only the tier label - the templateref label doesn't change", func(t *testing.T) { - // given - nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withNamespaces("abcde11", "dev"), withClusterResources("abcde11")) - codeNs := newNamespace("advanced", spacename, "code") - crq := newClusterResourceQuota(spacename, "advanced") - crq.Labels["toolchain.dev.openshift.com/tier"] = "base" - crq.Spec.Quota.Hard["limits.cpu"] = resource.MustParse("100m") - manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq, crb, codeNs) + nsTmplSet := newNSTmplSet(namespaceName, spaceName, "team", + withNamespaces("abcde11", "dev"), + withClusterResources("abcde11"), + withPreviouslyAppliedClusterResourcesInTier("advanced", "previousrevision")) + codeNs := newNamespace("advanced", spaceName, "code") + crq := newClusterResourceQuota(spaceName, "advanced") + emptyCrq := newClusterResourceQuota("empty", "advanced") + crb := newTektonClusterRoleBinding(spaceName, "advanced") + manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq, crb, codeNs, previousTierTemplate, emptyCrq) // when - updated, err := manager.ensure(ctx, nsTmplSet) + err = manager.ensure(ctx, nsTmplSet) // then require.NoError(t, err) - assert.True(t, updated) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl). + AssertThatNSTemplateSet(t, namespaceName, spaceName, cl). HasFinalizer(). HasConditions(Updating()) AssertThatCluster(t, cl). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, - WithLabel("toolchain.dev.openshift.com/templateref", "advanced-clusterresources-abcde11"), - WithLabel("toolchain.dev.openshift.com/tier", "advanced"), - Containing(`"limits.cpu":"2","limits.memory":"10Gi"`)) + HasResource("for-"+spaceName, "av1.ClusterResourceQuota{}). + HasNoResource("for-empty", &rbacv1.ClusterRoleBinding{}) }) - t.Run("promote from withemptycrq to advanced tier by removing the redundant CRQ", func(t *testing.T) { + t.Run("promote from 1 tier to another", func(t *testing.T) { // given - nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withNamespaces("dev"), withClusterResources("abcde11")) - codeNs := newNamespace("advanced", spacename, "code") - crq := newClusterResourceQuota(spacename, "withemptycrq") - crb := newTektonClusterRoleBinding(spacename, "withemptycrq") - emptyCrq := newClusterResourceQuota(spacename, "withemptycrq") + previousTierTemplate, err := createTierTemplate(scheme.Codecs.UniversalDeserializer(), + test.CreateTemplate( + test.WithObjects(advancedCrq, clusterTektonRb, emptyCrq), + test.WithParams(spacename), + ), "withemptycrq", "clusterresources", "previousrevision") + require.NoError(t, err) + nsTmplSet := newNSTmplSet(namespaceName, spaceName, "advanced", + withNamespaces("dev"), + withClusterResources("abcde11"), + withPreviouslyAppliedClusterResourcesInTier("withemptycrq", "previousrevision")) + codeNs := newNamespace("advanced", spaceName, "code") + crq := newClusterResourceQuota(spaceName, "withemptycrq") + crq.Labels["disappearingLabel"] = "value" + crq.Spec.Quota.Hard["limits.cpu"] = resource.MustParse("100m") + crb := newTektonClusterRoleBinding(spaceName, "withemptycrq") + crb.Labels["disappearingLabel"] = "value" + emptyCrq := newClusterResourceQuota(spaceName, "withemptycrq") emptyCrq.Name = "for-empty" - manager, cl := prepareClusterResourcesManager(t, nsTmplSet, emptyCrq, crq, crb, codeNs) + manager, cl := prepareClusterResourcesManager(t, nsTmplSet, emptyCrq, crq, crb, codeNs, previousTierTemplate) // when - updated, err := manager.ensure(ctx, nsTmplSet) + err = manager.ensure(ctx, nsTmplSet) // then require.NoError(t, err) - assert.True(t, updated) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl). + AssertThatNSTemplateSet(t, namespaceName, spaceName, cl). HasFinalizer(). HasConditions(Updating()) AssertThatCluster(t, cl). HasNoResource("for-empty", "av1.ClusterResourceQuota{}). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, - WithLabel("toolchain.dev.openshift.com/templateref", "withemptycrq-clusterresources-abcde11"), - WithLabel("toolchain.dev.openshift.com/tier", "withemptycrq")). - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}, - WithLabel("toolchain.dev.openshift.com/templateref", "withemptycrq-clusterresources-abcde11"), - WithLabel("toolchain.dev.openshift.com/tier", "withemptycrq")) - - t.Run("promote from withemptycrq to advanced tier by changing only the CRQ since redundant CRQ is already removed", func(t *testing.T) { - // when - updated, err := manager.ensure(ctx, nsTmplSet) - - // then - require.NoError(t, err) - assert.True(t, updated) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl). - HasFinalizer(). - HasConditions(Updating()) - AssertThatCluster(t, cl). - HasNoResource("for-empty", "av1.ClusterResourceQuota{}). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, - WithLabel("toolchain.dev.openshift.com/templateref", "advanced-clusterresources-abcde11"), - WithLabel("toolchain.dev.openshift.com/tier", "advanced")). - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}, - WithLabel("toolchain.dev.openshift.com/templateref", "withemptycrq-clusterresources-abcde11"), - WithLabel("toolchain.dev.openshift.com/tier", "withemptycrq")) - - }) + HasResource("for-"+spaceName, "av1.ClusterResourceQuota{}, + Containing(`"limits.cpu":"2","limits.memory":"10Gi"`), + WithoutLabel("disappearingLabel")). + HasResource(spaceName+"-tekton-view", &rbacv1.ClusterRoleBinding{}, + WithoutLabel("disappearingLabel")) }) - t.Run("downgrade from advanced to basic tier by removing CRQ", func(t *testing.T) { - // given - nsTmplSet := newNSTmplSet(namespaceName, spacename, "basic", withNamespaces("abcde11", "dev")) - // create namespace (and assume it is complete since it has the expected revision number) - crq := newClusterResourceQuota(spacename, "advanced") - manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq, crb) - - // when - updated, err := manager.ensure(ctx, nsTmplSet) - - // then + t.Run("promoting to no tier (nil cluster resources in spec) deletes all cluster resources", func(t *testing.T) { + previousTierTemplate, err := createTierTemplate(scheme.Codecs.UniversalDeserializer(), + test.CreateTemplate( + test.WithObjects(advancedCrq, clusterTektonRb), + test.WithParams(spacename), + ), "advanced", "clusterresources", "previousrevision") require.NoError(t, err) - assert.True(t, updated) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl). - HasFinalizer(). - HasConditions(Updating()) - AssertThatCluster(t, cl). - HasNoResource("for-"+spacename, "av1.ClusterResourceQuota{}). // no cluster resources in 'basic` tier - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) - - t.Run("downgrade from advanced to basic tier by removing CRB since CRQ is already removed", func(t *testing.T) { - // when - updated, err := manager.ensure(ctx, nsTmplSet) - - // then - require.NoError(t, err) - assert.True(t, updated) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl). - HasFinalizer(). - HasConditions(Updating()) - AssertThatCluster(t, cl). - HasNoResource("for-"+spacename, "av1.ClusterResourceQuota{}). // no cluster resources in 'basic` tier - HasNoResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) - }) - }) - - t.Run("delete redundant cluster resources when ClusterResources field is nil in NSTemplateSet", func(t *testing.T) { - // given 'advanced' NSTemplate only has a cluster resource - nsTmplSet := newNSTmplSet(namespaceName, spacename, "withemptycrq") // no cluster resources, so the "advancedCRQ" should be deleted even if the tier contains the "advancedCRQ" - crq := newClusterResourceQuota(spacename, "advanced") - manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq) + nsTmplSet := newNSTmplSet(namespaceName, spaceName, "withemptycrq", withPreviouslyAppliedClusterResourcesInTier("advanced", "previousrevision")) + crq := newClusterResourceQuota(spaceName, "advanced") + manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq, previousTierTemplate) // when - updated, err := manager.ensure(ctx, nsTmplSet) + err = manager.ensure(ctx, nsTmplSet) // then require.NoError(t, err) - assert.True(t, updated) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl). + AssertThatNSTemplateSet(t, namespaceName, spaceName, cl). HasFinalizer(). HasConditions(Updating()) AssertThatCluster(t, cl). - HasNoResource("for-"+spacename, "av1.ClusterResourceQuota{}). // resources were deleted - HasNoResource("tekton-view-for-"+spacename, &rbacv1.ClusterRole{}). - HasNoResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) + HasNoResource("for-"+spaceName, "av1.ClusterResourceQuota{}). // resources were deleted + HasNoResource(spaceName+"-tekton-view", &rbacv1.ClusterRoleBinding{}) }) - t.Run("upgrade from basic to advanced by creating only CRQ", func(t *testing.T) { + t.Run("promote to a new tier with a new feature enabled", func(t *testing.T) { // given - nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withClusterResources("abcde11")) - manager, cl := prepareClusterResourcesManager(t, nsTmplSet) - - // when - updated, err := manager.ensure(ctx, nsTmplSet) - - // then + previousTierTemplate, err := createTierTemplate(scheme.Codecs.UniversalDeserializer(), + test.CreateTemplate( + test.WithObjects(advancedCrq, clusterTektonRb), + test.WithParams(spacename), + ), "team", "clusterresources", "previousrevision") require.NoError(t, err) - assert.True(t, updated) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl). - HasFinalizer(). - HasConditions(Provisioning()) - AssertThatCluster(t, cl). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, - WithLabel("toolchain.dev.openshift.com/templateref", "advanced-clusterresources-abcde11"), - WithLabel("toolchain.dev.openshift.com/tier", "advanced"), - Containing(`"limits.cpu":"2","limits.memory":"10Gi"`)). - HasNoResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) - - t.Run("upgrade from basic to advanced by creating CRB since CRQ is already created", func(t *testing.T) { - // when - updated, err := manager.ensure(ctx, nsTmplSet) - - // then - require.NoError(t, err) - assert.True(t, updated) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl). - HasFinalizer(). - HasConditions(Provisioning()) - AssertThatCluster(t, cl). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, - WithLabel("toolchain.dev.openshift.com/tier", "advanced"), - Containing(`"limits.cpu":"2","limits.memory":"10Gi"`)). - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}, - WithLabel("toolchain.dev.openshift.com/tier", "advanced")) - }) - }) - - t.Run("upgrade from team to advanced with enabled features by updating existing CRQ", func(t *testing.T) { - // given nsTmplSet := newNSTmplSet(namespaceName, - spacename, + spaceName, "advanced", withClusterResources("abcde11"), + withPreviouslyAppliedClusterResourcesInTier("team", "previousrevision"), withNSTemplateSetFeatureAnnotation("feature-1")) - devNs := newNamespace("team", spacename, "dev") - crq := newClusterResourceQuota(spacename, "team") - manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq, devNs) + devNs := newNamespace("team", spaceName, "dev") + crq := newClusterResourceQuota(spaceName, "team") + crq.Spec.Quota.Hard["limits.cpu"] = resource.MustParse("100m") + manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq, devNs, previousTierTemplate) // when - updated, err := manager.ensure(ctx, nsTmplSet) + err = manager.ensure(ctx, nsTmplSet) // then require.NoError(t, err) - assert.True(t, updated) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl). + AssertThatNSTemplateSet(t, namespaceName, spaceName, cl). HasFinalizer(). HasConditions(Updating()) AssertThatCluster(t, cl). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, - WithLabel("toolchain.dev.openshift.com/templateref", "advanced-clusterresources-abcde11"), - WithLabel("toolchain.dev.openshift.com/tier", "advanced"), + HasResource("for-"+spaceName, "av1.ClusterResourceQuota{}, Containing(`"limits.cpu":"2","limits.memory":"10Gi"`)). - HasNoResource("feature-1-for-"+spacename, "av1.ClusterResourceQuota{}) - - t.Run("upgrade from team to advanced by creating featured CRQ since regular CRQ is already created", func(t *testing.T) { - // when - updated, err := manager.ensure(ctx, nsTmplSet) - - // then - require.NoError(t, err) - assert.True(t, updated) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl). - HasFinalizer(). - HasConditions(Updating()) - AssertThatCluster(t, cl). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}). - HasResource("feature-1-for-"+spacename, "av1.ClusterResourceQuota{}) - }) + HasResource("feature-1-for-"+spaceName, "av1.ClusterResourceQuota{}) }) - t.Run("downgrade from advanced to team with featured CRQ to be deleted", func(t *testing.T) { + t.Run("promote to a new tier with feature enabled", func(t *testing.T) { // given + previousTierTemplate, err := createTierTemplate(scheme.Codecs.UniversalDeserializer(), + test.CreateTemplate( + test.WithObjects(advancedCrq, clusterTektonRb, crqFeature1), + test.WithParams(spacename), + ), "advanced", "clusterresources", "previousrevision") + require.NoError(t, err) nsTmplSet := newNSTmplSet(namespaceName, - spacename, + spaceName, "team", withClusterResources("abcde11"), + withPreviouslyAppliedClusterResourcesInTier("advanced", "previousrevision"), withNSTemplateSetFeatureAnnotation("feature-1")) - devNs := newNamespace("advanced", spacename, "dev") - crq := newClusterResourceQuota(spacename, "advanced", withFeatureAnnotation("feature-1"), withName("feature-1-for-"+spacename)) - manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq, devNs) + devNs := newNamespace("advanced", spaceName, "dev") + crq := newClusterResourceQuota(spaceName, "advanced", withFeatureAnnotation("feature-1"), withName("feature-1-for-"+spaceName)) + manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq, devNs, previousTierTemplate) // when - updated, err := manager.ensure(ctx, nsTmplSet) + err = manager.ensure(ctx, nsTmplSet) // then require.NoError(t, err) - assert.True(t, updated) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl). + AssertThatNSTemplateSet(t, namespaceName, spaceName, cl). HasFinalizer(). HasConditions(Updating()) AssertThatCluster(t, cl). @@ -906,184 +499,132 @@ func TestPromoteClusterResources(t *testing.T) { t.Run("with another user", func(t *testing.T) { // given anotherNsTmplSet := newNSTmplSet(namespaceName, "another-user", "basic") - advancedCRQ := newClusterResourceQuota(spacename, "advanced") + advancedCRQ := newClusterResourceQuota(spaceName, "advanced") anotherCRQ := newClusterResourceQuota("another-user", "basic") anotherCrb := newTektonClusterRoleBinding("another", "basic") - idlerDev := newIdler(spacename, spacename+"-dev", "advanced") - idlerStage := newIdler(spacename, spacename+"-stage", "advanced") + idlerDev := newIdler(spaceName, spaceName+"-dev", "advanced") + idlerStage := newIdler(spaceName, spaceName+"-stage", "advanced") anotherIdlerDev := newIdler("another", "another-dev", "advanced") anotherIdlerStage := newIdler("another", "another-stage", "advanced") t.Run("no redundant cluster resources to be deleted for the given user", func(t *testing.T) { // given - nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withConditions(Provisioned()), withClusterResources("abcde11")) + nsTmplSet := newNSTmplSet(namespaceName, spaceName, "advanced", withConditions(Provisioned()), withClusterResources("abcde11"), withPreviouslyAppliedClusterResources("abcde11")) manager, cl := prepareClusterResourcesManager(t, anotherNsTmplSet, anotherCRQ, nsTmplSet, advancedCRQ, anotherCrb, crb, idlerDev, idlerStage, anotherIdlerDev, anotherIdlerStage) // when - updated, err := manager.ensure(ctx, nsTmplSet) + err := manager.ensure(ctx, nsTmplSet) // then require.NoError(t, err) - assert.False(t, updated) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl). + AssertThatNSTemplateSet(t, namespaceName, spaceName, cl). HasFinalizer(). HasConditions(Provisioned()) AssertThatCluster(t, cl). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}). + HasResource("for-"+spaceName, "av1.ClusterResourceQuota{}). HasResource("for-another-user", "av1.ClusterResourceQuota{}). - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}). + HasResource(spaceName+"-tekton-view", &rbacv1.ClusterRoleBinding{}). HasResource("another-tekton-view", &rbacv1.ClusterRoleBinding{}). - HasResource(spacename+"-dev", &toolchainv1alpha1.Idler{}). - HasResource(spacename+"-stage", &toolchainv1alpha1.Idler{}). + HasResource(spaceName+"-dev", &toolchainv1alpha1.Idler{}). + HasResource(spaceName+"-stage", &toolchainv1alpha1.Idler{}). HasResource("another-dev", &toolchainv1alpha1.Idler{}). HasResource("another-stage", &toolchainv1alpha1.Idler{}) }) - t.Run("cluster resources should be deleted since it doesn't contain clusterResources template", func(t *testing.T) { + t.Run("promoting to no tier should delete only the resources belonging to the nstemplateset", func(t *testing.T) { // given - nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withConditions(Provisioned())) - manager, cl := prepareClusterResourcesManager(t, anotherNsTmplSet, anotherCRQ, nsTmplSet, advancedCRQ, anotherCrb, crb) - - // when - let remove everything - var err error - updated := true - for ; updated; updated, err = manager.ensure(ctx, nsTmplSet) { - require.NoError(t, err) - } + previousTierTemplate, err := createTierTemplate(scheme.Codecs.UniversalDeserializer(), + test.CreateTemplate( + test.WithObjects(advancedCrq, clusterTektonRb, crqFeature1), + test.WithParams(spacename), + ), "advanced", "clusterresources", "previousrevision") + require.NoError(t, err) + nsTmplSet := newNSTmplSet(namespaceName, spaceName, "advanced", withConditions(Provisioned()), withPreviouslyAppliedClusterResources("previousrevision")) + manager, cl := prepareClusterResourcesManager(t, anotherNsTmplSet, anotherCRQ, nsTmplSet, advancedCRQ, anotherCrb, crb, previousTierTemplate) + + err = manager.ensure(ctx, nsTmplSet) // then require.NoError(t, err) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl). + AssertThatNSTemplateSet(t, namespaceName, spaceName, cl). HasFinalizer(). HasConditions(Updating()) AssertThatCluster(t, cl). - HasNoResource("for-"+spacename, "av1.ClusterResourceQuota{}). - HasNoResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}). + HasNoResource("for-"+spaceName, "av1.ClusterResourceQuota{}). + HasNoResource(spaceName+"-tekton-view", &rbacv1.ClusterRoleBinding{}). HasResource("for-another-user", "av1.ClusterResourceQuota{}). HasResource("another-tekton-view", &rbacv1.ClusterRoleBinding{}) - - }) - }) - - t.Run("delete only one redundant cluster resource during one call", func(t *testing.T) { - // given 'advanced' NSTemplate only has a cluster resource - nsTmplSet := newNSTmplSet(namespaceName, spacename, "basic") // no cluster resources, so the "advancedCRQ" should be deleted - advancedCRQ := newClusterResourceQuota(spacename, "withemptycrq") - anotherCRQ := newClusterResourceQuota(spacename, "withemptycrq") - crb := newTektonClusterRoleBinding(spacename, "withemptycrq") - anotherCRQ.Name = "for-empty" - manager, cl := prepareClusterResourcesManager(t, nsTmplSet, advancedCRQ, anotherCRQ, crb) - - // when - updated, err := manager.ensure(ctx, nsTmplSet) - - // then - require.NoError(t, err) - assert.True(t, updated) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl). - HasFinalizer(). - HasConditions(Updating()) // - quotas := "av1.ClusterResourceQuotaList{} - err = cl.List(context.TODO(), quotas, &client.ListOptions{}) - require.NoError(t, err) - assert.Len(t, quotas.Items, 1) - AssertThatCluster(t, cl). - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) - - t.Run("it should delete the second for-empty CRQ since it's the last one", func(t *testing.T) { - // when - should delete the second ClusterResourceQuota - updated, err = manager.ensure(ctx, nsTmplSet) - - // then - require.NoError(t, err) - assert.True(t, updated) - err = cl.List(context.TODO(), quotas, &client.ListOptions{}) - require.NoError(t, err) - assert.Empty(t, quotas.Items) - AssertThatCluster(t, cl). - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) - - t.Run("it should delete the CRB since both CRQs are already removed", func(t *testing.T) { - // when - should delete the second ClusterResourceQuota - updated, err = manager.ensure(ctx, nsTmplSet) - - // then - require.NoError(t, err) - assert.True(t, updated) - err = cl.List(context.TODO(), quotas, &client.ListOptions{}) - require.NoError(t, err) - assert.Empty(t, quotas.Items) - roleBindings := &rbacv1.ClusterRoleBindingList{} - err = cl.List(context.TODO(), roleBindings, &client.ListOptions{}) - require.NoError(t, err) - assert.Empty(t, roleBindings.Items) - }) }) }) }) t.Run("failure", func(t *testing.T) { - - t.Run("promotion to another tier fails because it cannot list current resources", func(t *testing.T) { + t.Run("promotion to another tier fails because it cannot create resources", func(t *testing.T) { // given - nsTmplSet := newNSTmplSet(namespaceName, spacename, "basic", withNamespaces("abcde11", "dev"), withConditions(Updating())) - crq := newClusterResourceQuota(spacename, "fail") - crb := newTektonClusterRoleBinding(spacename, "fail") - manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq, crb) - cl.MockList = func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + previousTierTemplate, err := createTierTemplate(scheme.Codecs.UniversalDeserializer(), + test.CreateTemplate( + test.WithObjects(advancedCrq), + test.WithParams(spacename), + ), "fail", "clusterresources", "previousrevision") + require.NoError(t, err) + nsTmplSet := newNSTmplSet(namespaceName, spaceName, "advanced", + withNamespaces("abcde11", "dev"), + withConditions(Updating()), + withClusterResources("abcde11"), + withPreviouslyAppliedClusterResourcesInTier("fail", "previousrevision")) + crq := newClusterResourceQuota(spaceName, "fail") + crb := newTektonClusterRoleBinding(spaceName, "fail") + manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq, crb, previousTierTemplate) + cl.MockCreate = func(_ context.Context, _ client.Object, _ ...client.CreateOption) error { return fmt.Errorf("some error") } // when - _, err := manager.ensure(ctx, nsTmplSet) + err = manager.ensure(ctx, nsTmplSet) // then require.Error(t, err) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl). + AssertThatNSTemplateSet(t, namespaceName, spaceName, cl). HasFinalizer(). - HasConditions(UpdateFailed("some error")) + HasConditions(UpdateFailed("failed to apply changes to the cluster resource johnsmith-dev, toolchain.dev.openshift.com/v1alpha1, Kind=Idler: failed to apply cluster resource: unable to create resource of kind: Idler, version: v1alpha1: unable to create resource of kind: Idler, version: v1alpha1: some error")) AssertThatCluster(t, cl). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, - WithLabel("toolchain.dev.openshift.com/templateref", "fail-clusterresources-abcde11"), - WithLabel("toolchain.dev.openshift.com/tier", "fail")). - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}, - WithLabel("toolchain.dev.openshift.com/templateref", "fail-clusterresources-abcde11"), - WithLabel("toolchain.dev.openshift.com/tier", "fail")) + HasResource("for-"+spaceName, "av1.ClusterResourceQuota{}). + HasResource(spaceName+"-tekton-view", &rbacv1.ClusterRoleBinding{}) }) t.Run("fail to downgrade from advanced to basic tier", func(t *testing.T) { // given - nsTmplSet := newNSTmplSet(namespaceName, spacename, "basic", withNamespaces("abcde11", "dev")) - crq := newClusterResourceQuota(spacename, "advanced") - manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq, crb) + previousTierTemplate, err := createTierTemplate(scheme.Codecs.UniversalDeserializer(), + test.CreateTemplate( + test.WithObjects(advancedCrq), + test.WithParams(spacename), + ), "advanced", "clusterresources", "previousrevision") + require.NoError(t, err) + nsTmplSet := newNSTmplSet(namespaceName, spaceName, "basic", withNamespaces("abcde11", "dev"), withPreviouslyAppliedClusterResourcesInTier("advanced", "previousrevision")) + crq := newClusterResourceQuota(spaceName, "advanced") + manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq, crb, previousTierTemplate) cl.MockDelete = func(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error { return fmt.Errorf("some error") } // when - updated, err := manager.ensure(ctx, nsTmplSet) + err = manager.ensure(ctx, nsTmplSet) // then require.Error(t, err) - assert.False(t, updated) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl). + AssertThatNSTemplateSet(t, namespaceName, spaceName, cl). HasFinalizer(). HasConditions(UpdateFailed( - "failed to delete an existing redundant cluster resource of name 'for-johnsmith' and gvk 'quota.openshift.io/v1, Kind=ClusterResourceQuota': some error")) + "failed to delete the cluster resource for-johnsmith, quota.openshift.io/v1, Kind=ClusterResourceQuota: some error")) AssertThatCluster(t, cl). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, - WithLabel("toolchain.dev.openshift.com/templateref", "advanced-clusterresources-abcde11"), - WithLabel("toolchain.dev.openshift.com/tier", "advanced")). - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}, - WithLabel("toolchain.dev.openshift.com/templateref", "advanced-clusterresources-abcde11"), - WithLabel("toolchain.dev.openshift.com/tier", "advanced")) + HasResource("for-"+spaceName, "av1.ClusterResourceQuota{}). + HasResource(spaceName+"-tekton-view", &rbacv1.ClusterRoleBinding{}) }) }) } func TestUpdateClusterResources(t *testing.T) { - restore := test.SetEnvVarAndRestore(t, commonconfig.WatchNamespaceEnvVar, "my-member-operator-namespace") t.Cleanup(restore) @@ -1091,66 +632,48 @@ func TestUpdateClusterResources(t *testing.T) { logger := zap.New(zap.UseDevMode(true)) log.SetLogger(logger) ctx := log.IntoContext(context.TODO(), logger) - spacename := "johnsmith" + spaceName := "johnsmith" namespaceName := "toolchain-member" - crb := newTektonClusterRoleBinding(spacename, "advanced") - crq := newClusterResourceQuota(spacename, "advanced") + crb := newTektonClusterRoleBinding(spaceName, "advanced") + crq := newClusterResourceQuota(spaceName, "advanced") t.Run("success", func(t *testing.T) { - t.Run("update from abcde11 revision to abcde12 revision as part of the advanced tier by updating CRQ", func(t *testing.T) { // given - nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withNamespaces("abcde12", "dev"), withClusterResources("abcde12")) - codeNs := newNamespace("advanced", spacename, "dev") + nsTmplSet := newNSTmplSet(namespaceName, spaceName, "advanced", withNamespaces("abcde12", "dev"), withClusterResources("abcde12"), withPreviouslyAppliedClusterResources("abcde11")) + codeNs := newNamespace("advanced", spaceName, "dev") manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq, crb, codeNs) // when - updated, err := manager.ensure(ctx, nsTmplSet) + err := manager.ensure(ctx, nsTmplSet) // then require.NoError(t, err) - assert.True(t, updated) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl).HasConditions(Updating()) + AssertThatNSTemplateSet(t, namespaceName, spaceName, cl).HasConditions(Updating()) AssertThatCluster(t, cl). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, - WithLabel("toolchain.dev.openshift.com/templateref", "advanced-clusterresources-abcde12")). - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}, - WithLabel("toolchain.dev.openshift.com/templateref", "advanced-clusterresources-abcde11")) - - t.Run("update from abcde11 revision to abcde12 revision by deleting CRB since CRQ is already changed", func(t *testing.T) { - // when - updated, err := manager.ensure(ctx, nsTmplSet) - - // then - require.NoError(t, err) - assert.True(t, updated) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl).HasConditions(Updating()) - AssertThatCluster(t, cl). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, - WithLabel("toolchain.dev.openshift.com/templateref", "advanced-clusterresources-abcde12")). - HasNoResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) - }) + HasResource("for-"+spaceName, "av1.ClusterResourceQuota{}). + HasNoResource(spaceName+"-tekton-view", &rbacv1.ClusterRoleBinding{}) }) t.Run("update from abcde11 revision to abcde12 revision as part of the advanced tier by deleting featured CRQ", func(t *testing.T) { // given nsTmplSet := newNSTmplSet(namespaceName, - spacename, + spaceName, "advanced", withNamespaces("abcde12", "dev"), withClusterResources("abcde12"), + withPreviouslyAppliedClusterResources("abcde11"), withNSTemplateSetFeatureAnnotation("feature-1")) - codeNs := newNamespace("advanced", spacename, "dev") - crqFeatured := newClusterResourceQuota(spacename, "advanced", withName("feature-1-for-"+spacename), withFeatureAnnotation("feature-1")) + codeNs := newNamespace("advanced", spaceName, "dev") + crqFeatured := newClusterResourceQuota(spaceName, "advanced", withName("feature-1-for-"+spaceName), withFeatureAnnotation("feature-1")) manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crqFeatured, codeNs) // when - updated, err := manager.ensure(ctx, nsTmplSet) + err := manager.ensure(ctx, nsTmplSet) // then require.NoError(t, err) - assert.True(t, updated) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl).HasConditions(Updating()) + AssertThatNSTemplateSet(t, namespaceName, spaceName, cl).HasConditions(Updating()) AssertThatCluster(t, cl). HasNoResource(crqFeatured.Name, "av1.ClusterResourceQuota{}) }) @@ -1158,26 +681,26 @@ func TestUpdateClusterResources(t *testing.T) { t.Run("update from abcde12 revision to abcde11 by restoring featured CRQ", func(t *testing.T) { // given nsTmplSet := newNSTmplSet(namespaceName, - spacename, + spaceName, "advanced", withNamespaces("abcde11", "dev"), withClusterResources("abcde11"), + withPreviouslyAppliedClusterResources("abcde12"), withNSTemplateSetFeatureAnnotation("feature-1")) - codeNs := newNamespace("advanced", spacename, "dev") - crqFeatured := newClusterResourceQuota(spacename, + codeNs := newNamespace("advanced", spaceName, "dev") + crqFeatured := newClusterResourceQuota(spaceName, "advanced", - withName("feature-1-for-"+spacename), + withName("feature-1-for-"+spaceName), withTemplateRefUsingRevision("abcde12"), withFeatureAnnotation("feature-1")) manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crqFeatured, codeNs) // when - updated, err := manager.ensure(ctx, nsTmplSet) + err := manager.ensure(ctx, nsTmplSet) // then require.NoError(t, err) - assert.True(t, updated) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl).HasConditions(Updating()) + AssertThatNSTemplateSet(t, namespaceName, spaceName, cl).HasConditions(Updating()) AssertThatCluster(t, cl). HasResource(crqFeatured.Name, "av1.ClusterResourceQuota{}, WithLabel("toolchain.dev.openshift.com/templateref", "advanced-clusterresources-abcde11")) @@ -1185,82 +708,69 @@ func TestUpdateClusterResources(t *testing.T) { t.Run("update from abcde12 revision to abcde11 revision as part of the advanced tier by updating CRQ", func(t *testing.T) { // given - nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withNamespaces("abcde11", "dev"), withClusterResources("abcde11")) - crq := newClusterResourceQuota(spacename, "advanced", withTemplateRefUsingRevision("abcde12")) + nsTmplSet := newNSTmplSet(namespaceName, spaceName, "advanced", withNamespaces("abcde11", "dev"), withClusterResources("abcde11"), withPreviouslyAppliedClusterResources("abcde12")) + crq := newClusterResourceQuota(spaceName, "advanced", withTemplateRefUsingRevision("abcde12")) manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq) // when - updated, err := manager.ensure(ctx, nsTmplSet) + err := manager.ensure(ctx, nsTmplSet) // then require.NoError(t, err) - assert.True(t, updated) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl).HasConditions(Updating()) + AssertThatNSTemplateSet(t, namespaceName, spaceName, cl).HasConditions(Updating()) AssertThatCluster(t, cl). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, - WithLabel("toolchain.dev.openshift.com/templateref", "advanced-clusterresources-abcde11")). - HasNoResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) - - t.Run("update from abcde12 revision to abcde11 revision as part of the advanced tier by creating CRB", func(t *testing.T) { - // when - updated, err := manager.ensure(ctx, nsTmplSet) - - // then - require.NoError(t, err) - assert.True(t, updated) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl).HasConditions(Updating()) - AssertThatCluster(t, cl). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, - WithLabel("toolchain.dev.openshift.com/templateref", "advanced-clusterresources-abcde11")). - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}, - WithLabel("toolchain.dev.openshift.com/templateref", "advanced-clusterresources-abcde11")) - }) + HasResource("for-"+spaceName, "av1.ClusterResourceQuota{}). + HasResource(spaceName+"-tekton-view", &rbacv1.ClusterRoleBinding{}) }) }) t.Run("failure", func(t *testing.T) { - t.Run("update to abcde11 fails because it cannot list current resources", func(t *testing.T) { // given - nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withClusterResources("abcde11"), withConditions(Updating())) - manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq) - cl.MockList = func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + previousTierTemplate, err := createTierTemplate(scheme.Codecs.UniversalDeserializer(), + test.CreateTemplate( + test.WithObjects(advancedCrq), + test.WithParams(spacename), + ), "advanced", "clusterresources", "previousrevision") + require.NoError(t, err) + nsTmplSet := newNSTmplSet(namespaceName, spaceName, "advanced", withClusterResources("abcde11"), withConditions(Updating()), withPreviouslyAppliedClusterResources("previousrevision")) + manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq, previousTierTemplate) + cl.MockCreate = func(_ context.Context, _ client.Object, _ ...client.CreateOption) error { return fmt.Errorf("some error") } // when - _, err := manager.ensure(ctx, nsTmplSet) + err = manager.ensure(ctx, nsTmplSet) // then require.Error(t, err) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl). + AssertThatNSTemplateSet(t, namespaceName, spaceName, cl). HasFinalizer(). - HasConditions(UpdateFailed("some error")) + HasConditions(UpdateFailed("failed to apply changes to the cluster resource johnsmith-tekton-view, rbac.authorization.k8s.io/v1, Kind=ClusterRoleBinding: failed to apply cluster resource: unable to create resource of kind: ClusterRoleBinding, version: v1: unable to create resource of kind: ClusterRoleBinding, version: v1: some error")) AssertThatCluster(t, cl). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, + HasResource("for-"+spaceName, "av1.ClusterResourceQuota{}, WithLabel("toolchain.dev.openshift.com/templateref", "advanced-clusterresources-abcde11")). - HasNoResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) + HasNoResource(spaceName+"-tekton-view", &rbacv1.ClusterRoleBinding{}) }) t.Run("update to abcde13 fails because it find the template", func(t *testing.T) { // given - nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withClusterResources("abcde13"), withConditions(Updating())) + nsTmplSet := newNSTmplSet(namespaceName, spaceName, "advanced", withClusterResources("abcde13"), withConditions(Updating())) manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq, crb) // when - updated, err := manager.ensure(ctx, nsTmplSet) + err := manager.ensure(ctx, nsTmplSet) // then require.Error(t, err) - assert.False(t, updated) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl). + AssertThatNSTemplateSet(t, namespaceName, spaceName, cl). HasFinalizer(). HasConditions(UpdateFailed( "unable to retrieve the TierTemplate 'advanced-clusterresources-abcde13' from 'Host' cluster: tiertemplates.toolchain.dev.openshift.com \"advanced-clusterresources-abcde13\" not found")) AssertThatCluster(t, cl). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, + HasResource("for-"+spaceName, "av1.ClusterResourceQuota{}, WithLabel("toolchain.dev.openshift.com/templateref", "advanced-clusterresources-abcde11")). - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}, + HasResource(spaceName+"-tekton-view", &rbacv1.ClusterRoleBinding{}, WithLabel("toolchain.dev.openshift.com/templateref", "advanced-clusterresources-abcde11")) }) }) @@ -1274,76 +784,33 @@ func TestDeleteFeatureFromNSTemplateSet(t *testing.T) { logger := zap.New(zap.UseDevMode(true)) log.SetLogger(logger) ctx := log.IntoContext(context.TODO(), logger) - spacename := "johnsmith" + spaceName := "johnsmith" namespaceName := "toolchain-member" + previousTierTemplate, err := createTierTemplate(scheme.Codecs.UniversalDeserializer(), + test.CreateTemplate( + test.WithObjects(advancedCrq, crqFeature1), + test.WithParams(spacename), + ), "advanced", "clusterresources", "previousrevision") + require.NoError(t, err) + nsTmplSet := newNSTmplSet(namespaceName, - spacename, + spaceName, "advanced", withNamespaces("abcde11", "dev"), - withClusterResources("abcde11")) // The NSTemplateSet does not have the feature annotation (anymore) - codeNs := newNamespace("advanced", spacename, "dev") - crqFeatured := newClusterResourceQuota(spacename, "advanced", withName("feature-1-for-"+spacename), withFeatureAnnotation("feature-1")) - manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crqFeatured, codeNs) + withClusterResources("abcde11"), + withPreviouslyAppliedClusterResources("previousrevision"), + ) // The NSTemplateSet does not have the feature annotation (anymore) + codeNs := newNamespace("advanced", spaceName, "dev") + crqFeatured := newClusterResourceQuota(spaceName, "advanced", withName("feature-1-for-"+spaceName), withFeatureAnnotation("feature-1")) + manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crqFeatured, codeNs, previousTierTemplate) // when - updated, err := manager.ensure(ctx, nsTmplSet) + err = manager.ensure(ctx, nsTmplSet) // then require.NoError(t, err) - assert.True(t, updated) - AssertThatNSTemplateSet(t, namespaceName, spacename, cl).HasConditions(Updating()) + AssertThatNSTemplateSet(t, namespaceName, spaceName, cl).HasConditions(Updating()) AssertThatCluster(t, cl). HasNoResource(crqFeatured.Name, "av1.ClusterResourceQuota{}) // The featured object is now deleted because the feature was disabled in the NSTemplateSet } - -func TestRetainObjectsOfSameGVK(t *testing.T) { - // given - clusterRole := runtime.RawExtension{Object: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "kind": "ClusterRole", - "apiVersion": "rbac.authorization.k8s.io/v1", - }}} - - namespace := runtime.RawExtension{Object: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "kind": "Namespace", - "apiVersion": "v1", - }}} - clusterResQuota := runtime.RawExtension{Object: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "kind": "ClusterResourceQuota", - "apiVersion": "quota.openshift.io/v1", - }}} - clusterRoleBinding := runtime.RawExtension{Object: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "kind": "ClusterRoleBinding", - "apiVersion": "rbac.authorization.k8s.io/v1", - }}} - - t.Run("verify retainObjectsOfSameGVK function for ClusterRole", func(t *testing.T) { - // given - retain := retainObjectsOfSameGVK(rbacv1.SchemeGroupVersion.WithKind("ClusterRole")) - - t.Run("should return false since the GVK doesn't match", func(t *testing.T) { - for _, obj := range []runtime.RawExtension{namespace, clusterResQuota, clusterRoleBinding} { - - // when - ok := retain(obj) - - // then - assert.False(t, ok) - - } - }) - - t.Run("should return true since the GVK matches", func(t *testing.T) { - - // when - ok := retain(clusterRole) - - // then - assert.True(t, ok) - }) - }) -} diff --git a/controllers/nstemplateset/nstemplateset_controller.go b/controllers/nstemplateset/nstemplateset_controller.go index e5f01cee1..888423dfc 100644 --- a/controllers/nstemplateset/nstemplateset_controller.go +++ b/controllers/nstemplateset/nstemplateset_controller.go @@ -133,12 +133,12 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. // we proceed with the cluster-scoped resources template, then all namespaces and finally space roles // as we want to be sure that cluster-scoped resources such as quotas are set // even before the namespaces exist - if createdOrUpdated, err := r.clusterResources.ensure(ctx, nsTmplSet); err != nil { + // NOTE: the cluster resources are applied ALL AT ONCE, unlike the namespaces. + // This is a work-in-progress change to unify how we apply cluster resources and namespaces. + // In the end, everything will be applied in one go. + if err := r.clusterResources.ensure(ctx, nsTmplSet); err != nil { logger.Error(err, "failed to either provision or update cluster resources") return reconcile.Result{}, err - } else if createdOrUpdated { - // we need to requeue to make sure we apply all cluster resources before continuing further - return reconcile.Result{Requeue: true}, nil // wait for cluster resources to be created } if err := r.status.updateStatusClusterResourcesRevisions(ctx, nsTmplSet); err != nil { return reconcile.Result{}, err @@ -217,10 +217,9 @@ func (r *Reconciler) deleteNSTemplateSet(ctx context.Context, nsTmplSet *toolcha } // if no namespace was to be deleted, then we can proceed with the cluster resources associated with the user - deletedAny, err := r.clusterResources.delete(ctx, nsTmplSet) - if err != nil || deletedAny { - // we need to check if there are some more cluster resources left - return reconcile.Result{Requeue: true}, err + err = r.clusterResources.delete(ctx, nsTmplSet) + if err != nil { + return reconcile.Result{}, err } // if nothing was to be deleted, then we can remove the finalizer and we're done diff --git a/controllers/nstemplateset/nstemplateset_controller_test.go b/controllers/nstemplateset/nstemplateset_controller_test.go index 5381a392c..21340ee9c 100644 --- a/controllers/nstemplateset/nstemplateset_controller_test.go +++ b/controllers/nstemplateset/nstemplateset_controller_test.go @@ -1750,6 +1750,22 @@ func withClusterResources(revision string) nsTmplSetOption { } } +func withPreviouslyAppliedClusterResources(revision string) nsTmplSetOption { + return func(nsTmplSet *toolchainv1alpha1.NSTemplateSet) { + nsTmplSet.Status.ClusterResources = &toolchainv1alpha1.NSTemplateSetClusterResources{ + TemplateRef: NewTierTemplateName(nsTmplSet.Spec.TierName, "clusterresources", revision), + } + } +} + +func withPreviouslyAppliedClusterResourcesInTier(tierName, revision string) nsTmplSetOption { + return func(nsTmplSet *toolchainv1alpha1.NSTemplateSet) { + nsTmplSet.Status.ClusterResources = &toolchainv1alpha1.NSTemplateSetClusterResources{ + TemplateRef: NewTierTemplateName(tierName, "clusterresources", revision), + } + } +} + func withSpaceRoles(roles map[string][]string) nsTmplSetOption { return func(nsTmplSet *toolchainv1alpha1.NSTemplateSet) { nsTmplSet.Spec.SpaceRoles = make([]toolchainv1alpha1.NSTemplateSetSpaceRole, 0, len(roles)) diff --git a/test/cluster_assertion.go b/test/cluster_assertion.go index d181e1fe0..9c76115f6 100644 --- a/test/cluster_assertion.go +++ b/test/cluster_assertion.go @@ -50,6 +50,13 @@ func WithLabel(key, value string) ResourceOption { } } +func WithoutLabel(key string) ResourceOption { + return func(t test.T, obj client.Object) { + _, exists := obj.GetLabels()[key] + assert.False(t, exists) + } +} + func Containing(value string) ResourceOption { return func(t test.T, obj client.Object) { content, err := json.Marshal(obj) diff --git a/test/nstemplateset_assertion.go b/test/nstemplateset_assertion.go index d66a3782d..b579c18b3 100644 --- a/test/nstemplateset_assertion.go +++ b/test/nstemplateset_assertion.go @@ -160,11 +160,19 @@ func (a *NSTemplateSetAssertion) HasTierName(tierName string) *NSTemplateSetAsse func (a *NSTemplateSetAssertion) HasClusterResourcesTemplateRef(templateRef string) *NSTemplateSetAssertion { err := a.loadNSTemplateSet() require.NoError(a.t, err) - assert.NotNil(a.t, a.nsTmplSet.Spec.ClusterResources.TemplateRef) + assert.NotNil(a.t, a.nsTmplSet.Spec.ClusterResources) assert.Equal(a.t, a.nsTmplSet.Spec.ClusterResources.TemplateRef, templateRef) return a } +func (a *NSTemplateSetAssertion) HasLastAppliedClusterResourcesTemplateRef(templateRef string) *NSTemplateSetAssertion { + err := a.loadNSTemplateSet() + require.NoError(a.t, err) + assert.NotNil(a.t, a.nsTmplSet.Status.ClusterResources) + assert.Equal(a.t, a.nsTmplSet.Status.ClusterResources.TemplateRef, templateRef) + return a +} + func (a *NSTemplateSetAssertion) HasClusterResourcesNil() *NSTemplateSetAssertion { err := a.loadNSTemplateSet() require.NoError(a.t, err) From 763f4c36d12121159644d724892edaaf72bfdb8d Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Mon, 8 Sep 2025 18:10:38 +0200 Subject: [PATCH 02/16] forgot about the rest of the tests, d'oh --- .../nstemplateset/cluster_resources_test.go | 38 +- .../nstemplateset_controller_test.go | 714 +++++------------- 2 files changed, 218 insertions(+), 534 deletions(-) diff --git a/controllers/nstemplateset/cluster_resources_test.go b/controllers/nstemplateset/cluster_resources_test.go index 006126366..19e4b7da0 100644 --- a/controllers/nstemplateset/cluster_resources_test.go +++ b/controllers/nstemplateset/cluster_resources_test.go @@ -150,7 +150,7 @@ func TestEnsureClusterResourcesOK(t *testing.T) { nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withNamespaces("abcde11", "dev"), withClusterResources("abcde11"), - withPreviouslyAppliedClusterResources("abcde11"), + withStatusClusterResources("abcde11"), withConditions(Provisioned())) crq := newClusterResourceQuota(spacename, "advanced") crb := newTektonClusterRoleBinding(spacename, "advanced") @@ -234,7 +234,7 @@ func TestDeleteClusterResources(t *testing.T) { namespaceName := "toolchain-member" crq := newClusterResourceQuota(spacename, "advanced") crb := newTektonClusterRoleBinding(spacename, "advanced") - nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withNamespaces("abcde11", "dev", "code"), withDeletionTs(), withClusterResources("abcde11"), withPreviouslyAppliedClusterResources("abcde11")) + nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withNamespaces("abcde11", "dev", "code"), withDeletionTs(), withClusterResources("abcde11"), withStatusClusterResources("abcde11")) t.Run("deletes all cluster resources", func(t *testing.T) { // given @@ -252,7 +252,7 @@ func TestDeleteClusterResources(t *testing.T) { t.Run("delete the second ClusterResourceQuota since the first one has deletion timestamp set", func(t *testing.T) { // given - nsTmplSet := newNSTmplSet(namespaceName, spacename, "withemptycrq", withNamespaces("abcde11", "dev"), withClusterResources("abcde11"), withPreviouslyAppliedClusterResources("abcde11")) + nsTmplSet := newNSTmplSet(namespaceName, spacename, "withemptycrq", withNamespaces("abcde11", "dev"), withClusterResources("abcde11"), withStatusClusterResources("abcde11")) crq := newClusterResourceQuota(spacename, "withemptycrq", withFinalizer()) deletionTS := metav1.NewTime(time.Now()) crq.SetDeletionTimestamp(&deletionTS) @@ -278,7 +278,7 @@ func TestDeleteClusterResources(t *testing.T) { withNamespaces("abcde11", "dev", "code"), withDeletionTs(), withClusterResources("abcde11"), - withPreviouslyAppliedClusterResources("abcde11"), + withStatusClusterResources("abcde11"), withNSTemplateSetFeatureAnnotation("feature-2")) crq := newClusterResourceQuota(spacename, "advanced", withFeatureAnnotation("feature-2"), withName("feature-2-for-"+spacename)) @@ -350,7 +350,7 @@ func TestPromoteClusterResources(t *testing.T) { nsTmplSet := newNSTmplSet(namespaceName, spaceName, "team", withNamespaces("abcde11", "dev"), withClusterResources("abcde11"), - withPreviouslyAppliedClusterResourcesInTier("advanced", "previousrevision")) + withStatusClusterResourcesInTier("advanced", "previousrevision")) codeNs := newNamespace("advanced", spaceName, "code") crq := newClusterResourceQuota(spaceName, "advanced") emptyCrq := newClusterResourceQuota("empty", "advanced") @@ -381,7 +381,7 @@ func TestPromoteClusterResources(t *testing.T) { nsTmplSet := newNSTmplSet(namespaceName, spaceName, "advanced", withNamespaces("dev"), withClusterResources("abcde11"), - withPreviouslyAppliedClusterResourcesInTier("withemptycrq", "previousrevision")) + withStatusClusterResourcesInTier("withemptycrq", "previousrevision")) codeNs := newNamespace("advanced", spaceName, "code") crq := newClusterResourceQuota(spaceName, "withemptycrq") crq.Labels["disappearingLabel"] = "value" @@ -416,7 +416,7 @@ func TestPromoteClusterResources(t *testing.T) { test.WithParams(spacename), ), "advanced", "clusterresources", "previousrevision") require.NoError(t, err) - nsTmplSet := newNSTmplSet(namespaceName, spaceName, "withemptycrq", withPreviouslyAppliedClusterResourcesInTier("advanced", "previousrevision")) + nsTmplSet := newNSTmplSet(namespaceName, spaceName, "withemptycrq", withStatusClusterResourcesInTier("advanced", "previousrevision")) crq := newClusterResourceQuota(spaceName, "advanced") manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq, previousTierTemplate) @@ -445,7 +445,7 @@ func TestPromoteClusterResources(t *testing.T) { spaceName, "advanced", withClusterResources("abcde11"), - withPreviouslyAppliedClusterResourcesInTier("team", "previousrevision"), + withStatusClusterResourcesInTier("team", "previousrevision"), withNSTemplateSetFeatureAnnotation("feature-1")) devNs := newNamespace("team", spaceName, "dev") crq := newClusterResourceQuota(spaceName, "team") @@ -478,7 +478,7 @@ func TestPromoteClusterResources(t *testing.T) { spaceName, "team", withClusterResources("abcde11"), - withPreviouslyAppliedClusterResourcesInTier("advanced", "previousrevision"), + withStatusClusterResourcesInTier("advanced", "previousrevision"), withNSTemplateSetFeatureAnnotation("feature-1")) devNs := newNamespace("advanced", spaceName, "dev") crq := newClusterResourceQuota(spaceName, "advanced", withFeatureAnnotation("feature-1"), withName("feature-1-for-"+spaceName)) @@ -510,7 +510,7 @@ func TestPromoteClusterResources(t *testing.T) { t.Run("no redundant cluster resources to be deleted for the given user", func(t *testing.T) { // given - nsTmplSet := newNSTmplSet(namespaceName, spaceName, "advanced", withConditions(Provisioned()), withClusterResources("abcde11"), withPreviouslyAppliedClusterResources("abcde11")) + nsTmplSet := newNSTmplSet(namespaceName, spaceName, "advanced", withConditions(Provisioned()), withClusterResources("abcde11"), withStatusClusterResources("abcde11")) manager, cl := prepareClusterResourcesManager(t, anotherNsTmplSet, anotherCRQ, nsTmplSet, advancedCRQ, anotherCrb, crb, idlerDev, idlerStage, anotherIdlerDev, anotherIdlerStage) // when @@ -540,7 +540,7 @@ func TestPromoteClusterResources(t *testing.T) { test.WithParams(spacename), ), "advanced", "clusterresources", "previousrevision") require.NoError(t, err) - nsTmplSet := newNSTmplSet(namespaceName, spaceName, "advanced", withConditions(Provisioned()), withPreviouslyAppliedClusterResources("previousrevision")) + nsTmplSet := newNSTmplSet(namespaceName, spaceName, "advanced", withConditions(Provisioned()), withStatusClusterResources("previousrevision")) manager, cl := prepareClusterResourcesManager(t, anotherNsTmplSet, anotherCRQ, nsTmplSet, advancedCRQ, anotherCrb, crb, previousTierTemplate) err = manager.ensure(ctx, nsTmplSet) @@ -572,7 +572,7 @@ func TestPromoteClusterResources(t *testing.T) { withNamespaces("abcde11", "dev"), withConditions(Updating()), withClusterResources("abcde11"), - withPreviouslyAppliedClusterResourcesInTier("fail", "previousrevision")) + withStatusClusterResourcesInTier("fail", "previousrevision")) crq := newClusterResourceQuota(spaceName, "fail") crb := newTektonClusterRoleBinding(spaceName, "fail") manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq, crb, previousTierTemplate) @@ -601,7 +601,7 @@ func TestPromoteClusterResources(t *testing.T) { test.WithParams(spacename), ), "advanced", "clusterresources", "previousrevision") require.NoError(t, err) - nsTmplSet := newNSTmplSet(namespaceName, spaceName, "basic", withNamespaces("abcde11", "dev"), withPreviouslyAppliedClusterResourcesInTier("advanced", "previousrevision")) + nsTmplSet := newNSTmplSet(namespaceName, spaceName, "basic", withNamespaces("abcde11", "dev"), withStatusClusterResourcesInTier("advanced", "previousrevision")) crq := newClusterResourceQuota(spaceName, "advanced") manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq, crb, previousTierTemplate) cl.MockDelete = func(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error { @@ -640,7 +640,7 @@ func TestUpdateClusterResources(t *testing.T) { t.Run("success", func(t *testing.T) { t.Run("update from abcde11 revision to abcde12 revision as part of the advanced tier by updating CRQ", func(t *testing.T) { // given - nsTmplSet := newNSTmplSet(namespaceName, spaceName, "advanced", withNamespaces("abcde12", "dev"), withClusterResources("abcde12"), withPreviouslyAppliedClusterResources("abcde11")) + nsTmplSet := newNSTmplSet(namespaceName, spaceName, "advanced", withNamespaces("abcde12", "dev"), withClusterResources("abcde12"), withStatusClusterResources("abcde11")) codeNs := newNamespace("advanced", spaceName, "dev") manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq, crb, codeNs) @@ -662,7 +662,7 @@ func TestUpdateClusterResources(t *testing.T) { "advanced", withNamespaces("abcde12", "dev"), withClusterResources("abcde12"), - withPreviouslyAppliedClusterResources("abcde11"), + withStatusClusterResources("abcde11"), withNSTemplateSetFeatureAnnotation("feature-1")) codeNs := newNamespace("advanced", spaceName, "dev") crqFeatured := newClusterResourceQuota(spaceName, "advanced", withName("feature-1-for-"+spaceName), withFeatureAnnotation("feature-1")) @@ -685,7 +685,7 @@ func TestUpdateClusterResources(t *testing.T) { "advanced", withNamespaces("abcde11", "dev"), withClusterResources("abcde11"), - withPreviouslyAppliedClusterResources("abcde12"), + withStatusClusterResources("abcde12"), withNSTemplateSetFeatureAnnotation("feature-1")) codeNs := newNamespace("advanced", spaceName, "dev") crqFeatured := newClusterResourceQuota(spaceName, @@ -708,7 +708,7 @@ func TestUpdateClusterResources(t *testing.T) { t.Run("update from abcde12 revision to abcde11 revision as part of the advanced tier by updating CRQ", func(t *testing.T) { // given - nsTmplSet := newNSTmplSet(namespaceName, spaceName, "advanced", withNamespaces("abcde11", "dev"), withClusterResources("abcde11"), withPreviouslyAppliedClusterResources("abcde12")) + nsTmplSet := newNSTmplSet(namespaceName, spaceName, "advanced", withNamespaces("abcde11", "dev"), withClusterResources("abcde11"), withStatusClusterResources("abcde12")) crq := newClusterResourceQuota(spaceName, "advanced", withTemplateRefUsingRevision("abcde12")) manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq) @@ -733,7 +733,7 @@ func TestUpdateClusterResources(t *testing.T) { test.WithParams(spacename), ), "advanced", "clusterresources", "previousrevision") require.NoError(t, err) - nsTmplSet := newNSTmplSet(namespaceName, spaceName, "advanced", withClusterResources("abcde11"), withConditions(Updating()), withPreviouslyAppliedClusterResources("previousrevision")) + nsTmplSet := newNSTmplSet(namespaceName, spaceName, "advanced", withClusterResources("abcde11"), withConditions(Updating()), withStatusClusterResources("previousrevision")) manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq, previousTierTemplate) cl.MockCreate = func(_ context.Context, _ client.Object, _ ...client.CreateOption) error { return fmt.Errorf("some error") @@ -799,7 +799,7 @@ func TestDeleteFeatureFromNSTemplateSet(t *testing.T) { "advanced", withNamespaces("abcde11", "dev"), withClusterResources("abcde11"), - withPreviouslyAppliedClusterResources("previousrevision"), + withStatusClusterResources("previousrevision"), ) // The NSTemplateSet does not have the feature annotation (anymore) codeNs := newNamespace("advanced", spaceName, "dev") crqFeatured := newClusterResourceQuota(spaceName, "advanced", withName("feature-1-for-"+spaceName), withFeatureAnnotation("feature-1")) diff --git a/controllers/nstemplateset/nstemplateset_controller_test.go b/controllers/nstemplateset/nstemplateset_controller_test.go index 21340ee9c..7a8fdaab3 100644 --- a/controllers/nstemplateset/nstemplateset_controller_test.go +++ b/controllers/nstemplateset/nstemplateset_controller_test.go @@ -612,7 +612,7 @@ func TestProvisionTwoUsers(t *testing.T) { restore := test.SetEnvVarAndRestore(t, commonconfig.WatchNamespaceEnvVar, "my-member-operator-namespace") t.Cleanup(restore) - t.Run("provision john's ClusterResourceQuota first", func(t *testing.T) { + t.Run("provision john's cluster resources and namespace first", func(t *testing.T) { // given nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withNamespaces("abcde11", "dev"), withClusterResources("abcde11")) r, req, fakeClient := prepareReconcile(t, namespaceName, spacename, nsTmplSet) @@ -622,231 +622,96 @@ func TestProvisionTwoUsers(t *testing.T) { // then require.NoError(t, err) - assert.Equal(t, reconcile.Result{Requeue: true}, res) + assert.Equal(t, reconcile.Result{}, res) AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). HasFinalizer(). HasSpecNamespaces("dev"). HasConditions(Provisioning()) - AssertThatNamespace(t, spacename+"-dev", fakeClient). - DoesNotExist() AssertThatCluster(t, fakeClient). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}). // created - HasNoResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) + HasResource("for-"+spacename, "av1.ClusterResourceQuota{}). + HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}). + HasResource(spacename+"-dev", &toolchainv1alpha1.Idler{}). + HasResource(spacename+"-stage", &toolchainv1alpha1.Idler{}) + AssertThatNamespace(t, spacename+"-dev", fakeClient). + HasLabel(toolchainv1alpha1.SpaceLabelKey, spacename). + HasLabel(toolchainv1alpha1.TypeLabelKey, "dev"). + HasNoLabel(toolchainv1alpha1.TemplateRefLabelKey). // no label until all the namespace inner resources have been created + HasNoLabel(toolchainv1alpha1.TierLabelKey). + HasLabel(toolchainv1alpha1.ProviderLabelKey, toolchainv1alpha1.ProviderLabelValue) - t.Run("provision john's clusterRoleBinding", func(t *testing.T) { + t.Run("provision john's inner resources of dev namespace", func(t *testing.T) { // when res, err := r.Reconcile(context.TODO(), req) // then require.NoError(t, err) - assert.Equal(t, reconcile.Result{Requeue: true}, res) + assert.Equal(t, reconcile.Result{}, res) AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). HasFinalizer(). HasSpecNamespaces("dev"). HasConditions(Provisioning()) AssertThatNamespace(t, spacename+"-dev", fakeClient). - DoesNotExist() + HasLabel(toolchainv1alpha1.SpaceLabelKey, spacename). + HasLabel(toolchainv1alpha1.TypeLabelKey, "dev"). + HasLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-dev-abcde11"). + HasLabel(toolchainv1alpha1.TierLabelKey, "advanced"). + HasLabel(toolchainv1alpha1.ProviderLabelKey, toolchainv1alpha1.ProviderLabelValue). + HasResource("crtadmin-pods", &rbacv1.RoleBinding{}) AssertThatCluster(t, fakeClient). HasResource("for-"+spacename, "av1.ClusterResourceQuota{}). - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) // created + HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) - t.Run("provision john's dev and stage Idlers", func(t *testing.T) { - // when - res, err := r.Reconcile(context.TODO(), req) - - // then + t.Run("provision cluster resources and namespace for the joe user (using cached TierTemplate)", func(t *testing.T) { + // given + joeUsername := "joe" + joeNsTmplSet := newNSTmplSet(namespaceName, joeUsername, "advanced", withNamespaces("abcde11", "dev"), withClusterResources("abcde11")) + err := fakeClient.Create(context.TODO(), joeNsTmplSet) require.NoError(t, err) - assert.Equal(t, reconcile.Result{Requeue: true}, res) - AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). - HasFinalizer(). - HasSpecNamespaces("dev"). - HasConditions(Provisioning()) - AssertThatNamespace(t, spacename+"-dev", fakeClient). - DoesNotExist() - AssertThatCluster(t, fakeClient). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}). - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}). - HasResource(spacename+"-dev", &toolchainv1alpha1.Idler{}) // created + joeReq := newReconcileRequest(namespaceName, joeUsername) // when - res, err = r.Reconcile(context.TODO(), req) + res, err := r.Reconcile(context.TODO(), joeReq) // then require.NoError(t, err) - assert.Equal(t, reconcile.Result{Requeue: true}, res) - AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). + assert.Equal(t, reconcile.Result{}, res) + AssertThatNSTemplateSet(t, namespaceName, joeUsername, fakeClient). + HasFinalizer(). + HasSpecNamespaces("dev"). HasConditions(Provisioning()) + AssertThatNamespace(t, joeUsername+"-dev", fakeClient). + HasLabel(toolchainv1alpha1.SpaceLabelKey, joeUsername). + HasLabel(toolchainv1alpha1.TypeLabelKey, "dev"). + HasNoLabel(toolchainv1alpha1.TemplateRefLabelKey). + HasNoLabel(toolchainv1alpha1.TierLabelKey). + HasLabel(toolchainv1alpha1.ProviderLabelKey, toolchainv1alpha1.ProviderLabelValue) AssertThatCluster(t, fakeClient). HasResource("for-"+spacename, "av1.ClusterResourceQuota{}). - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}). - HasResource(spacename+"-dev", &toolchainv1alpha1.Idler{}). - HasResource(spacename+"-stage", &toolchainv1alpha1.Idler{}) // created - AssertThatNamespace(t, spacename+"-dev", fakeClient).DoesNotExist() + HasResource(joeUsername+"-tekton-view", &rbacv1.ClusterRoleBinding{}). + HasResource(joeUsername+"-dev", &toolchainv1alpha1.Idler{}). + HasResource(joeUsername+"-stage", &toolchainv1alpha1.Idler{}) - t.Run("provision john's dev namespace", func(t *testing.T) { + t.Run("provision inner resources of joe's dev namespace", func(t *testing.T) { // when - res, err := r.Reconcile(context.TODO(), req) + res, err := r.Reconcile(context.TODO(), joeReq) // then require.NoError(t, err) assert.Equal(t, reconcile.Result{}, res) - AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). + AssertThatNSTemplateSet(t, namespaceName, joeUsername, fakeClient). HasFinalizer(). HasSpecNamespaces("dev"). HasConditions(Provisioning()) - AssertThatCluster(t, fakeClient). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}). - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}). - HasResource(spacename+"-dev", &toolchainv1alpha1.Idler{}). - HasResource(spacename+"-stage", &toolchainv1alpha1.Idler{}) - AssertThatNamespace(t, spacename+"-dev", fakeClient). - HasLabel(toolchainv1alpha1.SpaceLabelKey, spacename). + AssertThatNamespace(t, joeUsername+"-dev", fakeClient). + HasLabel(toolchainv1alpha1.SpaceLabelKey, joeUsername). HasLabel(toolchainv1alpha1.TypeLabelKey, "dev"). - HasNoLabel(toolchainv1alpha1.TemplateRefLabelKey). // no label until all the namespace inner resources have been created - HasNoLabel(toolchainv1alpha1.TierLabelKey). - HasLabel(toolchainv1alpha1.ProviderLabelKey, toolchainv1alpha1.ProviderLabelValue) - - t.Run("provision john's inner resources of dev namespace", func(t *testing.T) { - // when - res, err := r.Reconcile(context.TODO(), req) - - // then - require.NoError(t, err) - assert.Equal(t, reconcile.Result{}, res) - AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). - HasFinalizer(). - HasSpecNamespaces("dev"). - HasConditions(Provisioning()) - AssertThatNamespace(t, spacename+"-dev", fakeClient). - HasLabel(toolchainv1alpha1.SpaceLabelKey, spacename). - HasLabel(toolchainv1alpha1.TypeLabelKey, "dev"). - HasLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-dev-abcde11"). - HasLabel(toolchainv1alpha1.TierLabelKey, "advanced"). - HasLabel(toolchainv1alpha1.ProviderLabelKey, toolchainv1alpha1.ProviderLabelValue). - HasResource("crtadmin-pods", &rbacv1.RoleBinding{}) - AssertThatCluster(t, fakeClient). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}). - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) - - t.Run("provision ClusterResourceQuota for the joe user (using cached TierTemplate)", func(t *testing.T) { - // given - joeUsername := "joe" - joeNsTmplSet := newNSTmplSet(namespaceName, joeUsername, "advanced", withNamespaces("abcde11", "dev"), withClusterResources("abcde11")) - err := fakeClient.Create(context.TODO(), joeNsTmplSet) - require.NoError(t, err) - joeReq := newReconcileRequest(namespaceName, joeUsername) - - // when - res, err := r.Reconcile(context.TODO(), joeReq) - - // then - require.NoError(t, err) - assert.Equal(t, reconcile.Result{Requeue: true}, res) - AssertThatNSTemplateSet(t, namespaceName, joeUsername, fakeClient). - HasFinalizer(). - HasSpecNamespaces("dev"). - HasConditions(Provisioning()) - AssertThatNamespace(t, joeUsername+"-dev", fakeClient). - DoesNotExist() - AssertThatCluster(t, fakeClient). - HasResource("for-"+joeUsername, "av1.ClusterResourceQuota{}). - HasNoResource(joeUsername+"-tekton-view", &rbacv1.ClusterRoleBinding{}) - - t.Run("provision joe's clusterRoleBinding (using cached TierTemplate)", func(t *testing.T) { - // when - res, err := r.Reconcile(context.TODO(), joeReq) - - // then - require.NoError(t, err) - assert.Equal(t, reconcile.Result{Requeue: true}, res) - AssertThatNSTemplateSet(t, namespaceName, joeUsername, fakeClient). - HasFinalizer(). - HasSpecNamespaces("dev"). - HasConditions(Provisioning()) - AssertThatNamespace(t, joeUsername+"-dev", fakeClient). - DoesNotExist() - AssertThatCluster(t, fakeClient). - HasResource("for-"+joeUsername, "av1.ClusterResourceQuota{}). - HasResource(joeUsername+"-tekton-view", &rbacv1.ClusterRoleBinding{}) - - t.Run("provision joe's dev and stage Idlers", func(t *testing.T) { - // when - res, err := r.Reconcile(context.TODO(), joeReq) - - // then - require.NoError(t, err) - assert.Equal(t, reconcile.Result{Requeue: true}, res) - AssertThatNSTemplateSet(t, namespaceName, joeUsername, fakeClient). - HasFinalizer(). - HasSpecNamespaces("dev"). - HasConditions(Provisioning()) - AssertThatNamespace(t, joeUsername+"-dev", fakeClient). - DoesNotExist() - AssertThatCluster(t, fakeClient). - HasResource(joeUsername+"-tekton-view", &rbacv1.ClusterRoleBinding{}). - HasResource(joeUsername+"-dev", &toolchainv1alpha1.Idler{}) // created - - // when - res, err = r.Reconcile(context.TODO(), joeReq) - - // then - require.NoError(t, err) - assert.Equal(t, reconcile.Result{Requeue: true}, res) - AssertThatNSTemplateSet(t, namespaceName, joeUsername, fakeClient). - HasConditions(Provisioning()) - AssertThatCluster(t, fakeClient). - HasResource(joeUsername+"-tekton-view", &rbacv1.ClusterRoleBinding{}). - HasResource(joeUsername+"-dev", &toolchainv1alpha1.Idler{}). - HasResource(joeUsername+"-stage", &toolchainv1alpha1.Idler{}) // created - - t.Run("provision joe's dev namespace (using cached TierTemplate)", func(t *testing.T) { - // when - res, err := r.Reconcile(context.TODO(), joeReq) - - // then - require.NoError(t, err) - assert.Equal(t, reconcile.Result{}, res) - AssertThatNSTemplateSet(t, namespaceName, joeUsername, fakeClient). - HasFinalizer(). - HasSpecNamespaces("dev"). - HasConditions(Provisioning()) - AssertThatNamespace(t, joeUsername+"-dev", fakeClient). - HasLabel(toolchainv1alpha1.SpaceLabelKey, joeUsername). - HasLabel(toolchainv1alpha1.TypeLabelKey, "dev"). - HasNoLabel(toolchainv1alpha1.TemplateRefLabelKey). - HasNoLabel(toolchainv1alpha1.TierLabelKey). - HasLabel(toolchainv1alpha1.ProviderLabelKey, toolchainv1alpha1.ProviderLabelValue) - AssertThatCluster(t, fakeClient). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}). - HasResource(joeUsername+"-tekton-view", &rbacv1.ClusterRoleBinding{}) - - t.Run("provision inner resources of joe's dev namespace", func(t *testing.T) { - // when - res, err := r.Reconcile(context.TODO(), joeReq) - - // then - require.NoError(t, err) - assert.Equal(t, reconcile.Result{}, res) - AssertThatNSTemplateSet(t, namespaceName, joeUsername, fakeClient). - HasFinalizer(). - HasSpecNamespaces("dev"). - HasConditions(Provisioning()) - AssertThatNamespace(t, joeUsername+"-dev", fakeClient). - HasLabel(toolchainv1alpha1.SpaceLabelKey, joeUsername). - HasLabel(toolchainv1alpha1.TypeLabelKey, "dev"). - HasLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-dev-abcde11"). - HasLabel(toolchainv1alpha1.TierLabelKey, "advanced"). - HasLabel(toolchainv1alpha1.ProviderLabelKey, toolchainv1alpha1.ProviderLabelValue). - HasResource("crtadmin-pods", &rbacv1.RoleBinding{}) - AssertThatCluster(t, fakeClient). - HasResource("for-"+joeUsername, "av1.ClusterResourceQuota{}). - HasResource(joeUsername+"-tekton-view", &rbacv1.ClusterRoleBinding{}) - }) - }) - }) - }) - }) - }) + HasLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-dev-abcde11"). + HasLabel(toolchainv1alpha1.TierLabelKey, "advanced"). + HasLabel(toolchainv1alpha1.ProviderLabelKey, toolchainv1alpha1.ProviderLabelValue). + HasResource("crtadmin-pods", &rbacv1.RoleBinding{}) + AssertThatCluster(t, fakeClient). + HasResource("for-"+joeUsername, "av1.ClusterResourceQuota{}). + HasResource(joeUsername+"-tekton-view", &rbacv1.ClusterRoleBinding{}) }) }) }) @@ -865,7 +730,8 @@ func TestReconcilePromotion(t *testing.T) { t.Run("upgrade from basic to advanced tier", func(t *testing.T) { t.Run("create ClusterResourceQuota", func(t *testing.T) { // given - nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withNamespaces("abcde11", "dev"), withClusterResources("abcde11")) + previousClusterTemplate := newTierTemplate("basic", "clusterresources", "abcde11") + nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withNamespaces("abcde11", "dev"), withClusterResources("abcde11"), withStatusClusterResourcesInTier("basic", "abcde11")) // create namespace (and assume it is complete since it has the expected revision number) devNS := newNamespace("basic", spacename, "dev", withTemplateRefUsingRevision("abcde11")) stageNS := newNamespace("basic", spacename, "stage", withTemplateRefUsingRevision("abcde11")) @@ -875,7 +741,7 @@ func TestReconcilePromotion(t *testing.T) { devRb2 := newRoleBinding(devNS.Name, "crtadmin-view", spacename) stageRb := newRoleBinding(stageNS.Name, "crtadmin-pods", spacename) stageRb2 := newRoleBinding(stageNS.Name, "crtadmin-view", spacename) - r, req, fakeClient := prepareReconcile(t, namespaceName, spacename, nsTmplSet, devNS, stageNS, devRo, stageRo, devRb, devRb2, stageRb, stageRb2) + r, req, fakeClient := prepareReconcile(t, namespaceName, spacename, nsTmplSet, devNS, stageNS, devRo, stageRo, devRb, devRb2, stageRb, stageRb2, previousClusterTemplate) err := fakeClient.Update(context.TODO(), nsTmplSet) require.NoError(t, err) @@ -889,144 +755,69 @@ func TestReconcilePromotion(t *testing.T) { HasFinalizer(). HasConditions(Updating()) AssertThatCluster(t, fakeClient). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, - WithLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-clusterresources-abcde11"), - WithLabel(toolchainv1alpha1.TierLabelKey, "advanced")). // upgraded - HasNoResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) - - for _, nsType := range []string{"stage", "dev"} { - AssertThatNamespace(t, spacename+"-"+nsType, r.Client). - HasNoOwnerReference(). - HasLabel(toolchainv1alpha1.SpaceLabelKey, spacename). - HasLabel(toolchainv1alpha1.TemplateRefLabelKey, "basic-"+nsType+"-abcde11"). // not upgraded yet - HasLabel(toolchainv1alpha1.TierLabelKey, "basic"). - HasLabel(toolchainv1alpha1.TypeLabelKey, nsType). - HasLabel(toolchainv1alpha1.ProviderLabelKey, "codeready-toolchain"). - HasResource("exec-pods", &rbacv1.Role{}) - } - - t.Run("create ClusterRoleBinding", func(t *testing.T) { - // when + HasResource("for-"+spacename, "av1.ClusterResourceQuota{}). + HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}). + HasResource(spacename+"-dev", &toolchainv1alpha1.Idler{}). + HasResource(spacename+"-stage", &toolchainv1alpha1.Idler{}) + + AssertThatNamespace(t, stageNS.Name, r.Client). + DoesNotExist() // namespace was deleted + AssertThatNamespace(t, devNS.Name, r.Client). + HasNoOwnerReference(). + HasLabel(toolchainv1alpha1.SpaceLabelKey, spacename). + HasLabel(toolchainv1alpha1.TemplateRefLabelKey, "basic-dev-abcde11"). + HasLabel(toolchainv1alpha1.TypeLabelKey, "dev"). + HasLabel(toolchainv1alpha1.ProviderLabelKey, "codeready-toolchain"). + HasLabel(toolchainv1alpha1.TierLabelKey, "basic") // not upgraded yet + + t.Run("upgrade the dev namespace", func(t *testing.T) { + // when - should upgrade the namespace _, err = r.Reconcile(context.TODO(), req) // then require.NoError(t, err) + // NSTemplateSet provisioning is complete AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). HasFinalizer(). HasConditions(Updating()) AssertThatCluster(t, fakeClient). HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, WithLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-clusterresources-abcde11"), - WithLabel(toolchainv1alpha1.TierLabelKey, "advanced")). - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) - for _, nsType := range []string{"stage", "dev"} { - AssertThatNamespace(t, spacename+"-"+nsType, r.Client). - HasNoOwnerReference(). - HasLabel(toolchainv1alpha1.TemplateRefLabelKey, "basic-"+nsType+"-abcde11"). // not upgraded yet - HasLabel(toolchainv1alpha1.SpaceLabelKey, spacename). - HasLabel(toolchainv1alpha1.TierLabelKey, "basic"). // not upgraded yet - HasLabel(toolchainv1alpha1.TypeLabelKey, nsType). - HasLabel(toolchainv1alpha1.ProviderLabelKey, "codeready-toolchain"). - HasResource("exec-pods", &rbacv1.Role{}) - } + WithLabel(toolchainv1alpha1.TierLabelKey, "advanced")) + AssertThatNamespace(t, stageNS.Name, r.Client). + DoesNotExist() + AssertThatNamespace(t, spacename+"-dev", r.Client). + HasNoOwnerReference(). + HasLabel(toolchainv1alpha1.SpaceLabelKey, spacename). + HasLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-dev-abcde11"). + HasLabel(toolchainv1alpha1.TierLabelKey, "advanced"). + HasLabel(toolchainv1alpha1.TypeLabelKey, "dev"). + HasLabel(toolchainv1alpha1.ProviderLabelKey, "codeready-toolchain"). + HasResource("exec-pods", &rbacv1.Role{}). + HasResource("crtadmin-pods", &rbacv1.RoleBinding{}). + HasResource("crtadmin-view", &rbacv1.RoleBinding{}) - t.Run("create 2 Idlers", func(t *testing.T) { - // when + t.Run("when nothing to upgrade, then it should be provisioned", func(t *testing.T) { + // when - should check if everything is OK and set status to provisioned _, err = r.Reconcile(context.TODO(), req) - // then - require.NoError(t, err) - AssertThatCluster(t, fakeClient). - HasResource(spacename+"-dev", &toolchainv1alpha1.Idler{}, - WithLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-clusterresources-abcde11"), - WithLabel(toolchainv1alpha1.TierLabelKey, "advanced")) // created - // when - _, err = r.Reconcile(context.TODO(), req) // then require.NoError(t, err) + // NSTemplateSet provisioning is complete AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). HasFinalizer(). - HasConditions(Updating()) + HasConditions(Provisioned()) AssertThatCluster(t, fakeClient). - HasResource(spacename+"-dev", &toolchainv1alpha1.Idler{}). // still exists (no need to check again the labels) - HasResource(spacename+"-stage", &toolchainv1alpha1.Idler{}, - WithLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-clusterresources-abcde11"), - WithLabel(toolchainv1alpha1.TierLabelKey, "advanced")) // created - - t.Run("delete redundant namespace", func(t *testing.T) { - // when - should delete the -stage namespace - _, err := r.Reconcile(context.TODO(), req) - - // then - require.NoError(t, err) - AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). - HasFinalizer(). - HasConditions(Updating()) - AssertThatCluster(t, fakeClient). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, - WithLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-clusterresources-abcde11"), - WithLabel(toolchainv1alpha1.TierLabelKey, "advanced")) - AssertThatNamespace(t, stageNS.Name, r.Client). - DoesNotExist() // namespace was deleted - AssertThatNamespace(t, devNS.Name, r.Client). - HasNoOwnerReference(). - HasLabel(toolchainv1alpha1.SpaceLabelKey, spacename). - HasLabel(toolchainv1alpha1.TemplateRefLabelKey, "basic-dev-abcde11"). - HasLabel(toolchainv1alpha1.TypeLabelKey, "dev"). - HasLabel(toolchainv1alpha1.ProviderLabelKey, "codeready-toolchain"). - HasLabel(toolchainv1alpha1.TierLabelKey, "basic") // not upgraded yet - - t.Run("upgrade the dev namespace", func(t *testing.T) { - // when - should upgrade the namespace - _, err = r.Reconcile(context.TODO(), req) - - // then - require.NoError(t, err) - // NSTemplateSet provisioning is complete - AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). - HasFinalizer(). - HasConditions(Updating()) - AssertThatCluster(t, fakeClient). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, - WithLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-clusterresources-abcde11"), - WithLabel(toolchainv1alpha1.TierLabelKey, "advanced")) - AssertThatNamespace(t, stageNS.Name, r.Client). - DoesNotExist() - AssertThatNamespace(t, spacename+"-dev", r.Client). - HasNoOwnerReference(). - HasLabel(toolchainv1alpha1.SpaceLabelKey, spacename). - HasLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-dev-abcde11"). - HasLabel(toolchainv1alpha1.TierLabelKey, "advanced"). - HasLabel(toolchainv1alpha1.TypeLabelKey, "dev"). - HasLabel(toolchainv1alpha1.ProviderLabelKey, "codeready-toolchain"). - HasResource("exec-pods", &rbacv1.Role{}). - HasResource("crtadmin-pods", &rbacv1.RoleBinding{}). - HasResource("crtadmin-view", &rbacv1.RoleBinding{}) - - t.Run("when nothing to upgrade, then it should be provisioned", func(t *testing.T) { - // when - should check if everything is OK and set status to provisioned - _, err = r.Reconcile(context.TODO(), req) - - // then - require.NoError(t, err) - // NSTemplateSet provisioning is complete - AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). - HasFinalizer(). - HasConditions(Provisioned()) - AssertThatCluster(t, fakeClient). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, - WithLabel(toolchainv1alpha1.TierLabelKey, "advanced")) - AssertThatNamespace(t, spacename+"-dev", r.Client). - HasNoOwnerReference(). - HasLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-dev-abcde11"). - HasLabel(toolchainv1alpha1.SpaceLabelKey, spacename). - HasLabel(toolchainv1alpha1.TierLabelKey, "advanced"). // not updgraded yet - HasLabel(toolchainv1alpha1.TypeLabelKey, "dev"). - HasLabel(toolchainv1alpha1.ProviderLabelKey, "codeready-toolchain"). - HasResource("crtadmin-pods", &rbacv1.RoleBinding{}) // role has been removed - }) - }) - }) + HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, + WithLabel(toolchainv1alpha1.TierLabelKey, "advanced")) + AssertThatNamespace(t, spacename+"-dev", r.Client). + HasNoOwnerReference(). + HasLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-dev-abcde11"). + HasLabel(toolchainv1alpha1.SpaceLabelKey, spacename). + HasLabel(toolchainv1alpha1.TierLabelKey, "advanced"). // not updgraded yet + HasLabel(toolchainv1alpha1.TypeLabelKey, "dev"). + HasLabel(toolchainv1alpha1.ProviderLabelKey, "codeready-toolchain"). + HasResource("crtadmin-pods", &rbacv1.RoleBinding{}) // role has been removed }) }) }) @@ -1048,6 +839,7 @@ func TestReconcileUpdate(t *testing.T) { nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withNamespaces("abcde12", "dev"), withClusterResources("abcde12"), + withStatusClusterResources("abcde11"), withSpaceRoles(map[string][]string{ "advanced-admin-abcde12": {spacename}, "advanced-viewer-abcde12": {"zorro", spacename, "viewer"}, @@ -1085,7 +877,7 @@ func TestReconcileUpdate(t *testing.T) { require.NoError(t, err) AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). HasFinalizer(). - HasStatusClusterResourcesRevisionsValue(&toolchainv1alpha1.NSTemplateSetClusterResources{TemplateRef: "advanced-clusterresources-abcde11"}). // cluster resource still isn't fully updated + HasStatusClusterResourcesRevisionsValue(&toolchainv1alpha1.NSTemplateSetClusterResources{TemplateRef: "advanced-clusterresources-abcde12"}). // cluster resource fully updated HasStatusNamespaceRevisionsValue([]toolchainv1alpha1.NSTemplateSetNamespace{ { TemplateRef: "advanced-dev-abcde11", // still not updated, has the old value @@ -1102,133 +894,130 @@ func TestReconcileUpdate(t *testing.T) { }). HasConditions(Updating()) AssertThatCluster(t, fakeClient). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, - WithLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-clusterresources-abcde12"), - WithLabel(toolchainv1alpha1.TierLabelKey, "advanced")). // upgraded - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}, - WithLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-clusterresources-abcde11"), - WithLabel(toolchainv1alpha1.TierLabelKey, "advanced")) - - for _, nsType := range []string{"stage", "dev"} { - AssertThatNamespace(t, spacename+"-"+nsType, r.Client). - HasNoOwnerReference(). - HasLabel(toolchainv1alpha1.SpaceLabelKey, spacename). - HasLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-"+nsType+"-abcde11"). // not upgraded yet - HasLabel(toolchainv1alpha1.TierLabelKey, "advanced"). - HasLabel(toolchainv1alpha1.TypeLabelKey, nsType). - HasLabel(toolchainv1alpha1.ProviderLabelKey, "codeready-toolchain"). - HasResource("crtadmin-pods", &rbacv1.RoleBinding{}). - HasResource("exec-pods", &rbacv1.Role{}). - HasResource("crtadmin-view", &rbacv1.RoleBinding{}) - } + HasResource("for-"+spacename, "av1.ClusterResourceQuota{}). + HasNoResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) - t.Run("delete ClusterRoleBinding", func(t *testing.T) { - // when + AssertThatNamespace(t, stageNS.Name, r.Client). + DoesNotExist() // namespace was deleted + AssertThatNamespace(t, devNS.Name, r.Client). + HasNoOwnerReference(). + HasLabel(toolchainv1alpha1.SpaceLabelKey, spacename). + HasLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-dev-abcde11"). // not upgraded yet + HasLabel(toolchainv1alpha1.TypeLabelKey, "dev"). + HasLabel(toolchainv1alpha1.ProviderLabelKey, "codeready-toolchain"). + HasLabel(toolchainv1alpha1.TierLabelKey, "advanced"). + HasResource("crtadmin-pods", &rbacv1.RoleBinding{}). + HasResource("exec-pods", &rbacv1.Role{}). + HasResource("crtadmin-view", &rbacv1.RoleBinding{}) + + t.Run("upgrade the dev namespace", func(t *testing.T) { + // when - should upgrade the namespace _, err = r.Reconcile(context.TODO(), req) // then require.NoError(t, err) + // NSTemplateSet is still updating AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). HasFinalizer(). - HasStatusClusterResourcesRevisionsValue(&toolchainv1alpha1.NSTemplateSetClusterResources{TemplateRef: "advanced-clusterresources-abcde11"}). // cluster resource isn't still fully updated + HasStatusClusterResourcesRevisionsValue(&toolchainv1alpha1.NSTemplateSetClusterResources{TemplateRef: "advanced-clusterresources-abcde12"}). // cluster resource remains unchanged + // namespaces were not updated yet HasStatusNamespaceRevisionsValue([]toolchainv1alpha1.NSTemplateSetNamespace{ { - TemplateRef: "advanced-dev-abcde11", // still not updated, has the old value - }, - { - TemplateRef: "advanced-stage-abcde11", // still not updated, has the old value + TemplateRef: "advanced-dev-abcde11", }, - }). - HasStatusSpaceRolesRevisionsValue([]toolchainv1alpha1.NSTemplateSetSpaceRole{ { - TemplateRef: "advanced-admin-abcde11", // still not updated, has the old value - Usernames: []string{spacename}, + TemplateRef: "advanced-stage-abcde11", }, }). + HasStatusSpaceRolesRevisionsValue([]toolchainv1alpha1.NSTemplateSetSpaceRole{{ + TemplateRef: "advanced-admin-abcde11", + Usernames: []string{spacename}, // still not updated, has the old value + }}). HasConditions(Updating()) AssertThatCluster(t, fakeClient). HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, - WithLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-clusterresources-abcde12"), // the templateref label is being updated on all objects - WithLabel(toolchainv1alpha1.TierLabelKey, "advanced")). - HasNoResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) // deleted - for _, nsType := range []string{"stage", "dev"} { - AssertThatNamespace(t, spacename+"-"+nsType, r.Client). - HasNoOwnerReference(). - HasLabel(toolchainv1alpha1.SpaceLabelKey, spacename). - HasLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-"+nsType+"-abcde11"). // not upgraded yet - HasLabel(toolchainv1alpha1.TierLabelKey, "advanced"). - HasLabel(toolchainv1alpha1.TypeLabelKey, nsType). - HasLabel(toolchainv1alpha1.ProviderLabelKey, "codeready-toolchain"). - HasResource("crtadmin-pods", &rbacv1.RoleBinding{}). - HasResource("exec-pods", &rbacv1.Role{}). - HasResource("crtadmin-view", &rbacv1.RoleBinding{}) - } + WithLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-clusterresources-abcde12"), + WithLabel(toolchainv1alpha1.TierLabelKey, "advanced")). + HasNoResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) + AssertThatNamespace(t, stageNS.Name, r.Client). + DoesNotExist() + AssertThatNamespace(t, devNS.Name, r.Client). + HasNoOwnerReference(). + HasLabel(toolchainv1alpha1.SpaceLabelKey, spacename). + HasLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-dev-abcde12"). // upgraded + HasLabel(toolchainv1alpha1.TypeLabelKey, "dev"). + HasLabel(toolchainv1alpha1.ProviderLabelKey, "codeready-toolchain"). + HasLabel(toolchainv1alpha1.TierLabelKey, "advanced"). + HasResource("crtadmin-pods", &rbacv1.RoleBinding{}). + HasResource("exec-pods", &rbacv1.Role{}). + HasNoResource("crtadmin-view", &rbacv1.RoleBinding{}) - t.Run("delete redundant namespace", func(t *testing.T) { - // when - should delete the -stage namespace - _, err := r.Reconcile(context.TODO(), req) + t.Run("create missing space roles", func(t *testing.T) { + // when - should upgrade the space roles by creating the missing role(binding)s + _, err = r.Reconcile(context.TODO(), req) // then require.NoError(t, err) AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). HasFinalizer(). - HasStatusClusterResourcesRevisionsValue(&toolchainv1alpha1.NSTemplateSetClusterResources{TemplateRef: "advanced-clusterresources-abcde12"}). // cluster resource is updated now - // namespaces were not updated yet - HasStatusNamespaceRevisionsValue([]toolchainv1alpha1.NSTemplateSetNamespace{ - { - TemplateRef: "advanced-dev-abcde11", - }, - { - TemplateRef: "advanced-stage-abcde11", - }, - }). + HasStatusClusterResourcesRevisionsValue(&toolchainv1alpha1.NSTemplateSetClusterResources{TemplateRef: "advanced-clusterresources-abcde12"}). // cluster resource remains unchanged + HasStatusNamespaceRevisionsValue([]toolchainv1alpha1.NSTemplateSetNamespace{{ + TemplateRef: "advanced-dev-abcde12", // namespaces are updated now + }}). HasStatusSpaceRolesRevisionsValue([]toolchainv1alpha1.NSTemplateSetSpaceRole{{ TemplateRef: "advanced-admin-abcde11", - Usernames: []string{spacename}, // still not updated, has the old value + Usernames: []string{spacename}, // space roles were not updated yet }}). HasConditions(Updating()) + AssertThatCluster(t, fakeClient). HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, WithLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-clusterresources-abcde12"), WithLabel(toolchainv1alpha1.TierLabelKey, "advanced")). HasNoResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) + AssertThatNamespace(t, stageNS.Name, r.Client). - DoesNotExist() // namespace was deleted + DoesNotExist() AssertThatNamespace(t, devNS.Name, r.Client). HasNoOwnerReference(). HasLabel(toolchainv1alpha1.SpaceLabelKey, spacename). - HasLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-dev-abcde11"). // not upgraded yet + HasLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-dev-abcde12"). HasLabel(toolchainv1alpha1.TypeLabelKey, "dev"). HasLabel(toolchainv1alpha1.ProviderLabelKey, "codeready-toolchain"). HasLabel(toolchainv1alpha1.TierLabelKey, "advanced"). HasResource("crtadmin-pods", &rbacv1.RoleBinding{}). HasResource("exec-pods", &rbacv1.Role{}). - HasResource("crtadmin-view", &rbacv1.RoleBinding{}) - - t.Run("upgrade the dev namespace", func(t *testing.T) { - // when - should upgrade the namespace + HasNoResource("crtadmin-view", &rbacv1.RoleBinding{}). + HasResource("space-admin", &rbacv1.Role{}). + HasResource(spacename+"-space-admin", &rbacv1.RoleBinding{}). + HasResource("space-viewer", &rbacv1.Role{}). + HasResource(spacename+"-space-viewer", &rbacv1.RoleBinding{}) + + t.Run("when nothing to update, then it should be provisioned", func(t *testing.T) { + // when - should check if everything is OK and set status to provisioned _, err = r.Reconcile(context.TODO(), req) // then require.NoError(t, err) - // NSTemplateSet is still updating + // NSTemplateSet provisioning is complete AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). HasFinalizer(). - HasStatusClusterResourcesRevisionsValue(&toolchainv1alpha1.NSTemplateSetClusterResources{TemplateRef: "advanced-clusterresources-abcde12"}). // cluster resource remains unchanged - // namespaces were not updated yet - HasStatusNamespaceRevisionsValue([]toolchainv1alpha1.NSTemplateSetNamespace{ + HasStatusClusterResourcesRevisionsValue(&toolchainv1alpha1.NSTemplateSetClusterResources{TemplateRef: "advanced-clusterresources-abcde12"}). // cluster resources are unchanged + HasStatusNamespaceRevisionsValue([]toolchainv1alpha1.NSTemplateSetNamespace{{ + TemplateRef: "advanced-dev-abcde12", // namespaces are unchanged + }}). + // space roles are updated now + HasStatusSpaceRolesRevisionsValue([]toolchainv1alpha1.NSTemplateSetSpaceRole{ { - TemplateRef: "advanced-dev-abcde11", + TemplateRef: "advanced-admin-abcde12", + Usernames: []string{spacename}, }, { - TemplateRef: "advanced-stage-abcde11", + TemplateRef: "advanced-viewer-abcde12", + Usernames: []string{spacename, "viewer", "zorro"}, }, }). - HasStatusSpaceRolesRevisionsValue([]toolchainv1alpha1.NSTemplateSetSpaceRole{{ - TemplateRef: "advanced-admin-abcde11", - Usernames: []string{spacename}, // still not updated, has the old value - }}). - HasConditions(Updating()) + HasConditions(Provisioned()) AssertThatCluster(t, fakeClient). HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, WithLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-clusterresources-abcde12"), @@ -1245,97 +1034,11 @@ func TestReconcileUpdate(t *testing.T) { HasLabel(toolchainv1alpha1.TierLabelKey, "advanced"). HasResource("crtadmin-pods", &rbacv1.RoleBinding{}). HasResource("exec-pods", &rbacv1.Role{}). - HasNoResource("crtadmin-view", &rbacv1.RoleBinding{}) - - t.Run("create missing space roles", func(t *testing.T) { - // when - should upgrade the space roles by creating the missing role(binding)s - _, err = r.Reconcile(context.TODO(), req) - - // then - require.NoError(t, err) - AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). - HasFinalizer(). - HasStatusClusterResourcesRevisionsValue(&toolchainv1alpha1.NSTemplateSetClusterResources{TemplateRef: "advanced-clusterresources-abcde12"}). // cluster resource remains unchanged - HasStatusNamespaceRevisionsValue([]toolchainv1alpha1.NSTemplateSetNamespace{{ - TemplateRef: "advanced-dev-abcde12", // namespaces are updated now - }}). - HasStatusSpaceRolesRevisionsValue([]toolchainv1alpha1.NSTemplateSetSpaceRole{{ - TemplateRef: "advanced-admin-abcde11", - Usernames: []string{spacename}, // space roles were not updated yet - }}). - HasConditions(Updating()) - - AssertThatCluster(t, fakeClient). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, - WithLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-clusterresources-abcde12"), - WithLabel(toolchainv1alpha1.TierLabelKey, "advanced")). - HasNoResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) - - AssertThatNamespace(t, stageNS.Name, r.Client). - DoesNotExist() - AssertThatNamespace(t, devNS.Name, r.Client). - HasNoOwnerReference(). - HasLabel(toolchainv1alpha1.SpaceLabelKey, spacename). - HasLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-dev-abcde12"). - HasLabel(toolchainv1alpha1.TypeLabelKey, "dev"). - HasLabel(toolchainv1alpha1.ProviderLabelKey, "codeready-toolchain"). - HasLabel(toolchainv1alpha1.TierLabelKey, "advanced"). - HasResource("crtadmin-pods", &rbacv1.RoleBinding{}). - HasResource("exec-pods", &rbacv1.Role{}). - HasNoResource("crtadmin-view", &rbacv1.RoleBinding{}). - HasResource("space-admin", &rbacv1.Role{}). - HasResource(spacename+"-space-admin", &rbacv1.RoleBinding{}). - HasResource("space-viewer", &rbacv1.Role{}). - HasResource(spacename+"-space-viewer", &rbacv1.RoleBinding{}) - - t.Run("when nothing to update, then it should be provisioned", func(t *testing.T) { - // when - should check if everything is OK and set status to provisioned - _, err = r.Reconcile(context.TODO(), req) - - // then - require.NoError(t, err) - // NSTemplateSet provisioning is complete - AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). - HasFinalizer(). - HasStatusClusterResourcesRevisionsValue(&toolchainv1alpha1.NSTemplateSetClusterResources{TemplateRef: "advanced-clusterresources-abcde12"}). // cluster resources are unchanged - HasStatusNamespaceRevisionsValue([]toolchainv1alpha1.NSTemplateSetNamespace{{ - TemplateRef: "advanced-dev-abcde12", // namespaces are unchanged - }}). - // space roles are updated now - HasStatusSpaceRolesRevisionsValue([]toolchainv1alpha1.NSTemplateSetSpaceRole{ - { - TemplateRef: "advanced-admin-abcde12", - Usernames: []string{spacename}, - }, - { - TemplateRef: "advanced-viewer-abcde12", - Usernames: []string{spacename, "viewer", "zorro"}, - }, - }). - HasConditions(Provisioned()) - AssertThatCluster(t, fakeClient). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, - WithLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-clusterresources-abcde12"), - WithLabel(toolchainv1alpha1.TierLabelKey, "advanced")). - HasNoResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) - AssertThatNamespace(t, stageNS.Name, r.Client). - DoesNotExist() - AssertThatNamespace(t, devNS.Name, r.Client). - HasNoOwnerReference(). - HasLabel(toolchainv1alpha1.SpaceLabelKey, spacename). - HasLabel(toolchainv1alpha1.TemplateRefLabelKey, "advanced-dev-abcde12"). // upgraded - HasLabel(toolchainv1alpha1.TypeLabelKey, "dev"). - HasLabel(toolchainv1alpha1.ProviderLabelKey, "codeready-toolchain"). - HasLabel(toolchainv1alpha1.TierLabelKey, "advanced"). - HasResource("crtadmin-pods", &rbacv1.RoleBinding{}). - HasResource("exec-pods", &rbacv1.Role{}). - HasNoResource("crtadmin-view", &rbacv1.RoleBinding{}). - HasResource("space-admin", &rbacv1.Role{}). - HasResource(spacename+"-space-admin", &rbacv1.RoleBinding{}). - HasResource("space-viewer", &rbacv1.Role{}). - HasResource(spacename+"-space-viewer", &rbacv1.RoleBinding{}) - }) - }) + HasNoResource("crtadmin-view", &rbacv1.RoleBinding{}). + HasResource("space-admin", &rbacv1.Role{}). + HasResource(spacename+"-space-admin", &rbacv1.RoleBinding{}). + HasResource("space-viewer", &rbacv1.Role{}). + HasResource(spacename+"-space-viewer", &rbacv1.RoleBinding{}) }) }) }) @@ -1433,12 +1136,15 @@ func TestReconcileProvisionFail(t *testing.T) { } func TestDeleteNSTemplateSet(t *testing.T) { + restore := test.SetEnvVarAndRestore(t, commonconfig.WatchNamespaceEnvVar, "my-member-operator-namespace") + t.Cleanup(restore) + spacename := "johnsmith" namespaceName := "toolchain-member" t.Run("with cluster resources and 2 user namespaces to delete", func(t *testing.T) { // given an NSTemplateSet resource and 2 active user namespaces ("dev" and "stage") - nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withNamespaces("abcde11", "dev", "stage"), withDeletionTs(), withClusterResources("abcde11")) + nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withNamespaces("abcde11", "dev", "stage"), withDeletionTs(), withClusterResources("abcde11"), withStatusClusterResources("abcde11")) crq := newClusterResourceQuota(spacename, "advanced") devNS := newNamespace("advanced", spacename, "dev", withTemplateRefUsingRevision("abcde11")) stageNS := newNamespace("advanced", spacename, "stage", withTemplateRefUsingRevision("abcde11")) @@ -1473,36 +1179,22 @@ func TestDeleteNSTemplateSet(t *testing.T) { HasFinalizer(). // the finalizer should not have been removed either HasConditions(Terminating()) - t.Run("reconcile after second user namespace deletion triggers deletion of CRQ", func(t *testing.T) { + t.Run("reconcile after second user namespace deletion triggers deletion of cluster resources and removal of finalizer", func(t *testing.T) { // when _, err := r.Reconcile(context.TODO(), req) // then require.NoError(t, err) AssertThatNSTemplateSet(t, namespaceName, spacename, r.Client). - HasFinalizer(). // the finalizer should NOT have been removed yet - HasConditions(Terminating()) - AssertThatCluster(t, r.Client). - HasNoResource("for-"+spacename, "av1.ClusterResourceQuota{}) // resource was deleted - - t.Run("reconcile after cluster resource quota deletion triggers removal of the finalizer and thus successful deletion", func(t *testing.T) { - // when a last reconcile loop is triggered (when the NSTemplateSet resource is marked for deletion and there's a finalizer) - _, err := r.Reconcile(context.TODO(), req) - - // then - require.NoError(t, err) - // get the NSTemplateSet resource again and check its finalizers and status - AssertThatNSTemplateSet(t, namespaceName, spacename, r.Client). - DoesNotExist() - AssertThatCluster(t, r.Client).HasNoResource("for-"+spacename, "av1.ClusterResourceQuota{}) - }) + DoesNotExist() + AssertThatCluster(t, r.Client).HasNoResource("for-"+spacename, "av1.ClusterResourceQuota{}) }) }) }) }) t.Run("failed to delete cluster resources", func(t *testing.T) { - nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withDeletionTs(), withClusterResources("abcde11")) + nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withDeletionTs(), withClusterResources("abcde11"), withStatusClusterResources("abcde11")) crq := newClusterResourceQuota(spacename, "advanced") r, fakeClient := prepareController(t, nsTmplSet, crq) req := newReconcileRequest(namespaceName, spacename) @@ -1510,7 +1202,7 @@ func TestDeleteNSTemplateSet(t *testing.T) { // only add deletion timestamp, but not delete fakeClient.MockDelete = func(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error { t.Logf("deleting resource of kind %T '%s'", obj, obj.GetName()) - if _, ok := obj.(*quotav1.ClusterResourceQuota); ok { + if obj.GetObjectKind().GroupVersionKind().Kind == "ClusterResourceQuota" { return fmt.Errorf("mock error") } return fakeClient.Client.Delete(ctx, obj, opts...) @@ -1520,7 +1212,7 @@ func TestDeleteNSTemplateSet(t *testing.T) { result, err := r.Reconcile(context.TODO(), req) // then require.EqualError(t, err, "failed to delete cluster resource 'for-johnsmith': mock error") - require.Equal(t, controllerruntime.Result{Requeue: true}, result) + require.Equal(t, controllerruntime.Result{}, result) }) t.Run("NSTemplateSet deletion errors when namespace is not deleted in 1 min", func(t *testing.T) { @@ -1750,22 +1442,6 @@ func withClusterResources(revision string) nsTmplSetOption { } } -func withPreviouslyAppliedClusterResources(revision string) nsTmplSetOption { - return func(nsTmplSet *toolchainv1alpha1.NSTemplateSet) { - nsTmplSet.Status.ClusterResources = &toolchainv1alpha1.NSTemplateSetClusterResources{ - TemplateRef: NewTierTemplateName(nsTmplSet.Spec.TierName, "clusterresources", revision), - } - } -} - -func withPreviouslyAppliedClusterResourcesInTier(tierName, revision string) nsTmplSetOption { - return func(nsTmplSet *toolchainv1alpha1.NSTemplateSet) { - nsTmplSet.Status.ClusterResources = &toolchainv1alpha1.NSTemplateSetClusterResources{ - TemplateRef: NewTierTemplateName(tierName, "clusterresources", revision), - } - } -} - func withSpaceRoles(roles map[string][]string) nsTmplSetOption { return func(nsTmplSet *toolchainv1alpha1.NSTemplateSet) { nsTmplSet.Spec.SpaceRoles = make([]toolchainv1alpha1.NSTemplateSetSpaceRole, 0, len(roles)) @@ -1798,6 +1474,14 @@ func withStatusClusterResources(revision string) nsTmplSetOption { } } +func withStatusClusterResourcesInTier(tierName, revision string) nsTmplSetOption { + return func(nsTmplSet *toolchainv1alpha1.NSTemplateSet) { + nsTmplSet.Status.ClusterResources = &toolchainv1alpha1.NSTemplateSetClusterResources{ + TemplateRef: NewTierTemplateName(tierName, "clusterresources", revision), + } + } +} + func withStatusSpaceRoles(roles map[string][]string) nsTmplSetOption { return func(nsTmplSet *toolchainv1alpha1.NSTemplateSet) { nsTmplSet.Status.SpaceRoles = make([]toolchainv1alpha1.NSTemplateSetSpaceRole, 0, len(roles)) From a2265172616db9762289c625e0b016d0b2757a27 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Mon, 22 Sep 2025 16:56:13 +0200 Subject: [PATCH 03/16] use an already defined function for the object comparison --- controllers/nstemplateset/cluster_resources.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/controllers/nstemplateset/cluster_resources.go b/controllers/nstemplateset/cluster_resources.go index f6158ce02..f6ad51c4b 100644 --- a/controllers/nstemplateset/cluster_resources.go +++ b/controllers/nstemplateset/cluster_resources.go @@ -7,6 +7,7 @@ import ( toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" + commonclient "github.com/codeready-toolchain/toolchain-common/pkg/client" errs "github.com/pkg/errors" "github.com/redhat-cop/operator-utils/pkg/util" "k8s.io/apimachinery/pkg/api/errors" @@ -112,7 +113,7 @@ func (r *clusterResourcesManager) ensure(ctx context.Context, nsTmplSet *toolcha // by removing the new objects from the current objects, we are going to be left with the objects // that should be removed from the cluster after we're done with this loop for oldIdx, curObj := range currentObjects { - if curObj.GetName() == newObj.GetName() && curObj.GetNamespace() == newObj.GetNamespace() && curObj.GetObjectKind().GroupVersionKind() == newObj.GetObjectKind().GroupVersionKind() { + if commonclient.SameGVKandName(curObj, newObj) { currentObjects = slices.Delete(currentObjects, oldIdx, oldIdx+1) break } From 0a61aff8678a0162357f9a875ac955bf757c8b35 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Tue, 23 Sep 2025 15:11:15 +0200 Subject: [PATCH 04/16] simplify using already existing function --- controllers/nstemplateset/cluster_resources.go | 12 +++++------- controllers/nstemplateset/cluster_resources_test.go | 2 +- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/controllers/nstemplateset/cluster_resources.go b/controllers/nstemplateset/cluster_resources.go index f6ad51c4b..439d29a52 100644 --- a/controllers/nstemplateset/cluster_resources.go +++ b/controllers/nstemplateset/cluster_resources.go @@ -132,16 +132,14 @@ func (r *clusterResourcesManager) ensure(ctx context.Context, nsTmplSet *toolcha // what we're left with here is the list of currently existing objects that are no longer present in the template. // we need to delete them - for _, obj := range currentObjects { + if len(currentObjects) > 0 { if err := changeStatusIfNeeded(); err != nil { return err } - if err := r.Client.Delete(ctx, obj); err != nil { - if !errors.IsNotFound(err) { - err := fmt.Errorf("failed to delete the cluster resource %s, %s: %w", obj.GetName(), obj.GetObjectKind().GroupVersionKind().String(), err) - return r.wrapErrorWithStatusUpdate(ctx, nsTmplSet, failureStatusReason, err, "failure while syncing cluster resources") - } - } + } + + if err := deleteObsoleteObjects(ctx, r.Client, currentObjects, nil); err != nil { + return r.wrapErrorWithStatusUpdate(ctx, nsTmplSet, failureStatusReason, err, "failure while syncing cluster resources") } return nil diff --git a/controllers/nstemplateset/cluster_resources_test.go b/controllers/nstemplateset/cluster_resources_test.go index 19e4b7da0..882b6f8cf 100644 --- a/controllers/nstemplateset/cluster_resources_test.go +++ b/controllers/nstemplateset/cluster_resources_test.go @@ -616,7 +616,7 @@ func TestPromoteClusterResources(t *testing.T) { AssertThatNSTemplateSet(t, namespaceName, spaceName, cl). HasFinalizer(). HasConditions(UpdateFailed( - "failed to delete the cluster resource for-johnsmith, quota.openshift.io/v1, Kind=ClusterResourceQuota: some error")) + "failed to delete obsolete object 'for-johnsmith' of kind 'ClusterResourceQuota' in namespace '': some error")) AssertThatCluster(t, cl). HasResource("for-"+spaceName, "av1.ClusterResourceQuota{}). HasResource(spaceName+"-tekton-view", &rbacv1.ClusterRoleBinding{}) From c0e59ea2db8e49514c01ab7dec86f4e0c86085e3 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Wed, 24 Sep 2025 12:32:51 +0200 Subject: [PATCH 05/16] test what was actually intended --- controllers/nstemplateset/cluster_resources_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/nstemplateset/cluster_resources_test.go b/controllers/nstemplateset/cluster_resources_test.go index 882b6f8cf..976ee5a4d 100644 --- a/controllers/nstemplateset/cluster_resources_test.go +++ b/controllers/nstemplateset/cluster_resources_test.go @@ -367,7 +367,7 @@ func TestPromoteClusterResources(t *testing.T) { HasConditions(Updating()) AssertThatCluster(t, cl). HasResource("for-"+spaceName, "av1.ClusterResourceQuota{}). - HasNoResource("for-empty", &rbacv1.ClusterRoleBinding{}) + HasNoResource("for-empty", "av1.ClusterResourceQuota{}) }) t.Run("promote from 1 tier to another", func(t *testing.T) { From d8a5c6016255dc48979f065a50e9053a4d5473f6 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Wed, 24 Sep 2025 12:38:34 +0200 Subject: [PATCH 06/16] fix the error handling in the pre-existing code that got refactored a little --- controllers/nstemplateset/cluster_resources.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/controllers/nstemplateset/cluster_resources.go b/controllers/nstemplateset/cluster_resources.go index 439d29a52..7c8aafff6 100644 --- a/controllers/nstemplateset/cluster_resources.go +++ b/controllers/nstemplateset/cluster_resources.go @@ -186,7 +186,8 @@ func (r *clusterResourcesManager) delete(ctx context.Context, nsTmplSet *toolcha } for _, toDelete := range currentObjects { - if err := r.Client.Get(ctx, types.NamespacedName{Name: toDelete.GetName()}, toDelete); err != nil && !errors.IsNotFound(err) { + var err error + if err = r.Client.Get(ctx, types.NamespacedName{Name: toDelete.GetName()}, toDelete); err != nil && !errors.IsNotFound(err) { return r.wrapErrorWithStatusUpdate(ctx, nsTmplSet, r.setStatusTerminatingFailed, err, "failed to get the cluster resource '%s' (GVK '%s') while deleting cluster resources", toDelete.GetName(), toDelete.GetObjectKind().GroupVersionKind()) } @@ -196,7 +197,7 @@ func (r *clusterResourcesManager) delete(ctx context.Context, nsTmplSet *toolcha } log.FromContext(ctx).Info("deleting cluster resource", "name", toDelete.GetName(), "kind", toDelete.GetObjectKind().GroupVersionKind().Kind) - if err := r.Client.Delete(ctx, toDelete); err != nil && errors.IsNotFound(err) { + if err = r.Client.Delete(ctx, toDelete); err != nil && errors.IsNotFound(err) { // ignore case where the resource did not exist anymore, move to the next one to delete continue } else if err != nil { From 7773616288dbf74cac98343289fbf3d1850d8968 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Wed, 24 Sep 2025 13:01:24 +0200 Subject: [PATCH 07/16] use a more standard way of getting object key. this would cause a bug if this method got repurposed for namespaced-objects also --- controllers/nstemplateset/cluster_resources.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/controllers/nstemplateset/cluster_resources.go b/controllers/nstemplateset/cluster_resources.go index 7c8aafff6..f0b76c0f0 100644 --- a/controllers/nstemplateset/cluster_resources.go +++ b/controllers/nstemplateset/cluster_resources.go @@ -11,7 +11,6 @@ import ( errs "github.com/pkg/errors" "github.com/redhat-cop/operator-utils/pkg/util" "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/types" runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" ) @@ -187,7 +186,7 @@ func (r *clusterResourcesManager) delete(ctx context.Context, nsTmplSet *toolcha for _, toDelete := range currentObjects { var err error - if err = r.Client.Get(ctx, types.NamespacedName{Name: toDelete.GetName()}, toDelete); err != nil && !errors.IsNotFound(err) { + if err = r.Client.Get(ctx, runtimeclient.ObjectKeyFromObject(toDelete), toDelete); err != nil && !errors.IsNotFound(err) { return r.wrapErrorWithStatusUpdate(ctx, nsTmplSet, r.setStatusTerminatingFailed, err, "failed to get the cluster resource '%s' (GVK '%s') while deleting cluster resources", toDelete.GetName(), toDelete.GetObjectKind().GroupVersionKind()) } From d006d5fac37196f9a58b222f9511df59d7d17280 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Thu, 25 Sep 2025 02:21:52 +0200 Subject: [PATCH 08/16] fix handling of the feature toggles and add test for the correct behavior --- .../nstemplateset/cluster_resources.go | 2 +- .../nstemplateset/cluster_resources_test.go | 31 ++++++++++- controllers/nstemplateset/features.go | 6 +++ controllers/nstemplateset/features_test.go | 53 +++++++++++++++++++ .../nstemplateset_controller_test.go | 6 +++ controllers/nstemplateset/status.go | 37 +++++++++---- 6 files changed, 123 insertions(+), 12 deletions(-) diff --git a/controllers/nstemplateset/cluster_resources.go b/controllers/nstemplateset/cluster_resources.go index f0b76c0f0..3509a2431 100644 --- a/controllers/nstemplateset/cluster_resources.go +++ b/controllers/nstemplateset/cluster_resources.go @@ -42,7 +42,7 @@ func (r *clusterResourcesManager) ensure(ctx context.Context, nsTmplSet *toolcha oldTemplateRef = nsTmplSet.Status.ClusterResources.TemplateRef } - if oldTemplateRef == newTemplateRef { + if oldTemplateRef == newTemplateRef && !featuresChanged(nsTmplSet) { return nil } diff --git a/controllers/nstemplateset/cluster_resources_test.go b/controllers/nstemplateset/cluster_resources_test.go index 976ee5a4d..d53c53723 100644 --- a/controllers/nstemplateset/cluster_resources_test.go +++ b/controllers/nstemplateset/cluster_resources_test.go @@ -55,7 +55,7 @@ func TestEnsureClusterResourcesOK(t *testing.T) { HasResource(spacename+"-stage", &toolchainv1alpha1.Idler{}) }) - t.Run("should create only CRQs and set status to provisioning", func(t *testing.T) { + t.Run("should create objects and set status to provisioning", func(t *testing.T) { tests := []struct { name string enabledFeatures string @@ -172,6 +172,35 @@ func TestEnsureClusterResourcesOK(t *testing.T) { HasResource(spacename+"-dev", &toolchainv1alpha1.Idler{}). HasResource(spacename+"-stage", &toolchainv1alpha1.Idler{}) }) + + t.Run("should clean up resources from no longer active features", func(t *testing.T) { + nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", + withNSTemplateSetFeatureAnnotation("feature-2"), + withStatusFeatureToggles([]string{"feature-1"}), + withNamespaces("abcde11", "dev"), + withClusterResources("abcde11"), + withStatusClusterResources("abcde11"), + withConditions(Provisioned())) + crq := newClusterResourceQuota("feature-1-for-"+spacename, "advanced") + crb := newTektonClusterRoleBinding(fmt.Sprintf("feature-1-for-%s", spacename), "advanced") + idlerDev := newIdler(spacename, spacename+"-dev", "advanced") + idlerStage := newIdler(spacename, spacename+"-stage", "advanced") + manager, fakeClient := prepareClusterResourcesManager(t, nsTmplSet, crq, crb, idlerDev, idlerStage) + + // when + err := manager.ensure(ctx, nsTmplSet) + + // then + require.NoError(t, err) + AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). + HasFinalizer(). + HasConditions(Updating()) + AssertThatCluster(t, fakeClient). + HasNoResource("feature-1-for-"+spacename, "av1.ClusterResourceQuota{}). + HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}). + HasResource(spacename+"-dev", &toolchainv1alpha1.Idler{}). + HasResource(spacename+"-stage", &toolchainv1alpha1.Idler{}) + }) } func TestEnsureClusterResourcesFail(t *testing.T) { diff --git a/controllers/nstemplateset/features.go b/controllers/nstemplateset/features.go index 20179bbc7..ee0155100 100644 --- a/controllers/nstemplateset/features.go +++ b/controllers/nstemplateset/features.go @@ -24,3 +24,9 @@ func shouldCreate(toCreate runtimeclient.Object, nsTmplSet *toolchainv1alpha1.NS } return slices.Contains(utils.SplitCommaSeparatedList(winners), feature) } + +// featuresChanged returns true if the features on the NSTemplateSet changed since the last time it was applied. +func featuresChanged(nsTmplSet *toolchainv1alpha1.NSTemplateSet) bool { + changed, _ := featureAnnotationNeedsUpdate(nsTmplSet) + return changed +} diff --git a/controllers/nstemplateset/features_test.go b/controllers/nstemplateset/features_test.go index 61a7ac9a1..e2424e944 100644 --- a/controllers/nstemplateset/features_test.go +++ b/controllers/nstemplateset/features_test.go @@ -98,6 +98,59 @@ func TestShouldCreate(t *testing.T) { } } +func TestFeaturesChanged(t *testing.T) { + for _, test := range []struct { + name string + annoFeatures string + statusFeatures []string + changed bool + }{ + { + name: "should report no change when no features in either annos or status", + changed: false, + }, + { + name: "should report change when no features in status", + annoFeatures: "feature", + changed: true, + }, + { + name: "should report change when no features in anno", + statusFeatures: []string{"feature"}, + changed: true, + }, + { + name: "should report no change when features equal", + statusFeatures: []string{"feature1", "feature2"}, + annoFeatures: "feature1,feature2", + changed: false, + }, + { + name: "should report change when features differ in number", + statusFeatures: []string{"feature1", "feature2"}, + annoFeatures: "feature1", + changed: true, + }, + { + name: "should report change when features differ", + statusFeatures: []string{"feature1", "feature2"}, + annoFeatures: "feature1,feature3", + changed: true, + }, + } { + t.Run(test.name, func(t *testing.T) { + // given + nsts := newNSTmplSet("default", "ns", "base", withNSTemplateSetFeatureAnnotation(test.annoFeatures), withStatusFeatureToggles(test.statusFeatures)) + + // when + changed := featuresChanged(nsts) + + // then + assert.Equal(t, test.changed, changed) + }) + } +} + func p(s string) *string { return &s } diff --git a/controllers/nstemplateset/nstemplateset_controller_test.go b/controllers/nstemplateset/nstemplateset_controller_test.go index 7a8fdaab3..0af47f5da 100644 --- a/controllers/nstemplateset/nstemplateset_controller_test.go +++ b/controllers/nstemplateset/nstemplateset_controller_test.go @@ -1415,6 +1415,12 @@ func withNSTemplateSetFeatureAnnotation(feature string) nsTmplSetOption { } } +func withStatusFeatureToggles(features []string) nsTmplSetOption { + return func(nsTmplSet *toolchainv1alpha1.NSTemplateSet) { + nsTmplSet.Status.FeatureToggles = features + } +} + func withDeletionTs() nsTmplSetOption { return func(nsTmplSet *toolchainv1alpha1.NSTemplateSet) { deletionTS := metav1.Now() diff --git a/controllers/nstemplateset/status.go b/controllers/nstemplateset/status.go index 563214de3..7b74b753d 100644 --- a/controllers/nstemplateset/status.go +++ b/controllers/nstemplateset/status.go @@ -108,20 +108,37 @@ func (r *statusManager) updateStatusClusterResourcesRevisions(ctx context.Contex func featureAnnotationNeedsUpdate(nsTmplSet *toolchainv1alpha1.NSTemplateSet) (bool, []string) { featureAnnotation := nsTmplSet.Annotations[toolchainv1alpha1.FeatureToggleNameAnnotationKey] featureAnnotationList := utils.SplitCommaSeparatedList(featureAnnotation) - // order is not important, so we are sorting the lists just for the sake of the comparison - transform := cmp.Transformer("Sort", func(in []int) []int { - out := append([]int(nil), in...) // Copy input to avoid mutating it - sort.Ints(out) - return out - }) - return !cmp.Equal(featureAnnotationList, nsTmplSet.Status.FeatureToggles, transform), featureAnnotationList + + if len(featureAnnotationList) != len(nsTmplSet.Status.FeatureToggles) { + return true, featureAnnotationList + } + + // the number of features is always going to be very small, say < 5, so there's no point + // in allocating a map or doing anything fancy. An O(n^2) nested for-loop is good enough. + + for _, a := range featureAnnotationList { + found := false + for _, b := range nsTmplSet.Status.FeatureToggles { + if a == b { + found = true + break + } + } + if !found { + return true, featureAnnotationList + } + } + + return false, featureAnnotationList } // clusterResourcesNeedsUpdate checks if there is a drift between the cluster resources set in the spec and the status of the nstemplateset func clusterResourcesNeedsUpdate(nsTmplSet *toolchainv1alpha1.NSTemplateSet) bool { - return (nsTmplSet.Status.ClusterResources == nil && nsTmplSet.Spec.ClusterResources != nil) || - (nsTmplSet.Status.ClusterResources != nil && nsTmplSet.Spec.ClusterResources == nil) || - nsTmplSet.Status.ClusterResources.TemplateRef != nsTmplSet.Spec.ClusterResources.TemplateRef + if nsTmplSet.Status.ClusterResources != nil { + return nsTmplSet.Spec.ClusterResources == nil || nsTmplSet.Spec.ClusterResources.TemplateRef != nsTmplSet.Status.ClusterResources.TemplateRef + } else { + return nsTmplSet.Spec.ClusterResources != nil + } } func (r *statusManager) updateStatusNamespacesRevisions(ctx context.Context, nsTmplSet *toolchainv1alpha1.NSTemplateSet) error { From 7607f59d9eaa9d50834906d5d5bcabc9d54072d9 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Thu, 25 Sep 2025 13:31:40 +0200 Subject: [PATCH 09/16] improve the feature drift detection and add tests --- .../nstemplateset/cluster_resources_test.go | 8 +-- controllers/nstemplateset/features_test.go | 6 ++ controllers/nstemplateset/status.go | 34 +++++---- controllers/nstemplateset/status_test.go | 70 ++++++++++++++++++- 4 files changed, 94 insertions(+), 24 deletions(-) diff --git a/controllers/nstemplateset/cluster_resources_test.go b/controllers/nstemplateset/cluster_resources_test.go index d53c53723..8c94e32d5 100644 --- a/controllers/nstemplateset/cluster_resources_test.go +++ b/controllers/nstemplateset/cluster_resources_test.go @@ -181,11 +181,9 @@ func TestEnsureClusterResourcesOK(t *testing.T) { withClusterResources("abcde11"), withStatusClusterResources("abcde11"), withConditions(Provisioned())) - crq := newClusterResourceQuota("feature-1-for-"+spacename, "advanced") - crb := newTektonClusterRoleBinding(fmt.Sprintf("feature-1-for-%s", spacename), "advanced") - idlerDev := newIdler(spacename, spacename+"-dev", "advanced") - idlerStage := newIdler(spacename, spacename+"-stage", "advanced") - manager, fakeClient := prepareClusterResourcesManager(t, nsTmplSet, crq, crb, idlerDev, idlerStage) + crq := newClusterResourceQuota("", "advanced") + crq.Name = "feature-1-for-" + spacename // manually create the resource with the name matching the feature resource + manager, fakeClient := prepareClusterResourcesManager(t, nsTmplSet, crq) // when err := manager.ensure(ctx, nsTmplSet) diff --git a/controllers/nstemplateset/features_test.go b/controllers/nstemplateset/features_test.go index e2424e944..88586a46e 100644 --- a/controllers/nstemplateset/features_test.go +++ b/controllers/nstemplateset/features_test.go @@ -137,6 +137,12 @@ func TestFeaturesChanged(t *testing.T) { annoFeatures: "feature1,feature3", changed: true, }, + { + name: "should detect duplicates", + statusFeatures: []string{"feature1", "feature2", "feature1"}, + annoFeatures: "feature2,feature1", + changed: false, + }, } { t.Run(test.name, func(t *testing.T) { // given diff --git a/controllers/nstemplateset/status.go b/controllers/nstemplateset/status.go index 7b74b753d..c4f5c2fdb 100644 --- a/controllers/nstemplateset/status.go +++ b/controllers/nstemplateset/status.go @@ -2,6 +2,7 @@ package nstemplateset import ( "context" + "slices" "sort" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" @@ -108,28 +109,25 @@ func (r *statusManager) updateStatusClusterResourcesRevisions(ctx context.Contex func featureAnnotationNeedsUpdate(nsTmplSet *toolchainv1alpha1.NSTemplateSet) (bool, []string) { featureAnnotation := nsTmplSet.Annotations[toolchainv1alpha1.FeatureToggleNameAnnotationKey] featureAnnotationList := utils.SplitCommaSeparatedList(featureAnnotation) + // sort and deduplicate the list, so that the caller can use the "cleaned up" value + slices.Sort(featureAnnotationList) + featureAnnotationList = slices.Compact(featureAnnotationList) - if len(featureAnnotationList) != len(nsTmplSet.Status.FeatureToggles) { - return true, featureAnnotationList - } + statusFeatureList := nsTmplSet.Status.FeatureToggles - // the number of features is always going to be very small, say < 5, so there's no point - // in allocating a map or doing anything fancy. An O(n^2) nested for-loop is good enough. + // now that the features in annotation are sorted and deduplicated we can just loop through + // them and look for each one of them in the status features. Note that we cannot + // short-circuit on length, because the status feature list is potentially not deduplicated. - for _, a := range featureAnnotationList { - found := false - for _, b := range nsTmplSet.Status.FeatureToggles { - if a == b { - found = true - break - } - } - if !found { - return true, featureAnnotationList - } - } + annosNotInStatus := slices.ContainsFunc(featureAnnotationList, func(f string) bool { + return !slices.Contains(statusFeatureList, f) + }) + + statusesNotInAnno := slices.ContainsFunc(statusFeatureList, func(f string) bool { + return !slices.Contains(featureAnnotationList, f) + }) - return false, featureAnnotationList + return annosNotInStatus || statusesNotInAnno, featureAnnotationList } // clusterResourcesNeedsUpdate checks if there is a drift between the cluster resources set in the spec and the status of the nstemplateset diff --git a/controllers/nstemplateset/status_test.go b/controllers/nstemplateset/status_test.go index 99d065168..eb59aa681 100644 --- a/controllers/nstemplateset/status_test.go +++ b/controllers/nstemplateset/status_test.go @@ -157,7 +157,6 @@ func TestUpdateStatus(t *testing.T) { }) t.Run("status update failures", func(t *testing.T) { - t.Run("failed to update status during deletion", func(t *testing.T) { // given an NSTemplateSet resource which is being deleted and whose finalizer was not removed yet nsTmplSet := newNSTmplSet(namespaceName, spacename, "basic", withDeletionTs(), withClusterResources("abcde11"), withNamespaces("abcde11", "dev", "code")) @@ -217,6 +216,7 @@ func TestUpdateStatus(t *testing.T) { HasConditions(conditions...) }) } + func TestUpdateStatusToProvisionedWhenPreviouslyWasSetToFailed(t *testing.T) { logger := zap.New(zap.UseDevMode(true)) log.SetLogger(logger) @@ -273,3 +273,71 @@ func TestUpdateStatusToProvisionedWhenPreviouslyWasSetToFailed(t *testing.T) { HasConditions(Provisioned()) }) } + +func TestFeatureAnnotationNeedsUpdate(t *testing.T) { + for _, test := range []struct { + name string + annoFeatures string + statusFeatures []string + changed bool + featuresToApply []string + }{ + { + name: "should report no change when no features in either annos or status", + changed: false, + featuresToApply: []string{}, + }, + { + name: "should report change when no features in status", + annoFeatures: "feature", + changed: true, + featuresToApply: []string{"feature"}, + }, + { + name: "should report change when no features in anno", + statusFeatures: []string{"feature"}, + changed: true, + featuresToApply: []string{}, + }, + { + name: "should report no change when features equal", + statusFeatures: []string{"feature1", "feature2"}, + annoFeatures: "feature1,feature2", + changed: false, + featuresToApply: []string{"feature1", "feature2"}, + }, + { + name: "should report change when features differ in number", + statusFeatures: []string{"feature1", "feature2"}, + annoFeatures: "feature1", + changed: true, + featuresToApply: []string{"feature1"}, + }, + { + name: "should report change when features differ", + statusFeatures: []string{"feature1", "feature2"}, + annoFeatures: "feature1,feature3", + changed: true, + featuresToApply: []string{"feature1", "feature3"}, + }, + { + name: "should detect duplicates", + statusFeatures: []string{"feature1", "feature2", "feature1"}, + annoFeatures: "feature2,feature2,feature1,feature1", + changed: false, + featuresToApply: []string{"feature1", "feature2"}, + }, + } { + t.Run(test.name, func(t *testing.T) { + // given + nsts := newNSTmplSet("default", "ns", "base", withNSTemplateSetFeatureAnnotation(test.annoFeatures), withStatusFeatureToggles(test.statusFeatures)) + + // when + changed, featuresToApply := featureAnnotationNeedsUpdate(nsts) + + // then + assert.Equal(t, test.changed, changed) + assert.Equal(t, test.featuresToApply, featuresToApply) + }) + } +} From 603ea2e684187e930b43f2864a8b0ef4b416a20c Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Wed, 8 Oct 2025 21:00:59 +0200 Subject: [PATCH 10/16] Update controllers/nstemplateset/cluster_resources_test.go Co-authored-by: Matous Jobanek --- controllers/nstemplateset/cluster_resources_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/nstemplateset/cluster_resources_test.go b/controllers/nstemplateset/cluster_resources_test.go index 8c94e32d5..0011ab3a4 100644 --- a/controllers/nstemplateset/cluster_resources_test.go +++ b/controllers/nstemplateset/cluster_resources_test.go @@ -665,7 +665,7 @@ func TestUpdateClusterResources(t *testing.T) { crq := newClusterResourceQuota(spaceName, "advanced") t.Run("success", func(t *testing.T) { - t.Run("update from abcde11 revision to abcde12 revision as part of the advanced tier by updating CRQ", func(t *testing.T) { + t.Run("update from abcde11 revision to abcde12 revision as part of the advanced tier by updating CRQ and by deleting CRB", func(t *testing.T) { // given nsTmplSet := newNSTmplSet(namespaceName, spaceName, "advanced", withNamespaces("abcde12", "dev"), withClusterResources("abcde12"), withStatusClusterResources("abcde11")) codeNs := newNamespace("advanced", spaceName, "dev") From 7b27a43135cbbaff4868aeb56de7dbdc7a81516b Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Wed, 8 Oct 2025 21:02:11 +0200 Subject: [PATCH 11/16] Update controllers/nstemplateset/cluster_resources_test.go Co-authored-by: Matous Jobanek --- controllers/nstemplateset/cluster_resources_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/nstemplateset/cluster_resources_test.go b/controllers/nstemplateset/cluster_resources_test.go index 0011ab3a4..632e3eb5d 100644 --- a/controllers/nstemplateset/cluster_resources_test.go +++ b/controllers/nstemplateset/cluster_resources_test.go @@ -733,7 +733,7 @@ func TestUpdateClusterResources(t *testing.T) { WithLabel("toolchain.dev.openshift.com/templateref", "advanced-clusterresources-abcde11")) }) - t.Run("update from abcde12 revision to abcde11 revision as part of the advanced tier by updating CRQ", func(t *testing.T) { + t.Run("update from abcde12 revision to abcde11 revision as part of the advanced tier by updating CRQ and creating CRB", func(t *testing.T) { // given nsTmplSet := newNSTmplSet(namespaceName, spaceName, "advanced", withNamespaces("abcde11", "dev"), withClusterResources("abcde11"), withStatusClusterResources("abcde12")) crq := newClusterResourceQuota(spaceName, "advanced", withTemplateRefUsingRevision("abcde12")) From bf18ea8383743bc34c0e54aede40f6d8be1acedf Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Wed, 8 Oct 2025 16:42:46 +0200 Subject: [PATCH 12/16] Make the logic in clusterResourceManager.ensure() more digestable by splitting it up into more functions and structs. --- .../nstemplateset/cluster_resources.go | 245 ++++++++++-------- 1 file changed, 143 insertions(+), 102 deletions(-) diff --git a/controllers/nstemplateset/cluster_resources.go b/controllers/nstemplateset/cluster_resources.go index 3509a2431..7e0303e1f 100644 --- a/controllers/nstemplateset/cluster_resources.go +++ b/controllers/nstemplateset/cluster_resources.go @@ -27,121 +27,71 @@ type clusterResourcesManager struct { // resource. It is assumed that if a cluster-scoped resource should be common to more than 1 nstemplateset, it should // be deployed externally from Devsandbox, e.g. using ArgoCD. func (r *clusterResourcesManager) ensure(ctx context.Context, nsTmplSet *toolchainv1alpha1.NSTemplateSet) error { - logger := log.FromContext(ctx) - userTierLogger := logger.WithValues("spacename", nsTmplSet.GetName(), "tier", nsTmplSet.Spec.TierName) - userTierCtx := log.IntoContext(ctx, userTierLogger) - userTierLogger.Info("ensuring cluster resources") + logger := log.FromContext(ctx, "spacename", nsTmplSet.GetName(), "tier", nsTmplSet.Spec.TierName) + logger.Info("ensuring cluster resources") + ctx = log.IntoContext(ctx, logger) - spacename := nsTmplSet.GetName() - - var oldTemplateRef, newTemplateRef string - if nsTmplSet.Spec.ClusterResources != nil { - newTemplateRef = nsTmplSet.Spec.ClusterResources.TemplateRef - } - if nsTmplSet.Status.ClusterResources != nil { - oldTemplateRef = nsTmplSet.Status.ClusterResources.TemplateRef - } - - if oldTemplateRef == newTemplateRef && !featuresChanged(nsTmplSet) { + oldTemplateRef, newTemplateRef, changed := getOldAndNewTemplateRefsIfChanged(nsTmplSet) + if !changed { return nil } - var newTierTemplate *tierTemplate - var oldTierTemplate *tierTemplate - var err error - if newTemplateRef != "" { - newTierTemplate, err = getTierTemplate(ctx, r.GetHostClusterClient, newTemplateRef) - if err != nil { - return r.wrapErrorWithStatusUpdateForClusterResourceFailure(userTierCtx, nsTmplSet, err, - "failed to retrieve the TierTemplate for the to-be-applied cluster resources with the name '%s'", newTemplateRef) - } - } - if oldTemplateRef != "" { - oldTierTemplate, err = getTierTemplate(ctx, r.GetHostClusterClient, oldTemplateRef) - if err != nil { - return r.wrapErrorWithStatusUpdateForClusterResourceFailure(userTierCtx, nsTmplSet, err, - "failed to retrieve TierTemplate for the last-applied cluster resources with the name '%s'", oldTemplateRef) - } + newTierTemplate, newObjs, err := r.processTierTemplate(ctx, nsTmplSet.Spec.ClusterResources, nsTmplSet.Name) + if err != nil { + return r.wrapErrorWithStatusUpdateForClusterResourceFailure(ctx, nsTmplSet, err, + "failed to process the template for the to-be-applied cluster resources with the name '%s'", newTemplateRef) } - var newObjs, currentObjects []runtimeclient.Object - if newTierTemplate != nil { - newObjs, err = newTierTemplate.process(r.Scheme, map[string]string{SpaceName: spacename}) - if err != nil { - return r.wrapErrorWithStatusUpdateForClusterResourceFailure(ctx, nsTmplSet, err, - "failed to process template for the to-be-applied cluster resources with the name '%s'", newTemplateRef) - } - } - if oldTierTemplate != nil { - currentObjects, err = oldTierTemplate.process(r.Scheme, map[string]string{SpaceName: spacename}) - if err != nil { - return r.wrapErrorWithStatusUpdateForClusterResourceFailure(ctx, nsTmplSet, err, - "failed to process template for the last-applied cluster resources with the name '%s'", oldTemplateRef) - } + _, curObjs, err := r.processTierTemplate(ctx, nsTmplSet.Status.ClusterResources, nsTmplSet.Name) + if err != nil { + return r.wrapErrorWithStatusUpdateForClusterResourceFailure(ctx, nsTmplSet, err, + "failed to process the template for the last-applied cluster resources with the name '%s'", oldTemplateRef) } - statusChanged := false - firstDeployment := nsTmplSet.Status.ClusterResources == nil || nsTmplSet.Status.ClusterResources.TemplateRef == "" - var failureStatusReason statusUpdater - if firstDeployment { - failureStatusReason = r.setStatusClusterResourcesProvisionFailed - } else { - failureStatusReason = r.setStatusUpdateFailed - } + objectApplier := newObjectApplier(r, nsTmplSet, curObjs, newTierTemplate) - changeStatusIfNeeded := func() error { - if !statusChanged { - if firstDeployment { - err = r.setStatusProvisioningIfNotUpdating(ctx, nsTmplSet) - } else { - err = r.setStatusUpdatingIfNotProvisioning(ctx, nsTmplSet) - } - if err != nil { - return err - } - statusChanged = true + for _, newObj := range newObjs { + if err = objectApplier.Apply(ctx, newObj); err != nil { + return err } - return nil } - for _, newObj := range newObjs { - if !shouldCreate(newObj, nsTmplSet) { - continue - } + return objectApplier.Cleanup(ctx) +} - // by removing the new objects from the current objects, we are going to be left with the objects - // that should be removed from the cluster after we're done with this loop - for oldIdx, curObj := range currentObjects { - if commonclient.SameGVKandName(curObj, newObj) { - currentObjects = slices.Delete(currentObjects, oldIdx, oldIdx+1) - break - } - } +func getOldAndNewTemplateRefsIfChanged(nstt *toolchainv1alpha1.NSTemplateSet) (oldTemplateRef string, newTemplateRef string, changed bool) { + if nstt.Spec.ClusterResources != nil { + newTemplateRef = nstt.Spec.ClusterResources.TemplateRef + } + if nstt.Status.ClusterResources != nil { + oldTemplateRef = nstt.Status.ClusterResources.TemplateRef + } - if err := changeStatusIfNeeded(); err != nil { - return err - } + changed = oldTemplateRef != newTemplateRef || featuresChanged(nstt) - // create or update the resource - if _, err = r.apply(ctx, nsTmplSet, newTierTemplate, newObj); err != nil { - err := fmt.Errorf("failed to apply changes to the cluster resource %s, %s: %w", newObj.GetName(), newObj.GetObjectKind().GroupVersionKind().String(), err) - return r.wrapErrorWithStatusUpdate(ctx, nsTmplSet, failureStatusReason, err, "failure while syncing cluster resources") - } - } + return oldTemplateRef, newTemplateRef, changed +} - // what we're left with here is the list of currently existing objects that are no longer present in the template. - // we need to delete them - if len(currentObjects) > 0 { - if err := changeStatusIfNeeded(); err != nil { - return err - } +func (r *clusterResourcesManager) processTierTemplate(ctx context.Context, clusterResources *toolchainv1alpha1.NSTemplateSetClusterResources, spacename string) (*tierTemplate, []runtimeclient.Object, error) { + if clusterResources == nil { + return nil, nil, nil } - if err := deleteObsoleteObjects(ctx, r.Client, currentObjects, nil); err != nil { - return r.wrapErrorWithStatusUpdate(ctx, nsTmplSet, failureStatusReason, err, "failure while syncing cluster resources") + var tierTemplate *tierTemplate + var objs []runtimeclient.Object + if clusterResources.TemplateRef != "" { + var err error + tierTemplate, err = getTierTemplate(ctx, r.GetHostClusterClient, clusterResources.TemplateRef) + if err != nil { + return nil, nil, err + } + objs, err = tierTemplate.process(r.Scheme, map[string]string{SpaceName: spacename}) + if err != nil { + return nil, nil, err + } } - return nil + return tierTemplate, objs, nil } // apply creates or updates the given object with the set of toolchain labels. If the apply operation was successful, then it returns 'true, nil', @@ -173,15 +123,10 @@ func (r *clusterResourcesManager) delete(ctx context.Context, nsTmplSet *toolcha return nil } - currentTierTemplate, err := getTierTemplate(ctx, r.GetHostClusterClient, nsTmplSet.Status.ClusterResources.TemplateRef) + _, currentObjects, err := r.processTierTemplate(ctx, nsTmplSet.Status.ClusterResources, nsTmplSet.Name) if err != nil { return r.wrapErrorWithStatusUpdateForClusterResourceFailure(ctx, nsTmplSet, err, - "failed to read the existing cluster resources") - } - currentObjects, err := currentTierTemplate.process(r.Scheme, map[string]string{SpaceName: nsTmplSet.Name}) - if err != nil { - return r.wrapErrorWithStatusUpdateForClusterResourceFailure(ctx, nsTmplSet, err, - "failed to list existing cluster resources") + "failed to process the existing cluster resources") } for _, toDelete := range currentObjects { @@ -206,3 +151,99 @@ func (r *clusterResourcesManager) delete(ctx context.Context, nsTmplSet *toolcha } return nil } + +// objectApplier is a helper to the clusterResourcesManager.ensure() method. +// A new instance can be obtained using the newObjectApplier() function. +// +// It takes in the current state of the NSTemplateSet and then processes one new object to apply +// to the cluster at a time, updating the internal bookkeeping to be able to Cleanup() the old objects +// after all the new objects have been applied. +type objectApplier struct { + r *clusterResourcesManager + statusChanged bool + firstDeployment bool + failureStatusReason statusUpdater + nstt *toolchainv1alpha1.NSTemplateSet + currentObjects []runtimeclient.Object + newTierTemplate *tierTemplate +} + +func newObjectApplier(r *clusterResourcesManager, nstt *toolchainv1alpha1.NSTemplateSet, currentObjects []runtimeclient.Object, newTierTemplate *tierTemplate) *objectApplier { + firstDeployment := nstt.Status.ClusterResources == nil || nstt.Status.ClusterResources.TemplateRef == "" + var failureStatusReason statusUpdater + + if firstDeployment { + failureStatusReason = r.setStatusClusterResourcesProvisionFailed + } else { + failureStatusReason = r.setStatusUpdateFailed + } + + return &objectApplier{ + r: r, + statusChanged: false, + firstDeployment: firstDeployment, + failureStatusReason: failureStatusReason, + nstt: nstt, + currentObjects: currentObjects, + newTierTemplate: newTierTemplate, + } +} + +func (oa *objectApplier) changeStatusIfNeeded(ctx context.Context) error { + if !oa.statusChanged { + var err error + if oa.firstDeployment { + err = oa.r.setStatusProvisioningIfNotUpdating(ctx, oa.nstt) + } else { + err = oa.r.setStatusUpdatingIfNotProvisioning(ctx, oa.nstt) + } + if err != nil { + return err + } + oa.statusChanged = true + } + return nil +} + +func (oa *objectApplier) Apply(ctx context.Context, obj runtimeclient.Object) error { + if !shouldCreate(obj, oa.nstt) { + return nil + } + + // by removing the new objects from the current objects, we are going to be left with the objects + // that should be removed from the cluster after we're done with this loop + for oldIdx, curObj := range oa.currentObjects { + if commonclient.SameGVKandName(curObj, obj) { + oa.currentObjects = slices.Delete(oa.currentObjects, oldIdx, oldIdx+1) + break + } + } + + if err := oa.changeStatusIfNeeded(ctx); err != nil { + return err + } + + // create or update the resource + if _, err := oa.r.apply(ctx, oa.nstt, oa.newTierTemplate, obj); err != nil { + err := fmt.Errorf("failed to apply changes to the cluster resource %s, %s: %w", obj.GetName(), obj.GetObjectKind().GroupVersionKind().String(), err) + return oa.r.wrapErrorWithStatusUpdate(ctx, oa.nstt, oa.failureStatusReason, err, "failure while syncing cluster resources") + } + + return nil +} + +func (oa *objectApplier) Cleanup(ctx context.Context) error { + // what we're left with here is the list of currently existing objects that are no longer present in the template. + // we need to delete them + if len(oa.currentObjects) > 0 { + if err := oa.changeStatusIfNeeded(ctx); err != nil { + return err + } + } + + if err := deleteObsoleteObjects(ctx, oa.r.Client, oa.currentObjects, nil); err != nil { + return oa.r.wrapErrorWithStatusUpdate(ctx, oa.nstt, oa.failureStatusReason, err, "failure while syncing cluster resources") + } + + return nil +} From 22402144a7eb5bcbf680240e3fd03b18486ebc39 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Wed, 8 Oct 2025 16:42:46 +0200 Subject: [PATCH 13/16] Clean up the tests. --- .../nstemplateset/cluster_resources_test.go | 95 ++++++++++--------- .../nstemplateset_controller_test.go | 4 +- test/nstemplateset_assertion.go | 7 ++ 3 files changed, 58 insertions(+), 48 deletions(-) diff --git a/controllers/nstemplateset/cluster_resources_test.go b/controllers/nstemplateset/cluster_resources_test.go index 632e3eb5d..9f159fc31 100644 --- a/controllers/nstemplateset/cluster_resources_test.go +++ b/controllers/nstemplateset/cluster_resources_test.go @@ -42,8 +42,10 @@ func TestEnsureClusterResourcesOK(t *testing.T) { // given manager, failingClient := prepareClusterResourcesManager(t, nsTmplSet) + // when err := manager.ensure(ctx, nsTmplSet) + // then require.NoError(t, err) AssertThatNSTemplateSet(t, namespaceName, spacename, failingClient). HasFinalizer(). @@ -129,48 +131,50 @@ func TestEnsureClusterResourcesOK(t *testing.T) { } }) - t.Run("should not create ClusterResource objects when the field is nil", func(t *testing.T) { - // given - nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withNamespaces("abcde11", "dev")) - manager, fakeClient := prepareClusterResourcesManager(t, nsTmplSet) + t.Run("should not do anything when template refs match", func(t *testing.T) { + t.Run("when templaterefs are set", func(t *testing.T) { + // given + nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", + withNamespaces("abcde11", "dev"), + withClusterResources("abcde11"), + withStatusClusterResources("abcde11"), + withConditions(Provisioned())) + crq := newClusterResourceQuota(spacename, "advanced") + crb := newTektonClusterRoleBinding(spacename, "advanced") + idlerDev := newIdler(spacename, spacename+"-dev", "advanced") + idlerStage := newIdler(spacename, spacename+"-stage", "advanced") + manager, fakeClient := prepareClusterResourcesManager(t, nsTmplSet, crq, crb, idlerDev, idlerStage) - // when - err := manager.ensure(ctx, nsTmplSet) + // when + err := manager.ensure(ctx, nsTmplSet) - // then - require.NoError(t, err) - AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). - HasFinalizer(). - HasSpecNamespaces("dev"). - HasNoConditions() - }) + // then + require.NoError(t, err) + AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). + HasFinalizer(). + HasConditions(Provisioned()) + AssertThatCluster(t, fakeClient). + HasResource("for-"+spacename, "av1.ClusterResourceQuota{}). + HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}). + HasResource(spacename+"-dev", &toolchainv1alpha1.Idler{}). + HasResource(spacename+"-stage", &toolchainv1alpha1.Idler{}) + }) - t.Run("should not do anything when all cluster resources are already created", func(t *testing.T) { - // given - nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", - withNamespaces("abcde11", "dev"), - withClusterResources("abcde11"), - withStatusClusterResources("abcde11"), - withConditions(Provisioned())) - crq := newClusterResourceQuota(spacename, "advanced") - crb := newTektonClusterRoleBinding(spacename, "advanced") - idlerDev := newIdler(spacename, spacename+"-dev", "advanced") - idlerStage := newIdler(spacename, spacename+"-stage", "advanced") - manager, fakeClient := prepareClusterResourcesManager(t, nsTmplSet, crq, crb, idlerDev, idlerStage) + t.Run("when no templateref set", func(t *testing.T) { + nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withNamespaces("abcde11", "dev")) + manager, fakeClient := prepareClusterResourcesManager(t, nsTmplSet) - // when - err := manager.ensure(ctx, nsTmplSet) + // when + err := manager.ensure(ctx, nsTmplSet) - // then - require.NoError(t, err) - AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). - HasFinalizer(). - HasConditions(Provisioned()) - AssertThatCluster(t, fakeClient). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}). - HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}). - HasResource(spacename+"-dev", &toolchainv1alpha1.Idler{}). - HasResource(spacename+"-stage", &toolchainv1alpha1.Idler{}) + // then + require.NoError(t, err) + AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). + HasFinalizer(). + HasClusterResourcesNil(). + HasStatusClusterResourcesNil(). + HasNoConditions() + }) }) t.Run("should clean up resources from no longer active features", func(t *testing.T) { @@ -181,7 +185,7 @@ func TestEnsureClusterResourcesOK(t *testing.T) { withClusterResources("abcde11"), withStatusClusterResources("abcde11"), withConditions(Provisioned())) - crq := newClusterResourceQuota("", "advanced") + crq := newClusterResourceQuota(spacename, "advanced", withFeatureAnnotation("feature-1"), withName("feature-1-for-"+spacename)) crq.Name = "feature-1-for-" + spacename // manually create the resource with the name matching the feature resource manager, fakeClient := prepareClusterResourcesManager(t, nsTmplSet, crq) @@ -194,7 +198,7 @@ func TestEnsureClusterResourcesOK(t *testing.T) { HasFinalizer(). HasConditions(Updating()) AssertThatCluster(t, fakeClient). - HasNoResource("feature-1-for-"+spacename, "av1.ClusterResourceQuota{}). + HasNoResource(crq.Name, "av1.ClusterResourceQuota{}). HasResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}). HasResource(spacename+"-dev", &toolchainv1alpha1.Idler{}). HasResource(spacename+"-stage", &toolchainv1alpha1.Idler{}) @@ -223,7 +227,7 @@ func TestEnsureClusterResourcesFail(t *testing.T) { // then require.Error(t, err) - assert.Contains(t, err.Error(), "failed to retrieve the TierTemplate for the to-be-applied cluster resources with the name 'fail-clusterresources-abcde11'") + assert.Contains(t, err.Error(), "failed to process the template for the to-be-applied cluster resources with the name 'fail-clusterresources-abcde11'") AssertThatNSTemplateSet(t, namespaceName, spacename, fakeClient). HasFinalizer(). HasConditions(UnableToProvisionClusterResources( @@ -277,12 +281,12 @@ func TestDeleteClusterResources(t *testing.T) { HasNoResource(spacename+"-tekton-view", &rbacv1.ClusterRoleBinding{}) }) - t.Run("delete the second ClusterResourceQuota since the first one has deletion timestamp set", func(t *testing.T) { + t.Run("skips objects with deletion timestamp set", func(t *testing.T) { // given nsTmplSet := newNSTmplSet(namespaceName, spacename, "withemptycrq", withNamespaces("abcde11", "dev"), withClusterResources("abcde11"), withStatusClusterResources("abcde11")) crq := newClusterResourceQuota(spacename, "withemptycrq", withFinalizer()) deletionTS := metav1.NewTime(time.Now()) - crq.SetDeletionTimestamp(&deletionTS) + crq.SetDeletionTimestamp(&deletionTS) // this is ok, because crq has a finalizer emptyCrq := newClusterResourceQuota("empty", "withemptycrq") emptyCrq.Labels[toolchainv1alpha1.SpaceLabelKey] = spacename manager, cl := prepareClusterResourcesManager(t, nsTmplSet, crq, emptyCrq) @@ -293,8 +297,8 @@ func TestDeleteClusterResources(t *testing.T) { // then require.NoError(t, err) AssertThatCluster(t, cl). - HasResource("for-"+spacename, "av1.ClusterResourceQuota{}, HasDeletionTimestamp()). - HasNoResource("for-empty", "av1.ClusterResourceQuota{}) + HasResource(crq.Name, "av1.ClusterResourceQuota{}, HasDeletionTimestamp()). + HasNoResource(emptyCrq.Name, "av1.ClusterResourceQuota{}) }) t.Run("delete ClusterResourceQuota for enabled feature", func(t *testing.T) { @@ -409,7 +413,6 @@ func TestPromoteClusterResources(t *testing.T) { withNamespaces("dev"), withClusterResources("abcde11"), withStatusClusterResourcesInTier("withemptycrq", "previousrevision")) - codeNs := newNamespace("advanced", spaceName, "code") crq := newClusterResourceQuota(spaceName, "withemptycrq") crq.Labels["disappearingLabel"] = "value" crq.Spec.Quota.Hard["limits.cpu"] = resource.MustParse("100m") @@ -417,7 +420,7 @@ func TestPromoteClusterResources(t *testing.T) { crb.Labels["disappearingLabel"] = "value" emptyCrq := newClusterResourceQuota(spaceName, "withemptycrq") emptyCrq.Name = "for-empty" - manager, cl := prepareClusterResourcesManager(t, nsTmplSet, emptyCrq, crq, crb, codeNs, previousTierTemplate) + manager, cl := prepareClusterResourcesManager(t, nsTmplSet, emptyCrq, crq, crb, previousTierTemplate) // when err = manager.ensure(ctx, nsTmplSet) diff --git a/controllers/nstemplateset/nstemplateset_controller_test.go b/controllers/nstemplateset/nstemplateset_controller_test.go index 0af47f5da..7f9bccbae 100644 --- a/controllers/nstemplateset/nstemplateset_controller_test.go +++ b/controllers/nstemplateset/nstemplateset_controller_test.go @@ -728,7 +728,7 @@ func TestReconcilePromotion(t *testing.T) { t.Cleanup(restore) t.Run("upgrade from basic to advanced tier", func(t *testing.T) { - t.Run("create ClusterResourceQuota", func(t *testing.T) { + t.Run("create cluster-scoped resources", func(t *testing.T) { // given previousClusterTemplate := newTierTemplate("basic", "clusterresources", "abcde11") nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withNamespaces("abcde11", "dev"), withClusterResources("abcde11"), withStatusClusterResourcesInTier("basic", "abcde11")) @@ -834,7 +834,7 @@ func TestReconcileUpdate(t *testing.T) { t.Cleanup(restore) t.Run("upgrade from abcde11 to abcde12 as part of the advanced tier", func(t *testing.T) { - t.Run("update ClusterResourceQuota", func(t *testing.T) { + t.Run("syncs cluster-scoped resources", func(t *testing.T) { // given nsTmplSet := newNSTmplSet(namespaceName, spacename, "advanced", withNamespaces("abcde12", "dev"), diff --git a/test/nstemplateset_assertion.go b/test/nstemplateset_assertion.go index b579c18b3..4b1bf33c3 100644 --- a/test/nstemplateset_assertion.go +++ b/test/nstemplateset_assertion.go @@ -180,6 +180,13 @@ func (a *NSTemplateSetAssertion) HasClusterResourcesNil() *NSTemplateSetAssertio return a } +func (a *NSTemplateSetAssertion) HasStatusClusterResourcesNil() *NSTemplateSetAssertion { + err := a.loadNSTemplateSet() + require.NoError(a.t, err) + assert.Nil(a.t, a.nsTmplSet.Status.ClusterResources) + return a +} + func (a *NSTemplateSetAssertion) HasNamespaceTemplateRefs(templateRefs ...string) *NSTemplateSetAssertion { err := a.loadNSTemplateSet() require.NoError(a.t, err) From 7c4d504edb53a37a831fdd0dab4f9474dffce7a0 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Wed, 15 Oct 2025 14:58:39 +0200 Subject: [PATCH 14/16] setting the name manually is not necessary with 'withName' option --- controllers/nstemplateset/cluster_resources_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/controllers/nstemplateset/cluster_resources_test.go b/controllers/nstemplateset/cluster_resources_test.go index 9f159fc31..016cf127d 100644 --- a/controllers/nstemplateset/cluster_resources_test.go +++ b/controllers/nstemplateset/cluster_resources_test.go @@ -186,7 +186,6 @@ func TestEnsureClusterResourcesOK(t *testing.T) { withStatusClusterResources("abcde11"), withConditions(Provisioned())) crq := newClusterResourceQuota(spacename, "advanced", withFeatureAnnotation("feature-1"), withName("feature-1-for-"+spacename)) - crq.Name = "feature-1-for-" + spacename // manually create the resource with the name matching the feature resource manager, fakeClient := prepareClusterResourcesManager(t, nsTmplSet, crq) // when From 725c1596a2d7e77e166c8dc15c6a2b3460795900 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Thu, 16 Oct 2025 09:59:40 +0200 Subject: [PATCH 15/16] add a small explanatory comment --- controllers/nstemplateset/cluster_resources.go | 1 + 1 file changed, 1 insertion(+) diff --git a/controllers/nstemplateset/cluster_resources.go b/controllers/nstemplateset/cluster_resources.go index 7e0303e1f..82ab1441a 100644 --- a/controllers/nstemplateset/cluster_resources.go +++ b/controllers/nstemplateset/cluster_resources.go @@ -169,6 +169,7 @@ type objectApplier struct { } func newObjectApplier(r *clusterResourcesManager, nstt *toolchainv1alpha1.NSTemplateSet, currentObjects []runtimeclient.Object, newTierTemplate *tierTemplate) *objectApplier { + // if there's no clusterresources templateref mentioned in the tiertemplate's status, it was never deployed before. firstDeployment := nstt.Status.ClusterResources == nil || nstt.Status.ClusterResources.TemplateRef == "" var failureStatusReason statusUpdater From cbdd7de36f6a5e030b7302a563bf3b95aefe96e0 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Mon, 20 Oct 2025 10:45:25 +0200 Subject: [PATCH 16/16] add some clarification comments --- controllers/nstemplateset/cluster_resources.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/controllers/nstemplateset/cluster_resources.go b/controllers/nstemplateset/cluster_resources.go index 82ab1441a..f125e6ad2 100644 --- a/controllers/nstemplateset/cluster_resources.go +++ b/controllers/nstemplateset/cluster_resources.go @@ -234,14 +234,16 @@ func (oa *objectApplier) Apply(ctx context.Context, obj runtimeclient.Object) er } func (oa *objectApplier) Cleanup(ctx context.Context) error { - // what we're left with here is the list of currently existing objects that are no longer present in the template. - // we need to delete them if len(oa.currentObjects) > 0 { + // we'll be making changes to the cluster, because there are still some unprocessed objects. + // let's reflect that in the status, if it was not yet updated. if err := oa.changeStatusIfNeeded(ctx); err != nil { return err } } + // what we're left with here is the list of currently existing objects that are no longer present in the template. + // we need to delete them if err := deleteObsoleteObjects(ctx, oa.r.Client, oa.currentObjects, nil); err != nil { return oa.r.wrapErrorWithStatusUpdate(ctx, oa.nstt, oa.failureStatusReason, err, "failure while syncing cluster resources") }