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.

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

ngrok: add WithAppProtocol #141

Merged
merged 5 commits into from
Nov 29, 2023
Merged

Conversation

jrobsonchase
Copy link
Contributor

@jrobsonchase jrobsonchase commented Nov 21, 2023

Resolves #139

Adds the ForwardsProto field to the Bind request, as well as WithAppProtocol as an HTTP/labeled option to set it when creating a tunnel.

@jrobsonchase
Copy link
Contributor Author

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@jrobsonchase
Copy link
Contributor Author

Added the tentative user-facing config options, still need agreement on them though as well as documentation of valid values.

@jrobsonchase jrobsonchase changed the title ngrok: add ForwardsProto to Bind ngrok: add WithAppProtocol Nov 22, 2023
@jrobsonchase
Copy link
Contributor Author

Ready for review. Implements WithAppProtocol as specified here

Copy link
Contributor

@nikolay-ngrok nikolay-ngrok left a comment

Choose a reason for hiding this comment

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

looks straightforward 👍

config/app_protocol.go Show resolved Hide resolved
@salilsub
Copy link

@jrobsonchase What does the error-logic look like if someone puts an invalid option? For example, if someone used --app-protocol moose, what would they see?

@jrobsonchase
Copy link
Contributor Author

The configuration should fail validation and our services should return some sort of "invalid config" error. We could validate locally before sending it off, but I wouldn't. That way, we don't have to keep things synchronized between the sdk and our backend if new protocols are added. Kinda the same deal as with region validation (or lack thereof 😛)

@jrobsonchase jrobsonchase force-pushed the joshngrok_add_ForwardsProto_to_Bind branch from 4825d0d to 78bff0a Compare November 28, 2023 16:51
Copy link
Contributor

@bobzilladev bobzilladev left a comment

Choose a reason for hiding this comment

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

+1'd now that it's consistent http1/http2

Copy link
Contributor

@nikolay-ngrok nikolay-ngrok left a comment

Choose a reason for hiding this comment

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

👍

@salilsub
Copy link

I agree that local validation is not a great idea. I think as long as the user can identify where there is an issue, that's sufficient.

@jrobsonchase jrobsonchase merged commit b8b5d7f into main Nov 29, 2023
2 checks passed
@jrobsonchase jrobsonchase deleted the joshngrok_add_ForwardsProto_to_Bind branch November 29, 2023 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support WithAppProtocol for HTTP and Labeled listeners
4 participants