From 06e328627c68de2bd124065b133562b1d20d4956 Mon Sep 17 00:00:00 2001 From: Nick Sardo Date: Thu, 6 Jul 2017 16:58:27 -0700 Subject: [PATCH 1/2] Use network project id for firewall/route mgmt and zone listing --- .../gce/container-linux/configure-helper.sh | 5 +- cluster/gce/gci/configure-helper.sh | 3 +- pkg/cloudprovider/providers/gce/gce.go | 163 ++++++++++-------- .../providers/gce/gce_firewall.go | 8 +- pkg/cloudprovider/providers/gce/gce_routes.go | 6 +- pkg/cloudprovider/providers/gce/gce_zones.go | 4 +- test/e2e/e2e.go | 1 + 7 files changed, 105 insertions(+), 85 deletions(-) diff --git a/cluster/gce/container-linux/configure-helper.sh b/cluster/gce/container-linux/configure-helper.sh index dae77d44ce..551f51706c 100755 --- a/cluster/gce/container-linux/configure-helper.sh +++ b/cluster/gce/container-linux/configure-helper.sh @@ -230,6 +230,7 @@ EOF token-url = ${TOKEN_URL} token-body = ${TOKEN_BODY} project-id = ${PROJECT_ID} +network-project-id = ${NETWORK_PROJECT_ID} network-name = ${NODE_NETWORK} EOF if [[ -n "${NODE_SUBNETWORK:-}" ]]; then @@ -1269,7 +1270,7 @@ function start-kube-addons { if [[ "${NETWORK_POLICY_PROVIDER:-}" == "calico" ]]; then setup-addon-manifests "addons" "calico-policy-controller" - # Configure Calico based on cluster size and image type. + # Configure Calico based on cluster size and image type. local -r ds_file="${dst_dir}/calico-policy-controller/calico-node-daemonset.yaml" local -r typha_dep_file="${dst_dir}/calico-policy-controller/typha-deployment.yaml" sed -i -e "s@__CALICO_CNI_DIR__@/opt/cni/bin@g" "${ds_file}" @@ -1277,7 +1278,7 @@ function start-kube-addons { sed -i -e "s@__CALICO_TYPHA_CPU__@$(get-calico-typha-cpu)@g" "${typha_dep_file}" sed -i -e "s@__CALICO_TYPHA_REPLICAS__@$(get-calico-typha-replicas)@g" "${typha_dep_file}" else - # If not configured to use Calico, the set the typha replica count to 0, but only if the + # If not configured to use Calico, the set the typha replica count to 0, but only if the # addon is present. local -r typha_dep_file="${dst_dir}/calico-policy-controller/typha-deployment.yaml" if [[ -e $typha_dep_file ]]; then diff --git a/cluster/gce/gci/configure-helper.sh b/cluster/gce/gci/configure-helper.sh index 85639a2abc..823b7bb52e 100644 --- a/cluster/gce/gci/configure-helper.sh +++ b/cluster/gce/gci/configure-helper.sh @@ -390,6 +390,7 @@ EOF token-url = ${TOKEN_URL} token-body = ${TOKEN_BODY} project-id = ${PROJECT_ID} +network-project-id = ${NETWORK_PROJECT_ID} network-name = ${NODE_NETWORK} EOF if [[ -n "${NODE_SUBNETWORK:-}" ]]; then @@ -946,7 +947,7 @@ function start-kubelet { fi if [[ -n "${NODE_TAINTS:-}" ]]; then flags+=" --register-with-taints=${NODE_TAINTS}" - fi + fi if [[ -n "${EVICTION_HARD:-}" ]]; then flags+=" --eviction-hard=${EVICTION_HARD}" fi diff --git a/pkg/cloudprovider/providers/gce/gce.go b/pkg/cloudprovider/providers/gce/gce.go index 7025cf813d..cc9d50e6b7 100644 --- a/pkg/cloudprovider/providers/gce/gce.go +++ b/pkg/cloudprovider/providers/gce/gce.go @@ -83,17 +83,19 @@ type GCECloud struct { // for the cloudprovider to start watching the configmap. ClusterID ClusterID - service *compute.Service - serviceBeta *computebeta.Service - containerService *container.Service - cloudkmsService *cloudkms.Service - clientBuilder controller.ControllerClientBuilder - projectID string - region string - localZone string // The zone in which we are running - managedZones []string // List of zones we are spanning (for multi-AZ clusters, primarily when running on master) - networkURL string - subnetworkURL string + service *compute.Service + serviceBeta *computebeta.Service + containerService *container.Service + cloudkmsService *cloudkms.Service + clientBuilder controller.ControllerClientBuilder + projectID string + region string + localZone string // The zone in which we are running + managedZones []string // List of zones we are spanning (for multi-AZ clusters, primarily when running on master) + networkURL string + subnetworkURL string + // Project which contains the cluster's network. + // Used for specific network resources: firewalls, routes, and listing of zones in a region. networkProjectID string onXPN bool nodeTags []string // List of tags to use on firewall rules for load balancers @@ -131,9 +133,12 @@ type GCEServiceManager struct { type Config struct { Global struct { - TokenURL string `gcfg:"token-url"` - TokenBody string `gcfg:"token-body"` + TokenURL string `gcfg:"token-url"` + TokenBody string `gcfg:"token-body"` + // ProjectID and NetworkProjectID can either be the numeric or string-based unique identifier that starts with [a-z] + // However, both IDs need to be the same type for controllers to recognize this cluster as an XPN cluster. ProjectID string `gcfg:"project-id"` + NetworkProjectID string `gcfg:"network-project-id"` // Project which contains the cluster's network. See networkProjectID in GCECloud NetworkName string `gcfg:"network-name"` SubnetworkName string `gcfg:"subnetwork-name"` NodeTags []string `gcfg:"node-tags"` @@ -161,29 +166,32 @@ func (g *GCECloud) GetKMSService() *cloudkms.Service { return g.cloudkmsService } -// Returns the ProjectID corresponding to the project this cloud is in. -func (g *GCECloud) GetProjectID() string { - return g.projectID -} - // newGCECloud creates a new instance of GCECloud. func newGCECloud(config io.Reader) (*GCECloud, error) { apiEndpoint := "" - projectID, zone, err := getProjectAndZone() + + // projectNumber is the numeric identifier. Note: there is also a unique string-based project identifier as well (see https://cloud.google.com/resource-manager/docs/creating-managing-projects#identifying_projects) + projectNumber, zone, err := getProjectAndZone() if err != nil { return nil, err } + // Default projectID to known project number + projectID := projectNumber region, err := GetGCERegion(zone) if err != nil { return nil, err } - networkName, err := getNetworkNameViaMetadata() + // networkProjectNumber is a numeric identifier similar to the projectNumber above. + networkProjectNumber, networkName, err := getNetworkProjectAndNameViaMetadata() if err != nil { return nil, err } - networkURL := gceNetworkURL(apiEndpoint, projectID, networkName) + // Default networkProjectID to known network project number + networkProjectID := networkProjectNumber + + networkURL := gceNetworkURL(apiEndpoint, networkProjectNumber, networkName) subnetworkURL := "" // By default, Kubernetes clusters only run against one zone @@ -205,18 +213,31 @@ func newGCECloud(config io.Reader) (*GCECloud, error) { if cfg.Global.ProjectID != "" { projectID = cfg.Global.ProjectID } - - if cfg.Global.NetworkName != "" && strings.Contains(cfg.Global.NetworkName, "/") { - networkURL = cfg.Global.NetworkName - } else { - networkURL = gceNetworkURL(apiEndpoint, projectID, networkName) + if cfg.Global.NetworkProjectID != "" { + networkProjectID = cfg.Global.NetworkProjectID } - if cfg.Global.SubnetworkName != "" && strings.Contains(cfg.Global.SubnetworkName, "/") { - subnetworkURL = cfg.Global.SubnetworkName - } else { - subnetworkURL = gceSubnetworkURL(apiEndpoint, cfg.Global.ProjectID, region, cfg.Global.SubnetworkName) + if cfg.Global.NetworkName != "" { + if strings.Contains(cfg.Global.NetworkName, "/") { + otherNetworkProjectID, _ := getProjectIDInURL(cfg.Global.NetworkName) + if networkProjectID != otherNetworkProjectID { + glog.Warningf("Different network projects (may be id vs number). %q and %q", networkProjectID, otherNetworkProjectID) + } + + networkURL = cfg.Global.NetworkName + } else { + networkURL = gceNetworkURL(apiEndpoint, networkProjectID, cfg.Global.NetworkName) + } } + + if cfg.Global.SubnetworkName != "" { + if strings.Contains(cfg.Global.SubnetworkName, "/") { + subnetworkURL = cfg.Global.SubnetworkName + } else { + subnetworkURL = gceSubnetworkURL(apiEndpoint, networkProjectID, region, cfg.Global.SubnetworkName) + } + } + if cfg.Global.TokenURL != "" { tokenSource = NewAltTokenSource(cfg.Global.TokenURL, cfg.Global.TokenBody) } @@ -227,7 +248,7 @@ func newGCECloud(config io.Reader) (*GCECloud, error) { } } - return CreateGCECloud(apiEndpoint, projectID, region, zone, managedZones, networkURL, subnetworkURL, + return CreateGCECloud(apiEndpoint, projectID, networkProjectID, region, zone, managedZones, networkURL, subnetworkURL, nodeTags, nodeInstancePrefix, tokenSource, true /* useMetadataServer */) } @@ -235,9 +256,13 @@ func newGCECloud(config io.Reader) (*GCECloud, error) { // If no networkUrl is specified, loads networkName via rest call. // If no tokenSource is specified, uses oauth2.DefaultTokenSource. // If managedZones is nil / empty all zones in the region will be managed. -func CreateGCECloud(apiEndpoint, projectID, region, zone string, managedZones []string, networkURL, subnetworkURL string, nodeTags []string, +func CreateGCECloud(apiEndpoint, projectID, networkProjectID, region, zone string, managedZones []string, networkURL, subnetworkURL string, nodeTags []string, nodeInstancePrefix string, tokenSource oauth2.TokenSource, useMetadataServer bool) (*GCECloud, error) { + // Determine if cluster is on shared VPC network + // Must assert that the IDs are the same type (ID or number) before checking inequality + onXPN := isProjectNumber(projectID) == isProjectNumber(networkProjectID) && projectID != networkProjectID + client, err := newOauthClient(tokenSource) if err != nil { return nil, err @@ -268,20 +293,6 @@ func CreateGCECloud(apiEndpoint, projectID, region, zone string, managedZones [] return nil, err } - if networkURL == "" { - networkName, err := getNetworkNameViaAPICall(service, projectID) - if err != nil { - return nil, err - } - networkURL = gceNetworkURL(apiEndpoint, projectID, networkName) - } - - networkProjectID, err := getProjectIDInURL(networkURL) - if err != nil { - return nil, err - } - onXPN := networkProjectID != projectID - if len(managedZones) == 0 { managedZones, err = getZonesForRegion(service, projectID, region) if err != nil { @@ -358,7 +369,17 @@ func (gce *GCECloud) Region() string { return gce.region } -// OnXPN returns true if the cluster is running on a cross project network (XPN) +// ProjectID returns the project ID which owns the instances +func (gce *GCECloud) ProjectID() string { + return gce.projectID +} + +// NetworkProjectID returns the project ID which owns the network +func (gce *GCECloud) NetworkProjectID() string { + return gce.networkProjectID +} + +// OnXPN returns true if the cluster is running on a shared VPC network func (gce *GCECloud) OnXPN() bool { return gce.onXPN } @@ -390,18 +411,28 @@ func (gce *GCECloud) ScrubDNS(nameservers, searches []string) (nsOut, srchOut [] // GCECloud implements cloudprovider.Interface. var _ cloudprovider.Interface = (*GCECloud)(nil) -func gceNetworkURL(api_endpoint, project, network string) string { - if api_endpoint == "" { - api_endpoint = "https://www.googleapis.com/compute/v1/" +func gceNetworkURL(apiEndpoint, project, network string) string { + if apiEndpoint == "" { + apiEndpoint = "https://www.googleapis.com/compute/v1/" } - return fmt.Sprintf("%sprojects/%s/global/networks/%s", api_endpoint, project, network) + return fmt.Sprintf("%vprojects/%s/global/networks/%s", apiEndpoint, project, network) } -func gceSubnetworkURL(api_endpoint, project, region, subnetwork string) string { - if api_endpoint == "" { - api_endpoint = "https://www.googleapis.com/compute/v1/" +func gceSubnetworkURL(apiEndpoint, project, region, subnetwork string) string { + if apiEndpoint == "" { + apiEndpoint = "https://www.googleapis.com/compute/v1/" } - return fmt.Sprintf("%sprojects/%s/regions/%s/subnetworks/%s", api_endpoint, project, region, subnetwork) + return fmt.Sprintf("%vprojects/%s/regions/%s/subnetworks/%s", apiEndpoint, project, region, subnetwork) +} + +// Project IDs cannot have a digit for the first characeter. If the id contains a digit, +// then it must be a project number. +func isProjectNumber(idOrNumber string) bool { + if len(idOrNumber) == 0 { + return false + } + + return idOrNumber[0] >= '0' && idOrNumber[0] <= '9' } // getProjectIDInURL parses typical full resource URLS and shorter URLS @@ -418,30 +449,16 @@ func getProjectIDInURL(urlStr string) (string, error) { return "", fmt.Errorf("could not find project field in url: %v", urlStr) } -func getNetworkNameViaMetadata() (string, error) { +func getNetworkProjectAndNameViaMetadata() (string, string, error) { result, err := metadata.Get("instance/network-interfaces/0/network") if err != nil { - return "", err + return "", "", err } parts := strings.Split(result, "/") if len(parts) != 4 { - return "", fmt.Errorf("unexpected response: %s", result) + return "", "", fmt.Errorf("unexpected response: %s", result) } - return parts[3], nil -} - -func getNetworkNameViaAPICall(svc *compute.Service, projectID string) (string, error) { - // TODO: use PageToken to list all not just the first 500 - networkList, err := svc.Networks.List(projectID).Do() - if err != nil { - return "", err - } - - if networkList == nil || len(networkList.Items) <= 0 { - return "", fmt.Errorf("GCE Network List call returned no networks for project %q", projectID) - } - - return networkList.Items[0].Name, nil + return parts[1], parts[3], nil } func getZonesForRegion(svc *compute.Service, projectID, region string) ([]string, error) { diff --git a/pkg/cloudprovider/providers/gce/gce_firewall.go b/pkg/cloudprovider/providers/gce/gce_firewall.go index 668f2e0552..e007dc79fd 100644 --- a/pkg/cloudprovider/providers/gce/gce_firewall.go +++ b/pkg/cloudprovider/providers/gce/gce_firewall.go @@ -32,14 +32,14 @@ func newFirewallMetricContext(request string) *metricContext { // GetFirewall returns the Firewall by name. func (gce *GCECloud) GetFirewall(name string) (*compute.Firewall, error) { mc := newFirewallMetricContext("get") - v, err := gce.service.Firewalls.Get(gce.projectID, name).Do() + v, err := gce.service.Firewalls.Get(gce.networkProjectID, name).Do() return v, mc.Observe(err) } // CreateFirewall creates the passed firewall func (gce *GCECloud) CreateFirewall(f *compute.Firewall) error { mc := newFirewallMetricContext("create") - op, err := gce.service.Firewalls.Insert(gce.projectID, f).Do() + op, err := gce.service.Firewalls.Insert(gce.networkProjectID, f).Do() if err != nil { return mc.Observe(err) } @@ -50,7 +50,7 @@ func (gce *GCECloud) CreateFirewall(f *compute.Firewall) error { // DeleteFirewall deletes the given firewall rule. func (gce *GCECloud) DeleteFirewall(name string) error { mc := newFirewallMetricContext("delete") - op, err := gce.service.Firewalls.Delete(gce.projectID, name).Do() + op, err := gce.service.Firewalls.Delete(gce.networkProjectID, name).Do() if err != nil { return mc.Observe(err) } @@ -60,7 +60,7 @@ func (gce *GCECloud) DeleteFirewall(name string) error { // UpdateFirewall applies the given firewall as an update to an existing service. func (gce *GCECloud) UpdateFirewall(f *compute.Firewall) error { mc := newFirewallMetricContext("update") - op, err := gce.service.Firewalls.Update(gce.projectID, f.Name, f).Do() + op, err := gce.service.Firewalls.Update(gce.networkProjectID, f.Name, f).Do() if err != nil { return mc.Observe(err) } diff --git a/pkg/cloudprovider/providers/gce/gce_routes.go b/pkg/cloudprovider/providers/gce/gce_routes.go index b20e3c82eb..8d85c32fc3 100644 --- a/pkg/cloudprovider/providers/gce/gce_routes.go +++ b/pkg/cloudprovider/providers/gce/gce_routes.go @@ -43,7 +43,7 @@ func (gce *GCECloud) ListRoutes(clusterName string) ([]*cloudprovider.Route, err page := 0 for ; page == 0 || (pageToken != "" && page < maxPages); page++ { mc := newRoutesMetricContext("list_page") - listCall := gce.service.Routes.List(gce.projectID) + listCall := gce.service.Routes.List(gce.networkProjectID) prefix := truncateClusterName(clusterName) listCall = listCall.Filter("name eq " + prefix + "-.*") @@ -92,7 +92,7 @@ func (gce *GCECloud) CreateRoute(clusterName string, nameHint string, route *clo } mc := newRoutesMetricContext("create") - insertOp, err := gce.service.Routes.Insert(gce.projectID, &compute.Route{ + insertOp, err := gce.service.Routes.Insert(gce.networkProjectID, &compute.Route{ Name: routeName, DestRange: route.DestinationCIDR, NextHopInstance: fmt.Sprintf("zones/%s/instances/%s", targetInstance.Zone, targetInstance.Name), @@ -113,7 +113,7 @@ func (gce *GCECloud) CreateRoute(clusterName string, nameHint string, route *clo func (gce *GCECloud) DeleteRoute(clusterName string, route *cloudprovider.Route) error { mc := newRoutesMetricContext("delete") - deleteOp, err := gce.service.Routes.Delete(gce.projectID, route.Name).Do() + deleteOp, err := gce.service.Routes.Delete(gce.networkProjectID, route.Name).Do() if err != nil { return mc.Observe(err) } diff --git a/pkg/cloudprovider/providers/gce/gce_zones.go b/pkg/cloudprovider/providers/gce/gce_zones.go index 881fe652fc..c8258749a2 100644 --- a/pkg/cloudprovider/providers/gce/gce_zones.go +++ b/pkg/cloudprovider/providers/gce/gce_zones.go @@ -44,7 +44,7 @@ func (gce *GCECloud) GetZone() (cloudprovider.Zone, error) { func (gce *GCECloud) ListZonesInRegion(region string) ([]*compute.Zone, error) { mc := newZonesMetricContext("list", region) filter := fmt.Sprintf("region eq %v", gce.getRegionLink(region)) - list, err := gce.service.Zones.List(gce.projectID).Filter(filter).Do() + list, err := gce.service.Zones.List(gce.networkProjectID).Filter(filter).Do() if err != nil { return nil, mc.Observe(err) } @@ -52,5 +52,5 @@ func (gce *GCECloud) ListZonesInRegion(region string) ([]*compute.Zone, error) { } func (gce *GCECloud) getRegionLink(region string) string { - return fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/%v/regions/%v", gce.projectID, region) + return fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/%v/regions/%v", gce.networkProjectID, region) } diff --git a/test/e2e/e2e.go b/test/e2e/e2e.go index a77c3f128a..1c3397d4f9 100644 --- a/test/e2e/e2e.go +++ b/test/e2e/e2e.go @@ -77,6 +77,7 @@ func setupProviderConfig() error { managedZones = []string{zone} } cloudConfig.Provider, err = gcecloud.CreateGCECloud(framework.TestContext.CloudConfig.ApiEndpoint, + framework.TestContext.CloudConfig.ProjectID, framework.TestContext.CloudConfig.ProjectID, region, zone, managedZones, "" /* networkUrl */, "" /* subnetworkUrl */, nil, /* nodeTags */ "" /* nodeInstancePerfix */, nil /* tokenSource */, false /* useMetadataServer */) From 62d13f137942c4b9568e88476d92919c25c5b72a Mon Sep 17 00:00:00 2001 From: Nick Sardo Date: Thu, 6 Jul 2017 18:13:02 -0700 Subject: [PATCH 2/2] Use API that utilizes networkProjectId --- pkg/cloudprovider/providers/gce/gce_loadbalancer_external.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cloudprovider/providers/gce/gce_loadbalancer_external.go b/pkg/cloudprovider/providers/gce/gce_loadbalancer_external.go index 79bfb85343..6f0acb5937 100644 --- a/pkg/cloudprovider/providers/gce/gce_loadbalancer_external.go +++ b/pkg/cloudprovider/providers/gce/gce_loadbalancer_external.go @@ -727,7 +727,7 @@ func (gce *GCECloud) firewallNeedsUpdate(name, serviceName, region, ipAddress st return false, false, nil } - fw, err := gce.service.Firewalls.Get(gce.projectID, makeFirewallName(name)).Do() + fw, err := gce.GetFirewall(makeFirewallName(name)) if err != nil { if isHTTPErrorCode(err, http.StatusNotFound) { return false, true, nil @@ -774,7 +774,7 @@ func (gce *GCECloud) ensureHttpHealthCheckFirewall(serviceName, ipAddress, regio ports := []v1.ServicePort{{Protocol: "tcp", Port: hcPort}} fwName := MakeHealthCheckFirewallName(clusterID, hcName, isNodesHealthCheck) - fw, err := gce.service.Firewalls.Get(gce.projectID, fwName).Do() + fw, err := gce.GetFirewall(fwName) if err != nil { if !isHTTPErrorCode(err, http.StatusNotFound) { return fmt.Errorf("error getting firewall for health checks: %v", err)