Ingress HTTP/2 Support #146

agau4779 · 2018-03-08T18:52:00Z

Adds HTTP2 annotation to Service
Creates alpha HealthCheck and BackendService resources when the protocol is HTTP/2, by using a composite BackendService type
Tests for HealthCheck/BackendServices using HTTP/2

agau4779 · 2018-03-08T18:52:46Z

bowei · 2018-03-08T19:28:13Z

pkg/backends/backends.go

@@ -431,7 +457,7 @@ func (b *Backends) edgeHop(be *compute.BackendService, igs []*compute.InstanceGr
 		newBackends := getBackendsForIGs(addIGs, bm)
 		be.Backends = append(originalIGBackends, newBackends...)

-		if err := b.cloud.UpdateGlobalBackendService(be); err != nil {
+		if err := b.cloud.UpdateAlphaGlobalBackendService(be); err != nil {


alpha cannot be used on the beta, ga paths

bowei · 2018-03-08T19:28:29Z

pkg/backends/backends.go

@@ -388,7 +414,7 @@ func getBackendsForNEGs(negs []*computealpha.NetworkEndpointGroup) []*computealp

 // edgeHop checks the links of the given backend by executing an edge hop.
 // It fixes broken links.
-func (b *Backends) edgeHop(be *compute.BackendService, igs []*compute.InstanceGroup) error {
+func (b *Backends) edgeHop(be *computealpha.BackendService, igs []*compute.InstanceGroup) error {


we should probably keep the alpha types off of the ga paths.

bowei · 2018-03-08T19:29:58Z

pkg/backends/backends.go

 	if be == nil {
 		namedPort := &compute.NamedPort{
 			Name: b.namer.NamedPort(p.NodePort),
 			Port: p.NodePort,
 		}
 		glog.V(2).Infof("Creating backend service for port %v named port %v", p.NodePort, namedPort)
-		be, err = b.create(namedPort, hcLink, p, beName)
+		be, err = b.createAlpha(namedPort, hcLink, p, beName)


we can't call alpha on the GA, beta paths.

bowei

We need to keep the alpha API by itself, alpha APIs have no SLO and are whitelist only.

freehan

does it need to be feature gated? or always turning it on is okay.

agau4779 · 2018-03-09T23:48:25Z

@freehan - we're going to Feature Gate this.

nicksardo · 2018-03-13T15:21:43Z

/ok-to-test

freehan · 2018-03-14T22:04:24Z

pkg/backends/backends.go

 	}

-	// If NEG is enabled, do not link backend service to instance groups.
-	if p.NEGEnabled {
+	return b.Update(be)


This seems wrong. It will always update backend service. Please only update when necessary

Move this back to above

freehan · 2018-03-14T22:08:40Z

pkg/backends/backends.go

 	beIGs := sets.String{}
-	for _, beToIG := range be.Backends {
-		beIGs.Insert(beToIG.Group)
+	if be.isAlpha {


Do you have to distinguish between Alpha and GA backend services here?

Or consider have some helper function with BackendService struct

IIUC, the URLs are the same except for the API version part.

freehan · 2018-03-14T22:12:04Z

pkg/backends/backends.go

+func (b *Backends) edgeHopAndUpdate(be *BackendService, igs []*compute.InstanceGroup) error {
+	addIGs := getInstanceGroupsToAdd(be, igs)
+	if len(addIGs) == 0 {
+		return b.Update(be)


If no Instance Group needs to be added, why do you need to update?

edgeHop is mostly for linking backends and backend service

Oops, good point. Removing

freehan · 2018-03-14T22:15:06Z

pkg/backends/backends.go

+	if existingProtocol != string(p.Protocol) || existingHCName != expectedHCName || existingDescription != p.Description() {
+		glog.V(2).Infof("Updating backend protocol %v (%v) for change in protocol (%v) or health check", beName, existingProtocol, string(p.Protocol))
+		if be.isAlpha {
+			be.Alpha.Protocol = string(p.Protocol)


worth wraping this into a separate function that ensures protocol, hclink and description in a be

freehan · 2018-03-14T22:16:19Z

pkg/backends/backends.go

+	for _, bm := range []BalancingMode{Rate, Utilization} {
+		// Generate backends with given instance groups with a specific mode
+		if be.isAlpha {
+			originalIGBackends := []*computealpha.Backend{}


wrap this into a separate function to generate corresponding backends?

freehan · 2018-03-14T22:19:23Z

pkg/backends/backends.go

@@ -90,6 +90,14 @@ type Backends struct {
 	namer        *utils.Namer
 }

+// BackendService embeds both the GA and alpha compute BackendService types
+type BackendService struct {


Please add some helper functions to this struct. For instance,

Ensure protocol, healthcheck and description, (return if there is difference)

Ensure Backends (check if current backend instance groups are super set)

freehan · 2018-03-14T22:23:14Z

pkg/backends/backends.go

+}
+
+// Update calls either the GA or Alpha update path depending on Protocol
+func (b *Backends) Update(be *BackendService) error {


Move the log line here to log it is updating backend service

Does this function has to be public?

Opted to add new logging for update here - f6ca1c7

freehan · 2018-03-14T22:27:36Z

pkg/healthchecks/healthchecks.go

-	if newHC.ForNEG {
+	if newHC.isHttp2() {
+		glog.V(2).Infof("Updating health check with protocol %v", newHC.Type)
+		return h.cloud.UpdateAlphaHealthCheck(newHC.ToAlphaComputeHealthCheck())


Probably need to copy the same pattern for NEG health checks. Need to ensure existing settings are preserved.
@nicksardo Do you think it is necessary for HTTP2?

Yea, settings should absolutely be preserved as they are done today. IIRC, we change some parameters for NEGs, but never the path value.

We'll also need to consider the case of NEGs + HTTP2.

@agau4779 need to rethink the logic here.

Need to handle PortSpecification + HTTP2

Need to preserve whatever user healthcheck configuration even if protocol or NEG setting changes.

freehan · 2018-03-16T22:11:01Z

pkg/backends/backends.go

+
+	// If the Protocol is empty, this means this is a alpha BackendService and
+	// Protocol is expected to be HTTP2
+	if beGa.Protocol == "" {


Is there any possibility that Protocol is empty and GetAlphaGlobalBackendService keeps returning error?

GetGlobalBackendService gets the GA BackendService but returns an empty string in the Protocol if the Protocol is not HTTP or HTTPS - so here, we're anticipating that the user created an Alpha BackendService with an HTTP2 protocol (currently alpha-only), so calling the GA path will return a blank protocol.

GetAlphaGlobalBackendService may run into an isForbidden error if the user is no longer Alpha whitelisted - in which case, might cause the Ingress controller to be stuck forever. Will add this warning in a code comment.

freehan · 2018-03-16T22:12:09Z

pkg/backends/backends.go

-	if err := b.cloud.CreateGlobalBackendService(bs); err != nil {
-		return nil, err
+
+	bsAlpha := &computealpha.BackendService{}


move to the following else block

freehan · 2018-03-16T22:18:08Z

pkg/backends/backends.go

-		be.HealthChecks = []string{hcLink}
-		be.Description = p.Description()
-		if err = b.cloud.UpdateGlobalBackendService(be); err != nil {
+	existingProtocol := be.Ga.Protocol


existingProtocol := be.GetProtocol()
existingProtocol := be.GetDescription()

freehan · 2018-03-16T22:21:10Z

pkg/backends/backends.go

+	}
+
+	if existingProtocol != string(p.Protocol) || existingHCName != expectedHCName || existingDescription != p.Description() {
+		glog.V(2).Infof("Updating backend protocol %v (%v) for change in protocol (%v) or health check", beName, existingProtocol, string(p.Protocol))


remove this since b.update() has a log already.

freehan · 2018-03-16T22:25:30Z

pkg/backends/backends.go

+
+	if existingProtocol != string(p.Protocol) || existingHCName != expectedHCName || existingDescription != p.Description() {
+		glog.V(2).Infof("Updating backend protocol %v (%v) for change in protocol (%v) or health check", beName, existingProtocol, string(p.Protocol))
+		be.ensureProtocol(p, hcLink)


needUpdate := be.ensureProtocol(p.Protocol) needUpdate = needUpdate | be.ensureHealthCheck(hcLink) needUpdate = needUpdate | be.ensureDescription(p.Description()) if needUpdate { if err = b.update(be); err != nil { return err } }

or

if be.ensureConfig(p, hcLink) { if err = b.update(be); err != nil { return err } }

ensureConfig returns true if existing parameter does not match expected. Move all comparison between existing vs. expected into the helper functions.

freehan · 2018-03-16T22:27:18Z

pkg/backends/backends.go

@@ -277,8 +351,11 @@ func (b *Backends) ensureBackendService(p ServicePort, igs []*compute.InstanceGr

 	// Check that the backend service has the correct protocol and health check link
 	existingHCLink := ""


existingHCLink := be.GetHealthCheckLink()

freehan · 2018-03-16T22:31:25Z

pkg/backends/backends.go

+}
+
+// generateBackends generates backends with given instance groups with a specific mode
+func (be *BackendService) generateBackends(bm BalancingMode, addIGs []*compute.InstanceGroup) {


rename to ensureBackends?

freehan · 2018-03-16T22:37:46Z

pkg/backends/backends.go

 	}
+
 	igLinks := sets.String{}


s/igLinks/expectedIGs

freehan · 2018-03-16T22:40:10Z

pkg/healthchecks/healthchecks.go

-	if newHC.ForNEG {
+	if newHC.isHttp2() {
+		glog.V(2).Infof("Updating health check with protocol %v", newHC.Type)
+		return h.cloud.UpdateAlphaHealthCheck(newHC.ToAlphaComputeHealthCheck())


@agau4779 need to rethink the logic here.

Need to handle PortSpecification + HTTP2

Need to preserve whatever user healthcheck configuration even if protocol or NEG setting changes.

bowei · 2018-03-10T00:06:36Z

pkg/backends/backends.go

+	Alpha *computealpha.BackendService
+	Ga    *compute.BackendService
+	// Flag to indicate we want to use the Alpha compute type
+	isAlpha bool


can we have Alpha == nil denote that alpha compute type is used?

is there a possibility of isAlpha == true, but Alpha == nil?

There is not - when getting an Alpha BackendService, or promoting an GA BackendService to Alpha, isAlpha is set to true. Looks like it's redundant if we can just check BackendService.Alpha == nil - removing.

freehan · 2018-03-22T18:06:09Z

pkg/backends/backends.go

-	// If NEG is enabled, do not link backend service to instance groups.
-	if p.NEGEnabled {
+	var errs []string
+	for _, bm := range []BalancingMode{Rate, Utilization} {


move the original comments here.

freehan · 2018-03-22T18:12:59Z

pkg/backends/backends.go

+}
+
+// ensureProtocol updates the BackendService Protocol with the expected value
+func (be *BackendService) ensureProtocol(p ServicePort, hcLink string) (needsUpdate bool) {


hcLink not used right?

freehan · 2018-03-22T18:15:01Z

pkg/backends/backends.go

+	}
+
+	if be.Alpha != nil {
+		be.Alpha.Protocol = string(p.Protocol)


IIUC, if the existing backend-service is using HTTP2. If the new protocol set to HTTP. Then it will set protocol on the alpha object. Hence, during update, it will call Alpha API to update backend service.

Try to avoid calling alpha API as much as possible. SLA is different.

Hmm, then should we convert it to a Ga backend service in this function?
e.g.

if p.Protocol != annotations.ProtocolHTTP2 { be.Ga = toV1BackendService(be.Alpha) be.Alpha = nil }

freehan · 2018-03-22T18:17:34Z

pkg/healthchecks/healthchecks.go

@@ -146,9 +147,12 @@ func (h *HealthChecks) create(hc *HealthCheck) error {
 }

 func (h *HealthChecks) update(oldHC, newHC *HealthCheck) error {
-	if newHC.ForNEG {
+	if newHC.isHttp2() {


merge NEG and isHttp2 conditions

freehan · 2018-03-22T18:22:54Z

Please also unit test the ensurexxxxx methods.

…ompare InstanceGroups by comparable name instead of full URL

freehan · 2018-03-23T20:31:43Z

pkg/backends/backends.go

+			return nil, err
+		}
+	} else {
+		bsAlpha := toAlphaBackendService(bs)


toAlphaBackendService is mostly for testing.

Are you confident about its reliability for production use?

freehan · 2018-03-23T20:34:04Z

pkg/backends/backends.go

+
+	// Using the HTTP2 Protocol requires that the BackendService is alpha.
+	// If the BackendService wasn't originally alpha, convert it to Alpha.
+	if be.Alpha == nil && p.Protocol == annotations.ProtocolHTTP2 {


Probably do this: ensureProtocol just ensures the protocol for both GA and Alpha objects if exists.

In update, pick the api to call.

freehan · 2018-03-27T23:55:37Z

pkg/backends/backends.go

+	}
+
+	if be.Alpha != nil {
+		be.Alpha.HealthChecks = []string{hcLink}


Is it possible that alpha backend-service points to GA health check link? Or reverse (GA backend-service point to Alpha Health check)

IIRC, this is actually okay. But need to confirm.

Shouldn't be possible - GetHealthCheckLink performs a check - if be.Alpha != nil && len(be.Alpha.HealthChecks) == 1 then hcLink will return an Alpha healthcheck link; otherwise a Ga link is returned.

The compute API currently returns the alpha version of the Healthcheck link through the Alpha API and the GA version of the link through the GA API (gcloud [alpha] compute backend-services describe [service-name]), so I agree that it should be ok.

freehan · 2018-03-28T00:03:25Z

pkg/backends/backends.go

-		existingHCLink = be.HealthChecks[0]
-	}
+	needUpdate := be.ensureProtocol(p)
+	needUpdate = needUpdate || be.ensureHealthCheckLink(hcLink)


quick question:
GA health checks supports HTTP2?

GA Healthchecks do not support HTTP2. HTTP2 healthchecks are created through the Alpha API.

freehan · 2018-03-28T00:34:35Z

pkg/healthchecks/healthchecks.go

 // This is to preserve the existing health check setting as much as possible.
 // WARNING: if a service backend is converted from IG mode to NEG mode,
 // the existing health check setting will be preserve, although it may not suit the customer needs.
-func mergeHealthcheckForNEG(oldHC, newHC *HealthCheck) *HealthCheck {
+func mergeHealthcheck(oldHC, newHC *HealthCheck) *HealthCheck {


Seems incorrect. Does it preserve configs if oldHC is HTTPS or HTTP2?

freehan · 2018-03-28T00:37:40Z

pkg/backends/backends.go

+
+// edgeHop checks the links of the given backend by executing an edge hop.
+// It fixes broken links and updates the Backend accordingly.
+func (b *Backends) edgeHop(be *BackendService, igs []*compute.InstanceGroup) error {


Cannot think of a better way to avoid duplicate code.

Do you think it is worthwhile to hide the complexity behind ensureInstanceGroupBackends? And do something clever behind the scene and unit test it thoroughly.

Is it possible to only use GA backend service to ensure backends?

Will it overwrite the HTTP2 config?

If GA Backend-service set to HTTP2, you probably gets a validation error.

But if you get a GA backend-service, and update its backend without touching protocol. What will happen?

We can't use the GA BackendService API. Verified that setting the Protocol on an HTTP or HTTPS BackendService to HTTP2 causes a validation error, and that modifying any other attribute on an HTTP2 BackendService causes the BackendService's Protocol to default to HTTP. The Sync loop eventually calls backends.go#Ensure then causes it to revert back to HTTP2.

freehan · 2018-03-30T23:48:09Z

LGTM

@bowei @nicksardo do you want another look?

freehan · 2018-03-30T23:51:08Z

/lgtm

Automatic merge from submit-queue (batch tested with PRs 62208, 62114, 62144, 60460, 62214). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. add unreleased tag to http2 test **What this PR does / why we need it**: [HTTP2](kubernetes/ingress-gce#146) isn't yet in an Ingress release. Add Unreleased label to disable this test for gci-gke-ingress until Ingress gets a new release. **Release note**: ```release-note NONE ```

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 8, 2018

k8s-ci-robot assigned bowei and nicksardo Mar 8, 2018

bowei reviewed Mar 8, 2018

View reviewed changes

freehan reviewed Mar 8, 2018

View reviewed changes

agau4779 force-pushed the http2 branch from 785d4fc to dcdf6f2 Compare March 9, 2018 23:43

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 13, 2018

munjalpatel mentioned this pull request Mar 14, 2018

Add example using grpc and http2 #18

Closed

freehan reviewed Mar 14, 2018

View reviewed changes

agau4779 force-pushed the http2 branch 2 times, most recently from ea42fc7 to 6aa47f9 Compare March 15, 2018 20:12

freehan reviewed Mar 16, 2018

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 19, 2018

add http2 annotation to service

59f3bdf

agau4779 force-pushed the http2 branch from c87516f to 3857af5 Compare March 19, 2018 20:37

agau4779 force-pushed the http2 branch from 9db0892 to 4955018 Compare March 19, 2018 22:09

bowei reviewed Mar 19, 2018

View reviewed changes

This was referenced Mar 20, 2018

Add Ingress HTTP2 feature gate #161

Merged

[GCE] Add IngressHttp2 feature gate kubernetes/kubernetes#61422

Closed

freehan reviewed Mar 22, 2018

View reviewed changes

agau4779 added 4 commits March 22, 2018 11:53

add http2 compatibility to healthchecks

a35cd14

use composite type BackendService, to house ga/alpha versions

81b0fd7

refactor getting/setting BackendService fields into struct methods. c…

6d48fae

…ompare InstanceGroups by comparable name instead of full URL

tests for HTTP2 BackendService

4fc1249

agau4779 force-pushed the http2 branch 2 times, most recently from a2272b3 to 501b19b Compare March 23, 2018 00:31

freehan reviewed Mar 23, 2018

View reviewed changes

agau4779 force-pushed the http2 branch 3 times, most recently from 52b9572 to 76405ba Compare March 27, 2018 17:29

freehan reviewed Mar 28, 2018

View reviewed changes

remove isAlpha flag from BackendService. add isAlpha arg to Get. +tests

2959f6a

agau4779 force-pushed the http2 branch from 76405ba to 2959f6a Compare March 28, 2018 19:21

agau4779 added 3 commits March 28, 2018 15:28

add getAlphaInstanceGroupsToAdd

c839d2a

merge Port and PortSpecification differently depending on ForNEG

b462cdc

remove getAlphaInstanceGroupsToAdd

e1ac2c3

agau4779 force-pushed the http2 branch from 997eb13 to e1ac2c3 Compare March 30, 2018 23:37

k8s-ci-robot assigned freehan Mar 30, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 30, 2018

freehan merged commit 9857d92 into kubernetes:master Apr 2, 2018

agau4779 mentioned this pull request Apr 6, 2018

add unreleased tag to http2 test kubernetes/kubernetes#62208

Merged

agau4779 deleted the http2 branch April 9, 2018 21:25

EronWright mentioned this pull request Oct 6, 2018

Support external connectivity pravega/pravega-operator#61

Closed

		@@ -277,8 +351,11 @@ func (b Backends) ensureBackendService(p ServicePort, igs []compute.InstanceGr

		// Check that the backend service has the correct protocol and health check link
		existingHCLink := ""

Ingress HTTP/2 Support #146

Ingress HTTP/2 Support #146

Conversation

agau4779 commented Mar 8, 2018 • edited Loading

agau4779 commented Mar 8, 2018

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

freehan left a comment

Choose a reason for hiding this comment

agau4779 commented Mar 9, 2018

nicksardo commented Mar 13, 2018

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

freehan Mar 16, 2018 • 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

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

freehan commented Mar 22, 2018

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

freehan commented Mar 30, 2018

freehan commented Mar 30, 2018

agau4779 commented Mar 8, 2018 •

edited

Loading

freehan Mar 16, 2018 •

edited

Loading