[GCE] Support internal load balancers #46663

nicksardo · 2017-05-31T00:22:40Z

What this PR does / why we need it:
Allows users to expose K8s services externally of the K8s cluster but within their GCP network.

Fixes #33483

Important User Notes:

This is a beta feature. ILB could be enabled differently in the future.
Requires nodes having version 1.7.0+ (ILB requires health checking and a health check endpoint on kube-proxy has just been exposed)
This cannot be used for intra-cluster communication. Do not call the load balancer IP from a K8s node/pod.
There is no reservation system for private IPs. You can specify a RFC 1918 address in loadBalancerIP field, but it could be lost to another VM or LB if service settings are modified.
If you're running an ingress, your existing loadbalancer backend service must be using BalancingMode type RATE - not UTILIZATION.
- Option 1: With a 1.5.8+ or 1.6.4+ version master, delete all your ingresses, and re-create them.
- Option 2: Migrate to a new cluster running 1.7.0. Considering ILB requires nodes with 1.7.0, this isn't a bad idea.
- Option 3: Possible migration opportunity, but use at your own risk. More to come later.

Reviewer Notes:
Several files were renamed, so github thinks ~2k lines have changed. Review commits one-by-one to see the actual changes.

Release note:

Support creation of GCP Internal Load Balancers from Service objects

nicksardo · 2017-05-31T00:23:22Z

/assign @bowei @MrHohn @thockin

thockin · 2017-05-31T04:18:49Z

pkg/cloudprovider/providers/gce/gce_annotations.go

+	// ServiceAnnotationLoadBalancerInternal is the annotation used on a service with type LoadBalancer
+	// to signify that an Internal LB should be assembled instead of an External LB.
+	// Value should be set to "true" (string) to enable.
+	ServiceAnnotationLoadBalancerInternal = "cloud.google.com/load-balancer-internal"


Is it worth making this be cloud.google.com/load-balancer-type: (internal | external) and leave room for the other LB types that we don't currently represent at all?

I would argue against having an option named "external" as one would be confused as to how it's different from not having the annotation at all. However, if we have different types of "external" LBs supported in the future, having this annotation would be a nice spot to surface them.

I'll make the change tomorrow; the only supported option will be "internal" or no annotation set. Thanks

BTW, we are trying to keep all API related annotations in https://github.com/kubernetes/kubernetes/blob/master/pkg/api/annotation_key_constants.go and https://github.com/kubernetes/kubernetes/blob/master/pkg/api/v1/annotation_key_constants.go. Is there any concern for moving it to the same place?

Hum...I think the concern here is that ILB is GCE specific. May be we could move this out when other cloud providers would like to support ILB as well.

Other cloud providers already have their "internal" lb's created and use cloud-specific annotations. There is an issue open discussing consolidating the annotations - let's not jump the gun on that effort.

thockin · 2017-05-31T04:29:30Z

pkg/cloudprovider/providers/gce/gce_loadbalancer_internal.go

+	}
+
+	// Delete forwarding rule if any settings are different
+	fwdRuleDeleted := false


Is it possible to move this closer to where it is recreated, to minimize disruption?

Sure, I'll move instance group and health check creation above here. Thanks

thockin

Lots of code - not so much tests... Are there any parts of this that could be serviced by unit tests?

Overall looks great. would like an extra set of eyes, if possible.

thockin · 2017-05-31T04:33:34Z

pkg/cloudprovider/providers/gce/gce_loadbalancer_external.go

