Allow 403 for overwrite prevention #5134
Conversation
object_store/src/aws/client.rs
Outdated
| source: Box::new(source), | ||
| path: to.to_string(), | ||
| }, | ||
| Some(StatusCode::PRECONDITION_FAILED) | Some(StatusCode::FORBIDDEN) |
There was a problem hiding this comment.
I'm not sure about this, it will convert authorization errors into NotFound which whilst I can see the appeal for your specific workload, would be a regression/bug for others
There was a problem hiding this comment.
How about extending S3CopyIfNotExists to allow specifying the return code instead?
There was a problem hiding this comment.
Potentially, although I think it would need to be a new variant to avoid being a breaking change.
I would strongly encourage you to file this as a bug on whatever store is returning 403 though, it should not be returning 403. Perhaps it might be worth seeing if it can be fixed, as opposed to working around it here?
There was a problem hiding this comment.
I think this could be done non-breaking by allowing specification via a new environment variable (COPY_IF_NOT_EXISTS_RETURN_CODE) and make the default value 412?
I would strongly encourage you to file this as a bug on whatever store is returning 403 though, it should not be returning 403. Perhaps it might be worth seeing if it can be fixed, as opposed to working around it here?
That’s possible but it’s a commercial solution so the turn-around time makes this hard.
On your point about 403 being ‘wrong’ - I’d argue we are in non-standard territory and everything is fair game. I’m making use of some overwrite prevention logic and in that context 403 makes sense - you’re not allowed to overwrite the file.
There was a problem hiding this comment.
I’d argue we are in non-standard territory and everything is fair game
The HTTP specification is fairly unambiguous as to when a 403 is a valid return code - https://datatracker.ietf.org/doc/html/rfc7231#section-6.5.3
I think this could be done non-breaking by allowing specification via a new environment variable (COPY_IF_NOT_EXISTS_RETURN_CODE) and make the default value 412?
Yeah... It's kind of gross but AWS_COPY_IF_NOT_EXISTS_RETURN_CODE_OVERRIDE is unlikely to accidentally come up elsewhere, and might be a way to move forward should a solution from the vendor not be forthcoming
There was a problem hiding this comment.
The system I’m taking advantage of emulates object lock for non-versioned buckets, which returns 403 when you perform a destructive action:
so from its perspective, it’s implementing the API correctly. It’s not pretty but it works.
If this seems like an acceptable solution I’ll create the new environment variable check and modify the match.
There was a problem hiding this comment.
Fair enough - I also did a pass through https://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html#ErrorCodeList and there doesn't appear to be an error code for legal hold that we could detect and handle.
I think I would prefer adding a new HeaderWithStatus variant, as I think this will be more discoverable and consistent
|
Please ignore, I've just re-read your comment. Will re-implement with HeaderWithStatus |
object_store/src/aws/precondition.rs
Outdated
| } | ||
|
|
||
| impl std::fmt::Display for S3CopyIfNotExists { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| match self { | ||
| Self::Header(k, v) => write!(f, "header: {}: {}", k, v), | ||
| Self::HeaderWithStatus(k, v, code) => { | ||
| write!(f, "header-with-status: {}: {k}: {v}", code.as_u16()) |
There was a problem hiding this comment.
The status code comes first because as far as I can tell, all common separators one might use to separate the return code from the header are also valid to have inside the header.
There was a problem hiding this comment.
The HTTP protocol uses : as a separator itself, and so is forbidden in header names, and is probably a terrible idea in header values
There was a problem hiding this comment.
I'll swap the parse order and add some tests validating it.
object_store/src/aws/precondition.rs
Outdated
| } | ||
|
|
||
| impl std::fmt::Display for S3CopyIfNotExists { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| match self { | ||
| Self::Header(k, v) => write!(f, "header: {}: {}", k, v), | ||
| Self::HeaderWithStatus(k, v, code) => { | ||
| write!(f, "header-with-status: {}: {k}: {v}", code.as_u16()) |
There was a problem hiding this comment.
| write!(f, "header-with-status: {}: {k}: {v}", code.as_u16()) | |
| write!(f, "header-with-status: {k}: {v}: {}", code.as_u16()) |
I think the order should match the variant
Co-authored-by: Raphael Taylor-Davies <[email protected]>
Co-authored-by: Raphael Taylor-Davies <[email protected]>
Co-authored-by: Raphael Taylor-Davies <[email protected]>
Co-authored-by: Raphael Taylor-Davies <[email protected]>
|
Thank you for sticking with this |
Which issue does this PR close?
Closes #5132.
What changes are included in this PR?
This extends the match case and also guards the match on
if !overwrite.Are there any user-facing changes?
Enables receiving an
AlreadyExistserror for403whenoverwrite = false.Previously if you had
overwrite=falseand for some reason you receivedPRECONDITION_FAILED, you would have received anAlreadyExistserror.