Merge pull request #51 from projectsyn/fix/reconcileloop

Fix reconcile loop with GitRepo updates
projectsyn · May 6, 2020 · b9ff771 · b9ff771
2 parents c2152f9 + 28cd1e2
commit b9ff771
Show file tree

Hide file tree

Showing 8 changed files with 32 additions and 14 deletions.
diff --git a/examples/cluster.yaml b/examples/cluster.yaml
@@ -9,7 +9,7 @@ spec:
     repoName: cluster1
     apiSecretRef:
       name: example-secret
-      namespace: syn-lieutenant
+      # namespace: syn-lieutenant
     deployKeys:
       test:
         type: ssh-ed25519

diff --git a/examples/tenant.yaml b/examples/tenant.yaml
@@ -9,7 +9,7 @@ spec:
     repoName: tenant1
     apiSecretRef:
       name: example-secret
-      namespace: syn-lieutenant
+      # namespace: syn-lieutenant
     deployKeys:
       test:
         type: ssh-ed25519

diff --git a/go.mod b/go.mod
@@ -6,8 +6,10 @@ require (
 	github.com/ahmetb/gen-crd-api-reference-docs v0.2.0
 	github.com/banzaicloud/bank-vaults/pkg/sdk v0.3.0
 	github.com/go-logr/logr v0.1.0
+	github.com/go-test/deep v1.0.6
 	github.com/hashicorp/vault/api v1.0.4
 	github.com/icza/gox v0.0.0-20200320174535-a6ff52ab3d90
+	github.com/imdario/mergo v0.3.9
 	github.com/operator-framework/operator-sdk v0.17.0
 	github.com/spf13/pflag v1.0.5
 	github.com/stretchr/testify v1.5.1

diff --git a/go.sum b/go.sum
@@ -375,7 +375,10 @@ github.com/go-sql-driver/mysql v1.4.0/go.mod h1:zAC/RDZ24gD3HViQzih4MyKcchzm+sOG
 github.com/go-sql-driver/mysql v1.4.1/go.mod h1:zAC/RDZ24gD3HViQzih4MyKcchzm+sOG5ZlKdlhCg5w=
 github.com/go-stack/stack v1.8.0 h1:5SgMzNM5HxrEjV0ww2lTmX6E2Izsfxas4+YHWRs3Lsk=
 github.com/go-stack/stack v1.8.0/go.mod h1:v0f6uXyyMGvRgIKkXu+yp6POWl0qKG85gN/melR3HDY=
+github.com/go-test/deep v1.0.2-0.20181118220953-042da051cf31 h1:28FVBuwkwowZMjbA7M0wXsI6t3PYulRTMio3SO+eKCM=
 github.com/go-test/deep v1.0.2-0.20181118220953-042da051cf31/go.mod h1:wGDj63lr65AM2AQyKZd/NYHGb0R+1RLqB8NKt3aSFNA=
+github.com/go-test/deep v1.0.6 h1:UHSEyLZUwX9Qoi99vVwvewiMC8mM2bf7XEM2nqvzEn8=
+github.com/go-test/deep v1.0.6/go.mod h1:QV8Hv/iy04NyLBxAdO9njL0iVPN1S4d/A3NVv1V36o8=
 github.com/gobuffalo/envy v1.6.5/go.mod h1:N+GkhhZ/93bGZc6ZKhJLP6+m+tCNPKwgSpH9kaifseQ=
 github.com/gobuffalo/envy v1.7.0 h1:GlXgaiBkmrYMHco6t4j7SacKO4XUjvh5pwXh0f4uxXU=
 github.com/gobuffalo/envy v1.7.0/go.mod h1:n7DRkBerg/aorDM8kbduw5dN3oXGswK5liaSCx4T5NI=
@@ -563,6 +566,8 @@ github.com/imdario/mergo v0.3.5/go.mod h1:2EnlNZ0deacrJVfApfmtdGgDfMuh/nq6Ok1EcJ
 github.com/imdario/mergo v0.3.6/go.mod h1:2EnlNZ0deacrJVfApfmtdGgDfMuh/nq6Ok1EcJh5FfA=
 github.com/imdario/mergo v0.3.7 h1:Y+UAYTZ7gDEuOfhxKWy+dvb5dRQ6rJjFSdX2HZY1/gI=
 github.com/imdario/mergo v0.3.7/go.mod h1:2EnlNZ0deacrJVfApfmtdGgDfMuh/nq6Ok1EcJh5FfA=
+github.com/imdario/mergo v0.3.9 h1:UauaLniWCFHWd+Jp9oCEkTBj8VO/9DKg3PV3VCNMDIg=
+github.com/imdario/mergo v0.3.9/go.mod h1:2EnlNZ0deacrJVfApfmtdGgDfMuh/nq6Ok1EcJh5FfA=
 github.com/inconshreveable/mousetrap v1.0.0/go.mod h1:PxqpIevigyE2G7u3NXJIT2ANytuPF1OarO4DADm73n8=
 github.com/influxdata/influxdb v1.7.7/go.mod h1:qZna6X/4elxqT3yI9iZYdZrWWdeFOOprn86kgg4+IzY=
 github.com/jackc/fake v0.0.0-20150926172116-812a484cc733/go.mod h1:WrMFNQdiFJ80sQsxDoMokWK1W5TQtxBFNpzWTD84ibQ=

diff --git a/pkg/controller/gitrepo/gitrepo_reconcile.go b/pkg/controller/gitrepo/gitrepo_reconcile.go
@@ -28,6 +28,9 @@ const (
 // Reconcile will create or delete a git repository based on the event.
 // The Controller will requeue the Request to be processed again if the returned error is non-nil or
 // Result.Requeue is true, otherwise upon completion it will remove the work from the queue.
+
+// ATTENTION: do not manipulate the spec here, this will lead to loops, as the specs are
+// defined in the gitrepotemplates of the other CRDs (tenant, cluster).
 func (r *ReconcileGitRepo) Reconcile(request reconcile.Request) (reconcile.Result, error) {
 	reqLogger := log.WithValues("Request.Namespace", request.Namespace, "Request.Name", request.Name)
 	reqLogger.Info("Reconciling GitRepo")
@@ -43,10 +46,7 @@ func (r *ReconcileGitRepo) Reconcile(request reconcile.Request) (reconcile.Resul
 			}
 			return err
 		}
-		helpers.AddTenantLabel(&instance.ObjectMeta, instance.Spec.TenantRef.Name)
-		if instance.Spec.RepoType == synv1alpha1.DefaultRepoType {
-			instance.Spec.RepoType = synv1alpha1.AutoRepoType
-		}
+
 		secret := &corev1.Secret{}
 		namespacedName := types.NamespacedName{
 			Name:      instance.Spec.APISecretRef.Name,
@@ -137,6 +137,8 @@ func (r *ReconcileGitRepo) Reconcile(request reconcile.Request) (reconcile.Resul
 			reqLogger.Info("keys differed from CRD, keys re-applied to repository")
 		}
 
+		helpers.AddTenantLabel(&instance.ObjectMeta, instance.Spec.TenantRef.Name)
+
 		err = r.client.Update(context.TODO(), instance)
 		if err != nil {
 			return err

diff --git a/pkg/controller/gitrepo/gitrepo_reconcile_test.go b/pkg/controller/gitrepo/gitrepo_reconcile_test.go
@@ -135,7 +135,7 @@ func TestReconcileGitRepo_Reconcile(t *testing.T) {
 			assert.NoError(t, err)
 			if !tt.wantErr {
 				assert.Equal(t, string(secret.Data[SecretHostKeysName]), gitRepo.Status.HostKeys)
-				assert.Equal(t, synv1alpha1.AutoRepoType, gitRepo.Spec.RepoType)
+				assert.Equal(t, synv1alpha1.DefaultRepoType, gitRepo.Spec.RepoType)
 				assert.Equal(t, tt.fields.displayName, gitRepo.Spec.DisplayName)
 				assert.Equal(t, tt.fields.displayName, savedGitRepoOpt.DisplayName)
 				assert.Equal(t, tt.fields.tenantName, gitRepo.Labels[apis.LabelNameTenant])

diff --git a/pkg/helpers/crd.go b/pkg/helpers/crd.go
@@ -3,7 +3,6 @@ package helpers
 import (
 	"context"
 	"fmt"
-	"reflect"
 
 	corev1 "k8s.io/api/core/v1"
 
@@ -27,6 +26,10 @@ func CreateOrUpdateGitRepo(obj metav1.Object, gvk schema.GroupVersionKind, templ
 		return fmt.Errorf("the tenant name is empty")
 	}
 
+	if template.RepoType == synv1alpha1.DefaultRepoType {
+		template.RepoType = synv1alpha1.AutoRepoType
+	}
+
 	repo := &synv1alpha1.GitRepo{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      obj.GetName(),
@@ -54,22 +57,22 @@ func CreateOrUpdateGitRepo(obj metav1.Object, gvk schema.GroupVersionKind, templ
 		if err != nil {
 			return fmt.Errorf("could not update existing repo: %v", err)
 		}
+		existingRepo.Spec = repo.Spec
 
-		if !reflect.DeepEqual(existingRepo.Spec, repo.Spec) {
-			existingRepo.Spec = repo.Spec
+		err = client.Update(context.TODO(), existingRepo)
 
-			err = client.Update(context.TODO(), existingRepo)
-		}
 	}
 	return err
 }
 
-// AddTenantLabel adds the tenant label to an object
+// AddTenantLabel adds the tenant label to an object.
 func AddTenantLabel(meta *metav1.ObjectMeta, tenant string) {
 	if meta.Labels == nil {
 		meta.Labels = make(map[string]string)
 	}
-	meta.Labels[apis.LabelNameTenant] = tenant
+	if meta.Labels[apis.LabelNameTenant] != tenant {
+		meta.Labels[apis.LabelNameTenant] = tenant
+	}
 }
 
 func GetGitRepoURLAndHostKeys(obj metav1.Object, client client.Client) (string, string, error) {

diff --git a/pkg/helpers/crd_test.go b/pkg/helpers/crd_test.go
@@ -113,6 +113,12 @@ func TestCreateOrUpdateGitRepo(t *testing.T) {
 		assert.NoError(t, cl.Get(context.TODO(), namespacedName, checkRepo))
 		assert.Equal(t, tt.args.template, &checkRepo.Spec.GitRepoTemplate)
 
+		checkRepo.Spec.RepoType = synv1alpha1.AutoRepoType
+		assert.NoError(t, cl.Update(context.TODO(), checkRepo))
+		assert.NoError(t, CreateOrUpdateGitRepo(tt.args.obj, tt.args.gvk, tt.args.template, cl, tt.args.tenantRef))
+		finalRepo := &synv1alpha1.GitRepo{}
+		assert.NoError(t, cl.Get(context.TODO(), namespacedName, finalRepo))
+		assert.Equal(t, checkRepo.Spec.GitRepoTemplate, finalRepo.Spec.GitRepoTemplate)
 	}
 }