OCPBUGS-31384: UPSTREAM: <carry>: allow type mutation for specific secrets #1924

tkashem · 2024-03-29T20:28:02Z

some of our operators were accidentally creating secrets of type "SecretTypeTLS", and this patch enables us to move these secrest objects to the intended type in a ratcheting manner
we can drop this patch when we migrate all of the affected secret objects to to intended type, see https://issues.redhat.com/browse/API-1800

openshift-ci-robot · 2024-03-29T20:28:13Z

@tkashem: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

cd301d1|UPSTREAM: : allow type mutation for specific secrets: does not specify an upstream backport in the commit message

Comment /validate-backports to re-evaluate validity of the upstream PRs, for example when they are merged upstream.

openshift-ci-robot · 2024-03-29T20:57:27Z

@tkashem: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

446f2ee|UPSTREAM: : allow type mutation for specific secrets: does not specify an upstream backport in the commit message

Comment /validate-backports to re-evaluate validity of the upstream PRs, for example when they are merged upstream.

tkashem · 2024-03-30T13:21:39Z

/test e2e-aws-ovn-serial
/test verify

tkashem · 2024-03-30T14:40:03Z

/retest

openshift-ci-robot · 2024-04-01T02:56:25Z

@tkashem: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

41a5b72|UPSTREAM: : allow type mutation for specific secrets: does not specify an upstream backport in the commit message

Comment /validate-backports to re-evaluate validity of the upstream PRs, for example when they are merged upstream.

openshift-ci-robot · 2024-04-01T12:24:50Z

@tkashem: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

db872ce|UPSTREAM: : allow type mutation for specific secrets: does not specify an upstream backport in the commit message

Comment /validate-backports to re-evaluate validity of the upstream PRs, for example when they are merged upstream.

benluddy · 2024-04-01T14:45:04Z

pkg/apis/core/validation/validation.go

+	// TODO: this is a short term fix, the long term goal is to improve the
+	// cert rotation logic that is not reliant on this hack.


Should explicitly say that the criteria for dropping the patch is migrating all of the affected secret types.

benluddy · 2024-04-01T14:46:36Z

pkg/apis/core/validation/validation_patch.go

+)
+
+func OpenShiftValidateSecretUpdateIsTypeMutationAllowed(newSecret, oldSecret *core.Secret) bool {
+	// we allow "SecretTypeTLS" -> "kubernetes.io/tls" only


Explain that some of our operators were accidentally creating secrets with type SecretTypeTLS and this patch is to allow ratcheting them to the intended type.

benluddy · 2024-04-01T14:47:29Z

pkg/apis/core/validation/validation_patch.go

+func OpenShiftValidateSecretUpdateIsTypeMutationAllowed(newSecret, oldSecret *core.Secret) bool {
+	// we allow "SecretTypeTLS" -> "kubernetes.io/tls" only
+	if oldSecret.Type == "SecretTypeTLS" && newSecret.Type == core.SecretTypeTLS {
+		key := fmt.Sprintf("%s/%s", oldSecret.Namespace, oldSecret.Name)


Nit: Could use https://pkg.go.dev/k8s.io/apimachinery/pkg/types#NamespacedName to avoid the extra string allocation per secret update.

benluddy · 2024-04-01T14:54:31Z

pkg/apis/core/validation/validation_patch_test.go

+			ns, name := split[0], split[1]
+
+			// exercise a valid type mutation: "SecretTypeTLS" -> "kubernetes.io/tls"
+			oldSecret, newSecret := newSecretFn(ns, name, "SecretTypeTLS"), newSecretFn(ns, name, core.SecretTypeTLS)


Please add the reverse direction too.

pkg/apis/core/validation/validation_patch_test.go

benluddy · 2024-04-01T15:05:54Z

pkg/apis/core/validation/validation_patch_test.go

+			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))
+			}


I don't doubt that it works but I would like to see a test that shows the kubernetes.io/tls key validation is still enforced on these updates.

pkg/apis/core/validation/validation_patch.go

openshift-ci-robot · 2024-04-01T17:20:34Z

@tkashem: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

b7d80e8|UPSTREAM: : allow type mutation for specific secrets: does not specify an upstream backport in the commit message

Comment /validate-backports to re-evaluate validity of the upstream PRs, for example when they are merged upstream.

