refactor: remove clusternodemonitoring webhook

bernot-dev · bernot-dev · commit bc5cd97bc0c4 · 2025-02-10T13:22:02.000-07:00
Remove the ClusterNodeMonitoring validating webhook, in favor of CRD/CEL
validation.
diff --git a/charts/operator/templates/validatingwebhookconfiguration.yaml b/charts/operator/templates/validatingwebhookconfiguration.yaml
@@ -22,28 +22,6 @@ metadata:
     {{- include "prometheus-engine.labels" . | nindent 4 }}
   {{- end }}
 webhooks:
-- name: validate.clusternodemonitorings.gmp-operator.gmp-system.monitoring.googleapis.com
-  admissionReviewVersions:
-  - v1
-  clientConfig:
-    # caBundle populated by operator.
-    service:
-      name: gmp-operator
-      namespace: {{.Values.namespace.system}}
-      port: 443
-      path: /validate/monitoring.googleapis.com/v1/clusternodemonitorings
-  failurePolicy: Fail
-  rules:
-  - resources:
-    - clusternodemonitorings
-    apiGroups:
-    - monitoring.googleapis.com
-    apiVersions:
-    - v1
-    operations:
-    - CREATE
-    - UPDATE
-  sideEffects: None
 - name: validate.rules.gmp-operator.gmp-system.monitoring.googleapis.com
   admissionReviewVersions:
   - v1
diff --git a/manifests/operator.yaml b/manifests/operator.yaml
@@ -1041,28 +1041,6 @@ kind: ValidatingWebhookConfiguration
 metadata:
   name: gmp-operator.gmp-system.monitoring.googleapis.com
 webhooks:
-- name: validate.clusternodemonitorings.gmp-operator.gmp-system.monitoring.googleapis.com
-  admissionReviewVersions:
-  - v1
-  clientConfig:
-    # caBundle populated by operator.
-    service:
-      name: gmp-operator
-      namespace: gmp-system
-      port: 443
-      path: /validate/monitoring.googleapis.com/v1/clusternodemonitorings
-  failurePolicy: Fail
-  rules:
-  - resources:
-    - clusternodemonitorings
-    apiGroups:
-    - monitoring.googleapis.com
-    apiVersions:
-    - v1
-    operations:
-    - CREATE
-    - UPDATE
-  sideEffects: None
 - name: validate.rules.gmp-operator.gmp-system.monitoring.googleapis.com
   admissionReviewVersions:
   - v1
