Handle missing subnet for auto networks and legacy networks

pkg/cloudprovider/providers/gce/gce.go

@@ -114,6 +114,7 @@ type GCECloud struct {
 	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
+	isLegacyNetwork          bool
 	subnetworkURL            string
 	secondaryRangeName       string
 	networkProjectID         string
@@ -397,31 +398,49 @@ func CreateGCECloud(config *CloudConfig) (*GCECloud, error) {
 
 	// ProjectID and.NetworkProjectID may be project number or name.
 	projID, netProjID := tryConvertToProjectNames(config.ProjectID, config.NetworkProjectID, service)
-
 	onXPN := projID != netProjID
 
 	var networkURL string
 	var subnetURL string
+	var isLegacyNetwork bool
 
-	if config.NetworkName == "" && config.NetworkURL == "" {
-		// TODO: Stop using this call and return an error.
-		// This function returns the first network in a list of networks for a project. The project
-		// should be set via configuration instead of randomly taking the first.
-		networkName, err := getNetworkNameViaAPICall(service, config.NetworkProjectID)
-		if err != nil {
-			return nil, err
-		}
-		networkURL = gceNetworkURL(config.ApiEndpoint, netProjID, networkName)
-	} else if config.NetworkURL != "" {
+	if config.NetworkURL != "" {
 		networkURL = config.NetworkURL
-	} else {
+	} else if config.NetworkName != "" {
 		networkURL = gceNetworkURL(config.ApiEndpoint, netProjID, config.NetworkName)
+	} else {
+		// Other consumers may use the cloudprovider without utilizing the wrapped GCE API functions
+		// or functions requiring network/subnetwork URLs (e.g. Kubelet).
+		glog.Warningf("No network name or URL specified.")
 	}
 
 	if config.SubnetworkURL != "" {
 		subnetURL = config.SubnetworkURL
 	} else if config.SubnetworkName != "" {
 		subnetURL = gceSubnetworkURL(config.ApiEndpoint, netProjID, config.Region, config.SubnetworkName)
+	} else {
+		// Attempt to determine the subnetwork in case it's an automatic network.
+		// Legacy networks will not have a subnetwork, so subnetworkURL should remain empty.
+		if networkName := lastComponent(networkURL); networkName != "" {
+			if n, err := getNetwork(service, netProjID, networkName); err != nil {
+				// Gracefully fail because kubelet calls CreateGCECloud without any config, and API calls will fail coming from minions.
+				glog.Warningf("Could not retrieve network %q in attempt to determine if legacy network or see list of subnets, err %v", networkURL, err)
+			} else {
+				// Legacy networks have a non-empty IPv4Range
+				if len(n.IPv4Range) > 0 {
+					glog.Infof("Determined network %q is type legacy", networkURL)
+					isLegacyNetwork = true
+				} else {
+					// Try to find the subnet in the list of subnets
+					subnetURL = findSubnetForRegion(n.Subnetworks, config.Region)
+					if len(subnetURL) > 0 {
+						glog.Infof("Using subnet %q within network %q & region %q because none was specified.", subnetURL, n.Name, config.Region)
+					} else {
+						glog.Warningf("Could not find any subnet in region %q within list %v.", config.Region, n.Subnetworks)
+					}
+				}
+			}
+		}
 	}
 
 	if len(config.ManagedZones) == 0 {
@@ -449,6 +468,7 @@ func CreateGCECloud(config *CloudConfig) (*GCECloud, error) {
 		localZone:                config.Zone,
 		managedZones:             config.ManagedZones,
 		networkURL:               networkURL,
+		isLegacyNetwork:          isLegacyNetwork,
 		subnetworkURL:            subnetURL,
 		secondaryRangeName:       config.SecondaryRangeName,
 		nodeTags:                 config.NodeTags,
@@ -572,6 +592,10 @@ func (gce *GCECloud) SubnetworkURL() string {
 	return gce.subnetworkURL
 }
 
+func (gce *GCECloud) IsLegacyNetwork() bool {
+	return gce.isLegacyNetwork
+}
+
 // Known-useless DNS search path.
 var uselessDNSSearchRE = regexp.MustCompile(`^[0-9]+.google.internal.$`)
 
@@ -615,7 +639,7 @@ func gceSubnetworkURL(apiEndpoint, project, region, subnetwork string) string {
 	return apiEndpoint + strings.Join([]string{"projects", project, "regions", region, "subnetworks", subnetwork}, "/")
 }
 
-// getProjectIDInURL parses typical full resource URLS and shorter URLS
+// getProjectIDInURL parses full resource URLS and shorter URLS
 // https://www.googleapis.com/compute/v1/projects/myproject/global/networks/mycustom
 // projects/myproject/global/networks/mycustom
 // All return "myproject"
@@ -629,6 +653,20 @@ func getProjectIDInURL(urlStr string) (string, error) {
 	return "", fmt.Errorf("could not find project field in url: %v", urlStr)
 }
 
+// getRegionInURL parses full resource URLS and shorter URLS
+// https://www.googleapis.com/compute/v1/projects/myproject/regions/us-central1/subnetworks/a
+// projects/myproject/regions/us-central1/subnetworks/a
+// All return "us-central1"
+func getRegionInURL(urlStr string) string {
+	fields := strings.Split(urlStr, "/")
+	for i, v := range fields {
+		if v == "regions" && i < len(fields)-1 {
+			return fields[i+1]
+		}
+	}
+	return ""
+}
+
 func getNetworkNameViaMetadata() (string, error) {
 	result, err := metadata.Get("instance/network-interfaces/0/network")
 	if err != nil {
@@ -641,18 +679,9 @@ func getNetworkNameViaMetadata() (string, error) {
 	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
+// getNetwork returns a GCP network
+func getNetwork(svc *compute.Service, networkProjectID, networkID string) (*compute.Network, error) {
+	return svc.Networks.Get(networkProjectID, networkID).Do()
 }
 
 // getProjectID returns the project's string ID given a project number or string
@@ -687,6 +716,15 @@ func getZonesForRegion(svc *compute.Service, projectID, region string) ([]string
 	return zones, nil
 }
 
+func findSubnetForRegion(subnetURLs []string, region string) string {
+	for _, url := range subnetURLs {
+		if thisRegion := getRegionInURL(url); thisRegion == region {
+			return url
+		}
+	}
+	return ""
+}
+
 func newOauthClient(tokenSource oauth2.TokenSource) (*http.Client, error) {
 	if tokenSource == nil {
 		var err error

pkg/cloudprovider/providers/gce/gce_loadbalancer_internal.go

@@ -80,12 +80,18 @@ func (gce *GCECloud) ensureInternalLoadBalancer(clusterName, clusterID string, s
 	// Determine IP which will be used for this LB. If no forwarding rule has been established
 	// or specified in the Service spec, then requestedIP = "".
 	requestedIP := determineRequestedIP(svc, existingFwdRule)
-	addrMgr := newAddressManager(gce, nm.String(), gce.Region(), gce.getInternalSubnetURL(), loadBalancerName, requestedIP, schemeInternal)
-	ipToUse, err := addrMgr.HoldAddress()
-	if err != nil {
-		return nil, err
+	ipToUse := requestedIP
+
+	var addrMgr *addressManager
+	// If the network is not a legacy network, use the address manager
+	if !gce.IsLegacyNetwork() {
+		addrMgr = newAddressManager(gce, nm.String(), gce.Region(), gce.SubnetworkURL(), loadBalancerName, requestedIP, schemeInternal)
+		ipToUse, err = addrMgr.HoldAddress()
+		if err != nil {
+			return nil, err
+		}
+		glog.V(2).Infof("ensureInternalLoadBalancer(%v): reserved IP %q for the forwarding rule", loadBalancerName, ipToUse)
 	}
-	glog.V(2).Infof("ensureInternalLoadBalancer(%v): reserved IP %q for the forwarding rule", loadBalancerName, ipToUse)
 
 	// Ensure firewall rules if necessary
 	if err = gce.ensureInternalFirewalls(loadBalancerName, ipToUse, clusterID, nm, svc, strconv.Itoa(int(hcPort)), sharedHealthCheck, nodes); err != nil {
@@ -102,7 +108,7 @@ func (gce *GCECloud) ensureInternalLoadBalancer(clusterName, clusterID string, s
 		LoadBalancingScheme: string(scheme),
 	}
 
-	// Specify subnetwork if network type is manual
+	// Specify subnetwork if known
 	if len(gce.subnetworkURL) > 0 {
 		expectedFwdRule.Subnetwork = gce.subnetworkURL
 	} else {
@@ -138,13 +144,21 @@ func (gce *GCECloud) ensureInternalLoadBalancer(clusterName, clusterID string, s
 		gce.clearPreviousInternalResources(svc, loadBalancerName, existingBackendService, backendServiceName, hcName)
 	}
 
-	// Now that the controller knows the forwarding rule exists, we can release the address.
-	if err := addrMgr.ReleaseAddress(); err != nil {
-		glog.Errorf("ensureInternalLoadBalancer: failed to release address reservation, possibly causing an orphan: %v", err)
+	if addrMgr != nil {
+		// Now that the controller knows the forwarding rule exists, we can release the address.
+		if err := addrMgr.ReleaseAddress(); err != nil {
+			glog.Errorf("ensureInternalLoadBalancer: failed to release address reservation, possibly causing an orphan: %v", err)
+		}
+	}
+
+	// Get the most recent forwarding rule for the address.
+	updatedFwdRule, err := gce.GetRegionForwardingRule(loadBalancerName, gce.region)
+	if err != nil {
+		return nil, err
 	}
 
 	status := &v1.LoadBalancerStatus{}
-	status.Ingress = []v1.LoadBalancerIngress{{IP: ipToUse}}
+	status.Ingress = []v1.LoadBalancerIngress{{IP: updatedFwdRule.IPAddress}}
 	return status, nil
 }
 
@@ -662,20 +676,6 @@ func (gce *GCECloud) getBackendServiceLink(name string) string {
 	return gce.service.BasePath + strings.Join([]string{gce.projectID, "regions", gce.region, "backendServices", name}, "/")
 }
 
-// getInternalSubnetURL first attempts to return the configured SubnetURL.
-// If subnetwork-name was not specified, then a best-effort generation is made.
-// Note subnet names might not be the network name for some auto networks.
-func (gce *GCECloud) getInternalSubnetURL() string {
-	if gce.SubnetworkURL() != "" {
-		return gce.SubnetworkURL()
-	}
-
-	networkName := getNameFromLink(gce.NetworkURL())
-	v := gceSubnetworkURL("", gce.NetworkProjectID(), gce.Region(), networkName)
-	glog.Warningf("Generating subnetwork URL based off network since subnet name/URL was not configured: %q", v)
-	return v
-}
-
 func getNameFromLink(link string) string {
 	if link == "" {
 		return ""

pkg/cloudprovider/providers/gce/gce_test.go

@@ -626,3 +626,63 @@ func TestNewAlphaFeatureGate(t *testing.T) {
 	delete(knownAlphaFeatures, "foo")
 	delete(knownAlphaFeatures, "bar")
 }
+
+func TestGetRegionInURL(t *testing.T) {
+	cases := map[string]string{
+		"https://www.googleapis.com/compute/v1/projects/my-project/regions/us-central1/subnetworks/a": "us-central1",
+		"https://www.googleapis.com/compute/v1/projects/my-project/regions/us-west2/subnetworks/b":    "us-west2",
+		"projects/my-project/regions/asia-central1/subnetworks/c":                                     "asia-central1",
+		"regions/europe-north2":                                                                       "europe-north2",
+		"my-url":                                                                                      "",
+		"":                                                                                            "",
+	}
+	for input, output := range cases {
+		result := getRegionInURL(input)
+		if result != output {
+			t.Errorf("Actual result %q does not match expected result %q for input: %q", result, output, input)
+		}
+	}
+}
+
+func TestFindSubnetForRegion(t *testing.T) {
+	s := []string{
+		"https://www.googleapis.com/compute/v1/projects/my-project/regions/us-central1/subnetworks/default-38b01f54907a15a7",
+		"https://www.googleapis.com/compute/v1/projects/my-project/regions/us-west1/subnetworks/default",
+		"https://www.googleapis.com/compute/v1/projects/my-project/regions/us-east1/subnetworks/default-277eec3815f742b6",
+		"https://www.googleapis.com/compute/v1/projects/my-project/regions/us-east4/subnetworks/default",
+		"https://www.googleapis.com/compute/v1/projects/my-project/regions/asia-northeast1/subnetworks/default",
+		"https://www.googleapis.com/compute/v1/projects/my-project/regions/asia-east1/subnetworks/default-8e020b4b8b244809",
+		"https://www.googleapis.com/compute/v1/projects/my-project/regions/australia-southeast1/subnetworks/default",
+		"https://www.googleapis.com/compute/v1/projects/my-project/regions/southamerica-east1/subnetworks/default",
+		"https://www.googleapis.com/compute/v1/projects/my-project/regions/europe-west3/subnetworks/default",
+		"https://www.googleapis.com/compute/v1/projects/my-project/regions/asia-southeast1/subnetworks/default",
+		"",
+	}
+	actual := findSubnetForRegion(s, "asia-east1")
+	expectedResult := "https://www.googleapis.com/compute/v1/projects/my-project/regions/asia-east1/subnetworks/default-8e020b4b8b244809"
+	if actual != expectedResult {
+		t.Errorf("Actual result %q does not match expected result %q", actual, expectedResult)
+	}
+
+	var nilSlice []string
+	res := findSubnetForRegion(nilSlice, "us-central1")
+	if res != "" {
+		t.Errorf("expected an empty result, got %v", res)
+	}
+}
+
+func TestLastComponent(t *testing.T) {
+	cases := map[string]string{
+		"https://www.googleapis.com/compute/v1/projects/my-project/regions/us-central1/subnetworks/a": "a",
+		"https://www.googleapis.com/compute/v1/projects/my-project/regions/us-central1/subnetworks/b": "b",
+		"projects/my-project/regions/us-central1/subnetworks/c":                                       "c",
+		"d": "d",
+		"":  "",
+	}
+	for input, output := range cases {
+		result := lastComponent(input)
+		if result != output {
+			t.Errorf("Actual result %q does not match expected result %q for input: %q", result, output, input)
+		}
+	}
+}