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
Handle correct providerID format #36
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ import ( | |
"fmt" | ||
"net/http" | ||
"strconv" | ||
"strings" | ||
|
||
"k8s.io/api/core/v1" | ||
"k8s.io/apimachinery/pkg/types" | ||
|
@@ -54,8 +55,12 @@ func (i *instances) NodeAddresses(nodeName types.NodeName) ([]v1.NodeAddress, er | |
// NodeAddressesByProviderID returns all the valid addresses of the specified | ||
// node by providerId. For DO this is the public/private ipv4 addresses for now. | ||
func (i *instances) NodeAddressesByProviderID(providerId string) ([]v1.NodeAddress, error) { | ||
// we can technically get all the required data from metadata service | ||
droplet, err := dropletByID(context.TODO(), i.client, providerId) | ||
id, err := dropletIDFromProviderID(providerId) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
droplet, err := dropletByID(context.TODO(), i.client, id) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
@@ -111,7 +116,12 @@ func (i *instances) InstanceType(name types.NodeName) (string, error) { | |
|
||
// InstanceTypeByProviderID returns the type of the specified instance. | ||
func (i *instances) InstanceTypeByProviderID(providerId string) (string, error) { | ||
droplet, err := dropletByID(context.TODO(), i.client, providerId) | ||
id, err := dropletIDFromProviderID(providerId) | ||
if err != nil { | ||
return "", err | ||
} | ||
|
||
droplet, err := dropletByID(context.TODO(), i.client, id) | ||
if err != nil { | ||
return "", err | ||
} | ||
|
@@ -174,3 +184,24 @@ func dropletByName(ctx context.Context, client *godo.Client, nodeName types.Node | |
|
||
return nil, cloudprovider.InstanceNotFound | ||
} | ||
|
||
// 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) { | ||
if providerID == "" { | ||
return "", errors.New("providerID cannot be empty string") | ||
} | ||
|
||
split := strings.Split(providerID, "/") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same comment as above |
||
if len(split) != 3 { | ||
return "", fmt.Errorf("unexpected providerID format: %s, format should be: digitalocean://12345", providerID) | ||
} | ||
|
||
// since split[0] is actually "digitalocean:" | ||
if strings.TrimSuffix(split[0], ":") != providerName { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe just |
||
return "", fmt.Errorf("provider name from providerID should be digitalocean: %s", providerID) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
return split[2], nil | ||
} |
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 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
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.
Did you mean
?
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.
yes
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 here I ended up doing
strings.Split(providerID, "/")
since it makes validation stricter. For example if the provider ID happened to bedigitalocean:////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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great reason for using
splitN
orHasPrefix
, though. Right now on master,instances.NodeAddressesByProviderID
will look for theproviderID
given. All this PR is about is making sure thatdigitalocean://
is a prefix ofproviderID
. The PR, though, changes behavior where previouslyinstances.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having said all that, this is all a nitpick; I don't intend it to be a blocker.