Fix quarks secret SecretLabels & SecretAnnotations update bug (#59)

* Add qsec secret meta controller

* Modify the trigger logic for qsec controller

* Add integration test for qsec updates

integration/quarks_secret_copy_test.go

@@ -3,6 +3,7 @@ package integration_test
 import (
 	"bytes"
 	"fmt"
+	"reflect"
 	"time"
 
 	. "github.com/onsi/ginkgo"
@@ -171,6 +172,59 @@ var _ = Describe("QuarksCopies", func() {
 			}))
 			Expect(secret.StringData["password"]).NotTo(BeNil())
 		})
+
+		It("should copy the updated generated secret to the copy namespace when qsec is found", func() {
+			By("Checking the quarkssecret status")
+			checkStatus()
+			checkCopyStatus()
+
+			secret, err := env.CollectSecret(env.Namespace, sourceSecret)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(secret.Labels).To(Equal(map[string]string{
+				"quarks.cloudfoundry.org/secret-kind": "generated",
+			}))
+			oldSecretData := secret.Data["password"]
+
+			By("Add labels to `SecretLabels` spec of quarkssecret")
+			qs, err := env.GetQuarksSecret(env.Namespace, qsecName)
+			Expect(err).NotTo(HaveOccurred())
+			qs.Spec.SecretLabels = map[string]string{
+				"LabelKeyv2": "LabelValuev2",
+			}
+			qs, err = env.UpdateQuarksSecret(env.Namespace, qs)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(qs).NotTo(Equal(nil))
+
+			Eventually(func() bool {
+				secret, err = env.CollectSecret(env.Namespace, qs.Spec.SecretName)
+				Expect(err).NotTo(HaveOccurred())
+				return reflect.DeepEqual(secret.Labels, map[string]string{
+					"LabelKeyv2":                          "LabelValuev2",
+					"quarks.cloudfoundry.org/secret-kind": "generated",
+				})
+			}, 10*time.Second).Should(Equal(true))
+			secret, err = env.CollectSecret(env.Namespace, qs.Spec.SecretName)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(secret.Data["password"]).To(MatchRegexp("^\\w{64}$"))
+			Expect(oldSecretData).To(Equal(secret.Data["password"]))
+
+			By("Checking the quarkssecret status")
+			checkCopyStatus()
+
+			By("Check for the updated copy secret")
+			Eventually(func() bool {
+				secret, err = env.CollectSecret(qs.Spec.Copies[0].Namespace, qs.Spec.Copies[0].Name)
+				Expect(err).NotTo(HaveOccurred())
+				return reflect.DeepEqual(secret.Labels, map[string]string{
+					"LabelKeyv2":                          "LabelValuev2",
+					"quarks.cloudfoundry.org/secret-kind": "generated",
+				})
+			}, 10*time.Second).Should(Equal(true))
+
+			secret, err = env.CollectSecret(qs.Spec.Copies[0].Namespace, qs.Spec.Copies[0].Name)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(oldSecretData).To(Equal(secret.Data["password"]))
+		})
 	})
 
 	Context("the secret is generated by operator for qsec copies", func() {

integration/quarks_secret_test.go

@@ -1,9 +1,6 @@
 package integration_test
 
 import (
-	"strconv"
-	"time"
-
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
@@ -184,73 +181,4 @@ var _ = Describe("QuarksSecret", func() {
 			deleteQuarksSecret()
 		})
 	})
-
-	When("updating the qsec", func() {
-		var oldSecret *corev1.Secret
-
-		BeforeEach(func() {
-			qs = env.DefaultQuarksSecret(qsName)
-			secretName = qs.Spec.SecretName
-		})
-
-		JustBeforeEach(func() {
-			var err error
-			oldSecret, err = env.CollectSecret(env.Namespace, secretName)
-			Expect(err).NotTo(HaveOccurred())
-
-			By("Getting the current version")
-			qs, err = env.GetQuarksSecret(env.Namespace, qs.Name)
-			Expect(err).NotTo(HaveOccurred())
-		})
-
-		incResourceVersion := func(v string) string {
-			n, err := strconv.Atoi(v)
-			Expect(err).NotTo(HaveOccurred())
-			n++
-			return strconv.Itoa(n)
-		}
-
-		When("updating the spec", func() {
-			JustBeforeEach(func() {
-				var err error
-				By("Updating the quarks secret")
-				qs.Spec.SecretLabels = map[string]string{"sec": "label"}
-				qs, err = env.UpdateQuarksSecret(env.Namespace, qs)
-				Expect(err).NotTo(HaveOccurred())
-
-				old := incResourceVersion(qs.GetResourceVersion())
-				err = env.WaitForQuarksSecretChange(env.Namespace, qs.Name, 10*time.Second, func(qsec qsv1a1.QuarksSecret) bool {
-					return old < qsec.GetResourceVersion()
-				})
-				Expect(err).NotTo(HaveOccurred())
-
-			})
-
-			It("updates the secret", func() {
-				secret, err := env.CollectSecret(env.Namespace, secretName)
-				Expect(err).NotTo(HaveOccurred())
-
-				Expect(secret.Data["password"]).NotTo(Equal(oldSecret.Data["password"]))
-				qs, err = env.GetQuarksSecret(env.Namespace, qs.Name)
-				Expect(err).NotTo(HaveOccurred())
-			})
-		})
-
-		When("updating a label", func() {
-			JustBeforeEach(func() {
-				var err error
-				qs.Labels = map[string]string{"qsec": "label"}
-				qs, err = env.UpdateQuarksSecret(env.Namespace, qs)
-				Expect(err).NotTo(HaveOccurred())
-			})
-
-			It("does not update the secret", func() {
-				old := incResourceVersion(qs.GetResourceVersion())
-				err := env.WaitForQuarksSecretChange(env.Namespace, qs.Name, 10*time.Second, func(qsec qsv1a1.QuarksSecret) bool {
-					return old < qsec.GetResourceVersion()
-				})
-				Expect(err).To(HaveOccurred(), "check for update should fail")
-			})
-		})
-	})
 })

