Skip to content
This repository has been archived by the owner on Mar 16, 2024. It is now read-only.

support http2 protocol (#1523) #2306

Merged
merged 5 commits into from
Nov 2, 2023
Merged

support http2 protocol (#1523) #2306

merged 5 commits into from
Nov 2, 2023

Conversation

keyallis
Copy link
Contributor

acorn-io/manager#1523

Checklist

  • The title of this PR would make a good line in Acorn's Release Note's Changelog
  • The title of this PR ends with a link to the main issue being address in parentheses, like: This is a title (#1216). Here's an example
  • All relevant issues are referenced in the PR description. NOTE: don't use GitHub keywords that auto-close issues
  • Commits follow contributing guidance
  • Automated tests added to cover the changes. If tests couldn't be added, an explanation is provided in the Verification and Testing section
  • Changes to user-facing functionality, API, CLI, and upgrade impacts are clearly called out in PR description
  • PR has at least two approvals before merging (or a reasonable exception, like it's just a docs change)

Signed-off-by: Oscar Ward <oscar@acorn.io>
http2Ports := ports.ByProtocol(svcInstance.Spec.Ports, true, v1.ProtocolHTTP2)
httpPorts := ports.ByProtocol(svcInstance.Spec.Ports, true, v1.ProtocolHTTP)
// If we have both types of ports we need to create a separate service with the annotation
if len(http2Ports) > 0 && len(httpPorts) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll put some explanation on this, since we paired up to work on this yesterday and I still feel like this is a good and simple solution that can be a little confusing.

The background is that we have to put a traefik annotation on the K8s Service for http2 support (h2c). With that service we'd break the traffic for "normal" http traefik to other ports listed in that service though.
E.g. if someone uses http2 for the app server, but metrics are served via http.
That means, if we have both http/http2 mixed in a single ServiceInstance, we have to split it up to generate two K8s services.

However, all the code further down the stream is specific to a single service and even re-uses the ServiceInstance name in a lot of places.
So we need a "virtual" ServiceInstance with a different name, holding only the http2 ports to be able to leave lots of code untouched.

The DeepCopies are necessary to leave the original serviceInstance object untouched during that process and we only have to populated actively modified status fields on it.

Oscar Ward added 2 commits October 31, 2023 10:10
Signed-off-by: Oscar Ward <oscar@acorn.io>
Signed-off-by: Oscar Ward <oscar@acorn.io>
Copy link
Contributor

@StrongMonkey StrongMonkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, would you be able to add some intergation tests around it

@@ -19,6 +19,8 @@ func validProto(p string) (Protocol, bool) {
case ProtocolUDP:
fallthrough
case ProtocolHTTP:
fallthrough
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: all of these cases can be put on the same line:

switch ret {
    case ProtocolTCP, ProtocolUDP, ProtocolHTTP, ProtocolHTTP2, "":
        return ret, true
}

if len(http2Ports) > 0 {
svcCopy := svcInstance.DeepCopy()
svcCopy.Spec.Ports = http2Ports
svcCopy.Name = fmt.Sprintf("%s-%s", svcInstance.Name, v1.ProtocolHTTP2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am agreeing with @StrongMonkey here. Creating an additional ServiceInstance with a different name may cause issues with other parts of the system. For example, if something depends on the service, but the service has a different name. Or, the user may create a service that results in a ServiceInstance being created with the same name you are creating here.

Would it be possible to render the corev1.Service with the special annotations instead of creating an additional ServiceInstance?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ServiceInstance is only created as a temporary copy for our purposes so that we can pass the same value into the function logic. We don't actually return the temporarily created ServiceInstance object, we just use it to create a new corev1.Service.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spoke offline about a slightly different approach that would alleviate the concerns above.

Oscar Ward added 2 commits November 1, 2023 18:53
Signed-off-by: Oscar Ward <oscar@acorn.io>
Signed-off-by: Oscar Ward <oscar@acorn.io>
@keyallis keyallis merged commit 0e8da4d into acorn-io:main Nov 2, 2023
@keyallis keyallis deleted the issue-1523 branch November 2, 2023 02:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants