Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v23.2.x] cloud_storage: correct list_object() request headers and parameters (manual backport) #18446

Conversation

WillemKauf
Copy link
Contributor

@WillemKauf WillemKauf commented May 13, 2024

Backport of PR #18193
Fixes #18410
JIRA Link: https://redpandadata.atlassian.net/browse/CORE-2921

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v24.1.x
  • v23.3.x
  • v23.2.x

Release Notes

  • none

Previously, the `max_keys` value was erroneously added to the header
of a `list_objects` request in the `s3_client`. This is, in fact,
a URI request parameter, not a request header.

See:
 * https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListObjects.html
 * https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListObjectsV2.html

If `max_keys` is specified in a call to `remote::list_objects()`,
the onus is now on the user to deal with a possibly truncated value
at the call site, likely in a while loop.

The idea is that the user will be able to check for this case using
`list_result.is_truncated`, and then pass `list_result.next_continuation_token`
to `remote::list_objects()` in future requests.

(cherry picked from commit 39c48f4)
This commit adds an optional `continuation_token` parameter to
`remote::list_objects()` in order to support future work around
re-factoring of the `list_objects()` remote API.

This allows users to handle results that are truncated at the call site
to `list_objects()`. The `remote::list_result` will have `is_truncated`
set to true, and `next_continuation_token` set appropriately. To allow use
of this `next_continuation_token` in future requests, we have to expose
it as a parameter in `list_objects()`.

(cherry picked from commit eae9f43)
In `ListObjectsV2` for the `s3_client`, `prefix`, `delimiter`,
`start_after`, and `continuation-token` are all URI parameters,
not request headers.

(cherry picked from commit 15e3ec6)
There are a few places in our cloud storage testing where
we previously expected things like `prefix` and `continuation-token`
in the headers of a `list_objects` request, despite the fact S3 expects
these as URI parameters.

This commit corrects those checks by query parameters.

(cherry picked from commit 745eb7c)
The `continuation-token` (called `marker` in ABS) was not being passed
into `abs_request_creator::make_list_blob_request()`, despite being
properly set in `remote::list_objects()`.

This seems like dangerous behavior, as the `while()` loop
will continue to issue `list_objects()` requests to the `abs_client`
sans `marker`.

The `continuation-token` (`marker`) is now being passed to the request
creator and respected in the `abs_client`.

Also fixes an existing bug where `max_results` should be `maxresults`
in the URI parameters.

Also renames parameters to reflect ABS API naming.

See:
  * https://learn.microsoft.com/en-us/rest/api/storageservices/list-blobs#uri-parameters
(cherry picked from commit e6e595c)
Fixes behavior with `max_keys` and `continuation_token` within
`list_objects_resp()`. Fixture tests that use the `s3_imposter`
for requests can now expect proper behavior around these parameters.

(cherry picked from commit 96961f7)
To avoid undefined behavior due to accessing uninitialized memory,
default construct `list_bucket_result::is_truncated` with `false`.

(cherry picked from commit 13ecfd3)
@WillemKauf
Copy link
Contributor Author

CI failure is #8217

@piyushredpanda piyushredpanda merged commit 4e619eb into redpanda-data:v23.2.x May 13, 2024
23 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants