(#5522) Enhance environment trait to include values from secrets/conf… #5639
Conversation
|
|
There was a problem hiding this comment.
Thanks for the work. However, I think this is going too deep in introspecting the value provided by the user. The operator should not peek at any config or secret for a matter of sensitive data. In fact, the goal of the feature is to let the user only provide the configuration and the goal of the operator should be syntactic sugar, transforming the trait parameter into the env container format. We should not worry if the user has provided a missing value, this should be an error that the rest of kubernetes logic should worry about.
Also, it would be better to have some unit test and increase coverage. E2E are fine, but, they must be in conjunction with unit test.
| @@ -29,6 +29,7 @@ type EnvironmentTrait struct { | |||
| HTTPProxy *bool `property:"http-proxy" json:"httpProxy,omitempty"` | |||
| // A list of environment variables to be added to the integration container. | |||
| // The syntax is KEY=VALUE, e.g., `MY_VAR="my value"`. | |||
| // The value may also be a reference to a configmap or secret, e.g. `MY_VAR=configmap:my-cm/my-cm-key` | |||
There was a problem hiding this comment.
You need to perform make generate to regen the spec and documentation.
There was a problem hiding this comment.
I unfortunately get an NPE when running this on my Mac as well as on some Linux server
There was a problem hiding this comment.
Okey, we have an auto regen in the nightly so it will be eventually sync-ed, but you should report this to get it sorted.
Of course, I should have thought about that and read the issue description properly. I'll have something alternative to look at later today. |
|
|
tdiesler
force-pushed
the
ghi5522
branch
2 times, most recently
from
88509c0
to
9f64364
Compare
| @@ -29,6 +29,7 @@ type EnvironmentTrait struct { | |||
| HTTPProxy *bool `property:"http-proxy" json:"httpProxy,omitempty"` | |||
| // A list of environment variables to be added to the integration container. | |||
| // The syntax is KEY=VALUE, e.g., `MY_VAR="my value"`. | |||
| // The value may also be a reference to a configmap or secret, e.g. `MY_VAR=configmap:my-cm/my-cm-key` | |||
There was a problem hiding this comment.
Okey, we have an auto regen in the nightly so it will be eventually sync-ed, but you should report this to get it sorted.
|
|
||
| func setValFromConfigMapKeySelector(vars *[]corev1.EnvVar, envName string, path string) error { | ||
| vs, err := v1.DecodeValueSource(path, "", "invalid configmap reference: "+path) |
There was a problem hiding this comment.
Sorry, I only gave you the link to the struct without explaining how I was thinking to use it. If you see, the object is already in charge to decode a string of type configmap|secret:my-val. The idea is to leverage such an abstraction by passing the user value without worrying about the type. Then, we can iterate over such objects and again, just setting corev1.ConfigMapKeySelector if such value is not nil, or corev1.SecretMapKeySelector accordingly. This would reduce the need to have these setValFromXYZKeySelector funcs
There was a problem hiding this comment.
Can this really be optimised without loosing specific error messages and the override of potentially existing EnvVarSource for a given key? I don't really understand why the if envVar := envvar.Get(*vars, envName); envVar != nil check is necessary. Should we get rid of that?
These setValFromXYZKeySelector funcs are mainly there for better readability. I could put the code in the above switch but there isn't any redundant duplication here afaict.
There was a problem hiding this comment.
You don't need to provide specialized error messages. This can be much more compacted (hence more maintainable and less error prone) by:
- If not configmap/secret, just do the normal logic
- if configmap/secret, decode the parameter (note that it returns an error we can bubble up, the operator would know what to do with it)
- if the decoded value is a configmap (check its decoded struct is not nil), add it as a configmap, otherwise add as a secret
I don't see the need to verify if the env var already exists either. If it exists by any chance, we are just setting it again.
tdiesler
force-pushed
the
ghi5522
branch
2 times, most recently
from
b8a3796
to
cf56225
Compare
|
Failed (unrelated) in ... |
|
Superseded by #5754 |
No description provided.