From afa076d321c017a9f94acb17781ef64c0beb5dd6 Mon Sep 17 00:00:00 2001 From: Joris Coenen Date: Mon, 11 Apr 2022 15:55:28 +0200 Subject: [PATCH 1/2] Stop copying annotations from OnePasswordItem and Deployment to Secret There is no reason for random annotations to be carried over. This can lead to weird problems like the `kubectl.kubernetes.io/last-applied-configuration` annotation ending up on a Secret. --- CHANGELOG.md | 2 +- .../deployment/deployment_controller.go | 2 +- .../onepassworditem_controller.go | 5 ++-- .../kubernetes_secrets_builder.go | 13 ++++------ .../kubernetes_secrets_builder_test.go | 24 ++++--------------- 5 files changed, 13 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7519382c..c75ab213 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ ## Fixes -- A user-friendly description of a fix. {issue-number} +- Annotations from a Deployment or OnePasswordItem are no longer applied to Secrets that are created for it. {#102} ## Security diff --git a/pkg/controller/deployment/deployment_controller.go b/pkg/controller/deployment/deployment_controller.go index 0e709a60..76e030e7 100644 --- a/pkg/controller/deployment/deployment_controller.go +++ b/pkg/controller/deployment/deployment_controller.go @@ -218,5 +218,5 @@ func (r *ReconcileDeployment) HandleApplyingDeployment(deployment *appsv1.Deploy UID: deployment.GetUID(), } - return kubeSecrets.CreateKubernetesSecretFromItem(r.kubeClient, secretName, namespace, item, annotations[op.RestartDeploymentsAnnotation], secretLabels, secretType, annotations, ownerRef) + return kubeSecrets.CreateKubernetesSecretFromItem(r.kubeClient, secretName, namespace, item, annotations[op.RestartDeploymentsAnnotation], secretLabels, secretType, ownerRef) } diff --git a/pkg/controller/onepassworditem/onepassworditem_controller.go b/pkg/controller/onepassworditem/onepassworditem_controller.go index 4b2e6957..613aa411 100644 --- a/pkg/controller/onepassworditem/onepassworditem_controller.go +++ b/pkg/controller/onepassworditem/onepassworditem_controller.go @@ -147,9 +147,8 @@ func (r *ReconcileOnePasswordItem) removeOnePasswordFinalizerFromOnePasswordItem func (r *ReconcileOnePasswordItem) HandleOnePasswordItem(resource *onepasswordv1.OnePasswordItem, request reconcile.Request) error { secretName := resource.GetName() labels := resource.Labels - annotations := resource.Annotations secretType := resource.Type - autoRestart := annotations[op.RestartDeploymentsAnnotation] + autoRestart := resource.Annotations[op.RestartDeploymentsAnnotation] item, err := onepassword.GetOnePasswordItemByPath(r.opConnectClient, resource.Spec.ItemPath) if err != nil { @@ -168,5 +167,5 @@ func (r *ReconcileOnePasswordItem) HandleOnePasswordItem(resource *onepasswordv1 UID: resource.GetUID(), } - return kubeSecrets.CreateKubernetesSecretFromItem(r.kubeClient, secretName, resource.Namespace, item, autoRestart, labels, secretType, annotations, ownerRef) + return kubeSecrets.CreateKubernetesSecretFromItem(r.kubeClient, secretName, resource.Namespace, item, autoRestart, labels, secretType, ownerRef) } diff --git a/pkg/kubernetessecrets/kubernetes_secrets_builder.go b/pkg/kubernetessecrets/kubernetes_secrets_builder.go index 2cd3a052..b7587c79 100644 --- a/pkg/kubernetessecrets/kubernetes_secrets_builder.go +++ b/pkg/kubernetessecrets/kubernetes_secrets_builder.go @@ -35,18 +35,13 @@ var ErrCannotUpdateSecretType = errs.New("Cannot change secret type. Secret type var log = logf.Log -func CreateKubernetesSecretFromItem(kubeClient kubernetesClient.Client, secretName, namespace string, item *onepassword.Item, autoRestart string, labels map[string]string, secretType string, secretAnnotations map[string]string, ownerRef *metav1.OwnerReference) error { - +func CreateKubernetesSecretFromItem(kubeClient kubernetesClient.Client, secretName, namespace string, item *onepassword.Item, autoRestart string, labels map[string]string, secretType string, ownerRef *metav1.OwnerReference) error { itemVersion := fmt.Sprint(item.Version) - - // If secretAnnotations is nil we create an empty map so we can later assign values for the OP Annotations in the map - if secretAnnotations == nil { - secretAnnotations = map[string]string{} + secretAnnotations := map[string]string{ + VersionAnnotation: itemVersion, + ItemPathAnnotation: fmt.Sprintf("vaults/%v/items/%v", item.Vault.ID, item.ID), } - secretAnnotations[VersionAnnotation] = itemVersion - secretAnnotations[ItemPathAnnotation] = fmt.Sprintf("vaults/%v/items/%v", item.Vault.ID, item.ID) - if autoRestart != "" { _, err := utils.StringToBool(autoRestart) if err != nil { diff --git a/pkg/kubernetessecrets/kubernetes_secrets_builder_test.go b/pkg/kubernetessecrets/kubernetes_secrets_builder_test.go index 47475048..b24f26ac 100644 --- a/pkg/kubernetessecrets/kubernetes_secrets_builder_test.go +++ b/pkg/kubernetessecrets/kubernetes_secrets_builder_test.go @@ -33,12 +33,9 @@ func TestCreateKubernetesSecretFromOnePasswordItem(t *testing.T) { kubeClient := fake.NewFakeClient() secretLabels := map[string]string{} - secretAnnotations := map[string]string{ - "testAnnotation": "exists", - } secretType := "" - err := CreateKubernetesSecretFromItem(kubeClient, secretName, namespace, &item, restartDeploymentAnnotation, secretLabels, secretType, secretAnnotations, nil) + err := CreateKubernetesSecretFromItem(kubeClient, secretName, namespace, &item, restartDeploymentAnnotation, secretLabels, secretType, nil) if err != nil { t.Errorf("Unexpected error: %v", err) } @@ -50,10 +47,6 @@ func TestCreateKubernetesSecretFromOnePasswordItem(t *testing.T) { } compareFields(item.Fields, createdSecret.Data, t) compareAnnotationsToItem(createdSecret.Annotations, item, t) - - if createdSecret.Annotations["testAnnotation"] != "exists" { - t.Errorf("Expected testAnnotation to be merged with existing annotations, but wasn't.") - } } func TestKubernetesSecretFromOnePasswordItemOwnerReferences(t *testing.T) { @@ -68,9 +61,6 @@ func TestKubernetesSecretFromOnePasswordItemOwnerReferences(t *testing.T) { kubeClient := fake.NewFakeClient() secretLabels := map[string]string{} - secretAnnotations := map[string]string{ - "testAnnotation": "exists", - } secretType := "" ownerRef := &metav1.OwnerReference{ @@ -79,7 +69,7 @@ func TestKubernetesSecretFromOnePasswordItemOwnerReferences(t *testing.T) { Name: "test-deployment", UID: types.UID("test-uid"), } - err := CreateKubernetesSecretFromItem(kubeClient, secretName, namespace, &item, restartDeploymentAnnotation, secretLabels, secretType, secretAnnotations, ownerRef) + err := CreateKubernetesSecretFromItem(kubeClient, secretName, namespace, &item, restartDeploymentAnnotation, secretLabels, secretType, ownerRef) if err != nil { t.Errorf("Unexpected error: %v", err) } @@ -116,10 +106,9 @@ func TestUpdateKubernetesSecretFromOnePasswordItem(t *testing.T) { kubeClient := fake.NewFakeClient() secretLabels := map[string]string{} - secretAnnotations := map[string]string{} secretType := "" - err := CreateKubernetesSecretFromItem(kubeClient, secretName, namespace, &item, restartDeploymentAnnotation, secretLabels, secretType, secretAnnotations, nil) + err := CreateKubernetesSecretFromItem(kubeClient, secretName, namespace, &item, restartDeploymentAnnotation, secretLabels, secretType, nil) if err != nil { t.Errorf("Unexpected error: %v", err) @@ -131,7 +120,7 @@ func TestUpdateKubernetesSecretFromOnePasswordItem(t *testing.T) { newItem.Version = 456 newItem.Vault.ID = "hfnjvi6aymbsnfc2xeeoheizda" newItem.ID = "h46bb3jddvay7nxopfhvlwg35q" - err = CreateKubernetesSecretFromItem(kubeClient, secretName, namespace, &newItem, restartDeploymentAnnotation, secretLabels, secretType, secretAnnotations, nil) + err = CreateKubernetesSecretFromItem(kubeClient, secretName, namespace, &newItem, restartDeploymentAnnotation, secretLabels, secretType, nil) if err != nil { t.Errorf("Unexpected error: %v", err) } @@ -232,12 +221,9 @@ func TestCreateKubernetesTLSSecretFromOnePasswordItem(t *testing.T) { kubeClient := fake.NewFakeClient() secretLabels := map[string]string{} - secretAnnotations := map[string]string{ - "testAnnotation": "exists", - } secretType := "kubernetes.io/tls" - err := CreateKubernetesSecretFromItem(kubeClient, secretName, namespace, &item, restartDeploymentAnnotation, secretLabels, secretType, secretAnnotations, nil) + err := CreateKubernetesSecretFromItem(kubeClient, secretName, namespace, &item, restartDeploymentAnnotation, secretLabels, secretType, nil) if err != nil { t.Errorf("Unexpected error: %v", err) } From 6326a856ae62bf90d46e217c4b1a271752db7913 Mon Sep 17 00:00:00 2001 From: Joris Coenen Date: Tue, 12 Apr 2022 10:41:11 +0200 Subject: [PATCH 2/2] Fix test Annotations are no longer copied from the deployment to the secret, so the test should not assert that the secret has a name annotation. --- pkg/controller/deployment/deployment_controller_test.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pkg/controller/deployment/deployment_controller_test.go b/pkg/controller/deployment/deployment_controller_test.go index bd982313..0597c5fc 100644 --- a/pkg/controller/deployment/deployment_controller_test.go +++ b/pkg/controller/deployment/deployment_controller_test.go @@ -281,7 +281,6 @@ var tests = []testReconcileItem{ Annotations: map[string]string{ op.VersionAnnotation: fmt.Sprint(version), op.ItemPathAnnotation: itemPath, - op.NameAnnotation: name, }, }, Data: expectedSecretData, @@ -294,7 +293,6 @@ var tests = []testReconcileItem{ Annotations: map[string]string{ op.VersionAnnotation: fmt.Sprint(version), op.ItemPathAnnotation: itemPath, - op.NameAnnotation: name, }, Labels: map[string]string(nil), }, @@ -385,7 +383,7 @@ var tests = []testReconcileItem{ }, } -func TestReconcileDepoyment(t *testing.T) { +func TestReconcileDeployment(t *testing.T) { for _, testData := range tests { t.Run(testData.testName, func(t *testing.T) {