[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.
open-telemetry · Nov 25, 2024 · 9262ba8 · 9262ba8
1 parent cdff25a
commit 9262ba8
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,