GCE: Handle missing subnet for legacy networks and auto networks with unique subnet names #53410

nicksardo · 2017-10-03T22:23:57Z

Tested on three GKE clusters with automatic, manual, and legacy networks.

Release note:

GCE: Fixes ILB sync on legacy networks and auto networks with unique subnet names

bowei · 2017-10-03T22:26:55Z

pkg/cloudprovider/providers/gce/gce.go

+// 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, error) {


bowei · 2017-10-03T22:27:22Z

pkg/cloudprovider/providers/gce/gce.go

+			return nil, fmt.Errorf("could not retrieve network %v, err: %v", networkURL, err)
+		}
+
+		if len(n.IPv4Range) > 0 {


// Legacy networks have a non-empty IPv4Range

bowei · 2017-10-03T22:28:30Z

pkg/cloudprovider/providers/gce/gce.go

+			isLegacyNetwork = true
+		} else if !n.AutoCreateSubnetworks {
+			// If manual network, fail because no subnet is specified
+			return nil, fmt.Errorf("no subnet specified in cloud provider config despite the network %q being manual.", networkURL)


"manual networks require specifying a subnet'

bowei · 2017-10-03T22:29:11Z

pkg/cloudprovider/providers/gce/gce.go

@@ -687,6 +729,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, err := getRegionInURL(url); err == nil && thisRegion == region {


you are eating err here...

nicksardo · 2017-10-03T23:57:51Z

pkg/cloudprovider/providers/gce/gce.go

+			isLegacyNetwork = true
+		} else {
+			// Try to find the subnet in the list of subnets
+			subnetURL = findSubnetForRegion(n.Subnetworks, config.Region)


@bowei Originally, I threw an error if no subnet is specified and n.AutoCreateSubets is false. Given what we just saw for auto networks converted to manual networks, I removed the error.

Let me know if you disagree or have a better idea.

nicksardo · 2017-10-04T17:28:29Z

/cc @MrHohn
Would appreciate more eyes if possible.

nicksardo · 2017-10-04T17:36:54Z

pkg/cloudprovider/providers/gce/gce.go

 		networkURL = gceNetworkURL(config.ApiEndpoint, netProjID, config.NetworkName)
+	} else {
+		glog.Warningf("No network name or URL specified.")


Did two things in this area:

Reversed the order of conditions.

Removed the getNetworkNameViaAPICall invocation because I highly doubt it works.

nicksardo · 2017-10-04T17:37:49Z

pkg/cloudprovider/providers/gce/gce.go

+			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)


Changed the logic to gracefully fail because we need to handle the kubelet case: network name is known but the minion has no GCE permissions.

nicksardo · 2017-10-04T18:21:55Z

/test pull-kubernetes-unit

MrHohn · 2017-10-04T18:55:32Z

pkg/cloudprovider/providers/gce/gce.go

 		networkURL = gceNetworkURL(config.ApiEndpoint, netProjID, config.NetworkName)
+	} else {
+		glog.Warningf("No network name or URL specified.")


What happens in this case, should we return error? Or is the subnetURL below all we need?

Depends on what the user of this package wants to do. Kubelet inits the cloud provider but only uses a static function that's not related to networking.

I'm not convinced that we can return an error here and not have this bite us in the future.

Kubelet's use of cloud provider seems to be unnecessary. But does it make sense to have a cloud provider be initialized but not fully functional?

It's indeed unnecessary, but we have to live with it for now. Since gce.conf is not passed to kubelet, the cloudprovider is already mostly defunct. CreateGCECloud needs to not throw an error despite the lack of functionality. Meanwhile, ideally we do throw an error if we get into a defunct state for service/ingress controller. I don't think there's a great answer here until we limit the consumers of cloudprovider.

I'm hesitant to return an error here because I'm worried about the case of the metadata API call not returning the network name. This would cause kubelet to not start.

nicksardo · 2017-10-04T22:52:56Z

/test pull-kubernetes-e2e-gce-etcd3

MrHohn

LGTM, just feel like the logic to get URLs is surprisingly dedicated...

MrHohn · 2017-10-05T00:21:31Z

pkg/cloudprovider/providers/gce/gce.go

+			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)


