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.

Sign up for GitHub

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

Jump to bottom

rgw: fix opslog uri as per Amazon s3 #16958

Merged
merged 1 commit into from Oct 20, 2017
Merged

rgw: fix opslog uri as per Amazon s3 #16958

merged 1 commit into from Oct 20, 2017

Conversation

mikulely
Copy link
Contributor

@mikulely mikulely commented Aug 10, 2017 •
edited by joscollin

According to Amazon s3[1], current Request-URI opslog entry are missing:

  • request method
  • query string
  • http version number

[1] http://docs.aws.amazon.com/AmazonS3/latest/dev/LogFormat.html

Fixes: http://tracker.ceph.com/issues/20971

Reported-by: Zhang Shaowen zhangshaowen@cmss.chinamobile.com
Signed-off-by: Jiaying Ren jiaying.ren@umcloud.com

@mikulely mikulely force-pushed the fix-full-uri branch 2 times, most recently from dc221c3 to 3c8429d Compare August 10, 2017 10:20
@joscollin joscollin self-requested a review August 10, 2017 10:27
@joscollin joscollin changed the title rgw: opslog add mising query string & http version rgw: fix opslog uri Aug 10, 2017
@joscollin joscollin changed the title rgw: fix opslog uri rgw: fix opslog uri as per Amazon s3 Aug 10, 2017
joscollin
joscollin approved these changes Aug 10, 2017
View reviewed changes
Copy link
Member

@joscollin joscollin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

You forgot to specify request method in the items listed in the commit message. I have corrected the PR Description. Please fix the commit message by yourself.

mikulely pushed a commit to mikulely/ceph that referenced this pull request Aug 10, 2017
rgw: fix opslog uri as per Amazon s3 ceph#16958
d820d24 
According to s3[1], current Request-URI opslog entry are missing:

+ request method
+ query string
+ http version number

[1] http://docs.aws.amazon.com/AmazonS3/latest/dev/LogFormat.html

Fixes: http://tracker.ceph.com/issues/20971

Reported-by: Zhang Shaowen <zhangshaowen@cmss.chinamobile.com>
Signed-off-by: Jiaying Ren <jiaying.ren@umcloud.com>
@mikulely
Copy link
Contributor Author

@joscollin Thx! repushed. ;-)

@joscollin
Copy link
Member

Test Result:

@joscollin
Copy link
Member

Jenkins retest this please

yehudasa
yehudasa reviewed Aug 29, 2017
View reviewed changes
src/rgw/rgw_log.cc Outdated

uri.append(s->info.env->get("REQUEST_URI"));
if(s->info.env->get("QUERY_STRING") &&
(strlen(s->info.env->get("QUERY_STRING")) != 0)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikulely @joscollin instead of calling strlen() here, should do: *(const char *)s->info.env->get("QUERY_STRING")) != '\0'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikulely @joscollin but should only call s->info.env->get("QUERY_STRING") once, so put it in a temp variable first and then do the two checks and append on it

@mikulely mikulely force-pushed the fix-full-uri branch 2 times, most recently from 4c35a55 to e2bba83 Compare September 11, 2017 09:14
@mikulely
Copy link
Contributor Author

repushed.

@mikulely
Copy link
Contributor Author

@joscollin @yehudasa ping

joscollin
joscollin requested changes Sep 25, 2017
View reviewed changes
Copy link
Member

@joscollin joscollin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks better now. But could you please remove the unwanted lines introduced in rgw_log.cc ?

joscollin
joscollin requested changes Sep 25, 2017
View reviewed changes
src/rgw/rgw_log.cc Outdated
@@ -357,6 +357,23 @@ int rgw_log_op(RGWRados *store, RGWREST* const rest, struct req_state *s,
else
set_param_str(s, "HTTP_REFERER", entry.referrer);
set_param_str(s, "REQUEST_URI", entry.uri);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we unnecessarily setting entry.uri here ? Correct me if I'm wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch!

@mikulely
Copy link
Contributor Author

Jenkins, retest this please.

@joscollin
Copy link
Member

retest this please

@joscollin
Copy link
Member

Sigh.
retest this please

cbodley
cbodley requested changes Sep 26, 2017
View reviewed changes
src/rgw/rgw_civetweb.cc
@@ -112,6 +112,7 @@ void RGWCivetWeb::init_env(CephContext *cct)

env.set("REMOTE_ADDR", info->remote_addr);
env.set("REQUEST_METHOD", info->request_method);
env.set("HTTP_VERSION", info->http_version);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please make a similar change to ClientIO::init_env() in rgw_asio_client.cc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the upcoming new frontend？ Yes！

@mikulely mikulely force-pushed the fix-full-uri branch 3 times, most recently from 2a3eef5 to 6800073 Compare October 12, 2017 08:42
@mikulely
Copy link
Contributor Author

Jenkins, retest this please.

@mikulely
Copy link
Contributor Author

@cbodley done

cbodley
cbodley reviewed Oct 16, 2017
View reviewed changes
src/rgw/rgw_log.cc Outdated
uri.append("HTTP/");
uri.append(s->info.env->get("HTTP_VERSION"));

entry.uri = uri;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since uri is a temporary string, consider a move instead of copy here

rgw: fix opslog uri as per Amazon s3
96bb12a 
According to s3[1], current Request-URI opslog entry are missing:

+ request method
+ query string
+ http version number

[1] http://docs.aws.amazon.com/AmazonS3/latest/dev/LogFormat.html

Fixes: http://tracker.ceph.com/issues/20971

Reported-by: Zhang Shaowen <zhangshaowen@cmss.chinamobile.com>
Signed-off-by: Jiaying Ren <jiaying.ren@umcloud.com>
@mikulely
Copy link
Contributor Author

@cbodley done.

@yuriw
Copy link
Contributor

yuriw commented Oct 18, 2017

wip-yuri-testing-2017-10-18-1628

@yuriw yuriw merged commit 9b18093 into ceph:master Oct 20, 2017
@mikulely mikulely deleted the fix-full-uri branch November 8, 2017 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yehudasa yehudasa yehudasa left review comments

@cbodley cbodley cbodley approved these changes

@joscollin joscollin joscollin approved these changes

Assignees
No one assigned
Labels
bug-fix needs-qa rgw wip-yuri-testing
Projects
None yet
Milestone
No milestone
5 participants
@mikulely @joscollin @yuriw @yehudasa @cbodley