-
Notifications
You must be signed in to change notification settings - Fork 832
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
oci-proxy: Add AWS S3 bucket for registry-sandbox.k8s.io #3671
oci-proxy: Add AWS S3 bucket for registry-sandbox.k8s.io #3671
Conversation
@endocrimes @sftim still WIP but looking for early review. 🙇🏾 |
a86fcfa
to
2b5ff55
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I added some feedback, mainly to suggest not using ACLs for the logs bucket.
resource "aws_s3_bucket_acl" "access_log" { | ||
provider = aws.origin | ||
|
||
bucket = aws_s3_bucket.access_log.id | ||
acl = "log-delivery-write" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than using this ACL, I recommend using a bucket policy combined with a bucket that is set to bucket owner enforced—in Terraform, you'd use aws_s3_bucket_ownership_controls
, similar to what's done for the serving bucket.
Look under https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-ownership-migrating-acls-prerequisites.html#object-ownership-server-access-logs for a heading To migrate bucket ACL permissions for the S3 log delivery group to the logging service principal in a bucket policy and use a policy like that, with a suitable aws:SourceArn
condition. That'll let S3 logs write to the logs bucket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
# Objects are deleted after 90 days | ||
expiration { | ||
days = 90 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'd put the number of days into a local
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
*/ | ||
|
||
terraform { | ||
required_version = "~> 1.1.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
required_version = "~> 1.1.0" | |
required_version = "~> 1.1.2" |
Early patch releases in this series have a bug - details in https://github.com/hashicorp/terraform/releases/tag/v1.1.2
I would pin to a version that has that bug fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use tfswitch
locally. it will detect the version and automatically pick the latest version. I'm not sure we want to pick a specific version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do recommend this pin. We can't be sure that another contributor is also using the same tfswitch
setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
|
||
provider "aws" { | ||
profile = "default" | ||
region = "us-west-2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this region? It might be worth mentioning that in a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not specific reason. Since this is a sandbox environment supposed to be gone in the future I randomly picked one.
BTW:, if it's useful to have, we can generate events (EventBridge) on:
and, for example, push these to Slack. Or push a summary every 5 minutes. https://docs.aws.amazon.com/AmazonCloudWatch/latest/events/log-s3-data-events.html is an outline of how. |
@sftim working on it. Wanted to do it as a followup. (don't have enough bandwidth and the moment) |
Related to: - kubernetes#3620 Ensure a AWS S3 bucket exists so we can test ip based redirection of archeio. The bucket contains a copy for the images layers served by k8s.gcr.io The bucket: - is world readable - only allow HTTPS connections - only allow HTTP methods GET and HEAD - has versioning enabled Another private bucket is created for access logging. Signed-off-by: Arnaud Meukam <[email protected]>
2b5ff55
to
9e62b56
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ameukam, 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:
Approvers can indicate their approval by writing |
Related to:
Ensure a AWS S3 bucket exists so we can test IP based redirection of
archeio.
The bucket contains a copy of the images layers
served by k8s.gcr.io.
The bucket:
Another private bucket is created for access logging.
Signed-off-by: Arnaud Meukam [email protected]