allow bucket location responses with empty Message (fixes #629) #630
Conversation
bucket-cache.go
Outdated
| @@ -125,7 +125,7 @@ func processBucketLocationResponse(resp *http.Response, bucketName string) (buck | |||
| // For access denied error, it could be an anonymous | |||
| // request. Move forward and let the top level callers | |||
| // succeed if possible based on their policy. | |||
| if errResp.Code == "AccessDenied" && strings.Contains(errResp.Message, "Access Denied") { | |||
| if errResp.Code == "AccessDenied" && (errResp.Message == "" || strings.Contains(errResp.Message, "Access Denied")) { | |||
There was a problem hiding this comment.
This logic looks pretty ugly, we can just look for errResp.Code == "AccessDenied" only.
There was a problem hiding this comment.
@harshavardhana I would have no problem with removing the check on errResp.message entirely, I just was not aware of why that check existed and did not want to risk a regression against another system.
The conditions date back to when this code was first introduced in api-error-response.go in commit a856d43 but there does not appear to be any note there as to why the Message was a part of the condition. As you were the committer, if you have reason to think that errResp.code == "AccessDenied" is sufficient, I'll change to that.
There was a problem hiding this comment.
This change was introduced because some users with S3 have IAM policy where they disable certain operations i.e returning AccessDenied only.
Removes checks for "Access Denied" substring in the error response - if `errResp.Code == "AccessDenied"` for the bucket location request, that should be sufficient to trigger the fallback to default location.
|
Can you fix the build @jrandall ? |
|
Can you fix the build error @jrandall ? |
When Ceph radosgw responds to a bucket location request for a bucket which is not owned by the user, it issues an "AccessDenied" XML response that does not contain any
<Message>element. Because of this, the code that handles "anonymous" buckets inprocessBucketLocationResponseis not triggered, as it is looking for both anerrResp.Codeof "AccessDenied" and also anerrResp.Messagecontaining the substring "Access Denied".This PR changes the behaviour of
processBucketLocationResponseto allow an emptyerrResp.Messageto be treated the same as one that contains the substring "Access Denied".Fixes #629