You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by zizhong <gi...@git.apache.org> on 2016/06/02 12:45:52 UTC
[GitHub] trafficserver pull request #692: [TS-4500] add cookie-rewrite functionality ...
GitHub user zizhong opened a pull request:
https://github.com/apache/trafficserver/pull/692
[TS-4500] add cookie-rewrite functionality into header-rewrite plugin
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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zizhong/trafficserver headerrewrite
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/692.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #692
----
commit 4e3cf4cb2c2f0fadb3558bb3441eb8ac083990ef
Author: Zizhong Zhang <zi...@linkedin.com>
Date: 2016-06-02T12:33:13Z
add cookie-rewrite functionality into header-rewrite plugin
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #692: [TS-4500] add cookie-rewrite functionality into he...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/692
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #692: [TS-4500] add cookie-rewrite functionality into he...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/692
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/318/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #692: [TS-4500] add cookie-rewrite functionality into he...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/692
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?).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #692: [TS-4500] add cookie-rewrite functionality into he...
Posted by zizhong <gi...@git.apache.org>.
Github user zizhong commented on the issue:
https://github.com/apache/trafficserver/pull/692
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #692: [TS-4500] add cookie-rewrite functionality into he...
Posted by zizhong <gi...@git.apache.org>.
Github user zizhong commented on the issue:
https://github.com/apache/trafficserver/pull/692
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #692: [TS-4500] add cookie-rewrite functionality into he...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/692
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/212/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #692: [TS-4500] add cookie-rewrite functionality into he...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/692
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/184/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #692: [TS-4500] add cookie-rewrite functionality ...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop closed the pull request at:
https://github.com/apache/trafficserver/pull/692
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #692: [TS-4500] add cookie-rewrite functionality into he...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/692
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! :-)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #692: [TS-4500] add cookie-rewrite functionality into he...
Posted by zizhong <gi...@git.apache.org>.
Github user zizhong commented on the issue:
https://github.com/apache/trafficserver/pull/692
hey @zwoop , how is the code review going? is there any issue I need to address?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #692: [TS-4500] add cookie-rewrite functionality into he...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/692
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #692: [TS-4500] add cookie-rewrite functionality into he...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/692
[approve ci]
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #692: [TS-4500] add cookie-rewrite functionality into he...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/692
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #692: [TS-4500] add cookie-rewrite functionality into he...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/692
Testing once more ... Bloody jenkins [approve ci].
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #692: [TS-4500] add cookie-rewrite functionality into he...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/692
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/292/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #692: [TS-4500] add cookie-rewrite functionality into he...
Posted by bgaff <gi...@git.apache.org>.
Github user bgaff commented on the issue:
https://github.com/apache/trafficserver/pull/692
Well if I have to make the call let's just go Set-Cookie ;)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #692: [TS-4500] add cookie-rewrite functionality into he...
Posted by zizhong <gi...@git.apache.org>.
Github user zizhong commented on the issue:
https://github.com/apache/trafficserver/pull/692
Thanks @bgaff, @zwoop . All right. I'll change it back to set-cookie.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #692: [TS-4500] add cookie-rewrite functionality into he...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/692
Testing once more ... Bloody jenkins [approve ci].
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #692: [TS-4500] add cookie-rewrite functionality into he...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/692
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/65/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #692: [TS-4500] add cookie-rewrite functionality into he...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/692
Build finished.
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/67/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #692: [TS-4500] add cookie-rewrite functionality into he...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/692
Great, will take a look in a little bit, first CI build tests though [approve ci].
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #692: [TS-4500] add cookie-rewrite functionality into he...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/692
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/67/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---