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: add user_policy_document parameter #142

Merged
merged 5 commits into from
Nov 3, 2023

Conversation

dod38fr
Copy link
Contributor

@dod38fr dod38fr commented Jun 1, 2023

what

This parameter allows the user to specify policies that are applied to the S3 bucket with the policies defined by this module.

why

We want to add policies that forbid non admin users to access the bucket containing tfstates.

This commit allow us to specify a policy that implement these restriction without clobbering the policies put in place by this module.

Note that I have no problem to change the name of this new parameter if you want another.

references

Closes: #115

@dod38fr dod38fr requested review from a team as code owners June 1, 2023 13:19
@dod38fr dod38fr requested review from jamengual and woz5999 June 1, 2023 13:19
This parameter allows the user to specify policies that are applied to
the S3 bucket along the policies defined by this module.

Closes: cloudposse#115
@dod38fr dod38fr force-pushed the feat/user_policy_document branch from 3f01190 to 93a90d6 Compare June 8, 2023 13:29
@dod38fr
Copy link
Contributor Author

dod38fr commented Jun 19, 2023

Hi
Any news ?

@dod38fr
Copy link
Contributor Author

dod38fr commented Jul 21, 2023

Hi

Any news about this PR ? Is there a problem preventing a review ?

All the best

@osterman
Copy link
Member

Best way to get attention is in #pr-reviews channel in slack

@aknysh
Copy link
Member

aknysh commented Aug 24, 2023

@dod38fr thank you for the PR, it looks good

please run the following commands and commit the changes:

make init
make github/init
make readme

Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

please see comments

Copy link
Contributor

@Nuru Nuru left a comment

Choose a reason for hiding this comment

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

Sorry for the delay reviewing this. We were hoping to make these changes and more by now as part of a overhaul, but obviously we have not gotten to it yet.

Please make the requested changes so that this feature works just like source_policy_documents in terraform-aws-s3-bucket.

variables.tf Outdated Show resolved Hide resolved
main.tf Outdated
Comment on lines 59 to 60
source_policy_documents = var.user_policy_documents

Copy link
Contributor

Choose a reason for hiding this comment

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

Move to separate policy document which overrides the default bucket policy. See s3-bucket implementation for example:

https://github.com/cloudposse/terraform-aws-s3-bucket/blob/d7a49439ea48dfd91c06830fb5d9419329bd927d/main.tf#L454-L466

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Unfortunately, I won't be able to test this change on my client's system before October.

Choose a reason for hiding this comment

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

I can confirm that the change works as expected on said client's system where we are running the changed version in this PR, thanks @dod38fr!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All set then, @Nuru , can you resume the review ?

All the best

@dod38fr
Copy link
Contributor Author

dod38fr commented Aug 26, 2023

please run the following commands and commit the changes:

make init
make github/init
make readme

Done.

All the best

@dod38fr dod38fr requested a review from Nuru August 26, 2023 16:23
@Nuru
Copy link
Contributor

Nuru commented Sep 25, 2023

/terratest

Copy link
Contributor

@Nuru Nuru left a comment

Choose a reason for hiding this comment

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

This is failing the Terratest. I'm not sure why, something to do with order of operations or waiting. You can compare with our s3-bucket module. It might be a while before I get to look into this further.

@dod38fr
Copy link
Contributor Author

dod38fr commented Oct 2, 2023

@Nuru The terratest job (https://github.com/cloudposse/terraform-aws-tfstate-backend/actions/runs/6303986183) is now marked as successful.

I don't really know what went on. I guess that he failure you've seen was a temporary failure and that the job was restarted.

In any case, I cannot investigate further as I cannot see any error message.

Is there something more I can do ?

@dod38fr
Copy link
Contributor Author

dod38fr commented Oct 18, 2023

Hi

I've successfully run the test that failed in your CI:

$ make -C test/src/
make: Entering directory '/home/domi/terraform-stuff/terraform-aws-tfstate-backend/test/src'
go mod download
go test -v -timeout 60m
=== RUN   TestExamplesComplete
=== PAUSE TestExamplesComplete
=== RUN   TestExamplesCompleteDisabled
=== PAUSE TestExamplesCompleteDisabled
=== CONT  TestExamplesComplete
=== CONT  TestExamplesCompleteDisabled
TestExamplesComplete 2023-10-18T17:53:30+02:00 test_structure.go:130: Copied terraform folder ../../examples/complete to /tmp/TestExamplesComplete578001989/terraform-aws-tfstate-backend/examples/complete
TestExamplesComplete 2023-10-18T17:53:30+02:00 retry.go:91: terraform [init -upgrade=true]
TestExamplesComplete 2023-10-18T17:53:30+02:00 logger.go:66: Running command terraform with args [init -upgrade=true]

[ snip]

TestExamplesComplete 2023-10-18T17:59:00+02:00 logger.go:66: module.tfstate_backend.aws_s3_bucket.default[0]: Still destroying... [id=eg-use2-test-terraform-tfstate-backend-rxcfw2, 4m40s elapsed]
TestExamplesComplete 2023-10-18T17:59:10+02:00 logger.go:66: module.tfstate_backend.aws_s3_bucket.default[0]: Still destroying... [id=eg-use2-test-terraform-tfstate-backend-rxcfw2, 4m50s elapsed]
TestExamplesComplete 2023-10-18T17:59:20+02:00 logger.go:66: module.tfstate_backend.aws_s3_bucket.default[0]: Still destroying... [id=eg-use2-test-terraform-tfstate-backend-rxcfw2, 5m0s elapsed]
TestExamplesComplete 2023-10-18T17:59:21+02:00 logger.go:66: module.tfstate_backend.aws_s3_bucket.default[0]: Destruction complete after 5m2s
TestExamplesComplete 2023-10-18T17:59:21+02:00 logger.go:66: 
TestExamplesComplete 2023-10-18T17:59:21+02:00 logger.go:66: Destroy complete! Resources: 9 destroyed.
TestExamplesComplete 2023-10-18T17:59:21+02:00 logger.go:66: 
--- PASS: TestExamplesComplete (351.46s)
PASS
ok      github.com/cloudposse/terraform-aws-tfstate-backend     351.483s
make: Leaving directory '/home/domi/terraform-stuff/terraform-aws-tfstate-backend/test/src'

I really don't know what can be done to unblock this PR.

Any idea ?

@Nuru
Copy link
Contributor

Nuru commented Nov 3, 2023

/terratest

@Nuru Nuru self-requested a review November 3, 2023 22:17
@Nuru Nuru merged commit ac43e7b into cloudposse:main Nov 3, 2023
8 checks passed
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.

Allow making changes to the s3 bucket policy
5 participants