Skip to content

Commit

Permalink
fix(collector-webhook): ensure stabilizationWindowSeconds validatio…
Browse files Browse the repository at this point in the history
…n matches `k8s.io/api/autoscaling/v2` requirements (#3346)

* fix(collector-webhook): ensure `stabilizationWindowSeconds` validation matches `k8s.io/api/autoscaling/v2` requirements

* chore: add changelog for fix
  • Loading branch information
onematchfox authored Oct 11, 2024
1 parent 3147097 commit 7a79233
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 10 deletions.
18 changes: 18 additions & 0 deletions .chloggen/fix_validation-stabilizationWindowSeconds.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: bug_fix

# The name of the component, or a single word describing the area of concern, (e.g. collector, target allocator, auto-instrumentation, opamp, github action)
component: collector-webhook

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: "Fixed validation of `stabilizationWindowSeconds` in autoscaler behaviour"

# One or more tracking issues related to the change
issues: [3345]

# (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: |
The validation of `stabilizationWindowSeconds` in the `autoscaler.behaviour.scale[Up|Down]` incorrectly rejected 0 as an invalid value.
This has been fixed to ensure that the value is validated correctly (should be >=0 and <=3600) and the error messsage has been updated to reflect this.
8 changes: 4 additions & 4 deletions apis/v1beta1/collector_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -385,13 +385,13 @@ func ValidatePorts(ports []PortsSpec) error {
func checkAutoscalerSpec(autoscaler *AutoscalerSpec) error {
if autoscaler.Behavior != nil {
if autoscaler.Behavior.ScaleDown != nil && autoscaler.Behavior.ScaleDown.StabilizationWindowSeconds != nil &&
*autoscaler.Behavior.ScaleDown.StabilizationWindowSeconds < int32(1) {
return fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, scaleDown should be one or more")
(*autoscaler.Behavior.ScaleDown.StabilizationWindowSeconds < int32(0) || *autoscaler.Behavior.ScaleDown.StabilizationWindowSeconds > 3600) {
return fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, scaleDown.stabilizationWindowSeconds should be >=0 and <=3600")
}

if autoscaler.Behavior.ScaleUp != nil && autoscaler.Behavior.ScaleUp.StabilizationWindowSeconds != nil &&
*autoscaler.Behavior.ScaleUp.StabilizationWindowSeconds < int32(1) {
return fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, scaleUp should be one or more")
(*autoscaler.Behavior.ScaleUp.StabilizationWindowSeconds < int32(0) || *autoscaler.Behavior.ScaleUp.StabilizationWindowSeconds > 3600) {
return fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, scaleUp.stabilizationWindowSeconds should be >=0 and <=3600")
}
}
if autoscaler.TargetCPUUtilization != nil && *autoscaler.TargetCPUUtilization < int32(1) {
Expand Down
46 changes: 40 additions & 6 deletions apis/v1beta1/collector_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package v1beta1_test
import (
"context"
"fmt"
"math"
"os"
"testing"

Expand Down Expand Up @@ -582,6 +583,7 @@ func TestOTELColValidatingWebhook(t *testing.T) {
one := int32(1)
three := int32(3)
five := int32(5)
maxInt := int32(math.MaxInt32)

cfg := v1beta1.Config{}
err := yaml.Unmarshal([]byte(cfgYaml), &cfg)
Expand Down Expand Up @@ -913,36 +915,68 @@ func TestOTELColValidatingWebhook(t *testing.T) {
expectedErr: "minReplicas should be one or more",
},
{
name: "invalid autoscaler scale down",
name: "invalid autoscaler scale down stablization window - <0",
otelcol: v1beta1.OpenTelemetryCollector{
Spec: v1beta1.OpenTelemetryCollectorSpec{
Autoscaler: &v1beta1.AutoscalerSpec{
MaxReplicas: &three,
Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{
ScaleDown: &autoscalingv2.HPAScalingRules{
StabilizationWindowSeconds: &zero,
StabilizationWindowSeconds: &minusOne,
},
},
},
},
},
expectedErr: "scaleDown should be one or more",
expectedErr: "scaleDown.stabilizationWindowSeconds should be >=0 and <=3600",
},
{
name: "invalid autoscaler scale up",
name: "invalid autoscaler scale down stablization window - >3600",
otelcol: v1beta1.OpenTelemetryCollector{
Spec: v1beta1.OpenTelemetryCollectorSpec{
Autoscaler: &v1beta1.AutoscalerSpec{
MaxReplicas: &three,
Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{
ScaleDown: &autoscalingv2.HPAScalingRules{
StabilizationWindowSeconds: &maxInt,
},
},
},
},
},
expectedErr: "scaleDown.stabilizationWindowSeconds should be >=0 and <=3600",
},
{
name: "invalid autoscaler scale up stablization window - <0",
otelcol: v1beta1.OpenTelemetryCollector{
Spec: v1beta1.OpenTelemetryCollectorSpec{
Autoscaler: &v1beta1.AutoscalerSpec{
MaxReplicas: &three,
Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{
ScaleUp: &autoscalingv2.HPAScalingRules{
StabilizationWindowSeconds: &minusOne,
},
},
},
},
},
expectedErr: "scaleUp.stabilizationWindowSeconds should be >=0 and <=3600",
},
{
name: "invalid autoscaler scale up stablization window - >3600",
otelcol: v1beta1.OpenTelemetryCollector{
Spec: v1beta1.OpenTelemetryCollectorSpec{
Autoscaler: &v1beta1.AutoscalerSpec{
MaxReplicas: &three,
Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{
ScaleUp: &autoscalingv2.HPAScalingRules{
StabilizationWindowSeconds: &zero,
StabilizationWindowSeconds: &maxInt,
},
},
},
},
},
expectedErr: "scaleUp should be one or more",
expectedErr: "scaleUp.stabilizationWindowSeconds should be >=0 and <=3600",
},
{
name: "invalid autoscaler target cpu utilization",
Expand Down

0 comments on commit 7a79233

Please sign in to comment.