From 8d24435a9e7eb72f7a88b6eb50aa5e7d53e89c5d Mon Sep 17 00:00:00 2001 From: Francisc Munteanu Date: Thu, 7 Dec 2023 12:21:12 +0100 Subject: [PATCH 1/9] remove spacebinding request migration controller --- ...pacebindingrequest_migration_controller.go | 176 --------- ...indingrequest_migration_controller_test.go | 335 ------------------ ...spacebindingrequest_spacebinding_mapper.go | 44 --- ...bindingrequest_spacebinding_mapper_test.go | 47 --- main.go | 14 - 5 files changed, 616 deletions(-) delete mode 100644 controllers/spacebindingrequestmigration/spacebindingrequest_migration_controller.go delete mode 100644 controllers/spacebindingrequestmigration/spacebindingrequest_migration_controller_test.go delete mode 100644 controllers/spacebindingrequestmigration/spacebindingrequest_spacebinding_mapper.go delete mode 100644 controllers/spacebindingrequestmigration/spacebindingrequest_spacebinding_mapper_test.go diff --git a/controllers/spacebindingrequestmigration/spacebindingrequest_migration_controller.go b/controllers/spacebindingrequestmigration/spacebindingrequest_migration_controller.go deleted file mode 100644 index 4e5036da0..000000000 --- a/controllers/spacebindingrequestmigration/spacebindingrequest_migration_controller.go +++ /dev/null @@ -1,176 +0,0 @@ -package spacebindingrequestmigration - -import ( - "context" - "fmt" - - toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" - "github.com/codeready-toolchain/host-operator/pkg/cluster" - errs "github.com/pkg/errors" - "github.com/redhat-cop/operator-utils/pkg/util" - "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" - ctrl "sigs.k8s.io/controller-runtime" - runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "sigs.k8s.io/controller-runtime/pkg/handler" - "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/reconcile" - "sigs.k8s.io/controller-runtime/pkg/source" -) - -// Reconciler reconciles a SpaceBindingRequestMigration object -type Reconciler struct { - Client runtimeclient.Client - Scheme *runtime.Scheme - Namespace string - MemberClusters map[string]cluster.Cluster -} - -// SetupWithManager sets up the controller with the Manager. -func (r *Reconciler) SetupWithManager(mgr ctrl.Manager, memberClusters map[string]cluster.Cluster) error { - // Watch SpaceBindings from host cluster. - b := ctrl.NewControllerManagedBy(mgr). - For(&toolchainv1alpha1.SpaceBinding{}) - - // Watch SpaceBindingRequests in all member clusters and all namespaces and map those to their respective SpaceBinding resources. - for _, memberCluster := range memberClusters { - b = b.Watches( - source.NewKindWithCache(&toolchainv1alpha1.SpaceBindingRequest{}, memberCluster.Cache), - handler.EnqueueRequestsFromMapFunc(MapSpaceBindingRequestToSpaceBinding(r.Client, r.Namespace))) - } - return b.Complete(r) -} - -//+kubebuilder:rbac:groups=toolchain.dev.openshift.com,resources=spacebindingrequests,verbs=get;list;watch;create;update;patch;delete -//+kubebuilder:rbac:groups=toolchain.dev.openshift.com,resources=spacebindingrequests/status,verbs=get;update;patch -//+kubebuilder:rbac:groups=toolchain.dev.openshift.com,resources=spacebindingrequests/finalizers,verbs=update - -// Reconcile converts all the SpaceBindings created using the sandbox-cli to SpaceBindingRequests -func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) { - logger := log.FromContext(ctx) - logger.Info("reconciling SpaceBindingRequestMigration") - - // Fetch the SpaceBinding instance - spaceBinding := &toolchainv1alpha1.SpaceBinding{} - err := r.Client.Get(ctx, request.NamespacedName, spaceBinding) - if err != nil { - if errors.IsNotFound(err) { - // Request object not found, could have been deleted after reconcile request. - // Return and don't requeue - return reconcile.Result{}, nil - } - // Error reading the object - requeue the request. - return reconcile.Result{}, errs.Wrapf(err, "unable to get spacebinding") - } - if util.IsBeingDeleted(spaceBinding) { - logger.Info("the SpaceBinding is already being deleted") - return reconcile.Result{}, nil - } - // check if spaceBinding was created from SpaceBindingRequest, - // in that case we can skip it - if hasSpaceBindingRequest(spaceBinding) { - return reconcile.Result{}, nil - } - - spaceName := types.NamespacedName{Namespace: spaceBinding.Namespace, Name: spaceBinding.Spec.Space} - space := &toolchainv1alpha1.Space{} - if err := r.Client.Get(ctx, spaceName, space); err != nil { - if errors.IsNotFound(err) { - // space was deleted - return reconcile.Result{}, nil - } - // error while reading space - return ctrl.Result{}, errs.Wrapf(err, "unable to get the bound Space") - } - - murName := types.NamespacedName{Namespace: spaceBinding.Namespace, Name: spaceBinding.Spec.MasterUserRecord} - mur := &toolchainv1alpha1.MasterUserRecord{} - if err := r.Client.Get(ctx, murName, mur); err != nil { - if errors.IsNotFound(err) { - // mur was deleted - return reconcile.Result{}, nil - } - // error while reading MUR - return ctrl.Result{}, errs.Wrapf(err, "unable to get the bound MUR") - } - - // error when mur has no owner label (should not happen in prod) - if _, ok := mur.Labels[toolchainv1alpha1.MasterUserRecordOwnerLabelKey]; !ok { - return ctrl.Result{}, errs.New("mur has no MasterUserRecordOwnerLabelKey set") - } - // error when space has no creator label (should not happen in prod) - if _, ok := space.Labels[toolchainv1alpha1.SpaceCreatorLabelKey]; !ok { - return ctrl.Result{}, errs.New("space has no SpaceCreatorLabelKey set") - } - - // skip workspace creator spacebinding - // the controller will convert only spacebindings created by system admins using the sandbox-cli. - // If the creator label on the space matches the owner label on the MUR then this is the owner of the space - // and the spacebinding should not be migrated. - if space.Labels[toolchainv1alpha1.SpaceCreatorLabelKey] == mur.Labels[toolchainv1alpha1.MasterUserRecordOwnerLabelKey] { - return reconcile.Result{}, nil - } - - // get the spaceRole - spaceRole := spaceBinding.Spec.SpaceRole - - // get member cluster name where the space was provisioned - targetCluster := space.Spec.TargetCluster - memberCluster, memberClusterFound := r.MemberClusters[targetCluster] - if !memberClusterFound { - return ctrl.Result{}, errs.New(fmt.Sprintf("unable to find member cluster: %s", targetCluster)) - } - - // get the home namespace from space - defaultNamespace := getDefaultNamespace(space.Status.ProvisionedNamespaces) - - // construct a SpaceBindingRequest object - sbrName := mur.GetName() + "-" + spaceRole - sbr := &toolchainv1alpha1.SpaceBindingRequest{ - ObjectMeta: metav1.ObjectMeta{ - Name: sbrName, - Namespace: defaultNamespace, - }, - } - - result, err := controllerutil.CreateOrUpdate(ctx, memberCluster.Client, sbr, func() error { - sbr.Spec = toolchainv1alpha1.SpaceBindingRequestSpec{ - MasterUserRecord: mur.GetName(), - SpaceRole: spaceRole, - } - return nil - }) - - if err != nil { - // something happened when we tried to read or write the sbr - return ctrl.Result{}, errs.Wrapf(err, "Failed to create or update space binding request %v", sbrName) - } - - if result == controllerutil.OperationResultCreated { - // let's requeue after we created the SBR, so that in next loop the migrated SpaceBinding object will be deleted - return ctrl.Result{Requeue: true}, nil - } - // if the SBR was found (was created from the previous reconcile loop), we can now delete the SpaceBinding object - if err := r.Client.Delete(ctx, spaceBinding); err != nil && !errors.IsNotFound(err) { - return ctrl.Result{}, errs.Wrapf(err, "unable to delete the SpaceBinding") - } - - return ctrl.Result{}, nil -} - -func getDefaultNamespace(provisionedNamespaces []toolchainv1alpha1.SpaceNamespace) string { - for _, namespaceObj := range provisionedNamespaces { - if namespaceObj.Type == "default" { - return namespaceObj.Name - } - } - return "" -} - -func hasSpaceBindingRequest(spaceBinding *toolchainv1alpha1.SpaceBinding) bool { - _, sbrNameFound := spaceBinding.Labels[toolchainv1alpha1.SpaceBindingRequestLabelKey] - return sbrNameFound -} diff --git a/controllers/spacebindingrequestmigration/spacebindingrequest_migration_controller_test.go b/controllers/spacebindingrequestmigration/spacebindingrequest_migration_controller_test.go deleted file mode 100644 index 0fe58206a..000000000 --- a/controllers/spacebindingrequestmigration/spacebindingrequest_migration_controller_test.go +++ /dev/null @@ -1,335 +0,0 @@ -package spacebindingrequestmigration_test - -import ( - "context" - "fmt" - "testing" - - toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" - "github.com/codeready-toolchain/host-operator/controllers/spacebindingrequestmigration" - "github.com/codeready-toolchain/host-operator/pkg/apis" - "github.com/codeready-toolchain/host-operator/pkg/cluster" - . "github.com/codeready-toolchain/host-operator/test" - spacebindingtest "github.com/codeready-toolchain/host-operator/test/spacebinding" - spacebindingrequesttest "github.com/codeready-toolchain/host-operator/test/spacebindingrequest" - commoncluster "github.com/codeready-toolchain/toolchain-common/pkg/cluster" - "github.com/codeready-toolchain/toolchain-common/pkg/test" - "github.com/codeready-toolchain/toolchain-common/pkg/test/masteruserrecord" - spacetest "github.com/codeready-toolchain/toolchain-common/pkg/test/space" - "github.com/stretchr/testify/require" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/kubernetes/scheme" - runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" - logf "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/log/zap" - "sigs.k8s.io/controller-runtime/pkg/reconcile" -) - -func TestMigrateSpaceBindingToSBR(t *testing.T) { - // given - logf.SetLogger(zap.New(zap.UseDevMode(true))) - err := apis.AddToScheme(scheme.Scheme) - require.NoError(t, err) - janeSpace := spacetest.NewSpace(test.HostOperatorNs, "jane", - spacetest.WithSpecTargetCluster("member-1"), - spacetest.WithStatusProvisionedNamespaces([]toolchainv1alpha1.SpaceNamespace{{ - Name: "jane-tenant", - Type: "default", - }}), - spacetest.WithLabel(toolchainv1alpha1.SpaceCreatorLabelKey, "jane"), - ) - - janeMur := masteruserrecord.NewMasterUserRecord(t, "jane", masteruserrecord.WithLabel(toolchainv1alpha1.MasterUserRecordOwnerLabelKey, "jane")) - sbForCreator := spacebindingtest.NewSpaceBinding(janeMur.Name, janeSpace.Name, "admin", janeMur.Name) - // we have a user which was added to the space via sandbox-cli - johnMur := masteruserrecord.NewMasterUserRecord(t, "john", masteruserrecord.WithLabel(toolchainv1alpha1.MasterUserRecordOwnerLabelKey, "john")) - sbForJohn := spacebindingtest.NewSpaceBinding(johnMur.Name, janeSpace.Name, "admin", janeMur.GetName()) - t.Run("success", func(t *testing.T) { - - t.Run("create sbr for sb added via sandbox-cli", func(t *testing.T) { - // given - member1 := NewMemberClusterWithClient(test.NewFakeClient(t), "member-1", corev1.ConditionTrue) - hostClient := test.NewFakeClient(t, janeSpace, janeMur, johnMur, sbForCreator, sbForJohn) - ctrl := newReconciler(t, hostClient, member1) - - // when - res, err := ctrl.Reconcile(context.TODO(), requestFor(sbForJohn)) - - // then - require.NoError(t, err) - require.True(t, res.Requeue) // requeue should be triggered once SBR is created - // spaceBindingRequest with expected name, namespace and spec should be created - spacebindingrequesttest.AssertThatSpaceBindingRequest(t, "jane-tenant", johnMur.Name+"-admin", member1.Client). - HasSpecSpaceRole("admin"). - HasSpecMasterUserRecord(johnMur.Name) - // the migrated spacebinding is still there, it will be deleted at the next reconcile loop - spacebindingtest.AssertThatSpaceBinding(t, test.HostOperatorNs, johnMur.Name, janeSpace.Name, hostClient). - Exists() - // the spacebinding for the space creator is still there - spacebindingtest.AssertThatSpaceBinding(t, test.HostOperatorNs, janeMur.Name, janeSpace.Name, hostClient). - Exists() - - t.Run("the next reconcile deletes the migrated spacebinding", func(t *testing.T) { - // when - res, err := ctrl.Reconcile(context.TODO(), requestFor(sbForJohn)) - - // then - require.NoError(t, err) - require.False(t, res.Requeue) // no requeue this time - // the migrated spacebinding was deleted - spacebindingtest.AssertThatSpaceBinding(t, test.HostOperatorNs, johnMur.Name, janeSpace.Name, hostClient). - DoesNotExist() - // spaceBindingRequest with expected name, namespace and spec is still there - spacebindingrequesttest.AssertThatSpaceBindingRequest(t, "jane-tenant", johnMur.Name+"-admin", member1.Client). - HasSpecSpaceRole("admin"). - HasSpecMasterUserRecord(johnMur.Name) - // the spacebinding for the space creator is still there - spacebindingtest.AssertThatSpaceBinding(t, test.HostOperatorNs, janeMur.Name, janeSpace.Name, hostClient). - Exists() - }) - }) - - t.Run("skip space creator spacebinding ", func(t *testing.T) { - // given - // we have the workspace creator spacebinding, it should not be migrated to SpaceBindingRequest - member1 := NewMemberClusterWithClient(test.NewFakeClient(t), "member-1", corev1.ConditionTrue) - hostClient := test.NewFakeClient(t, janeSpace, janeMur, sbForCreator) - ctrl := newReconciler(t, hostClient, member1) - - // when - _, err = ctrl.Reconcile(context.TODO(), requestFor(sbForCreator)) - - // then - require.NoError(t, err) - // the spacebinding for the space creator is still there - spacebindingtest.AssertThatSpaceBinding(t, test.HostOperatorNs, janeMur.Name, janeSpace.Name, hostClient). - Exists() - // the spaceBindingRequest wasn't created - spacebindingrequesttest.AssertThatSpaceBindingRequest(t, "jane-tenant", janeMur.Name+"-admin", member1.Client). - DoesNotExist() - }) - - t.Run("space creator name is different than mur name", func(t *testing.T) { - // given - batmanSpace := spacetest.NewSpace(test.HostOperatorNs, "batman", - spacetest.WithStatusTargetCluster("member-1"), - spacetest.WithStatusProvisionedNamespaces([]toolchainv1alpha1.SpaceNamespace{{ - Name: "batman-tenant", - Type: "default", - }}), - spacetest.WithLabel(toolchainv1alpha1.SpaceCreatorLabelKey, "batman"), - ) - // mur name differs from the space creator label - // but the usersignup matches the space creator name - batmanMur := masteruserrecord.NewMasterUserRecord(t, "batman123", masteruserrecord.WithLabel(toolchainv1alpha1.MasterUserRecordOwnerLabelKey, "batman")) - sbForBatman := spacebindingtest.NewSpaceBinding(batmanMur.GetName(), batmanSpace.GetName(), "admin", "batman") - member1 := NewMemberClusterWithClient(test.NewFakeClient(t), "member-1", corev1.ConditionTrue) - hostClient := test.NewFakeClient(t, batmanSpace, batmanMur, sbForBatman) - ctrl := newReconciler(t, hostClient, member1) - - // when - _, err = ctrl.Reconcile(context.TODO(), requestFor(sbForBatman)) - - // then - require.NoError(t, err) - // the spacebinding for the space creator is still there - spacebindingtest.AssertThatSpaceBinding(t, test.HostOperatorNs, batmanMur.Name, batmanSpace.Name, hostClient). - Exists() - // the spaceBindingRequest wasn't created - spacebindingrequesttest.AssertThatSpaceBindingRequest(t, "batman-tenant", batmanMur.Name+"-admin", member1.Client). - DoesNotExist() - }) - - t.Run("space not found", func(t *testing.T) { - // given - member1 := NewMemberClusterWithClient(test.NewFakeClient(t), "member-1", corev1.ConditionTrue) - // let's not load the space object - hostClient := test.NewFakeClient(t, janeMur, johnMur, sbForCreator, sbForJohn) - ctrl := newReconciler(t, hostClient, member1) - - // when - _, err = ctrl.Reconcile(context.TODO(), requestFor(sbForJohn)) - - // then - require.NoError(t, err) - // the spacebinding for the space creator is still there - spacebindingtest.AssertThatSpaceBinding(t, test.HostOperatorNs, janeMur.Name, janeSpace.Name, hostClient). - Exists() - // the spacebinding for john user is still there - spacebindingtest.AssertThatSpaceBinding(t, test.HostOperatorNs, johnMur.Name, janeSpace.Name, hostClient). - Exists() - // the spaceBindingRequest wasn't created - spacebindingrequesttest.AssertThatSpaceBindingRequest(t, "jane-tenant", johnMur.Name+"-admin", member1.Client). - DoesNotExist() - }) - - t.Run("mur not found", func(t *testing.T) { - // given - member1 := NewMemberClusterWithClient(test.NewFakeClient(t), "member-1", corev1.ConditionTrue) - // let's not load the mur object - hostClient := test.NewFakeClient(t, janeMur, janeSpace, sbForCreator, sbForJohn) - ctrl := newReconciler(t, hostClient, member1) - - // when - _, err = ctrl.Reconcile(context.TODO(), requestFor(sbForJohn)) - - // then - require.NoError(t, err) - // the spacebinding for the space creator is still there - spacebindingtest.AssertThatSpaceBinding(t, test.HostOperatorNs, janeMur.Name, janeSpace.Name, hostClient). - Exists() - // the spacebinding for john user is still there - spacebindingtest.AssertThatSpaceBinding(t, test.HostOperatorNs, johnMur.Name, janeSpace.Name, hostClient). - Exists() - // the spaceBindingRequest wasn't created - spacebindingrequesttest.AssertThatSpaceBindingRequest(t, "jane-tenant", johnMur.Name+"-admin", member1.Client). - DoesNotExist() - }) - - t.Run("spacebinding has spacebindingrequest", func(t *testing.T) { - // given - // the spacebinding has a spacebindingrequest - sbrForJohn := spacebindingrequesttest.NewSpaceBindingRequest("john-admin", "jane-tenant", spacebindingrequesttest.WithLabel(toolchainv1alpha1.SpaceCreatorLabelKey, "somevalue")) - sbForJohnWithSBR := spacebindingtest.NewSpaceBinding(johnMur.Name, janeSpace.Name, "admin", janeMur.GetName(), spacebindingtest.WithSpaceBindingRequest(sbrForJohn)) - member1 := NewMemberClusterWithClient(test.NewFakeClient(t, sbrForJohn), "member-1", corev1.ConditionTrue) - hostClient := test.NewFakeClient(t, janeMur, janeSpace, johnMur, sbForJohnWithSBR) - ctrl := newReconciler(t, hostClient, member1) - - // when - _, err = ctrl.Reconcile(context.TODO(), requestFor(sbForJohnWithSBR)) - - // then - require.NoError(t, err) - // the spacebinding for john user is still there - spacebindingtest.AssertThatSpaceBinding(t, test.HostOperatorNs, johnMur.Name, janeSpace.Name, hostClient). - Exists() - // the spaceBindingRequest is unchanged - // no migration label as creator - spacebindingrequesttest.AssertThatSpaceBindingRequest(t, "jane-tenant", johnMur.Name+"-admin", member1.Client). - HasLabelWithValue(toolchainv1alpha1.SpaceCreatorLabelKey, "somevalue"). - Exists() - }) - - t.Run("spacebinding is being deleted", func(t *testing.T) { - // given - member1 := NewMemberClusterWithClient(test.NewFakeClient(t), "member-1", corev1.ConditionTrue) - // the spacebinding is being deleted - sbForJohn := spacebindingtest.NewSpaceBinding(johnMur.Name, janeSpace.Name, "admin", janeMur.GetName(), spacebindingtest.WithDeletionTimestamp()) - hostClient := test.NewFakeClient(t, janeMur, janeSpace, sbForCreator, johnMur, sbForJohn) - ctrl := newReconciler(t, hostClient, member1) - - // when - _, err = ctrl.Reconcile(context.TODO(), requestFor(sbForJohn)) - - // then - require.NoError(t, err) - // the spacebinding for the space creator is still there - spacebindingtest.AssertThatSpaceBinding(t, test.HostOperatorNs, janeMur.Name, janeSpace.Name, hostClient). - Exists() - // the spacebinding for john user is still there - spacebindingtest.AssertThatSpaceBinding(t, test.HostOperatorNs, johnMur.Name, janeSpace.Name, hostClient). - Exists() - // the spaceBindingRequest wasn't created - spacebindingrequesttest.AssertThatSpaceBindingRequest(t, "jane-tenant", johnMur.Name+"-admin", member1.Client). - DoesNotExist() - }) - }) - - t.Run("error", func(t *testing.T) { - t.Run("unable to get spacebinding", func(t *testing.T) { - hostClient := test.NewFakeClient(t, sbForCreator) - hostClient.MockGet = mockGetSpaceBindingFail(hostClient) - ctrl := newReconciler(t, hostClient) - - // when - _, err := ctrl.Reconcile(context.TODO(), requestFor(sbForCreator)) - - // then - // space binding request should not be there - require.EqualError(t, err, "unable to get spacebinding: mock error") - }) - - t.Run("member cluster not found", func(t *testing.T) { - spaceWithInvalidTargetCluster := spacetest.NewSpace(test.HostOperatorNs, "jane", - spacetest.WithSpecTargetCluster("invalid"), - spacetest.WithLabel(toolchainv1alpha1.SpaceCreatorLabelKey, "jane"), - ) - sb := spacebindingtest.NewSpaceBinding(johnMur.Name, spaceWithInvalidTargetCluster.Name, "admin", janeMur.Name) - hostClient := test.NewFakeClient(t, sb, spaceWithInvalidTargetCluster, johnMur) - ctrl := newReconciler(t, hostClient) - - // when - _, err := ctrl.Reconcile(context.TODO(), requestFor(sb)) - - // then - // space binding request should not be there - require.EqualError(t, err, "unable to find member cluster: invalid") - }) - - t.Run("mur has no owner label", func(t *testing.T) { - murWithNoOwnership := masteruserrecord.NewMasterUserRecord(t, "jane") - sb := spacebindingtest.NewSpaceBinding(murWithNoOwnership.Name, janeSpace.Name, "admin", janeMur.Name) - hostClient := test.NewFakeClient(t, sb, janeSpace, murWithNoOwnership) - ctrl := newReconciler(t, hostClient) - - // when - _, err := ctrl.Reconcile(context.TODO(), requestFor(sb)) - - // then - // space binding request should not be there - require.EqualError(t, err, "mur has no MasterUserRecordOwnerLabelKey set") - }) - - }) -} - -func newReconciler(t *testing.T, hostCl runtimeclient.Client, memberClusters ...*commoncluster.CachedToolchainCluster) *spacebindingrequestmigration.Reconciler { - s := scheme.Scheme - err := apis.AddToScheme(s) - require.NoError(t, err) - - clusters := map[string]cluster.Cluster{} - for _, c := range memberClusters { - clusters[c.Name] = cluster.Cluster{ - Config: &commoncluster.Config{ - Type: commoncluster.Member, - OperatorNamespace: c.OperatorNamespace, - OwnerClusterName: test.MemberClusterName, - }, - Client: c.Client, - } - } - return &spacebindingrequestmigration.Reconciler{ - Client: hostCl, - Scheme: s, - Namespace: test.HostOperatorNs, - MemberClusters: clusters, - } -} - -func requestFor(s *toolchainv1alpha1.SpaceBinding) reconcile.Request { - if s != nil { - return reconcile.Request{ - NamespacedName: types.NamespacedName{ - Namespace: s.Namespace, - Name: s.Name, - }, - } - } - return reconcile.Request{ - NamespacedName: types.NamespacedName{ - Namespace: test.HostOperatorNs, - Name: "unknown", - }, - } -} - -func mockGetSpaceBindingFail(cl runtimeclient.Client) func(ctx context.Context, key runtimeclient.ObjectKey, obj runtimeclient.Object, opts ...runtimeclient.GetOption) error { - return func(ctx context.Context, key runtimeclient.ObjectKey, obj runtimeclient.Object, opts ...runtimeclient.GetOption) error { - if _, ok := obj.(*toolchainv1alpha1.SpaceBinding); ok { - return fmt.Errorf("mock error") - } - return cl.Get(ctx, key, obj, opts...) - } -} diff --git a/controllers/spacebindingrequestmigration/spacebindingrequest_spacebinding_mapper.go b/controllers/spacebindingrequestmigration/spacebindingrequest_spacebinding_mapper.go deleted file mode 100644 index 051ad3c07..000000000 --- a/controllers/spacebindingrequestmigration/spacebindingrequest_spacebinding_mapper.go +++ /dev/null @@ -1,44 +0,0 @@ -package spacebindingrequestmigration - -import ( - "context" - - toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" - ctrl "sigs.k8s.io/controller-runtime" - - "k8s.io/apimachinery/pkg/types" - runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/reconcile" -) - -// MapSpaceBindingRequestToSpaceBinding returns an event for the spacebinding that owns. -func MapSpaceBindingRequestToSpaceBinding(cl runtimeclient.Client, watchNamespace string) func(spaceBindingRequest runtimeclient.Object) []reconcile.Request { - mapperLog := ctrl.Log.WithName("SpaceBindingRequestToSpaceBinding") - return func(obj runtimeclient.Object) []reconcile.Request { - spaceBindings := &toolchainv1alpha1.SpaceBindingList{} - err := cl.List(context.TODO(), spaceBindings, - runtimeclient.InNamespace(watchNamespace), - runtimeclient.MatchingLabels{ - toolchainv1alpha1.SpaceBindingRequestLabelKey: obj.GetName(), - toolchainv1alpha1.SpaceBindingRequestNamespaceLabelKey: obj.GetNamespace(), - }) - if err != nil { - mapperLog.Error(err, "cannot list spacebindings") - return []reconcile.Request{} - } - if len(spaceBindings.Items) == 0 { - return []reconcile.Request{} - } - - req := make([]reconcile.Request, len(spaceBindings.Items)) - for i, item := range spaceBindings.Items { - req[i] = reconcile.Request{ - NamespacedName: types.NamespacedName{ - Namespace: item.Namespace, - Name: item.Name, - }, - } - } - return req - } -} diff --git a/controllers/spacebindingrequestmigration/spacebindingrequest_spacebinding_mapper_test.go b/controllers/spacebindingrequestmigration/spacebindingrequest_spacebinding_mapper_test.go deleted file mode 100644 index b9e70ae3a..000000000 --- a/controllers/spacebindingrequestmigration/spacebindingrequest_spacebinding_mapper_test.go +++ /dev/null @@ -1,47 +0,0 @@ -package spacebindingrequestmigration_test - -import ( - "testing" - - "github.com/codeready-toolchain/api/api/v1alpha1" - "github.com/codeready-toolchain/host-operator/controllers/spacebindingrequestmigration" - sb "github.com/codeready-toolchain/host-operator/test/spacebinding" - spacebindingrequesttest "github.com/codeready-toolchain/host-operator/test/spacebindingrequest" - "github.com/codeready-toolchain/toolchain-common/pkg/test" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "k8s.io/apimachinery/pkg/types" - "sigs.k8s.io/controller-runtime/pkg/reconcile" -) - -func TestMapSpaceBindingRequestToSpaceBinding(t *testing.T) { - // given - restore := test.SetEnvVarAndRestore(t, "WATCH_NAMESPACE", test.HostOperatorNs) - defer restore() - spaceBindingRequest := spacebindingrequesttest.NewSpaceBindingRequest("mySpaceBindingRequest", "jane") - // following spaceBinding has a spaceBindingRequest associated - spaceBinding := sb.NewSpaceBinding("jane", "jane", "admin", "signupAdmin") - spaceBinding.Labels[v1alpha1.SpaceBindingRequestLabelKey] = spaceBindingRequest.Name - spaceBinding.Labels[v1alpha1.SpaceBindingRequestNamespaceLabelKey] = spaceBindingRequest.Namespace - - cl := test.NewFakeClient(t, spaceBinding) - - t.Run("should return SpaceBinding requests for SpaceBindingRequest", func(t *testing.T) { - // when - requests := spacebindingrequestmigration.MapSpaceBindingRequestToSpaceBinding(cl, test.HostOperatorNs)(spaceBindingRequest) - - // then - require.Len(t, requests, 1) - assert.Contains(t, requests, newRequest(spaceBinding.Name)) - }) - -} - -func newRequest(name string) reconcile.Request { - return reconcile.Request{ - NamespacedName: types.NamespacedName{ - Namespace: test.HostOperatorNs, - Name: name, - }, - } -} diff --git a/main.go b/main.go index aec7519a1..8e97464a7 100644 --- a/main.go +++ b/main.go @@ -16,7 +16,6 @@ import ( "github.com/codeready-toolchain/host-operator/controllers/space" "github.com/codeready-toolchain/host-operator/controllers/spacebindingcleanup" "github.com/codeready-toolchain/host-operator/controllers/spacebindingrequest" - "github.com/codeready-toolchain/host-operator/controllers/spacebindingrequestmigration" "github.com/codeready-toolchain/host-operator/controllers/spacecleanup" "github.com/codeready-toolchain/host-operator/controllers/spacecompletion" "github.com/codeready-toolchain/host-operator/controllers/spacerequest" @@ -324,19 +323,6 @@ func main() { // nolint:gocyclo os.Exit(1) } } - // TEMPORARY controller that converts spacebindings created via sandbox-cli into spacebinding requests - // once the migration effort is completed , the controller can be disabled and deleted. - if crtConfig.SpaceConfig().SpaceBindingRequestIsEnabled() { - if err = (&spacebindingrequestmigration.Reconciler{ - Client: mgr.GetClient(), - Namespace: namespace, - MemberClusters: clusterScopedMemberClusters, - Scheme: mgr.GetScheme(), - }).SetupWithManager(mgr, clusterScopedMemberClusters); err != nil { - setupLog.Error(err, "unable to create controller", "controller", "SpaceBindingRequestMigration") - os.Exit(1) - } - } if err = (&space.Reconciler{ Client: mgr.GetClient(), Namespace: namespace, From ff377e2f7593350f2b3ebe6693fc006ef45f519b Mon Sep 17 00:00:00 2001 From: Devtools Date: Fri, 24 Jan 2025 11:52:35 +0100 Subject: [PATCH 2/9] Create new TTR when tiertemplate changes --- .../nstemplatetier_controller.go | 17 ++- .../nstemplatetier_controller_test.go | 137 +++++++++++++++--- test/nstemplatetier/assertion.go | 7 +- 3 files changed, 134 insertions(+), 27 deletions(-) diff --git a/controllers/nstemplatetier/nstemplatetier_controller.go b/controllers/nstemplatetier/nstemplatetier_controller.go index fe27f36d2..26cf66b73 100644 --- a/controllers/nstemplatetier/nstemplatetier_controller.go +++ b/controllers/nstemplatetier/nstemplatetier_controller.go @@ -3,6 +3,7 @@ package nstemplatetier import ( "context" "fmt" + "reflect" "time" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" @@ -146,15 +147,25 @@ func (r *Reconciler) ensureTTRforTemplate(ctx context.Context, nsTmplTier *toolc return false, "", err } } - // TODO compare TierTemplate content with TTR content // if the TierTemplate has changes we need to create new TTR + // compare TierTemplate content with TTR content + if !reflect.DeepEqual(tierTemplate.Spec.TemplateObjects, tierTemplateRevision.Spec.TemplateObjects) { + ttrName, err := r.createNewTierTemplateRevision(ctx, nsTmplTier, tierTemplate) + return true, ttrName, err + } + // even when there are changes to the NSTemplateTiers parameters we need to create a new TTR + if !reflect.DeepEqual(nsTmplTier.Spec.Parameters, tierTemplateRevision.Spec.Parameters) { + ttrName, err := r.createNewTierTemplateRevision(ctx, nsTmplTier, tierTemplate) + return true, ttrName, err + } + + // nothing changed + return false, tierTemplateRevisionName, nil } else { // no revision was set for this TierTemplate CR, let's create a TTR for it ttrName, err := r.createNewTierTemplateRevision(ctx, nsTmplTier, tierTemplate) return true, ttrName, err } - // nothing changed - return false, "", nil } func (r *Reconciler) createNewTierTemplateRevision(ctx context.Context, nsTmplTier *toolchainv1alpha1.NSTemplateTier, tierTemplate *toolchainv1alpha1.TierTemplate) (string, error) { diff --git a/controllers/nstemplatetier/nstemplatetier_controller_test.go b/controllers/nstemplatetier/nstemplatetier_controller_test.go index 1835c8e2b..71df9bc9d 100644 --- a/controllers/nstemplatetier/nstemplatetier_controller_test.go +++ b/controllers/nstemplatetier/nstemplatetier_controller_test.go @@ -129,29 +129,7 @@ func TestReconcile(t *testing.T) { t.Run("using TTR as revisions", func(t *testing.T) { // initialize tier templates with templateObjects field populated // for simplicity we initialize all of them with the same objects - var crq = unstructured.Unstructured{Object: map[string]interface{}{ - "kind": "ClusterResourceQuota", - "metadata": map[string]interface{}{ - "name": "for-{{.SPACE_NAME}}-deployments", - }, - "spec": map[string]interface{}{ - "quota": map[string]interface{}{ - "hard": map[string]interface{}{ - "count/deploymentconfigs.apps": "{{.DEPLOYMENT_QUOTA}}", - "count/deployments.apps": "{{.DEPLOYMENT_QUOTA}}", - "count/pods": "600", - }, - }, - "selector": map[string]interface{}{ - "annotations": map[string]interface{}{}, - "labels": map[string]interface{}{ - "matchLabels": map[string]interface{}{ - "toolchain.dev.openshift.com/space": "'{{.SPACE_NAME}}'", - }, - }, - }, - }, - }} + crq := newTestCRQ("600") t.Run("add revisions when they are missing ", func(t *testing.T) { // given tierTemplates := initTierTemplates(t, withTemplateObjects(crq), base1nsTier.Name) @@ -309,6 +287,119 @@ func TestReconcile(t *testing.T) { } +func TestUpdateNSTemplateTier(t *testing.T) { + logf.SetLogger(zap.New(zap.UseDevMode(true))) + // given + base1nsTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, + // the tiertemplate revision CR should have a copy of those parameters + tiertest.WithParameter("DEPLOYMENT_QUOTA", "60"), + ) + tierTemplatesRefs := []string{ + "base1ns-admin-123456new", "base1ns-clusterresources-123456new", "base1ns-code-123456new", "base1ns-dev-123456new", "base1ns-edit-123456new", "base1ns-stage-123456new", "base1ns-viewer-123456new", + } + + // initialize tier templates with templateObjects field populated + // for simplicity we initialize all of them with the same objects + crq := newTestCRQ("600") + // given + tierTemplates := initTierTemplates(t, withTemplateObjects(crq), base1nsTier.Name) + r, req, cl := prepareReconcile(t, base1nsTier.Name, append(tierTemplates, base1nsTier)...) + // when + res, err := r.Reconcile(context.TODO(), req) + // then + require.NoError(t, err) + require.Equal(t, reconcile.Result{RequeueAfter: time.Second}, res) // explicit requeue after the adding revisions in `status.revisions` + // check that revisions field was populated + oldNSTemplateTier := tiertest.AssertThatNSTemplateTier(t, "base1ns", cl). + HasStatusTierTemplateRevisions(tierTemplatesRefs).Tier() + + t.Run("revision field is set but TierTemplate content has changed, new ttr should be created", func(t *testing.T) { + // given + // the NSTemplateTier already has the revisions from parent test, + // we update the cluster resource tier template content by setting a higher number of pods + tierTemplate := &toolchainv1alpha1.TierTemplate{} + err = cl.Get(context.TODO(), types.NamespacedName{Namespace: operatorNamespace, Name: "base1ns-clusterresources-123456new"}, tierTemplate) + require.NoError(t, err) + updatedCRQ := newTestCRQ("700") + tierTemplate.Spec.TemplateObjects = withTemplateObjects(updatedCRQ) + err = cl.Update(context.TODO(), tierTemplate) + require.NoError(t, err) + + // when + res, err = r.Reconcile(context.TODO(), req) + + // then + require.NoError(t, err) + // revisions values should be different compared to the previous ones + newNSTmplTier := tiertest.AssertThatNSTemplateTier(t, "base1ns", cl). + HasStatusTierTemplateRevisions(tierTemplatesRefs).Tier() + require.NotEqual(t, oldNSTemplateTier.Status.Revisions, newNSTmplTier.Status.Revisions) + // there should be one new ttr created by the change in the TierTemplate + tiertemplaterevision.AssertThatTTRs(t, cl, newNSTmplTier.GetNamespace()). + NumberOfPresentCRs(len(tierTemplatesRefs) + 1) + + t.Run("new ttr should be created also when parameters are changed in the NSTemplateTier", func(t *testing.T) { + // given + // the NSTemplateTier already has the revisions from previous test, + // but we update the parameters in the NSTemplateTier + // let's increase the quota parameter + newNSTmplTier.Spec.Parameters = []toolchainv1alpha1.Parameter{{Name: "DEPLOYMENT_QUOTA", Value: "100"}} + err = cl.Update(context.TODO(), newNSTmplTier) + require.NoError(t, err) + + // when + res, err = r.Reconcile(context.TODO(), req) + + // then + require.NoError(t, err) + // revisions values should be different compared to the previous ones + // ensure the old revisions are not there anymore + newNSTmplTier = tiertest.AssertThatNSTemplateTier(t, "base1ns", cl). + HasStatusTierTemplateRevisions(tierTemplatesRefs).Tier() + require.NotEqual(t, oldNSTemplateTier.Status.Revisions, newNSTmplTier.Status.Revisions) + // check if the change was propagated to the ttrs + tiertemplaterevision.AssertThatTTRs(t, cl, newNSTmplTier.GetNamespace()). + // a new set of TTRs should be created due the parameter change in the NSTemplateTier + // thus we now have double the initial ttrs plus the one created in the parent test. + NumberOfPresentCRs((len(tierTemplatesRefs) * 2) + 1). + // check that the NSTemplateTier parameter was propagated to all the TTRs from the NSTemplateTier + ForEach(func(ttr *toolchainv1alpha1.TierTemplateRevision) { + // if the ttr is being used by the NSTemplateTier we compare the parameters + if ttrInUse, found := newNSTmplTier.Status.Revisions[ttr.GetLabels()[toolchainv1alpha1.TemplateRefLabelKey]]; found && ttrInUse == ttr.GetName() { + assert.Equal(t, newNSTmplTier.Spec.Parameters, ttr.Spec.Parameters) + } + }) + }) + }) +} + +func newTestCRQ(podsCount string) unstructured.Unstructured { + var crq = unstructured.Unstructured{Object: map[string]interface{}{ + "kind": "ClusterResourceQuota", + "metadata": map[string]interface{}{ + "name": "for-{{.SPACE_NAME}}-deployments", + }, + "spec": map[string]interface{}{ + "quota": map[string]interface{}{ + "hard": map[string]interface{}{ + "count/deploymentconfigs.apps": "{{.DEPLOYMENT_QUOTA}}", + "count/deployments.apps": "{{.DEPLOYMENT_QUOTA}}", + "count/pods": podsCount, + }, + }, + "selector": map[string]interface{}{ + "annotations": map[string]interface{}{}, + "labels": map[string]interface{}{ + "matchLabels": map[string]interface{}{ + "toolchain.dev.openshift.com/space": "'{{.SPACE_NAME}}'", + }, + }, + }, + }, + }} + return crq +} + // initTierTemplates creates the TierTemplates objects for the base1ns tier func initTierTemplates(t *testing.T, withTemplateObjects []runtime.RawExtension, tierName string) []runtimeclient.Object { s := scheme.Scheme diff --git a/test/nstemplatetier/assertion.go b/test/nstemplatetier/assertion.go index 93bfae4be..48f5d7e27 100644 --- a/test/nstemplatetier/assertion.go +++ b/test/nstemplatetier/assertion.go @@ -19,6 +19,10 @@ type Assertion struct { t test.T } +func (a *Assertion) Tier() *toolchainv1alpha1.NSTemplateTier { + return a.tier +} + func (a *Assertion) loadResource() error { tier := &toolchainv1alpha1.NSTemplateTier{} err := a.client.Get(context.TODO(), a.namespacedName, tier) @@ -42,8 +46,9 @@ func (a *Assertion) HasStatusTierTemplateRevisions(revisions []string) *Assertio // check that each TierTemplate REF has a TierTemplateRevision set for _, tierTemplateRef := range revisions { require.NotNil(a.t, a.tier.Status.Revisions) - _, ok := a.tier.Status.Revisions[tierTemplateRef] + value, ok := a.tier.Status.Revisions[tierTemplateRef] require.True(a.t, ok) + require.NotEmpty(a.t, value) } return a } From e14adf7fafa9215b97984864363be6b06b7c0698 Mon Sep 17 00:00:00 2001 From: Francisc Munteanu Date: Thu, 30 Jan 2025 13:58:24 +0100 Subject: [PATCH 3/9] Update controllers/nstemplatetier/nstemplatetier_controller.go Co-authored-by: Lukas Krejci --- .../nstemplatetier_controller.go | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/controllers/nstemplatetier/nstemplatetier_controller.go b/controllers/nstemplatetier/nstemplatetier_controller.go index 1e1c84a94..20f9bd4c7 100644 --- a/controllers/nstemplatetier/nstemplatetier_controller.go +++ b/controllers/nstemplatetier/nstemplatetier_controller.go @@ -153,19 +153,19 @@ func (r *Reconciler) ensureTTRforTemplate(ctx context.Context, nsTmplTier *toolc } } // if the TierTemplate has changes we need to create new TTR - // compare TierTemplate content with TTR content - if !reflect.DeepEqual(tierTemplate.Spec.TemplateObjects, tierTemplateRevision.Spec.TemplateObjects) { - ttrName, err := r.createNewTierTemplateRevision(ctx, nsTmplTier, tierTemplate) - return true, ttrName, err + // if the TierTemplate objects are the same as they are in the TTR + // and the parameters are the same, too, we don't have to create + // a new revision. + if reflect.DeepEqual(tierTemplate.Spec.TemplateObjects, tierTemplateRevision.Spec.TemplateObjects) && reflect.DeepEqual(nsTmplTier.Spec.Parameters, tierTemplateRevision.Spec.Parameters) { + return false, tierTemplateRevisionName, nil } - // even when there are changes to the NSTemplateTiers parameters we need to create a new TTR - if !reflect.DeepEqual(nsTmplTier.Spec.Parameters, tierTemplateRevision.Spec.Parameters) { - ttrName, err := r.createNewTierTemplateRevision(ctx, nsTmplTier, tierTemplate) - return true, ttrName, err + + // there are some changes, a new TTR is needed + ttrName, err := r.createNewTierTemplateRevision(ctx, nsTmplTier, tierTemplate) + if err != nil { + return false, "", err } - - // nothing changed - return false, tierTemplateRevisionName, nil + return true, ttrName, nil } else { // no revision was set for this TierTemplate CR, let's create a TTR for it ttrName, err := r.createNewTierTemplateRevision(ctx, nsTmplTier, tierTemplate) From 7a94bf4f96c64b0537117ce6e0715fcc108ce5c3 Mon Sep 17 00:00:00 2001 From: Devtools Date: Thu, 30 Jan 2025 16:28:44 +0100 Subject: [PATCH 4/9] simplify code --- .../nstemplatetier_controller.go | 62 ++++++++++--------- 1 file changed, 32 insertions(+), 30 deletions(-) diff --git a/controllers/nstemplatetier/nstemplatetier_controller.go b/controllers/nstemplatetier/nstemplatetier_controller.go index 20f9bd4c7..9a37b3363 100644 --- a/controllers/nstemplatetier/nstemplatetier_controller.go +++ b/controllers/nstemplatetier/nstemplatetier_controller.go @@ -137,40 +137,42 @@ func (r *Reconciler) ensureTTRforTemplate(ctx context.Context, nsTmplTier *toolc return false, "", nil } - if tierTemplateRevisionName, found := nsTmplTier.Status.Revisions[tierTemplate.GetName()]; found { - logger.Info("TTR set in the status.revisions for tiertemplate", "tierTemplate.Name", tierTemplate.GetName(), "ttr.Name", tierTemplateRevisionName) - var tierTemplateRevision toolchainv1alpha1.TierTemplateRevision - if err := r.Client.Get(ctx, types.NamespacedName{Namespace: nsTmplTier.GetNamespace(), Name: tierTemplateRevisionName}, &tierTemplateRevision); err != nil { - if errors.IsNotFound(err) { - // no tierTemplateRevision CR was found, - logger.Info("TTR CR not found", "tierTemplateRevision.Name", tierTemplateRevisionName) - // let's create one - ttrName, err := r.createNewTierTemplateRevision(ctx, nsTmplTier, tierTemplate) - return true, ttrName, err - } else { - // something wrong happened - return false, "", err - } - } - // if the TierTemplate has changes we need to create new TTR - // if the TierTemplate objects are the same as they are in the TTR - // and the parameters are the same, too, we don't have to create - // a new revision. - if reflect.DeepEqual(tierTemplate.Spec.TemplateObjects, tierTemplateRevision.Spec.TemplateObjects) && reflect.DeepEqual(nsTmplTier.Spec.Parameters, tierTemplateRevision.Spec.Parameters) { - return false, tierTemplateRevisionName, nil - } - - // there are some changes, a new TTR is needed - ttrName, err := r.createNewTierTemplateRevision(ctx, nsTmplTier, tierTemplate) - if err != nil { - return false, "", err - } - return true, ttrName, nil - } else { + tierTemplateRevisionName, found := nsTmplTier.Status.Revisions[tierTemplate.GetName()] + if !found { // no revision was set for this TierTemplate CR, let's create a TTR for it ttrName, err := r.createNewTierTemplateRevision(ctx, nsTmplTier, tierTemplate) return true, ttrName, err } + + logger.Info("TTR set in the status.revisions for tiertemplate", "tierTemplate.Name", tierTemplate.GetName(), "ttr.Name", tierTemplateRevisionName) + var tierTemplateRevision toolchainv1alpha1.TierTemplateRevision + if err := r.Client.Get(ctx, types.NamespacedName{Namespace: nsTmplTier.GetNamespace(), Name: tierTemplateRevisionName}, &tierTemplateRevision); err != nil { + if errors.IsNotFound(err) { + // no tierTemplateRevision CR was found, + logger.Info("TTR CR not found", "tierTemplateRevision.Name", tierTemplateRevisionName) + // let's create one + ttrName, err := r.createNewTierTemplateRevision(ctx, nsTmplTier, tierTemplate) + return true, ttrName, err + } else { + // something wrong happened + return false, "", err + } + } + // if the TierTemplate has changes we need to create new TTR + // if the TierTemplate objects are the same as they are in the TTR + // and the parameters are the same, too, we don't have to create + // a new revision. + if reflect.DeepEqual(tierTemplate.Spec.TemplateObjects, tierTemplateRevision.Spec.TemplateObjects) && reflect.DeepEqual(nsTmplTier.Spec.Parameters, tierTemplateRevision.Spec.Parameters) { + return false, tierTemplateRevisionName, nil + } + + // there are some changes, a new TTR is needed + ttrName, err := r.createNewTierTemplateRevision(ctx, nsTmplTier, tierTemplate) + if err != nil { + return false, "", err + } + return true, ttrName, nil + } func (r *Reconciler) createNewTierTemplateRevision(ctx context.Context, nsTmplTier *toolchainv1alpha1.NSTemplateTier, tierTemplate *toolchainv1alpha1.TierTemplate) (string, error) { From e8b1d8e4d9dbd73632db1385eabfea3e9a6f90ac Mon Sep 17 00:00:00 2001 From: Devtools Date: Tue, 4 Feb 2025 15:07:06 +0100 Subject: [PATCH 5/9] watch for tiertemplates --- controllers/nstemplatetier/mapper.go | 53 +++++++++++ controllers/nstemplatetier/mapper_test.go | 93 +++++++++++++++++++ .../nstemplatetier_controller.go | 3 + 3 files changed, 149 insertions(+) create mode 100644 controllers/nstemplatetier/mapper.go create mode 100644 controllers/nstemplatetier/mapper_test.go diff --git a/controllers/nstemplatetier/mapper.go b/controllers/nstemplatetier/mapper.go new file mode 100644 index 000000000..af82c8479 --- /dev/null +++ b/controllers/nstemplatetier/mapper.go @@ -0,0 +1,53 @@ +package nstemplatetier + +import ( + "context" + + toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" + + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/reconcile" +) + +var mapperLog = ctrl.Log.WithName("MapTierTemplateToNSTemplateTier") + +// MapTierTemplateToNSTemplateTier maps the TierTemplate to all the NSTemplateTiers that are referecing it. +func MapTierTemplateToNSTemplateTier(cl runtimeclient.Client) func(ctx context.Context, object runtimeclient.Object) []reconcile.Request { + return func(ctx context.Context, obj runtimeclient.Object) []reconcile.Request { + logger := mapperLog.WithValues("object-name", obj.GetName(), "object-kind", obj.GetObjectKind()) + nsTmplTierList := &toolchainv1alpha1.NSTemplateTierList{} + err := cl.List(context.TODO(), nsTmplTierList, + runtimeclient.InNamespace(obj.GetNamespace())) + if err != nil { + logger.Error(err, "unable to list NSTemplateTier") + return []reconcile.Request{} + } + if len(nsTmplTierList.Items) == 0 { + logger.Info("no NSTemplateTier found for an object") + return []reconcile.Request{} + } + + req := make([]reconcile.Request, 0, len(nsTmplTierList.Items)) + for _, item := range nsTmplTierList.Items { + if len(item.Status.Revisions) == 0 { + // this tier doesn't use the template + continue + } + _, inUse := item.Status.Revisions[obj.GetName()] + if !inUse { + // this tier doesn't use the template + continue + } + // the tier uses this template + req = append(req, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Namespace: item.Namespace, + Name: item.Name, + }, + }) + } + return req + } +} diff --git a/controllers/nstemplatetier/mapper_test.go b/controllers/nstemplatetier/mapper_test.go new file mode 100644 index 000000000..e4397bdec --- /dev/null +++ b/controllers/nstemplatetier/mapper_test.go @@ -0,0 +1,93 @@ +package nstemplatetier_test + +import ( + "context" + "testing" + + toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" + "github.com/codeready-toolchain/host-operator/controllers/nstemplatetier" + tiertest "github.com/codeready-toolchain/host-operator/test/nstemplatetier" + "github.com/codeready-toolchain/toolchain-common/pkg/test" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/reconcile" +) + +func TestMapTierTemplateToNSTemplateTier(t *testing.T) { + t.Run("should return the only NSTemplateTier referencing the TierTemplate", func(t *testing.T) { + // given + base1nsTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates) + prepareTierWithRevisions(base1nsTier) + // and we have a tier without the revisions + otherTier := tiertest.AppStudioEnvTier(t, tiertest.AppStudioEnvTemplates) + cl := test.NewFakeClient(t, base1nsTier, otherTier) + + // when + // for the simplicity of the test, we only try to map one tiertemplate + clusterResourceTierTemplate := createTierTemplate(t, "clusterresources", nil, base1nsTier.Name) + requests := nstemplatetier.MapTierTemplateToNSTemplateTier(cl)(context.TODO(), clusterResourceTierTemplate) + + // then + require.Len(t, requests, 1) + assert.Contains(t, requests, newRequest(base1nsTier.Name)) + }) + + t.Run("should return two NSTemplateTier when both are referencing the same TierTemplate", func(t *testing.T) { + // given + base1nsTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates) + prepareTierWithRevisions(base1nsTier) + // we update the other tier to use the same revisions + otherTier := tiertest.AppStudioEnvTier(t, tiertest.AppStudioEnvTemplates) + prepareTierWithRevisions(otherTier) + cl := test.NewFakeClient(t, base1nsTier, otherTier) + + // when + // for the simplicity of the test we only try to map one tiertemplate + clusterResourceTierTemplate := createTierTemplate(t, "clusterresources", nil, base1nsTier.Name) + requests := nstemplatetier.MapTierTemplateToNSTemplateTier(cl)(context.TODO(), clusterResourceTierTemplate) + + // then + require.Len(t, requests, 2) + assert.Contains(t, requests, newRequest(base1nsTier.Name)) + assert.Contains(t, requests, newRequest(otherTier.Name)) + }) + + t.Run("should return no NSTemplateTier when they don't use the TierTemplate", func(t *testing.T) { + // given + // both tiers are without revisions + base1nsTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates) + otherTier := tiertest.AppStudioEnvTier(t, tiertest.AppStudioEnvTemplates) + cl := test.NewFakeClient(t, base1nsTier, otherTier) + + // when + // for the simplicity of the test, we only try to map one tiertemplate + clusterResourceTierTemplate := createTierTemplate(t, "clusterresources", nil, base1nsTier.Name) + requests := nstemplatetier.MapTierTemplateToNSTemplateTier(cl)(context.TODO(), clusterResourceTierTemplate) + + // then + require.Empty(t, requests) + }) +} + +func prepareTierWithRevisions(tier *toolchainv1alpha1.NSTemplateTier) { + initialRevisions := map[string]string{ + "base1ns-admin-123456new": "base1ns-admin-123456new-abcd", + "base1ns-clusterresources-123456new": "base1ns-clusterresources-123456new-abcd", + "base1ns-code-123456new": "base1ns-code-123456new-abcd", + "base1ns-dev-123456new": "base1ns-dev-123456new-abcd", + "base1ns-edit-123456new": "`base1ns-edit-123456new-abcd", + "base1ns-stage-123456new": "base1ns-stage-123456new-abcd", + "base1ns-viewer-123456new": "base1ns-viewer-123456new-abcd", + } + tier.Status.Revisions = initialRevisions +} + +func newRequest(name string) reconcile.Request { + return reconcile.Request{ + NamespacedName: types.NamespacedName{ + Namespace: test.HostOperatorNs, + Name: name, + }, + } +} diff --git a/controllers/nstemplatetier/nstemplatetier_controller.go b/controllers/nstemplatetier/nstemplatetier_controller.go index 9a37b3363..bd14abee3 100644 --- a/controllers/nstemplatetier/nstemplatetier_controller.go +++ b/controllers/nstemplatetier/nstemplatetier_controller.go @@ -11,6 +11,7 @@ import ( "github.com/redhat-cop/operator-utils/pkg/util" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/builder" + "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/log" "k8s.io/apimachinery/pkg/api/errors" @@ -27,6 +28,8 @@ import ( func (r *Reconciler) SetupWithManager(mgr manager.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&toolchainv1alpha1.NSTemplateTier{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})). + Watches(&toolchainv1alpha1.TierTemplate{}, + handler.EnqueueRequestsFromMapFunc(MapTierTemplateToNSTemplateTier(r.Client))). Complete(r) } From f7b8c06fdefb5bf1615cbe4d8f3026325c6b704f Mon Sep 17 00:00:00 2001 From: Francisc Munteanu Date: Wed, 5 Feb 2025 13:27:42 +0100 Subject: [PATCH 6/9] Update controllers/nstemplatetier/mapper.go Co-authored-by: Matous Jobanek --- controllers/nstemplatetier/mapper.go | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/controllers/nstemplatetier/mapper.go b/controllers/nstemplatetier/mapper.go index af82c8479..670fa9e7d 100644 --- a/controllers/nstemplatetier/mapper.go +++ b/controllers/nstemplatetier/mapper.go @@ -35,18 +35,15 @@ func MapTierTemplateToNSTemplateTier(cl runtimeclient.Client) func(ctx context.C // this tier doesn't use the template continue } - _, inUse := item.Status.Revisions[obj.GetName()] - if !inUse { - // this tier doesn't use the template - continue + if _, inUse := item.Status.Revisions[obj.GetName()]; inUse { + // the tier uses this template + req = append(req, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Namespace: item.Namespace, + Name: item.Name, + }, + }) } - // the tier uses this template - req = append(req, reconcile.Request{ - NamespacedName: types.NamespacedName{ - Namespace: item.Namespace, - Name: item.Name, - }, - }) } return req } From dc6be5bfd799476d8e8e4ce72ccec6884534ac61 Mon Sep 17 00:00:00 2001 From: Francisc Munteanu Date: Wed, 5 Feb 2025 13:28:27 +0100 Subject: [PATCH 7/9] Update controllers/nstemplatetier/mapper_test.go Co-authored-by: Matous Jobanek --- controllers/nstemplatetier/mapper_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/controllers/nstemplatetier/mapper_test.go b/controllers/nstemplatetier/mapper_test.go index e4397bdec..4e1e2df4e 100644 --- a/controllers/nstemplatetier/mapper_test.go +++ b/controllers/nstemplatetier/mapper_test.go @@ -58,6 +58,8 @@ func TestMapTierTemplateToNSTemplateTier(t *testing.T) { // both tiers are without revisions base1nsTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates) otherTier := tiertest.AppStudioEnvTier(t, tiertest.AppStudioEnvTemplates) + prepareTierWithRevisions(otherTier) + delete(otherTier.Status.Revisions, "base1ns-clusterresources-123456new") cl := test.NewFakeClient(t, base1nsTier, otherTier) // when From 87ea29c751184e89e7bf476da2c6b26c1ca669b4 Mon Sep 17 00:00:00 2001 From: Devtools Date: Wed, 5 Feb 2025 13:35:05 +0100 Subject: [PATCH 8/9] remove log config --- controllers/nstemplatetier/nstemplatetier_controller_test.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/controllers/nstemplatetier/nstemplatetier_controller_test.go b/controllers/nstemplatetier/nstemplatetier_controller_test.go index 58ae17fef..64f04f261 100644 --- a/controllers/nstemplatetier/nstemplatetier_controller_test.go +++ b/controllers/nstemplatetier/nstemplatetier_controller_test.go @@ -27,8 +27,6 @@ import ( "k8s.io/apimachinery/pkg/util/validation" "k8s.io/client-go/kubernetes/scheme" runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" - logf "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/log/zap" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) @@ -38,8 +36,6 @@ const ( func TestReconcile(t *testing.T) { // given - logf.SetLogger(zap.New(zap.UseDevMode(true))) - t.Run("failures", func(t *testing.T) { t.Run("unable to get NSTemplateTier", func(t *testing.T) { @@ -317,7 +313,6 @@ func TestReconcile(t *testing.T) { } func TestUpdateNSTemplateTier(t *testing.T) { - logf.SetLogger(zap.New(zap.UseDevMode(true))) // given base1nsTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, // the tiertemplate revision CR should have a copy of those parameters From 18fcac648d0f0bdc871e129c36db795060a4b056 Mon Sep 17 00:00:00 2001 From: Francisc Munteanu Date: Fri, 7 Feb 2025 11:05:46 +0100 Subject: [PATCH 9/9] Update controllers/nstemplatetier/mapper.go Co-authored-by: Feny Mehta --- controllers/nstemplatetier/mapper.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/nstemplatetier/mapper.go b/controllers/nstemplatetier/mapper.go index 670fa9e7d..bfe1fdba5 100644 --- a/controllers/nstemplatetier/mapper.go +++ b/controllers/nstemplatetier/mapper.go @@ -13,7 +13,7 @@ import ( var mapperLog = ctrl.Log.WithName("MapTierTemplateToNSTemplateTier") -// MapTierTemplateToNSTemplateTier maps the TierTemplate to all the NSTemplateTiers that are referecing it. +// MapTierTemplateToNSTemplateTier maps the TierTemplate to all the NSTemplateTiers that are referencing it. func MapTierTemplateToNSTemplateTier(cl runtimeclient.Client) func(ctx context.Context, object runtimeclient.Object) []reconcile.Request { return func(ctx context.Context, obj runtimeclient.Object) []reconcile.Request { logger := mapperLog.WithValues("object-name", obj.GetName(), "object-kind", obj.GetObjectKind())