-	mc := newTargetPoolMetricContext("delete", region)
-	op, err := gce.service.TargetPools.Delete(gce.projectID, region, name).Do()
-
+func (gce *GCECloud) DeleteExternalTargetPoolAndChecks(name, region string, hcNames ...string) error {


I understand why some of these are no public, but this one?

This was public before - I just renamed it and merged it with the logic of the non-public func. https://github.com/kubernetes/kubernetes/pull/46663/files/b836aa75b780046cb290d3ab01f303ffb21ba316..10e59b429409a59c26f985f8a441626e169e37a0#diff-4cae6102def67324f04d6988b5aab0f6L527

Unfortunately, it's public because it's being used in resource cleanup in test/e2e/framework/utils.go.

thockin · 2017-05-31T04:37:19Z

pkg/cloudprovider/providers/gce/gce_loadbalancer.go

+
+func init() {
+	var err error
+	lbSrcRngsFlag.ipn, err = netsets.ParseIPNets([]string{"130.211.0.0/22", "35.191.0.0/16", "209.85.152.0/22", "209.85.204.0/22"}...)


comment please

thockin · 2017-05-31T04:41:31Z

/approve

nicksardo · 2017-05-31T05:37:18Z

pkg/cloudprovider/providers/gce/gce_instances.go

+		return defaultZone
+	}
+	return zone
+}


FYI to reviewers: this logic was taken from the ingress controller.

nicksardo · 2017-05-31T06:01:06Z

Lots of code - not so much tests... Are there any parts of this that could be serviced by unit tests?

Agreed, the lack of tests disturb me as well. I don't think we can run any meaningful unit tests without mocking a significant portion of the compute API. This PR is so large partly because I'm separating compute API calls from the load balancer logic in order to support mocking. I'd love to spend a week or two on that effort but would like to hear from other folks who work on the GCE cloudprovider.

For the immediate concern of this PR, I think the only unit tests worth considering are a few around resource naming. Would love to hear any other ideas...

thockin · 2017-05-31T06:10:01Z

Agree, the mocking/faking/interfacing here will be deep. Thanks for breaking into many commits. Made it WAY easier to review.

…

On Tue, May 30, 2017 at 11:01 PM, Nick Sardo ***@***.***> wrote: Lots of code - not so much tests... Are there any parts of this that could be serviced by unit tests? Agreed, the lack of tests disturb me as well. I don't think we can run any meaningful unit tests without mocking a significant portion of the compute API. This PR is so large partly because I'm separating compute API calls from the load balancer logic in order to support mocking. I'd love to spend a week or two on that effort but would like to hear from other folks who work on the GCE cloudprovider. For the immediate concern of this PR, I think the only unit tests worth considering are a few around resource naming. Would love to hear any other ideas... — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#46663 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVMv6uZ9rYX-r3fMVfRffcw2YKeLiks5r_QI2gaJpZM4NrAni> .

MrHohn

Haven't gone through all commits...Leave some comments ahead.

MrHohn · 2017-05-31T07:05:32Z

pkg/cloudprovider/providers/gce/gce_annotations.go

+	// ServiceAnnotationLoadBalancerInternal is the annotation used on a service with type LoadBalancer
+	// to signify that an Internal LB should be assembled instead of an External LB.
+	// Value should be set to "true" (string) to enable.
+	ServiceAnnotationLoadBalancerInternal = "cloud.google.com/load-balancer-internal"


BTW, we are trying to keep all API related annotations in https://github.com/kubernetes/kubernetes/blob/master/pkg/api/annotation_key_constants.go and https://github.com/kubernetes/kubernetes/blob/master/pkg/api/v1/annotation_key_constants.go. Is there any concern for moving it to the same place?

MrHohn · 2017-05-31T07:10:31Z

pkg/cloudprovider/providers/gce/gce_annotations.go

+	// ServiceAnnotationLoadBalancerInternal is the annotation used on a service with type LoadBalancer
+	// to signify that an Internal LB should be assembled instead of an External LB.
+	// Value should be set to "true" (string) to enable.
+	ServiceAnnotationLoadBalancerInternal = "cloud.google.com/load-balancer-internal"


Hum...I think the concern here is that ILB is GCE specific. May be we could move this out when other cloud providers would like to support ILB as well.

MrHohn · 2017-05-31T07:17:14Z

pkg/cloudprovider/providers/gce/gce_instances.go

@@ -35,13 +35,46 @@ import (
 	"k8s.io/kubernetes/pkg/cloudprovider"
 )

