Allow the operator to be configured from a file #3570
Conversation
| If you use a combination of all or some of the methods listed above, the descending order of precedence in case of a conflict is as follows: | ||
|
|
||
| - Flag | ||
| - Environment variable |
There was a problem hiding this comment.
It is probably hard to change the precedence with viper, right? For OLM deployments the only customization option is via environment variables as far as I can tell from this https://github.com/operator-framework/operator-lifecycle-manager/blob/master/doc/design/subscription-config.md and it would be good if they took precedence, but I guess we could also just avoid flags in the OLM manifest?
There was a problem hiding this comment.
Unfortunately, Viper does not allow configuring the order of precedence. OLM is an interesting problem. As far as I can tell, they don't yet support config maps to be bundled with the CSV. However, they do seem to allow the user to mount volumes using the Subscription object. I haven't tested whether that actually works but assuming that it does, then I propose the following:
- We bundle a default config file in the operator container image at a well known path (say
/conf/eck.yaml). - In the CSV deployment section, we only set the
--configflag which will point to the default config file above.
With this approach, in addition to environment variables, the user has the option of using the Subscription to mount a config map at /conf that will shadow the default file bundled with the operator. That ought to take care of the configuration problem for OLM. WDYT?
There was a problem hiding this comment.
I think that sounds interesting. We could do that in a separate PR if you want.
There was a problem hiding this comment.
I created #3591 to track it.
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
nice improvement over the original 👍
|
As discussed during the team meeting, currently there is no effective way to cleanly stop the goroutines that are spawned in the code. Therefore, instead of trying to restart eveything in-process, the operator will simply exit and rely on an external mechanism (Statefulset/Pod controller) to restart it. |
|
Regarding |
Co-authored-by: Michael Morello <michael.morello@gmail.com>
|
The watch mechanism in Viper seems to be fairly well implemented (including accounting for config map updates) so I think I would go with |
|
Jenkins test this please |
Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com>
| ContainerRegistryFlag = "container-registry" | ||
| DebugHTTPListenFlag = "debug-http-listen" | ||
| DisableConfigWatch = "disable-config-watch" |
There was a problem hiding this comment.
nit: naming: config vs configuration, we are already using the latter in WebhookConfigurationNameFlag = "webhook-configuration-name"
There was a problem hiding this comment.
Thanks for pointing out the inconsistency. I don't think expanding config to configuration achieves much (there's no ambiguity) other than making the flags comically long. Since the webhook flag hasn't been released yet, I think I will rename that instead. Even Kubernetes uses ConfigMap instead of ConfigurationMap 😄.
| apiVersion: v1 | ||
| kind: ConfigMap | ||
| metadata: | ||
| name: {{ .Values.operator.name }} |
There was a problem hiding this comment.
> k get cm -n elastic-system
NAME DATA AGE
elastic-operator 1 5m
elastic-operator-uuid 1 19s
Should we call the ConfigMap elastic-operator-configuration to make its purpose more explicit ?
There was a problem hiding this comment.
I have seen quite a fair amount of applications simply using the name of the application and I think that convention makes sense. It is a config map after all so the purpose is obvious without the additional -configuration suffix. If there are strong opinions against that, I'd be happy to reconsider.
There was a problem hiding this comment.
It is a config map after all so the purpose is obvious
I'm not sure I would agree with that, we already have 2 ConfigMaps which are not really related to the configuration and we might have a third one for the election mechanism:
k get cm -n elastic-system
NAME DATA AGE
elastic-licensing 4 18h
elastic-operator-leader 0 18h
elastic-operator-uuid 1 18h
I would be curious what other people's thoughts are, but I don't want to block this PR.
There was a problem hiding this comment.
I see your point. I am probably building up the convention in my mind because I have been conditioned by it but that probably doesn't apply to everybody. I'll merge this for now and if it turns out to be confusing, we still have time to change it before the next release goes out.
Simply because I have it in hand, Istio config maps look like the following.
istio 2 63d
istio-ca-root-cert 1 63d
istio-leader 0 63d
istio-namespace-controller-election 0 63d
istio-security 1 63d
istio-sidecar-injector 2 63d
istio-validation-controller-election 0 63d
kiali 1 58d
prometheus 1 63d
I am not suggesting that this is a canonical example. However, I think it's quite obvious what each config map does. I reckon that ours would look similar.
Adds the ability to configure the operator from a file. This is a backward-compatible change that retains the existing configuration methods using flags and environment variables. Users can provide the configuration file using the
--configflag. If there are any conflicts, the order of precedence for resolving them is:This PR also includes:
all-in-one.yamlto use a ConfigMap for operator configurationFixes #3401