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..2eb761bbb3 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 + // run Update if metadata needs changing needsMetadataUpdate := ensureMetadataUpdate(signingCertKeyPairSecret, c.Owner, c.AdditionalAnnotations) needsTypeChange := ensureSecretTLSTypeSet(signingCertKeyPairSecret) - modified = needsMetadataUpdate || needsTypeChange || modified + updateRequired = needsMetadataUpdate || needsTypeChange + // run Update if signer content needs changing 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) } diff --git a/pkg/operator/resource/resourceapply/admissionregistration.go b/pkg/operator/resource/resourceapply/admissionregistration.go index b6bcad5e0b..88bd00b251 100644 --- a/pkg/operator/resource/resourceapply/admissionregistration.go +++ b/pkg/operator/resource/resourceapply/admissionregistration.go @@ -5,6 +5,7 @@ import ( "fmt" "github.com/openshift/library-go/pkg/operator/events" + "github.com/openshift/library-go/pkg/operator/resource/resourcehelper" "github.com/openshift/library-go/pkg/operator/resource/resourcemerge" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" admissionregistrationv1beta1 "k8s.io/api/admissionregistration/v1beta1" @@ -34,7 +35,7 @@ func ApplyMutatingWebhookConfigurationImproved(ctx context.Context, client admis required := requiredOriginal.DeepCopy() actual, err := client.MutatingWebhookConfigurations().Create( ctx, resourcemerge.WithCleanLabelsAndAnnotations(required).(*admissionregistrationv1.MutatingWebhookConfiguration), metav1.CreateOptions{}) - reportCreateEvent(recorder, required, err) + resourcehelper.ReportCreateEvent(recorder, required, err) if err != nil { return nil, false, err } @@ -68,7 +69,7 @@ func ApplyMutatingWebhookConfigurationImproved(ctx context.Context, client admis klog.V(2).Infof("MutatingWebhookConfiguration %q changes: %v", required.GetNamespace()+"/"+required.GetName(), JSONPatchNoError(existing, toWrite)) actual, err := client.MutatingWebhookConfigurations().Update(ctx, toWrite, metav1.UpdateOptions{}) - reportUpdateEvent(recorder, required, err) + resourcehelper.ReportUpdateEvent(recorder, required, err) if err != nil { return nil, false, err } @@ -109,7 +110,7 @@ func ApplyValidatingWebhookConfigurationImproved(ctx context.Context, client adm required := requiredOriginal.DeepCopy() actual, err := client.ValidatingWebhookConfigurations().Create( ctx, resourcemerge.WithCleanLabelsAndAnnotations(required).(*admissionregistrationv1.ValidatingWebhookConfiguration), metav1.CreateOptions{}) - reportCreateEvent(recorder, required, err) + resourcehelper.ReportCreateEvent(recorder, required, err) if err != nil { return nil, false, err } @@ -143,7 +144,7 @@ func ApplyValidatingWebhookConfigurationImproved(ctx context.Context, client adm klog.V(2).Infof("ValidatingWebhookConfiguration %q changes: %v", required.GetNamespace()+"/"+required.GetName(), JSONPatchNoError(existing, toWrite)) actual, err := client.ValidatingWebhookConfigurations().Update(ctx, toWrite, metav1.UpdateOptions{}) - reportUpdateEvent(recorder, required, err) + resourcehelper.ReportUpdateEvent(recorder, required, err) if err != nil { return nil, false, err } @@ -160,7 +161,7 @@ func DeleteValidatingWebhookConfiguration(ctx context.Context, client admissionr if err != nil { return nil, false, err } - reportDeleteEvent(recorder, required, err) + resourcehelper.ReportDeleteEvent(recorder, required, err) return nil, true, nil } @@ -196,7 +197,7 @@ func ApplyValidatingAdmissionPolicyV1beta1(ctx context.Context, client admission required := requiredOriginal.DeepCopy() actual, err := client.ValidatingAdmissionPolicies().Create( ctx, resourcemerge.WithCleanLabelsAndAnnotations(required).(*admissionregistrationv1beta1.ValidatingAdmissionPolicy), metav1.CreateOptions{}) - reportCreateEvent(recorder, required, err) + resourcehelper.ReportCreateEvent(recorder, required, err) if err != nil { return nil, false, err } @@ -229,7 +230,7 @@ func ApplyValidatingAdmissionPolicyV1beta1(ctx context.Context, client admission klog.V(2).Infof("ValidatingAdmissionPolicyConfigurationV1beta1 %q changes: %v", required.GetNamespace()+"/"+required.GetName(), JSONPatchNoError(existing, toWrite)) actual, err := client.ValidatingAdmissionPolicies().Update(ctx, toWrite, metav1.UpdateOptions{}) - reportUpdateEvent(recorder, required, err) + resourcehelper.ReportUpdateEvent(recorder, required, err) if err != nil { return nil, false, err } @@ -255,7 +256,7 @@ func ApplyValidatingAdmissionPolicyV1(ctx context.Context, client admissionregis required := requiredOriginal.DeepCopy() actual, err := client.ValidatingAdmissionPolicies().Create( ctx, resourcemerge.WithCleanLabelsAndAnnotations(required).(*admissionregistrationv1.ValidatingAdmissionPolicy), metav1.CreateOptions{}) - reportCreateEvent(recorder, required, err) + resourcehelper.ReportCreateEvent(recorder, required, err) if err != nil { return nil, false, err } @@ -288,7 +289,7 @@ func ApplyValidatingAdmissionPolicyV1(ctx context.Context, client admissionregis klog.V(2).Infof("ValidatingAdmissionPolicyConfigurationV1 %q changes: %v", required.GetNamespace()+"/"+required.GetName(), JSONPatchNoError(existing, toWrite)) actual, err := client.ValidatingAdmissionPolicies().Update(ctx, toWrite, metav1.UpdateOptions{}) - reportUpdateEvent(recorder, required, err) + resourcehelper.ReportUpdateEvent(recorder, required, err) if err != nil { return nil, false, err } @@ -314,7 +315,7 @@ func ApplyValidatingAdmissionPolicyBindingV1beta1(ctx context.Context, client ad required := requiredOriginal.DeepCopy() actual, err := client.ValidatingAdmissionPolicyBindings().Create( ctx, resourcemerge.WithCleanLabelsAndAnnotations(required).(*admissionregistrationv1beta1.ValidatingAdmissionPolicyBinding), metav1.CreateOptions{}) - reportCreateEvent(recorder, required, err) + resourcehelper.ReportCreateEvent(recorder, required, err) if err != nil { return nil, false, err } @@ -347,7 +348,7 @@ func ApplyValidatingAdmissionPolicyBindingV1beta1(ctx context.Context, client ad klog.V(2).Infof("ValidatingAdmissionPolicyBindingConfigurationV1beta1 %q changes: %v", required.GetNamespace()+"/"+required.GetName(), JSONPatchNoError(existing, toWrite)) actual, err := client.ValidatingAdmissionPolicyBindings().Update(ctx, toWrite, metav1.UpdateOptions{}) - reportUpdateEvent(recorder, required, err) + resourcehelper.ReportUpdateEvent(recorder, required, err) if err != nil { return nil, false, err } @@ -373,7 +374,7 @@ func ApplyValidatingAdmissionPolicyBindingV1(ctx context.Context, client admissi required := requiredOriginal.DeepCopy() actual, err := client.ValidatingAdmissionPolicyBindings().Create( ctx, resourcemerge.WithCleanLabelsAndAnnotations(required).(*admissionregistrationv1.ValidatingAdmissionPolicyBinding), metav1.CreateOptions{}) - reportCreateEvent(recorder, required, err) + resourcehelper.ReportCreateEvent(recorder, required, err) if err != nil { return nil, false, err } @@ -406,7 +407,7 @@ func ApplyValidatingAdmissionPolicyBindingV1(ctx context.Context, client admissi klog.V(2).Infof("ValidatingAdmissionPolicyBindingConfigurationV1 %q changes: %v", required.GetNamespace()+"/"+required.GetName(), JSONPatchNoError(existing, toWrite)) actual, err := client.ValidatingAdmissionPolicyBindings().Update(ctx, toWrite, metav1.UpdateOptions{}) - reportUpdateEvent(recorder, required, err) + resourcehelper.ReportUpdateEvent(recorder, required, err) if err != nil { return nil, false, err } diff --git a/pkg/operator/resource/resourceapply/apiextensions.go b/pkg/operator/resource/resourceapply/apiextensions.go index 587c9bd556..0e76cf8341 100644 --- a/pkg/operator/resource/resourceapply/apiextensions.go +++ b/pkg/operator/resource/resourceapply/apiextensions.go @@ -4,6 +4,7 @@ import ( "context" "github.com/openshift/library-go/pkg/operator/events" + "github.com/openshift/library-go/pkg/operator/resource/resourcehelper" "github.com/openshift/library-go/pkg/operator/resource/resourcemerge" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apiextclientv1 "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1" @@ -19,7 +20,7 @@ func ApplyCustomResourceDefinitionV1(ctx context.Context, client apiextclientv1. requiredCopy := required.DeepCopy() actual, err := client.CustomResourceDefinitions().Create( ctx, resourcemerge.WithCleanLabelsAndAnnotations(requiredCopy).(*apiextensionsv1.CustomResourceDefinition), metav1.CreateOptions{}) - reportCreateEvent(recorder, required, err) + resourcehelper.ReportCreateEvent(recorder, required, err) return actual, true, err } if err != nil { @@ -38,7 +39,7 @@ func ApplyCustomResourceDefinitionV1(ctx context.Context, client apiextclientv1. } actual, err := client.CustomResourceDefinitions().Update(ctx, existingCopy, metav1.UpdateOptions{}) - reportUpdateEvent(recorder, required, err) + resourcehelper.ReportUpdateEvent(recorder, required, err) return actual, true, err } @@ -51,6 +52,6 @@ func DeleteCustomResourceDefinitionV1(ctx context.Context, client apiextclientv1 if err != nil { return nil, false, err } - reportDeleteEvent(recorder, required, err) + resourcehelper.ReportDeleteEvent(recorder, required, err) return nil, true, nil } diff --git a/pkg/operator/resource/resourceapply/apiregistration.go b/pkg/operator/resource/resourceapply/apiregistration.go index 931a6c0e1b..e465438f6b 100644 --- a/pkg/operator/resource/resourceapply/apiregistration.go +++ b/pkg/operator/resource/resourceapply/apiregistration.go @@ -11,6 +11,7 @@ import ( apiregistrationv1client "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/typed/apiregistration/v1" "github.com/openshift/library-go/pkg/operator/events" + "github.com/openshift/library-go/pkg/operator/resource/resourcehelper" "github.com/openshift/library-go/pkg/operator/resource/resourcemerge" ) @@ -21,7 +22,7 @@ func ApplyAPIService(ctx context.Context, client apiregistrationv1client.APIServ requiredCopy := required.DeepCopy() actual, err := client.APIServices().Create( ctx, resourcemerge.WithCleanLabelsAndAnnotations(requiredCopy).(*apiregistrationv1.APIService), metav1.CreateOptions{}) - reportCreateEvent(recorder, required, err) + resourcehelper.ReportCreateEvent(recorder, required, err) return actual, true, err } if err != nil { @@ -46,6 +47,6 @@ func ApplyAPIService(ctx context.Context, client apiregistrationv1client.APIServ klog.Infof("APIService %q changes: %s", existing.Name, JSONPatchNoError(existing, existingCopy)) } actual, err := client.APIServices().Update(ctx, existingCopy, metav1.UpdateOptions{}) - reportUpdateEvent(recorder, required, err) + resourcehelper.ReportUpdateEvent(recorder, required, err) return actual, true, err } diff --git a/pkg/operator/resource/resourceapply/apps.go b/pkg/operator/resource/resourceapply/apps.go index 0560c66abc..650cf9b4f8 100644 --- a/pkg/operator/resource/resourceapply/apps.go +++ b/pkg/operator/resource/resourceapply/apps.go @@ -15,6 +15,7 @@ import ( appsclientv1 "k8s.io/client-go/kubernetes/typed/apps/v1" "github.com/openshift/library-go/pkg/operator/events" + "github.com/openshift/library-go/pkg/operator/resource/resourcehelper" "github.com/openshift/library-go/pkg/operator/resource/resourcemerge" ) @@ -118,7 +119,7 @@ func ApplyDeploymentWithForce(ctx context.Context, client appsclientv1.Deploymen existing, err := client.Deployments(required.Namespace).Get(ctx, required.Name, metav1.GetOptions{}) if apierrors.IsNotFound(err) { actual, err := client.Deployments(required.Namespace).Create(ctx, required, metav1.CreateOptions{}) - reportCreateEvent(recorder, required, err) + resourcehelper.ReportCreateEvent(recorder, required, err) return actual, true, err } if err != nil { @@ -155,7 +156,7 @@ func ApplyDeploymentWithForce(ctx context.Context, client appsclientv1.Deploymen } actual, err := client.Deployments(required.Namespace).Update(ctx, toWrite, metav1.UpdateOptions{}) - reportUpdateEvent(recorder, required, err) + resourcehelper.ReportUpdateEvent(recorder, required, err) return actual, true, err } @@ -205,7 +206,7 @@ func ApplyDaemonSetWithForce(ctx context.Context, client appsclientv1.DaemonSets existing, err := client.DaemonSets(required.Namespace).Get(ctx, required.Name, metav1.GetOptions{}) if apierrors.IsNotFound(err) { actual, err := client.DaemonSets(required.Namespace).Create(ctx, required, metav1.CreateOptions{}) - reportCreateEvent(recorder, required, err) + resourcehelper.ReportCreateEvent(recorder, required, err) return actual, true, err } if err != nil { @@ -241,6 +242,6 @@ func ApplyDaemonSetWithForce(ctx context.Context, client appsclientv1.DaemonSets klog.Infof("DaemonSet %q changes: %v", required.Namespace+"/"+required.Name, JSONPatchNoError(existing, toWrite)) } actual, err := client.DaemonSets(required.Namespace).Update(ctx, toWrite, metav1.UpdateOptions{}) - reportUpdateEvent(recorder, required, err) + resourcehelper.ReportUpdateEvent(recorder, required, err) return actual, true, err } diff --git a/pkg/operator/resource/resourceapply/core.go b/pkg/operator/resource/resourceapply/core.go index 09aa5f9966..f954d48cc6 100644 --- a/pkg/operator/resource/resourceapply/core.go +++ b/pkg/operator/resource/resourceapply/core.go @@ -8,6 +8,7 @@ import ( "strings" "github.com/openshift/library-go/pkg/operator/events" + "github.com/openshift/library-go/pkg/operator/resource/resourcehelper" "github.com/openshift/library-go/pkg/operator/resource/resourcemerge" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" @@ -95,7 +96,7 @@ func ApplyNamespaceImproved(ctx context.Context, client coreclientv1.NamespacesG requiredCopy := required.DeepCopy() actual, err := client.Namespaces(). Create(ctx, resourcemerge.WithCleanLabelsAndAnnotations(requiredCopy).(*corev1.Namespace), metav1.CreateOptions{}) - reportCreateEvent(recorder, requiredCopy, err) + resourcehelper.ReportCreateEvent(recorder, requiredCopy, err) cache.UpdateCachedResourceMetadata(required, actual) return actual, true, err } @@ -121,7 +122,7 @@ func ApplyNamespaceImproved(ctx context.Context, client coreclientv1.NamespacesG } actual, err := client.Namespaces().Update(ctx, existingCopy, metav1.UpdateOptions{}) - reportUpdateEvent(recorder, required, err) + resourcehelper.ReportUpdateEvent(recorder, required, err) cache.UpdateCachedResourceMetadata(required, actual) return actual, true, err } @@ -142,7 +143,7 @@ func ApplyServiceImproved(ctx context.Context, client coreclientv1.ServicesGette requiredCopy := required.DeepCopy() actual, err := client.Services(requiredCopy.Namespace). Create(ctx, resourcemerge.WithCleanLabelsAndAnnotations(requiredCopy).(*corev1.Service), metav1.CreateOptions{}) - reportCreateEvent(recorder, requiredCopy, err) + resourcehelper.ReportCreateEvent(recorder, requiredCopy, err) cache.UpdateCachedResourceMetadata(required, actual) return actual, true, err } @@ -182,7 +183,7 @@ func ApplyServiceImproved(ctx context.Context, client coreclientv1.ServicesGette } actual, err := client.Services(required.Namespace).Update(ctx, existingCopy, metav1.UpdateOptions{}) - reportUpdateEvent(recorder, required, err) + resourcehelper.ReportUpdateEvent(recorder, required, err) cache.UpdateCachedResourceMetadata(required, actual) return actual, true, err } @@ -194,7 +195,7 @@ func ApplyPodImproved(ctx context.Context, client coreclientv1.PodsGetter, recor requiredCopy := required.DeepCopy() actual, err := client.Pods(requiredCopy.Namespace). Create(ctx, resourcemerge.WithCleanLabelsAndAnnotations(requiredCopy).(*corev1.Pod), metav1.CreateOptions{}) - reportCreateEvent(recorder, requiredCopy, err) + resourcehelper.ReportCreateEvent(recorder, requiredCopy, err) cache.UpdateCachedResourceMetadata(required, actual) return actual, true, err } @@ -220,7 +221,7 @@ func ApplyPodImproved(ctx context.Context, client coreclientv1.PodsGetter, recor } actual, err := client.Pods(required.Namespace).Update(ctx, existingCopy, metav1.UpdateOptions{}) - reportUpdateEvent(recorder, required, err) + resourcehelper.ReportUpdateEvent(recorder, required, err) cache.UpdateCachedResourceMetadata(required, actual) return actual, true, err } @@ -232,7 +233,7 @@ func ApplyServiceAccountImproved(ctx context.Context, client coreclientv1.Servic requiredCopy := required.DeepCopy() actual, err := client.ServiceAccounts(requiredCopy.Namespace). Create(ctx, resourcemerge.WithCleanLabelsAndAnnotations(requiredCopy).(*corev1.ServiceAccount), metav1.CreateOptions{}) - reportCreateEvent(recorder, requiredCopy, err) + resourcehelper.ReportCreateEvent(recorder, requiredCopy, err) cache.UpdateCachedResourceMetadata(required, actual) return actual, true, err } @@ -256,7 +257,7 @@ func ApplyServiceAccountImproved(ctx context.Context, client coreclientv1.Servic klog.Infof("ServiceAccount %q changes: %v", required.Namespace+"/"+required.Name, JSONPatchNoError(existing, required)) } actual, err := client.ServiceAccounts(required.Namespace).Update(ctx, existingCopy, metav1.UpdateOptions{}) - reportUpdateEvent(recorder, required, err) + resourcehelper.ReportUpdateEvent(recorder, required, err) cache.UpdateCachedResourceMetadata(required, actual) return actual, true, err } @@ -268,7 +269,7 @@ func ApplyConfigMapImproved(ctx context.Context, client coreclientv1.ConfigMapsG requiredCopy := required.DeepCopy() actual, err := client.ConfigMaps(requiredCopy.Namespace). Create(ctx, resourcemerge.WithCleanLabelsAndAnnotations(requiredCopy).(*corev1.ConfigMap), metav1.CreateOptions{}) - reportCreateEvent(recorder, requiredCopy, err) + resourcehelper.ReportCreateEvent(recorder, requiredCopy, err) cache.UpdateCachedResourceMetadata(required, actual) return actual, true, err } @@ -350,7 +351,7 @@ func ApplyConfigMapImproved(ctx context.Context, client coreclientv1.ConfigMapsG if klog.V(2).Enabled() { klog.Infof("ConfigMap %q changes: %v", required.Namespace+"/"+required.Name, JSONPatchNoError(existing, required)) } - reportUpdateEvent(recorder, required, err, details) + resourcehelper.ReportUpdateEvent(recorder, required, err, details) cache.UpdateCachedResourceMetadata(required, actual) return actual, true, err } @@ -386,7 +387,7 @@ func ApplySecretImproved(ctx context.Context, client coreclientv1.SecretsGetter, requiredCopy := required.DeepCopy() actual, err := client.Secrets(requiredCopy.Namespace). Create(ctx, resourcemerge.WithCleanLabelsAndAnnotations(requiredCopy).(*corev1.Secret), metav1.CreateOptions{}) - reportCreateEvent(recorder, requiredCopy, err) + resourcehelper.ReportCreateEvent(recorder, requiredCopy, err) cache.UpdateCachedResourceMetadata(requiredInput, actual) return actual, true, err } @@ -438,7 +439,7 @@ func ApplySecretImproved(ctx context.Context, client coreclientv1.SecretsGetter, */ if existingCopy.Type == existing.Type { actual, err = client.Secrets(required.Namespace).Update(ctx, existingCopy, metav1.UpdateOptions{}) - reportUpdateEvent(recorder, existingCopy, err) + resourcehelper.ReportUpdateEvent(recorder, existingCopy, err) if err == nil { return actual, true, err @@ -450,12 +451,12 @@ func ApplySecretImproved(ctx context.Context, client coreclientv1.SecretsGetter, // if the field was immutable on a secret, we're going to be stuck until we delete it. Try to delete and then create deleteErr := client.Secrets(required.Namespace).Delete(ctx, existingCopy.Name, metav1.DeleteOptions{}) - reportDeleteEvent(recorder, existingCopy, deleteErr) + resourcehelper.ReportDeleteEvent(recorder, existingCopy, deleteErr) // clear the RV and track the original actual and error for the return like our create value. existingCopy.ResourceVersion = "" actual, err = client.Secrets(required.Namespace).Create(ctx, existingCopy, metav1.CreateOptions{}) - reportCreateEvent(recorder, existingCopy, err) + resourcehelper.ReportCreateEvent(recorder, existingCopy, err) cache.UpdateCachedResourceMetadata(requiredInput, actual) return actual, true, err } @@ -602,7 +603,7 @@ func DeleteNamespace(ctx context.Context, client coreclientv1.NamespacesGetter, if err != nil { return nil, false, err } - reportDeleteEvent(recorder, required, err) + resourcehelper.ReportDeleteEvent(recorder, required, err) return nil, true, nil } @@ -614,7 +615,7 @@ func DeleteService(ctx context.Context, client coreclientv1.ServicesGetter, reco if err != nil { return nil, false, err } - reportDeleteEvent(recorder, required, err) + resourcehelper.ReportDeleteEvent(recorder, required, err) return nil, true, nil } @@ -626,7 +627,7 @@ func DeletePod(ctx context.Context, client coreclientv1.PodsGetter, recorder eve if err != nil { return nil, false, err } - reportDeleteEvent(recorder, required, err) + resourcehelper.ReportDeleteEvent(recorder, required, err) return nil, true, nil } @@ -638,7 +639,7 @@ func DeleteServiceAccount(ctx context.Context, client coreclientv1.ServiceAccoun if err != nil { return nil, false, err } - reportDeleteEvent(recorder, required, err) + resourcehelper.ReportDeleteEvent(recorder, required, err) return nil, true, nil } @@ -650,7 +651,7 @@ func DeleteConfigMap(ctx context.Context, client coreclientv1.ConfigMapsGetter, if err != nil { return nil, false, err } - reportDeleteEvent(recorder, required, err) + resourcehelper.ReportDeleteEvent(recorder, required, err) return nil, true, nil } @@ -662,6 +663,6 @@ func DeleteSecret(ctx context.Context, client coreclientv1.SecretsGetter, record if err != nil { return nil, false, err } - reportDeleteEvent(recorder, required, err) + resourcehelper.ReportDeleteEvent(recorder, required, err) return nil, true, nil } diff --git a/pkg/operator/resource/resourceapply/event_helpers.go b/pkg/operator/resource/resourceapply/event_helpers.go deleted file mode 100644 index af598993f9..0000000000 --- a/pkg/operator/resource/resourceapply/event_helpers.go +++ /dev/null @@ -1,56 +0,0 @@ -package resourceapply - -import ( - "fmt" - "strings" - - "k8s.io/apimachinery/pkg/runtime" - - openshiftapi "github.com/openshift/api" - - "github.com/openshift/library-go/pkg/operator/events" - "github.com/openshift/library-go/pkg/operator/resource/resourcehelper" -) - -var ( - openshiftScheme = runtime.NewScheme() -) - -func init() { - if err := openshiftapi.Install(openshiftScheme); err != nil { - panic(err) - } -} - -func reportCreateEvent(recorder events.Recorder, obj runtime.Object, originalErr error) { - gvk := resourcehelper.GuessObjectGroupVersionKind(obj) - if originalErr == nil { - recorder.Eventf(fmt.Sprintf("%sCreated", gvk.Kind), "Created %s because it was missing", resourcehelper.FormatResourceForCLIWithNamespace(obj)) - return - } - recorder.Warningf(fmt.Sprintf("%sCreateFailed", gvk.Kind), "Failed to create %s: %v", resourcehelper.FormatResourceForCLIWithNamespace(obj), originalErr) -} - -func reportUpdateEvent(recorder events.Recorder, obj runtime.Object, originalErr error, details ...string) { - gvk := resourcehelper.GuessObjectGroupVersionKind(obj) - switch { - case originalErr != nil: - recorder.Warningf(fmt.Sprintf("%sUpdateFailed", gvk.Kind), "Failed to update %s: %v", resourcehelper.FormatResourceForCLIWithNamespace(obj), originalErr) - case len(details) == 0: - recorder.Eventf(fmt.Sprintf("%sUpdated", gvk.Kind), "Updated %s because it changed", resourcehelper.FormatResourceForCLIWithNamespace(obj)) - default: - recorder.Eventf(fmt.Sprintf("%sUpdated", gvk.Kind), "Updated %s:\n%s", resourcehelper.FormatResourceForCLIWithNamespace(obj), strings.Join(details, "\n")) - } -} - -func reportDeleteEvent(recorder events.Recorder, obj runtime.Object, originalErr error, details ...string) { - gvk := resourcehelper.GuessObjectGroupVersionKind(obj) - switch { - case originalErr != nil: - recorder.Warningf(fmt.Sprintf("%sDeleteFailed", gvk.Kind), "Failed to delete %s: %v", resourcehelper.FormatResourceForCLIWithNamespace(obj), originalErr) - case len(details) == 0: - recorder.Eventf(fmt.Sprintf("%sDeleted", gvk.Kind), "Deleted %s", resourcehelper.FormatResourceForCLIWithNamespace(obj)) - default: - recorder.Eventf(fmt.Sprintf("%sDeleted", gvk.Kind), "Deleted %s:\n%s", resourcehelper.FormatResourceForCLIWithNamespace(obj), strings.Join(details, "\n")) - } -} diff --git a/pkg/operator/resource/resourceapply/migration.go b/pkg/operator/resource/resourceapply/migration.go index 7c0dcf6051..2bf3d74b6c 100644 --- a/pkg/operator/resource/resourceapply/migration.go +++ b/pkg/operator/resource/resourceapply/migration.go @@ -5,6 +5,7 @@ import ( "reflect" "github.com/openshift/library-go/pkg/operator/events" + "github.com/openshift/library-go/pkg/operator/resource/resourcehelper" "github.com/openshift/library-go/pkg/operator/resource/resourcemerge" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -21,7 +22,7 @@ func ApplyStorageVersionMigration(ctx context.Context, client migrationclientv1a if apierrors.IsNotFound(err) { requiredCopy := required.DeepCopy() actual, err := clientInterface.Create(ctx, resourcemerge.WithCleanLabelsAndAnnotations(requiredCopy).(*v1alpha1.StorageVersionMigration), metav1.CreateOptions{}) - reportCreateEvent(recorder, requiredCopy, err) + resourcehelper.ReportCreateEvent(recorder, requiredCopy, err) return actual, true, err } if err != nil { @@ -41,7 +42,7 @@ func ApplyStorageVersionMigration(ctx context.Context, client migrationclientv1a required.Spec.Resource.DeepCopyInto(&existingCopy.Spec.Resource) actual, err := clientInterface.Update(ctx, existingCopy, metav1.UpdateOptions{}) - reportUpdateEvent(recorder, required, err) + resourcehelper.ReportUpdateEvent(recorder, required, err) return actual, true, err } @@ -54,6 +55,6 @@ func DeleteStorageVersionMigration(ctx context.Context, client migrationclientv1 if err != nil { return nil, false, err } - reportDeleteEvent(recorder, required, err) + resourcehelper.ReportDeleteEvent(recorder, required, err) return nil, true, nil } diff --git a/pkg/operator/resource/resourceapply/monitoring.go b/pkg/operator/resource/resourceapply/monitoring.go index 555f7a3821..8b64f23b72 100644 --- a/pkg/operator/resource/resourceapply/monitoring.go +++ b/pkg/operator/resource/resourceapply/monitoring.go @@ -4,6 +4,7 @@ import ( "context" "github.com/openshift/library-go/pkg/operator/events" + "github.com/openshift/library-go/pkg/operator/resource/resourcehelper" "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -150,7 +151,7 @@ func DeletePrometheusRule(ctx context.Context, client dynamic.Interface, recorde if err != nil { return nil, false, err } - reportDeleteEvent(recorder, required, err) + resourcehelper.ReportDeleteEvent(recorder, required, err) return nil, true, nil } @@ -163,6 +164,6 @@ func DeleteServiceMonitor(ctx context.Context, client dynamic.Interface, recorde if err != nil { return nil, false, err } - reportDeleteEvent(recorder, required, err) + resourcehelper.ReportDeleteEvent(recorder, required, err) return nil, true, nil } diff --git a/pkg/operator/resource/resourceapply/policy.go b/pkg/operator/resource/resourceapply/policy.go index 6cf4793253..86d45fad08 100644 --- a/pkg/operator/resource/resourceapply/policy.go +++ b/pkg/operator/resource/resourceapply/policy.go @@ -11,6 +11,7 @@ import ( "k8s.io/klog/v2" "github.com/openshift/library-go/pkg/operator/events" + "github.com/openshift/library-go/pkg/operator/resource/resourcehelper" "github.com/openshift/library-go/pkg/operator/resource/resourcemerge" ) @@ -20,7 +21,7 @@ func ApplyPodDisruptionBudget(ctx context.Context, client policyclientv1.PodDisr requiredCopy := required.DeepCopy() actual, err := client.PodDisruptionBudgets(required.Namespace).Create( ctx, resourcemerge.WithCleanLabelsAndAnnotations(requiredCopy).(*policyv1.PodDisruptionBudget), metav1.CreateOptions{}) - reportCreateEvent(recorder, required, err) + resourcehelper.ReportCreateEvent(recorder, required, err) return actual, true, err } if err != nil { @@ -43,7 +44,7 @@ func ApplyPodDisruptionBudget(ctx context.Context, client policyclientv1.PodDisr } actual, err := client.PodDisruptionBudgets(required.Namespace).Update(ctx, existingCopy, metav1.UpdateOptions{}) - reportUpdateEvent(recorder, required, err) + resourcehelper.ReportUpdateEvent(recorder, required, err) return actual, true, err } @@ -55,6 +56,6 @@ func DeletePodDisruptionBudget(ctx context.Context, client policyclientv1.PodDis if err != nil { return nil, false, err } - reportDeleteEvent(recorder, required, err) + resourcehelper.ReportDeleteEvent(recorder, required, err) return nil, true, nil } diff --git a/pkg/operator/resource/resourceapply/rbac.go b/pkg/operator/resource/resourceapply/rbac.go index 4b45c8818e..223c64d4f8 100644 --- a/pkg/operator/resource/resourceapply/rbac.go +++ b/pkg/operator/resource/resourceapply/rbac.go @@ -11,6 +11,7 @@ import ( "k8s.io/klog/v2" "github.com/openshift/library-go/pkg/operator/events" + "github.com/openshift/library-go/pkg/operator/resource/resourcehelper" "github.com/openshift/library-go/pkg/operator/resource/resourcemerge" ) @@ -21,7 +22,7 @@ func ApplyClusterRole(ctx context.Context, client rbacclientv1.ClusterRolesGette requiredCopy := required.DeepCopy() actual, err := client.ClusterRoles().Create( ctx, resourcemerge.WithCleanLabelsAndAnnotations(requiredCopy).(*rbacv1.ClusterRole), metav1.CreateOptions{}) - reportCreateEvent(recorder, required, err) + resourcehelper.ReportCreateEvent(recorder, required, err) return actual, true, err } if err != nil { @@ -55,7 +56,7 @@ func ApplyClusterRole(ctx context.Context, client rbacclientv1.ClusterRolesGette } actual, err := client.ClusterRoles().Update(ctx, existingCopy, metav1.UpdateOptions{}) - reportUpdateEvent(recorder, required, err) + resourcehelper.ReportUpdateEvent(recorder, required, err) return actual, true, err } @@ -67,7 +68,7 @@ func ApplyClusterRoleBinding(ctx context.Context, client rbacclientv1.ClusterRol requiredCopy := required.DeepCopy() actual, err := client.ClusterRoleBindings().Create( ctx, resourcemerge.WithCleanLabelsAndAnnotations(requiredCopy).(*rbacv1.ClusterRoleBinding), metav1.CreateOptions{}) - reportCreateEvent(recorder, required, err) + resourcehelper.ReportCreateEvent(recorder, required, err) return actual, true, err } if err != nil { @@ -110,7 +111,7 @@ func ApplyClusterRoleBinding(ctx context.Context, client rbacclientv1.ClusterRol } actual, err := client.ClusterRoleBindings().Update(ctx, existingCopy, metav1.UpdateOptions{}) - reportUpdateEvent(recorder, requiredCopy, err) + resourcehelper.ReportUpdateEvent(recorder, requiredCopy, err) return actual, true, err } @@ -121,7 +122,7 @@ func ApplyRole(ctx context.Context, client rbacclientv1.RolesGetter, recorder ev requiredCopy := required.DeepCopy() actual, err := client.Roles(required.Namespace).Create( ctx, resourcemerge.WithCleanLabelsAndAnnotations(requiredCopy).(*rbacv1.Role), metav1.CreateOptions{}) - reportCreateEvent(recorder, required, err) + resourcehelper.ReportCreateEvent(recorder, required, err) return actual, true, err } if err != nil { @@ -143,7 +144,7 @@ func ApplyRole(ctx context.Context, client rbacclientv1.RolesGetter, recorder ev klog.Infof("Role %q changes: %v", required.Namespace+"/"+required.Name, JSONPatchNoError(existing, existingCopy)) } actual, err := client.Roles(required.Namespace).Update(ctx, existingCopy, metav1.UpdateOptions{}) - reportUpdateEvent(recorder, required, err) + resourcehelper.ReportUpdateEvent(recorder, required, err) return actual, true, err } @@ -155,7 +156,7 @@ func ApplyRoleBinding(ctx context.Context, client rbacclientv1.RoleBindingsGette requiredCopy := required.DeepCopy() actual, err := client.RoleBindings(required.Namespace).Create( ctx, resourcemerge.WithCleanLabelsAndAnnotations(requiredCopy).(*rbacv1.RoleBinding), metav1.CreateOptions{}) - reportCreateEvent(recorder, required, err) + resourcehelper.ReportCreateEvent(recorder, required, err) return actual, true, err } if err != nil { @@ -198,7 +199,7 @@ func ApplyRoleBinding(ctx context.Context, client rbacclientv1.RoleBindingsGette } actual, err := client.RoleBindings(requiredCopy.Namespace).Update(ctx, existingCopy, metav1.UpdateOptions{}) - reportUpdateEvent(recorder, requiredCopy, err) + resourcehelper.ReportUpdateEvent(recorder, requiredCopy, err) return actual, true, err } @@ -210,7 +211,7 @@ func DeleteClusterRole(ctx context.Context, client rbacclientv1.ClusterRolesGett if err != nil { return nil, false, err } - reportDeleteEvent(recorder, required, err) + resourcehelper.ReportDeleteEvent(recorder, required, err) return nil, true, nil } @@ -222,7 +223,7 @@ func DeleteClusterRoleBinding(ctx context.Context, client rbacclientv1.ClusterRo if err != nil { return nil, false, err } - reportDeleteEvent(recorder, required, err) + resourcehelper.ReportDeleteEvent(recorder, required, err) return nil, true, nil } @@ -234,7 +235,7 @@ func DeleteRole(ctx context.Context, client rbacclientv1.RolesGetter, recorder e if err != nil { return nil, false, err } - reportDeleteEvent(recorder, required, err) + resourcehelper.ReportDeleteEvent(recorder, required, err) return nil, true, nil } @@ -246,6 +247,6 @@ func DeleteRoleBinding(ctx context.Context, client rbacclientv1.RoleBindingsGett if err != nil { return nil, false, err } - reportDeleteEvent(recorder, required, err) + resourcehelper.ReportDeleteEvent(recorder, required, err) return nil, true, nil } diff --git a/pkg/operator/resource/resourceapply/storage.go b/pkg/operator/resource/resourceapply/storage.go index 1d08e4cca2..3199d2db05 100644 --- a/pkg/operator/resource/resourceapply/storage.go +++ b/pkg/operator/resource/resourceapply/storage.go @@ -12,6 +12,7 @@ import ( "k8s.io/klog/v2" "github.com/openshift/library-go/pkg/operator/events" + "github.com/openshift/library-go/pkg/operator/resource/resourcehelper" "github.com/openshift/library-go/pkg/operator/resource/resourcemerge" ) @@ -37,7 +38,7 @@ func ApplyStorageClass(ctx context.Context, client storageclientv1.StorageClasse requiredCopy := required.DeepCopy() actual, err := client.StorageClasses().Create( ctx, resourcemerge.WithCleanLabelsAndAnnotations(requiredCopy).(*storagev1.StorageClass), metav1.CreateOptions{}) - reportCreateEvent(recorder, required, err) + resourcehelper.ReportCreateEvent(recorder, required, err) return actual, true, err } if err != nil { @@ -84,7 +85,7 @@ func ApplyStorageClass(ctx context.Context, client storageclientv1.StorageClasse if storageClassNeedsRecreate(existingCopy, requiredCopy) { requiredCopy.ObjectMeta.ResourceVersion = "" err = client.StorageClasses().Delete(ctx, existingCopy.Name, metav1.DeleteOptions{}) - reportDeleteEvent(recorder, requiredCopy, err, "Deleting StorageClass to re-create it with updated parameters") + resourcehelper.ReportDeleteEvent(recorder, requiredCopy, err, "Deleting StorageClass to re-create it with updated parameters") if err != nil && !apierrors.IsNotFound(err) { return existing, false, err } @@ -99,13 +100,13 @@ func ApplyStorageClass(ctx context.Context, client storageclientv1.StorageClasse } else if err != nil { err = fmt.Errorf("failed to re-create StorageClass %s: %s", existingCopy.Name, err) } - reportCreateEvent(recorder, actual, err) + resourcehelper.ReportCreateEvent(recorder, actual, err) return actual, true, err } // Only mutable fields need a change actual, err := client.StorageClasses().Update(ctx, requiredCopy, metav1.UpdateOptions{}) - reportUpdateEvent(recorder, required, err) + resourcehelper.ReportUpdateEvent(recorder, required, err) return actual, true, err } @@ -153,7 +154,7 @@ func ApplyCSIDriver(ctx context.Context, client storageclientv1.CSIDriversGetter requiredCopy := required.DeepCopy() actual, err := client.CSIDrivers().Create( ctx, resourcemerge.WithCleanLabelsAndAnnotations(requiredCopy).(*storagev1.CSIDriver), metav1.CreateOptions{}) - reportCreateEvent(recorder, required, err) + resourcehelper.ReportCreateEvent(recorder, required, err) return actual, true, err } if err != nil { @@ -187,7 +188,7 @@ func ApplyCSIDriver(ctx context.Context, client storageclientv1.CSIDriversGetter if sameSpec { // Update metadata by a simple Update call actual, err := client.CSIDrivers().Update(ctx, existingCopy, metav1.UpdateOptions{}) - reportUpdateEvent(recorder, required, err) + resourcehelper.ReportUpdateEvent(recorder, required, err) return actual, true, err } @@ -195,7 +196,7 @@ func ApplyCSIDriver(ctx context.Context, client storageclientv1.CSIDriversGetter existingCopy.ObjectMeta.ResourceVersion = "" // Spec is read-only after creation. Delete and re-create the object err = client.CSIDrivers().Delete(ctx, existingCopy.Name, metav1.DeleteOptions{}) - reportDeleteEvent(recorder, existingCopy, err, "Deleting CSIDriver to re-create it with updated parameters") + resourcehelper.ReportDeleteEvent(recorder, existingCopy, err, "Deleting CSIDriver to re-create it with updated parameters") if err != nil && !apierrors.IsNotFound(err) { return existing, false, err } @@ -210,7 +211,7 @@ func ApplyCSIDriver(ctx context.Context, client storageclientv1.CSIDriversGetter } else if err != nil { err = fmt.Errorf("failed to re-create CSIDriver %s: %s", existingCopy.Name, err) } - reportCreateEvent(recorder, existingCopy, err) + resourcehelper.ReportCreateEvent(recorder, existingCopy, err) return actual, true, err } @@ -242,7 +243,7 @@ func DeleteStorageClass(ctx context.Context, client storageclientv1.StorageClass if err != nil { return nil, false, err } - reportDeleteEvent(recorder, required, err) + resourcehelper.ReportDeleteEvent(recorder, required, err) return nil, true, nil } @@ -254,6 +255,6 @@ func DeleteCSIDriver(ctx context.Context, client storageclientv1.CSIDriversGette if err != nil { return nil, false, err } - reportDeleteEvent(recorder, required, err) + resourcehelper.ReportDeleteEvent(recorder, required, err) return nil, true, nil } diff --git a/pkg/operator/resource/resourceapply/volumesnapshotclass.go b/pkg/operator/resource/resourceapply/volumesnapshotclass.go index 4c89e65291..763e03d5d5 100644 --- a/pkg/operator/resource/resourceapply/volumesnapshotclass.go +++ b/pkg/operator/resource/resourceapply/volumesnapshotclass.go @@ -13,6 +13,7 @@ import ( "k8s.io/client-go/dynamic" "github.com/openshift/library-go/pkg/operator/events" + "github.com/openshift/library-go/pkg/operator/resource/resourcehelper" ) const ( @@ -124,6 +125,6 @@ func DeleteVolumeSnapshotClass(ctx context.Context, client dynamic.Interface, re if err != nil { return nil, false, err } - reportDeleteEvent(recorder, required, err) + resourcehelper.ReportDeleteEvent(recorder, required, err) return nil, true, nil } diff --git a/pkg/operator/resource/resourcehelper/event_helpers.go b/pkg/operator/resource/resourcehelper/event_helpers.go new file mode 100644 index 0000000000..8e8ebbe96a --- /dev/null +++ b/pkg/operator/resource/resourcehelper/event_helpers.go @@ -0,0 +1,43 @@ +package resourcehelper + +import ( + "fmt" + "strings" + + "k8s.io/apimachinery/pkg/runtime" + + "github.com/openshift/library-go/pkg/operator/events" +) + +func ReportCreateEvent(recorder events.Recorder, obj runtime.Object, originalErr error) { + gvk := GuessObjectGroupVersionKind(obj) + if originalErr == nil { + recorder.Eventf(fmt.Sprintf("%sCreated", gvk.Kind), "Created %s because it was missing", FormatResourceForCLIWithNamespace(obj)) + return + } + recorder.Warningf(fmt.Sprintf("%sCreateFailed", gvk.Kind), "Failed to create %s: %v", FormatResourceForCLIWithNamespace(obj), originalErr) +} + +func ReportUpdateEvent(recorder events.Recorder, obj runtime.Object, originalErr error, details ...string) { + gvk := GuessObjectGroupVersionKind(obj) + switch { + case originalErr != nil: + recorder.Warningf(fmt.Sprintf("%sUpdateFailed", gvk.Kind), "Failed to update %s: %v", FormatResourceForCLIWithNamespace(obj), originalErr) + case len(details) == 0: + recorder.Eventf(fmt.Sprintf("%sUpdated", gvk.Kind), "Updated %s because it changed", FormatResourceForCLIWithNamespace(obj)) + default: + recorder.Eventf(fmt.Sprintf("%sUpdated", gvk.Kind), "Updated %s:\n%s", FormatResourceForCLIWithNamespace(obj), strings.Join(details, "\n")) + } +} + +func ReportDeleteEvent(recorder events.Recorder, obj runtime.Object, originalErr error, details ...string) { + gvk := GuessObjectGroupVersionKind(obj) + switch { + case originalErr != nil: + recorder.Warningf(fmt.Sprintf("%sDeleteFailed", gvk.Kind), "Failed to delete %s: %v", FormatResourceForCLIWithNamespace(obj), originalErr) + case len(details) == 0: + recorder.Eventf(fmt.Sprintf("%sDeleted", gvk.Kind), "Deleted %s", FormatResourceForCLIWithNamespace(obj)) + default: + recorder.Eventf(fmt.Sprintf("%sDeleted", gvk.Kind), "Deleted %s:\n%s", FormatResourceForCLIWithNamespace(obj), strings.Join(details, "\n")) + } +} diff --git a/pkg/operator/resource/resourceapply/event_helpers_test.go b/pkg/operator/resource/resourcehelper/event_helpers_test.go similarity index 95% rename from pkg/operator/resource/resourceapply/event_helpers_test.go rename to pkg/operator/resource/resourcehelper/event_helpers_test.go index 9cac7273a3..d6c771e360 100644 --- a/pkg/operator/resource/resourceapply/event_helpers_test.go +++ b/pkg/operator/resource/resourcehelper/event_helpers_test.go @@ -1,10 +1,10 @@ -package resourceapply +package resourcehelper import ( "errors" "testing" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -51,7 +51,7 @@ func TestReportCreateEvent(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { recorder := events.NewInMemoryRecorder("test") - reportCreateEvent(recorder, test.object, test.err) + ReportCreateEvent(recorder, test.object, test.err) recordedEvents := recorder.Events() if eventCount := len(recordedEvents); eventCount != 1 { @@ -125,9 +125,9 @@ func TestReportUpdateEvent(t *testing.T) { t.Run(test.name, func(t *testing.T) { recorder := events.NewInMemoryRecorder("test") if len(test.details) == 0 { - reportUpdateEvent(recorder, test.object, test.err) + ReportUpdateEvent(recorder, test.object, test.err) } else { - reportUpdateEvent(recorder, test.object, test.err, test.details) + ReportUpdateEvent(recorder, test.object, test.err, test.details) } recordedEvents := recorder.Events() @@ -202,9 +202,9 @@ func TestReportDeleteEvent(t *testing.T) { t.Run(test.name, func(t *testing.T) { recorder := events.NewInMemoryRecorder("test") if len(test.details) == 0 { - reportDeleteEvent(recorder, test.object, test.err) + ReportDeleteEvent(recorder, test.object, test.err) } else { - reportDeleteEvent(recorder, test.object, test.err, test.details) + ReportDeleteEvent(recorder, test.object, test.err, test.details) } recordedEvents := recorder.Events()