You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by robguima <gi...@git.apache.org> on 2016/02/05 20:59:34 UTC
[GitHub] trafficserver pull request: TS-4176: s3_auth add support for matri...
GitHub user robguima opened a pull request:
https://github.com/apache/trafficserver/pull/455
TS-4176: s3_auth add support for matrix params
@shukitchan please review
https://issues.apache.org/jira/browse/TS-4176
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/robguima/trafficserver TS-4176
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/455.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 #455
----
commit 476a540f650d7515b14e3064e5d3820123f9b811
Author: guima <gu...@yahoo-inc.com>
Date: 2016-02-05T19:55:59Z
TS-4176: s3_auth add support for matrix params
----
---
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: TS-4176: s3_auth add support for matri...
Posted by shukitchan <gi...@git.apache.org>.
Github user shukitchan commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/455#discussion_r52065719
--- Diff: plugins/experimental/s3_auth/s3_auth.cc ---
@@ -346,26 +349,48 @@ S3Request::authorize(S3Config *s3)
// If the configuration is a "virtual host" (foo.s3.aws ...), extract the
// first portion into the Host: header.
if (s3->virt_host()) {
- host_loc = TSMimeHdrFieldFind(_bufp, _hdr_loc, TS_MIME_FIELD_HOST, TS_MIME_LEN_HOST);
- if (host_loc) {
- host = TSMimeHdrFieldValueStringGet(_bufp, _hdr_loc, host_loc, -1, &host_len);
+ field_loc = TSMimeHdrFieldFind(_bufp, _hdr_loc, TS_MIME_FIELD_HOST, TS_MIME_LEN_HOST);
+ if (field_loc) {
+ host = TSMimeHdrFieldValueStringGet(_bufp, _hdr_loc, field_loc, -1, &host_len);
host_endp = static_cast<const char *>(memchr(host, '.', host_len));
} else {
return TS_HTTP_STATUS_INTERNAL_SERVER_ERROR;
}
}
+ // Just in case we add Content-MD5 if present
+ field_loc = TSMimeHdrFieldFind(_bufp, _hdr_loc, TS_MIME_FIELD_CONTENT_MD5, TS_MIME_LEN_CONTENT_MD5);
+ if (field_loc) {
+ con_md5 = TSMimeHdrFieldValueStringGet(_bufp, _hdr_loc, field_loc, -1, &con_md5_len);
+ }
+
+ // get the Content-Type if available - (buggy) clients may send it
+ // for GET requests too
+ field_loc = TSMimeHdrFieldFind(_bufp, _hdr_loc, TS_MIME_FIELD_CONTENT_TYPE, TS_MIME_LEN_CONTENT_TYPE);
+ if (field_loc) {
+ con_type = TSMimeHdrFieldValueStringGet(_bufp, _hdr_loc, field_loc, -1, &con_type_len);
+ }
+
--- End diff --
I think we should do a TSHandleMLocRelease on field_loc after each usage instead of one at the end.
---
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: TS-4176: s3_auth add support for matri...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/455#issuecomment-180905142
One question: Can we be sure this doesn't break anything "backwards compatible" ? I.e. someone using the plugin today, it should just work the same with these changes?
---
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: TS-4176: s3_auth add support for matri...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/455#issuecomment-180669497
Please squash this down to one commit IMO.
---
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: TS-4176: s3_auth add support for matri...
Posted by shukitchan <gi...@git.apache.org>.
Github user shukitchan commented on the pull request:
https://github.com/apache/trafficserver/pull/455#issuecomment-185373900
thanks... let me merge it tonight.
---
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: TS-4176: s3_auth add support for matri...
Posted by robguima <gi...@git.apache.org>.
Github user robguima commented on the pull request:
https://github.com/apache/trafficserver/pull/455#issuecomment-180640259
@shukitchan just to undo my formatting change from older patch - think someone did that between 5.3 and 6.x
---
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: TS-4176: s3_auth add support for matri...
Posted by shukitchan <gi...@git.apache.org>.
Github user shukitchan commented on the pull request:
https://github.com/apache/trafficserver/pull/455#issuecomment-180587604
looks good now.
---
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: TS-4176: s3_auth add support for matri...
Posted by shukitchan <gi...@git.apache.org>.
Github user shukitchan commented on the pull request:
https://github.com/apache/trafficserver/pull/455#issuecomment-180535806
@jpeach , can you take a look as well?
---
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: TS-4176: s3_auth add support for matri...
Posted by robguima <gi...@git.apache.org>.
Github user robguima commented on the pull request:
https://github.com/apache/trafficserver/pull/455#issuecomment-180907419
Yes, we can be fairly sure it doesn't (break anything), because:
a) we've added functionality not changing the way it worked before (bug aside). So, if content-type, md5, and matrix params are not present, it should behave as before.
b) These fields not being present is actually the most common situation - so we've actually verified that the old functionality remains as is, because in most cases, md5, content-type, and matrix params are not used in GET requests.
The only caveat I can think of is using virtual host, which I am not and can't (very easily). Based on the code though, that little logic (prepending the host) seems unchanged by this PR.
This code is deployed in "production" right now in our aws region, and being tested routinely.
---
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: TS-4176: s3_auth add support for matri...
Posted by shukitchan <gi...@git.apache.org>.
Github user shukitchan commented on the pull request:
https://github.com/apache/trafficserver/pull/455#issuecomment-180906411
I think we should use TSDebug as well.
The change should be "backwards compatible".
---
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: TS-4176: s3_auth add support for matri...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafficserver/pull/455
---
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: TS-4176: s3_auth add support for matri...
Posted by robguima <gi...@git.apache.org>.
Github user robguima commented on the pull request:
https://github.com/apache/trafficserver/pull/455#issuecomment-181600678
2) hmm string is one of the few things I like about C++ :) especially for, like you said, code that is not in the main code path (e.g. debugging). It's just convenient and safer (null-termination) than using fixed size buffer with a bunch of checks - but your call.
---
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: TS-4176: s3_auth add support for matri...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/455#issuecomment-180911692
Changing to TSDebug is fine, I don't recall why printf() was used, probably laziness :) I'll try to test this in our setup next week, but I agree that the changes ought to be backwards compatible.
---
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: TS-4176: s3_auth add support for matri...
Posted by gtenev <gi...@git.apache.org>.
Github user gtenev commented on the pull request:
https://github.com/apache/trafficserver/pull/455#issuecomment-185330773
- Reviewed and tested the patch and it looks OK.
- Verified that it does not break backward compatibility, tested end-to-end against S3 with both virtual-hosted-style and path-style methods.
- Tested the Content-Type and Content-MD5 features and they worked correctly.
- Tested the matrix parameters feature, did not do end-to-end against S3 since we don't use matrix parameters in our setup but verified that the matrix parameters are added correctly to the CanonicalizedResource in the StringToSign and that the Authorization header is calculated correctly.
---
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: TS-4176: s3_auth add support for matri...
Posted by robguima <gi...@git.apache.org>.
Github user robguima commented on the pull request:
https://github.com/apache/trafficserver/pull/455#issuecomment-181591169
@zwoop I will squash it to one commit if this change is OK.
---
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: TS-4176: s3_auth add support for matri...
Posted by robguima <gi...@git.apache.org>.
Github user robguima commented on the pull request:
https://github.com/apache/trafficserver/pull/455#issuecomment-181994021
@zwoop @shukitchan please let me know if the std::string free version above is OK - if so I will squash it thx
---
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: TS-4176: s3_auth add support for matri...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/455#issuecomment-181604662
Yeah, the problem with it (and pretty much all of STL) is that it has hidden (and sometimes) unpredictable memory allocations behavior. I did some tests recently with bcall, where different compilers does different things when passing strings as argument to a function (one copied the string, the other did not).
I'm ok with std::string inside the if(debug tag) if you insist. My preference would be not to use std::string unless absolutely necessary (because it sets bad precedence), but it's difficult to make a strong argument here against 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 pull request: TS-4176: s3_auth add support for matri...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/455#issuecomment-181594755
Couple of things on that last patch:
1) You have to run clang-format over the code, I'm fairly certain we would never indent like this:
if (con_md5) TSDebug(PLUGIN_NAME, "%.*s", con_md5_len, con_md5);
2) Must we really introduce usage of std::string left("/"); ?? I guess this is inside a debug if() check, it's not a huge deal, but I cringe when I see std::string :-).
---
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: TS-4176: s3_auth add support for matri...
Posted by robguima <gi...@git.apache.org>.
Github user robguima commented on the pull request:
https://github.com/apache/trafficserver/pull/455#issuecomment-181605333
@zwoop thought that was a pretty good argument myself. :) not a prob will change it - also forgot you guys are multiplat...
---
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: TS-4176: s3_auth add support for matri...
Posted by robguima <gi...@git.apache.org>.
Github user robguima commented on the pull request:
https://github.com/apache/trafficserver/pull/455#issuecomment-180702368
@zwoop done thanks!
---
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.
---