From 40a16211e56ea084695c269ed06b915786aec2c9 Mon Sep 17 00:00:00 2001 From: Simon Beck Date: Thu, 30 Jul 2020 11:33:25 +0200 Subject: [PATCH 01/23] Refactored everything --- examples/gitrepo-secret.yaml | 4 +- go.sum | 1 + pkg/apis/syn/v1alpha1/cluster_types.go | 26 ++ pkg/apis/syn/v1alpha1/gitrepo_types.go | 25 ++ pkg/apis/syn/v1alpha1/tenant_types.go | 26 ++ pkg/controller/cluster/cluster_reconcile.go | 250 +----------------- .../cluster/cluster_reconcile_test.go | 14 +- pkg/controller/gitrepo/gitrepo_reconcile.go | 91 +------ pkg/controller/tenant/tenant_reconcile.go | 59 +---- .../tenant/tenant_reconcile_test.go | 5 +- pkg/helpers/crd.go | 160 ----------- pkg/pipeline/cluster.go | 232 ++++++++++++++++ pkg/pipeline/common.go | 203 ++++++++++++++ pkg/pipeline/git.go | 189 +++++++++++++ pkg/pipeline/pipelines.go | 73 +++++ pkg/pipeline/reconcile_types.go | 36 +++ pkg/pipeline/tenant.go | 17 ++ pkg/pipeline/vault.go | 92 +++++++ 18 files changed, 951 insertions(+), 552 deletions(-) create mode 100644 pkg/pipeline/cluster.go create mode 100644 pkg/pipeline/common.go create mode 100644 pkg/pipeline/git.go create mode 100644 pkg/pipeline/pipelines.go create mode 100644 pkg/pipeline/reconcile_types.go create mode 100644 pkg/pipeline/tenant.go create mode 100644 pkg/pipeline/vault.go diff --git a/examples/gitrepo-secret.yaml b/examples/gitrepo-secret.yaml index 47cef88e..f36780fb 100644 --- a/examples/gitrepo-secret.yaml +++ b/examples/gitrepo-secret.yaml @@ -1,7 +1,7 @@ apiVersion: v1 stringData: - endpoint: http://192.168.5.42:8080 - token: vY3gHvPs82NvYK8dKAGw + endpoint: http://192.168.5.195:8080 + token: oqepRZZtm2QymfTXErHX kind: Secret metadata: name: example-secret diff --git a/go.sum b/go.sum index 21087114..33714703 100644 --- a/go.sum +++ b/go.sum @@ -167,6 +167,7 @@ github.com/coreos/etcd v3.3.17+incompatible/go.mod h1:uF7uidLiAD3TWHmW31ZFd/JWoc github.com/coreos/go-etcd v2.0.0+incompatible/go.mod h1:Jez6KQU2B/sWsbdaef3ED8NzMklzPG4d5KIOhIy30Tk= github.com/coreos/go-oidc v2.1.0+incompatible/go.mod h1:CgnwVTmzoESiwO9qyAFEMiHoZ1nMCKZlZ9V6mm3/LKc= github.com/coreos/go-semver v0.2.0/go.mod h1:nnelYz7RCh+5ahJtPPxZlU+153eP4D4r3EedlOD2RNk= +github.com/coreos/go-semver v0.3.0 h1:wkHLiw0WNATZnSG7epLsujiMCgPAc9xhjJ4tgnAxmfM= github.com/coreos/go-semver v0.3.0/go.mod h1:nnelYz7RCh+5ahJtPPxZlU+153eP4D4r3EedlOD2RNk= github.com/coreos/go-systemd v0.0.0-20180511133405-39ca1b05acc7/go.mod h1:F5haX7vjVVG0kc13fIWeqUViNPyEJxv/OmvnBo0Yme4= github.com/coreos/go-systemd v0.0.0-20181012123002-c6f51f82210d/go.mod h1:F5haX7vjVVG0kc13fIWeqUViNPyEJxv/OmvnBo0Yme4= diff --git a/pkg/apis/syn/v1alpha1/cluster_types.go b/pkg/apis/syn/v1alpha1/cluster_types.go index 435b0c2f..aca9486a 100644 --- a/pkg/apis/syn/v1alpha1/cluster_types.go +++ b/pkg/apis/syn/v1alpha1/cluster_types.go @@ -81,3 +81,29 @@ type ClusterList struct { func init() { SchemeBuilder.Register(&Cluster{}, &ClusterList{}) } + +// GetGitTemplate returns the git repository template +func (c *Cluster) GetGitTemplate() *GitRepoTemplate { + return c.Spec.GitRepoTemplate +} + +// GetTenantRef returns the tenant of this CR +func (c *Cluster) GetTenantRef() corev1.LocalObjectReference { + return c.Spec.TenantRef +} + +// GetDeletionPolicy returns the object's deletion policy +func (c *Cluster) GetDeletionPolicy() DeletionPolicy { + return c.Spec.DeletionPolicy +} + +// GetDisplayName returns the display name of the object +func (c *Cluster) GetDisplayName() string { + return c.Spec.DisplayName +} + +// SetGitRepoURLAndHostKeys +func (c *Cluster) SetGitRepoURLAndHostKeys(URL, hostKeys string) { + c.Spec.GitRepoURL = URL + c.Spec.GitHostKeys = hostKeys +} diff --git a/pkg/apis/syn/v1alpha1/gitrepo_types.go b/pkg/apis/syn/v1alpha1/gitrepo_types.go index 66996782..58ec2f54 100644 --- a/pkg/apis/syn/v1alpha1/gitrepo_types.go +++ b/pkg/apis/syn/v1alpha1/gitrepo_types.go @@ -130,3 +130,28 @@ type GitRepoList struct { func init() { SchemeBuilder.Register(&GitRepo{}, &GitRepoList{}) } + +// GetGitTemplate returns the git repository template +func (g *GitRepo) GetGitTemplate() *GitRepoTemplate { + return &g.Spec.GitRepoTemplate +} + +// GetTenantRef returns the tenant of this CR +func (g *GitRepo) GetTenantRef() corev1.LocalObjectReference { + return g.Spec.TenantRef +} + +// GetDeletionPolicy returns the object's deletion policy +func (g *GitRepo) GetDeletionPolicy() DeletionPolicy { + return g.Spec.DeletionPolicy +} + +// GetDisplayName returns the display name of the object +func (g *GitRepo) GetDisplayName() string { + return g.Spec.DisplayName +} + +// SetGitRepoURLAndHostKeys is currenlty a noop for gitrepo +func (g *GitRepo) SetGitRepoURLAndHostKeys(URL, hostKeys string) { + //NOOP +} diff --git a/pkg/apis/syn/v1alpha1/tenant_types.go b/pkg/apis/syn/v1alpha1/tenant_types.go index 97ca5623..4a001a38 100644 --- a/pkg/apis/syn/v1alpha1/tenant_types.go +++ b/pkg/apis/syn/v1alpha1/tenant_types.go @@ -1,6 +1,7 @@ package v1alpha1 import ( + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -62,3 +63,28 @@ type TenantList struct { func init() { SchemeBuilder.Register(&Tenant{}, &TenantList{}) } + +// GetGitTemplate returns the git repository template +func (t *Tenant) GetGitTemplate() *GitRepoTemplate { + return t.Spec.GitRepoTemplate +} + +// GetTenantRef returns the tenant of this CR +func (t *Tenant) GetTenantRef() corev1.LocalObjectReference { + return corev1.LocalObjectReference{Name: t.GetName()} +} + +// GetDeletionPolicy returns the object's deletion policy +func (t *Tenant) GetDeletionPolicy() DeletionPolicy { + return t.Spec.DeletionPolicy +} + +// GetDisplayName returns the display name of the object +func (t *Tenant) GetDisplayName() string { + return t.Spec.DisplayName +} + +// SetGitRepoURLAndHostKeys will only set the URL for the tenant +func (t *Tenant) SetGitRepoURLAndHostKeys(URL, hostKeys string) { + t.Spec.GitRepoURL = URL +} diff --git a/pkg/controller/cluster/cluster_reconcile.go b/pkg/controller/cluster/cluster_reconcile.go index 3fd16a59..5045f5c8 100644 --- a/pkg/controller/cluster/cluster_reconcile.go +++ b/pkg/controller/cluster/cluster_reconcile.go @@ -2,28 +2,11 @@ package cluster import ( "context" - "crypto/rand" - "encoding/base64" - "fmt" - "os" - "path" - "sort" - "strings" - "time" - "github.com/go-logr/logr" synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" - synTenant "github.com/projectsyn/lieutenant-operator/pkg/controller/tenant" - "github.com/projectsyn/lieutenant-operator/pkg/git/manager" - "github.com/projectsyn/lieutenant-operator/pkg/helpers" - "github.com/projectsyn/lieutenant-operator/pkg/vault" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/equality" + "github.com/projectsyn/lieutenant-operator/pkg/pipeline" "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/util/retry" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) @@ -68,235 +51,14 @@ func (r *ReconcileCluster) Reconcile(request reconcile.Request) (reconcile.Resul return err } - if err := r.createClusterRBAC(*instance); err != nil { - return err - } - - if instance.Status.BootstrapToken == nil { - reqLogger.Info("Adding status to Cluster object") - err := r.newStatus(instance) - if err != nil { - return err - } - } - - if time.Now().After(instance.Status.BootstrapToken.ValidUntil.Time) { - instance.Status.BootstrapToken.TokenValid = false - } - - if instance.Spec.GitRepoTemplate != nil { - if len(instance.Spec.GitRepoTemplate.DisplayName) == 0 { - instance.Spec.GitRepoTemplate.DisplayName = instance.Spec.DisplayName - } - - instance.Spec.GitRepoTemplate.DeletionPolicy = instance.Spec.DeletionPolicy - - result, err := helpers.CreateOrUpdateGitRepo(instance, r.scheme, instance.Spec.GitRepoTemplate, r.client, instance.Spec.TenantRef) - if err != nil { - reqLogger.Error(err, "Cannot create or update git repo object") - return err - } - - if result != controllerutil.OperationResultCreated { - instance.Spec.GitRepoURL, instance.Spec.GitHostKeys, err = helpers.GetGitRepoURLAndHostKeys(instance, r.client) - if err != nil { - return err - } - } - } - - repoName := request.NamespacedName - repoName.Name = instance.Spec.TenantRef.Name - - var vaultClient vault.VaultClient = nil - secretPath := path.Join(instance.Spec.TenantRef.Name, instance.Name, "steward") - - deletionPolicy := instance.Spec.DeletionPolicy - if deletionPolicy == "" { - deletionPolicy = helpers.GetDeletionPolicy() - } - - if strings.ToLower(os.Getenv("SKIP_VAULT_SETUP")) != "true" { - - vaultClient, err = vault.NewClient(deletionPolicy, reqLogger) - if err != nil { - return err - } - - token, err := r.getServiceAccountToken(instance) - if err != nil { - return err - } - - err = vaultClient.AddSecrets([]vault.VaultSecret{{Path: secretPath, Value: token}}) - if err != nil { - return err - } - - } - - deleted := helpers.HandleDeletion(instance, finalizerName, r.client) - if deleted.FinalizerRemoved { - if vaultClient != nil { - err := vaultClient.RemoveSecrets([]vault.VaultSecret{{Path: path.Dir(secretPath), Value: ""}}) - if err != nil { - return err - } - } - // TODO: Move logic to tenant reconcile to avoid conflicts https://github.com/projectsyn/lieutenant-operator/issues/80 - err = r.removeClusterFileFromTenant(instance.GetName(), repoName, reqLogger) - if err != nil { - return err - } - } - if deleted.Deleted { - return r.client.Update(context.TODO(), instance) - } - - // TODO: Move logic to tenant reconcile to avoid conflicts https://github.com/projectsyn/lieutenant-operator/issues/80 - err = r.updateTenantGitRepo(repoName, instance.GetName()) - if err != nil { - return err + data := &pipeline.ExecutionContext{ + Client: r.client, + Log: reqLogger, + FinalizerName: finalizerName, } - helpers.AddTenantLabel(&instance.ObjectMeta, instance.Spec.TenantRef.Name) - helpers.AddDeletionProtection(instance) - controllerutil.AddFinalizer(instance, finalizerName) - - if !equality.Semantic.DeepEqual(instanceCopy.Status, instance.Status) { - if err := r.client.Status().Update(context.TODO(), instance); err != nil { - return err - } - } - if !equality.Semantic.DeepEqual(instanceCopy, instance) { - if err := r.client.Update(context.TODO(), instance); err != nil { - return err - } - } - return nil + return pipeline.ReconcileCluster(instance, data) }) return reconcile.Result{}, err } - -func (r *ReconcileCluster) generateToken() (string, error) { - b := make([]byte, 16) - _, err := rand.Read(b) - if err != nil { - return "", err - } - return base64.URLEncoding.EncodeToString(b), err -} - -//newStatus will create a default lifetime of 30 minutes if it wasn't set in the object. -func (r *ReconcileCluster) newStatus(cluster *synv1alpha1.Cluster) error { - - parseTime := "30m" - if cluster.Spec.TokenLifeTime != "" { - parseTime = cluster.Spec.TokenLifeTime - } - - duration, err := time.ParseDuration(parseTime) - if err != nil { - return err - } - - validUntil := time.Now().Add(duration) - - token, err := r.generateToken() - if err != nil { - return err - } - - cluster.Status.BootstrapToken = &synv1alpha1.BootstrapToken{ - Token: token, - ValidUntil: metav1.NewTime(validUntil), - TokenValid: true, - } - return nil -} - -func (r *ReconcileCluster) getTenantCR(tenant types.NamespacedName) (*synv1alpha1.Tenant, error) { - tenantCR := &synv1alpha1.Tenant{} - return tenantCR, r.client.Get(context.TODO(), tenant, tenantCR) -} - -func (r *ReconcileCluster) updateTenantGitRepo(tenant types.NamespacedName, clusterName string) error { - return retry.RetryOnConflict(retry.DefaultBackoff, func() error { - - tenantCR, err := r.getTenantCR(tenant) - if err != nil { - return err - } - - if tenantCR.Spec.GitRepoTemplate == nil { - return nil - } - - if tenantCR.Spec.GitRepoTemplate.TemplateFiles == nil { - tenantCR.Spec.GitRepoTemplate.TemplateFiles = map[string]string{} - } - - clusterClassFile := clusterName + ".yml" - if _, ok := tenantCR.Spec.GitRepoTemplate.TemplateFiles[clusterClassFile]; !ok { - fileContent := fmt.Sprintf(clusterClassContent, tenant.Name, synTenant.CommonClassName) - tenantCR.Spec.GitRepoTemplate.TemplateFiles[clusterClassFile] = fileContent - return r.client.Update(context.TODO(), tenantCR) - } - return nil - }) -} - -func (r *ReconcileCluster) getServiceAccountToken(instance metav1.Object) (string, error) { - secrets := &corev1.SecretList{} - - err := r.client.List(context.TODO(), secrets) - if err != nil { - return "", err - } - - sortSecrets := helpers.SecretSortList(*secrets) - - sort.Sort(sort.Reverse(sortSecrets)) - - for _, secret := range sortSecrets.Items { - - if secret.Type != corev1.SecretTypeServiceAccountToken { - continue - } - - if secret.Annotations[corev1.ServiceAccountNameKey] == instance.GetName() { - if string(secret.Data["token"]) == "" { - // We'll skip the secrets if the token is not yet populated. - continue - } - return string(secret.Data["token"]), nil - } - } - - return "", fmt.Errorf("no matching secrets found") -} - -func (r *ReconcileCluster) removeClusterFileFromTenant(clusterName string, tenantInfo types.NamespacedName, reqLogger logr.Logger) error { - - tenantCR, err := r.getTenantCR(tenantInfo) - if err != nil { - return err - } - - fileName := clusterName + ".yml" - - if tenantCR.Spec.GitRepoTemplate == nil || tenantCR.Spec.GitRepoTemplate.TemplateFiles == nil { - return nil - } - - if _, ok := tenantCR.Spec.GitRepoTemplate.TemplateFiles[fileName]; ok { - tenantCR.Spec.GitRepoTemplate.TemplateFiles[fileName] = manager.DeletionMagicString - err := r.client.Update(context.TODO(), tenantCR) - if err != nil { - return err - } - } - - return nil -} diff --git a/pkg/controller/cluster/cluster_reconcile_test.go b/pkg/controller/cluster/cluster_reconcile_test.go index d007fa81..c77a3de9 100644 --- a/pkg/controller/cluster/cluster_reconcile_test.go +++ b/pkg/controller/cluster/cluster_reconcile_test.go @@ -12,7 +12,7 @@ import ( "github.com/projectsyn/lieutenant-operator/pkg/apis" synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" - "github.com/projectsyn/lieutenant-operator/pkg/controller/tenant" + "github.com/projectsyn/lieutenant-operator/pkg/pipeline" "github.com/projectsyn/lieutenant-operator/pkg/vault" "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" @@ -267,7 +267,7 @@ func TestReconcileCluster_Reconcile(t *testing.T) { if tt.skipVault { assert.Empty(t, testMockClient.secrets) } else { - saToken, err := r.getServiceAccountToken(newCluster) + saToken, err := pipeline.GetServiceAccountToken(newCluster, &pipeline.ExecutionContext{Client: cl}) saSecrets := []vault.VaultSecret{{Value: saToken, Path: path.Join(tt.fields.tenantName, tt.fields.objName, "steward")}} assert.NoError(t, err) assert.Equal(t, testMockClient.secrets, saSecrets) @@ -288,7 +288,7 @@ func TestReconcileCluster_Reconcile(t *testing.T) { assert.NoError(t, err) fileContent, found := testTenant.Spec.GitRepoTemplate.TemplateFiles[tt.fields.objName+".yml"] assert.True(t, found) - assert.Equal(t, fileContent, fmt.Sprintf(clusterClassContent, tt.fields.tenantName, tenant.CommonClassName)) + assert.Equal(t, fileContent, fmt.Sprintf(clusterClassContent, tt.fields.tenantName, pipeline.CommonClassName)) }) } } @@ -435,13 +435,9 @@ func TestReconcileCluster_getServiceAccountToken(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cl, s := testSetupClient(tt.args.objs) + cl, _ := testSetupClient(tt.args.objs) - r := &ReconcileCluster{ - client: cl, - scheme: s, - } - got, err := r.getServiceAccountToken(tt.args.instance) + got, err := pipeline.GetServiceAccountToken(tt.args.instance, &pipeline.ExecutionContext{Client: cl}) if (err != nil) != tt.wantErr { t.Errorf("ReconcileCluster.getServiceAccountToken() error = %v, wantErr %v", err, tt.wantErr) return diff --git a/pkg/controller/gitrepo/gitrepo_reconcile.go b/pkg/controller/gitrepo/gitrepo_reconcile.go index 2dd94810..ab45d8dc 100644 --- a/pkg/controller/gitrepo/gitrepo_reconcile.go +++ b/pkg/controller/gitrepo/gitrepo_reconcile.go @@ -2,16 +2,13 @@ package gitrepo import ( "context" - "fmt" - "github.com/projectsyn/lieutenant-operator/pkg/git/manager" - "github.com/projectsyn/lieutenant-operator/pkg/helpers" + "github.com/projectsyn/lieutenant-operator/pkg/pipeline" synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/client-go/util/retry" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) @@ -46,90 +43,14 @@ func (r *ReconcileGitRepo) Reconcile(request reconcile.Request) (reconcile.Resul return nil } - repo, hostKeys, err := manager.GetGitClient(&instance.Spec.GitRepoTemplate, instance.GetNamespace(), reqLogger, r.client) - if err != nil { - return err - } - - instance.Status.HostKeys = hostKeys - - if !r.repoExists(repo) { - reqLogger.Info("creating git repo", manager.SecretEndpointName, repo.FullURL()) - err := repo.Create() - if err != nil { - return r.handleRepoError(err, instance, repo) - } - reqLogger.Info("successfully created the repository") - } - - deleted := helpers.HandleDeletion(instance, finalizerName, r.client) - if deleted.FinalizerRemoved { - err := repo.Remove() - if err != nil { - return err - } - } - if deleted.Deleted { - return r.client.Update(context.TODO(), instance) - } - - err = repo.CommitTemplateFiles() - if err != nil { - return r.handleRepoError(err, instance, repo) - } - - changed, err := repo.Update() - if err != nil { - return err + data := &pipeline.ExecutionContext{ + Client: r.client, + Log: reqLogger, + FinalizerName: finalizerName, } - if changed { - reqLogger.Info("keys differed from CRD, keys re-applied to repository") - } - - helpers.AddTenantLabel(&instance.ObjectMeta, instance.Spec.TenantRef.Name) - helpers.AddDeletionProtection(instance) - - controllerutil.AddFinalizer(instance, finalizerName) - - if !equality.Semantic.DeepEqual(instanceCopy, instance) { - err = r.client.Update(context.TODO(), instance) - } - if err != nil { - return err - } - phase := synv1alpha1.Created - instance.Status.Phase = &phase - instance.Status.URL = repo.FullURL().String() - instance.Status.Type = synv1alpha1.GitType(repo.Type()) - if !equality.Semantic.DeepEqual(instanceCopy.Status, instance.Status) { - if err := r.client.Status().Update(context.TODO(), instance); err != nil { - return err - } - } - return nil + return pipeline.ReconcileGitRep(instance, data) }) return reconcile.Result{}, err } - -func (r *ReconcileGitRepo) repoExists(repo manager.Repo) bool { - if err := repo.Read(); err == nil { - return true - } - - return false -} - -func (r *ReconcileGitRepo) handleRepoError(repoErr error, instance *synv1alpha1.GitRepo, repo manager.Repo) error { - instanceCopy := instance.DeepCopy() - phase := synv1alpha1.Failed - instance.Status.Phase = &phase - instance.Status.URL = repo.FullURL().String() - if !equality.Semantic.DeepEqual(instanceCopy.Status, instance.Status) { - if err := r.client.Status().Update(context.TODO(), instance); err != nil { - return fmt.Errorf("could not set status while handling error: %w: %s", err, repoErr) - } - } - return repoErr -} diff --git a/pkg/controller/tenant/tenant_reconcile.go b/pkg/controller/tenant/tenant_reconcile.go index 9c439995..ab83049d 100644 --- a/pkg/controller/tenant/tenant_reconcile.go +++ b/pkg/controller/tenant/tenant_reconcile.go @@ -4,20 +4,12 @@ import ( "context" "os" - corev1 "k8s.io/api/core/v1" "k8s.io/client-go/util/retry" + "sigs.k8s.io/controller-runtime/pkg/reconcile" synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" - "github.com/projectsyn/lieutenant-operator/pkg/helpers" - "k8s.io/apimachinery/pkg/api/equality" + "github.com/projectsyn/lieutenant-operator/pkg/pipeline" "k8s.io/apimachinery/pkg/api/errors" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "sigs.k8s.io/controller-runtime/pkg/reconcile" -) - -const ( - // CommonClassName is the name of the tenant's common class - CommonClassName = "common" ) // Reconcile The Controller will requeue the Request to be processed again if the returned error is non-nil or @@ -32,56 +24,23 @@ func (r *ReconcileTenant) Reconcile(request reconcile.Request) (reconcile.Result err := r.client.Get(context.TODO(), request.NamespacedName, instance) if err != nil { if errors.IsNotFound(err) { - // Request object not found, could have been deleted after reconcile request. - // Owned objects are automatically garbage collected. For additional cleanup logic use finalizers. - // Return and don't requeue return nil } - // Error reading the object - requeue the request. return err } instanceCopy := instance.DeepCopy() - if instance.Spec.GitRepoTemplate != nil { - if len(instance.Spec.GitRepoTemplate.DisplayName) == 0 { - instance.Spec.GitRepoTemplate.DisplayName = instance.Spec.DisplayName - } - - commonClassFile := CommonClassName + ".yml" - if instance.Spec.GitRepoTemplate.TemplateFiles == nil { - instance.Spec.GitRepoTemplate.TemplateFiles = map[string]string{} - } - if _, ok := instance.Spec.GitRepoTemplate.TemplateFiles[commonClassFile]; !ok { - instance.Spec.GitRepoTemplate.TemplateFiles[commonClassFile] = "" - } - - instance.Spec.GitRepoTemplate.DeletionPolicy = instance.Spec.DeletionPolicy - - result, err := helpers.CreateOrUpdateGitRepo(instance, r.scheme, instance.Spec.GitRepoTemplate, r.client, corev1.LocalObjectReference{Name: instance.GetName()}) - if err != nil { - return err - } - - if result != controllerutil.OperationResultCreated { - instance.Spec.GitRepoURL, _, err = helpers.GetGitRepoURLAndHostKeys(instance, r.client) - if err != nil { - return err - } - } + data := &pipeline.ExecutionContext{ + Client: r.client, + Log: reqLogger, + FinalizerName: "", } - // Set URL of global git repo from default - defaultGlobalGitRepoURL := os.Getenv("DEFAULT_GLOBAL_GIT_REPO_URL") - if len(instance.Spec.GlobalGitRepoURL) == 0 && len(defaultGlobalGitRepoURL) > 0 { - instance.Spec.GlobalGitRepoURL = defaultGlobalGitRepoURL + err = pipeline.ReconcileTenant(instance, data) + if err != nil { + return err } - helpers.AddDeletionProtection(instance) - if !equality.Semantic.DeepEqual(instanceCopy, instance) { - if err := r.client.Update(context.TODO(), instance); err != nil { - return err - } - } return nil }) return reconcile.Result{}, err diff --git a/pkg/controller/tenant/tenant_reconcile_test.go b/pkg/controller/tenant/tenant_reconcile_test.go index 37effdd9..cce926ec 100644 --- a/pkg/controller/tenant/tenant_reconcile_test.go +++ b/pkg/controller/tenant/tenant_reconcile_test.go @@ -8,6 +8,7 @@ import ( "github.com/stretchr/testify/assert" synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" + "github.com/projectsyn/lieutenant-operator/pkg/pipeline" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -20,7 +21,7 @@ import ( func testSetupClient(objs []runtime.Object) (client.Client, *runtime.Scheme) { s := scheme.Scheme s.AddKnownTypes(synv1alpha1.SchemeGroupVersion, objs...) - return fake.NewFakeClient(objs...), s + return fake.NewFakeClientWithScheme(s, objs...), s } func TestHandleNilGitRepoTemplate(t *testing.T) { @@ -118,7 +119,7 @@ func TestCreateGitRepo(t *testing.T) { err = cl.Get(context.TODO(), gitRepoNamespacedName, gitRepo) assert.NoError(t, err) assert.Equal(t, tenant.Spec.DisplayName, gitRepo.Spec.GitRepoTemplate.DisplayName) - fileContent, found := gitRepo.Spec.GitRepoTemplate.TemplateFiles[CommonClassName+".yml"] + fileContent, found := gitRepo.Spec.GitRepoTemplate.TemplateFiles[pipeline.CommonClassName+".yml"] assert.True(t, found) assert.Equal(t, "", fileContent) }) diff --git a/pkg/helpers/crd.go b/pkg/helpers/crd.go index 965161e9..47542a21 100644 --- a/pkg/helpers/crd.go +++ b/pkg/helpers/crd.go @@ -1,101 +1,9 @@ package helpers import ( - "context" - "fmt" - "os" - "strconv" - corev1 "k8s.io/api/core/v1" - - "github.com/projectsyn/lieutenant-operator/pkg/apis" - synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" - "github.com/projectsyn/lieutenant-operator/pkg/git/manager" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" -) - -const ( - protectionSettingEnvVar = "LIEUTENANT_DELETE_PROTECTION" ) -// DeletionState of the instance -type DeletionState struct { - FinalizerRemoved bool - Deleted bool -} - -// CreateOrUpdateGitRepo will create the gitRepo object if it doesn't already exist. If the owner object itself is a tenant tenantRef can be set nil. -func CreateOrUpdateGitRepo(obj metav1.Object, scheme *runtime.Scheme, template *synv1alpha1.GitRepoTemplate, client client.Client, tenantRef corev1.LocalObjectReference) (controllerutil.OperationResult, error) { - - if template == nil { - return controllerutil.OperationResultNone, fmt.Errorf("gitRepo template is empty") - } - - if tenantRef.Name == "" { - return controllerutil.OperationResultNone, fmt.Errorf("the tenant name is empty") - } - - if template.DeletionPolicy == "" { - template.DeletionPolicy = GetDeletionPolicy() - } - - if template.RepoType == synv1alpha1.DefaultRepoType { - template.RepoType = synv1alpha1.AutoRepoType - } - - repo := &synv1alpha1.GitRepo{ - ObjectMeta: metav1.ObjectMeta{ - Name: obj.GetName(), - Namespace: obj.GetNamespace(), - }, - } - - result, err := controllerutil.CreateOrUpdate(context.TODO(), client, repo, func() error { - repo.Spec.GitRepoTemplate = *template - repo.Spec.TenantRef = tenantRef - AddDeletionProtection(repo) - return controllerutil.SetControllerReference(obj, repo, scheme) - }) - - for file, content := range template.TemplateFiles { - if content == manager.DeletionMagicString { - delete(template.TemplateFiles, file) - } - } - - return result, err -} - -// 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) - } - if meta.Labels[apis.LabelNameTenant] != tenant { - meta.Labels[apis.LabelNameTenant] = tenant - } -} - -// GetGitRepoURLAndHostKeys for an instance -func GetGitRepoURLAndHostKeys(obj metav1.Object, client client.Client) (string, string, error) { - gitRepo := &synv1alpha1.GitRepo{} - repoNamespacedName := types.NamespacedName{ - Namespace: obj.GetNamespace(), - Name: obj.GetName(), - } - err := client.Get(context.TODO(), repoNamespacedName, gitRepo) - if err != nil { - return "", "", err - } - - return gitRepo.Status.URL, gitRepo.Status.HostKeys, nil -} - -// SecretSortList to sort secrets type SecretSortList corev1.SecretList func (s SecretSortList) Len() int { return len(s.Items) } @@ -109,71 +17,3 @@ func (s SecretSortList) Less(i, j int) bool { return s.Items[i].CreationTimestamp.Before(&s.Items[j].CreationTimestamp) } - -// SliceContainsString checks if the slice of strings contains a specific string -func SliceContainsString(list []string, s string) bool { - for _, v := range list { - if v == s { - return true - } - } - return false -} - -// HandleDeletion will handle the finalizers if the object was deleted. -// It will return true, if the finalizer was removed. If the object was -// removed the reconcile can be returned. -func HandleDeletion(instance metav1.Object, finalizerName string, client client.Client) DeletionState { - if instance.GetDeletionTimestamp().IsZero() { - return DeletionState{FinalizerRemoved: false, Deleted: false} - } - - annotationValue, exists := instance.GetAnnotations()[DeleteProtectionAnnotation] - - var protected bool - var err error - if exists { - protected, err = strconv.ParseBool(annotationValue) - // Assume true if it can't be parsed - if err != nil { - protected = true - // We need to reset the error again, so we don't trigger any unwanted side effects... - err = nil - } - } else { - protected = false - } - - if SliceContainsString(instance.GetFinalizers(), finalizerName) && !protected { - - controllerutil.RemoveFinalizer(instance, finalizerName) - - return DeletionState{Deleted: true, FinalizerRemoved: true} - } - - return DeletionState{Deleted: true, FinalizerRemoved: false} -} - -// AddDeletionProtection annotations to the instance -func AddDeletionProtection(instance metav1.Object) { - config := os.Getenv(protectionSettingEnvVar) - - protected, err := strconv.ParseBool(config) - if err != nil { - protected = true - } - - if protected { - annotations := instance.GetAnnotations() - - if annotations == nil { - annotations = make(map[string]string) - } - - if _, ok := annotations[DeleteProtectionAnnotation]; !ok { - annotations[DeleteProtectionAnnotation] = "true" - } - - instance.SetAnnotations(annotations) - } -} diff --git a/pkg/pipeline/cluster.go b/pkg/pipeline/cluster.go new file mode 100644 index 00000000..1f47367e --- /dev/null +++ b/pkg/pipeline/cluster.go @@ -0,0 +1,232 @@ +package pipeline + +import ( + "context" + "crypto/rand" + "encoding/base64" + "fmt" + "os" + "strings" + "time" + + synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" + "github.com/projectsyn/lieutenant-operator/pkg/git/manager" + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" +) + +const ( + clusterClassContent = `classes: +- %s.%s +` +) + +func clusterSpecificSteps(obj PipelineObject, data *ExecutionContext) ExecutionResult { + result := createClusterRBAC(obj, data) + if resultNotOK(result) { + result.Err = wrapError("create cluster RBAC", result.Err) + return result + } + + result = checkIfDeleted(obj, data) + if resultNotOK(result) { + result.Err = wrapError("deletion check", result.Err) + return result + } + + result = setBootstrapToken(obj, data) + if resultNotOK(result) { + result.Err = wrapError("set bootstrap token", result.Err) + return result + } + + if strings.ToLower(os.Getenv("SKIP_VAULT_SETUP")) != "true" { + result = createOrUpdateVault(obj, data) + if resultNotOK(result) { + result.Err = wrapError("create or update vault", result.Err) + return result + } + + result = handleVaultDeletion(obj, data) + if resultNotOK(result) { + result.Err = wrapError("delete vault entries", result.Err) + return result + } + + } + + result = updateTenantGitRepo(obj, data) + if resultNotOK(result) { + result.Err = wrapError("update tenant git repo", result.Err) + return result + } + + result = removeClusterFileFromTenant(obj, data) + if resultNotOK(result) { + result.Err = wrapError("set bootstrap token", result.Err) + return result + } + + return ExecutionResult{} +} + +func createClusterRBAC(obj PipelineObject, data *ExecutionContext) ExecutionResult { + objMeta := metav1.ObjectMeta{ + Name: obj.GetObjectMeta().GetName(), + Namespace: obj.GetObjectMeta().GetNamespace(), + OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(obj.GetObjectMeta(), synv1alpha1.SchemeBuilder.GroupVersion.WithKind("Cluster"))}, + } + serviceAccount := &corev1.ServiceAccount{ObjectMeta: objMeta} + role := &rbacv1.Role{ + ObjectMeta: objMeta, + Rules: []rbacv1.PolicyRule{{ + APIGroups: []string{synv1alpha1.SchemeGroupVersion.Group}, + Resources: []string{"clusters"}, + Verbs: []string{"get", "update"}, + ResourceNames: []string{obj.GetObjectMeta().GetName()}, + }}, + } + roleBinding := &rbacv1.RoleBinding{ + ObjectMeta: objMeta, + RoleRef: rbacv1.RoleRef{ + Kind: "Role", + Name: role.Name, + }, + Subjects: []rbacv1.Subject{{ + Kind: "ServiceAccount", + Name: serviceAccount.Name, + Namespace: serviceAccount.Namespace, + }}, + } + for _, item := range []runtime.Object{serviceAccount, role, roleBinding} { + if err := data.Client.Create(context.TODO(), item); err != nil && !errors.IsAlreadyExists(err) { + return ExecutionResult{Err: err} + } + } + return ExecutionResult{} +} + +func setBootstrapToken(obj PipelineObject, data *ExecutionContext) ExecutionResult { + + instance, ok := obj.(*synv1alpha1.Cluster) + if !ok { + return ExecutionResult{Err: fmt.Errorf("%s is not a cluster object", obj.GetObjectMeta().GetName())} + } + + if instance.Status.BootstrapToken == nil { + data.Log.Info("Adding status to Cluster object") + err := newClusterStatus(instance) + if err != nil { + return ExecutionResult{Err: err} + } + } + + if time.Now().After(instance.Status.BootstrapToken.ValidUntil.Time) { + instance.Status.BootstrapToken.TokenValid = false + } + + return ExecutionResult{} + +} + +//newClusterStatus will create a default lifetime of 30 minutes if it wasn't set in the object. +func newClusterStatus(cluster *synv1alpha1.Cluster) error { + + parseTime := "30m" + if cluster.Spec.TokenLifeTime != "" { + parseTime = cluster.Spec.TokenLifeTime + } + + duration, err := time.ParseDuration(parseTime) + if err != nil { + return err + } + + validUntil := time.Now().Add(duration) + + token, err := generateToken() + if err != nil { + return err + } + + cluster.Status.BootstrapToken = &synv1alpha1.BootstrapToken{ + Token: token, + ValidUntil: metav1.NewTime(validUntil), + TokenValid: true, + } + return nil +} + +func generateToken() (string, error) { + b := make([]byte, 16) + _, err := rand.Read(b) + if err != nil { + return "", err + } + return base64.URLEncoding.EncodeToString(b), err +} + +// TODO: this should get removed and switched to a poll system on the tenant +func removeClusterFileFromTenant(obj PipelineObject, data *ExecutionContext) ExecutionResult { + if !data.Deleted { + return ExecutionResult{} + } + + tenantCR, err := getTenantCR(obj, data) + if err != nil { + return ExecutionResult{Err: err} + } + + fileName := obj.GetObjectMeta().GetName() + ".yml" + + if tenantCR.Spec.GitRepoTemplate.TemplateFiles == nil { + return ExecutionResult{} + } + + if _, ok := tenantCR.Spec.GitRepoTemplate.TemplateFiles[fileName]; ok { + tenantCR.Spec.GitRepoTemplate.TemplateFiles[fileName] = manager.DeletionMagicString + err := data.Client.Update(context.TODO(), tenantCR) + if err != nil { + return ExecutionResult{Err: err} + } + } + + return ExecutionResult{} +} + +func getTenantCR(obj PipelineObject, data *ExecutionContext) (*synv1alpha1.Tenant, error) { + + tenant := types.NamespacedName{ + Name: obj.GetTenantRef().Name, + Namespace: obj.GetObjectMeta().GetNamespace(), + } + + tenantCR := &synv1alpha1.Tenant{} + return tenantCR, data.Client.Get(context.TODO(), tenant, tenantCR) +} + +// TODO: also poll system here... +func updateTenantGitRepo(obj PipelineObject, data *ExecutionContext) ExecutionResult { + + tenantCR, err := getTenantCR(obj, data) + if err != nil { + return ExecutionResult{Err: err} + } + + if tenantCR.Spec.GitRepoTemplate.TemplateFiles == nil { + tenantCR.Spec.GitRepoTemplate.TemplateFiles = map[string]string{} + } + + clusterClassFile := obj.GetObjectMeta().GetName() + ".yml" + if _, ok := tenantCR.Spec.GitRepoTemplate.TemplateFiles[clusterClassFile]; !ok { + fileContent := fmt.Sprintf(clusterClassContent, obj.GetTenantRef().Name, CommonClassName) + tenantCR.Spec.GitRepoTemplate.TemplateFiles[clusterClassFile] = fileContent + err := data.Client.Update(context.TODO(), tenantCR) + return ExecutionResult{Err: err} + } + return ExecutionResult{} +} diff --git a/pkg/pipeline/common.go b/pkg/pipeline/common.go new file mode 100644 index 00000000..7ba876b2 --- /dev/null +++ b/pkg/pipeline/common.go @@ -0,0 +1,203 @@ +// pipeline contains pipelines that define all the steps that a CRD has to go +// through in order to be considered reconciled. + +package pipeline + +import ( + "context" + "fmt" + "os" + "strconv" + + "github.com/projectsyn/lieutenant-operator/pkg/apis" + synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" +) + +const ( + protectionSettingEnvVar = "LIEUTENANT_DELETE_PROTECTION" + DeleteProtectionAnnotation = "syn.tools/protected-delete" +) + +func getDefaultDeletionPolicy() synv1alpha1.DeletionPolicy { + policy := synv1alpha1.DeletionPolicy(os.Getenv("DEFAULT_DELETION_POLICY")) + switch policy { + case synv1alpha1.ArchivePolicy, synv1alpha1.DeletePolicy, synv1alpha1.RetainPolicy: + return policy + default: + return synv1alpha1.ArchivePolicy + } +} + +func addDeletionProtection(instance PipelineObject, data *ExecutionContext) ExecutionResult { + + if data.Deleted { + return ExecutionResult{} + } + + config := os.Getenv(protectionSettingEnvVar) + + protected, err := strconv.ParseBool(config) + if err != nil { + protected = true + } + + if protected { + annotations := instance.GetObjectMeta().GetAnnotations() + + if annotations == nil { + annotations = make(map[string]string) + } + + if _, ok := annotations[DeleteProtectionAnnotation]; !ok { + annotations[DeleteProtectionAnnotation] = "true" + } + + instance.GetObjectMeta().SetAnnotations(annotations) + } + + return ExecutionResult{} + +} + +// addTenantLabel adds the tenant label to an object. +func addTenantLabel(obj PipelineObject, data *ExecutionContext) ExecutionResult { + labels := obj.GetObjectMeta().GetLabels() + if labels == nil { + labels = make(map[string]string) + } + if labels[apis.LabelNameTenant] != obj.GetTenantRef().Name { + labels[apis.LabelNameTenant] = obj.GetTenantRef().Name + } + + obj.GetObjectMeta().SetLabels(labels) + + return ExecutionResult{} +} + +func updateObject(obj PipelineObject, data *ExecutionContext) ExecutionResult { + + rtObj, ok := obj.(runtime.Object) + if !ok { + return ExecutionResult{ + Abort: true, + Err: fmt.Errorf("object ist not a valid runtime.object: %v", obj.GetObjectMeta().GetName()), + } + } + + err := data.Client.Update(context.TODO(), rtObj) + if err != nil { + return ExecutionResult{Err: err} + } + + err = data.Client.Status().Update(context.TODO(), rtObj) + return ExecutionResult{Abort: true, Err: err} +} + +func common(obj PipelineObject, data *ExecutionContext) ExecutionResult { + + result := handleDeletion(obj.GetObjectMeta(), data) + if result.Abort || result.Err != nil { + result.Err = wrapError("deletion", result.Err) + return result + } + + result = addTenantLabel(obj, data) + if result.Abort || result.Err != nil { + result.Err = wrapError("add tenant label", result.Err) + return result + } + + result = addDeletionProtection(obj, data) + if result.Abort || result.Err != nil { + result.Err = wrapError("add deletion protection", result.Err) + return result + } + + result = handleFinalizer(obj, data) + if result.Abort || result.Err != nil { + result.Err = wrapError("add deletion protection", result.Err) + return result + } + + result = updateObject(obj, data) + if result.Abort || result.Err != nil { + result.Err = wrapError("update object", result.Err) + return result + } + + return ExecutionResult{} +} + +func wrapError(name string, err error) error { + if err == nil { + return nil + } + return fmt.Errorf("step %s failed: %w", name, err) +} + +func resultNotOK(result ExecutionResult) bool { + return result.Abort || result.Err != nil +} + +// handleDeletion will handle the finalizers if the object was deleted. +// It will only trigger if data.Deleted is true. +func handleDeletion(instance metav1.Object, data *ExecutionContext) ExecutionResult { + if !data.Deleted { + return ExecutionResult{} + } + + annotationValue, exists := instance.GetAnnotations()[DeleteProtectionAnnotation] + + var protected bool + var err error + if exists { + protected, err = strconv.ParseBool(annotationValue) + // Assume true if it can't be parsed + if err != nil { + protected = true + // We need to reset the error again, so we don't trigger any unwanted side effects... + err = nil + } + } else { + protected = false + } + + if sliceContainsString(instance.GetFinalizers(), data.FinalizerName) && !protected { + + data.Deleted = true + return ExecutionResult{} + } + + return ExecutionResult{Err: fmt.Errorf("finalzier was not removed")} +} + +// Checks if the slice of strings contains a specific string +func sliceContainsString(list []string, s string) bool { + for _, v := range list { + if v == s { + return true + } + } + return false +} + +func checkIfDeleted(obj PipelineObject, data *ExecutionContext) ExecutionResult { + if !obj.GetObjectMeta().GetDeletionTimestamp().IsZero() { + data.Deleted = true + + } + return ExecutionResult{} + +} + +func handleFinalizer(obj PipelineObject, data *ExecutionContext) ExecutionResult { + if data.FinalizerName != "" && !data.Deleted { + controllerutil.AddFinalizer(obj.GetObjectMeta(), data.FinalizerName) + } else { + controllerutil.RemoveFinalizer(obj.GetObjectMeta(), data.FinalizerName) + } + return ExecutionResult{} +} diff --git a/pkg/pipeline/git.go b/pkg/pipeline/git.go new file mode 100644 index 00000000..59cc157c --- /dev/null +++ b/pkg/pipeline/git.go @@ -0,0 +1,189 @@ +package pipeline + +import ( + "context" + "fmt" + + synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" + "github.com/projectsyn/lieutenant-operator/pkg/git/manager" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +func gitRepoSpecificSteps(obj PipelineObject, data *ExecutionContext) ExecutionResult { + + instance, ok := obj.(*synv1alpha1.GitRepo) + if !ok { + return ExecutionResult{Err: fmt.Errorf("object is not a GitRepository")} + } + + repo, hostKeys, err := manager.GetGitClient(&instance.Spec.GitRepoTemplate, instance.GetNamespace(), data.Log, data.Client) + if err != nil { + return ExecutionResult{Err: err} + } + + instance.Status.HostKeys = hostKeys + + if !repoExists(repo) { + data.Log.Info("creating git repo", manager.SecretEndpointName, repo.FullURL()) + err := repo.Create() + if err != nil { + return ExecutionResult{Err: handleRepoError(err, instance, repo, data.Client)} + + } + data.Log.Info("successfully created the repository") + } + + if data.Deleted { + err := repo.Remove() + if err != nil { + return ExecutionResult{Err: err} + } + return ExecutionResult{} + } + + err = repo.CommitTemplateFiles() + if err != nil { + return ExecutionResult{Err: handleRepoError(err, instance, repo, data.Client)} + } + + changed, err := repo.Update() + if err != nil { + return ExecutionResult{Err: err} + } + + if changed { + data.Log.Info("keys differed from CRD, keys re-applied to repository") + } + + phase := synv1alpha1.Created + instance.Status.Phase = &phase + instance.Status.URL = repo.FullURL().String() + instance.Status.Type = synv1alpha1.GitType(repo.Type()) + + return ExecutionResult{} +} + +// createOrUpdateGitRepo will create the gitRepo object if it doesn't already exist. +func createOrUpdateGitRepo(obj PipelineObject, data *ExecutionContext) ExecutionResult { + + template := obj.GetGitTemplate() + + if template.DisplayName == "" { + template.DisplayName = obj.GetDisplayName() + } + + if template == nil { + return ExecutionResult{ + Abort: true, + Err: fmt.Errorf("gitRepo template is empty"), + } + } + + if obj.GetTenantRef().Name == "" { + return ExecutionResult{ + Abort: true, + Err: fmt.Errorf("the tenant name is empty"), + } + } + + if template.DeletionPolicy == "" { + if obj.GetDeletionPolicy() == "" { + template.DeletionPolicy = getDefaultDeletionPolicy() + } else { + template.DeletionPolicy = obj.GetDeletionPolicy() + } + } + + if template.RepoType == synv1alpha1.DefaultRepoType { + template.RepoType = synv1alpha1.AutoRepoType + } + + repo := &synv1alpha1.GitRepo{ + ObjectMeta: metav1.ObjectMeta{ + Name: obj.GetObjectMeta().GetName(), + Namespace: obj.GetObjectMeta().GetNamespace(), + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(obj.GetObjectMeta(), obj.GroupVersionKind()), + }, + }, + Spec: synv1alpha1.GitRepoSpec{ + GitRepoTemplate: *template, + TenantRef: obj.GetTenantRef(), + }, + } + + addDeletionProtection(repo, data) + + err := data.Client.Create(context.TODO(), repo) + if err != nil && errors.IsAlreadyExists(err) { + existingRepo := &synv1alpha1.GitRepo{} + + namespacedName := types.NamespacedName{ + Name: repo.GetName(), + Namespace: repo.GetNamespace(), + } + + err = data.Client.Get(context.TODO(), namespacedName, existingRepo) + if err != nil { + return ExecutionResult{ + Abort: true, + Err: fmt.Errorf("could not update existing repo: %w", err), + } + } + existingRepo.Spec = repo.Spec + + err = data.Client.Update(context.TODO(), existingRepo) + + } + + for file, content := range template.TemplateFiles { + if content == manager.DeletionMagicString { + delete(template.TemplateFiles, file) + } + } + + return ExecutionResult{ + Abort: false, + Err: err, + } +} + +func setGitRepoURLAndHostKeys(obj PipelineObject, data *ExecutionContext) ExecutionResult { + gitRepo := &synv1alpha1.GitRepo{} + repoNamespacedName := types.NamespacedName{ + Namespace: obj.GetObjectMeta().GetNamespace(), + Name: obj.GetObjectMeta().GetName(), + } + err := data.Client.Get(context.TODO(), repoNamespacedName, gitRepo) + if err != nil { + if errors.IsNotFound(err) { + return ExecutionResult{} + } + return ExecutionResult{Abort: true, Err: err} + } + + obj.SetGitRepoURLAndHostKeys(gitRepo.Status.URL, gitRepo.Status.HostKeys) + + return ExecutionResult{} +} + +func repoExists(repo manager.Repo) bool { + if err := repo.Read(); err == nil { + return true + } + + return false +} + +func handleRepoError(err error, instance *synv1alpha1.GitRepo, repo manager.Repo, client client.Client) error { + phase := synv1alpha1.Failed + instance.Status.Phase = &phase + instance.Status.URL = repo.FullURL().String() + if updateErr := client.Status().Update(context.TODO(), instance); updateErr != nil { + return fmt.Errorf("could not set status while handling error: %s: %s", updateErr, err) + } + return err +} diff --git a/pkg/pipeline/pipelines.go b/pkg/pipeline/pipelines.go new file mode 100644 index 00000000..23dac757 --- /dev/null +++ b/pkg/pipeline/pipelines.go @@ -0,0 +1,73 @@ +package pipeline + +// Function defines the general form of a pipeline function. +type Function func(PipelineObject, *ExecutionContext) ExecutionResult + +func ReconcileTenant(obj PipelineObject, data *ExecutionContext) error { + + result := tenantSpecificSteps(obj, data) + if resultNotOK(result) { + return wrapError("tenant specific steps", result.Err) + } + + result = createOrUpdateGitRepo(obj, data) + if resultNotOK(result) { + return wrapError("create or uptdate git repo", result.Err) + } + + result = setGitRepoURLAndHostKeys(obj, data) + if resultNotOK(result) { + return wrapError("set gitrepo url and hostkeys", result.Err) + } + + result = common(obj, data) + if resultNotOK(result) { + return wrapError("common", result.Err) + } + return nil +} + +func ReconcileCluster(obj PipelineObject, data *ExecutionContext) error { + + result := clusterSpecificSteps(obj, data) + if resultNotOK(result) { + return wrapError("cluster specific steps failes", result.Err) + } + + result = createOrUpdateGitRepo(obj, data) + if resultNotOK(result) { + return wrapError("create or uptdate git repo", result.Err) + } + + result = setGitRepoURLAndHostKeys(obj, data) + if resultNotOK(result) { + return wrapError("set gitrepo url and hostkeys", result.Err) + } + + result = common(obj, data) + if resultNotOK(result) { + return wrapError("common", result.Err) + } + + return nil +} + +func ReconcileGitRep(obj PipelineObject, data *ExecutionContext) error { + + result := checkIfDeleted(obj, data) + if resultNotOK(result) { + return wrapError("deletion check", result.Err) + } + + result = gitRepoSpecificSteps(obj, data) + if resultNotOK(result) { + return wrapError("cluster specific steps failes", result.Err) + } + + result = common(obj, data) + if resultNotOK(result) { + return wrapError("common", result.Err) + } + + return nil +} diff --git a/pkg/pipeline/reconcile_types.go b/pkg/pipeline/reconcile_types.go new file mode 100644 index 00000000..fd1c7fde --- /dev/null +++ b/pkg/pipeline/reconcile_types.go @@ -0,0 +1,36 @@ +package pipeline + +import ( + "github.com/go-logr/logr" + synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// PipelineObject defines an interface to extract necessary information from the CRs +type PipelineObject interface { + GetObjectMeta() metav1.Object + GetGitTemplate() *synv1alpha1.GitRepoTemplate + GroupVersionKind() schema.GroupVersionKind + GetTenantRef() corev1.LocalObjectReference + GetDeletionPolicy() synv1alpha1.DeletionPolicy + GetDisplayName() string + SetGitRepoURLAndHostKeys(URL, hostKeys string) +} + +// ExecutionContext contains additional data about the CRD bein processed. +type ExecutionContext struct { + FinalizerName string + Client client.Client + Log logr.Logger + Deleted bool +} + +// ExecutionResult indicates wether the current execution should be aborted and +// if there was an error. +type ExecutionResult struct { + Abort bool + Err error +} diff --git a/pkg/pipeline/tenant.go b/pkg/pipeline/tenant.go new file mode 100644 index 00000000..4effaa78 --- /dev/null +++ b/pkg/pipeline/tenant.go @@ -0,0 +1,17 @@ +package pipeline + +const ( + // CommonClassName is the name of the tenant's common class + CommonClassName = "common" +) + +func tenantSpecificSteps(obj PipelineObject, data *ExecutionContext) ExecutionResult { + commonClassFile := CommonClassName + ".yml" + if obj.GetGitTemplate().TemplateFiles == nil { + obj.GetGitTemplate().TemplateFiles = map[string]string{} + } + if _, ok := obj.GetGitTemplate().TemplateFiles[commonClassFile]; !ok { + obj.GetGitTemplate().TemplateFiles[commonClassFile] = "" + } + return ExecutionResult{} +} diff --git a/pkg/pipeline/vault.go b/pkg/pipeline/vault.go new file mode 100644 index 00000000..121ef065 --- /dev/null +++ b/pkg/pipeline/vault.go @@ -0,0 +1,92 @@ +package pipeline + +import ( + "context" + "fmt" + "path" + "sort" + + "github.com/projectsyn/lieutenant-operator/pkg/helpers" + "github.com/projectsyn/lieutenant-operator/pkg/vault" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" +) + +func getVaultClient(obj PipelineObject, data *ExecutionContext) (vault.VaultClient, error) { + deletionPolicy := obj.GetDeletionPolicy() + if deletionPolicy == "" { + deletionPolicy = getDefaultDeletionPolicy() + } + + return vault.NewClient(deletionPolicy, data.Log) +} + +func createOrUpdateVault(obj PipelineObject, data *ExecutionContext) ExecutionResult { + secretPath := path.Join(obj.GetTenantRef().Name, obj.GetObjectMeta().GetName(), "steward") + + token, err := GetServiceAccountToken(obj.GetObjectMeta(), data) + if err != nil { + return ExecutionResult{Err: err} + } + + vaultClient, err := getVaultClient(obj, data) + + err = vaultClient.AddSecrets([]vault.VaultSecret{{Path: secretPath, Value: token}}) + if err != nil { + return ExecutionResult{Err: err} + } + + return ExecutionResult{} +} + +func GetServiceAccountToken(instance metav1.Object, data *ExecutionContext) (string, error) { + secrets := &corev1.SecretList{} + + err := data.Client.List(context.TODO(), secrets) + if err != nil { + return "", err + } + + sortSecrets := helpers.SecretSortList(*secrets) + + sort.Sort(sort.Reverse(sortSecrets)) + + for _, secret := range sortSecrets.Items { + + if secret.Type != corev1.SecretTypeServiceAccountToken { + continue + } + + if secret.Annotations[corev1.ServiceAccountNameKey] == instance.GetName() { + if string(secret.Data["token"]) == "" { + // We'll skip the secrets if the token is not yet populated. + continue + } + return string(secret.Data["token"]), nil + } + } + + return "", fmt.Errorf("no matching secrets found") +} + +func handleVaultDeletion(obj PipelineObject, data *ExecutionContext) ExecutionResult { + repoName := types.NamespacedName{ + Name: obj.GetTenantRef().Name, + Namespace: obj.GetObjectMeta().GetNamespace(), + } + + secretPath := path.Join(repoName.Name, obj.GetObjectMeta().GetName(), "steward") + + if data.Deleted { + vaultClient, err := getVaultClient(obj, data) + if err != nil { + return ExecutionResult{Err: err} + } + err = vaultClient.RemoveSecrets([]vault.VaultSecret{{Path: path.Dir(secretPath), Value: ""}}) + if err != nil { + return ExecutionResult{Err: err} + } + } + return ExecutionResult{} +} From d9645b8c4e895f6681e36c0fb1a03855ac86bbe7 Mon Sep 17 00:00:00 2001 From: Simon Beck Date: Tue, 20 Oct 2020 16:29:58 +0200 Subject: [PATCH 02/23] Fix race conditions Add tenant as owner to clusters --- pkg/controller/cluster/cluster_controller.go | 3 +- pkg/controller/cluster/cluster_reconcile.go | 45 ++++-------- pkg/controller/gitrepo/gitrepo_controller.go | 14 +++- pkg/controller/gitrepo/gitrepo_reconcile.go | 38 ++++------ pkg/controller/tenant/tenant_controller.go | 10 ++- pkg/controller/tenant/tenant_reconcile.go | 38 ++++------ pkg/pipeline/cluster.go | 68 +++--------------- pkg/pipeline/common.go | 8 ++- pkg/pipeline/git.go | 71 ++++++++++++------- pkg/pipeline/pipelines.go | 9 +-- pkg/pipeline/tenant.go | 73 ++++++++++++++++++++ 11 files changed, 202 insertions(+), 175 deletions(-) diff --git a/pkg/controller/cluster/cluster_controller.go b/pkg/controller/cluster/cluster_controller.go index 78a08578..04b0cbbd 100644 --- a/pkg/controller/cluster/cluster_controller.go +++ b/pkg/controller/cluster/cluster_controller.go @@ -38,10 +38,9 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { if err != nil { return err } - return c.Watch(&source.Kind{Type: &synv1alpha1.GitRepo{}}, &handler.EnqueueRequestForOwner{ IsController: true, - OwnerType: &synv1alpha1.Cluster{}, + OwnerType: &synv1alpha1.Tenant{}, }) } diff --git a/pkg/controller/cluster/cluster_reconcile.go b/pkg/controller/cluster/cluster_reconcile.go index 5045f5c8..e5870073 100644 --- a/pkg/controller/cluster/cluster_reconcile.go +++ b/pkg/controller/cluster/cluster_reconcile.go @@ -6,7 +6,6 @@ import ( synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" "github.com/projectsyn/lieutenant-operator/pkg/pipeline" "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) @@ -25,40 +24,22 @@ func (r *ReconcileCluster) Reconcile(request reconcile.Request) (reconcile.Resul reqLogger := log.WithValues("Request.Namespace", request.Namespace, "Request.Name", request.Name) reqLogger.Info("Reconciling Cluster") - err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + instance := &synv1alpha1.Cluster{} - instance := &synv1alpha1.Cluster{} - - err := r.client.Get(context.TODO(), request.NamespacedName, instance) - if err != nil { - if errors.IsNotFound(err) { - return nil - } - return err - } - instanceCopy := instance.DeepCopy() - - nsName := request.NamespacedName - nsName.Name = instance.Spec.TenantRef.Name - - tenant := &synv1alpha1.Tenant{} - - if err := r.client.Get(context.TODO(), nsName, tenant); err != nil { - return fmt.Errorf("Couldn't find tenant: %w", err) - } - - if err := applyClusterTemplate(instance, tenant); err != nil { - return err + err := r.client.Get(context.TODO(), request.NamespacedName, instance) + if err != nil { + if errors.IsNotFound(err) { + return reconcile.Result{}, nil } + return reconcile.Result{}, err + } - data := &pipeline.ExecutionContext{ - Client: r.client, - Log: reqLogger, - FinalizerName: finalizerName, - } + data := &pipeline.ExecutionContext{ + Client: r.client, + Log: reqLogger, + FinalizerName: finalizerName, + } - return pipeline.ReconcileCluster(instance, data) - }) + return reconcile.Result{}, pipeline.ReconcileCluster(instance, data) - return reconcile.Result{}, err } diff --git a/pkg/controller/gitrepo/gitrepo_controller.go b/pkg/controller/gitrepo/gitrepo_controller.go index c4f21ca0..063ae575 100644 --- a/pkg/controller/gitrepo/gitrepo_controller.go +++ b/pkg/controller/gitrepo/gitrepo_controller.go @@ -33,13 +33,23 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { return err } - // Watch for changes to primary resource GitRepo + // Watch for changes to primary resource Tenant err = c.Watch(&source.Kind{Type: &synv1alpha1.GitRepo{}}, &handler.EnqueueRequestForObject{}) if err != nil { return err } - return nil + err = c.Watch(&source.Kind{Type: &synv1alpha1.Cluster{}}, &handler.EnqueueRequestForOwner{ + IsController: true, + OwnerType: &synv1alpha1.GitRepo{}, + }) + if err != nil { + return err + } + return c.Watch(&source.Kind{Type: &synv1alpha1.Tenant{}}, &handler.EnqueueRequestForOwner{ + IsController: true, + OwnerType: &synv1alpha1.GitRepo{}, + }) } // blank assignment to verify that ReconcileGitRepo implements reconcile.Reconciler diff --git a/pkg/controller/gitrepo/gitrepo_reconcile.go b/pkg/controller/gitrepo/gitrepo_reconcile.go index ab45d8dc..82282c43 100644 --- a/pkg/controller/gitrepo/gitrepo_reconcile.go +++ b/pkg/controller/gitrepo/gitrepo_reconcile.go @@ -8,7 +8,6 @@ import ( synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) @@ -25,32 +24,23 @@ func (r *ReconcileGitRepo) Reconcile(request reconcile.Request) (reconcile.Resul reqLogger := log.WithValues("Request.Namespace", request.Namespace, "Request.Name", request.Name) reqLogger.Info("Reconciling GitRepo") - err := retry.RetryOnConflict(retry.DefaultRetry, func() error { - // Fetch the GitRepo instance - instance := &synv1alpha1.GitRepo{} + // Fetch the GitRepo instance + instance := &synv1alpha1.GitRepo{} - err := r.client.Get(context.TODO(), request.NamespacedName, instance) - if err != nil { - if errors.IsNotFound(err) { - return nil - } - return err + err := r.client.Get(context.TODO(), request.NamespacedName, instance) + if err != nil { + if errors.IsNotFound(err) { + return reconcile.Result{}, nil } - instanceCopy := instance.DeepCopy() + return reconcile.Result{}, err + } - if instance.Spec.RepoType == synv1alpha1.UnmanagedRepoType { - reqLogger.Info("Skipping GitRepo because it is unmanaged") - return nil - } - - data := &pipeline.ExecutionContext{ - Client: r.client, - Log: reqLogger, - FinalizerName: finalizerName, - } + data := &pipeline.ExecutionContext{ + Client: r.client, + Log: reqLogger, + FinalizerName: finalizerName, + } - return pipeline.ReconcileGitRep(instance, data) - }) + return reconcile.Result{}, pipeline.ReconcileGitRep(instance, data) - return reconcile.Result{}, err } diff --git a/pkg/controller/tenant/tenant_controller.go b/pkg/controller/tenant/tenant_controller.go index 9facefbb..116e89b5 100644 --- a/pkg/controller/tenant/tenant_controller.go +++ b/pkg/controller/tenant/tenant_controller.go @@ -39,7 +39,15 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { return err } - return c.Watch(&source.Kind{Type: &synv1alpha1.GitRepo{}}, &handler.EnqueueRequestForOwner{ + err = c.Watch(&source.Kind{Type: &synv1alpha1.GitRepo{}}, &handler.EnqueueRequestForOwner{ + IsController: true, + OwnerType: &synv1alpha1.Tenant{}, + }) + if err != nil { + return err + } + + return c.Watch(&source.Kind{Type: &synv1alpha1.Cluster{}}, &handler.EnqueueRequestForOwner{ IsController: true, OwnerType: &synv1alpha1.Tenant{}, }) diff --git a/pkg/controller/tenant/tenant_reconcile.go b/pkg/controller/tenant/tenant_reconcile.go index ab83049d..da23e6f3 100644 --- a/pkg/controller/tenant/tenant_reconcile.go +++ b/pkg/controller/tenant/tenant_reconcile.go @@ -4,7 +4,6 @@ import ( "context" "os" - "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/reconcile" synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" @@ -18,30 +17,21 @@ func (r *ReconcileTenant) Reconcile(request reconcile.Request) (reconcile.Result reqLogger := log.WithValues("Request.Namespace", request.Namespace, "Request.Name", request.Name) reqLogger.Info("Reconciling Tenant") - err := retry.RetryOnConflict(retry.DefaultBackoff, func() error { - // Fetch the Tenant instance - instance := &synv1alpha1.Tenant{} - err := r.client.Get(context.TODO(), request.NamespacedName, instance) - if err != nil { - if errors.IsNotFound(err) { - return nil - } - return err + // Fetch the Tenant instance + instance := &synv1alpha1.Tenant{} + err := r.client.Get(context.TODO(), request.NamespacedName, instance) + if err != nil { + if errors.IsNotFound(err) { + return reconcile.Result{}, nil } - instanceCopy := instance.DeepCopy() + return reconcile.Result{}, err + } - data := &pipeline.ExecutionContext{ - Client: r.client, - Log: reqLogger, - FinalizerName: "", - } - - err = pipeline.ReconcileTenant(instance, data) - if err != nil { - return err - } + data := &pipeline.ExecutionContext{ + Client: r.client, + Log: reqLogger, + FinalizerName: "", + } - return nil - }) - return reconcile.Result{}, err + return reconcile.Result{}, pipeline.ReconcileTenant(instance, data) } diff --git a/pkg/pipeline/cluster.go b/pkg/pipeline/cluster.go index 1f47367e..99e993b4 100644 --- a/pkg/pipeline/cluster.go +++ b/pkg/pipeline/cluster.go @@ -10,7 +10,6 @@ import ( "time" synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" - "github.com/projectsyn/lieutenant-operator/pkg/git/manager" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -59,15 +58,9 @@ func clusterSpecificSteps(obj PipelineObject, data *ExecutionContext) ExecutionR } - result = updateTenantGitRepo(obj, data) + result = setTenantOwner(obj, data) if resultNotOK(result) { - result.Err = wrapError("update tenant git repo", result.Err) - return result - } - - result = removeClusterFileFromTenant(obj, data) - if resultNotOK(result) { - result.Err = wrapError("set bootstrap token", result.Err) + result.Err = wrapError("set tenant owner", result.Err) return result } @@ -170,63 +163,20 @@ func generateToken() (string, error) { return base64.URLEncoding.EncodeToString(b), err } -// TODO: this should get removed and switched to a poll system on the tenant -func removeClusterFileFromTenant(obj PipelineObject, data *ExecutionContext) ExecutionResult { - if !data.Deleted { - return ExecutionResult{} - } - - tenantCR, err := getTenantCR(obj, data) - if err != nil { - return ExecutionResult{Err: err} - } - - fileName := obj.GetObjectMeta().GetName() + ".yml" - - if tenantCR.Spec.GitRepoTemplate.TemplateFiles == nil { - return ExecutionResult{} - } - - if _, ok := tenantCR.Spec.GitRepoTemplate.TemplateFiles[fileName]; ok { - tenantCR.Spec.GitRepoTemplate.TemplateFiles[fileName] = manager.DeletionMagicString - err := data.Client.Update(context.TODO(), tenantCR) - if err != nil { - return ExecutionResult{Err: err} - } - } - - return ExecutionResult{} -} - -func getTenantCR(obj PipelineObject, data *ExecutionContext) (*synv1alpha1.Tenant, error) { - - tenant := types.NamespacedName{ - Name: obj.GetTenantRef().Name, - Namespace: obj.GetObjectMeta().GetNamespace(), - } +func setTenantOwner(obj PipelineObject, data *ExecutionContext) ExecutionResult { - tenantCR := &synv1alpha1.Tenant{} - return tenantCR, data.Client.Get(context.TODO(), tenant, tenantCR) -} + tenant := &synv1alpha1.Tenant{} -// TODO: also poll system here... -func updateTenantGitRepo(obj PipelineObject, data *ExecutionContext) ExecutionResult { + tenantName := types.NamespacedName{Name: obj.GetTenantRef().Name, Namespace: obj.GetObjectMeta().GetNamespace()} - tenantCR, err := getTenantCR(obj, data) + err := data.Client.Get(context.TODO(), tenantName, tenant) if err != nil { return ExecutionResult{Err: err} } - if tenantCR.Spec.GitRepoTemplate.TemplateFiles == nil { - tenantCR.Spec.GitRepoTemplate.TemplateFiles = map[string]string{} - } + obj.GetObjectMeta().SetOwnerReferences([]metav1.OwnerReference{ + *metav1.NewControllerRef(tenant.GetObjectMeta(), tenant.GroupVersionKind()), + }) - clusterClassFile := obj.GetObjectMeta().GetName() + ".yml" - if _, ok := tenantCR.Spec.GitRepoTemplate.TemplateFiles[clusterClassFile]; !ok { - fileContent := fmt.Sprintf(clusterClassContent, obj.GetTenantRef().Name, CommonClassName) - tenantCR.Spec.GitRepoTemplate.TemplateFiles[clusterClassFile] = fileContent - err := data.Client.Update(context.TODO(), tenantCR) - return ExecutionResult{Err: err} - } return ExecutionResult{} } diff --git a/pkg/pipeline/common.go b/pkg/pipeline/common.go index 7ba876b2..5161c20e 100644 --- a/pkg/pipeline/common.go +++ b/pkg/pipeline/common.go @@ -79,6 +79,8 @@ func addTenantLabel(obj PipelineObject, data *ExecutionContext) ExecutionResult func updateObject(obj PipelineObject, data *ExecutionContext) ExecutionResult { + resourceVersion := obj.GetObjectMeta().GetResourceVersion() + rtObj, ok := obj.(runtime.Object) if !ok { return ExecutionResult{ @@ -92,7 +94,11 @@ func updateObject(obj PipelineObject, data *ExecutionContext) ExecutionResult { return ExecutionResult{Err: err} } - err = data.Client.Status().Update(context.TODO(), rtObj) + // Updating the status if either there were changes or the object is deleted will + // lead to some race conditions. By checking first we can avoid them. + if resourceVersion == obj.GetObjectMeta().GetResourceVersion() && !data.Deleted { + err = data.Client.Status().Update(context.TODO(), rtObj) + } return ExecutionResult{Abort: true, Err: err} } diff --git a/pkg/pipeline/git.go b/pkg/pipeline/git.go index 59cc157c..f69bc419 100644 --- a/pkg/pipeline/git.go +++ b/pkg/pipeline/git.go @@ -19,6 +19,11 @@ func gitRepoSpecificSteps(obj PipelineObject, data *ExecutionContext) ExecutionR return ExecutionResult{Err: fmt.Errorf("object is not a GitRepository")} } + err := fetchGitRepoTemplate(instance, data) + if err != nil { + return ExecutionResult{Err: err} + } + repo, hostKeys, err := manager.GetGitClient(&instance.Spec.GitRepoTemplate, instance.GetNamespace(), data.Log, data.Client) if err != nil { return ExecutionResult{Err: err} @@ -66,8 +71,8 @@ func gitRepoSpecificSteps(obj PipelineObject, data *ExecutionContext) ExecutionR return ExecutionResult{} } -// createOrUpdateGitRepo will create the gitRepo object if it doesn't already exist. -func createOrUpdateGitRepo(obj PipelineObject, data *ExecutionContext) ExecutionResult { +// createGitRepo will create the gitRepo object if it doesn't already exist. +func createGitRepo(obj PipelineObject, data *ExecutionContext) ExecutionResult { template := obj.GetGitTemplate() @@ -115,28 +120,11 @@ func createOrUpdateGitRepo(obj PipelineObject, data *ExecutionContext) Execution }, } - addDeletionProtection(repo, data) - err := data.Client.Create(context.TODO(), repo) - if err != nil && errors.IsAlreadyExists(err) { - existingRepo := &synv1alpha1.GitRepo{} - - namespacedName := types.NamespacedName{ - Name: repo.GetName(), - Namespace: repo.GetNamespace(), - } - - err = data.Client.Get(context.TODO(), namespacedName, existingRepo) - if err != nil { - return ExecutionResult{ - Abort: true, - Err: fmt.Errorf("could not update existing repo: %w", err), - } + if err != nil { + if errors.IsAlreadyExists(err) { + return ExecutionResult{} } - existingRepo.Spec = repo.Spec - - err = data.Client.Update(context.TODO(), existingRepo) - } for file, content := range template.TemplateFiles { @@ -145,10 +133,7 @@ func createOrUpdateGitRepo(obj PipelineObject, data *ExecutionContext) Execution } } - return ExecutionResult{ - Abort: false, - Err: err, - } + return ExecutionResult{Err: err} } func setGitRepoURLAndHostKeys(obj PipelineObject, data *ExecutionContext) ExecutionResult { @@ -187,3 +172,37 @@ func handleRepoError(err error, instance *synv1alpha1.GitRepo, repo manager.Repo } return err } + +func fetchGitRepoTemplate(obj *synv1alpha1.GitRepo, data *ExecutionContext) error { + tenant := &synv1alpha1.Tenant{} + + tenantName := types.NamespacedName{Name: obj.GetObjectMeta().GetName(), Namespace: obj.GetObjectMeta().GetNamespace()} + + err := data.Client.Get(context.TODO(), tenantName, tenant) + if err != nil { + if !errors.IsNotFound(err) { + return err + } + } + + if tenant != nil && tenant.Spec.GitRepoTemplate != nil { + obj.Spec.GitRepoTemplate = *tenant.Spec.GitRepoTemplate + } + + cluster := &synv1alpha1.Cluster{} + + clusterName := types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()} + + err = data.Client.Get(context.TODO(), clusterName, cluster) + if err != nil { + if !errors.IsNotFound(err) { + return err + } + } + + if cluster != nil && cluster.Spec.GitRepoTemplate != nil { + obj.Spec.GitRepoTemplate = *cluster.Spec.GitRepoTemplate + } + + return nil +} diff --git a/pkg/pipeline/pipelines.go b/pkg/pipeline/pipelines.go index 23dac757..4c15c13c 100644 --- a/pkg/pipeline/pipelines.go +++ b/pkg/pipeline/pipelines.go @@ -10,9 +10,9 @@ func ReconcileTenant(obj PipelineObject, data *ExecutionContext) error { return wrapError("tenant specific steps", result.Err) } - result = createOrUpdateGitRepo(obj, data) + result = createGitRepo(obj, data) if resultNotOK(result) { - return wrapError("create or uptdate git repo", result.Err) + return wrapError("create git repo", result.Err) } result = setGitRepoURLAndHostKeys(obj, data) @@ -29,12 +29,13 @@ func ReconcileTenant(obj PipelineObject, data *ExecutionContext) error { func ReconcileCluster(obj PipelineObject, data *ExecutionContext) error { + //TODO: the cluster has to get the right tenant and set it as its owner result := clusterSpecificSteps(obj, data) if resultNotOK(result) { return wrapError("cluster specific steps failes", result.Err) } - result = createOrUpdateGitRepo(obj, data) + result = createGitRepo(obj, data) if resultNotOK(result) { return wrapError("create or uptdate git repo", result.Err) } @@ -61,7 +62,7 @@ func ReconcileGitRep(obj PipelineObject, data *ExecutionContext) error { result = gitRepoSpecificSteps(obj, data) if resultNotOK(result) { - return wrapError("cluster specific steps failes", result.Err) + return wrapError("git repo specific steps failes", result.Err) } result = common(obj, data) diff --git a/pkg/pipeline/tenant.go b/pkg/pipeline/tenant.go index 4effaa78..1b8a8197 100644 --- a/pkg/pipeline/tenant.go +++ b/pkg/pipeline/tenant.go @@ -1,11 +1,40 @@ package pipeline +import ( + "context" + "fmt" + + "github.com/projectsyn/lieutenant-operator/pkg/apis" + synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" + "github.com/projectsyn/lieutenant-operator/pkg/git/manager" + "k8s.io/apimachinery/pkg/labels" + "sigs.k8s.io/controller-runtime/pkg/client" +) + const ( // CommonClassName is the name of the tenant's common class CommonClassName = "common" ) func tenantSpecificSteps(obj PipelineObject, data *ExecutionContext) ExecutionResult { + + result := addDefaultClassFile(obj, data) + if resultNotOK(result) { + result.Err = wrapError("add default class file", result.Err) + return result + } + + result = updateTenantGitRepo(obj, data) + if resultNotOK(result) { + result.Err = wrapError("update tenant git repo", result.Err) + return result + } + + return ExecutionResult{} + +} + +func addDefaultClassFile(obj PipelineObject, data *ExecutionContext) ExecutionResult { commonClassFile := CommonClassName + ".yml" if obj.GetGitTemplate().TemplateFiles == nil { obj.GetGitTemplate().TemplateFiles = map[string]string{} @@ -15,3 +44,47 @@ func tenantSpecificSteps(obj PipelineObject, data *ExecutionContext) ExecutionRe } return ExecutionResult{} } + +func updateTenantGitRepo(obj PipelineObject, data *ExecutionContext) ExecutionResult { + + tenantCR, ok := obj.(*synv1alpha1.Tenant) + if !ok { + return ExecutionResult{Err: fmt.Errorf("object is not a tenant")} + } + + oldFiles := tenantCR.Spec.GitRepoTemplate.TemplateFiles + + tenantCR.Spec.GitRepoTemplate.TemplateFiles = map[string]string{} + + clusterList := &synv1alpha1.ClusterList{} + + selector := labels.Set(map[string]string{apis.LabelNameTenant: tenantCR.Name}).AsSelector() + + listOptions := &client.ListOptions{ + LabelSelector: selector, + Namespace: obj.GetObjectMeta().GetNamespace(), + } + + err := data.Client.List(context.TODO(), clusterList, listOptions) + if err != nil { + return ExecutionResult{Err: err} + } + + for _, cluster := range clusterList.Items { + fileName := cluster.GetName() + ".yml" + fileContent := fmt.Sprintf(clusterClassContent, cluster.GetName(), CommonClassName) + tenantCR.Spec.GitRepoTemplate.TemplateFiles[fileName] = fileContent + delete(oldFiles, fileName) + } + + for fileName := range oldFiles { + if fileName == CommonClassName+".yml" { + tenantCR.Spec.GitRepoTemplate.TemplateFiles[CommonClassName+".yml"] = "" + } else { + tenantCR.Spec.GitRepoTemplate.TemplateFiles[fileName] = manager.DeletionMagicString + + } + } + + return ExecutionResult{} +} From 38bed2a2563248bb21af0b214723b5339c1bf4e8 Mon Sep 17 00:00:00 2001 From: Simon Beck Date: Wed, 21 Oct 2020 11:55:30 +0200 Subject: [PATCH 03/23] Make git repos watch tenants and clusters With this change the pull methods now work instantly. If a `cluster` is created, it will trigger a `tenant` reconcile via owner reference. The tenant reconcile in turn triggers a gitrepo reconcile via the `EnqueueRequestsFromMapFunc` and watches on both `clusters` and `tenants`. --- pkg/controller/gitrepo/gitrepo_controller.go | 27 +++++++++++++++----- pkg/controller/tenant/tenant_reconcile.go | 1 - 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/pkg/controller/gitrepo/gitrepo_controller.go b/pkg/controller/gitrepo/gitrepo_controller.go index 063ae575..a0608983 100644 --- a/pkg/controller/gitrepo/gitrepo_controller.go +++ b/pkg/controller/gitrepo/gitrepo_controller.go @@ -3,6 +3,7 @@ package gitrepo import ( synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/handler" @@ -39,17 +40,31 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { return err } - err = c.Watch(&source.Kind{Type: &synv1alpha1.Cluster{}}, &handler.EnqueueRequestForOwner{ - IsController: true, - OwnerType: &synv1alpha1.GitRepo{}, + err = c.Watch(&source.Kind{Type: &synv1alpha1.Tenant{}}, &handler.EnqueueRequestsFromMapFunc{ + ToRequests: handler.ToRequestsFunc(func(a handler.MapObject) []reconcile.Request { + return []reconcile.Request{ + {NamespacedName: types.NamespacedName{ + Name: a.Meta.GetName(), + Namespace: a.Meta.GetNamespace(), + }}, + } + }), }) if err != nil { return err } - return c.Watch(&source.Kind{Type: &synv1alpha1.Tenant{}}, &handler.EnqueueRequestForOwner{ - IsController: true, - OwnerType: &synv1alpha1.GitRepo{}, + + return c.Watch(&source.Kind{Type: &synv1alpha1.Cluster{}}, &handler.EnqueueRequestsFromMapFunc{ + ToRequests: handler.ToRequestsFunc(func(a handler.MapObject) []reconcile.Request { + return []reconcile.Request{ + {NamespacedName: types.NamespacedName{ + Name: a.Meta.GetName(), + Namespace: a.Meta.GetNamespace(), + }}, + } + }), }) + } // blank assignment to verify that ReconcileGitRepo implements reconcile.Reconciler diff --git a/pkg/controller/tenant/tenant_reconcile.go b/pkg/controller/tenant/tenant_reconcile.go index da23e6f3..6ee605f6 100644 --- a/pkg/controller/tenant/tenant_reconcile.go +++ b/pkg/controller/tenant/tenant_reconcile.go @@ -2,7 +2,6 @@ package tenant import ( "context" - "os" "sigs.k8s.io/controller-runtime/pkg/reconcile" From 8c2264880c560e6ece6659e7821cf8226247de17 Mon Sep 17 00:00:00 2001 From: Simon Beck Date: Wed, 21 Oct 2020 09:38:02 +0200 Subject: [PATCH 04/23] Cherry pick changes since refactor --- go.mod | 1 + go.sum | 1 + pkg/controller/gitrepo/gitrepo_reconcile.go | 1 - pkg/helpers/values.go | 14 ---------- pkg/helpers/values_test.go | 13 --------- pkg/pipeline/cluster.go | 27 +++++++++++++++++++ .../cluster/cluster_template.go | 2 +- .../cluster/cluster_template_test.go | 8 +++--- pkg/pipeline/common_test.go | 20 ++++++++++++++ pkg/pipeline/git.go | 9 +++++++ pkg/pipeline/tenant.go | 24 ++++++++++++++++- 11 files changed, 86 insertions(+), 34 deletions(-) rename pkg/{controller => pipeline}/cluster/cluster_template.go (94%) rename pkg/{controller => pipeline}/cluster/cluster_template_test.go (93%) create mode 100644 pkg/pipeline/common_test.go diff --git a/go.mod b/go.mod index a9b0ccf0..16ab1877 100644 --- a/go.mod +++ b/go.mod @@ -13,6 +13,7 @@ require ( github.com/spf13/pflag v1.0.5 github.com/stretchr/testify v1.6.1 github.com/xanzy/go-gitlab v0.38.2 + gotest.tools v2.2.0+incompatible k8s.io/api v0.17.4 k8s.io/apimachinery v0.17.4 k8s.io/client-go v12.0.0+incompatible diff --git a/go.sum b/go.sum index 33714703..41711da1 100644 --- a/go.sum +++ b/go.sum @@ -1276,6 +1276,7 @@ gopkg.in/yaml.v2 v2.3.0/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v3 v3.0.0-20190905181640-827449938966/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c h1:dUUwHk2QECo/6vqA44rthZ8ie2QXMNeKRTHCNY2nXvo= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= +gotest.tools v2.2.0+incompatible h1:VsBPFP1AI068pPrMxtb/S8Zkgf9xEmTLJjfM+P5UIEo= gotest.tools v2.2.0+incompatible/go.mod h1:DsYFclhRJ6vuDpmuTbkuFWG+y2sxOXAzmJt81HFBacw= helm.sh/helm/v3 v3.1.0/go.mod h1:WYsFJuMASa/4XUqLyv54s0U/f3mlAaRErGmyy4z921g= helm.sh/helm/v3 v3.1.2 h1:VpNzaNv2DX4aRnOCcV7v5Of+XT2SZrJ8iOQ25AGKOos= diff --git a/pkg/controller/gitrepo/gitrepo_reconcile.go b/pkg/controller/gitrepo/gitrepo_reconcile.go index 82282c43..ba931cb9 100644 --- a/pkg/controller/gitrepo/gitrepo_reconcile.go +++ b/pkg/controller/gitrepo/gitrepo_reconcile.go @@ -6,7 +6,6 @@ import ( "github.com/projectsyn/lieutenant-operator/pkg/pipeline" synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" - "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/errors" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) diff --git a/pkg/helpers/values.go b/pkg/helpers/values.go index a683fb01..9f665efd 100644 --- a/pkg/helpers/values.go +++ b/pkg/helpers/values.go @@ -3,10 +3,7 @@ package helpers import ( "bytes" "fmt" - "os" "text/template" - - synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" ) const ( @@ -14,17 +11,6 @@ const ( DeleteProtectionAnnotation = "syn.tools/protected-delete" ) -// GetDeletionPolicy gets the configured default deletion policy -func GetDeletionPolicy() synv1alpha1.DeletionPolicy { - policy := synv1alpha1.DeletionPolicy(os.Getenv("DEFAULT_DELETION_POLICY")) - switch policy { - case synv1alpha1.ArchivePolicy, synv1alpha1.DeletePolicy, synv1alpha1.RetainPolicy: - return policy - default: - return synv1alpha1.ArchivePolicy - } -} - // RenderTemplate renders a given template with the given data func RenderTemplate(tmpl string, data interface{}) (string, error) { tmp, err := template.New("template").Parse(tmpl) diff --git a/pkg/helpers/values_test.go b/pkg/helpers/values_test.go index 8a8046c3..77735122 100644 --- a/pkg/helpers/values_test.go +++ b/pkg/helpers/values_test.go @@ -1,24 +1,11 @@ package helpers import ( - "os" "testing" - synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" "github.com/stretchr/testify/assert" ) -func TestGetDeletionPolicyDefault(t *testing.T) { - policy := GetDeletionPolicy() - assert.Equal(t, synv1alpha1.ArchivePolicy, policy) -} - -func TestGetDeletionPolicyNonDefault(t *testing.T) { - os.Setenv("DEFAULT_DELETION_POLICY", "Retain") - policy := GetDeletionPolicy() - assert.Equal(t, synv1alpha1.RetainPolicy, policy) -} - func TestRenderTemplateRawString(t *testing.T) { str, err := RenderTemplate("raw string", nil) assert.NoError(t, err) diff --git a/pkg/pipeline/cluster.go b/pkg/pipeline/cluster.go index 99e993b4..11978c52 100644 --- a/pkg/pipeline/cluster.go +++ b/pkg/pipeline/cluster.go @@ -10,6 +10,7 @@ import ( "time" synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" + "github.com/projectsyn/lieutenant-operator/pkg/pipeline/cluster" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -64,6 +65,12 @@ func clusterSpecificSteps(obj PipelineObject, data *ExecutionContext) ExecutionR return result } + result = applyTenantTemplate(obj, data) + if resultNotOK(result) { + result.Err = wrapError("apply tenant template", result.Err) + return result + } + return ExecutionResult{} } @@ -180,3 +187,23 @@ func setTenantOwner(obj PipelineObject, data *ExecutionContext) ExecutionResult return ExecutionResult{} } + +func applyTenantTemplate(obj PipelineObject, data *ExecutionContext) ExecutionResult { + nsName := types.NamespacedName{Name: obj.GetTenantRef().Name, Namespace: obj.GetObjectMeta().GetNamespace()} + + tenant := &synv1alpha1.Tenant{} + + if err := data.Client.Get(context.TODO(), nsName, tenant); err != nil { + return ExecutionResult{Err: fmt.Errorf("Couldn't find tenant: %w", err)} + } + + instance, ok := obj.(*synv1alpha1.Cluster) + if !ok { + return ExecutionResult{Err: fmt.Errorf("object is not a cluster")} + } + + if err := cluster.ApplyClusterTemplate(instance, tenant); err != nil { + return ExecutionResult{Err: err} + } + return ExecutionResult{} +} diff --git a/pkg/controller/cluster/cluster_template.go b/pkg/pipeline/cluster/cluster_template.go similarity index 94% rename from pkg/controller/cluster/cluster_template.go rename to pkg/pipeline/cluster/cluster_template.go index 545d1375..c1f8498d 100644 --- a/pkg/controller/cluster/cluster_template.go +++ b/pkg/pipeline/cluster/cluster_template.go @@ -26,7 +26,7 @@ func (r *templateParser) ParseString(in string) interface{} { return str } -func applyClusterTemplate(cluster *synv1alpha1.Cluster, tenant *synv1alpha1.Tenant) error { +func ApplyClusterTemplate(cluster *synv1alpha1.Cluster, tenant *synv1alpha1.Tenant) error { if tenant.Spec.ClusterTemplate == nil { return nil } diff --git a/pkg/controller/cluster/cluster_template_test.go b/pkg/pipeline/cluster/cluster_template_test.go similarity index 93% rename from pkg/controller/cluster/cluster_template_test.go rename to pkg/pipeline/cluster/cluster_template_test.go index d598e75e..9ac1edc0 100644 --- a/pkg/controller/cluster/cluster_template_test.go +++ b/pkg/pipeline/cluster/cluster_template_test.go @@ -29,7 +29,7 @@ func TestApplyClusterTemplateRaw(t *testing.T) { }, } - err := applyClusterTemplate(cluster, tenant) + err := ApplyClusterTemplate(cluster, tenant) assert.NoError(t, err) assert.Equal(t, "test", cluster.Spec.DisplayName) assert.Equal(t, "repo", cluster.Spec.GitRepoTemplate.RepoName) @@ -67,7 +67,7 @@ func TestApplyClusterTemplate(t *testing.T) { }, } - err := applyClusterTemplate(cluster, tenant) + err := ApplyClusterTemplate(cluster, tenant) assert.NoError(t, err) assert.Equal(t, "test", cluster.Spec.DisplayName) assert.Equal(t, "c-some-test", cluster.Spec.GitRepoTemplate.RepoName) @@ -90,13 +90,13 @@ func TestApplyClusterTemplateFail(t *testing.T) { } cluster := &synv1alpha1.Cluster{} - err := applyClusterTemplate(cluster, tenant) + err := ApplyClusterTemplate(cluster, tenant) assert.Error(t, err) assert.Contains(t, err.Error(), "parse") // Non existent data in template tenant.Spec.ClusterTemplate.GitRepoTemplate.RepoName = "{{ .nonexistent }}" - err = applyClusterTemplate(cluster, tenant) + err = ApplyClusterTemplate(cluster, tenant) assert.Error(t, err) assert.Contains(t, err.Error(), "render") } diff --git a/pkg/pipeline/common_test.go b/pkg/pipeline/common_test.go new file mode 100644 index 00000000..86c1f9a1 --- /dev/null +++ b/pkg/pipeline/common_test.go @@ -0,0 +1,20 @@ +package pipeline + +import ( + "os" + "testing" + + synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" + "gotest.tools/assert" +) + +func TestGetDeletionPolicyDefault(t *testing.T) { + policy := getDefaultDeletionPolicy() + assert.Equal(t, synv1alpha1.ArchivePolicy, policy) +} + +func TestGetDeletionPolicyNonDefault(t *testing.T) { + os.Setenv("DEFAULT_DELETION_POLICY", "Retain") + policy := getDefaultDeletionPolicy() + assert.Equal(t, synv1alpha1.RetainPolicy, policy) +} diff --git a/pkg/pipeline/git.go b/pkg/pipeline/git.go index f69bc419..fb57cb01 100644 --- a/pkg/pipeline/git.go +++ b/pkg/pipeline/git.go @@ -24,6 +24,11 @@ func gitRepoSpecificSteps(obj PipelineObject, data *ExecutionContext) ExecutionR return ExecutionResult{Err: err} } + if instance.Spec.RepoType == synv1alpha1.UnmanagedRepoType { + data.Log.Info("Skipping GitRepo because it is unmanaged") + return ExecutionResult{} + } + repo, hostKeys, err := manager.GetGitClient(&instance.Spec.GitRepoTemplate, instance.GetNamespace(), data.Log, data.Client) if err != nil { return ExecutionResult{Err: err} @@ -76,6 +81,10 @@ func createGitRepo(obj PipelineObject, data *ExecutionContext) ExecutionResult { template := obj.GetGitTemplate() + if template == nil { + return ExecutionResult{} + } + if template.DisplayName == "" { template.DisplayName = obj.GetDisplayName() } diff --git a/pkg/pipeline/tenant.go b/pkg/pipeline/tenant.go index 1b8a8197..fc3f3e1d 100644 --- a/pkg/pipeline/tenant.go +++ b/pkg/pipeline/tenant.go @@ -3,6 +3,7 @@ package pipeline import ( "context" "fmt" + "os" "github.com/projectsyn/lieutenant-operator/pkg/apis" synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" @@ -13,7 +14,8 @@ import ( const ( // CommonClassName is the name of the tenant's common class - CommonClassName = "common" + CommonClassName = "common" + DefaultGlobalGitRepoURL = "DEFAULT_GLOBAL_GIT_REPO_URL" ) func tenantSpecificSteps(obj PipelineObject, data *ExecutionContext) ExecutionResult { @@ -30,6 +32,12 @@ func tenantSpecificSteps(obj PipelineObject, data *ExecutionContext) ExecutionRe return result } + result = setGlobalGitRepoURL(obj, data) + if resultNotOK(result) { + result.Err = wrapError("set global gir repo URL", result.Err) + return result + } + return ExecutionResult{} } @@ -88,3 +96,17 @@ func updateTenantGitRepo(obj PipelineObject, data *ExecutionContext) ExecutionRe return ExecutionResult{} } + +func setGlobalGitRepoURL(obj PipelineObject, data *ExecutionContext) ExecutionResult { + + instance, ok := obj.(*synv1alpha1.Tenant) + if !ok { + return ExecutionResult{Err: fmt.Errorf("object is not a tenant")} + } + + defaultGlobalGitRepoURL := os.Getenv(DefaultGlobalGitRepoURL) + if len(instance.Spec.GlobalGitRepoURL) == 0 && len(defaultGlobalGitRepoURL) > 0 { + instance.Spec.GlobalGitRepoURL = defaultGlobalGitRepoURL + } + return ExecutionResult{} +} From dd58cbbf270e7b145a975dffdab34bdc0ccf808d Mon Sep 17 00:00:00 2001 From: Simon Beck Date: Wed, 21 Oct 2020 09:39:05 +0200 Subject: [PATCH 05/23] Update gitignore --- .gitignore | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.gitignore b/.gitignore index 32dff3ce..3ec04c0e 100644 --- a/.gitignore +++ b/.gitignore @@ -6,3 +6,5 @@ lieutenant-operator cmd/manager/__debug_bin tempgopath + +.cache/ From ac9f0deb12f15ba981a447ca953a30c1a45ccac4 Mon Sep 17 00:00:00 2001 From: Simon Beck Date: Wed, 21 Oct 2020 12:41:39 +0200 Subject: [PATCH 06/23] Fix tenant repo cleanup --- pkg/pipeline/git.go | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/pkg/pipeline/git.go b/pkg/pipeline/git.go index fb57cb01..32306a63 100644 --- a/pkg/pipeline/git.go +++ b/pkg/pipeline/git.go @@ -131,18 +131,12 @@ func createGitRepo(obj PipelineObject, data *ExecutionContext) ExecutionResult { err := data.Client.Create(context.TODO(), repo) if err != nil { - if errors.IsAlreadyExists(err) { + if !errors.IsAlreadyExists(err) { return ExecutionResult{} } } - for file, content := range template.TemplateFiles { - if content == manager.DeletionMagicString { - delete(template.TemplateFiles, file) - } - } - - return ExecutionResult{Err: err} + return ExecutionResult{} } func setGitRepoURLAndHostKeys(obj PipelineObject, data *ExecutionContext) ExecutionResult { From 69c22f4a40987e3235750636c5a71010beafadf8 Mon Sep 17 00:00:00 2001 From: Simon Beck Date: Wed, 21 Oct 2020 13:44:11 +0200 Subject: [PATCH 07/23] Also move unit tests --- pkg/helpers/crd.go | 19 --- pkg/helpers/crd_test.go | 264 ------------------------------------ pkg/helpers/values.go | 16 +++ pkg/pipeline/common_test.go | 173 ++++++++++++++++++++++- pkg/pipeline/git.go | 7 - pkg/pipeline/git_test.go | 103 ++++++++++++++ 6 files changed, 291 insertions(+), 291 deletions(-) delete mode 100644 pkg/helpers/crd.go delete mode 100644 pkg/helpers/crd_test.go create mode 100644 pkg/pipeline/git_test.go diff --git a/pkg/helpers/crd.go b/pkg/helpers/crd.go deleted file mode 100644 index 47542a21..00000000 --- a/pkg/helpers/crd.go +++ /dev/null @@ -1,19 +0,0 @@ -package helpers - -import ( - corev1 "k8s.io/api/core/v1" -) - -type SecretSortList corev1.SecretList - -func (s SecretSortList) Len() int { return len(s.Items) } -func (s SecretSortList) Swap(i, j int) { s.Items[i], s.Items[j] = s.Items[j], s.Items[i] } - -func (s SecretSortList) Less(i, j int) bool { - - if s.Items[i].CreationTimestamp.Equal(&s.Items[j].CreationTimestamp) { - return s.Items[i].Name < s.Items[j].Name - } - - return s.Items[i].CreationTimestamp.Before(&s.Items[j].CreationTimestamp) -} diff --git a/pkg/helpers/crd_test.go b/pkg/helpers/crd_test.go deleted file mode 100644 index 48d4dcc3..00000000 --- a/pkg/helpers/crd_test.go +++ /dev/null @@ -1,264 +0,0 @@ -package helpers - -import ( - "context" - "os" - "testing" - "time" - - "github.com/projectsyn/lieutenant-operator/pkg/apis" - synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" - "github.com/stretchr/testify/assert" - v1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/kubernetes/scheme" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/client/fake" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" -) - -func TestAddTenantLabel(t *testing.T) { - type args struct { - meta *metav1.ObjectMeta - tenant string - } - tests := []struct { - name string - args args - }{ - { - name: "add labels", - args: args{ - meta: &metav1.ObjectMeta{}, - tenant: "test", - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - AddTenantLabel(tt.args.meta, tt.args.tenant) - - if tt.args.meta.Labels[apis.LabelNameTenant] != tt.args.tenant { - t.Error("labels do not match") - } - - }) - } -} - -func TestCreateOrUpdateGitRepo(t *testing.T) { - type args struct { - obj metav1.Object - template *synv1alpha1.GitRepoTemplate - tenantRef v1.LocalObjectReference - } - tests := []struct { - name string - args args - wantErr bool - }{ - { - name: "create and update git repo", - args: args{ - obj: &synv1alpha1.GitRepo{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - Namespace: "test", - }, - }, - template: &synv1alpha1.GitRepoTemplate{ - APISecretRef: v1.SecretReference{Name: "testSecret"}, - DeployKeys: nil, - Path: "testPath", - RepoName: "testRepo", - }, - tenantRef: v1.LocalObjectReference{ - Name: "testTenant", - }, - }, - }, - } - for _, tt := range tests { - - objs := []runtime.Object{ - &synv1alpha1.GitRepo{}, - } - - cl := testSetupClient(objs) - - t.Run(tt.name, func(t *testing.T) { - if res, err := CreateOrUpdateGitRepo(tt.args.obj, scheme.Scheme, tt.args.template, cl, tt.args.tenantRef); (err != nil) != tt.wantErr { - t.Errorf("CreateOrUpdateGitRepo() error = %v, wantErr %v", err, tt.wantErr) - } else { - assert.Equal(t, controllerutil.OperationResultCreated, res) - } - }) - - namespacedName := types.NamespacedName{ - Name: tt.args.obj.GetName(), - Namespace: tt.args.obj.GetNamespace(), - } - - checkRepo := &synv1alpha1.GitRepo{} - assert.NoError(t, cl.Get(context.TODO(), namespacedName, checkRepo)) - assert.Equal(t, tt.args.template, &checkRepo.Spec.GitRepoTemplate) - tt.args.template.RepoName = "changedName" - res, err := CreateOrUpdateGitRepo(tt.args.obj, scheme.Scheme, tt.args.template, cl, tt.args.tenantRef) - assert.NoError(t, err) - assert.Equal(t, controllerutil.OperationResultUpdated, res) - 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)) - res, err = CreateOrUpdateGitRepo(tt.args.obj, scheme.Scheme, tt.args.template, cl, tt.args.tenantRef) - assert.NoError(t, err) - assert.Equal(t, controllerutil.OperationResultNone, res) - finalRepo := &synv1alpha1.GitRepo{} - assert.NoError(t, cl.Get(context.TODO(), namespacedName, finalRepo)) - assert.Equal(t, checkRepo.Spec.GitRepoTemplate, finalRepo.Spec.GitRepoTemplate) - } -} - -// testSetupClient returns a client containing all objects in objs -func testSetupClient(objs []runtime.Object) client.Client { - s := scheme.Scheme - s.AddKnownTypes(synv1alpha1.SchemeGroupVersion, objs...) - return fake.NewFakeClient(objs...) -} - -func TestHandleDeletion(t *testing.T) { - type args struct { - instance metav1.Object - finalizerName string - } - tests := []struct { - name string - args args - want DeletionState - }{ - { - name: "Normal deletion", - want: DeletionState{Deleted: true, FinalizerRemoved: true}, - args: args{ - instance: &synv1alpha1.Cluster{ - ObjectMeta: metav1.ObjectMeta{ - DeletionTimestamp: &metav1.Time{Time: time.Now()}, - Finalizers: []string{ - "test", - }, - }, - }, - finalizerName: "test", - }, - }, - { - name: "Deletion protection set", - want: DeletionState{Deleted: true, FinalizerRemoved: false}, - args: args{ - instance: &synv1alpha1.Cluster{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - DeleteProtectionAnnotation: "true", - }, - DeletionTimestamp: &metav1.Time{Time: time.Now()}, - Finalizers: []string{ - "test", - }, - }, - }, - finalizerName: "test", - }, - }, - { - name: "Nonsense annotation value", - want: DeletionState{Deleted: true, FinalizerRemoved: false}, - args: args{ - instance: &synv1alpha1.Cluster{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - DeleteProtectionAnnotation: "trugadse", - }, - DeletionTimestamp: &metav1.Time{Time: time.Now()}, - Finalizers: []string{ - "test", - }, - }, - }, - finalizerName: "test", - }, - }, - { - name: "Object not deleted", - want: DeletionState{Deleted: false, FinalizerRemoved: false}, - args: args{ - instance: &synv1alpha1.Cluster{}, - finalizerName: "test", - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - - client := testSetupClient([]runtime.Object{&synv1alpha1.Cluster{}}) - - got := HandleDeletion(tt.args.instance, tt.args.finalizerName, client) - if got != tt.want { - t.Errorf("HandleDeletion() = %v, want %v", got, tt.want) - } - - }) - } -} - -func TestAddDeletionProtection(t *testing.T) { - type args struct { - instance metav1.Object - enable string - result string - } - tests := []struct { - name string - args args - }{ - { - name: "Add deletion protection", - args: args{ - instance: &synv1alpha1.Cluster{}, - enable: "true", - result: "true", - }, - }, - { - name: "Don't add deletion protection", - args: args{ - instance: &synv1alpha1.Cluster{}, - enable: "false", - result: "", - }, - }, - { - name: "Invalid setting", - args: args{ - instance: &synv1alpha1.Cluster{}, - enable: "gaga", - result: "true", - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - - os.Setenv(protectionSettingEnvVar, tt.args.enable) - - AddDeletionProtection(tt.args.instance) - - result := tt.args.instance.GetAnnotations()[DeleteProtectionAnnotation] - if result != tt.args.result { - t.Errorf("AddDeletionProtection() value = %v, wantValue %v", result, tt.args.result) - } - }) - } -} diff --git a/pkg/helpers/values.go b/pkg/helpers/values.go index 9f665efd..b8425773 100644 --- a/pkg/helpers/values.go +++ b/pkg/helpers/values.go @@ -4,6 +4,8 @@ import ( "bytes" "fmt" "text/template" + + corev1 "k8s.io/api/core/v1" ) const ( @@ -11,6 +13,20 @@ const ( DeleteProtectionAnnotation = "syn.tools/protected-delete" ) +type SecretSortList corev1.SecretList + +func (s SecretSortList) Len() int { return len(s.Items) } +func (s SecretSortList) Swap(i, j int) { s.Items[i], s.Items[j] = s.Items[j], s.Items[i] } + +func (s SecretSortList) Less(i, j int) bool { + + if s.Items[i].CreationTimestamp.Equal(&s.Items[j].CreationTimestamp) { + return s.Items[i].Name < s.Items[j].Name + } + + return s.Items[i].CreationTimestamp.Before(&s.Items[j].CreationTimestamp) +} + // RenderTemplate renders a given template with the given data func RenderTemplate(tmpl string, data interface{}) (string, error) { tmp, err := template.New("template").Parse(tmpl) diff --git a/pkg/pipeline/common_test.go b/pkg/pipeline/common_test.go index 86c1f9a1..ac86e880 100644 --- a/pkg/pipeline/common_test.go +++ b/pkg/pipeline/common_test.go @@ -3,9 +3,14 @@ package pipeline import ( "os" "testing" + "time" + "github.com/projectsyn/lieutenant-operator/pkg/apis" synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" - "gotest.tools/assert" + "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" ) func TestGetDeletionPolicyDefault(t *testing.T) { @@ -18,3 +23,169 @@ func TestGetDeletionPolicyNonDefault(t *testing.T) { policy := getDefaultDeletionPolicy() assert.Equal(t, synv1alpha1.RetainPolicy, policy) } + +func TestAddTenantLabel(t *testing.T) { + type args struct { + obj *synv1alpha1.Cluster + } + tests := []struct { + name string + args args + }{ + { + name: "add labels", + args: args{ + obj: &synv1alpha1.Cluster{ + Spec: synv1alpha1.ClusterSpec{ + TenantRef: corev1.LocalObjectReference{Name: "test"}, + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + addTenantLabel(tt.args.obj, &ExecutionContext{}) + + if tt.args.obj.GetLabels()[apis.LabelNameTenant] != tt.args.obj.Spec.TenantRef.Name { + t.Error("labels do not match") + } + + }) + } +} + +func TestHandleDeletion(t *testing.T) { + type args struct { + instance *synv1alpha1.Cluster + finalizerName string + } + tests := []struct { + name string + args args + wantErr bool + }{ + { + name: "Normal deletion", + args: args{ + instance: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + DeletionTimestamp: &metav1.Time{Time: time.Now()}, + Finalizers: []string{ + "test", + }, + }, + }, + finalizerName: "test", + }, + }, + { + name: "Deletion protection set", + wantErr: true, + args: args{ + instance: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + DeleteProtectionAnnotation: "true", + }, + DeletionTimestamp: &metav1.Time{Time: time.Now()}, + Finalizers: []string{ + "test", + }, + }, + }, + finalizerName: "test", + }, + }, + { + name: "Nonsense annotation value", + wantErr: true, + args: args{ + instance: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + DeleteProtectionAnnotation: "trugadse", + }, + DeletionTimestamp: &metav1.Time{Time: time.Now()}, + Finalizers: []string{ + "test", + }, + }, + }, + finalizerName: "test", + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + + client := testSetupClient([]runtime.Object{&synv1alpha1.Cluster{}}) + + data := &ExecutionContext{ + Client: client, + Deleted: true, + FinalizerName: tt.args.finalizerName, + } + + got := handleDeletion(tt.args.instance, data) + if got.Err != nil != tt.wantErr { + t.Errorf("HandleDeletion() = had error: %v", got.Err) + } + + want := []string{tt.args.finalizerName} + + assert.Equal(t, want, tt.args.instance.GetFinalizers()) + + }) + } +} + +func TestAddDeletionProtection(t *testing.T) { + type args struct { + instance *synv1alpha1.Cluster + enable string + result string + } + tests := []struct { + name string + args args + }{ + { + name: "Add deletion protection", + args: args{ + instance: &synv1alpha1.Cluster{}, + enable: "true", + result: "true", + }, + }, + { + name: "Don't add deletion protection", + args: args{ + instance: &synv1alpha1.Cluster{}, + enable: "false", + result: "", + }, + }, + { + name: "Invalid setting", + args: args{ + instance: &synv1alpha1.Cluster{}, + enable: "gaga", + result: "true", + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + + os.Setenv(protectionSettingEnvVar, tt.args.enable) + + addDeletionProtection(tt.args.instance, &ExecutionContext{}) + + result := tt.args.instance.GetAnnotations()[DeleteProtectionAnnotation] + if result != tt.args.result { + t.Errorf("AddDeletionProtection() value = %v, wantValue %v", result, tt.args.result) + } + }) + } +} diff --git a/pkg/pipeline/git.go b/pkg/pipeline/git.go index 32306a63..4ee81e92 100644 --- a/pkg/pipeline/git.go +++ b/pkg/pipeline/git.go @@ -89,13 +89,6 @@ func createGitRepo(obj PipelineObject, data *ExecutionContext) ExecutionResult { template.DisplayName = obj.GetDisplayName() } - if template == nil { - return ExecutionResult{ - Abort: true, - Err: fmt.Errorf("gitRepo template is empty"), - } - } - if obj.GetTenantRef().Name == "" { return ExecutionResult{ Abort: true, diff --git a/pkg/pipeline/git_test.go b/pkg/pipeline/git_test.go new file mode 100644 index 00000000..3ffa2284 --- /dev/null +++ b/pkg/pipeline/git_test.go @@ -0,0 +1,103 @@ +package pipeline + +import ( + "context" + "testing" + + synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" + "github.com/stretchr/testify/assert" + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/kubectl/pkg/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +// testSetupClient returns a client containing all objects in objs +func testSetupClient(objs []runtime.Object) client.Client { + s := scheme.Scheme + s.AddKnownTypes(synv1alpha1.SchemeGroupVersion, objs...) + return fake.NewFakeClientWithScheme(s, objs...) +} + +func TestCreateOrUpdateGitRepo(t *testing.T) { + type args struct { + obj *synv1alpha1.Cluster + template *synv1alpha1.GitRepoTemplate + tenantRef v1.LocalObjectReference + } + tests := []struct { + name string + args args + wantErr bool + }{ + { + name: "create git repo", + args: args{ + obj: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "test", + }, + }, + template: &synv1alpha1.GitRepoTemplate{ + APISecretRef: v1.SecretReference{Name: "testSecret"}, + DeployKeys: nil, + Path: "testPath", + RepoName: "testRepo", + }, + tenantRef: v1.LocalObjectReference{ + Name: "testTenant", + }, + }, + }, + { + name: "empty template", + args: args{ + obj: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "test", + }, + }, + template: nil, + tenantRef: v1.LocalObjectReference{ + Name: "testTenant", + }, + }, + }, + } + for _, tt := range tests { + + objs := []runtime.Object{ + &synv1alpha1.GitRepo{}, + } + + cl := testSetupClient(objs) + + tt.args.obj.Spec.GitRepoTemplate = tt.args.template + tt.args.obj.Spec.TenantRef = tt.args.tenantRef + + t.Run(tt.name, func(t *testing.T) { + if res := createGitRepo(tt.args.obj, &ExecutionContext{Client: cl}); (res.Err != nil) != tt.wantErr { + t.Errorf("CreateGitRepo() error = %v, wantErr %v", res.Err, tt.wantErr) + } + }) + + if tt.args.template == nil { + continue + } + + namespacedName := types.NamespacedName{ + Name: tt.args.obj.GetName(), + Namespace: tt.args.obj.GetNamespace(), + } + + checkRepo := &synv1alpha1.GitRepo{} + assert.NoError(t, cl.Get(context.TODO(), namespacedName, checkRepo)) + assert.Equal(t, tt.args.template, &checkRepo.Spec.GitRepoTemplate) + + } +} From 508cf82419efc95ab040314a2a17beb43e303304 Mon Sep 17 00:00:00 2001 From: Simon Beck Date: Wed, 21 Oct 2020 16:04:03 +0200 Subject: [PATCH 08/23] Add lots of unit tests --- go.mod | 2 +- .../cluster/cluster_reconcile_test.go | 2 +- .../gitrepo/gitrepo_reconcile_test.go | 2 +- pkg/pipeline/cluster.go | 148 ---------- pkg/pipeline/cluster_steps.go | 152 ++++++++++ pkg/pipeline/cluster_steps_test.go | 266 ++++++++++++++++++ pkg/pipeline/common.go | 165 ----------- pkg/pipeline/common_steps.go | 166 +++++++++++ .../{common_test.go => common_steps_test.go} | 170 ++++++++++- pkg/pipeline/git_test.go | 97 ++++++- pkg/pipeline/pipelines.go | 1 - pkg/pipeline/tenant.go | 81 ------ pkg/pipeline/tenant_steps.go | 82 ++++++ pkg/pipeline/vault.go | 3 + pkg/pipeline/vault_test.go | 149 ++++++++++ pkg/vault/client.go | 6 + 16 files changed, 1082 insertions(+), 410 deletions(-) create mode 100644 pkg/pipeline/cluster_steps.go create mode 100644 pkg/pipeline/cluster_steps_test.go create mode 100644 pkg/pipeline/common_steps.go rename pkg/pipeline/{common_test.go => common_steps_test.go} (52%) create mode 100644 pkg/pipeline/tenant_steps.go create mode 100644 pkg/pipeline/vault_test.go diff --git a/go.mod b/go.mod index 16ab1877..1e8667a5 100644 --- a/go.mod +++ b/go.mod @@ -13,11 +13,11 @@ require ( github.com/spf13/pflag v1.0.5 github.com/stretchr/testify v1.6.1 github.com/xanzy/go-gitlab v0.38.2 - gotest.tools v2.2.0+incompatible k8s.io/api v0.17.4 k8s.io/apimachinery v0.17.4 k8s.io/client-go v12.0.0+incompatible k8s.io/kube-openapi v0.0.0-20191107075043-30be4d16710a + k8s.io/kubectl v0.17.4 k8s.io/utils v0.0.0-20191114200735-6ca3b61696b6 sigs.k8s.io/controller-runtime v0.5.2 ) diff --git a/pkg/controller/cluster/cluster_reconcile_test.go b/pkg/controller/cluster/cluster_reconcile_test.go index c77a3de9..ca0eb721 100644 --- a/pkg/controller/cluster/cluster_reconcile_test.go +++ b/pkg/controller/cluster/cluster_reconcile_test.go @@ -30,7 +30,7 @@ import ( func testSetupClient(objs []runtime.Object) (client.Client, *runtime.Scheme) { s := scheme.Scheme s.AddKnownTypes(synv1alpha1.SchemeGroupVersion, objs...) - return fake.NewFakeClient(objs...), s + return fake.NewFakeClientWithScheme(s, objs...), s } func TestReconcileCluster_NoCluster(t *testing.T) { diff --git a/pkg/controller/gitrepo/gitrepo_reconcile_test.go b/pkg/controller/gitrepo/gitrepo_reconcile_test.go index 293dedfd..0581f433 100644 --- a/pkg/controller/gitrepo/gitrepo_reconcile_test.go +++ b/pkg/controller/gitrepo/gitrepo_reconcile_test.go @@ -29,7 +29,7 @@ var savedGitRepoOpt manager.RepoOptions func testSetupClient(objs []runtime.Object) (client.Client, *runtime.Scheme) { s := scheme.Scheme s.AddKnownTypes(synv1alpha1.SchemeGroupVersion, objs...) - return fake.NewFakeClient(objs...), s + return fake.NewFakeClientWithScheme(s, objs...), s } func TestReconcileGitRepo_Reconcile(t *testing.T) { diff --git a/pkg/pipeline/cluster.go b/pkg/pipeline/cluster.go index 11978c52..592be44d 100644 --- a/pkg/pipeline/cluster.go +++ b/pkg/pipeline/cluster.go @@ -1,22 +1,8 @@ package pipeline import ( - "context" - "crypto/rand" - "encoding/base64" - "fmt" "os" "strings" - "time" - - synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" - "github.com/projectsyn/lieutenant-operator/pkg/pipeline/cluster" - corev1 "k8s.io/api/core/v1" - rbacv1 "k8s.io/api/rbac/v1" - "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" ) const ( @@ -73,137 +59,3 @@ func clusterSpecificSteps(obj PipelineObject, data *ExecutionContext) ExecutionR return ExecutionResult{} } - -func createClusterRBAC(obj PipelineObject, data *ExecutionContext) ExecutionResult { - objMeta := metav1.ObjectMeta{ - Name: obj.GetObjectMeta().GetName(), - Namespace: obj.GetObjectMeta().GetNamespace(), - OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(obj.GetObjectMeta(), synv1alpha1.SchemeBuilder.GroupVersion.WithKind("Cluster"))}, - } - serviceAccount := &corev1.ServiceAccount{ObjectMeta: objMeta} - role := &rbacv1.Role{ - ObjectMeta: objMeta, - Rules: []rbacv1.PolicyRule{{ - APIGroups: []string{synv1alpha1.SchemeGroupVersion.Group}, - Resources: []string{"clusters"}, - Verbs: []string{"get", "update"}, - ResourceNames: []string{obj.GetObjectMeta().GetName()}, - }}, - } - roleBinding := &rbacv1.RoleBinding{ - ObjectMeta: objMeta, - RoleRef: rbacv1.RoleRef{ - Kind: "Role", - Name: role.Name, - }, - Subjects: []rbacv1.Subject{{ - Kind: "ServiceAccount", - Name: serviceAccount.Name, - Namespace: serviceAccount.Namespace, - }}, - } - for _, item := range []runtime.Object{serviceAccount, role, roleBinding} { - if err := data.Client.Create(context.TODO(), item); err != nil && !errors.IsAlreadyExists(err) { - return ExecutionResult{Err: err} - } - } - return ExecutionResult{} -} - -func setBootstrapToken(obj PipelineObject, data *ExecutionContext) ExecutionResult { - - instance, ok := obj.(*synv1alpha1.Cluster) - if !ok { - return ExecutionResult{Err: fmt.Errorf("%s is not a cluster object", obj.GetObjectMeta().GetName())} - } - - if instance.Status.BootstrapToken == nil { - data.Log.Info("Adding status to Cluster object") - err := newClusterStatus(instance) - if err != nil { - return ExecutionResult{Err: err} - } - } - - if time.Now().After(instance.Status.BootstrapToken.ValidUntil.Time) { - instance.Status.BootstrapToken.TokenValid = false - } - - return ExecutionResult{} - -} - -//newClusterStatus will create a default lifetime of 30 minutes if it wasn't set in the object. -func newClusterStatus(cluster *synv1alpha1.Cluster) error { - - parseTime := "30m" - if cluster.Spec.TokenLifeTime != "" { - parseTime = cluster.Spec.TokenLifeTime - } - - duration, err := time.ParseDuration(parseTime) - if err != nil { - return err - } - - validUntil := time.Now().Add(duration) - - token, err := generateToken() - if err != nil { - return err - } - - cluster.Status.BootstrapToken = &synv1alpha1.BootstrapToken{ - Token: token, - ValidUntil: metav1.NewTime(validUntil), - TokenValid: true, - } - return nil -} - -func generateToken() (string, error) { - b := make([]byte, 16) - _, err := rand.Read(b) - if err != nil { - return "", err - } - return base64.URLEncoding.EncodeToString(b), err -} - -func setTenantOwner(obj PipelineObject, data *ExecutionContext) ExecutionResult { - - tenant := &synv1alpha1.Tenant{} - - tenantName := types.NamespacedName{Name: obj.GetTenantRef().Name, Namespace: obj.GetObjectMeta().GetNamespace()} - - err := data.Client.Get(context.TODO(), tenantName, tenant) - if err != nil { - return ExecutionResult{Err: err} - } - - obj.GetObjectMeta().SetOwnerReferences([]metav1.OwnerReference{ - *metav1.NewControllerRef(tenant.GetObjectMeta(), tenant.GroupVersionKind()), - }) - - return ExecutionResult{} -} - -func applyTenantTemplate(obj PipelineObject, data *ExecutionContext) ExecutionResult { - nsName := types.NamespacedName{Name: obj.GetTenantRef().Name, Namespace: obj.GetObjectMeta().GetNamespace()} - - tenant := &synv1alpha1.Tenant{} - - if err := data.Client.Get(context.TODO(), nsName, tenant); err != nil { - return ExecutionResult{Err: fmt.Errorf("Couldn't find tenant: %w", err)} - } - - instance, ok := obj.(*synv1alpha1.Cluster) - if !ok { - return ExecutionResult{Err: fmt.Errorf("object is not a cluster")} - } - - if err := cluster.ApplyClusterTemplate(instance, tenant); err != nil { - return ExecutionResult{Err: err} - } - return ExecutionResult{} -} diff --git a/pkg/pipeline/cluster_steps.go b/pkg/pipeline/cluster_steps.go new file mode 100644 index 00000000..8bbe4445 --- /dev/null +++ b/pkg/pipeline/cluster_steps.go @@ -0,0 +1,152 @@ +package pipeline + +import ( + "context" + "crypto/rand" + "encoding/base64" + "fmt" + "time" + + synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" + "github.com/projectsyn/lieutenant-operator/pkg/pipeline/cluster" + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" +) + +func createClusterRBAC(obj PipelineObject, data *ExecutionContext) ExecutionResult { + objMeta := metav1.ObjectMeta{ + Name: obj.GetObjectMeta().GetName(), + Namespace: obj.GetObjectMeta().GetNamespace(), + OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(obj.GetObjectMeta(), synv1alpha1.SchemeBuilder.GroupVersion.WithKind("Cluster"))}, + } + serviceAccount := &corev1.ServiceAccount{ObjectMeta: objMeta} + role := &rbacv1.Role{ + ObjectMeta: objMeta, + Rules: []rbacv1.PolicyRule{{ + APIGroups: []string{synv1alpha1.SchemeGroupVersion.Group}, + Resources: []string{"clusters"}, + Verbs: []string{"get", "update"}, + ResourceNames: []string{obj.GetObjectMeta().GetName()}, + }}, + } + roleBinding := &rbacv1.RoleBinding{ + ObjectMeta: objMeta, + RoleRef: rbacv1.RoleRef{ + Kind: "Role", + Name: role.Name, + }, + Subjects: []rbacv1.Subject{{ + Kind: "ServiceAccount", + Name: serviceAccount.Name, + Namespace: serviceAccount.Namespace, + }}, + } + for _, item := range []runtime.Object{serviceAccount, role, roleBinding} { + if err := data.Client.Create(context.TODO(), item); err != nil && !errors.IsAlreadyExists(err) { + return ExecutionResult{Err: err} + } + } + return ExecutionResult{} +} + +func setBootstrapToken(obj PipelineObject, data *ExecutionContext) ExecutionResult { + + instance, ok := obj.(*synv1alpha1.Cluster) + if !ok { + return ExecutionResult{Err: fmt.Errorf("%s is not a cluster object", obj.GetObjectMeta().GetName())} + } + + if instance.Status.BootstrapToken == nil { + data.Log.Info("Adding status to Cluster object") + err := newClusterStatus(instance) + if err != nil { + return ExecutionResult{Err: err} + } + } + + if time.Now().After(instance.Status.BootstrapToken.ValidUntil.Time) { + instance.Status.BootstrapToken.TokenValid = false + } + + return ExecutionResult{} + +} + +//newClusterStatus will create a default lifetime of 30 minutes if it wasn't set in the object. +func newClusterStatus(cluster *synv1alpha1.Cluster) error { + + parseTime := "30m" + if cluster.Spec.TokenLifeTime != "" { + parseTime = cluster.Spec.TokenLifeTime + } + + duration, err := time.ParseDuration(parseTime) + if err != nil { + return err + } + + validUntil := time.Now().Add(duration) + + token, err := generateToken() + if err != nil { + return err + } + + cluster.Status.BootstrapToken = &synv1alpha1.BootstrapToken{ + Token: token, + ValidUntil: metav1.NewTime(validUntil), + TokenValid: true, + } + return nil +} + +func generateToken() (string, error) { + b := make([]byte, 16) + _, err := rand.Read(b) + if err != nil { + return "", err + } + return base64.URLEncoding.EncodeToString(b), err +} + +func setTenantOwner(obj PipelineObject, data *ExecutionContext) ExecutionResult { + + tenant := &synv1alpha1.Tenant{} + + tenantName := types.NamespacedName{Name: obj.GetTenantRef().Name, Namespace: obj.GetObjectMeta().GetNamespace()} + + err := data.Client.Get(context.TODO(), tenantName, tenant) + if err != nil { + return ExecutionResult{Err: err} + } + + obj.GetObjectMeta().SetOwnerReferences([]metav1.OwnerReference{ + *metav1.NewControllerRef(tenant.GetObjectMeta(), tenant.GroupVersionKind()), + }) + + return ExecutionResult{} +} + +func applyTenantTemplate(obj PipelineObject, data *ExecutionContext) ExecutionResult { + nsName := types.NamespacedName{Name: obj.GetTenantRef().Name, Namespace: obj.GetObjectMeta().GetNamespace()} + + tenant := &synv1alpha1.Tenant{} + + if err := data.Client.Get(context.TODO(), nsName, tenant); err != nil { + return ExecutionResult{Err: fmt.Errorf("Couldn't find tenant: %w", err)} + } + + instance, ok := obj.(*synv1alpha1.Cluster) + if !ok { + return ExecutionResult{Err: fmt.Errorf("object is not a cluster")} + } + + if err := cluster.ApplyClusterTemplate(instance, tenant); err != nil { + return ExecutionResult{Err: err} + } + return ExecutionResult{} +} diff --git a/pkg/pipeline/cluster_steps_test.go b/pkg/pipeline/cluster_steps_test.go new file mode 100644 index 00000000..524dd3d0 --- /dev/null +++ b/pkg/pipeline/cluster_steps_test.go @@ -0,0 +1,266 @@ +package pipeline + +import ( + "context" + "testing" + + synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" + "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/log/zap" +) + +func Test_createClusterRBAC(t *testing.T) { + type args struct { + obj *synv1alpha1.Cluster + data *ExecutionContext + } + tests := []struct { + name string + args args + wantErr bool + }{ + { + name: "create cluster RBAC", + wantErr: false, + args: args{ + obj: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "testnamespace", + }, + }, + }, + }, + } + for _, tt := range tests { + + client, _ := testSetupClient([]runtime.Object{ + tt.args.obj, + }) + + tt.args.data = &ExecutionContext{Client: client} + + t.Run(tt.name, func(t *testing.T) { + if got := createClusterRBAC(tt.args.obj, tt.args.data); got.Err != nil != tt.wantErr { + t.Errorf("createClusterRBAC() = had error: %v", got.Err) + } + }) + + roleBinding := &rbacv1.RoleBinding{} + serviceAccount := &corev1.ServiceAccount{} + + namespacedName := types.NamespacedName{Name: tt.args.obj.Name, Namespace: tt.args.obj.Namespace} + + assert.NoError(t, client.Get(context.TODO(), namespacedName, roleBinding)) + assert.NoError(t, client.Get(context.TODO(), namespacedName, serviceAccount)) + + assert.Equal(t, serviceAccount.Name, roleBinding.Subjects[len(roleBinding.Subjects)-1].Name) + assert.Equal(t, serviceAccount.Namespace, roleBinding.Subjects[len(roleBinding.Subjects)-1].Namespace) + + } +} + +func Test_setBootstrapToken(t *testing.T) { + type args struct { + obj *synv1alpha1.Cluster + data *ExecutionContext + } + tests := []struct { + name string + args args + wantErr bool + }{ + { + name: "Set bootstrap token", + args: args{ + obj: &synv1alpha1.Cluster{}, + data: &ExecutionContext{ + Log: zap.New(), + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := setBootstrapToken(tt.args.obj, tt.args.data); got.Err != nil != tt.wantErr { + t.Errorf("setBootstrapToken() = had error: %v", got.Err) + } + }) + + assert.NotNil(t, tt.args.obj.Status.BootstrapToken) + + } +} + +func Test_newClusterStatus(t *testing.T) { + type args struct { + cluster *synv1alpha1.Cluster + } + tests := []struct { + name string + args args + wantErr bool + }{ + { + name: "new cluster status", + args: args{ + cluster: &synv1alpha1.Cluster{}, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := newClusterStatus(tt.args.cluster); (err != nil) != tt.wantErr { + t.Errorf("newClusterStatus() error = %v, wantErr %v", err, tt.wantErr) + } + }) + + assert.NotNil(t, tt.args.cluster.Status.BootstrapToken) + + } +} + +func Test_setTenantOwner(t *testing.T) { + type args struct { + obj *synv1alpha1.Cluster + tenant *synv1alpha1.Tenant + data *ExecutionContext + } + tests := []struct { + name string + args args + wantErr bool + }{ + { + name: "set tenant owner", + args: args{ + obj: &synv1alpha1.Cluster{ + Spec: synv1alpha1.ClusterSpec{ + TenantRef: corev1.LocalObjectReference{Name: "testTenant"}, + }, + }, + tenant: &synv1alpha1.Tenant{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testTenant", + }, + }, + data: &ExecutionContext{}, + }, + }, + { + name: "tenant does not exist", + wantErr: true, + args: args{ + obj: &synv1alpha1.Cluster{ + Spec: synv1alpha1.ClusterSpec{ + TenantRef: corev1.LocalObjectReference{Name: "wrongTenant"}, + }, + }, + tenant: &synv1alpha1.Tenant{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testTenant", + }, + }, + data: &ExecutionContext{}, + }, + }, + } + for _, tt := range tests { + + tt.args.data.Client, _ = testSetupClient([]runtime.Object{ + tt.args.obj, + tt.args.tenant, + }) + + t.Run(tt.name, func(t *testing.T) { + if got := setTenantOwner(tt.args.obj, tt.args.data); (got.Err != nil) != tt.wantErr { + t.Errorf("setTenantOwner() = had error: %v", got.Err) + } + }) + + if !tt.wantErr { + assert.NotEmpty(t, tt.args.obj.GetOwnerReferences()) + } + + } +} + +func Test_applyTenantTemplate(t *testing.T) { + type args struct { + obj *synv1alpha1.Cluster + tenant *synv1alpha1.Tenant + data *ExecutionContext + } + tests := []struct { + name string + args args + wantErr bool + }{ + { + name: "apply tenant template", + args: args{ + data: &ExecutionContext{}, + obj: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "c-some-test", + }, + }, + tenant: &synv1alpha1.Tenant{ + Spec: synv1alpha1.TenantSpec{ + ClusterTemplate: &synv1alpha1.ClusterSpec{ + DisplayName: "test", + GitRepoTemplate: &synv1alpha1.GitRepoTemplate{ + RepoName: "{{ .Name }}", + }, + }, + }, + }, + }, + }, + { + name: "wrong syntax", + wantErr: true, + args: args{ + data: &ExecutionContext{}, + obj: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "c-some-test", + }, + }, + tenant: &synv1alpha1.Tenant{ + Spec: synv1alpha1.TenantSpec{ + ClusterTemplate: &synv1alpha1.ClusterSpec{ + DisplayName: "test", + GitRepoTemplate: &synv1alpha1.GitRepoTemplate{ + RepoName: "{{ .Name }", + }, + }, + }, + }, + }, + }, + } + for _, tt := range tests { + + tt.args.data.Client, _ = testSetupClient([]runtime.Object{ + tt.args.obj, + tt.args.tenant, + }) + + t.Run(tt.name, func(t *testing.T) { + if got := applyTenantTemplate(tt.args.obj, tt.args.data); (got.Err != nil) != tt.wantErr { + t.Errorf("applyTenantTemplate() = had error: %v", got.Err) + } + }) + + if !tt.wantErr { + assert.Equal(t, "c-some-test", tt.args.obj.Spec.GitRepoTemplate.RepoName) + } + + } +} diff --git a/pkg/pipeline/common.go b/pkg/pipeline/common.go index 5161c20e..a085fba8 100644 --- a/pkg/pipeline/common.go +++ b/pkg/pipeline/common.go @@ -3,105 +3,11 @@ package pipeline -import ( - "context" - "fmt" - "os" - "strconv" - - "github.com/projectsyn/lieutenant-operator/pkg/apis" - synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" -) - const ( protectionSettingEnvVar = "LIEUTENANT_DELETE_PROTECTION" DeleteProtectionAnnotation = "syn.tools/protected-delete" ) -func getDefaultDeletionPolicy() synv1alpha1.DeletionPolicy { - policy := synv1alpha1.DeletionPolicy(os.Getenv("DEFAULT_DELETION_POLICY")) - switch policy { - case synv1alpha1.ArchivePolicy, synv1alpha1.DeletePolicy, synv1alpha1.RetainPolicy: - return policy - default: - return synv1alpha1.ArchivePolicy - } -} - -func addDeletionProtection(instance PipelineObject, data *ExecutionContext) ExecutionResult { - - if data.Deleted { - return ExecutionResult{} - } - - config := os.Getenv(protectionSettingEnvVar) - - protected, err := strconv.ParseBool(config) - if err != nil { - protected = true - } - - if protected { - annotations := instance.GetObjectMeta().GetAnnotations() - - if annotations == nil { - annotations = make(map[string]string) - } - - if _, ok := annotations[DeleteProtectionAnnotation]; !ok { - annotations[DeleteProtectionAnnotation] = "true" - } - - instance.GetObjectMeta().SetAnnotations(annotations) - } - - return ExecutionResult{} - -} - -// addTenantLabel adds the tenant label to an object. -func addTenantLabel(obj PipelineObject, data *ExecutionContext) ExecutionResult { - labels := obj.GetObjectMeta().GetLabels() - if labels == nil { - labels = make(map[string]string) - } - if labels[apis.LabelNameTenant] != obj.GetTenantRef().Name { - labels[apis.LabelNameTenant] = obj.GetTenantRef().Name - } - - obj.GetObjectMeta().SetLabels(labels) - - return ExecutionResult{} -} - -func updateObject(obj PipelineObject, data *ExecutionContext) ExecutionResult { - - resourceVersion := obj.GetObjectMeta().GetResourceVersion() - - rtObj, ok := obj.(runtime.Object) - if !ok { - return ExecutionResult{ - Abort: true, - Err: fmt.Errorf("object ist not a valid runtime.object: %v", obj.GetObjectMeta().GetName()), - } - } - - err := data.Client.Update(context.TODO(), rtObj) - if err != nil { - return ExecutionResult{Err: err} - } - - // Updating the status if either there were changes or the object is deleted will - // lead to some race conditions. By checking first we can avoid them. - if resourceVersion == obj.GetObjectMeta().GetResourceVersion() && !data.Deleted { - err = data.Client.Status().Update(context.TODO(), rtObj) - } - return ExecutionResult{Abort: true, Err: err} -} - func common(obj PipelineObject, data *ExecutionContext) ExecutionResult { result := handleDeletion(obj.GetObjectMeta(), data) @@ -136,74 +42,3 @@ func common(obj PipelineObject, data *ExecutionContext) ExecutionResult { return ExecutionResult{} } - -func wrapError(name string, err error) error { - if err == nil { - return nil - } - return fmt.Errorf("step %s failed: %w", name, err) -} - -func resultNotOK(result ExecutionResult) bool { - return result.Abort || result.Err != nil -} - -// handleDeletion will handle the finalizers if the object was deleted. -// It will only trigger if data.Deleted is true. -func handleDeletion(instance metav1.Object, data *ExecutionContext) ExecutionResult { - if !data.Deleted { - return ExecutionResult{} - } - - annotationValue, exists := instance.GetAnnotations()[DeleteProtectionAnnotation] - - var protected bool - var err error - if exists { - protected, err = strconv.ParseBool(annotationValue) - // Assume true if it can't be parsed - if err != nil { - protected = true - // We need to reset the error again, so we don't trigger any unwanted side effects... - err = nil - } - } else { - protected = false - } - - if sliceContainsString(instance.GetFinalizers(), data.FinalizerName) && !protected { - - data.Deleted = true - return ExecutionResult{} - } - - return ExecutionResult{Err: fmt.Errorf("finalzier was not removed")} -} - -// Checks if the slice of strings contains a specific string -func sliceContainsString(list []string, s string) bool { - for _, v := range list { - if v == s { - return true - } - } - return false -} - -func checkIfDeleted(obj PipelineObject, data *ExecutionContext) ExecutionResult { - if !obj.GetObjectMeta().GetDeletionTimestamp().IsZero() { - data.Deleted = true - - } - return ExecutionResult{} - -} - -func handleFinalizer(obj PipelineObject, data *ExecutionContext) ExecutionResult { - if data.FinalizerName != "" && !data.Deleted { - controllerutil.AddFinalizer(obj.GetObjectMeta(), data.FinalizerName) - } else { - controllerutil.RemoveFinalizer(obj.GetObjectMeta(), data.FinalizerName) - } - return ExecutionResult{} -} diff --git a/pkg/pipeline/common_steps.go b/pkg/pipeline/common_steps.go new file mode 100644 index 00000000..fbac42a7 --- /dev/null +++ b/pkg/pipeline/common_steps.go @@ -0,0 +1,166 @@ +package pipeline + +import ( + "context" + "fmt" + "os" + "strconv" + + "github.com/projectsyn/lieutenant-operator/pkg/apis" + synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" +) + +func getDefaultDeletionPolicy() synv1alpha1.DeletionPolicy { + policy := synv1alpha1.DeletionPolicy(os.Getenv("DEFAULT_DELETION_POLICY")) + switch policy { + case synv1alpha1.ArchivePolicy, synv1alpha1.DeletePolicy, synv1alpha1.RetainPolicy: + return policy + default: + return synv1alpha1.ArchivePolicy + } +} + +func addDeletionProtection(instance PipelineObject, data *ExecutionContext) ExecutionResult { + + if data.Deleted { + return ExecutionResult{} + } + + config := os.Getenv(protectionSettingEnvVar) + + protected, err := strconv.ParseBool(config) + if err != nil { + protected = true + } + + if protected { + annotations := instance.GetObjectMeta().GetAnnotations() + + if annotations == nil { + annotations = make(map[string]string) + } + + if _, ok := annotations[DeleteProtectionAnnotation]; !ok { + annotations[DeleteProtectionAnnotation] = "true" + } + + instance.GetObjectMeta().SetAnnotations(annotations) + } + + return ExecutionResult{} + +} + +// addTenantLabel adds the tenant label to an object. +func addTenantLabel(obj PipelineObject, data *ExecutionContext) ExecutionResult { + labels := obj.GetObjectMeta().GetLabels() + if labels == nil { + labels = make(map[string]string) + } + if labels[apis.LabelNameTenant] != obj.GetTenantRef().Name { + labels[apis.LabelNameTenant] = obj.GetTenantRef().Name + } + + obj.GetObjectMeta().SetLabels(labels) + + return ExecutionResult{} +} + +func updateObject(obj PipelineObject, data *ExecutionContext) ExecutionResult { + + resourceVersion := obj.GetObjectMeta().GetResourceVersion() + + rtObj, ok := obj.(runtime.Object) + if !ok { + return ExecutionResult{ + Abort: true, + Err: fmt.Errorf("object ist not a valid runtime.object: %v", obj.GetObjectMeta().GetName()), + } + } + + err := data.Client.Update(context.TODO(), rtObj) + if err != nil { + return ExecutionResult{Err: err} + } + + // Updating the status if either there were changes or the object is deleted will + // lead to some race conditions. By checking first we can avoid them. + if resourceVersion == obj.GetObjectMeta().GetResourceVersion() && !data.Deleted { + err = data.Client.Status().Update(context.TODO(), rtObj) + } + return ExecutionResult{Abort: true, Err: err} +} + +func wrapError(name string, err error) error { + if err == nil { + return nil + } + return fmt.Errorf("step %s failed: %w", name, err) +} + +func resultNotOK(result ExecutionResult) bool { + return result.Abort || result.Err != nil +} + +// handleDeletion will handle the finalizers if the object was deleted. +// It will only trigger if data.Deleted is true. +func handleDeletion(instance metav1.Object, data *ExecutionContext) ExecutionResult { + if !data.Deleted { + return ExecutionResult{} + } + + annotationValue, exists := instance.GetAnnotations()[DeleteProtectionAnnotation] + + var protected bool + var err error + if exists { + protected, err = strconv.ParseBool(annotationValue) + // Assume true if it can't be parsed + if err != nil { + protected = true + // We need to reset the error again, so we don't trigger any unwanted side effects... + err = nil + } + } else { + protected = false + } + + if sliceContainsString(instance.GetFinalizers(), data.FinalizerName) && !protected { + + data.Deleted = true + return ExecutionResult{} + } + + return ExecutionResult{Err: fmt.Errorf("finalzier was not removed")} +} + +// Checks if the slice of strings contains a specific string +func sliceContainsString(list []string, s string) bool { + for _, v := range list { + if v == s { + return true + } + } + return false +} + +func checkIfDeleted(obj PipelineObject, data *ExecutionContext) ExecutionResult { + if !obj.GetObjectMeta().GetDeletionTimestamp().IsZero() { + data.Deleted = true + + } + return ExecutionResult{} + +} + +func handleFinalizer(obj PipelineObject, data *ExecutionContext) ExecutionResult { + if data.FinalizerName != "" && !data.Deleted { + controllerutil.AddFinalizer(obj.GetObjectMeta(), data.FinalizerName) + } else { + controllerutil.RemoveFinalizer(obj.GetObjectMeta(), data.FinalizerName) + } + return ExecutionResult{} +} diff --git a/pkg/pipeline/common_test.go b/pkg/pipeline/common_steps_test.go similarity index 52% rename from pkg/pipeline/common_test.go rename to pkg/pipeline/common_steps_test.go index ac86e880..1b0b4059 100644 --- a/pkg/pipeline/common_test.go +++ b/pkg/pipeline/common_steps_test.go @@ -1,6 +1,7 @@ package pipeline import ( + "fmt" "os" "testing" "time" @@ -11,8 +12,18 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/kubectl/pkg/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" ) +// testSetupClient returns a client containing all objects in objs +func testSetupClient(objs []runtime.Object) (client.Client, *runtime.Scheme) { + s := scheme.Scheme + s.AddKnownTypes(synv1alpha1.SchemeGroupVersion, objs...) + return fake.NewFakeClientWithScheme(s, objs...), s +} + func TestGetDeletionPolicyDefault(t *testing.T) { policy := getDefaultDeletionPolicy() assert.Equal(t, synv1alpha1.ArchivePolicy, policy) @@ -119,7 +130,7 @@ func TestHandleDeletion(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - client := testSetupClient([]runtime.Object{&synv1alpha1.Cluster{}}) + client, _ := testSetupClient([]runtime.Object{&synv1alpha1.Cluster{}}) data := &ExecutionContext{ Client: client, @@ -189,3 +200,160 @@ func TestAddDeletionProtection(t *testing.T) { }) } } + +func Test_checkIfDeleted(t *testing.T) { + type args struct { + obj *synv1alpha1.Tenant + data *ExecutionContext + } + tests := []struct { + name string + args args + wantErr bool + want bool + }{ + { + name: "object deleted", + want: true, + args: args{ + data: &ExecutionContext{}, + obj: &synv1alpha1.Tenant{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + DeletionTimestamp: &metav1.Time{Time: time.Now()}, + }, + }, + }, + }, + { + name: "object not deleted", + want: false, + args: args{ + data: &ExecutionContext{}, + obj: &synv1alpha1.Tenant{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := checkIfDeleted(tt.args.obj, tt.args.data); (got.Err != nil) != tt.wantErr { + t.Errorf("checkIfDeleted() = had error %v", got.Err) + } + + assert.Equal(t, tt.want, tt.args.data.Deleted) + + }) + } +} + +func Test_handleFinalizer(t *testing.T) { + type args struct { + obj *synv1alpha1.Cluster + data *ExecutionContext + } + tests := []struct { + name string + args args + wantErr bool + }{ + { + name: "add finalizers", + args: args{ + data: &ExecutionContext{ + FinalizerName: "test", + }, + obj: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + }, + }, + }, + { + name: "remove finalizers", + args: args{ + data: &ExecutionContext{ + Deleted: true, + FinalizerName: "test", + }, + obj: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := handleFinalizer(tt.args.obj, tt.args.data); (got.Err != nil) != tt.wantErr { + t.Errorf("handleFinalizer() = had error: %v", got.Err) + } + + if tt.args.data.Deleted { + assert.Empty(t, tt.args.obj.GetFinalizers()) + } else { + assert.NotEmpty(t, tt.args.obj.GetFinalizers()) + } + + }) + } +} + +func Test_resultNotOK(t *testing.T) { + assert.True(t, resultNotOK(ExecutionResult{Err: fmt.Errorf("test")})) + assert.False(t, resultNotOK(ExecutionResult{})) +} + +func Test_wrapError(t *testing.T) { + assert.Equal(t, "step test failed: test", wrapError("test", fmt.Errorf("test")).Error()) + assert.Nil(t, wrapError("test", nil)) +} + +func Test_updateObject(t *testing.T) { + type args struct { + obj *synv1alpha1.Tenant + data *ExecutionContext + } + tests := []struct { + name string + args args + wantErr bool + }{ + { + name: "update objects", + args: args{ + data: &ExecutionContext{}, + obj: &synv1alpha1.Tenant{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + }, + }, + }, + { + name: "update fail", + args: args{ + data: &ExecutionContext{}, + obj: &synv1alpha1.Tenant{}, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + + tt.args.data.Client, _ = testSetupClient([]runtime.Object{ + tt.args.obj, + }) + + if got := updateObject(tt.args.obj, tt.args.data); (got.Err != nil) != tt.wantErr { + t.Errorf("updateObject() = had error: %v", got.Err) + } + + }) + } +} diff --git a/pkg/pipeline/git_test.go b/pkg/pipeline/git_test.go index 3ffa2284..4d325a4d 100644 --- a/pkg/pipeline/git_test.go +++ b/pkg/pipeline/git_test.go @@ -10,18 +10,8 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" - "k8s.io/kubectl/pkg/scheme" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/client/fake" ) -// testSetupClient returns a client containing all objects in objs -func testSetupClient(objs []runtime.Object) client.Client { - s := scheme.Scheme - s.AddKnownTypes(synv1alpha1.SchemeGroupVersion, objs...) - return fake.NewFakeClientWithScheme(s, objs...) -} - func TestCreateOrUpdateGitRepo(t *testing.T) { type args struct { obj *synv1alpha1.Cluster @@ -75,7 +65,7 @@ func TestCreateOrUpdateGitRepo(t *testing.T) { &synv1alpha1.GitRepo{}, } - cl := testSetupClient(objs) + cl, _ := testSetupClient(objs) tt.args.obj.Spec.GitRepoTemplate = tt.args.template tt.args.obj.Spec.TenantRef = tt.args.tenantRef @@ -101,3 +91,88 @@ func TestCreateOrUpdateGitRepo(t *testing.T) { } } + +func Test_fetchGitRepoTemplate(t *testing.T) { + type args struct { + obj *synv1alpha1.GitRepo + templateObj PipelineObject + data *ExecutionContext + } + tests := []struct { + name string + args args + wantErr bool + }{ + { + name: "fetch tenant changes", + args: args{ + data: &ExecutionContext{}, + obj: &synv1alpha1.GitRepo{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + }, + templateObj: &synv1alpha1.Tenant{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + Spec: synv1alpha1.TenantSpec{ + GitRepoTemplate: &synv1alpha1.GitRepoTemplate{ + RepoName: "Test Repo", + RepoType: synv1alpha1.AutoRepoType, + Path: "test", + }, + }, + }, + }, + }, + { + name: "fetch cluster changes", + args: args{ + data: &ExecutionContext{}, + obj: &synv1alpha1.GitRepo{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + }, + templateObj: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + Spec: synv1alpha1.ClusterSpec{ + GitRepoTemplate: &synv1alpha1.GitRepoTemplate{ + RepoName: "Test Repo", + RepoType: synv1alpha1.AutoRepoType, + Path: "test", + }, + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + + rtObj := tt.args.templateObj.(runtime.Object) + + tt.args.data.Client, _ = testSetupClient([]runtime.Object{ + tt.args.obj, + &synv1alpha1.Tenant{}, + &synv1alpha1.Cluster{}, + rtObj, + }) + + if err := fetchGitRepoTemplate(tt.args.obj, tt.args.data); (err != nil) != tt.wantErr { + t.Errorf("fetchGitRepoTemplate() error = %v, wantErr %v", err, tt.wantErr) + } + + assert.Equal(t, tt.args.templateObj.GetGitTemplate(), &tt.args.obj.Spec.GitRepoTemplate) + tt.args.templateObj.GetGitTemplate().RepoName = "another test" + rtObj = tt.args.templateObj.(runtime.Object) + assert.NoError(t, tt.args.data.Client.Update(context.TODO(), rtObj)) + assert.NoError(t, fetchGitRepoTemplate(tt.args.obj, tt.args.data)) + assert.Equal(t, tt.args.templateObj.GetGitTemplate(), &tt.args.obj.Spec.GitRepoTemplate) + + }) + } +} diff --git a/pkg/pipeline/pipelines.go b/pkg/pipeline/pipelines.go index 4c15c13c..424f0a2f 100644 --- a/pkg/pipeline/pipelines.go +++ b/pkg/pipeline/pipelines.go @@ -29,7 +29,6 @@ func ReconcileTenant(obj PipelineObject, data *ExecutionContext) error { func ReconcileCluster(obj PipelineObject, data *ExecutionContext) error { - //TODO: the cluster has to get the right tenant and set it as its owner result := clusterSpecificSteps(obj, data) if resultNotOK(result) { return wrapError("cluster specific steps failes", result.Err) diff --git a/pkg/pipeline/tenant.go b/pkg/pipeline/tenant.go index fc3f3e1d..96bed8b3 100644 --- a/pkg/pipeline/tenant.go +++ b/pkg/pipeline/tenant.go @@ -1,17 +1,5 @@ package pipeline -import ( - "context" - "fmt" - "os" - - "github.com/projectsyn/lieutenant-operator/pkg/apis" - synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" - "github.com/projectsyn/lieutenant-operator/pkg/git/manager" - "k8s.io/apimachinery/pkg/labels" - "sigs.k8s.io/controller-runtime/pkg/client" -) - const ( // CommonClassName is the name of the tenant's common class CommonClassName = "common" @@ -41,72 +29,3 @@ func tenantSpecificSteps(obj PipelineObject, data *ExecutionContext) ExecutionRe return ExecutionResult{} } - -func addDefaultClassFile(obj PipelineObject, data *ExecutionContext) ExecutionResult { - commonClassFile := CommonClassName + ".yml" - if obj.GetGitTemplate().TemplateFiles == nil { - obj.GetGitTemplate().TemplateFiles = map[string]string{} - } - if _, ok := obj.GetGitTemplate().TemplateFiles[commonClassFile]; !ok { - obj.GetGitTemplate().TemplateFiles[commonClassFile] = "" - } - return ExecutionResult{} -} - -func updateTenantGitRepo(obj PipelineObject, data *ExecutionContext) ExecutionResult { - - tenantCR, ok := obj.(*synv1alpha1.Tenant) - if !ok { - return ExecutionResult{Err: fmt.Errorf("object is not a tenant")} - } - - oldFiles := tenantCR.Spec.GitRepoTemplate.TemplateFiles - - tenantCR.Spec.GitRepoTemplate.TemplateFiles = map[string]string{} - - clusterList := &synv1alpha1.ClusterList{} - - selector := labels.Set(map[string]string{apis.LabelNameTenant: tenantCR.Name}).AsSelector() - - listOptions := &client.ListOptions{ - LabelSelector: selector, - Namespace: obj.GetObjectMeta().GetNamespace(), - } - - err := data.Client.List(context.TODO(), clusterList, listOptions) - if err != nil { - return ExecutionResult{Err: err} - } - - for _, cluster := range clusterList.Items { - fileName := cluster.GetName() + ".yml" - fileContent := fmt.Sprintf(clusterClassContent, cluster.GetName(), CommonClassName) - tenantCR.Spec.GitRepoTemplate.TemplateFiles[fileName] = fileContent - delete(oldFiles, fileName) - } - - for fileName := range oldFiles { - if fileName == CommonClassName+".yml" { - tenantCR.Spec.GitRepoTemplate.TemplateFiles[CommonClassName+".yml"] = "" - } else { - tenantCR.Spec.GitRepoTemplate.TemplateFiles[fileName] = manager.DeletionMagicString - - } - } - - return ExecutionResult{} -} - -func setGlobalGitRepoURL(obj PipelineObject, data *ExecutionContext) ExecutionResult { - - instance, ok := obj.(*synv1alpha1.Tenant) - if !ok { - return ExecutionResult{Err: fmt.Errorf("object is not a tenant")} - } - - defaultGlobalGitRepoURL := os.Getenv(DefaultGlobalGitRepoURL) - if len(instance.Spec.GlobalGitRepoURL) == 0 && len(defaultGlobalGitRepoURL) > 0 { - instance.Spec.GlobalGitRepoURL = defaultGlobalGitRepoURL - } - return ExecutionResult{} -} diff --git a/pkg/pipeline/tenant_steps.go b/pkg/pipeline/tenant_steps.go new file mode 100644 index 00000000..20816a4f --- /dev/null +++ b/pkg/pipeline/tenant_steps.go @@ -0,0 +1,82 @@ +package pipeline + +import ( + "context" + "fmt" + "os" + + "github.com/projectsyn/lieutenant-operator/pkg/apis" + synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" + "github.com/projectsyn/lieutenant-operator/pkg/git/manager" + "k8s.io/apimachinery/pkg/labels" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +func addDefaultClassFile(obj PipelineObject, data *ExecutionContext) ExecutionResult { + commonClassFile := CommonClassName + ".yml" + if obj.GetGitTemplate().TemplateFiles == nil { + obj.GetGitTemplate().TemplateFiles = map[string]string{} + } + if _, ok := obj.GetGitTemplate().TemplateFiles[commonClassFile]; !ok { + obj.GetGitTemplate().TemplateFiles[commonClassFile] = "" + } + return ExecutionResult{} +} + +func updateTenantGitRepo(obj PipelineObject, data *ExecutionContext) ExecutionResult { + + tenantCR, ok := obj.(*synv1alpha1.Tenant) + if !ok { + return ExecutionResult{Err: fmt.Errorf("object is not a tenant")} + } + + oldFiles := tenantCR.Spec.GitRepoTemplate.TemplateFiles + + tenantCR.Spec.GitRepoTemplate.TemplateFiles = map[string]string{} + + clusterList := &synv1alpha1.ClusterList{} + + selector := labels.Set(map[string]string{apis.LabelNameTenant: tenantCR.Name}).AsSelector() + + listOptions := &client.ListOptions{ + LabelSelector: selector, + Namespace: obj.GetObjectMeta().GetNamespace(), + } + + err := data.Client.List(context.TODO(), clusterList, listOptions) + if err != nil { + return ExecutionResult{Err: err} + } + + for _, cluster := range clusterList.Items { + fileName := cluster.GetName() + ".yml" + fileContent := fmt.Sprintf(clusterClassContent, cluster.GetName(), CommonClassName) + tenantCR.Spec.GitRepoTemplate.TemplateFiles[fileName] = fileContent + delete(oldFiles, fileName) + } + + for fileName := range oldFiles { + if fileName == CommonClassName+".yml" { + tenantCR.Spec.GitRepoTemplate.TemplateFiles[CommonClassName+".yml"] = "" + } else { + tenantCR.Spec.GitRepoTemplate.TemplateFiles[fileName] = manager.DeletionMagicString + + } + } + + return ExecutionResult{} +} + +func setGlobalGitRepoURL(obj PipelineObject, data *ExecutionContext) ExecutionResult { + + instance, ok := obj.(*synv1alpha1.Tenant) + if !ok { + return ExecutionResult{Err: fmt.Errorf("object is not a tenant")} + } + + defaultGlobalGitRepoURL := os.Getenv(DefaultGlobalGitRepoURL) + if len(instance.Spec.GlobalGitRepoURL) == 0 && len(defaultGlobalGitRepoURL) > 0 { + instance.Spec.GlobalGitRepoURL = defaultGlobalGitRepoURL + } + return ExecutionResult{} +} diff --git a/pkg/pipeline/vault.go b/pkg/pipeline/vault.go index 121ef065..0af723e9 100644 --- a/pkg/pipeline/vault.go +++ b/pkg/pipeline/vault.go @@ -31,6 +31,9 @@ func createOrUpdateVault(obj PipelineObject, data *ExecutionContext) ExecutionRe } vaultClient, err := getVaultClient(obj, data) + if err != nil { + return ExecutionResult{Err: err} + } err = vaultClient.AddSecrets([]vault.VaultSecret{{Path: secretPath, Value: token}}) if err != nil { diff --git a/pkg/pipeline/vault_test.go b/pkg/pipeline/vault_test.go new file mode 100644 index 00000000..0aa38727 --- /dev/null +++ b/pkg/pipeline/vault_test.go @@ -0,0 +1,149 @@ +package pipeline + +import ( + "testing" + + synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" + "github.com/projectsyn/lieutenant-operator/pkg/vault" + "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/log/zap" +) + +type testMockClient struct { + secrets []vault.VaultSecret + deletionPolicy synv1alpha1.DeletionPolicy +} + +func (m *testMockClient) AddSecrets(secrets []vault.VaultSecret) error { return nil } + +func (m *testMockClient) RemoveSecrets(secrets []vault.VaultSecret) error { return nil } + +func (m *testMockClient) SetDeletionPolicy(deletionPolicy synv1alpha1.DeletionPolicy) { + m.deletionPolicy = deletionPolicy +} + +func Test_getVaultClient(t *testing.T) { + type args struct { + obj *synv1alpha1.Cluster + data *ExecutionContext + } + tests := []struct { + name string + args args + want synv1alpha1.DeletionPolicy + wantErr bool + }{ + { + name: "without specific deletion policy", + want: getDefaultDeletionPolicy(), + args: args{ + obj: &synv1alpha1.Cluster{}, + data: &ExecutionContext{ + Log: zap.New(), + }, + }, + }, + { + name: "specific deletion policy", + want: synv1alpha1.DeletePolicy, + args: args{ + obj: &synv1alpha1.Cluster{ + Spec: synv1alpha1.ClusterSpec{ + DeletionPolicy: synv1alpha1.DeletePolicy, + }, + }, + data: &ExecutionContext{ + Log: zap.New(), + }, + }, + }, + } + + mockClient := &testMockClient{} + + vault.SetCustomClient(mockClient) + + for _, tt := range tests { + + t.Run(tt.name, func(t *testing.T) { + _, err := getVaultClient(tt.args.obj, tt.args.data) + assert.NoError(t, err) + + assert.Equal(t, tt.want, mockClient.deletionPolicy) + + }) + } +} + +func Test_handleVaultDeletion(t *testing.T) { + type args struct { + obj *synv1alpha1.Cluster + data *ExecutionContext + } + tests := []struct { + name string + args args + want synv1alpha1.DeletionPolicy + }{ + { + name: "noop", + want: getDefaultDeletionPolicy(), + args: args{ + obj: &synv1alpha1.Cluster{ + Spec: synv1alpha1.ClusterSpec{ + DeletionPolicy: getDefaultDeletionPolicy(), + }, + }, + data: &ExecutionContext{}, + }, + }, + { + name: "archive", + want: synv1alpha1.ArchivePolicy, + args: args{ + obj: &synv1alpha1.Cluster{ + Spec: synv1alpha1.ClusterSpec{ + DeletionPolicy: synv1alpha1.ArchivePolicy, + }, + }, + data: &ExecutionContext{ + Deleted: true, + }, + }, + }, + { + name: "delete", + want: synv1alpha1.DeletePolicy, + args: args{ + obj: &synv1alpha1.Cluster{ + Spec: synv1alpha1.ClusterSpec{ + DeletionPolicy: synv1alpha1.DeletePolicy, + }, + }, + data: &ExecutionContext{ + Deleted: true, + }, + }, + }, + } + + mockClient := &testMockClient{ + deletionPolicy: getDefaultDeletionPolicy(), + } + + vault.SetCustomClient(mockClient) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + + tt.args.data.Client, _ = testSetupClient([]runtime.Object{ + tt.args.obj, + }) + + got := handleVaultDeletion(tt.args.obj, tt.args.data) + assert.NoError(t, got.Err) + assert.Equal(t, tt.want, mockClient.deletionPolicy) + }) + } +} diff --git a/pkg/vault/client.go b/pkg/vault/client.go index 267e4b27..792aafcb 100644 --- a/pkg/vault/client.go +++ b/pkg/vault/client.go @@ -33,6 +33,7 @@ type VaultClient interface { AddSecrets(secrets []VaultSecret) error // remove specific secret RemoveSecrets(secret []VaultSecret) error + SetDeletionPolicy(synv1alpha1.DeletionPolicy) } type BankVaultClient struct { @@ -48,6 +49,7 @@ type BankVaultClient struct { func NewClient(deletionPolicy synv1alpha1.DeletionPolicy, log logr.Logger) (VaultClient, error) { if instanceClient != nil { + instanceClient.SetDeletionPolicy(deletionPolicy) return instanceClient, nil } @@ -259,3 +261,7 @@ func (b *BankVaultClient) listSecrets(secretPath string) ([]string, error) { return result, nil } + +func (b *BankVaultClient) SetDeletionPolicy(deletionPolicy synv1alpha1.DeletionPolicy) { + b.deletionPolicy = deletionPolicy +} From 371bc6958e0b619c6dc03a9534a408ac6e948d60 Mon Sep 17 00:00:00 2001 From: Simon Beck Date: Thu, 22 Oct 2020 12:40:59 +0200 Subject: [PATCH 09/23] Last batch of unit tests --- .../cluster/cluster_reconcile_test.go | 2 + .../tenant/tenant_reconcile_test.go | 3 +- pkg/pipeline/git_test.go | 175 ++++++++++++++++ pkg/pipeline/tenant_steps.go | 7 +- pkg/pipeline/tenant_steps_test.go | 188 ++++++++++++++++++ 5 files changed, 372 insertions(+), 3 deletions(-) create mode 100644 pkg/pipeline/tenant_steps_test.go diff --git a/pkg/controller/cluster/cluster_reconcile_test.go b/pkg/controller/cluster/cluster_reconcile_test.go index ca0eb721..f890066c 100644 --- a/pkg/controller/cluster/cluster_reconcile_test.go +++ b/pkg/controller/cluster/cluster_reconcile_test.go @@ -306,6 +306,8 @@ func (m *TestMockClient) RemoveSecrets(secrets []vault.VaultSecret) error { return nil } +func (m *TestMockClient) SetDeletionPolicy(deletionPolicy synv1alpha1.DeletionPolicy) {} + func TestReconcileCluster_getServiceAccountToken(t *testing.T) { type args struct { instance metav1.Object diff --git a/pkg/controller/tenant/tenant_reconcile_test.go b/pkg/controller/tenant/tenant_reconcile_test.go index cce926ec..2891756d 100644 --- a/pkg/controller/tenant/tenant_reconcile_test.go +++ b/pkg/controller/tenant/tenant_reconcile_test.go @@ -49,8 +49,7 @@ func TestHandleNilGitRepoTemplate(t *testing.T) { updatedTenant := &synv1alpha1.Tenant{} err = cl.Get(context.TODO(), types.NamespacedName{Name: tenant.Name}, updatedTenant) assert.NoError(t, err) - assert.Nil(t, tenant.Spec.GitRepoTemplate) - assert.Nil(t, updatedTenant.Spec.GitRepoTemplate) + assert.Contains(t, updatedTenant.Spec.GitRepoTemplate.TemplateFiles, "common.yml") } func TestCreateGitRepo(t *testing.T) { diff --git a/pkg/pipeline/git_test.go b/pkg/pipeline/git_test.go index 4d325a4d..8aa7abfa 100644 --- a/pkg/pipeline/git_test.go +++ b/pkg/pipeline/git_test.go @@ -2,14 +2,18 @@ package pipeline import ( "context" + "fmt" + "net/url" "testing" synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" + "github.com/projectsyn/lieutenant-operator/pkg/git/manager" "github.com/stretchr/testify/assert" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" ) func TestCreateOrUpdateGitRepo(t *testing.T) { @@ -176,3 +180,174 @@ func Test_fetchGitRepoTemplate(t *testing.T) { }) } } + +type repoMock struct { + failRead bool +} + +func (r *repoMock) Type() string { return "mock" } +func (r *repoMock) FullURL() *url.URL { return &url.URL{} } +func (r *repoMock) Create() error { return nil } +func (r *repoMock) Update() (bool, error) { return false, nil } +func (r *repoMock) Read() error { + if r.failRead { + return fmt.Errorf("this should fail") + } + return nil +} +func (r *repoMock) Connect() error { return nil } +func (r *repoMock) Remove() error { return nil } +func (r *repoMock) CommitTemplateFiles() error { return nil } + +func Test_repoExists(t *testing.T) { + type args struct { + repo manager.Repo + } + tests := []struct { + name string + args args + want bool + }{ + { + name: "repo exists", + want: true, + args: args{ + repo: &repoMock{}, + }, + }, + { + name: "repo doesn't exist", + want: false, + args: args{ + repo: &repoMock{ + failRead: true, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := repoExists(tt.args.repo); got != tt.want { + t.Errorf("repoExists() = %v, want %v", got, tt.want) + } + }) + } +} + +func Test_handleRepoError(t *testing.T) { + type args struct { + err error + instance *synv1alpha1.GitRepo + repo manager.Repo + fail bool + } + tests := []struct { + name string + args args + }{ + { + name: "add error", + args: args{ + err: fmt.Errorf("lol nope"), + instance: &synv1alpha1.GitRepo{}, + repo: &repoMock{}, + }, + }, + { + name: "add error failure", + args: args{ + err: fmt.Errorf("lol nope"), + instance: &synv1alpha1.GitRepo{}, + repo: &repoMock{}, + fail: true, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + + var client client.Client + + if tt.args.fail { + client, _ = testSetupClient([]runtime.Object{}) + } else { + client, _ = testSetupClient([]runtime.Object{tt.args.instance}) + } + + err := handleRepoError(tt.args.err, tt.args.instance, tt.args.repo, client) + assert.Error(t, err) + failedPhase := synv1alpha1.Failed + assert.Equal(t, &failedPhase, tt.args.instance.Status.Phase) + + if tt.args.fail { + assert.Contains(t, err.Error(), "could not set status") + } + + }) + } +} + +func Test_setGitRepoURLAndHostKeys(t *testing.T) { + type args struct { + obj *synv1alpha1.Cluster + gitRepo *synv1alpha1.GitRepo + data *ExecutionContext + } + tests := []struct { + name string + args args + wantErr bool + }{ + { + name: "set url and keys", + wantErr: false, + args: args{ + obj: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + }, + data: &ExecutionContext{}, + gitRepo: &synv1alpha1.GitRepo{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + Status: synv1alpha1.GitRepoStatus{ + URL: "someURL", + HostKeys: "someKeys", + }, + }, + }, + }, + { + name: "set url and keys not found", + wantErr: false, + args: args{ + obj: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "invalid", + }, + }, + data: &ExecutionContext{}, + gitRepo: &synv1alpha1.GitRepo{}, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + + tt.args.data.Client, _ = testSetupClient([]runtime.Object{ + tt.args.obj, + tt.args.gitRepo, + }) + + if got := setGitRepoURLAndHostKeys(tt.args.obj, tt.args.data); (got.Err != nil) != tt.wantErr { + t.Errorf("setGitRepoURLAndHostKeys() = had error: %v", got.Err) + } + + assert.Equal(t, tt.args.gitRepo.Status.URL, tt.args.obj.Spec.GitRepoURL) + assert.Equal(t, tt.args.gitRepo.Status.HostKeys, tt.args.obj.Spec.GitHostKeys) + + }) + } +} diff --git a/pkg/pipeline/tenant_steps.go b/pkg/pipeline/tenant_steps.go index 20816a4f..b0b89c04 100644 --- a/pkg/pipeline/tenant_steps.go +++ b/pkg/pipeline/tenant_steps.go @@ -30,7 +30,12 @@ func updateTenantGitRepo(obj PipelineObject, data *ExecutionContext) ExecutionRe return ExecutionResult{Err: fmt.Errorf("object is not a tenant")} } - oldFiles := tenantCR.Spec.GitRepoTemplate.TemplateFiles + var oldFiles map[string]string + if tenantCR.Spec.GitRepoTemplate != nil { + oldFiles = tenantCR.Spec.GitRepoTemplate.TemplateFiles + } else { + tenantCR.Spec.GitRepoTemplate = &synv1alpha1.GitRepoTemplate{} + } tenantCR.Spec.GitRepoTemplate.TemplateFiles = map[string]string{} diff --git a/pkg/pipeline/tenant_steps_test.go b/pkg/pipeline/tenant_steps_test.go new file mode 100644 index 00000000..44fab059 --- /dev/null +++ b/pkg/pipeline/tenant_steps_test.go @@ -0,0 +1,188 @@ +package pipeline + +import ( + "os" + "testing" + + "github.com/projectsyn/lieutenant-operator/pkg/apis" + synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" + "github.com/stretchr/testify/assert" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" +) + +func Test_addDefaultClassFile(t *testing.T) { + type args struct { + obj *synv1alpha1.Tenant + data *ExecutionContext + } + tests := []struct { + name string + args args + }{ + { + name: "add default class", + args: args{ + obj: &synv1alpha1.Tenant{}, + data: &ExecutionContext{}, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + + got := addDefaultClassFile(tt.args.obj, tt.args.data) + assert.NoError(t, got.Err) + + assert.Contains(t, tt.args.obj.Spec.GitRepoTemplate.TemplateFiles, "common.yml") + assert.NotEmpty(t, tt.args.obj.Spec.GitRepoTemplate.TemplateFiles) + + }) + } +} + +func Test_setGlobalGitRepoURL(t *testing.T) { + type args struct { + obj *synv1alpha1.Tenant + data *ExecutionContext + defaultRepo string + } + tests := []struct { + name string + want string + args args + }{ + { + name: "set global git repo url via env var", + want: "test", + args: args{ + obj: &synv1alpha1.Tenant{}, + defaultRepo: "test", + }, + }, + { + name: "don't override", + want: "foo", + args: args{ + obj: &synv1alpha1.Tenant{ + Spec: synv1alpha1.TenantSpec{ + GlobalGitRepoURL: "foo", + }, + }, + defaultRepo: "test", + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + + os.Setenv(DefaultGlobalGitRepoURL, tt.args.defaultRepo) + + got := setGlobalGitRepoURL(tt.args.obj, tt.args.data) + assert.NoError(t, got.Err) + + assert.Equal(t, tt.want, tt.args.obj.Spec.GlobalGitRepoURL) + + }) + } +} + +func Test_updateTenantGitRepo(t *testing.T) { + type args struct { + obj *synv1alpha1.Tenant + cluster *synv1alpha1.Cluster + data *ExecutionContext + } + tests := []struct { + name string + args args + want *synv1alpha1.GitRepoTemplate + }{ + { + name: "fetch git repos", + want: &synv1alpha1.GitRepoTemplate{ + TemplateFiles: map[string]string{ + "testCluster.yml": "classes:\n- testCluster.common\n", + }, + }, + args: args{ + data: &ExecutionContext{}, + obj: &synv1alpha1.Tenant{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testTenant", + }, + }, + cluster: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testCluster", + Labels: map[string]string{ + apis.LabelNameTenant: "testTenant", + }, + }, + Spec: synv1alpha1.ClusterSpec{ + GitRepoTemplate: &synv1alpha1.GitRepoTemplate{ + TemplateFiles: map[string]string{ + "testCluster.yml": "classes:\n- testCluster.common\n", + }, + }, + }, + }, + }, + }, + { + name: "remove files", + want: &synv1alpha1.GitRepoTemplate{ + TemplateFiles: map[string]string{ + "testCluster.yml": "classes:\n- testCluster.common\n", + "oldFile.yml": "{delete}", + }, + }, + args: args{ + data: &ExecutionContext{}, + obj: &synv1alpha1.Tenant{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testTenant", + }, + Spec: synv1alpha1.TenantSpec{ + GitRepoTemplate: &synv1alpha1.GitRepoTemplate{ + TemplateFiles: map[string]string{ + "oldFile.yml": "not important", + }, + }, + }, + }, + cluster: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testCluster", + Labels: map[string]string{ + apis.LabelNameTenant: "testTenant", + }, + }, + Spec: synv1alpha1.ClusterSpec{ + GitRepoTemplate: &synv1alpha1.GitRepoTemplate{ + TemplateFiles: map[string]string{ + "testCluster.yml": "classes:\n- testCluster.common\n", + }, + }, + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + + tt.args.data.Client, _ = testSetupClient([]runtime.Object{ + tt.args.cluster, + tt.args.obj, + &synv1alpha1.ClusterList{}, + }) + + got := updateTenantGitRepo(tt.args.obj, tt.args.data) + assert.NoError(t, got.Err) + + assert.Equal(t, tt.want, tt.args.obj.GetGitTemplate()) + + }) + } +} From 39de8782d3b483405d56a8c1813e93b13eac07f0 Mon Sep 17 00:00:00 2001 From: Simon Beck Date: Thu, 22 Oct 2020 08:37:30 +0200 Subject: [PATCH 10/23] Fix failing unit tests Some changes to the order of thins make the unit tests fail. Also there were tests that checked the older push method for gitRepo changes. Those were removed. --- pkg/apis/syn/v1alpha1/tenant_types.go | 3 +++ pkg/controller/cluster/cluster_reconcile_test.go | 10 ++-------- pkg/controller/gitrepo/gitrepo_reconcile_test.go | 2 ++ pkg/controller/tenant/tenant_reconcile_test.go | 6 +++++- 4 files changed, 12 insertions(+), 9 deletions(-) diff --git a/pkg/apis/syn/v1alpha1/tenant_types.go b/pkg/apis/syn/v1alpha1/tenant_types.go index 4a001a38..77bd1792 100644 --- a/pkg/apis/syn/v1alpha1/tenant_types.go +++ b/pkg/apis/syn/v1alpha1/tenant_types.go @@ -66,6 +66,9 @@ func init() { // GetGitTemplate returns the git repository template func (t *Tenant) GetGitTemplate() *GitRepoTemplate { + if t.Spec.GitRepoTemplate == nil { + t.Spec.GitRepoTemplate = &GitRepoTemplate{} + } return t.Spec.GitRepoTemplate } diff --git a/pkg/controller/cluster/cluster_reconcile_test.go b/pkg/controller/cluster/cluster_reconcile_test.go index f890066c..73f10675 100644 --- a/pkg/controller/cluster/cluster_reconcile_test.go +++ b/pkg/controller/cluster/cluster_reconcile_test.go @@ -2,7 +2,6 @@ package cluster import ( "context" - "fmt" "os" "path" "reflect" @@ -76,7 +75,7 @@ func TestReconcileCluster_NoTenant(t *testing.T) { _, err := r.Reconcile(req) assert.Error(t, err) - assert.Contains(t, err.Error(), "find tenant") + assert.Contains(t, err.Error(), "no matching secrets found") } func TestReconcileCluster_NoGitRepoTemplate(t *testing.T) { @@ -99,6 +98,7 @@ func TestReconcileCluster_NoGitRepoTemplate(t *testing.T) { objs := []runtime.Object{ tenant, cluster, + &synv1alpha1.GitRepo{}, } cl, s := testSetupClient(objs) @@ -283,12 +283,6 @@ func TestReconcileCluster_Reconcile(t *testing.T) { assert.Equal(t, roleBinding.RoleRef.Name, role.Name) assert.Equal(t, roleBinding.Subjects[0].Name, sa.Name) - testTenant := &synv1alpha1.Tenant{} - err = cl.Get(context.TODO(), types.NamespacedName{Name: tt.fields.tenantName, Namespace: tt.fields.objNamespace}, testTenant) - assert.NoError(t, err) - fileContent, found := testTenant.Spec.GitRepoTemplate.TemplateFiles[tt.fields.objName+".yml"] - assert.True(t, found) - assert.Equal(t, fileContent, fmt.Sprintf(clusterClassContent, tt.fields.tenantName, pipeline.CommonClassName)) }) } } diff --git a/pkg/controller/gitrepo/gitrepo_reconcile_test.go b/pkg/controller/gitrepo/gitrepo_reconcile_test.go index 0581f433..bb814ada 100644 --- a/pkg/controller/gitrepo/gitrepo_reconcile_test.go +++ b/pkg/controller/gitrepo/gitrepo_reconcile_test.go @@ -110,6 +110,8 @@ func TestReconcileGitRepo_Reconcile(t *testing.T) { objs := []runtime.Object{ repo, + &synv1alpha1.Tenant{}, + &synv1alpha1.Cluster{}, } cl, s := testSetupClient(objs) diff --git a/pkg/controller/tenant/tenant_reconcile_test.go b/pkg/controller/tenant/tenant_reconcile_test.go index 2891756d..42f65668 100644 --- a/pkg/controller/tenant/tenant_reconcile_test.go +++ b/pkg/controller/tenant/tenant_reconcile_test.go @@ -34,7 +34,11 @@ func TestHandleNilGitRepoTemplate(t *testing.T) { }, } - cl, s := testSetupClient([]runtime.Object{tenant}) + cl, s := testSetupClient([]runtime.Object{ + tenant, + &synv1alpha1.ClusterList{}, + &synv1alpha1.GitRepo{}, + }) r := &ReconcileTenant{client: cl, scheme: s} From 53bc48e948dcd487fb8e2ee04a3694069dfe0891 Mon Sep 17 00:00:00 2001 From: Simon Beck Date: Thu, 22 Oct 2020 12:50:29 +0200 Subject: [PATCH 11/23] Update changelog --- CHANGELOG.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ff1226b..75c85a1e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,14 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). + +## [unreleased] - 2020-10-22 +### Changed +- Redesigned reconcile handling ([#120]) +- GitRepo objects now fetch their information instead of them receiving from other controllers ([#120]) +### Fixed +- Lot's of race conditions and smaller bugs ([#120]) + ## [v0.4.0] - 2020-10-20 ### Added - Configurable revisions for git repositories ([#116]) @@ -113,3 +121,4 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#104]: https://github.com/projectsyn/lieutenant-operator/pull/104 [#110]: https://github.com/projectsyn/lieutenant-operator/pull/110 [#116]: https://github.com/projectsyn/lieutenant-operator/pull/116 +[#120]: https://github.com/projectsyn/lieutenant-operator/pull/120 From 97644942780bd3d4a78ebee25a94c53b69762c6d Mon Sep 17 00:00:00 2001 From: Simon Beck Date: Fri, 23 Oct 2020 09:46:56 +0200 Subject: [PATCH 12/23] Remove kubectl dependency --- go.mod | 1 - pkg/pipeline/common_steps_test.go | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/go.mod b/go.mod index 1e8667a5..a9b0ccf0 100644 --- a/go.mod +++ b/go.mod @@ -17,7 +17,6 @@ require ( k8s.io/apimachinery v0.17.4 k8s.io/client-go v12.0.0+incompatible k8s.io/kube-openapi v0.0.0-20191107075043-30be4d16710a - k8s.io/kubectl v0.17.4 k8s.io/utils v0.0.0-20191114200735-6ca3b61696b6 sigs.k8s.io/controller-runtime v0.5.2 ) diff --git a/pkg/pipeline/common_steps_test.go b/pkg/pipeline/common_steps_test.go index 1b0b4059..b7dffef9 100644 --- a/pkg/pipeline/common_steps_test.go +++ b/pkg/pipeline/common_steps_test.go @@ -12,7 +12,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/kubectl/pkg/scheme" + "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" ) From 3dca636eaf39b1fa3c7f44fe0554700ab4c37a22 Mon Sep 17 00:00:00 2001 From: Simon Beck Date: Fri, 23 Oct 2020 10:57:45 +0200 Subject: [PATCH 13/23] Use loops to execute steps --- pkg/pipeline/cluster.go | 57 +++++--------------------- pkg/pipeline/common.go | 34 ++++------------ pkg/pipeline/common_steps.go | 5 ++- pkg/pipeline/pipelines.go | 78 ++++++++++++++---------------------- pkg/pipeline/tenant.go | 22 +++------- pkg/pipeline/vault.go | 11 +++++ 6 files changed, 68 insertions(+), 139 deletions(-) diff --git a/pkg/pipeline/cluster.go b/pkg/pipeline/cluster.go index 592be44d..f4766a9a 100644 --- a/pkg/pipeline/cluster.go +++ b/pkg/pipeline/cluster.go @@ -1,10 +1,5 @@ package pipeline -import ( - "os" - "strings" -) - const ( clusterClassContent = `classes: - %s.%s @@ -12,50 +7,18 @@ const ( ) func clusterSpecificSteps(obj PipelineObject, data *ExecutionContext) ExecutionResult { - result := createClusterRBAC(obj, data) - if resultNotOK(result) { - result.Err = wrapError("create cluster RBAC", result.Err) - return result - } - - result = checkIfDeleted(obj, data) - if resultNotOK(result) { - result.Err = wrapError("deletion check", result.Err) - return result - } - - result = setBootstrapToken(obj, data) - if resultNotOK(result) { - result.Err = wrapError("set bootstrap token", result.Err) - return result - } - - if strings.ToLower(os.Getenv("SKIP_VAULT_SETUP")) != "true" { - result = createOrUpdateVault(obj, data) - if resultNotOK(result) { - result.Err = wrapError("create or update vault", result.Err) - return result - } - - result = handleVaultDeletion(obj, data) - if resultNotOK(result) { - result.Err = wrapError("delete vault entries", result.Err) - return result - } + steps := []Step{ + {Name: "create cluster RBAC", F: createClusterRBAC}, + {Name: "deletion check", F: checkIfDeleted}, + {Name: "set bootstrap token", F: setBootstrapToken}, + {Name: "create or update vault", F: createOrUpdateVault}, + {Name: "delete vault entries", F: handleVaultDeletion}, + {Name: "set tenant owner", F: setTenantOwner}, + {Name: "apply tenant template", F: applyTenantTemplate}, } - result = setTenantOwner(obj, data) - if resultNotOK(result) { - result.Err = wrapError("set tenant owner", result.Err) - return result - } - - result = applyTenantTemplate(obj, data) - if resultNotOK(result) { - result.Err = wrapError("apply tenant template", result.Err) - return result - } + err := RunPipeline(obj, data, steps) - return ExecutionResult{} + return ExecutionResult{Err: err} } diff --git a/pkg/pipeline/common.go b/pkg/pipeline/common.go index a085fba8..6215e6f9 100644 --- a/pkg/pipeline/common.go +++ b/pkg/pipeline/common.go @@ -10,35 +10,15 @@ const ( func common(obj PipelineObject, data *ExecutionContext) ExecutionResult { - result := handleDeletion(obj.GetObjectMeta(), data) - if result.Abort || result.Err != nil { - result.Err = wrapError("deletion", result.Err) - return result + steps := []Step{ + {Name: "deletion", F: handleDeletion}, + {Name: "add deletion protection", F: addDeletionProtection}, + {Name: "handle finalizer", F: handleFinalizer}, + {Name: "update object", F: updateObject}, } - result = addTenantLabel(obj, data) - if result.Abort || result.Err != nil { - result.Err = wrapError("add tenant label", result.Err) - return result - } - - result = addDeletionProtection(obj, data) - if result.Abort || result.Err != nil { - result.Err = wrapError("add deletion protection", result.Err) - return result - } + err := RunPipeline(obj, data, steps) - result = handleFinalizer(obj, data) - if result.Abort || result.Err != nil { - result.Err = wrapError("add deletion protection", result.Err) - return result - } - - result = updateObject(obj, data) - if result.Abort || result.Err != nil { - result.Err = wrapError("update object", result.Err) - return result - } + return ExecutionResult{Err: err} - return ExecutionResult{} } diff --git a/pkg/pipeline/common_steps.go b/pkg/pipeline/common_steps.go index fbac42a7..06cca5ad 100644 --- a/pkg/pipeline/common_steps.go +++ b/pkg/pipeline/common_steps.go @@ -8,7 +8,6 @@ import ( "github.com/projectsyn/lieutenant-operator/pkg/apis" synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) @@ -107,11 +106,13 @@ func resultNotOK(result ExecutionResult) bool { // handleDeletion will handle the finalizers if the object was deleted. // It will only trigger if data.Deleted is true. -func handleDeletion(instance metav1.Object, data *ExecutionContext) ExecutionResult { +func handleDeletion(obj PipelineObject, data *ExecutionContext) ExecutionResult { if !data.Deleted { return ExecutionResult{} } + instance := obj.GetObjectMeta() + annotationValue, exists := instance.GetAnnotations()[DeleteProtectionAnnotation] var protected bool diff --git a/pkg/pipeline/pipelines.go b/pkg/pipeline/pipelines.go index 424f0a2f..02111b02 100644 --- a/pkg/pipeline/pipelines.go +++ b/pkg/pipeline/pipelines.go @@ -3,70 +3,54 @@ package pipeline // Function defines the general form of a pipeline function. type Function func(PipelineObject, *ExecutionContext) ExecutionResult -func ReconcileTenant(obj PipelineObject, data *ExecutionContext) error { - - result := tenantSpecificSteps(obj, data) - if resultNotOK(result) { - return wrapError("tenant specific steps", result.Err) - } +type Step struct { + Name string + F Function +} - result = createGitRepo(obj, data) - if resultNotOK(result) { - return wrapError("create git repo", result.Err) - } +func ReconcileTenant(obj PipelineObject, data *ExecutionContext) error { - result = setGitRepoURLAndHostKeys(obj, data) - if resultNotOK(result) { - return wrapError("set gitrepo url and hostkeys", result.Err) + steps := []Step{ + {Name: "tenant specific steps", F: tenantSpecificSteps}, + {Name: "create git repo", F: createGitRepo}, + {Name: "set gitrepo url and hostkeys", F: setGitRepoURLAndHostKeys}, + {Name: "common", F: common}, } - result = common(obj, data) - if resultNotOK(result) { - return wrapError("common", result.Err) - } - return nil + return RunPipeline(obj, data, steps) } func ReconcileCluster(obj PipelineObject, data *ExecutionContext) error { - result := clusterSpecificSteps(obj, data) - if resultNotOK(result) { - return wrapError("cluster specific steps failes", result.Err) - } - - result = createGitRepo(obj, data) - if resultNotOK(result) { - return wrapError("create or uptdate git repo", result.Err) - } - - result = setGitRepoURLAndHostKeys(obj, data) - if resultNotOK(result) { - return wrapError("set gitrepo url and hostkeys", result.Err) - } - - result = common(obj, data) - if resultNotOK(result) { - return wrapError("common", result.Err) + steps := []Step{ + {Name: "cluster specific steps", F: clusterSpecificSteps}, + {Name: "create git repo", F: createGitRepo}, + {Name: "set gitrepo url and hostkeys", F: setGitRepoURLAndHostKeys}, + {Name: "add tenant label", F: addTenantLabel}, + {Name: "common", F: common}, } - return nil + return RunPipeline(obj, data, steps) } func ReconcileGitRep(obj PipelineObject, data *ExecutionContext) error { - result := checkIfDeleted(obj, data) - if resultNotOK(result) { - return wrapError("deletion check", result.Err) + steps := []Step{ + {Name: "deletion check", F: checkIfDeleted}, + {Name: "git repo specific steps", F: gitRepoSpecificSteps}, + {Name: "add tenant label", F: addTenantLabel}, + {Name: "common", F: common}, } - result = gitRepoSpecificSteps(obj, data) - if resultNotOK(result) { - return wrapError("git repo specific steps failes", result.Err) - } + return RunPipeline(obj, data, steps) +} + +func RunPipeline(obj PipelineObject, data *ExecutionContext, steps []Step) error { - result = common(obj, data) - if resultNotOK(result) { - return wrapError("common", result.Err) + for _, step := range steps { + if r := step.F(obj, data); resultNotOK(r) { + return wrapError(step.Name, r.Err) + } } return nil diff --git a/pkg/pipeline/tenant.go b/pkg/pipeline/tenant.go index 96bed8b3..3879c15f 100644 --- a/pkg/pipeline/tenant.go +++ b/pkg/pipeline/tenant.go @@ -8,24 +8,14 @@ const ( func tenantSpecificSteps(obj PipelineObject, data *ExecutionContext) ExecutionResult { - result := addDefaultClassFile(obj, data) - if resultNotOK(result) { - result.Err = wrapError("add default class file", result.Err) - return result + steps := []Step{ + {Name: "add default class file", F: addDefaultClassFile}, + {Name: "uptade tenant git repo", F: updateTenantGitRepo}, + {Name: "set global git repo url", F: setGlobalGitRepoURL}, } - result = updateTenantGitRepo(obj, data) - if resultNotOK(result) { - result.Err = wrapError("update tenant git repo", result.Err) - return result - } - - result = setGlobalGitRepoURL(obj, data) - if resultNotOK(result) { - result.Err = wrapError("set global gir repo URL", result.Err) - return result - } + err := RunPipeline(obj, data, steps) - return ExecutionResult{} + return ExecutionResult{Err: err} } diff --git a/pkg/pipeline/vault.go b/pkg/pipeline/vault.go index 0af723e9..a753eecb 100644 --- a/pkg/pipeline/vault.go +++ b/pkg/pipeline/vault.go @@ -3,8 +3,10 @@ package pipeline import ( "context" "fmt" + "os" "path" "sort" + "strings" "github.com/projectsyn/lieutenant-operator/pkg/helpers" "github.com/projectsyn/lieutenant-operator/pkg/vault" @@ -23,6 +25,10 @@ func getVaultClient(obj PipelineObject, data *ExecutionContext) (vault.VaultClie } func createOrUpdateVault(obj PipelineObject, data *ExecutionContext) ExecutionResult { + if strings.ToLower(os.Getenv("SKIP_VAULT_SETUP")) == "true" { + return ExecutionResult{} + } + secretPath := path.Join(obj.GetTenantRef().Name, obj.GetObjectMeta().GetName(), "steward") token, err := GetServiceAccountToken(obj.GetObjectMeta(), data) @@ -74,6 +80,11 @@ func GetServiceAccountToken(instance metav1.Object, data *ExecutionContext) (str } func handleVaultDeletion(obj PipelineObject, data *ExecutionContext) ExecutionResult { + + if strings.ToLower(os.Getenv("SKIP_VAULT_SETUP")) == "true" { + return ExecutionResult{} + } + repoName := types.NamespacedName{ Name: obj.GetTenantRef().Name, Namespace: obj.GetObjectMeta().GetNamespace(), From d6afaee03b95ea347835b4360df646b2dd10497d Mon Sep 17 00:00:00 2001 From: Simon Beck Date: Fri, 23 Oct 2020 11:03:38 +0200 Subject: [PATCH 14/23] Get rid of generic utils package --- pkg/{helpers => collection}/values.go | 2 +- pkg/{helpers => collection}/values_test.go | 2 +- pkg/pipeline/vault.go | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) rename pkg/{helpers => collection}/values.go (98%) rename pkg/{helpers => collection}/values_test.go (97%) diff --git a/pkg/helpers/values.go b/pkg/collection/values.go similarity index 98% rename from pkg/helpers/values.go rename to pkg/collection/values.go index b8425773..9c94c98a 100644 --- a/pkg/helpers/values.go +++ b/pkg/collection/values.go @@ -1,4 +1,4 @@ -package helpers +package collection import ( "bytes" diff --git a/pkg/helpers/values_test.go b/pkg/collection/values_test.go similarity index 97% rename from pkg/helpers/values_test.go rename to pkg/collection/values_test.go index 77735122..635e0c3d 100644 --- a/pkg/helpers/values_test.go +++ b/pkg/collection/values_test.go @@ -1,4 +1,4 @@ -package helpers +package collection import ( "testing" diff --git a/pkg/pipeline/vault.go b/pkg/pipeline/vault.go index a753eecb..ae322041 100644 --- a/pkg/pipeline/vault.go +++ b/pkg/pipeline/vault.go @@ -8,7 +8,7 @@ import ( "sort" "strings" - "github.com/projectsyn/lieutenant-operator/pkg/helpers" + "github.com/projectsyn/lieutenant-operator/pkg/collection" "github.com/projectsyn/lieutenant-operator/pkg/vault" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -57,7 +57,7 @@ func GetServiceAccountToken(instance metav1.Object, data *ExecutionContext) (str return "", err } - sortSecrets := helpers.SecretSortList(*secrets) + sortSecrets := collection.SecretSortList(*secrets) sort.Sort(sort.Reverse(sortSecrets)) From efdeb37f68ab203b2a395f196d24ad35a13ab75e Mon Sep 17 00:00:00 2001 From: Simon Beck Date: Fri, 23 Oct 2020 13:05:42 +0200 Subject: [PATCH 15/23] Move test cases outside of tests funcs --- pkg/collection/values.go | 17 - pkg/collection/values_test.go | 34 -- pkg/pipeline/cluster/cluster_template.go | 18 +- pkg/pipeline/cluster/cluster_template_test.go | 27 ++ pkg/pipeline/cluster_steps_test.go | 267 ++++++------ pkg/pipeline/common_steps_test.go | 379 ++++++++---------- pkg/pipeline/reconcile_types_test.go | 16 + pkg/pipeline/tenant_steps_test.go | 229 +++++------ pkg/pipeline/vault_test.go | 153 ++++--- 9 files changed, 525 insertions(+), 615 deletions(-) delete mode 100644 pkg/collection/values_test.go create mode 100644 pkg/pipeline/reconcile_types_test.go diff --git a/pkg/collection/values.go b/pkg/collection/values.go index 9c94c98a..737aa34a 100644 --- a/pkg/collection/values.go +++ b/pkg/collection/values.go @@ -1,10 +1,6 @@ package collection import ( - "bytes" - "fmt" - "text/template" - corev1 "k8s.io/api/core/v1" ) @@ -26,16 +22,3 @@ func (s SecretSortList) Less(i, j int) bool { return s.Items[i].CreationTimestamp.Before(&s.Items[j].CreationTimestamp) } - -// RenderTemplate renders a given template with the given data -func RenderTemplate(tmpl string, data interface{}) (string, error) { - tmp, err := template.New("template").Parse(tmpl) - if err != nil { - return "", fmt.Errorf("Could not parse template: %w", err) - } - buf := new(bytes.Buffer) - if err := tmp.Execute(buf, data); err != nil { - return "", fmt.Errorf("Could not render template: %w", err) - } - return buf.String(), nil -} diff --git a/pkg/collection/values_test.go b/pkg/collection/values_test.go deleted file mode 100644 index 635e0c3d..00000000 --- a/pkg/collection/values_test.go +++ /dev/null @@ -1,34 +0,0 @@ -package collection - -import ( - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestRenderTemplateRawString(t *testing.T) { - str, err := RenderTemplate("raw string", nil) - assert.NoError(t, err) - assert.Equal(t, "raw string", str) -} - -func TestRenderTemplateData(t *testing.T) { - str, err := RenderTemplate("{{ .Some }}/{{ .Data }}", struct { - Some string - Data string - }{"some", "data"}) - assert.NoError(t, err) - assert.Equal(t, "some/data", str) -} - -func TestRenderTemplateSyntaxError(t *testing.T) { - _, err := RenderTemplate("{{ }", nil) - assert.Error(t, err) - assert.Contains(t, err.Error(), "parse") -} - -func TestRenderTemplateDataError(t *testing.T) { - _, err := RenderTemplate("{{ .none }}", "data") - assert.Error(t, err) - assert.Contains(t, err.Error(), "render") -} diff --git a/pkg/pipeline/cluster/cluster_template.go b/pkg/pipeline/cluster/cluster_template.go index c1f8498d..01dba343 100644 --- a/pkg/pipeline/cluster/cluster_template.go +++ b/pkg/pipeline/cluster/cluster_template.go @@ -1,11 +1,12 @@ package cluster import ( + "bytes" "fmt" + "text/template" "github.com/imdario/mergo" synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" - "github.com/projectsyn/lieutenant-operator/pkg/helpers" "github.com/ryankurte/go-structparse" ) @@ -18,7 +19,7 @@ func (r *templateParser) ParseString(in string) interface{} { if r.err != nil || len(in) == 0 { return in } - str, err := helpers.RenderTemplate(in, r.cluster) + str, err := RenderTemplate(in, r.cluster) if err != nil { r.err = err return in @@ -50,3 +51,16 @@ func ApplyClusterTemplate(cluster *synv1alpha1.Cluster, tenant *synv1alpha1.Tena return nil } + +// RenderTemplate renders a given template with the given data +func RenderTemplate(tmpl string, data interface{}) (string, error) { + tmp, err := template.New("template").Parse(tmpl) + if err != nil { + return "", fmt.Errorf("Could not parse template: %w", err) + } + buf := new(bytes.Buffer) + if err := tmp.Execute(buf, data); err != nil { + return "", fmt.Errorf("Could not render template: %w", err) + } + return buf.String(), nil +} diff --git a/pkg/pipeline/cluster/cluster_template_test.go b/pkg/pipeline/cluster/cluster_template_test.go index 9ac1edc0..c81f50b9 100644 --- a/pkg/pipeline/cluster/cluster_template_test.go +++ b/pkg/pipeline/cluster/cluster_template_test.go @@ -100,3 +100,30 @@ func TestApplyClusterTemplateFail(t *testing.T) { assert.Error(t, err) assert.Contains(t, err.Error(), "render") } + +func TestRenderTemplateRawString(t *testing.T) { + str, err := RenderTemplate("raw string", nil) + assert.NoError(t, err) + assert.Equal(t, "raw string", str) +} + +func TestRenderTemplateData(t *testing.T) { + str, err := RenderTemplate("{{ .Some }}/{{ .Data }}", struct { + Some string + Data string + }{"some", "data"}) + assert.NoError(t, err) + assert.Equal(t, "some/data", str) +} + +func TestRenderTemplateSyntaxError(t *testing.T) { + _, err := RenderTemplate("{{ }", nil) + assert.Error(t, err) + assert.Contains(t, err.Error(), "parse") +} + +func TestRenderTemplateDataError(t *testing.T) { + _, err := RenderTemplate("{{ .none }}", "data") + assert.Error(t, err) + assert.Contains(t, err.Error(), "render") +} diff --git a/pkg/pipeline/cluster_steps_test.go b/pkg/pipeline/cluster_steps_test.go index 524dd3d0..53589d62 100644 --- a/pkg/pipeline/cluster_steps_test.go +++ b/pkg/pipeline/cluster_steps_test.go @@ -14,39 +14,32 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log/zap" ) -func Test_createClusterRBAC(t *testing.T) { - type args struct { - obj *synv1alpha1.Cluster - data *ExecutionContext - } - tests := []struct { - name string - args args - wantErr bool - }{ - { - name: "create cluster RBAC", - wantErr: false, - args: args{ - obj: &synv1alpha1.Cluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - Namespace: "testnamespace", - }, +var rbacCases = genericCases{ + "create cluster RBAC": { + wantErr: false, + args: args{ + cluster: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "testnamespace", }, }, }, - } - for _, tt := range tests { + }, +} + +func Test_createClusterRBAC(t *testing.T) { + + for name, tt := range rbacCases { client, _ := testSetupClient([]runtime.Object{ - tt.args.obj, + tt.args.cluster, }) tt.args.data = &ExecutionContext{Client: client} - t.Run(tt.name, func(t *testing.T) { - if got := createClusterRBAC(tt.args.obj, tt.args.data); got.Err != nil != tt.wantErr { + t.Run(name, func(t *testing.T) { + if got := createClusterRBAC(tt.args.cluster, tt.args.data); got.Err != nil != tt.wantErr { t.Errorf("createClusterRBAC() = had error: %v", got.Err) } }) @@ -54,7 +47,7 @@ func Test_createClusterRBAC(t *testing.T) { roleBinding := &rbacv1.RoleBinding{} serviceAccount := &corev1.ServiceAccount{} - namespacedName := types.NamespacedName{Name: tt.args.obj.Name, Namespace: tt.args.obj.Namespace} + namespacedName := types.NamespacedName{Name: tt.args.cluster.Name, Namespace: tt.args.cluster.Namespace} assert.NoError(t, client.Get(context.TODO(), namespacedName, roleBinding)) assert.NoError(t, client.Get(context.TODO(), namespacedName, serviceAccount)) @@ -65,56 +58,41 @@ func Test_createClusterRBAC(t *testing.T) { } } -func Test_setBootstrapToken(t *testing.T) { - type args struct { - obj *synv1alpha1.Cluster - data *ExecutionContext - } - tests := []struct { - name string - args args - wantErr bool - }{ - { - name: "Set bootstrap token", - args: args{ - obj: &synv1alpha1.Cluster{}, - data: &ExecutionContext{ - Log: zap.New(), - }, +var setBootstrapTokenCases = genericCases{ + "Set bootstrap token": { + args: args{ + cluster: &synv1alpha1.Cluster{}, + data: &ExecutionContext{ + Log: zap.New(), }, }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := setBootstrapToken(tt.args.obj, tt.args.data); got.Err != nil != tt.wantErr { + }, +} + +func Test_setBootstrapToken(t *testing.T) { + for name, tt := range setBootstrapTokenCases { + t.Run(name, func(t *testing.T) { + if got := setBootstrapToken(tt.args.cluster, tt.args.data); got.Err != nil != tt.wantErr { t.Errorf("setBootstrapToken() = had error: %v", got.Err) } }) - assert.NotNil(t, tt.args.obj.Status.BootstrapToken) + assert.NotNil(t, tt.args.cluster.Status.BootstrapToken) } } -func Test_newClusterStatus(t *testing.T) { - type args struct { - cluster *synv1alpha1.Cluster - } - tests := []struct { - name string - args args - wantErr bool - }{ - { - name: "new cluster status", - args: args{ - cluster: &synv1alpha1.Cluster{}, - }, +var clusterStatusCases = genericCases{ + "new cluster status": { + args: args{ + cluster: &synv1alpha1.Cluster{}, }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + }, +} + +func Test_newClusterStatus(t *testing.T) { + for name, tt := range clusterStatusCases { + t.Run(name, func(t *testing.T) { if err := newClusterStatus(tt.args.cluster); (err != nil) != tt.wantErr { t.Errorf("newClusterStatus() error = %v, wantErr %v", err, tt.wantErr) } @@ -125,141 +103,122 @@ func Test_newClusterStatus(t *testing.T) { } } -func Test_setTenantOwner(t *testing.T) { - type args struct { - obj *synv1alpha1.Cluster - tenant *synv1alpha1.Tenant - data *ExecutionContext - } - tests := []struct { - name string - args args - wantErr bool - }{ - { - name: "set tenant owner", - args: args{ - obj: &synv1alpha1.Cluster{ - Spec: synv1alpha1.ClusterSpec{ - TenantRef: corev1.LocalObjectReference{Name: "testTenant"}, - }, +var setTenantOwnerCases = genericCases{ + "set tenant owner": { + args: args{ + cluster: &synv1alpha1.Cluster{ + Spec: synv1alpha1.ClusterSpec{ + TenantRef: corev1.LocalObjectReference{Name: "testTenant"}, }, - tenant: &synv1alpha1.Tenant{ - ObjectMeta: metav1.ObjectMeta{ - Name: "testTenant", - }, + }, + tenant: &synv1alpha1.Tenant{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testTenant", }, - data: &ExecutionContext{}, }, + data: &ExecutionContext{}, }, - { - name: "tenant does not exist", - wantErr: true, - args: args{ - obj: &synv1alpha1.Cluster{ - Spec: synv1alpha1.ClusterSpec{ - TenantRef: corev1.LocalObjectReference{Name: "wrongTenant"}, - }, + }, + "tenant does not exist": { + wantErr: true, + args: args{ + cluster: &synv1alpha1.Cluster{ + Spec: synv1alpha1.ClusterSpec{ + TenantRef: corev1.LocalObjectReference{Name: "wrongTenant"}, }, - tenant: &synv1alpha1.Tenant{ - ObjectMeta: metav1.ObjectMeta{ - Name: "testTenant", - }, + }, + tenant: &synv1alpha1.Tenant{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testTenant", }, - data: &ExecutionContext{}, }, + data: &ExecutionContext{}, }, - } - for _, tt := range tests { + }, +} + +func Test_setTenantOwner(t *testing.T) { + for name, tt := range setTenantOwnerCases { tt.args.data.Client, _ = testSetupClient([]runtime.Object{ - tt.args.obj, + tt.args.cluster, tt.args.tenant, }) - t.Run(tt.name, func(t *testing.T) { - if got := setTenantOwner(tt.args.obj, tt.args.data); (got.Err != nil) != tt.wantErr { + t.Run(name, func(t *testing.T) { + if got := setTenantOwner(tt.args.cluster, tt.args.data); (got.Err != nil) != tt.wantErr { t.Errorf("setTenantOwner() = had error: %v", got.Err) } }) if !tt.wantErr { - assert.NotEmpty(t, tt.args.obj.GetOwnerReferences()) + assert.NotEmpty(t, tt.args.cluster.GetOwnerReferences()) } } } -func Test_applyTenantTemplate(t *testing.T) { - type args struct { - obj *synv1alpha1.Cluster - tenant *synv1alpha1.Tenant - data *ExecutionContext - } - tests := []struct { - name string - args args - wantErr bool - }{ - { - name: "apply tenant template", - args: args{ - data: &ExecutionContext{}, - obj: &synv1alpha1.Cluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: "c-some-test", - }, +var applyTenantTemplateCases = genericCases{ + "apply tenant template": { + args: args{ + data: &ExecutionContext{}, + cluster: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "c-some-test", }, - tenant: &synv1alpha1.Tenant{ - Spec: synv1alpha1.TenantSpec{ - ClusterTemplate: &synv1alpha1.ClusterSpec{ - DisplayName: "test", - GitRepoTemplate: &synv1alpha1.GitRepoTemplate{ - RepoName: "{{ .Name }}", - }, + }, + tenant: &synv1alpha1.Tenant{ + Spec: synv1alpha1.TenantSpec{ + ClusterTemplate: &synv1alpha1.ClusterSpec{ + DisplayName: "test", + GitRepoTemplate: &synv1alpha1.GitRepoTemplate{ + RepoName: "{{ .Name }}", }, }, }, }, }, - { - name: "wrong syntax", - wantErr: true, - args: args{ - data: &ExecutionContext{}, - obj: &synv1alpha1.Cluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: "c-some-test", - }, + }, + "wrong syntax": { + wantErr: true, + args: args{ + data: &ExecutionContext{}, + cluster: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "c-some-test", }, - tenant: &synv1alpha1.Tenant{ - Spec: synv1alpha1.TenantSpec{ - ClusterTemplate: &synv1alpha1.ClusterSpec{ - DisplayName: "test", - GitRepoTemplate: &synv1alpha1.GitRepoTemplate{ - RepoName: "{{ .Name }", - }, + }, + tenant: &synv1alpha1.Tenant{ + Spec: synv1alpha1.TenantSpec{ + ClusterTemplate: &synv1alpha1.ClusterSpec{ + DisplayName: "test", + GitRepoTemplate: &synv1alpha1.GitRepoTemplate{ + RepoName: "{{ .Name }", }, }, }, }, }, - } - for _, tt := range tests { + }, +} + +func Test_applyTenantTemplate(t *testing.T) { + + for name, tt := range applyTenantTemplateCases { tt.args.data.Client, _ = testSetupClient([]runtime.Object{ - tt.args.obj, + tt.args.cluster, tt.args.tenant, }) - t.Run(tt.name, func(t *testing.T) { - if got := applyTenantTemplate(tt.args.obj, tt.args.data); (got.Err != nil) != tt.wantErr { + t.Run(name, func(t *testing.T) { + if got := applyTenantTemplate(tt.args.cluster, tt.args.data); (got.Err != nil) != tt.wantErr { t.Errorf("applyTenantTemplate() = had error: %v", got.Err) } }) if !tt.wantErr { - assert.Equal(t, "c-some-test", tt.args.obj.Spec.GitRepoTemplate.RepoName) + assert.Equal(t, "c-some-test", tt.args.cluster.Spec.GitRepoTemplate.RepoName) } } diff --git a/pkg/pipeline/common_steps_test.go b/pkg/pipeline/common_steps_test.go index b7dffef9..2f5126dd 100644 --- a/pkg/pipeline/common_steps_test.go +++ b/pkg/pipeline/common_steps_test.go @@ -35,30 +35,25 @@ func TestGetDeletionPolicyNonDefault(t *testing.T) { assert.Equal(t, synv1alpha1.RetainPolicy, policy) } -func TestAddTenantLabel(t *testing.T) { - type args struct { - obj *synv1alpha1.Cluster - } - tests := []struct { - name string - args args - }{ - { - name: "add labels", - args: args{ - obj: &synv1alpha1.Cluster{ - Spec: synv1alpha1.ClusterSpec{ - TenantRef: corev1.LocalObjectReference{Name: "test"}, - }, +var addTenantLabelCases = genericCases{ + "add labels": { + args: args{ + cluster: &synv1alpha1.Cluster{ + Spec: synv1alpha1.ClusterSpec{ + TenantRef: corev1.LocalObjectReference{Name: "test"}, }, }, }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - addTenantLabel(tt.args.obj, &ExecutionContext{}) + }, +} + +func TestAddTenantLabel(t *testing.T) { + + for name, tt := range addTenantLabelCases { + t.Run(name, func(t *testing.T) { + addTenantLabel(tt.args.cluster, &ExecutionContext{}) - if tt.args.obj.GetLabels()[apis.LabelNameTenant] != tt.args.obj.Spec.TenantRef.Name { + if tt.args.cluster.GetLabels()[apis.LabelNameTenant] != tt.args.cluster.Spec.TenantRef.Name { t.Error("labels do not match") } @@ -66,69 +61,60 @@ func TestAddTenantLabel(t *testing.T) { } } -func TestHandleDeletion(t *testing.T) { - type args struct { - instance *synv1alpha1.Cluster - finalizerName string - } - tests := []struct { - name string - args args - wantErr bool - }{ - { - name: "Normal deletion", - args: args{ - instance: &synv1alpha1.Cluster{ - ObjectMeta: metav1.ObjectMeta{ - DeletionTimestamp: &metav1.Time{Time: time.Now()}, - Finalizers: []string{ - "test", - }, +var handleDeletionCases = genericCases{ + "Normal deletion": { + args: args{ + cluster: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + DeletionTimestamp: &metav1.Time{Time: time.Now()}, + Finalizers: []string{ + "test", }, }, - finalizerName: "test", }, + finalizerName: "test", }, - { - name: "Deletion protection set", - wantErr: true, - args: args{ - instance: &synv1alpha1.Cluster{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - DeleteProtectionAnnotation: "true", - }, - DeletionTimestamp: &metav1.Time{Time: time.Now()}, - Finalizers: []string{ - "test", - }, + }, + "Deletion protection set": { + wantErr: true, + args: args{ + cluster: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + DeleteProtectionAnnotation: "true", + }, + DeletionTimestamp: &metav1.Time{Time: time.Now()}, + Finalizers: []string{ + "test", }, }, - finalizerName: "test", }, + finalizerName: "test", }, - { - name: "Nonsense annotation value", - wantErr: true, - args: args{ - instance: &synv1alpha1.Cluster{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - DeleteProtectionAnnotation: "trugadse", - }, - DeletionTimestamp: &metav1.Time{Time: time.Now()}, - Finalizers: []string{ - "test", - }, + }, + "Nonsense annotation value": { + wantErr: true, + args: args{ + cluster: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + DeleteProtectionAnnotation: "trugadse", + }, + DeletionTimestamp: &metav1.Time{Time: time.Now()}, + Finalizers: []string{ + "test", }, }, - finalizerName: "test", }, + finalizerName: "test", }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + }, +} + +func TestHandleDeletion(t *testing.T) { + + for name, tt := range handleDeletionCases { + t.Run(name, func(t *testing.T) { client, _ := testSetupClient([]runtime.Object{&synv1alpha1.Cluster{}}) @@ -138,56 +124,56 @@ func TestHandleDeletion(t *testing.T) { FinalizerName: tt.args.finalizerName, } - got := handleDeletion(tt.args.instance, data) + got := handleDeletion(tt.args.cluster, data) if got.Err != nil != tt.wantErr { t.Errorf("HandleDeletion() = had error: %v", got.Err) } want := []string{tt.args.finalizerName} - assert.Equal(t, want, tt.args.instance.GetFinalizers()) + assert.Equal(t, want, tt.args.cluster.GetFinalizers()) }) } } -func TestAddDeletionProtection(t *testing.T) { - type args struct { - instance *synv1alpha1.Cluster - enable string - result string - } - tests := []struct { - name string - args args - }{ - { - name: "Add deletion protection", - args: args{ - instance: &synv1alpha1.Cluster{}, - enable: "true", - result: "true", - }, +type addDeletionProtectionArgs struct { + instance *synv1alpha1.Cluster + enable string + result string +} + +var addDeletionProtectionCases = map[string]struct { + args addDeletionProtectionArgs + wantErr bool +}{ + "Add deletion protection": { + args: addDeletionProtectionArgs{ + instance: &synv1alpha1.Cluster{}, + enable: "true", + result: "true", }, - { - name: "Don't add deletion protection", - args: args{ - instance: &synv1alpha1.Cluster{}, - enable: "false", - result: "", - }, + }, + "Don't add deletion protection": { + args: addDeletionProtectionArgs{ + instance: &synv1alpha1.Cluster{}, + enable: "false", + result: "", }, - { - name: "Invalid setting", - args: args{ - instance: &synv1alpha1.Cluster{}, - enable: "gaga", - result: "true", - }, + }, + "Invalid setting": { + args: addDeletionProtectionArgs{ + instance: &synv1alpha1.Cluster{}, + enable: "gaga", + result: "true", }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + }, +} + +func TestAddDeletionProtection(t *testing.T) { + + for name, tt := range addDeletionProtectionCases { + t.Run(name, func(t *testing.T) { os.Setenv(protectionSettingEnvVar, tt.args.enable) @@ -201,46 +187,41 @@ func TestAddDeletionProtection(t *testing.T) { } } -func Test_checkIfDeleted(t *testing.T) { - type args struct { - obj *synv1alpha1.Tenant - data *ExecutionContext - } - tests := []struct { - name string - args args - wantErr bool - want bool - }{ - { - name: "object deleted", - want: true, - args: args{ - data: &ExecutionContext{}, - obj: &synv1alpha1.Tenant{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - DeletionTimestamp: &metav1.Time{Time: time.Now()}, - }, +var checkIfDeletedCases = map[string]struct { + args args + wantErr bool + want bool +}{ + "object deleted": { + want: true, + args: args{ + data: &ExecutionContext{}, + tenant: &synv1alpha1.Tenant{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + DeletionTimestamp: &metav1.Time{Time: time.Now()}, }, }, }, - { - name: "object not deleted", - want: false, - args: args{ - data: &ExecutionContext{}, - obj: &synv1alpha1.Tenant{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - }, + }, + "object not deleted": { + want: false, + args: args{ + data: &ExecutionContext{}, + tenant: &synv1alpha1.Tenant{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", }, }, }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := checkIfDeleted(tt.args.obj, tt.args.data); (got.Err != nil) != tt.wantErr { + }, +} + +func Test_checkIfDeleted(t *testing.T) { + + for name, tt := range checkIfDeletedCases { + t.Run(name, func(t *testing.T) { + if got := checkIfDeleted(tt.args.tenant, tt.args.data); (got.Err != nil) != tt.wantErr { t.Errorf("checkIfDeleted() = had error %v", got.Err) } @@ -250,54 +231,46 @@ func Test_checkIfDeleted(t *testing.T) { } } -func Test_handleFinalizer(t *testing.T) { - type args struct { - obj *synv1alpha1.Cluster - data *ExecutionContext - } - tests := []struct { - name string - args args - wantErr bool - }{ - { - name: "add finalizers", - args: args{ - data: &ExecutionContext{ - FinalizerName: "test", - }, - obj: &synv1alpha1.Cluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - }, +var handleFinalizerCases = genericCases{ + "add finalizers": { + args: args{ + data: &ExecutionContext{ + FinalizerName: "test", + }, + cluster: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", }, }, }, - { - name: "remove finalizers", - args: args{ - data: &ExecutionContext{ - Deleted: true, - FinalizerName: "test", - }, - obj: &synv1alpha1.Cluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - }, + }, + "remove finalizers": { + args: args{ + data: &ExecutionContext{ + Deleted: true, + FinalizerName: "test", + }, + cluster: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", }, }, }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := handleFinalizer(tt.args.obj, tt.args.data); (got.Err != nil) != tt.wantErr { + }, +} + +func Test_handleFinalizer(t *testing.T) { + + for name, tt := range handleFinalizerCases { + t.Run(name, func(t *testing.T) { + if got := handleFinalizer(tt.args.cluster, tt.args.data); (got.Err != nil) != tt.wantErr { t.Errorf("handleFinalizer() = had error: %v", got.Err) } if tt.args.data.Deleted { - assert.Empty(t, tt.args.obj.GetFinalizers()) + assert.Empty(t, tt.args.cluster.GetFinalizers()) } else { - assert.NotEmpty(t, tt.args.obj.GetFinalizers()) + assert.NotEmpty(t, tt.args.cluster.GetFinalizers()) } }) @@ -314,43 +287,35 @@ func Test_wrapError(t *testing.T) { assert.Nil(t, wrapError("test", nil)) } -func Test_updateObject(t *testing.T) { - type args struct { - obj *synv1alpha1.Tenant - data *ExecutionContext - } - tests := []struct { - name string - args args - wantErr bool - }{ - { - name: "update objects", - args: args{ - data: &ExecutionContext{}, - obj: &synv1alpha1.Tenant{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - }, +var updateObjectCases = genericCases{ + "update objects": { + args: args{ + data: &ExecutionContext{}, + tenant: &synv1alpha1.Tenant{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", }, }, }, - { - name: "update fail", - args: args{ - data: &ExecutionContext{}, - obj: &synv1alpha1.Tenant{}, - }, + }, + "update fail": { + args: args{ + data: &ExecutionContext{}, + tenant: &synv1alpha1.Tenant{}, }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + }, +} + +func Test_updateObject(t *testing.T) { + + for name, tt := range updateObjectCases { + t.Run(name, func(t *testing.T) { tt.args.data.Client, _ = testSetupClient([]runtime.Object{ - tt.args.obj, + tt.args.tenant, }) - if got := updateObject(tt.args.obj, tt.args.data); (got.Err != nil) != tt.wantErr { + if got := updateObject(tt.args.tenant, tt.args.data); (got.Err != nil) != tt.wantErr { t.Errorf("updateObject() = had error: %v", got.Err) } diff --git a/pkg/pipeline/reconcile_types_test.go b/pkg/pipeline/reconcile_types_test.go new file mode 100644 index 00000000..c43b003a --- /dev/null +++ b/pkg/pipeline/reconcile_types_test.go @@ -0,0 +1,16 @@ +package pipeline + +import synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" + +type genericCases map[string]struct { + args args + wantErr bool +} + +type args struct { + cluster *synv1alpha1.Cluster + tenant *synv1alpha1.Tenant + gitRepo synv1alpha1.GitRepo + data *ExecutionContext + finalizerName string +} diff --git a/pkg/pipeline/tenant_steps_test.go b/pkg/pipeline/tenant_steps_test.go index 44fab059..a1dc09bb 100644 --- a/pkg/pipeline/tenant_steps_test.go +++ b/pkg/pipeline/tenant_steps_test.go @@ -11,177 +11,164 @@ import ( "k8s.io/apimachinery/pkg/runtime" ) -func Test_addDefaultClassFile(t *testing.T) { - type args struct { - obj *synv1alpha1.Tenant - data *ExecutionContext - } - tests := []struct { - name string - args args - }{ - { - name: "add default class", - args: args{ - obj: &synv1alpha1.Tenant{}, - data: &ExecutionContext{}, - }, +var addDefaultClassFileCases = genericCases{ + "add default class": { + args: args{ + tenant: &synv1alpha1.Tenant{}, + data: &ExecutionContext{}, }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + }, +} - got := addDefaultClassFile(tt.args.obj, tt.args.data) +func Test_addDefaultClassFile(t *testing.T) { + for name, tt := range addDefaultClassFileCases { + t.Run(name, func(t *testing.T) { + + got := addDefaultClassFile(tt.args.tenant, tt.args.data) assert.NoError(t, got.Err) - assert.Contains(t, tt.args.obj.Spec.GitRepoTemplate.TemplateFiles, "common.yml") - assert.NotEmpty(t, tt.args.obj.Spec.GitRepoTemplate.TemplateFiles) + assert.Contains(t, tt.args.tenant.Spec.GitRepoTemplate.TemplateFiles, "common.yml") + assert.NotEmpty(t, tt.args.tenant.Spec.GitRepoTemplate.TemplateFiles) }) } } -func Test_setGlobalGitRepoURL(t *testing.T) { - type args struct { - obj *synv1alpha1.Tenant - data *ExecutionContext - defaultRepo string - } - tests := []struct { - name string - want string - args args - }{ - { - name: "set global git repo url via env var", - want: "test", - args: args{ - obj: &synv1alpha1.Tenant{}, - defaultRepo: "test", - }, +type setGlobalGitRepoURLArgs struct { + tenant *synv1alpha1.Tenant + defaultRepo string + data *ExecutionContext +} + +var setGlobalGitRepoURLCases = map[string]struct { + want string + args setGlobalGitRepoURLArgs +}{ + "set global git repo url via env var": { + want: "test", + args: setGlobalGitRepoURLArgs{ + tenant: &synv1alpha1.Tenant{}, + defaultRepo: "test", }, - { - name: "don't override", - want: "foo", - args: args{ - obj: &synv1alpha1.Tenant{ - Spec: synv1alpha1.TenantSpec{ - GlobalGitRepoURL: "foo", - }, + }, + "don't override": { + want: "foo", + args: setGlobalGitRepoURLArgs{ + tenant: &synv1alpha1.Tenant{ + Spec: synv1alpha1.TenantSpec{ + GlobalGitRepoURL: "foo", }, - defaultRepo: "test", }, + defaultRepo: "test", }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + }, +} + +func Test_setGlobalGitRepoURL(t *testing.T) { + + for name, tt := range setGlobalGitRepoURLCases { + t.Run(name, func(t *testing.T) { os.Setenv(DefaultGlobalGitRepoURL, tt.args.defaultRepo) - got := setGlobalGitRepoURL(tt.args.obj, tt.args.data) + got := setGlobalGitRepoURL(tt.args.tenant, tt.args.data) assert.NoError(t, got.Err) - assert.Equal(t, tt.want, tt.args.obj.Spec.GlobalGitRepoURL) + assert.Equal(t, tt.want, tt.args.tenant.Spec.GlobalGitRepoURL) }) } } -func Test_updateTenantGitRepo(t *testing.T) { - type args struct { - obj *synv1alpha1.Tenant - cluster *synv1alpha1.Cluster - data *ExecutionContext - } - tests := []struct { - name string - args args - want *synv1alpha1.GitRepoTemplate - }{ - { - name: "fetch git repos", - want: &synv1alpha1.GitRepoTemplate{ - TemplateFiles: map[string]string{ - "testCluster.yml": "classes:\n- testCluster.common\n", +var updateTenantGitRepoCases = map[string]struct { + want *synv1alpha1.GitRepoTemplate + args args +}{ + "fetch git repos": { + want: &synv1alpha1.GitRepoTemplate{ + TemplateFiles: map[string]string{ + "testCluster.yml": "classes:\n- testCluster.common\n", + }, + }, + args: args{ + data: &ExecutionContext{}, + tenant: &synv1alpha1.Tenant{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testTenant", }, }, - args: args{ - data: &ExecutionContext{}, - obj: &synv1alpha1.Tenant{ - ObjectMeta: metav1.ObjectMeta{ - Name: "testTenant", + cluster: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testCluster", + Labels: map[string]string{ + apis.LabelNameTenant: "testTenant", }, }, - cluster: &synv1alpha1.Cluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: "testCluster", - Labels: map[string]string{ - apis.LabelNameTenant: "testTenant", - }, - }, - Spec: synv1alpha1.ClusterSpec{ - GitRepoTemplate: &synv1alpha1.GitRepoTemplate{ - TemplateFiles: map[string]string{ - "testCluster.yml": "classes:\n- testCluster.common\n", - }, + Spec: synv1alpha1.ClusterSpec{ + GitRepoTemplate: &synv1alpha1.GitRepoTemplate{ + TemplateFiles: map[string]string{ + "testCluster.yml": "classes:\n- testCluster.common\n", }, }, }, }, }, - { - name: "remove files", - want: &synv1alpha1.GitRepoTemplate{ - TemplateFiles: map[string]string{ - "testCluster.yml": "classes:\n- testCluster.common\n", - "oldFile.yml": "{delete}", - }, + }, + "remove files": { + want: &synv1alpha1.GitRepoTemplate{ + TemplateFiles: map[string]string{ + "testCluster.yml": "classes:\n- testCluster.common\n", + "oldFile.yml": "{delete}", }, - args: args{ - data: &ExecutionContext{}, - obj: &synv1alpha1.Tenant{ - ObjectMeta: metav1.ObjectMeta{ - Name: "testTenant", - }, - Spec: synv1alpha1.TenantSpec{ - GitRepoTemplate: &synv1alpha1.GitRepoTemplate{ - TemplateFiles: map[string]string{ - "oldFile.yml": "not important", - }, + }, + args: args{ + data: &ExecutionContext{}, + tenant: &synv1alpha1.Tenant{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testTenant", + }, + Spec: synv1alpha1.TenantSpec{ + GitRepoTemplate: &synv1alpha1.GitRepoTemplate{ + TemplateFiles: map[string]string{ + "oldFile.yml": "not important", }, }, }, - cluster: &synv1alpha1.Cluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: "testCluster", - Labels: map[string]string{ - apis.LabelNameTenant: "testTenant", - }, + }, + cluster: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testCluster", + Labels: map[string]string{ + apis.LabelNameTenant: "testTenant", }, - Spec: synv1alpha1.ClusterSpec{ - GitRepoTemplate: &synv1alpha1.GitRepoTemplate{ - TemplateFiles: map[string]string{ - "testCluster.yml": "classes:\n- testCluster.common\n", - }, + }, + Spec: synv1alpha1.ClusterSpec{ + GitRepoTemplate: &synv1alpha1.GitRepoTemplate{ + TemplateFiles: map[string]string{ + "testCluster.yml": "classes:\n- testCluster.common\n", }, }, }, }, }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + }, +} + +func Test_updateTenantGitRepo(t *testing.T) { + + for name, tt := range updateTenantGitRepoCases { + t.Run(name, func(t *testing.T) { tt.args.data.Client, _ = testSetupClient([]runtime.Object{ tt.args.cluster, - tt.args.obj, + tt.args.tenant, &synv1alpha1.ClusterList{}, }) - got := updateTenantGitRepo(tt.args.obj, tt.args.data) + got := updateTenantGitRepo(tt.args.tenant, tt.args.data) assert.NoError(t, got.Err) - assert.Equal(t, tt.want, tt.args.obj.GetGitTemplate()) + assert.Equal(t, tt.want, tt.args.tenant.GetGitTemplate()) }) } diff --git a/pkg/pipeline/vault_test.go b/pkg/pipeline/vault_test.go index 0aa38727..088781a8 100644 --- a/pkg/pipeline/vault_test.go +++ b/pkg/pipeline/vault_test.go @@ -1,6 +1,7 @@ package pipeline import ( + "os" "testing" synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" @@ -23,51 +24,47 @@ func (m *testMockClient) SetDeletionPolicy(deletionPolicy synv1alpha1.DeletionPo m.deletionPolicy = deletionPolicy } -func Test_getVaultClient(t *testing.T) { - type args struct { - obj *synv1alpha1.Cluster - data *ExecutionContext - } - tests := []struct { - name string - args args - want synv1alpha1.DeletionPolicy - wantErr bool - }{ - { - name: "without specific deletion policy", - want: getDefaultDeletionPolicy(), - args: args{ - obj: &synv1alpha1.Cluster{}, - data: &ExecutionContext{ - Log: zap.New(), - }, +var getVaultCases = map[string]struct { + want synv1alpha1.DeletionPolicy + args args +}{ + "without specific deletion policy": { + want: getDefaultDeletionPolicy(), + args: args{ + cluster: &synv1alpha1.Cluster{}, + data: &ExecutionContext{ + Log: zap.New(), }, }, - { - name: "specific deletion policy", - want: synv1alpha1.DeletePolicy, - args: args{ - obj: &synv1alpha1.Cluster{ - Spec: synv1alpha1.ClusterSpec{ - DeletionPolicy: synv1alpha1.DeletePolicy, - }, - }, - data: &ExecutionContext{ - Log: zap.New(), + }, + "specific deletion policy": { + want: synv1alpha1.DeletePolicy, + args: args{ + cluster: &synv1alpha1.Cluster{ + Spec: synv1alpha1.ClusterSpec{ + DeletionPolicy: synv1alpha1.DeletePolicy, }, }, + data: &ExecutionContext{ + Log: zap.New(), + }, }, - } + }, +} + +func Test_getVaultClient(t *testing.T) { + + // ensure that it isn't set to anything from previous tests + os.Unsetenv("DEFAULT_DELETION_POLICY") mockClient := &testMockClient{} vault.SetCustomClient(mockClient) - for _, tt := range tests { + for name, tt := range getVaultCases { - t.Run(tt.name, func(t *testing.T) { - _, err := getVaultClient(tt.args.obj, tt.args.data) + t.Run(name, func(t *testing.T) { + _, err := getVaultClient(tt.args.cluster, tt.args.data) assert.NoError(t, err) assert.Equal(t, tt.want, mockClient.deletionPolicy) @@ -76,57 +73,53 @@ func Test_getVaultClient(t *testing.T) { } } -func Test_handleVaultDeletion(t *testing.T) { - type args struct { - obj *synv1alpha1.Cluster - data *ExecutionContext - } - tests := []struct { - name string - args args - want synv1alpha1.DeletionPolicy - }{ - { - name: "noop", - want: getDefaultDeletionPolicy(), - args: args{ - obj: &synv1alpha1.Cluster{ - Spec: synv1alpha1.ClusterSpec{ - DeletionPolicy: getDefaultDeletionPolicy(), - }, +var handleVaultDeletionCases = map[string]struct { + want synv1alpha1.DeletionPolicy + args args +}{ + "noop": { + want: getDefaultDeletionPolicy(), + args: args{ + cluster: &synv1alpha1.Cluster{ + Spec: synv1alpha1.ClusterSpec{ + DeletionPolicy: getDefaultDeletionPolicy(), }, - data: &ExecutionContext{}, }, + data: &ExecutionContext{}, }, - { - name: "archive", - want: synv1alpha1.ArchivePolicy, - args: args{ - obj: &synv1alpha1.Cluster{ - Spec: synv1alpha1.ClusterSpec{ - DeletionPolicy: synv1alpha1.ArchivePolicy, - }, - }, - data: &ExecutionContext{ - Deleted: true, + }, + "archive": { + want: synv1alpha1.ArchivePolicy, + args: args{ + cluster: &synv1alpha1.Cluster{ + Spec: synv1alpha1.ClusterSpec{ + DeletionPolicy: synv1alpha1.ArchivePolicy, }, }, + data: &ExecutionContext{ + Deleted: true, + }, }, - { - name: "delete", - want: synv1alpha1.DeletePolicy, - args: args{ - obj: &synv1alpha1.Cluster{ - Spec: synv1alpha1.ClusterSpec{ - DeletionPolicy: synv1alpha1.DeletePolicy, - }, - }, - data: &ExecutionContext{ - Deleted: true, + }, + "delete": { + want: synv1alpha1.DeletePolicy, + args: args{ + cluster: &synv1alpha1.Cluster{ + Spec: synv1alpha1.ClusterSpec{ + DeletionPolicy: synv1alpha1.DeletePolicy, }, }, + data: &ExecutionContext{ + Deleted: true, + }, }, - } + }, +} + +func Test_handleVaultDeletion(t *testing.T) { + + // ensure that it isn't set to anything from previous tests + os.Unsetenv("DEFAULT_DELETION_POLICY") mockClient := &testMockClient{ deletionPolicy: getDefaultDeletionPolicy(), @@ -134,14 +127,14 @@ func Test_handleVaultDeletion(t *testing.T) { vault.SetCustomClient(mockClient) - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + for name, tt := range handleVaultDeletionCases { + t.Run(name, func(t *testing.T) { tt.args.data.Client, _ = testSetupClient([]runtime.Object{ - tt.args.obj, + tt.args.cluster, }) - got := handleVaultDeletion(tt.args.obj, tt.args.data) + got := handleVaultDeletion(tt.args.cluster, tt.args.data) assert.NoError(t, got.Err) assert.Equal(t, tt.want, mockClient.deletionPolicy) }) From 5d5468004279cf4ddeb3eb1e82fe44fad7563a3d Mon Sep 17 00:00:00 2001 From: Simon Beck Date: Fri, 23 Oct 2020 13:26:13 +0200 Subject: [PATCH 16/23] Fix assert scopes --- pkg/pipeline/cluster_steps_test.go | 45 +++++++++++++++++------------- 1 file changed, 25 insertions(+), 20 deletions(-) diff --git a/pkg/pipeline/cluster_steps_test.go b/pkg/pipeline/cluster_steps_test.go index 53589d62..3c861b5b 100644 --- a/pkg/pipeline/cluster_steps_test.go +++ b/pkg/pipeline/cluster_steps_test.go @@ -42,18 +42,19 @@ func Test_createClusterRBAC(t *testing.T) { if got := createClusterRBAC(tt.args.cluster, tt.args.data); got.Err != nil != tt.wantErr { t.Errorf("createClusterRBAC() = had error: %v", got.Err) } - }) - roleBinding := &rbacv1.RoleBinding{} - serviceAccount := &corev1.ServiceAccount{} + roleBinding := &rbacv1.RoleBinding{} + serviceAccount := &corev1.ServiceAccount{} + + namespacedName := types.NamespacedName{Name: tt.args.cluster.Name, Namespace: tt.args.cluster.Namespace} - namespacedName := types.NamespacedName{Name: tt.args.cluster.Name, Namespace: tt.args.cluster.Namespace} + assert.NoError(t, client.Get(context.TODO(), namespacedName, roleBinding)) + assert.NoError(t, client.Get(context.TODO(), namespacedName, serviceAccount)) - assert.NoError(t, client.Get(context.TODO(), namespacedName, roleBinding)) - assert.NoError(t, client.Get(context.TODO(), namespacedName, serviceAccount)) + assert.Equal(t, serviceAccount.Name, roleBinding.Subjects[len(roleBinding.Subjects)-1].Name) + assert.Equal(t, serviceAccount.Namespace, roleBinding.Subjects[len(roleBinding.Subjects)-1].Namespace) - assert.Equal(t, serviceAccount.Name, roleBinding.Subjects[len(roleBinding.Subjects)-1].Name) - assert.Equal(t, serviceAccount.Namespace, roleBinding.Subjects[len(roleBinding.Subjects)-1].Namespace) + }) } } @@ -75,9 +76,10 @@ func Test_setBootstrapToken(t *testing.T) { if got := setBootstrapToken(tt.args.cluster, tt.args.data); got.Err != nil != tt.wantErr { t.Errorf("setBootstrapToken() = had error: %v", got.Err) } - }) - assert.NotNil(t, tt.args.cluster.Status.BootstrapToken) + assert.NotNil(t, tt.args.cluster.Status.BootstrapToken) + + }) } } @@ -96,9 +98,10 @@ func Test_newClusterStatus(t *testing.T) { if err := newClusterStatus(tt.args.cluster); (err != nil) != tt.wantErr { t.Errorf("newClusterStatus() error = %v, wantErr %v", err, tt.wantErr) } - }) - assert.NotNil(t, tt.args.cluster.Status.BootstrapToken) + assert.NotNil(t, tt.args.cluster.Status.BootstrapToken) + + }) } } @@ -149,11 +152,12 @@ func Test_setTenantOwner(t *testing.T) { if got := setTenantOwner(tt.args.cluster, tt.args.data); (got.Err != nil) != tt.wantErr { t.Errorf("setTenantOwner() = had error: %v", got.Err) } - }) - if !tt.wantErr { - assert.NotEmpty(t, tt.args.cluster.GetOwnerReferences()) - } + if !tt.wantErr { + assert.NotEmpty(t, tt.args.cluster.GetOwnerReferences()) + } + + }) } } @@ -215,11 +219,12 @@ func Test_applyTenantTemplate(t *testing.T) { if got := applyTenantTemplate(tt.args.cluster, tt.args.data); (got.Err != nil) != tt.wantErr { t.Errorf("applyTenantTemplate() = had error: %v", got.Err) } - }) - if !tt.wantErr { - assert.Equal(t, "c-some-test", tt.args.cluster.Spec.GitRepoTemplate.RepoName) - } + if !tt.wantErr { + assert.Equal(t, "c-some-test", tt.args.cluster.Spec.GitRepoTemplate.RepoName) + } + + }) } } From 6405e5e15e9e636c13c938220a5dff619a4a4e16 Mon Sep 17 00:00:00 2001 From: Simon Beck Date: Fri, 23 Oct 2020 14:52:57 +0200 Subject: [PATCH 17/23] Implement PR suggestions --- pkg/controller/cluster/cluster_reconcile_test.go | 5 +++++ pkg/controller/gitrepo/gitrepo_controller.go | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/pkg/controller/cluster/cluster_reconcile_test.go b/pkg/controller/cluster/cluster_reconcile_test.go index 73f10675..a106e907 100644 --- a/pkg/controller/cluster/cluster_reconcile_test.go +++ b/pkg/controller/cluster/cluster_reconcile_test.go @@ -242,6 +242,10 @@ func TestReconcileCluster_Reconcile(t *testing.T) { t.Errorf("Reconcile() got = %v, want %v", got, tt.want) } + // BootstrapToken is now only populated after the second reconcile. + _, err = r.Reconcile(req) + assert.NoError(t, err) + gitRepoNamespacedName := types.NamespacedName{ Name: tt.fields.objName, Namespace: tt.fields.objNamespace, @@ -258,6 +262,7 @@ func TestReconcileCluster_Reconcile(t *testing.T) { assert.Equal(t, tt.fields.tenantName, newCluster.Labels[apis.LabelNameTenant]) + assert.NotNil(t, newCluster.Status.BootstrapToken) assert.NotEmpty(t, newCluster.Status.BootstrapToken.Token) sa := &corev1.ServiceAccount{} diff --git a/pkg/controller/gitrepo/gitrepo_controller.go b/pkg/controller/gitrepo/gitrepo_controller.go index a0608983..d504ae74 100644 --- a/pkg/controller/gitrepo/gitrepo_controller.go +++ b/pkg/controller/gitrepo/gitrepo_controller.go @@ -34,7 +34,7 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { return err } - // Watch for changes to primary resource Tenant + // Watch for changes to primary resource GitRepo err = c.Watch(&source.Kind{Type: &synv1alpha1.GitRepo{}}, &handler.EnqueueRequestForObject{}) if err != nil { return err From 4e0bd2d92b69337047323f6630db418d794b48d8 Mon Sep 17 00:00:00 2001 From: Simon Beck Date: Mon, 26 Oct 2020 08:05:59 +0100 Subject: [PATCH 18/23] Trim excessive whitespaces --- pkg/pipeline/cluster.go | 1 - pkg/pipeline/cluster_steps.go | 6 ------ pkg/pipeline/cluster_steps_test.go | 13 ------------- pkg/pipeline/common.go | 2 -- pkg/pipeline/common_steps.go | 1 - pkg/pipeline/common_steps_test.go | 14 -------------- pkg/pipeline/git.go | 2 -- pkg/pipeline/git_test.go | 4 ---- pkg/pipeline/pipelines.go | 4 ---- pkg/pipeline/tenant.go | 2 -- pkg/pipeline/tenant_steps.go | 2 -- pkg/pipeline/tenant_steps_test.go | 2 -- pkg/pipeline/vault.go | 1 - pkg/pipeline/vault_test.go | 2 -- 14 files changed, 56 deletions(-) diff --git a/pkg/pipeline/cluster.go b/pkg/pipeline/cluster.go index f4766a9a..c274ee8a 100644 --- a/pkg/pipeline/cluster.go +++ b/pkg/pipeline/cluster.go @@ -7,7 +7,6 @@ const ( ) func clusterSpecificSteps(obj PipelineObject, data *ExecutionContext) ExecutionResult { - steps := []Step{ {Name: "create cluster RBAC", F: createClusterRBAC}, {Name: "deletion check", F: checkIfDeleted}, diff --git a/pkg/pipeline/cluster_steps.go b/pkg/pipeline/cluster_steps.go index 8bbe4445..3d8c8060 100644 --- a/pkg/pipeline/cluster_steps.go +++ b/pkg/pipeline/cluster_steps.go @@ -54,7 +54,6 @@ func createClusterRBAC(obj PipelineObject, data *ExecutionContext) ExecutionResu } func setBootstrapToken(obj PipelineObject, data *ExecutionContext) ExecutionResult { - instance, ok := obj.(*synv1alpha1.Cluster) if !ok { return ExecutionResult{Err: fmt.Errorf("%s is not a cluster object", obj.GetObjectMeta().GetName())} @@ -73,12 +72,10 @@ func setBootstrapToken(obj PipelineObject, data *ExecutionContext) ExecutionResu } return ExecutionResult{} - } //newClusterStatus will create a default lifetime of 30 minutes if it wasn't set in the object. func newClusterStatus(cluster *synv1alpha1.Cluster) error { - parseTime := "30m" if cluster.Spec.TokenLifeTime != "" { parseTime = cluster.Spec.TokenLifeTime @@ -114,9 +111,7 @@ func generateToken() (string, error) { } func setTenantOwner(obj PipelineObject, data *ExecutionContext) ExecutionResult { - tenant := &synv1alpha1.Tenant{} - tenantName := types.NamespacedName{Name: obj.GetTenantRef().Name, Namespace: obj.GetObjectMeta().GetNamespace()} err := data.Client.Get(context.TODO(), tenantName, tenant) @@ -135,7 +130,6 @@ func applyTenantTemplate(obj PipelineObject, data *ExecutionContext) ExecutionRe nsName := types.NamespacedName{Name: obj.GetTenantRef().Name, Namespace: obj.GetObjectMeta().GetNamespace()} tenant := &synv1alpha1.Tenant{} - if err := data.Client.Get(context.TODO(), nsName, tenant); err != nil { return ExecutionResult{Err: fmt.Errorf("Couldn't find tenant: %w", err)} } diff --git a/pkg/pipeline/cluster_steps_test.go b/pkg/pipeline/cluster_steps_test.go index 3c861b5b..5f94268a 100644 --- a/pkg/pipeline/cluster_steps_test.go +++ b/pkg/pipeline/cluster_steps_test.go @@ -29,9 +29,7 @@ var rbacCases = genericCases{ } func Test_createClusterRBAC(t *testing.T) { - for name, tt := range rbacCases { - client, _ := testSetupClient([]runtime.Object{ tt.args.cluster, }) @@ -55,7 +53,6 @@ func Test_createClusterRBAC(t *testing.T) { assert.Equal(t, serviceAccount.Namespace, roleBinding.Subjects[len(roleBinding.Subjects)-1].Namespace) }) - } } @@ -78,9 +75,7 @@ func Test_setBootstrapToken(t *testing.T) { } assert.NotNil(t, tt.args.cluster.Status.BootstrapToken) - }) - } } @@ -102,7 +97,6 @@ func Test_newClusterStatus(t *testing.T) { assert.NotNil(t, tt.args.cluster.Status.BootstrapToken) }) - } } @@ -142,7 +136,6 @@ var setTenantOwnerCases = genericCases{ func Test_setTenantOwner(t *testing.T) { for name, tt := range setTenantOwnerCases { - tt.args.data.Client, _ = testSetupClient([]runtime.Object{ tt.args.cluster, tt.args.tenant, @@ -156,9 +149,7 @@ func Test_setTenantOwner(t *testing.T) { if !tt.wantErr { assert.NotEmpty(t, tt.args.cluster.GetOwnerReferences()) } - }) - } } @@ -207,9 +198,7 @@ var applyTenantTemplateCases = genericCases{ } func Test_applyTenantTemplate(t *testing.T) { - for name, tt := range applyTenantTemplateCases { - tt.args.data.Client, _ = testSetupClient([]runtime.Object{ tt.args.cluster, tt.args.tenant, @@ -223,8 +212,6 @@ func Test_applyTenantTemplate(t *testing.T) { if !tt.wantErr { assert.Equal(t, "c-some-test", tt.args.cluster.Spec.GitRepoTemplate.RepoName) } - }) - } } diff --git a/pkg/pipeline/common.go b/pkg/pipeline/common.go index 6215e6f9..ccded261 100644 --- a/pkg/pipeline/common.go +++ b/pkg/pipeline/common.go @@ -9,7 +9,6 @@ const ( ) func common(obj PipelineObject, data *ExecutionContext) ExecutionResult { - steps := []Step{ {Name: "deletion", F: handleDeletion}, {Name: "add deletion protection", F: addDeletionProtection}, @@ -20,5 +19,4 @@ func common(obj PipelineObject, data *ExecutionContext) ExecutionResult { err := RunPipeline(obj, data, steps) return ExecutionResult{Err: err} - } diff --git a/pkg/pipeline/common_steps.go b/pkg/pipeline/common_steps.go index 06cca5ad..7551929a 100644 --- a/pkg/pipeline/common_steps.go +++ b/pkg/pipeline/common_steps.go @@ -23,7 +23,6 @@ func getDefaultDeletionPolicy() synv1alpha1.DeletionPolicy { } func addDeletionProtection(instance PipelineObject, data *ExecutionContext) ExecutionResult { - if data.Deleted { return ExecutionResult{} } diff --git a/pkg/pipeline/common_steps_test.go b/pkg/pipeline/common_steps_test.go index 2f5126dd..f27242ac 100644 --- a/pkg/pipeline/common_steps_test.go +++ b/pkg/pipeline/common_steps_test.go @@ -48,7 +48,6 @@ var addTenantLabelCases = genericCases{ } func TestAddTenantLabel(t *testing.T) { - for name, tt := range addTenantLabelCases { t.Run(name, func(t *testing.T) { addTenantLabel(tt.args.cluster, &ExecutionContext{}) @@ -56,7 +55,6 @@ func TestAddTenantLabel(t *testing.T) { if tt.args.cluster.GetLabels()[apis.LabelNameTenant] != tt.args.cluster.Spec.TenantRef.Name { t.Error("labels do not match") } - }) } } @@ -112,10 +110,8 @@ var handleDeletionCases = genericCases{ } func TestHandleDeletion(t *testing.T) { - for name, tt := range handleDeletionCases { t.Run(name, func(t *testing.T) { - client, _ := testSetupClient([]runtime.Object{&synv1alpha1.Cluster{}}) data := &ExecutionContext{ @@ -132,7 +128,6 @@ func TestHandleDeletion(t *testing.T) { want := []string{tt.args.finalizerName} assert.Equal(t, want, tt.args.cluster.GetFinalizers()) - }) } } @@ -171,10 +166,8 @@ var addDeletionProtectionCases = map[string]struct { } func TestAddDeletionProtection(t *testing.T) { - for name, tt := range addDeletionProtectionCases { t.Run(name, func(t *testing.T) { - os.Setenv(protectionSettingEnvVar, tt.args.enable) addDeletionProtection(tt.args.instance, &ExecutionContext{}) @@ -218,7 +211,6 @@ var checkIfDeletedCases = map[string]struct { } func Test_checkIfDeleted(t *testing.T) { - for name, tt := range checkIfDeletedCases { t.Run(name, func(t *testing.T) { if got := checkIfDeleted(tt.args.tenant, tt.args.data); (got.Err != nil) != tt.wantErr { @@ -226,7 +218,6 @@ func Test_checkIfDeleted(t *testing.T) { } assert.Equal(t, tt.want, tt.args.data.Deleted) - }) } } @@ -260,7 +251,6 @@ var handleFinalizerCases = genericCases{ } func Test_handleFinalizer(t *testing.T) { - for name, tt := range handleFinalizerCases { t.Run(name, func(t *testing.T) { if got := handleFinalizer(tt.args.cluster, tt.args.data); (got.Err != nil) != tt.wantErr { @@ -272,7 +262,6 @@ func Test_handleFinalizer(t *testing.T) { } else { assert.NotEmpty(t, tt.args.cluster.GetFinalizers()) } - }) } } @@ -307,10 +296,8 @@ var updateObjectCases = genericCases{ } func Test_updateObject(t *testing.T) { - for name, tt := range updateObjectCases { t.Run(name, func(t *testing.T) { - tt.args.data.Client, _ = testSetupClient([]runtime.Object{ tt.args.tenant, }) @@ -318,7 +305,6 @@ func Test_updateObject(t *testing.T) { if got := updateObject(tt.args.tenant, tt.args.data); (got.Err != nil) != tt.wantErr { t.Errorf("updateObject() = had error: %v", got.Err) } - }) } } diff --git a/pkg/pipeline/git.go b/pkg/pipeline/git.go index 4ee81e92..fe8b14cd 100644 --- a/pkg/pipeline/git.go +++ b/pkg/pipeline/git.go @@ -13,7 +13,6 @@ import ( ) func gitRepoSpecificSteps(obj PipelineObject, data *ExecutionContext) ExecutionResult { - instance, ok := obj.(*synv1alpha1.GitRepo) if !ok { return ExecutionResult{Err: fmt.Errorf("object is not a GitRepository")} @@ -78,7 +77,6 @@ func gitRepoSpecificSteps(obj PipelineObject, data *ExecutionContext) ExecutionR // createGitRepo will create the gitRepo object if it doesn't already exist. func createGitRepo(obj PipelineObject, data *ExecutionContext) ExecutionResult { - template := obj.GetGitTemplate() if template == nil { diff --git a/pkg/pipeline/git_test.go b/pkg/pipeline/git_test.go index 8aa7abfa..da696070 100644 --- a/pkg/pipeline/git_test.go +++ b/pkg/pipeline/git_test.go @@ -156,7 +156,6 @@ func Test_fetchGitRepoTemplate(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - rtObj := tt.args.templateObj.(runtime.Object) tt.args.data.Client, _ = testSetupClient([]runtime.Object{ @@ -176,7 +175,6 @@ func Test_fetchGitRepoTemplate(t *testing.T) { assert.NoError(t, tt.args.data.Client.Update(context.TODO(), rtObj)) assert.NoError(t, fetchGitRepoTemplate(tt.args.obj, tt.args.data)) assert.Equal(t, tt.args.templateObj.GetGitTemplate(), &tt.args.obj.Spec.GitRepoTemplate) - }) } } @@ -335,7 +333,6 @@ func Test_setGitRepoURLAndHostKeys(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - tt.args.data.Client, _ = testSetupClient([]runtime.Object{ tt.args.obj, tt.args.gitRepo, @@ -347,7 +344,6 @@ func Test_setGitRepoURLAndHostKeys(t *testing.T) { assert.Equal(t, tt.args.gitRepo.Status.URL, tt.args.obj.Spec.GitRepoURL) assert.Equal(t, tt.args.gitRepo.Status.HostKeys, tt.args.obj.Spec.GitHostKeys) - }) } } diff --git a/pkg/pipeline/pipelines.go b/pkg/pipeline/pipelines.go index 02111b02..4e306144 100644 --- a/pkg/pipeline/pipelines.go +++ b/pkg/pipeline/pipelines.go @@ -9,7 +9,6 @@ type Step struct { } func ReconcileTenant(obj PipelineObject, data *ExecutionContext) error { - steps := []Step{ {Name: "tenant specific steps", F: tenantSpecificSteps}, {Name: "create git repo", F: createGitRepo}, @@ -21,7 +20,6 @@ func ReconcileTenant(obj PipelineObject, data *ExecutionContext) error { } func ReconcileCluster(obj PipelineObject, data *ExecutionContext) error { - steps := []Step{ {Name: "cluster specific steps", F: clusterSpecificSteps}, {Name: "create git repo", F: createGitRepo}, @@ -34,7 +32,6 @@ func ReconcileCluster(obj PipelineObject, data *ExecutionContext) error { } func ReconcileGitRep(obj PipelineObject, data *ExecutionContext) error { - steps := []Step{ {Name: "deletion check", F: checkIfDeleted}, {Name: "git repo specific steps", F: gitRepoSpecificSteps}, @@ -46,7 +43,6 @@ func ReconcileGitRep(obj PipelineObject, data *ExecutionContext) error { } func RunPipeline(obj PipelineObject, data *ExecutionContext, steps []Step) error { - for _, step := range steps { if r := step.F(obj, data); resultNotOK(r) { return wrapError(step.Name, r.Err) diff --git a/pkg/pipeline/tenant.go b/pkg/pipeline/tenant.go index 3879c15f..87e0575c 100644 --- a/pkg/pipeline/tenant.go +++ b/pkg/pipeline/tenant.go @@ -7,7 +7,6 @@ const ( ) func tenantSpecificSteps(obj PipelineObject, data *ExecutionContext) ExecutionResult { - steps := []Step{ {Name: "add default class file", F: addDefaultClassFile}, {Name: "uptade tenant git repo", F: updateTenantGitRepo}, @@ -17,5 +16,4 @@ func tenantSpecificSteps(obj PipelineObject, data *ExecutionContext) ExecutionRe err := RunPipeline(obj, data, steps) return ExecutionResult{Err: err} - } diff --git a/pkg/pipeline/tenant_steps.go b/pkg/pipeline/tenant_steps.go index b0b89c04..ff10c6b8 100644 --- a/pkg/pipeline/tenant_steps.go +++ b/pkg/pipeline/tenant_steps.go @@ -24,7 +24,6 @@ func addDefaultClassFile(obj PipelineObject, data *ExecutionContext) ExecutionRe } func updateTenantGitRepo(obj PipelineObject, data *ExecutionContext) ExecutionResult { - tenantCR, ok := obj.(*synv1alpha1.Tenant) if !ok { return ExecutionResult{Err: fmt.Errorf("object is not a tenant")} @@ -73,7 +72,6 @@ func updateTenantGitRepo(obj PipelineObject, data *ExecutionContext) ExecutionRe } func setGlobalGitRepoURL(obj PipelineObject, data *ExecutionContext) ExecutionResult { - instance, ok := obj.(*synv1alpha1.Tenant) if !ok { return ExecutionResult{Err: fmt.Errorf("object is not a tenant")} diff --git a/pkg/pipeline/tenant_steps_test.go b/pkg/pipeline/tenant_steps_test.go index a1dc09bb..4e083a8e 100644 --- a/pkg/pipeline/tenant_steps_test.go +++ b/pkg/pipeline/tenant_steps_test.go @@ -65,7 +65,6 @@ var setGlobalGitRepoURLCases = map[string]struct { } func Test_setGlobalGitRepoURL(t *testing.T) { - for name, tt := range setGlobalGitRepoURLCases { t.Run(name, func(t *testing.T) { @@ -155,7 +154,6 @@ var updateTenantGitRepoCases = map[string]struct { } func Test_updateTenantGitRepo(t *testing.T) { - for name, tt := range updateTenantGitRepoCases { t.Run(name, func(t *testing.T) { diff --git a/pkg/pipeline/vault.go b/pkg/pipeline/vault.go index ae322041..6677eb58 100644 --- a/pkg/pipeline/vault.go +++ b/pkg/pipeline/vault.go @@ -80,7 +80,6 @@ func GetServiceAccountToken(instance metav1.Object, data *ExecutionContext) (str } func handleVaultDeletion(obj PipelineObject, data *ExecutionContext) ExecutionResult { - if strings.ToLower(os.Getenv("SKIP_VAULT_SETUP")) == "true" { return ExecutionResult{} } diff --git a/pkg/pipeline/vault_test.go b/pkg/pipeline/vault_test.go index 088781a8..90b4394c 100644 --- a/pkg/pipeline/vault_test.go +++ b/pkg/pipeline/vault_test.go @@ -53,7 +53,6 @@ var getVaultCases = map[string]struct { } func Test_getVaultClient(t *testing.T) { - // ensure that it isn't set to anything from previous tests os.Unsetenv("DEFAULT_DELETION_POLICY") @@ -117,7 +116,6 @@ var handleVaultDeletionCases = map[string]struct { } func Test_handleVaultDeletion(t *testing.T) { - // ensure that it isn't set to anything from previous tests os.Unsetenv("DEFAULT_DELETION_POLICY") From 17c97810ae0c61f4bce53372def25b29bfced8b3 Mon Sep 17 00:00:00 2001 From: Simon Beck Date: Mon, 26 Oct 2020 08:06:24 +0100 Subject: [PATCH 19/23] Remove ineffective assignement --- pkg/pipeline/common_steps.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/pipeline/common_steps.go b/pkg/pipeline/common_steps.go index 7551929a..826030d1 100644 --- a/pkg/pipeline/common_steps.go +++ b/pkg/pipeline/common_steps.go @@ -121,8 +121,6 @@ func handleDeletion(obj PipelineObject, data *ExecutionContext) ExecutionResult // Assume true if it can't be parsed if err != nil { protected = true - // We need to reset the error again, so we don't trigger any unwanted side effects... - err = nil } } else { protected = false From a6d311cf3f0e676a69f3328a36cd0156acd6c35c Mon Sep 17 00:00:00 2001 From: Simon Beck Date: Mon, 26 Oct 2020 08:06:42 +0100 Subject: [PATCH 20/23] Remove unused code --- pkg/pipeline/reconcile_types_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/pipeline/reconcile_types_test.go b/pkg/pipeline/reconcile_types_test.go index c43b003a..4baf62e8 100644 --- a/pkg/pipeline/reconcile_types_test.go +++ b/pkg/pipeline/reconcile_types_test.go @@ -10,7 +10,6 @@ type genericCases map[string]struct { type args struct { cluster *synv1alpha1.Cluster tenant *synv1alpha1.Tenant - gitRepo synv1alpha1.GitRepo data *ExecutionContext finalizerName string } From 19fcb0fa9cef3f974e483a51fb9cda1a65e12823 Mon Sep 17 00:00:00 2001 From: Simon Beck Date: Mon, 26 Oct 2020 08:07:05 +0100 Subject: [PATCH 21/23] Remove unused code --- pkg/pipeline/vault_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/pipeline/vault_test.go b/pkg/pipeline/vault_test.go index 90b4394c..5133d691 100644 --- a/pkg/pipeline/vault_test.go +++ b/pkg/pipeline/vault_test.go @@ -12,7 +12,6 @@ import ( ) type testMockClient struct { - secrets []vault.VaultSecret deletionPolicy synv1alpha1.DeletionPolicy } From 5ccb0cfcba616f3e5e3016e3c0e5968782127573 Mon Sep 17 00:00:00 2001 From: Simon Beck Date: Tue, 27 Oct 2020 10:28:40 +0100 Subject: [PATCH 22/23] Align git repo unit tests with the rest --- pkg/pipeline/git_test.go | 409 ++++++++++++++++++--------------------- 1 file changed, 191 insertions(+), 218 deletions(-) diff --git a/pkg/pipeline/git_test.go b/pkg/pipeline/git_test.go index da696070..d4036c38 100644 --- a/pkg/pipeline/git_test.go +++ b/pkg/pipeline/git_test.go @@ -9,172 +9,163 @@ import ( synv1alpha1 "github.com/projectsyn/lieutenant-operator/pkg/apis/syn/v1alpha1" "github.com/projectsyn/lieutenant-operator/pkg/git/manager" "github.com/stretchr/testify/assert" - v1 "k8s.io/api/core/v1" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" ) -func TestCreateOrUpdateGitRepo(t *testing.T) { - type args struct { - obj *synv1alpha1.Cluster - template *synv1alpha1.GitRepoTemplate - tenantRef v1.LocalObjectReference - } - tests := []struct { - name string - args args - wantErr bool - }{ - { - name: "create git repo", - args: args{ - obj: &synv1alpha1.Cluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - Namespace: "test", - }, - }, - template: &synv1alpha1.GitRepoTemplate{ - APISecretRef: v1.SecretReference{Name: "testSecret"}, - DeployKeys: nil, - Path: "testPath", - RepoName: "testRepo", - }, - tenantRef: v1.LocalObjectReference{ - Name: "testTenant", +type gitRepoArgs struct { + repo *synv1alpha1.GitRepo + cluster *synv1alpha1.Cluster + template *synv1alpha1.GitRepoTemplate + tenantRef corev1.LocalObjectReference + templateObj PipelineObject + data *ExecutionContext +} + +var createOrUpdateGitRepoCases = map[string]struct { + args gitRepoArgs + wantErr bool +}{ + "create git repo": { + args: gitRepoArgs{ + cluster: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "test", }, }, + template: &synv1alpha1.GitRepoTemplate{ + APISecretRef: corev1.SecretReference{Name: "testSecret"}, + DeployKeys: nil, + Path: "testPath", + RepoName: "testRepo", + }, + tenantRef: corev1.LocalObjectReference{ + Name: "testTenant", + }, }, - { - name: "empty template", - args: args{ - obj: &synv1alpha1.Cluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - Namespace: "test", - }, - }, - template: nil, - tenantRef: v1.LocalObjectReference{ - Name: "testTenant", + }, + "empty template": { + args: gitRepoArgs{ + cluster: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "test", }, }, + template: nil, + tenantRef: corev1.LocalObjectReference{ + Name: "testTenant", + }, }, - } - for _, tt := range tests { + }, +} +func TestCreateOrUpdateGitRepo(t *testing.T) { + for name, tt := range createOrUpdateGitRepoCases { objs := []runtime.Object{ &synv1alpha1.GitRepo{}, } cl, _ := testSetupClient(objs) - tt.args.obj.Spec.GitRepoTemplate = tt.args.template - tt.args.obj.Spec.TenantRef = tt.args.tenantRef + tt.args.cluster.Spec.GitRepoTemplate = tt.args.template + tt.args.cluster.Spec.TenantRef = tt.args.tenantRef - t.Run(tt.name, func(t *testing.T) { - if res := createGitRepo(tt.args.obj, &ExecutionContext{Client: cl}); (res.Err != nil) != tt.wantErr { + t.Run(name, func(t *testing.T) { + if res := createGitRepo(tt.args.cluster, &ExecutionContext{Client: cl}); (res.Err != nil) != tt.wantErr { t.Errorf("CreateGitRepo() error = %v, wantErr %v", res.Err, tt.wantErr) } - }) - - if tt.args.template == nil { - continue - } - - namespacedName := types.NamespacedName{ - Name: tt.args.obj.GetName(), - Namespace: tt.args.obj.GetNamespace(), - } - checkRepo := &synv1alpha1.GitRepo{} - assert.NoError(t, cl.Get(context.TODO(), namespacedName, checkRepo)) - assert.Equal(t, tt.args.template, &checkRepo.Spec.GitRepoTemplate) + if tt.args.template != nil { + namespacedName := types.NamespacedName{ + Name: tt.args.cluster.GetName(), + Namespace: tt.args.cluster.GetNamespace(), + } + checkRepo := &synv1alpha1.GitRepo{} + assert.NoError(t, cl.Get(context.TODO(), namespacedName, checkRepo)) + assert.Equal(t, tt.args.template, &checkRepo.Spec.GitRepoTemplate) + } + }) } } -func Test_fetchGitRepoTemplate(t *testing.T) { - type args struct { - obj *synv1alpha1.GitRepo - templateObj PipelineObject - data *ExecutionContext - } - tests := []struct { - name string - args args - wantErr bool - }{ - { - name: "fetch tenant changes", - args: args{ - data: &ExecutionContext{}, - obj: &synv1alpha1.GitRepo{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - }, +var fetchGitRepoTemplateCases = map[string]struct { + args gitRepoArgs + wantErr bool +}{ + "fetch tenant changes": { + args: gitRepoArgs{ + data: &ExecutionContext{}, + repo: &synv1alpha1.GitRepo{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", }, - templateObj: &synv1alpha1.Tenant{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - }, - Spec: synv1alpha1.TenantSpec{ - GitRepoTemplate: &synv1alpha1.GitRepoTemplate{ - RepoName: "Test Repo", - RepoType: synv1alpha1.AutoRepoType, - Path: "test", - }, + }, + templateObj: &synv1alpha1.Tenant{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + Spec: synv1alpha1.TenantSpec{ + GitRepoTemplate: &synv1alpha1.GitRepoTemplate{ + RepoName: "Test Repo", + RepoType: synv1alpha1.AutoRepoType, + Path: "test", }, }, }, }, - { - name: "fetch cluster changes", - args: args{ - data: &ExecutionContext{}, - obj: &synv1alpha1.GitRepo{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - }, + }, + "fetch cluster changes": { + args: gitRepoArgs{ + data: &ExecutionContext{}, + repo: &synv1alpha1.GitRepo{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", }, - templateObj: &synv1alpha1.Cluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - }, - Spec: synv1alpha1.ClusterSpec{ - GitRepoTemplate: &synv1alpha1.GitRepoTemplate{ - RepoName: "Test Repo", - RepoType: synv1alpha1.AutoRepoType, - Path: "test", - }, + }, + templateObj: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + Spec: synv1alpha1.ClusterSpec{ + GitRepoTemplate: &synv1alpha1.GitRepoTemplate{ + RepoName: "Test Repo", + RepoType: synv1alpha1.AutoRepoType, + Path: "test", }, }, }, }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + }, +} + +func Test_fetchGitRepoTemplate(t *testing.T) { + for name, tt := range fetchGitRepoTemplateCases { + t.Run(name, func(t *testing.T) { rtObj := tt.args.templateObj.(runtime.Object) tt.args.data.Client, _ = testSetupClient([]runtime.Object{ - tt.args.obj, + tt.args.repo, &synv1alpha1.Tenant{}, &synv1alpha1.Cluster{}, rtObj, }) - if err := fetchGitRepoTemplate(tt.args.obj, tt.args.data); (err != nil) != tt.wantErr { + if err := fetchGitRepoTemplate(tt.args.repo, tt.args.data); (err != nil) != tt.wantErr { t.Errorf("fetchGitRepoTemplate() error = %v, wantErr %v", err, tt.wantErr) } - assert.Equal(t, tt.args.templateObj.GetGitTemplate(), &tt.args.obj.Spec.GitRepoTemplate) + assert.Equal(t, tt.args.templateObj.GetGitTemplate(), &tt.args.repo.Spec.GitRepoTemplate) tt.args.templateObj.GetGitTemplate().RepoName = "another test" rtObj = tt.args.templateObj.(runtime.Object) assert.NoError(t, tt.args.data.Client.Update(context.TODO(), rtObj)) - assert.NoError(t, fetchGitRepoTemplate(tt.args.obj, tt.args.data)) - assert.Equal(t, tt.args.templateObj.GetGitTemplate(), &tt.args.obj.Spec.GitRepoTemplate) + assert.NoError(t, fetchGitRepoTemplate(tt.args.repo, tt.args.data)) + assert.Equal(t, tt.args.templateObj.GetGitTemplate(), &tt.args.repo.Spec.GitRepoTemplate) }) } } @@ -197,72 +188,61 @@ func (r *repoMock) Connect() error { return nil } func (r *repoMock) Remove() error { return nil } func (r *repoMock) CommitTemplateFiles() error { return nil } -func Test_repoExists(t *testing.T) { - type args struct { - repo manager.Repo - } - tests := []struct { - name string - args args - want bool - }{ - { - name: "repo exists", - want: true, - args: args{ - repo: &repoMock{}, - }, +var repoExistsCases = map[string]struct { + args manager.Repo + want bool +}{ + "repo exists": { + want: true, + args: &repoMock{}, + }, + "repo doesn't exist": { + want: false, + args: &repoMock{ + failRead: true, }, - { - name: "repo doesn't exist", - want: false, - args: args{ - repo: &repoMock{ - failRead: true, - }, - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := repoExists(tt.args.repo); got != tt.want { + }} + +func Test_repoExists(t *testing.T) { + for name, tt := range repoExistsCases { + t.Run(name, func(t *testing.T) { + if got := repoExists(tt.args); got != tt.want { t.Errorf("repoExists() = %v, want %v", got, tt.want) } }) } } -func Test_handleRepoError(t *testing.T) { - type args struct { - err error - instance *synv1alpha1.GitRepo - repo manager.Repo - fail bool - } - tests := []struct { - name string - args args - }{ - { - name: "add error", - args: args{ - err: fmt.Errorf("lol nope"), - instance: &synv1alpha1.GitRepo{}, - repo: &repoMock{}, - }, +type handleRepoErrorArgs struct { + err error + instance *synv1alpha1.GitRepo + repo manager.Repo + fail bool +} + +var handleRepoErrorCases = map[string]struct { + args handleRepoErrorArgs +}{ + "add error": { + args: handleRepoErrorArgs{ + err: fmt.Errorf("lol nope"), + instance: &synv1alpha1.GitRepo{}, + repo: &repoMock{}, }, - { - name: "add error failure", - args: args{ - err: fmt.Errorf("lol nope"), - instance: &synv1alpha1.GitRepo{}, - repo: &repoMock{}, - fail: true, - }, + }, + "add error failure": { + args: handleRepoErrorArgs{ + err: fmt.Errorf("lol nope"), + instance: &synv1alpha1.GitRepo{}, + repo: &repoMock{}, + fail: true, }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + }, +} + +func Test_handleRepoError(t *testing.T) { + for name, tt := range handleRepoErrorCases { + t.Run(name, func(t *testing.T) { var client client.Client @@ -285,65 +265,58 @@ func Test_handleRepoError(t *testing.T) { } } -func Test_setGitRepoURLAndHostKeys(t *testing.T) { - type args struct { - obj *synv1alpha1.Cluster - gitRepo *synv1alpha1.GitRepo - data *ExecutionContext - } - tests := []struct { - name string - args args - wantErr bool - }{ - { - name: "set url and keys", - wantErr: false, - args: args{ - obj: &synv1alpha1.Cluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - }, +var setGitRepoURLAndHostKeysCases = map[string]struct { + args gitRepoArgs + wantErr bool +}{ + "set url and keys": { + wantErr: false, + args: gitRepoArgs{ + cluster: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", }, - data: &ExecutionContext{}, - gitRepo: &synv1alpha1.GitRepo{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - }, - Status: synv1alpha1.GitRepoStatus{ - URL: "someURL", - HostKeys: "someKeys", - }, + }, + data: &ExecutionContext{}, + repo: &synv1alpha1.GitRepo{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + Status: synv1alpha1.GitRepoStatus{ + URL: "someURL", + HostKeys: "someKeys", }, }, }, - { - name: "set url and keys not found", - wantErr: false, - args: args{ - obj: &synv1alpha1.Cluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: "invalid", - }, + }, + "set url and keys not found": { + wantErr: false, + args: gitRepoArgs{ + cluster: &synv1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "invalid", }, - data: &ExecutionContext{}, - gitRepo: &synv1alpha1.GitRepo{}, }, + data: &ExecutionContext{}, + repo: &synv1alpha1.GitRepo{}, }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + }, +} + +func Test_setGitRepoURLAndHostKeys(t *testing.T) { + for name, tt := range setGitRepoURLAndHostKeysCases { + t.Run(name, func(t *testing.T) { tt.args.data.Client, _ = testSetupClient([]runtime.Object{ - tt.args.obj, - tt.args.gitRepo, + tt.args.cluster, + tt.args.repo, }) - if got := setGitRepoURLAndHostKeys(tt.args.obj, tt.args.data); (got.Err != nil) != tt.wantErr { + if got := setGitRepoURLAndHostKeys(tt.args.cluster, tt.args.data); (got.Err != nil) != tt.wantErr { t.Errorf("setGitRepoURLAndHostKeys() = had error: %v", got.Err) } - assert.Equal(t, tt.args.gitRepo.Status.URL, tt.args.obj.Spec.GitRepoURL) - assert.Equal(t, tt.args.gitRepo.Status.HostKeys, tt.args.obj.Spec.GitHostKeys) + assert.Equal(t, tt.args.repo.Status.URL, tt.args.cluster.Spec.GitRepoURL) + assert.Equal(t, tt.args.repo.Status.HostKeys, tt.args.cluster.Spec.GitHostKeys) }) } } From 8586189f8f911649bbbe4acbe49c774d8827c4a0 Mon Sep 17 00:00:00 2001 From: Simon Beck Date: Tue, 27 Oct 2020 10:32:58 +0100 Subject: [PATCH 23/23] Remove `resultNotOK` and `wrapError` They are no longer necessary. --- pkg/pipeline/common_steps.go | 11 ----------- pkg/pipeline/common_steps_test.go | 11 ----------- pkg/pipeline/pipelines.go | 9 +++++++-- 3 files changed, 7 insertions(+), 24 deletions(-) diff --git a/pkg/pipeline/common_steps.go b/pkg/pipeline/common_steps.go index 826030d1..d8fdc006 100644 --- a/pkg/pipeline/common_steps.go +++ b/pkg/pipeline/common_steps.go @@ -92,17 +92,6 @@ func updateObject(obj PipelineObject, data *ExecutionContext) ExecutionResult { return ExecutionResult{Abort: true, Err: err} } -func wrapError(name string, err error) error { - if err == nil { - return nil - } - return fmt.Errorf("step %s failed: %w", name, err) -} - -func resultNotOK(result ExecutionResult) bool { - return result.Abort || result.Err != nil -} - // handleDeletion will handle the finalizers if the object was deleted. // It will only trigger if data.Deleted is true. func handleDeletion(obj PipelineObject, data *ExecutionContext) ExecutionResult { diff --git a/pkg/pipeline/common_steps_test.go b/pkg/pipeline/common_steps_test.go index f27242ac..d989d66b 100644 --- a/pkg/pipeline/common_steps_test.go +++ b/pkg/pipeline/common_steps_test.go @@ -1,7 +1,6 @@ package pipeline import ( - "fmt" "os" "testing" "time" @@ -266,16 +265,6 @@ func Test_handleFinalizer(t *testing.T) { } } -func Test_resultNotOK(t *testing.T) { - assert.True(t, resultNotOK(ExecutionResult{Err: fmt.Errorf("test")})) - assert.False(t, resultNotOK(ExecutionResult{})) -} - -func Test_wrapError(t *testing.T) { - assert.Equal(t, "step test failed: test", wrapError("test", fmt.Errorf("test")).Error()) - assert.Nil(t, wrapError("test", nil)) -} - var updateObjectCases = genericCases{ "update objects": { args: args{ diff --git a/pkg/pipeline/pipelines.go b/pkg/pipeline/pipelines.go index 4e306144..5993eb04 100644 --- a/pkg/pipeline/pipelines.go +++ b/pkg/pipeline/pipelines.go @@ -1,5 +1,7 @@ package pipeline +import "fmt" + // Function defines the general form of a pipeline function. type Function func(PipelineObject, *ExecutionContext) ExecutionResult @@ -44,8 +46,11 @@ func ReconcileGitRep(obj PipelineObject, data *ExecutionContext) error { func RunPipeline(obj PipelineObject, data *ExecutionContext, steps []Step) error { for _, step := range steps { - if r := step.F(obj, data); resultNotOK(r) { - return wrapError(step.Name, r.Err) + if r := step.F(obj, data); r.Abort || r.Err != nil { + if r.Err == nil { + return nil + } + return fmt.Errorf("step %s failed: %w", step.Name, r.Err) } }