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

inline http cookie header #17234

Closed
wbpcode opened this issue Jul 5, 2021 · 5 comments · Fixed by #17560
Closed

inline http cookie header #17234

wbpcode opened this issue Jul 5, 2021 · 5 comments · Fixed by #17560
Labels
area/http stale stalebot believes this issue/PR has not been touched recently

Comments

@wbpcode
Copy link
Member

wbpcode commented Jul 5, 2021

The current cookie is not be implemented as O(1) inline header which make the cookie-related operation very expensive.

Perhaps we can set a special delimiter ; for the cookie to make the cookie an inline header.

@wbpcode wbpcode added enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Jul 5, 2021
@mattklein123 mattklein123 added area/http and removed enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Jul 6, 2021
@mattklein123
Copy link
Member

cc @asraa @alyssawilk

@alyssawilk
Copy link
Contributor

I think Dan actually fixed the delimiter issue (https://github.com/envoyproxy/envoy/blob/main/source/common/http/header_map_impl.cc#L437) but where I agree O(n) isn't good I suspect inlining cookies would break things for some users. it'd be nice to have a configurable bootstrap option for which headers to inline so folks could optimize on their own.

@mattklein123
Copy link
Member

it'd be nice to have a configurable bootstrap option for which headers to inline so folks could optimize on their own.

FWIW when I wrote the custom inline header code I intended to eventually make it bootstrap configurable. It should be quite easy:

// Immediate after the bootstrap has been loaded, override the header prefix, if configured to
// do so. This must be set before any other code block references the HeaderValues ConstSingleton.
if (!bootstrap_.header_prefix().empty()) {
// setPrefix has a release assert verifying that setPrefix() is not called after prefix()
ThreadSafeSingleton<Http::PrefixValue>::get().setPrefix(bootstrap_.header_prefix().c_str());
}
// TODO(mattklein123): Custom O(1) headers can be registered at this point for creating/finalizing
// any header maps.
ENVOY_LOG(info, "HTTP header map info:");
for (const auto& info : Http::HeaderMapImplUtility::getAllHeaderMapImplInfo()) {
ENVOY_LOG(info, " {}: {} bytes: {}", info.name_, info.size_,
absl::StrJoin(info.registered_headers_, ","));
}

@wbpcode
Copy link
Member Author

wbpcode commented Jul 8, 2021

Then, after that, we will have three ways to define the inline header.
The first is the macro definition in HeaderMap. This type of inline header provides related methods with the same name(getXXX, setXXX).
The second is to implement the inline header through RegisterCustomInlineHeader.
The third is to specify the header name to be set as the inline header in the bootstrap.

@github-actions
Copy link

github-actions bot commented Aug 7, 2021

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 7, 2021
alyssawilk pushed a commit that referenced this issue Aug 10, 2021
Commit Message: ensure that the inline cookie header will be folded correctly
Additional Description:

This PR is a supplement to #17330 and #14969 and will finally close #17234. This PR mainly did following works:

update insertByKey to choose suitable delimiter for inline header.
update parseCookie to avoid unnecessary iteration for parsing cookie value.
Risk Level: Low.
Testing: Add.
Docs Changes: N/A.
Release Notes: N/A.
Signed-off-by: wbpcode <wbphub@live.com>
leyao-daily pushed a commit to leyao-daily/envoy that referenced this issue Sep 30, 2021
…roxy#17560)

Commit Message: ensure that the inline cookie header will be folded correctly
Additional Description:

This PR is a supplement to envoyproxy#17330 and envoyproxy#14969 and will finally close envoyproxy#17234. This PR mainly did following works:

update insertByKey to choose suitable delimiter for inline header.
update parseCookie to avoid unnecessary iteration for parsing cookie value.
Risk Level: Low.
Testing: Add.
Docs Changes: N/A.
Release Notes: N/A.
Signed-off-by: wbpcode <wbphub@live.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/http stale stalebot believes this issue/PR has not been touched recently
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants