From 28cd1e290d41655881ee1a8a534650f5d425c727 Mon Sep 17 00:00:00 2001 From: Simon Beck Date: Wed, 29 Apr 2020 16:08:07 +0200 Subject: [PATCH] Fix reconcile loop with GitRepo updates Updating the git repository every on every reconcile triggers a loop. That's because the gitRepo controller adds fields that aren't set in the gitRepoTemplates of the tenants and clusters. So it will always apply changes. The operator will permanently reconcile all tenants and clusters as they have the Gitrepository as their secondary objects. That triggers and endless loop of reconciles. The spec of the GitRepo object must not be changed in the reconcile function! GitRepo objects will be created from templates contained within the cluster or tenant objects. --- examples/cluster.yaml | 2 +- examples/tenant.yaml | 2 +- go.mod | 2 ++ go.sum | 5 +++++ pkg/controller/gitrepo/gitrepo_reconcile.go | 10 ++++++---- .../gitrepo/gitrepo_reconcile_test.go | 2 +- pkg/helpers/crd.go | 17 ++++++++++------- pkg/helpers/crd_test.go | 6 ++++++ 8 files changed, 32 insertions(+), 14 deletions(-) diff --git a/examples/cluster.yaml b/examples/cluster.yaml index 0b7b4c80..95e607cd 100644 --- 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 index b4490487..15160d8d 100644 --- 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 index 78bd2885..de107179 100644 --- 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 index f5aaa307..870fe349 100644 --- 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 index 22ad4a71..c2b9f7ca 100644 --- 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 index b2fe9658..479f39e9 100644 --- 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 index f416d968..3698d1e3 100644 --- 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 index eb12de31..09ac356c 100644 --- 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) } }