-
Notifications
You must be signed in to change notification settings - Fork 326
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: move server config from command flags to configmap #1127
Conversation
We don't need to join individual server instances as the headless service DNS will resolve to all the server pod IPs, and consul will use that to join the other agents.
e8687b2
to
5be3b40
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! What is the glue that binds these config maps to where consul reads them? Does the config-checksum annotation lay these out similar to having .json files in the -config-dir?
{{- range $index := until (.Values.server.replicas | int) }} | ||
-retry-join="${CONSUL_FULLNAME}-server-{{ $index }}.${CONSUL_FULLNAME}-server.${NAMESPACE}.svc:{{ $serverSerfLANPort }}" \ | ||
{{- end }} | ||
-retry-join="${CONSUL_FULLNAME}-server.${NAMESPACE}.svc:{{ .Values.server.ports.serflan.port }}" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
Configmap is mounted as a volume to pods (similar to secrets): consul-k8s/charts/consul/templates/server-statefulset.yaml Lines 130 to 132 in a713f85
consul-k8s/charts/consul/templates/server-statefulset.yaml Lines 363 to 364 in a713f85
All files from the configmap will be in that
Then we pass the entire config dir to consul via
Consul will consume all files in alphabetical order and merge them all into one configuration.
The config-checksum is there to detect that update server statefulset when configmap changes. For example, let's say you had an installation without TLS and now you want to enable TLS. Without this annotation, if you run With an annotation, when run helm upgrade, the checksum of the configmap will change and so the helm upgrade will need to update the checksum annotation on your server statefulset pods, which will trigger a restart of the pods. |
@ishustava Thank you. This is cool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! The commit by commit readability made it so nice to review!
Changes proposed in this PR:
Move as much of the server config into config map as possible. Things that could not be moved into config maps are flags that contain env variables and that are being scripted inside the command (e.g. DNS
recursors
).The motivation for this change is to make the configuration more readable and consolidate it in one place as much as possible. For historical context, we were not doing that from the beginning because when you update the config map, the server stateful set would not be automatically updated and so we were using command flags instead to ensure that servers will be restarted. However, this has changed with hashicorp/consul-helm#550, but we never went back and updated it.
This change could be easier to read via commits:
Note: the client configmap and flags will be potentially updated in a future PR
How I've tested this PR:
How I expect reviewers to test this PR:
👀
Checklist: