OCPBUGS-30119: certrotation: update all secret types to kubernetes.io/tls preserving existing content

pkg/operator/certrotation/metadata.go

@@ -0,0 +1,36 @@
+package certrotation
+
+import (
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func ensureMetadataUpdate(secret *corev1.Secret, owner *metav1.OwnerReference, additionalAnnotations AdditionalAnnotations) bool {
+	needsMetadataUpdate := false
+	// no ownerReference set
+	if owner != nil {
+		needsMetadataUpdate = ensureOwnerReference(&secret.ObjectMeta, owner)
+	}
+	// ownership annotations not set
+	return additionalAnnotations.EnsureTLSMetadataUpdate(&secret.ObjectMeta) || needsMetadataUpdate
+}
+
+func ensureSecretTLSTypeSet(secret *corev1.Secret) bool {
+	// Existing secret not found - no need to update metadata (will be done by needNewSigningCertKeyPair / NeedNewTargetCertKeyPair)
+	if len(secret.ResourceVersion) == 0 {
+		return false
+	}
+
+	// convert outdated secret type (created by pre 4.7 installer)
+	if secret.Type != corev1.SecretTypeTLS {
+		secret.Type = corev1.SecretTypeTLS
+		// wipe secret contents if tls.crt and tls.key are missing
+		_, certExists := secret.Data[corev1.TLSCertKey]
+		_, keyExists := secret.Data[corev1.TLSPrivateKeyKey]
+		if !certExists || !keyExists {
+			secret.Data = map[string][]byte{}
+		}
+		return true
+	}
+	return false
+}

pkg/operator/certrotation/signer.go

@@ -62,24 +62,24 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (*
 	signingCertKeyPairSecret := originalSigningCertKeyPairSecret.DeepCopy()
 	if apierrors.IsNotFound(err) {
 		// create an empty one
-		signingCertKeyPairSecret = &corev1.Secret{ObjectMeta: NewTLSArtifactObjectMeta(
-			c.Name,
-			c.Namespace,
-			c.AdditionalAnnotations,
-		)}
+		signingCertKeyPairSecret = &corev1.Secret{
+			ObjectMeta: NewTLSArtifactObjectMeta(
+				c.Name,
+				c.Namespace,
+				c.AdditionalAnnotations,
+			),
+			Type: corev1.SecretTypeTLS,
+		}
 	}
-	signingCertKeyPairSecret.Type = corev1.SecretTypeTLS
 
-	needsMetadataUpdate := false
-	if c.Owner != nil {
-		needsMetadataUpdate = ensureOwnerReference(&signingCertKeyPairSecret.ObjectMeta, c.Owner)
-	}
-	needsMetadataUpdate = c.AdditionalAnnotations.EnsureTLSMetadataUpdate(&signingCertKeyPairSecret.ObjectMeta) || needsMetadataUpdate
-	if needsMetadataUpdate && len(signingCertKeyPairSecret.ResourceVersion) > 0 {
-		_, _, err := resourceapply.ApplySecret(ctx, c.Client, c.EventRecorder, signingCertKeyPairSecret)
+	// apply necessary metadata (possibly via delete+recreate) if secret exists
+	// this is done before content update to prevent unexpected rollouts
+	if ensureMetadataUpdate(signingCertKeyPairSecret, c.Owner, c.AdditionalAnnotations) && ensureSecretTLSTypeSet(signingCertKeyPairSecret) {
+		actualSigningCertKeyPairSecret, _, err := resourceapply.ApplySecret(ctx, c.Client, c.EventRecorder, signingCertKeyPairSecret)
 		if err != nil {
 			return nil, err
 		}
+		signingCertKeyPairSecret = actualSigningCertKeyPairSecret
 	}
 
 	if needed, reason := needNewSigningCertKeyPair(signingCertKeyPairSecret.Annotations, c.Refresh, c.RefreshOnlyWhenExpired); needed {

pkg/operator/certrotation/signer_test.go

@@ -15,6 +15,7 @@ import (
 	clienttesting "k8s.io/client-go/testing"
 	"k8s.io/client-go/tools/cache"
 
+	"github.com/openshift/api/annotations"
 	"github.com/openshift/library-go/pkg/operator/events"
 )
 
@@ -50,13 +51,30 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
 				if len(actual.Data["tls.crt"]) == 0 || len(actual.Data["tls.key"]) == 0 {
 					t.Error(actual.Data)
 				}
+				if len(actual.Annotations) == 0 {
+					t.Errorf("expected certificates to be annotated")
+				}
+				ownershipValue, found := actual.Annotations[annotations.OpenShiftComponent]
+				if !found {
+					t.Errorf("expected secret to have ownership annotations, got: %v", actual.Annotations)
+				}
+				if ownershipValue != "test" {
+					t.Errorf("expected ownership annotation to be 'test', got: %v", ownershipValue)
+				}
+				if len(actual.OwnerReferences) != 1 {
+					t.Errorf("expected to have exactly one owner reference")
+				}
+				if actual.OwnerReferences[0].Name != "operator" {
+					t.Errorf("expected owner reference to be 'operator', got %v", actual.OwnerReferences[0].Name)
+				}
 			},
 		},
 		{
 			name: "update no annotations",
 			initialSecret: &corev1.Secret{
-				ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "signer"},
+				ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "signer", ResourceVersion: "10"},
 				Type:       corev1.SecretTypeTLS,
+				Data:       map[string][]byte{"tls.crt": {}, "tls.key": {}},
 			},
 			verifyActions: func(t *testing.T, client *kubefake.Clientset) {
 				t.Helper()
@@ -65,27 +83,46 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
 					t.Fatal(spew.Sdump(actions))
 				}
 
+				if !actions[0].Matches("get", "secrets") {
+					t.Error(actions[0])
+				}
 				if !actions[1].Matches("update", "secrets") {
 					t.Error(actions[1])
 				}
-
 				actual := actions[1].(clienttesting.UpdateAction).GetObject().(*corev1.Secret)
 				if certType, _ := CertificateTypeFromObject(actual); certType != CertificateTypeSigner {
 					t.Errorf("expected certificate type 'signer', got: %v", certType)
 				}
 				if len(actual.Data["tls.crt"]) == 0 || len(actual.Data["tls.key"]) == 0 {
 					t.Error(actual.Data)
 				}
+				ownershipValue, found := actual.Annotations[annotations.OpenShiftComponent]
+				if !found {
+					t.Errorf("expected secret to have ownership annotations, got: %v", actual.Annotations)
+				}
+				if ownershipValue != "test" {
+					t.Errorf("expected ownership annotation to be 'test', got: %v", ownershipValue)
+				}
+				if len(actual.OwnerReferences) != 1 {
+					t.Errorf("expected to have exactly one owner reference")
+				}
+				if actual.OwnerReferences[0].Name != "operator" {
+					t.Errorf("expected owner reference to be 'operator', got %v", actual.OwnerReferences[0].Name)
+				}
 			},
 		},
 		{
 			name: "update no work",
 			initialSecret: &corev1.Secret{
 				ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "signer",
+					ResourceVersion: "10",
 					Annotations: map[string]string{
 						"auth.openshift.io/certificate-not-after":  "2108-09-08T22:47:31-07:00",
 						"auth.openshift.io/certificate-not-before": "2108-09-08T20:47:31-07:00",
+						annotations.OpenShiftComponent:             "test",
 					}},
+				Type: corev1.SecretTypeTLS,
+				Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
 			},
 			verifyActions: func(t *testing.T, client *kubefake.Clientset) {
 				t.Helper()
@@ -96,6 +133,102 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
 			},
 			expectedError: "certFile missing", // this means we tried to read the cert from the existing secret.  If we created one, we fail in the client check
 		},
+		{
+			name: "update SecretTLSType secrets",
+			initialSecret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "signer",
+					ResourceVersion: "10",
+					Annotations: map[string]string{
+						"auth.openshift.io/certificate-not-after":  "2108-09-08T22:47:31-07:00",
+						"auth.openshift.io/certificate-not-before": "2108-09-08T20:47:31-07:00",
+					}},
+				Type: "SecretTypeTLS",
+				Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
+			},
+			verifyActions: func(t *testing.T, client *kubefake.Clientset) {
+				t.Helper()
+				actions := client.Actions()
+				if len(actions) != 3 {
+					t.Fatal(spew.Sdump(actions))
+				}
+
+				if !actions[0].Matches("get", "secrets") {
+					t.Error(actions[0])
+				}
+				if !actions[1].Matches("delete", "secrets") {
+					t.Error(actions[1])
+				}
+				if !actions[2].Matches("create", "secrets") {
+					t.Error(actions[2])
+				}
+				actual := actions[2].(clienttesting.UpdateAction).GetObject().(*corev1.Secret)
+				if actual.Type != corev1.SecretTypeTLS {
+					t.Errorf("expected secret type to be kubernetes.io/tls, got: %v", actual.Type)
+				}
+				cert, found := actual.Data["tls.crt"]
+				if !found {
+					t.Errorf("expected to have tls.crt key")
+				}
+				if len(cert) != 0 {
+					t.Errorf("expected tls.crt to be empty, got %v", cert)
+				}
+				key, found := actual.Data["tls.key"]
+				if !found {
+					t.Errorf("expected to have tls.key key")
+				}
+				if len(key) != 0 {
+					t.Errorf("expected tls.key to be empty, got %v", key)
+				}
+				if len(actual.OwnerReferences) != 1 {
+					t.Errorf("expected to have exactly one owner reference")
+				}
+				if actual.OwnerReferences[0].Name != "operator" {
+					t.Errorf("expected owner reference to be 'operator', got %v", actual.OwnerReferences[0].Name)
+				}
+			},
+			expectedError: "certFile missing", // this means we tried to read the cert from the existing secret.  If we created one, we fail in the client check
+		},
+		{
+			name: "recreate invalid type secrets",
+			initialSecret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "signer",
+					ResourceVersion: "10",
+					Annotations: map[string]string{
+						"auth.openshift.io/certificate-not-after":  "2108-09-08T22:47:31-07:00",
+						"auth.openshift.io/certificate-not-before": "2108-09-08T20:47:31-07:00",
+					}},
+				Type: corev1.SecretTypeOpaque,
+				Data: map[string][]byte{"foo": {}, "bar": {}},
+			},
+			verifyActions: func(t *testing.T, client *kubefake.Clientset) {
+				t.Helper()
+				actions := client.Actions()
+				if len(actions) != 3 {
+					t.Fatal(spew.Sdump(actions))
+				}
+
+				if !actions[0].Matches("get", "secrets") {
+					t.Error(actions[0])
+				}
+				if !actions[1].Matches("delete", "secrets") {
+					t.Error(actions[1])
+				}
+				if !actions[2].Matches("create", "secrets") {
+					t.Error(actions[2])
+				}
+				actual := actions[2].(clienttesting.UpdateAction).GetObject().(*corev1.Secret)
+				if actual.Type != corev1.SecretTypeTLS {
+					t.Errorf("expected secret type to be kubernetes.io/tls, got: %v", actual.Type)
+				}
+				if len(actual.OwnerReferences) != 1 {
+					t.Errorf("expected to have exactly one owner reference")
+				}
+				if actual.OwnerReferences[0].Name != "operator" {
+					t.Errorf("expected owner reference to be 'operator', got %v", actual.OwnerReferences[0].Name)
+				}
+			},
+			expectedError: "certFile missing", // this means we tried to read the cert from the existing secret.  If we created one, we fail in the client check
+		},
 	}
 
 	for _, test := range tests {
@@ -116,6 +249,12 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
 				Client:        client.CoreV1(),
 				Lister:        corev1listers.NewSecretLister(indexer),
 				EventRecorder: events.NewInMemoryRecorder("test"),
+				AdditionalAnnotations: AdditionalAnnotations{
+					JiraComponent: "test",
+				},
+				Owner: &metav1.OwnerReference{
+					Name: "operator",
+				},
 			}
 
 			_, err := c.EnsureSigningCertKeyPair(context.TODO())

pkg/operator/certrotation/target.go

@@ -97,24 +97,24 @@ func (c RotatedSelfSignedCertKeySecret) EnsureTargetCertKeyPair(ctx context.Cont
 	targetCertKeyPairSecret := originalTargetCertKeyPairSecret.DeepCopy()
 	if apierrors.IsNotFound(err) {
 		// create an empty one
-		targetCertKeyPairSecret = &corev1.Secret{ObjectMeta: NewTLSArtifactObjectMeta(
-			c.Name,
-			c.Namespace,
-			c.AdditionalAnnotations,
-		)}
+		targetCertKeyPairSecret = &corev1.Secret{
+			ObjectMeta: NewTLSArtifactObjectMeta(
+				c.Name,
+				c.Namespace,
+				c.AdditionalAnnotations,
+			),
+			Type: corev1.SecretTypeTLS,
+		}
 	}
-	targetCertKeyPairSecret.Type = corev1.SecretTypeTLS
 
-	needsMetadataUpdate := false
-	if c.Owner != nil {
-		needsMetadataUpdate = ensureOwnerReference(&targetCertKeyPairSecret.ObjectMeta, c.Owner)
-	}
-	needsMetadataUpdate = c.AdditionalAnnotations.EnsureTLSMetadataUpdate(&targetCertKeyPairSecret.ObjectMeta) || needsMetadataUpdate
-	if needsMetadataUpdate && len(targetCertKeyPairSecret.ResourceVersion) > 0 {
-		_, _, err := resourceapply.ApplySecret(ctx, c.Client, c.EventRecorder, targetCertKeyPairSecret)
+	// apply necessary metadata (possibly via delete+recreate) if secret exists
+	// this is done before content update to prevent unexpected rollouts
+	if ensureMetadataUpdate(targetCertKeyPairSecret, c.Owner, c.AdditionalAnnotations) && ensureSecretTLSTypeSet(targetCertKeyPairSecret) {
+		actualTargetCertKeyPairSecret, _, err := resourceapply.ApplySecret(ctx, c.Client, c.EventRecorder, targetCertKeyPairSecret)
 		if err != nil {
 			return nil, err
 		}
+		targetCertKeyPairSecret = actualTargetCertKeyPairSecret
 	}
 
 	if reason := c.CertCreator.NeedNewTargetCertKeyPair(targetCertKeyPairSecret.Annotations, signingCertKeyPair, caBundleCerts, c.Refresh, c.RefreshOnlyWhenExpired); len(reason) > 0 {