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.

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: Fix Host->bucket fallback logic inversion #10873

Merged
merged 1 commit into from Sep 12, 2016

Conversation

Projects
None yet
3 participants
@robbat2
Copy link
Contributor

commented Aug 25, 2016

The logic (added in 46aae19) for falling back to just using the hostname as
the possible bucket name contained an accidental inversion, because
RGWHandler_REST::validate_bucket_name returns success as zero.

Backport: jewel
Fixes: http://tracker.ceph.com/issues/17136
Re-Fixes: http://tracker.ceph.com/issues/15975
Signed-off-by: Robin H. Johnson robin.johnson@dreamhost.com

rgw: Fix Host->bucket fallback logic inversion
The logic (added in 46aae19) for falling back to just using the hostname as
the possible bucket name contained an accidental inversion, because
RGWHandler_REST::validate_bucket_name returns success as zero.

Backport: jewel
Fixes: http://tracker.ceph.com/issues/17136
Re-Fixes: http://tracker.ceph.com/issues/15975
Signed-off-by: Robin H. Johnson <robin.johnson@dreamhost.com>

@ghost ghost added rgw bug fix labels Aug 25, 2016

@cbodley

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2016

👍

@robbat2

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2016

Still waiting for this to be merged into master.

@cbodley cbodley merged commit a879c15 into ceph:master Sep 12, 2016

2 checks passed

Signed-off-by all commits in this PR are signed
Details
default Build finished.
Details
@cbodley

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2016

@robbat2 sorry, i should have run it through teuthology before merging. i had to revert this commit with #11102, because it causes us to add a / to the end of request_uri, which breaks s3 signature verification

we'll need to find a way to support this feature without side effects on authentication

@robbat2

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2016

@cbodley can you link the teuth output, ideally the s3tests? I'm wondering why it broke in your environment when it passed s3tests here.

@cbodley

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2016

@robbat2 an example here: http://qa-proxy.ceph.com/teuthology/cbodley-2016-09-14_14:00:58-rgw-wip-cbodley-testing---basic-mira/415753/teuthology.log

I was able to reproduce locally by changing the host = localhost to a fqdn in my s3tests.conf

@cbodley

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2016

@robbat2 okay, turns out that these s3test failures were due to a misconfigured rgw_dns_name. i'm guessing that yours matches the host you provided in s3tests.conf. vstart.sh was setting rgw_dns_name = localhost for me, and teuthology was using the empty default value (the log showed RGW hostnames:). in those cases, all requests were treated as virtual hosted buckets

@yehudasa because rgw_dns_name defaults to empty, would you be in favor of disabling the virtual hosting feature in that case? (i.e. if hostnames_set in rgw_rest.cc is empty, don't allow in_hosted_domain to be set)

@robbat2

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2016

If rgw_dns_name is empty, but the zonegroup has hostnames/hostnames_s3website set, then virtual hosting is still valid in my opinion.

@yehudasa

This comment has been minimized.

Copy link
Member

commented Sep 28, 2016

@cbodley @robbat2 I'm mostly in favor of documenting this clearly (whatever we choose).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.