Application Load Balancer Support for End-to-End HTTP/2 #458

universam1 · 2021-12-09T10:48:44Z

ref: https://aws.amazon.com/de/blogs/aws/new-application-load-balancer-support-for-end-to-end-http-2-and-grpc/

Currently HTTP/2 is only half way implemented, that is the ALB can be configured for HTTP/2 but the TargetGroup is not configured.
This is probably due to the AWS delay of supporting that protocal version for the TargetGroup which is available in meantime.

There is however an update missing for mweagle/go-cloudformation which is deprecated, thus a fork has been made that includes this Cloudformation feature: o11n/go-cloudformation@b975e65

fixes #391

universam1 · 2021-12-09T10:49:56Z

@szuecs I assume Zalando would be more inclined to fork mweagle/go-cloudformation themselves and apply the mentioned patch there?

AlexanderYastrebov · 2021-12-09T11:21:34Z

aws/cf_template.go

+		if spec.http2 {
+			targetGroup.ProtocolVersion = cloudformation.String("HTTP2")
+		} else {
+			targetGroup.ProtocolVersion = cloudformation.String("HTTP1")
+		}


spec.http2 is true by default. If I get it right this change would enable HTTP/2 on the target group and according to this table https://docs.aws.amazon.com/elasticloadbalancing/latest/application/load-balancer-target-groups.html#target-group-protocol-version

Request protocol Protocol version Result

HTTP/1.1 HTTP/2 Error

HTTP/2 HTTP/2 Success

this change

is not backward-compatible as default protocol version is HTTP/1.1

would not work if target supports only HTTP/1.1

will disable HTTP/1.1 incoming requests

If that is correct then we may probably need to have a new flag and/or annotation to steer Target Group protocol version independently and backward-compatible.

Besides that HTTP/2 is only supported in conjunction with HTTPS protocol so this kind of config should be around here

kube-ingress-aws-controller/aws/cf_template.go

Lines 455 to 458 in 5c66137

} else if spec.targetHTTPS {

protocol = "HTTPS"

healthCheckProtocol = "HTTPS"

}

This is true, the http/2 TG protocol is a breaking change for http/1.1 clients. Looking for a procedure how to deal with while supporting end-to-end, found this statement: https://stackoverflow.com/a/66800142

Probably going to open a case at AWS

well known issue: https://forums.aws.amazon.com/message.jspa?messageID=985494

It also shows the complexity that everything is based on the client. I am thinking for a while to support h2c in skipper, but the current implementations have some critical bugs and if we would be able to serve it then I also do not know if grpc backends can listen on handlers without TLS....

@AlexanderYastrebov interestingly the protocol H2 can be set via AWS console also for non-https target groups, also I found that documented in the TF provider https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/lb_target_group#protocol_version

Nevertheless I implemented as requested

AlexanderYastrebov · 2021-12-09T11:28:15Z

I assume Zalando would be more inclined to fork mweagle/go-cloudformation themselves and apply the mentioned patch there?

The https://github.com/mweagle/go-cloudformation#this-repo-is-no-longer-supported suggests to move to https://github.com/awslabs/goformation which is most likely will be a large undertaking

universam1 · 2021-12-09T14:51:01Z

I assume Zalando would be more inclined to fork mweagle/go-cloudformation themselves and apply the mentioned patch there?

The https://github.com/mweagle/go-cloudformation#this-repo-is-no-longer-supported suggests to move to https://github.com/awslabs/goformation which is most likely will be a large undertaking

This PR is naive mainly to trigger a decision where to go with the stale dependency

szuecs · 2021-12-09T15:35:53Z

In the long run we should migrate to https://github.com/awslabs/goformation. In general I don't mind to use a fork of an unmaintained library if this buys us some time to migrate to a more supported or better version.
@universam1 do you want to do ALB -> grpc-application or do you want to pass this through skipper?
For the latter I think there is some more work required for the former: why don't you use NLB instead?

universam1 · 2021-12-09T15:58:14Z

In the long run we should migrate to https://github.com/awslabs/goformation. In general I don't mind to use a fork of an unmaintained library if this buys us some time to migrate to a more supported or better version. @universam1 do you want to do ALB -> grpc-application or do you want to pass this through skipper? For the latter I think there is some more work required for the former: why don't you use NLB instead?

Primary goal is to support http/2 pass-thru or end-to-end. Currently that is cross converted loosing the main benefits, see https://stackoverflow.com/a/36057190

@szuecs do you suggest replacing ALBs with NLBs for that use case? Not much experience with that type of LB, sounds counter intuitive using an OSI 4 LB for OSI 7 application

ref: https://aws.amazon.com/de/blogs/aws/new-application-load-balancer-support-for-end-to-end-http-2-and-grpc/ Currently HTTP/2 is only half way implemented, that is the ALB can be configured for HTTP/2 but the TargetGroup is not configured. This is probably due to the AWS delay of supporting that protocal version for the TargetGroup which is available in meantime. There is however an update missing for `mweagle/go-cloudformation` which is deprecated, thus a fork has been made that includes this Cloudformation feature: o11n/go-cloudformation@b975e65 Signed-off-by: Samuel Lang <gh@lang-sam.de>

universam1 · 2021-12-10T10:10:37Z

@AlexanderYastrebov @szuecs despite of the AWS ALB compatibility limitations this PR is tested and it does work as expected. Added a feature toggle in order not to break compatibility.
Let me know if there is anything else, thanks

dhohengassner · 2021-12-10T10:30:56Z

aws/cf_template_test.go

@@ -614,6 +619,38 @@ func TestGenerateTemplate(t *testing.T) {
 				require.NotEqual(t, cloudformation.Integer(3), tg.UnhealthyThresholdCount)
 			},
 		},
+		{
+			name: "target port http2 when https listener and configured",


@universam1 should be there a test case for GRPC as well?

gRPC is using H2, so I guess this test is good enough, no?

dhohengassner · 2021-12-10T10:32:38Z

@universam1 thanks for implementing - good to have these additional options ready 👍

szuecs · 2021-12-10T12:28:56Z

@universam1 I just wanted to understand if possible options were thought about (NLB vs ALB with grpc TG). In general the less the load balancer has to do the better for latency. NLB would not break grpc I think, so that's why I asked. I have nothing against the PR itself and have no suggestions besides to answer other open comments.
Maybe @AlexanderYastrebov has more input.

universam1 · 2021-12-10T13:23:44Z

@AlexanderYastrebov @szuecs Sorry accidentally closed this PR by force pushing the commit, reopened here #460

szuecs · 2021-12-10T13:31:46Z

no worries

AlexanderYastrebov reviewed Dec 9, 2021

View reviewed changes

universam1 changed the title ~~Application Load Balancer Support for End-to-End HTTP/2~~ Draft: Application Load Balancer Support for End-to-End HTTP/2 Dec 9, 2021

AlexanderYastrebov marked this pull request as draft December 9, 2021 15:51

universam1 force-pushed the http2End2End branch 2 times, most recently from 6fb36ed to 8536f77 Compare December 10, 2021 10:00

universam1 changed the title ~~Draft: Application Load Balancer Support for End-to-End HTTP/2~~ Application Load Balancer Support for End-to-End HTTP/2 Dec 10, 2021

universam1 force-pushed the http2End2End branch from 8536f77 to fcf50d4 Compare December 10, 2021 10:08

dhohengassner reviewed Dec 10, 2021

View reviewed changes

universam1 closed this Dec 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Application Load Balancer Support for End-to-End HTTP/2 #458

Application Load Balancer Support for End-to-End HTTP/2 #458

universam1 commented Dec 9, 2021

universam1 commented Dec 9, 2021 •

edited

AlexanderYastrebov Dec 9, 2021

universam1 Dec 9, 2021

universam1 Dec 9, 2021

szuecs Dec 9, 2021

universam1 Dec 10, 2021

AlexanderYastrebov commented Dec 9, 2021

universam1 commented Dec 9, 2021

szuecs commented Dec 9, 2021 •

edited

universam1 commented Dec 9, 2021

universam1 commented Dec 10, 2021

dhohengassner Dec 10, 2021

szuecs Dec 10, 2021

dhohengassner commented Dec 10, 2021

szuecs commented Dec 10, 2021

universam1 commented Dec 10, 2021

szuecs commented Dec 10, 2021

	} else if spec.targetHTTPS {
	protocol = "HTTPS"
	healthCheckProtocol = "HTTPS"
	}

Application Load Balancer Support for End-to-End HTTP/2 #458

Application Load Balancer Support for End-to-End HTTP/2 #458

Conversation

universam1 commented Dec 9, 2021

universam1 commented Dec 9, 2021 • edited

AlexanderYastrebov Dec 9, 2021

Choose a reason for hiding this comment

universam1 Dec 9, 2021

Choose a reason for hiding this comment

universam1 Dec 9, 2021

Choose a reason for hiding this comment

szuecs Dec 9, 2021

Choose a reason for hiding this comment

universam1 Dec 10, 2021

Choose a reason for hiding this comment

AlexanderYastrebov commented Dec 9, 2021

universam1 commented Dec 9, 2021

szuecs commented Dec 9, 2021 • edited

universam1 commented Dec 9, 2021

universam1 commented Dec 10, 2021

dhohengassner Dec 10, 2021

Choose a reason for hiding this comment

szuecs Dec 10, 2021

Choose a reason for hiding this comment

dhohengassner commented Dec 10, 2021

szuecs commented Dec 10, 2021

universam1 commented Dec 10, 2021

szuecs commented Dec 10, 2021

universam1 commented Dec 9, 2021 •

edited

szuecs commented Dec 9, 2021 •

edited