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

Use head_bucket vs list_buckets to determine if s3 bucket exists #357

Merged
merged 3 commits into from
Aug 27, 2021

Conversation

swindmill
Copy link
Contributor

SUMMARY

Some S3 endpoints, notably FIPS ones, only support Virtual Hosted-Style addressing which means operations like ListBuckets / Boto3's list_buckets aren't available.

head_bucket is a suitable alternative in this case, and also performs better than list_buckets, especially when many S3 buckets are present

I'm not sure which inconsistencies the developer(s) experienced that led to the comment at https://github.com/ansible-collections/amazon.aws/blob/main/plugins/modules/s3_bucket.py#L448

I didn't run into any issues when running this code modified to use list_buckets and it correctly works with both FIPS and non-FIPS S3 endpoints.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

amazon.aws.s3_bucket

@jillr jillr self-assigned this May 14, 2021
@jillr
Copy link
Collaborator

jillr commented May 14, 2021

Looks like that comment was added in the boto3 refactor of this module (ansible/ansible#37189 (comment)). I'm running a loop of the s3_bucket tests with this patch from my local environment to see if any intermittent failures are still reproducible and will report back.

@jillr
Copy link
Collaborator

jillr commented May 17, 2021

I still see occasional test failures but they all appear to be eventual consistency failures (ie; idempotency checks that are not idempotent - this assertion failed a few times https://github.com/ansible-collections/amazon.aws/blob/main/tests/integration/targets/s3_bucket/roles/s3_bucket/tasks/encryption_sse.yml#L59:L69). They are also far less frequent that was reported in the original PR (averaging 3-4 failures per 50 test runs). It's hard to say if something changed in the sdk, on S3, or something else but this appears to be a safe change, IMO. The code lgtm.

@swindmill can you please add a changelog fragment to changelogs/fragments/ about the change (ie; the module now supports FIPS endpoints)? The changelog format is documented here: https://github.com/ansible-community/antsibull-changelog/blob/main/docs/changelogs.rst#examples

@ansibullbot
Copy link

@ansibullbot ansibullbot added bug This issue/PR relates to a bug community_review module module new_contributor Help guide this first time contributor plugins plugin (any type) labels May 21, 2021
@jillr
Copy link
Collaborator

jillr commented Jul 7, 2021

Hi @swindmill. It would be great to get this bugfix merged, would you be able to add a changelog?

@tremble
Copy link
Contributor

tremble commented Aug 27, 2021

Just in case someone comes looking: https://docs.aws.amazon.com/en_us/AmazonS3/latest/API/API_HeadBucket.html

HeadBucket requires the ListBuckets permission (the same IAM policy action needed by the original code)

Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

The change from List to Head LGTM, I've added a changelog fragment and moved the exception handling over to using is_boto3_error_code for consistency with the rest of the code.

@tremble tremble requested review from jillr and alinabuzachis August 27, 2021 11:01
@tremble tremble added the gate label Aug 27, 2021
Copy link
Contributor

@ansible-zuul ansible-zuul bot left a comment

Choose a reason for hiding this comment

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

LGTM!

@ansible-zuul ansible-zuul bot merged commit 31dacbd into ansible-collections:main Aug 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug community_review module module new_contributor Help guide this first time contributor plugins plugin (any type)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants