Skip to content

rgw/multisite: include request body when CreateBucket op is forwarded to master#59960

Merged
cbodley merged 1 commit intoceph:mainfrom
smanjara:wip-fix-missing-http-data
Jan 9, 2025
Merged

rgw/multisite: include request body when CreateBucket op is forwarded to master#59960
cbodley merged 1 commit intoceph:mainfrom
smanjara:wip-fix-missing-http-data

Conversation

@smanjara
Copy link
Contributor

@smanjara smanjara commented Sep 24, 2024

Contribution Guidelines

  • To sign and title your commits, please refer to Submitting Patches to Ceph.

  • If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

  • When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
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
  • jenkins test rook e2e

@smanjara smanjara requested a review from a team as a code owner September 24, 2024 21:26
@cbodley
Copy link
Contributor

cbodley commented Sep 25, 2024

cc @clwluvw

@clwluvw
Copy link
Member

clwluvw commented Sep 25, 2024

Can you please take a look at #59305? I believe there is more about it.

@cbodley
Copy link
Contributor

cbodley commented Sep 25, 2024

Can you please take a look at #59305? I believe there is more about it.

if we're forwarding the same request body, we shouldn't need to mess with content-sha256 or re-sign the request. maybe you can suggest a test case to show what's missing here?

@clwluvw
Copy link
Member

clwluvw commented Sep 25, 2024

There are two different sides to the problem. One is about the behavior of RGWRESTSimpleRequest::forward_request when the body is null or needs a re-sign and one is regarding the create bucket which uses the forwarded rgwx- header for location param

ceph/src/rgw/rgw_op.cc

Lines 3477 to 3481 in 37e0285

if (s->system_request) {
// allow system requests to override the target zonegroup. for forwarded
// requests, we'll create the bucket for the originating zonegroup
createparams.zonegroup_id = s->info.args.get(RGW_SYS_PARAM_PREFIX "zonegroup");
}

This PR eliminates the fact that the body is empty and the signature is mismatched but the other problems of the mentioned code and when it needs to be re-sign (I faced that in delete I guess) and when the body is empty (which probably with this PR we don't have the case but can be fixed generally as the logic) are still there.

@smanjara smanjara mentioned this pull request Sep 25, 2024
Closed
@clwluvw clwluvw mentioned this pull request Oct 9, 2024
Closed
@clwluvw
Copy link
Member

clwluvw commented Oct 10, 2024

FYI, another issue on bucket creation: #60254

clwluvw
clwluvw reviewed Nov 17, 2024
View reviewed changes
@mattbenjamin
Copy link
Contributor

mattbenjamin commented Nov 18, 2024

@shilpa are you going to approve this? It is in use downstream.

@smanjara
Copy link
Contributor Author

smanjara commented Nov 19, 2024

@shilpa are you going to approve this? It is in use downstream.

@mattbenjamin wasn't happy with the test I added. but i guess it'll do.

scheduled a run https://pulpito.ceph.com/smanjara-2024-11-19_20:26:15-rgw:multisite-wip-fix-missing-http-data-test-distro-default-smithi/

@clwluvw
Copy link
Member

clwluvw commented Nov 27, 2024

jenkins test make check arm64

This was referenced Dec 3, 2024
Merged
Merged
@cbodley cbodley mentioned this pull request Dec 9, 2024
Merged
@cbodley
Copy link
Contributor

cbodley commented Dec 9, 2024

@shilpa are you going to approve this? It is in use downstream.

@mattbenjamin wasn't happy with the test I added. but i guess it'll do.

scheduled a run https://pulpito.ceph.com/smanjara-2024-11-19_20:26:15-rgw:multisite-wip-fix-missing-http-data-test-distro-default-smithi/

2024-11-19T21:15:47.660 INFO:tasks.rgw_multisite_tests:rgw_multi.tests.test_bucket_location_constraint ... ERROR
2024-11-19T21:15:47.660 INFO:tasks.rgw_multisite_tests:
2024-11-19T21:15:47.660 INFO:tasks.rgw_multisite_tests:======================================================================
2024-11-19T21:15:47.660 INFO:tasks.rgw_multisite_tests:ERROR: rgw_multi.tests.test_bucket_location_constraint
2024-11-19T21:15:47.660 INFO:tasks.rgw_multisite_tests:----------------------------------------------------------------------
2024-11-19T21:15:47.660 INFO:tasks.rgw_multisite_tests:Traceback (most recent call last):
2024-11-19T21:15:47.660 INFO:tasks.rgw_multisite_tests:  File "/home/teuthworker/src/git.ceph.com_teuthology_029bda193b1d38b67b279eb5c1037caa8408be24/virtualenv/lib/python3.10/site-packages/nose/case.py", line 170, in runTest
2024-11-19T21:15:47.660 INFO:tasks.rgw_multisite_tests:    self.test(*self.arg)
2024-11-19T21:15:47.660 INFO:tasks.rgw_multisite_tests:  File "/home/teuthworker/src/git.ceph.com_ceph-c_48505f3628a7c7a5f9e063923577af58ccacb8ce/qa/../src/test/rgw/rgw_multi/tests.py", line 3646, in test_bucket_location_constraint
2024-11-19T21:15:47.661 INFO:tasks.rgw_multisite_tests:    bucket = secondary.create_bucket(gen_bucket_name(), location=zonegroup)
2024-11-19T21:15:47.661 INFO:tasks.rgw_multisite_tests:TypeError: RadosZone.Conn.create_bucket() got an unexpected keyword argument 'location'

@cbodley cbodley self-assigned this Dec 17, 2024
@cbodley
Copy link
Contributor

cbodley commented Dec 17, 2024

i opened https://tracker.ceph.com/issues/69281 for backports to reef and squid

@clwluvw
Copy link
Member

clwluvw commented Dec 17, 2024

i opened https://tracker.ceph.com/issues/69281 for backports to reef and squid

I guess reef is immune from this as this was a regression by #50599 that hasn't been backported to reef if I'm not mistaken.

@cbodley
Copy link
Contributor

cbodley commented Dec 17, 2024

I guess reef is immune from this as this was a regression by #50599 that hasn't been backported to reef if I'm not mistaken.

thanks, updated the tracker

@cbodley
Copy link
Contributor

cbodley commented Dec 17, 2024

https://jenkins.ceph.com/job/ceph-pull-requests/148473/

The following tests FAILED:
248 - unittest_rbd_mirror (SEGFAULT)

looks like https://tracker.ceph.com/issues/63798 or https://tracker.ceph.com/issues/53959

@cbodley
Copy link
Contributor

cbodley commented Dec 17, 2024

jenkins test make check

@cbodley
Copy link
Contributor

cbodley commented Dec 18, 2024

https://jenkins.ceph.com/job/ceph-pull-requests/148481/

The following tests FAILED:
31 - run-rbd-unit-tests-1.sh (Failed)

commented on https://tracker.ceph.com/issues/46875

@cbodley
Copy link
Contributor

cbodley commented Dec 18, 2024

jenkins test make check

@cbodley
Copy link
Contributor

cbodley commented Dec 18, 2024

had it working locally, but both multisite jobs failed in https://pulpito.ceph.com/cbodley-2024-12-17_21:39:06-rgw-wip-69281-distro-default-smithi/

2024-12-17T22:57:12.223 INFO:tasks.rgw_multisite_tests:======================================================================
2024-12-17T22:57:12.223 INFO:tasks.rgw_multisite_tests:ERROR: rgw_multi.tests.test_bucket_location_constraint
2024-12-17T22:57:12.223 INFO:tasks.rgw_multisite_tests:----------------------------------------------------------------------
2024-12-17T22:57:12.223 INFO:tasks.rgw_multisite_tests:Traceback (most recent call last):
2024-12-17T22:57:12.223 INFO:tasks.rgw_multisite_tests:  File "/home/teuthworker/src/git.ceph.com_teuthology_e4eeebda7d988dc20ee63a4f7e7d82e07061f6d5/virtualenv/lib/python3.10/site-packages/nose/case.py", line 170, in runTest
2024-12-17T22:57:12.223 INFO:tasks.rgw_multisite_tests:    self.test(*self.arg)
2024-12-17T22:57:12.223 INFO:tasks.rgw_multisite_tests:  File "/home/teuthworker/src/git.ceph.com_ceph-c_4c0384ac6b9ff4a86f471eb1b6cde95cc218d09a/qa/../src/test/rgw/rgw_multi/tests.py", line 3649, in test_bucket_location_constraint
2024-12-17T22:57:12.224 INFO:tasks.rgw_multisite_tests:    bucket_name = secondary.create_bucket(gen_bucket_name(), location=zonegroup.name)
2024-12-17T22:57:12.224 INFO:tasks.rgw_multisite_tests:  File "/home/teuthworker/src/git.ceph.com_ceph-c_4c0384ac6b9ff4a86f471eb1b6cde95cc218d09a/qa/tasks/rgw_multi/zone_rados.py", line 56, in create_bucket
2024-12-17T22:57:12.224 INFO:tasks.rgw_multisite_tests:    return self.conn.create_bucket(name, **kwargs)
2024-12-17T22:57:12.224 INFO:tasks.rgw_multisite_tests:  File "/home/teuthworker/src/git.ceph.com_teuthology_e4eeebda7d988dc20ee63a4f7e7d82e07061f6d5/virtualenv/lib/python3.10/site-packages/boto/s3/connection.py", line 627, in create_bucket
2024-12-17T22:57:12.224 INFO:tasks.rgw_multisite_tests:    raise self.provider.storage_response_error(
2024-12-17T22:57:12.224 INFO:tasks.rgw_multisite_tests:boto.exception.S3ResponseError: S3ResponseError: 400 Bad Request
2024-12-17T22:57:12.224 INFO:tasks.rgw_multisite_tests:<?xml version="1.0" encoding="UTF-8"?><Error><Code>InvalidLocationConstraint</Code><Message>The specified location-constraint is not valid</Message><BucketName>ynftuh-67</BucketName><RequestId>tx00000027c23143dd5a3fa-00676201c8-6211-a2</RequestId><HostId>6211-a2-a</HostId></Error>

@cbodley
Copy link
Contributor

cbodley commented Jan 8, 2025

@smanjara #52791 has a more detailed test case for bucket location. can you please remove the test changes from this PR so i can test/merge the two PRs together?

@smanjara smanjara force-pushed the wip-fix-missing-http-data branch from 74a2cb5 to ac75ecb Compare January 8, 2025 17:12
@smanjara smanjara force-pushed the wip-fix-missing-http-data branch 2 times, most recently from 6e4763d to b0e6036 Compare January 8, 2025 17:15
cbodley
cbodley reviewed Jan 8, 2025
View reviewed changes
rgw/multisite: the create_bucket forward request omits the
43a6f12 
the request body, thus missing some data if specified inside
CreateBucketConfiguration xml on the non-master zone.
also, now that we perform cksum validation against empty payloads,
such a request would fail with -ERR_AMZ_CONTENT_SHA256_MISMATCH due
to a zero content-length but a non-empty payload hash.
this fix ensures that request body is forwarded during create_bucket

Signed-off-by: Shilpa Jagannath <[email protected]>
@smanjara smanjara force-pushed the wip-fix-missing-http-data branch from b0e6036 to 43a6f12 Compare January 8, 2025 17:19
@cbodley
Copy link
Contributor

cbodley commented Jan 8, 2025

jenkins test api

@cbodley
Copy link
Contributor

cbodley commented Jan 9, 2025

passed qa in https://pulpito.ceph.com/cbodley-2025-01-08_19:44:28-rgw-wip-69281-distro-default-smithi/ (batched with #52791)

@cbodley cbodley merged commit 65271d8 into ceph:main Jan 9, 2025
4 checks passed
@clwluvw clwluvw mentioned this pull request Mar 20, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cbodley cbodley cbodley approved these changes

@clwluvw clwluvw clwluvw left review comments

Assignees

@cbodley cbodley

Labels

rgw tests wip-cbodley-testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@smanjara @cbodley @clwluvw @mattbenjamin