diff --git a/pkg/operator/apis/monitoring/v1/node_types.go b/pkg/operator/apis/monitoring/v1/node_types.go
@@ -15,7 +15,6 @@
 package v1
 
 import (
-	"errors"
 	"fmt"
 	"strings"
 
@@ -26,8 +25,6 @@ import (
 	discoverykube "github.com/prometheus/prometheus/discovery/kubernetes"
 	"github.com/prometheus/prometheus/model/relabel"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
-	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 )
 
 // ScrapeNodeEndpoint specifies a Prometheus metrics endpoint on a node to scrape.
@@ -118,26 +115,6 @@ func (c *ClusterNodeMonitoring) GetMonitoringStatus() *MonitoringStatus {
 	return &c.Status
 }
 
-func (c *ClusterNodeMonitoring) ValidateCreate() (admission.Warnings, error) {
-	if len(c.Spec.Endpoints) == 0 {
-		return nil, errors.New("at least one endpoint is required")
-	}
-	// TODO(freinartz): extract validator into dedicated object (like defaulter). For now using
-	// example values has no adverse effects.
-	_, err := c.ScrapeConfigs("test_project", "test_location", "test_cluster")
-	return nil, err
-}
-
-func (c *ClusterNodeMonitoring) ValidateUpdate(runtime.Object) (admission.Warnings, error) {
-	// Validity does not depend on state changes.
-	return c.ValidateCreate()
-}
-
-func (*ClusterNodeMonitoring) ValidateDelete() (admission.Warnings, error) {
-	// Deletions are always valid.
-	return nil, nil
-}
-
 func (c *ClusterNodeMonitoring) ScrapeConfigs(projectID, location, cluster string) (res []*promconfig.ScrapeConfig, err error) {
 	for i, ep := range c.Spec.Endpoints {
 		sc, err := c.endpointScrapeConfig(&ep, projectID, location, cluster)
diff --git a/pkg/operator/apis/monitoring/v1/node_types_test.go b/pkg/operator/apis/monitoring/v1/node_types_test.go
@@ -15,101 +15,13 @@
 package v1
 
 import (
-	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
 	yaml "gopkg.in/yaml.v2"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-func TestValidateClusterNodeMonitoring(t *testing.T) {
-	cases := []struct {
-		desc        string
-		pm          ClusterNodeMonitoringSpec
-		eps         []ScrapeNodeEndpoint
-		fail        bool
-		errContains string
-	}{
-		{
-			desc: "OK metadata labels",
-			eps: []ScrapeNodeEndpoint{
-				{
-					Interval: "10s",
-				},
-			},
-		},
-		{
-			desc: "Scrape interval missing",
-			eps: []ScrapeNodeEndpoint{
-				{},
-			},
-			fail:        true,
-			errContains: "empty duration string",
-		},
-		{
-			desc: "scrape interval malformed",
-			eps: []ScrapeNodeEndpoint{
-				{
-					Interval: "foo",
-				},
-			},
-			fail:        true,
-			errContains: "invalid scrape interval: not a valid duration string",
-		},
-		{
-			desc: "scrape timeout greater than interval",
-			eps: []ScrapeNodeEndpoint{
-				{
-					Interval: "1s",
-					Timeout:  "2s",
-				},
-			},
-			fail:        true,
-			errContains: "scrape timeout 2s must not be greater than scrape interval 1s",
-		},
-		{
-			// Regression test for https://github.com/GoogleCloudPlatform/prometheus-engine/issues/479
-			desc: "Duplicated job name",
-			eps: []ScrapeNodeEndpoint{
-				{
-					Interval: "10s",
-				},
-				{
-					Interval: "10000ms",
-				},
-			},
-			fail:        true,
-			errContains: "/r1/metrics for endpoints with index 0 and 1;consider creating a separate custom resource (PodMonitoring, etc.) for endpoints that share the same resource name, namespace and port name",
-		},
-	}
-
-	for _, c := range cases {
-		t.Run(c.desc+"", func(t *testing.T) {
-			nm := &ClusterNodeMonitoring{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "r1",
-				},
-				Spec: ClusterNodeMonitoringSpec{
-					Endpoints: c.eps,
-				},
-			}
-			_, err := nm.ValidateCreate()
-			t.Log(err)
-
-			if err == nil && c.fail {
-				t.Fatalf("expected failure but passed")
-			}
-			if err != nil && !c.fail {
-				t.Fatalf("unexpected failure: %s", err)
-			}
-			if err != nil && c.fail && !strings.Contains(err.Error(), c.errContains) {
-				t.Fatalf("expected error to contain %q but got %q", c.errContains, err)
-			}
-		})
-	}
-}
-
 func TestClusterNodeMonitoring_ScrapeConfig(t *testing.T) {
 	// Generate YAML for one complex scrape config and make sure everything
 	// adds up. This primarily verifies that everything is included and marshalling
diff --git a/pkg/operator/webhook.go b/pkg/operator/webhook.go
@@ -56,10 +56,6 @@ func setupAdmissionWebhooks(ctx context.Context, logger logr.Logger, kubeClient
 	scheme := kubeClient.Scheme()
 
 	// Validating webhooks.
-	webhookServer.Register(
-		validatePath(monitoringv1.ClusterNodeMonitoringResource()),
-		admission.ValidatingWebhookFor(scheme, &monitoringv1.ClusterNodeMonitoring{}),
-	)
 	webhookServer.Register(
 		validatePath(monitoringv1.OperatorConfigResource()),
 		admission.WithCustomValidator(scheme, &monitoringv1.OperatorConfig{}, &monitoringv1.OperatorConfigValidator{
