Change multi instrumentation feature gate into command line flag (#2635)

* add multiIntrumentation to command line

* add multiinstrumentation to config and add test

* change github workflow for multi-instrumentation

* rename command

* remove unwanted comments

* add chlog

* modify chlog

* add missing description in config for multi-instrumentation

Author: changexd

.chloggen/multi-instrumentation-command-flag.yaml

@@ -0,0 +1,16 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: breaking
+
+# The name of the component, or a single word describing the area of concern, (e.g. operator, target allocator, github action)
+component: operator
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: change multi instrumentation feature gate into command line flag --enable-multi-instrumentation
+
+# One or more tracking issues related to the change
+issues: [2582]
+
+# (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: 

.github/workflows/e2e.yaml

@@ -36,7 +36,7 @@ jobs:
          - group: e2e-prometheuscr
            setup: "prepare-e2e-with-featuregates FEATUREGATES=+operator.observability.prometheus"
          - group: e2e-multi-instrumentation
-           setup: "prepare-e2e-with-featuregates FEATUREGATES=+operator.autoinstrumentation.multi-instrumentation"
+           setup: "add-operator-arg OPERATOR_ARG=--enable-multi-instrumentation prepare-e2e"
 
     steps:
     - name: Check out code into the Go module directory

internal/config/main.go

@@ -43,6 +43,7 @@ type Config struct {
 	collectorImage                      string
 	collectorConfigMapEntry             string
 	createRBACPermissions               bool
+	enableMultiInstrumentation          bool
 	autoInstrumentationDotNetImage      string
 	autoInstrumentationGoImage          string
 	autoInstrumentationApacheHttpdImage string
@@ -75,6 +76,7 @@ func New(opts ...Option) Config {
 		collectorImage:                      o.collectorImage,
 		collectorConfigMapEntry:             o.collectorConfigMapEntry,
 		createRBACPermissions:               o.createRBACPermissions,
+		enableMultiInstrumentation:          o.enableMultiInstrumentation,
 		targetAllocatorImage:                o.targetAllocatorImage,
 		operatorOpAMPBridgeImage:            o.operatorOpAMPBridgeImage,
 		targetAllocatorConfigMapEntry:       o.targetAllocatorConfigMapEntry,
@@ -109,6 +111,11 @@ func (c *Config) CollectorImage() string {
 	return c.collectorImage
 }
 
+// EnableMultiInstrumentation is true when the operator supports multi instrumentation.
+func (c *Config) EnableMultiInstrumentation() bool {
+	return c.enableMultiInstrumentation
+}
+
 // CollectorConfigMapEntry represents the configuration file name for the collector. Immutable.
 func (c *Config) CollectorConfigMapEntry() string {
 	return c.collectorConfigMapEntry

internal/config/options.go

@@ -42,6 +42,7 @@ type options struct {
 	collectorImage                      string
 	collectorConfigMapEntry             string
 	createRBACPermissions               bool
+	enableMultiInstrumentation          bool
 	targetAllocatorConfigMapEntry       string
 	operatorOpAMPBridgeConfigMapEntry   string
 	targetAllocatorImage                string
@@ -80,6 +81,11 @@ func WithCreateRBACPermissions(s bool) Option {
 		o.createRBACPermissions = s
 	}
 }
+func WithEnableMultiInstrumentation(s bool) Option {
+	return func(o *options) {
+		o.enableMultiInstrumentation = s
+	}
+}
 func WithTargetAllocatorConfigMapEntry(s string) Option {
 	return func(o *options) {
 		o.targetAllocatorConfigMapEntry = s

main.go

@@ -104,6 +104,7 @@ func main() {
 		pprofAddr                      string
 		enableLeaderElection           bool
 		createRBACPermissions          bool
+		enableMultiInstrumentation     bool
 		collectorImage                 string
 		targetAllocatorImage           string
 		operatorOpAMPBridgeImage       string
@@ -126,6 +127,7 @@ func main() {
 		"Enable leader election for controller manager. "+
 			"Enabling this will ensure there is only one active controller manager.")
 	pflag.BoolVar(&createRBACPermissions, "create-rbac-permissions", false, "Automatically create RBAC permissions needed by the processors")
+	pflag.BoolVar(&enableMultiInstrumentation, "enable-multi-instrumentation", false, "Controls whether the operator supports multi instrumentation")
 	stringFlagOrEnv(&collectorImage, "collector-image", "RELATED_IMAGE_COLLECTOR", fmt.Sprintf("ghcr.io/open-telemetry/opentelemetry-collector-releases/opentelemetry-collector:%s", v.OpenTelemetryCollector), "The default OpenTelemetry collector image. This image is used when no image is specified in the CustomResource.")
 	stringFlagOrEnv(&targetAllocatorImage, "target-allocator-image", "RELATED_IMAGE_TARGET_ALLOCATOR", fmt.Sprintf("ghcr.io/open-telemetry/opentelemetry-operator/target-allocator:%s", v.TargetAllocator), "The default OpenTelemetry target allocator image. This image is used when no image is specified in the CustomResource.")
 	stringFlagOrEnv(&operatorOpAMPBridgeImage, "operator-opamp-bridge-image", "RELATED_IMAGE_OPERATOR_OPAMP_BRIDGE", fmt.Sprintf("ghcr.io/open-telemetry/opentelemetry-operator/operator-opamp-bridge:%s", v.OperatorOpAMPBridge), "The default OpenTelemetry Operator OpAMP Bridge image. This image is used when no image is specified in the CustomResource.")
@@ -163,6 +165,7 @@ func main() {
 		"go-arch", runtime.GOARCH,
 		"go-os", runtime.GOOS,
 		"labels-filter", labelsFilter,
+		"enable-multi-instrumentation", enableMultiInstrumentation,
 	)
 
 	restConfig := ctrl.GetConfigOrDie()
@@ -179,6 +182,7 @@ func main() {
 		config.WithVersion(v),
 		config.WithCollectorImage(collectorImage),
 		config.WithCreateRBACPermissions(createRBACPermissions),
+		config.WithEnableMultiInstrumentation(enableMultiInstrumentation),
 		config.WithTargetAllocatorImage(targetAllocatorImage),
 		config.WithOperatorOpAMPBridgeImage(operatorOpAMPBridgeImage),
 		config.WithAutoInstrumentationJavaImage(autoInstrumentationJava),
@@ -292,7 +296,7 @@ func main() {
 			Handler: podmutation.NewWebhookHandler(cfg, ctrl.Log.WithName("pod-webhook"), decoder, mgr.GetClient(),
 				[]podmutation.PodMutator{
 					sidecar.NewMutator(logger, cfg, mgr.GetClient()),
-					instrumentation.NewMutator(logger, mgr.GetClient(), mgr.GetEventRecorderFor("opentelemetry-operator")),
+					instrumentation.NewMutator(logger, mgr.GetClient(), mgr.GetEventRecorderFor("opentelemetry-operator"), cfg),
 				}),
 		})
 

pkg/instrumentation/golang.go

@@ -21,7 +21,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 
 	"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
-	"github.com/open-telemetry/opentelemetry-operator/pkg/featuregate"
+	"github.com/open-telemetry/opentelemetry-operator/internal/config"
 )
 
 const (
@@ -31,7 +31,7 @@ const (
 	kernelDebugVolumePath = "/sys/kernel/debug"
 )
 
-func injectGoSDK(goSpec v1alpha1.Go, pod corev1.Pod) (corev1.Pod, error) {
+func injectGoSDK(goSpec v1alpha1.Go, pod corev1.Pod, cfg config.Config) (corev1.Pod, error) {
 	// skip instrumentation if share process namespaces is explicitly disabled
 	if pod.Spec.ShareProcessNamespace != nil && !*pod.Spec.ShareProcessNamespace {
 		return pod, fmt.Errorf("shared process namespace has been explicitly disabled")
@@ -40,7 +40,7 @@ func injectGoSDK(goSpec v1alpha1.Go, pod corev1.Pod) (corev1.Pod, error) {
 	// skip instrumentation when more than one containers provided
 	containerNames := ""
 	ok := false
-	if featuregate.EnableMultiInstrumentationSupport.IsEnabled() {
+	if cfg.EnableMultiInstrumentation() {
 		containerNames, ok = pod.Annotations[annotationInjectGoContainersName]
 	} else {
 		containerNames, ok = pod.Annotations[annotationInjectContainerName]

pkg/instrumentation/golang_test.go

@@ -19,13 +19,11 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	colfeaturegate "go.opentelemetry.io/collector/featuregate"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
-	"github.com/open-telemetry/opentelemetry-operator/pkg/featuregate"
+	"github.com/open-telemetry/opentelemetry-operator/internal/config"
 )
 
 func TestInjectGoSDK(t *testing.T) {
@@ -36,10 +34,10 @@ func TestInjectGoSDK(t *testing.T) {
 	tests := []struct {
 		name string
 		v1alpha1.Go
-		pod             corev1.Pod
-		expected        corev1.Pod
-		err             error
-		setFeatureGates func(t *testing.T)
+		pod      corev1.Pod
+		expected corev1.Pod
+		err      error
+		config   config.Config
 	}{
 		{
 			name: "shared process namespace disabled",
@@ -73,14 +71,8 @@ func TestInjectGoSDK(t *testing.T) {
 					},
 				},
 			},
-			err: fmt.Errorf("go instrumentation cannot be injected into a pod, multiple containers configured"),
-			setFeatureGates: func(t *testing.T) {
-				originalVal := featuregate.EnableMultiInstrumentationSupport.IsEnabled()
-				require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableMultiInstrumentationSupport.ID(), true))
-				t.Cleanup(func() {
-					require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableMultiInstrumentationSupport.ID(), originalVal))
-				})
-			},
+			err:    fmt.Errorf("go instrumentation cannot be injected into a pod, multiple containers configured"),
+			config: config.New(config.WithEnableMultiInstrumentation(true)),
 		},
 		{
 			name: "using container-names",
@@ -287,10 +279,7 @@ func TestInjectGoSDK(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			if test.setFeatureGates != nil {
-				test.setFeatureGates(t)
-			}
-			pod, err := injectGoSDK(test.Go, test.pod)
+			pod, err := injectGoSDK(test.Go, test.pod, test.config)
 			assert.Equal(t, test.expected, pod)
 			assert.Equal(t, test.err, err)
 		})

pkg/instrumentation/podmutator.go

@@ -27,6 +27,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
+	"github.com/open-telemetry/opentelemetry-operator/internal/config"
 	"github.com/open-telemetry/opentelemetry-operator/internal/webhook/podmutation"
 	"github.com/open-telemetry/opentelemetry-operator/pkg/featuregate"
 )
@@ -41,6 +42,7 @@ type instPodMutator struct {
 	sdkInjector *sdkInjector
 	Logger      logr.Logger
 	Recorder    record.EventRecorder
+	config      config.Config
 }
 
 type instrumentationWithContainers struct {
@@ -193,7 +195,7 @@ func (langInsts *languageInstrumentations) setInstrumentationLanguageContainers(
 
 var _ podmutation.PodMutator = (*instPodMutator)(nil)
 
-func NewMutator(logger logr.Logger, client client.Client, recorder record.EventRecorder) *instPodMutator {
+func NewMutator(logger logr.Logger, client client.Client, recorder record.EventRecorder, cfg config.Config) *instPodMutator {
 	return &instPodMutator{
 		Logger: logger,
 		Client: client,
@@ -202,6 +204,7 @@ func NewMutator(logger logr.Logger, client client.Client, recorder record.EventR
 			client: client,
 		},
 		Recorder: recorder,
+		config:   cfg,
 	}
 }
 
@@ -323,7 +326,7 @@ func (pm *instPodMutator) Mutate(ctx context.Context, ns corev1.Namespace, pod c
 	}
 
 	// We retrieve the annotation for podname
-	if featuregate.EnableMultiInstrumentationSupport.IsEnabled() {
+	if pm.config.EnableMultiInstrumentation() {
 		// We use annotations specific for instrumentation language
 		insts.Java.Containers = annotationValue(ns.ObjectMeta, pod.ObjectMeta, annotationInjectJavaContainersName)
 		insts.NodeJS.Containers = annotationValue(ns.ObjectMeta, pod.ObjectMeta, annotationInjectNodeJSContainersName)
@@ -357,7 +360,7 @@ func (pm *instPodMutator) Mutate(ctx context.Context, ns corev1.Namespace, pod c
 	// once it's been determined that instrumentation is desired, none exists yet, and we know which instance it should talk to,
 	// we should inject the instrumentation.
 	modifiedPod := pod
-	modifiedPod = pm.sdkInjector.inject(ctx, insts, ns, modifiedPod)
+	modifiedPod = pm.sdkInjector.inject(ctx, insts, ns, modifiedPod, pm.config)
 
 	return modifiedPod, nil
 }

pkg/instrumentation/podmutator_test.go

@@ -28,12 +28,11 @@ import (
 	"k8s.io/client-go/tools/record"
 
 	"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
+	"github.com/open-telemetry/opentelemetry-operator/internal/config"
 	"github.com/open-telemetry/opentelemetry-operator/pkg/featuregate"
 )
 
 func TestMutatePod(t *testing.T) {
-	mutator := NewMutator(logr.Discard(), k8sClient, record.NewFakeRecorder(100))
-	require.NotNil(t, mutator)
 
 	true := true
 	zero := int64(0)
@@ -46,6 +45,7 @@ func TestMutatePod(t *testing.T) {
 		inst            v1alpha1.Instrumentation
 		ns              corev1.Namespace
 		setFeatureGates func(t *testing.T)
+		config          config.Config
 	}{
 		{
 			name: "javaagent injection, true",
@@ -2509,16 +2509,13 @@ func TestMutatePod(t *testing.T) {
 					},
 				},
 			},
+			config: config.New(config.WithEnableMultiInstrumentation(true)),
 			setFeatureGates: func(t *testing.T) {
 				originalVal := featuregate.EnableGoAutoInstrumentationSupport.IsEnabled()
-				mtVal := featuregate.EnableMultiInstrumentationSupport.IsEnabled()
-
-				require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableMultiInstrumentationSupport.ID(), true))
 
 				require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableGoAutoInstrumentationSupport.ID(), true))
 				t.Cleanup(func() {
 					require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableGoAutoInstrumentationSupport.ID(), originalVal))
-					require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableMultiInstrumentationSupport.ID(), mtVal))
 
 				})
 			},
@@ -3860,13 +3857,7 @@ func TestMutatePod(t *testing.T) {
 					},
 				},
 			},
-			setFeatureGates: func(t *testing.T) {
-				originalVal := featuregate.EnableMultiInstrumentationSupport.IsEnabled()
-				require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableMultiInstrumentationSupport.ID(), true))
-				t.Cleanup(func() {
-					require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableMultiInstrumentationSupport.ID(), originalVal))
-				})
-			},
+			config: config.New(config.WithEnableMultiInstrumentation(true)),
 		},
 		{
 			name: "multi instrumentation for multiple containers feature gate enabled, container-names not used",
@@ -4524,13 +4515,7 @@ func TestMutatePod(t *testing.T) {
 					},
 				},
 			},
-			setFeatureGates: func(t *testing.T) {
-				originalVal := featuregate.EnableMultiInstrumentationSupport.IsEnabled()
-				require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableMultiInstrumentationSupport.ID(), true))
-				t.Cleanup(func() {
-					require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableMultiInstrumentationSupport.ID(), originalVal))
-				})
-			},
+			config: config.New(config.WithEnableMultiInstrumentation(true)),
 		},
 		{
 			name: "multi instrumentation for multiple containers feature gate disabled, multiple instrumentation annotations set",
@@ -4684,13 +4669,7 @@ func TestMutatePod(t *testing.T) {
 					},
 				},
 			},
-			setFeatureGates: func(t *testing.T) {
-				originalVal := featuregate.EnableMultiInstrumentationSupport.IsEnabled()
-				require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableMultiInstrumentationSupport.ID(), false))
-				t.Cleanup(func() {
-					require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableMultiInstrumentationSupport.ID(), originalVal))
-				})
-			},
+			config: config.New(config.WithEnableMultiInstrumentation(false)),
 		},
 		{
 			name: "multi instrumentation feature gate enabled, multiple instrumentation annotations set, no containers",
@@ -4834,13 +4813,7 @@ func TestMutatePod(t *testing.T) {
 					},
 				},
 			},
