http: pathless CONNECT#11048
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
Hey @mattklein123 would like your thoughts on the behavioral change. I was going to special case, and change the HCM "send upgrade rejected" response to happen if an upgrade failed (route match, no opgrade configured) or for CONNECT requests which had no route match, to preserve the existing behavior. I feel like 403 on connect is more clear than 404, and behavioral changes and runtime guards, yadda yadda. I had second thoughts, because if we add more logic to the extensible connect matcher, it gets complicated to tell if the 404 is a connect rejection, or if a fancy extended connect matcher failed to match. Basically keeping the old behavior doesn't seem very future proof. Wanted to get your take on which way to go, and I'll either runtime guard and add release notes and such, or special-case to preserve existing behavior, revert the test changes, and we'll have to remember to be careful if anyone extends the connect matcher. |
|
Also I don't remember where we left off with our comfort with Pathless connect. I think the plan was to improve fuzzing and I think @asraa has done that so I think we're OK, but if you want more guards in place before this lands LMK |
mattklein123
left a comment
There was a problem hiding this comment.
Wanted to get your take on which way to go, and I'll either runtime guard and add release notes and such, or special-case to preserve existing behavior, revert the test changes, and we'll have to remember to be careful if anyone extends the connect matcher.
IMO the 404 when we fail to match is more future proof, agreed. I wonder if we should be setting any access log codes and/or error reasons specific to these cases though to help with debugging? WDYT?
Also I don't remember where we left off with our comfort with Pathless connect. I think the plan was to improve fuzzing and I think @asraa has done that so I think we're OK, but if you want more guards in place before this lands LMK
If we feel comfortable with the fuzzing coverage I'm fine landing it.
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
LGTM but needs a format fix. /wait |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
/azp run envoy-presubmit |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run envoy-presubmit |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
/retest |
|
🔨 rebuilding |
Removing the synthetic path added to CONNECT requests theoretically completing Envoy CONNECT support.
Additional Description:
This has the side-effect of changing Envoy's default response to CONNECT requests from 403 (upgrade forbidden) to 404 (route not found) because without the path header, CONNECT request fail to match traditional route rules.
Risk Level: Medium (high for early CONNECT users, low otherwise)
Testing: Altered tests
Docs Changes: will un-hide in a follow-up
Release Notes: added
Runtime guard: envoy.reloadable_features.stop_faking_paths
Part of #1451 and #1630