-
Notifications
You must be signed in to change notification settings - Fork 1.7k
ingress/controllers/nginx: WebSocket documentation #1808
ingress/controllers/nginx: WebSocket documentation #1808
Conversation
For those that do not understand the default way in which nginx proxies requests not containing a "Connection" header, the approach for enabling WebSocket support might not make sense. This commit adds documentation that explains why things are done this way.
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
[CLA-PING] @whitlockjc Thanks for your pull request. It looks like this may be your first contribution to a CNCF open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://identity.linuxfoundation.org/projects/cncf to sign. Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.
|
{{/* "proxy_set_header Connection $http_connection" for WebSocket support because in this case, the */}} | ||
{{/* "Connection" header would be set to "" whenever the original request did not have a "Connection" header, */}} | ||
{{/* which would mean no "Connection" header would be in the target request. Since this would deviate from */}} | ||
{{/* normal nginx behavior we have to use this approach. */}} |
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.
why add this here instead of a section in the readme? or better yet, add an example under https://github.com/kubernetes/contrib/tree/master/ingress/controllers/nginx/examples and document it there?
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.
@bprashanth I think this a better place because the next map changes the http behavior and the comment show the why and consequences of changing the values
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.
That was my thought as well. It might not make sense why we're going through all of this effort just to set the Connection
header and documenting this as close to where it's happening made most sense to me. I'm up for whatever you decide.
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.
SG, a websockets example would still be good if you have time.
Thanks for the pr, you probably need to auth linuxfoundation access to your github account so it verifies CLA |
I've signed both CLAs. |
CLAs look good, thanks! |
LGTM |
@bprashanth ping |
Automatic merge from submit-queue |
…ginx-connection-header Automatic merge from submit-queue ingress/controllers/nginx: WebSocket documentation For those that do not understand the default way in which nginx proxies requests not containing a "Connection" header, the approach for enabling WebSocket support might not make sense. This commit adds documentation that explains why things are done this way.
For those that do not understand the default way in which nginx proxies
requests not containing a "Connection" header, the approach for enabling
WebSocket support might not make sense. This commit adds documentation
that explains why things are done this way.
This change is