Add Terraform for registry-k8s-io s3 buckets #3605

BobyMCbobs · 2022-04-07T21:33:27Z

Adds

buckets for different regions (currently us-west-1, us-west-2)
an IAM user for read+write access

to be used for registry.k8s.io in AWS

BobyMCbobs · 2022-04-07T21:34:11Z

Currently having issues with making the buckets publicly accessible but other than that it appears to be working

terraform apply -var prefix=test-

/hold

BobyMCbobs · 2022-04-07T21:35:01Z

/cc @jaypipes @ameukam @dims @BenTheElder @hh

ameukam · 2022-04-08T02:37:14Z

/approve

Leave the LGTM to @dims or @jaypipes for the AWS features needed.

dims · 2022-04-08T12:41:29Z

/approve
/lgtm

/hold for @jaypipes to peek (feel free to remove hold in a day or so)

k8s-ci-robot · 2022-04-08T12:41:48Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [ameukam,dims]

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

BobyMCbobs · 2022-04-11T02:51:13Z

I managed to get there with the public bucket read 🎉

cc @jaypipes

This was commented out in case it might've been used. Removing since the "syncing" will just be pushing to all of the buckets.

jaypipes · 2022-04-11T14:07:17Z

infra/aws/terraform/registry.k8s.io/main.tf

+limitations under the License.
+*/
+
+variable "prefix" {


Is this to support things like a sandbox?

Yeah! So before or after it's provisioned, for testing the Terraform a prefix can be added to test in another account with different names for things.

jaypipes · 2022-04-11T14:18:24Z

infra/aws/terraform/registry.k8s.io/s3/bucket.tf

+  bucket = aws_s3_bucket.registry-k8s-io.bucket
+
+  rule {
+    object_ownership = "BucketOwnerEnforced"


jaypipes · 2022-04-11T14:23:25Z

infra/aws/terraform/registry.k8s.io/s3/bucket.tf

+          "s3:PutObject",
+          "s3:GetObject",
+          "s3:ListBucket",
+          "s3:DeleteObject"


Since the Resource here refers to just the bucket resource, I believe the only permission you need in this block is:

s3:ListBucket

since all of the other permissions are only applicable to the Object resources referenced by the Resource string "${aws_s3_bucket.registry-k8s-io.arn}/*"

The intent of this aws_iam_user_policy is to allow the IAM user created in the above folder's accounts.tf to r/w the bucket for CI related things. The IAM user would be the one that is used with the promotion tools.

Should be permissions still be reduced here?

@BobyMCbobs The IAM user that is writing objects to these buckets needs these permissions:

On the bucket resource (i.e. "${aws_s3_bucket.registry-k8s-io.arn}/":

s3:ListBucket

On the object resources within that bucket (i.e. "${aws_s3_bucket.registry-k8s-io.arn}/*":

s3:PutObject

s3:GetObject

s3:DeleteObject

Hope that makes sense! :)

Thank you @jaypipes, I've updated the permissions in 3a99bd5

jaypipes · 2022-04-11T14:24:03Z

infra/aws/terraform/registry.k8s.io/s3/bucket.tf

+        "Action" : [
+          "s3:PutObject",
+          "s3:GetObject",
+          "s3:ListBucket",


And here, I don't believe you need to have s3:ListBucket since that is applicable to the bucket, not the objects within it.

jaypipes · 2022-04-11T14:24:33Z

infra/aws/terraform/registry.k8s.io/s3/variables.tf

+  type = string
+}
+
+variable "prefix" {


Might be useful to have a comment here explaining the use of the prefix variable.

@jaypipes, this is a good idea. I'll add it

Added a comment in 6dc9f4c

BobyMCbobs · 2022-04-11T23:58:53Z

/unhold

riaankleinhans · 2022-04-12T00:56:37Z

/lgtm

sftim · 2022-04-19T09:07:58Z

infra/aws/terraform/registry.k8s.io/accounts.tf

+resource "aws_iam_user" "registry-k8s-io-access" {
+  name = "${var.prefix}registry-k8s-io-access"
+  path = "/"
+}


What's the purpose of this IAM user?

It's the principal that has the ability to push to the mirrored image layers we're storing in S3. I suppose we could have used an IAM Role instead of an IAM User, though.

BobyMCbobs added 9 commits April 4, 2022 16:44

Add Terraform for registry.k8s.io s3 buckets

ceca954

Fix typo

85f6147

Use region aliases

6e2f2a0

Add multi-regioned bucket creation

ad640ca

Fix bucket names

b306173

Add prefix and simplify creation

bf157c9

Comment out management central bucket

4c44425

Add regex for prefix

83f16ca

Update public-read attempt

7010088

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 Apr 7, 2022

k8s-ci-robot requested review from spiffxp and thockin April 7, 2022 21:33

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

k8s-ci-robot requested review from ameukam, BenTheElder, dims, hh and jaypipes April 7, 2022 21:35

Fix boilerplates for tf files

dd4c87e

This was referenced Apr 8, 2022

[Umbrella issue] Building AWS infrastructure for registry.k8s.io #3568

Closed

Create S3 buckets in AWS regions #3595

Closed

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 8, 2022

k8s-ci-robot assigned dims Apr 8, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 8, 2022

Add public bucket read

cee4223

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 11, 2022

Remove management bucket related code

d6f478b

This was commented out in case it might've been used. Removing since the "syncing" will just be pushing to all of the buckets.

jaypipes reviewed Apr 11, 2022

View reviewed changes

BobyMCbobs added 2 commits April 12, 2022 09:34

Add comment on prefix

6dc9f4c

Update s3 bucket permissions for iam user

3a99bd5

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

k8s-ci-robot assigned riaankleinhans Apr 12, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 12, 2022

k8s-ci-robot merged commit 0b85573 into kubernetes:main Apr 12, 2022

k8s-ci-robot added this to the v1.24 milestone Apr 12, 2022

ameukam mentioned this pull request Apr 13, 2022

Add AWS registry infra terraform #3403

Closed

sftim reviewed Apr 19, 2022

View reviewed changes

BobyMCbobs mentioned this pull request Apr 26, 2022

Umbrella issue: k8s.gcr.io => registry.k8s.io solution #1834

Closed

25 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Terraform for registry-k8s-io s3 buckets #3605

Add Terraform for registry-k8s-io s3 buckets #3605

BobyMCbobs commented Apr 7, 2022 •

edited

Loading

BobyMCbobs commented Apr 7, 2022

BobyMCbobs commented Apr 7, 2022

ameukam commented Apr 8, 2022

dims commented Apr 8, 2022

k8s-ci-robot commented Apr 8, 2022

BobyMCbobs commented Apr 11, 2022

jaypipes Apr 11, 2022

BobyMCbobs Apr 11, 2022

jaypipes Apr 11, 2022

jaypipes Apr 11, 2022

jaypipes Apr 11, 2022

BobyMCbobs Apr 11, 2022

jaypipes Apr 11, 2022

BobyMCbobs Apr 11, 2022

jaypipes Apr 11, 2022

jaypipes Apr 11, 2022

BobyMCbobs Apr 11, 2022

BobyMCbobs Apr 11, 2022

BobyMCbobs commented Apr 11, 2022

riaankleinhans commented Apr 12, 2022

sftim Apr 19, 2022

jaypipes Apr 19, 2022

Add Terraform for registry-k8s-io s3 buckets #3605

Add Terraform for registry-k8s-io s3 buckets #3605

Conversation

BobyMCbobs commented Apr 7, 2022 • edited Loading

BobyMCbobs commented Apr 7, 2022

BobyMCbobs commented Apr 7, 2022

ameukam commented Apr 8, 2022

dims commented Apr 8, 2022

k8s-ci-robot commented Apr 8, 2022

BobyMCbobs commented Apr 11, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BobyMCbobs commented Apr 11, 2022

riaankleinhans commented Apr 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BobyMCbobs commented Apr 7, 2022 •

edited

Loading