integration/quarks_secret_update_test.go

@@ -0,0 +1,129 @@
+package integration_test
+
+import (
+	"reflect"
+	"strconv"
+	"time"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	corev1 "k8s.io/api/core/v1"
+
+	qsv1a1 "code.cloudfoundry.org/quarks-secret/pkg/kube/apis/quarkssecret/v1alpha1"
+	"code.cloudfoundry.org/quarks-utils/testing/machine"
+)
+
+var _ = Describe("QuarksSecret", func() {
+	var (
+		secretName string
+		qs         qsv1a1.QuarksSecret
+		qsName     = "qsec-test"
+		tearDowns  []machine.TearDownFunc
+	)
+
+	JustBeforeEach(func() {
+		By("Creating the quarks secret", func() {
+			qsec, tearDown, err := env.CreateQuarksSecret(env.Namespace, qs)
+			qs = *qsec
+			Expect(err).NotTo(HaveOccurred())
+			Expect(qs).NotTo(Equal(nil))
+			tearDowns = append(tearDowns, tearDown)
+		})
+	})
+
+	AfterEach(func() {
+		Expect(env.TearDownAll(tearDowns)).To(Succeed())
+	})
+
+	When("updating the qsec", func() {
+		var oldSecret *corev1.Secret
+
+		BeforeEach(func() {
+			qs = env.DefaultQuarksSecretWithLabels(qsName)
+			secretName = qs.Spec.SecretName
+		})
+
+		JustBeforeEach(func() {
+			var err error
+			oldSecret, err = env.CollectSecret(env.Namespace, secretName)
+			Expect(err).NotTo(HaveOccurred())
+
+			By("Getting the current version")
+			qs, err = env.GetQuarksSecret(env.Namespace, qs.Name)
+			Expect(err).NotTo(HaveOccurred())
+		})
+
+		incResourceVersion := func(v string) string {
+			n, err := strconv.Atoi(v)
+			Expect(err).NotTo(HaveOccurred())
+			n++
+			return strconv.Itoa(n)
+		}
+
+		When("updating a label", func() {
+			JustBeforeEach(func() {
+				var err error
+				qs.Labels = map[string]string{"qsec": "label"}
+				qs, err = env.UpdateQuarksSecret(env.Namespace, qs)
+				Expect(err).NotTo(HaveOccurred())
+			})
+
+			It("does not update the secret", func() {
+				old := incResourceVersion(qs.GetResourceVersion())
+				err := env.WaitForQuarksSecretChange(env.Namespace, qs.Name, 10*time.Second, func(qsec qsv1a1.QuarksSecret) bool {
+					return old < qsec.GetResourceVersion()
+				})
+				Expect(err).To(HaveOccurred(), "check for update should fail")
+			})
+		})
+
+		When("updating `SecretLabels` key", func() {
+			JustBeforeEach(func() {
+				var err error
+				qs.Spec.SecretLabels["LabelKeyv2"] = "LabelValuev2"
+				qs, err = env.UpdateQuarksSecret(env.Namespace, qs)
+				Expect(err).NotTo(HaveOccurred())
+			})
+
+			It("does not rotate/regenerate secret, when quarks secret's `SecretLabels` key is changed", func() {
+				By("Check for the updated secret")
+				secret, err := env.CollectSecret(env.Namespace, secretName)
+				Expect(err).NotTo(HaveOccurred())
+
+				Eventually(func() bool {
+					secret, err = env.CollectSecret(env.Namespace, secretName)
+					Expect(err).NotTo(HaveOccurred())
+					return reflect.DeepEqual(secret.Labels, map[string]string{
+						"LabelKey":                            "LabelValue",
+						"LabelKeyv2":                          "LabelValuev2",
+						"quarks.cloudfoundry.org/secret-kind": "generated",
+					})
+				}, 10*time.Second).Should(Equal(true))
+				secret, err = env.CollectSecret(env.Namespace, secretName)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(secret.Data["password"]).To(MatchRegexp("^\\w{64}$"))
+				Expect(oldSecret.Data["password"]).To(Equal(secret.Data["password"]))
+
+				By("Remove labels to `SecretLabels` spec of quarkssecret")
+				qs, err = env.GetQuarksSecret(env.Namespace, qsName)
+				Expect(err).NotTo(HaveOccurred())
+				qs.Spec.SecretLabels = nil
+				qs, err = env.UpdateQuarksSecret(env.Namespace, qs)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(qs).NotTo(Equal(nil))
+
+				By("Check for the updated secret")
+				Eventually(func() bool {
+					secret, err = env.CollectSecret(env.Namespace, secretName)
+					Expect(err).NotTo(HaveOccurred())
+					return reflect.DeepEqual(secret.Labels, map[string]string{
+						"quarks.cloudfoundry.org/secret-kind": "generated",
+					})
+				}, 10*time.Second).Should(Equal(true))
+				secret, err = env.CollectSecret(env.Namespace, secretName)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(oldSecret.Data["password"]).To(Equal(secret.Data["password"]))
+			})
+		})
+	})
+})

