New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
loadbalancers: Support nodes where nodeName is the private or public IP #75
Conversation
do/loadbalancers.go
Outdated
for _, droplet := range droplets { | ||
if node.Name == droplet.Name { | ||
dropletIDs = append(dropletIDs, droplet.ID) | ||
break | ||
} | ||
addresses, _ := nodeAddresses(&droplet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we handle this error here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should only log it for now or change the nodeAddresses
function, right now a single node without private network
would cause a error. Why do we require private network
anyway? It should be optional.
digitalocean-cloud-controller-manager/do/droplets.go
Lines 75 to 92 in 5761801
func nodeAddresses(droplet *godo.Droplet) ([]v1.NodeAddress, error) { | |
var addresses []v1.NodeAddress | |
addresses = append(addresses, v1.NodeAddress{Type: v1.NodeHostName, Address: droplet.Name}) | |
privateIP, err := droplet.PrivateIPv4() | |
if err != nil || privateIP == "" { | |
return nil, fmt.Errorf("could not get private ip: %v", err) | |
} | |
addresses = append(addresses, v1.NodeAddress{Type: v1.NodeInternalIP, Address: privateIP}) | |
publicIP, err := droplet.PublicIPv4() | |
if err != nil || publicIP == "" { | |
return nil, fmt.Errorf("could not get public ip: %v", err) | |
} | |
addresses = append(addresses, v1.NodeAddress{Type: v1.NodeExternalIP, Address: publicIP}) | |
return addresses, nil | |
} |
We do not handle error in
addresses, _ := nodeAddresses(&droplet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is very reasonable, just logging works for me :)
@klausenbusk thanks so much for this PR. Just had one minor comment and it should good to go |
Ready for merge I suppose. I'm not sure if I did the logging thing correctly (?). |
for _, droplet := range droplets { | ||
if node.Name == droplet.Name { | ||
dropletIDs = append(dropletIDs, droplet.ID) | ||
break | ||
} | ||
addresses, err := nodeAddresses(&droplet) | ||
if err != nil { | ||
glog.Errorf("error getting node addresses for %s: %v", droplet.Name, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a continue
here otherwise there would be a panic on the next for loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, are you sure about the panic
? The spec says For a nil slice, the number of iterations is 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL :)
sorry @klausenbusk one more comment |
LGTM thanks! |
Fix #69