Skip to content

Comments

rgw: fetch_remote_obj() preserves original part lengths for BlockDecrypt#52248

Merged
cbodley merged 7 commits intoceph:mainfrom
cbodley:wip-46062
Aug 4, 2023
Merged

rgw: fetch_remote_obj() preserves original part lengths for BlockDecrypt#52248
cbodley merged 7 commits intoceph:mainfrom
cbodley:wip-46062

Conversation

@cbodley
Copy link
Contributor

@cbodley cbodley commented Jun 28, 2023

because multisite replicates multipart objects as a single part, we lose information about the part sizes from the original manifest that is necessary to correctly decrypt across those part boundaries

on replication, parse the part lengths out of the source object's manifest, and store them in a separate RGW_ATTR_CRYPT_PARTS for use on decryption

Fixes: https://tracker.ceph.com/issues/46062

Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows

@cbodley cbodley requested a review from mdw-at-linuxbox June 28, 2023 21:22
@cbodley
Copy link
Contributor Author

cbodley commented Jun 29, 2023

this does seem to fix the WARNING: MD5 signatures do not match from the reproducer in https://tracker.ceph.com/issues/46062#note-6

@mattbenjamin
Copy link
Contributor

mattbenjamin commented Jun 29, 2023

@cbodley how is this different from what @mdw-at-linuxbox is working on?

@cbodley
Copy link
Contributor Author

cbodley commented Jun 29, 2023

@cbodley how is this different from what @mdw-at-linuxbox is working on?

it's the same thing. i emailed him about this yesterday

@cbodley cbodley mentioned this pull request Jun 29, 2023
Closed
2 tasks
@cbodley
Copy link
Contributor Author

cbodley commented Jun 30, 2023 •
edited
Loading

review feedback from @mdw-at-linuxbox:

  • add versioning to encoding of RGW_ATTR_CRYPT_PARTS
  • support AppendObject on replicated objects by appending to RGW_ATTR_CRYPT_PARTS
  • handle chaining replication (site A -> site B -> site C) by preserving source RGW_ATTR_CRYPT_PARTS if they exist

@cbodley cbodley marked this pull request as ready for review August 1, 2023 18:19
@cbodley cbodley requested a review from a team as a code owner August 1, 2023 18:19
@cbodley
Copy link
Contributor Author

cbodley commented Aug 2, 2023

jenkins test make check

@cbodley
Copy link
Contributor Author

cbodley commented Aug 2, 2023

jenkins test api

@cbodley
Copy link
Contributor Author

cbodley commented Aug 3, 2023

a make check build failure:

src/test/rgw/test_rgw_crypto.cc:431:28: error: no matching constructor for initialization of 'RGWGetObj_BlockDecrypt'
    RGWGetObj_BlockDecrypt decrypt(&no_dpp, g_ceph_context, &get_sink, std::move(cbc), null_yield);
                           ^       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

and multisite tests showed crashes on realm reload:

src/rgw/rgw_coroutine.cc: 117: FAILED ceph_assert(waiters.find(opaque) == waiters.end())
 
 ceph version 18.0.0-5291-gf936671d (f936671d75974064c7553c1944a8f21421a054f8) reef (dev)
 1: (ceph::__ceph_assert_fail(char const*, char const*, int, char const*)+0x12e) [0x7fa0b156107d]
 2: /usr/lib64/ceph/libceph-common.so.2(+0x16123b) [0x7fa0b156123b]
 3: (RGWCompletionManager::wait_interval(void*, utime_t const&, void*)+0x2ec) [0x56162175821c]
 4: (RGWContinuousLeaseCR::operate(DoutPrefixProvider const*)+0x300) [0x561621b23010]
 5: (RGWCoroutinesStack::operate(DoutPrefixProvider const*, RGWCoroutinesEnv*)+0x125) [0x561621762215]
 6: (RGWCoroutinesManager::run(DoutPrefixProvider const*, std::__cxx11::list<RGWCoroutinesStack*, std::allocator<RGWCoroutinesStack*> >&)+0x2b6) [0x561621763a86]

i doubt it's caused by this PR (maybe sync fairness?) so i scheduled tests against main in https://pulpito.ceph.com/cbodley-2023-08-03_02:45:05-rgw:multisite-main-distro-default-smithi/

@cbodley
Copy link
Contributor Author

cbodley commented Aug 3, 2023

i doubt it's caused by this PR (maybe sync fairness?) so i scheduled tests against main in https://pulpito.ceph.com/cbodley-2023-08-03_02:45:05-rgw:multisite-main-distro-default-smithi/

i see the same crash there on main. opened https://tracker.ceph.com/issues/62298 with details (cc @smanjara)

cbodley added 3 commits August 3, 2023 09:01
@cbodley
rgw: BlockDecrypt filter parses manifest parts before construction
d8a82f5 
users now call a static read_manifest_parts() function, and pass the
resulting vector into the BlockDecrypt constructor

Signed-off-by: Casey Bodley <[email protected]>
@cbodley
rgw: fetch_remote_obj() preserves original part lengths for BlockDecrypt
e7d98fb 
because multisite replicates multipart objects as a single part, we lose
information about the part sizes from the original manifest that is
necessary to correctly decrypt across those part boundaries

on replication, parse the part lengths out of the source object's
manifest, and store them in a separate RGW_ATTR_CRYPT_PARTS for use on
decryption

Fixes: https://tracker.ceph.com/issues/46062

Signed-off-by: Casey Bodley <[email protected]>
@cbodley
rgw: preserve RGW_ATTR_CRYPT_PARTS of already-replicated objects
902d074 
Signed-off-by: Casey Bodley <[email protected]>
@cbodley cbodley requested a review from smanjara August 3, 2023 13:01
@github-actions github-actions bot added the tests label Aug 3, 2023
@cbodley
Copy link
Contributor Author

cbodley commented Aug 3, 2023

@smanjara i'd love your help with review. Marcus hasn't approved, but he did cherry-pick this downstream where it was verified by QE

@smanjara
Copy link
Contributor

smanjara commented Aug 3, 2023

@cbodley the pr looks good to me! also ran a quick test locally along with #51786 and it passes successfully.

i'm looking into the crash.

@cbodley
Copy link
Contributor Author

cbodley commented Aug 3, 2023

@cbodley the pr looks good to me! also ran a quick test locally along with #51786 and it passes successfully.

thanks! could i ask for your approval on #51786 as well?

@cbodley
Copy link
Contributor Author

cbodley commented Aug 3, 2023

i pulled in the commits from #51842 and added a release note with recovery instructions

@cbodley cbodley merged commit fb13353 into ceph:main Aug 4, 2023
This was referenced Aug 4, 2023
Merged
Merged
@cbodley cbodley mentioned this pull request Sep 12, 2023
Merged
14 tasks
@tobias-urdin
Copy link
Contributor

tobias-urdin commented Oct 13, 2023

@cbodley this was never pulled in to pacific what i can find? this tracker https://tracker.ceph.com/issues/62322

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@smanjara smanjara smanjara approved these changes

@mdw-at-linuxbox mdw-at-linuxbox Awaiting requested review from mdw-at-linuxbox

Assignees

No one assigned

Labels

bug-fix documentation rgw tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cbodley @mattbenjamin @smanjara @tobias-urdin