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.
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
rgw: return valid Location element, CompleteMultipartUpload #19902
Conversation
src/rgw/rgw_rest_s3.cc
Outdated
} | ||
return domain; | ||
}(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for wrapping the code with the immediately executed lambda?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just permits lining it into the ternary assignment expr. most likely this should be consolidated into a routine, but I don't know where all it should be used yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, having a routine for that sounds nice. We might also want to unify with dump_uri_from_state
of rgw_rest.cc
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does my routine (based on the code formerly in this location but adjusted for tenant) do the right thing? dump_uri_from_state() is different, ignores tenants, deals with domain differently; need to get it right for all cases...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<Location>
crafted in the new way is definitely more consistent with examples from the AWS doc. The last thing I see is the HTTPS case.
Actually dump_uri_from_state
looks like a dead code. I will make a patch to drop it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rzarzynski that's a big help, I'll update this one with the cleanups
src/rgw/rgw_rest_s3.cc
Outdated
s->info.domain.c_str()); | ||
std::string domain = (!s->info.domain.empty()) ? s->info.domain : | ||
[&]() -> std::string { | ||
std::string domain = "http://"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we consider HTTPS here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I thought of that after typing. And we make this same mistake elsewhere. But are we supposed to just pick http/s, or match the incoming proto?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should stick to the incoming proto, I bet.
f8773d2
to
0357a8b
Compare
Returns URIs that should reach the server regardless of domain configuration, and following tenant:bucket convention, if applicable. Fixes: https://tracker.ceph.com/issues/22655 Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
http://pulpito.ceph.com/mbenjamin-2018-02-01_21:22:30-rgw-wip-rgw-mploc---basic-smithi/ |
Returns URIs that should reach the server regardless of domain
configuration, and following tenant:bucket convention, if
applicable.
Fixes: https://tracker.ceph.com/issues/22655
Signed-off-by: Matt Benjamin mbenjamin@redhat.com