eks-prow-build-cluster: fix root volume for node groups and setup IAM role with admin access #4989

xmudrii · 2023-03-20T15:17:57Z

This PR brings two improvements to the eks-prow-build-cluster:

Root volume is now properly defined, so Terraform is going to modify the default root volume instead of creating a new one
Create an IAM role with the administrator privileges
- The idea is that IAM users are supposed to assume that role instead of giving permissions to each IAM user
- That IAM role is also specified in the aws-auth ConfigMap
- That IAM role is also specified as service user for the KMS key so it can be used with Terraform
- Additionally, that IAM role is Administrator in AWS, so it can be used to run Terraform
- The only problem with this IAM role is upon creating the cluster for the first time. In that case, assume role must be commented in main.tf for the first Terraform run

/hold for discussing
/assign @ameukam @upodroid

infra/aws/terraform/prow-build-cluster/main.tf

infra/aws/terraform/prow-build-cluster/eks.tf

infra/aws/terraform/prow-build-cluster/terraform.tfvars

sftim · 2023-03-20T19:10:19Z

infra/aws/terraform/prow-build-cluster/eks.tf

+      "userarn"  = var.root_account_arn
+      "username" = "root"
+      "groups" = [
+        "system:masters"


Are we sure about this? I tend to not trust the root principal for any use.
Perhaps comment about why it's OK here.

This is to give access to the EKS options via the Web Console. We can later drop this if Web Console access is not needed, but at this stage, it can be useful. I'm not sure we can easily scope this down as I'm not sure what permissions are needed by the Web Console.

How about just granting read for now?

This is now using a ClusterRoleBinding/Group with cluster-admin access. We'll probably remove this after the POC phase, so I wouldn't spend too much time trying to scope down permissions.

+1 to merge and iterate

infra/aws/terraform/prow-build-cluster/eks.tf

sftim · 2023-03-20T19:11:42Z

infra/aws/terraform/prow-build-cluster/eks.tf

+# Give administrator access to the admin IAM role so it can be used with Terraform.
+resource "aws_iam_role_policy_attachment" "iam_policy_cluster_admin" {
+  role       = aws_iam_role.iam_cluster_admin.name
+  policy_arn = "arn:aws:iam::aws:policy/AdministratorAccess"


This is more access than is needed. Consider a permissions boundary or a more specific policy.

For example, this allows organizations:LeaveOrganization. A deployer should not need to be allowed to do so.

The problem here is how to determine what are needed permissions. This account should be able to deploy those Terraform configs, and it needs quite a lot of access, so going Administrator level made sense. If anyone has any idea how to properly collect needed permissions, it would be appreciated.

Try https://skildops.com/blog/generate-restricted-aws-iam-policy-via-cloudtrail as a starting point.

Let's do this in a follow up.

+1 to merge and iterate

infra/aws/terraform/prow-build-cluster/main.tf

infra/aws/terraform/prow-build-cluster/terraform.tfvars

infra/aws/terraform/prow-build-cluster/eks.tf

Signed-off-by: Marko Mudrinić <mudrinic.mare@gmail.com>

xmudrii · 2023-03-27T13:50:50Z

The latest changes are applied. Remaining action items are documented here: #4686 (comment)

Signed-off-by: Marko Mudrinić <mudrinic.mare@gmail.com>

dims · 2023-03-27T14:15:29Z

/approve
/lgtm
/hold please go ahead remove hold when ready to apply this change.

k8s-ci-robot · 2023-03-27T14:16:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, xmudrii

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:

~~infra/aws/OWNERS~~ [dims]

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

xmudrii · 2023-03-27T14:18:15Z

Changes are already applied.
/hold cancel

k8s-ci-robot assigned ameukam Mar 20, 2023

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 20, 2023

k8s-ci-robot assigned upodroid Mar 20, 2023

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/infra Infrastructure management, infrastructure design, code in infra/ area/infra/aws Issues or PRs related to Kubernetes AWS infrastructure labels Mar 20, 2023

k8s-ci-robot requested review from dims and thockin March 20, 2023 15:18

k8s-ci-robot added 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. labels Mar 20, 2023

pkprzekwas reviewed Mar 20, 2023

View reviewed changes

infra/aws/terraform/prow-build-cluster/main.tf Outdated Show resolved Hide resolved

pkprzekwas reviewed Mar 20, 2023

View reviewed changes

infra/aws/terraform/prow-build-cluster/eks.tf Outdated Show resolved Hide resolved

ameukam reviewed Mar 20, 2023

View reviewed changes

infra/aws/terraform/prow-build-cluster/terraform.tfvars Show resolved Hide resolved

sftim suggested changes Mar 20, 2023

View reviewed changes

k8s-ci-robot assigned sftim Mar 20, 2023

pkprzekwas mentioned this pull request Mar 24, 2023

eks-prow-build-cluster: create simple monitoring stack #5011

Merged

eks-prow-build-cluster: expand the root volume

45d901c

Signed-off-by: Marko Mudrinić <mudrinic.mare@gmail.com>

xmudrii force-pushed the eks-improvements branch from db443ed to 8199e5f Compare March 27, 2023 13:38

xmudrii mentioned this pull request Mar 27, 2023

AWS: create build clusters with EKS #4686

Closed

Create IAM role and delegate admin permissions to it

bfecd62

Signed-off-by: Marko Mudrinić <mudrinic.mare@gmail.com>

xmudrii force-pushed the eks-improvements branch from 8199e5f to bfecd62 Compare March 27, 2023 14:07

k8s-ci-robot assigned dims Mar 27, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 27, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 27, 2023

k8s-ci-robot merged commit d2ae54e into kubernetes:main Mar 27, 2023

k8s-ci-robot added this to the v1.28 milestone Mar 27, 2023

xmudrii deleted the eks-improvements branch March 27, 2023 14:25

pkprzekwas mentioned this pull request Mar 27, 2023

REQUEST: New membership for pkprzekwas kubernetes/org#4126

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eks-prow-build-cluster: fix root volume for node groups and setup IAM role with admin access #4989

eks-prow-build-cluster: fix root volume for node groups and setup IAM role with admin access #4989

xmudrii commented Mar 20, 2023

sftim Mar 20, 2023

xmudrii Mar 20, 2023

sftim Mar 21, 2023

xmudrii Mar 27, 2023

dims Mar 27, 2023

sftim Mar 20, 2023

xmudrii Mar 20, 2023

sftim Mar 21, 2023

xmudrii Mar 27, 2023

dims Mar 27, 2023

xmudrii commented Mar 27, 2023

dims commented Mar 27, 2023

k8s-ci-robot commented Mar 27, 2023

xmudrii commented Mar 27, 2023

eks-prow-build-cluster: fix root volume for node groups and setup IAM role with admin access #4989

eks-prow-build-cluster: fix root volume for node groups and setup IAM role with admin access #4989

Conversation

xmudrii commented Mar 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xmudrii commented Mar 27, 2023

dims commented Mar 27, 2023

k8s-ci-robot commented Mar 27, 2023

xmudrii commented Mar 27, 2023