From 3cc4367322a16de91fc1cd2d6b663faaa148b778 Mon Sep 17 00:00:00 2001 From: Amir Blum Date: Fri, 6 Dec 2024 14:12:48 +0200 Subject: [PATCH 1/3] fix: correctly use changes in environment in manifest --- common/envOverwrite/overwriter.go | 50 +++++++++++++++---- common/envOverwrite/overwriter_test.go | 2 +- .../instrumentation/instrumentation.go | 17 ++++--- k8sutils/pkg/envoverwrite/origenv.go | 21 +++++--- 4 files changed, 64 insertions(+), 26 deletions(-) diff --git a/common/envOverwrite/overwriter.go b/common/envOverwrite/overwriter.go index 14e6206310..3257e9ddd2 100644 --- a/common/envOverwrite/overwriter.go +++ b/common/envOverwrite/overwriter.go @@ -71,34 +71,62 @@ 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 { + if currentContainerManifestValue == nil { + return &desiredOdigosPart, nil + } + + 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 { + 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 @@ -106,7 +134,7 @@ func GetPatchedEnvValue(envName string, observedValue string, currentSdk *common // 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 +163,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 +178,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 } } diff --git a/common/envOverwrite/overwriter_test.go b/common/envOverwrite/overwriter_test.go index fcc2d97df3..498e4a805b 100644 --- a/common/envOverwrite/overwriter_test.go +++ b/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 { diff --git a/instrumentor/instrumentation/instrumentation.go b/instrumentor/instrumentation/instrumentation.go index 94b6803bf5..298598b0dd 100644 --- a/instrumentor/instrumentation/instrumentation.go +++ b/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, ¤tContainerManifestValue, 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, diff --git a/k8sutils/pkg/envoverwrite/origenv.go b/k8sutils/pkg/envoverwrite/origenv.go index 32369d13f1..396d5ebac3 100644 --- a/k8sutils/pkg/envoverwrite/origenv.go +++ b/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 := o.origManifestValues[containerName][envName] + if ((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 From a9d05b04486135bdf88d9a857881cd6582705903 Mon Sep 17 00:00:00 2001 From: Amir Blum Date: Fri, 6 Dec 2024 15:12:10 +0200 Subject: [PATCH 2/3] fix: handle values in dockerfile --- common/envOverwrite/overwriter.go | 14 ++++++++++++-- k8sutils/pkg/envoverwrite/origenv.go | 4 ++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/common/envOverwrite/overwriter.go b/common/envOverwrite/overwriter.go index 3257e9ddd2..3b4bcac378 100644 --- a/common/envOverwrite/overwriter.go +++ b/common/envOverwrite/overwriter.go @@ -100,6 +100,13 @@ func GetPatchedEnvValue(envName string, observedValue string, currentSdk *common 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 + desiredFullValue := observedValue + envMetadata.delim + desiredOdigosPart + if desiredFullValue == *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. @@ -117,8 +124,11 @@ func GetPatchedEnvValue(envName string, observedValue string, currentSdk *common } else { // else means there is no annotation on the workload. if currentContainerManifestValue != nil { - userNewValAndOdigos := *currentContainerManifestValue + envMetadata.delim + desiredOdigosPart - return &userNewValAndOdigos, currentContainerManifestValue + currentManifestContainsOdigosValue := strings.Contains(*currentContainerManifestValue, desiredOdigosPart) + if !currentManifestContainsOdigosValue { + userNewValAndOdigos := *currentContainerManifestValue + envMetadata.delim + desiredOdigosPart + return &userNewValAndOdigos, currentContainerManifestValue + } } } diff --git a/k8sutils/pkg/envoverwrite/origenv.go b/k8sutils/pkg/envoverwrite/origenv.go index 396d5ebac3..e200551d56 100644 --- a/k8sutils/pkg/envoverwrite/origenv.go +++ b/k8sutils/pkg/envoverwrite/origenv.go @@ -64,8 +64,8 @@ func (o *OrigWorkloadEnvValues) UpsertOriginalValue(containerName string, envNam } } - currentValue := o.origManifestValues[containerName][envName] - if ((currentValue == nil) != (val == nil)) || (currentValue != nil && *currentValue != *val) { + currentValue, found := o.origManifestValues[containerName][envName] + if !found || ((currentValue == nil) != (val == nil)) || (currentValue != nil && *currentValue != *val) { o.origManifestValues[containerName][envName] = val o.modifiedSinceCreated = true } From a50adef4814f1d637916c0e05f17c5d2a8eb46a3 Mon Sep 17 00:00:00 2001 From: Amir Blum Date: Fri, 6 Dec 2024 15:26:33 +0200 Subject: [PATCH 3/3] fix: handle delete of docker env value --- common/envOverwrite/overwriter.go | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/common/envOverwrite/overwriter.go b/common/envOverwrite/overwriter.go index 3b4bcac378..87237cc2f2 100644 --- a/common/envOverwrite/overwriter.go +++ b/common/envOverwrite/overwriter.go @@ -96,14 +96,24 @@ func GetPatchedEnvValue(envName string, observedValue string, currentSdk *common // 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 { - return &desiredOdigosPart, 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 - desiredFullValue := observedValue + envMetadata.delim + desiredOdigosPart - if desiredFullValue == *currentContainerManifestValue { + if desiredFullValueWithObserved == *currentContainerManifestValue { return currentContainerManifestValue, annotationOriginalValue }