diff --git a/pkg/operator/certrotation/metadata.go b/pkg/operator/certrotation/metadata.go new file mode 100644 index 0000000000..f64bde8fe0 --- /dev/null +++ b/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 +} diff --git a/pkg/operator/certrotation/signer.go b/pkg/operator/certrotation/signer.go index d143dc3056..59bf926d5a 100644 --- a/pkg/operator/certrotation/signer.go +++ b/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 { diff --git a/pkg/operator/certrotation/signer_test.go b/pkg/operator/certrotation/signer_test.go index f39e832e96..f58e8c41ba 100644 --- a/pkg/operator/certrotation/signer_test.go +++ b/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,10 +83,12 @@ 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) @@ -76,16 +96,33 @@ func TestEnsureSigningCertKeyPair(t *testing.T) { 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()) diff --git a/pkg/operator/certrotation/target.go b/pkg/operator/certrotation/target.go index ad1caa6379..f7e37f4c81 100644 --- a/pkg/operator/certrotation/target.go +++ b/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 { diff --git a/pkg/operator/certrotation/target_test.go b/pkg/operator/certrotation/target_test.go index 01428669a8..16ad7c3c25 100644 --- a/pkg/operator/certrotation/target_test.go +++ b/pkg/operator/certrotation/target_test.go @@ -9,6 +9,7 @@ import ( "github.com/davecgh/go-spew/spew" + "github.com/openshift/api/annotations" "github.com/openshift/library-go/pkg/crypto" "github.com/openshift/library-go/pkg/operator/events" @@ -161,9 +162,25 @@ func TestEnsureTargetCertKeyPair(t *testing.T) { } actual := actions[1].(clienttesting.CreateAction).GetObject().(*corev1.Secret) + 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.Data["tls.crt"]) == 0 || len(actual.Data["tls.key"]) == 0 { t.Error(actual.Data) } + 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) + } }, }, { @@ -173,7 +190,7 @@ func TestEnsureTargetCertKeyPair(t *testing.T) { }, initialSecretFn: func() *corev1.Secret { caBundleSecret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "target-secret"}, + ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "target-secret", ResourceVersion: "10"}, Data: map[string][]byte{}, Type: corev1.SecretTypeTLS, } @@ -190,13 +207,154 @@ func TestEnsureTargetCertKeyPair(t *testing.T) { } actual := actions[1].(clienttesting.UpdateAction).GetObject().(*corev1.Secret) + 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.Data["tls.crt"]) == 0 || len(actual.Data["tls.key"]) == 0 { t.Error(actual.Data) } if actual.Annotations[CertificateHostnames] != "bar,foo" { t.Error(actual.Annotations[CertificateHostnames]) } + 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 SecretTLSType secrets", + caFn: func() (*crypto.CA, error) { + return newTestCACertificate(pkix.Name{CommonName: "signer-tests"}, int64(1), metav1.Duration{Duration: time.Hour * 24 * 60}, time.Now) + }, + initialSecretFn: func() *corev1.Secret { + caBundleSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "target-secret", ResourceVersion: "10"}, + Data: map[string][]byte{}, + Type: "SecretTypeTLS", + } + return caBundleSecret + }, + verifyActions: func(t *testing.T, client *kubefake.Clientset) { + actions := client.Actions() + if len(actions) != 5 { + 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]) + } + if !actions[3].Matches("get", "secrets") { + t.Error(actions[3]) + } + if !actions[4].Matches("update", "secrets") { + t.Error(actions[4]) + } + actual := actions[4].(clienttesting.UpdateAction).GetObject().(*corev1.Secret) + 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 actual.Type != corev1.SecretTypeTLS { + t.Errorf("expected secret type to be kubernetes.io/tls, got: %v", actual.Type) + } + if len(actual.Data["tls.crt"]) == 0 || len(actual.Data["tls.key"]) == 0 { + t.Error(actual.Data) + } + if actual.Annotations[CertificateHostnames] != "bar,foo" { + t.Error(actual.Annotations[CertificateHostnames]) + } + 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: "recreate invalid secret type", + caFn: func() (*crypto.CA, error) { + return newTestCACertificate(pkix.Name{CommonName: "signer-tests"}, int64(1), metav1.Duration{Duration: time.Hour * 24 * 60}, time.Now) + }, + initialSecretFn: func() *corev1.Secret { + caBundleSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "target-secret", ResourceVersion: "10"}, + Type: corev1.SecretTypeOpaque, + Data: map[string][]byte{"foo": {}, "bar": {}}, + } + return caBundleSecret + }, + verifyActions: func(t *testing.T, client *kubefake.Clientset) { + actions := client.Actions() + if len(actions) != 5 { + 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]) + } + if !actions[3].Matches("get", "secrets") { + t.Error(actions[3]) + } + if !actions[4].Matches("update", "secrets") { + t.Error(actions[4]) + } + + actual := actions[4].(clienttesting.UpdateAction).GetObject().(*corev1.Secret) + 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 actual.Type != corev1.SecretTypeTLS { + t.Errorf("expected secret type to be kubernetes.io/tls, got: %v", actual.Type) + } + if len(actual.Data["tls.crt"]) == 0 || len(actual.Data["tls.key"]) == 0 { + t.Error(actual.Data) + } + if actual.Annotations[CertificateHostnames] != "bar,foo" { + t.Error(actual.Annotations[CertificateHostnames]) + } + 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) + } }, }, } @@ -223,6 +381,12 @@ func TestEnsureTargetCertKeyPair(t *testing.T) { Client: client.CoreV1(), Lister: corev1listers.NewSecretLister(indexer), EventRecorder: events.NewInMemoryRecorder("test"), + AdditionalAnnotations: AdditionalAnnotations{ + JiraComponent: "test", + }, + Owner: &metav1.OwnerReference{ + Name: "operator", + }, } newCA, err := test.caFn()