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

feat: added dynamic bucket region session option #146

Merged
merged 1 commit into from
May 24, 2022

Conversation

pregnor
Copy link
Contributor

@pregnor pregnor commented Apr 26, 2021

Scope

Minor (Feature)

What

  1. Added an AWS session initialization option for dynamically retrieving the region of the S3 bucket which is used in the helm command.
  2. Updated the calls to the session constructor to use this option.
  3. Added unit test cases covering all paths of this option.

Why

Without the correct region being set currently the plugin throws an error:

fetch from s3 uri=s3://pregnor-helm-s3-repo-test/stable/cadence/cadence-0.18.0.tgz: fetch object from s3: BucketRegionError: incorrect region, the bucket is not in 'us-west-2' region at endpoint ''
        status code: 301, request id: , host id: 
Error: plugin "bin/helms3" exited with error

We wanted to allow the plugin to work without manual region setting and also support multiple repositories residing in different regions without updating the regions in-between fetching charts from those repositories. This is especially relevant when the plugin is used in a Terraform environment.

How

The HEAD {{bucket}} request allows querying the bucket's actual region without prior knowledge with the proper configuration.

Unfortunately this behavior is not explicitly documented - or at least I couldn't find it - but the AWS SDK Go team confirmed in an issue comment this is an intended and supported behavior of the endpoint.

If the option runs into any issue, it has no effect on the session/configuration as a fallback mechanism. In such a case the plugin's behavior remains the same as before, namely falling back to the HELM_S3_REGION environment variable's value or to the corresponding AWS environment variable's value (AWS_REGION or AWS_DEFAULT_REGION).

Notes

The buckets used in the unit tests were found randomly, they are not mine and I do not have access to them, but they fit the test cases perfectly. They can be replaced with different bucket names if preferred.

I ran into issues when I tried to run the end to end tests, the local variant didn't work with my macOS Big Sur machine and the non-local variant didn't like the AWS users I tried to use it with so I couldn't run those tests myself.
The added unit tests should prove the solution works as expected.
Also the option can be trivially extracted to a self-contained test to run outside the plugin for more advanced testing purposes.

I also tested manually by installing the development version of the plugin and fetching a chart from an S3 repository in a different region than the set one.

I ran these tests on my buckets which had a fine-grained permission policy blocking public request to reassure the solution works for those buckets as well.

@pregnor pregnor force-pushed the feat/dynamic-bucket-region branch 2 times, most recently from 0b47213 to 11e8186 Compare April 26, 2021 14:17
@pregnor pregnor marked this pull request as draft April 26, 2021 14:21
@pregnor pregnor force-pushed the feat/dynamic-bucket-region branch 2 times, most recently from 04dd07b to bd2e2cb Compare April 26, 2021 14:32
@codecov-commenter
Copy link

codecov-commenter commented Apr 26, 2021

Codecov Report

Merging #146 (c21ba4e) into master (e208d4e) will increase coverage by 4.20%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #146      +/-   ##
==========================================
+ Coverage   40.80%   45.01%   +4.20%     
==========================================
  Files          18       18              
  Lines         272      291      +19     
==========================================
+ Hits          111      131      +20     
+ Misses        151      150       -1     
  Partials       10       10              
Impacted Files Coverage Δ
internal/awsutil/session.go 94.59% <100.00%> (+11.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e208d4e...c21ba4e. Read the comment docs.

@pregnor pregnor marked this pull request as ready for review April 26, 2021 14:35
@pregnor pregnor force-pushed the feat/dynamic-bucket-region branch 3 times, most recently from 095137c to eda2008 Compare April 26, 2021 14:40
@pregnor pregnor marked this pull request as draft April 26, 2021 14:42
@pregnor pregnor force-pushed the feat/dynamic-bucket-region branch 2 times, most recently from 34056db to d9df9f3 Compare April 26, 2021 15:01
@pregnor pregnor marked this pull request as ready for review April 26, 2021 15:03
@pregnor
Copy link
Contributor Author

pregnor commented May 4, 2021

@hypnoglow please take a look at this when you can.
(I don't have access rights to set reviewers/assignees, so I'm commenting instead, don't mean to be pushy, just making sure the proper notification is done.)

Update: Couldn't wait any longer for the feature to be merged upstream, so I had to fit our fork for use, release a version there and use it instead.

@hypnoglow
Copy link
Owner

This is a great feature, thanks a lot!

@hypnoglow hypnoglow merged commit 8ed24f9 into hypnoglow:master May 24, 2022
@hypnoglow hypnoglow added this to the 0.11.0 milestone May 24, 2022
@pregnor pregnor deleted the feat/dynamic-bucket-region branch May 25, 2022 07:27
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.

None yet

3 participants