Small workaround to allow istiod to not use values#19449
Small workaround to allow istiod to not use values#19449istio-testing merged 8 commits intoistio:masterfrom
Conversation
pkg/kube/inject/inject.go
Outdated
| // Migrating from values.yaml to mesh or env (for istiod - set using fromEnv instead of values) | ||
| // This allows an optional config map with vendor-specific settings/overrides - alternative is creating | ||
| // vendor-specific injection templates or a huge mess. | ||
| funcMap["value"] = func(key string, def string) string { |
There was a problem hiding this comment.
I don't get it. So in the template we have something like {{ value "foo.bar" "" }} and it will lookup the environment variable foo_bar if it exists, and if not it will get .global.foo.bar?
Why not just do something like {{ valueOrDefault (getEnv "PILOT_FOO_BAR") .Values.global.foo.bar }}? its much more explicit I think
There was a problem hiding this comment.
That will make other problems below go away
There was a problem hiding this comment.
We don't have getEnv - if I can add getEnv instead it's great - it was the first implementation :-)
But knowing people are sensitive to env variables - I tried to not expose them directly.
There was a problem hiding this comment.
Seems more desired to just explicitly add an getEnv function rather than to create one, mask it under a name value that does it anyways but is less flexible and doesn't work in a lot of cases?
There was a problem hiding this comment.
renamed to {{ env }}
pkg/kube/inject/inject.go
Outdated
| // Special case for 'Values.global' - where most settings are | ||
| if !strings.Contains(key, ".") { | ||
| global := values["global"] | ||
| if globalM, ok := global.(map[string]interface{}); ok { |
There was a problem hiding this comment.
this won't work if you have a nested value like foo.bar, right?
There was a problem hiding this comment.
Well, it will work if I find the code used by galley/cobra/etc - having env variables override cli and yaml is already supported somewhere.
Again - it's not intended to be the final solution, istio/api changes is the right path and happy to remove this code as soon as we get the needed changes.
There was a problem hiding this comment.
After rename - this is gone.
Also: the istio/api change adds 'ProxyConfig.env' - after it is merged I'll start using it directly, with same semantics.
pkg/kube/inject/inject.go
Outdated
| if globalM, ok := global.(map[string]interface{}); ok { | ||
| sval := globalM[key] | ||
| if sval != nil { | ||
| if val, ok := sval.(string); ok { |
There was a problem hiding this comment.
This is not intended as a generic jsonpatch reinvention - just a way to not have to keep values.yaml around for injector. I'll add whatever is needed - so far I replaced only strings.
There was a problem hiding this comment.
We don't need to reinvent anything with {{ valueOrDefault (getEnv "PILOT_FOO_BAR") .Values.global.foo.bar }} other than added the env function though?
There was a problem hiding this comment.
Well - the point is to stop using .Values.
But it doesn't matter - since the api will be 'env' map in proxy config, that's the way to go.
This PR is just a stop gap until api is agreed.
| // Allows the template to use env variables from istiod. | ||
| // Istiod will use a custom template, without 'values.yaml', and the pod will have | ||
| // an optional 'vendor' configmap where additional settings can be defined. | ||
| funcMap["env"] = func(key string, def string) string { |
There was a problem hiding this comment.
default is not used?
Also nit, this can probably be inline in the funcMap declaration?
There was a problem hiding this comment.
It is used now.
There is nothing else in the funcMap declaration that inlines stuff - consistency.
| conIDresourceNamePrefix, discReq.Node.Id, err) | ||
| return err | ||
| } | ||
| sdsServiceLog.Infoa("Pushed secret for ", discReq) |
There was a problem hiding this comment.
Might want to make these in another PR if you want only my approval to merge this
|
PTAL |
| // Istiod will use a custom template, without 'values.yaml', and the pod will have | ||
| // an optional 'vendor' configmap where additional settings can be defined. | ||
| funcMap["env"] = func(key string, def string) string { | ||
| val := os.Getenv(key) |
There was a problem hiding this comment.
Do we ever want to set an env var to "" explicitly? if so, we should use lookup env which can distinguish unset and ""
There was a problem hiding this comment.
When we find a use case - we could. But by that time - the mesh API will be in place, and we can replace this code with the equivalent ( we'll still need injector to process the 'vendor' envs, but
will be placed on overrides of mesh.proxyConfig.env )
|
/retest |
|
/test unit-tests_istio |
Follow up to istiod install, this allows migrating the majority of 'values.yaml' settings from injector.
Note that istio/api#1194 will add the inject env to mesh.yaml - and after it is
merged I'll adjust this accordingly ( with the agreed name, and merging the env from mesh.yaml)
We will still need to support env overrides - istiod allows using a k8s configmap with 'vendor' specific settings, created for example when the cluster is created.
The final plan is to use only mesh.yaml and merge 'vendor/cluster' mesh.yaml with 'install' defaults and user overrides.
But on the short term - this PR allows progress on cleaning up the injection template and removing
the dep on values.yaml.
[ ] Configuration Infrastructure
[ ] Docs
[ ] Installation
[*] Networking
[ ] Performance and Scalability
[ ] Policies and Telemetry
[ ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure