Add support for L7-ILB (take 2) #789

spencerhance · 2019-06-29T03:15:47Z

This PR supersedes #773

Adds support for the L7 Internal Load Balancer to the controller. A follow up PR with e2e tests will be added.

In the interest of time, I've added the L7-ILB work on top of #788 so that it can be reviewed in tandem. Unless you foresee gigantic changes in that PR, I would review this as well so it can be merged shortly after. The first commit on this PR is from that branch.

Currently, the feature is flag-gated with the '--enable-l7-ilb' flag. Enabling the flag will require resolving #767

k8s-ci-robot · 2019-06-29T03:15:55Z

Hi @spencerhance. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

rramkumar1 · 2019-06-29T17:30:32Z

/ok-to-test

spencerhance · 2019-07-01T20:35:59Z

/cc @freehan

rramkumar1 · 2019-07-02T17:02:06Z

@spencerhance Can you squash commits so that it's easier to see which commits are from the parent PR?

spencerhance · 2019-07-02T19:36:28Z

@rramkumar1 good call, should be good now

pkg/backends/syncer.go

freehan · 2019-07-02T23:45:03Z

pkg/controller/utils.go

@@ -88,3 +88,15 @@ func nodePorts(svcPorts []utils.ServicePort) []int64 {
 	}
 	return ports
 }
+
+func UpdateServicePortsForILB(ingSvcPorts []utils.ServicePort, ing *extensions.Ingress) error {


I feel like there is a better way to do this. You should probably fail at validation if there is service backend that does not have NEG enabled.

True, but I do need to also handle the default backend case

freehan · 2019-07-02T23:49:04Z

pkg/firewalls/firewalls.go

@@ -78,6 +78,7 @@ func (fr *FirewallRules) Sync(nodeNames, additionalPorts []string) error {
 	sort.Strings(targetTags)

 	ports := sets.NewString(additionalPorts...)
+	fr.srcRanges = append(fr.srcRanges, additionalRanges...)


do we want to have a separate firewall rule for ILB?

it's not a separate firewall rule, rather adding the ILB range to the firewall rule since that is a requirement.

Oh I think I misunderstood your question. It's possible to add another rule but I'm not sure if that would make much of a difference.

It could make the features more error independent. For instance, you could get an error updating ILB ranges, but still successfully update NEG

Hmm, good point, I'll look at adding this today.

This looks like it will require non-trivial refactoring since we currently only support one firewall. It's still possible, but it might make more sense to just update the src ranges separately to catch two different errors.

bowei

It feels like we need more unit tests?

pkg/backends/backends.go

pkg/backends/features/features.go

pkg/backends/syncer.go

pkg/backends/syncer_test.go

pkg/healthchecks/healthchecks.go

pkg/loadbalancers/features/l7ilb.go

pkg/utils/utils.go

spencerhance · 2019-07-19T00:58:04Z

/hold cancel

pkg/utils/utils.go

pkg/loadbalancers/l7.go

pkg/loadbalancers/features/l7ilb.go

pkg/healthchecks/healthchecks.go

pkg/firewalls/controller.go

pkg/backends/syncer_test.go

pkg/composite/utils.go

bowei · 2019-07-23T21:28:33Z

pkg/composite/utils.go

-	versions := []meta.Version{meta.VersionGA}
-	keys := []*meta.Key{key1}
+	versions := []meta.Version{meta.VersionGA, meta.VersionAlpha}
+	keys := []*meta.Key{key1, key2}

 	for i := range versions {
 		list, err := ListBackendServices(gceCloud, keys[i], versions[i])


This still seems quite strange to me that the list operation takes a key that it then only looks @ the scope for.

I agree, there's a TODO in pkg/composite/gen/main.go from when we last discussed it. I can update here or in another PR depending on your preference

pkg/firewalls/firewalls_test.go

bowei · 2019-07-23T21:37:04Z

pkg/loadbalancers/features/l7ilb.go

+			return subnet.IpCidrRange, nil
+		}
+	}
+	return "", fmt.Errorf("L7 ILB Subnet not found in region: %s", region)


You could do

var ErrSubnetNotFound = errors.New(""primary subnet not found")

bowei

Fix the remaining issues and we can merge this. There are a lot of TODOs, but should pick them up in follow on PRs.

bowei · 2019-07-26T16:34:14Z

pkg/annotations/ingress.go

@@ -54,6 +54,7 @@ const (
 	IngressClassKey      = "kubernetes.io/ingress.class"
 	GceIngressClass      = "gce"
 	GceMultiIngressClass = "gce-multi-cluster"
+	GceL7ILBIngressClass = "gce-l7-ilb"


Let's call this "gce-ilb". L7 is already implicit in the API.

Changed it to "gce-internal," which I believe was the original name fwiw

bowei · 2019-07-26T16:37:22Z

pkg/backends/backends.go

 	}

 	if err != nil || len(hs.HealthStatus) == 0 || hs.HealthStatus[0] == nil {
-		return "Unknown"
+		return "Unknown", fmt.Errorf("error getting health for backend %s: %v", name, err)


for strings that come from input, use %q

bowei · 2019-07-29T22:36:55Z

pkg/backends/backends.go

+	if flags.F.EnableL7Ilb {
+		backends, err = composite.ListAllBackendServices(b.cloud)
+	} else {
+		backends, err = composite.ListBackendServices(b.cloud, meta.GlobalKey(""), meta.VersionGA)


Add a TODO that this needs to be changed to not take a key

bowei · 2019-07-29T22:44:24Z

pkg/composite/utils.go

 func ListAllUrlMaps(gceCloud *gce.Cloud) ([]*UrlMap, error) {
 	resultMap := map[string]*UrlMap{}
 	key1, err := CreateKey(gceCloud, "", meta.Global)
 	if err != nil {
 		return nil, err
 	}
+	key2, err := CreateKey(gceCloud, "", meta.Regional)


// TODO(shance): this all needs to be replaced with `scope` and not key. globalKey, err := CreateKey(gceCloud, "", meta.Global) if err != nil { return nil, err } regionalKey, err := ... for _, vk := range []struct { v meta.Version k meta.Key // TODO: change this to scope } { {meta.VersionGA, GlobalKey}, {meta.VersionAlpha, RegionalKey}, } { ... }

bowei · 2019-07-29T22:45:45Z

pkg/firewalls/controller.go

@@ -163,9 +170,13 @@ func (fwc *FirewallController) sync(key string) error {
 		return err
 	}
 	negPorts := fwc.translator.GatherEndpointPorts(gceSvcPorts)
+	additionalRanges, err := fwc.ilbFirewallSrcRanges(gceIngresses)


flag gate this?

bowei · 2019-07-29T22:46:19Z

pkg/firewalls/controller.go

@@ -123,7 +126,11 @@ func (fwc *FirewallController) ToSvcPorts(ings []*extensions.Ingress) []utils.Se
 	var knownPorts []utils.ServicePort
 	for _, ing := range ings {
 		urlMap, _ := fwc.translator.TranslateIngress(ing, fwc.ctx.DefaultBackendSvcPortID)
-		knownPorts = append(knownPorts, urlMap.AllServicePorts()...)
+		svcPorts := urlMap.AllServicePorts()
+		if utils.IsGCEL7ILBIngress(ing) {


this should be if enableILBL7 && utils.IsGCEL7Ingress(ing) so it's completely disabled

bowei · 2019-07-29T22:46:44Z

pkg/firewalls/controller.go

+		for _, ing := range gceIngresses {
+			if utils.IsGCEL7ILBIngress(ing) {
+				ilbEnabled = true
+			}


bowei · 2019-07-29T22:48:14Z

pkg/firewalls/controller.go

+func (fwc *FirewallController) ilbFirewallSrcRanges(gceIngresses []*extensions.Ingress) ([]string, error) {
+	var additionalRanges []string
+	ilbEnabled := false
+	if flags.F.EnableL7Ilb {


I don't like hiding the enablement inside the function. Then when we unit test this, it has different behavior based on a global variable flag

bowei · 2019-07-29T22:48:50Z

pkg/firewalls/controller.go

+		if err != nil {
+			return nil, fmt.Errorf("error unable to get ILB subnet source ranges: %v", err)
+		}
+		additionalRanges = append(additionalRanges, L7ILBSrcRange)


no need to append if we are not in a loop

return L7ILBSrcRange, nil

bowei · 2019-07-29T22:53:11Z

pkg/loadbalancers/features/features_test.go

@@ -46,10 +48,37 @@ var (
 		meta.Zonal:    {fakeZonalFeature},
 	}

-	fakeVersionToFeatures = map[meta.Version][]string{


you can reduce verbosity here:

makeEntry := func(ga, alpha, beta meta.Version) { return &map[meta.Version]{ meta.VersionGA: {ga}, meta.VersionAlpha:{alpha}, meta.VersionBeta:{beta} } } ... UrlMap: ..., ForwardingRule: makeEntry(fakeGaFeature, fakeAlphaFeature, fakeBetaFeature),

bowei · 2019-07-31T22:49:20Z

pkg/backends/ig_linker.go

@@ -94,6 +94,7 @@ func (l *instanceGroupLinker) Link(sp utils.ServicePort, groups []GroupKey) erro
 	}

 	// ig_linker only supports L7 HTTP(s) External Load Balancer
+	// Hardcoded here since IGs are not supported for non GA-Global right now


you need to add a TODO here so we can find this later

Intiial support for L7-ILB and corresponding ingress class "gce-internal." Functionality is flag-gated with "--enable-l7-ilb".

bowei · 2019-07-31T22:58:58Z

pkg/healthchecks/healthchecks.go

 	ForNEG bool
+	// ForILB designates whether the health check is for an ILB
+	// This means that the HC should be alpha + regional


Are you going to remove the comment?

bowei · 2019-07-31T23:11:48Z

pkg/utils/utils_test.go

+			desc: "foo ingress class",
+			ingress: &v1beta1.Ingress{
+				ObjectMeta: v1.ObjectMeta{
+					Annotations: map[string]string{


either do

{ ... }

or

&blah{\n ...\n },\n

don't do

&blah{\n ...},

bowei · 2019-07-31T23:12:50Z

/lgtm
/approve

there are a ton of TODOs, but let's merge this for now and handle things in addons. The github UI is not handling PRs of this size very easily.

k8s-ci-robot · 2019-07-31T23:13:07Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [bowei]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

hach-que · 2019-09-14T18:05:09Z

Is there a way to get this working on Google Kubernetes Engine?

I added the annotation kubernetes.io/ingress.class: "gce-internal" to an ingress on GKE, but it doesn't seem to be doing anything.

Do I have to manually deploy ingress-gce to the cluster, and disable the built-in version?

hach-que · 2019-09-15T03:05:35Z

For anyone trying to get this working on Google Kubernetes Engine, you need to:

Make sure your GKE cluster is on 1.14. 1.13 definitely does not work.
Ask your Google support channel or technical account manager to enable Alpha Access on the Compute API for the projects you want to deploy this in. This is because the GA and beta APIs do not yet surface subnets reserved for internal HTTPS load balancing (more on that in a bit).
- If you're desperate to get around this and you're only using ILBs in one region, you can override the implementation of ILBSubnetSourceRange in i7ilb.go to just straight up return the CIDR of the subnet you reserve in step 5.
Before running the setup script linked below, modify glbc.yaml and add the --enable-l7-ilb=true argument after --enable-csm=[ENABLE_CSM].
Follow the instructions here: https://github.com/kubernetes/ingress-gce/blob/master/docs/deploy/gke/README.md
I had to make some fixes to the script e.g. to support multiple node pools and to make API server detection more reliable, which can be found in our fork: https://github.com/networknext/ingress-gce/blob/master/docs/deploy/gke/gke-self-managed.sh
You will need to remove the IAM permission from the GLBC service account and re-add Compute Admin permission. For some reason, the controller kept saying access denied until we did this.
In the Google Cloud Console, go to add a new HTTPS load balancer, select "Only between my VMs", select a specific region, then you will be prompted to "Reserve subnet". Click that and choose a CIDR mask that doesn't overlap with an existing subnet (e.g. 10.28.0.0/20). Give it whatever name you like. Finish creating the load balancer (just pick whatever), and then delete it after it's spun up.
If you rolled out your ingress before this point, you need to delete it as the GCE ingress controller won't have created it properly.
Make sure your ingress is using networking.k8s.io/v1beta1, e.g. it should look something like this:

apiVersion: networking.k8s.io/v1beta1
kind: Ingress
metadata:
  name: blah
  annotations:
    kubernetes.io/ingress.allow-http: "false"   
    kubernetes.io/ingress.class: "gce-internal"
spec:
  backend:
    serviceName: blah
    servicePort: grpc

spencerhance · 2019-09-18T21:48:40Z

@hach-que I'm glad you were able to to get it working! As you figured out, the feature hasn't been released to GKE yet but can be used if you deploy the controller yourself. As for step 3, you can actually avoid enabling CSM by adding the enable NEG annotation to your k8s service. Here is an example service yaml:

apiVersion: v1
kind: Service
metadata:
  name: internal-lb-svc
  namespace: default
  annotations:
    cloud.google.com/neg: '{"ingress": true}'
spec:
  ports:
  - name: host1
    port: 80
    protocol: TCP
    targetPort: 9376
  selector:
    run: internal-lb-app
  type: ClusterIP

Also feel free to file issues if you notice any bugs 😃

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 29, 2019

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 29, 2019

k8s-ci-robot requested review from bowei and MrHohn June 29, 2019 03:16

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 29, 2019

spencerhance mentioned this pull request Jun 29, 2019

Add support for L7-ILB #773

Closed

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 29, 2019

spencerhance force-pushed the add-l7-ilb-take-2 branch from 45048ab to bc5d8ee Compare July 1, 2019 20:12

k8s-ci-robot requested a review from freehan July 1, 2019 20:36

spencerhance force-pushed the add-l7-ilb-take-2 branch from bc5d8ee to be2aca1 Compare July 2, 2019 18:31

freehan reviewed Jul 2, 2019

View reviewed changes

bowei reviewed Jul 3, 2019

View reviewed changes

spencerhance force-pushed the add-l7-ilb-take-2 branch 11 times, most recently from b5c84d4 to 2872362 Compare July 8, 2019 21:33

spencerhance force-pushed the add-l7-ilb-take-2 branch from 20b3584 to 3592c71 Compare July 19, 2019 00:14

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 19, 2019

spencerhance mentioned this pull request Jul 22, 2019

L7-ILB Support Tracking #731

Closed

10 tasks

bowei reviewed Jul 23, 2019

View reviewed changes

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 23, 2019

bowei reviewed Jul 23, 2019

View reviewed changes

spencerhance force-pushed the add-l7-ilb-take-2 branch from 9e4afb7 to d5a2cbf Compare July 25, 2019 21:49

bowei reviewed Jul 29, 2019

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 30, 2019

spencerhance force-pushed the add-l7-ilb-take-2 branch from d5a2cbf to 9831d0b Compare July 30, 2019 20:21

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 30, 2019

spencerhance force-pushed the add-l7-ilb-take-2 branch 2 times, most recently from afae10b to 2de3d93 Compare July 30, 2019 21:53

bowei reviewed Jul 31, 2019

View reviewed changes

Add L7-ILB Support

596af29

Intiial support for L7-ILB and corresponding ingress class "gce-internal." Functionality is flag-gated with "--enable-l7-ilb".

spencerhance force-pushed the add-l7-ilb-take-2 branch from 2de3d93 to 596af29 Compare July 31, 2019 22:51

bowei reviewed Jul 31, 2019

View reviewed changes

k8s-ci-robot assigned bowei Jul 31, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 31, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 31, 2019

k8s-ci-robot merged commit 8458c23 into kubernetes:master Jul 31, 2019

spencerhance deleted the add-l7-ilb-take-2 branch July 31, 2019 23:31

freehan mentioned this pull request Sep 20, 2019

NEG controller should create NEG for default backend when enabled #767

Closed

Add support for L7-ILB (take 2) #789

Add support for L7-ILB (take 2) #789

Conversation

spencerhance commented Jun 29, 2019 • edited Loading

k8s-ci-robot commented Jun 29, 2019

rramkumar1 commented Jun 29, 2019

spencerhance commented Jul 1, 2019

rramkumar1 commented Jul 2, 2019

spencerhance commented Jul 2, 2019

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

bowei left a comment

Choose a reason for hiding this comment

spencerhance commented Jul 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowei 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

bowei commented Jul 31, 2019

k8s-ci-robot commented Jul 31, 2019

hach-que commented Sep 14, 2019

hach-que commented Sep 15, 2019 • edited Loading

spencerhance commented Sep 18, 2019

spencerhance commented Jun 29, 2019 •

edited

Loading

hach-que commented Sep 15, 2019 •

edited

Loading