tkashem · 2024-04-01T21:40:21Z

/retest-required

tkashem · 2024-04-02T01:10:22Z

/retest-required

p0lyn0mial · 2024-04-02T07:18:55Z

pkg/apis/core/validation/validation_patch.go

+	//
+	// we can drop this patch when we migrate all of the affected secret
+	// objects to to intended type: https://issues.redhat.com/browse/API-1800
+	whitelist = map[apimachinerytypes.NamespacedName]struct{}{


The list grew quickly. I think it contains all the secrets reconciled by concurrently running certificate controllers.

p0lyn0mial · 2024-04-02T07:23:08Z

pkg/apis/core/validation/validation_patch.go

+)
+
+var (
+	// we make an exception for the following secret objects, during update


Maybe it's worth explaining why we are allowing for this exception. We have multiple controllers reconciling the same object, resulting in unexpected outcomes such as the generation of new key pairs. Our goal is to prevent the generation of new key pairs by disallowing deletions and permitting only updates, which appear to be 'safe'.

p0lyn0mial · 2024-04-02T07:25:51Z

pkg/apis/core/validation/validation_patch.go

+	// we allow "SecretTypeTLS" -> "kubernetes.io/tls" only
+	if oldSecret.Type == "SecretTypeTLS" && newSecret.Type == core.SecretTypeTLS {
+		key := apimachinerytypes.NamespacedName{Namespace: oldSecret.Namespace, Name: oldSecret.Name}
+		if _, ok := whitelist[key]; ok {


I think you this could be changed to if whitelist[key] { return true }

p0lyn0mial · 2024-04-02T07:35:11Z

pkg/apis/core/validation/validation.go

@@ -6407,7 +6407,12 @@ func ValidateSecret(secret *core.Secret) field.ErrorList {
 func ValidateSecretUpdate(newSecret, oldSecret *core.Secret) field.ErrorList {
 	allErrs := ValidateObjectMetaUpdate(&newSecret.ObjectMeta, &oldSecret.ObjectMeta, field.NewPath("metadata"))

-	allErrs = append(allErrs, ValidateImmutableField(newSecret.Type, oldSecret.Type, field.NewPath("type"))...)
+	// TODO: this is a short term fix, we can drop this patch once we


shouldn't we change the commit title to UPSTREAM: <drop>: allow type mutation for specific secrets ?

p0lyn0mial · 2024-04-02T07:36:31Z

pkg/apis/core/validation/validation.go

+	// TODO: this is a short term fix, we can drop this patch once we
+	// migrate all of the affected secret objects to to intended type,
+	// see https://issues.redhat.com/browse/API-1800
+	if !OpenShiftValidateSecretUpdateIsTypeMutationAllowed(newSecret, oldSecret) {


I'm wondering how we could e2e validate that the type change is allowed ?

I think the easiest way would be to have an integration test. Let me write one.

p0lyn0mial@9bb0876

p0lyn0mial · 2024-04-02T08:25:38Z

pkg/apis/core/validation/validation_patch.go

+		apimachinerytypes.NamespacedName{Namespace: "openshift-kube-apiserver-operator", Name: "localhost-recovery-serving-signer"}: {},
+		apimachinerytypes.NamespacedName{Namespace: "openshift-kube-apiserver-operator", Name: "kube-control-plane-signer"}:         {},
+		apimachinerytypes.NamespacedName{Namespace: "openshift-kube-apiserver-operator", Name: "node-system-admin-signer"}:          {},
+		apimachinerytypes.NamespacedName{Namespace: "openshift-etcd-operator", Name: "etcd-client"}:                                 {},


are secrets in openshift-etcd-operator and openshift-kube-controller-manager-operator also reconciled concurrently ? How are we going to ensure these secrets can be transitioned to the new type ?

shouldn't we change etcd-signer instead ? (https://github.com/openshift/cluster-etcd-operator/blob/eeef8033eb058b0f9899c7b16aead237d5b84462/pkg/operator/etcdcertsigner/etcdcertsignercontroller.go#L36)

p0lyn0mial · 2024-04-02T09:08:49Z

pkg/apis/core/validation/validation_patch.go

+	}
+)
+
+func OpenShiftValidateSecretUpdateIsTypeMutationAllowed(newSecret, oldSecret *core.Secret) bool {


we could make it a private function.

openshift-ci-robot · 2024-04-02T12:53:33Z

@tkashem: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

b5d8bec|UPSTREAM: : e2e validate if type mutation is allowed for specific secrets: does not specify an upstream backport in the commit message
b7d80e8|UPSTREAM: : allow type mutation for specific secrets: does not specify an upstream backport in the commit message

Comment /validate-backports to re-evaluate validity of the upstream PRs, for example when they are merged upstream.

This is a short term fix, once we improve the cert rotation logic in library-go that does not depend on this hack, then we can remove this carry patch. squash with the previous PR during the rebase openshift#1924 squash with the previous PRs during the rebase openshift#1924 openshift#1929

openshift-ci-robot added the backports/unvalidated-commits Indicates that not all commits come to merged upstream PRs. label Mar 29, 2024

tkashem changed the title ~~UPSTREAM: <carry>: allow type mutation for specific secrets~~ [WIP] UPSTREAM: <carry>: allow type mutation for specific secrets Mar 29, 2024

openshift-ci bot requested review from deads2k and mfojtik March 29, 2024 20:28

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 29, 2024

tkashem force-pushed the secret-hack branch from cd301d1 to 446f2ee Compare March 29, 2024 20:57

tkashem force-pushed the secret-hack branch from 446f2ee to 41a5b72 Compare April 1, 2024 02:56

tkashem force-pushed the secret-hack branch from 41a5b72 to db872ce Compare April 1, 2024 12:24

benluddy reviewed Apr 1, 2024

View reviewed changes

tkashem force-pushed the secret-hack branch from db872ce to b7d80e8 Compare April 1, 2024 17:20

tkashem changed the title ~~[WIP] UPSTREAM: <carry>: allow type mutation for specific secrets~~ UPSTREAM: <carry>: allow type mutation for specific secrets Apr 1, 2024

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 1, 2024

p0lyn0mial reviewed Apr 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-31384: UPSTREAM: <carry>: allow type mutation for specific secrets #1924

OCPBUGS-31384: UPSTREAM: <carry>: allow type mutation for specific secrets #1924

tkashem commented Mar 29, 2024 •

edited

Loading

openshift-ci-robot commented Mar 29, 2024

openshift-ci-robot commented Mar 29, 2024

tkashem commented Mar 30, 2024

tkashem commented Mar 30, 2024

openshift-ci-robot commented Apr 1, 2024

openshift-ci-robot commented Apr 1, 2024

benluddy Apr 1, 2024

benluddy Apr 1, 2024

benluddy Apr 1, 2024

benluddy Apr 1, 2024

benluddy Apr 1, 2024

openshift-ci-robot commented Apr 1, 2024

tkashem commented Apr 1, 2024

tkashem commented Apr 2, 2024

p0lyn0mial Apr 2, 2024

p0lyn0mial Apr 2, 2024

p0lyn0mial Apr 2, 2024

p0lyn0mial Apr 2, 2024

p0lyn0mial Apr 2, 2024

p0lyn0mial Apr 2, 2024

p0lyn0mial Apr 2, 2024 •

edited

Loading

p0lyn0mial Apr 2, 2024

p0lyn0mial Apr 2, 2024

p0lyn0mial Apr 2, 2024

openshift-ci-robot commented Apr 2, 2024

		// TODO: this is a short term fix, the long term goal is to improve the
		// cert rotation logic that is not reliant on this hack.

OCPBUGS-31384: UPSTREAM: <carry>: allow type mutation for specific secrets #1924

OCPBUGS-31384: UPSTREAM: <carry>: allow type mutation for specific secrets #1924

Conversation

tkashem commented Mar 29, 2024 • edited Loading

openshift-ci-robot commented Mar 29, 2024

openshift-ci-robot commented Mar 29, 2024

tkashem commented Mar 30, 2024

tkashem commented Mar 30, 2024

openshift-ci-robot commented Apr 1, 2024

openshift-ci-robot commented Apr 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Apr 1, 2024

tkashem commented Apr 1, 2024

tkashem commented Apr 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Apr 2, 2024

tkashem commented Mar 29, 2024 •

edited

Loading

p0lyn0mial Apr 2, 2024 •

edited

Loading