UPSTREAM: <carry>: allow type mutation for specific secrets

squash with the previous PRs during the rebase openshift#1924 openshift#1929
openshift · Apr 4, 2024 · 8662730 · 8662730
1 parent 8628c3c
commit 8662730
Show file tree

Hide file tree

Showing 2 changed files with 46 additions and 38 deletions.
diff --git a/pkg/apis/core/validation/validation_patch.go b/pkg/apis/core/validation/validation_patch.go
@@ -44,8 +44,14 @@ var (
 )
 
 func openShiftValidateSecretUpdateIsTypeMutationAllowed(newSecret, oldSecret *core.Secret) bool {
-	// we allow "SecretTypeTLS" -> "kubernetes.io/tls" only
-	if oldSecret.Type == "SecretTypeTLS" && newSecret.Type == core.SecretTypeTLS {
+	// initially, this check was stricter.
+	// however, due to the platform's long history (spanning several years)
+	// and the complexity of ensuring that resources were consistently created with only one type,
+	// it is now permissible for any type to transition to "kubernetes.io/tls".
+	//
+	// additionally, it should be noted that default values might also be applied in some cases.
+	// (https://github.com/openshift/kubernetes/blob/258f1d5fb6491ba65fd8201c827e179432430627/pkg/apis/core/v1/defaults.go#L280-L284)
+	if oldSecret.Type != core.SecretTypeTLS && newSecret.Type == core.SecretTypeTLS {
 		if _, ok := whitelist[oldSecret.Namespace]; ok {
 			return true
 		}

diff --git a/pkg/apis/core/validation/validation_patch_test.go b/pkg/apis/core/validation/validation_patch_test.go
@@ -17,6 +17,7 @@ limitations under the License.
 package validation
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -52,45 +53,46 @@ func TestOpenShiftValidateSecretUpdate(t *testing.T) {
 		}
 	}
 
-	t.Run("verify whitelist", func(t *testing.T) {
+	for _, secretType := range []core.SecretType{"", "SecretTypeTLS", core.SecretTypeOpaque} {
 		for key := range whitelist {
 			ns, name := key, "foo"
-
-			// exercise a valid type mutation: "SecretTypeTLS" -> "kubernetes.io/tls"
-			oldSecret, newSecret := newSecretFn(ns, name, "SecretTypeTLS"), newSecretFn(ns, name, core.SecretTypeTLS)
-			if errs := ValidateSecretUpdate(newSecret, oldSecret); len(errs) > 0 {
-				t.Errorf("unexpected error: %v", errs)
-			}
-
-			// the reverse should not be allowed
-			errExpected := invalidTypeErrFn("SecretTypeTLS")
-			oldSecret, newSecret = newSecretFn(ns, name, core.SecretTypeTLS), newSecretFn(ns, name, "SecretTypeTLS")
-			if errGot := ValidateSecretUpdate(newSecret, oldSecret); !cmp.Equal(errExpected, errGot) {
-				t.Errorf("expected error: %v, diff: %s", errExpected, cmp.Diff(errExpected, errGot))
-			}
-
-			// no type change, no validation failure expected
-			oldSecret, newSecret = newSecretFn(ns, name, core.SecretTypeTLS), newSecretFn(ns, name, core.SecretTypeTLS)
-			if errs := ValidateSecretUpdate(newSecret, oldSecret); len(errs) > 0 {
-				t.Errorf("unexpected error: %v", errs)
-			}
-
-			// exercise an invalid type mutation, we expect validation failure
-			errExpected = invalidTypeErrFn(core.SecretTypeOpaque)
-			oldSecret, newSecret = newSecretFn(ns, name, "SecretTypeTLS"), newSecretFn(ns, name, core.SecretTypeOpaque)
-			if errGot := ValidateSecretUpdate(newSecret, oldSecret); !cmp.Equal(errExpected, errGot) {
-				t.Errorf("expected error: %v, diff: %s", errExpected, cmp.Diff(errExpected, errGot))
-			}
-
-			// verify that kbernetes.io/tls validation ar enforced
-			errExpected = tlsKeyRequiredErrFn()
-			oldSecret, newSecret = newSecretFn(ns, name, "SecretTypeTLS"), newSecretFn(ns, name, core.SecretTypeTLS)
-			newSecret.Data = nil
-			if errGot := ValidateSecretUpdate(newSecret, oldSecret); !cmp.Equal(errExpected, errGot) {
-				t.Errorf("expected error: %v, diff: %s", errExpected, cmp.Diff(errExpected, errGot))
-			}
+			t.Run(fmt.Sprintf("verify whitelist, key = %v, secretType = %v", key, secretType), func(t *testing.T) {
+				// exercise a valid type mutation: "secretType" -> "kubernetes.io/tls"
+				oldSecret, newSecret := newSecretFn(ns, name, secretType), newSecretFn(ns, name, core.SecretTypeTLS)
+				if errs := ValidateSecretUpdate(newSecret, oldSecret); len(errs) > 0 {
+					t.Errorf("unexpected error: %v", errs)
+				}
+
+				// the reverse should not be allowed
+				errExpected := invalidTypeErrFn(secretType)
+				oldSecret, newSecret = newSecretFn(ns, name, core.SecretTypeTLS), newSecretFn(ns, name, secretType)
+				if errGot := ValidateSecretUpdate(newSecret, oldSecret); !cmp.Equal(errExpected, errGot) {
+					t.Errorf("expected error: %v, diff: %s", errExpected, cmp.Diff(errExpected, errGot))
+				}
+
+				// no type change, no validation failure expected
+				oldSecret, newSecret = newSecretFn(ns, name, core.SecretTypeTLS), newSecretFn(ns, name, core.SecretTypeTLS)
+				if errs := ValidateSecretUpdate(newSecret, oldSecret); len(errs) > 0 {
+					t.Errorf("unexpected error: %v", errs)
+				}
+
+				// exercise an invalid type mutation, we expect validation failure
+				errExpected = invalidTypeErrFn(secretType)
+				oldSecret, newSecret = newSecretFn(ns, name, "AnyType"), newSecretFn(ns, name, secretType)
+				if errGot := ValidateSecretUpdate(newSecret, oldSecret); !cmp.Equal(errExpected, errGot) {
+					t.Errorf("expected error: %v, diff: %s", errExpected, cmp.Diff(errExpected, errGot))
+				}
+
+				// verify that kbernetes.io/tls validation ar enforced
+				errExpected = tlsKeyRequiredErrFn()
+				oldSecret, newSecret = newSecretFn(ns, name, secretType), newSecretFn(ns, name, core.SecretTypeTLS)
+				newSecret.Data = nil
+				if errGot := ValidateSecretUpdate(newSecret, oldSecret); !cmp.Equal(errExpected, errGot) {
+					t.Errorf("expected error: %v, diff: %s", errExpected, cmp.Diff(errExpected, errGot))
+				}
+			})
 		}
-	})
+	}
 
 	// we must not break secrets that are not in the whitelist
 	tests := []struct {