OCPBUGS-1062: update webhook configuration only if required

go.mod

@@ -34,7 +34,7 @@ require (
 	k8s.io/klog/v2 v2.100.1
 	k8s.io/kube-aggregator v0.26.1
 	k8s.io/metrics v0.26.1
-	k8s.io/utils v0.0.0-20230505201702-9f6742963106
+	k8s.io/utils v0.0.0-20230726121419-3b25d923346b
 )
 
 require (

go.sum

@@ -1230,8 +1230,8 @@ k8s.io/metrics v0.26.1 h1:iB+QdMLa2V70a7zb0XYEcaUpPM0y+p4fZN0UtxcPHLk=
 k8s.io/metrics v0.26.1/go.mod h1:fMeLXmK/xgvckFG63GJ0kDjFiQH7P0Dpi5Lvhlo5DXE=
 k8s.io/utils v0.0.0-20191114184206-e782cd3c129f/go.mod h1:sZAwmy6armz5eXlNoLmJcl4F1QuKu7sr+mFQ0byX7Ew=
 k8s.io/utils v0.0.0-20200229041039-0a110f9eb7ab/go.mod h1:sZAwmy6armz5eXlNoLmJcl4F1QuKu7sr+mFQ0byX7Ew=
-k8s.io/utils v0.0.0-20230505201702-9f6742963106 h1:EObNQ3TW2D+WptiYXlApGNLVy0zm/JIBVY9i+M4wpAU=
-k8s.io/utils v0.0.0-20230505201702-9f6742963106/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0=
+k8s.io/utils v0.0.0-20230726121419-3b25d923346b h1:sgn3ZU783SCgtaSJjpcVVlRqd6GSnlTLKgpAAttJvpI=
+k8s.io/utils v0.0.0-20230726121419-3b25d923346b/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0=
 modernc.org/cc v1.0.0/go.mod h1:1Sk4//wdnYJiUIxnW8ddKpaOJCF37yAdqYnkxUpaYxw=
 modernc.org/golex v1.0.0/go.mod h1:b/QX9oBD/LhixY6NDh+IdGv17hgB+51fET1i2kPSmvk=
 modernc.org/mathutil v1.0.0/go.mod h1:wU0vUrJsVWBZ4P6e7xtFJEhFSNsfRLJ8H458uRjg03k=

pkg/alert/rule_controller.go

@@ -29,7 +29,7 @@ import (
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/util/workqueue"
 	"k8s.io/klog/v2"
-	"k8s.io/utils/pointer"
+	"k8s.io/utils/ptr"
 
 	osmv1 "github.com/openshift/api/monitoring/v1"
 	"github.com/openshift/cluster-monitoring-operator/pkg/client"
@@ -281,8 +281,8 @@ func (rc *RuleController) sync(ctx context.Context, key string) error {
 					Kind:               rule.Kind,
 					Name:               rule.Name,
 					UID:                rule.UID,
-					Controller:         pointer.Bool(true),
-					BlockOwnerDeletion: pointer.Bool(true),
+					Controller:         ptr.To(true),
+					BlockOwnerDeletion: ptr.To(true),
 				},
 			},
 			Labels: map[string]string{

pkg/client/client.go

@@ -96,6 +96,7 @@ type Client struct {
 	aggclient   aggregatorclient.Interface
 
 	eventRecorder events.Recorder
+	resourceCache resourceapply.ResourceCache
 }
 
 func NewForConfig(cfg *rest.Config, version string, namespace, userWorkloadNamespace string, options ...Option) (*Client, error) {
@@ -264,6 +265,7 @@ func New(version string, namespace, userWorkloadNamespace string, options ...Opt
 		version:               version,
 		namespace:             namespace,
 		userWorkloadNamespace: userWorkloadNamespace,
+		resourceCache:         resourceapply.NewResourceCache(),
 	}
 
 	for _, opt := range options {
@@ -485,28 +487,18 @@ func (c *Client) CreateOrUpdateAlertRelabelConfig(ctx context.Context, arc *osmv
 }
 
 func (c *Client) CreateOrUpdateValidatingWebhookConfiguration(ctx context.Context, w *admissionv1.ValidatingWebhookConfiguration) error {
-	admclient := c.kclient.AdmissionregistrationV1().ValidatingWebhookConfigurations()
-	existing, err := admclient.Get(ctx, w.GetName(), metav1.GetOptions{})
-	if apierrors.IsNotFound(err) {
-		_, err := admclient.Create(ctx, w, metav1.CreateOptions{})
-		return errors.Wrap(err, "creating ValidatingWebhookConfiguration object failed")
-	}
+	_, _, err := resourceapply.ApplyValidatingWebhookConfigurationImproved(
+		ctx,
+		c.kclient.AdmissionregistrationV1(),
+		c.eventRecorder,
+		w,
+		c.resourceCache,
+	)
 	if err != nil {
-		return errors.Wrap(err, "retrieving ValidatingWebhookConfiguration object failed")
+		return errors.Wrap(err, "updating ValidatingWebhookConfiguration object failed")
 	}
 
-	required := w.DeepCopy()
-	required.ResourceVersion = existing.ResourceVersion
-	// retain the CABundle that service-ca-operator created if the proper annotation is found
-	if val, ok := required.Annotations["service.beta.openshift.io/inject-cabundle"]; ok && val == "true" {
-		for i := range required.Webhooks {
-			if len(existing.Webhooks[i].ClientConfig.CABundle) > 0 {
-				required.Webhooks[i].ClientConfig.CABundle = existing.Webhooks[i].ClientConfig.CABundle
-			}
-		}
-	}
-	_, err = admclient.Update(ctx, required, metav1.UpdateOptions{})
-	return errors.Wrap(err, "updating ValidatingWebhookConfiguration object failed")
+	return nil
 }
 
 func (c *Client) CreateOrUpdateSecurityContextConstraints(ctx context.Context, s *secv1.SecurityContextConstraints) error {
@@ -1707,8 +1699,15 @@ func (c *Client) CreateOrUpdateClusterRoleBinding(ctx context.Context, crb *rbac
 }
 
 func (c *Client) CreateOrUpdateServiceAccount(ctx context.Context, sa *v1.ServiceAccount) error {
-	_, _, err := resourceapply.ApplyServiceAccount(ctx, c.kclient.CoreV1(), c.eventRecorder, sa)
-	return errors.Wrap(err, "patching ServiceAccount object failed")
+	_, _, err := resourceapply.ApplyServiceAccountImproved(
+		ctx,
+		c.kclient.CoreV1(),
+		c.eventRecorder,
+		sa,
+		c.resourceCache,
+	)
+
+	return errors.Wrap(err, "updating ServiceAccount object failed")
 }
 
 func (c *Client) CreateOrUpdateServiceMonitor(ctx context.Context, sm *monv1.ServiceMonitor) error {

pkg/client/client_test.go

@@ -15,20 +15,21 @@
 package client
 
 import (
-	"bytes"
 	"context"
 	"reflect"
 	"testing"
 
 	secv1 "github.com/openshift/api/security/v1"
 	"github.com/openshift/library-go/pkg/operator/events"
+	"github.com/openshift/library-go/pkg/operator/resource/resourceapply"
 	admissionv1 "k8s.io/api/admissionregistration/v1"
 	appsv1 "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/utils/ptr"
 
 	routev1 "github.com/openshift/api/route/v1"
 
@@ -1059,16 +1060,19 @@ func TestCreateOrUpdateServiceAccount(t *testing.T) {
 				},
 				Secrets: tc.initialSecrets,
 			}
+
 			var c Client
 			if tc.initialAnnotations == nil && tc.initialLabels == nil {
 				c = Client{
 					kclient:       fake.NewSimpleClientset(),
 					eventRecorder: eventRecorder,
+					resourceCache: resourceapply.NewResourceCache(),
 				}
 			} else {
 				c = Client{
 					kclient:       fake.NewSimpleClientset(sa.DeepCopy()),
 					eventRecorder: eventRecorder,
+					resourceCache: resourceapply.NewResourceCache(),
 				}
 				_, err := c.kclient.CoreV1().ServiceAccounts(ns).Get(ctx, sa.Name, metav1.GetOptions{})
 				if err != nil {
@@ -2037,66 +2041,82 @@ func TestCreateOrUpdateAlertmanager(t *testing.T) {
 
 func TestCreateOrUpdateValidatingWebhookConfiguration(t *testing.T) {
 	ctx := context.Background()
-	testCases := []struct {
-		name               string
-		initialAnnotations map[string]string
-		initialCABundle    []byte
-		expectedCABundle   []byte
-	}{
-		{
-			name: "retain CA bundle",
-			initialAnnotations: map[string]string{
-				"service.beta.openshift.io/inject-cabundle": "true",
+	webhook := &admissionv1.ValidatingWebhookConfiguration{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "test",
+			Labels: map[string]string{
+				"app.kubernetes.io/part-of": "openshift-monitoring",
+			},
+			Annotations: map[string]string{
+				"foo": "bar",
 			},
-			initialCABundle:  []byte("fooCA"),
-			expectedCABundle: []byte("fooCA"),
 		},
-		{
-			name:               "drop CA bundle of annotation is missing",
-			initialAnnotations: map[string]string{},
-			initialCABundle:    []byte("fooCA"),
-			expectedCABundle:   []byte(""),
+		Webhooks: []admissionv1.ValidatingWebhook{
+			{
+				ClientConfig: admissionv1.WebhookClientConfig{
+					CABundle: []byte("<PEM-encoded CA bundle>"),
+				},
+			},
 		},
 	}
 
-	for _, tc := range testCases {
-		t.Run(tc.name, func(st *testing.T) {
-			webhook := admissionv1.ValidatingWebhookConfiguration{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:        "test",
-					Annotations: tc.initialAnnotations,
-				},
-				Webhooks: []admissionv1.ValidatingWebhook{
-					{
-						ClientConfig: admissionv1.WebhookClientConfig{
-							CABundle: tc.initialCABundle,
-						},
-					},
-				},
-			}
+	c := Client{
+		kclient:       fake.NewSimpleClientset(webhook.DeepCopy()),
+		eventRecorder: events.NewInMemoryRecorder("cluster-monitoring-operator"),
+		resourceCache: resourceapply.NewResourceCache(),
+	}
 
-			c := Client{
-				kclient: fake.NewSimpleClientset(webhook.DeepCopy()),
-			}
+	if _, err := c.kclient.AdmissionregistrationV1().ValidatingWebhookConfigurations().Get(ctx, webhook.Name, metav1.GetOptions{}); err != nil {
+		t.Fatal(err)
+	}
 
-			if _, err := c.kclient.AdmissionregistrationV1().ValidatingWebhookConfigurations().Get(ctx, webhook.Name, metav1.GetOptions{}); err != nil {
-				t.Fatal(err)
-			}
+	// Labels and annotations should be merged.
+	webhook.Labels = map[string]string{
+		"app.kubernetes.io/name": "prometheus-operator",
+	}
+	webhook.Annotations = map[string]string{
+		"service.beta.openshift.io/inject-cabundle": "true",
+	}
+	// CA bundle should be retained.
+	webhook.Webhooks[0].ClientConfig.CABundle = nil
+	// Failure policy should be overwritten.
+	webhook.Webhooks[0].FailurePolicy = ptr.To(admissionv1.Ignore)
 
-			webhook.Webhooks[0].ClientConfig.CABundle = []byte{}
-			if err := c.CreateOrUpdateValidatingWebhookConfiguration(ctx, &webhook); err != nil {
-				t.Fatal(err)
-			}
-			newWebhook, err := c.kclient.AdmissionregistrationV1().ValidatingWebhookConfigurations().Get(ctx, webhook.Name, metav1.GetOptions{})
-			after := newWebhook.Webhooks[0].ClientConfig.CABundle
-			if err != nil {
-				t.Fatal(err)
-			}
+	if err := c.CreateOrUpdateValidatingWebhookConfiguration(ctx, webhook); err != nil {
+		t.Fatal(err)
+	}
 
-			if bytes.Compare(tc.expectedCABundle, after) != 0 {
-				t.Errorf("%q: expected CABundle %q, got %q", tc.name, tc.expectedCABundle, after)
-			}
-		})
+	newWebhook, err := c.kclient.AdmissionregistrationV1().ValidatingWebhookConfigurations().Get(ctx, webhook.Name, metav1.GetOptions{})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if string(newWebhook.Webhooks[0].ClientConfig.CABundle) != "<PEM-encoded CA bundle>" {
+		t.Fatalf("expected CABundle %q, got %q", "<PEM-encoded CA bundle>", newWebhook.Webhooks[0].ClientConfig.CABundle)
+	}
+
+	expected := map[string]string{
+		"app.kubernetes.io/name":    "prometheus-operator",
+		"app.kubernetes.io/part-of": "openshift-monitoring",
+	}
+	if !reflect.DeepEqual(newWebhook.Labels, expected) {
+		t.Fatalf("expected labels %v, got %v", expected, newWebhook.Labels)
+	}
+
+	expected = map[string]string{
+		"foo": "bar",
+		"service.beta.openshift.io/inject-cabundle": "true",
+	}
+	if !reflect.DeepEqual(newWebhook.Annotations, expected) {
+		t.Fatalf("expected annotations %v, got %v", expected, newWebhook.Annotations)
+	}
+
+	if newWebhook.Webhooks[0].FailurePolicy == nil {
+		t.Fatal("expected non-nil failure policy")
+	}
+
+	if *newWebhook.Webhooks[0].FailurePolicy != admissionv1.Ignore {
+		t.Fatalf("expected failure policy %q, got %q", admissionv1.Ignore, *newWebhook.Webhooks[0].FailurePolicy)
 	}
 }