Skip to content

Comments

<rgw> Ensure the ETag format is consistent with AWS S3 API#60477

Merged
cbodley merged 2 commits intoceph:mainfrom
VVoidV:lbr-ETAG-consistent-with-AWS
Apr 1, 2025
Merged

<rgw> Ensure the ETag format is consistent with AWS S3 API#60477
cbodley merged 2 commits intoceph:mainfrom
VVoidV:lbr-ETAG-consistent-with-AWS

Conversation

@VVoidV
Copy link
Contributor

@VVoidV VVoidV commented Oct 24, 2024 •
edited
Loading

https://tracker.ceph.com/issues/68712
AWS S3 API quotes ETag everywhere.
Most ETags returned by RGW are quoted, but some are still not. This can cause false positives during consistency checks when comparing ETags returned by different interfaces.

checked with following cmd.

$ rg -i "dump.*\"ETag\"" ceph
./src/rgw/rgw_rest_s3.cc
1863:        s->formatter->dump_format("ETag", "\"%s\"", iter->meta.etag.c_str());
1955:      s->formatter->dump_format("ETag", "\"%s\"", iter->meta.etag.c_str());
2030:        s->formatter->dump_format("ETag", "\"%s\"", iter->meta.etag.c_str());
2099:      s->formatter->dump_format("ETag", "\"%s\"", iter->meta.etag.c_str());
2785:      s->formatter->dump_format("ETag", "\"%s\"", etag.c_str());
3422:    s->formatter->dump_string("ETag", etag);
3725:      s->formatter->dump_format("ETag", "\"%s\"",etag.c_str());
4228:    s->formatter->dump_string("ETag", etag);
4298:      s->formatter->dump_format("ETag", "\"%s\"", part->get_etag().c_str());

./src/rgw/rgw_rest.cc
421:    return dump_header(s, "etag", etag);
423:    return dump_header_quoted(s, "ETag", etag);

./src/rgw/driver/rados/rgw_sync_module_es_rest.cc
347:      s->formatter->dump_format("ETag", "\"%s\"", e.meta.etag.c_str());

./src/rgw/rgw_admin.cc
8612:        handled = dump_string("etag", bl, formatter.get());

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

@VVoidV VVoidV requested a review from a team as a code owner October 24, 2024 14:49
@github-actions github-actions bot added the rgw label Oct 24, 2024
@VVoidV
Copy link
Contributor Author

VVoidV commented Oct 24, 2024

@cbodley Please take a look at this. It's similar to #47527

@VVoidV
Copy link
Contributor Author

VVoidV commented Oct 28, 2024

jenkins test make check

@VVoidV
Copy link
Contributor Author

VVoidV commented Oct 28, 2024

jenkins test api

@VVoidV
Copy link
Contributor Author

VVoidV commented Oct 29, 2024

@cbodley gentle ping ：）

@cbodley
Copy link
Contributor

cbodley commented Oct 30, 2024

@cbodley gentle ping ：）

thanks @VVoidV. it's hard for me to evaluate changes like this, where there's some risk of breaking clients that rely on the current format

what kind of testing would we need to validate this? cc @adamemerson @ivancich

@VVoidV
Copy link
Contributor Author

VVoidV commented Oct 31, 2024

thanks @VVoidV. it's hard for me to evaluate changes like this, where there's some risk of breaking clients that rely on the current format

what kind of testing would we need to validate this? cc @adamemerson @ivancich

@cbodley One of our apps is complaining about data inconsistency under certain loads. It uses ETag to detect whether the data has changed remotely. The ETag can come from HEAD, LIST, PUT, CompleteMultipartUpload, etc.

We use the AWS Go SDK, and the ETag is always referred to in the response entity. Now, we need to handle both quoted and unquoted versions.

If others have already dealt with this, they might encounter issues after we add the quotes.
But for newcomers, it will be helpful to have consistent ETag formatting.

@cbodley cbodley mentioned this pull request Oct 31, 2024
Merged
14 tasks
@github-actions
Copy link

