fix: correctly use changes in environment in manifest

common/envOverwrite/overwriter.go

@@ -71,42 +71,90 @@ func GetRelevantEnvVarsKeys() []string {
 // the are 2 parts to the environment value: odigos part and user part.
 // either one can be set or empty.
 // so we have 4 cases to handle:
-func GetPatchedEnvValue(envName string, observedValue string, currentSdk *common.OtelSdk, language common.ProgrammingLanguage) *string {
+func GetPatchedEnvValue(envName string, observedValue string, currentSdk *common.OtelSdk, language common.ProgrammingLanguage, currentContainerManifestValue *string, annotationOriginalValue *string, annotationOriginalValueFound bool) (*string, *string) {
 	envMetadata, ok := EnvValuesMap[envName]
 	if !ok {
 		// Odigos does not manipulate this environment variable, so ignore it
-		return nil
+		return nil, nil
 	}
 
 	if envMetadata.programmingLanguage != language {
 		// Odigos does not manipulate this environment variable for the given language, so ignore it
-		return nil
+		return nil, nil
 	}
 
 	if currentSdk == nil {
 		// When we have no sdk injected, we should not inject any odigos values.
-		return nil
+		return nil, nil
 	}
 
 	desiredOdigosPart, ok := envMetadata.values[*currentSdk]
 	if !ok {
 		// No specific overwrite is required for this SDK
-		return nil
+		return nil, nil
+	}
+
+	// check if we already processed this env since it's annotation
+	if annotationOriginalValueFound {
+
+		desiredFullValueWithObserved := observedValue
+		if !strings.Contains(observedValue, desiredOdigosPart) {
+			desiredFullValueWithObserved = observedValue + envMetadata.delim + desiredOdigosPart
+		}
+
+		if currentContainerManifestValue == nil {
+			// if the value is from dockerfile, we need to add observed value to it
+			if annotationOriginalValue == nil {
+				return &desiredFullValueWithObserved, nil
+			} else {
+				return &desiredOdigosPart, nil
+			}
+		}
+
+		// this part checks if the value in the env is what odigos would use,
+		// and if it is, don't make any changes
+		if desiredFullValueWithObserved == *currentContainerManifestValue {
+			return currentContainerManifestValue, annotationOriginalValue
+		}
+
+		currentManifestContainsOdigosValue := strings.Contains(*currentContainerManifestValue, desiredOdigosPart)
+		if currentManifestContainsOdigosValue {
+			// our goal is to be in the environment. if we are already there, so no overwrite is required.
+			// Note 1: assuming that the desired odigos part will not change! (since we are comparing to it).
+			// Note 2: if the runtime detected value changed from dockerfile, we will not pick it up here.
+			userEnvValue := strings.Replace(*currentContainerManifestValue, envMetadata.delim+desiredOdigosPart, "", -1)
+			return currentContainerManifestValue, &userEnvValue
+		} else {
+			// assuming that the manifest value set by the user is what should be used and ignore runtime details.
+			// just add odigos value
+			// TODO: update annotation with the new value
+			newValue := *currentContainerManifestValue + envMetadata.delim + desiredOdigosPart
+			return &newValue, currentContainerManifestValue
+		}
+	} else {
+		// else means there is no annotation on the workload.
+		if currentContainerManifestValue != nil {
+			currentManifestContainsOdigosValue := strings.Contains(*currentContainerManifestValue, desiredOdigosPart)
+			if !currentManifestContainsOdigosValue {
+				userNewValAndOdigos := *currentContainerManifestValue + envMetadata.delim + desiredOdigosPart
+				return &userNewValAndOdigos, currentContainerManifestValue
+			}
+		}
 	}
 
 	// scenario 1: no user defined values and no odigos value
 	// happens: might be the case right after the source is instrumented, and before the instrumentation is applied.
 	// action: there are no user defined values, so no need to make any changes.
 	if observedValue == "" {
-		return nil
+		return nil, nil
 	}
 
 	// scenario 2: no user defined values, only odigos value
 	// happens: when the user did not set any value to this env (either via manifest or dockerfile)
 	// action: we don't need to overwrite the value, just let odigos handle it
 	for _, sdkEnvValue := range envMetadata.values {
 		if sdkEnvValue == observedValue {
-			return nil
+			return nil, nil
 		}
 	}
 
@@ -135,13 +183,13 @@ func GetPatchedEnvValue(envName string, observedValue string, currentSdk *common
 				// both the odigos part equals to the new value, and the user part we want to keep
 				// 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
+				return &observedValue, currentContainerManifestValue
 			} else {
 				// The environment variable is patched by some other odigos sdk.
 				// replace just the odigos part with the new desired value.
 				// this can happen when moving between SDKs.
 				patchedEvnValue := strings.ReplaceAll(observedValue, sdkEnvValue, desiredOdigosPart)
-				return &patchedEvnValue
+				return &patchedEvnValue, currentContainerManifestValue
 			}
 		}
 	}
@@ -150,11 +198,11 @@ func GetPatchedEnvValue(envName string, observedValue string, currentSdk *common
 	// 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.
 	if observedValue == "" {
-		return &desiredOdigosPart
+		return &desiredOdigosPart, currentContainerManifestValue
 	} else {
 		// no user defined values, just append the odigos value
 		mergedEnvValue := observedValue + envMetadata.delim + desiredOdigosPart
-		return &mergedEnvValue
+		return &mergedEnvValue, currentContainerManifestValue
 	}
 }
 

common/envOverwrite/overwriter_test.go

@@ -126,7 +126,7 @@ func TestGetPatchedEnvValue(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			patchedValue := GetPatchedEnvValue(tt.envName, tt.observedValue, tt.sdk, tt.programmingLanguage)
+			patchedValue, _ := GetPatchedEnvValue(tt.envName, tt.observedValue, tt.sdk, tt.programmingLanguage, "", nil, false)
 			if patchedValue == nil {
 				assert.Equal(t, tt.patchedValueExpected, "", "mismatch in GetPatchedEnvValue: %s", tt.name)
 			} else {

instrumentor/instrumentation/instrumentation.go

@@ -201,7 +201,7 @@ func getEnvVarsOfContainer(instrumentation *odigosv1.InstrumentedApplication, co
 
 // when otelsdk is nil, it means that the container is not instrumented.
 // this will trigger reverting of any existing env vars which were set by odigos before.
-func patchEnvVarsForContainer(runtimeDetails *odigosv1.InstrumentedApplication, container *corev1.Container, sdk *common.OtelSdk, programmingLanguage common.ProgrammingLanguage, manifestEnvOriginal *envoverwrite.OrigWorkloadEnvValues) error {
+func patchEnvVarsForContainer(runtimeDetails *odigosv1.InstrumentedApplication, container *corev1.Container, sdk *common.OtelSdk, programmingLanguage common.ProgrammingLanguage, annotationManifestEnv *envoverwrite.OrigWorkloadEnvValues) error {
 
 	observedEnvs := getEnvVarsOfContainer(runtimeDetails, container.Name)
 
@@ -210,13 +210,15 @@ func patchEnvVarsForContainer(runtimeDetails *odigosv1.InstrumentedApplication,
 	for _, envVar := range container.Env {
 
 		// extract the observed value for this env var, which might be empty if not currently exists
-		observedEnvValue := observedEnvs[envVar.Name]
+		runtimeDetailsValue := observedEnvs[envVar.Name]
+		currentContainerManifestValue := envVar.Value
+		annotationOriginalValue, annotationOriginalValueFound := annotationManifestEnv.GetContainerStoredEnvs(container.Name)[envVar.Name]
 
-		desiredEnvValue := envOverwrite.GetPatchedEnvValue(envVar.Name, observedEnvValue, sdk, programmingLanguage)
+		desiredEnvValue, annotationValue := envOverwrite.GetPatchedEnvValue(envVar.Name, runtimeDetailsValue, sdk, programmingLanguage, &currentContainerManifestValue, annotationOriginalValue, annotationOriginalValueFound)
 
 		if desiredEnvValue == nil {
 			// no need to patch this env var, so make sure it is reverted to its original value
-			origValue, found := manifestEnvOriginal.RemoveOriginalValue(container.Name, envVar.Name)
+			origValue, found := annotationManifestEnv.RemoveOriginalValue(container.Name, envVar.Name)
 			if !found {
 				newEnvs = append(newEnvs, envVar)
 			} else { // found, we need to update the env var to it's original value
@@ -233,7 +235,7 @@ func patchEnvVarsForContainer(runtimeDetails *odigosv1.InstrumentedApplication,
 			}
 		} else { // there is a desired value to inject
 			// if it's the first time we patch this env var, save the original value
-			manifestEnvOriginal.InsertOriginalValue(container.Name, envVar.Name, &envVar.Value)
+			annotationManifestEnv.UpsertOriginalValue(container.Name, envVar.Name, annotationValue)
 			// update the env var to it's desired value
 			newEnvs = append(newEnvs, corev1.EnvVar{
 				Name:  envVar.Name,
@@ -248,10 +250,11 @@ func patchEnvVarsForContainer(runtimeDetails *odigosv1.InstrumentedApplication,
 	// Step 2: add the new env vars which odigos might patch, but which are not defined in the manifest
 	if sdk != nil {
 		for envName, envValue := range observedEnvs {
-			desiredEnvValue := envOverwrite.GetPatchedEnvValue(envName, envValue, sdk, programmingLanguage)
+			annotationOriginalValue, annotationOriginalValueFound := annotationManifestEnv.GetContainerStoredEnvs(container.Name)[envName]
+			desiredEnvValue, annotationNewValue := envOverwrite.GetPatchedEnvValue(envName, envValue, sdk, programmingLanguage, nil, annotationOriginalValue, annotationOriginalValueFound)
 			if desiredEnvValue != nil {
 				// store that it was empty to begin with
-				manifestEnvOriginal.InsertOriginalValue(container.Name, envName, nil)
+				annotationManifestEnv.UpsertOriginalValue(container.Name, envName, annotationNewValue)
 				// and add this new env var to the manifest
 				newEnvs = append(newEnvs, corev1.EnvVar{
 					Name:  envName,

k8sutils/pkg/envoverwrite/origenv.go

@@ -3,6 +3,7 @@ package envoverwrite
 import (
 	"encoding/json"
 	"fmt"
+	"strings"
 
 	"github.com/odigos-io/odigos/common/consts"
 	"github.com/odigos-io/odigos/common/envOverwrite"
@@ -51,17 +52,23 @@ func (o *OrigWorkloadEnvValues) RemoveOriginalValue(containerName string, envNam
 	return nil, false
 }
 
-func (o *OrigWorkloadEnvValues) InsertOriginalValue(containerName string, envName string, val *string) {
+func (o *OrigWorkloadEnvValues) UpsertOriginalValue(containerName string, envName string, val *string) {
 	if _, ok := o.origManifestValues[containerName]; !ok {
 		o.origManifestValues[containerName] = make(envOverwrite.OriginalEnv)
 	}
-	if _, alreadyExists := o.origManifestValues[containerName][envName]; alreadyExists {
-		// we already have the original value for this env, will not update it
-		// TODO: should we update it if the value is different?
-		return
+
+	// make sure we are not recording any odigos values into the annotation
+	if val != nil {
+		if strings.Contains(*val, "/var/odigos") {
+			return
+		}
+	}
+
+	currentValue, found := o.origManifestValues[containerName][envName]
+	if !found || ((currentValue == nil) != (val == nil)) || (currentValue != nil && *currentValue != *val) {
+		o.origManifestValues[containerName][envName] = val
+		o.modifiedSinceCreated = true
 	}
-	o.origManifestValues[containerName][envName] = val
-	o.modifiedSinceCreated = true
 }
 
 // stores the original values back into the manifest annotations