fix: Sops secrets name length limitation issue (#2)

* Use secret name to find existing secrets * Some bugfixes after testing * Revert go.mod and go.sum
isindir · Nov 8, 2019 · 8736ba1 · 8736ba1
1 parent c2a37ff
commit 8736ba1
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 115 deletions.
diff --git a/pkg/controller/sopssecret/sopssecret_controller.go b/pkg/controller/sopssecret/sopssecret_controller.go
@@ -10,9 +10,9 @@ import (
 	isindirv1alpha1 "github.com/isindir/sops-secrets-operator/pkg/apis/isindir/v1alpha1"
 
 	corev1 "k8s.io/api/core/v1"
+	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -118,57 +118,20 @@ func (r *ReconcileSopsSecret) Reconcile(request reconcile.Request) (reconcile.Re
 		return reconcile.Result{}, err
 	}
 
-	// Garbage collection logic - for templates removed from SopsSecret
-	// Get List of all kube secrets for this sops secret in this namespace.
-	existingSecretList := &corev1.SecretList{}
-	labelSelector := labels.SelectorFromSet(labelsForSecret(instanceEncrypted))
-	listOps := &client.ListOptions{
-		Namespace:     request.Namespace,
-		LabelSelector: labelSelector,
-	}
-	// Obtain List of secrets - filter by labels
-	if err = r.client.List(
-		context.TODO(),
-		listOps,
-		existingSecretList,
-	); err != nil {
-		return reconcile.Result{}, err
-	}
-
-	// Garbage collection loop - iterate through all fetched secrets and check
-	// for matching any template by name, if not - delete secret
-	for _, existingKubeSecret := range existingSecretList.Items {
-		found := false
-		for _, secretTemplateValue := range instance.Spec.SecretsTemplate {
-			if secretTemplateValue.Name == existingKubeSecret.Name {
-				found = true
-				break
-			}
-		}
-		if !found {
-			err = r.client.Delete(
-				context.TODO(),
-				&existingKubeSecret,
-				client.GracePeriodSeconds(0),
-			)
-			if err != nil {
-				return reconcile.Result{}, err
-			}
-		}
-	}
+	// Garbage collection logic - using the fact that owned objects automatically get cleaned up by k8s
 
 	reqLogger.Info("Enetring template data loop.")
 	for _, secretTemplateValue := range instance.Spec.SecretsTemplate {
 		// Define a new secret object
-		secret, err := newSecretForCR(instance, &secretTemplateValue)
+		newSecret, err := newSecretForCR(instance, &secretTemplateValue)
 		if err != nil {
 			return reconcile.Result{}, err
 		}
 
 		// Set SopsSecret instance as the owner and controller
 		if err := controllerutil.SetControllerReference(
 			instance,
-			secret,
+			newSecret,
 			r.scheme,
 		); err != nil {
 			return reconcile.Result{}, err
@@ -179,47 +142,40 @@ func (r *ReconcileSopsSecret) Reconcile(request reconcile.Request) (reconcile.Re
 		err = r.client.Get(
 			context.TODO(),
 			types.NamespacedName{
-				Name:      secret.Name,
-				Namespace: secret.Namespace,
+				Name:      newSecret.Name,
+				Namespace: newSecret.Namespace,
 			},
 			foundSecret,
 		)
-		if err != nil && errors.IsNotFound(err) {
-			reqLogger.Info(
-				"Creating a new Secret",
-				"Secret.Namespace",
-				secret.Namespace,
-				"Secret.Name",
-				secret.Name,
-			)
-			err = r.client.Create(context.TODO(), secret)
-			if err != nil {
-				return reconcile.Result{}, err
-			}
-
-			// Secret created successfully - don't requeue
-			return reconcile.Result{}, nil
-		} else if err != nil {
+		if errors.IsNotFound(err) {
+			reqLogger.Info("Creating a new Secret")
+			err = r.client.Create(context.TODO(), newSecret)
+			foundSecret = newSecret.DeepCopy()
+		}
+		if err != nil {
 			return reconcile.Result{}, err
-		} else {
-
-			// Secret already exists - enforce update
-			reqLogger.Info(
-				"Secret already exists: Update",
-				"Secret.Namespace",
-				foundSecret.Namespace,
-				"Secret.Name",
-				foundSecret.Name,
-			)
-			foundSecret.Labels = secret.Labels
-			foundSecret.Annotations = secret.Annotations
-			foundSecret.StringData = secret.StringData
+		}
+
+		if !metav1.IsControlledBy(foundSecret, instance) {
+			return reconcile.Result{}, fmt.Errorf("Secret isn't currently owned by sops-secrets-operator")
+		}
+
+		origSecret := foundSecret
+		foundSecret = foundSecret.DeepCopy()
 
+		foundSecret.StringData = newSecret.StringData
+		foundSecret.Type = newSecret.Type
+		foundSecret.ObjectMeta.Annotations = newSecret.ObjectMeta.Annotations
+		foundSecret.ObjectMeta.Labels = newSecret.ObjectMeta.Labels
+
+		if !apiequality.Semantic.DeepEqual(origSecret, foundSecret) {
+			reqLogger.Info("Secret already exists and needs updated")
 			if err = r.client.Update(context.TODO(), foundSecret); err != nil {
 				return reconcile.Result{}, err
 			}
 		}
 	}
+
 	return reconcile.Result{}, nil
 }
 
@@ -256,12 +212,7 @@ func newSecretForCR(
 	cr *isindirv1alpha1.SopsSecret,
 	secretTpl *isindirv1alpha1.SopsSecretTemplate,
 ) (*corev1.Secret, error) {
-
-	// Construct labels for the secret
-	// TODO: instead of using label for GC - find the way to query secrets by
-	// parent, than this label is not needed anymore
-	// see: https://kubernetes.io/docs/concepts/workloads/controllers/garbage-collection/
-	labels := labelsForSecret(cr)
+	labels := make(map[string]string)
 	for key, value := range secretTpl.Labels {
 		labels[key] = value
 	}
@@ -315,14 +266,6 @@ func sanitizeLabel(str string) string {
 	return replacer.Replace(str)
 }
 
-func labelsForSecret(cr *isindirv1alpha1.SopsSecret) map[string]string {
-	return map[string]string{
-		"sopssecret": sanitizeLabel(
-			fmt.Sprintf("%s.%s.%s", cr.Kind, cr.APIVersion, cr.Name),
-		),
-	}
-}
-
 // Data is a helper that takes encrypted data and a format string,
 // decrypts the data and returns its cleartext in an []byte.
 // The format string can be `json`, `yaml`, `dotenv` or `binary`.

diff --git a/pkg/controller/sopssecret/sopssecret_controller_test.go b/pkg/controller/sopssecret/sopssecret_controller_test.go
@@ -24,31 +24,6 @@ func TestSanitizeLabel(t *testing.T) {
 	}
 }
 
-// test labelsForSecret()
-func TestLabelsForSecret(t *testing.T) {
-	cr := &isindirv1alpha1.SopsSecret{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "jenkins-secrets",
-			Namespace: "jenkins",
-		},
-		TypeMeta: metav1.TypeMeta{
-			Kind:       "SopsSecret",
-			APIVersion: "isindir.github.com/v1alpha1",
-		},
-	}
-	newLabels := labelsForSecret(cr)
-	if len(newLabels) != 1 {
-		t.Errorf("labelsForSecret() returned map of size = %d; want 1", len(newLabels))
-	}
-	val, ok := newLabels["sopssecret"]
-	if !ok {
-		t.Errorf("labelsForSecret() returned map does not contain \"sopssecret\" key")
-	}
-	if val != "SopsSecret.isindir.github.com.v1alpha1.jenkins-secrets" {
-		t.Errorf("labelsForSecret() returned incorrect value for key \"sopssecret\" %s; want SopsSecret.isindir.github.com.v1alpha1.jenkins-secrets", val)
-	}
-}
-
 // test newSecretForCR()
 func TestNewSecretForCR(t *testing.T) {
 	cr := &isindirv1alpha1.SopsSecret{
@@ -90,12 +65,9 @@ func TestNewSecretForCR(t *testing.T) {
 	if secret.Namespace != "jenkins" {
 		t.Errorf("newSecretForCR() returned incorrect secret namespace %s; want \"jenkins\"", secret.Namespace)
 	}
-	if len(secret.Labels) != 3 {
+	if len(secret.Labels) != 2 {
 		t.Errorf("newSecretForCR() returned secret with label list of size = %d; want 3", len(secret.Labels))
 	}
-	if secret.Labels["sopssecret"] != "SopsSecret.isindir.github.com.v1alpha1.jenkins-secrets" {
-		t.Errorf("newSecretForCR() returned incorrect secret label value for key \"sopssecret\" %s; want SopsSecret.isindir.github.com.v1alpha1.jenkins-secrets", secret.Labels["sopssecret"])
-	}
 	if len(secret.Annotations) != 2 {
 		t.Errorf("newSecretForCR() returned secret with Annotations list of size = %d; want 2", len(secret.Annotations))
 	}