[chore] Fix featuregate usage in controller tests (#3490)

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.
open-telemetry · Nov 25, 2024 · 3d254a4 · 3d254a4
1 parent cdff25a
commit 3d254a4
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 27 deletions.
diff --git 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
@@ -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,