-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Fix etcd not starting up when using a custom access address #11388
Conversation
Hi @derselbst. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Hi thanks for the PR, this makes sense to me however could you revert the change you did to switch from using the /ok-to-test |
Right, previously, The compromise to keep old and new behavior, i.e.
could result in duplicate entries, if Alternatively, I could completely remove the line which means that it won't work when What's your suggestion? |
BTW, currently, kubespray's default behavior is to expand everything to IPs. I.e. most users will rely on the (static) IPs being listed in the SAN section for etcd to work correctly by default. |
FYI I am not entirely sure the condition is triggered but kubespray could populates
You could either revert the change on |
The code you have linked here will always end up writing the
My assumption in 1. will only fail, if facts gathering has been disabled. Then, using |
Ok so yes indeed this condition in the code that populate Two things though:
Thanks again! |
0adf426
to
e68ad47
Compare
I wasn't aware that this deployment method exists. I looked into that. It is currently not needed to update the SANs here, because kind: ClusterConfiguration
clusterName: cluster.local
etcd:
local:
imageRepository: "quay.io/coreos"
imageTag: "v3.5.10"
dataDir: "/var/lib/etcd"
extraArgs:
metrics: basic
election-timeout: "5000"
heartbeat-interval: "250"
auto-compaction-retention: "8"
snapshot-count: "10000"
serverCertSANs:
- etcd.kube-system.svc.cluster.local
- etcd.kube-system.svc
- etcd.kube-system
- etcd
peerCertSANs:
- etcd.kube-system.svc.cluster.local
- etcd.kube-system.svc
- etcd.kube-system
- etcd And the generated static pod manifest for the etcd pod will contain hardcoded IPs. This there is no way to override any bind addresses currently, hence it is not required to adjust the SAN for this deployment method, as it currently does not behave "similar" to the Yet, I find the
During the above exercise I found that a cleaner approach is to include the I have restored the previous line |
@@ -25,6 +25,9 @@ authorityKeyIdentifier=keyid:always,issuer | |||
[alt_names] | |||
DNS.1 = localhost | |||
{% for host in groups['etcd'] %} | |||
{# The address which etcd uses to access its members must be included in the SAN, otherwise etcd will fail with a TLS error upon startup. #} | |||
DNS.{{ counter["dns"] }} = {{ hostvars[host]['etcd_access_address'] }}{{ increment(counter, 'dns') }} |
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 sounds a good idea but this var is an IP by default so this won't work in the default case so maybe there should be a check similar to this not (hostvars[host]['etcd_access_address'] | ansible.utils.ipaddr) }}
? WDYT?
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.
Yeah, I saw failing tests complaining about it not being defined as well. I've added a guard.
e68ad47
to
2e8938f
Compare
2e8938f
to
02c7e8b
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.
Thanks!
/lgtm
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.
thanks!
/lgtm
Thanks @derselbst |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cyclinder, derselbst, MrFreezeex, yankay 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 |
What type of PR is this?
/kind bug
What this PR does / why we need it:
It is already possible to override the address of etcd via variables
etcd_address
andetcd_access_address
. When configuring etcd to run in a DHCP network (nodes with dynamic IPs), it might be desirable to setCurrently, this results in a TLS error, because the certificate of etcd only includes the pure hostname and IPs, but it doesn't include
etcd_access_address
as subject alternative name (SAN). Hence, when etcd attempts to communicate via a custom access address, the certificate is untrusted and etcd fails to start up.This PR fixes this, by including
etcd_access_address
as SAN.Which issue(s) this PR fixes:
None I'm aware of.
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
NONE