From 47f9094202ed6e4b7d6da1d59673e27199dd5396 Mon Sep 17 00:00:00 2001 From: Ben Oukhanov Date: Tue, 1 Aug 2023 12:13:54 +0300 Subject: [PATCH] refactor: cleanup tekton operands Cleanup Tekton operands to improve code quality and readability. Signed-off-by: Ben Oukhanov --- config/rbac/role.yaml | 18 ++-- .../ssp-operator.clusterserviceversion.yaml | 18 ++-- internal/common/resource.go | 11 +++ .../operands/tekton-pipelines/reconcile.go | 29 ++---- internal/operands/tekton-tasks/reconcile.go | 93 +++++++------------ .../operands/tekton-tasks/reconcile_test.go | 80 +++++++++------- 6 files changed, 115 insertions(+), 134 deletions(-) diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 782420451..128da9480 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -25,15 +25,6 @@ rules: - list - update - watch -- apiGroups: - - '*' - resources: - - configmaps - verbs: - - create - - delete - - list - - watch - apiGroups: - '*' resources: @@ -142,6 +133,15 @@ rules: - infrastructures verbs: - get +- apiGroups: + - "" + resources: + - configmaps + verbs: + - create + - delete + - list + - watch - apiGroups: - "" resources: diff --git a/data/olm-catalog/ssp-operator.clusterserviceversion.yaml b/data/olm-catalog/ssp-operator.clusterserviceversion.yaml index 72b0f2495..2caad78b2 100644 --- a/data/olm-catalog/ssp-operator.clusterserviceversion.yaml +++ b/data/olm-catalog/ssp-operator.clusterserviceversion.yaml @@ -86,15 +86,6 @@ spec: - list - update - watch - - apiGroups: - - '*' - resources: - - configmaps - verbs: - - create - - delete - - list - - watch - apiGroups: - '*' resources: @@ -203,6 +194,15 @@ spec: - infrastructures verbs: - get + - apiGroups: + - "" + resources: + - configmaps + verbs: + - create + - delete + - list + - watch - apiGroups: - "" resources: diff --git a/internal/common/resource.go b/internal/common/resource.go index 3960758fd..68b16c605 100644 --- a/internal/common/resource.go +++ b/internal/common/resource.go @@ -549,3 +549,14 @@ func defaultStatusFunc(obj client.Object) ResourceStatus { } return status } + +func AppendDeepCopies[PT interface { + *T + client.Object +}, T any](destination []client.Object, objects []T) []client.Object { + for i := range objects { + var object = PT(&objects[i]) + destination = append(destination, object.DeepCopyObject().(client.Object)) + } + return destination +} diff --git a/internal/operands/tekton-pipelines/reconcile.go b/internal/operands/tekton-pipelines/reconcile.go index 9816ddb0e..03acf2d46 100644 --- a/internal/operands/tekton-pipelines/reconcile.go +++ b/internal/operands/tekton-pipelines/reconcile.go @@ -17,7 +17,7 @@ import ( ) // +kubebuilder:rbac:groups=tekton.dev,resources=pipelines,verbs=list;watch;create;update;delete -// +kubebuilder:rbac:groups=*,resources=configmaps,verbs=list;watch;create;delete +// +kubebuilder:rbac:groups=core,resources=configmaps,verbs=list;watch;create;delete // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=rolebindings,verbs=list;watch;create;update;delete const ( @@ -115,33 +115,20 @@ func (t *tektonPipelines) Reconcile(request *common.Request) ([]common.Reconcile func (t *tektonPipelines) Cleanup(request *common.Request) ([]common.CleanupResult, error) { var objects []client.Object + if request.CrdList.CrdExists(tektonCrd) { - for _, p := range t.pipelines { - o := p.DeepCopy() - objects = append(objects, o) - } - } - for _, cm := range t.configMaps { - o := cm.DeepCopy() - objects = append(objects, o) - } - for _, rb := range t.roleBindings { - o := rb.DeepCopy() - objects = append(objects, o) - } - for _, sa := range t.serviceAccounts { - o := sa.DeepCopy() - objects = append(objects, o) + objects = common.AppendDeepCopies(objects, t.pipelines) } + objects = common.AppendDeepCopies(objects, t.configMaps) + objects = common.AppendDeepCopies(objects, t.roleBindings) + objects = common.AppendDeepCopies(objects, t.serviceAccounts) + for i := range objects { objects[i].SetNamespace(getTektonPipelinesNamespace(request)) } - for _, cr := range t.clusterRoles { - o := cr.DeepCopy() - objects = append(objects, o) - } + objects = common.AppendDeepCopies(objects, t.clusterRoles) return common.DeleteAll(request, objects...) } diff --git a/internal/operands/tekton-tasks/reconcile.go b/internal/operands/tekton-tasks/reconcile.go index 1ac48444b..44463e576 100644 --- a/internal/operands/tekton-tasks/reconcile.go +++ b/internal/operands/tekton-tasks/reconcile.go @@ -7,6 +7,7 @@ import ( pipeline "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" v1 "k8s.io/api/core/v1" rbac "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "kubevirt.io/ssp-operator/internal/common" "kubevirt.io/ssp-operator/internal/operands" @@ -24,9 +25,9 @@ import ( // +kubebuilder:rbac:groups=cdi.kubevirt.io,resources=datavolumes,verbs=* // +kubebuilder:rbac:groups=cdi.kubevirt.io,resources=datasources,verbs=get;create;delete // +kubebuilder:rbac:groups=kubevirt.io,resources=virtualmachines/finalizers,verbs=* -// +kubebuilder:rbac:groups=*,resources=persistentvolumeclaims,verbs=* -// +kubebuilder:rbac:groups=*,resources=pods,verbs=create -// +kubebuilder:rbac:groups=*,resources=secrets,verbs=* +// +kubebuilder:rbac:groups=core,resources=persistentvolumeclaims,verbs=* +// +kubebuilder:rbac:groups=core,resources=pods,verbs=create +// +kubebuilder:rbac:groups=core,resources=secrets,verbs=* const ( operandName = "tekton-tasks" @@ -87,43 +88,11 @@ type tektonTasks struct { var _ operands.Operand = &tektonTasks{} func New(bundle *tektonbundle.Bundle) operands.Operand { - newTasks := []pipeline.Task{} - for _, task := range bundle.Tasks { - if _, ok := AllowedTasks[task.Name]; ok { - newTasks = append(newTasks, task) - } - } - bundle.Tasks = newTasks - - newServiceAccounts := []v1.ServiceAccount{} - for _, serviceAccount := range bundle.ServiceAccounts { - if _, ok := AllowedTasks[strings.TrimSuffix(serviceAccount.Name, "-task")]; ok { - newServiceAccounts = append(newServiceAccounts, serviceAccount) - } - } - bundle.ServiceAccounts = newServiceAccounts - - newRoleBinding := []rbac.RoleBinding{} - for _, roleBinding := range bundle.RoleBindings { - if _, ok := AllowedTasks[strings.TrimSuffix(roleBinding.Name, "-task")]; ok { - newRoleBinding = append(newRoleBinding, roleBinding) - } - } - bundle.RoleBindings = newRoleBinding - - newClusterRole := []rbac.ClusterRole{} - for _, clusterRole := range bundle.ClusterRoles { - if _, ok := AllowedTasks[strings.TrimSuffix(clusterRole.Name, "-task")]; ok { - newClusterRole = append(newClusterRole, clusterRole) - } - } - bundle.ClusterRoles = newClusterRole - return &tektonTasks{ - tasks: bundle.Tasks, - serviceAccounts: bundle.ServiceAccounts, - roleBindings: bundle.RoleBindings, - clusterRoles: bundle.ClusterRoles, + tasks: filterTektonResources(bundle.Tasks, false), + serviceAccounts: filterTektonResources(bundle.ServiceAccounts, true), + roleBindings: filterTektonResources(bundle.RoleBindings, true), + clusterRoles: filterTektonResources(bundle.ClusterRoles, true), } } @@ -174,39 +143,27 @@ func (t *tektonTasks) Reconcile(request *common.Request) ([]common.ReconcileResu func (t *tektonTasks) Cleanup(request *common.Request) ([]common.CleanupResult, error) { var objects []client.Object + if request.CrdList.CrdExists(tektonCrd) { - for _, t := range t.tasks { - o := t.DeepCopy() - objects = append(objects, o) - } - } - for _, rb := range t.roleBindings { - o := rb.DeepCopy() - objects = append(objects, o) - } - for _, sa := range t.serviceAccounts { - o := sa.DeepCopy() - objects = append(objects, o) + objects = common.AppendDeepCopies(objects, t.tasks) } + objects = common.AppendDeepCopies(objects, t.roleBindings) + objects = common.AppendDeepCopies(objects, t.serviceAccounts) + for i := range objects { objects[i].SetNamespace(getTektonTasksNamespace(request)) } - for _, cr := range t.clusterRoles { - o := cr.DeepCopy() - objects = append(objects, o) - } + objects = common.AppendDeepCopies(objects, t.clusterRoles) if request.CrdList.CrdExists(tektonCrd) { clusterTasks, err := listDeprecatedClusterTasks(request) if err != nil { return nil, err } - for _, ct := range clusterTasks { - o := ct.DeepCopy() - objects = append(objects, o) - } + + objects = common.AppendDeepCopies(objects, clusterTasks) } return common.DeleteAll(request, objects...) @@ -307,3 +264,21 @@ func getTektonTasksNamespace(request *common.Request) string { } return request.Instance.Namespace } + +func filterTektonResources[PT interface { + *T + metav1.Object +}, T any](items []T, trimSuffix bool) []T { + var results []T + for i := range items { + object := PT(&items[i]) + name := object.GetName() + if trimSuffix { + name = strings.TrimSuffix(name, "-task") + } + if _, ok := AllowedTasks[name]; ok { + results = append(results, *object) + } + } + return results +} diff --git a/internal/operands/tekton-tasks/reconcile_test.go b/internal/operands/tekton-tasks/reconcile_test.go index fd728f1ca..b711b6cea 100644 --- a/internal/operands/tekton-tasks/reconcile_test.go +++ b/internal/operands/tekton-tasks/reconcile_test.go @@ -32,9 +32,9 @@ const ( var _ = Describe("environments", func() { var ( - operand operands.Operand bundle *tektonbundle.Bundle - request *common.Request + operand operands.Operand + request common.Request ) BeforeEach(func() { @@ -54,69 +54,69 @@ var _ = Describe("environments", func() { }) It("Reconcile function should return correct functions", func() { - functions, err := operand.Reconcile(request) + functions, err := operand.Reconcile(&request) Expect(err).ToNot(HaveOccurred(), "should not throw err") Expect(functions).To(HaveLen(8), "should return correct number of reconcile functions") }) It("Should create tekton-tasks resources", func() { - _, err := operand.Reconcile(request) + _, err := operand.Reconcile(&request) Expect(err).ToNot(HaveOccurred()) for _, task := range bundle.Tasks { - ExpectResourceExists(&task, *request) + ExpectResourceExists(&task, request) } for _, clusterRole := range bundle.ClusterRoles { - ExpectResourceExists(&clusterRole, *request) + ExpectResourceExists(&clusterRole, request) } for _, serviceAccount := range bundle.ServiceAccounts { - ExpectResourceExists(&serviceAccount, *request) + ExpectResourceExists(&serviceAccount, request) } for _, roleBinding := range bundle.RoleBindings { - ExpectResourceExists(&roleBinding, *request) + ExpectResourceExists(&roleBinding, request) } }) - It("should remove tekton-tasks resources on cleanup", func() { - _, err := operand.Reconcile(request) + It("Should remove tekton-tasks resources on cleanup", func() { + _, err := operand.Reconcile(&request) Expect(err).ToNot(HaveOccurred()) for _, task := range bundle.Tasks { - ExpectResourceExists(&task, *request) + ExpectResourceExists(&task, request) } for _, clusterRole := range bundle.ClusterRoles { - ExpectResourceExists(&clusterRole, *request) + ExpectResourceExists(&clusterRole, request) } for _, serviceAccount := range bundle.ServiceAccounts { - ExpectResourceExists(&serviceAccount, *request) + ExpectResourceExists(&serviceAccount, request) } for _, roleBinding := range bundle.RoleBindings { - ExpectResourceExists(&roleBinding, *request) + ExpectResourceExists(&roleBinding, request) } - _, err = operand.Cleanup(request) + _, err = operand.Cleanup(&request) Expect(err).ToNot(HaveOccurred()) for _, task := range bundle.Tasks { - ExpectResourceNotExists(&task, *request) + ExpectResourceNotExists(&task, request) } for _, clusterRole := range bundle.ClusterRoles { - ExpectResourceNotExists(&clusterRole, *request) + ExpectResourceNotExists(&clusterRole, request) } for _, serviceAccount := range bundle.ServiceAccounts { - ExpectResourceNotExists(&serviceAccount, *request) + ExpectResourceNotExists(&serviceAccount, request) } for _, roleBinding := range bundle.RoleBindings { - ExpectResourceNotExists(&roleBinding, *request) + ExpectResourceNotExists(&roleBinding, request) } }) }) @@ -127,23 +127,23 @@ var _ = Describe("environments", func() { }) It("Should not create tekton-tasks resources", func() { - _, err := operand.Reconcile(request) + _, err := operand.Reconcile(&request) Expect(err).ToNot(HaveOccurred()) for _, task := range bundle.Tasks { - ExpectResourceNotExists(&task, *request) + ExpectResourceNotExists(&task, request) } for _, clusterRole := range bundle.ClusterRoles { - ExpectResourceNotExists(&clusterRole, *request) + ExpectResourceNotExists(&clusterRole, request) } for _, serviceAccount := range bundle.ServiceAccounts { - ExpectResourceNotExists(&serviceAccount, *request) + ExpectResourceNotExists(&serviceAccount, request) } for _, roleBinding := range bundle.RoleBindings { - ExpectResourceNotExists(&roleBinding, *request) + ExpectResourceNotExists(&roleBinding, request) } }) }) @@ -154,7 +154,7 @@ func TestTektonTasks(t *testing.T) { RunSpecs(t, "Tekton Tasks Suite") } -func getMockedRequest() *common.Request { +func getMockedRequest() common.Request { log := logf.Log.WithName("tekton-tasks-operand") Expect(internalmeta.AddToScheme(scheme.Scheme)).To(Succeed()) @@ -179,7 +179,7 @@ func getMockedRequest() *common.Request { crdWatch := crd_watch.New(tektonCrd) Expect(crdWatch.Init(context.Background(), client)).To(Succeed()) - return &common.Request{ + return common.Request{ Request: reconcile.Request{ NamespacedName: types.NamespacedName{ Namespace: namespace, @@ -217,8 +217,9 @@ func getMockedTestBundle() *tektonbundle.Bundle { Tasks: []pipeline.Task{ { ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{}, - Name: diskVirtSysprepTaskName, + Labels: map[string]string{}, + Name: diskVirtSysprepTaskName, + Namespace: namespace, }, Spec: pipeline.TaskSpec{ Steps: []pipeline.Step{ @@ -229,8 +230,9 @@ func getMockedTestBundle() *tektonbundle.Bundle { }, }, { ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{}, - Name: modifyTemplateTaskName, + Labels: map[string]string{}, + Name: modifyTemplateTaskName, + Namespace: namespace, }, Spec: pipeline.TaskSpec{ Steps: []pipeline.Step{ @@ -244,33 +246,39 @@ func getMockedTestBundle() *tektonbundle.Bundle { ServiceAccounts: []v1.ServiceAccount{ { ObjectMeta: metav1.ObjectMeta{ - Name: diskVirtSysprepTaskName + "-task", + Name: diskVirtSysprepTaskName + "-task", + Namespace: namespace, }, }, { ObjectMeta: metav1.ObjectMeta{ - Name: modifyTemplateTaskName + "-task", + Name: modifyTemplateTaskName + "-task", + Namespace: namespace, }, }, }, RoleBindings: []rbac.RoleBinding{ { ObjectMeta: metav1.ObjectMeta{ - Name: diskVirtSysprepTaskName + "-task", + Name: diskVirtSysprepTaskName + "-task", + Namespace: namespace, }, }, { ObjectMeta: metav1.ObjectMeta{ - Name: modifyTemplateTaskName + "-task", + Name: modifyTemplateTaskName + "-task", + Namespace: namespace, }, }, }, ClusterRoles: []rbac.ClusterRole{ { ObjectMeta: metav1.ObjectMeta{ - Name: diskVirtSysprepTaskName + "-task", + Name: diskVirtSysprepTaskName + "-task", + Namespace: namespace, }, }, { ObjectMeta: metav1.ObjectMeta{ - Name: modifyTemplateTaskName + "-task", + Name: modifyTemplateTaskName + "-task", + Namespace: namespace, }, }, },