Server side encryption migrations should not be reported as an object… - #6292
Conversation
Hello sylvainsenechal,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
❌ 1 Tests Failed:
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
bdebe28 to
9814e36
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
| // oplog driven logic (CRR, bucket notifications). | ||
| // eslint-disable-next-line no-param-reassign | ||
| objMD.originOp = 's3:ObjectCreated:Copy'; | ||
| // Copy should be tested for 9.5 in INTGR-1038 |
There was a problem hiding this comment.
@BourgoisMickael On this old ticket : https://scality.atlassian.net/browse/INTGR-1038
you wrote a comment "It will trigger new bucket notification, but it will not trigger a replication for CRR"
With this change, it will not trigger bucket notif anymore. But I think you have integration tests that may expect it, so I'm letting you know you may have to update your tests
There was a problem hiding this comment.
I did that to follow the AWS way of updating the encryption: you do a copy of the object on itself while changing the encryption in metadata.
This is an internal operation but follows the way a user would do.
I can't find that actionUpdateObjectEncryption in the bucket notification documentation.
There was a problem hiding this comment.
Well that's the point 🤔 actionUpdateObjectEncryption is not in the list of notifications, so it wont trigger a notif unlike the existing ObjectCreated
There was a problem hiding this comment.
I though we wanted to actually receive a notification (even if it's internal change) as the metadata changes.
Is it how it's done in AWS as well ?
Otherwise I'm fine with it. Should this fix ship in development/9.4 ?
There was a problem hiding this comment.
Im gonna put it on 9.4
I don't know about AWS, but I guess there is no concept of "sse migration" in aws
|
Adding Maha as you kinda created the ticket from the discussion on the bb pr, and Mickael which may be affected on the Ring tests |
1f36e5b to
1bf89fa
Compare
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
| dataLocator.masterKeyId = `${keyArnPrefix}${dataLocator.masterKeyId}`; | ||
| } | ||
| } | ||
| // Reformating the key arn is not an operation that should trigger any |
There was a problem hiding this comment.
Typo: "Reformating" → "Reformatting" (double t).
| // Reformating the key arn is not an operation that should trigger any | |
| // Reformatting the key arn is not an operation that should trigger any |
There was a problem hiding this comment.
Spot it faster next time its at least the third time that I push and I just approved and created the waterfall branch -_-
|
/approve |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option The following options are set: approve |
Build failedThe build for commit did not succeed in branch bugfix/CLDSRV-990 The following options are set: approve |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
This pull request did not target the following hotfix branch(es) so they
Please check the status of the associated issue CLDSRV-990. Goodbye sylvainsenechal. The following options are set: approve |
4 participants
Issue: CLDSRV-990
Taking a look at the ticket :
We have this updateObjectEncryption function setting
objMD.originOp = 's3:ObjectCreated:Copy';
all the time which is indeed problematic as it sends bucket notifications (although this is debatable, sending a notif is not exactly a P0..), and also can trigger transition (which is more problematic and the reason why this ticket was created scality/backbeat#2830 (comment))
But, upon looking, the function is called from 5 paths :
objectPut
objectCopy
initMpu
objectGet
objectHead
The first 3 are writes, which calls updateEncryption with a flag, so that the originOp rewrite path is not even exercised at all (because these 3 writes will have their own logic to set origin op)
Only the 2 reads get/head can make a call that would wrongly set originOp to copy. But even then, this only happens in the even of a sse migration.
So I'm making a change on the originOp to use something that will not trigger bucket notif/transition
2 remarks :