From 5e1c8e56282ebb594501eeff2c4f63063984aee8 Mon Sep 17 00:00:00 2001 From: Sebastian Widmer Date: Fri, 2 Aug 2024 15:19:20 +0200 Subject: [PATCH] Don't manage `ACCESS_TOKEN_` variables for unmanaged cluster git repos (#303) --- .../cluster_compile_pipeline_controller.go | 9 +- ...luster_compile_pipeline_controller_test.go | 131 ++++++++++++++++-- 2 files changed, 121 insertions(+), 19 deletions(-) diff --git a/controllers/cluster_compile_pipeline_controller.go b/controllers/cluster_compile_pipeline_controller.go index a4301e7d..3d0d633c 100644 --- a/controllers/cluster_compile_pipeline_controller.go +++ b/controllers/cluster_compile_pipeline_controller.go @@ -64,10 +64,11 @@ func (r *ClusterCompilePipelineReconciler) Reconcile(ctx context.Context, reques } } - if ensureClusterCiVariable(tenant, instance) { - err = r.Client.Update(ctx, tenant) - if err != nil { - return reconcile.Result{}, err + if instance.GetGitTemplate().RepoType != synv1alpha1.UnmanagedRepoType { + if ensureClusterCiVariable(tenant, instance) { + if err := r.Client.Update(ctx, tenant); err != nil { + return reconcile.Result{}, err + } } } diff --git a/controllers/cluster_compile_pipeline_controller_test.go b/controllers/cluster_compile_pipeline_controller_test.go index 803e735f..843bb929 100644 --- a/controllers/cluster_compile_pipeline_controller_test.go +++ b/controllers/cluster_compile_pipeline_controller_test.go @@ -2,20 +2,22 @@ package controllers_test import ( "context" + "fmt" "testing" "time" "github.com/stretchr/testify/assert" - - synv1alpha1 "github.com/projectsyn/lieutenant-operator/api/v1alpha1" - "github.com/projectsyn/lieutenant-operator/controllers" + "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" + + synv1alpha1 "github.com/projectsyn/lieutenant-operator/api/v1alpha1" + "github.com/projectsyn/lieutenant-operator/controllers" ) -func Test_AddClusterToPipelineStatus(t *testing.T) { +func Test_ClusterCompilePipelineReconciler_AddClusterToPipelineStatus(t *testing.T) { tenant := &synv1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ Name: "t-tenant", @@ -50,7 +52,7 @@ func Test_AddClusterToPipelineStatus(t *testing.T) { assert.Contains(t, mod_tenant.Status.CompilePipeline.Clusters, "c-cluster") } -func Test_RemoveClusterFromPipelineStatus(t *testing.T) { +func Test_ClusterCompilePipelineReconciler_RemoveClusterFromPipelineStatus(t *testing.T) { tenant := &synv1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ Name: "t-tenant", @@ -90,7 +92,7 @@ func Test_RemoveClusterFromPipelineStatus(t *testing.T) { assert.NotContains(t, mod_tenant.Status.CompilePipeline.Clusters, "c-cluster") } -func Test_RemoveClusterFromPipelineStatus_WhenDeleting(t *testing.T) { +func Test_ClusterCompilePipelineReconciler_RemoveClusterFromPipelineStatus_WhenDeleting(t *testing.T) { now := metav1.NewTime(time.Now()) tenant := &synv1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ @@ -133,7 +135,7 @@ func Test_RemoveClusterFromPipelineStatus_WhenDeleting(t *testing.T) { assert.NotContains(t, mod_tenant.Finalizers, synv1alpha1.PipelineFinalizerName) } -func Test_FinalizerAdded(t *testing.T) { +func Test_ClusterCompilePipelineReconciler_FinalizerAdded(t *testing.T) { cluster := &synv1alpha1.Cluster{ ObjectMeta: metav1.ObjectMeta{ Name: "c-cluster", @@ -160,7 +162,7 @@ func Test_FinalizerAdded(t *testing.T) { assert.Contains(t, mod_cluster.Finalizers, synv1alpha1.PipelineFinalizerName) } -func Test_RemoveClusterFromPipelineStatus_EnableUnset(t *testing.T) { +func Test_ClusterCompilePipelineReconciler_RemoveClusterFromPipelineStatus_EnableUnset(t *testing.T) { tenant := &synv1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ Name: "t-tenant", @@ -199,7 +201,7 @@ func Test_RemoveClusterFromPipelineStatus_EnableUnset(t *testing.T) { assert.NotContains(t, mod_tenant.Status.CompilePipeline.Clusters, "c-cluster") } -func Test_NoChangeIfClusterInList(t *testing.T) { +func Test_ClusterCompilePipelineReconciler_NoChangeIfClusterInList(t *testing.T) { tenant := &synv1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ Name: "t-tenant", @@ -239,7 +241,7 @@ func Test_NoChangeIfClusterInList(t *testing.T) { assert.Contains(t, mod_tenant.Status.CompilePipeline.Clusters, "c-cluster") } -func Test_LeaveOtherListEntriesBe_WhenRemoving(t *testing.T) { +func Test_ClusterCompilePipelineReconciler_LeaveOtherListEntriesBe_WhenRemoving(t *testing.T) { tenant := &synv1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ Name: "t-tenant", @@ -280,7 +282,7 @@ func Test_LeaveOtherListEntriesBe_WhenRemoving(t *testing.T) { assert.Contains(t, mod_tenant.Status.CompilePipeline.Clusters, "c-other-cluster") } -func Test_LeaveOtherListEntriesBe_WhenAdding(t *testing.T) { +func Test_ClusterCompilePipelineReconciler_LeaveOtherListEntriesBe_WhenAdding(t *testing.T) { tenant := &synv1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ Name: "t-tenant", @@ -320,7 +322,7 @@ func Test_LeaveOtherListEntriesBe_WhenAdding(t *testing.T) { assert.Contains(t, mod_tenant.Status.CompilePipeline.Clusters, "c-cluster") assert.Contains(t, mod_tenant.Status.CompilePipeline.Clusters, "c-other-cluster") } -func Test_CiVariableNotUpdated_IfNotEnabled(t *testing.T) { +func Test_ClusterCompilePipelineReconciler_CiVariableNotUpdated_IfNotEnabled(t *testing.T) { tenant := &synv1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ Name: "t-tenant", @@ -364,7 +366,7 @@ func Test_CiVariableNotUpdated_IfNotEnabled(t *testing.T) { assert.Empty(t, mod_tenant.GetGitTemplate().CIVariables) } -func Test_CiVariableNotUpdated_IfNoToken(t *testing.T) { +func Test_ClusterCompilePipelineReconciler_CiVariableNotUpdated_IfNoToken(t *testing.T) { tenant := &synv1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ Name: "t-tenant", @@ -403,7 +405,7 @@ func Test_CiVariableNotUpdated_IfNoToken(t *testing.T) { assert.Empty(t, mod_tenant.GetGitTemplate().CIVariables) } -func Test_CiVariableUpdated_IfEnabled(t *testing.T) { +func Test_ClusterCompilePipelineReconciler_CiVariableUpdated_IfEnabled(t *testing.T) { tenant := &synv1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ Name: "t-tenant", @@ -457,7 +459,8 @@ func Test_CiVariableUpdated_IfEnabled(t *testing.T) { assert.Equal(t, mod_tenant.Spec.GitRepoTemplate.CIVariables[0].GitlabOptions.Protected, false) assert.Equal(t, mod_tenant.Spec.GitRepoTemplate.CIVariables[0].GitlabOptions.Raw, true) } -func Test_KeepListInAlphabeticalOrder(t *testing.T) { + +func Test_ClusterCompilePipelineReconciler_KeepListInAlphabeticalOrder(t *testing.T) { tenant := &synv1alpha1.Tenant{ ObjectMeta: metav1.ObjectMeta{ Name: "t-tenant", @@ -553,3 +556,101 @@ func clusterCompilePipelineReconciler(c client.Client) *controllers.ClusterCompi } return &r } + +func Test_ClusterCompilePipelineReconciler_CIVariable_IgnoreUnmanagedRepo(t *testing.T) { + for _, tc := range []struct { + preExisting bool + clusterPipelineEnabled bool + }{ + { + preExisting: false, + clusterPipelineEnabled: true, + }, + { + preExisting: true, + clusterPipelineEnabled: true, + }, + { + preExisting: true, + clusterPipelineEnabled: false, + }, + } { + t.Run(fmt.Sprintf("PreExisting=%t,ClusterPipelineEnabled=%t", tc.preExisting, tc.clusterPipelineEnabled), func(t *testing.T) { + tenant := &synv1alpha1.Tenant{ + ObjectMeta: metav1.ObjectMeta{ + Name: "t-tenant", + Namespace: "lieutenant", + }, + Spec: synv1alpha1.TenantSpec{ + CompilePipeline: &synv1alpha1.CompilePipelineSpec{ + Enabled: true, + }, + GitRepoTemplate: &synv1alpha1.GitRepoTemplate{}, + }, + Status: synv1alpha1.TenantStatus{ + CompilePipeline: &synv1alpha1.CompilePipelineStatus{ + Clusters: []string{"c-cluster"}, + }, + }, + } + cluster := &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "c-cluster", + Namespace: "lieutenant", + Finalizers: []string{synv1alpha1.PipelineFinalizerName}, + }, + Spec: synv1alpha1.ClusterSpec{ + TenantRef: corev1.LocalObjectReference{ + Name: "t-tenant", + }, + EnableCompilePipeline: tc.clusterPipelineEnabled, + GitRepoTemplate: &synv1alpha1.GitRepoTemplate{ + RepoType: synv1alpha1.UnmanagedRepoType, + AccessToken: synv1alpha1.AccessToken{ + SecretRef: "my-secret", + }, + }, + }, + } + preExistingVar := synv1alpha1.EnvVar{ + Name: "ACCESS_TOKEN_c_cluster", + Value: "whatever", + } + if tc.preExisting { + tenant.Spec.GitRepoTemplate.CIVariables = []synv1alpha1.EnvVar{preExistingVar} + } + + c := preparePipelineTestClient(t, tenant, cluster) + r := clusterCompilePipelineReconciler(c) + ctx := context.Background() + + _, err := r.Reconcile(ctx, requestFor(cluster)) + assert.NoError(t, err) + + modTenant := &synv1alpha1.Tenant{} + err = c.Get(ctx, types.NamespacedName{Name: "t-tenant", Namespace: "lieutenant"}, modTenant) + assert.NoError(t, err) + + findCIVar := func(name string) (ciVar synv1alpha1.EnvVar, ok bool) { + for _, ciVar := range modTenant.GetGitTemplate().CIVariables { + if ciVar.Name == name { + return ciVar, true + } + } + return synv1alpha1.EnvVar{}, false + } + + if tc.clusterPipelineEnabled && tc.preExisting { + ciVar, ok := findCIVar("ACCESS_TOKEN_c_cluster") + require.True(t, ok) + assert.Equal(t, preExistingVar, ciVar, "Unmanged CIVariable should not have been changed") + } else if tc.clusterPipelineEnabled && !tc.preExisting { + _, ok := findCIVar("ACCESS_TOKEN_c_cluster") + assert.False(t, ok, "Unmanged CIVariable should not have been added") + } else if !tc.clusterPipelineEnabled && tc.preExisting { + _, ok := findCIVar("ACCESS_TOKEN_c_cluster") + assert.True(t, ok, "Unmanged CIVariable should not have been removed") + } + }) + } +}