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

[terraform] Give AWSS3FullAccess to s3 IAM users #13317

Merged
merged 1 commit into from
Sep 27, 2022
Merged

Conversation

nandajavarma
Copy link
Contributor

@nandajavarma nandajavarma commented Sep 26, 2022

Description

This PR essentially reverts this one. Even though with AWS CLI the permission seems enough, mc cli that we use for preflights(probably for other ops in Gitpod, not sure) requires a lot more permissions to do S3 ops. This is made clean in their documentation here.

Related Issue(s)

Fixes #13017

How to test

You can run the pipeline using:

werft run github -j .werft/eks-installer-tests.yaml -a deps=external -a domain=tests.doptig.com

Release Notes

NONE

Documentation

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide

@nandajavarma nandajavarma requested a review from a team September 26, 2022 14:17
@github-actions github-actions bot added the team: delivery Issue belongs to the self-hosted team label Sep 26, 2022
@nandajavarma
Copy link
Contributor Author

/hold for preview test to pass https://werft.gitpod-dev.com/job/gitpod-custom-nvn-fix-s3-preflights.1

@mrzarquon
Copy link
Contributor

mrzarquon commented Sep 26, 2022

@nandajavarma since we are handing this out to customers as example reference architecture, I don't think we should be including extremely broad permissions that only need to exist for our CI jobs to work properly.

Double checking - is this the S3 preflight check done in our CI or in KOTS? In either case, it these are the minimum required permissions for our services to function, requiring them to be more permissive just because of how we want to validate them isn't a good idea.

@adrienthebo
Copy link
Contributor

@nandajavarma since we are handing this out to customers as example reference architecture, I don't think we should be including extremely broad permissions that only need to exist for our CI jobs to work properly.

Double checking - is this the S3 preflight check done in our CI or in KOTS? In either case, it these are the minimum required permissions for our services to function, requiring them to be more permissive just because of how we want to validate them isn't a good idea.

I believe that this is the preflight check in KOTS.

That said, I believe our reference architecture documentation uses a reduced permission set, will double check that.

@nandajavarma
Copy link
Contributor Author

Have created these follow up issues to fix the preflight check and reduce the permissions:

@adrienthebo @mrzarquon Do you think we can ship this now for the sake of the release and take up the follow-ups after?

@mrzarquon
Copy link
Contributor

@nandajavarma since the preflight check in the wild, I agree we shouldn't update it and rush it out. But we should change the permission profile to be what is linked in the minio docs instead of just enabling all access. s3:HeadBucket is likely the permission we need to add, as MC is polling that to see if it has permission / access to the provided bucket. While this is their provided example, can we test with just adding the headbucket permission to the limited scope I provided:

statement {
    actions   = ["s3:HeadBucket"]
    resources = "*"
    effect    = "Allow"
  }

And then if that doesn't work, fall back to their provided example:

statement {
    actions   = ["s3:*"]
    resources = [
                "${aws_s3_bucket.gitpod-storage[count.index].arn}/*",
                aws_s3_bucket.gitpod-storage[count.index].arn
    ]
    effect    = "Allow"
  }
statement {
    actions   = ["s3:HeadBucket"]
    resources = "*"
    effect    = "Allow"
  }

Copy link
Contributor

@adrienthebo adrienthebo left a comment

Choose a reason for hiding this comment

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

Since the Terraform module isn't shipped as part of the KOTS release and we're running on a schedule this week, let's merge this change, cut the release branch, and then clean up the IAM permissions right after the release. We take on minimal liability with this and merging this change keeps the release on track.

Edit: thinking about this for a moment longer, even if someone picks up the Terraform module with the widened permission set, they'll get the permission reduction after updating the Terraform module.

🚢

@adrienthebo
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-none size/M team: delivery Issue belongs to the self-hosted team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] Fix the S3 backend registry pre-flight check failure for eks automated test
4 participants