Repository navigation
CLDSRV-910: Rewrite the data of a self-copy when its encryption changes - #6346
Conversation
Hello dvasilas,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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 |
|
/create_integration_branches |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_integration_branches |
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 1 file with indirect coverage changes @@ Coverage Diff @@
## development/9.3 #6346 +/- ##
===================================================
+ Coverage 85.35% 85.45% +0.09%
===================================================
Files 207 207
Lines 13468 13487 +19
===================================================
+ Hits 11496 11525 +29
+ Misses 1972 1962 -10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
19797ca to
f454274
Compare
History mismatchMerge commit #b754602cdec1f8112d03bb5a3d69865f654808ce on the integration branch It is likely due to a rebase of the branch Please use the The following options are set: create_integration_branches |
|
/force_reset |
Reset completeI have successfully deleted this pull request's integration branches. The following options are set: create_integration_branches |
ConflictA conflict has been raised during the creation of I have not created the integration branch. Here are the steps to resolve this conflict: git fetch
git checkout -B w/9.4/bugfix/CLDSRV-910-reencrypt-data-on-self-copy origin/development/9.4
git merge origin/bugfix/CLDSRV-910-reencrypt-data-on-self-copy
# <intense conflict resolution>
git commit
git push -u origin w/9.4/bugfix/CLDSRV-910-reencrypt-data-on-self-copyThe following options are set: create_integration_branches |
ConflictA conflict has been raised during the creation of I have not created the integration branch. Here are the steps to resolve this conflict: git fetch
git checkout -B w/9.5/bugfix/CLDSRV-910-reencrypt-data-on-self-copy origin/development/9.5
git merge origin/w/9.4/bugfix/CLDSRV-910-reencrypt-data-on-self-copy
# <intense conflict resolution>
git commit
git push -u origin w/9.5/bugfix/CLDSRV-910-reencrypt-data-on-self-copyThe following options are set: create_integration_branches |
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: create_integration_branches |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_integration_branches |
| !location.masterKeyId || | ||
| !masterKeyId || |
There was a problem hiding this comment.
Out of curiosity, why do we need to reencrypt if neither the existing encryption nor the configured encryption have a master key ID?
There was a problem hiding this comment.
Good point. This checks two cases that normally cannot happen:
!location.masterKeyId: The source object is encrypted (location.cipheredDataKey), but the metadata is missing the master key ID, so we can't tell whether it is the same key.!masterKeyId: The copy asks for encryption, but the destination has no master key ID
They cover potential regressions, we could arguably drop them to simplify the code.
But it was confusing to readers. I added a new commit to use kms.isSameMasterKey instead (returns false when either key is missing) and added the case to the function doc.
ConflictThere is a conflict between your branch Please resolve the conflict on the feature branch ( git fetch && \
git checkout origin/bugfix/CLDSRV-910-reencrypt-data-on-self-copy && \
git merge origin/development/9.3Resolve merge conflicts and commit git push origin HEAD:bugfix/CLDSRV-910-reencrypt-data-on-self-copyThe following options are set: create_integration_branches |
(cherry picked from commit 1e3ec08)
Only when the destination is encrypted and the data is unencrypted or under another master key; a same-key self-copy stays metadata-only.
e41b544 to
681a0ae
Compare
History mismatchMerge commit #ba2f7f3b1fa90db14b23a6bad738b2db285f4ae1 on the integration branch It is likely due to a rebase of the branch Please use the The following options are set: create_integration_branches |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
/force_reset |
Reset completeI have successfully deleted this pull request's integration branches. The following options are set: create_integration_branches |
ConflictA conflict has been raised during the creation of I have not created the integration branch. Here are the steps to resolve this conflict: git fetch
git checkout -B w/9.4/bugfix/CLDSRV-910-reencrypt-data-on-self-copy origin/development/9.4
git merge origin/bugfix/CLDSRV-910-reencrypt-data-on-self-copy
# <intense conflict resolution>
git commit
git push -u origin w/9.4/bugfix/CLDSRV-910-reencrypt-data-on-self-copyThe following options are set: create_integration_branches |
ConflictA conflict has been raised during the creation of I have not created the integration branch. Here are the steps to resolve this conflict: git fetch
git checkout -B w/9.5/bugfix/CLDSRV-910-reencrypt-data-on-self-copy origin/development/9.5
git merge origin/w/9.4/bugfix/CLDSRV-910-reencrypt-data-on-self-copy
# <intense conflict resolution>
git commit
git push -u origin w/9.5/bugfix/CLDSRV-910-reencrypt-data-on-self-copyThe following options are set: create_integration_branches |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_integration_branches |
|
/approve |
|
/status |
Status
The following options are set: approve, create_integration_branches |
Queue out of orderThe changeset has received all authorizations to enter the merge queue, Look into https://devdocs.scality.net/support/faq/#bert-e for guidance about The following options are set: approve, create_integration_branches |
|
/status |
Status
The following options are set: approve, create_integration_branches |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
This pull request does not target the following hotfix branch(es) so they
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve, create_integration_branches |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue CLDSRV-910. Goodbye dvasilas. |
A
CopyObjectof an object onto itself (same bucket and key, same location, versioning disabled) didn't rewrite the data, even when the copy changed the encryption.This is the recommended way to encrypt an unencrypted object or to re-encrypt under another master key
With this PR, a self-copy rewrites the data when the destination is encrypted, and the source data is either unencrypted or encrypted under another master key. A self-copy under the same master key stays metadata-only.
The PR cherry-picks CLDSRV-908, so that a self-copy that rewrites the data deletes the old data.
Note:
lib/kms/wrapper.jsis reformatted with prettier in a separate commit.