fix: do not inject env special value to dep manifest managed by webho…

…ok (#1738) This is a special exception, used to temporarly filter out a specific value that is known to be managed by a webhook. For this case, we only want to populate the odigos value --------- Co-authored-by: Eden Federman <[email protected]>
odigos-io · Nov 18, 2024 · c699098 · c699098
1 parent 994090b
commit c699098
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 25 deletions.
diff --git a/common/envOverwrite/overwriter.go b/common/envOverwrite/overwriter.go
@@ -66,7 +66,6 @@ func GetRelevantEnvVarsKeys() []string {
 	return keys
 }
 
-
 // returns the current value that should be populated in a specific environment variable.
 // if we should not patch the value, returns nil.
 // the are 2 parts to the environment value: odigos part and user part.
@@ -111,6 +110,21 @@ func GetPatchedEnvValue(envName string, observedValue string, currentSdk *common
 		}
 	}
 
+	// temporary fix clean up observed value from the known webhook injected value
+	parts := strings.Split(observedValue, envMetadata.delim)
+	ignoreEnvValue := "-javaagent:/opt/sre-agent/sre-agent.jar"
+	newValues := []string{}
+	for _, part := range parts {
+		if part == ignoreEnvValue {
+			continue
+		}
+		if strings.TrimSpace(part) == "" {
+			continue
+		}
+		newValues = append(newValues, part)
+	}
+	observedValue = strings.Join(newValues, envMetadata.delim)
+
 	// Scenario 3: both odigos and user defined values are present
 	// happens: when the user set some values to this env (either via manifest or dockerfile) and odigos instrumentation is applied.
 	// action: we want to keep the user defined values and upsert the odigos value.
@@ -119,25 +133,8 @@ func GetPatchedEnvValue(envName string, observedValue string, currentSdk *common
 			if sdkEnvValue == desiredOdigosPart {
 				// shortcut, the value is already patched
 				// both the odigos part equals to the new value, and the user part we want to keep
-				// Exception: if there is a webhook involved that inject the env value,
-				// we need to remove duplicate values, otherwise it will grow indefinitely in each iteration
-				parts := strings.Split(observedValue, envMetadata.delim)
-				specialEnvValue := "-javaagent:/opt/sre-agent/sre-agent.jar"
-				specialFound := false
-				newValues := []string{}
-				for _, part := range parts {
-					if part == specialEnvValue {
-						if specialFound {
-							continue
-						}
-						specialFound = true
-					}
-					if strings.TrimSpace(part) == "" {
-						continue
-					}
-					newValues = append(newValues, part)
-				}
-				observedValue = strings.Join(newValues, envMetadata.delim)
+				// Exception: for a value that is injected by a webhook, we don't want to add it to
+				// the deployment, as the webhook will manage when it is needed.
 				return &observedValue
 			} else {
 				// The environment variable is patched by some other odigos sdk.
@@ -152,8 +149,13 @@ func GetPatchedEnvValue(envName string, observedValue string, currentSdk *common
 	// Scenario 4: only user defined values are present
 	// happens: when the user set some values to this env (either via manifest or dockerfile) and odigos instrumentation not yet applied.
 	// action: we want to keep the user defined values and append the odigos value.
-	mergedEnvValue := observedValue + envMetadata.delim + desiredOdigosPart
-	return &mergedEnvValue
+	if observedValue == "" {
+		return &desiredOdigosPart
+	} else {
+		// no user defined values, just append the odigos value
+		mergedEnvValue := observedValue + envMetadata.delim + desiredOdigosPart
+		return &mergedEnvValue
+	}
 }
 
 func ValToAppend(envName string, sdk common.OtelSdk) (string, bool) {

diff --git a/common/envOverwrite/overwriter_test.go b/common/envOverwrite/overwriter_test.go
@@ -96,23 +96,31 @@ func TestGetPatchedEnvValue(t *testing.T) {
 			observedValue:        fmt.Sprintf("%s %s %s %s", specialEnvValueJava, specialEnvValueJava, specialEnvValueJava, javaToolsNativeCommunity),
 			sdk:                  &common.OtelSdkNativeCommunity,
 			programmingLanguage:  common.JavaProgrammingLanguage,
-			patchedValueExpected: specialEnvValueJava + " " + javaToolsNativeCommunity,
+			patchedValueExpected: javaToolsNativeCommunity,
 		},
 		{
 			name:                 "multiple spaces in special env value",
 			envName:              "JAVA_TOOL_OPTIONS",
 			observedValue:        fmt.Sprintf("%s %s              %s", specialEnvValueJava, specialEnvValueJava, javaToolsNativeCommunity),
 			sdk:                  &common.OtelSdkNativeCommunity,
 			programmingLanguage:  common.JavaProgrammingLanguage,
-			patchedValueExpected: specialEnvValueJava + " " + javaToolsNativeCommunity,
+			patchedValueExpected: javaToolsNativeCommunity,
 		},
 		{
 			name:                 "tabs in special env value",
 			envName:              "JAVA_TOOL_OPTIONS",
 			observedValue:        fmt.Sprintf("%s \t %s \t %s", specialEnvValueJava, specialEnvValueJava, javaToolsNativeCommunity),
 			sdk:                  &common.OtelSdkNativeCommunity,
 			programmingLanguage:  common.JavaProgrammingLanguage,
-			patchedValueExpected: specialEnvValueJava + " " + javaToolsNativeCommunity,
+			patchedValueExpected: javaToolsNativeCommunity,
+		},
+		{
+			name:                 "special env with only user value",
+			envName:              "JAVA_TOOL_OPTIONS",
+			observedValue:        fmt.Sprintf("%s ", specialEnvValueJava),
+			sdk:                  &common.OtelSdkNativeCommunity,
+			programmingLanguage:  common.JavaProgrammingLanguage,
+			patchedValueExpected: javaToolsNativeCommunity,
 		},
 	}