+const (
+	zoneKey     = "failure-domain.beta.kubernetes.io/zone"


Is it possible to import this label from the original place? Like https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/well_known_labels.go#L21.

Thanks for finding it.

Missed changing this in first round of changes. Will add this later.

MrHohn · 2017-05-31T07:23:38Z

pkg/cloudprovider/providers/gce/gce_loadbalancer_naming.go

+
+// MakeHealthCheckFirewallName returns the firewall name used by the GCE load
+// balancers (l4) for performing health checks.
+func MakeHealthCheckFirewallName(clusterID, hcName string, isNodesHealthCheck bool) string {


Maybe better to make the function naming consistent (we have getXXX and makeXXX now) between ELB and ILB. I'm guessing you made changes in later commits but leave this comment just in case :)

Sadly, we also have a mix of exported and private funcs... Those that are exported are used in test/e2e/framework/util.go. Reckon we should export all these funcs regardless of external use.

Renamed all to make... and kept the same export status as before.

MrHohn · 2017-05-31T07:32:54Z

pkg/cloudprovider/providers/gce/gce_loadbalancer_internal.go

+	balanceModeCONNECTION lbBalancingMode = "CONNECTION"
+)
+
+type lbBalancingMode string


Maybe add a link to the public documentations? Obviously I already forgot what those two modes mean :)

MrHohn · 2017-05-31T07:36:07Z

pkg/cloudprovider/providers/gce/gce_loadbalancer_internal.go

+	ports, protocol := getPortsAndProtocol(svc.Spec.Ports)
+	scheme := schemeInternal
+	loadBalancerName := cloudprovider.GetLoadBalancerName(svc)
+	specialHCPath, specialHCPort := v1_service.GetServiceHealthCheckPathPort(svc)


Calling these specialXXX seems not specific enough...Probably localXXX or localTrafficXXX?

And I think checking either one of Path or Port should be enough? Just saw you are only checking Port ==0 in another function.

MrHohn · 2017-05-31T07:44:55Z

pkg/cloudprovider/providers/gce/gce_loadbalancer_internal.go

+		return nil, err
+	}
+
+	// Get health check settings assuming it's an OnlyLocal service


This comment looks a bit confusing, which exact settings is it referring to?

MrHohn · 2017-05-31T07:55:04Z

pkg/cloudprovider/providers/gce/gce_loadbalancer_internal.go

+	return err
+}
+
+func (gce *GCECloud) ensureInternalLoadBalancerDeleted(clusterName, clusterID string, svc *v1.Service) error {


Didn't see the sharedResourceLock is used, did it turn out that ILB management does not need locking?

Thanks for reminding me. Will add the lock.

Done, but needs testing.

MrHohn · 2017-05-31T07:59:08Z

pkg/cloudprovider/providers/gce/gce_loadbalancer_internal.go

+	return ports, protocol
+}
+
+func (gce *GCECloud) getBSLink(name string) string {


Probably just name it getBackendServiceLink?

What is this? Java?!?!

Haha. I'll rename it, thanks.

nicksardo · 2017-05-31T17:21:14Z

pkg/cloudprovider/providers/gce/gce_addresses.go

 	v, err := gce.service.GlobalAddresses.Get(gce.projectID, name).Do()
 	return v, mc.Observe(err)
 }
+
+func (gce *GCECloud) ReserveRegionAddress(addr *compute.Address, region string) (*compute.Address, error) {


Missing comment

nicksardo · 2017-05-31T17:22:26Z

pkg/cloudprovider/providers/gce/gce_forwardingrule.go

+
+// DeleteRegionForwardingRule deletes the RegionalForwardingRule by name & region.
+func (gce *GCECloud) DeleteRegionForwardingRule(name, region string) error {
+	mc := newForwardingRuleMetricContext("delete", "")


Set region in each metric context

nicksardo · 2017-05-31T17:23:52Z

pkg/cloudprovider/providers/gce/gce_util.go

+		return a
+	}
+	return b
+}


Take this out... Was being used in naming code that didn't make the final cut.

nicksardo · 2017-05-31T17:24:54Z

pkg/cloudprovider/providers/gce/gce_zones.go

