[stable/consul] Provide advanced configuration options for consul #1269
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
@mitom Thanks for the contrib. I'll review as soon as I can. If anyone else is available to review that would be great too. Thanks |
|
A thing I discovered is that due to the fact that these are mounted as volumes and kubernetes won't allow the volume specification to change for pods in a statefulset after creation, the list can only be updated by re-creating the statefulset. This would lead to #1143 as all containers would attempt to leave the cluster and would also cause downtime. The contents of each item in the list can be changed easily and the pods reloaded/roll-restarted. |
unguiculus
added
the
needs-rebase
stable/consul/README.md
Outdated
| @@ -32,6 +32,8 @@ The following tables lists the configurable parameters of the consul chart and t | |||
| | `ImagePullPolicy` | Container pull policy | `Always` | | |||
| | `Replicas` | k8s statefulset replicas | `3` | | |||
| | `Component` | k8s selector key | `consul` | | |||
| | `ConsulConfig.[].type` | either `secret` or `configMap` | - | | |||
There was a problem hiding this comment.
I don't like the syntax you used. This is confusing. May just add ConsulConfig and add an example to values.yaml.
stable/consul/values.yaml
Outdated
| @@ -45,6 +45,8 @@ uiService: | |||
| enabled: true | |||
| type: "NodePort" | |||
|
|
|||
| ConsulConfig: [] | |||
|
|
|||
There was a problem hiding this comment.
Add example.
ConsulConfig: []
# - type: secret
# name: consul-defaults
# - type: configMap
# name: consul-defaults
|
ping @lachie83 |
|
I agree with your review @unguiculus |
mitom
force-pushed
the
consul_extended_config
branch
from
83140e1 to
63d81f3
Compare
|
I have made the changes you requested and also rebased the branch. |
|
ping @lachie83 |
k8s-ci-robot
added
the
size/M
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
|
/lgtm |
k8s-ci-robot
added
the
approved
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mitom, unguiculus The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@mitom Any chance you can fix the conflicts? |
|
@unguiculus conflicts have been fixed in this PR #4765 by @timstoop |
|
We can now close this PR since #4765 have been merged. |
As per #1198 this adds another option to configure consul, allowing to pass in configuration by both
configMaps andsecrets. It would be fairly trivial to extend this to any other type that kubernetes can mount as a volume (like hostpath) if the list grows, or if there is a usecase.It also allows to re-use parts of the configuration in case of multiple clusters as these can be separated into multiple objects.
There is one drawback, the new
ConsulConfigoption can't be set using helm's--set. The reason being--setcan't set list of hashes at the moment (AFAIK). The suggested approach is to just use a hash, however in this scenario it could be possible to have asecretandconfigMapnamed the same and therefore a hash wouldn't work. The attribute could be separated into 2, which would solve the--setissue as well, however this would degrade functionality as the order in which configuration is loaded could not be influenced per type, not globally (without post-processing the files on the container).There are no breaking changes or degradations.