when --node-ip provided, use that as first InternalIP #297
Conversation
|
Fixes #296 |
| unique[addr] = true | ||
| addresses = append(addresses, addr) | ||
| } | ||
| } | ||
| for _, address := range device.Network { | ||
| if address.AddressFamily == int(metadata.IPv4) { |
There was a problem hiding this comment.
| if address.AddressFamily == int(metadata.IPv4) { | |
| if providedNodeIP == address.Address { | |
| continue | |
| } | |
| if address.AddressFamily == int(metadata.IPv4) { |
The unique map would have the public providedNodeIP address indexed as NodeInternalIP and if we don't do something like the above we'll add a second index for the same address as NodeExternalIP.
(GKE permits public internalips too https://cloud.google.com/kubernetes-engine/docs/how-to/alias-ips#enable_pupis)
There was a problem hiding this comment.
Maybe it makes more sense to have the unique map index on the address string, rather than the v1.Address struct? That way it guarantees uniqueness?
// partial code
var unique = map[v1.NodeAddress]bool{}
addr := v1.NodeAddress{Type: v1.NodeHostName, Address: device.Hostname}
unique[addr.Address] = true
// etc.There was a problem hiding this comment.
I don't see this change committed.
I would expect to find:
var unique = map[string]bool{} // this differs from the partial code snippet above
addr := v1.NodeAddress{Type: v1.NodeHostName, Address: device.Hostname}
unique[addr.Address] = true // unique["1.2.3.4"] = trueBut instead I see: https://github.com/equinix/cloud-provider-equinix-metal/pull/297/files#diff-3ddaf4333cb941b353834040ab733477c56a207db5bcf2e854b62fe663db5494R109-R113
(maybe I'm github'ing incorrectly)
There was a problem hiding this comment.
Aha! I had a failed push due to bad network, which I hadn't realized. Pushed now and updated.
There was a problem hiding this comment.
githubing
Is that a verb now? 😁
metal/devices.go
Outdated
| addresses = append(addresses, v1.NodeAddress{Type: addrType, Address: address.Address}) | ||
| addr = v1.NodeAddress{Type: addrType, Address: address.Address} | ||
|
|
||
| if _, ok := unique[addr]; !ok { |
There was a problem hiding this comment.
We could take advantage of this CloudProvider helper (helpers.AddToNodeAddresses) and avoid some of the logic here. It ensures uniqueness.
There was a problem hiding this comment.
Instead of the whole unique thing? I guess so. I don't really care that much. It replaces 4 lines of code with 1 line of code 3 times, which really isn't all that much. It also is inefficient, looping each time (which doesn't matter all that much in the scale of a few entries). But it also eliminates our ability to do the unique change I suggested above to fit your requirements.
| if node.Annotations != nil { | ||
| providedNodeIP = node.Annotations[cpapi.AnnotationAlphaProvidedIPAddr] | ||
| } | ||
| nodeAddresses, err := nodeAddresses(device, providedNodeIP) |
There was a problem hiding this comment.
I think we would get this for free by starting with nodeAddresses := append([]v1.NodeAddress, node.Status.Addresses...) (however it is we get the existing addresses).
If we don't use the existing node addresses, we may omit addresses manually defined or defined through other kubelet options. For example --hostname-override:
There was a problem hiding this comment.
There are no other structures to provide addresses to the kubelet other than --node-ip and --hostname-override. This PR addresses --node-ip, but I think you are correct to raise that it does not resolve the issue of --hostname-override. It actually explicitly sets the hostname based on the response from the EQXM API.
I am going to leave this one for now. It isn't quite so simple. There are 3 distinct hostnames to worry about:
- whatever the kubelet got from the node automatically (by running
hostname); this might to might not be what is in the EQXM API, e.g. the user changed the hostname via command manually, userdata script, etc. --hostname-override- EQXM API
Reconciling all three of those is its own logical mess. So I am leaving it out of this PR. This PR doesn't change the logic, however flawed: hostname is set via EQXM API.
We can set a new one for hostname after this one is done.
| // was a node IP provided | ||
| var providedNodeIP string | ||
| if node.Annotations != nil { | ||
| providedNodeIP = node.Annotations[cpapi.AnnotationAlphaProvidedIPAddr] |
There was a problem hiding this comment.
I noticed something interesting about --node-ip behavior.
// * If nodeIP matches an address of a particular type (internal or external),
// that will be the only address of that type returned.
There was a problem hiding this comment.
I saw it as well, when digging into how cloud-provider used the returned addresses from InstanceMetadata(). But it doesn't affect us. That is called independently by cloud-provider after InstanceMetadata() is returned by a CCM. It is cloud-provider's own internal logic for how to manage the returned addresses.
It isn't up to CCM to limit it; cloud-provider does that.
Signed-off-by: Avi Deitcher <avi@deitcher.net>
This provides full support to using VLAN IPs and floating IPs.