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

Add AWS registry infra terraform #3403

Closed
wants to merge 5 commits into from

Conversation

BobyMCbobs
Copy link
Member

@BobyMCbobs BobyMCbobs commented Feb 16, 2022

Adds Terraform to provision a bucket in s3 for kpromo tests

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/k8s-infra Categorizes an issue or PR as relevant to SIG K8s Infra. labels Feb 16, 2022
Copy link
Member

@puerco puerco left a comment

Choose a reason for hiding this comment

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

Thanks @BobyMCbobs, I'm humbled to get a piece of infra after me!

However, the idea of this bucket is to use it for the container image promoter unit tests. WDYT if we rename everything kpromo-* to make it more related to the actual project that will use it?

infra/aws/terraform/main.tf Outdated Show resolved Hide resolved
infra/aws/terraform/main.tf Outdated Show resolved Hide resolved
infra/aws/terraform/main.tf Outdated Show resolved Hide resolved
infra/aws/terraform/main.tf Outdated Show resolved Hide resolved
infra/aws/terraform/main.tf Outdated Show resolved Hide resolved
infra/aws/terraform/main.tf Outdated Show resolved Hide resolved
infra/aws/terraform/main.tf Outdated Show resolved Hide resolved
infra/aws/terraform/main.tf Outdated Show resolved Hide resolved
infra/aws/terraform/main.tf Outdated Show resolved Hide resolved
infra/aws/terraform/outputs.tf Outdated Show resolved Hide resolved
@BobyMCbobs
Copy link
Member Author

Thanks @BobyMCbobs, I'm humbled to get a piece of infra after me!
However, the idea of this bucket is to use it for the container image promoter unit tests. WDYT if we rename everything kpromo-* to make it more related to the actual project that will use it?

@puerco, I'm all for naming things for their purpose! kpromo FTW!
Updated in b0a1559

@ameukam
Copy link
Member

ameukam commented Feb 16, 2022

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ameukam, BobyMCbobs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 16, 2022
@@ -0,0 +1,21 @@
#+TITLE: Kubernetes AWS Infra Terrform
Copy link
Member

Choose a reason for hiding this comment

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

@BobyMCbobs Is it possible to use the Markdown format for all the docs ? Let's use a format well adopted by the community.

Copy link
Member Author

@BobyMCbobs BobyMCbobs Feb 16, 2022

Choose a reason for hiding this comment

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

Swapped out the format in e02cb71

@ameukam
Copy link
Member

ameukam commented Feb 16, 2022

We should also make sure release admins have access to this account :

- email-id: k8s-infra-release-admins@kubernetes.io

"s3:DeleteObject"
],
"Effect" : "Allow",
"Resource" : aws_s3_bucket.kpromo-test-1.arn
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs both the bucket and its sub resources to work.

[arn:aws:s3:::test, arn:aws:s3:::test/*]

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in 2dcf793

@ameukam
Copy link
Member

ameukam commented Feb 16, 2022

@BobyMCbobs -- Suggestions for this PR:

  • Create a new AWS account under the same AWS organisational as the account requested here using an email accessible by the SIG Release Chairs/TLs. (possibly sig-release-leads@kubernetes.io). I suggest to enable MFA for this account once it's done.
  • Create individual account and give permissions with the built-in policy arn:aws:iam::aws:policy/AdministratorAccess to every member of this list :
    - email-id: k8s-infra-release-admins@kubernetes.io
    name: k8s-infra-release-admins
    description: |-
    ACL for Release Engineering subproject owners (partial admin access to Release GCP projects)
    https://git.k8s.io/sig-release/release-managers.md
    settings:
    ReconcileMembers: "true"
    members:
    - adolfo.garcia@uservers.net
    - ctadeu@gmail.com
    - k8s@auggie.dev
    - saschagrunert@gmail.com
    .

@BobyMCbobs
Copy link
Member Author

@BobyMCbobs -- Suggestions for this PR:

  • Create a new AWS account under the same AWS organisational as the account requested here using an email accessible by the SIG Release Chairs/TLs. (possibly sig-release-leads@kubernetes.io). I suggest to enable MFA for this account once it's done.
  • Create individual account and give permissions with the built-in policy arn:aws:iam::aws:policy/AdministratorAccess to every member of this list :
    - email-id: k8s-infra-release-admins@kubernetes.io
    name: k8s-infra-release-admins
    description: |-
    ACL for Release Engineering subproject owners (partial admin access to Release GCP projects)
    https://git.k8s.io/sig-release/release-managers.md
    settings:
    ReconcileMembers: "true"
    members:
    - adolfo.garcia@uservers.net
    - ctadeu@gmail.com
    - k8s@auggie.dev
    - saschagrunert@gmail.com

    .

@ameukam, thank you for your suggestions.
I think the scope of regular AWS accounts is not well suited to this action, due to the accounts being isolated and not exactly nested.
Perhaps IAM users is better, so that they have access inside the k8s-infra-aws-root-account@kubernetes.io account.
From my explorations in AWS accounts and orgs:

  • AWS accounts are all at the same level
  • billing is required to create separate AWS accounts
  • adding AWS accounts to access other AWS accounts requires access to the k8s-infra-aws-root-account@kubernetes.io

That said in order to make IAM accounts, some Terraform will need to run against the k8s-infra-aws-root-account@kubernetes.io to provision inside it.
IAM users will mean for:

  • easier user management
  • everyone lives out of the same account
    • billing isn't a concern to the IAM users
  • no having to deal with the perils of real accounts
    • email invitations, etc..
    • no prompts for validating if you're a human
  • MFA is available too!

@ameukam
Copy link
Member

ameukam commented Feb 17, 2022

adding AWS accounts to access other AWS accounts requires access to the k8s-infra-aws-root-account@kubernetes.io

I don't think this is necessary. The account created using this e-mail is independent and the AWS accounts should be in the same OU.

some Terraform will need to run against the k8s-infra-aws-root-account@kubernetes.io to provision inside it.

You can setup an new AWS account using sig-release-leads@kubernetes.io and run the HCL code against it.

@hh
Copy link
Member

hh commented Feb 17, 2022

I'm concerned that we are creating completely isolated accounts that will be difficult to manage, audit, and replicate the way in which we work to other CNCF projects.

Might be good to get some AWS experts in, maybe we can get some input from @sftim? I’ve asked @eddiezane and hoping to get an AWS Solutions Architectect permanently assigned to the CNCF Cloud Credits / #sig-k8s-infra if possible.

I am reluctant to make this kind of irrevocable call with regards to account structure without wider input. So far everything is a sandbox, this is a move to permanent production.

@sftim Are you available for a sync with @ameukam or can you help weigh in here?

@sftim
Copy link
Contributor

sftim commented Feb 17, 2022

I'll follow this up on Slack to discuss availability.

@dims
Copy link
Member

dims commented Feb 18, 2022

@hh i would like us to time box this discussion. I am concerned about other CNCF projects for sure, but we can rework this stuff until we get this to production, so i am not worried as much. Note that other CNCF projects should not get in the way of us doing what we need to do right now to reduce our cost. please take that into consideration when making decisions. other projects are not in the same position as us.

@hh
Copy link
Member

hh commented Feb 18, 2022

Absolutely @dims! I'm meeting with @jaypipes first thing Monday to see if we can get an AWS Solutions Architect assigned to help with #sig-k8s-infra ongoing to help us with designs decisions like this.

To unblock and keep velocity, Caleb went ahead and created the account as requested.

image

Note that the OU is kubernetes-2022-q1, to denote both of the OU and accounts as temporary.

The pattern we seem to be initiating is a new account per sig, within the Kubernetes OU, alasig-X-leads@kubernetes.io. This would need to be initiated from the CNCF root account instead of a Kubernetes level account. I'm not sure this is what we want long term, and want to be sure we can back out and change direction later.

As it stands, when we create a new account, we lack a process and policy to setup some auditing and visibility for that account. Looking forward to seeing this evolve and move forward!

@ameukam
Copy link
Member

ameukam commented Apr 13, 2022

@BobyMCbobs @hh We should close this one. #3605 create the production buckets and the needed IAM role and user.

@BobyMCbobs BobyMCbobs changed the title Add AWS infra terraform Add AWS registry infra terraform Apr 19, 2022
@BobyMCbobs
Copy link
Member Author

Closing in favor of #3605.

Will reopen or open a new PR for account infra related stuff as discussed in the last sig-k8s-infra meeting

@BobyMCbobs
Copy link
Member Author

/close

@k8s-ci-robot
Copy link
Contributor

@BobyMCbobs: Closed this PR.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/k8s-infra Categorizes an issue or PR as relevant to SIG K8s Infra. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Development

Successfully merging this pull request may close these issues.

8 participants