Skip to content

Commit

Permalink
more updates
Browse files Browse the repository at this point in the history
- reorg/update goals
- add results of NodeHostName investigation

- reference the original NodeAddresses-patching PR
- reference externalIPs-on-bare-metal PR
- reference externalIP-vs-internalIP discussion in another PR
  • Loading branch information
danwinship committed Apr 13, 2020
1 parent 24e57a1 commit ca95478
Showing 1 changed file with 67 additions and 19 deletions.
86 changes: 67 additions & 19 deletions keps/sig-node/1664-node-ips/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -169,40 +169,51 @@ cloud-controller-manager, this has no effect.
#### API cleanliness

`Node.Status.Addresses` is annotated with an incorrect strategic-merge
patch annotation, and so it is not possible to use strategic merge patch
to change that field without losing information. Although kubelet and
cloud-controller-manager have been fixed to work around the problem, it
is still annoying to have the incorrect annotation there, and it is
possible that other components might attempt to update the field
incorrectly in the future.
patch annotation, and so it is not possible to use strategic merge
patch to change that field without losing information. (See discussion
in [kubernetes #79391].) Although kubelet and cloud-controller-manager
have been fixed to work around the problem, it is still annoying to
have the incorrect annotation there, and it is possible that other
components might attempt to update the field incorrectly in the
future.

Additionally, `Node.Status.Addresses` is not currently sufficiently
checked by validation; it is possible for an external cloud provider
to declare an `InternalIP` or `ExternalIP` address that is not a
syntactically-valid IP address, meaning clients that read this field
may need to sanity-check it themselves ([kubernetes #89817]).

[kubernetes #79391](https://github.com/kubernetes/kubernetes/pull/79391)
[kubernetes #89817](https://github.com/kubernetes/kubernetes/pull/89817)

#### `NodeAddressType` semantics

The difference between `InternalIP` and `ExternalIP` node addresses is
not well-defined except in public clouds, and the two are used
inconsistently outside of that environment (though for the most part,
`ExternalIP` is used to mean "involves address rewriting from a public
IP to a private IP", and `InternalIP` is used for anything else).
IP to a private IP", and `InternalIP` is used for anything else). (eg,
See [kubernetes #86918] as an example of uncertainty about the right
type to use in a particular situation.)

[kubernetes #86918](https://github.com/kubernetes/kubernetes/pull/86918#discussion_r363997750)

### Goals

- Add `Node.Status.Hostname` and `Node.Status.IPs` and deprecate
`Node.Status.NodeAddresses`
`Node.Status.Addresses`

- Clarify the expectations of cloud providers and
cloud-controller-manager with respect to single-stack IPv6 and
dual-stack clusters.
- Ensure these fields are fully checked by validation code

- Do better error handling when, eg, trying to bring up a single-stack
IPv6 cluster on a cloud that only supports IPv4.
- Improve the communication between kubelet and
cloud-controller-manager to ensure CCM selects the proper node
addresses.

- Extend the bare-metal node address code to allow for dual-stack

- Clarify the expected behavior of `--node-ip` and extend it to handle
dual-stack clusters.

- Clarify the semantics of "internal" vs "external" node IPs and ensure
that all cloud providers are using the distinction correctly.
- Make the behavior of `--node-ip` consistent between bare-metal,
legacy cloud providers, and external cloud providers, and make it
possible to specify explicit dual-stack node IPs.

### Non-Goals

Expand All @@ -214,6 +225,10 @@ IP to a private IP", and `InternalIP` is used for anything else).
bare-metal nodes ([kubernetes #42125]). This is potentially distinct
from being able to set a dual-stack pair of `InternalIP` addresses.

- Improve start-up logging / error handling in clusters with
inconsistent IP families (eg, dual-stack cluster CIDR but
single-stack node IPs).

[kubernetes #42125](https://github.com/kubernetes/kubernetes/issues/42125)

## Proposal
Expand Down Expand Up @@ -269,7 +284,40 @@ The `NodeAddress` of type `NodeHostName` will be moved to a new

```
<<[UNRESOLVED hostname ]>>
Is `Node.Status.Hostname` required or optional? Check existing constraints.
What does this get used for?
- kube-apiserver's --kubelet-preferred-address-types starts with
"Hostname" by default, meaning apiserver will try to DNS-resolve
that value first when trying to reach a kubelet.
- pkg/kubelet/certificate/kubelet.go will include the NodeHostName
as a DNS name in the certificate request (along with any
NodeInternalDNS or NodeExternalDNS names)
That's all I could find in k/k. It is *not* used to set
Node.ObjectMeta.Name (though in many cases they end up being the same
anyway).
GCE and AWS always return NodeAddresses where the NodeHostName value
is a duplicate of a NodeInternalDNS value, but Azure, vSphere, and
OpenStack do not (and the bare-metal case currently doesn't allow for
providing any names other than NodeHostName). So deprecating
NodeHostName addresses entirely (ie, without adding
Node.Status.Hostname) would potentially lose a small amount of
information.
<<[/UNRESOLVED]>>
<<[UNRESOLVED hostname-fqdn ]>>
If we keep `Node.Status.Hostname`, is it
expected/required/explicitly-not-expected to be a fully-qualified domain name?
<<[/UNRESOLVED]>>
<<[UNRESOLVED hostname-label ]>>
There is a "kubernetes.io/hostname" label that is automatically set,
but it is not guaranteed to match the value of the NodeHostName
address. (The cloud provider gets no input, so it's always the
autodetected (`os.Hostname()`) or command-line-overridden value.)
Should we fix this (deprecate and add a new label)?
<<[/UNRESOLVED]>>
```

Expand Down

0 comments on commit ca95478

Please sign in to comment.