Make Node IP families configurable #251
Conversation
k8s-ci-robot
added
release-note
|
Right. This is likely to break the other way if someone has an ipv4-only cluster then. I think we need that config option that says which families to add and the ordering. |
Or look at cluster/pod CIDR? |
|
What CIDR we use for the Pods doesn't say anything about what families we should use for Nodes really. Also, Kubelet uses |
If one has the Pod CIDR set to some IPv6 address or a dual-stack list, it kind of expresses the preference about the order in which the CCM should send the addresses. Was hoping that such info can be retrieved via the k8s api or some more elegant way that to set it as a flag.
|
It is CCM and KCM (depending on which of these runs the NodeIpam controller) that is the canonical source for the CIDRs that Nodes can assign to Pods. So this has to be decided by the flags set on which of these have the IPAM controller running. But as mentioned I think the Node's IP adresses and PodCIDRs are two different concerns.
Sorry, I confused this with the |
k8s-ci-robot
added
size/L
olemarkus
force-pushed
the
ipv6-first
branch
2 times, most recently
from
96bd86c
to
93170fe
Compare
olemarkus
changed the title
| for _, family := range c.cfg.Global.NodeIPFamilies { | ||
| switch family { | ||
| case "ipv4": | ||
| if c.selfAWSInstance.nodeName == name || len(name) == 0 { |
There was a problem hiding this comment.
At best, this path seems like an unnecessary optimization given that kubelet is no longer a caller (as opposed to the legacy-cloud-provider). At worst, it maintains a non-obvious dependency on instance metadata. I think its right to leave the behavior as-is in this PR, and we can consider removing this path in a future change.
There was a problem hiding this comment.
Yeah. I was wondering if not NodeAddresses(nodename) should just perform the instanceID lookup and then call NodeAddressesByProviderID(instanceID). The code is overly redundant right now. I can do that in another PR after this one has merged.
pkg/providers/v1/aws.go
Outdated
| sort.Slice(macIDs, func(i, j int) bool { | ||
| return macDevNum[macIDs[i]] < macDevNum[macIDs[j]] | ||
| }) | ||
| func (c *Cloud) ipv4Addresses() ([]v1.NodeAddress, error) { |
There was a problem hiding this comment.
Given that this function is only exercised by CCM for the instance its running on, and exclusively retrieves addresses via metadata, I'd advocate for "metadata", "IMDS", or "local" in the function name to make it clear when looking at the caller.
pkg/providers/v1/aws.go
Outdated
| return addresses, nil | ||
| } | ||
|
|
||
| func (c *Cloud) ipv6Addresses() ([]v1.NodeAddress, error) { |
There was a problem hiding this comment.
Same comment as above, rename function to be specific to metadata.
| @@ -1621,7 +1710,7 @@ func parseMetadataLocalHostname(metadata string) (string, []string) { | |||
| } | |||
|
|
|||
| // extractNodeAddresses maps the instance information from EC2 to an array of NodeAddresses | |||
| func extractNodeAddresses(instance *ec2.Instance) ([]v1.NodeAddress, error) { | |||
| func extractIPv4NodeAddresses(instance *ec2.Instance) ([]v1.NodeAddress, error) { | |||
There was a problem hiding this comment.
Can we add a comment or rename the variables to make it clear that privateDNSName and publicDNSName are ipv4 specific?
Both which families to include and which the ordering of the families
|
/lgtm |
k8s-ci-robot
added
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nckturner, olemarkus 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 |
Many pods (e.g metrics-server) will always pick the first InternalIP regardless of the family the pod itself has. This will break for any pod not running on host network if the Pod only has an ipv6 address.
This PR will make which IP families to add to Node objects configurable through cloud config.
Does this PR introduce a user-facing change?: