From ed81ddc6fe1e0a283d8cd67f4f561e41d3e98030 Mon Sep 17 00:00:00 2001 From: Kristina Pathak Date: Mon, 26 Sep 2022 19:51:27 -0700 Subject: [PATCH] add, fix tests --- .../opentelemetrycollector_webhook.go | 8 +- .../opentelemetrycollector_webhook_test.go | 254 ++++++++++++++++++ pkg/collector/container_test.go | 43 --- 3 files changed, 259 insertions(+), 46 deletions(-) diff --git a/apis/v1alpha1/opentelemetrycollector_webhook.go b/apis/v1alpha1/opentelemetrycollector_webhook.go index 12b6d98904..8e7bd7c802 100644 --- a/apis/v1alpha1/opentelemetrycollector_webhook.go +++ b/apis/v1alpha1/opentelemetrycollector_webhook.go @@ -133,7 +133,7 @@ func (r *OpenTelemetryCollector) validateCRDSpec() error { // validate autoscale with horizontal pod autoscaler if r.Spec.MaxReplicas != nil { - if *r.Spec.MaxReplicas < int32(1) { + if *r.Spec.MaxReplicas <= int32(1) { return fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, maxReplicas should be defined and more than one") } @@ -150,11 +150,13 @@ func (r *OpenTelemetryCollector) validateCRDSpec() error { } if r.Spec.Autoscaler != nil && r.Spec.Autoscaler.Behavior != nil { - if r.Spec.Autoscaler.Behavior.ScaleDown != nil && *r.Spec.Autoscaler.Behavior.ScaleDown.StabilizationWindowSeconds < int32(1) { + if r.Spec.Autoscaler.Behavior.ScaleDown != nil && r.Spec.Autoscaler.Behavior.ScaleDown.StabilizationWindowSeconds != nil && + *r.Spec.Autoscaler.Behavior.ScaleDown.StabilizationWindowSeconds < int32(1) { return fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, scaleDown should be one or more") } - if r.Spec.Autoscaler.Behavior.ScaleUp != nil && *r.Spec.Autoscaler.Behavior.ScaleUp.StabilizationWindowSeconds < int32(1) { + if r.Spec.Autoscaler.Behavior.ScaleUp != nil && r.Spec.Autoscaler.Behavior.ScaleUp.StabilizationWindowSeconds != nil && + *r.Spec.Autoscaler.Behavior.ScaleUp.StabilizationWindowSeconds < int32(1) { return fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, scaleUp should be one or more") } } diff --git a/apis/v1alpha1/opentelemetrycollector_webhook_test.go b/apis/v1alpha1/opentelemetrycollector_webhook_test.go index 39dc709551..a0d5b12bf4 100644 --- a/apis/v1alpha1/opentelemetrycollector_webhook_test.go +++ b/apis/v1alpha1/opentelemetrycollector_webhook_test.go @@ -18,6 +18,8 @@ import ( "testing" "github.com/stretchr/testify/assert" + autoscalingv2 "k8s.io/api/autoscaling/v2" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -76,3 +78,255 @@ func TestOTELColDefaultingWebhook(t *testing.T) { }) } } + +func TestOTELColValidatingWebhook(t *testing.T) { + zero := int32(0) + one := int32(1) + three := int32(3) + five := int32(5) + + tests := []struct { + name string + otelcol OpenTelemetryCollector + expectedErr string + }{ + { + name: "valid empty spec", + otelcol: OpenTelemetryCollector{}, + }, + { + name: "valid full spec", + otelcol: OpenTelemetryCollector{ + Spec: OpenTelemetryCollectorSpec{ + Mode: ModeStatefulSet, + MinReplicas: &one, + Replicas: &three, + MaxReplicas: &five, + UpgradeStrategy: "adhoc", + TargetAllocator: OpenTelemetryTargetAllocator{ + Enabled: true, + }, + Config: `receivers: + examplereceiver: + endpoint: "0.0.0.0:12345" + examplereceiver/settings: + endpoint: "0.0.0.0:12346" + prometheus: + config: + scrape_config: + job_name: otel-collector + scrape_interval: 10s + jaeger/custom: + protocols: + thrift_http: + endpoint: 0.0.0.0:15268 +`, + Ports: []v1.ServicePort{ + { + Name: "port1", + Port: 5555, + }, + { + Name: "port2", + Port: 5554, + Protocol: v1.ProtocolUDP, + }, + }, + Autoscaler: &AutoscalerSpec{ + Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{ + ScaleDown: &autoscalingv2.HPAScalingRules{ + StabilizationWindowSeconds: &three, + }, + ScaleUp: &autoscalingv2.HPAScalingRules{ + StabilizationWindowSeconds: &five, + }, + }, + TargetCPUUtilization: &five, + }, + }, + }, + }, + { + name: "invalid mode with volume claim templates", + otelcol: OpenTelemetryCollector{ + Spec: OpenTelemetryCollectorSpec{ + Mode: ModeSidecar, + VolumeClaimTemplates: []v1.PersistentVolumeClaim{{}, {}}, + }, + }, + expectedErr: "does not support the attribute 'volumeClaimTemplates'", + }, + { + name: "invalid mode with tolerations", + otelcol: OpenTelemetryCollector{ + Spec: OpenTelemetryCollectorSpec{ + Mode: ModeSidecar, + Tolerations: []v1.Toleration{{}, {}}, + }, + }, + expectedErr: "does not support the attribute 'tolerations'", + }, + { + name: "invalid mode with target allocator", + otelcol: OpenTelemetryCollector{ + Spec: OpenTelemetryCollectorSpec{ + Mode: ModeDeployment, + TargetAllocator: OpenTelemetryTargetAllocator{ + Enabled: true, + }, + }, + }, + expectedErr: "does not support the target allocation deployment", + }, + { + name: "invalid target allocator config", + otelcol: OpenTelemetryCollector{ + Spec: OpenTelemetryCollectorSpec{ + Mode: ModeStatefulSet, + TargetAllocator: OpenTelemetryTargetAllocator{ + Enabled: true, + }, + }, + }, + expectedErr: "the OpenTelemetry Spec Prometheus configuration is incorrect", + }, + { + name: "invalid port name", + otelcol: OpenTelemetryCollector{ + Spec: OpenTelemetryCollectorSpec{ + Ports: []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{ + Ports: []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{ + Ports: []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{ + MaxReplicas: &one, + }, + }, + expectedErr: "maxReplicas should be defined and more than one", + }, + { + name: "invalid replicas, greater than max", + otelcol: OpenTelemetryCollector{ + Spec: OpenTelemetryCollectorSpec{ + MaxReplicas: &three, + Replicas: &five, + }, + }, + expectedErr: "replicas must not be greater than maxReplicas", + }, + { + name: "invalid min replicas, greater than max", + otelcol: OpenTelemetryCollector{ + Spec: OpenTelemetryCollectorSpec{ + MaxReplicas: &three, + MinReplicas: &five, + }, + }, + expectedErr: "minReplicas must not be greater than maxReplicas", + }, + { + name: "invalid min replicas, lesser than 1", + otelcol: OpenTelemetryCollector{ + Spec: OpenTelemetryCollectorSpec{ + MaxReplicas: &three, + MinReplicas: &zero, + }, + }, + expectedErr: "minReplicas should be one or more", + }, + { + name: "invalid autoscaler scale down", + otelcol: OpenTelemetryCollector{ + Spec: OpenTelemetryCollectorSpec{ + MaxReplicas: &three, + Autoscaler: &AutoscalerSpec{ + Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{ + ScaleDown: &autoscalingv2.HPAScalingRules{ + StabilizationWindowSeconds: &zero, + }, + }, + }, + }, + }, + expectedErr: "scaleDown should be one or more", + }, + { + name: "invalid autoscaler scale up", + otelcol: OpenTelemetryCollector{ + Spec: OpenTelemetryCollectorSpec{ + MaxReplicas: &three, + Autoscaler: &AutoscalerSpec{ + 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{ + MaxReplicas: &three, + Autoscaler: &AutoscalerSpec{ + TargetCPUUtilization: &zero, + }, + }, + }, + expectedErr: "targetCPUUtilization should be greater than 0 and less than 100", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + err := test.otelcol.validateCRDSpec() + if test.expectedErr == "" { + assert.NoError(t, err) + return + } + assert.ErrorContains(t, err, test.expectedErr) + }) + } +} diff --git a/pkg/collector/container_test.go b/pkg/collector/container_test.go index 4f95a9012e..a0d7155e59 100644 --- a/pkg/collector/container_test.go +++ b/pkg/collector/container_test.go @@ -152,49 +152,6 @@ service: }, }, }, - { - description: "invalid port name", - specConfig: goodConfig, - specPorts: []corev1.ServicePort{ - { - // this port name contains a non alphanumeric character, which is invalid. - Name: "-testšŸ¦„port", - Port: 12345, - Protocol: corev1.ProtocolTCP, - }, - }, - expectedPorts: []corev1.ContainerPort{ - { - Name: "examplereceiver", - ContainerPort: 12345, - }, - metricContainerPort, - }, - }, - { - description: "long port name", - specConfig: goodConfig, - specPorts: []corev1.ServicePort{ - { - // this port name is longer than 15 characters, which is invalid. - Name: "testportaaaabbbb", - Port: 5, - Protocol: corev1.ProtocolTCP, - }, - }, - expectedPorts: []corev1.ContainerPort{ - { - Name: "examplereceiver", - ContainerPort: 12345, - }, - metricContainerPort, - { - Name: "testportaaaabbb", - ContainerPort: 5, - Protocol: corev1.ProtocolTCP, - }, - }, - }, { description: "duplicate port name", specConfig: goodConfig,