-
Notifications
You must be signed in to change notification settings - Fork 196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: env overwriter #1297
refactor: env overwriter #1297
Changes from 2 commits
3d4c249
89d46f5
c3bdb00
8a41ee9
42061f8
ff9a6b7
eab6f63
f84284b
360750b
8628107
cfef922
576daec
2aea966
3ae28df
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
package envOverwrite | ||
|
||
// this type is used to store the original values of the environment variables the user has set | ||
// for a specific deployment manifest. | ||
// In k8s we need to store such map for each container in the pod. | ||
// In systemd, this can store the original values of the systemd service. | ||
// | ||
// When we want to rollback the changes we made to the environment variables, we can fetch the original | ||
// values from this map and set them back to the manifest. | ||
// | ||
// The key is the environment variable name. | ||
// The value is either the original value of the environment variable or nil if the environment variable | ||
// was not set by the user. | ||
type OriginalEnv map[string]*string | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,9 +7,10 @@ import ( | |
) | ||
|
||
type envValues struct { | ||
delim string | ||
delim string | ||
values map[common.OtelSdk]string | ||
} | ||
|
||
// EnvValuesMap is a map of environment variables odigos uses for various languages and goals. | ||
// The key is the environment variable name and the value is the value to be set or appended | ||
// to the environment variable. We need to make sure that in case any of these environment | ||
|
@@ -22,67 +23,93 @@ var EnvValuesMap = map[string]envValues{ | |
delim: " ", | ||
values: map[common.OtelSdk]string{ | ||
common.OtelSdkNativeCommunity: "--require /var/odigos/nodejs/autoinstrumentation.js", | ||
common.OtelSdkEbpfEnterprise: "--require /var/odigos/nodejs-ebpf/autoinstrumentation.js", | ||
common.OtelSdkEbpfEnterprise: "--require /var/odigos/nodejs-ebpf/autoinstrumentation.js", | ||
}, | ||
}, | ||
"PYTHONPATH": { | ||
delim: ":", | ||
values: map[common.OtelSdk]string{ | ||
common.OtelSdkNativeCommunity: "/var/odigos/python:/var/odigos/python/opentelemetry/instrumentation/auto_instrumentation", | ||
common.OtelSdkEbpfEnterprise: "/var/odigos/python-ebpf:/var/odigos/python/opentelemetry/instrumentation/auto_instrumentation:/var/odigos/python", | ||
common.OtelSdkEbpfEnterprise: "/var/odigos/python-ebpf:/var/odigos/python/opentelemetry/instrumentation/auto_instrumentation:/var/odigos/python", | ||
}, | ||
}, | ||
"JAVA_OPTS": { | ||
delim: " ", | ||
values: map[common.OtelSdk]string{ | ||
common.OtelSdkNativeCommunity: "-javaagent:/var/odigos/java/javaagent.jar", | ||
common.OtelSdkEbpfEnterprise: "-javaagent:/var/odigos/java-ebpf/dtrace-injector.jar", | ||
common.OtelSdkEbpfEnterprise: "-javaagent:/var/odigos/java-ebpf/dtrace-injector.jar", | ||
common.OtelSdkNativeEnterprise: "-javaagent:/var/odigos/java-ext-ebpf/javaagent.jar " + | ||
"-Dotel.javaagent.extensions=/var/odigos/java-ext-ebpf/otel_agent_extension.jar", | ||
"-Dotel.javaagent.extensions=/var/odigos/java-ext-ebpf/otel_agent_extension.jar", | ||
}, | ||
}, | ||
"JAVA_TOOL_OPTIONS": { | ||
delim: " ", | ||
values: map[common.OtelSdk]string{ | ||
common.OtelSdkNativeCommunity: "-javaagent:/var/odigos/java/javaagent.jar", | ||
common.OtelSdkEbpfEnterprise: "-javaagent:/var/odigos/java-ebpf/dtrace-injector.jar", | ||
common.OtelSdkEbpfEnterprise: "-javaagent:/var/odigos/java-ebpf/dtrace-injector.jar", | ||
common.OtelSdkNativeEnterprise: "-javaagent:/var/odigos/java-ext-ebpf/javaagent.jar " + | ||
"-Dotel.javaagent.extensions=/var/odigos/java-ext-ebpf/otel_agent_extension.jar", | ||
"-Dotel.javaagent.extensions=/var/odigos/java-ext-ebpf/otel_agent_extension.jar", | ||
}, | ||
}, | ||
} | ||
|
||
func ShouldPatch(envName string, observedValue string, sdk common.OtelSdk) bool { | ||
env, ok := EnvValuesMap[envName] | ||
// 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. | ||
// either one can be set or empty. | ||
// so we have 4 cases to handle: | ||
func GetPatchedEnvValue(envName string, observedValue string, currentSdk common.OtelSdk) *string { | ||
envMetadata, ok := EnvValuesMap[envName] | ||
if !ok { | ||
// Odigos does not manipulate this environment variable, so ignore it | ||
return false | ||
return nil | ||
} | ||
|
||
desiredValue, ok := env.values[sdk] | ||
desiredOdigosPart, ok := envMetadata.values[currentSdk] | ||
if !ok { | ||
// No specific overwrite is required for this SDK | ||
return false | ||
return nil | ||
} | ||
|
||
if strings.Contains(observedValue, desiredValue) { | ||
// if the observed value contains the value odigos sets for this SDK, | ||
// that means that either: | ||
// 1. the user does not add any additional values and we see here our own value only, | ||
// 2. we already patched the value in the deployment manifest and we see here the patched value, | ||
// so we should not patch it | ||
return false | ||
// 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 | ||
} | ||
|
||
// if we are moving from one SDK to another, avoid patching the value | ||
// it there is no user defined value (observedValue is equal to the value odigos sets for the previous SDK) | ||
for _, v := range env.values { | ||
if v == observedValue { | ||
return false | ||
// 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 true | ||
// 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. | ||
for _, sdkEnvValue := range envMetadata.values { | ||
if strings.Contains(observedValue, sdkEnvValue) { | ||
if sdkEnvValue == desiredOdigosPart { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the start we do We are also checking for equality in scenario 2, so I'm not sure about this part There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This part is in This if will be untrue when:
|
||
// shortcut, the value is already patched | ||
// both the odigos part equals to the new value, and the user part we want to keep | ||
return &observedValue | ||
} else { | ||
// The environment variable is patched by some other odigos sdk. | ||
// replace just the odigos part with the new desired value. | ||
blumamir marked this conversation as resolved.
Show resolved
Hide resolved
|
||
patchedEvnValue := strings.ReplaceAll(observedValue, sdkEnvValue, desiredOdigosPart) | ||
return &patchedEvnValue | ||
} | ||
} | ||
} | ||
|
||
// 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 | ||
} | ||
|
||
func ShouldRevert(envName string, observedValue string) bool { | ||
|
@@ -103,38 +130,6 @@ func ShouldRevert(envName string, observedValue string) bool { | |
return false | ||
} | ||
|
||
func Patch(envName string, currentVal string, sdk common.OtelSdk) string { | ||
env, exists := EnvValuesMap[envName] | ||
if !exists { | ||
return "" | ||
} | ||
|
||
valToAppend, ok := env.values[sdk] | ||
if !ok { | ||
return "" | ||
} | ||
|
||
if currentVal == "" { | ||
return valToAppend | ||
} | ||
|
||
if strings.Contains(currentVal, valToAppend) { | ||
// The environment variable is already patched with the correct value for this SDK | ||
return currentVal | ||
} | ||
|
||
for sdkKey, val := range env.values { | ||
if sdkKey != sdk && strings.Contains(currentVal, val){ | ||
// The environment variable is patched by another SDK | ||
// but we need to append our value to it, so replace the | ||
// value of the other SDK with ours | ||
return strings.ReplaceAll(currentVal, val, valToAppend) | ||
} | ||
} | ||
|
||
return currentVal + env.delim + valToAppend | ||
} | ||
|
||
func ValToAppend(envName string, sdk common.OtelSdk) (string, bool) { | ||
env, exists := EnvValuesMap[envName] | ||
if !exists { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the env var was not set by the user I think we should not create an entry for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We create an entry with
nil
so that we know to delete it when reverting. Otherwise it's not straight forward to know if an arbitrary environment variable should be deleted from manifest (injected by odigos) or if it was there to begin with and should be untouched.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We used the annotation for that, right? If an env var is in the map it means we appended to its value in the manifest, if it's not it is not taken from the manifest.
This nil approach will add a lot of odigos data on deployments and I don't think it is required.