http: header_map coalescing is not explicitly handled

[jmarantz]

I could not find a canonical source for this, but in the past I've used https://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_util.cc#l368 (Chrome's HttpUtil::IsNonCoalescingHeader() function) as the source of truth of how to represent multiple values for the same header.

For Envoy, this is probably most important for Cookies. But the Chrome source has these:

const char* kNonCoalescingHeaders[] = {
  "date",
  "expires",
  "last-modified",
  "location",  // See bug 1050541 for details
  "retry-after",
  "set-cookie",
  // The format of auth-challenges mixes both space separated tokens and
  // comma separated properties, so coalescing on comma won't work.
  "www-authenticate",
  "proxy-authenticate",
  // STS specifies that UAs must not process any STS headers after the first
  // one.
  "strict-transport-security"
};

This works properly in Envoy for Set-Cookie and there's a test for it, but it only works because Set-Cookie is not listed as one of the predefined O(1) headers. Adding SetCookie to that list in include/envoy/http/header_map.h breaks HeaderMapImplTest.DoubleCookieAdd . There is no test for the behavior of Cookie AFAICT. But at the very least, I think adding "Set-Cookie" to the O(1) list should not change its functional behavior.

#7488 makes interesting reading also.

@alyssawilk @mattklein123 @asraa

[alyssawilk]

Josh, I'm not sure what you're asking for here. The O(1) list works by auto-coalescing, so if you add set-cookie to the O(1) list, you're coalescing, which is a functional behavior change.
Are you suggesting we coalesce in-Envoy and de-coalesce any special headers which can't be handled that way as we write to the wire, are you suggesting that the O(1) list should handle multiple references, or are you asking that we explicitly note at the O(1) header list that adding headers there implies coalescing?

[jmarantz]

3 things to consider:

1. Separate the decision about whether a header can be accessed fast vs whether it should be coalesced. The current situation seems brittle and potentially suboptimal.
2. do a more thorough analysis of which headers should be coalesced. We could cargo-cult Chrome (which I did in the past) or maybe figure out where that list came from, other than the specific call-out for set-cookie in the http spec.
3. Populate headers received from clients or servers via codec with zero interpretation; just assume they are semantically valid. So no coalescing is attempted. Then in the internal APIs we could make them more clearly intent based, e.g. append() vs set() vs merge(), and let the caller determine coalescing behavior. I thought of this because the speed-test seems relatively slow when populating headers that are not O(1). A filter can always sanitize these, but population can be verbatim.

[stale]

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 other activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted". Thank you for your contributions.

[stale]

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 other activity occurs. Thank you for your contributions.