-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Application Load Balancer Support for End-to-End HTTP/2 #460
base: master
Are you sure you want to change the base?
Conversation
controller.go
Outdated
if !strings.Contains("HTTP1 HTTP2 GRPC", targetGroupProtocolVersion) { | ||
return fmt.Errorf("invalid target group protocol version: %s. must be one of HTTP1, HTTP2, GRPC", targetGroupProtocolVersion) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to decide also if we would like to support ingress annotation in this scope to enable this individually.
The default value is defined as "HTTP1"
but before this change we did not set it so I am wondering if changing empty to "HTTP1"
will cause any issues (e.g. trigger loadbalancer recreation).
Lets check if we can use kingpin.Enum
. If no it would be still better to check three possible cases instead of this clever trick which e.g. allows "HTTP1 HTTP2"
or "TTP"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @szuecs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The update behavior of TG is recreating, which is causing a new resource to be created first and the replacing the link. But I'm fine to change accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about annotation support, since this change is invasive and potentially breaking HTTP1.1 clients, why I would see this as a central administrative decision
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enum sounds like a good idea to me.
In general it should be the administrator that decides about the default via "flag" and developers can opt-in a different choice via annotation to override the global decision.
863019d
to
65c76c4
Compare
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 <[email protected]>
65c76c4
to
6f82046
Compare
Is this still being worked on? Having HTTP2 support down through the TG would be nice. RIght now, the traffic downgrades to HTTP1 which doesnt let us take advantage of all of the benefits of HTTP2. |
I don't know what work is happening on it lately, but we're also still interested in this support |
We wait first for the test infrastructure changes before we add new features. Right now the colleague is in vacation. I hope next week we can finish the reviews and go on by merging it. |
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@b975e65fixes #391