[TS-4500] add cookie-rewrite functionality into header-rewrite plugin

[zizhong]

add cookie-rewrite functionality into header-rewrite plugin.

There're three cookie handling operators added, including add-cookie, rm-cookie and update-cookie.
add-cookie adds a key-value pair into Cookie. If the given key is already in Cookie, do nothing.
rm-cookie remove a pair with the given key from Cookie.
update-cookie sets the value with the given key to the given value. If the given key doesn't exist, add a new pair into Cookie. So the only difference between add-cookie and update-cookie is overwriting an existing pair or not.

[zwoop]

This passes build tests on the CI

https://ci.trafficserver.apache.org/view/github/job/Github-Linux/48/
https://ci.trafficserver.apache.org/view/github/job/Github-FreeBSD/36/

I'll review the code later this weekend.

[atsci]

Build finished.
Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/67/ for details.

[zizhong]

hey @zwoop , how is the code review going? is there any issue I need to address?

[zwoop]

I'm generally ok with this approach, at least until e.g. TS-3563 lands (or, we switch to Lua :). Let me do some more detailed code review, and then land it.

[zwoop]

Ok, so I have reviewed this, and I'm happy with it. At the risk of bike shedding, I'm throwing in some thoughts on coding style here. It's entirely up to you if you feel it worth changing or not. If you think it's fine as is, then I will commit this as is :). #5 below (avoiding std::string creations) is probably the one I care most about, since it has noticeable impact on performance.

1. We (or at least me) generally prefer Yoda style conditions. E.g. prefer

if (NULL == field_loc) { ...

I understand our code here is inconsistent either, but it might be nice to be consistent at least within the new stuff we add? The PR here mixes styles liberally :)

2. There are a couple of places where it nests if()'s seemingly unnecessarily, e.g.

+    if (cookie_modified) {
+      if (TS_SUCCESS ==

We could merge those into

 +   if (cookie_modified && (TS_SUCESS == ....

3. I personally prefer to have a little white space between local/block scope variables and the code. e.g.

+    }
+    int cookies_len = 0;
+    const char *cookies = TSMimeHdrFieldValueStringGet(res.bufp, res.hdr_loc, field_loc, -1, &cookies_len);
+    std::string updated_cookie;
+    bool cookie_modified =
+      CookieHelper::cookieModifyHelper(cookies, cookies_len, updated_cookie, CookieHelper::COOKIE_OP_DEL, _cookie);
+    if (cookie_modified) {
+      if (TS_SUCCESS ==

would become

+    }
+
+    int cookies_len = 0;
+    const char *cookies = TSMimeHdrFieldValueStringGet(res.bufp, res.hdr_loc, field_loc, -1, &cookies_len);
+    std::string updated_cookie;
+    bool cookie_modified =
+      CookieHelper::cookieModifyHelper(cookies, cookies_len, updated_cookie, CookieHelper::COOKIE_OP_DEL, _cookie);
+
+    if (cookie_modified) {
+      if (TS_SUCCESS ==

You do this in many places, but not consistently. Also, use your judgement here, for some cases, it makes sense not to introduce those empty lines, specially for very short functions.

4. I'm not super keen on namespace here. We have no precedence here, we use name spaces in some places, but this would be the first time in the header_rewrite plugin. Since we have no rules or guidelines around this, I'm probably ok with it, but my preference would be to not use namespace.

5. I'm definitely no expert on std::string, but this seems a bit wasteful (potentially):

      updated_cookies = std::string(cookies, value_start_idx) + cookie_value +
                        std::string(cookies + value_end_idx, cookies_len - value_end_idx);

Couldn't that be done with e.g.

update_cookies.append(cookies, value_start_idx);
update_cookies.append(cookie_value);
update_cookies.append(cookies+value_end_idx, cookies_len - value_end_idx);

or some such? Basically avoid creating those intermediary std::strings. I did a quick benchmark, the second pattern is roughly 4x faster than the first one (compiled with -O3). So, my recommendation is to avoid as many std::string intermediary strings as possible. Heck, it's probably (but I didn't test) preferable to use snprintf() over std:string creations :-).

6. Super nit-pick, but in e.g.

          TSDebug(PLUGIN_NAME, "Adding cookies %s", _cookie.c_str());

Shouldn't that be "Adding cookie %s"? Can you add more than one cookie with this?

One final thought: Have you considered something similar for Set-Cookie headers (from origin)? Can the existing code be extended (later) to support modifying those too? If so, do we make yet new operators for that, or somehow try to tag these ones with which direction to modify them? It seems generally useful to be able to do these manipulations both on the Cookie: (client) and Set-Cookie: (server). The wrinkle is that Set-Cookie supports multi-line headers, whereas Cookie does not.

I'm not saying this PR has to include the support for Set-Cookie, that's a separate Jira and PR. But something to think about maybe?

[zizhong]

Thanks for the instructive comments. I've fixed most of them. For the namespace, I would keep it as it makes code clearer.
About supporting Set-Cookie, let me address it with another ticket.

[zwoop]

Great, will take a look in a little bit, first CI build tests though [approve ci].

[atsci]

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/184/ for details.

[atsci]

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/292/ for details.

[zwoop]

Looks good. One more thing, didn't think about it last time: The update-cookie, will that work if the cookie doesn't already exist? If so, can we call it set-cookie instead? This is in line with our existing set-header / add-header operators. update-cookie sounds like it'd only work if the cookie value already exist, which seems harder to use than just a set-cookie which will overwrite any existing value for the cookie (whereas add-cookie should add it regardless I think?).

[zizhong]

The original name for this operator is exactly set-cookie. However, Brain was worried about the name could be confusing with set-cookie in HTTP headers.

[zwoop]

Hmmm. Well, update-cookie is similarly confusing, in that the name implies that it'd only work if the cookie already exists (at least to me :). Damned if we do, damned if we don't ... set-cookie would be inline with what other existing operators do, but I agree that there is the potential for confusion on Set-Cookie. @bgaff make a decision! :-)

[bgaff]

Well if I have to make the call let's just go Set-Cookie ;)

[zizhong]

Thanks @bgaff, @zwoop . All right. I'll change it back to set-cookie.

[zwoop]

[approve ci]

[atsci]

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/318/ for details.

[atsci]

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/212/ for details.