feat: added tf infra for AWS ami account #6517

richardcase · 2024-03-04T16:29:52Z

This adds AWS infra provisioning using terraform for the AWS account to be used to publish CAPA AMIs.

It adds the following:

IAM role with the permissions required for packer AMI publishing
OIDC provider for GitHub
Associates the role to the provider and limits the kubernetes-sigs/cluster-api-provider-aws

k8s-ci-robot · 2024-03-04T16:29:55Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

nrb · 2024-03-04T17:36:24Z

infra/aws/terraform/cncf-k8s-infra-aws-capa-ami/iam-capa-maintainers-users.tf

+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+


Is there an automated way to keep this file in sync with the team membership?

We could look at doing something for that in the future

Maybe using .tfvars?

salasberryfin

Only nitpicking on the README.

infra/aws/terraform/cncf-k8s-infra-aws-capa-ami/README.md

Danil-Grigorev · 2024-05-03T13:05:53Z

Thanks for addressing it @richardcase
/lgtm

bryantbiggs · 2024-05-22T12:06:28Z

infra/aws/terraform/cncf-k8s-infra-aws-capa-ami/iam-capa-maintainers-users.tf

+*/
+
+
+resource "aws_iam_user" "ankitasw" {


seeing IAM users like this in 2024 is a bit sad - is this because we do not have SSO setup?

bryantbiggs · 2024-05-22T12:07:36Z

infra/aws/terraform/cncf-k8s-infra-aws-capa-ami/providers.tf

+*/
+
+terraform {
+  backend "s3" {}


where is state stored, or how is the bucket identified to store the state?

richardcase · 2024-07-04T11:02:14Z

@bryantbiggs - thats for the review comments. I've been AFK for the last ~5 weeks on a break. I'll pick this up again and will get back to you asap on the comments.

richardcase · 2024-08-06T07:55:49Z

We could potentially use IAM Identity centre for the AMI account(819546954734) instead of using IAM users/groups directly. We have identity centre enabled for the management account (see here.

Off the top of my head this would mean roughly:

Creating a new IAM Identity centre group for CAPA maintainers
Adding IAM identity centre users for each maintainer (with there email address) and assign to the group
- Ideally ensuring that the OTP option is enabled on create user
- Enforce MFA
Assign a "Admin" permission for account 819546954734 to the CAPA maintainer group

seeing IAM users like this in 2024 is a bit sad - is this because we do not have SSO setup?

@bryantbiggs - do you think that using IAM Identity Centre would be better than whats is currently in this PR? It still requires us to create/list the users in TF but at least we are not relying on the password output and a human to send them to the respective maintainer.

bryantbiggs · 2024-08-06T11:55:59Z

yes, SSO would be better. no matter what, we'll have to map users somewhere at the end of the day - but with SSO, we avoid the use of long lived credentials and it will give us the ability to map users to multiple accounts (as needed)

cc @upodroid

richardcase · 2024-09-09T15:59:18Z

Done:

Update to use OIDC

Todo:

Update to use minimal permissions as per the recommendation fro @sftim . See https://blog.stefan-koch.name/2021/05/16/restricted-packer-aws-permissions

bryantbiggs · 2024-09-09T17:47:47Z

infra/aws/terraform/cncf-k8s-infra-aws-capa-ami/oidc.tf

@@ -0,0 +1,42 @@
+resource "aws_iam_openid_connect_provider" "github" {


FYI - we do have this https://github.com/terraform-aws-modules/terraform-aws-iam/tree/master/modules/iam-github-oidc-provider

Also, this is set ONCE per account - not sure if this would be the right place or if there is somewher where account level settings are created cc @ameukam

Great, i wasn't aware of these. Thanks @bryantbiggs . I will update to use the modules instead of the handcrafted versions.

This is account is purely for the CAPA amis, so seems reasonable to have it here.

Updated to use this module.

This looks technically fine but we need to double check with the Github admin team if we are allowed to do this.
(Kubernetes organizations are not the responsibilities of the Infra SIG)

Shall i ask in #github-management?

Feedback from #github-management is that we shouldn't use OIDC from GHA in this way. Prow with IRSA should be used.

I will update the PR accordingly.

infra/aws/terraform/cncf-k8s-infra-aws-capa-ami/oidc.tf

infra/aws/terraform/cncf-k8s-infra-aws-capa-ami/providers.tf

ameukam · 2024-09-10T07:12:16Z

infra/aws/terraform/cncf-k8s-infra-aws-capa-ami/hack/disable-block.sh

+for val in "${arr[@]}"; do
+    aws ec2 disable-image-block-public-access --region "$val"
+done


we could do this with a HCL code ? cc @bryantbiggs

https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/ec2_image_block_public_access

Don't have to be in this PR. Just want to be sure we can skip bash configuration

I'm asking the other CAPA maintainer whether we need to continue publishing the AMIs to 15 regions. And whether we can just publish to 1 instead. If we change to just one then we can remove this completly.

But if there was a way that this could be done in HCL then that would give us options.

richardcase · 2024-09-10T07:21:22Z

Thank you @ameukam and @bryantbiggs for the feedback, its very much appreciated.

upodroid · 2024-09-10T09:06:27Z

infra/aws/terraform/cncf-k8s-infra-aws-capa-ami/oidc.tf

+  source    = "terraform-aws-modules/iam/aws//modules/iam-github-oidc-role"
+
+  name = "gh-image-builder"
+  subjects = ["kubernetes-sigs/cluster-api-provider-aws:*"]


Is there a particular reason why we are using github actions instead of prow? We can configure prow jobs to assume AWS roles very easily

When we started this work in was advised to use TF. Do we use IRSA or pod identity with any Prow jobs at present?

Having a quick look i only see assume roles in a few jobs and these are "trusted" jobs.

We do, with kops and capz.

Excellent, i will hunt them out. Thanks @upodroid

@upodroid - any chance you could point me at one of the jobs? I'm not really seeing how this is done from Prow.

I can see this trusted job: https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes/sig-k8s-infra/trusted/sig-k8s-infra-registry.yaml#L32

@upodroid - any chance you could point me at one of the existing jobs that use assume roles?

richardcase · 2024-10-02T21:02:37Z

After chatting with @upodroid the assume_role block has been removed.

richardcase · 2024-10-03T19:38:23Z

@upodroid @ameukam - do you think there are any other changes required to this? I'd love to run this against the AMI account so that we can start publishing AMIs again.

upodroid

LGTM
Please apply this and cancel the hold when you are ready to merge this.
/hold

This adds AWS infra provisioning using terraform for the AWS account to be used to publish CAPA AMIs. Signed-off-by: Richard Case <[email protected]>

richardcase · 2024-10-04T10:34:28Z

Thanks @upodroid . This has now been run against the AMI account. I had to make a slight change to the bucket name as i created it in the wrong region. Would you be able to LGTM again?

k8s-ci-robot · 2024-10-04T12:20:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: richardcase, upodroid

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~~ [upodroid]

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

richardcase · 2024-10-04T12:46:48Z

/unhold

k8s-ci-robot requested review from dims and thockin March 4, 2024 16:30

nrb reviewed Mar 4, 2024

View reviewed changes

dlipovetsky mentioned this pull request Mar 15, 2024

🐛 Write sensitive cloud-init user-data into /etc/cloud/cloud.cfg.d kubernetes-sigs/cluster-api-provider-aws#4746

Open

5 tasks

richardcase mentioned this pull request Apr 30, 2024

[CAPA] Configure account for AMI publishing rancher/highlander#84

Closed

richardcase force-pushed the capa_ami_account_users branch from bba9e8a to a2789d4 Compare April 30, 2024 09:56

richardcase changed the title ~~[WIP] feat: added tf infra for AWS ami account~~ feat: added tf infra for AWS ami account Apr 30, 2024

richardcase marked this pull request as ready for review April 30, 2024 09:56

k8s-ci-robot added area/bash Bash scripts, testing them, writing less of them, code in infra/gcp/ and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Apr 30, 2024

richardcase force-pushed the capa_ami_account_users branch 3 times, most recently from d3086f6 to eedff35 Compare April 30, 2024 10:34

salasberryfin reviewed Apr 30, 2024

View reviewed changes

infra/aws/terraform/cncf-k8s-infra-aws-capa-ami/README.md Outdated Show resolved Hide resolved

richardcase force-pushed the capa_ami_account_users branch from eedff35 to 640607f Compare April 30, 2024 10:57

k8s-ci-robot assigned Danil-Grigorev May 3, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 3, 2024

bryantbiggs reviewed May 22, 2024

View reviewed changes

bryantbiggs reviewed Sep 9, 2024

View reviewed changes

infra/aws/terraform/cncf-k8s-infra-aws-capa-ami/oidc.tf Outdated Show resolved Hide resolved

richardcase force-pushed the capa_ami_account_users branch 2 times, most recently from a4c8147 to dc3bd39 Compare September 10, 2024 07:02

ameukam reviewed Sep 10, 2024

View reviewed changes

infra/aws/terraform/cncf-k8s-infra-aws-capa-ami/providers.tf Outdated Show resolved Hide resolved

ameukam reviewed Sep 10, 2024

View reviewed changes

upodroid reviewed Sep 10, 2024

View reviewed changes

nrb mentioned this pull request Oct 2, 2024

Some Pre-built Kubernetes AMIs are “disappearing” kubernetes-sigs/cluster-api-provider-aws#5131

Closed

upodroid mentioned this pull request Oct 2, 2024

Configure AWS SSO for aws-capa-ami account #7366

Merged

richardcase force-pushed the capa_ami_account_users branch from dc3bd39 to 2dd2e5c Compare October 2, 2024 20:51

upodroid approved these changes Oct 3, 2024

View reviewed changes

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

k8s-ci-robot assigned upodroid Oct 3, 2024

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 3, 2024

feat: added tf infra for AWS ami account

9fc1d94

This adds AWS infra provisioning using terraform for the AWS account to be used to publish CAPA AMIs. Signed-off-by: Richard Case <[email protected]>

richardcase force-pushed the capa_ami_account_users branch from 2dd2e5c to 9fc1d94 Compare October 4, 2024 10:33

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2024

upodroid approved these changes Oct 4, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2024

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

k8s-ci-robot merged commit 4ef41d1 into kubernetes:main Oct 4, 2024
3 checks passed

k8s-ci-robot added this to the v1.32 milestone Oct 4, 2024

		@@ -0,0 +1,42 @@
		resource "aws_iam_openid_connect_provider" "github" {

feat: added tf infra for AWS ami account #6517

feat: added tf infra for AWS ami account #6517

Conversation

richardcase commented Mar 4, 2024 • edited Loading

k8s-ci-robot commented Mar 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salasberryfin left a comment

Choose a reason for hiding this comment

Danil-Grigorev commented May 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardcase commented Jul 4, 2024

richardcase commented Aug 6, 2024 • edited Loading

bryantbiggs commented Aug 6, 2024 • edited Loading

richardcase commented Sep 9, 2024

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

richardcase commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardcase Sep 10, 2024 • edited Loading

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

richardcase commented Oct 2, 2024

richardcase commented Oct 3, 2024

upodroid left a comment

Choose a reason for hiding this comment

richardcase commented Oct 4, 2024 • edited Loading

k8s-ci-robot commented Oct 4, 2024

richardcase commented Oct 4, 2024

richardcase commented Mar 4, 2024 •

edited

Loading

richardcase commented Aug 6, 2024 •

edited

Loading

bryantbiggs commented Aug 6, 2024 •

edited

Loading

richardcase Sep 10, 2024 •

edited

Loading

richardcase commented Oct 4, 2024 •

edited

Loading