Skip to content
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

GCE for cloud-controller-manager #45313

Merged
merged 1 commit into from
May 19, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
67 changes: 51 additions & 16 deletions pkg/cloudprovider/providers/gce/gce_instances.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ limitations under the License.
package gce

import (
"errors"
"fmt"
"net/http"
"strconv"
Expand Down Expand Up @@ -62,15 +61,43 @@ func (gce *GCECloud) NodeAddresses(_ types.NodeName) ([]v1.NodeAddress, error) {
// This method will not be called from the node that is requesting this ID.
// i.e. metadata service and other local methods cannot be used here
func (gce *GCECloud) NodeAddressesByProviderID(providerID string) ([]v1.NodeAddress, error) {
return []v1.NodeAddress{}, errors.New("unimplemented")
project, zone, name, err := splitProviderID(providerID)
if err != nil {
return []v1.NodeAddress{}, err
}

instance, err := gce.service.Instances.Get(project, zone, canonicalizeInstanceName(name)).Do()
if err != nil {
return []v1.NodeAddress{}, fmt.Errorf("error while querying for providerID %q: %v", providerID, err)
}

if len(instance.NetworkInterfaces) < 1 {
return []v1.NodeAddress{}, fmt.Errorf("could not find network interfaces for providerID %q", providerID)
}
networkInterface := instance.NetworkInterfaces[0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a chance that there are multiple internal IPs?
In that case you should loop through instance.NetworkInterfaces here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation states: An array of configurations for this interface. [...]. Only one interface is supported per instance.


nodeAddresses := []v1.NodeAddress{{Type: v1.NodeInternalIP, Address: networkInterface.NetworkIP}}
for _, config := range networkInterface.AccessConfigs {
nodeAddresses = append(nodeAddresses, v1.NodeAddress{Type: v1.NodeExternalIP, Address: config.NatIP})
}

return nodeAddresses, nil
}

// InstanceTypeByProviderID returns the cloudprovider instance type of the node
// with the specified unique providerID This method will not be called from the
// node that is requesting this ID. i.e. metadata service and other local
// methods cannot be used here
func (gce *GCECloud) InstanceTypeByProviderID(providerID string) (string, error) {
return "", errors.New("unimplemented")
project, zone, name, err := splitProviderID(providerID)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if err is not nil here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if zone is unknown (empty)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The provider id is a concatinated string "gce://" + gce.InstanceID(). gce.InstanceID() returns an ID which is constructed like projectID + "/" + zone + "/" + instanceName. The zone for this is retrieved by the API and must not be nil.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please return the error and fail fast if err != nil here

if err != nil {
return "", err
}
instance, err := gce.getInstanceFromProjectInZoneByName(project, zone, name)
if err != nil {
return "", err
}
return instance.Type, nil
}

// ExternalID returns the cloud provider ID of the node with the specified NodeName (deprecated).
Expand Down Expand Up @@ -339,30 +366,38 @@ func (gce *GCECloud) getInstancesByNames(names []string) ([]*gceInstance, error)
func (gce *GCECloud) getInstanceByName(name string) (*gceInstance, error) {
// Avoid changing behaviour when not managing multiple zones
for _, zone := range gce.managedZones {
name = canonicalizeInstanceName(name)
mc := newInstancesMetricContext("get", zone)
res, err := gce.service.Instances.Get(gce.projectID, zone, name).Do()
mc.Observe(err)
instance, err := gce.getInstanceFromProjectInZoneByName(gce.projectID, zone, name)
if err != nil {
glog.Errorf("getInstanceByName: failed to get instance %s; err: %v", name, err)

if isHTTPErrorCode(err, http.StatusNotFound) {
continue
}
return nil, err
}
return &gceInstance{
Zone: lastComponent(res.Zone),
Name: res.Name,
ID: res.Id,
Disks: res.Disks,
Type: lastComponent(res.MachineType),
}, nil
return instance, nil
}

return nil, cloudprovider.InstanceNotFound
}

func (gce *GCECloud) getInstanceFromProjectInZoneByName(project, zone, name string) (*gceInstance, error) {
name = canonicalizeInstanceName(name)
mc := newInstancesMetricContext("get", zone)
res, err := gce.service.Instances.Get(project, zone, name).Do()
mc.Observe(err)
if err != nil {
glog.Errorf("getInstanceFromProjectInZoneByName: failed to get instance %s; err: %v", name, err)
return nil, err
}

return &gceInstance{
Zone: lastComponent(res.Zone),
Name: res.Name,
ID: res.Id,
Disks: res.Disks,
Type: lastComponent(res.MachineType),
}, nil
}

func getInstanceIDViaMetadata() (string, error) {
result, err := metadata.Get("instance/hostname")
if err != nil {
Expand Down
90 changes: 90 additions & 0 deletions pkg/cloudprovider/providers/gce/gce_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -158,3 +158,93 @@ func TestCreateFirewallFails(t *testing.T) {
t.Errorf("error expected when creating firewall without any tags found")
}
}

func TestSplitProviderID(t *testing.T) {
providers := []struct {
providerID string

project string
zone string
instance string

fail bool
}{
{
providerID: ProviderName + "://project-example-164317/us-central1-f/kubernetes-node-fhx1",
project: "project-example-164317",
zone: "us-central1-f",
instance: "kubernetes-node-fhx1",
fail: false,
},
{
providerID: ProviderName + "://project-example.164317/us-central1-f/kubernetes-node-fhx1",
project: "project-example.164317",
zone: "us-central1-f",
instance: "kubernetes-node-fhx1",
fail: false,
},
{
providerID: ProviderName + "://project-example-164317/us-central1-fkubernetes-node-fhx1",
project: "",
zone: "",
instance: "",
fail: true,
},
{
providerID: ProviderName + ":/project-example-164317/us-central1-f/kubernetes-node-fhx1",
project: "",
zone: "",
instance: "",
fail: true,
},
{
providerID: "aws://project-example-164317/us-central1-f/kubernetes-node-fhx1",
project: "",
zone: "",
instance: "",
fail: true,
},
{
providerID: ProviderName + "://project-example-164317/us-central1-f/kubernetes-node-fhx1/",
project: "",
zone: "",
instance: "",
fail: true,
},
{
providerID: ProviderName + "://project-example.164317//kubernetes-node-fhx1",
project: "",
zone: "",
instance: "",
fail: true,
},
{
providerID: ProviderName + "://project-example.164317/kubernetes-node-fhx1",
project: "",
zone: "",
instance: "",
fail: true,
},
}

for _, test := range providers {
project, zone, instance, err := splitProviderID(test.providerID)
if (err != nil) != test.fail {
t.Errorf("Expected to failt=%t, with pattern %v", test.fail, test)
}

if test.fail {
continue
}

if project != test.project {
t.Errorf("Expected %v, but got %v", test.project, project)
}
if zone != test.zone {
t.Errorf("Expected %v, but got %v", test.zone, zone)
}
if instance != test.instance {
t.Errorf("Expected %v, but got %v", test.instance, instance)
}
}
}
15 changes: 15 additions & 0 deletions pkg/cloudprovider/providers/gce/gce_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ limitations under the License.
package gce

import (
"errors"
"fmt"
"regexp"
"strings"

"k8s.io/apimachinery/pkg/types"
Expand All @@ -35,6 +37,8 @@ type gceInstance struct {
Type string
}

var providerIdRE = regexp.MustCompile(`^` + ProviderName + `://([^/]+)/([^/]+)/([^/]+)$`)

func getProjectAndZone() (string, string, error) {
result, err := metadata.Get("instance/zone")
if err != nil {
Expand Down Expand Up @@ -100,3 +104,14 @@ func isHTTPErrorCode(err error, code int) bool {
apiErr, ok := err.(*googleapi.Error)
return ok && apiErr.Code == code
}

// splitProviderID splits a provider's id into core components.
// A providerID is build out of '${ProviderName}://${project-id}/${zone}/${instance-name}'
// See cloudprovider.GetInstanceProviderID.
func splitProviderID(providerID string) (project, zone, instance string, err error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you check if it was doable with url.Parse BTW?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is doable with url.Parse() but it still would be an equal amount of complexity.
You couldn't tell apart empty parameters for example in gce:///foo/bar.
You however can use it to get the Scheme of the URI, but afterwards you still have to split the path and access URL.Host.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use a regular expression rather than a series of string splits?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I use regexp only for non trivial things to parse. But a simple regex could easily enhance the readability of this function. I'd go with something like this^gce://(.+?)/(.+?)/(.+)$.

matches := providerIdRE.FindStringSubmatch(providerID)
if len(matches) != 4 {
return "", "", "", errors.New("error splitting providerID")
}
return matches[1], matches[2], matches[3], nil
}