From 04942afde626f3fde3b436a697fedddd4b27551d Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Tue, 25 Jun 2024 14:50:28 -0700 Subject: [PATCH 01/38] initial commit --- apis/v1beta1/collector_webhook.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/apis/v1beta1/collector_webhook.go b/apis/v1beta1/collector_webhook.go index 930c2c819f..511a87a00b 100644 --- a/apis/v1beta1/collector_webhook.go +++ b/apis/v1beta1/collector_webhook.go @@ -17,6 +17,9 @@ package v1beta1 import ( "context" "fmt" + "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" + "github.com/open-telemetry/opentelemetry-operator/internal/manifests" + "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" "strings" "github.com/go-logr/logr" @@ -343,6 +346,20 @@ func (c CollectorWebhook) validate(ctx context.Context, r *OpenTelemetryCollecto if r.Spec.Mode != ModeDeployment && len(r.Spec.DeploymentUpdateStrategy.Type) > 0 { return warnings, fmt.Errorf("the OpenTelemetry Collector mode is set to %s, which does not support the attribute 'deploymentUpdateStrategy'", r.Spec.Mode) } + + _, err = collector.Build(manifests.Params{ + Client: nil, + Recorder: nil, + Scheme: nil, + Log: logr.Logger{}, + OtelCol: OpenTelemetryCollector{}, + TargetAllocator: nil, + OpAMPBridge: v1alpha1.OpAMPBridge{}, + Config: config.Config{}, + }) + if err != nil { + return nil, err + } return warnings, nil } From ffa2407bf4aa972df065115bf1efd80ed5d264bd Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Mon, 15 Jul 2024 11:34:02 -0700 Subject: [PATCH 02/38] added enhanced reconciliation errors --- apis/v1beta1/collector_webhook.go | 29 +++++++------------ apis/v1beta1/collector_webhook_test.go | 4 +++ .../opentelemetrycollector_controller.go | 4 +-- main.go | 23 ++++++++++++--- 4 files changed, 35 insertions(+), 25 deletions(-) diff --git a/apis/v1beta1/collector_webhook.go b/apis/v1beta1/collector_webhook.go index 511a87a00b..912807386f 100644 --- a/apis/v1beta1/collector_webhook.go +++ b/apis/v1beta1/collector_webhook.go @@ -17,9 +17,6 @@ package v1beta1 import ( "context" "fmt" - "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" - "github.com/open-telemetry/opentelemetry-operator/internal/manifests" - "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" "strings" "github.com/go-logr/logr" @@ -80,6 +77,7 @@ type CollectorWebhook struct { scheme *runtime.Scheme reviewer *rbac.Reviewer metrics *Metrics + bv buildValidator } func (c CollectorWebhook) Default(_ context.Context, obj runtime.Object) error { @@ -178,8 +176,12 @@ func (c CollectorWebhook) ValidateCreate(ctx context.Context, obj runtime.Object if c.metrics != nil { c.metrics.create(ctx, otelcol) } + newWarnings, err := c.bv(*otelcol) + if err != nil { + return append(warnings, newWarnings...), err + } - return warnings, nil + return append(warnings, newWarnings...), nil } func (c CollectorWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { @@ -346,20 +348,6 @@ func (c CollectorWebhook) validate(ctx context.Context, r *OpenTelemetryCollecto if r.Spec.Mode != ModeDeployment && len(r.Spec.DeploymentUpdateStrategy.Type) > 0 { return warnings, fmt.Errorf("the OpenTelemetry Collector mode is set to %s, which does not support the attribute 'deploymentUpdateStrategy'", r.Spec.Mode) } - - _, err = collector.Build(manifests.Params{ - Client: nil, - Recorder: nil, - Scheme: nil, - Log: logr.Logger{}, - OtelCol: OpenTelemetryCollector{}, - TargetAllocator: nil, - OpAMPBridge: v1alpha1.OpAMPBridge{}, - Config: config.Config{}, - }) - if err != nil { - return nil, err - } return warnings, nil } @@ -471,13 +459,16 @@ func checkAutoscalerSpec(autoscaler *AutoscalerSpec) error { return nil } -func SetupCollectorWebhook(mgr ctrl.Manager, cfg config.Config, reviewer *rbac.Reviewer, metrics *Metrics) error { +type buildValidator func(c OpenTelemetryCollector) (admission.Warnings, error) + +func SetupCollectorWebhook(mgr ctrl.Manager, cfg config.Config, reviewer *rbac.Reviewer, metrics *Metrics, bv buildValidator) error { cvw := &CollectorWebhook{ reviewer: reviewer, logger: mgr.GetLogger().WithValues("handler", "CollectorWebhook", "version", "v1beta1"), scheme: mgr.GetScheme(), cfg: cfg, metrics: metrics, + bv: bv, } return ctrl.NewWebhookManagedBy(mgr). For(&OpenTelemetryCollector{}). diff --git a/apis/v1beta1/collector_webhook_test.go b/apis/v1beta1/collector_webhook_test.go index 8262a6fd07..a1b46d3cc1 100644 --- a/apis/v1beta1/collector_webhook_test.go +++ b/apis/v1beta1/collector_webhook_test.go @@ -18,6 +18,7 @@ import ( "context" "fmt" "os" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "testing" "github.com/go-logr/logr" @@ -1247,6 +1248,9 @@ func TestOTELColValidatingWebhook(t *testing.T) { config.WithTargetAllocatorImage("ta:v0.0.0"), ), reviewer: getReviewer(test.shouldFailSar), + bv: func(col OpenTelemetryCollector) (admission.Warnings, error) { + return nil, fmt.Errorf("error1") + }, } ctx := context.Background() warnings, err := cvw.ValidateCreate(ctx, &test.otelcol) diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index ea6f8908d6..88a6f39ed9 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -168,7 +168,7 @@ func (r *OpenTelemetryCollectorReconciler) getConfigMapsToRemove(configVersionsT return ownedConfigMaps } -func (r *OpenTelemetryCollectorReconciler) getParams(instance v1beta1.OpenTelemetryCollector) (manifests.Params, error) { +func (r *OpenTelemetryCollectorReconciler) GetParams(instance v1beta1.OpenTelemetryCollector) (manifests.Params, error) { p := manifests.Params{ Config: r.config, Client: r.Client, @@ -229,7 +229,7 @@ func (r *OpenTelemetryCollectorReconciler) Reconcile(ctx context.Context, req ct return ctrl.Result{}, client.IgnoreNotFound(err) } - params, err := r.getParams(instance) + params, err := r.GetParams(instance) if err != nil { log.Error(err, "Failed to create manifest.Params") return ctrl.Result{}, err diff --git a/main.go b/main.go index 798bbeada8..309572bce0 100644 --- a/main.go +++ b/main.go @@ -19,6 +19,7 @@ import ( "crypto/tls" "flag" "fmt" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect" "os" "regexp" "runtime" @@ -50,10 +51,10 @@ import ( otelv1alpha1 "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" otelv1beta1 "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" "github.com/open-telemetry/opentelemetry-operator/controllers" - "github.com/open-telemetry/opentelemetry-operator/internal/autodetect" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus" "github.com/open-telemetry/opentelemetry-operator/internal/config" + collectorManifests "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" "github.com/open-telemetry/opentelemetry-operator/internal/rbac" "github.com/open-telemetry/opentelemetry-operator/internal/version" "github.com/open-telemetry/opentelemetry-operator/internal/webhook/podmutation" @@ -353,13 +354,15 @@ func main() { os.Exit(1) } - if err = controllers.NewReconciler(controllers.Params{ + collectorReconciler := controllers.NewReconciler(controllers.Params{ Client: mgr.GetClient(), Log: ctrl.Log.WithName("controllers").WithName("OpenTelemetryCollector"), Scheme: mgr.GetScheme(), Config: cfg, Recorder: mgr.GetEventRecorderFor("opentelemetry-operator"), - }).SetupWithManager(mgr); err != nil { + }) + + if err = collectorReconciler.SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "OpenTelemetryCollector") os.Exit(1) } @@ -391,7 +394,19 @@ func main() { } - if err = otelv1beta1.SetupCollectorWebhook(mgr, cfg, reviewer, crdMetrics); err != nil { + bv := func(collector otelv1beta1.OpenTelemetryCollector) (admission.Warnings, error) { + params, err := collectorReconciler.GetParams(collector) + if err != nil { + return nil, err + } + _, err = collectorManifests.Build(params) + if err != nil { + return nil, err + } + return nil, nil + } + + if err = otelv1beta1.SetupCollectorWebhook(mgr, cfg, reviewer, crdMetrics, bv); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "OpenTelemetryCollector") os.Exit(1) } From 8c599582bd1ff679b5355291a79083f039de1113 Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Mon, 15 Jul 2024 15:38:47 -0700 Subject: [PATCH 03/38] tests --- apis/v1beta1/collector_webhook.go | 12 +++++++----- apis/v1beta1/collector_webhook_test.go | 2 +- controllers/suite_test.go | 2 +- .../webhook/podmutation/webhookhandler_suite_test.go | 2 +- pkg/collector/upgrade/suite_test.go | 2 +- 5 files changed, 11 insertions(+), 9 deletions(-) diff --git a/apis/v1beta1/collector_webhook.go b/apis/v1beta1/collector_webhook.go index 912807386f..ed3fd259b0 100644 --- a/apis/v1beta1/collector_webhook.go +++ b/apis/v1beta1/collector_webhook.go @@ -176,12 +176,14 @@ func (c CollectorWebhook) ValidateCreate(ctx context.Context, obj runtime.Object if c.metrics != nil { c.metrics.create(ctx, otelcol) } - newWarnings, err := c.bv(*otelcol) - if err != nil { - return append(warnings, newWarnings...), err + if c.bv != nil { + newWarnings, err := c.bv(*otelcol) + if err != nil { + return append(warnings, newWarnings...), err + } + warnings = append(warnings, newWarnings...) } - - return append(warnings, newWarnings...), nil + return warnings, nil } func (c CollectorWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { diff --git a/apis/v1beta1/collector_webhook_test.go b/apis/v1beta1/collector_webhook_test.go index a1b46d3cc1..b8247bcd50 100644 --- a/apis/v1beta1/collector_webhook_test.go +++ b/apis/v1beta1/collector_webhook_test.go @@ -1249,7 +1249,7 @@ func TestOTELColValidatingWebhook(t *testing.T) { ), reviewer: getReviewer(test.shouldFailSar), bv: func(col OpenTelemetryCollector) (admission.Warnings, error) { - return nil, fmt.Errorf("error1") + return nil, nil }, } ctx := context.Background() diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 0b8ee89adf..f24a2fd6ac 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -177,7 +177,7 @@ func TestMain(m *testing.M) { } reviewer := rbac.NewReviewer(clientset) - if err = v1beta1.SetupCollectorWebhook(mgr, config.New(), reviewer, nil); err != nil { + if err = v1beta1.SetupCollectorWebhook(mgr, config.New(), reviewer, nil, nil); err != nil { fmt.Printf("failed to SetupWebhookWithManager: %v", err) os.Exit(1) } diff --git a/internal/webhook/podmutation/webhookhandler_suite_test.go b/internal/webhook/podmutation/webhookhandler_suite_test.go index 1336cab0e8..8448762f5d 100644 --- a/internal/webhook/podmutation/webhookhandler_suite_test.go +++ b/internal/webhook/podmutation/webhookhandler_suite_test.go @@ -105,7 +105,7 @@ func TestMain(m *testing.M) { } reviewer := rbac.NewReviewer(clientset) - if err = v1beta1.SetupCollectorWebhook(mgr, config.New(), reviewer, nil); err != nil { + if err = v1beta1.SetupCollectorWebhook(mgr, config.New(), reviewer, nil, nil); err != nil { fmt.Printf("failed to SetupWebhookWithManager: %v", err) os.Exit(1) } diff --git a/pkg/collector/upgrade/suite_test.go b/pkg/collector/upgrade/suite_test.go index fdafdca245..89a56d8b40 100644 --- a/pkg/collector/upgrade/suite_test.go +++ b/pkg/collector/upgrade/suite_test.go @@ -105,7 +105,7 @@ func TestMain(m *testing.M) { } reviewer := rbac.NewReviewer(clientset) - if err = v1beta1.SetupCollectorWebhook(mgr, config.New(), reviewer, nil); err != nil { + if err = v1beta1.SetupCollectorWebhook(mgr, config.New(), reviewer, nil, nil); err != nil { fmt.Printf("failed to SetupWebhookWithManager: %v", err) os.Exit(1) } From f80e75e79f37ed16176446100ec5c6dc006d5a9e Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Wed, 17 Jul 2024 14:12:30 -0700 Subject: [PATCH 04/38] update --- apis/v1beta1/collector_webhook.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/apis/v1beta1/collector_webhook.go b/apis/v1beta1/collector_webhook.go index ed3fd259b0..5d7f2d1f1c 100644 --- a/apis/v1beta1/collector_webhook.go +++ b/apis/v1beta1/collector_webhook.go @@ -206,6 +206,13 @@ func (c CollectorWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj run c.metrics.update(ctx, otelcolOld, otelcol) } + if c.bv != nil { + newWarnings, err := c.bv(*otelcol) + if err != nil { + return append(warnings, newWarnings...), err + } + warnings = append(warnings, newWarnings...) + } return warnings, nil } From 64b77303dd7aa1ac730572f1375a89d0067ce92b Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Thu, 18 Jul 2024 14:18:12 -0700 Subject: [PATCH 05/38] tests --- apis/v1beta1/collector_webhook.go | 22 +- apis/v1beta1/collector_webhook_test.go | 784 +---------------------- apis/v1beta1/verifier_test.go | 852 +++++++++++++++++++++++++ 3 files changed, 872 insertions(+), 786 deletions(-) create mode 100644 apis/v1beta1/verifier_test.go diff --git a/apis/v1beta1/collector_webhook.go b/apis/v1beta1/collector_webhook.go index 5d7f2d1f1c..517ad74777 100644 --- a/apis/v1beta1/collector_webhook.go +++ b/apis/v1beta1/collector_webhook.go @@ -77,7 +77,7 @@ type CollectorWebhook struct { scheme *runtime.Scheme reviewer *rbac.Reviewer metrics *Metrics - bv buildValidator + bv BuildValidator } func (c CollectorWebhook) Default(_ context.Context, obj runtime.Object) error { @@ -468,9 +468,25 @@ func checkAutoscalerSpec(autoscaler *AutoscalerSpec) error { return nil } -type buildValidator func(c OpenTelemetryCollector) (admission.Warnings, error) +type BuildValidator func(c OpenTelemetryCollector) (admission.Warnings, error) + +func NewCollectorWebhook( + logger logr.Logger, + scheme *runtime.Scheme, + cfg config.Config, + reviewer *rbac.Reviewer, + bv BuildValidator, +) *CollectorWebhook { + return &CollectorWebhook{ + logger: logger, + scheme: scheme, + cfg: cfg, + reviewer: reviewer, + bv: bv, + } +} -func SetupCollectorWebhook(mgr ctrl.Manager, cfg config.Config, reviewer *rbac.Reviewer, metrics *Metrics, bv buildValidator) error { +func SetupCollectorWebhook(mgr ctrl.Manager, cfg config.Config, reviewer *rbac.Reviewer, metrics *Metrics, bv BuildValidator) error { cvw := &CollectorWebhook{ reviewer: reviewer, logger: mgr.GetLogger().WithValues("handler", "CollectorWebhook", "version", "v1beta1"), diff --git a/apis/v1beta1/collector_webhook_test.go b/apis/v1beta1/collector_webhook_test.go index b8247bcd50..b11470eb3c 100644 --- a/apis/v1beta1/collector_webhook_test.go +++ b/apis/v1beta1/collector_webhook_test.go @@ -18,27 +18,15 @@ import ( "context" "fmt" "os" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "testing" "github.com/go-logr/logr" + "github.com/open-telemetry/opentelemetry-operator/internal/config" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "gopkg.in/yaml.v3" - appsv1 "k8s.io/api/apps/v1" - authv1 "k8s.io/api/authorization/v1" - autoscalingv2 "k8s.io/api/autoscaling/v2" - v1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" - "k8s.io/client-go/kubernetes/fake" "k8s.io/client-go/kubernetes/scheme" - kubeTesting "k8s.io/client-go/testing" - - "github.com/open-telemetry/opentelemetry-operator/internal/config" - "github.com/open-telemetry/opentelemetry-operator/internal/rbac" ) var ( @@ -514,773 +502,3 @@ func TestCollectorDefaultingWebhook(t *testing.T) { }) } } - -var cfgYaml = `receivers: - examplereceiver: - endpoint: "0.0.0.0:12345" - examplereceiver/settings: - endpoint: "0.0.0.0:12346" - prometheus: - config: - scrape_configs: - - job_name: otel-collector - scrape_interval: 10s - jaeger/custom: - protocols: - thrift_http: - endpoint: 0.0.0.0:15268 -` - -func TestOTELColValidatingWebhook(t *testing.T) { - minusOne := int32(-1) - zero := int32(0) - zero64 := int64(0) - one := int32(1) - three := int32(3) - five := int32(5) - - cfg := Config{} - err := yaml.Unmarshal([]byte(cfgYaml), &cfg) - require.NoError(t, err) - - tests := []struct { //nolint:govet - name string - otelcol OpenTelemetryCollector - expectedErr string - expectedWarnings []string - shouldFailSar bool - }{ - { - name: "valid empty spec", - otelcol: OpenTelemetryCollector{}, - }, - { - name: "valid full spec", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeStatefulSet, - OpenTelemetryCommonFields: OpenTelemetryCommonFields{ - Replicas: &three, - Ports: []PortsSpec{ - { - ServicePort: v1.ServicePort{ - Name: "port1", - Port: 5555, - }, - }, - { - ServicePort: v1.ServicePort{ - Name: "port2", - Port: 5554, - Protocol: v1.ProtocolUDP, - }, - }, - }, - }, - Autoscaler: &AutoscalerSpec{ - MinReplicas: &one, - MaxReplicas: &five, - Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{ - ScaleDown: &autoscalingv2.HPAScalingRules{ - StabilizationWindowSeconds: &three, - }, - ScaleUp: &autoscalingv2.HPAScalingRules{ - StabilizationWindowSeconds: &five, - }, - }, - TargetCPUUtilization: &five, - }, - UpgradeStrategy: "adhoc", - TargetAllocator: TargetAllocatorEmbedded{ - Enabled: true, - }, - Config: cfg, - }, - }, - }, - { - name: "prom CR admissions warning", - shouldFailSar: true, // force failure - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeStatefulSet, - OpenTelemetryCommonFields: OpenTelemetryCommonFields{ - Replicas: &three, - Ports: []PortsSpec{ - { - ServicePort: v1.ServicePort{ - Name: "port1", - Port: 5555, - }, - }, - { - ServicePort: v1.ServicePort{ - Name: "port2", - Port: 5554, - Protocol: v1.ProtocolUDP, - }, - }, - }, - }, - Autoscaler: &AutoscalerSpec{ - MinReplicas: &one, - MaxReplicas: &five, - Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{ - ScaleDown: &autoscalingv2.HPAScalingRules{ - StabilizationWindowSeconds: &three, - }, - ScaleUp: &autoscalingv2.HPAScalingRules{ - StabilizationWindowSeconds: &five, - }, - }, - TargetCPUUtilization: &five, - }, - UpgradeStrategy: "adhoc", - TargetAllocator: TargetAllocatorEmbedded{ - Enabled: true, - PrometheusCR: TargetAllocatorPrometheusCR{Enabled: true}, - }, - Config: cfg, - }, - }, - expectedWarnings: []string{ - "missing the following rules for monitoring.coreos.com/servicemonitors: [*]", - "missing the following rules for monitoring.coreos.com/podmonitors: [*]", - "missing the following rules for nodes/metrics: [get,list,watch]", - "missing the following rules for services: [get,list,watch]", - "missing the following rules for endpoints: [get,list,watch]", - "missing the following rules for namespaces: [get,list,watch]", - "missing the following rules for networking.k8s.io/ingresses: [get,list,watch]", - "missing the following rules for nodes: [get,list,watch]", - "missing the following rules for pods: [get,list,watch]", - "missing the following rules for configmaps: [get]", - "missing the following rules for discovery.k8s.io/endpointslices: [get,list,watch]", - "missing the following rules for nonResourceURL: /metrics: [get]", - }, - }, - { - name: "prom CR no admissions warning", - shouldFailSar: false, // force SAR okay - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeStatefulSet, - UpgradeStrategy: "adhoc", - OpenTelemetryCommonFields: OpenTelemetryCommonFields{ - Replicas: &three, - Ports: []PortsSpec{ - { - ServicePort: v1.ServicePort{ - Name: "port1", - Port: 5555, - }, - }, - { - ServicePort: v1.ServicePort{ - Name: "port2", - Port: 5554, - Protocol: v1.ProtocolUDP, - }, - }, - }, - }, - Autoscaler: &AutoscalerSpec{ - Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{ - ScaleDown: &autoscalingv2.HPAScalingRules{ - StabilizationWindowSeconds: &three, - }, - ScaleUp: &autoscalingv2.HPAScalingRules{ - StabilizationWindowSeconds: &five, - }, - }, - TargetCPUUtilization: &five, - }, - TargetAllocator: TargetAllocatorEmbedded{ - Enabled: true, - PrometheusCR: TargetAllocatorPrometheusCR{Enabled: true}, - }, - Config: cfg, - }, - }, - }, - { - name: "invalid mode with volume claim templates", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeSidecar, - StatefulSetCommonFields: StatefulSetCommonFields{ - VolumeClaimTemplates: []v1.PersistentVolumeClaim{{}, {}}, - }, - }, - }, - expectedErr: "does not support the attribute 'volumeClaimTemplates'", - }, - { - name: "invalid mode with tolerations", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeSidecar, - OpenTelemetryCommonFields: OpenTelemetryCommonFields{ - Tolerations: []v1.Toleration{{}, {}}, - }, - }, - }, - expectedErr: "does not support the attribute 'tolerations'", - }, - { - name: "invalid mode with target allocator", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeDeployment, - TargetAllocator: TargetAllocatorEmbedded{ - Enabled: true, - }, - }, - }, - expectedErr: "does not support the target allocation deployment", - }, - { - name: "invalid target allocator config", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeStatefulSet, - TargetAllocator: TargetAllocatorEmbedded{ - Enabled: true, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec Prometheus configuration is incorrect", - }, - { - name: "invalid target allocation strategy", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeDaemonSet, - TargetAllocator: TargetAllocatorEmbedded{ - Enabled: true, - AllocationStrategy: TargetAllocatorAllocationStrategyLeastWeighted, - }, - }, - }, - expectedErr: "mode is set to daemonset, which must be used with target allocation strategy per-node", - }, - { - name: "invalid port name", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - OpenTelemetryCommonFields: OpenTelemetryCommonFields{ - Ports: []PortsSpec{ - { - ServicePort: v1.ServicePort{ - // this port name contains a non alphanumeric character, which is invalid. - Name: "-testšŸ¦„port", - Port: 12345, - Protocol: v1.ProtocolTCP, - }, - }, - }, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec Ports configuration is incorrect", - }, - { - name: "invalid port name, too long", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - OpenTelemetryCommonFields: OpenTelemetryCommonFields{ - Ports: []PortsSpec{ - { - ServicePort: v1.ServicePort{ - Name: "aaaabbbbccccdddd", // len: 16, too long - Port: 5555, - }, - }, - }, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec Ports configuration is incorrect", - }, - { - name: "invalid port num", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - OpenTelemetryCommonFields: OpenTelemetryCommonFields{ - Ports: []PortsSpec{ - { - ServicePort: v1.ServicePort{ - Name: "aaaabbbbccccddd", // len: 15 - // no port set means it's 0, which is invalid - }, - }, - }, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec Ports configuration is incorrect", - }, - { - name: "invalid max replicas", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Autoscaler: &AutoscalerSpec{ - MaxReplicas: &zero, - }, - }, - }, - expectedErr: "maxReplicas should be defined and one or more", - }, - { - name: "invalid replicas, greater than max", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - OpenTelemetryCommonFields: OpenTelemetryCommonFields{ - Replicas: &five, - }, - Autoscaler: &AutoscalerSpec{ - MaxReplicas: &three, - }, - }, - }, - expectedErr: "replicas must not be greater than maxReplicas", - }, - { - name: "invalid min replicas, greater than max", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Autoscaler: &AutoscalerSpec{ - MaxReplicas: &three, - MinReplicas: &five, - }, - }, - }, - expectedErr: "minReplicas must not be greater than maxReplicas", - }, - { - name: "invalid min replicas, lesser than 1", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Autoscaler: &AutoscalerSpec{ - MaxReplicas: &three, - MinReplicas: &zero, - }, - }, - }, - expectedErr: "minReplicas should be one or more", - }, - { - name: "invalid autoscaler scale down", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Autoscaler: &AutoscalerSpec{ - MaxReplicas: &three, - Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{ - ScaleDown: &autoscalingv2.HPAScalingRules{ - StabilizationWindowSeconds: &zero, - }, - }, - }, - }, - }, - expectedErr: "scaleDown should be one or more", - }, - { - name: "invalid autoscaler scale up", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Autoscaler: &AutoscalerSpec{ - MaxReplicas: &three, - Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{ - ScaleUp: &autoscalingv2.HPAScalingRules{ - StabilizationWindowSeconds: &zero, - }, - }, - }, - }, - }, - expectedErr: "scaleUp should be one or more", - }, - { - name: "invalid autoscaler target cpu utilization", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Autoscaler: &AutoscalerSpec{ - MaxReplicas: &three, - TargetCPUUtilization: &zero, - }, - }, - }, - expectedErr: "targetCPUUtilization should be greater than 0 and less than 100", - }, - { - name: "autoscaler minReplicas is less than maxReplicas", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Autoscaler: &AutoscalerSpec{ - MaxReplicas: &one, - MinReplicas: &five, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, minReplicas must not be greater than maxReplicas", - }, - { - name: "invalid autoscaler metric type", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Autoscaler: &AutoscalerSpec{ - MaxReplicas: &three, - Metrics: []MetricSpec{ - { - Type: autoscalingv2.ResourceMetricSourceType, - }, - }, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, metric type unsupported. Expected metric of source type Pod", - }, - { - name: "invalid pod metric average value", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Autoscaler: &AutoscalerSpec{ - MaxReplicas: &three, - Metrics: []MetricSpec{ - { - Type: autoscalingv2.PodsMetricSourceType, - Pods: &autoscalingv2.PodsMetricSource{ - Metric: autoscalingv2.MetricIdentifier{ - Name: "custom1", - }, - Target: autoscalingv2.MetricTarget{ - Type: autoscalingv2.AverageValueMetricType, - AverageValue: resource.NewQuantity(int64(0), resource.DecimalSI), - }, - }, - }, - }, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, average value should be greater than 0", - }, - { - name: "utilization target is not valid with pod metrics", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Autoscaler: &AutoscalerSpec{ - MaxReplicas: &three, - Metrics: []MetricSpec{ - { - Type: autoscalingv2.PodsMetricSourceType, - Pods: &autoscalingv2.PodsMetricSource{ - Metric: autoscalingv2.MetricIdentifier{ - Name: "custom1", - }, - Target: autoscalingv2.MetricTarget{ - Type: autoscalingv2.UtilizationMetricType, - AverageUtilization: &one, - }, - }, - }, - }, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, invalid pods target type", - }, - { - name: "invalid deployment mode incompabible with ingress settings", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeSidecar, - Ingress: Ingress{ - Type: IngressTypeIngress, - }, - }, - }, - expectedErr: fmt.Sprintf("Ingress can only be used in combination with the modes: %s, %s, %s", - ModeDeployment, ModeDaemonSet, ModeStatefulSet, - ), - }, - { - name: "invalid mode with priorityClassName", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeSidecar, - OpenTelemetryCommonFields: OpenTelemetryCommonFields{ - PriorityClassName: "test-class", - }, - }, - }, - expectedErr: "does not support the attribute 'priorityClassName'", - }, - { - name: "invalid mode with affinity", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeSidecar, - OpenTelemetryCommonFields: OpenTelemetryCommonFields{ - Affinity: &v1.Affinity{ - NodeAffinity: &v1.NodeAffinity{ - RequiredDuringSchedulingIgnoredDuringExecution: &v1.NodeSelector{ - NodeSelectorTerms: []v1.NodeSelectorTerm{ - { - MatchExpressions: []v1.NodeSelectorRequirement{ - { - Key: "node", - Operator: v1.NodeSelectorOpIn, - Values: []string{"test-node"}, - }, - }, - }, - }, - }, - }, - }, - }, - }, - }, - expectedErr: "does not support the attribute 'affinity'", - }, - { - name: "invalid InitialDelaySeconds", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - LivenessProbe: &Probe{ - InitialDelaySeconds: &minusOne, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec LivenessProbe InitialDelaySeconds configuration is incorrect", - }, - { - name: "invalid InitialDelaySeconds readiness", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - ReadinessProbe: &Probe{ - InitialDelaySeconds: &minusOne, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec ReadinessProbe InitialDelaySeconds configuration is incorrect", - }, - { - name: "invalid PeriodSeconds", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - LivenessProbe: &Probe{ - PeriodSeconds: &zero, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec LivenessProbe PeriodSeconds configuration is incorrect", - }, - { - name: "invalid PeriodSeconds readiness", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - ReadinessProbe: &Probe{ - PeriodSeconds: &zero, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec ReadinessProbe PeriodSeconds configuration is incorrect", - }, - { - name: "invalid TimeoutSeconds", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - LivenessProbe: &Probe{ - TimeoutSeconds: &zero, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec LivenessProbe TimeoutSeconds configuration is incorrect", - }, - { - name: "invalid TimeoutSeconds readiness", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - ReadinessProbe: &Probe{ - TimeoutSeconds: &zero, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec ReadinessProbe TimeoutSeconds configuration is incorrect", - }, - { - name: "invalid SuccessThreshold", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - LivenessProbe: &Probe{ - SuccessThreshold: &zero, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec LivenessProbe SuccessThreshold configuration is incorrect", - }, - { - name: "invalid SuccessThreshold readiness", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - ReadinessProbe: &Probe{ - SuccessThreshold: &zero, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec ReadinessProbe SuccessThreshold configuration is incorrect", - }, - { - name: "invalid FailureThreshold", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - LivenessProbe: &Probe{ - FailureThreshold: &zero, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec LivenessProbe FailureThreshold configuration is incorrect", - }, - { - name: "invalid FailureThreshold readiness", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - ReadinessProbe: &Probe{ - FailureThreshold: &zero, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec ReadinessProbe FailureThreshold configuration is incorrect", - }, - { - name: "invalid TerminationGracePeriodSeconds", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - LivenessProbe: &Probe{ - TerminationGracePeriodSeconds: &zero64, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec LivenessProbe TerminationGracePeriodSeconds configuration is incorrect", - }, - { - name: "invalid TerminationGracePeriodSeconds readiness", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - ReadinessProbe: &Probe{ - TerminationGracePeriodSeconds: &zero64, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec ReadinessProbe TerminationGracePeriodSeconds configuration is incorrect", - }, - { - name: "invalid AdditionalContainers", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeSidecar, - OpenTelemetryCommonFields: OpenTelemetryCommonFields{ - AdditionalContainers: []v1.Container{ - { - Name: "test", - }, - }, - }, - }, - }, - expectedErr: "the OpenTelemetry Collector mode is set to sidecar, which does not support the attribute 'AdditionalContainers'", - }, - { - name: "missing ingress hostname for subdomain ruleType", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Ingress: Ingress{ - RuleType: IngressRuleTypeSubdomain, - }, - }, - }, - expectedErr: "a valid Ingress hostname has to be defined for subdomain ruleType", - }, - { - name: "invalid updateStrategy for Deployment mode", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeDeployment, - DaemonSetUpdateStrategy: appsv1.DaemonSetUpdateStrategy{ - Type: "RollingUpdate", - RollingUpdate: &appsv1.RollingUpdateDaemonSet{ - MaxSurge: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)}, - MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)}, - }, - }, - }, - }, - expectedErr: "the OpenTelemetry Collector mode is set to deployment, which does not support the attribute 'updateStrategy'", - }, - { - name: "invalid updateStrategy for Statefulset mode", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeStatefulSet, - DeploymentUpdateStrategy: appsv1.DeploymentStrategy{ - Type: "RollingUpdate", - RollingUpdate: &appsv1.RollingUpdateDeployment{ - MaxSurge: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)}, - MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)}, - }, - }, - }, - }, - expectedErr: "the OpenTelemetry Collector mode is set to statefulset, which does not support the attribute 'deploymentUpdateStrategy'", - }, - } - - for _, test := range tests { - test := test - t.Run(test.name, func(t *testing.T) { - cvw := &CollectorWebhook{ - logger: logr.Discard(), - scheme: testScheme, - cfg: config.New( - config.WithCollectorImage("collector:v0.0.0"), - config.WithTargetAllocatorImage("ta:v0.0.0"), - ), - reviewer: getReviewer(test.shouldFailSar), - bv: func(col OpenTelemetryCollector) (admission.Warnings, error) { - return nil, nil - }, - } - ctx := context.Background() - warnings, err := cvw.ValidateCreate(ctx, &test.otelcol) - if test.expectedErr == "" { - assert.NoError(t, err) - } else { - assert.ErrorContains(t, err, test.expectedErr) - } - assert.Equal(t, len(test.expectedWarnings), len(warnings)) - assert.ElementsMatch(t, warnings, test.expectedWarnings) - }) - } -} - -func getReviewer(shouldFailSAR bool) *rbac.Reviewer { - c := fake.NewSimpleClientset() - c.PrependReactor("create", "subjectaccessreviews", func(action kubeTesting.Action) (handled bool, ret runtime.Object, err error) { - // check our expectation here - if !action.Matches("create", "subjectaccessreviews") { - return false, nil, fmt.Errorf("must be a create for a SAR") - } - sar, ok := action.(kubeTesting.CreateAction).GetObject().DeepCopyObject().(*authv1.SubjectAccessReview) - if !ok || sar == nil { - return false, nil, fmt.Errorf("bad object") - } - sar.Status = authv1.SubjectAccessReviewStatus{ - Allowed: !shouldFailSAR, - Denied: shouldFailSAR, - } - return true, sar, nil - }) - return rbac.NewReviewer(c) -} diff --git a/apis/v1beta1/verifier_test.go b/apis/v1beta1/verifier_test.go new file mode 100644 index 0000000000..c25299867b --- /dev/null +++ b/apis/v1beta1/verifier_test.go @@ -0,0 +1,852 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package v1beta1_test + +import ( + "context" + "fmt" + "github.com/go-logr/logr" + "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" + "github.com/open-telemetry/opentelemetry-operator/internal/config" + "github.com/open-telemetry/opentelemetry-operator/internal/manifests" + collectorManifests "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" + "github.com/open-telemetry/opentelemetry-operator/internal/rbac" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" + appsv1 "k8s.io/api/apps/v1" + authv1 "k8s.io/api/authorization/v1" + autoscalingv2 "k8s.io/api/autoscaling/v2" + v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/kubernetes/scheme" + kubeTesting "k8s.io/client-go/testing" + "os" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + "testing" +) + +var ( + testScheme = scheme.Scheme +) + +var cfgYaml = `receivers: + examplereceiver: + endpoint: "0.0.0.0:12345" + examplereceiver/settings: + endpoint: "0.0.0.0:12346" + prometheus: + config: + scrape_configs: + - job_name: otel-collector + scrape_interval: 10s + jaeger/custom: + protocols: + thrift_http: + endpoint: 0.0.0.0:15268 +` + +func TestOTELColValidatingWebhook(t *testing.T) { + minusOne := int32(-1) + zero := int32(0) + zero64 := int64(0) + one := int32(1) + three := int32(3) + five := int32(5) + + if err := v1beta1.AddToScheme(testScheme); err != nil { + fmt.Printf("failed to register scheme: %v", err) + os.Exit(1) + } + + cfg := v1beta1.Config{} + err := yaml.Unmarshal([]byte(cfgYaml), &cfg) + require.NoError(t, err) + + tests := []struct { //nolint:govet + name string + otelcol v1beta1.OpenTelemetryCollector + expectedErr string + expectedWarnings []string + shouldFailSar bool + }{ + { + name: "valid empty spec", + otelcol: v1beta1.OpenTelemetryCollector{}, + }, + { + name: "valid full spec", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeStatefulSet, + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + Replicas: &three, + Ports: []v1beta1.PortsSpec{ + { + ServicePort: v1.ServicePort{ + Name: "port1", + Port: 5555, + }, + }, + { + ServicePort: v1.ServicePort{ + Name: "port2", + Port: 5554, + Protocol: v1.ProtocolUDP, + }, + }, + }, + }, + Autoscaler: &v1beta1.AutoscalerSpec{ + MinReplicas: &one, + MaxReplicas: &five, + Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{ + ScaleDown: &autoscalingv2.HPAScalingRules{ + StabilizationWindowSeconds: &three, + }, + ScaleUp: &autoscalingv2.HPAScalingRules{ + StabilizationWindowSeconds: &five, + }, + }, + TargetCPUUtilization: &five, + }, + UpgradeStrategy: "adhoc", + TargetAllocator: v1beta1.TargetAllocatorEmbedded{ + Enabled: true, + }, + Config: cfg, + }, + }, + }, + { + name: "prom CR admissions warning", + shouldFailSar: true, // force failure + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeStatefulSet, + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + Replicas: &three, + Ports: []v1beta1.PortsSpec{ + { + ServicePort: v1.ServicePort{ + Name: "port1", + Port: 5555, + }, + }, + { + ServicePort: v1.ServicePort{ + Name: "port2", + Port: 5554, + Protocol: v1.ProtocolUDP, + }, + }, + }, + }, + Autoscaler: &v1beta1.AutoscalerSpec{ + MinReplicas: &one, + MaxReplicas: &five, + Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{ + ScaleDown: &autoscalingv2.HPAScalingRules{ + StabilizationWindowSeconds: &three, + }, + ScaleUp: &autoscalingv2.HPAScalingRules{ + StabilizationWindowSeconds: &five, + }, + }, + TargetCPUUtilization: &five, + }, + UpgradeStrategy: "adhoc", + TargetAllocator: v1beta1.TargetAllocatorEmbedded{ + Enabled: true, + PrometheusCR: v1beta1.TargetAllocatorPrometheusCR{Enabled: true}, + }, + Config: cfg, + }, + }, + expectedWarnings: []string{ + "missing the following rules for monitoring.coreos.com/servicemonitors: [*]", + "missing the following rules for monitoring.coreos.com/podmonitors: [*]", + "missing the following rules for nodes/metrics: [get,list,watch]", + "missing the following rules for services: [get,list,watch]", + "missing the following rules for endpoints: [get,list,watch]", + "missing the following rules for namespaces: [get,list,watch]", + "missing the following rules for networking.k8s.io/ingresses: [get,list,watch]", + "missing the following rules for nodes: [get,list,watch]", + "missing the following rules for pods: [get,list,watch]", + "missing the following rules for configmaps: [get]", + "missing the following rules for discovery.k8s.io/endpointslices: [get,list,watch]", + "missing the following rules for nonResourceURL: /metrics: [get]", + }, + }, + { + name: "prom CR no admissions warning", + shouldFailSar: false, // force SAR okay + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeStatefulSet, + UpgradeStrategy: "adhoc", + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + Replicas: &three, + Ports: []v1beta1.PortsSpec{ + { + ServicePort: v1.ServicePort{ + Name: "port1", + Port: 5555, + }, + }, + { + ServicePort: v1.ServicePort{ + Name: "port2", + Port: 5554, + Protocol: v1.ProtocolUDP, + }, + }, + }, + }, + Autoscaler: &v1beta1.AutoscalerSpec{ + Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{ + ScaleDown: &autoscalingv2.HPAScalingRules{ + StabilizationWindowSeconds: &three, + }, + ScaleUp: &autoscalingv2.HPAScalingRules{ + StabilizationWindowSeconds: &five, + }, + }, + TargetCPUUtilization: &five, + }, + TargetAllocator: v1beta1.TargetAllocatorEmbedded{ + Enabled: true, + PrometheusCR: v1beta1.TargetAllocatorPrometheusCR{Enabled: true}, + }, + Config: cfg, + }, + }, + }, + { + name: "invalid mode with volume claim templates", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeSidecar, + StatefulSetCommonFields: v1beta1.StatefulSetCommonFields{ + VolumeClaimTemplates: []v1.PersistentVolumeClaim{{}, {}}, + }, + }, + }, + expectedErr: "does not support the attribute 'volumeClaimTemplates'", + }, + { + name: "invalid mode with tolerations", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeSidecar, + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + Tolerations: []v1.Toleration{{}, {}}, + }, + }, + }, + expectedErr: "does not support the attribute 'tolerations'", + }, + { + name: "invalid mode with target allocator", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeDeployment, + TargetAllocator: v1beta1.TargetAllocatorEmbedded{ + Enabled: true, + }, + }, + }, + expectedErr: "does not support the target allocation deployment", + }, + { + name: "invalid target allocator config", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeStatefulSet, + TargetAllocator: v1beta1.TargetAllocatorEmbedded{ + Enabled: true, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec Prometheus configuration is incorrect", + }, + { + name: "invalid target allocation strategy", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeDaemonSet, + TargetAllocator: v1beta1.TargetAllocatorEmbedded{ + Enabled: true, + AllocationStrategy: v1beta1.TargetAllocatorAllocationStrategyLeastWeighted, + }, + }, + }, + expectedErr: "mode is set to daemonset, which must be used with target allocation strategy per-node", + }, + { + name: "invalid port name", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + Ports: []v1beta1.PortsSpec{ + { + ServicePort: v1.ServicePort{ + // this port name contains a non alphanumeric character, which is invalid. + Name: "-testšŸ¦„port", + Port: 12345, + Protocol: v1.ProtocolTCP, + }, + }, + }, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec Ports configuration is incorrect", + }, + { + name: "invalid port name, too long", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + Ports: []v1beta1.PortsSpec{ + { + ServicePort: v1.ServicePort{ + Name: "aaaabbbbccccdddd", // len: 16, too long + Port: 5555, + }, + }, + }, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec Ports configuration is incorrect", + }, + { + name: "invalid port num", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + Ports: []v1beta1.PortsSpec{ + { + ServicePort: v1.ServicePort{ + Name: "aaaabbbbccccddd", // len: 15 + // no port set means it's 0, which is invalid + }, + }, + }, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec Ports configuration is incorrect", + }, + { + name: "invalid max replicas", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Autoscaler: &v1beta1.AutoscalerSpec{ + MaxReplicas: &zero, + }, + }, + }, + expectedErr: "maxReplicas should be defined and one or more", + }, + { + name: "invalid replicas, greater than max", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + Replicas: &five, + }, + Autoscaler: &v1beta1.AutoscalerSpec{ + MaxReplicas: &three, + }, + }, + }, + expectedErr: "replicas must not be greater than maxReplicas", + }, + { + name: "invalid min replicas, greater than max", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Autoscaler: &v1beta1.AutoscalerSpec{ + MaxReplicas: &three, + MinReplicas: &five, + }, + }, + }, + expectedErr: "minReplicas must not be greater than maxReplicas", + }, + { + name: "invalid min replicas, lesser than 1", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Autoscaler: &v1beta1.AutoscalerSpec{ + MaxReplicas: &three, + MinReplicas: &zero, + }, + }, + }, + expectedErr: "minReplicas should be one or more", + }, + { + name: "invalid autoscaler scale down", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Autoscaler: &v1beta1.AutoscalerSpec{ + MaxReplicas: &three, + Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{ + ScaleDown: &autoscalingv2.HPAScalingRules{ + StabilizationWindowSeconds: &zero, + }, + }, + }, + }, + }, + expectedErr: "scaleDown should be one or more", + }, + { + name: "invalid autoscaler scale up", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Autoscaler: &v1beta1.AutoscalerSpec{ + MaxReplicas: &three, + Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{ + ScaleUp: &autoscalingv2.HPAScalingRules{ + StabilizationWindowSeconds: &zero, + }, + }, + }, + }, + }, + expectedErr: "scaleUp should be one or more", + }, + { + name: "invalid autoscaler target cpu utilization", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Autoscaler: &v1beta1.AutoscalerSpec{ + MaxReplicas: &three, + TargetCPUUtilization: &zero, + }, + }, + }, + expectedErr: "targetCPUUtilization should be greater than 0 and less than 100", + }, + { + name: "autoscaler minReplicas is less than maxReplicas", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Autoscaler: &v1beta1.AutoscalerSpec{ + MaxReplicas: &one, + MinReplicas: &five, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, minReplicas must not be greater than maxReplicas", + }, + { + name: "invalid autoscaler metric type", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Autoscaler: &v1beta1.AutoscalerSpec{ + MaxReplicas: &three, + Metrics: []v1beta1.MetricSpec{ + { + Type: autoscalingv2.ResourceMetricSourceType, + }, + }, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, metric type unsupported. Expected metric of source type Pod", + }, + { + name: "invalid pod metric average value", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Autoscaler: &v1beta1.AutoscalerSpec{ + MaxReplicas: &three, + Metrics: []v1beta1.MetricSpec{ + { + Type: autoscalingv2.PodsMetricSourceType, + Pods: &autoscalingv2.PodsMetricSource{ + Metric: autoscalingv2.MetricIdentifier{ + Name: "custom1", + }, + Target: autoscalingv2.MetricTarget{ + Type: autoscalingv2.AverageValueMetricType, + AverageValue: resource.NewQuantity(int64(0), resource.DecimalSI), + }, + }, + }, + }, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, average value should be greater than 0", + }, + { + name: "utilization target is not valid with pod metrics", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Autoscaler: &v1beta1.AutoscalerSpec{ + MaxReplicas: &three, + Metrics: []v1beta1.MetricSpec{ + { + Type: autoscalingv2.PodsMetricSourceType, + Pods: &autoscalingv2.PodsMetricSource{ + Metric: autoscalingv2.MetricIdentifier{ + Name: "custom1", + }, + Target: autoscalingv2.MetricTarget{ + Type: autoscalingv2.UtilizationMetricType, + AverageUtilization: &one, + }, + }, + }, + }, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, invalid pods target type", + }, + { + name: "invalid deployment mode incompabible with ingress settings", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeSidecar, + Ingress: v1beta1.Ingress{ + Type: v1beta1.IngressTypeIngress, + }, + }, + }, + expectedErr: fmt.Sprintf("Ingress can only be used in combination with the modes: %s, %s, %s", + v1beta1.ModeDeployment, v1beta1.ModeDaemonSet, v1beta1.ModeStatefulSet, + ), + }, + { + name: "invalid mode with priorityClassName", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeSidecar, + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + PriorityClassName: "test-class", + }, + }, + }, + expectedErr: "does not support the attribute 'priorityClassName'", + }, + { + name: "invalid mode with affinity", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeSidecar, + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + Affinity: &v1.Affinity{ + NodeAffinity: &v1.NodeAffinity{ + RequiredDuringSchedulingIgnoredDuringExecution: &v1.NodeSelector{ + NodeSelectorTerms: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{ + { + Key: "node", + Operator: v1.NodeSelectorOpIn, + Values: []string{"test-node"}, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + expectedErr: "does not support the attribute 'affinity'", + }, + { + name: "invalid InitialDelaySeconds", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + LivenessProbe: &v1beta1.Probe{ + InitialDelaySeconds: &minusOne, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec LivenessProbe InitialDelaySeconds configuration is incorrect", + }, + { + name: "invalid InitialDelaySeconds readiness", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + ReadinessProbe: &v1beta1.Probe{ + InitialDelaySeconds: &minusOne, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec ReadinessProbe InitialDelaySeconds configuration is incorrect", + }, + { + name: "invalid PeriodSeconds", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + LivenessProbe: &v1beta1.Probe{ + PeriodSeconds: &zero, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec LivenessProbe PeriodSeconds configuration is incorrect", + }, + { + name: "invalid PeriodSeconds readiness", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + ReadinessProbe: &v1beta1.Probe{ + PeriodSeconds: &zero, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec ReadinessProbe PeriodSeconds configuration is incorrect", + }, + { + name: "invalid TimeoutSeconds", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + LivenessProbe: &v1beta1.Probe{ + TimeoutSeconds: &zero, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec LivenessProbe TimeoutSeconds configuration is incorrect", + }, + { + name: "invalid TimeoutSeconds readiness", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + ReadinessProbe: &v1beta1.Probe{ + TimeoutSeconds: &zero, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec ReadinessProbe TimeoutSeconds configuration is incorrect", + }, + { + name: "invalid SuccessThreshold", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + LivenessProbe: &v1beta1.Probe{ + SuccessThreshold: &zero, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec LivenessProbe SuccessThreshold configuration is incorrect", + }, + { + name: "invalid SuccessThreshold readiness", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + ReadinessProbe: &v1beta1.Probe{ + SuccessThreshold: &zero, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec ReadinessProbe SuccessThreshold configuration is incorrect", + }, + { + name: "invalid FailureThreshold", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + LivenessProbe: &v1beta1.Probe{ + FailureThreshold: &zero, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec LivenessProbe FailureThreshold configuration is incorrect", + }, + { + name: "invalid FailureThreshold readiness", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + ReadinessProbe: &v1beta1.Probe{ + FailureThreshold: &zero, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec ReadinessProbe FailureThreshold configuration is incorrect", + }, + { + name: "invalid TerminationGracePeriodSeconds", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + LivenessProbe: &v1beta1.Probe{ + TerminationGracePeriodSeconds: &zero64, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec LivenessProbe TerminationGracePeriodSeconds configuration is incorrect", + }, + { + name: "invalid TerminationGracePeriodSeconds readiness", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + ReadinessProbe: &v1beta1.Probe{ + TerminationGracePeriodSeconds: &zero64, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec ReadinessProbe TerminationGracePeriodSeconds configuration is incorrect", + }, + { + name: "invalid AdditionalContainers", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeSidecar, + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + AdditionalContainers: []v1.Container{ + { + Name: "test", + }, + }, + }, + }, + }, + expectedErr: "the OpenTelemetry Collector mode is set to sidecar, which does not support the attribute 'AdditionalContainers'", + }, + { + name: "missing ingress hostname for subdomain ruleType", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Ingress: v1beta1.Ingress{ + RuleType: v1beta1.IngressRuleTypeSubdomain, + }, + }, + }, + expectedErr: "a valid Ingress hostname has to be defined for subdomain ruleType", + }, + { + name: "invalid updateStrategy for Deployment mode", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeDeployment, + DaemonSetUpdateStrategy: appsv1.DaemonSetUpdateStrategy{ + Type: "RollingUpdate", + RollingUpdate: &appsv1.RollingUpdateDaemonSet{ + MaxSurge: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)}, + MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)}, + }, + }, + }, + }, + expectedErr: "the OpenTelemetry Collector mode is set to deployment, which does not support the attribute 'updateStrategy'", + }, + { + name: "invalid updateStrategy for Statefulset mode", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeStatefulSet, + DeploymentUpdateStrategy: appsv1.DeploymentStrategy{ + Type: "RollingUpdate", + RollingUpdate: &appsv1.RollingUpdateDeployment{ + MaxSurge: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)}, + MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)}, + }, + }, + }, + }, + expectedErr: "the OpenTelemetry Collector mode is set to statefulset, which does not support the attribute 'deploymentUpdateStrategy'", + }, + { + name: "", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeStatefulSet, + DeploymentUpdateStrategy: appsv1.DeploymentStrategy{ + Type: "RollingUpdate", + RollingUpdate: &appsv1.RollingUpdateDeployment{ + MaxSurge: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)}, + MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)}, + }, + }, + }, + }, + expectedErr: "", + }, + } + + bv := func(collector v1beta1.OpenTelemetryCollector) (admission.Warnings, error) { + cfg := config.New( + config.WithCollectorImage("default-collector"), + config.WithTargetAllocatorImage("default-ta-allocator"), + ) + params := manifests.Params{ + Log: logr.Discard(), + Config: cfg, + OtelCol: collector, + } + _, err = collectorManifests.Build(params) + if err != nil { + return nil, err + } + return nil, nil + } + + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + cvw := v1beta1.NewCollectorWebhook( + logr.Discard(), + testScheme, + config.New( + config.WithCollectorImage("collector:v0.0.0"), + config.WithTargetAllocatorImage("ta:v0.0.0"), + ), + getReviewer(test.shouldFailSar), + bv, + ) + ctx := context.Background() + warnings, err := cvw.ValidateCreate(ctx, &test.otelcol) + if test.expectedErr == "" { + assert.NoError(t, err) + } else { + assert.ErrorContains(t, err, test.expectedErr) + } + assert.Equal(t, len(test.expectedWarnings), len(warnings)) + assert.ElementsMatch(t, warnings, test.expectedWarnings) + }) + } +} + +func getReviewer(shouldFailSAR bool) *rbac.Reviewer { + c := fake.NewSimpleClientset() + c.PrependReactor("create", "subjectaccessreviews", func(action kubeTesting.Action) (handled bool, ret runtime.Object, err error) { + // check our expectation here + if !action.Matches("create", "subjectaccessreviews") { + return false, nil, fmt.Errorf("must be a create for a SAR") + } + sar, ok := action.(kubeTesting.CreateAction).GetObject().DeepCopyObject().(*authv1.SubjectAccessReview) + if !ok || sar == nil { + return false, nil, fmt.Errorf("bad object") + } + sar.Status = authv1.SubjectAccessReviewStatus{ + Allowed: !shouldFailSAR, + Denied: shouldFailSAR, + } + return true, sar, nil + }) + return rbac.NewReviewer(c) +} From 5f290a9ecec369dc46b60008b9942ac59eda14be Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Mon, 29 Jul 2024 10:41:00 -0700 Subject: [PATCH 06/38] add test --- apis/v1beta1/verifier_test.go | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/apis/v1beta1/verifier_test.go b/apis/v1beta1/verifier_test.go index c25299867b..be3803e1ce 100644 --- a/apis/v1beta1/verifier_test.go +++ b/apis/v1beta1/verifier_test.go @@ -526,7 +526,7 @@ func TestOTELColValidatingWebhook(t *testing.T) { expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, invalid pods target type", }, { - name: "invalid deployment mode incompabible with ingress settings", + name: "invalid deployment mode incompatible with ingress settings", otelcol: v1beta1.OpenTelemetryCollector{ Spec: v1beta1.OpenTelemetryCollectorSpec{ Mode: v1beta1.ModeSidecar, @@ -535,9 +535,7 @@ func TestOTELColValidatingWebhook(t *testing.T) { }, }, }, - expectedErr: fmt.Sprintf("Ingress can only be used in combination with the modes: %s, %s, %s", - v1beta1.ModeDeployment, v1beta1.ModeDaemonSet, v1beta1.ModeStatefulSet, - ), + expectedErr: fmt.Sprintf("Ingress can only be used in combination with the modes: %s, %s, %s", v1beta1.ModeDeployment, v1beta1.ModeDaemonSet, v1beta1.ModeStatefulSet), }, { name: "invalid mode with priorityClassName", @@ -771,20 +769,22 @@ func TestOTELColValidatingWebhook(t *testing.T) { expectedErr: "the OpenTelemetry Collector mode is set to statefulset, which does not support the attribute 'deploymentUpdateStrategy'", }, { - name: "", + name: "missing port for ingress type", otelcol: v1beta1.OpenTelemetryCollector{ Spec: v1beta1.OpenTelemetryCollectorSpec{ - Mode: v1beta1.ModeStatefulSet, - DeploymentUpdateStrategy: appsv1.DeploymentStrategy{ - Type: "RollingUpdate", - RollingUpdate: &appsv1.RollingUpdateDeployment{ - MaxSurge: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)}, - MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)}, + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + Ports: []v1beta1.PortsSpec{ + { + ServicePort: v1.ServicePort{}, + }, }, }, + Ingress: v1beta1.Ingress{ + Type: v1beta1.IngressTypeIngress, + }, }, }, - expectedErr: "", + expectedErr: "the OpenTelemetry Spec Ports configuration is incorrect", }, } @@ -798,7 +798,7 @@ func TestOTELColValidatingWebhook(t *testing.T) { Config: cfg, OtelCol: collector, } - _, err = collectorManifests.Build(params) + _, err := collectorManifests.Build(params) if err != nil { return nil, err } From 72acf242a025bce5c597a542d32ed30acd50a345 Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Mon, 29 Jul 2024 12:28:25 -0700 Subject: [PATCH 07/38] changelog --- .chloggen/enhanced-webhook.yaml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 .chloggen/enhanced-webhook.yaml diff --git a/.chloggen/enhanced-webhook.yaml b/.chloggen/enhanced-webhook.yaml new file mode 100644 index 0000000000..6358c9c2ba --- /dev/null +++ b/.chloggen/enhanced-webhook.yaml @@ -0,0 +1,16 @@ +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: enhancement + +# The name of the component, or a single word describing the area of concern, (e.g. operator, target allocator, github action) +component: operator + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: added reconciliation errors for webhook events + +# One or more tracking issues related to the change +issues: [2399] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: \ No newline at end of file From 348dcdbfdd789e0811d82be14e6bd30819fce7ff Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Mon, 29 Jul 2024 13:45:45 -0700 Subject: [PATCH 08/38] initial fixes --- apis/v1beta1/collector_webhook.go | 9 +- apis/v1beta1/collector_webhook_test.go | 1174 +++++++++++++++++++++--- apis/v1beta1/verifier_test.go | 852 ----------------- main.go | 2 +- 4 files changed, 1027 insertions(+), 1010 deletions(-) delete mode 100644 apis/v1beta1/verifier_test.go diff --git a/apis/v1beta1/collector_webhook.go b/apis/v1beta1/collector_webhook.go index 517ad74777..dba9770ba5 100644 --- a/apis/v1beta1/collector_webhook.go +++ b/apis/v1beta1/collector_webhook.go @@ -169,7 +169,7 @@ func (c CollectorWebhook) ValidateCreate(ctx context.Context, obj runtime.Object return nil, fmt.Errorf("expected an OpenTelemetryCollector, received %T", obj) } - warnings, err := c.validate(ctx, otelcol) + warnings, err := c.Validate(ctx, otelcol) if err != nil { return warnings, err } @@ -197,7 +197,7 @@ func (c CollectorWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj run return nil, fmt.Errorf("expected an OpenTelemetryCollector, received %T", oldObj) } - warnings, err := c.validate(ctx, otelcol) + warnings, err := c.Validate(ctx, otelcol) if err != nil { return warnings, err } @@ -222,7 +222,7 @@ func (c CollectorWebhook) ValidateDelete(ctx context.Context, obj runtime.Object return nil, fmt.Errorf("expected an OpenTelemetryCollector, received %T", obj) } - warnings, err := c.validate(ctx, otelcol) + warnings, err := c.Validate(ctx, otelcol) if err != nil { return warnings, err } @@ -234,7 +234,7 @@ func (c CollectorWebhook) ValidateDelete(ctx context.Context, obj runtime.Object return warnings, nil } -func (c CollectorWebhook) validate(ctx context.Context, r *OpenTelemetryCollector) (admission.Warnings, error) { +func (c CollectorWebhook) Validate(ctx context.Context, r *OpenTelemetryCollector) (admission.Warnings, error) { warnings := admission.Warnings{} nullObjects := r.Spec.Config.nullObjects() @@ -468,6 +468,7 @@ func checkAutoscalerSpec(autoscaler *AutoscalerSpec) error { return nil } +// BuildValidator is mostly used for testing purposes type BuildValidator func(c OpenTelemetryCollector) (admission.Warnings, error) func NewCollectorWebhook( diff --git a/apis/v1beta1/collector_webhook_test.go b/apis/v1beta1/collector_webhook_test.go index b11470eb3c..98feff6b80 100644 --- a/apis/v1beta1/collector_webhook_test.go +++ b/apis/v1beta1/collector_webhook_test.go @@ -12,46 +12,76 @@ // See the License for the specific language governing permissions and // limitations under the License. -package v1beta1 +package v1beta1_test import ( "context" "fmt" - "os" - "testing" - "github.com/go-logr/logr" + "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" "github.com/open-telemetry/opentelemetry-operator/internal/config" + "github.com/open-telemetry/opentelemetry-operator/internal/manifests" + collectorManifests "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" + "github.com/open-telemetry/opentelemetry-operator/internal/rbac" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" + appsv1 "k8s.io/api/apps/v1" + authv1 "k8s.io/api/authorization/v1" + autoscalingv2 "k8s.io/api/autoscaling/v2" + v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/client-go/kubernetes/fake" "k8s.io/client-go/kubernetes/scheme" + kubeTesting "k8s.io/client-go/testing" + "os" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + "testing" ) var ( testScheme = scheme.Scheme ) +var cfgYaml = `receivers: + examplereceiver: + endpoint: "0.0.0.0:12345" + examplereceiver/settings: + endpoint: "0.0.0.0:12346" + prometheus: + config: + scrape_configs: + - job_name: otel-collector + scrape_interval: 10s + jaeger/custom: + protocols: + thrift_http: + endpoint: 0.0.0.0:15268 +` + func TestValidate(t *testing.T) { tests := []struct { - name string - collector OpenTelemetryCollector - warnings []string - err string + name string + collector v1beta1.OpenTelemetryCollector + warnings []string + err string + shouldFailSar bool }{ { name: "Test ", - collector: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Config: Config{ - Processors: &AnyConfig{ + collector: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Config: v1beta1.Config{ + Processors: &v1beta1.AnyConfig{ Object: map[string]interface{}{ "batch": nil, "foo": nil, }, }, - Extensions: &AnyConfig{ + Extensions: &v1beta1.AnyConfig{ Object: map[string]interface{}{ "foo": nil, }, @@ -65,11 +95,39 @@ func TestValidate(t *testing.T) { }, }, } + + bv := func(collector v1beta1.OpenTelemetryCollector) (admission.Warnings, error) { + cfg := config.New( + config.WithCollectorImage("default-collector"), + config.WithTargetAllocatorImage("default-ta-allocator"), + ) + params := manifests.Params{ + Log: logr.Discard(), + Config: cfg, + OtelCol: collector, + } + _, err := collectorManifests.Build(params) + if err != nil { + return nil, err + } + return nil, nil + } + for _, tt := range tests { - webhook := CollectorWebhook{} + test := tt + webhook := v1beta1.NewCollectorWebhook( + logr.Discard(), + testScheme, + config.New( + config.WithCollectorImage("collector:v0.0.0"), + config.WithTargetAllocatorImage("ta:v0.0.0"), + ), + getReviewer(test.shouldFailSar), + bv, + ) t.Run(tt.name, func(t *testing.T) { tt := tt - warnings, err := webhook.validate(context.Background(), &tt.collector) + warnings, err := webhook.Validate(context.Background(), &tt.collector) if tt.err == "" { require.NoError(t, err) } else { @@ -85,65 +143,66 @@ func TestCollectorDefaultingWebhook(t *testing.T) { five := int32(5) defaultCPUTarget := int32(90) - if err := AddToScheme(testScheme); err != nil { + if err := v1beta1.AddToScheme(testScheme); err != nil { fmt.Printf("failed to register scheme: %v", err) os.Exit(1) } tests := []struct { - name string - otelcol OpenTelemetryCollector - expected OpenTelemetryCollector + name string + otelcol v1beta1.OpenTelemetryCollector + expected v1beta1.OpenTelemetryCollector + shouldFailSar bool }{ { name: "all fields default", - otelcol: OpenTelemetryCollector{}, - expected: OpenTelemetryCollector{ + otelcol: v1beta1.OpenTelemetryCollector{}, + expected: v1beta1.OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ "app.kubernetes.io/managed-by": "opentelemetry-operator", }, }, - Spec: OpenTelemetryCollectorSpec{ - OpenTelemetryCommonFields: OpenTelemetryCommonFields{ - ManagementState: ManagementStateManaged, + Spec: v1beta1.OpenTelemetryCollectorSpec{ + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + ManagementState: v1beta1.ManagementStateManaged, Replicas: &one, - PodDisruptionBudget: &PodDisruptionBudgetSpec{ + PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ MaxUnavailable: &intstr.IntOrString{ Type: intstr.Int, IntVal: 1, }, }, }, - Mode: ModeDeployment, - UpgradeStrategy: UpgradeStrategyAutomatic, + Mode: v1beta1.ModeDeployment, + UpgradeStrategy: v1beta1.UpgradeStrategyAutomatic, }, }, }, { name: "provided values in spec", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeSidecar, + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeSidecar, UpgradeStrategy: "adhoc", - OpenTelemetryCommonFields: OpenTelemetryCommonFields{ + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ Replicas: &five, }, }, }, - expected: OpenTelemetryCollector{ + expected: v1beta1.OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ "app.kubernetes.io/managed-by": "opentelemetry-operator", }, }, - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeSidecar, + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeSidecar, UpgradeStrategy: "adhoc", - OpenTelemetryCommonFields: OpenTelemetryCommonFields{ + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ Replicas: &five, - ManagementState: ManagementStateManaged, - PodDisruptionBudget: &PodDisruptionBudgetSpec{ + ManagementState: v1beta1.ManagementStateManaged, + PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ MaxUnavailable: &intstr.IntOrString{ Type: intstr.Int, IntVal: 1, @@ -155,29 +214,29 @@ func TestCollectorDefaultingWebhook(t *testing.T) { }, { name: "doesn't override unmanaged", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeSidecar, + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeSidecar, UpgradeStrategy: "adhoc", - OpenTelemetryCommonFields: OpenTelemetryCommonFields{ + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ Replicas: &five, - ManagementState: ManagementStateUnmanaged, + ManagementState: v1beta1.ManagementStateUnmanaged, }, }, }, - expected: OpenTelemetryCollector{ + expected: v1beta1.OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ "app.kubernetes.io/managed-by": "opentelemetry-operator", }, }, - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeSidecar, + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeSidecar, UpgradeStrategy: "adhoc", - OpenTelemetryCommonFields: OpenTelemetryCommonFields{ + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ Replicas: &five, - ManagementState: ManagementStateUnmanaged, - PodDisruptionBudget: &PodDisruptionBudgetSpec{ + ManagementState: v1beta1.ManagementStateUnmanaged, + PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ MaxUnavailable: &intstr.IntOrString{ Type: intstr.Int, IntVal: 1, @@ -189,34 +248,34 @@ func TestCollectorDefaultingWebhook(t *testing.T) { }, { name: "Setting Autoscaler MaxReplicas", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Autoscaler: &AutoscalerSpec{ + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Autoscaler: &v1beta1.AutoscalerSpec{ MaxReplicas: &five, MinReplicas: &one, }, }, }, - expected: OpenTelemetryCollector{ + expected: v1beta1.OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ "app.kubernetes.io/managed-by": "opentelemetry-operator", }, }, - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeDeployment, - UpgradeStrategy: UpgradeStrategyAutomatic, - OpenTelemetryCommonFields: OpenTelemetryCommonFields{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeDeployment, + UpgradeStrategy: v1beta1.UpgradeStrategyAutomatic, + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ Replicas: &one, - ManagementState: ManagementStateManaged, - PodDisruptionBudget: &PodDisruptionBudgetSpec{ + ManagementState: v1beta1.ManagementStateManaged, + PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ MaxUnavailable: &intstr.IntOrString{ Type: intstr.Int, IntVal: 1, }, }, }, - Autoscaler: &AutoscalerSpec{ + Autoscaler: &v1beta1.AutoscalerSpec{ TargetCPUUtilization: &defaultCPUTarget, MaxReplicas: &five, MinReplicas: &one, @@ -226,49 +285,49 @@ func TestCollectorDefaultingWebhook(t *testing.T) { }, { name: "Missing route termination", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeDeployment, - Ingress: Ingress{ - Type: IngressTypeRoute, + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeDeployment, + Ingress: v1beta1.Ingress{ + Type: v1beta1.IngressTypeRoute, }, }, }, - expected: OpenTelemetryCollector{ + expected: v1beta1.OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ "app.kubernetes.io/managed-by": "opentelemetry-operator", }, }, - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeDeployment, - OpenTelemetryCommonFields: OpenTelemetryCommonFields{ - ManagementState: ManagementStateManaged, + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeDeployment, + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + ManagementState: v1beta1.ManagementStateManaged, Replicas: &one, - PodDisruptionBudget: &PodDisruptionBudgetSpec{ + PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ MaxUnavailable: &intstr.IntOrString{ Type: intstr.Int, IntVal: 1, }, }, }, - Ingress: Ingress{ - Type: IngressTypeRoute, - Route: OpenShiftRoute{ - Termination: TLSRouteTerminationTypeEdge, + Ingress: v1beta1.Ingress{ + Type: v1beta1.IngressTypeRoute, + Route: v1beta1.OpenShiftRoute{ + Termination: v1beta1.TLSRouteTerminationTypeEdge, }, }, - UpgradeStrategy: UpgradeStrategyAutomatic, + UpgradeStrategy: v1beta1.UpgradeStrategyAutomatic, }, }, }, { name: "Defined PDB for collector", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeDeployment, - OpenTelemetryCommonFields: OpenTelemetryCommonFields{ - PodDisruptionBudget: &PodDisruptionBudgetSpec{ + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeDeployment, + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ MinAvailable: &intstr.IntOrString{ Type: intstr.String, StrVal: "10%", @@ -277,37 +336,37 @@ func TestCollectorDefaultingWebhook(t *testing.T) { }, }, }, - expected: OpenTelemetryCollector{ + expected: v1beta1.OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ "app.kubernetes.io/managed-by": "opentelemetry-operator", }, }, - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeDeployment, - OpenTelemetryCommonFields: OpenTelemetryCommonFields{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeDeployment, + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ Replicas: &one, - ManagementState: ManagementStateManaged, - PodDisruptionBudget: &PodDisruptionBudgetSpec{ + ManagementState: v1beta1.ManagementStateManaged, + PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ MinAvailable: &intstr.IntOrString{ Type: intstr.String, StrVal: "10%", }, }, }, - UpgradeStrategy: UpgradeStrategyAutomatic, + UpgradeStrategy: v1beta1.UpgradeStrategyAutomatic, }, }, }, { name: "Defined PDB for target allocator", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeDeployment, - TargetAllocator: TargetAllocatorEmbedded{ + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeDeployment, + TargetAllocator: v1beta1.TargetAllocatorEmbedded{ Enabled: true, - AllocationStrategy: TargetAllocatorAllocationStrategyConsistentHashing, - PodDisruptionBudget: &PodDisruptionBudgetSpec{ + AllocationStrategy: v1beta1.TargetAllocatorAllocationStrategyConsistentHashing, + PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ MinAvailable: &intstr.IntOrString{ Type: intstr.String, StrVal: "10%", @@ -316,30 +375,30 @@ func TestCollectorDefaultingWebhook(t *testing.T) { }, }, }, - expected: OpenTelemetryCollector{ + expected: v1beta1.OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ "app.kubernetes.io/managed-by": "opentelemetry-operator", }, }, - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeDeployment, - OpenTelemetryCommonFields: OpenTelemetryCommonFields{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeDeployment, + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ Replicas: &one, - ManagementState: ManagementStateManaged, - PodDisruptionBudget: &PodDisruptionBudgetSpec{ + ManagementState: v1beta1.ManagementStateManaged, + PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ MaxUnavailable: &intstr.IntOrString{ Type: intstr.Int, IntVal: 1, }, }, }, - UpgradeStrategy: UpgradeStrategyAutomatic, - TargetAllocator: TargetAllocatorEmbedded{ + UpgradeStrategy: v1beta1.UpgradeStrategyAutomatic, + TargetAllocator: v1beta1.TargetAllocatorEmbedded{ Enabled: true, Replicas: &one, - AllocationStrategy: TargetAllocatorAllocationStrategyConsistentHashing, - PodDisruptionBudget: &PodDisruptionBudgetSpec{ + AllocationStrategy: v1beta1.TargetAllocatorAllocationStrategyConsistentHashing, + PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ MinAvailable: &intstr.IntOrString{ Type: intstr.String, StrVal: "10%", @@ -351,13 +410,13 @@ func TestCollectorDefaultingWebhook(t *testing.T) { }, { name: "Defined PDB for target allocator per-node", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeDeployment, - TargetAllocator: TargetAllocatorEmbedded{ + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeDeployment, + TargetAllocator: v1beta1.TargetAllocatorEmbedded{ Enabled: true, - AllocationStrategy: TargetAllocatorAllocationStrategyPerNode, - PodDisruptionBudget: &PodDisruptionBudgetSpec{ + AllocationStrategy: v1beta1.TargetAllocatorAllocationStrategyPerNode, + PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ MinAvailable: &intstr.IntOrString{ Type: intstr.String, StrVal: "10%", @@ -366,30 +425,30 @@ func TestCollectorDefaultingWebhook(t *testing.T) { }, }, }, - expected: OpenTelemetryCollector{ + expected: v1beta1.OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ "app.kubernetes.io/managed-by": "opentelemetry-operator", }, }, - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeDeployment, - OpenTelemetryCommonFields: OpenTelemetryCommonFields{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeDeployment, + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ Replicas: &one, - ManagementState: ManagementStateManaged, - PodDisruptionBudget: &PodDisruptionBudgetSpec{ + ManagementState: v1beta1.ManagementStateManaged, + PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ MaxUnavailable: &intstr.IntOrString{ Type: intstr.Int, IntVal: 1, }, }, }, - UpgradeStrategy: UpgradeStrategyAutomatic, - TargetAllocator: TargetAllocatorEmbedded{ + UpgradeStrategy: v1beta1.UpgradeStrategyAutomatic, + TargetAllocator: v1beta1.TargetAllocatorEmbedded{ Enabled: true, Replicas: &one, - AllocationStrategy: TargetAllocatorAllocationStrategyPerNode, - PodDisruptionBudget: &PodDisruptionBudgetSpec{ + AllocationStrategy: v1beta1.TargetAllocatorAllocationStrategyPerNode, + PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ MinAvailable: &intstr.IntOrString{ Type: intstr.String, StrVal: "10%", @@ -401,40 +460,40 @@ func TestCollectorDefaultingWebhook(t *testing.T) { }, { name: "Undefined PDB for target allocator and consistent-hashing strategy", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeDeployment, - TargetAllocator: TargetAllocatorEmbedded{ + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeDeployment, + TargetAllocator: v1beta1.TargetAllocatorEmbedded{ Enabled: true, Replicas: &one, - AllocationStrategy: TargetAllocatorAllocationStrategyConsistentHashing, + AllocationStrategy: v1beta1.TargetAllocatorAllocationStrategyConsistentHashing, }, }, }, - expected: OpenTelemetryCollector{ + expected: v1beta1.OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ "app.kubernetes.io/managed-by": "opentelemetry-operator", }, }, - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeDeployment, - OpenTelemetryCommonFields: OpenTelemetryCommonFields{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeDeployment, + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ Replicas: &one, - ManagementState: ManagementStateManaged, - PodDisruptionBudget: &PodDisruptionBudgetSpec{ + ManagementState: v1beta1.ManagementStateManaged, + PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ MaxUnavailable: &intstr.IntOrString{ Type: intstr.Int, IntVal: 1, }, }, }, - UpgradeStrategy: UpgradeStrategyAutomatic, - TargetAllocator: TargetAllocatorEmbedded{ + UpgradeStrategy: v1beta1.UpgradeStrategyAutomatic, + TargetAllocator: v1beta1.TargetAllocatorEmbedded{ Enabled: true, Replicas: &one, - AllocationStrategy: TargetAllocatorAllocationStrategyConsistentHashing, - PodDisruptionBudget: &PodDisruptionBudgetSpec{ + AllocationStrategy: v1beta1.TargetAllocatorAllocationStrategyConsistentHashing, + PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ MaxUnavailable: &intstr.IntOrString{ Type: intstr.Int, IntVal: 1, @@ -446,55 +505,74 @@ func TestCollectorDefaultingWebhook(t *testing.T) { }, { name: "Undefined PDB for target allocator and not consistent-hashing strategy", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeDeployment, - TargetAllocator: TargetAllocatorEmbedded{ + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeDeployment, + TargetAllocator: v1beta1.TargetAllocatorEmbedded{ Enabled: true, - AllocationStrategy: TargetAllocatorAllocationStrategyLeastWeighted, + AllocationStrategy: v1beta1.TargetAllocatorAllocationStrategyLeastWeighted, }, }, }, - expected: OpenTelemetryCollector{ + expected: v1beta1.OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ "app.kubernetes.io/managed-by": "opentelemetry-operator", }, }, - Spec: OpenTelemetryCollectorSpec{ - Mode: ModeDeployment, - OpenTelemetryCommonFields: OpenTelemetryCommonFields{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeDeployment, + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ Replicas: &one, - ManagementState: ManagementStateManaged, - PodDisruptionBudget: &PodDisruptionBudgetSpec{ + ManagementState: v1beta1.ManagementStateManaged, + PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ MaxUnavailable: &intstr.IntOrString{ Type: intstr.Int, IntVal: 1, }, }, }, - UpgradeStrategy: UpgradeStrategyAutomatic, - TargetAllocator: TargetAllocatorEmbedded{ + UpgradeStrategy: v1beta1.UpgradeStrategyAutomatic, + TargetAllocator: v1beta1.TargetAllocatorEmbedded{ Enabled: true, Replicas: &one, - AllocationStrategy: TargetAllocatorAllocationStrategyLeastWeighted, + AllocationStrategy: v1beta1.TargetAllocatorAllocationStrategyLeastWeighted, }, }, }, }, } + bv := func(collector v1beta1.OpenTelemetryCollector) (admission.Warnings, error) { + cfg := config.New( + config.WithCollectorImage("default-collector"), + config.WithTargetAllocatorImage("default-ta-allocator"), + ) + params := manifests.Params{ + Log: logr.Discard(), + Config: cfg, + OtelCol: collector, + } + _, err := collectorManifests.Build(params) + if err != nil { + return nil, err + } + return nil, nil + } + for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { - cvw := &CollectorWebhook{ - logger: logr.Discard(), - scheme: testScheme, - cfg: config.New( + cvw := v1beta1.NewCollectorWebhook( + logr.Discard(), + testScheme, + config.New( config.WithCollectorImage("collector:v0.0.0"), config.WithTargetAllocatorImage("ta:v0.0.0"), ), - } + getReviewer(test.shouldFailSar), + bv, + ) ctx := context.Background() err := cvw.Default(ctx, &test.otelcol) assert.NoError(t, err) @@ -502,3 +580,793 @@ func TestCollectorDefaultingWebhook(t *testing.T) { }) } } + +func TestOTELColValidatingWebhook(t *testing.T) { + minusOne := int32(-1) + zero := int32(0) + zero64 := int64(0) + one := int32(1) + three := int32(3) + five := int32(5) + + if err := v1beta1.AddToScheme(testScheme); err != nil { + fmt.Printf("failed to register scheme: %v", err) + os.Exit(1) + } + + cfg := v1beta1.Config{} + err := yaml.Unmarshal([]byte(cfgYaml), &cfg) + require.NoError(t, err) + + tests := []struct { //nolint:govet + name string + otelcol v1beta1.OpenTelemetryCollector + expectedErr string + expectedWarnings []string + shouldFailSar bool + }{ + { + name: "valid empty spec", + otelcol: v1beta1.OpenTelemetryCollector{}, + }, + { + name: "valid full spec", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeStatefulSet, + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + Replicas: &three, + Ports: []v1beta1.PortsSpec{ + { + ServicePort: v1.ServicePort{ + Name: "port1", + Port: 5555, + }, + }, + { + ServicePort: v1.ServicePort{ + Name: "port2", + Port: 5554, + Protocol: v1.ProtocolUDP, + }, + }, + }, + }, + Autoscaler: &v1beta1.AutoscalerSpec{ + MinReplicas: &one, + MaxReplicas: &five, + Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{ + ScaleDown: &autoscalingv2.HPAScalingRules{ + StabilizationWindowSeconds: &three, + }, + ScaleUp: &autoscalingv2.HPAScalingRules{ + StabilizationWindowSeconds: &five, + }, + }, + TargetCPUUtilization: &five, + }, + UpgradeStrategy: "adhoc", + TargetAllocator: v1beta1.TargetAllocatorEmbedded{ + Enabled: true, + }, + Config: cfg, + }, + }, + }, + { + name: "prom CR admissions warning", + shouldFailSar: true, // force failure + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeStatefulSet, + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + Replicas: &three, + Ports: []v1beta1.PortsSpec{ + { + ServicePort: v1.ServicePort{ + Name: "port1", + Port: 5555, + }, + }, + { + ServicePort: v1.ServicePort{ + Name: "port2", + Port: 5554, + Protocol: v1.ProtocolUDP, + }, + }, + }, + }, + Autoscaler: &v1beta1.AutoscalerSpec{ + MinReplicas: &one, + MaxReplicas: &five, + Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{ + ScaleDown: &autoscalingv2.HPAScalingRules{ + StabilizationWindowSeconds: &three, + }, + ScaleUp: &autoscalingv2.HPAScalingRules{ + StabilizationWindowSeconds: &five, + }, + }, + TargetCPUUtilization: &five, + }, + UpgradeStrategy: "adhoc", + TargetAllocator: v1beta1.TargetAllocatorEmbedded{ + Enabled: true, + PrometheusCR: v1beta1.TargetAllocatorPrometheusCR{Enabled: true}, + }, + Config: cfg, + }, + }, + expectedWarnings: []string{ + "missing the following rules for monitoring.coreos.com/servicemonitors: [*]", + "missing the following rules for monitoring.coreos.com/podmonitors: [*]", + "missing the following rules for nodes/metrics: [get,list,watch]", + "missing the following rules for services: [get,list,watch]", + "missing the following rules for endpoints: [get,list,watch]", + "missing the following rules for namespaces: [get,list,watch]", + "missing the following rules for networking.k8s.io/ingresses: [get,list,watch]", + "missing the following rules for nodes: [get,list,watch]", + "missing the following rules for pods: [get,list,watch]", + "missing the following rules for configmaps: [get]", + "missing the following rules for discovery.k8s.io/endpointslices: [get,list,watch]", + "missing the following rules for nonResourceURL: /metrics: [get]", + }, + }, + { + name: "prom CR no admissions warning", + shouldFailSar: false, // force SAR okay + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeStatefulSet, + UpgradeStrategy: "adhoc", + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + Replicas: &three, + Ports: []v1beta1.PortsSpec{ + { + ServicePort: v1.ServicePort{ + Name: "port1", + Port: 5555, + }, + }, + { + ServicePort: v1.ServicePort{ + Name: "port2", + Port: 5554, + Protocol: v1.ProtocolUDP, + }, + }, + }, + }, + Autoscaler: &v1beta1.AutoscalerSpec{ + Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{ + ScaleDown: &autoscalingv2.HPAScalingRules{ + StabilizationWindowSeconds: &three, + }, + ScaleUp: &autoscalingv2.HPAScalingRules{ + StabilizationWindowSeconds: &five, + }, + }, + TargetCPUUtilization: &five, + }, + TargetAllocator: v1beta1.TargetAllocatorEmbedded{ + Enabled: true, + PrometheusCR: v1beta1.TargetAllocatorPrometheusCR{Enabled: true}, + }, + Config: cfg, + }, + }, + }, + { + name: "invalid mode with volume claim templates", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeSidecar, + StatefulSetCommonFields: v1beta1.StatefulSetCommonFields{ + VolumeClaimTemplates: []v1.PersistentVolumeClaim{{}, {}}, + }, + }, + }, + expectedErr: "does not support the attribute 'volumeClaimTemplates'", + }, + { + name: "invalid mode with tolerations", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeSidecar, + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + Tolerations: []v1.Toleration{{}, {}}, + }, + }, + }, + expectedErr: "does not support the attribute 'tolerations'", + }, + { + name: "invalid mode with target allocator", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeDeployment, + TargetAllocator: v1beta1.TargetAllocatorEmbedded{ + Enabled: true, + }, + }, + }, + expectedErr: "does not support the target allocation deployment", + }, + { + name: "invalid target allocator config", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeStatefulSet, + TargetAllocator: v1beta1.TargetAllocatorEmbedded{ + Enabled: true, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec Prometheus configuration is incorrect", + }, + { + name: "invalid target allocation strategy", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeDaemonSet, + TargetAllocator: v1beta1.TargetAllocatorEmbedded{ + Enabled: true, + AllocationStrategy: v1beta1.TargetAllocatorAllocationStrategyLeastWeighted, + }, + }, + }, + expectedErr: "mode is set to daemonset, which must be used with target allocation strategy per-node", + }, + { + name: "invalid port name", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + Ports: []v1beta1.PortsSpec{ + { + ServicePort: v1.ServicePort{ + // this port name contains a non alphanumeric character, which is invalid. + Name: "-testšŸ¦„port", + Port: 12345, + Protocol: v1.ProtocolTCP, + }, + }, + }, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec Ports configuration is incorrect", + }, + { + name: "invalid port name, too long", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + Ports: []v1beta1.PortsSpec{ + { + ServicePort: v1.ServicePort{ + Name: "aaaabbbbccccdddd", // len: 16, too long + Port: 5555, + }, + }, + }, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec Ports configuration is incorrect", + }, + { + name: "invalid port num", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + Ports: []v1beta1.PortsSpec{ + { + ServicePort: v1.ServicePort{ + Name: "aaaabbbbccccddd", // len: 15 + // no port set means it's 0, which is invalid + }, + }, + }, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec Ports configuration is incorrect", + }, + { + name: "invalid max replicas", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Autoscaler: &v1beta1.AutoscalerSpec{ + MaxReplicas: &zero, + }, + }, + }, + expectedErr: "maxReplicas should be defined and one or more", + }, + { + name: "invalid replicas, greater than max", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + Replicas: &five, + }, + Autoscaler: &v1beta1.AutoscalerSpec{ + MaxReplicas: &three, + }, + }, + }, + expectedErr: "replicas must not be greater than maxReplicas", + }, + { + name: "invalid min replicas, greater than max", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Autoscaler: &v1beta1.AutoscalerSpec{ + MaxReplicas: &three, + MinReplicas: &five, + }, + }, + }, + expectedErr: "minReplicas must not be greater than maxReplicas", + }, + { + name: "invalid min replicas, lesser than 1", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Autoscaler: &v1beta1.AutoscalerSpec{ + MaxReplicas: &three, + MinReplicas: &zero, + }, + }, + }, + expectedErr: "minReplicas should be one or more", + }, + { + name: "invalid autoscaler scale down", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Autoscaler: &v1beta1.AutoscalerSpec{ + MaxReplicas: &three, + Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{ + ScaleDown: &autoscalingv2.HPAScalingRules{ + StabilizationWindowSeconds: &zero, + }, + }, + }, + }, + }, + expectedErr: "scaleDown should be one or more", + }, + { + name: "invalid autoscaler scale up", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Autoscaler: &v1beta1.AutoscalerSpec{ + MaxReplicas: &three, + Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{ + ScaleUp: &autoscalingv2.HPAScalingRules{ + StabilizationWindowSeconds: &zero, + }, + }, + }, + }, + }, + expectedErr: "scaleUp should be one or more", + }, + { + name: "invalid autoscaler target cpu utilization", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Autoscaler: &v1beta1.AutoscalerSpec{ + MaxReplicas: &three, + TargetCPUUtilization: &zero, + }, + }, + }, + expectedErr: "targetCPUUtilization should be greater than 0 and less than 100", + }, + { + name: "autoscaler minReplicas is less than maxReplicas", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Autoscaler: &v1beta1.AutoscalerSpec{ + MaxReplicas: &one, + MinReplicas: &five, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, minReplicas must not be greater than maxReplicas", + }, + { + name: "invalid autoscaler metric type", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Autoscaler: &v1beta1.AutoscalerSpec{ + MaxReplicas: &three, + Metrics: []v1beta1.MetricSpec{ + { + Type: autoscalingv2.ResourceMetricSourceType, + }, + }, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, metric type unsupported. Expected metric of source type Pod", + }, + { + name: "invalid pod metric average value", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Autoscaler: &v1beta1.AutoscalerSpec{ + MaxReplicas: &three, + Metrics: []v1beta1.MetricSpec{ + { + Type: autoscalingv2.PodsMetricSourceType, + Pods: &autoscalingv2.PodsMetricSource{ + Metric: autoscalingv2.MetricIdentifier{ + Name: "custom1", + }, + Target: autoscalingv2.MetricTarget{ + Type: autoscalingv2.AverageValueMetricType, + AverageValue: resource.NewQuantity(int64(0), resource.DecimalSI), + }, + }, + }, + }, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, average value should be greater than 0", + }, + { + name: "utilization target is not valid with pod metrics", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Autoscaler: &v1beta1.AutoscalerSpec{ + MaxReplicas: &three, + Metrics: []v1beta1.MetricSpec{ + { + Type: autoscalingv2.PodsMetricSourceType, + Pods: &autoscalingv2.PodsMetricSource{ + Metric: autoscalingv2.MetricIdentifier{ + Name: "custom1", + }, + Target: autoscalingv2.MetricTarget{ + Type: autoscalingv2.UtilizationMetricType, + AverageUtilization: &one, + }, + }, + }, + }, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, invalid pods target type", + }, + { + name: "invalid deployment mode incompatible with ingress settings", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeSidecar, + Ingress: v1beta1.Ingress{ + Type: v1beta1.IngressTypeIngress, + }, + }, + }, + expectedErr: fmt.Sprintf("Ingress can only be used in combination with the modes: %s, %s, %s", v1beta1.ModeDeployment, v1beta1.ModeDaemonSet, v1beta1.ModeStatefulSet), + }, + { + name: "invalid mode with priorityClassName", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeSidecar, + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + PriorityClassName: "test-class", + }, + }, + }, + expectedErr: "does not support the attribute 'priorityClassName'", + }, + { + name: "invalid mode with affinity", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeSidecar, + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + Affinity: &v1.Affinity{ + NodeAffinity: &v1.NodeAffinity{ + RequiredDuringSchedulingIgnoredDuringExecution: &v1.NodeSelector{ + NodeSelectorTerms: []v1.NodeSelectorTerm{ + { + MatchExpressions: []v1.NodeSelectorRequirement{ + { + Key: "node", + Operator: v1.NodeSelectorOpIn, + Values: []string{"test-node"}, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + expectedErr: "does not support the attribute 'affinity'", + }, + { + name: "invalid InitialDelaySeconds", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + LivenessProbe: &v1beta1.Probe{ + InitialDelaySeconds: &minusOne, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec LivenessProbe InitialDelaySeconds configuration is incorrect", + }, + { + name: "invalid InitialDelaySeconds readiness", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + ReadinessProbe: &v1beta1.Probe{ + InitialDelaySeconds: &minusOne, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec ReadinessProbe InitialDelaySeconds configuration is incorrect", + }, + { + name: "invalid PeriodSeconds", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + LivenessProbe: &v1beta1.Probe{ + PeriodSeconds: &zero, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec LivenessProbe PeriodSeconds configuration is incorrect", + }, + { + name: "invalid PeriodSeconds readiness", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + ReadinessProbe: &v1beta1.Probe{ + PeriodSeconds: &zero, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec ReadinessProbe PeriodSeconds configuration is incorrect", + }, + { + name: "invalid TimeoutSeconds", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + LivenessProbe: &v1beta1.Probe{ + TimeoutSeconds: &zero, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec LivenessProbe TimeoutSeconds configuration is incorrect", + }, + { + name: "invalid TimeoutSeconds readiness", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + ReadinessProbe: &v1beta1.Probe{ + TimeoutSeconds: &zero, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec ReadinessProbe TimeoutSeconds configuration is incorrect", + }, + { + name: "invalid SuccessThreshold", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + LivenessProbe: &v1beta1.Probe{ + SuccessThreshold: &zero, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec LivenessProbe SuccessThreshold configuration is incorrect", + }, + { + name: "invalid SuccessThreshold readiness", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + ReadinessProbe: &v1beta1.Probe{ + SuccessThreshold: &zero, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec ReadinessProbe SuccessThreshold configuration is incorrect", + }, + { + name: "invalid FailureThreshold", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + LivenessProbe: &v1beta1.Probe{ + FailureThreshold: &zero, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec LivenessProbe FailureThreshold configuration is incorrect", + }, + { + name: "invalid FailureThreshold readiness", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + ReadinessProbe: &v1beta1.Probe{ + FailureThreshold: &zero, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec ReadinessProbe FailureThreshold configuration is incorrect", + }, + { + name: "invalid TerminationGracePeriodSeconds", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + LivenessProbe: &v1beta1.Probe{ + TerminationGracePeriodSeconds: &zero64, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec LivenessProbe TerminationGracePeriodSeconds configuration is incorrect", + }, + { + name: "invalid TerminationGracePeriodSeconds readiness", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + ReadinessProbe: &v1beta1.Probe{ + TerminationGracePeriodSeconds: &zero64, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec ReadinessProbe TerminationGracePeriodSeconds configuration is incorrect", + }, + { + name: "invalid AdditionalContainers", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeSidecar, + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + AdditionalContainers: []v1.Container{ + { + Name: "test", + }, + }, + }, + }, + }, + expectedErr: "the OpenTelemetry Collector mode is set to sidecar, which does not support the attribute 'AdditionalContainers'", + }, + { + name: "missing ingress hostname for subdomain ruleType", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Ingress: v1beta1.Ingress{ + RuleType: v1beta1.IngressRuleTypeSubdomain, + }, + }, + }, + expectedErr: "a valid Ingress hostname has to be defined for subdomain ruleType", + }, + { + name: "invalid updateStrategy for Deployment mode", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeDeployment, + DaemonSetUpdateStrategy: appsv1.DaemonSetUpdateStrategy{ + Type: "RollingUpdate", + RollingUpdate: &appsv1.RollingUpdateDaemonSet{ + MaxSurge: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)}, + MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)}, + }, + }, + }, + }, + expectedErr: "the OpenTelemetry Collector mode is set to deployment, which does not support the attribute 'updateStrategy'", + }, + { + name: "invalid updateStrategy for Statefulset mode", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Mode: v1beta1.ModeStatefulSet, + DeploymentUpdateStrategy: appsv1.DeploymentStrategy{ + Type: "RollingUpdate", + RollingUpdate: &appsv1.RollingUpdateDeployment{ + MaxSurge: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)}, + MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)}, + }, + }, + }, + }, + expectedErr: "the OpenTelemetry Collector mode is set to statefulset, which does not support the attribute 'deploymentUpdateStrategy'", + }, + { + name: "missing port for ingress type", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ + Ports: []v1beta1.PortsSpec{ + { + ServicePort: v1.ServicePort{}, + }, + }, + }, + Ingress: v1beta1.Ingress{ + Type: v1beta1.IngressTypeIngress, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec Ports configuration is incorrect", + }, + } + + bv := func(collector v1beta1.OpenTelemetryCollector) (admission.Warnings, error) { + cfg := config.New( + config.WithCollectorImage("default-collector"), + config.WithTargetAllocatorImage("default-ta-allocator"), + ) + params := manifests.Params{ + Log: logr.Discard(), + Config: cfg, + OtelCol: collector, + } + _, err := collectorManifests.Build(params) + if err != nil { + return nil, err + } + return nil, nil + } + + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + cvw := v1beta1.NewCollectorWebhook( + logr.Discard(), + testScheme, + config.New( + config.WithCollectorImage("collector:v0.0.0"), + config.WithTargetAllocatorImage("ta:v0.0.0"), + ), + getReviewer(test.shouldFailSar), + bv, + ) + ctx := context.Background() + warnings, err := cvw.ValidateCreate(ctx, &test.otelcol) + if test.expectedErr == "" { + assert.NoError(t, err) + } else { + assert.ErrorContains(t, err, test.expectedErr) + } + assert.Equal(t, len(test.expectedWarnings), len(warnings)) + assert.ElementsMatch(t, warnings, test.expectedWarnings) + }) + } +} + +func getReviewer(shouldFailSAR bool) *rbac.Reviewer { + c := fake.NewSimpleClientset() + c.PrependReactor("create", "subjectaccessreviews", func(action kubeTesting.Action) (handled bool, ret runtime.Object, err error) { + // check our expectation here + if !action.Matches("create", "subjectaccessreviews") { + return false, nil, fmt.Errorf("must be a create for a SAR") + } + sar, ok := action.(kubeTesting.CreateAction).GetObject().DeepCopyObject().(*authv1.SubjectAccessReview) + if !ok || sar == nil { + return false, nil, fmt.Errorf("bad object") + } + sar.Status = authv1.SubjectAccessReviewStatus{ + Allowed: !shouldFailSAR, + Denied: shouldFailSAR, + } + return true, sar, nil + }) + return rbac.NewReviewer(c) +} diff --git a/apis/v1beta1/verifier_test.go b/apis/v1beta1/verifier_test.go deleted file mode 100644 index be3803e1ce..0000000000 --- a/apis/v1beta1/verifier_test.go +++ /dev/null @@ -1,852 +0,0 @@ -// Copyright The OpenTelemetry Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package v1beta1_test - -import ( - "context" - "fmt" - "github.com/go-logr/logr" - "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" - "github.com/open-telemetry/opentelemetry-operator/internal/config" - "github.com/open-telemetry/opentelemetry-operator/internal/manifests" - collectorManifests "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" - "github.com/open-telemetry/opentelemetry-operator/internal/rbac" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "gopkg.in/yaml.v3" - appsv1 "k8s.io/api/apps/v1" - authv1 "k8s.io/api/authorization/v1" - autoscalingv2 "k8s.io/api/autoscaling/v2" - v1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/resource" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/intstr" - "k8s.io/client-go/kubernetes/fake" - "k8s.io/client-go/kubernetes/scheme" - kubeTesting "k8s.io/client-go/testing" - "os" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" - "testing" -) - -var ( - testScheme = scheme.Scheme -) - -var cfgYaml = `receivers: - examplereceiver: - endpoint: "0.0.0.0:12345" - examplereceiver/settings: - endpoint: "0.0.0.0:12346" - prometheus: - config: - scrape_configs: - - job_name: otel-collector - scrape_interval: 10s - jaeger/custom: - protocols: - thrift_http: - endpoint: 0.0.0.0:15268 -` - -func TestOTELColValidatingWebhook(t *testing.T) { - minusOne := int32(-1) - zero := int32(0) - zero64 := int64(0) - one := int32(1) - three := int32(3) - five := int32(5) - - if err := v1beta1.AddToScheme(testScheme); err != nil { - fmt.Printf("failed to register scheme: %v", err) - os.Exit(1) - } - - cfg := v1beta1.Config{} - err := yaml.Unmarshal([]byte(cfgYaml), &cfg) - require.NoError(t, err) - - tests := []struct { //nolint:govet - name string - otelcol v1beta1.OpenTelemetryCollector - expectedErr string - expectedWarnings []string - shouldFailSar bool - }{ - { - name: "valid empty spec", - otelcol: v1beta1.OpenTelemetryCollector{}, - }, - { - name: "valid full spec", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Mode: v1beta1.ModeStatefulSet, - OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ - Replicas: &three, - Ports: []v1beta1.PortsSpec{ - { - ServicePort: v1.ServicePort{ - Name: "port1", - Port: 5555, - }, - }, - { - ServicePort: v1.ServicePort{ - Name: "port2", - Port: 5554, - Protocol: v1.ProtocolUDP, - }, - }, - }, - }, - Autoscaler: &v1beta1.AutoscalerSpec{ - MinReplicas: &one, - MaxReplicas: &five, - Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{ - ScaleDown: &autoscalingv2.HPAScalingRules{ - StabilizationWindowSeconds: &three, - }, - ScaleUp: &autoscalingv2.HPAScalingRules{ - StabilizationWindowSeconds: &five, - }, - }, - TargetCPUUtilization: &five, - }, - UpgradeStrategy: "adhoc", - TargetAllocator: v1beta1.TargetAllocatorEmbedded{ - Enabled: true, - }, - Config: cfg, - }, - }, - }, - { - name: "prom CR admissions warning", - shouldFailSar: true, // force failure - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Mode: v1beta1.ModeStatefulSet, - OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ - Replicas: &three, - Ports: []v1beta1.PortsSpec{ - { - ServicePort: v1.ServicePort{ - Name: "port1", - Port: 5555, - }, - }, - { - ServicePort: v1.ServicePort{ - Name: "port2", - Port: 5554, - Protocol: v1.ProtocolUDP, - }, - }, - }, - }, - Autoscaler: &v1beta1.AutoscalerSpec{ - MinReplicas: &one, - MaxReplicas: &five, - Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{ - ScaleDown: &autoscalingv2.HPAScalingRules{ - StabilizationWindowSeconds: &three, - }, - ScaleUp: &autoscalingv2.HPAScalingRules{ - StabilizationWindowSeconds: &five, - }, - }, - TargetCPUUtilization: &five, - }, - UpgradeStrategy: "adhoc", - TargetAllocator: v1beta1.TargetAllocatorEmbedded{ - Enabled: true, - PrometheusCR: v1beta1.TargetAllocatorPrometheusCR{Enabled: true}, - }, - Config: cfg, - }, - }, - expectedWarnings: []string{ - "missing the following rules for monitoring.coreos.com/servicemonitors: [*]", - "missing the following rules for monitoring.coreos.com/podmonitors: [*]", - "missing the following rules for nodes/metrics: [get,list,watch]", - "missing the following rules for services: [get,list,watch]", - "missing the following rules for endpoints: [get,list,watch]", - "missing the following rules for namespaces: [get,list,watch]", - "missing the following rules for networking.k8s.io/ingresses: [get,list,watch]", - "missing the following rules for nodes: [get,list,watch]", - "missing the following rules for pods: [get,list,watch]", - "missing the following rules for configmaps: [get]", - "missing the following rules for discovery.k8s.io/endpointslices: [get,list,watch]", - "missing the following rules for nonResourceURL: /metrics: [get]", - }, - }, - { - name: "prom CR no admissions warning", - shouldFailSar: false, // force SAR okay - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Mode: v1beta1.ModeStatefulSet, - UpgradeStrategy: "adhoc", - OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ - Replicas: &three, - Ports: []v1beta1.PortsSpec{ - { - ServicePort: v1.ServicePort{ - Name: "port1", - Port: 5555, - }, - }, - { - ServicePort: v1.ServicePort{ - Name: "port2", - Port: 5554, - Protocol: v1.ProtocolUDP, - }, - }, - }, - }, - Autoscaler: &v1beta1.AutoscalerSpec{ - Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{ - ScaleDown: &autoscalingv2.HPAScalingRules{ - StabilizationWindowSeconds: &three, - }, - ScaleUp: &autoscalingv2.HPAScalingRules{ - StabilizationWindowSeconds: &five, - }, - }, - TargetCPUUtilization: &five, - }, - TargetAllocator: v1beta1.TargetAllocatorEmbedded{ - Enabled: true, - PrometheusCR: v1beta1.TargetAllocatorPrometheusCR{Enabled: true}, - }, - Config: cfg, - }, - }, - }, - { - name: "invalid mode with volume claim templates", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Mode: v1beta1.ModeSidecar, - StatefulSetCommonFields: v1beta1.StatefulSetCommonFields{ - VolumeClaimTemplates: []v1.PersistentVolumeClaim{{}, {}}, - }, - }, - }, - expectedErr: "does not support the attribute 'volumeClaimTemplates'", - }, - { - name: "invalid mode with tolerations", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Mode: v1beta1.ModeSidecar, - OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ - Tolerations: []v1.Toleration{{}, {}}, - }, - }, - }, - expectedErr: "does not support the attribute 'tolerations'", - }, - { - name: "invalid mode with target allocator", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Mode: v1beta1.ModeDeployment, - TargetAllocator: v1beta1.TargetAllocatorEmbedded{ - Enabled: true, - }, - }, - }, - expectedErr: "does not support the target allocation deployment", - }, - { - name: "invalid target allocator config", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Mode: v1beta1.ModeStatefulSet, - TargetAllocator: v1beta1.TargetAllocatorEmbedded{ - Enabled: true, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec Prometheus configuration is incorrect", - }, - { - name: "invalid target allocation strategy", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Mode: v1beta1.ModeDaemonSet, - TargetAllocator: v1beta1.TargetAllocatorEmbedded{ - Enabled: true, - AllocationStrategy: v1beta1.TargetAllocatorAllocationStrategyLeastWeighted, - }, - }, - }, - expectedErr: "mode is set to daemonset, which must be used with target allocation strategy per-node", - }, - { - name: "invalid port name", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ - Ports: []v1beta1.PortsSpec{ - { - ServicePort: v1.ServicePort{ - // this port name contains a non alphanumeric character, which is invalid. - Name: "-testšŸ¦„port", - Port: 12345, - Protocol: v1.ProtocolTCP, - }, - }, - }, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec Ports configuration is incorrect", - }, - { - name: "invalid port name, too long", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ - Ports: []v1beta1.PortsSpec{ - { - ServicePort: v1.ServicePort{ - Name: "aaaabbbbccccdddd", // len: 16, too long - Port: 5555, - }, - }, - }, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec Ports configuration is incorrect", - }, - { - name: "invalid port num", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ - Ports: []v1beta1.PortsSpec{ - { - ServicePort: v1.ServicePort{ - Name: "aaaabbbbccccddd", // len: 15 - // no port set means it's 0, which is invalid - }, - }, - }, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec Ports configuration is incorrect", - }, - { - name: "invalid max replicas", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Autoscaler: &v1beta1.AutoscalerSpec{ - MaxReplicas: &zero, - }, - }, - }, - expectedErr: "maxReplicas should be defined and one or more", - }, - { - name: "invalid replicas, greater than max", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ - Replicas: &five, - }, - Autoscaler: &v1beta1.AutoscalerSpec{ - MaxReplicas: &three, - }, - }, - }, - expectedErr: "replicas must not be greater than maxReplicas", - }, - { - name: "invalid min replicas, greater than max", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Autoscaler: &v1beta1.AutoscalerSpec{ - MaxReplicas: &three, - MinReplicas: &five, - }, - }, - }, - expectedErr: "minReplicas must not be greater than maxReplicas", - }, - { - name: "invalid min replicas, lesser than 1", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Autoscaler: &v1beta1.AutoscalerSpec{ - MaxReplicas: &three, - MinReplicas: &zero, - }, - }, - }, - expectedErr: "minReplicas should be one or more", - }, - { - name: "invalid autoscaler scale down", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Autoscaler: &v1beta1.AutoscalerSpec{ - MaxReplicas: &three, - Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{ - ScaleDown: &autoscalingv2.HPAScalingRules{ - StabilizationWindowSeconds: &zero, - }, - }, - }, - }, - }, - expectedErr: "scaleDown should be one or more", - }, - { - name: "invalid autoscaler scale up", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Autoscaler: &v1beta1.AutoscalerSpec{ - MaxReplicas: &three, - Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{ - ScaleUp: &autoscalingv2.HPAScalingRules{ - StabilizationWindowSeconds: &zero, - }, - }, - }, - }, - }, - expectedErr: "scaleUp should be one or more", - }, - { - name: "invalid autoscaler target cpu utilization", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Autoscaler: &v1beta1.AutoscalerSpec{ - MaxReplicas: &three, - TargetCPUUtilization: &zero, - }, - }, - }, - expectedErr: "targetCPUUtilization should be greater than 0 and less than 100", - }, - { - name: "autoscaler minReplicas is less than maxReplicas", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Autoscaler: &v1beta1.AutoscalerSpec{ - MaxReplicas: &one, - MinReplicas: &five, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, minReplicas must not be greater than maxReplicas", - }, - { - name: "invalid autoscaler metric type", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Autoscaler: &v1beta1.AutoscalerSpec{ - MaxReplicas: &three, - Metrics: []v1beta1.MetricSpec{ - { - Type: autoscalingv2.ResourceMetricSourceType, - }, - }, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, metric type unsupported. Expected metric of source type Pod", - }, - { - name: "invalid pod metric average value", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Autoscaler: &v1beta1.AutoscalerSpec{ - MaxReplicas: &three, - Metrics: []v1beta1.MetricSpec{ - { - Type: autoscalingv2.PodsMetricSourceType, - Pods: &autoscalingv2.PodsMetricSource{ - Metric: autoscalingv2.MetricIdentifier{ - Name: "custom1", - }, - Target: autoscalingv2.MetricTarget{ - Type: autoscalingv2.AverageValueMetricType, - AverageValue: resource.NewQuantity(int64(0), resource.DecimalSI), - }, - }, - }, - }, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, average value should be greater than 0", - }, - { - name: "utilization target is not valid with pod metrics", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Autoscaler: &v1beta1.AutoscalerSpec{ - MaxReplicas: &three, - Metrics: []v1beta1.MetricSpec{ - { - Type: autoscalingv2.PodsMetricSourceType, - Pods: &autoscalingv2.PodsMetricSource{ - Metric: autoscalingv2.MetricIdentifier{ - Name: "custom1", - }, - Target: autoscalingv2.MetricTarget{ - Type: autoscalingv2.UtilizationMetricType, - AverageUtilization: &one, - }, - }, - }, - }, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, invalid pods target type", - }, - { - name: "invalid deployment mode incompatible with ingress settings", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Mode: v1beta1.ModeSidecar, - Ingress: v1beta1.Ingress{ - Type: v1beta1.IngressTypeIngress, - }, - }, - }, - expectedErr: fmt.Sprintf("Ingress can only be used in combination with the modes: %s, %s, %s", v1beta1.ModeDeployment, v1beta1.ModeDaemonSet, v1beta1.ModeStatefulSet), - }, - { - name: "invalid mode with priorityClassName", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Mode: v1beta1.ModeSidecar, - OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ - PriorityClassName: "test-class", - }, - }, - }, - expectedErr: "does not support the attribute 'priorityClassName'", - }, - { - name: "invalid mode with affinity", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Mode: v1beta1.ModeSidecar, - OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ - Affinity: &v1.Affinity{ - NodeAffinity: &v1.NodeAffinity{ - RequiredDuringSchedulingIgnoredDuringExecution: &v1.NodeSelector{ - NodeSelectorTerms: []v1.NodeSelectorTerm{ - { - MatchExpressions: []v1.NodeSelectorRequirement{ - { - Key: "node", - Operator: v1.NodeSelectorOpIn, - Values: []string{"test-node"}, - }, - }, - }, - }, - }, - }, - }, - }, - }, - }, - expectedErr: "does not support the attribute 'affinity'", - }, - { - name: "invalid InitialDelaySeconds", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - LivenessProbe: &v1beta1.Probe{ - InitialDelaySeconds: &minusOne, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec LivenessProbe InitialDelaySeconds configuration is incorrect", - }, - { - name: "invalid InitialDelaySeconds readiness", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - ReadinessProbe: &v1beta1.Probe{ - InitialDelaySeconds: &minusOne, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec ReadinessProbe InitialDelaySeconds configuration is incorrect", - }, - { - name: "invalid PeriodSeconds", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - LivenessProbe: &v1beta1.Probe{ - PeriodSeconds: &zero, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec LivenessProbe PeriodSeconds configuration is incorrect", - }, - { - name: "invalid PeriodSeconds readiness", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - ReadinessProbe: &v1beta1.Probe{ - PeriodSeconds: &zero, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec ReadinessProbe PeriodSeconds configuration is incorrect", - }, - { - name: "invalid TimeoutSeconds", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - LivenessProbe: &v1beta1.Probe{ - TimeoutSeconds: &zero, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec LivenessProbe TimeoutSeconds configuration is incorrect", - }, - { - name: "invalid TimeoutSeconds readiness", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - ReadinessProbe: &v1beta1.Probe{ - TimeoutSeconds: &zero, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec ReadinessProbe TimeoutSeconds configuration is incorrect", - }, - { - name: "invalid SuccessThreshold", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - LivenessProbe: &v1beta1.Probe{ - SuccessThreshold: &zero, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec LivenessProbe SuccessThreshold configuration is incorrect", - }, - { - name: "invalid SuccessThreshold readiness", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - ReadinessProbe: &v1beta1.Probe{ - SuccessThreshold: &zero, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec ReadinessProbe SuccessThreshold configuration is incorrect", - }, - { - name: "invalid FailureThreshold", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - LivenessProbe: &v1beta1.Probe{ - FailureThreshold: &zero, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec LivenessProbe FailureThreshold configuration is incorrect", - }, - { - name: "invalid FailureThreshold readiness", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - ReadinessProbe: &v1beta1.Probe{ - FailureThreshold: &zero, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec ReadinessProbe FailureThreshold configuration is incorrect", - }, - { - name: "invalid TerminationGracePeriodSeconds", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - LivenessProbe: &v1beta1.Probe{ - TerminationGracePeriodSeconds: &zero64, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec LivenessProbe TerminationGracePeriodSeconds configuration is incorrect", - }, - { - name: "invalid TerminationGracePeriodSeconds readiness", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - ReadinessProbe: &v1beta1.Probe{ - TerminationGracePeriodSeconds: &zero64, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec ReadinessProbe TerminationGracePeriodSeconds configuration is incorrect", - }, - { - name: "invalid AdditionalContainers", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Mode: v1beta1.ModeSidecar, - OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ - AdditionalContainers: []v1.Container{ - { - Name: "test", - }, - }, - }, - }, - }, - expectedErr: "the OpenTelemetry Collector mode is set to sidecar, which does not support the attribute 'AdditionalContainers'", - }, - { - name: "missing ingress hostname for subdomain ruleType", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Ingress: v1beta1.Ingress{ - RuleType: v1beta1.IngressRuleTypeSubdomain, - }, - }, - }, - expectedErr: "a valid Ingress hostname has to be defined for subdomain ruleType", - }, - { - name: "invalid updateStrategy for Deployment mode", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Mode: v1beta1.ModeDeployment, - DaemonSetUpdateStrategy: appsv1.DaemonSetUpdateStrategy{ - Type: "RollingUpdate", - RollingUpdate: &appsv1.RollingUpdateDaemonSet{ - MaxSurge: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)}, - MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)}, - }, - }, - }, - }, - expectedErr: "the OpenTelemetry Collector mode is set to deployment, which does not support the attribute 'updateStrategy'", - }, - { - name: "invalid updateStrategy for Statefulset mode", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Mode: v1beta1.ModeStatefulSet, - DeploymentUpdateStrategy: appsv1.DeploymentStrategy{ - Type: "RollingUpdate", - RollingUpdate: &appsv1.RollingUpdateDeployment{ - MaxSurge: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)}, - MaxUnavailable: &intstr.IntOrString{Type: intstr.Int, IntVal: int32(1)}, - }, - }, - }, - }, - expectedErr: "the OpenTelemetry Collector mode is set to statefulset, which does not support the attribute 'deploymentUpdateStrategy'", - }, - { - name: "missing port for ingress type", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{ - Ports: []v1beta1.PortsSpec{ - { - ServicePort: v1.ServicePort{}, - }, - }, - }, - Ingress: v1beta1.Ingress{ - Type: v1beta1.IngressTypeIngress, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec Ports configuration is incorrect", - }, - } - - bv := func(collector v1beta1.OpenTelemetryCollector) (admission.Warnings, error) { - cfg := config.New( - config.WithCollectorImage("default-collector"), - config.WithTargetAllocatorImage("default-ta-allocator"), - ) - params := manifests.Params{ - Log: logr.Discard(), - Config: cfg, - OtelCol: collector, - } - _, err := collectorManifests.Build(params) - if err != nil { - return nil, err - } - return nil, nil - } - - for _, test := range tests { - test := test - t.Run(test.name, func(t *testing.T) { - cvw := v1beta1.NewCollectorWebhook( - logr.Discard(), - testScheme, - config.New( - config.WithCollectorImage("collector:v0.0.0"), - config.WithTargetAllocatorImage("ta:v0.0.0"), - ), - getReviewer(test.shouldFailSar), - bv, - ) - ctx := context.Background() - warnings, err := cvw.ValidateCreate(ctx, &test.otelcol) - if test.expectedErr == "" { - assert.NoError(t, err) - } else { - assert.ErrorContains(t, err, test.expectedErr) - } - assert.Equal(t, len(test.expectedWarnings), len(warnings)) - assert.ElementsMatch(t, warnings, test.expectedWarnings) - }) - } -} - -func getReviewer(shouldFailSAR bool) *rbac.Reviewer { - c := fake.NewSimpleClientset() - c.PrependReactor("create", "subjectaccessreviews", func(action kubeTesting.Action) (handled bool, ret runtime.Object, err error) { - // check our expectation here - if !action.Matches("create", "subjectaccessreviews") { - return false, nil, fmt.Errorf("must be a create for a SAR") - } - sar, ok := action.(kubeTesting.CreateAction).GetObject().DeepCopyObject().(*authv1.SubjectAccessReview) - if !ok || sar == nil { - return false, nil, fmt.Errorf("bad object") - } - sar.Status = authv1.SubjectAccessReviewStatus{ - Allowed: !shouldFailSAR, - Denied: shouldFailSAR, - } - return true, sar, nil - }) - return rbac.NewReviewer(c) -} diff --git a/main.go b/main.go index 309572bce0..0e98cf7aea 100644 --- a/main.go +++ b/main.go @@ -19,7 +19,6 @@ import ( "crypto/tls" "flag" "fmt" - "github.com/open-telemetry/opentelemetry-operator/internal/autodetect" "os" "regexp" "runtime" @@ -51,6 +50,7 @@ import ( otelv1alpha1 "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" otelv1beta1 "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" "github.com/open-telemetry/opentelemetry-operator/controllers" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus" "github.com/open-telemetry/opentelemetry-operator/internal/config" From 1f420d8c826f556527db5aa356bb1a496e850da2 Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Mon, 29 Jul 2024 13:51:31 -0700 Subject: [PATCH 09/38] linting --- apis/v1alpha1/zz_generated.deepcopy.go | 7 ++++--- apis/v1beta1/collector_webhook_test.go | 17 ++++++++++------- apis/v1beta1/zz_generated.deepcopy.go | 4 ++-- 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/apis/v1alpha1/zz_generated.deepcopy.go b/apis/v1alpha1/zz_generated.deepcopy.go index 3918521e75..d4930fdc59 100644 --- a/apis/v1alpha1/zz_generated.deepcopy.go +++ b/apis/v1alpha1/zz_generated.deepcopy.go @@ -19,13 +19,14 @@ package v1alpha1 import ( - "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" - "k8s.io/api/autoscaling/v2" - "k8s.io/api/core/v1" + v2 "k8s.io/api/autoscaling/v2" + v1 "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" + + "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" ) // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. diff --git a/apis/v1beta1/collector_webhook_test.go b/apis/v1beta1/collector_webhook_test.go index 98feff6b80..e686b46304 100644 --- a/apis/v1beta1/collector_webhook_test.go +++ b/apis/v1beta1/collector_webhook_test.go @@ -17,12 +17,12 @@ package v1beta1_test import ( "context" "fmt" - "github.com/go-logr/logr" - "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" - "github.com/open-telemetry/opentelemetry-operator/internal/config" - "github.com/open-telemetry/opentelemetry-operator/internal/manifests" + "os" + "testing" + collectorManifests "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" - "github.com/open-telemetry/opentelemetry-operator/internal/rbac" + + "github.com/go-logr/logr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gopkg.in/yaml.v3" @@ -37,9 +37,12 @@ import ( "k8s.io/client-go/kubernetes/fake" "k8s.io/client-go/kubernetes/scheme" kubeTesting "k8s.io/client-go/testing" - "os" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" - "testing" + + "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" + "github.com/open-telemetry/opentelemetry-operator/internal/config" + "github.com/open-telemetry/opentelemetry-operator/internal/manifests" + "github.com/open-telemetry/opentelemetry-operator/internal/rbac" ) var ( diff --git a/apis/v1beta1/zz_generated.deepcopy.go b/apis/v1beta1/zz_generated.deepcopy.go index e5fb8ef086..6c89ed1937 100644 --- a/apis/v1beta1/zz_generated.deepcopy.go +++ b/apis/v1beta1/zz_generated.deepcopy.go @@ -19,8 +19,8 @@ package v1beta1 import ( - "k8s.io/api/autoscaling/v2" - "k8s.io/api/core/v1" + v2 "k8s.io/api/autoscaling/v2" + v1 "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" From 0eae73d26bbff76fd28f1b20f7ceb28dbe0e247b Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Tue, 30 Jul 2024 09:36:39 -0700 Subject: [PATCH 10/38] changing to warnings --- apis/v1beta1/collector_webhook.go | 6 +++--- apis/v1beta1/collector_webhook_test.go | 24 +++++++++++++++--------- main.go | 11 +++++++---- 3 files changed, 25 insertions(+), 16 deletions(-) diff --git a/apis/v1beta1/collector_webhook.go b/apis/v1beta1/collector_webhook.go index dba9770ba5..9d89912ee9 100644 --- a/apis/v1beta1/collector_webhook.go +++ b/apis/v1beta1/collector_webhook.go @@ -177,7 +177,7 @@ func (c CollectorWebhook) ValidateCreate(ctx context.Context, obj runtime.Object c.metrics.create(ctx, otelcol) } if c.bv != nil { - newWarnings, err := c.bv(*otelcol) + newWarnings := c.bv(*otelcol) if err != nil { return append(warnings, newWarnings...), err } @@ -207,7 +207,7 @@ func (c CollectorWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj run } if c.bv != nil { - newWarnings, err := c.bv(*otelcol) + newWarnings := c.bv(*otelcol) if err != nil { return append(warnings, newWarnings...), err } @@ -469,7 +469,7 @@ func checkAutoscalerSpec(autoscaler *AutoscalerSpec) error { } // BuildValidator is mostly used for testing purposes -type BuildValidator func(c OpenTelemetryCollector) (admission.Warnings, error) +type BuildValidator func(c OpenTelemetryCollector) admission.Warnings func NewCollectorWebhook( logger logr.Logger, diff --git a/apis/v1beta1/collector_webhook_test.go b/apis/v1beta1/collector_webhook_test.go index e686b46304..e795d2a691 100644 --- a/apis/v1beta1/collector_webhook_test.go +++ b/apis/v1beta1/collector_webhook_test.go @@ -99,7 +99,8 @@ func TestValidate(t *testing.T) { }, } - bv := func(collector v1beta1.OpenTelemetryCollector) (admission.Warnings, error) { + bv := func(collector v1beta1.OpenTelemetryCollector) admission.Warnings { + var warnings admission.Warnings cfg := config.New( config.WithCollectorImage("default-collector"), config.WithTargetAllocatorImage("default-ta-allocator"), @@ -111,9 +112,10 @@ func TestValidate(t *testing.T) { } _, err := collectorManifests.Build(params) if err != nil { - return nil, err + warnings := append(warnings, err.Error()) + return warnings } - return nil, nil + return nil } for _, tt := range tests { @@ -546,7 +548,8 @@ func TestCollectorDefaultingWebhook(t *testing.T) { }, } - bv := func(collector v1beta1.OpenTelemetryCollector) (admission.Warnings, error) { + bv := func(collector v1beta1.OpenTelemetryCollector) admission.Warnings { + var warnings admission.Warnings cfg := config.New( config.WithCollectorImage("default-collector"), config.WithTargetAllocatorImage("default-ta-allocator"), @@ -558,9 +561,10 @@ func TestCollectorDefaultingWebhook(t *testing.T) { } _, err := collectorManifests.Build(params) if err != nil { - return nil, err + warnings := append(warnings, err.Error()) + return warnings } - return nil, nil + return nil } for _, test := range tests { @@ -1311,7 +1315,8 @@ func TestOTELColValidatingWebhook(t *testing.T) { }, } - bv := func(collector v1beta1.OpenTelemetryCollector) (admission.Warnings, error) { + bv := func(collector v1beta1.OpenTelemetryCollector) admission.Warnings { + var warnings admission.Warnings cfg := config.New( config.WithCollectorImage("default-collector"), config.WithTargetAllocatorImage("default-ta-allocator"), @@ -1323,9 +1328,10 @@ func TestOTELColValidatingWebhook(t *testing.T) { } _, err := collectorManifests.Build(params) if err != nil { - return nil, err + warnings := append(warnings, err.Error()) + return warnings } - return nil, nil + return nil } for _, test := range tests { diff --git a/main.go b/main.go index 0e98cf7aea..3e7abc8bb4 100644 --- a/main.go +++ b/main.go @@ -394,16 +394,19 @@ func main() { } - bv := func(collector otelv1beta1.OpenTelemetryCollector) (admission.Warnings, error) { + bv := func(collector otelv1beta1.OpenTelemetryCollector) admission.Warnings { + var warnings admission.Warnings params, err := collectorReconciler.GetParams(collector) if err != nil { - return nil, err + warnings = append(warnings, err.Error()) + return warnings } _, err = collectorManifests.Build(params) if err != nil { - return nil, err + warnings = append(warnings, err.Error()) + return warnings } - return nil, nil + return warnings } if err = otelv1beta1.SetupCollectorWebhook(mgr, cfg, reviewer, crdMetrics, bv); err != nil { From 55f7488100a9937ebc9262c6caec20036be994c0 Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Tue, 30 Jul 2024 10:07:04 -0700 Subject: [PATCH 11/38] shadow declarations --- apis/v1beta1/collector_webhook.go | 5 +---- apis/v1beta1/collector_webhook_test.go | 8 ++++---- main.go | 6 +++--- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/apis/v1beta1/collector_webhook.go b/apis/v1beta1/collector_webhook.go index 3e25e8734d..208f049d66 100644 --- a/apis/v1beta1/collector_webhook.go +++ b/apis/v1beta1/collector_webhook.go @@ -152,9 +152,6 @@ func (c CollectorWebhook) ValidateCreate(ctx context.Context, obj runtime.Object } if c.bv != nil { newWarnings := c.bv(*otelcol) - if err != nil { - return append(warnings, newWarnings...), err - } warnings = append(warnings, newWarnings...) } return warnings, nil @@ -452,7 +449,7 @@ func checkAutoscalerSpec(autoscaler *AutoscalerSpec) error { return nil } -// BuildValidator is mostly used for testing purposes +// BuildValidator is mostly used for testing purposes. type BuildValidator func(c OpenTelemetryCollector) admission.Warnings func NewCollectorWebhook( diff --git a/apis/v1beta1/collector_webhook_test.go b/apis/v1beta1/collector_webhook_test.go index e39edd5f9b..65d4b5a468 100644 --- a/apis/v1beta1/collector_webhook_test.go +++ b/apis/v1beta1/collector_webhook_test.go @@ -112,7 +112,7 @@ func TestValidate(t *testing.T) { } _, err := collectorManifests.Build(params) if err != nil { - warnings := append(warnings, err.Error()) + warnings = append(warnings, err.Error()) return warnings } return nil @@ -571,7 +571,7 @@ func TestCollectorDefaultingWebhook(t *testing.T) { } _, err := collectorManifests.Build(params) if err != nil { - warnings := append(warnings, err.Error()) + warnings = append(warnings, err.Error()) return warnings } return nil @@ -1338,7 +1338,7 @@ func TestOTELColValidatingWebhook(t *testing.T) { } _, err := collectorManifests.Build(params) if err != nil { - warnings := append(warnings, err.Error()) + warnings = append(warnings, err.Error()) return warnings } return nil @@ -1403,7 +1403,7 @@ func TestOTELColValidateUpdateWebhook(t *testing.T) { } _, err := collectorManifests.Build(params) if err != nil { - warnings := append(warnings, err.Error()) + warnings = append(warnings, err.Error()) return warnings } return nil diff --git a/main.go b/main.go index b95371f7d5..0b5cb03520 100644 --- a/main.go +++ b/main.go @@ -409,9 +409,9 @@ func main() { bv := func(collector otelv1beta1.OpenTelemetryCollector) admission.Warnings { var warnings admission.Warnings - params, err := collectorReconciler.GetParams(collector) - if err != nil { - warnings = append(warnings, err.Error()) + params, newErr := collectorReconciler.GetParams(collector) + if newErr != nil { + warnings = append(warnings, newErr.Error()) return warnings } _, err = collectorManifests.Build(params) From e57985ba3379fd67683ae722ab28af641fedeee7 Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Tue, 30 Jul 2024 10:16:44 -0700 Subject: [PATCH 12/38] fixing errors --- apis/v1beta1/collector_webhook.go | 3 --- apis/v1beta1/collector_webhook_test.go | 3 +-- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/apis/v1beta1/collector_webhook.go b/apis/v1beta1/collector_webhook.go index 208f049d66..ee993f177c 100644 --- a/apis/v1beta1/collector_webhook.go +++ b/apis/v1beta1/collector_webhook.go @@ -182,9 +182,6 @@ func (c CollectorWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj run if c.bv != nil { newWarnings := c.bv(*otelcol) - if err != nil { - return append(warnings, newWarnings...), err - } warnings = append(warnings, newWarnings...) } return warnings, nil diff --git a/apis/v1beta1/collector_webhook_test.go b/apis/v1beta1/collector_webhook_test.go index 65d4b5a468..930b4e5579 100644 --- a/apis/v1beta1/collector_webhook_test.go +++ b/apis/v1beta1/collector_webhook_test.go @@ -20,8 +20,6 @@ import ( "os" "testing" - collectorManifests "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" - "github.com/go-logr/logr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -42,6 +40,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" "github.com/open-telemetry/opentelemetry-operator/internal/config" "github.com/open-telemetry/opentelemetry-operator/internal/manifests" + collectorManifests "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" "github.com/open-telemetry/opentelemetry-operator/internal/rbac" ) From b594d972f71997360c42907083025edf4864ecc1 Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Tue, 30 Jul 2024 12:34:55 -0700 Subject: [PATCH 13/38] kubebuilder test --- main.go | 1 + 1 file changed, 1 insertion(+) diff --git a/main.go b/main.go index 0b5cb03520..3b158709e6 100644 --- a/main.go +++ b/main.go @@ -66,6 +66,7 @@ import ( instrumentationupgrade "github.com/open-telemetry/opentelemetry-operator/pkg/instrumentation/upgrade" "github.com/open-telemetry/opentelemetry-operator/pkg/sidecar" // +kubebuilder:scaffold:imports + // +kubebuilder:object:generate=false ) var ( From 2fe46302cecbf81ac431bfb974bd58f03d4c3ec9 Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Tue, 30 Jul 2024 12:51:51 -0700 Subject: [PATCH 14/38] kubebuilder testing --- apis/v1beta1/collector_webhook.go | 1 + config/manager/kustomization.yaml | 14 +++++++++++++- main.go | 1 - 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/apis/v1beta1/collector_webhook.go b/apis/v1beta1/collector_webhook.go index ee993f177c..f5c9aba646 100644 --- a/apis/v1beta1/collector_webhook.go +++ b/apis/v1beta1/collector_webhook.go @@ -447,6 +447,7 @@ func checkAutoscalerSpec(autoscaler *AutoscalerSpec) error { } // BuildValidator is mostly used for testing purposes. +// +kubebuilder:object:generate=false type BuildValidator func(c OpenTelemetryCollector) admission.Warnings func NewCollectorWebhook( diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index 372a75ae43..2cbc0cc747 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -1,3 +1,15 @@ resources: - manager.yaml - +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +images: +- name: controller + newName: ghcr.io/open-telemetry/opentelemetry-operator/opentelemetry-operator + newTag: e2e +patches: +- patch: '[{"op":"add","path":"/spec/template/spec/containers/0/args/-","value":"--target-allocator-image=ghcr.io/open-telemetry/opentelemetry-operator/target-allocator:ve2e"}]' + target: + kind: Deployment +- patch: '[{"op":"add","path":"/spec/template/spec/containers/0/args/-","value":"--operator-opamp-bridge-image=ghcr.io/open-telemetry/opentelemetry-operator/operator-opamp-bridge:ve2e"}]' + target: + kind: Deployment diff --git a/main.go b/main.go index 3b158709e6..0b5cb03520 100644 --- a/main.go +++ b/main.go @@ -66,7 +66,6 @@ import ( instrumentationupgrade "github.com/open-telemetry/opentelemetry-operator/pkg/instrumentation/upgrade" "github.com/open-telemetry/opentelemetry-operator/pkg/sidecar" // +kubebuilder:scaffold:imports - // +kubebuilder:object:generate=false ) var ( From 32bac14b202925db25b478e91b3d52d0954b69a9 Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Tue, 30 Jul 2024 13:04:05 -0700 Subject: [PATCH 15/38] fixing file --- config/manager/kustomization.yaml | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index 2cbc0cc747..5c5f0b84cb 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -1,15 +1,2 @@ resources: - manager.yaml -apiVersion: kustomize.config.k8s.io/v1beta1 -kind: Kustomization -images: -- name: controller - newName: ghcr.io/open-telemetry/opentelemetry-operator/opentelemetry-operator - newTag: e2e -patches: -- patch: '[{"op":"add","path":"/spec/template/spec/containers/0/args/-","value":"--target-allocator-image=ghcr.io/open-telemetry/opentelemetry-operator/target-allocator:ve2e"}]' - target: - kind: Deployment -- patch: '[{"op":"add","path":"/spec/template/spec/containers/0/args/-","value":"--operator-opamp-bridge-image=ghcr.io/open-telemetry/opentelemetry-operator/operator-opamp-bridge:ve2e"}]' - target: - kind: Deployment From 69c39d6ba1404905a9cc5368545b64740aea79ea Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Tue, 30 Jul 2024 13:13:23 -0700 Subject: [PATCH 16/38] unit test --- apis/v1alpha1/zz_generated.deepcopy.go | 7 +++---- apis/v1beta1/zz_generated.deepcopy.go | 4 ++-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/apis/v1alpha1/zz_generated.deepcopy.go b/apis/v1alpha1/zz_generated.deepcopy.go index 57a6717f44..87092d21f8 100644 --- a/apis/v1alpha1/zz_generated.deepcopy.go +++ b/apis/v1alpha1/zz_generated.deepcopy.go @@ -19,14 +19,13 @@ package v1alpha1 import ( - v2 "k8s.io/api/autoscaling/v2" - v1 "k8s.io/api/core/v1" + "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" + "k8s.io/api/autoscaling/v2" + "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" - - "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" ) // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. diff --git a/apis/v1beta1/zz_generated.deepcopy.go b/apis/v1beta1/zz_generated.deepcopy.go index 068e5f69de..d0334a8051 100644 --- a/apis/v1beta1/zz_generated.deepcopy.go +++ b/apis/v1beta1/zz_generated.deepcopy.go @@ -19,8 +19,8 @@ package v1beta1 import ( - v2 "k8s.io/api/autoscaling/v2" - v1 "k8s.io/api/core/v1" + "k8s.io/api/autoscaling/v2" + "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" From 895c802b9cf7194be101c207596cdbd51027cfe2 Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Wed, 7 Aug 2024 12:53:37 -0700 Subject: [PATCH 17/38] initial changes --- apis/v1beta1/collector_webhook.go | 15 ++++++-------- apis/v1beta1/collector_webhook_test.go | 18 ++++++++++------- .../opentelemetrycollector_controller.go | 20 +++++++++++++++++++ main.go | 19 +++--------------- 4 files changed, 40 insertions(+), 32 deletions(-) diff --git a/apis/v1beta1/collector_webhook.go b/apis/v1beta1/collector_webhook.go index a7bbfca5fa..abe5ff9f87 100644 --- a/apis/v1beta1/collector_webhook.go +++ b/apis/v1beta1/collector_webhook.go @@ -445,8 +445,10 @@ func checkAutoscalerSpec(autoscaler *AutoscalerSpec) error { return nil } -// BuildValidator is mostly used for testing purposes. +// BuildValidator is //purpose and description +// Kubebuilder is used for testing // +kubebuilder:object:generate=false + type BuildValidator func(c OpenTelemetryCollector) admission.Warnings func NewCollectorWebhook( @@ -454,6 +456,7 @@ func NewCollectorWebhook( scheme *runtime.Scheme, cfg config.Config, reviewer *rbac.Reviewer, + metrics *Metrics, bv BuildValidator, ) *CollectorWebhook { return &CollectorWebhook{ @@ -461,19 +464,13 @@ func NewCollectorWebhook( scheme: scheme, cfg: cfg, reviewer: reviewer, + metrics: metrics, bv: bv, } } func SetupCollectorWebhook(mgr ctrl.Manager, cfg config.Config, reviewer *rbac.Reviewer, metrics *Metrics, bv BuildValidator) error { - cvw := &CollectorWebhook{ - reviewer: reviewer, - logger: mgr.GetLogger().WithValues("handler", "CollectorWebhook", "version", "v1beta1"), - scheme: mgr.GetScheme(), - cfg: cfg, - metrics: metrics, - bv: bv, - } + cvw := NewCollectorWebhook(mgr.GetLogger().WithValues("handler", "CollectorWebhook", "version", "v1beta1"), mgr.GetScheme(), cfg, reviewer, metrics, bv) return ctrl.NewWebhookManagedBy(mgr). For(&OpenTelemetryCollector{}). WithValidator(cvw). diff --git a/apis/v1beta1/collector_webhook_test.go b/apis/v1beta1/collector_webhook_test.go index 930b4e5579..9e3b34268c 100644 --- a/apis/v1beta1/collector_webhook_test.go +++ b/apis/v1beta1/collector_webhook_test.go @@ -17,10 +17,16 @@ package v1beta1_test import ( "context" "fmt" + "github.com/open-telemetry/opentelemetry-operator/internal/manifests" + collectorManifests "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" "os" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "testing" "github.com/go-logr/logr" + "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" + "github.com/open-telemetry/opentelemetry-operator/internal/config" + "github.com/open-telemetry/opentelemetry-operator/internal/rbac" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gopkg.in/yaml.v3" @@ -35,13 +41,6 @@ import ( "k8s.io/client-go/kubernetes/fake" "k8s.io/client-go/kubernetes/scheme" kubeTesting "k8s.io/client-go/testing" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" - - "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" - "github.com/open-telemetry/opentelemetry-operator/internal/config" - "github.com/open-telemetry/opentelemetry-operator/internal/manifests" - collectorManifests "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" - "github.com/open-telemetry/opentelemetry-operator/internal/rbac" ) var ( @@ -127,6 +126,7 @@ func TestValidate(t *testing.T) { config.WithTargetAllocatorImage("ta:v0.0.0"), ), getReviewer(test.shouldFailSar), + nil, bv, ) t.Run(tt.name, func(t *testing.T) { @@ -587,6 +587,7 @@ func TestCollectorDefaultingWebhook(t *testing.T) { config.WithTargetAllocatorImage("ta:v0.0.0"), ), getReviewer(test.shouldFailSar), + nil, bv, ) ctx := context.Background() @@ -1354,6 +1355,7 @@ func TestOTELColValidatingWebhook(t *testing.T) { config.WithTargetAllocatorImage("ta:v0.0.0"), ), getReviewer(test.shouldFailSar), + nil, bv, ) ctx := context.Background() @@ -1389,6 +1391,7 @@ func TestOTELColValidateUpdateWebhook(t *testing.T) { expectedErr: "which does not support modification", }, } + bv := func(collector v1beta1.OpenTelemetryCollector) admission.Warnings { var warnings admission.Warnings cfg := config.New( @@ -1419,6 +1422,7 @@ func TestOTELColValidateUpdateWebhook(t *testing.T) { config.WithTargetAllocatorImage("ta:v0.0.0"), ), getReviewer(test.shouldFailSar), + nil, bv, ) ctx := context.Background() diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index 88a6f39ed9..6b73e43e13 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -18,6 +18,7 @@ package controllers import ( "context" "fmt" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "sort" "github.com/go-logr/logr" @@ -75,6 +76,25 @@ type Params struct { Config config.Config } +func (r *OpenTelemetryCollectorReconciler) Validate(otelcol v1beta1.OpenTelemetryCollector) admission.Warnings { + var warnings admission.Warnings + cfg := config.New( + config.WithCollectorImage("default-collector"), + config.WithTargetAllocatorImage("default-ta-allocator"), + ) + params := manifests.Params{ + Log: logr.Discard(), + Config: cfg, + OtelCol: otelcol, + } + _, err := collector.Build(params) + if err != nil { + warnings = append(warnings, err.Error()) + return warnings + } + return nil +} + func (r *OpenTelemetryCollectorReconciler) findOtelOwnedObjects(ctx context.Context, params manifests.Params) (map[types.UID]client.Object, error) { ownedObjects := map[types.UID]client.Object{} ownedObjectTypes := []client.Object{ diff --git a/main.go b/main.go index 0b5cb03520..94490133ba 100644 --- a/main.go +++ b/main.go @@ -54,7 +54,6 @@ import ( "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus" "github.com/open-telemetry/opentelemetry-operator/internal/config" - collectorManifests "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" openshiftDashboards "github.com/open-telemetry/opentelemetry-operator/internal/openshift/dashboards" "github.com/open-telemetry/opentelemetry-operator/internal/rbac" "github.com/open-telemetry/opentelemetry-operator/internal/version" @@ -407,22 +406,10 @@ func main() { } - bv := func(collector otelv1beta1.OpenTelemetryCollector) admission.Warnings { - var warnings admission.Warnings - params, newErr := collectorReconciler.GetParams(collector) - if newErr != nil { - warnings = append(warnings, newErr.Error()) - return warnings - } - _, err = collectorManifests.Build(params) - if err != nil { - warnings = append(warnings, err.Error()) - return warnings - } - return warnings - } + reconciler := controllers.NewReconciler(controllers.Params{}) + bv := otelv1beta1.SetupCollectorWebhook(mgr, cfg, reviewer, crdMetrics, reconciler.Validate) - if err = otelv1beta1.SetupCollectorWebhook(mgr, cfg, reviewer, crdMetrics, bv); err != nil { + if err = bv; err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "OpenTelemetryCollector") os.Exit(1) } From f620524ffda6f9eeb2f2f871497a07a952e1db0c Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Mon, 12 Aug 2024 15:11:09 -0700 Subject: [PATCH 18/38] clean up --- .chloggen/enhanced-webhook.yaml | 2 +- apis/v1beta1/collector_webhook.go | 2 +- controllers/opentelemetrycollector_controller.go | 4 ++-- main.go | 6 ++---- 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/.chloggen/enhanced-webhook.yaml b/.chloggen/enhanced-webhook.yaml index 6358c9c2ba..ddacb9ee1a 100644 --- a/.chloggen/enhanced-webhook.yaml +++ b/.chloggen/enhanced-webhook.yaml @@ -5,7 +5,7 @@ change_type: enhancement component: operator # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). -note: added reconciliation errors for webhook events +note: Added reconciliation errors for webhook events. The webhooks run the manifest generators to check for any errors. # One or more tracking issues related to the change issues: [2399] diff --git a/apis/v1beta1/collector_webhook.go b/apis/v1beta1/collector_webhook.go index abe5ff9f87..d71d97a507 100644 --- a/apis/v1beta1/collector_webhook.go +++ b/apis/v1beta1/collector_webhook.go @@ -445,7 +445,7 @@ func checkAutoscalerSpec(autoscaler *AutoscalerSpec) error { return nil } -// BuildValidator is //purpose and description +// BuildValidator enables running the manifest generators for the collector reconciler // Kubebuilder is used for testing // +kubebuilder:object:generate=false diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index 6b73e43e13..a8c83c426c 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -188,7 +188,7 @@ func (r *OpenTelemetryCollectorReconciler) getConfigMapsToRemove(configVersionsT return ownedConfigMaps } -func (r *OpenTelemetryCollectorReconciler) GetParams(instance v1beta1.OpenTelemetryCollector) (manifests.Params, error) { +func (r *OpenTelemetryCollectorReconciler) getParams(instance v1beta1.OpenTelemetryCollector) (manifests.Params, error) { p := manifests.Params{ Config: r.config, Client: r.Client, @@ -249,7 +249,7 @@ func (r *OpenTelemetryCollectorReconciler) Reconcile(ctx context.Context, req ct return ctrl.Result{}, client.IgnoreNotFound(err) } - params, err := r.GetParams(instance) + params, err := r.getParams(instance) if err != nil { log.Error(err, "Failed to create manifest.Params") return ctrl.Result{}, err diff --git a/main.go b/main.go index 94490133ba..d3bee2d871 100644 --- a/main.go +++ b/main.go @@ -366,15 +366,13 @@ func main() { os.Exit(1) } - collectorReconciler := controllers.NewReconciler(controllers.Params{ + if err = controllers.NewReconciler(controllers.Params{ Client: mgr.GetClient(), Log: ctrl.Log.WithName("controllers").WithName("OpenTelemetryCollector"), Scheme: mgr.GetScheme(), Config: cfg, Recorder: mgr.GetEventRecorderFor("opentelemetry-operator"), - }) - - if err = collectorReconciler.SetupWithManager(mgr); err != nil { + }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "OpenTelemetryCollector") os.Exit(1) } From 59f50c1eb59b7daca0a4c534890312af68c156dd Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Mon, 12 Aug 2024 15:19:24 -0700 Subject: [PATCH 19/38] merge conflicts --- apis/v1beta1/collector_webhook_test.go | 60 -------------------------- 1 file changed, 60 deletions(-) diff --git a/apis/v1beta1/collector_webhook_test.go b/apis/v1beta1/collector_webhook_test.go index 9e3b34268c..739ceb0d97 100644 --- a/apis/v1beta1/collector_webhook_test.go +++ b/apis/v1beta1/collector_webhook_test.go @@ -172,12 +172,6 @@ func TestCollectorDefaultingWebhook(t *testing.T) { Args: map[string]string{"feature-gates": "-component.UseLocalHostAsDefaultHost"}, ManagementState: v1beta1.ManagementStateManaged, Replicas: &one, - PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ - MaxUnavailable: &intstr.IntOrString{ - Type: intstr.Int, - IntVal: 1, - }, - }, }, Mode: v1beta1.ModeDeployment, UpgradeStrategy: v1beta1.UpgradeStrategyAutomatic, @@ -208,12 +202,6 @@ func TestCollectorDefaultingWebhook(t *testing.T) { Args: map[string]string{"feature-gates": "-component.UseLocalHostAsDefaultHost"}, Replicas: &five, ManagementState: v1beta1.ManagementStateManaged, - PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ - MaxUnavailable: &intstr.IntOrString{ - Type: intstr.Int, - IntVal: 1, - }, - }, }, }, }, @@ -243,12 +231,6 @@ func TestCollectorDefaultingWebhook(t *testing.T) { Args: map[string]string{"feature-gates": "-component.UseLocalHostAsDefaultHost"}, Replicas: &five, ManagementState: v1beta1.ManagementStateUnmanaged, - PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ - MaxUnavailable: &intstr.IntOrString{ - Type: intstr.Int, - IntVal: 1, - }, - }, }, }, }, @@ -276,12 +258,6 @@ func TestCollectorDefaultingWebhook(t *testing.T) { Args: map[string]string{"feature-gates": "-component.UseLocalHostAsDefaultHost"}, Replicas: &one, ManagementState: v1beta1.ManagementStateManaged, - PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ - MaxUnavailable: &intstr.IntOrString{ - Type: intstr.Int, - IntVal: 1, - }, - }, }, Autoscaler: &v1beta1.AutoscalerSpec{ TargetCPUUtilization: &defaultCPUTarget, @@ -313,12 +289,6 @@ func TestCollectorDefaultingWebhook(t *testing.T) { Args: map[string]string{"feature-gates": "-component.UseLocalHostAsDefaultHost"}, ManagementState: v1beta1.ManagementStateManaged, Replicas: &one, - PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ - MaxUnavailable: &intstr.IntOrString{ - Type: intstr.Int, - IntVal: 1, - }, - }, }, Ingress: v1beta1.Ingress{ Type: v1beta1.IngressTypeRoute, @@ -397,12 +367,6 @@ func TestCollectorDefaultingWebhook(t *testing.T) { Args: map[string]string{"feature-gates": "-component.UseLocalHostAsDefaultHost"}, Replicas: &one, ManagementState: v1beta1.ManagementStateManaged, - PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ - MaxUnavailable: &intstr.IntOrString{ - Type: intstr.Int, - IntVal: 1, - }, - }, }, UpgradeStrategy: v1beta1.UpgradeStrategyAutomatic, TargetAllocator: v1beta1.TargetAllocatorEmbedded{ @@ -448,12 +412,6 @@ func TestCollectorDefaultingWebhook(t *testing.T) { Args: map[string]string{"feature-gates": "-component.UseLocalHostAsDefaultHost"}, Replicas: &one, ManagementState: v1beta1.ManagementStateManaged, - PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ - MaxUnavailable: &intstr.IntOrString{ - Type: intstr.Int, - IntVal: 1, - }, - }, }, UpgradeStrategy: v1beta1.UpgradeStrategyAutomatic, TargetAllocator: v1beta1.TargetAllocatorEmbedded{ @@ -494,24 +452,12 @@ func TestCollectorDefaultingWebhook(t *testing.T) { Args: map[string]string{"feature-gates": "-component.UseLocalHostAsDefaultHost"}, Replicas: &one, ManagementState: v1beta1.ManagementStateManaged, - PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ - MaxUnavailable: &intstr.IntOrString{ - Type: intstr.Int, - IntVal: 1, - }, - }, }, UpgradeStrategy: v1beta1.UpgradeStrategyAutomatic, TargetAllocator: v1beta1.TargetAllocatorEmbedded{ Enabled: true, Replicas: &one, AllocationStrategy: v1beta1.TargetAllocatorAllocationStrategyConsistentHashing, - PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ - MaxUnavailable: &intstr.IntOrString{ - Type: intstr.Int, - IntVal: 1, - }, - }, }, }, }, @@ -539,12 +485,6 @@ func TestCollectorDefaultingWebhook(t *testing.T) { Args: map[string]string{"feature-gates": "-component.UseLocalHostAsDefaultHost"}, Replicas: &one, ManagementState: v1beta1.ManagementStateManaged, - PodDisruptionBudget: &v1beta1.PodDisruptionBudgetSpec{ - MaxUnavailable: &intstr.IntOrString{ - Type: intstr.Int, - IntVal: 1, - }, - }, }, UpgradeStrategy: v1beta1.UpgradeStrategyAutomatic, TargetAllocator: v1beta1.TargetAllocatorEmbedded{ From a93594c17bcd6cb95bb5af7341981cb056447307 Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Tue, 13 Aug 2024 09:24:59 -0700 Subject: [PATCH 20/38] Update apis/v1beta1/collector_webhook_test.go Co-authored-by: Israel Blancas --- apis/v1beta1/collector_webhook_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/apis/v1beta1/collector_webhook_test.go b/apis/v1beta1/collector_webhook_test.go index 739ceb0d97..a1f968363c 100644 --- a/apis/v1beta1/collector_webhook_test.go +++ b/apis/v1beta1/collector_webhook_test.go @@ -546,10 +546,8 @@ func TestOTELColValidatingWebhook(t *testing.T) { three := int32(3) five := int32(5) - if err := v1beta1.AddToScheme(testScheme); err != nil { - fmt.Printf("failed to register scheme: %v", err) - os.Exit(1) - } + v1beta1.AddToScheme(testScheme) + require.NoError(e, err) cfg := v1beta1.Config{} err := yaml.Unmarshal([]byte(cfgYaml), &cfg) From a1d71f2210fc9f201a233b0807627a1234c45419 Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Tue, 13 Aug 2024 09:36:57 -0700 Subject: [PATCH 21/38] fixed test --- apis/v1beta1/collector_webhook_test.go | 37 +++++++++++--------------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/apis/v1beta1/collector_webhook_test.go b/apis/v1beta1/collector_webhook_test.go index 739ceb0d97..8c8ebd42df 100644 --- a/apis/v1beta1/collector_webhook_test.go +++ b/apis/v1beta1/collector_webhook_test.go @@ -47,22 +47,6 @@ var ( testScheme = scheme.Scheme ) -var cfgYaml = `receivers: - examplereceiver: - endpoint: "0.0.0.0:12345" - examplereceiver/settings: - endpoint: "0.0.0.0:12346" - prometheus: - config: - scrape_configs: - - job_name: otel-collector - scrape_interval: 10s - jaeger/custom: - protocols: - thrift_http: - endpoint: 0.0.0.0:15268 -` - func TestValidate(t *testing.T) { tests := []struct { name string @@ -538,6 +522,22 @@ func TestCollectorDefaultingWebhook(t *testing.T) { } } +var cfgYaml = `receivers: + examplereceiver: + endpoint: "0.0.0.0:12345" + examplereceiver/settings: + endpoint: "0.0.0.0:12346" + prometheus: + config: + scrape_configs: + - job_name: otel-collector + scrape_interval: 10s + jaeger/custom: + protocols: + thrift_http: + endpoint: 0.0.0.0:15268 +` + func TestOTELColValidatingWebhook(t *testing.T) { minusOne := int32(-1) zero := int32(0) @@ -546,11 +546,6 @@ func TestOTELColValidatingWebhook(t *testing.T) { three := int32(3) five := int32(5) - if err := v1beta1.AddToScheme(testScheme); err != nil { - fmt.Printf("failed to register scheme: %v", err) - os.Exit(1) - } - cfg := v1beta1.Config{} err := yaml.Unmarshal([]byte(cfgYaml), &cfg) require.NoError(t, err) From 1f5442b2efc5ba60132b367094e6b627f11e69ba Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Tue, 13 Aug 2024 09:52:34 -0700 Subject: [PATCH 22/38] fixed test --- apis/v1beta1/collector_webhook_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/apis/v1beta1/collector_webhook_test.go b/apis/v1beta1/collector_webhook_test.go index 8c8ebd42df..9ffd67c69d 100644 --- a/apis/v1beta1/collector_webhook_test.go +++ b/apis/v1beta1/collector_webhook_test.go @@ -546,8 +546,11 @@ func TestOTELColValidatingWebhook(t *testing.T) { three := int32(3) five := int32(5) + err := v1beta1.AddToScheme(testScheme) + require.NoError(t, err) + cfg := v1beta1.Config{} - err := yaml.Unmarshal([]byte(cfgYaml), &cfg) + err = yaml.Unmarshal([]byte(cfgYaml), &cfg) require.NoError(t, err) tests := []struct { //nolint:govet From 33fbc036797b9750aad771e27b95df559a7b7925 Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Tue, 13 Aug 2024 10:40:59 -0700 Subject: [PATCH 23/38] unneeded code --- apis/v1beta1/collector_webhook_test.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/apis/v1beta1/collector_webhook_test.go b/apis/v1beta1/collector_webhook_test.go index 9ffd67c69d..8c8ebd42df 100644 --- a/apis/v1beta1/collector_webhook_test.go +++ b/apis/v1beta1/collector_webhook_test.go @@ -546,11 +546,8 @@ func TestOTELColValidatingWebhook(t *testing.T) { three := int32(3) five := int32(5) - err := v1beta1.AddToScheme(testScheme) - require.NoError(t, err) - cfg := v1beta1.Config{} - err = yaml.Unmarshal([]byte(cfgYaml), &cfg) + err := yaml.Unmarshal([]byte(cfgYaml), &cfg) require.NoError(t, err) tests := []struct { //nolint:govet From f8fb046055647e56d10247b45b2fdac773bf7174 Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Tue, 13 Aug 2024 11:09:42 -0700 Subject: [PATCH 24/38] linting --- apis/v1alpha1/zz_generated.deepcopy.go | 7 ++++--- apis/v1beta1/collector_webhook_test.go | 15 +++++++++------ apis/v1beta1/zz_generated.deepcopy.go | 4 ++-- controllers/opentelemetrycollector_controller.go | 3 ++- 4 files changed, 17 insertions(+), 12 deletions(-) diff --git a/apis/v1alpha1/zz_generated.deepcopy.go b/apis/v1alpha1/zz_generated.deepcopy.go index 87092d21f8..57a6717f44 100644 --- a/apis/v1alpha1/zz_generated.deepcopy.go +++ b/apis/v1alpha1/zz_generated.deepcopy.go @@ -19,13 +19,14 @@ package v1alpha1 import ( - "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" - "k8s.io/api/autoscaling/v2" - "k8s.io/api/core/v1" + v2 "k8s.io/api/autoscaling/v2" + v1 "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" + + "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" ) // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. diff --git a/apis/v1beta1/collector_webhook_test.go b/apis/v1beta1/collector_webhook_test.go index 8c8ebd42df..83196bcd2c 100644 --- a/apis/v1beta1/collector_webhook_test.go +++ b/apis/v1beta1/collector_webhook_test.go @@ -17,16 +17,15 @@ package v1beta1_test import ( "context" "fmt" - "github.com/open-telemetry/opentelemetry-operator/internal/manifests" - collectorManifests "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" "os" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "testing" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + "github.com/open-telemetry/opentelemetry-operator/internal/manifests" + collectorManifests "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" + "github.com/go-logr/logr" - "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" - "github.com/open-telemetry/opentelemetry-operator/internal/config" - "github.com/open-telemetry/opentelemetry-operator/internal/rbac" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gopkg.in/yaml.v3" @@ -41,6 +40,10 @@ import ( "k8s.io/client-go/kubernetes/fake" "k8s.io/client-go/kubernetes/scheme" kubeTesting "k8s.io/client-go/testing" + + "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" + "github.com/open-telemetry/opentelemetry-operator/internal/config" + "github.com/open-telemetry/opentelemetry-operator/internal/rbac" ) var ( diff --git a/apis/v1beta1/zz_generated.deepcopy.go b/apis/v1beta1/zz_generated.deepcopy.go index d0334a8051..068e5f69de 100644 --- a/apis/v1beta1/zz_generated.deepcopy.go +++ b/apis/v1beta1/zz_generated.deepcopy.go @@ -19,8 +19,8 @@ package v1beta1 import ( - "k8s.io/api/autoscaling/v2" - "k8s.io/api/core/v1" + v2 "k8s.io/api/autoscaling/v2" + v1 "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index a8c83c426c..c7f92b4b61 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -18,9 +18,10 @@ package controllers import ( "context" "fmt" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "sort" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + "github.com/go-logr/logr" routev1 "github.com/openshift/api/route/v1" monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" From 65477b55214a53495343e1d2a7e39c85b3c33290 Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Tue, 13 Aug 2024 12:50:24 -0700 Subject: [PATCH 25/38] linting and unit tests --- apis/v1alpha1/zz_generated.deepcopy.go | 7 +++---- apis/v1beta1/collector_webhook_test.go | 8 +++----- apis/v1beta1/zz_generated.deepcopy.go | 4 ++-- controllers/opentelemetrycollector_controller.go | 3 +-- 4 files changed, 9 insertions(+), 13 deletions(-) diff --git a/apis/v1alpha1/zz_generated.deepcopy.go b/apis/v1alpha1/zz_generated.deepcopy.go index 57a6717f44..87092d21f8 100644 --- a/apis/v1alpha1/zz_generated.deepcopy.go +++ b/apis/v1alpha1/zz_generated.deepcopy.go @@ -19,14 +19,13 @@ package v1alpha1 import ( - v2 "k8s.io/api/autoscaling/v2" - v1 "k8s.io/api/core/v1" + "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" + "k8s.io/api/autoscaling/v2" + "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" - - "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" ) // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. diff --git a/apis/v1beta1/collector_webhook_test.go b/apis/v1beta1/collector_webhook_test.go index 83196bcd2c..27865fe746 100644 --- a/apis/v1beta1/collector_webhook_test.go +++ b/apis/v1beta1/collector_webhook_test.go @@ -20,11 +20,6 @@ import ( "os" "testing" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" - - "github.com/open-telemetry/opentelemetry-operator/internal/manifests" - collectorManifests "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" - "github.com/go-logr/logr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -40,9 +35,12 @@ import ( "k8s.io/client-go/kubernetes/fake" "k8s.io/client-go/kubernetes/scheme" kubeTesting "k8s.io/client-go/testing" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" "github.com/open-telemetry/opentelemetry-operator/internal/config" + "github.com/open-telemetry/opentelemetry-operator/internal/manifests" + collectorManifests "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" "github.com/open-telemetry/opentelemetry-operator/internal/rbac" ) diff --git a/apis/v1beta1/zz_generated.deepcopy.go b/apis/v1beta1/zz_generated.deepcopy.go index 068e5f69de..d0334a8051 100644 --- a/apis/v1beta1/zz_generated.deepcopy.go +++ b/apis/v1beta1/zz_generated.deepcopy.go @@ -19,8 +19,8 @@ package v1beta1 import ( - v2 "k8s.io/api/autoscaling/v2" - v1 "k8s.io/api/core/v1" + "k8s.io/api/autoscaling/v2" + "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index c7f92b4b61..d30f2629a0 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -20,8 +20,6 @@ import ( "fmt" "sort" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" - "github.com/go-logr/logr" routev1 "github.com/openshift/api/route/v1" monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" @@ -39,6 +37,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" From 3fdc89f36eb91622c1625b08ede217b769ec13dc Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Thu, 15 Aug 2024 13:00:29 -0700 Subject: [PATCH 26/38] reuse code --- controllers/opentelemetrycollector_controller.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index d30f2629a0..680f4cb2cd 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -78,16 +78,14 @@ type Params struct { func (r *OpenTelemetryCollectorReconciler) Validate(otelcol v1beta1.OpenTelemetryCollector) admission.Warnings { var warnings admission.Warnings - cfg := config.New( - config.WithCollectorImage("default-collector"), - config.WithTargetAllocatorImage("default-ta-allocator"), - ) - params := manifests.Params{ - Log: logr.Discard(), - Config: cfg, - OtelCol: otelcol, + + params, err := r.getParams(otelcol) + if err != nil { + warnings = append(warnings, err.Error()) + return warnings } - _, err := collector.Build(params) + + _, err = collector.Build(params) if err != nil { warnings = append(warnings, err.Error()) return warnings From 8e7282c774eab7cc8cff341a2773853a4861da3c Mon Sep 17 00:00:00 2001 From: Tania Pham Date: Fri, 16 Aug 2024 10:01:56 -0700 Subject: [PATCH 27/38] Update apis/v1beta1/collector_webhook.go Co-authored-by: Jacob Aronoff --- apis/v1beta1/collector_webhook.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/apis/v1beta1/collector_webhook.go b/apis/v1beta1/collector_webhook.go index b47e0fb75d..989c60ed57 100644 --- a/apis/v1beta1/collector_webhook.go +++ b/apis/v1beta1/collector_webhook.go @@ -416,9 +416,7 @@ func checkAutoscalerSpec(autoscaler *AutoscalerSpec) error { } // BuildValidator enables running the manifest generators for the collector reconciler -// Kubebuilder is used for testing // +kubebuilder:object:generate=false - type BuildValidator func(c OpenTelemetryCollector) admission.Warnings func NewCollectorWebhook( From b6f0911cda24680d2f1ae521d469b01b4f4e13e6 Mon Sep 17 00:00:00 2001 From: "tania.pham" Date: Thu, 29 Aug 2024 12:50:09 -0700 Subject: [PATCH 28/38] revert to not use getParams --- controllers/opentelemetrycollector_controller.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index 680f4cb2cd..c875a4dccf 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -79,13 +79,16 @@ type Params struct { func (r *OpenTelemetryCollectorReconciler) Validate(otelcol v1beta1.OpenTelemetryCollector) admission.Warnings { var warnings admission.Warnings - params, err := r.getParams(otelcol) - if err != nil { - warnings = append(warnings, err.Error()) - return warnings + cfg := config.New( + config.WithCollectorImage("default-collector"), + config.WithTargetAllocatorImage("default-ta-allocator"), + ) + params := manifests.Params{ + Log: logr.Discard(), + Config: cfg, + OtelCol: otelcol, } - - _, err = collector.Build(params) + _, err := collector.Build(params) if err != nil { warnings = append(warnings, err.Error()) return warnings From 56e4402ef589ebdb52bfe0f242eddcb476346a4f Mon Sep 17 00:00:00 2001 From: "tania.pham" Date: Thu, 29 Aug 2024 13:06:11 -0700 Subject: [PATCH 29/38] use config --- controllers/opentelemetrycollector_controller.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index c875a4dccf..0b7050dade 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -78,14 +78,10 @@ type Params struct { func (r *OpenTelemetryCollectorReconciler) Validate(otelcol v1beta1.OpenTelemetryCollector) admission.Warnings { var warnings admission.Warnings - - cfg := config.New( - config.WithCollectorImage("default-collector"), - config.WithTargetAllocatorImage("default-ta-allocator"), - ) + params := manifests.Params{ Log: logr.Discard(), - Config: cfg, + Config: r.config, OtelCol: otelcol, } _, err := collector.Build(params) From df46a051adce579200293f0bd39249464eb93091 Mon Sep 17 00:00:00 2001 From: "tania.pham" Date: Thu, 29 Aug 2024 13:14:28 -0700 Subject: [PATCH 30/38] linting --- apis/v1alpha1/zz_generated.deepcopy.go | 7 ++++--- apis/v1beta1/zz_generated.deepcopy.go | 4 ++-- controllers/opentelemetrycollector_controller.go | 2 +- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/apis/v1alpha1/zz_generated.deepcopy.go b/apis/v1alpha1/zz_generated.deepcopy.go index 87092d21f8..57a6717f44 100644 --- a/apis/v1alpha1/zz_generated.deepcopy.go +++ b/apis/v1alpha1/zz_generated.deepcopy.go @@ -19,13 +19,14 @@ package v1alpha1 import ( - "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" - "k8s.io/api/autoscaling/v2" - "k8s.io/api/core/v1" + v2 "k8s.io/api/autoscaling/v2" + v1 "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" + + "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" ) // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. diff --git a/apis/v1beta1/zz_generated.deepcopy.go b/apis/v1beta1/zz_generated.deepcopy.go index d0334a8051..068e5f69de 100644 --- a/apis/v1beta1/zz_generated.deepcopy.go +++ b/apis/v1beta1/zz_generated.deepcopy.go @@ -19,8 +19,8 @@ package v1beta1 import ( - "k8s.io/api/autoscaling/v2" - "k8s.io/api/core/v1" + v2 "k8s.io/api/autoscaling/v2" + v1 "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index 0b7050dade..f94d1f40d5 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -78,7 +78,7 @@ type Params struct { func (r *OpenTelemetryCollectorReconciler) Validate(otelcol v1beta1.OpenTelemetryCollector) admission.Warnings { var warnings admission.Warnings - + params := manifests.Params{ Log: logr.Discard(), Config: r.config, From 4289b4481ab36cb4dd1cf169da66e9fe95abbde7 Mon Sep 17 00:00:00 2001 From: "tania.pham" Date: Thu, 29 Aug 2024 13:27:52 -0700 Subject: [PATCH 31/38] more linting --- apis/v1alpha1/zz_generated.deepcopy.go | 7 +++---- apis/v1beta1/zz_generated.deepcopy.go | 4 ++-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/apis/v1alpha1/zz_generated.deepcopy.go b/apis/v1alpha1/zz_generated.deepcopy.go index 57a6717f44..87092d21f8 100644 --- a/apis/v1alpha1/zz_generated.deepcopy.go +++ b/apis/v1alpha1/zz_generated.deepcopy.go @@ -19,14 +19,13 @@ package v1alpha1 import ( - v2 "k8s.io/api/autoscaling/v2" - v1 "k8s.io/api/core/v1" + "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" + "k8s.io/api/autoscaling/v2" + "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" - - "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" ) // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. diff --git a/apis/v1beta1/zz_generated.deepcopy.go b/apis/v1beta1/zz_generated.deepcopy.go index 068e5f69de..d0334a8051 100644 --- a/apis/v1beta1/zz_generated.deepcopy.go +++ b/apis/v1beta1/zz_generated.deepcopy.go @@ -19,8 +19,8 @@ package v1beta1 import ( - v2 "k8s.io/api/autoscaling/v2" - v1 "k8s.io/api/core/v1" + "k8s.io/api/autoscaling/v2" + "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" From 42f9a1fb1ad28e8a316152ab4b5ad89334a81f01 Mon Sep 17 00:00:00 2001 From: "tania.pham" Date: Wed, 4 Sep 2024 09:42:50 -0700 Subject: [PATCH 32/38] validate as anon func --- .../opentelemetrycollector_controller.go | 42 ++++++------------- main.go | 25 ++++++++--- 2 files changed, 32 insertions(+), 35 deletions(-) diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index f94d1f40d5..71c0676416 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -21,6 +21,16 @@ import ( "sort" "github.com/go-logr/logr" + "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac" + "github.com/open-telemetry/opentelemetry-operator/internal/config" + "github.com/open-telemetry/opentelemetry-operator/internal/manifests" + "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" + "github.com/open-telemetry/opentelemetry-operator/internal/manifests/manifestutils" + collectorStatus "github.com/open-telemetry/opentelemetry-operator/internal/status/collector" + "github.com/open-telemetry/opentelemetry-operator/pkg/featuregate" routev1 "github.com/openshift/api/route/v1" monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" appsv1 "k8s.io/api/apps/v1" @@ -37,18 +47,6 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "sigs.k8s.io/controller-runtime/pkg/webhook/admission" - - "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" - "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" - "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus" - "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac" - "github.com/open-telemetry/opentelemetry-operator/internal/config" - "github.com/open-telemetry/opentelemetry-operator/internal/manifests" - "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" - "github.com/open-telemetry/opentelemetry-operator/internal/manifests/manifestutils" - collectorStatus "github.com/open-telemetry/opentelemetry-operator/internal/status/collector" - "github.com/open-telemetry/opentelemetry-operator/pkg/featuregate" ) var ( @@ -76,22 +74,6 @@ type Params struct { Config config.Config } -func (r *OpenTelemetryCollectorReconciler) Validate(otelcol v1beta1.OpenTelemetryCollector) admission.Warnings { - var warnings admission.Warnings - - params := manifests.Params{ - Log: logr.Discard(), - Config: r.config, - OtelCol: otelcol, - } - _, err := collector.Build(params) - if err != nil { - warnings = append(warnings, err.Error()) - return warnings - } - return nil -} - func (r *OpenTelemetryCollectorReconciler) findOtelOwnedObjects(ctx context.Context, params manifests.Params) (map[types.UID]client.Object, error) { ownedObjects := map[types.UID]client.Object{} ownedObjectTypes := []client.Object{ @@ -185,7 +167,7 @@ func (r *OpenTelemetryCollectorReconciler) getConfigMapsToRemove(configVersionsT return ownedConfigMaps } -func (r *OpenTelemetryCollectorReconciler) getParams(instance v1beta1.OpenTelemetryCollector) (manifests.Params, error) { +func (r *OpenTelemetryCollectorReconciler) GetParams(instance v1beta1.OpenTelemetryCollector) (manifests.Params, error) { p := manifests.Params{ Config: r.config, Client: r.Client, @@ -246,7 +228,7 @@ func (r *OpenTelemetryCollectorReconciler) Reconcile(ctx context.Context, req ct return ctrl.Result{}, client.IgnoreNotFound(err) } - params, err := r.getParams(instance) + params, err := r.GetParams(instance) if err != nil { log.Error(err, "Failed to create manifest.Params") return ctrl.Result{}, err diff --git a/main.go b/main.go index 6d605a6c75..00ed5b6cbb 100644 --- a/main.go +++ b/main.go @@ -19,6 +19,7 @@ import ( "crypto/tls" "flag" "fmt" + collectorManifests "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" "os" "regexp" "runtime" @@ -366,13 +367,15 @@ func main() { os.Exit(1) } - if err = controllers.NewReconciler(controllers.Params{ + collectorReconciler := controllers.NewReconciler(controllers.Params{ Client: mgr.GetClient(), Log: ctrl.Log.WithName("controllers").WithName("OpenTelemetryCollector"), Scheme: mgr.GetScheme(), Config: cfg, Recorder: mgr.GetEventRecorderFor("opentelemetry-operator"), - }).SetupWithManager(mgr); err != nil { + }) + + if err = collectorReconciler.SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "OpenTelemetryCollector") os.Exit(1) } @@ -404,10 +407,22 @@ func main() { } - reconciler := controllers.NewReconciler(controllers.Params{}) - bv := otelv1beta1.SetupCollectorWebhook(mgr, cfg, reviewer, crdMetrics, reconciler.Validate) + bv := func(collector otelv1beta1.OpenTelemetryCollector) admission.Warnings { + var warnings admission.Warnings + params, err := collectorReconciler.GetParams(collector) + if err != nil { + warnings = append(warnings, err.Error()) + return warnings + } + _, err = collectorManifests.Build(params) + if err != nil { + warnings = append(warnings, err.Error()) + return warnings + } + return warnings + } - if err = bv; err != nil { + if err = otelv1beta1.SetupCollectorWebhook(mgr, cfg, reviewer, crdMetrics, bv); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "OpenTelemetryCollector") os.Exit(1) } From eab4fd19e3d704f7c6782335348d454bb2e717d9 Mon Sep 17 00:00:00 2001 From: "tania.pham" Date: Wed, 4 Sep 2024 13:10:39 -0700 Subject: [PATCH 33/38] linting --- apis/v1alpha1/zz_generated.deepcopy.go | 7 ++++--- apis/v1beta1/zz_generated.deepcopy.go | 4 ++-- .../opentelemetrycollector_controller.go | 21 ++++++++++--------- main.go | 13 ++++++------ 4 files changed, 24 insertions(+), 21 deletions(-) diff --git a/apis/v1alpha1/zz_generated.deepcopy.go b/apis/v1alpha1/zz_generated.deepcopy.go index 87092d21f8..57a6717f44 100644 --- a/apis/v1alpha1/zz_generated.deepcopy.go +++ b/apis/v1alpha1/zz_generated.deepcopy.go @@ -19,13 +19,14 @@ package v1alpha1 import ( - "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" - "k8s.io/api/autoscaling/v2" - "k8s.io/api/core/v1" + v2 "k8s.io/api/autoscaling/v2" + v1 "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" + + "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" ) // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. diff --git a/apis/v1beta1/zz_generated.deepcopy.go b/apis/v1beta1/zz_generated.deepcopy.go index d0334a8051..068e5f69de 100644 --- a/apis/v1beta1/zz_generated.deepcopy.go +++ b/apis/v1beta1/zz_generated.deepcopy.go @@ -19,8 +19,8 @@ package v1beta1 import ( - "k8s.io/api/autoscaling/v2" - "k8s.io/api/core/v1" + v2 "k8s.io/api/autoscaling/v2" + v1 "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index 71c0676416..88a6f39ed9 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -21,16 +21,6 @@ import ( "sort" "github.com/go-logr/logr" - "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" - "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" - "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus" - "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac" - "github.com/open-telemetry/opentelemetry-operator/internal/config" - "github.com/open-telemetry/opentelemetry-operator/internal/manifests" - "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" - "github.com/open-telemetry/opentelemetry-operator/internal/manifests/manifestutils" - collectorStatus "github.com/open-telemetry/opentelemetry-operator/internal/status/collector" - "github.com/open-telemetry/opentelemetry-operator/pkg/featuregate" routev1 "github.com/openshift/api/route/v1" monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" appsv1 "k8s.io/api/apps/v1" @@ -47,6 +37,17 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + + "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac" + "github.com/open-telemetry/opentelemetry-operator/internal/config" + "github.com/open-telemetry/opentelemetry-operator/internal/manifests" + "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" + "github.com/open-telemetry/opentelemetry-operator/internal/manifests/manifestutils" + collectorStatus "github.com/open-telemetry/opentelemetry-operator/internal/status/collector" + "github.com/open-telemetry/opentelemetry-operator/pkg/featuregate" ) var ( diff --git a/main.go b/main.go index 00ed5b6cbb..9b1b252018 100644 --- a/main.go +++ b/main.go @@ -19,13 +19,14 @@ import ( "crypto/tls" "flag" "fmt" - collectorManifests "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" "os" "regexp" "runtime" "strings" "time" + collectorManifests "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" + routev1 "github.com/openshift/api/route/v1" monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" "github.com/spf13/pflag" @@ -409,14 +410,14 @@ func main() { bv := func(collector otelv1beta1.OpenTelemetryCollector) admission.Warnings { var warnings admission.Warnings - params, err := collectorReconciler.GetParams(collector) + params, newErr := collectorReconciler.GetParams(collector) if err != nil { - warnings = append(warnings, err.Error()) + warnings = append(warnings, newErr.Error()) return warnings } - _, err = collectorManifests.Build(params) - if err != nil { - warnings = append(warnings, err.Error()) + _, newErr = collectorManifests.Build(params) + if newErr != nil { + warnings = append(warnings, newErr.Error()) return warnings } return warnings From a4f19e2865a591610c40f6255a04f1efb747e384 Mon Sep 17 00:00:00 2001 From: "tania.pham" Date: Wed, 4 Sep 2024 14:39:09 -0700 Subject: [PATCH 34/38] linting --- main.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/main.go b/main.go index 9b1b252018..64bd55d7af 100644 --- a/main.go +++ b/main.go @@ -25,8 +25,6 @@ import ( "strings" "time" - collectorManifests "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" - routev1 "github.com/openshift/api/route/v1" monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" "github.com/spf13/pflag" @@ -56,6 +54,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus" "github.com/open-telemetry/opentelemetry-operator/internal/config" + collectorManifests "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" openshiftDashboards "github.com/open-telemetry/opentelemetry-operator/internal/openshift/dashboards" "github.com/open-telemetry/opentelemetry-operator/internal/rbac" "github.com/open-telemetry/opentelemetry-operator/internal/version" From 68d16861e3b1b9328a8fe3eeff18693706e909f7 Mon Sep 17 00:00:00 2001 From: "tania.pham" Date: Wed, 4 Sep 2024 15:33:11 -0700 Subject: [PATCH 35/38] testing without test --- apis/v1beta1/collector_webhook_test.go | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/apis/v1beta1/collector_webhook_test.go b/apis/v1beta1/collector_webhook_test.go index 27865fe746..b5ce0e8311 100644 --- a/apis/v1beta1/collector_webhook_test.go +++ b/apis/v1beta1/collector_webhook_test.go @@ -920,18 +920,6 @@ func TestOTELColValidatingWebhook(t *testing.T) { }, expectedErr: "targetCPUUtilization should be greater than 0 and less than 100", }, - { - name: "autoscaler minReplicas is less than maxReplicas", - otelcol: v1beta1.OpenTelemetryCollector{ - Spec: v1beta1.OpenTelemetryCollectorSpec{ - Autoscaler: &v1beta1.AutoscalerSpec{ - MaxReplicas: &one, - MinReplicas: &five, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, minReplicas must not be greater than maxReplicas", - }, { name: "invalid autoscaler metric type", otelcol: v1beta1.OpenTelemetryCollector{ From f7f571b6e973bead7fd6599e86adbced3a11d528 Mon Sep 17 00:00:00 2001 From: "tania.pham" Date: Wed, 4 Sep 2024 15:40:27 -0700 Subject: [PATCH 36/38] added back test --- apis/v1beta1/collector_webhook_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/apis/v1beta1/collector_webhook_test.go b/apis/v1beta1/collector_webhook_test.go index b5ce0e8311..27865fe746 100644 --- a/apis/v1beta1/collector_webhook_test.go +++ b/apis/v1beta1/collector_webhook_test.go @@ -920,6 +920,18 @@ func TestOTELColValidatingWebhook(t *testing.T) { }, expectedErr: "targetCPUUtilization should be greater than 0 and less than 100", }, + { + name: "autoscaler minReplicas is less than maxReplicas", + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Autoscaler: &v1beta1.AutoscalerSpec{ + MaxReplicas: &one, + MinReplicas: &five, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, minReplicas must not be greater than maxReplicas", + }, { name: "invalid autoscaler metric type", otelcol: v1beta1.OpenTelemetryCollector{ From 616726e73af6fd75a29ad18037997e73814c09ac Mon Sep 17 00:00:00 2001 From: Jacob Aronoff Date: Thu, 5 Sep 2024 11:20:56 -0400 Subject: [PATCH 37/38] fix --- apis/v1beta1/collector_webhook_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/apis/v1beta1/collector_webhook_test.go b/apis/v1beta1/collector_webhook_test.go index 4126996bef..64ffff48ea 100644 --- a/apis/v1beta1/collector_webhook_test.go +++ b/apis/v1beta1/collector_webhook_test.go @@ -902,9 +902,9 @@ func TestOTELColValidatingWebhook(t *testing.T) { }, { name: "invalid autoscaler target memory utilization", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - Autoscaler: &AutoscalerSpec{ + otelcol: v1beta1.OpenTelemetryCollector{ + Spec: v1beta1.OpenTelemetryCollectorSpec{ + Autoscaler: &v1beta1.AutoscalerSpec{ MaxReplicas: &three, TargetMemoryUtilization: &zero, }, From c552bd0446406df691e6401a8865b888a9260b7f Mon Sep 17 00:00:00 2001 From: "tania.pham" Date: Thu, 5 Sep 2024 10:38:43 -0700 Subject: [PATCH 38/38] make generate --- apis/v1alpha1/zz_generated.deepcopy.go | 7 +++---- apis/v1beta1/zz_generated.deepcopy.go | 4 ++-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/apis/v1alpha1/zz_generated.deepcopy.go b/apis/v1alpha1/zz_generated.deepcopy.go index 57a6717f44..87092d21f8 100644 --- a/apis/v1alpha1/zz_generated.deepcopy.go +++ b/apis/v1alpha1/zz_generated.deepcopy.go @@ -19,14 +19,13 @@ package v1alpha1 import ( - v2 "k8s.io/api/autoscaling/v2" - v1 "k8s.io/api/core/v1" + "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" + "k8s.io/api/autoscaling/v2" + "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" - - "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" ) // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. diff --git a/apis/v1beta1/zz_generated.deepcopy.go b/apis/v1beta1/zz_generated.deepcopy.go index 068e5f69de..d0334a8051 100644 --- a/apis/v1beta1/zz_generated.deepcopy.go +++ b/apis/v1beta1/zz_generated.deepcopy.go @@ -19,8 +19,8 @@ package v1beta1 import ( - v2 "k8s.io/api/autoscaling/v2" - v1 "k8s.io/api/core/v1" + "k8s.io/api/autoscaling/v2" + "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime"