-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
http: add ability to add/remove query parameters #33977
Closed
derekargueta
wants to merge
58
commits into
envoyproxy:main
from
derekargueta:derek-add-remove-query-params
Closed
Changes from all commits
Commits
Show all changes
58 commits
Select commit
Hold shift + click to select a range
57d85ad
functional implementation of add/remove query params
derekargueta 15933e0
fix tests - dont parse query params when nothing is being added/removed
derekargueta 60517b8
add changelog entry
derekargueta 2db1cab
add basic initial config impl tests
derekargueta abc2de0
apply clang-format
derekargueta 0f9782e
switch query_params_to_add to use QueryParameter protobuf type
derekargueta 95b7237
add tests
derekargueta 9c06fcd
query_params -> query_parameters in config API
derekargueta e636741
doc clarification on query_parameters_to_add
derekargueta 7ec2cd9
fix format
derekargueta 8f34e51
make query param mutation filter
derekargueta 104fba4
Merge branch 'main' into derek-add-remove-query-params
derekargueta e31911f
remove query add/remove from router config_impl
derekargueta 4eabeac
remove query params add/remove from router api, fix extension api
derekargueta c154196
clang-format
derekargueta e804f8f
move query param evaluator code to extension
derekargueta df82c4c
clang-format, remove no-longer-needed defaultEvaluator
derekargueta 96e2ca9
initial review feedback
derekargueta 5713609
give QueryParamsEvaluator normal constructor rather than singleton co…
derekargueta dd7cdac
code review
derekargueta 733fdc0
add config test
derekargueta d07288f
Merge branch 'main' into derek-add-remove-query-params
derekargueta a139bb0
remove unused mock changes
derekargueta 9f3eaa8
clang-format
derekargueta 57a5f2f
one more format nit
derekargueta ded4304
refactor tests, add command substitution
derekargueta 23b5558
more formatting...
derekargueta 13ca58d
trim trailing whitespace...
derekargueta e88eeeb
proto formatting
derekargueta 7eb433a
fix use of single backticks in docs
derekargueta 130509d
add filter metadata
derekargueta 7bb39c8
add filter to build registration
derekargueta 9c85c52
fix rst yaml snippet
derekargueta cf14049
rst fix #2
derekargueta 683e592
try to fix changelog
derekargueta 5e59c68
try to address test TSAN error
derekargueta e3f0758
ability to specify query parameter append behavior
derekargueta 6ce0d92
add type aliases to clean up code
derekargueta 5475a3f
move query params to header mutation filter
derekargueta 1aca2ba
clean out query parameter filter
derekargueta 0c0bc51
formatting
derekargueta a2a76e5
Merge branch 'main' into derek-add-remove-query-params
derekargueta d88f04d
fix proto import order
derekargueta df4ea81
address line-too-long in changelog
derekargueta 09b1dac
Merge branch 'main' into derek-add-remove-query-params
derekargueta f2b1ce7
restructure query parameters as list of mutations
derekargueta 02fb669
alias protobuf types, clang-format
derekargueta 07a795f
Merge branch 'main' into derek-add-remove-query-params
derekargueta 7dcc321
fix typo in comments, caught by spelling CI
derekargueta aa96e59
remove proto dependency on v3 core - no longer needed, also caught by CI
derekargueta 4939a35
Merge branch 'main' into derek-add-remove-query-params
derekargueta 642c2a1
Merge branch 'main' into derek-add-remove-query-params
derekargueta fd155b7
update API
derekargueta ff55245
formatting
derekargueta 0c00cc7
remove unused protos
derekargueta c276942
document query_parameter_mutations
derekargueta 2c38641
use bytes
derekargueta 466d1cd
Merge branch 'main' into derek-add-remove-query-params
derekargueta File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
76 changes: 76 additions & 0 deletions
76
source/extensions/filters/http/header_mutation/query_params_evaluator.cc
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
#include "source/extensions/filters/http/header_mutation/query_params_evaluator.h" | ||
|
||
#include <memory> | ||
#include <string> | ||
#include <utility> | ||
|
||
#include "envoy/http/query_params.h" | ||
#include "envoy/stream_info/stream_info.h" | ||
|
||
#include "source/common/formatter/substitution_formatter.h" | ||
|
||
namespace Envoy { | ||
namespace Extensions { | ||
namespace HttpFilters { | ||
namespace HeaderMutation { | ||
|
||
using Http::Utility::QueryParamsMulti; | ||
|
||
QueryParamsEvaluator::QueryParamsEvaluator( | ||
const Protobuf::RepeatedPtrField<KeyValueMutationProto>& query_param_mutations) | ||
: formatter_(std::make_unique<Formatter::FormatterImpl>("", true)) { | ||
|
||
for (const auto& query_param : query_param_mutations) { | ||
mutations_.emplace_back(query_param); | ||
} | ||
} | ||
|
||
void QueryParamsEvaluator::evaluateQueryParams(Http::RequestHeaderMap& headers, | ||
const StreamInfo::StreamInfo& stream_info) const { | ||
if (mutations_.empty()) { | ||
return; | ||
} | ||
|
||
absl::string_view path = headers.getPathValue(); | ||
QueryParamsMulti query_params = QueryParamsMulti::parseAndDecodeQueryString(path); | ||
|
||
Formatter::HttpFormatterContext ctx{&headers}; | ||
for (const auto& mutation : mutations_) { | ||
if (!mutation.remove().empty()) { | ||
query_params.remove(mutation.remove()); | ||
} else { | ||
const auto value_option = mutation.append(); | ||
const auto key = value_option.entry().key(); | ||
const auto value = value_option.entry().value(); | ||
const auto formatter = std::make_unique<Formatter::FormatterImpl>(value, true); | ||
switch (AppendAction(value_option.action())) { | ||
case AppendAction::AppendIfExistsOrAdd: | ||
query_params.add(key, formatter->formatWithContext(ctx, stream_info)); | ||
break; | ||
case AppendAction::AddIfAbsent: | ||
if (!query_params.getFirstValue(key).has_value()) { | ||
query_params.add(key, formatter->formatWithContext(ctx, stream_info)); | ||
} | ||
break; | ||
case AppendAction::OverwriteIfExistsOrAdd: | ||
query_params.overwrite(key, value); | ||
break; | ||
case AppendAction::OverwriteIfExists: | ||
if (query_params.getFirstValue(key).has_value()) { | ||
query_params.overwrite(key, value); | ||
} | ||
break; | ||
default: | ||
PANIC("unreachable"); | ||
} | ||
} | ||
} | ||
|
||
const auto new_path = query_params.replaceQueryString(headers.Path()->value()); | ||
headers.setPath(new_path); | ||
} | ||
|
||
} // namespace HeaderMutation | ||
} // namespace HttpFilters | ||
} // namespace Extensions | ||
} // namespace Envoy |
61 changes: 61 additions & 0 deletions
61
source/extensions/filters/http/header_mutation/query_params_evaluator.h
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
#pragma once | ||
|
||
#include <string> | ||
#include <tuple> | ||
#include <vector> | ||
|
||
#include "envoy/config/core/v3/base.pb.h" | ||
#include "envoy/extensions/filters/http/header_mutation/v3/header_mutation.pb.h" | ||
#include "envoy/formatter/substitution_formatter.h" | ||
#include "envoy/http/header_map.h" | ||
#include "envoy/http/query_params.h" | ||
#include "envoy/stream_info/stream_info.h" | ||
|
||
#include "source/common/protobuf/protobuf.h" | ||
|
||
namespace Envoy { | ||
namespace Extensions { | ||
namespace HttpFilters { | ||
namespace HeaderMutation { | ||
|
||
using KeyValueAppendProto = envoy::config::core::v3::KeyValueAppend; | ||
using KeyValueAppendActionProto = envoy::config::core::v3::KeyValueAppend_KeyValueAppendAction; | ||
using KeyValueMutationProto = envoy::config::core::v3::KeyValueMutation; | ||
|
||
enum class AppendAction { | ||
AppendIfExistsOrAdd = envoy::config::core::v3::KeyValueAppend::APPEND_IF_EXISTS_OR_ADD, | ||
AddIfAbsent = envoy::config::core::v3::KeyValueAppend::ADD_IF_ABSENT, | ||
OverwriteIfExistsOrAdd = envoy::config::core::v3::KeyValueAppend::OVERWRITE_IF_EXISTS_OR_ADD, | ||
OverwriteIfExists = envoy::config::core::v3::KeyValueAppend::OVERWRITE_IF_EXISTS, | ||
}; | ||
|
||
class QueryParamsEvaluator; | ||
using QueryParamsEvaluatorPtr = std::unique_ptr<QueryParamsEvaluator>; | ||
|
||
class QueryParamsEvaluator { | ||
public: | ||
QueryParamsEvaluator( | ||
const Protobuf::RepeatedPtrField<KeyValueMutationProto>& query_param_mutations); | ||
|
||
/** | ||
* Processes headers first through query parameter removals then through query parameter | ||
* additions. Header is modified in-place. | ||
* @param headers supplies the request headers. | ||
* @param stream_info used by the substitution formatter. Can be retrieved via | ||
* decoder_callbacks_.streamInfo(); | ||
*/ | ||
void evaluateQueryParams(Http::RequestHeaderMap& headers, | ||
const StreamInfo::StreamInfo& stream_info) const; | ||
|
||
protected: | ||
QueryParamsEvaluator() = default; | ||
|
||
private: | ||
std::vector<KeyValueMutationProto> mutations_; | ||
Formatter::FormatterPtr formatter_; | ||
}; | ||
|
||
} // namespace HeaderMutation | ||
} // namespace HttpFilters | ||
} // namespace Extensions | ||
} // namespace Envoy |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Please initialize the formatter when loading configuration to avoid do this at runtime.
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.
The formatter needs the
value
?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, you need to parse the value when you constructing
QueryParamsEvaluator
. Creating the formatter will bring big overhead and may throw exception. It's not allowed to do that at key data path.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.
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