Fix etcd not starting up when using a custom access address

[derselbst]

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 and etcd_access_address. When configuring etcd to run in a DHCP network (nodes with dynamic IPs), it might be desirable to set

etcd_address: "0.0.0.0" # i.e. bind etcd to any address
etcd_access_address: "{{ ansible_fqdn }}" # access etcd via the host's FQDN

Currently, 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

Fix etcd not starting up when using a custom access address

[k8s-ci-robot]

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

[MrFreezeex]

Hi thanks for the PR, this makes sense to me however could you revert the change you did to switch from using the host var to ansible_hostname AFAICS those two can be different as one represent the name in the inventory and the other one is the hostname configured on the host IIUC. Otherwise lgtm, thanks!

/ok-to-test

[derselbst]

Right, previously, host was expanded to inventory_hostname. This can be any name, esp. it can be a different name than the true hostname. IMO, it doesn't make sense to put this arbitrary inventory_hostname into the SAN section of the certificate, which is why I believe using ansible_hostname is the correct approach here.

The compromise to keep old and new behavior, i.e.

DNS.{{ counter["dns"] }} = {{ host }}{{ increment(counter, 'dns') }}
DNS.{{ counter["dns"] }} = {{ hostvars[host]['ansible_hostname'] }}{{ increment(counter, 'dns') }}
DNS.{{ counter["dns"] }} = {{ hostvars[host]['ansible_fqdn'] }}{{ increment(counter, 'dns') }}

could result in duplicate entries, if inventory_hostname == ansible_hostname, and I'm not sure if that wouldn't cause issues.

Alternatively, I could completely remove the line
DNS.{{ counter["dns"] }} = {{ hostvars[host]['ansible_hostname'] }}{{ increment(counter, 'dns') }}

which means that it won't work when etcd_access_address is set to ansible_hostname AND inventory_hostname != ansible_hostname.

What's your suggestion?

[derselbst]

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.

[MrFreezeex]

Right, previously, host was expanded to inventory_hostname. This can be any name, esp. it can be a different name than the true hostname. IMO, it doesn't make sense to put this arbitrary inventory_hostname into the SAN section of the certificate, which is why I believe using ansible_hostname is the correct approach here.

FYI I am not entirely sure the condition is triggered but kubespray could populates /etc/hosts with that in some case with this code:

kubespray/roles/kubernetes/preinstall/tasks/0090-etchosts.yml

Line 8 in 5f35b66

{%- if ('ansible_hostname' in hostvars[item] and item != hostvars[item]['ansible_hostname']) %} {{ hostvars[item]['ansible_hostname'] }}.{{ dns_domain }} {{ hostvars[item]['ansible_hostname'] }} {% else %} {{ item }}.{{ dns_domain }} {{ item }} {% endif %}

The compromise to keep old and new behavior, i.e.

DNS.{{ counter["dns"] }} = {{ host }}{{ increment(counter, 'dns') }}
DNS.{{ counter["dns"] }} = {{ hostvars[host]['ansible_hostname'] }}{{ increment(counter, 'dns') }}
DNS.{{ counter["dns"] }} = {{ hostvars[host]['ansible_fqdn'] }}{{ increment(counter, 'dns') }}

could result in duplicate entries, if inventory_hostname == ansible_hostname, and I'm not sure if that wouldn't cause issues.

Alternatively, I could completely remove the line DNS.{{ counter["dns"] }} = {{ hostvars[host]['ansible_hostname'] }}{{ increment(counter, 'dns') }}

which means that it won't work when etcd_access_address is set to ansible_hostname AND inventory_hostname != ansible_hostname.

What's your suggestion?

You could either revert the change on inventory_hostname/ansible_hostname which would not change the current behavior or adds both if the two are different with an if condition IMO.

[derselbst]

FYI I am not entirely sure the condition is triggered but kubespray could populates /etc/hosts with that in some case with this code:

The code you have linked here will always end up writing the ansible_hostname to the /etc/hosts, no matter what condition becomes true or false:

1. if ('ansible_hostname' in hostvars[item] - I can hardly think of a case where ansible was unable to discover the host's name, so I consider this always to be true
2. item != hostvars[item]
   a. If evaluates to true, it writes the ansible_hostname
   b. If evaluates to false, it still writes ansible_hostname because that's just the identical to what item is set to.

My assumption in 1. will only fail, if facts gathering has been disabled. Then, using inventory_hostname will truely be different to using ansible_hostname, as the latter is undefined. If that's your argument, please give me a final "you insist", and I will add extra guards for these ansible_* variables to be defined and ansible_hostname to be different to host as you suggested. (IMO, that would make the logic unnecessarily complex and silently result in an incompletely populated etcd certificate when facts gathering has been disabled for whatever reason).

[MrFreezeex]

Ok so yes indeed this condition in the code that populate /etc/hosts looks to be not very useful as we gather facts before executing preinstall. Also the apiserver cert is also using ansible_hostname so sounds good to go as you are suggesting, thanks for digging into this 👍.

Two things though:

• Could you highlight this switch/cleanup in the changelog just in case someone use inventory hostname somehow?
• Could you update the SANs with ansible_hostname + ansible_fqdn of etcd deployed through kubeadm so that both deployment methods have somewhat similar behaviors:

  kubespray/roles/kubernetes/control-plane/templates/kubeadm-config.v1beta3.yaml.j2

  Lines 77 to 90 in 5f35b66

  	serverCertSANs:
  	{% for san in etcd_cert_alt_names %}
  	- "{{ san }}"
  	{% endfor %}
  	{% for san in etcd_cert_alt_ips %}
  	- "{{ san }}"
  	{% endfor %}
  	peerCertSANs:
  	{% for san in etcd_cert_alt_names %}
  	- "{{ san }}"
  	{% endfor %}
  	{% for san in etcd_cert_alt_ips %}
  	- "{{ san }}"
  	{% endfor %}

Thanks again!

[derselbst]

Could you update the SANs with ansible_hostname + ansible_fqdn of etcd deployed through kubeadm

I wasn't aware that this deployment method exists. I looked into that. It is currently not needed to update the SANs here, because etcd_access_addresses and etcd_address are not propagated when this deployment method is used. Because of this, the generated kubeadm config will look like this:

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 host deployment method.

Yet, I find the kubeadm deployment method interesting. I'll probably adopt it for my next deployment and when doing so, I'll have to make the two mentioned settings propagate correctly. So it's likely that I'll file a follow-up PR in a few weeks or so.

Could you highlight this switch/cleanup in the changelog just in case someone use inventory hostname somehow?

During the above exercise I found that a cleaner approach is to include the etcd_access_address of the groups[etcd] member nodes in that SAN section, rather than hardcoding ansible_fqdn and friends.

I have restored the previous line DNS.{{ counter["dns"] }} = {{ host }}{{ increment(counter, 'dns') }} - yet I still don't think this is necessary.

[MrFreezeex]

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?

[derselbst]

Yeah, I saw failing tests complaining about it not being defined as well. I've added a guard.

[MrFreezeex]

Thanks!
/lgtm

[cyclinder]

thanks!

/lgtm

[yankay]

Thanks @derselbst
/approve

[k8s-ci-robot]

[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:

• OWNERS [yankay]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment