-
Notifications
You must be signed in to change notification settings - Fork 841
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 service account to k8s-infra-ii-coop #2262
Conversation
Hi @bernokl. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
groups/groups.yaml
Outdated
@@ -1278,6 +1277,7 @@ groups: | |||
- riaan@ii.coop | |||
- stephen@ii.coop | |||
- zz@ii.coop | |||
- asn-etl@k8s-infra-ii-sandbox.iam.gserviceaccount.com |
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.
@bernokl I'm not sure to understand why you need to add this SA to this google group. What is the particularity of this SA ?
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.
@ameukam it will be our reporting user that is intended to run the asn-ip-lookup job on prow. Is there a more appropriate way for a job runner to get permissions to the logs?
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.
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.
@bernokl My suggestion would be look at https://cloud.google.com/iam/docs/understanding-roles to identity what permissions you need for this SA and add the binding as a Terraform code in https://github.com/kubernetes/k8s.io/tree/main/infra/gcp/clusters/projects/k8s-infra-ii-sandbox.
Ultimately, every resource used to do PII analysis will be moved to https://github.com/kubernetes/k8s.io/tree/main/infra/gcp/clusters/projects/k8s-infra-public-pii. See : #1968.
You can directly create the Service Account with Terraform here : https://github.com/kubernetes/k8s.io/blob/main/infra/gcp/clusters/projects/k8s-infra-ii-sandbox/main.tf |
e789d1b
to
8fb21ed
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bernokl The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I changed the pr to create the SA and bind-role in permissions.tf, if needed I can move it to main.tf as suggested above. The project for the role will potentially need to be kubernetes.io but need some guidance. |
/test pull-k8sio-verify |
/ok-to-test |
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.
question and some naming nits
resource "google_service_account" "ii-logs-sa@k8s-infra-ii-sandbox.iam.gserviceaccount.com" { | ||
account_id = "ii-logs-sa-id" |
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.
resource "google_service_account" "ii-logs-sa@k8s-infra-ii-sandbox.iam.gserviceaccount.com" { | |
account_id = "ii-logs-sa-id" | |
resource "google_service_account" "ii_logs_sa" { | |
account_id = "ii-logs" |
resource "google_service_account" "ii-logs-sa@k8s-infra-ii-sandbox.iam.gserviceaccount.com" { | ||
account_id = "ii-logs-sa-id" | ||
display_name = "ii logs service account" | ||
description = "service-account-to-facilitate-ii-log-analysis" |
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.
description = "service-account-to-facilitate-ii-log-analysis" | |
description = "service account to facilitate log analysis by ii" |
project = google_project.project.id | ||
role = "roles/storage.objectViewer" | ||
members = [ | ||
"user:ii-logs-sa@k8s-infra-ii-sandbox.iam.gserviceaccount.com", |
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.
"user:ii-logs-sa@k8s-infra-ii-sandbox.iam.gserviceaccount.com", | |
"user:ii-logs@k8s-infra-ii-sandbox.iam.gserviceaccount.com", |
project = google_project.project.id | ||
} | ||
|
||
resource "google_project_iam_binding" "k8s-infra-ii-sandbox" { |
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.
From https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/google_project_iam
google_project_iam_binding
: Authoritative for a given role. Updates the IAM policy to grant a role to a list of members. Other roles within the IAM policy for the project are preserved.
Are you sure you want this behavior? It means nobody else gets roles/storage.objectViewer
in this project except the service account.
What you might want instead want is to say "this service account should get this role, regardless of who else has the role"
Which would be
resource "google_project_iam_binding" "k8s-infra-ii-sandbox" { | |
resource "google_project_iam_member" "k8s-infra-ii-sandbox" { |
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.
(earlier review was meant to be "request changes")
@bernokl: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
/close |
@Riaankl: Closed this PR. In response to this:
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. |
I think this should also use ensure_service_account from lib_iam.sh, but I am unsure where to call it from