Skip to content
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

route: implementation of HeaderAppendAction #22723

Merged
merged 21 commits into from Aug 26, 2022

Conversation

wbpcode
Copy link
Member

@wbpcode wbpcode commented Aug 16, 2022

Commit Message: route: implementation of HeaderAppendAction
Additional Description:

The HeaderAppendAction was added in the #18246. But the implementation never done. This PR try to complete this feature.

Further work of #18246. To close #22713.

Risk Level: Low.
Testing: Unit Test.
Docs Changes: n/a.
Release Notes: wait.
Platform Specific Features: n/a

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
@repokitteh-read-only
Copy link

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

🐱

Caused by: #22723 was opened by wbpcode.

see: more, trace.

api/envoy/config/core/v3/base.proto Outdated Show resolved Hide resolved
source/common/router/header_parser.cc Outdated Show resolved Hide resolved
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
@wbpcode wbpcode changed the title [WIP] route: implementation of HeaderAppendAction route: implementation of HeaderAppendAction Aug 23, 2022
@wbpcode
Copy link
Member Author

wbpcode commented Aug 23, 2022

It's ready for a formal review for now. cc @htuch @soulxu 😸

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Copy link
Member

@soulxu soulxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, two small comments

source/common/router/header_parser.h Show resolved Hide resolved
source/common/router/header_parser.cc Show resolved Hide resolved
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
soulxu
soulxu previously approved these changes Aug 24, 2022
Copy link
Member

@soulxu soulxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
api/envoy/config/core/v3/base.proto Show resolved Hide resolved
source/common/router/header_parser.cc Outdated Show resolved Hide resolved
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
@wbpcode
Copy link
Member Author

wbpcode commented Aug 25, 2022

Because some special behavior in the ext_authz/ext_proc (#22845), most update in the ext_authz & ext_proc has been reverted.

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
@@ -55,7 +54,7 @@ HeaderFormatterPtr parseInternal(const envoy::config::core::v3::HeaderValue& hea

absl::string_view format(header_value.value());
if (format.empty()) {
return std::make_unique<PlainHeaderFormatter>("", append);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my education, why did we ever have append here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see actually, you now propagate this independent of the header formatter, seems cleaner.

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Question] How can I insert a HTTP header only when that header does not exist?
3 participants