Skip to content
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.

Sign up for GitHub

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

Jump to bottom

appProtocol support for http2 #323

Merged
merged 8 commits into from
Dec 1, 2023
Merged

appProtocol support for http2 #323

merged 8 commits into from
Dec 1, 2023

Conversation

nikolay-ngrok
Copy link
Contributor

@nikolay-ngrok nikolay-ngrok commented Nov 29, 2023 •
edited
Loading

What

We are adding http2 support, expose this in the controller.

How

Resolve and pass around to tunnel CRD.

Breaking Changes

None. We should merge only after we update to ngrok-go stable.

@nikolay-ngrok nikolay-ngrok changed the title add appProtocol support for http2 appProtocol support for http2 Nov 29, 2023
@github-actions github-actions bot added the area/controller Issues dealing with the controller label Nov 29, 2023
@github-actions github-actions bot added the area/helm-chart Issues dealing with the helm chart label Nov 29, 2023
@nikolay-ngrok nikolay-ngrok self-assigned this Nov 29, 2023
@nikolay-ngrok nikolay-ngrok marked this pull request as ready for review November 30, 2023 14:36
@nikolay-ngrok nikolay-ngrok requested a review from a team as a code owner November 30, 2023 14:36
jrobsonchase
jrobsonchase approved these changes Nov 30, 2023
View reviewed changes
Copy link
Contributor

@jrobsonchase jrobsonchase left a comment

Choose a reason for hiding this comment

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

Let's add a little more support for standard values, but otherwise lgtm!

internal/store/driver.go Outdated Show resolved Hide resolved
@jrobsonchase
Copy link
Contributor

Welp, I tried to help you out with https://github.com/ngrok/kubernetes-ingress-controller/pull/324, but since we can't use tls.Config.InsecureSkipVerify via the forwarding api, you'll have to continue duplicating a bit of logic 😢

If http2 is configured as the appProtocol and we're also terminating TLS against the upstream/backend, you'll also need to set tls.Config.NextProto to []string{"h2", "http/1.1"} so that the service knows what to expect.

@jrobsonchase jrobsonchase self-requested a review November 30, 2023 20:40
@nikolay-ngrok nikolay-ngrok merged commit 84534fe into main Dec 1, 2023
@nikolay-ngrok nikolay-ngrok deleted the nikolay/app-protocol branch December 1, 2023 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrobsonchase jrobsonchase jrobsonchase approved these changes

@bobzilladev bobzilladev Awaiting requested review from bobzilladev

Assignees

@nikolay-ngrok nikolay-ngrok

Labels
area/controller Issues dealing with the controller area/helm-chart Issues dealing with the helm chart
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nikolay-ngrok @jrobsonchase