-			setFeatureGates: func(t *testing.T) {
-				originalVal := featuregate.EnableMultiInstrumentationSupport.IsEnabled()
-				require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableMultiInstrumentationSupport.ID(), true))
-				t.Cleanup(func() {
-					require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableMultiInstrumentationSupport.ID(), originalVal))
-				})
-			},
+			config: config.New(config.WithEnableMultiInstrumentation(true)),
 		},
 		{
 			name: "multi instrumentation feature gate enabled, single instrumentation annotation set, no containers",
@@ -5019,13 +4992,7 @@ func TestMutatePod(t *testing.T) {
 					},
 				},
 			},
-			setFeatureGates: func(t *testing.T) {
-				originalVal := featuregate.EnableMultiInstrumentationSupport.IsEnabled()
-				require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableMultiInstrumentationSupport.ID(), true))
-				t.Cleanup(func() {
-					require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableMultiInstrumentationSupport.ID(), originalVal))
-				})
-			},
+			config: config.New(config.WithEnableMultiInstrumentation(true)),
 		},
 		{
 			name: "multi instrumentation feature gate disabled, instrumentation feature gate disabled and annotation set, multiple specific containers set",
@@ -5125,13 +5092,11 @@ func TestMutatePod(t *testing.T) {
 					},
 				},
 			},
+			config: config.New(config.WithEnableMultiInstrumentation(true)),
 			setFeatureGates: func(t *testing.T) {
-				originalValMultiInstr := featuregate.EnableMultiInstrumentationSupport.IsEnabled()
-				require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableMultiInstrumentationSupport.ID(), true))
 				originalValDotNetInstr := featuregate.EnableDotnetAutoInstrumentationSupport.IsEnabled()
 				require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableDotnetAutoInstrumentationSupport.ID(), false))
 				t.Cleanup(func() {
-					require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableMultiInstrumentationSupport.ID(), originalValMultiInstr))
 					require.NoError(t, colfeaturegate.GlobalRegistry().Set(featuregate.EnableDotnetAutoInstrumentationSupport.ID(), originalValDotNetInstr))
 				})
 			},
@@ -5141,6 +5106,8 @@ func TestMutatePod(t *testing.T) {
 	for _, test := range tests {
 		test := test
 		t.Run(test.name, func(t *testing.T) {
+			mutator := NewMutator(logr.Discard(), k8sClient, record.NewFakeRecorder(100), test.config)
+			require.NotNil(t, mutator)
 			if test.setFeatureGates != nil {
 				test.setFeatureGates(t)
 			}