[http] API changes to split path header normalization and forwarding

[asraa]

Signed-off-by: Asra Ali asraa@google.com

Commit Message: Configuration for path normalization so that normalization can be configured for internal only use and/or forwarding.
Additional Description: Just API changes. normalize_path and merge_slashes to be deprecated.

There will be normalizations Envoy applies to an incoming path header (forwarding_transformation) that "permanently" transform an incoming path header and affect the forwarded path header. Internal transformations (filter_transformations) apply only within Envoy processing. If an intermediate filter changes the internal path header, we don't restore the header.

The plan:

• Make ConnManagerUtility and Router friend classes with HeaderMap
• Create private HeaderMap methods: setNormalizedPath that ConnManagerUtility calls prior to filter processing and routing that sets private members original_path_ and normalized_path_. Then restoreOriginalPathIfUnchanged called by Router if we are doing internal-only normalization that restores :path with the original header only filters didn't change the path.
• No HTTP filters have access to the original path for forwarding to prevent misuse.

Risk Level: Low, just API
Release notes: TODO on implementation
Runtime Guard: TODO on implementation. Runtime guard to overwrite forwarding http_connection_manager.forward_normalized_path (can revert right now)
Starts: #6589

[repokitteh-read-only]

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

🐱

Caused by: #15044 was opened by asraa.

see: more, trace.

[adisuissa]

Thanks for working on this!

[adisuissa]

Should FORWARD be the default behavior?
I'm not sure what the intended default should be, and in this case it might make more sense to add a new value POLICY_UNSPECIFIED = 0 if the policy should be explicit, and increase the values of FORWARD and INTERNAL by 1.

[adisuissa]

Just saw in the doc update that FORWARD is the intended default behavior.

[adisuissa]

According to the API_STYLE guide, the wrapped scalar type should be used in cases that the default value isn't compatible with proto3 default (false in this case). Is there an explicit reason for using the wrapper rather than bool?

[htuch]

This allows us to change defaults in future..

[htuch]

Awesome, thanks for tackling this important feature!

[htuch]

What about observability (logging, tracing)?

[asraa]

It depends on the context. Only the normalized path is visible internally so if a filter logs headers, they will see the normalized header. If we finalize spans before forwarding to an upstream, we see the internal header, otherwise we see the forwarded header. This can obviously be changed in the code.

I'm not really sure what the right answer here is. If it should always be the "forwarded header" or always the original header, then we lose out on observability in some context. I think we either have to keep the logging context dependent as is, OR we allow HttpTracerUtility visibility into the original header (and for logs, I don't know -- as soon as we open the original header to logs, we open it up to filters).

e.g. if you log the request headers directly prior to forwarding, you will see the to-be-forwarded header, like here

envoy/source/common/router/router.cc

Line 583 in 280654b

ENVOY_STREAM_LOG(debug, "router decoding headers:\n{}", *callbacks_, headers);

. before that, you see the internal header.

[htuch]

Why only mention these two? I.e. are there other path transforms in router we should consider? What happens when a filter tries to modify the path and it's only interacting with the normalized path?

[asraa]

Those are the only router transformations, but that may change in the future so I will change the language.

I mentioned that in this comment, it's only relevant for INTERNAL:

// Normalizations only apply internally. The original :path header will
// be sent upstream unless the path has been mutated by a filter.

[htuch]

This allows us to change defaults in future..

[adisuissa]

LGTM! Thanks for working on this!

[asraa]

/cc @chaoqin-li1123

[mattklein123]

Thanks just a few API questions.

/wait

[mattklein123]

This might be getting into the implementation weeds, but do you envision any issues if someone selects INTERNAL for one and FORWARD for another? Is it going to lead to complicated state tracking?

[asraa]

It's open to do so.

If the HeaderMap privately stores the original path, then setting the FORWARD header could be independent of the internal transformed path. (However, we do have to store the original internal header too, to make sure we don't override the path if a filter changed it)

HeaderMap:

• Path: visible path. during internal processing, this is INTERNAL(FORWARD(path)).
• private: original_path: store the original path (used for access logging)
• private: forward_path: FORWARD(path)
• private: internal_path: INTERNAL(FORWARD(path)); the original internal path set by the HCM. the router uses this to check if the path was mutated by a filter. if so, we forward the mutated path. otherwise, we forward forward_path

[asraa]

I added a comment about internal_transformations:

    // These will be applied after :ref:`full_transformation
    // <envoy_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.PathNormalizationOptions.full_transformation`
    // is applied. The *:path* header before this transformation will be restored in the router
    // filter and sent upstream unless it was mutated by a filter```

[asraa]

@chaoqin-li1123 suggested that we should clarify this too. full_transformation (previously called FORWARD) should be though of as incoming header normalization that mutates the header before processing and affects the forwarded.

internal ones only exist temporarily in Envoy during processing. we forward the path with the incoming header normalization.

I am happy to make the name change from full_transformation to incoming_transformation if that's clearer.

[yanavlasov]

Awesome changes.
/wait

[asraa]

@mattklein123 could you please take another look? Controls have individual messages, hopefully the internal/full transformation interaction is more clear.

[mattklein123]

Thanks at a high level this LGTM with some small remaining comments.

/wait

[htuch]

Looks great. A few bike shed comments.

[mattklein123]

Thanks LGTM with one small comment.

/wait

[mattklein123]

Thanks!