Merge branch 'main' of github.com:1Password/onepassword-operator into secret-path-updates

.VERSION

@@ -1 +1 @@
-v1.0.1
+1.0.2

CHANGELOG.md

@@ -12,6 +12,14 @@
 
 ---
 
+[//]: # (START/v1.0.2)
+# v1.0.2
+
+## Fixes
+ * Name normalizer added to handle non-conforming item names.
+
+---
+
 [//]: # (START/v1.0.1)
 # v1.0.1
 

pkg/controller/deployment/deployment_controller.go

@@ -191,6 +191,7 @@ func (r *ReconcileDeployment) HandleApplyingDeployment(namespace string, annotat
 	reqLog := log.WithValues("Request.Namespace", request.Namespace, "Request.Name", request.Name)
 
 	secretName := annotations[op.NameAnnotation]
+	secretLabels := map[string]string(nil)
 	if len(secretName) == 0 {
 		reqLog.Info("No 'item-name' annotation set. 'item-path' and 'item-name' must be set as annotations to add new secret.")
 		return nil
@@ -201,5 +202,5 @@ func (r *ReconcileDeployment) HandleApplyingDeployment(namespace string, annotat
 		return fmt.Errorf("Failed to retrieve item: %v", err)
 	}
 
-	return kubeSecrets.CreateKubernetesSecretFromItem(r.kubeClient, secretName, namespace, item, annotations[op.RestartDeploymentsAnnotation])
+	return kubeSecrets.CreateKubernetesSecretFromItem(r.kubeClient, secretName, namespace, item, annotations[op.RestartDeploymentsAnnotation], secretLabels, annotations)
 }

pkg/controller/deployment/deployment_controller_test.go

@@ -258,7 +258,7 @@ var tests = []testReconcileItem{
 		},
 	},
 	{
-		testName: "Test Do not update if OnePassword Item Version or VaultPath has not changed",
+		testName: "Test Do not update if Annotations have not changed",
 		deploymentResource: &appsv1.Deployment{
 			TypeMeta: metav1.TypeMeta{
 				Kind:       deploymentKind,
@@ -271,6 +271,7 @@ var tests = []testReconcileItem{
 					op.ItemPathAnnotation: itemPath,
 					op.NameAnnotation:     name,
 				},
+				Labels: map[string]string{},
 			},
 		},
 		existingSecret: &corev1.Secret{
@@ -278,8 +279,9 @@ var tests = []testReconcileItem{
 				Name:      name,
 				Namespace: namespace,
 				Annotations: map[string]string{
-					op.VersionAnnotation:  fmt.Sprint(version),
+					op.VersionAnnotation: fmt.Sprint(version),
 					op.ItemPathAnnotation: itemPath,
+					op.NameAnnotation:     name,
 				},
 			},
 			Data: expectedSecretData,
@@ -291,7 +293,10 @@ var tests = []testReconcileItem{
 				Namespace: namespace,
 				Annotations: map[string]string{
 					op.VersionAnnotation: fmt.Sprint(version),
+					op.ItemPathAnnotation: itemPath,
+					op.NameAnnotation:     name,
 				},
+				Labels: map[string]string(nil),
 			},
 			Data: expectedSecretData,
 		},

pkg/controller/onepassworditem/onepassworditem_controller.go

@@ -143,12 +143,14 @@ func (r *ReconcileOnePasswordItem) removeOnePasswordFinalizerFromOnePasswordItem
 
 func (r *ReconcileOnePasswordItem) HandleOnePasswordItem(resource *onepasswordv1.OnePasswordItem, request reconcile.Request) error {
 	secretName := resource.GetName()
-	autoRestart := resource.Annotations[op.RestartDeploymentsAnnotation]
+	labels := resource.Labels
+	annotations := resource.Annotations
+	autoRestart := annotations[op.RestartDeploymentsAnnotation]
 
 	item, err := onepassword.GetOnePasswordItemByPath(r.opConnectClient, resource.Spec.ItemPath)
 	if err != nil {
 		return fmt.Errorf("Failed to retrieve item: %v", err)
 	}
 
-	return kubeSecrets.CreateKubernetesSecretFromItem(r.kubeClient, secretName, resource.Namespace, item, autoRestart)
+	return kubeSecrets.CreateKubernetesSecretFromItem(r.kubeClient, secretName, resource.Namespace, item, autoRestart, labels, annotations)
 }

pkg/controller/onepassworditem/onepassworditem_test.go

@@ -132,6 +132,7 @@ var tests = []testReconcileItem{
 				Namespace: namespace,
 				Annotations: map[string]string{
 					op.VersionAnnotation: fmt.Sprint(version),
+					op.ItemPathAnnotation: itemPath,
 				},
 			},
 			Data: expectedSecretData,
@@ -151,6 +152,11 @@ var tests = []testReconcileItem{
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      name,
 				Namespace: namespace,
+				Annotations: map[string]string{
+					op.VersionAnnotation: fmt.Sprint(version),
+					op.ItemPathAnnotation: itemPath,
+				},
+				Labels: map[string]string{},
 			},
 			Spec: onepasswordv1.OnePasswordItemSpec{
 				ItemPath: itemPath,
@@ -162,7 +168,9 @@ var tests = []testReconcileItem{
 				Namespace: namespace,
 				Annotations: map[string]string{
 					op.VersionAnnotation: "456",
+					op.ItemPathAnnotation: itemPath,
 				},
+				Labels: map[string]string{},
 			},
 			Data: expectedSecretData,
 		},
@@ -173,7 +181,9 @@ var tests = []testReconcileItem{
 				Namespace: namespace,
 				Annotations: map[string]string{
 					op.VersionAnnotation: fmt.Sprint(version),
+					op.ItemPathAnnotation: itemPath,
 				},
+				Labels: map[string]string{},
 			},
 			Data: expectedSecretData,
 		},
@@ -275,7 +285,7 @@ var tests = []testReconcileItem{
 				"password":       []byte(password),
 				"username":       []byte(username),
 				"first-host":     []byte(firstHost),
-				"aws-access-key": []byte(awsKey),
+				"AWS-Access-Key": []byte(awsKey),
 				"ice-cream-type": []byte(iceCream),
 			},
 		},
@@ -287,6 +297,47 @@ var tests = []testReconcileItem{
 			"😄 ice-cream type": iceCream,
 		},
 	},
+	{
+		testName: "Secret from 1Password item with `-`, `_` and `.`",
+		customResource: &onepasswordv1.OnePasswordItem{
+			TypeMeta: metav1.TypeMeta{
+				Kind:       onePasswordItemKind,
+				APIVersion: onePasswordItemAPIVersion,
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "!.my_sECReT.it3m%-_",
+				Namespace: namespace,
+			},
+			Spec: onepasswordv1.OnePasswordItemSpec{
+				ItemPath: itemPath,
+			},
+		},
+		existingSecret: nil,
+		expectedError:  nil,
+		expectedResultSecret: &corev1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "my-secret.it3m",
+				Namespace: namespace,
+				Annotations: map[string]string{
+					op.VersionAnnotation: fmt.Sprint(version),
+				},
+			},
+			Data: map[string][]byte{
+				"password":          []byte(password),
+				"username":          []byte(username),
+				"first-host":        []byte(firstHost),
+				"AWS-Access-Key":    []byte(awsKey),
+				"-_ice_cream.type.": []byte(iceCream),
+			},
+		},
+		opItem: map[string]string{
+			userKey:               username,
+			passKey:               password,
+			"first host":          firstHost,
+			"AWS Access Key":      awsKey,
+			"😄 -_ice_cream.type.": iceCream,
+		},
+	},
 }
 
 func TestReconcileOnePasswordItem(t *testing.T) {

pkg/kubernetessecrets/kubernetes_secrets_builder.go

@@ -13,6 +13,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"reflect"
 	kubeValidate "k8s.io/apimachinery/pkg/util/validation"
 
 	kubernetesClient "sigs.k8s.io/controller-runtime/pkg/client"
@@ -28,22 +29,27 @@ const RestartDeploymentsAnnotation = OnepasswordPrefix + "/auto-restart"
 
 var log = logf.Log
 
-func CreateKubernetesSecretFromItem(kubeClient kubernetesClient.Client, secretName, namespace string, item *onepassword.Item, autoRestart string) error {
+func CreateKubernetesSecretFromItem(kubeClient kubernetesClient.Client, secretName, namespace string, item *onepassword.Item, autoRestart string, labels map[string]string, secretAnnotations map[string]string) error {
 
 	itemVersion := fmt.Sprint(item.Version)
-	annotations := map[string]string{
+
-		VersionAnnotation:  itemVersion,
+	// If secretAnnotations is nil we create an empty map so we can later assign values for the OP Annotations in the map
-		ItemPathAnnotation: fmt.Sprintf("vaults/%v/items/%v", item.Vault.ID, item.ID),
+	if secretAnnotations == nil {
+		secretAnnotations = map[string]string{}
 	}
+
+	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 {
 			log.Error(err, "Error parsing %v annotation on Secret %v. Must be true or false. Defaulting to false.", RestartDeploymentsAnnotation, secretName)
 			return err
 		}
-		annotations[RestartDeploymentsAnnotation] = autoRestart
+		secretAnnotations[RestartDeploymentsAnnotation] = autoRestart
 	}
-	secret := BuildKubernetesSecretFromOnePasswordItem(secretName, namespace, annotations, *item)
+	secret := BuildKubernetesSecretFromOnePasswordItem(secretName, namespace, secretAnnotations, labels, *item)
 
 	currentSecret := &corev1.Secret{}
 	err := kubeClient.Get(context.Background(), types.NamespacedName{Name: secret.Name, Namespace: secret.Namespace}, currentSecret)
@@ -54,9 +60,10 @@ func CreateKubernetesSecretFromItem(kubeClient kubernetesClient.Client, secretNa
 		return err
 	}
 
-	if currentSecret.Annotations[VersionAnnotation] != itemVersion || currentSecret.Annotations[ItemPathAnnotation] != annotations[ItemPathAnnotation]{
+	if ! reflect.DeepEqual(currentSecret.Annotations, secretAnnotations) || ! reflect.DeepEqual(currentSecret.Labels, labels) {
 		log.Info(fmt.Sprintf("Updating Secret %v at namespace '%v'", secret.Name, secret.Namespace))
-		currentSecret.ObjectMeta.Annotations = annotations
+		currentSecret.ObjectMeta.Annotations = secretAnnotations
+		currentSecret.ObjectMeta.Labels = labels
 		currentSecret.Data = secret.Data
 		return kubeClient.Update(context.Background(), currentSecret)
 	}
@@ -65,12 +72,13 @@ func CreateKubernetesSecretFromItem(kubeClient kubernetesClient.Client, secretNa
 	return nil
 }
 
-func BuildKubernetesSecretFromOnePasswordItem(name, namespace string, annotations map[string]string, item onepassword.Item) *corev1.Secret {
+func BuildKubernetesSecretFromOnePasswordItem(name, namespace string, annotations map[string]string, labels map[string]string, item onepassword.Item) *corev1.Secret {
 	return &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:        formatSecretName(name),
 			Namespace:   namespace,
 			Annotations: annotations,
+			Labels:      labels,
 		},
 		Data: BuildKubernetesSecretData(item.Fields),
 	}
@@ -80,16 +88,17 @@ func BuildKubernetesSecretData(fields []*onepassword.ItemField) map[string][]byt
 	secretData := map[string][]byte{}
 	for i := 0; i < len(fields); i++ {
 		if fields[i].Value != "" {
-			key := formatSecretName(fields[i].Label)
+			key := formatSecretDataName(fields[i].Label)
 			secretData[key] = []byte(fields[i].Value)
 		}
 	}
 	return secretData
 }
 
-// formatSecretName rewrites a value to be a valid Secret name or Secret data key.
+// formatSecretName rewrites a value to be a valid Secret name.
 //
-// The Secret meta.name and data keys must be valid DNS subdomain names (https://kubernetes.io/docs/concepts/configuration/secret/#overview-of-secrets)
+// The Secret meta.name and data keys must be valid DNS subdomain names
+// (https://kubernetes.io/docs/concepts/configuration/secret/#overview-of-secrets)
 func formatSecretName(value string) string {
 	if errs := kubeValidate.IsDNS1123Subdomain(value); len(errs) == 0 {
 		return value
@@ -97,7 +106,18 @@ func formatSecretName(value string) string {
 	return createValidSecretName(value)
 }
 
-var invalidDNS1123Chars = regexp.MustCompile("[^a-z0-9-]+")
+// formatSecretDataName rewrites a value to be a valid Secret data key.
+//
+// The Secret data keys must consist of alphanumeric numbers, `-`, `_` or `.`
+// (https://kubernetes.io/docs/concepts/configuration/secret/#overview-of-secrets)
+func formatSecretDataName(value string) string {
+	if errs := kubeValidate.IsConfigMapKey(value); len(errs) == 0 {
+		return value
+	}
+	return createValidSecretDataName(value)
+}
+
+var invalidDNS1123Chars = regexp.MustCompile("[^a-z0-9-.]+")
 
 func createValidSecretName(value string) string {
 	result := strings.ToLower(value)
@@ -108,5 +128,19 @@ func createValidSecretName(value string) string {
 	}
 
 	// first and last character MUST be alphanumeric
-	return strings.Trim(result, "-")
+	return strings.Trim(result, "-.")
+}
+
+var invalidDataChars = regexp.MustCompile("[^a-zA-Z0-9-._]+")
+var invalidStartEndChars = regexp.MustCompile("(^[^a-zA-Z0-9-._]+|[^a-zA-Z0-9-._]+$)")
+
+func createValidSecretDataName(value string) string {
+	result := invalidStartEndChars.ReplaceAllString(value, "")
+	result = invalidDataChars.ReplaceAllString(result, "-")
+
+	if len(result) > kubeValidate.DNS1123SubdomainMaxLength {
+		result = result[0:kubeValidate.DNS1123SubdomainMaxLength]
+	}
+
+	return result
 }

pkg/kubernetessecrets/kubernetes_secrets_builder_test.go

@@ -3,13 +3,13 @@ package kubernetessecrets
 import (
 	"context"
 	"fmt"
-	kubeValidate "k8s.io/apimachinery/pkg/util/validation"
 	"strings"
 	"testing"
 
 	"github.com/1Password/connect-sdk-go/onepassword"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
+	kubeValidate "k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/client-go/kubernetes"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 )
@@ -31,7 +31,11 @@ func TestCreateKubernetesSecretFromOnePasswordItem(t *testing.T) {
 	item.ID = "h46bb3jddvay7nxopfhvlwg35q"
 
 	kubeClient := fake.NewFakeClient()
-	err := CreateKubernetesSecretFromItem(kubeClient, secretName, namespace, &item, restartDeploymentAnnotation)
+	secretLabels := map[string]string{}
+	secretAnnotations := map[string]string{
+		"testAnnotation": "exists",
+	}
+	err := CreateKubernetesSecretFromItem(kubeClient, secretName, namespace, &item, restartDeploymentAnnotation, secretLabels, secretAnnotations)
 	if err != nil {
 		t.Errorf("Unexpected error: %v", err)
 	}
@@ -43,6 +47,10 @@ 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 TestUpdateKubernetesSecretFromOnePasswordItem(t *testing.T) {
@@ -56,7 +64,9 @@ func TestUpdateKubernetesSecretFromOnePasswordItem(t *testing.T) {
 	item.ID = "h46bb3jddvay7nxopfhvlwg35q"
 
 	kubeClient := fake.NewFakeClient()
-	err := CreateKubernetesSecretFromItem(kubeClient, secretName, namespace, &item, restartDeploymentAnnotation)
+	secretLabels := map[string]string{}
+	secretAnnotations := map[string]string{}
+	err := CreateKubernetesSecretFromItem(kubeClient, secretName, namespace, &item, restartDeploymentAnnotation, secretLabels, secretAnnotations)
 	if err != nil {
 		t.Errorf("Unexpected error: %v", err)
 	}
@@ -67,7 +77,7 @@ func TestUpdateKubernetesSecretFromOnePasswordItem(t *testing.T) {
 	newItem.Version = 456
 	newItem.Vault.ID = "hfnjvi6aymbsnfc2xeeoheizda"
 	newItem.ID = "h46bb3jddvay7nxopfhvlwg35q"
-	err = CreateKubernetesSecretFromItem(kubeClient, secretName, namespace, &newItem, restartDeploymentAnnotation)
+	err = CreateKubernetesSecretFromItem(kubeClient, secretName, namespace, &newItem, restartDeploymentAnnotation, secretLabels, secretAnnotations)
 	if err != nil {
 		t.Errorf("Unexpected error: %v", err)
 	}
@@ -100,8 +110,9 @@ func TestBuildKubernetesSecretFromOnePasswordItem(t *testing.T) {
 	}
 	item := onepassword.Item{}
 	item.Fields = generateFields(5)
+	labels := map[string]string{}
 
-	kubeSecret := BuildKubernetesSecretFromOnePasswordItem(name, namespace, annotations, item)
+	kubeSecret := BuildKubernetesSecretFromOnePasswordItem(name, namespace, annotations, labels, item)
 	if kubeSecret.Name != strings.ToLower(name) {
 		t.Errorf("Expected name value: %v but got: %v", name, kubeSecret.Name)
 	}
@@ -121,6 +132,7 @@ func TestBuildKubernetesSecretFixesInvalidLabels(t *testing.T) {
 	annotations := map[string]string{
 		"annotationKey": "annotationValue",
 	}
+	labels := map[string]string{}
 	item := onepassword.Item{}
 
 	item.Fields = []*onepassword.ItemField{
@@ -134,7 +146,7 @@ func TestBuildKubernetesSecretFixesInvalidLabels(t *testing.T) {
 		},
 	}
 
-	kubeSecret := BuildKubernetesSecretFromOnePasswordItem(name, namespace, annotations, item)
+	kubeSecret := BuildKubernetesSecretFromOnePasswordItem(name, namespace, annotations, labels,  item)
 
 	// Assert Secret's meta.name was fixed
 	if kubeSecret.Name != expectedName {
@@ -205,7 +217,7 @@ func ParseVaultIdAndItemIdFromPath(path string) (string, string, error) {
 }
 
 func validLabel(v string) bool {
-	if err := kubeValidate.IsDNS1123Subdomain(v); len(err) > 0 {
+	if err := kubeValidate.IsConfigMapKey(v); len(err) > 0 {
 		return false
 	}
 	return true

pkg/onepassword/secret_update_handler.go

@@ -138,7 +138,7 @@ func (h *SecretUpdateHandler) updateKubernetesSecrets() (map[string]map[string]*
 			log.Info(fmt.Sprintf("Updating kubernetes secret '%v'", secret.GetName()))
 			secret.Annotations[VersionAnnotation] = itemVersion
 			secret.Annotations[ItemPathAnnotation] = itemPathString
-			updatedSecret := kubeSecrets.BuildKubernetesSecretFromOnePasswordItem(secret.Name, secret.Namespace, secret.Annotations, *item)
+			updatedSecret := kubeSecrets.BuildKubernetesSecretFromOnePasswordItem(secret.Name, secret.Namespace, secret.Annotations, secret.Labels, *item)
 			log.Info(fmt.Sprintf("New secret path: %v and version: %v", updatedSecret.Annotations[ItemPathAnnotation], updatedSecret.Annotations[VersionAnnotation]))
 			h.client.Update(context.Background(), updatedSecret)
 			if updatedSecrets[secret.Namespace] == nil {