pkg/kube/controllers/controllers.go

@@ -19,6 +19,7 @@ var addToManagerFuncs = []func(context.Context, *config.Config, manager.Manager)
 	quarkssecret.AddCopy,
 	quarkssecret.AddQuarksSecret,
 	quarkssecret.AddSecretRotation,
+	quarkssecret.AddQuarksSecretSecretMeta,
 }
 
 var addToSchemes = runtime.SchemeBuilder{

pkg/kube/controllers/quarkssecret/copy_reconciler.go

@@ -18,6 +18,7 @@ import (
 
 	"code.cloudfoundry.org/quarks-secret/pkg/credsgen"
 	qsv1a1 "code.cloudfoundry.org/quarks-secret/pkg/kube/apis/quarkssecret/v1alpha1"
+	"code.cloudfoundry.org/quarks-secret/pkg/kube/util/mutate"
 	"code.cloudfoundry.org/quarks-utils/pkg/config"
 	"code.cloudfoundry.org/quarks-utils/pkg/ctxlog"
 	"code.cloudfoundry.org/quarks-utils/pkg/pointers"
@@ -92,7 +93,7 @@ func (r *ReconcileCopy) updateCopyStatus(ctx context.Context, qsec *qsv1a1.Quark
 
 func (r *ReconcileCopy) handleQuarksSecretCopies(ctx context.Context, sourceQuarksSecret *qsv1a1.QuarksSecret) error {
 	for _, copy := range sourceQuarksSecret.Spec.Copies {
-		sourceSecret, err := r.getSourceSecret(ctx, sourceQuarksSecret)
+		sourceSecret, err := GetSourceSecret(ctx, r.client, sourceQuarksSecret)
 		if err != nil {
 			return err
 		}
@@ -134,10 +135,11 @@ func (r *ReconcileCopy) handleQuarksSecretCopies(ctx context.Context, sourceQuar
 	return nil
 }
 
-func (r *ReconcileCopy) getSourceSecret(ctx context.Context, qsec *qsv1a1.QuarksSecret) (*corev1.Secret, error) {
+// GetSourceSecret fetches the secret generated by QuarkSecret
+func GetSourceSecret(ctx context.Context, client client.Client, qsec *qsv1a1.QuarksSecret) (*corev1.Secret, error) {
 	secretName := qsec.Spec.SecretName
 	sourceSecret := &corev1.Secret{}
-	err := r.client.Get(ctx, types.NamespacedName{Name: secretName, Namespace: qsec.GetNamespace()}, sourceSecret)
+	err := client.Get(ctx, types.NamespacedName{Name: secretName, Namespace: qsec.GetNamespace()}, sourceSecret)
 	if err != nil {
 		return nil, errors.Wrapf(err, "could not fetch source secret")
 	}
@@ -227,7 +229,7 @@ func (r *ReconcileCopy) createUpdateCopySecret(ctx context.Context, targetSecret
 		}
 	}
 
-	op, err := controllerutil.CreateOrUpdate(ctx, r.client, targetSecret, func() error { return nil })
+	op, err := controllerutil.CreateOrUpdate(ctx, r.client, targetSecret, mutate.SecretMutateFn(targetSecret))
 	if err != nil {
 		return errors.Wrapf(err, "could not create or update target secret '%s/%s'", targetSecret.Namespace, targetSecret.GetName())
 	}

pkg/kube/controllers/quarkssecret/quarkssecret_controller.go

@@ -64,12 +64,27 @@ func AddQuarksSecret(ctx context.Context, config *config.Config, mgr manager.Man
 			n := e.ObjectNew.(*qsv1a1.QuarksSecret)
 			o := e.ObjectOld.(*qsv1a1.QuarksSecret)
 
-			// reconcile if it was already generated and the spec changed
+			// reconcile if it was already generated and the spec changed except for `SecretLabels` & `SecretAnnotations`
+			if o.Status.IsGenerated() {
+				for _, key := range []string{"Type", "Request", "SecretName", "Copies"} {
+					old := reflect.ValueOf(o.Spec).FieldByName(key)
+					new := reflect.ValueOf(n.Spec).FieldByName(key)
+
+					if !reflect.DeepEqual(old.Interface(), new.Interface()) {
+						ctxlog.NewPredicateEvent(e.ObjectNew).Debug(
+							ctx, e.MetaNew, "qsv1a1.QuarksSecret",
+							fmt.Sprintf("Update predicate passed for '%s/%s'.", e.MetaNew.GetNamespace(), e.MetaNew.GetName()),
+						)
+						return true
+					}
+				}
+			}
+
 			// reconcile if it was already generated and controller requested update
-			if o.Status.IsGenerated() && (!reflect.DeepEqual(o.Spec, n.Spec) || n.Status.NotGenerated()) {
+			if n.Status.NotGenerated() {
 				ctxlog.NewPredicateEvent(e.ObjectNew).Debug(
 					ctx, e.MetaNew, "qsv1a1.QuarksSecret",
-					fmt.Sprintf("Update predicate passed for '%s/%s'.", e.MetaNew.GetNamespace(), e.MetaNew.GetName()),
+					fmt.Sprintf("Update predicate passed for '%s/%s': new object is not generated", e.MetaNew.GetNamespace(), e.MetaNew.GetName()),
 				)
 				return true
 			}