-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Documentation/Config changes (proposed) for #6589 #11111
Conversation
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.
Thank you. A few questions to get started with. cc @alyssawilk also to PTAL.
/wait
api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto
Show resolved
Hide resolved
// See `Normalization and Comparison <https://tools.ietf.org/html/rfc3986#section-6>` | ||
// for details of normalization. | ||
// Note that Envoy does not perform | ||
// `case normalization <https://tools.ietf.org/html/rfc3986#section-6.2.2.1>` | ||
google.protobuf.BoolValue normalize_path = 30; | ||
|
||
// Should paths to upstream be normalized according to RFC 3986 after any processing of |
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.
A few questions to get started:
- When does this return 400 and if we want to return a 400 why would someone do this here vs. just doing it at the beginning before filters run? It's not clear to me what utility this has.
- Should we actually deprecate normalize_path and have a new field that is an enum that describes which normalizing approach to use? Also, in the above docs it says that normalize_path defaults to true. I thought it defaults to false or maybe I am confused?
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.
- Agreed, yes that makes sense.
- The addition on a the extra header was intended to minimize the impact on routing/filtering code changes to deal with potential changes to, as you say, replacing normalize_path with an enum. Idealy yes an enum is a cleaner 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.
A deprecation makes sense IMHO, as long as we preserve existing behavior today.
@@ -310,6 +310,7 @@ class HeaderEntry { | |||
HEADER_FUNC(OtSpanContext) \ | |||
HEADER_FUNC(Origin) \ | |||
HEADER_FUNC(Path) \ | |||
HEADER_FUNC(OriginalPathForUpstream) \ |
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.
Do we need a new header? If the path is change can we still just use EnvoyOriginalPath above?
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.
Ah, the EnvoyOriginalPath, I was curious about using that. The issue as I understand it, is that ideally keeping everything "backward compatible" was a high priority. So leaving normalize_path in situ as is, with minimal processing changes presented a lower risk option. I'd prefer to re-use what code variables already exist rather than adding a new header. If we can leverage the EnvoyOriginalPath for this then all the better. I will dig into it's usage. If I understand the requirement correctly, it would seem that once filtering and routing phase is complete then the Path could be set to the upstream normalized/non-normalized value depending on settings. The caveat here is extra checking of the OriginalEnvoyPath might be needed for any code that relies on the original usage of the Path variable (will verify usage).
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.
One thing I worry about is potential misuse and inconsistent use by filters. Ideally we only make use of the original path when sending to upstream..
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.
Yeah I think we should use a single header here. Re: back compat, as this is a new feature, and controlled by an enum, IMO we can make it clear what that header holds in each enum case.
🙀 Error while processing event:
|
// See `Normalization and Comparison <https://tools.ietf.org/html/rfc3986#section-6>` | ||
// for details of normalization. | ||
// Note that Envoy does not perform | ||
// `case normalization <https://tools.ietf.org/html/rfc3986#section-6.2.2.1>` |
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.
[#not-implemented-hide:]
?
// true in the future. When not specified, this value may be overridden by the | ||
// runtime variable | ||
// paths that are malformed. This defaults to true. | ||
// This by default affects the upstream *:path* header as well. |
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.
Above already says This affects the upstream :path header
Maybe just change the first mention to include default?
// true in the future. When not specified, this value may be overridden by the | ||
// runtime variable | ||
// paths that are malformed. This defaults to true. | ||
// This by default affects the upstream *:path* header as well. |
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.
Actually reading this I was really confused about what was meant by affecting the upstream path header. Can we maybe rephrase to contract normalizing path for in-Envoy code, vs normalizing the path sent uptream?
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md
Commit Message:
Documentation and Config changes as per request #6589 (comment)
NOTE: Incomplete, do not merge this PR was incorrectly attributed to v2 and v2 is frozen.
Additional Description:
These changes are not the complete fix.
Risk Level:
Testing:
Docs Changes:
rst and proto files as per commit.
Release Notes:
Issue 6589 allows for optional normalization of upstream requests independent of the normalization settings received from downstream. Upstream normalization defaults to the normalize_path setting and can be overridden with the setting normalize_path_for_upstream.
It is imperative that existing functionality/behaviour is preserved. As an example, the continued support for 0/100 in the runtime configuration for the normalize_path* properties in the http listener, while introducing support for Boolean true/false case.
Fixes #Issue 6589
There are several consequences to implementing support for this new property:
.proto
file. This will require some changes to the initialisation code. Will have to be sure to check all places where class is loaded.runtime.rst
inroot/configuration/http_conn_man
refers to % in relation to normalize_path. Harvey's note in issueconfig.cc(see next task). Support backward compatibility.// TODO(htuch): we should have a boolean variant of featureEnabled() here.
ENVOY_NORMALIZE_PATH_BY_DEFAULT
envoy/blob/master/source/extensions/filters/network/http_connection_manager/config.cc
test for
ENVOY_NORMALIZE_PATH_BY_DEFAULT
line 205 should this be modified witha boolean? (Needs a boolean featureEnabled() method as per HT's comment.) When the adding the featureEnabled(absl::string_view, bool) ensure that specify no default conversion from int to boolean. Include support for the
ENVOY_NORMALIZE_PATH_FOR_UPSTREAM_BY_DEFAULT
.There is already support for
getBoolean(absl::string_view, bool default_value)
in
envoy/source/common/runtime/runtime_impl.cc
. This can be reused. NOTE:Support of existing functionality must be maintained (ie 0/100 -> false/true).
add normalize_path_for_upstream_
to the constructorHttpConnectionManagerConfig::HttpConnectionManagerConfig()
inenvoy/blob/master/source/extensions/filters/network/http_connection_manager/config.cc
config.h
Need to addnormalize_path_for_upstream
toenvoy/source/extensions/filters/network/http_connection_manager/config.h