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

fix(storage-scrubber): use default AWS authentication #8299

Merged
merged 4 commits into from
Jul 9, 2024
Merged

Conversation

skyzh
Copy link
Member

@skyzh skyzh commented Jul 5, 2024

Problem

part of https://github.com/neondatabase/cloud/issues/14024
close #7665

Things running in k8s container use this authentication: https://docs.aws.amazon.com/sdkref/latest/guide/feature-container-credentials.html while we did not configure the client to use it. This pull request simply uses the default s3 client credential chain for storage scrubber. It might break compatibility with minio.

Summary of changes

  • Use default AWS credential provider chain.
  • Improvements for s3 errors, we now have detailed errors and correct backtrace on last trial of the operation.

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.
  • Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
  • If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

  • Do not forget to reformat commit message to not include the above checklist

@skyzh skyzh force-pushed the skyzh/scrubber-aws branch from 5a33320 to ddebdb6 Compare July 5, 2024 20:40
Copy link

github-actions bot commented Jul 5, 2024

3042 tests run: 2927 passed, 0 failed, 115 skipped (full report)


Flaky tests (6)

Postgres 16

  • test_subscriber_restart: release
  • test_tenant_creation_fails: debug

Postgres 15

  • test_change_pageserver: debug
  • test_storage_controller_smoke: debug

Postgres 14

Code coverage* (full report)

  • functions: 32.6% (6938 of 21284 functions)
  • lines: 50.0% (54544 of 109038 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
500a397 at 2024-07-09T16:35:31.694Z :recycle:

@skyzh skyzh force-pushed the skyzh/scrubber-aws branch from ddebdb6 to 7412f37 Compare July 5, 2024 20:41
@arpad-m
Copy link
Member

arpad-m commented Jul 5, 2024

I like the documenting quality of this piece of code: we see the tried methods listed explicitly. That being said, Christian has suggested precisely this over in #7667.

I think it's okay to merge it though given that with #7547 we want to remove the scrubber-specific SDK setup entirely.

skyzh added 3 commits July 8, 2024 16:05
Signed-off-by: Alex Chi Z <chi@neon.tech>
Signed-off-by: Alex Chi Z <chi@neon.tech>
Signed-off-by: Alex Chi Z <chi@neon.tech>
@skyzh skyzh force-pushed the skyzh/scrubber-aws branch from b83dade to eb6bb90 Compare July 8, 2024 20:05
@skyzh skyzh marked this pull request as ready for review July 8, 2024 20:05
@skyzh skyzh requested review from problame and arpad-m July 8, 2024 20:05
Co-authored-by: Joonas Koivunen <joonas@neon.tech>
@skyzh skyzh enabled auto-merge (squash) July 9, 2024 15:23
@skyzh skyzh merged commit b1fe825 into main Jul 9, 2024
65 checks passed
@skyzh skyzh deleted the skyzh/scrubber-aws branch July 9, 2024 17:41
skyzh added a commit that referenced this pull request Jul 15, 2024
part of neondatabase/cloud#14024
close #7665

Things running in k8s container use this authentication:
https://docs.aws.amazon.com/sdkref/latest/guide/feature-container-credentials.html
while we did not configure the client to use it. This pull request
simply uses the default s3 client credential chain for storage scrubber.
It might break compatibility with minio.

## Summary of changes

* Use default AWS credential provider chain.
* Improvements for s3 errors, we now have detailed errors and correct
backtrace on last trial of the operation.

---------

Signed-off-by: Alex Chi Z <chi@neon.tech>
Co-authored-by: Joonas Koivunen <joonas@neon.tech>
arpad-m added a commit that referenced this pull request Jul 19, 2024
PR #8299 has switched the storage scrubber to use
`DefaultCredentialsChain`. Now we do this for `remote_storage`, as it
allows us to use `remote_storage` from inside kubernetes. Most of the
diff is due to `GenericRemoteStorage::from_config` becoming `async fn`.
problame pushed a commit that referenced this pull request Jul 22, 2024
PR #8299 has switched the storage scrubber to use
`DefaultCredentialsChain`. Now we do this for `remote_storage`, as it
allows us to use `remote_storage` from inside kubernetes. Most of the
diff is due to `GenericRemoteStorage::from_config` becoming `async fn`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remote_storage: update S3 SDK BehaviorVersion
4 participants