Refactor manager cluster resource #992
Conversation
| needIPv4 := (net.ParseIP(ip)).To4() != nil | ||
|
|
||
| for _, hostIP := range hostIPs { | ||
| isIPv4 := (net.ParseIP(hostIP)).To4() != nil |
There was a problem hiding this comment.
are 100% sure hostIp is always a valid Ip address? otherwise we might first need to check ParseIP result.
There was a problem hiding this comment.
We validate hostIP in resolveHostIPs, which is happening before this call
|
|
||
| for _, hostIP := range hostIPs { | ||
| isIPv4 := (net.ParseIP(hostIP)).To4() != nil | ||
| if needIPv4 == isIPv4 { |
There was a problem hiding this comment.
The code looks good but I find some difficulties to read it. It's not easy to understand that if needIPv4=False and isIPv4=False then we want to return an IPv6 address?
Do you agree or is it just me getting older?
There was a problem hiding this comment.
Its a bit tricky to read, I agree, I've added a comment
| if (entity.IpAddress != nil) && (*entity.IpAddress == address) { | ||
| return true | ||
| } | ||
| if (entity.Ipv6Address != nil) && (*entity.Ipv6Address == address) { |
There was a problem hiding this comment.
shall we convert string to IPs to avoid equality check to fail due to differences in Ipv6 short/full representation?
| for i := 0; i < maxRetries; i++ { | ||
| clusterConfig, err := client.Get() | ||
| if err != nil { | ||
| return "", "", "", handleReadError(d, "Cluster Config", "", err) | ||
| return "", "", hostIPs, err |
There was a problem hiding this comment.
if this fails after the first iteration we might return a non empty host IP list.
Is that expected?
There was a problem hiding this comment.
Should be ok since error is always checked, and other return values are disregarded when error is present
| } | ||
|
|
||
| log.Printf("[DEBUG]: Host resolved to IP addresses %v", hostIPs) | ||
| } | ||
| clusterID := *clusterConfig.ClusterId | ||
| nodes := clusterConfig.Nodes | ||
| node := nodes[0] | ||
| apiListenAddr := node.ApiListenAddr | ||
| if apiListenAddr != nil { |
There was a problem hiding this comment.
Could it be better to first do this check, and only if it succeeds collect hostIPs? It seems while we wait for ApiListenAddr to populate we will keep repeating resolveHostIPs
There was a problem hiding this comment.
resolveHostIPs only happens on first iteration, if len(hostIPs) == 0, so it shouldn't be repeated..
| } | ||
|
|
||
| func getHostIPFromClusterConfig(client interface{}, clusterConfig nsxModel.ClusterConfig) (string, error) { | ||
| func resolveHostIPs(client interface{}) ([]string, error) { | ||
| c := client.(nsxtClients) | ||
| host := c.Host | ||
| host = strings.TrimPrefix(host, "https://") | ||
| // Check if host is ip address or fqdn, if it's ip address then we are all set, |
There was a problem hiding this comment.
with the approach in resolveHostIPs I think this comment section is no longer needed
There was a problem hiding this comment.
Updated the comment, thanks
annakhm
force-pushed
the
refactor-manager-cluster
branch
from
9fdbf6b to
2e7171f
Compare
Main changes: 1. Resolve provider host address, if needed, rather than relying on comparing host names in cluster config and in provider config 2. Rely on nodes IP version rather than host IP version, which is unknown when not specified in provider config 3. Refactor Read function to update only Computed fields in state Signed-off-by: Anna Khmelnitsky <[email protected]>
annakhm
force-pushed
the
refactor-manager-cluster
branch
from
2e7171f to
7b23d00
Compare
Main changes: