add code review todos and light refactoring

Co-authored-by: Ryan Richard <richardry@vmware.com>

internal/controller/tlsconfigutil/tls_config_util.go

@@ -149,33 +149,31 @@ func getCertPool(
 
 // ValidateTLSConfig reads ca bundle in the tlsSpec, supplied either inline using the CertificateAuthorityDate
 // or as a reference to a kubernetes secret or configmap using the CertificateAuthorityDataSource, and returns
-// a condition of type TLSConfigurationValid based on the validity of the ca bundle,
-// a pem encoded ca bundle
-// a X509 cert pool with the ca bundle
-// any error encountered.
-// TODO: it should suffice that this function returns a TLSConfigurationValid condition, and perhaps we could skip
-// returning the error. This can be done once all controllers are able to use this function.
+// - a condition of type TLSConfigurationValid based on the validity of the ca bundle,
+// - a pem encoded ca bundle
+// - a X509 cert pool with the ca bundle
+// TODO: should this show the resource version of the Secret/ConfigMap to the user on all conditions?
 func ValidateTLSConfig(
 	tlsSpec *TLSSpec,
 	conditionPrefix string,
 	namespace string,
 	secretInformer corev1informers.SecretInformer,
 	configMapInformer corev1informers.ConfigMapInformer,
-) (*metav1.Condition, []byte, *x509.CertPool, error) {
+) (*metav1.Condition, []byte, *x509.CertPool) {
 	// try to build a x509 cert pool using the ca data specified in the tlsSpec.
 	certPool, bundle, err := getCertPool(tlsSpec, conditionPrefix, namespace, secretInformer, configMapInformer)
 	if err != nil {
 		// an error encountered during building a certpool using the ca data from the tlsSpec results in an invalid
 		// TLS condition.
-		return invalidTLSCondition(err.Error()), nil, nil, err
+		return invalidTLSCondition(err.Error()), nil, nil
 	}
 	// for us, an empty or nil ca bundle read is results in a valid TLS condition, but we do want to convey that
 	// no ca data was supplied.
 	if bundle == nil {
-		return validTLSCondition(fmt.Sprintf("%s is valid: %s", conditionPrefix, noTLSConfigurationMessage)), bundle, certPool, err
+		return validTLSCondition(fmt.Sprintf("%s is valid: %s", conditionPrefix, noTLSConfigurationMessage)), bundle, certPool
 	}
 
-	return validTLSCondition(fmt.Sprintf("%s is valid: %s", conditionPrefix, loadedTLSConfigurationMessage)), bundle, certPool, err
+	return validTLSCondition(fmt.Sprintf("%s is valid: %s", conditionPrefix, loadedTLSConfigurationMessage)), bundle, certPool
 }
 
 func readCABundleFromSource(source *caBundleSource, namespace string, secretInformer corev1informers.SecretInformer, configMapInformer corev1informers.ConfigMapInformer) (string, error) {
@@ -190,33 +188,37 @@ func readCABundleFromSource(source *caBundleSource, namespace string, secretInfo
 }
 
 func readCABundleFromK8sSecret(namespace string, name string, key string, secretInformer corev1informers.SecretInformer) (string, error) {
+	namespacedName := fmt.Sprintf("%s/%s", namespace, name)
+
 	s, err := secretInformer.Lister().Secrets(namespace).Get(name)
 	if err != nil {
-		return "", errors.Wrapf(err, "failed to get secret %s/%s", namespace, name)
+		return "", errors.Wrapf(err, "failed to get secret %q", namespacedName)
 	}
 	// for kubernetes secrets to be used as a certificate authority data source, the secret should be of type
 	// kubernetes.io/tls or Opaque. It is an error to use a secret that is of any other type.
 	if s.Type != corev1.SecretTypeTLS && s.Type != corev1.SecretTypeOpaque {
-		return "", fmt.Errorf("secret %s/%s of type %s cannot be used as a certificate authority data source", namespace, name, s.Type)
+		return "", fmt.Errorf("secret %q of type %q cannot be used as a certificate authority data source", namespacedName, s.Type)
 	}
 	// ca bundle in the secret is expected to exist in a specific key, if that key does not exist, then it is an error
 	if val, exists := s.Data[key]; exists {
 		return string(val), nil
 	}
-	return "", fmt.Errorf("key %s not found in secret %s/%s", key, namespace, name)
+	return "", fmt.Errorf("key %q not found in secret %q", key, namespacedName)
 }
 
 func readCABundleFromK8sConfigMap(namespace string, name string, key string, configMapInformer corev1informers.ConfigMapInformer) (string, error) {
+	namespacedName := fmt.Sprintf("%s/%s", namespace, name)
+
 	c, err := configMapInformer.Lister().ConfigMaps(namespace).Get(name)
 	if err != nil {
-		return "", errors.Wrapf(err, "failed to get configmap %s/%s", namespace, name)
+		return "", errors.Wrapf(err, "failed to get configmap %q", namespacedName)
 	}
 
 	// ca bundle in the secret is expected to exist in a specific key, if that key does not exist, then it is an error
 	if val, exists := c.Data[key]; exists {
 		return val, nil
 	}
-	return "", fmt.Errorf("key %s not found in configmap %s/%s", key, namespace, name)
+	return "", fmt.Errorf("key %q not found in configmap %q", key, namespacedName)
 }
 
 func validTLSCondition(message string) *metav1.Condition {

internal/controller/tlsconfigutil/tls_config_util_test.go

@@ -193,7 +193,7 @@ func TestValidateTLSConfig(t *testing.T) {
 				Type:    typeTLSConfigurationValid,
 				Status:  metav1.ConditionFalse,
 				Reason:  ReasonInvalidTLSConfig,
-				Message: "tls.certificateAuthorityDataSource is invalid: secret awesome-namespace/awesome-secret-ba of type kubernetes.io/basic-auth cannot be used as a certificate authority data source",
+				Message: `tls.certificateAuthorityDataSource is invalid: secret "awesome-namespace/awesome-secret-ba" of type "kubernetes.io/basic-auth" cannot be used as a certificate authority data source`,
 			},
 		},
 
@@ -250,7 +250,7 @@ func TestValidateTLSConfig(t *testing.T) {
 				Type:    typeTLSConfigurationValid,
 				Status:  metav1.ConditionFalse,
 				Reason:  ReasonInvalidTLSConfig,
-				Message: "tls.certificateAuthorityDataSource is invalid: failed to get secret awesome-namespace/does-not-exist: secret \"does-not-exist\" not found",
+				Message: `tls.certificateAuthorityDataSource is invalid: failed to get secret "awesome-namespace/does-not-exist": secret "does-not-exist" not found`,
 			},
 		},
 		{
@@ -278,7 +278,7 @@ func TestValidateTLSConfig(t *testing.T) {
 				Type:    typeTLSConfigurationValid,
 				Status:  metav1.ConditionFalse,
 				Reason:  ReasonInvalidTLSConfig,
-				Message: "tls.certificateAuthorityDataSource is invalid: failed to get configmap awesome-namespace/does-not-exist: configmap \"does-not-exist\" not found",
+				Message: `tls.certificateAuthorityDataSource is invalid: failed to get configmap "awesome-namespace/does-not-exist": configmap "does-not-exist" not found`,
 			},
 		},
 		{
@@ -333,7 +333,7 @@ func TestValidateTLSConfig(t *testing.T) {
 				sharedInformers.Start(ctx.Done())
 				sharedInformers.WaitForCacheSync(ctx.Done())
 			}
-			actualCondition, _, _, _ := ValidateTLSConfig(tt.tlsSpec, "tls", tt.namespace, secretsInformer, configMapInformer)
+			actualCondition, _, _ := ValidateTLSConfig(tt.tlsSpec, "tls", tt.namespace, secretsInformer, configMapInformer)
 			require.Equal(t, tt.expectedCondition, actualCondition)
 		})
 	}
@@ -347,46 +347,8 @@ func TestReadCABundleFromK8sSecret(t *testing.T) {
 		secretKey       string
 		k8sObjects      []runtime.Object
 		expectedData    string
-		expectError     bool
+		expectError     string
 	}{
-		{
-			name:            "should return error reading a non-existent secret",
-			secretNamespace: "awesome-namespace",
-			secretName:      "does-not-exist",
-			secretKey:       "does-not-matter",
-			k8sObjects: []runtime.Object{
-				&corev1.Secret{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      "awesome-secret",
-						Namespace: "awesome-namespace",
-					},
-					Data: map[string][]byte{
-						"awesome": []byte("pinniped-is-awesome"),
-					},
-				},
-			},
-			expectedData: "",
-			expectError:  true,
-		},
-		{
-			name:            "should return error reading a non-existing key in an existing secret",
-			secretNamespace: "awesome-namespace",
-			secretName:      "awesome-secret",
-			secretKey:       "something-else",
-			k8sObjects: []runtime.Object{
-				&corev1.Secret{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      "awesome-secret",
-						Namespace: "awesome-namespace",
-					},
-					Data: map[string][]byte{
-						"awesome": []byte("pinniped-is-awesome"),
-					},
-				},
-			},
-			expectedData: "",
-			expectError:  true,
-		},
 		{
 			name:            "should return data from existing tls secret and existing key",
 			secretNamespace: "awesome-namespace",
@@ -405,7 +367,7 @@ func TestReadCABundleFromK8sSecret(t *testing.T) {
 				},
 			},
 			expectedData: "pinniped-is-awesome",
-			expectError:  false,
+			expectError:  "",
 		},
 		{
 			name:            "should return data from existing opaque secret and existing key",
@@ -425,7 +387,66 @@ func TestReadCABundleFromK8sSecret(t *testing.T) {
 				},
 			},
 			expectedData: "pinniped-is-awesome",
-			expectError:  false,
+			expectError:  "",
+		},
+		{
+			name:            "should return error reading a non-existent secret",
+			secretNamespace: "awesome-namespace",
+			secretName:      "does-not-exist",
+			secretKey:       "does-not-matter",
+			k8sObjects: []runtime.Object{
+				&corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "awesome-secret",
+						Namespace: "awesome-namespace",
+					},
+					Data: map[string][]byte{
+						"awesome": []byte("pinniped-is-awesome"),
+					},
+				},
+			},
+			expectedData: "",
+			expectError:  `failed to get secret "awesome-namespace/does-not-exist": secret "does-not-exist" not found`,
+		},
+		{
+			name:            "should return error when secret has wrong type",
+			secretNamespace: "awesome-namespace",
+			secretName:      "awesome-secret",
+			secretKey:       "awesome",
+			k8sObjects: []runtime.Object{
+				&corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "awesome-secret",
+						Namespace: "awesome-namespace",
+					},
+					Type: "other-type",
+					Data: map[string][]byte{
+						"awesome": []byte("pinniped-is-awesome"),
+					},
+				},
+			},
+
+			expectError: `secret "awesome-namespace/awesome-secret" of type "other-type" cannot be used as a certificate authority data source`,
+		},
+		{
+			name:            "should return error reading a non-existing key in an existing secret",
+			secretNamespace: "awesome-namespace",
+			secretName:      "awesome-secret",
+			secretKey:       "something-else",
+			k8sObjects: []runtime.Object{
+				&corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "awesome-secret",
+						Namespace: "awesome-namespace",
+					},
+					Type: corev1.SecretTypeOpaque,
+					Data: map[string][]byte{
+						"awesome": []byte("pinniped-is-awesome"),
+					},
+				},
+			},
+			expectedData: "",
+			expectError:  `key "something-else" not found in secret "awesome-namespace/awesome-secret"`,
 		},
 	}
 
@@ -446,8 +467,8 @@ func TestReadCABundleFromK8sSecret(t *testing.T) {
 			sharedInformers.WaitForCacheSync(ctx.Done())
 			// now the objects from kubernetes should be sync'd into the informer cache.
 			actualData, actualError := readCABundleFromK8sSecret(tt.secretNamespace, tt.secretName, tt.secretKey, secretsInformer)
-			if tt.expectError {
-				require.Error(t, actualError)
+			if tt.expectError != "" {
+				require.ErrorContains(t, actualError, tt.expectError)
 			} else {
 				require.NoError(t, actualError)
 			}
@@ -464,8 +485,26 @@ func TestReadCABundleFromK8sConfigMap(t *testing.T) {
 		configMapKey       string
 		k8sObjects         []runtime.Object
 		expectedData       string
-		expectError        bool
+		expectError        string
 	}{
+		{
+			name:               "should return expected data from an existing key in an existing configMap",
+			configMapNamespace: "awesome-namespace",
+			configMapName:      "awesome-configmap",
+			configMapKey:       "awesome",
+			k8sObjects: []runtime.Object{
+				&corev1.ConfigMap{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "awesome-configmap",
+						Namespace: "awesome-namespace",
+					},
+					Data: map[string]string{
+						"awesome": "pinniped-is-awesome",
+					},
+				},
+			},
+			expectedData: "pinniped-is-awesome",
+		},
 		{
 			name:               "should return error reading a non-existent configMap",
 			configMapNamespace: "awesome-namespace",
@@ -483,7 +522,7 @@ func TestReadCABundleFromK8sConfigMap(t *testing.T) {
 				},
 			},
 			expectedData: "",
-			expectError:  true,
+			expectError:  `failed to get configmap "awesome-namespace/does-not-exist": configmap "does-not-exist" not found`,
 		},
 		{
 			name:               "should return error reading a non-existing key in an existing configMap",
@@ -502,26 +541,7 @@ func TestReadCABundleFromK8sConfigMap(t *testing.T) {
 				},
 			},
 			expectedData: "",
-			expectError:  true,
-		},
-		{
-			name:               "should return expected data from an existing key in an existing configMap",
-			configMapNamespace: "awesome-namespace",
-			configMapName:      "awesome-configmap",
-			configMapKey:       "awesome",
-			k8sObjects: []runtime.Object{
-				&corev1.ConfigMap{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      "awesome-configmap",
-						Namespace: "awesome-namespace",
-					},
-					Data: map[string]string{
-						"awesome": "pinniped-is-awesome",
-					},
-				},
-			},
-			expectedData: "pinniped-is-awesome",
-			expectError:  false,
+			expectError:  `key "does-not-exist" not found in configmap "awesome-namespace/awesome-configmap"`,
 		},
 	}
 
@@ -542,8 +562,8 @@ func TestReadCABundleFromK8sConfigMap(t *testing.T) {
 			sharedInformers.Start(ctx.Done())
 			sharedInformers.WaitForCacheSync(ctx.Done())
 			actualData, actualError := readCABundleFromK8sConfigMap(tt.configMapNamespace, tt.configMapName, tt.configMapKey, configMapInformer)
-			if tt.expectError {
-				require.Error(t, actualError)
+			if tt.expectError != "" {
+				require.ErrorContains(t, actualError, tt.expectError)
 			} else {
 				require.NoError(t, actualError)
 			}