chore:refactor opampbridge webhook

apis/v1alpha1/opampbridge_webhook.go

@@ -15,33 +15,69 @@
 package v1alpha1
 
 import (
+	"context"
 	"fmt"
 	"strings"
 
+	"github.com/go-logr/logr"
+
+	"github.com/open-telemetry/opentelemetry-operator/internal/config"
+
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation"
 	ctrl "sigs.k8s.io/controller-runtime"
-	logf "sigs.k8s.io/controller-runtime/pkg/log"
-	"sigs.k8s.io/controller-runtime/pkg/webhook"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 )
 
-// log is for logging in this package.
-var opampbridgelog = logf.Log.WithName("opampbridge-resource")
+var (
+	_ admission.CustomValidator = &OpAMPBridgeWebhook{}
+	_ admission.CustomDefaulter = &OpAMPBridgeWebhook{}
+)
 
-func (r *OpAMPBridge) SetupWebhookWithManager(mgr ctrl.Manager) error {
-	return ctrl.NewWebhookManagedBy(mgr).
-		For(r).
-		Complete()
+//+kubebuilder:webhook:path=/mutate-opentelemetry-io-v1alpha1-opampbridge,mutating=true,failurePolicy=fail,sideEffects=None,groups=opentelemetry.io,resources=opampbridges,verbs=create;update,versions=v1alpha1,name=mopampbridge.kb.io,admissionReviewVersions=v1
+//+kubebuilder:webhook:path=/validate-opentelemetry-io-v1alpha1-opampbridge,mutating=false,failurePolicy=fail,sideEffects=None,groups=opentelemetry.io,resources=opampbridges,verbs=create;update,versions=v1alpha1,name=vopampbridgecreateupdate.kb.io,admissionReviewVersions=v1
+//+kubebuilder:webhook:path=/validate-opentelemetry-io-v1alpha1-opampbridge,mutating=false,failurePolicy=ignore,sideEffects=None,groups=opentelemetry.io,resources=opampbridges,verbs=delete,versions=v1alpha1,name=vopampbridgedelete.kb.io,admissionReviewVersions=v1
+//+kubebuilder:object:generate=false
+
+type OpAMPBridgeWebhook struct {
+	logger logr.Logger
+	cfg    config.Config
+	scheme *runtime.Scheme
 }
 
-//+kubebuilder:webhook:path=/mutate-opentelemetry-io-v1alpha1-opampbridge,mutating=true,failurePolicy=fail,sideEffects=None,groups=opentelemetry.io,resources=opampbridges,verbs=create;update,versions=v1alpha1,name=mopampbridge.kb.io,admissionReviewVersions=v1
+func (o *OpAMPBridgeWebhook) Default(ctx context.Context, obj runtime.Object) error {
+	opampBridge, ok := obj.(*OpAMPBridge)
+	if !ok {
+		return fmt.Errorf("expected an OpAMPBridge, received %T", obj)
+	}
+	return o.defaulter(opampBridge)
+}
 
-var _ webhook.Defaulter = &OpAMPBridge{}
+func (c OpAMPBridgeWebhook) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
+	opampBridge, ok := obj.(*OpAMPBridge)
+	if !ok {
+		return nil, fmt.Errorf("expected an OpAMPBridge, received %T", obj)
+	}
+	return c.validate(opampBridge)
+}
+
+func (c OpAMPBridgeWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
+	opampBridge, ok := newObj.(*OpAMPBridge)
+	if !ok {
+		return nil, fmt.Errorf("expected an OpAMPBridge, received %T", newObj)
+	}
+	return c.validate(opampBridge)
+}
+
+func (o OpAMPBridgeWebhook) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
+	opampBridge, ok := obj.(*OpAMPBridge)
+	if !ok || opampBridge == nil {
+		return nil, fmt.Errorf("expected an OpAMPBridge, received %T", obj)
+	}
+	return o.validate(opampBridge)
+}
 
-// Default implements webhook.Defaulter so a webhook will be registered for the type.
-func (r *OpAMPBridge) Default() {
-	opampbridgelog.Info("default", "name", r.Name)
+func (o OpAMPBridgeWebhook) defaulter(r *OpAMPBridge) error {
 	if len(r.Spec.UpgradeStrategy) == 0 {
 		r.Spec.UpgradeStrategy = UpgradeStrategyAutomatic
 	}
@@ -66,55 +102,48 @@ func (r *OpAMPBridge) Default() {
 	if !enabled || !found {
 		r.Spec.Capabilities[OpAMPBridgeCapabilityReportsStatus] = true
 	}
+	return nil
 }
 
-//+kubebuilder:webhook:path=/validate-opentelemetry-io-v1alpha1-opampbridge,mutating=false,failurePolicy=fail,sideEffects=None,groups=opentelemetry.io,resources=opampbridges,verbs=create;update;delete,versions=v1alpha1,name=vopampbridge.kb.io,admissionReviewVersions=v1
-
-var _ webhook.Validator = &OpAMPBridge{}
-
-// ValidateCreate implements webhook.Validator so a webhook will be registered for the type.
-func (r *OpAMPBridge) ValidateCreate() (admission.Warnings, error) {
-	opampbridgelog.Info("validate create", "name", r.Name)
-	return nil, r.validateCRDSpec()
-}
-
-// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type.
-func (r *OpAMPBridge) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
-	opampbridgelog.Info("validate update", "name", r.Name)
-	return nil, r.validateCRDSpec()
-}
-
-// ValidateDelete implements webhook.Validator so a webhook will be registered for the type.
-func (r *OpAMPBridge) ValidateDelete() (admission.Warnings, error) {
-	opampbridgelog.Info("validate delete", "name", r.Name)
-	return nil, nil
-}
-
-func (r *OpAMPBridge) validateCRDSpec() error {
-
-	// check required fields
+func (o OpAMPBridgeWebhook) validate(r *OpAMPBridge) (admission.Warnings, error) {
+	warnings := admission.Warnings{}
 
+	// validate OpAMP server endpoint
 	if len(strings.TrimSpace(r.Spec.Endpoint)) == 0 {
-		return fmt.Errorf("the OpAMP server endpoint is not specified")
+		return warnings, fmt.Errorf("the OpAMP server endpoint is not specified")
 	}
 
+	// validate OpAMPBridge capabilities
 	if len(r.Spec.Capabilities) == 0 {
-		return fmt.Errorf("the capabilities supported by OpAMP Bridge are not specified")
+		return warnings, fmt.Errorf("the capabilities supported by OpAMP Bridge are not specified")
 	}
 
 	// validate port config
 	for _, p := range r.Spec.Ports {
 		nameErrs := validation.IsValidPortName(p.Name)
 		numErrs := validation.IsValidPortNum(int(p.Port))
 		if len(nameErrs) > 0 || len(numErrs) > 0 {
-			return fmt.Errorf("the OpAMPBridge Spec Ports configuration is incorrect, port name '%s' errors: %s, num '%d' errors: %s",
+			return warnings, fmt.Errorf("the OpAMPBridge Spec Ports configuration is incorrect, port name '%s' errors: %s, num '%d' errors: %s",
 				p.Name, nameErrs, p.Port, numErrs)
 		}
 	}
 
 	// check for maximum replica count
 	if r.Spec.Replicas != nil && *r.Spec.Replicas > 1 {
-		return fmt.Errorf("replica count must not be greater than 1")
+		return warnings, fmt.Errorf("replica count must not be greater than 1")
 	}
-	return nil
+	return warnings, nil
+}
+
+func SetupOpAMPBridgeWebhook(mgr ctrl.Manager, cfg config.Config) error {
+	webhook := &OpAMPBridgeWebhook{
+		logger: mgr.GetLogger().WithValues("handler", "OpAMPBridgeWebhook"),
+		scheme: mgr.GetScheme(),
+		cfg:    cfg,
+	}
+	return ctrl.NewWebhookManagedBy(mgr).
+		For(&OpAMPBridge{}).
+		WithValidator(webhook).
+		WithDefaulter(webhook).
+		Complete()
 }

apis/v1alpha1/opampbridge_webhook_test.go

@@ -15,9 +15,17 @@
 package v1alpha1
 
 import (
+	"context"
+	"fmt"
+	"os"
 	"testing"
 
+	"github.com/go-logr/logr"
+
+	"github.com/open-telemetry/opentelemetry-operator/internal/config"
+
 	"github.com/stretchr/testify/assert"
+
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@ -26,13 +34,18 @@ func TestOpAMPBridgeDefaultingWebhook(t *testing.T) {
 	one := int32(1)
 	five := int32(5)
 
+	if err := AddToScheme(testScheme); err != nil {
+		fmt.Printf("failed to register scheme: %v", err)
+		os.Exit(1)
+	}
+
 	tests := []struct {
 		name        string
 		opampBridge OpAMPBridge
 		expected    OpAMPBridge
 	}{
 		{
-			name:        "provide only required values in spec",
+			name:        "all fields default",
 			opampBridge: OpAMPBridge{},
 			expected: OpAMPBridge{
 				ObjectMeta: metav1.ObjectMeta{
@@ -48,7 +61,7 @@ func TestOpAMPBridgeDefaultingWebhook(t *testing.T) {
 			},
 		},
 		{
-			name: "provided optional values in spec",
+			name: "provided values in spec",
 			opampBridge: OpAMPBridge{
 				Spec: OpAMPBridgeSpec{
 					Replicas:        &five,
@@ -96,7 +109,18 @@ func TestOpAMPBridgeDefaultingWebhook(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			test.opampBridge.Default()
+			webhook := &OpAMPBridgeWebhook{
+				logger: logr.Discard(),
+				scheme: testScheme,
+				cfg: config.New(
+					config.WithCollectorImage("collector:v0.0.0"),
+					config.WithTargetAllocatorImage("ta:v0.0.0"),
+					config.WithOperatorOpAMPBridgeImage("opampbridge:v0.0.0"),
+				),
+			}
+			ctx := context.Background()
+			err := webhook.Default(ctx, &test.opampBridge)
+			assert.NoError(t, err)
 			assert.Equal(t, test.expected, test.opampBridge)
 		})
 	}
@@ -107,9 +131,10 @@ func TestOpAMPBridgeValidatingWebhook(t *testing.T) {
 	two := int32(2)
 
 	tests := []struct { //nolint:govet
-		name        string
-		opampBridge OpAMPBridge
-		expectedErr string
+		name             string
+		opampBridge      OpAMPBridge
+		expectedErr      string
+		expectedWarnings []string
 	}{
 		{
 			name: "specify all required fields, should not return error",
@@ -270,11 +295,26 @@ func TestOpAMPBridgeValidatingWebhook(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			err := test.opampBridge.validateCRDSpec()
+			webhook := &OpAMPBridgeWebhook{
+				logger: logr.Discard(),
+				scheme: testScheme,
+				cfg: config.New(
+					config.WithCollectorImage("collector:v0.0.0"),
+					config.WithTargetAllocatorImage("ta:v0.0.0"),
+					config.WithOperatorOpAMPBridgeImage("opampbridge:v0.0.0"),
+				),
+			}
+			ctx := context.Background()
+			warnings, err := webhook.ValidateCreate(ctx, &test.opampBridge)
 			if test.expectedErr == "" {
 				assert.NoError(t, err)
 				return
 			}
+			if len(test.expectedWarnings) == 0 {
+				assert.Empty(t, warnings, test.expectedWarnings)
+			} else {
+				assert.ElementsMatch(t, warnings, test.expectedWarnings)
+			}
 			assert.ErrorContains(t, err, test.expectedErr)
 		})
 	}

bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml

@@ -578,7 +578,7 @@ spec:
     containerPort: 443
     deploymentName: opentelemetry-operator-controller-manager
     failurePolicy: Fail
-    generateName: vopampbridge.kb.io
+    generateName: vopampbridgecreateupdate.kb.io
     rules:
     - apiGroups:
       - opentelemetry.io
@@ -587,6 +587,24 @@ spec:
       operations:
       - CREATE
       - UPDATE
+      resources:
+      - opampbridges
+    sideEffects: None
+    targetPort: 9443
+    type: ValidatingAdmissionWebhook
+    webhookPath: /validate-opentelemetry-io-v1alpha1-opampbridge
+  - admissionReviewVersions:
+    - v1
+    containerPort: 443
+    deploymentName: opentelemetry-operator-controller-manager
+    failurePolicy: Ignore
+    generateName: vopampbridgedelete.kb.io
+    rules:
+    - apiGroups:
+      - opentelemetry.io
+      apiVersions:
+      - v1alpha1
+      operations:
       - DELETE
       resources:
       - opampbridges

config/webhook/manifests.yaml

@@ -137,7 +137,7 @@ webhooks:
       namespace: system
       path: /validate-opentelemetry-io-v1alpha1-opampbridge
   failurePolicy: Fail
-  name: vopampbridge.kb.io
+  name: vopampbridgecreateupdate.kb.io
   rules:
   - apiGroups:
     - opentelemetry.io
@@ -146,6 +146,24 @@ webhooks:
     operations:
     - CREATE
     - UPDATE
+    resources:
+    - opampbridges
+  sideEffects: None
+- admissionReviewVersions:
+  - v1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: system
+      path: /validate-opentelemetry-io-v1alpha1-opampbridge
+  failurePolicy: Ignore
+  name: vopampbridgedelete.kb.io
+  rules:
+  - apiGroups:
+    - opentelemetry.io
+    apiVersions:
+    - v1alpha1
+    operations:
     - DELETE
     resources:
     - opampbridges

controllers/suite_test.go

@@ -131,7 +131,7 @@ func TestMain(m *testing.M) {
 		os.Exit(1)
 	}
 
-	if err = (&v1alpha1.OpAMPBridge{}).SetupWebhookWithManager(mgr); err != nil {
+	if err = v1alpha1.SetupOpAMPBridgeWebhook(mgr, config.New()); err != nil {
 		fmt.Printf("failed to SetupWebhookWithManager: %v", err)
 		os.Exit(1)
 	}

main.go

@@ -274,7 +274,7 @@ func main() {
 				}),
 		})
 
-		if err = (&otelv1alpha1.OpAMPBridge{}).SetupWebhookWithManager(mgr); err != nil {
+		if err = otelv1alpha1.SetupOpAMPBridgeWebhook(mgr, cfg); err != nil {
 			setupLog.Error(err, "unable to create webhook", "webhook", "OpAMPBridge")
 			os.Exit(1)
 		}