nit: could -> Could

bowei · 2017-10-05T00:56:32Z

/lgtm

k8s-github-robot · 2017-10-05T00:57:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, nicksardo

Associated issue: 53409

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/cloudprovider/providers/gce/OWNERS~~ [bowei,nicksardo]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

fejta-bot · 2017-10-05T04:09:29Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

k8s-github-robot · 2017-10-05T13:09:03Z

Automatic merge from submit-queue (batch tested with PRs 51750, 53195, 53384, 53410). If you want to cherry-pick this change to another branch, please follow the instructions here.

…410-upstream-release-1.8 Automatic merge from submit-queue. Automated cherry pick of #53410 Cherry pick of #53410 on release-1.8. #53410: Handle missing subnet for auto networks and legacy networks **Release note**: ```release-note GCE: Fixes ILB sync on legacy networks and auto networks with unique subnet names ```

k8s-ci-robot assigned bowei Oct 3, 2017

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 3, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 3, 2017

bowei reviewed Oct 3, 2017

View reviewed changes

nicksardo commented Oct 3, 2017

View reviewed changes

nicksardo force-pushed the gce-hack-subnet branch 2 times, most recently from 68e2526 to 5ea3b2c Compare October 4, 2017 17:27

k8s-ci-robot requested a review from MrHohn October 4, 2017 17:28

nicksardo commented Oct 4, 2017

View reviewed changes

MrHohn reviewed Oct 4, 2017

View reviewed changes

MrHohn approved these changes Oct 5, 2017

View reviewed changes

Handle missing subnet for auto networks and legacy networks

995dd32

nicksardo force-pushed the gce-hack-subnet branch from 5ea3b2c to 995dd32 Compare October 5, 2017 00:53

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 5, 2017

k8s-github-robot merged commit 9af3398 into kubernetes:master Oct 5, 2017

nicksardo mentioned this pull request Oct 5, 2017

Automated cherry pick of #53410 #53505

Merged

nicksardo deleted the gce-hack-subnet branch January 12, 2018 06:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GCE: Handle missing subnet for legacy networks and auto networks with unique subnet names #53410

GCE: Handle missing subnet for legacy networks and auto networks with unique subnet names #53410

nicksardo commented Oct 3, 2017 •

edited

Loading

bowei Oct 3, 2017

bowei Oct 3, 2017

bowei Oct 3, 2017

bowei Oct 3, 2017

nicksardo Oct 3, 2017

nicksardo Oct 3, 2017

nicksardo commented Oct 4, 2017

nicksardo Oct 4, 2017

nicksardo Oct 4, 2017

nicksardo commented Oct 4, 2017

MrHohn Oct 4, 2017

nicksardo Oct 4, 2017

bowei Oct 4, 2017

nicksardo Oct 4, 2017

nicksardo commented Oct 4, 2017

MrHohn left a comment

MrHohn Oct 5, 2017

nicksardo Oct 5, 2017

bowei commented Oct 5, 2017

k8s-github-robot commented Oct 5, 2017

fejta-bot commented Oct 5, 2017

k8s-github-robot commented Oct 5, 2017

GCE: Handle missing subnet for legacy networks and auto networks with unique subnet names #53410

GCE: Handle missing subnet for legacy networks and auto networks with unique subnet names #53410

Conversation

nicksardo commented Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicksardo commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicksardo commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicksardo commented Oct 4, 2017

MrHohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowei commented Oct 5, 2017

k8s-github-robot commented Oct 5, 2017

fejta-bot commented Oct 5, 2017

k8s-github-robot commented Oct 5, 2017

nicksardo commented Oct 3, 2017 •

edited

Loading