diff --git a/pkg/operator/certrotation/cabundle.go b/pkg/operator/certrotation/cabundle.go index bf8cd95a71..1cb4685b1f 100644 --- a/pkg/operator/certrotation/cabundle.go +++ b/pkg/operator/certrotation/cabundle.go @@ -21,7 +21,7 @@ import ( "github.com/openshift/library-go/pkg/certs" "github.com/openshift/library-go/pkg/crypto" "github.com/openshift/library-go/pkg/operator/events" - "github.com/openshift/library-go/pkg/operator/resource/resourceapply" + "github.com/openshift/library-go/pkg/operator/resource/resourcehelper" ) // CABundleConfigMap maintains a CA bundle config map, by adding new CA certs coming from RotatedSigningCASecret, and by removing expired old ones. @@ -44,7 +44,8 @@ type CABundleConfigMap struct { func (c CABundleConfigMap) EnsureConfigMapCABundle(ctx context.Context, signingCertKeyPair *crypto.CA, signingCertKeyPairLocation string) ([]*x509.Certificate, error) { // by this point we have current signing cert/key pair. We now need to make sure that the ca-bundle configmap has this cert and // doesn't have any expired certs - modified := false + updateRequired := false + creationRequired := false originalCABundleConfigMap, err := c.Lister.ConfigMaps(c.Namespace).Get(c.Name) if err != nil && !apierrors.IsNotFound(err) { @@ -58,7 +59,7 @@ func (c CABundleConfigMap) EnsureConfigMapCABundle(ctx context.Context, signingC c.Namespace, c.AdditionalAnnotations, )} - modified = true + creationRequired = true } needsOwnerUpdate := false @@ -66,7 +67,7 @@ func (c CABundleConfigMap) EnsureConfigMapCABundle(ctx context.Context, signingC needsOwnerUpdate = ensureOwnerReference(&caBundleConfigMap.ObjectMeta, c.Owner) } needsMetadataUpdate := c.AdditionalAnnotations.EnsureTLSMetadataUpdate(&caBundleConfigMap.ObjectMeta) - modified = needsOwnerUpdate || needsMetadataUpdate || modified + updateRequired = needsOwnerUpdate || needsMetadataUpdate updatedCerts, err := manageCABundleConfigMap(caBundleConfigMap, signingCertKeyPair.Config.Certs[0]) if err != nil { @@ -74,7 +75,7 @@ func (c CABundleConfigMap) EnsureConfigMapCABundle(ctx context.Context, signingC } if originalCABundleConfigMap == nil || originalCABundleConfigMap.Data == nil || !equality.Semantic.DeepEqual(originalCABundleConfigMap.Data, caBundleConfigMap.Data) { reason := "" - if originalCABundleConfigMap == nil { + if creationRequired { reason = "configmap doesn't exist" } else if originalCABundleConfigMap.Data == nil { reason = "configmap is empty" @@ -84,18 +85,24 @@ func (c CABundleConfigMap) EnsureConfigMapCABundle(ctx context.Context, signingC c.EventRecorder.Eventf("CABundleUpdateRequired", "%q in %q requires a new cert: %s", c.Name, c.Namespace, reason) LabelAsManagedConfigMap(caBundleConfigMap, CertificateTypeCABundle) - modified = true + updateRequired = true } - if modified { - - actualCABundleConfigMap, updated, err := resourceapply.ApplyConfigMap(ctx, c.Client, c.EventRecorder, caBundleConfigMap) + if creationRequired { + actualCABundleConfigMap, err := c.Client.ConfigMaps(c.Namespace).Create(ctx, caBundleConfigMap, metav1.CreateOptions{}) + resourcehelper.ReportCreateEvent(c.EventRecorder, actualCABundleConfigMap, err) if err != nil { return nil, err } - if updated { - klog.V(2).Infof("Updated ca-bundle.crt configmap %s/%s with:\n%s", certs.CertificateBundleToString(updatedCerts), caBundleConfigMap.Namespace, caBundleConfigMap.Name) + klog.V(2).Infof("Created ca-bundle.crt configmap %s/%s with:\n%s", certs.CertificateBundleToString(updatedCerts), caBundleConfigMap.Namespace, caBundleConfigMap.Name) + caBundleConfigMap = actualCABundleConfigMap + } else if updateRequired { + actualCABundleConfigMap, err := c.Client.ConfigMaps(c.Namespace).Update(ctx, caBundleConfigMap, metav1.UpdateOptions{}) + resourcehelper.ReportUpdateEvent(c.EventRecorder, actualCABundleConfigMap, err) + if err != nil { + return nil, err } + klog.V(2).Infof("Updated ca-bundle.crt configmap %s/%s with:\n%s", certs.CertificateBundleToString(updatedCerts), caBundleConfigMap.Namespace, caBundleConfigMap.Name) caBundleConfigMap = actualCABundleConfigMap } diff --git a/pkg/operator/certrotation/cabundle_test.go b/pkg/operator/certrotation/cabundle_test.go index 10d7c2aedb..e03e708f6c 100644 --- a/pkg/operator/certrotation/cabundle_test.go +++ b/pkg/operator/certrotation/cabundle_test.go @@ -46,18 +46,14 @@ func TestEnsureConfigMapCABundle(t *testing.T) { initialConfigMapFn: func() *corev1.ConfigMap { return nil }, verifyActions: func(t *testing.T, client *kubefake.Clientset) { actions := client.Actions() - if len(actions) != 2 { + if len(actions) != 1 { t.Fatal(spew.Sdump(actions)) } - - if !actions[0].Matches("get", "configmaps") { + if !actions[0].Matches("create", "configmaps") { t.Error(actions[0]) } - if !actions[1].Matches("create", "configmaps") { - t.Error(actions[1]) - } - actual := actions[1].(clienttesting.CreateAction).GetObject().(*corev1.ConfigMap) + actual := actions[0].(clienttesting.CreateAction).GetObject().(*corev1.ConfigMap) if certType, _ := CertificateTypeFromObject(actual); certType != CertificateTypeCABundle { t.Errorf("expected certificate type 'ca-bundle', got: %v", certType) } @@ -73,7 +69,7 @@ func TestEnsureConfigMapCABundle(t *testing.T) { }, initialConfigMapFn: func() *corev1.ConfigMap { caBundleConfigMap := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "trust-bundle"}, + ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "trust-bundle", ResourceVersion: "10"}, Data: map[string]string{}, } certs, err := newTestCACertificate(pkix.Name{CommonName: "signer-tests"}, int64(1), metav1.Duration{Duration: time.Hour * 24 * 60}, time.Now) @@ -89,15 +85,15 @@ func TestEnsureConfigMapCABundle(t *testing.T) { }, verifyActions: func(t *testing.T, client *kubefake.Clientset) { actions := client.Actions() - if len(actions) != 2 { + if len(actions) != 1 { t.Fatal(spew.Sdump(actions)) } - if !actions[1].Matches("update", "configmaps") { - t.Error(actions[1]) + if !actions[0].Matches("update", "configmaps") { + t.Error(actions[0]) } - actual := actions[1].(clienttesting.UpdateAction).GetObject().(*corev1.ConfigMap) + actual := actions[0].(clienttesting.UpdateAction).GetObject().(*corev1.ConfigMap) if len(actual.Data["ca-bundle.crt"]) == 0 { t.Error(actual.Data) } @@ -120,7 +116,7 @@ func TestEnsureConfigMapCABundle(t *testing.T) { }, initialConfigMapFn: func() *corev1.ConfigMap { caBundleConfigMap := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "trust-bundle"}, + ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "trust-bundle", ResourceVersion: "10"}, Data: map[string]string{}, } certs, err := newTestCACertificate(pkix.Name{CommonName: "signer-tests"}, int64(1), metav1.Duration{Duration: time.Hour * 24 * 60}, time.Now) @@ -136,15 +132,15 @@ func TestEnsureConfigMapCABundle(t *testing.T) { }, verifyActions: func(t *testing.T, client *kubefake.Clientset) { actions := client.Actions() - if len(actions) != 2 { + if len(actions) != 1 { t.Fatal(spew.Sdump(actions)) } - if !actions[1].Matches("update", "configmaps") { - t.Error(actions[1]) + if !actions[0].Matches("update", "configmaps") { + t.Error(actions[0]) } - actual := actions[1].(clienttesting.UpdateAction).GetObject().(*corev1.ConfigMap) + actual := actions[0].(clienttesting.UpdateAction).GetObject().(*corev1.ConfigMap) if len(actual.Data["ca-bundle.crt"]) == 0 { t.Error(actual.Data) } @@ -167,7 +163,7 @@ func TestEnsureConfigMapCABundle(t *testing.T) { }, initialConfigMapFn: func() *corev1.ConfigMap { caBundleConfigMap := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "trust-bundle"}, + ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "trust-bundle", ResourceVersion: "10"}, Data: map[string]string{}, } certBytes, err := os.ReadFile("./testfiles/tls-expired.crt") @@ -187,15 +183,15 @@ func TestEnsureConfigMapCABundle(t *testing.T) { }, verifyActions: func(t *testing.T, client *kubefake.Clientset) { actions := client.Actions() - if len(actions) != 2 { + if len(actions) != 1 { t.Fatal(spew.Sdump(actions)) } - if !actions[1].Matches("update", "configmaps") { - t.Error(actions[1]) + if !actions[0].Matches("update", "configmaps") { + t.Error(actions[0]) } - actual := actions[1].(clienttesting.UpdateAction).GetObject().(*corev1.ConfigMap) + actual := actions[0].(clienttesting.UpdateAction).GetObject().(*corev1.ConfigMap) if len(actual.Data["ca-bundle.crt"]) == 0 { t.Error(actual.Data) } diff --git a/pkg/operator/certrotation/signer.go b/pkg/operator/certrotation/signer.go index caa78a6707..a593b1b333 100644 --- a/pkg/operator/certrotation/signer.go +++ b/pkg/operator/certrotation/signer.go @@ -8,13 +8,14 @@ import ( "github.com/openshift/library-go/pkg/crypto" "github.com/openshift/library-go/pkg/operator/events" - "github.com/openshift/library-go/pkg/operator/resource/resourceapply" + "github.com/openshift/library-go/pkg/operator/resource/resourcehelper" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" corev1informers "k8s.io/client-go/informers/core/v1" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" corev1listers "k8s.io/client-go/listers/core/v1" + "k8s.io/klog/v2" ) // RotatedSigningCASecret rotates a self-signed signing CA stored in a secret. It creates a new one when @@ -57,12 +58,12 @@ type RotatedSigningCASecret struct { // EnsureSigningCertKeyPair manages the entire lifecycle of a signer cert as a secret, from creation to continued rotation. // It always returns the currently used CA pair, a bool indicating whether it was created/updated within this function call and an error. func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (*crypto.CA, bool, error) { - modified := false + creationRequired := false + updateRequired := false originalSigningCertKeyPairSecret, err := c.Lister.Secrets(c.Namespace).Get(c.Name) if err != nil && !apierrors.IsNotFound(err) { return nil, false, err } - var signerExists = true signingCertKeyPairSecret := originalSigningCertKeyPairSecret.DeepCopy() if apierrors.IsNotFound(err) { // create an empty one @@ -74,18 +75,18 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (* ), Type: corev1.SecretTypeTLS, } - signerExists = false + creationRequired = true } // apply necessary metadata (possibly via delete+recreate) if secret exists // this is done before content update to prevent unexpected rollouts needsMetadataUpdate := ensureMetadataUpdate(signingCertKeyPairSecret, c.Owner, c.AdditionalAnnotations) needsTypeChange := ensureSecretTLSTypeSet(signingCertKeyPairSecret) - modified = needsMetadataUpdate || needsTypeChange || modified + updateRequired = needsMetadataUpdate || needsTypeChange signerUpdated := false - if needed, reason := needNewSigningCertKeyPair(signingCertKeyPairSecret, c.Refresh, c.RefreshOnlyWhenExpired); needed || !signerExists { - if !signerExists { + if needed, reason := needNewSigningCertKeyPair(signingCertKeyPairSecret, c.Refresh, c.RefreshOnlyWhenExpired); needed || creationRequired { + if creationRequired { reason = "secret doesn't exist" } c.EventRecorder.Eventf("SignerUpdateRequired", "%q in %q requires a new signing cert/key pair: %v", c.Name, c.Namespace, reason) @@ -95,15 +96,25 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (* LabelAsManagedSecret(signingCertKeyPairSecret, CertificateTypeSigner) - modified = true + updateRequired = true signerUpdated = true } - if modified { - actualSigningCertKeyPairSecret, _, err := resourceapply.ApplySecret(ctx, c.Client, c.EventRecorder, signingCertKeyPairSecret) + if creationRequired { + actualSigningCertKeyPairSecret, err := c.Client.Secrets(c.Namespace).Create(ctx, signingCertKeyPairSecret, metav1.CreateOptions{}) + resourcehelper.ReportCreateEvent(c.EventRecorder, actualSigningCertKeyPairSecret, err) if err != nil { return nil, false, err } + klog.V(2).Infof("Created secret %s/%s", actualSigningCertKeyPairSecret.Namespace, actualSigningCertKeyPairSecret.Name) + signingCertKeyPairSecret = actualSigningCertKeyPairSecret + } else if updateRequired { + actualSigningCertKeyPairSecret, err := c.Client.Secrets(c.Namespace).Update(ctx, signingCertKeyPairSecret, metav1.UpdateOptions{}) + resourcehelper.ReportUpdateEvent(c.EventRecorder, actualSigningCertKeyPairSecret, err) + if err != nil { + return nil, false, err + } + klog.V(2).Infof("Updated secret %s/%s", actualSigningCertKeyPairSecret.Namespace, actualSigningCertKeyPairSecret.Name) signingCertKeyPairSecret = actualSigningCertKeyPairSecret } diff --git a/pkg/operator/certrotation/signer_test.go b/pkg/operator/certrotation/signer_test.go index 385558081b..ce66a61768 100644 --- a/pkg/operator/certrotation/signer_test.go +++ b/pkg/operator/certrotation/signer_test.go @@ -25,26 +25,27 @@ func TestEnsureSigningCertKeyPair(t *testing.T) { initialSecret *corev1.Secret - verifyActions func(t *testing.T, client *kubefake.Clientset) + verifyActions func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool) expectedError string }{ { name: "initial create", - verifyActions: func(t *testing.T, client *kubefake.Clientset) { + verifyActions: func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool) { t.Helper() actions := client.Actions() - if len(actions) != 2 { + if len(actions) != 1 { t.Fatal(spew.Sdump(actions)) } - if !actions[0].Matches("get", "secrets") { - t.Error(actions[0]) + if !controllerUpdatedSecret { + t.Errorf("expected controller to update secret") } - if !actions[1].Matches("create", "secrets") { - t.Error(actions[1]) + + if !actions[0].Matches("create", "secrets") { + t.Error(actions[0]) } - actual := actions[1].(clienttesting.CreateAction).GetObject().(*corev1.Secret) + actual := actions[0].(clienttesting.CreateAction).GetObject().(*corev1.Secret) if certType, _ := CertificateTypeFromObject(actual); certType != CertificateTypeSigner { t.Errorf("expected certificate type 'signer', got: %v", certType) } @@ -76,21 +77,21 @@ func TestEnsureSigningCertKeyPair(t *testing.T) { Type: corev1.SecretTypeTLS, Data: map[string][]byte{"tls.crt": {}, "tls.key": {}}, }, - verifyActions: func(t *testing.T, client *kubefake.Clientset) { + verifyActions: func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool) { t.Helper() actions := client.Actions() - if len(actions) != 2 { + if len(actions) != 1 { t.Fatal(spew.Sdump(actions)) } - if !actions[0].Matches("get", "secrets") { + if !actions[0].Matches("update", "secrets") { t.Error(actions[0]) } - if !actions[1].Matches("update", "secrets") { - t.Error(actions[1]) + if !controllerUpdatedSecret { + t.Errorf("expected controller to update secret") } - actual := actions[1].(clienttesting.UpdateAction).GetObject().(*corev1.Secret) + actual := actions[0].(clienttesting.UpdateAction).GetObject().(*corev1.Secret) if certType, _ := CertificateTypeFromObject(actual); certType != CertificateTypeSigner { t.Errorf("expected certificate type 'signer', got: %v", certType) } @@ -129,7 +130,7 @@ func TestEnsureSigningCertKeyPair(t *testing.T) { Type: corev1.SecretTypeTLS, Data: map[string][]byte{"tls.crt": {}, "tls.key": {}}, }, - verifyActions: func(t *testing.T, client *kubefake.Clientset) { + verifyActions: func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool) { t.Helper() actions := client.Actions() if len(actions) != 0 { @@ -138,102 +139,6 @@ 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 { @@ -262,7 +167,7 @@ func TestEnsureSigningCertKeyPair(t *testing.T) { }, } - _, _, err := c.EnsureSigningCertKeyPair(context.TODO()) + _, updated, err := c.EnsureSigningCertKeyPair(context.TODO()) switch { case err != nil && len(test.expectedError) == 0: t.Error(err) @@ -272,7 +177,7 @@ func TestEnsureSigningCertKeyPair(t *testing.T) { t.Errorf("missing %q", test.expectedError) } - test.verifyActions(t, client) + test.verifyActions(t, client, updated) }) } } diff --git a/pkg/operator/certrotation/target.go b/pkg/operator/certrotation/target.go index 8e6fc700e1..b68aea1633 100644 --- a/pkg/operator/certrotation/target.go +++ b/pkg/operator/certrotation/target.go @@ -12,11 +12,12 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apiserver/pkg/authentication/user" + "k8s.io/klog/v2" "github.com/openshift/library-go/pkg/certs" "github.com/openshift/library-go/pkg/crypto" "github.com/openshift/library-go/pkg/operator/events" - "github.com/openshift/library-go/pkg/operator/resource/resourceapply" + "github.com/openshift/library-go/pkg/operator/resource/resourcehelper" corev1informers "k8s.io/client-go/informers/core/v1" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" corev1listers "k8s.io/client-go/listers/core/v1" @@ -74,7 +75,7 @@ type TargetCertCreator interface { // NewCertificate creates a new key-cert pair with the given signer. NewCertificate(signer *crypto.CA, validity time.Duration) (*crypto.TLSCertificateConfig, error) // NeedNewTargetCertKeyPair decides whether a new cert-key pair is needed. It returns a non-empty reason if it is the case. - NeedNewTargetCertKeyPair(currentCertSecret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired, secretDoesntExist bool) string + NeedNewTargetCertKeyPair(currentCertSecret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired, creationRequired bool) string // SetAnnotations gives an option to override or set additional annotations SetAnnotations(cert *crypto.TLSCertificateConfig, annotations map[string]string) map[string]string } @@ -91,8 +92,8 @@ func (c RotatedSelfSignedCertKeySecret) EnsureTargetCertKeyPair(ctx context.Cont // and need to mint one // TODO do the cross signing thing, but this shows the API consumers want and a very simple impl. - secretDoesntExist := false - modified := false + creationRequired := false + updateRequired := false originalTargetCertKeyPairSecret, err := c.Lister.Secrets(c.Namespace).Get(c.Name) if err != nil && !apierrors.IsNotFound(err) { return nil, err @@ -108,21 +109,14 @@ func (c RotatedSelfSignedCertKeySecret) EnsureTargetCertKeyPair(ctx context.Cont ), Type: corev1.SecretTypeTLS, } - modified = true - secretDoesntExist = true + creationRequired = true } - // 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 - } + needsMetadataUpdate := ensureMetadataUpdate(targetCertKeyPairSecret, c.Owner, c.AdditionalAnnotations) + needsTypeChange := ensureSecretTLSTypeSet(targetCertKeyPairSecret) + updateRequired = needsMetadataUpdate || needsTypeChange - if reason := c.CertCreator.NeedNewTargetCertKeyPair(targetCertKeyPairSecret, signingCertKeyPair, caBundleCerts, c.Refresh, c.RefreshOnlyWhenExpired, secretDoesntExist); len(reason) > 0 { + if reason := c.CertCreator.NeedNewTargetCertKeyPair(targetCertKeyPairSecret, signingCertKeyPair, caBundleCerts, c.Refresh, c.RefreshOnlyWhenExpired, creationRequired); len(reason) > 0 { c.EventRecorder.Eventf("TargetUpdateRequired", "%q in %q requires a new target cert/key pair: %v", c.Name, c.Namespace, reason) if err := setTargetCertKeyPairSecret(targetCertKeyPairSecret, c.Validity, signingCertKeyPair, c.CertCreator, c.AdditionalAnnotations); err != nil { return nil, err @@ -130,22 +124,31 @@ func (c RotatedSelfSignedCertKeySecret) EnsureTargetCertKeyPair(ctx context.Cont LabelAsManagedSecret(targetCertKeyPairSecret, CertificateTypeTarget) - modified = true + updateRequired = true } - - if modified { - actualTargetCertKeyPairSecret, _, err := resourceapply.ApplySecret(ctx, c.Client, c.EventRecorder, targetCertKeyPairSecret) + if creationRequired { + actualTargetCertKeyPairSecret, err := c.Client.Secrets(c.Namespace).Create(ctx, targetCertKeyPairSecret, metav1.CreateOptions{}) + resourcehelper.ReportCreateEvent(c.EventRecorder, actualTargetCertKeyPairSecret, err) + if err != nil { + return nil, err + } + klog.V(2).Infof("Created secret %s/%s", actualTargetCertKeyPairSecret.Namespace, actualTargetCertKeyPairSecret.Name) + targetCertKeyPairSecret = actualTargetCertKeyPairSecret + } else if updateRequired { + actualTargetCertKeyPairSecret, err := c.Client.Secrets(c.Namespace).Update(ctx, targetCertKeyPairSecret, metav1.UpdateOptions{}) + resourcehelper.ReportUpdateEvent(c.EventRecorder, actualTargetCertKeyPairSecret, err) if err != nil { return nil, err } + klog.V(2).Infof("Updated secret %s/%s", actualTargetCertKeyPairSecret.Namespace, actualTargetCertKeyPairSecret.Name) targetCertKeyPairSecret = actualTargetCertKeyPairSecret } return targetCertKeyPairSecret, nil } -func needNewTargetCertKeyPair(secret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired, secretDoesntExist bool) string { - if secretDoesntExist { +func needNewTargetCertKeyPair(secret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired, creationRequired bool) string { + if creationRequired { return "secret doesn't exist" } @@ -266,8 +269,8 @@ func (r *ClientRotation) NewCertificate(signer *crypto.CA, validity time.Duratio return signer.MakeClientCertificateForDuration(r.UserInfo, validity) } -func (r *ClientRotation) NeedNewTargetCertKeyPair(currentCertSecret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired, secretDoesntExist bool) string { - return needNewTargetCertKeyPair(currentCertSecret, signer, caBundleCerts, refresh, refreshOnlyWhenExpired, secretDoesntExist) +func (r *ClientRotation) NeedNewTargetCertKeyPair(currentCertSecret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired, exists bool) string { + return needNewTargetCertKeyPair(currentCertSecret, signer, caBundleCerts, refresh, refreshOnlyWhenExpired, exists) } func (r *ClientRotation) SetAnnotations(cert *crypto.TLSCertificateConfig, annotations map[string]string) map[string]string { @@ -291,8 +294,8 @@ func (r *ServingRotation) RecheckChannel() <-chan struct{} { return r.HostnamesChanged } -func (r *ServingRotation) NeedNewTargetCertKeyPair(currentCertSecret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired, secretDoesntExist bool) string { - reason := needNewTargetCertKeyPair(currentCertSecret, signer, caBundleCerts, refresh, refreshOnlyWhenExpired, secretDoesntExist) +func (r *ServingRotation) NeedNewTargetCertKeyPair(currentCertSecret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired, creationRequired bool) string { + reason := needNewTargetCertKeyPair(currentCertSecret, signer, caBundleCerts, refresh, refreshOnlyWhenExpired, creationRequired) if len(reason) > 0 { return reason } @@ -337,8 +340,8 @@ func (r *SignerRotation) NewCertificate(signer *crypto.CA, validity time.Duratio return crypto.MakeCAConfigForDuration(signerName, validity, signer) } -func (r *SignerRotation) NeedNewTargetCertKeyPair(currentCertSecret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired, secretDoesntExist bool) string { - return needNewTargetCertKeyPair(currentCertSecret, signer, caBundleCerts, refresh, refreshOnlyWhenExpired, secretDoesntExist) +func (r *SignerRotation) NeedNewTargetCertKeyPair(currentCertSecret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired, exists bool) string { + return needNewTargetCertKeyPair(currentCertSecret, signer, caBundleCerts, refresh, refreshOnlyWhenExpired, exists) } func (r *SignerRotation) SetAnnotations(cert *crypto.TLSCertificateConfig, annotations map[string]string) map[string]string { diff --git a/pkg/operator/certrotation/target_test.go b/pkg/operator/certrotation/target_test.go index 16ad7c3c25..b7be8acb8c 100644 --- a/pkg/operator/certrotation/target_test.go +++ b/pkg/operator/certrotation/target_test.go @@ -150,18 +150,14 @@ func TestEnsureTargetCertKeyPair(t *testing.T) { initialSecretFn: func() *corev1.Secret { return nil }, verifyActions: func(t *testing.T, client *kubefake.Clientset) { actions := client.Actions() - if len(actions) != 2 { + if len(actions) != 1 { t.Fatal(spew.Sdump(actions)) } - - if !actions[0].Matches("get", "secrets") { + if !actions[0].Matches("create", "secrets") { t.Error(actions[0]) } - if !actions[1].Matches("create", "secrets") { - t.Error(actions[1]) - } - actual := actions[1].(clienttesting.CreateAction).GetObject().(*corev1.Secret) + actual := actions[0].(clienttesting.CreateAction).GetObject().(*corev1.Secret) if len(actual.Annotations) == 0 { t.Errorf("expected certificates to be annotated") } @@ -198,75 +194,15 @@ func TestEnsureTargetCertKeyPair(t *testing.T) { }, verifyActions: func(t *testing.T, client *kubefake.Clientset) { actions := client.Actions() - if len(actions) != 2 { - t.Fatal(spew.Sdump(actions)) - } - - if !actions[1].Matches("update", "secrets") { - t.Error(actions[1]) - } - - 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 { + if len(actions) != 1 { t.Fatal(spew.Sdump(actions)) } - if !actions[0].Matches("get", "secrets") { + if !actions[0].Matches("update", "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) + actual := actions[0].(clienttesting.UpdateAction).GetObject().(*corev1.Secret) if len(actual.Annotations) == 0 { t.Errorf("expected certificates to be annotated") } @@ -277,72 +213,6 @@ func TestEnsureTargetCertKeyPair(t *testing.T) { 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) } @@ -480,18 +350,15 @@ func TestEnsureTargetSignerCertKeyPair(t *testing.T) { initialSecretFn: func() *corev1.Secret { return nil }, verifyActions: func(t *testing.T, client *kubefake.Clientset) { actions := client.Actions() - if len(actions) != 2 { + if len(actions) != 1 { t.Fatal(spew.Sdump(actions)) } - if !actions[0].Matches("get", "secrets") { + if !actions[0].Matches("create", "secrets") { t.Error(actions[0]) } - if !actions[1].Matches("create", "secrets") { - t.Error(actions[1]) - } - actual := actions[1].(clienttesting.CreateAction).GetObject().(*corev1.Secret) + actual := actions[0].(clienttesting.CreateAction).GetObject().(*corev1.Secret) if len(actual.Data["tls.crt"]) == 0 || len(actual.Data["tls.key"]) == 0 { t.Error(actual.Data) } @@ -520,7 +387,7 @@ func TestEnsureTargetSignerCertKeyPair(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, } @@ -528,15 +395,15 @@ func TestEnsureTargetSignerCertKeyPair(t *testing.T) { }, verifyActions: func(t *testing.T, client *kubefake.Clientset) { actions := client.Actions() - if len(actions) != 2 { + if len(actions) != 1 { t.Fatal(spew.Sdump(actions)) } - if !actions[1].Matches("update", "secrets") { - t.Error(actions[1]) + if !actions[0].Matches("update", "secrets") { + t.Error(actions[0]) } - actual := actions[1].(clienttesting.UpdateAction).GetObject().(*corev1.Secret) + actual := actions[0].(clienttesting.UpdateAction).GetObject().(*corev1.Secret) if len(actual.Data["tls.crt"]) == 0 || len(actual.Data["tls.key"]) == 0 { t.Error(actual.Data) }