.NET Intrumentation - validate all env. vars. before start injecting env. vars.

[Kielek]

I think this behavior and the log message are misleading.

When false is returned then it interrupts processing next env vars.
It could leave the container in a "semi-instrumented" state. Is it expected?

This comment applies also to the code BEFORE the changes introduced in this PR. Therefore, it would be better to make a separate PR to address it.

Originally posted by @pellared in #1081 (comment)

For now, .NET injecting process is validating env. variables and injecting it one by one, instead of make the whole validation upfront.

The goal is to validate first and inject env. variables only when needed.

[avadhut123pisal]

@Kielek In case of .NET, during the validation phase, environment variables are set to the copy of container object and at last actual container object is updated.

pod.Spec.Containers[index] = container

In case of other language instrumentation (Java, Python, NodeJS), the problem described here do exists.
I have raised the PR to fix that. Along with that, I have added a check for all language instrumentation, to skip the env var injection and OTEL SDK configuration, if the first step related to language SDK injection fails.

Please let me know, if this is ok or some other cases needs to be handled.

[pellared]

@avadhut123pisal It is only a "shallow copy" and the "original" container env vars can be still modified here:

opentelemetry-operator/pkg/instrumentation/dotnet.go

Line 133 in 10404eb

container.Env[idx].Value = fmt.Sprintf("%s:%s", container.Env[idx].Value, envVarValue)

This is how I think it could be fixed:

1. ALL validation should be done before processing. This means that container.Env[idx].ValueFrom != nil for all env vars should be checked before setting anything.
2. The manipulated object should be deep copied before processing. This means that at least container.Env would have to be "deep cloned" before it is processed and validated on the fly.

I would prefer to do validation before manipulation as it would result in a more performant code. It can be more readable after refactoring.

[avadhut123pisal]

@pellared I got your point. I will update the PR accordingly.

[Kielek]

@avadhut123pisal, I agree with @pellared, I have one more comment, in your draft PR.

[avadhut123pisal]

• ALL validation should be done before processing. This means that container.Env[idx].ValueFrom != nil for all env vars should be checked before setting anything.

@pellared Looking at the existing implementation, it seems that we need to validate only those env vars, whose value needs to be concatenated with the value defined in Operator code, for eg. JAVA_TOOL_OPTIONS, NODE_OPTIONS. Other env vars can have their value populated using valueFrom syntax.

@pellared @Kielek Can you please have a look at the PR ?