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.

Sign up for GitHub

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

Allow for s3 backend path style #53

Closed
wants to merge 6 commits into from
Closed

Allow for s3 backend path style #53

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
249d8d2
add .venv to .gitignore
cloutierMat Mar 19, 2024
f4a5750
Set default of `use_path_style` and legacy `force_path_style` to `true`
cloutierMat Mar 19, 2024
6ce1ff6
Merge branch 'main' into Allow-for-s3-backend-path_style
cloutierMat Mar 19, 2024
149b08d
Set use_path_style default to false and reuse use_s3_path_style to de…
cloutierMat Mar 22, 2024
95eab83
explanded test of use_s3_path_style to ensure behavior when endpoint …
cloutierMat Mar 22, 2024
954726c
added tests for path style in backend
cloutierMat Mar 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Custom
.envrc
.env
.venv
tmp/

.DS_Store
Expand Down
5 changes: 5 additions & 0 deletions bin/tflocal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ terraform {
bucket = "<bucket>"
key = "<key>"
dynamodb_table = "<dynamodb_table>"
use_path_style = "<use_path_style>"
Copy link
Contributor

Choose a reason for hiding this comment

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

This must be added dynamically, see my other comment below why.

Copy link
Author

Choose a reason for hiding this comment

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

To keep transparency and a similar style, I left it here, but changed the default to false.


access_key = "test"
secret_key = "test"
Expand Down Expand Up @@ -220,6 +221,7 @@ def generate_s3_backend_config() -> str:
"key": "terraform.tfstate",
"dynamodb_table": "tf-test-state",
"region": get_region(),
"use_path_style": "true",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure everyone would be happy if we pin this option here or force them to add it explicitly with false when they don't need it.
The best option probably if we introduce a similar logic like this instead:

terraform-local/bin/tflocal

Line 136 in 07643c4

if use_s3_path_style():

(Sidenote: users use tflocal in combination with real s3 backends too, so we must not break those setups either.)

Copy link
Author

Choose a reason for hiding this comment

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

Agreed, I will work on that!

Copy link
Author

Choose a reason for hiding this comment

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

I set the default to false and I am reusing the use_s3_path_style method so it should be easier for a user to use different combinations of backend/s3 target

"endpoints": {
"s3": get_service_endpoint("s3"),
"iam": get_service_endpoint("iam"),
Expand Down Expand Up @@ -247,6 +249,9 @@ def generate_s3_backend_config() -> str:
get_or_create_bucket(configs["bucket"])
get_or_create_ddb_table(configs["dynamodb_table"], region=configs["region"])
result = TF_S3_BACKEND_CONFIG
if is_tf_legacy:
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice backward compatibility proofing. I believe we should introduce the same logic for the override file too if we address this here. Fancy to add it somewhere here?

terraform-local/bin/tflocal

Line 136 in 07643c4

if use_s3_path_style():

Copy link
Author

Choose a reason for hiding this comment

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

I looked at that, and if I understand properly, you are suggesting to change the configuration of the aws provider from s3_use_path_style supported in provider >4 to s3_force_path_style for providers version <4.

It can be quite the complex problem to define which version of a provider will be used as all modules are looked at by terraform before deciding on the providers version.

Let me know if I am missing something 🤔

result = result.replace("use_path_style", "force_path_style")
configs["force_path_style"] = configs.pop("use_path_style")
for key, value in configs.items():
if isinstance(value, bool):
value = str(value).lower()
Expand Down
Loading