http: add ability to add/remove query parameters

[derekargueta]

Commit Message: Add ability to add/remove query parameters.
Additional Description:
Risk Level: Low
Testing: Included
Docs Changes: Included
Release Notes: Included
Platform Specific Features: N/A
Fixes: #2098

Signed-off-by: Derek Argueta darguetap@gmail.com

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #33977 was opened by derekargueta.

see: more, trace.

[derekargueta]

some TODOs I'd like to get into this PR:

• support command substitution, e.g. populate query params with request header value, environment variable, metadata, etc.
• looks like we probably want the same APPEND/OVERWRITE-type options as header manipulation so there's clearly defined behavior if you add a query parameter and value(s) for it already exist

[wbpcode]

Rather than to implement this in our core code path (and core route API), considering we have a full featured L7 filter chain, I will prefer to implement this in a filter like header_mutation.

/wait-any

[derekargueta]

@wbpcode I had asked about that here as I began working on this: #2098 (comment)

[derekargueta]

I don't disagree though, if that's the desired direction I can pivot.

[wbpcode]

@derekargueta sorry, I forgot to check the original issue and related comments and different maintainers may have different ideas.

IMO, I will still recommend to implement it as part of HTTP filter. (I think you can implement it the exist header_mutation filter directly, because the envoy treat the query as part of :path header, you can wrap these query change as one single header evaluator). Here are my reasons:

1. The route is fairly complex. (I think I shared same idea with you in this point).
2. The issue was opened at 2017. It's almost 7 years ago. I agree it's a reasonable requirement, but I will assume it's not a very common requirement or most users may have their own solutions. It would be better to implement it in our extensions rather the core code base.

cc @mattklein123 for more inputs.

[derekargueta]

@wbpcode no worries! yeah after sleeping on it another advantage that came to mind might be ability to modify query parameters before/after certain filters. I don't have a concrete use-case in mind but might be useful to add parameters after certain operations e.g. auth/rate-limiting to compose higher level functionality by controlling the order of filters.

I think you can implement it the exist header_mutation filter directly

interesting thought, I was initially thinking of just a new query_parameter_mutation filter since, while it is technically a :path header I feel path is generally thought of as its own construct in a request but you're right

[mattklein123]

I don't feel that strongly about it. If you both feel it would be better to do it as a filter that is fine with me. I just figured all the rest of this stuff is already in the router but this doesn't have to be.

[wbpcode]

See #34550 for our latest common API for all these map-like mutations. :)

To use the new common API, I think you only need only to change the type of proto and needn't much change to the code.

[wbpcode]

Oh, thanks for the update. Some comments to the API is added.

[wbpcode]

Please no. bytes is used here by design.

[derekargueta]

doh, left an unsent github comment. I was wondering about the use of bytes? IIUC this makes command substitution a no-go which is the primary use-case I have in mind (appending a query param from metadata). It also seems inconsistent - elsewhere I see an option for a value string and a raw_value bytes.

[wbpcode]

IIUC this makes command substitution a no-go which is the primary use-case I have in mind (appending a query param from metadata).

The bytes is used to support non-UTF8. But I think it's also OK to use it as a command formatter for Envoy. But it may not be Okay for non-C++ xDS clients.

@htuch WDYT? should we use string value as value with command formatter and use bytes value as raw value?

[htuch]

Strings/bytes on the wire are identical. How we opt to use them then comes down to the Envoy implementation. Can you elaborate on how "command substitution" is working here? It looks like regular header or parameter append operation etc.

[wbpcode]

It's same as the HeaderValue message. The value field may be a string with substitution formatter like %REQ(CUSTOM-KEY)%. bytes should be fine for C++ xDS client like Envoy in this scenario, because bytes and string has no essential difference in C++. But not sure if it's OK for non-C++ client, like go gRPC proxy.

cc @htuch

[htuch]

@markdroth

[wbpcode]

Please initialize the formatter when loading configuration to avoid do this at runtime.

[derekargueta]

The formatter needs the value?

[wbpcode]

Yeah, you need to parse the value when you constructing QueryParamsEvaluator. Creating the formatter will bring big overhead and my throw exception. It's not allowed to do that at key data path.

[wbpcode]

I think this is problem is still not be resolved.

We should load/create the formatter when loading configuration.

You can take the header mutations implemention as an example.

/lgtm api

/wait

[wbpcode]

/wait

[soulxu]

@wbpcode gentle ping, also @derekargueta it seems need a merge with main also

[wbpcode]

I have to say sorry to the @derekargueta .

Now, this is blocked by the API. We may did incorrect decision in past about the API.

And I will try give a final conclusion next week early.

[wbpcode]

/wait-any

Temporary label and well update it tomorrow.

[github-actions]

This pull request has been automatically marked as stale because it has not had activity in the last 30 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!

[github-actions]

This pull request has been automatically closed because it has not had activity in the last 37 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!