Handle correct providerID format #36
Conversation
| return "", errors.New("providerID cannot be empty string") | ||
| } | ||
|
|
||
| split := strings.Split(providerID, "/") |
There was a problem hiding this comment.
strings.SplitN(providerID, "/", 3)?
There was a problem hiding this comment.
same comment as above
| } | ||
|
|
||
| // since split[0] is actually "digitalocean:" | ||
| if strings.TrimSuffix(split[0], ":") != providerName { |
There was a problem hiding this comment.
Maybe just if split[0] != providerName + ":" {?
| // since split[0] is actually "digitalocean:" | ||
| if strings.TrimSuffix(split[0], ":") != providerName { | ||
| return "", fmt.Errorf("provider name from providerID should be digitalocean: %s", providerID) | ||
| } |
There was a problem hiding this comment.
return "", fmt.Errorf("provider name from providerID should be %s://%s", providerName, providerID)?
There was a problem hiding this comment.
providerID is already in the format providerName://id so it would be digitalocean://digitalocean://droplet-id. I think the confusion here is that providerID is the droplet ID which it is not, it's a separate concept from Kubernetes land :P
| // dropletIDFromProviderID returns a droplet's ID extracted from the node's | ||
| // providerID spec. The providerID spec should be retrievable from the Kubernetes | ||
| // node object. The expected format is: digitalocean://droplet-id | ||
| func dropletIDFromProviderID(providerID string) (string, error) { |
There was a problem hiding this comment.
There are some comments below that are just nitpicks. Take them or not.
Also consider: is there any reason to split on the slashes at all? Why not just
func dropletIDFromProviderID(providerID string) (string, error) {
const scheme := providerID + "://"
if !strings.HasPrefix(providerID, scheme) != 0 {
return "", fmt.Errorf("unexpected providerID format: %s, format should be: digitalocean://12345", providerID)
}
return strings.TrimPrefix(providerID, scheme), nil
}
There was a problem hiding this comment.
Did you mean
const scheme := providerName + "://"
?
There was a problem hiding this comment.
I think here I ended up doing strings.Split(providerID, "/") since it makes validation stricter. For example if the provider ID happened to be digitalocean:////droplet-id (which I have seen before on AWS clusters) then trim prefix wouldn't work here. Seems unlikely but possible? The problem mainly is that CCM doesn't have control over what the node provider ID is set to, we can only hope for the best and assume it's a format we expect, otherwise we actually call the *ByNodeName methods instead.
There was a problem hiding this comment.
That's a great reason for using splitN or HasPrefix, though. Right now on master, instances.NodeAddressesByProviderID will look for the providerID given. All this PR is about is making sure that digitalocean:// is a prefix of providerID. The PR, though, changes behavior where previously instances.dropletById would have been given //droplet-id.
Unless we're prepared to validate other rules about the droplet-id value, not allowing leading /s seems out of scope.
There was a problem hiding this comment.
Having said all that, this is all a nitpick; I don't intend it to be a blocker.
Addresses #15.
In v1.7.x this wasn't important since CCM never set providerID in the first place. In v1.8, it will so we should handle the expected format. The expected format for node's providerID is
digitalocean://droplet-id, example: