fix putData : detect collision on specific provided version id - #6230
fix putData : detect collision on specific provided version id#6230SylvainSenechal wants to merge 1 commit into
Conversation
Hello sylvainsenechal,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
| errorInstances.BadRequest.customizeDescription('bad request: invalid x-scal-version-id header'), | ||
| ); | ||
| } | ||
| if (objMd && objMd.versionId === incomingVersionIdDecoded) { |
There was a problem hiding this comment.
No need for objMd.versionId === incomingVersionIdDecoded anymore as the middleware is already fetching objMd for the specific versionID provided in the header
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files
@@ Coverage Diff @@
## development/9.4 #6230 +/- ##
===================================================
+ Coverage 86.39% 86.40% +0.01%
===================================================
Files 212 212
Lines 14577 14574 -3
===================================================
- Hits 12594 12593 -1
+ Misses 1983 1981 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
148ca98 to
18d305d
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes Backbeat putData collision detection for replication of non-current object versions by ensuring the metadata lookup targets the specific replicated version (from x-scal-version-id) rather than always using the master version.
Changes:
- Decode
x-scal-version-idin the Backbeat router forPUT /_/backbeat/dataand use it as theversionIdfor metadata lookup soobjMdcorresponds to the replicated version. - Simplify collision detection in
putDatato treat the presence of that specific version’sobjMdas a collision (while explicitly excludingExternalNullVersionId/"null"). - Add functional test coverage for collisions on a non-current version and adjust the
"null"version test to cover cases where a master already exists.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/functional/backbeat/putData.js | Adds/updates functional tests to validate collision behavior for non-current versions and "null" version behavior. |
| lib/routes/routeBackbeat.js | Uses decoded x-scal-version-id to fetch version-specific metadata for putData, fixing collision detection semantics. |
maeldonn
left a comment
maeldonn
left a comment
There was a problem hiding this comment.
Logic is fine, need a little refactor
| } | ||
| if (objMd && objMd.versionId === incomingVersionIdDecoded) { | ||
| // Data already at destination for this version; return 409 with the existing | ||
| if (objMd) { |
There was a problem hiding this comment.
if (objMd) only means "this version exists" because the router's isPutDataApi block fetched the header version instead of the master; that coupling is easy to break. Clearer to drop the router block and fetch the version here with metadataGetObject (returns undefined when missing):
const incomingVersionId = decode(incomingVersionIdEncoded);
// ... BadRequest on decode error ...
return metadataGetObject(request.bucketName, request.objectKey, incomingVersionId, null, log, (err, versionMd) => {
if (err) {
return callback(err);
}
if (versionMd) {
// existing 409 conflict path, using versionMd.microVersionId
}
return writeData();
});There was a problem hiding this comment.
Mhh I'll let other reviewers comment on that but I don't fully agree :
Middleware/router is usually a good place to pre-extract generic info, here, the router already get the object after validating query params (bucket/object names), and it does it for all backbeat routes.
If we re add a metadataGetObject in this specific api, we are gonna call it twice because it's already called in the router.
I think it's an ok design to have the router handle the get object, the only thing a bit dirty is indeed that new logic where the router fetches an object specific version id only for putData
There was a problem hiding this comment.
I think this is not a theoretical problem, we need to be practical:
- the router makes one metadata call already; we must not make a second metadata to avoid modifying the router on principle. Metadata calls are costly at scale, we should not make a useless one.
- in addition, the router does auth check on the object it requests. So pointing at the wrong object could lead to incorrect auth (esp. in cross-account case)
- modifying the router must be done carefully though, to ensure we don't break any case: neither
putData, nor other cases - I think there is a case in putMetadata where we also make the call without versionId, and eventually need to make a second metadata call to get actual version. Bonus point if we can fix it at the same time (but anyway should look at it, as it could steer the change in another direction)
There was a problem hiding this comment.
Ok so lets keet it here, will recheck putmetadata
| // For putData api: the version to check is passed | ||
| // via x-scal-version-id header, not the URL query. Fetch that specific | ||
| // version so objMd matches the replicated version, not always the master. | ||
| const isPutDataApi = request.method === 'PUT' && request.resourceType === 'data'; |
There was a problem hiding this comment.
The router shouldn't special-case one handler, and these guards duplicate putData's. Move the version lookup into putData itself (see handler comment) so this block goes away.
There was a problem hiding this comment.
This is debatable if we consider that the router acts as a middleware that fetches objMd for all apis, in that case, we wouldn't want each apis functions to have their own extra ways of dealing with fetching again objMd.
Let's discuss with the first comment
There was a problem hiding this comment.
the router already has many cases, which are kind of hard-coded.
Maybe we can do this based on the presence of the header (if versionIdHeader ...)? I.e. set the query.versionId field from this header if it was not present already, before calling decodeVersionId ?
either way, even if we keep the "is put api" guard, no point doing both decodeVersionId and handling if this header:
| const isPutDataApi = request.method === 'PUT' && request.resourceType === 'data'; | |
| const versionId = ...... ? // whatever condition: header present, put api, ... | |
| decodeVersionId(request.query) : decodeScalVersionId(header); |
(or move it in a separate function, out of here)
There was a problem hiding this comment.
I checked cloudserverClient, we only have 3 other apis (MPUxx) using the versionId in header, and these request are treated differently. We can remove the "if isPutData" and do something simpler
c3d0e4b to
c9814a4
Compare
There was a problem hiding this comment.
General comment regarding Maël 3 feedbacks : I mostly agree with him that there is weirdness in the code changes, and that the current solution doesn't feel fully satisfying, but I don't think there is any perfect way to do it, else we would need a bigger refactoring
There was a problem hiding this comment.
else we would need a bigger refactoring
generally : if the proper way requires a refactoring, it should be done. The risk otherwise is to end up with an accumulation of code which is just slightly off - but taken together ends up being a huge pile of mess...
so one must dig into this "not fully satisfying feeling" : is it because we are introducing debt? is that debt something that may come back at us, or something easily manageable? what refactoring would be required? would this refactoring really help the problem here, or just cleanup the rest (but the change would still be as complex)? How long would this refactor take, is it worth it? is it worth it to solve the specific problem at hand? Is it because the problem should be solved more simply? in that case, does the complexity come from the problem it self (i.e. not so easy because of some corner case/...), from some implementation choice, ...? are there some conditions (context) which could allow for a better solution? ...
| incomingVersionIdEncoded !== 'null' ? decode(incomingVersionIdEncoded) : 'null'; | ||
| if (incomingVersionIdDecoded instanceof Error) { | ||
| // ExternalNullVersionId means a pre-versioning null object: collision detection is not applicable. | ||
| if (incomingVersionIdEncoded !== undefined && incomingVersionIdEncoded !== ExternalNullVersionId) { |
There was a problem hiding this comment.
abit hard to read but the diff is equivalent, in both case, the logic is :
"do not try to decode a 'null' versionId", as this would throw an error, and we dont wanna throw on 'null' versionId, they are just special case.
Prefer this new code, to avoid weird ternary and start using ExternalNullVersionId arsenal constant which will remind developers that 'null' versionId is a thing 👀
|
@francoisferrand added as Maël not here |
| if (objMd && objMd.versionId === incomingVersionIdDecoded) { | ||
| // Data already at destination for this version; return 409 with the existing | ||
| if (objMd) { | ||
| // objMd is the specific version (fetched by versionId from x-scal-version-id header) |
There was a problem hiding this comment.
not true : in case of decode error, you fallback to the "old" path.... so you may still receive an objMD from the master, even though a version id was provided.
i.e. same as https://github.com/scality/cloudserver/pull/6230/changes#r3637279080 : on header decode error the API should fail. It should not fallback to requesting master if a versionId is provided.
There was a problem hiding this comment.
you're right, considering that versionIdHeader is provided, it means the client specifically asks for a version ID, so now the middleware will return an error when failing to decode
c9814a4 to
7129a18
Compare
7129a18 to
031c3c4
Compare
| ); | ||
| // ExternalNullVersionId means a null version, which does not need decoding | ||
| if (incomingVersionIdEncoded !== ExternalNullVersionId) { | ||
| if (objMd) { |
There was a problem hiding this comment.
Actually removed the whole decode here : It's already done in the middleware, we could almost do the whole remaining logic in the middleware but its probably not the right place
There was a problem hiding this comment.
the conflict logic does not belong in middleware - it is specific to some routes only. It's not about code (we could do anything), but about abstractions and layers: and that logic certainly does not belong in the middleware, while decoding the versionId does.
There was a problem hiding this comment.
else we would need a bigger refactoring
generally : if the proper way requires a refactoring, it should be done. The risk otherwise is to end up with an accumulation of code which is just slightly off - but taken together ends up being a huge pile of mess...
so one must dig into this "not fully satisfying feeling" : is it because we are introducing debt? is that debt something that may come back at us, or something easily manageable? what refactoring would be required? would this refactoring really help the problem here, or just cleanup the rest (but the change would still be as complex)? How long would this refactor take, is it worth it? is it worth it to solve the specific problem at hand? Is it because the problem should be solved more simply? in that case, does the complexity come from the problem it self (i.e. not so easy because of some corner case/...), from some implementation choice, ...? are there some conditions (context) which could allow for a better solution? ...
| // ExternalNullVersionId means a null version, which does not need decoding | ||
| if (incomingVersionIdEncoded !== ExternalNullVersionId) { | ||
| if (objMd) { |
There was a problem hiding this comment.
should have a conflict as well in case of an existing object with null versionId!
| // ExternalNullVersionId means a null version, which does not need decoding | |
| if (incomingVersionIdEncoded !== ExternalNullVersionId) { | |
| if (objMd) { | |
| if (incomingVersionIdEncoded !== undefined && objMd) { |
There was a problem hiding this comment.
This is true, I just verified and added a test that when provided version id is 'null', the backend does fetch the null version instead of master which i wasnt sure about
| error: decodedVidResult, | ||
| }); | ||
| return next(errors.InvalidArgument); | ||
| const versionIdHeader = request.headers['x-scal-version-id']; |
There was a problem hiding this comment.
this makes the x-scal-version-id overrides the (possibly missing) query.versionId field.
- Can you please double check which routes are affected / in which case (i.e. which route use x-scal-versionId), and especially confirm in each case that it should indeed look up this object instead of the query.versionId / master one?
- Should we abort/fail -or at least log a warning- if the request has both x-scal-version-id and query.versionId ? If fear this code could allow abused to silently override the query.versionId field, and possibly break stuff...
- Do we have some validation about allowed/required fields, eg. putData has an
x-scal-version-id, deleteObjectFromLifecycle has a requiredquery.versionId, etc... It does not seem to be handled here, is it handled in each handler? (maybe not a new issue, but we should consider if we need to add something for putData/putMetadata, and possibly a followup to cover the other/existing API...)
There was a problem hiding this comment.
Good remarks, and I talked about it in these 2 comments :
#6230 (comment)
#6230 (comment)
The bottom line is, I feel we messed up something in the design of CRR Cascade by adding version id in the header instead of query params, and well we still have time to change it so maybe we should 🤔
| if (versionIdHeader !== ExternalNullVersionId) { | ||
| const decoded = decode(versionIdHeader); | ||
| if (decoded instanceof Error) { |
There was a problem hiding this comment.
is this not the decodeVID() function ?
There was a problem hiding this comment.
no decodeVersionId is for request.query (helper that extract the version id from the query before decoding)
and for the header we use directly decode from arsenal
edit: ok there is a third one decodeVID...
I'm gonna check together with your other comments, I think this will be clearer if extracted into a function
| }); | ||
| return next(errors.InvalidArgument); | ||
| } | ||
| versionId = decodedVidResult; |
There was a problem hiding this comment.
nit: code is kind of duplicated, and kind of not the right level for this function
could we change "remap" the x-scal-version-id for some routes maybe?
e.g.
if (request.headers['x-scal-version-id'] !== undefined) {
request.query = request.headers['x-scal-version-id'];
}
const decodedVidResult = decodeVersionId(request.query);
[...]or just move this logic in a dedicated function?
func decodeRequestVersionId(request) {
const versionIdHeader = request.headers['x-scal-version-id']
if (versionIdHeader !== undefined) {
return versionIdHeader;
}
return decodeVersionId(request.query);
}
[...]
const decodedVidResult = decodeRequestVersionId(request);
if (decodedVidResult instanceof Error) {
return next(errors.InvalidArgument.customizeDescription(decodedVidResult.error));
}
const versionId = decodedVidResult;There was a problem hiding this comment.
I did some refactoring, its nice the decodeVID function handle the 'null' case.
I think it's simple enough that we don't need a helper function and can inline it.
i can't help but notice we could refactor it further... between arsenal and cloudserver we have 3 decodes functions....
decodeVersionId
decodeVID
decode
| ); | ||
| // ExternalNullVersionId means a null version, which does not need decoding | ||
| if (incomingVersionIdEncoded !== ExternalNullVersionId) { | ||
| if (objMd) { |
There was a problem hiding this comment.
the conflict logic does not belong in middleware - it is specific to some routes only. It's not about code (we could do anything), but about abstractions and layers: and that logic certainly does not belong in the middleware, while decoding the versionId does.
|
@francoisferrand I think the decode version id is clearer now although as I said in one of my response comment we could probably go even further in the refactoring of this function. What I wanna address though is this comment, not sure you saw it : #6230 (comment) I realize the version id passed in the header was added for CRR Cascade, and it doesn't seem like we had a strong argument to do it vs just passing it in query params like the other. If we changed cloudserver client to pass it in the query param, it would be less troublesome and cleaner, actually this whole pr wouldn't have existed because we would've fetched the right version from the query directly.. |
031c3c4 to
1b7b0e2
Compare
| err instanceof VersionIdCollisionException, | ||
| `expected VersionIdCollisionException, got ${err.constructor.name}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The other collision tests (lines 88-92 and 146) assert err.microVersionId. This test should do the same for consistency and to catch regressions where the microVersionId is incorrect for null-version collisions.
| } | |
| assert.ok( | |
| err instanceof VersionIdCollisionException, | |
| `expected VersionIdCollisionException, got ${err.constructor.name}`, | |
| ); | |
| assert.strictEqual(err.microVersionId, '', 'microVersionId should be empty for null-version collision'); |
Issue: CLDSRV-953
The putData collision detection previously fetched the master object and compared its versionId against the incoming x-scal-version-id header. This caused issues when replicating a non-current version : the master's versionId wouldn't match, so no collision was detected and data was re-uploaded unnecessarily
Fix : decode the x-scal-version-id header in the router and use it as the metadata lookup key, so objMd is always the specific version being replicated