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

disable http/2 for websockets #2399

Merged
merged 1 commit into from
Jul 27, 2021
Merged

disable http/2 for websockets #2399

merged 1 commit into from
Jul 27, 2021

Conversation

calebdoxsey
Copy link
Contributor

Summary

I guess you can't use http/2 with websockets because they use the same header field. This PR updates the envoy config to explicitly disable http/2 when using web sockets.

Related issues

Checklist

  • reference any related issues
  • updated docs
  • updated unit tests
  • updated UPGRADING.md
  • add appropriate tag (improvement / bug / etc)
  • ready for review

@calebdoxsey calebdoxsey added the bug Something isn't working label Jul 26, 2021
@calebdoxsey calebdoxsey requested a review from a team as a code owner July 26, 2021 21:29
@calebdoxsey calebdoxsey requested a review from wasaga July 26, 2021 21:29
@codeclimate
Copy link

codeclimate bot commented Jul 26, 2021

Code Climate has analyzed commit 101c29c and detected 0 issues on this pull request.

View more on Code Climate.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 65.358% when pulling 101c29c on cdoxsey/2388-fix-websockets into d9bc9d7 on master.

@wasaga
Copy link
Contributor

wasaga commented Jul 26, 2021

I tried that; it did not fix my test using streamlit.io as websocket server

policy:
  - name: ws-1
    from: https://ws1.localhost.pomerium.io
    to: http://localhost:8501
    allow_websockets: true
    allow_public_unauthenticated_access: true
  - name: ws-2
    from: https://ws2.localhost.pomerium.io
    to: https://ws1.localhost.pomerium.io
    allow_websockets: true
    allow_public_unauthenticated_access: true
    tls_custom_ca_file: "/path/to/mkcert/rootCA.pem"

@calebdoxsey
Copy link
Contributor Author

It works for me. Can you provide concrete feedback. Or should I just close this PR too?

@calebdoxsey
Copy link
Contributor Author

FWIW https://gist.github.com/calebdoxsey/b2040f758c91fa306178fa3fff3e2162

wasaga
wasaga approved these changes Jul 27, 2021
View reviewed changes
Copy link
Contributor

@wasaga wasaga left a comment

Choose a reason for hiding this comment

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

your test environment is different, and yes this PR fixes it for that case.

the reason my configuration still does not work is different, I'll file a separate ticket about it.

@calebdoxsey calebdoxsey merged commit 1c627e5 into master Jul 27, 2021
@calebdoxsey calebdoxsey deleted the cdoxsey/2388-fix-websockets branch July 27, 2021 02:09
desimone
desimone requested changes Jul 27, 2021
View reviewed changes
config/envoyconfig/clusters.go
@@ -211,6 +227,16 @@ func (b *Builder) buildPolicyTransportSocket(
return nil, err
}

var alpn []string
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we abstract out into a little sub function?

config/envoyconfig/protocols.go
for _, e := range endpoints {
if e.transportSocket != nil {
tlsCount++
case upstreamProtocolAuto:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add some contextualizing comments to this case? The fallthrough and empty default is uncommon enough where I'm not sure what it does without searching.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there are already comments. I don't know what else to add. I will remove the default case.

config/envoyconfig/clusters.go
upstreamProtocol := upstreamProtocolAuto
if policy.AllowWebsockets {
// #2388, force http/1 when using web sockets
upstreamProtocol = upstreamProtocolHTTP1
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a log message to make clear we are using HTTP1 on this policy because websockets was set?

Copy link
Contributor

Choose a reason for hiding this comment

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

To that point, I think we could abstract this little loop into a small function and call it directly in buildPolicyEndpoints and buildCluster?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be very noisy, but ok.

config/envoyconfig/clusters.go
@@ -201,6 +216,7 @@ func (b *Builder) buildPolicyTransportSocket(
options *config.Options,
policy *config.Policy,
dst url.URL,
upstreamProtocol upstreamProtocolConfig,
Copy link
Contributor

Choose a reason for hiding this comment

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

Having an additional function argument for upstreamProtocol here feels weird to me since we already have the info we need in policy *config.Policy to determine the upstreamProtocol.

@desimone
Copy link
Contributor

desimone commented Jul 27, 2021 •
edited
Loading

Welp, guess I missed the bus on this PR. 😆

edit: I think the code is fine, but would prefer not to keep adding to the list of function arguments if the information we need (upstreamProtocol) is already available available in another function argument (policy) and is deterministic and insignificant perf impact.

@calebdoxsey calebdoxsey mentioned this pull request Jul 27, 2021
Merged
6 tasks
calebdoxsey added a commit that referenced this pull request Jul 28, 2021
@calebdoxsey
disable http/2 for websockets (#2399)
4c32d07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@desimone desimone desimone requested changes

@wasaga wasaga wasaga approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@calebdoxsey @coveralls @wasaga @desimone