OCPBUGS-30304: [release-4.15] certrotation: update all secret types to kubernetes.io/tls preserving existing content

pkg/operator/certrotation/annotations.go

@@ -5,30 +5,34 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-func NewTLSArtifactObjectMeta(name, namespace, jiraComponent, description string) metav1.ObjectMeta {
-	return metav1.ObjectMeta{
-		Namespace: namespace,
-		Name:      name,
-		Annotations: map[string]string{
-			annotations.OpenShiftComponent:   jiraComponent,
-			annotations.OpenShiftDescription: description,
-		},
-	}
+type AdditionalAnnotations struct {
+	// JiraComponent annotates tls artifacts so that owner could be easily found
+	JiraComponent string
+	// Description is a human-readable one sentence description of certificate purpose
+	Description string
 }
 
-// EnsureTLSMetadataUpdate mutates objectMeta setting necessary annotations if unset
-func EnsureTLSMetadataUpdate(meta *metav1.ObjectMeta, jiraComponent, description string) bool {
+func (a AdditionalAnnotations) EnsureTLSMetadataUpdate(meta *metav1.ObjectMeta) bool {
 	modified := false
 	if meta.Annotations == nil {
 		meta.Annotations = make(map[string]string)
 	}
-	if len(jiraComponent) > 0 && meta.Annotations[annotations.OpenShiftComponent] != jiraComponent {
-		meta.Annotations[annotations.OpenShiftComponent] = jiraComponent
+	if len(a.JiraComponent) > 0 && meta.Annotations[annotations.OpenShiftComponent] != a.JiraComponent {
+		meta.Annotations[annotations.OpenShiftComponent] = a.JiraComponent
 		modified = true
 	}
-	if len(description) > 0 && meta.Annotations[annotations.OpenShiftDescription] != description {
-		meta.Annotations[annotations.OpenShiftDescription] = description
+	if len(a.Description) > 0 && meta.Annotations[annotations.OpenShiftDescription] != a.Description {
+		meta.Annotations[annotations.OpenShiftDescription] = a.Description
 		modified = true
 	}
 	return modified
 }
+
+func NewTLSArtifactObjectMeta(name, namespace string, annotations AdditionalAnnotations) metav1.ObjectMeta {
+	meta := metav1.ObjectMeta{
+		Namespace: namespace,
+		Name:      name,
+	}
+	_ = annotations.EnsureTLSMetadataUpdate(&meta)
+	return meta
+}

pkg/operator/certrotation/cabundle.go

@@ -30,11 +30,8 @@ type CABundleConfigMap struct {
 	Name string
 	// Owner is an optional reference to add to the secret that this rotator creates.
 	Owner *metav1.OwnerReference
-	// JiraComponent annotates tls artifacts so that owner could be easily found
-	JiraComponent string
-	// Description is a human-readable one sentence description of certificate purpose
-	Description string
-
+	// AdditionalAnnotations is a collection of annotations set for the secret
+	AdditionalAnnotations AdditionalAnnotations
 	// Plumbing:
 	Informer      corev1informers.ConfigMapInformer
 	Lister        corev1listers.ConfigMapLister
@@ -55,18 +52,15 @@ func (c CABundleConfigMap) ensureConfigMapCABundle(ctx context.Context, signingC
 		caBundleConfigMap = &corev1.ConfigMap{ObjectMeta: NewTLSArtifactObjectMeta(
 			c.Name,
 			c.Namespace,
-			c.JiraComponent,
-			c.Description,
+			c.AdditionalAnnotations,
 		)}
 	}
 
 	needsMetadataUpdate := false
 	if c.Owner != nil {
 		needsMetadataUpdate = ensureOwnerReference(&caBundleConfigMap.ObjectMeta, c.Owner)
 	}
-	if len(c.JiraComponent) > 0 || len(c.Description) > 0 {
-		needsMetadataUpdate = EnsureTLSMetadataUpdate(&caBundleConfigMap.ObjectMeta, c.JiraComponent, c.Description) || needsMetadataUpdate
-	}
+	needsMetadataUpdate = c.AdditionalAnnotations.EnsureTLSMetadataUpdate(&caBundleConfigMap.ObjectMeta) || needsMetadataUpdate
 	if needsMetadataUpdate && len(caBundleConfigMap.ResourceVersion) > 0 {
 		_, _, err := resourceapply.ApplyConfigMap(ctx, c.Client, c.EventRecorder, caBundleConfigMap)
 		if err != nil {

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

@@ -44,11 +44,9 @@ type RotatedSigningCASecret struct {
 	// is used, early deletion will be catastrophic.
 	Owner *metav1.OwnerReference
 
-	// JiraComponent annotates tls artifacts so that owner could be easily found
-	JiraComponent string
+	// AdditionalAnnotations is a collection of annotations set for the secret
+	AdditionalAnnotations AdditionalAnnotations
 
-	// Description is a human-readable one sentence description of certificate purpose
-	Description string
 	// Plumbing:
 	Informer      corev1informers.SecretInformer
 	Lister        corev1listers.SecretLister
@@ -64,27 +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.JiraComponent,
-			c.Description,
-		)}
+		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)
-	}
-	if len(c.JiraComponent) > 0 || len(c.Description) > 0 {
-		needsMetadataUpdate = EnsureTLSMetadataUpdate(&signingCertKeyPairSecret.ObjectMeta, c.JiraComponent, c.Description) || 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())