+
+func (gce *GCECloud) ListZonesInRegion(region string) ([]*compute.Zone, error) {
+	mc := newZonesMetricContext("list", region)
+	filter := fmt.Sprintf("region eq https://www.googleapis.com/compute/v1/projects/%v/regions/%v", gce.projectID, region)


Separate region link creation into own func in gce_util.go

bowei · 2017-05-31T17:14:22Z

pkg/cloudprovider/providers/gce/gce_addresses.go

+	return gce.GetRegionAddress(addr.Name, region)
+}
+
+// DeleteGlobalStaticIP deletes a global static IP by name.


match comment with func name

bowei · 2017-05-31T17:14:32Z

pkg/cloudprovider/providers/gce/gce_addresses.go

+	return v, mc.Observe(err)
+}
+
+func (gce *GCECloud) ReserveRegionAddress(addr *compute.Address, region string) (*compute.Address, error) {


nicksardo · 2017-05-31T17:37:47Z

pkg/cloudprovider/providers/gce/gce_loadbalancer_internal.go

+
+func (gce *GCECloud) updateInternalLoadBalancer(clusterName, clusterID string, service *v1.Service, nodes []*v1.Node) error {
+	igName := getInstanceGroupName(clusterID)
+	_, err := gce.ensureInstanceGroups(igName, nodes)


Need to update backend service if a new group is created.

nicksardo · 2017-05-31T17:41:47Z

pkg/cloudprovider/providers/gce/gce_loadbalancer_internal.go

+		return err
+	}
+
+	if hcInUse {


Add comment explaining this.

nicksardo · 2017-05-31T17:44:06Z

pkg/cloudprovider/providers/gce/gce_loadbalancer_internal.go

+	if err != nil && !isNotFound(err) {
+		return err
+	}
+	// Create expected firewall


Remove unnecessary comment

nicksardo · 2017-05-31T17:48:22Z

pkg/cloudprovider/providers/gce/gce_loadbalancer_internal.go

+	if err != nil {
+		return err
+	}
+	err = gce.ensureFirewall(fwName, fwDesc, sourceRanges.StringSlice(), ports, protocol, nodes)


use loadBalancerName and remove fwName

nicksardo · 2017-05-31T17:48:59Z

pkg/cloudprovider/providers/gce/gce_loadbalancer_internal.go

+
+func (gce *GCECloud) ensureHealthCheck(name, path string, port int32) (*compute.HealthCheck, error) {
+	glog.V(2).Infof("ensureHealthCheck(%v, %v, %v): checking existing health check", name, path, port)
+	wantedHC := newInternalLBHealthCheck(name, path, port)


Rename all wantedXXX to expectedXXX

nicksardo · 2017-05-31T17:49:57Z

pkg/cloudprovider/providers/gce/gce_loadbalancer_internal.go

+	wantedHC := newInternalLBHealthCheck(name, path, port)
+
+	hc, err := gce.GetHealthCheck(name)
+	if hc == nil || err != nil && isNotFound(err) {


This was copied logic from loadbalancer_external.

Separate into two code blocks. One checking for non-NotFound errors; another for handling hc == nil

nicksardo · 2017-05-31T17:55:15Z

pkg/cloudprovider/providers/gce/gce_loadbalancer_internal.go

+	for _, igLink := range igLinks {
+		backends = append(backends, &compute.Backend{
+			Group:         igLink,
+			BalancingMode: string(balancingMode),


Add comment saying that balancing mode settings like MaxConnections and MaxRate cannot be used with ILB (currently).

nicksardo · 2017-05-31T17:56:02Z

pkg/cloudprovider/providers/gce/gce_loadbalancer_internal.go

+
+	wantedBS := &compute.BackendService{
+		Name:                name,
+		Region:              gce.region,


Remove Region. This is output only.

mikedanese · 2017-06-04T05:13:21Z

@k8s-bot test this

nicksardo · 2017-06-04T18:13:49Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

nicksardo · 2017-06-04T18:34:06Z

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

nicksardo · 2017-06-04T18:53:31Z

@mikedanese Can you re-lgtm this? I fixed it in the last commit. Thanks

MrHohn · 2017-06-04T18:54:43Z

/lgtm

k8s-github-robot · 2017-06-04T18:55:02Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [MrHohn,bowei,nicksardo,thockin]
~~test/OWNERS~~ [bowei,thockin]

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

MrHohn · 2017-06-04T19:02:36Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

nicksardo · 2017-06-04T19:35:10Z

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

Federation e2e seems to be failing consistently. Will re-test that when there's hope.

k8s-ci-robot · 2017-06-05T11:11:40Z

@nicksardo: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-federation-e2e-gce	`025f178`	link	`@k8s-bot pull-kubernetes-federation-e2e-gce test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

nicksardo · 2017-06-05T14:04:25Z

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this
@k8s-bot pull-kubernetes-federation-e2e-gce test this

k8s-github-robot · 2017-06-05T23:43:41Z

Automatic merge from submit-queue (batch tested with PRs 46550, 46663, 46816, 46820, 46460)

myaghini · 2017-06-06T06:09:28Z

Is there any documentation how to use this new feature?

bowei · 2017-06-06T06:30:06Z

We will be documenting it as part of the 1.7 release

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 31, 2017

k8s-ci-robot assigned bowei, MrHohn and thockin May 31, 2017

nicksardo force-pushed the gce-internallb branch from 4a7905a to 46b23eb Compare May 31, 2017 00:27

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 31, 2017

nicksardo force-pushed the gce-internallb branch from 46b23eb to 1267fc4 Compare May 31, 2017 01:05

thockin reviewed May 31, 2017

View reviewed changes

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

nicksardo commented May 31, 2017

View reviewed changes

MrHohn reviewed May 31, 2017

View reviewed changes

nicksardo commented May 31, 2017

View reviewed changes

bowei reviewed May 31, 2017

View reviewed changes

nicksardo commented May 31, 2017

View reviewed changes

Nick Sardo added 8 commits June 4, 2017 10:25

Rename gce_loadbalancer.go to gce_loadbalancer_external.go

66773fe

Rename gce_staticip.go to gce_addresses.go

b631061

Refactor compute API calls

2cdaf1f

Modify external LB logic

1283d65

Add internal LB logic

660452d

Hook external & internal lb together

05aaef3

Update test utilities & build file

7248c61

Use new kubelet apis pkg for labels

025f178

nicksardo force-pushed the gce-internallb branch from 8d7604e to 025f178 Compare June 4, 2017 17:26

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 4, 2017

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

k8s-github-robot merged commit 4faf7f1 into kubernetes:master Jun 5, 2017

This was referenced Jun 6, 2017

FEATURE REQUEST: Support GCE Internal Load Balancer #33483

Closed

[GCE] Shared backend services for ILBs leak settings to other ILBs #47092

Closed

MrHohn mentioned this pull request Jun 23, 2017

Make gce_loadbalancer_external and gce_loadbalancer_internal more consistent #48002

Closed

nicksardo deleted the gce-internallb branch July 7, 2017 00:53

skmatti mentioned this pull request May 8, 2020

Promote GlobalAccess for GCE Internal Loadbalancers to GA #88392

Merged

[GCE] Support internal load balancers #46663

[GCE] Support internal load balancers #46663

Conversation

nicksardo commented May 31, 2017 • edited Loading

nicksardo commented May 31, 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

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

thockin left a 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

thockin commented May 31, 2017

Choose a reason for hiding this comment

nicksardo commented May 31, 2017

thockin commented May 31, 2017 via email

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

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

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

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

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

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

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

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

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

mikedanese commented Jun 4, 2017

nicksardo commented Jun 4, 2017

nicksardo commented Jun 4, 2017

nicksardo commented Jun 4, 2017

MrHohn commented Jun 4, 2017

k8s-github-robot commented Jun 4, 2017

nicksardo commented May 31, 2017 •

edited

Loading

k8s-ci-robot commented Jun 5, 2017 •

edited

Loading