From df431acc4be58212279e729c5be0f6c9bad44a57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?= Date: Sun, 24 Nov 2024 18:49:16 +0100 Subject: [PATCH] [chore] Fix featuregate usage in controller tests We haven't been unsetting feature gates in controller tests after ending the test, leading to them being enabled for the duration of the test suite. In one case, a test actually depended on this fact, and I needed to set the gate in it explicitly. Also switched to use the gates explicitly vs parsing flags. --- controllers/builder_test.go | 68 +++++++++++++++++++++-------------- controllers/reconcile_test.go | 14 ++++++++ 2 files changed, 55 insertions(+), 27 deletions(-) diff --git a/controllers/builder_test.go b/controllers/builder_test.go index 6a5f4803c1..793bc217e2 100644 --- a/controllers/builder_test.go +++ b/controllers/builder_test.go @@ -15,7 +15,6 @@ package controllers import ( - "strings" "testing" cmv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" @@ -1245,7 +1244,7 @@ service: name string args args want []client.Object - featuregates []string + featuregates []*colfeaturegate.Gate wantErr bool opts []config.Option }{ @@ -2188,8 +2187,7 @@ prometheus_cr: }, }, }, - wantErr: false, - featuregates: []string{}, + wantErr: false, }, { name: "target allocator mtls enabled", @@ -2827,7 +2825,7 @@ prometheus_cr: opts: []config.Option{ config.WithCertManagerAvailability(certmanager.Available), }, - featuregates: []string{"operator.targetallocator.mtls"}, + featuregates: []*colfeaturegate.Gate{featuregate.EnableTargetAllocatorMTLS}, }, } for _, tt := range tests { @@ -2848,13 +2846,18 @@ prometheus_cr: targetAllocator, err := collector.TargetAllocator(params) require.NoError(t, err) params.TargetAllocator = targetAllocator - if len(tt.featuregates) > 0 { - fg := strings.Join(tt.featuregates, ",") - flagset := featuregate.Flags(colfeaturegate.GlobalRegistry()) - if err = flagset.Set(featuregate.FeatureGatesFlag, fg); err != nil { - t.Errorf("featuregate setting error = %v", err) + registry := colfeaturegate.GlobalRegistry() + for _, gate := range tt.featuregates { + current := gate.IsEnabled() + require.False(t, current, "only enable gates which are disabled by default") + if setErr := registry.Set(gate.ID(), true); setErr != nil { + require.NoError(t, setErr) return } + t.Cleanup(func() { + setErr := registry.Set(gate.ID(), current) + require.NoError(t, setErr) + }) } got, err := BuildCollector(params) if (err != nil) != tt.wantErr { @@ -2909,7 +2912,7 @@ service: name string args args want []client.Object - featuregates []string + featuregates []*colfeaturegate.Gate wantErr bool opts []config.Option }{ @@ -3396,7 +3399,7 @@ service: }, }, wantErr: false, - featuregates: []string{}, + featuregates: []*colfeaturegate.Gate{}, }, } for _, tt := range tests { @@ -3417,13 +3420,20 @@ service: targetAllocator, err := collector.TargetAllocator(params) require.NoError(t, err) params.TargetAllocator = targetAllocator - featuregates := []string{"operator.collector.targetallocatorcr"} + featuregates := []*colfeaturegate.Gate{featuregate.CollectorUsesTargetAllocatorCR} featuregates = append(featuregates, tt.featuregates...) - fg := strings.Join(featuregates, ",") - flagset := featuregate.Flags(colfeaturegate.GlobalRegistry()) - if err = flagset.Set(featuregate.FeatureGatesFlag, fg); err != nil { - t.Errorf("featuregate setting error = %v", err) - return + registry := colfeaturegate.GlobalRegistry() + for _, gate := range featuregates { + current := gate.IsEnabled() + require.False(t, current, "only enable gates which are disabled by default") + if setErr := registry.Set(gate.ID(), true); setErr != nil { + require.NoError(t, setErr) + return + } + t.Cleanup(func() { + setErr := registry.Set(gate.ID(), current) + require.NoError(t, setErr) + }) } got, err := BuildCollector(params) if (err != nil) != tt.wantErr { @@ -3445,7 +3455,7 @@ func TestBuildTargetAllocator(t *testing.T) { name string args args want []client.Object - featuregates []string + featuregates []*colfeaturegate.Gate wantErr bool opts []config.Option }{ @@ -4019,8 +4029,7 @@ prometheus_cr: }, }, }, - wantErr: false, - featuregates: []string{}, + wantErr: false, }, { name: "collector present", @@ -4776,7 +4785,7 @@ prometheus_cr: opts: []config.Option{ config.WithCertManagerAvailability(certmanager.Available), }, - featuregates: []string{"operator.targetallocator.mtls"}, + featuregates: []*colfeaturegate.Gate{featuregate.EnableTargetAllocatorMTLS}, }, } for _, tt := range tests { @@ -4795,13 +4804,18 @@ prometheus_cr: TargetAllocator: tt.args.instance, Collector: tt.args.collector, } - if len(tt.featuregates) > 0 { - fg := strings.Join(tt.featuregates, ",") - flagset := featuregate.Flags(colfeaturegate.GlobalRegistry()) - if err := flagset.Set(featuregate.FeatureGatesFlag, fg); err != nil { - t.Errorf("featuregate setting error = %v", err) + registry := colfeaturegate.GlobalRegistry() + for _, gate := range tt.featuregates { + current := gate.IsEnabled() + require.False(t, current, "only enable gates which are disabled by default") + if err := registry.Set(gate.ID(), true); err != nil { + require.NoError(t, err) return } + t.Cleanup(func() { + err := registry.Set(gate.ID(), current) + require.NoError(t, err) + }) } got, err := BuildTargetAllocator(params) if (err != nil) != tt.wantErr { diff --git a/controllers/reconcile_test.go b/controllers/reconcile_test.go index 703489b5d4..a0d6fc3bed 100644 --- a/controllers/reconcile_test.go +++ b/controllers/reconcile_test.go @@ -22,6 +22,7 @@ import ( routev1 "github.com/openshift/api/route/v1" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + colfeaturegate "go.opentelemetry.io/collector/featuregate" appsv1 "k8s.io/api/apps/v1" autoscalingv2 "k8s.io/api/autoscaling/v2" v1 "k8s.io/api/core/v1" @@ -48,6 +49,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/internal/config" "github.com/open-telemetry/opentelemetry-operator/internal/manifests" "github.com/open-telemetry/opentelemetry-operator/internal/naming" + "github.com/open-telemetry/opentelemetry-operator/pkg/featuregate" ) const ( @@ -74,6 +76,18 @@ var ( type check[T any] func(t *testing.T, params T) func TestOpenTelemetryCollectorReconciler_Reconcile(t *testing.T) { + // enable the collector CR feature flag, as these tests assume it + // TODO: drop this after the flag is enabled by default + registry := colfeaturegate.GlobalRegistry() + current := featuregate.CollectorUsesTargetAllocatorCR.IsEnabled() + require.False(t, current, "don't set gates which are enabled by default") + err := registry.Set(featuregate.CollectorUsesTargetAllocatorCR.ID(), true) + require.NoError(t, err) + t.Cleanup(func() { + err := registry.Set(featuregate.CollectorUsesTargetAllocatorCR.ID(), current) + require.NoError(t, err) + }) + addedMetadataDeployment := testCollectorWithMode("test-deployment", v1alpha1.ModeDeployment) addedMetadataDeployment.Labels = map[string]string{ labelName: labelVal,