Add handling of environment variables in Secrets

Load also environment variables defined in Secret to make them visible to OpenTelemetry Operator. Signed-off-by: Oldřich Jedlička <[email protected]>
open-telemetry · Oct 24, 2024 · 6107eec · 6107eec
1 parent cad425e
commit 6107eec
Show file tree

Hide file tree

Showing 13 changed files with 567 additions and 63 deletions.
diff --git a/.chloggen/enhancement-envs-from-secret.yaml b/.chloggen/enhancement-envs-from-secret.yaml
@@ -0,0 +1,19 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: enhancement
+
+# 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: auto-instrumentation
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Add support for detecting and reading environment variables from POD's Secret resources.
+
+# One or more tracking issues related to the change
+issues: [1393, 1814]
+
+# (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: |
+  Now the auto-instrumentation will see the environment variables defined via POD's `env.valueFrom.secretKeyRef` and
+  `envFrom.secretRef` fields, so the auto-instrumentation will be able to prevent overriding them, or it will be able to
+  extend the existing definition.
diff --git a/pkg/instrumentation/container.go b/pkg/instrumentation/container.go
@@ -33,6 +33,7 @@ type Container struct {
 	index        int
 	inheritedEnv map[string]string
 	configMaps   map[string]*corev1.ConfigMap
+	secrets      map[string]*corev1.Secret
 }
 
 func NewContainer(client client.Reader, ctx context.Context, logger logr.Logger, namespace string, pod *corev1.Pod, index int) (Container, error) {
@@ -42,22 +43,17 @@ func NewContainer(client client.Reader, ctx context.Context, logger logr.Logger,
 	container := &pod.Spec.Containers[index]
 
 	configMaps := make(map[string]*corev1.ConfigMap)
+	secrets := make(map[string]*corev1.Secret)
 	inheritedEnv := make(map[string]string)
 	for _, envsFrom := range container.EnvFrom {
 		if envsFrom.ConfigMapRef != nil {
-			prefix := envsFrom.Prefix
-			name := envsFrom.ConfigMapRef.Name
-			if cm, err := getOrLoadResource(client, ctx, namespace, configMaps, name); err == nil {
-				for k, v := range cm.Data {
-					// Safely overwrite the value, last one from EnvFrom wins in Kubernetes, with the direct value
-					// from the container itself taking precedence
-					inheritedEnv[prefix+k] = v
-				}
-			} else if envsFrom.ConfigMapRef.Optional == nil || !*envsFrom.ConfigMapRef.Optional {
-				return Container{}, fmt.Errorf("failed to load environment variables: %w", err)
+			if err := loadAllEnvVars(client, ctx, namespace, configMaps, envsFrom.ConfigMapRef.Name, envsFrom.Prefix, envsFrom.ConfigMapRef.Optional, inheritedEnv); err != nil {
+				return Container{}, err
 			}
 		} else if envsFrom.SecretRef != nil {
-			logger.V(2).Info("ignoring SecretRef in EnvFrom", "container", container.Name, "secret", envsFrom.SecretRef.Name)
+			if err := loadAllEnvVars(client, ctx, namespace, secrets, envsFrom.SecretRef.Name, envsFrom.Prefix, envsFrom.SecretRef.Optional, inheritedEnv); err != nil {
+				return Container{}, err
+			}
 		}
 	}
 
@@ -73,39 +69,10 @@ func NewContainer(client client.Reader, ctx context.Context, logger logr.Logger,
 		index:        index,
 		inheritedEnv: inheritedEnv,
 		configMaps:   configMaps,
+		secrets:      secrets,
 	}, nil
 }
 
-func getOrLoadResource[T any, PT interface {
-	client.Object
-	*T
-}](client client.Reader, ctx context.Context, namespace string, cache map[string]*T, name string) (*T, error) {
-	var obj T
-	if cached, ok := cache[name]; ok {
-		if cached != nil {
-			return cached, nil
-		} else {
-			return nil, fmt.Errorf("failed to get %s %s/%s", reflect.TypeOf(obj).Name(), namespace, name)
-		}
-	}
-
-	if client == nil || ctx == nil {
-		// Cache error value
-		cache[name] = nil
-		return nil, fmt.Errorf("client or context is nil, cannot load %s %s/%s", reflect.TypeOf(obj).Name(), namespace, name)
-	}
-
-	err := client.Get(ctx, types.NamespacedName{Namespace: namespace, Name: name}, PT(&obj))
-	if err != nil {
-		// Cache error value
-		cache[name] = nil
-		return nil, fmt.Errorf("failed to get %s %s/%s: %w", reflect.TypeOf(obj).Name(), namespace, name, err)
-	}
-
-	cache[name] = &obj
-	return &obj, nil
-}
-
 func (c *Container) validate(pod *corev1.Pod, envsToBeValidated ...string) error {
 	// Try if the value is resolvable
 	for _, envToBeValidated := range envsToBeValidated {
@@ -169,21 +136,9 @@ func (c *Container) getOrMakeEnvVar(pod *corev1.Pod, name string) (corev1.EnvVar
 func (c *Container) resolveEnvVar(envVar corev1.EnvVar) (corev1.EnvVar, error) {
 	if envVar.Value == "" && envVar.ValueFrom != nil {
 		if envVar.ValueFrom.ConfigMapKeyRef != nil {
-			configMapName := envVar.ValueFrom.ConfigMapKeyRef.Name
-			configMapKey := envVar.ValueFrom.ConfigMapKeyRef.Key
-			if cm, err := getOrLoadResource(c.client, c.ctx, c.namespace, c.configMaps, configMapName); err == nil {
-				if value, ok := cm.Data[configMapKey]; ok {
-					return corev1.EnvVar{Name: envVar.Name, Value: value}, nil
-				} else if envVar.ValueFrom.ConfigMapKeyRef.Optional == nil || !*envVar.ValueFrom.ConfigMapKeyRef.Optional {
-					return corev1.EnvVar{}, fmt.Errorf("failed to resolve environment variable %s, key %s not found in ConfigMap %s/%s", envVar.Name, configMapKey, c.namespace, configMapName)
-				} else {
-					return corev1.EnvVar{Name: envVar.Name, Value: ""}, nil
-				}
-			} else if envVar.ValueFrom.ConfigMapKeyRef.Optional == nil || !*envVar.ValueFrom.ConfigMapKeyRef.Optional {
-				return corev1.EnvVar{}, fmt.Errorf("failed to resolve environment variable %s: %w", envVar.Name, err)
-			} else {
-				return corev1.EnvVar{Name: envVar.Name, Value: ""}, nil
-			}
+			return loadEnvVar(c.client, c.ctx, c.namespace, c.configMaps, envVar.Name, envVar.ValueFrom.ConfigMapKeyRef.Name, envVar.ValueFrom.ConfigMapKeyRef.Key, envVar.ValueFrom.ConfigMapKeyRef.Optional)
+		} else if envVar.ValueFrom.SecretKeyRef != nil {
+			return loadEnvVar(c.client, c.ctx, c.namespace, c.secrets, envVar.Name, envVar.ValueFrom.SecretKeyRef.Name, envVar.ValueFrom.SecretKeyRef.Key, envVar.ValueFrom.SecretKeyRef.Optional)
 		} else {
 			v := reflect.ValueOf(*envVar.ValueFrom)
 			for i := 0; i < v.NumField(); i++ {
@@ -197,6 +152,98 @@ func (c *Container) resolveEnvVar(envVar corev1.EnvVar) (corev1.EnvVar, error) {
 	return envVar, nil
 }
 
+func getOrLoadResource[T any, PT interface {
+	client.Object
+	*T
+}](client client.Reader, ctx context.Context, namespace string, cache map[string]*T, name string) (*T, error) {
+	var obj T
+	if cached, ok := cache[name]; ok {
+		if cached != nil {
+			return cached, nil
+		} else {
+			return nil, fmt.Errorf("failed to get %s %s/%s", reflect.TypeOf(obj).Name(), namespace, name)
+		}
+	}
+
+	if client == nil || ctx == nil {
+		// Cache error value
+		cache[name] = nil
+		return nil, fmt.Errorf("client or context is nil, cannot load %s %s/%s", reflect.TypeOf(obj).Name(), namespace, name)
+	}
+
+	err := client.Get(ctx, types.NamespacedName{Namespace: namespace, Name: name}, PT(&obj))
+	if err != nil {
+		// Cache error value
+		cache[name] = nil
+		return nil, fmt.Errorf("failed to get %s %s/%s: %w", reflect.TypeOf(obj).Name(), namespace, name, err)
+	}
+
+	cache[name] = &obj
+	return &obj, nil
+}
+
+func loadAllEnvVars[T any, PT interface {
+	client.Object
+	*T
+}](client client.Reader, ctx context.Context, namespace string, cache map[string]*T, name string, prefix string, optional *bool, inheritedEnv map[string]string) error {
+	if obj, err := getOrLoadResource[T, PT](client, ctx, namespace, cache, name); err == nil {
+		return fillEnvVars(obj, prefix, inheritedEnv)
+	} else if optional == nil || !*optional {
+		return fmt.Errorf("failed to load environment variables: %w", err)
+	} else {
+		return nil
+	}
+}
+
+func loadEnvVar[T any, PT interface {
+	client.Object
+	*T
+}](client client.Reader, ctx context.Context, namespace string, cache map[string]*T, variable string, name string, key string, optional *bool) (corev1.EnvVar, error) {
+	if resource, err := getOrLoadResource[T, PT](client, ctx, namespace, cache, name); err == nil {
+		if value, ok := getResourceValue(resource, key); ok {
+			return corev1.EnvVar{Name: variable, Value: value}, nil
+		} else if optional == nil || !*optional {
+			return corev1.EnvVar{}, fmt.Errorf("failed to resolve environment variable %s, key %s not found in %s %s/%s", variable, key, reflect.TypeOf(*resource).Name(), namespace, name)
+		} else {
+			return corev1.EnvVar{Name: variable, Value: ""}, nil
+		}
+	} else if optional == nil || !*optional {
+		return corev1.EnvVar{}, fmt.Errorf("failed to resolve environment variable %s: %w", variable, err)
+	} else {
+		return corev1.EnvVar{Name: variable, Value: ""}, nil
+	}
+}
+
+func fillEnvVars(obj any, prefix string, inheritedEnv map[string]string) error {
+	switch o := obj.(type) {
+	case *corev1.ConfigMap:
+		for k, v := range o.Data {
+			inheritedEnv[prefix+k] = v
+		}
+		return nil
+	case *corev1.Secret:
+		for k, v := range o.Data {
+			inheritedEnv[prefix+k] = string(v)
+		}
+		return nil
+	default:
+		return fmt.Errorf("unsupported type %T", obj)
+	}
+}
+
+func getResourceValue(obj interface{}, key string) (string, bool) {
+	switch o := obj.(type) {
+	case *corev1.ConfigMap:
+		val, ok := o.Data[key]
+		return val, ok
+	case *corev1.Secret:
+		val, ok := o.Data[key]
+		return string(val), ok
+	default:
+		return "", false
+	}
+}
+
 func existsEnvVarInEnv(env []corev1.EnvVar, name string) bool {
 	for i := range env {
 		if env[i].Name == name {

diff --git a/pkg/instrumentation/container_test.go b/pkg/instrumentation/container_test.go
@@ -377,7 +377,7 @@ func TestInheritedEnv(t *testing.T) {
 			err: "failed to get ConfigMap inheritedenv-notfoundexplicit/my-config",
 		},
 		{
-			name: "SecretRef not supported",
+			name: "Simple SecretRef usage",
 			ns: corev1.Namespace{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "inheritedenv-secretref",
@@ -410,7 +410,9 @@ func TestInheritedEnv(t *testing.T) {
 					},
 				},
 			},
-			log: "ignoring SecretRef in EnvFrom",
+			expected: map[string]string{
+				"OTEL_ENVFROM_SECRET_VALUE1": "my-secret-value1",
+			},
 		},
 	}
 
@@ -1011,7 +1013,7 @@ func TestResolving(t *testing.T) {
 				Namespace: "validations",
 			},
 			Data: map[string][]byte{
-				"secret-ref-value1": []byte("my-valuefrom-value1"),
+				"secret-ref-value1": []byte("my-secret-valuefrom-value1"),
 			},
 		},
 	}
@@ -1151,10 +1153,10 @@ func TestResolving(t *testing.T) {
 			expectedResolve: "",
 		},
 		{
-			name:           "Test unsupported existing Secret variable",
-			variable:       "OTEL_ENV_VALUEFROM_SECRET1",
-			expectedExists: true,
-			err:            "the container defines env var value via ValueFrom.SecretKeyRef, envVar: OTEL_ENV_VALUEFROM_SECRET1",
+			name:            "Test unsupported existing Secret variable",
+			variable:        "OTEL_ENV_VALUEFROM_SECRET1",
+			expectedExists:  true,
+			expectedResolve: "my-secret-valuefrom-value1",
 		},
 		{
 			name:           "Test unsupported existing FieldRef variable",

diff --git a/tests/e2e-instrumentation/instrumentation-java-envfrom-secret/00-install-collector.yaml b/tests/e2e-instrumentation/instrumentation-java-envfrom-secret/00-install-collector.yaml
@@ -0,0 +1,22 @@
+apiVersion: opentelemetry.io/v1alpha1
+kind: OpenTelemetryCollector
+metadata:
+  name: sidecar
+spec:
+  config: |
+    receivers:
+      otlp:
+        protocols:
+          grpc:
+          http:
+    processors:
+
+    exporters:
+      debug:
+
+    service:
+      pipelines:
+        traces:
+          receivers: [otlp]
+          exporters: [debug]
+  mode: sidecar
diff --git a/...s/e2e-instrumentation/instrumentation-java-envfrom-secret/00-install-instrumentation.yaml b/...s/e2e-instrumentation/instrumentation-java-envfrom-secret/00-install-instrumentation.yaml
@@ -0,0 +1,34 @@
+apiVersion: opentelemetry.io/v1alpha1
+kind: Instrumentation
+metadata:
+  name: java
+spec:
+  env:
+    - name: OTEL_TRACES_EXPORTER
+      value: otlp
+    - name: OTEL_EXPORTER_OTLP_ENDPOINT
+      value: http://localhost:4317
+    - name: OTEL_EXPORTER_OTLP_TIMEOUT
+      value: "20"
+    - name: OTEL_TRACES_SAMPLER
+      value: parentbased_traceidratio
+    - name: OTEL_TRACES_SAMPLER_ARG
+      value: "0.85"
+    - name: SPLUNK_TRACE_RESPONSE_HEADER_ENABLED
+      value: "true"
+  exporter:
+    endpoint: http://localhost:4317
+  propagators:
+    - jaeger
+    - b3
+  sampler:
+    type: parentbased_traceidratio
+    argument: "0.25"
+  java:
+    env:
+    - name: OTEL_JAVAAGENT_DEBUG
+      value: "true"
+    - name: OTEL_INSTRUMENTATION_JDBC_ENABLED
+      value: "false"
+    - name: SPLUNK_PROFILER_ENABLED
+      value: "false"