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

docs: clarify use of Extended CONNECT for h/2 #13051

Merged
merged 1 commit into from
Sep 11, 2020

Conversation

nicktrav
Copy link
Contributor

Commit Message:

Tweak the HTTP upgrades documentation to mention RFC8841 in the
documentation body (complementing the existing link to the RFC).

Minor fix to the warning text for CONNECT support.

Make explicit mention of "Extended CONNECT" in the API docs for
RouteMatch.

Closes #13044.

Additional Description: n/a
Risk Level: Low
Testing: n/a
Docs Changes: Minor clarification in API docs + Upgrade docs
Release Notes: n/a

Tweak the HTTP upgrades documentation to mention RFC8841 in the
documentation body (complementing the existing link to the RFC).

Minor fix to the warning text for CONNECT support.

Make explicit mention of "Extended CONNECT" in the API docs for
`RouteMatch`.

Closes envoyproxy#13044.

Signed-off-by: Nick Travers <[email protected]>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #13051 was opened by nicktrav.

see: more, trace.

@nicktrav
Copy link
Contributor Author

cc: @alyssawilk

@@ -85,7 +85,7 @@ and forward the HTTP payload upstream. On receipt of initial TCP data from upstr
will synthesize 200 response headers, and then forward the TCP data as the HTTP response body.

.. warning::
This mode of CONNECT support can create major security holes if configured correctly, as the upstream
This mode of CONNECT support can create major security holes if not configured correctly, as the upstream
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed this along the way, and assumed this was the intention.

Could have also possibly meant "... create holes even if configured correctly ..."?

@alyssawilk alyssawilk self-assigned this Sep 10, 2020
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

Awesome - thanks for future-proofing the docs w.r.t. extended connect confusion and fixing that typo which inverted the safety statement :-P

@alyssawilk
Copy link
Contributor

cc @mattklein123 or @htuch for the extra word in the APIs :-P

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thank you!

@mattklein123 mattklein123 merged commit 12d7b17 into envoyproxy:master Sep 11, 2020
@nicktrav nicktrav deleted the nickt.connect-docs branch September 11, 2020 16:44
lhluo pushed a commit to lhluo/envoy that referenced this pull request Sep 11, 2020
…code

* upstream/master:
  lint: add more linters for using absl:: over std:: (envoyproxy#13043)
  udpa: filesystem list collection support for inline entries. (envoyproxy#13028)
  filter: http: jwt: implement matching for HTTP CONNECT (envoyproxy#13064)
  [fuzz] split http filter logic into a fuzzing class (envoyproxy#13016)
  xds: allow empty delta update (envoyproxy#12699)
  CacheFilter: parses the allowed_vary_headers from the cache config. (envoyproxy#12928)
  router: extend HTTP CONNECT route matching criteria (envoyproxy#13056)
  docs: clarify use of Extended CONNECT for h/2 (envoyproxy#13051)
  build: shellcheck tools/ (envoyproxy#13007)
  [fuzz] Refactored Health Checker Impl Tests (envoyproxy#13017)

Signed-off-by: Lihao Luo <[email protected]>
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.

HTTP/2 CONNECT implementation is possibly not compliant with h/2 spec
3 participants