github-actions bot commented Dec 30, 2024

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@github-actions github-actions bot added the stale label Dec 30, 2024
@cbodley cbodley removed the stale label Jan 6, 2025
This was referenced Jan 21, 2025
Merged
Open
@github-actions
Copy link

github-actions bot commented Mar 7, 2025

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@github-actions github-actions bot added the stale label Mar 7, 2025
@cbodley cbodley requested review from adamemerson and ivancich March 26, 2025 14:02
@cbodley cbodley removed the stale label Mar 26, 2025
@cbodley cbodley force-pushed the lbr-ETAG-consistent-with-AWS branch from 2c553e1 to 1a31f7d Compare March 26, 2025 16:23
@ceph-jenkins
Copy link
Collaborator

ceph-jenkins commented Mar 26, 2025

Can one of the admins verify this patch?

@cbodley cbodley force-pushed the lbr-ETAG-consistent-with-AWS branch from 1a31f7d to 44b95c1 Compare March 26, 2025 16:25
cbodley
cbodley approved these changes Mar 26, 2025
View reviewed changes
Copy link
Contributor

@cbodley cbodley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rebased and added a release note

@github-actions
Copy link

github-actions bot commented Mar 31, 2025

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@cbodley
Copy link
Contributor

cbodley commented Mar 31, 2025

passed qa in https://pulpito.ceph.com/cbodley-2025-03-28_17:53:04-rgw-wip-cbodley-testing-distro-default-smithi/

@VVoidV @cbodley
<rgw> Ensure the ETag format is consistent with AWS S3 API
b879529 
AWS S3 API quotes ETAG everywhere. We still missed a few places.

checked with following cmd.
```
$ rg -i "dump.*\"ETag\"" ceph
./src/rgw/rgw_rest_s3.cc
1863:        s->formatter->dump_format("ETag", "\"%s\"", iter->meta.etag.c_str());
1955:      s->formatter->dump_format("ETag", "\"%s\"", iter->meta.etag.c_str());
2030:        s->formatter->dump_format("ETag", "\"%s\"", iter->meta.etag.c_str());
2099:      s->formatter->dump_format("ETag", "\"%s\"", iter->meta.etag.c_str());
2785:      s->formatter->dump_format("ETag", "\"%s\"", etag.c_str());
3422:    s->formatter->dump_string("ETag", etag);
3725:      s->formatter->dump_format("ETag", "\"%s\"",etag.c_str());
4228:    s->formatter->dump_string("ETag", etag);
4298:      s->formatter->dump_format("ETag", "\"%s\"", part->get_etag().c_str());

./src/rgw/rgw_rest.cc
421:    return dump_header(s, "etag", etag);
423:    return dump_header_quoted(s, "ETag", etag);

./src/rgw/driver/rados/rgw_sync_module_es_rest.cc
347:      s->formatter->dump_format("ETag", "\"%s\"", e.meta.etag.c_str());

./src/rgw/rgw_admin.cc
8612:        handled = dump_string("etag", bl, formatter.get());
```

Signed-off-by: liubingrun <[email protected]>
@cbodley cbodley force-pushed the lbr-ETAG-consistent-with-AWS branch from 44b95c1 to b1c7958 Compare March 31, 2025 20:26
@cbodley
Copy link
Contributor

cbodley commented Mar 31, 2025

i pushed a rebase to resolve a minor conflict with #61878

@cbodley cbodley merged commit 5749b63 into ceph:main Apr 1, 2025
12 checks passed
@cbodley
Copy link
Contributor

cbodley commented Apr 1, 2025

thanks @VVoidV, sorry for the delay

This was referenced Apr 1, 2025
Merged
Merged
@cbodley cbodley mentioned this pull request Apr 9, 2025
Closed
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

@adamemerson adamemerson Awaiting requested review from adamemerson

@ivancich ivancich Awaiting requested review from ivancich

Assignees

No one assigned

Labels

documentation ready-to-merge rgw TESTED

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@VVoidV @cbodley @ceph-jenkins