Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

bernokl
Copy link
Contributor

@bernokl bernokl commented Jun 23, 2021

I think this should also use ensure_service_account from lib_iam.sh, but I am unsure where to call it from

@k8s-ci-robot
Copy link
Contributor

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. area/access Define who has access to what via IAM bindings, role bindings, policy, etc. wg/k8s-infra labels Jun 23, 2021
@k8s-ci-robot k8s-ci-robot requested review from dims and thockin June 23, 2021 03:57
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 23, 2021
@@ -1278,6 +1277,7 @@ groups:
- riaan@ii.coop
- stephen@ii.coop
- zz@ii.coop
- asn-etl@k8s-infra-ii-sandbox.iam.gserviceaccount.com
Copy link
Member

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 ?

Copy link
Contributor Author

@bernokl bernokl Jun 28, 2021

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather we keep service accounts out of groups. I can't recall the specific example right now, but it doesn't always work. I removed all previous SA memberships in #2010, specifically 9ebc221

Bind the service account directly to what is needed

Copy link
Member

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.

@ameukam
Copy link
Member

ameukam commented Jun 28, 2021

I think this should also use ensure_service_account from lib_iam.sh, but I am unsure where to call it from

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

@bernokl bernokl force-pushed the log_viewer_service_account branch from e789d1b to 8fb21ed Compare July 13, 2021 18:58
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bernokl
To complete the pull request process, please assign spiffxp after the PR has been reviewed.
You can assign the PR to them by writing /assign @spiffxp in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added area/terraform Terraform modules, testing them, writing more of them, code in infra/gcp/clusters/ size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 13, 2021
@bernokl
Copy link
Contributor Author

bernokl commented Jul 13, 2021

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.
The logs this SA would currently consume will be in gs://k8s-artifacts-gcslogs/ ideally it pulls from k8s-infra-public-pii, but I think that is only staging data right now.

@spiffxp
Copy link
Member

spiffxp commented Jul 21, 2021

/test pull-k8sio-verify

@spiffxp
Copy link
Member

spiffxp commented Jul 21, 2021

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 21, 2021
Copy link
Member

@spiffxp spiffxp left a 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

Comment on lines +28 to +29
resource "google_service_account" "ii-logs-sa@k8s-infra-ii-sandbox.iam.gserviceaccount.com" {
account_id = "ii-logs-sa-id"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"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" {
Copy link
Member

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

Suggested change
resource "google_project_iam_binding" "k8s-infra-ii-sandbox" {
resource "google_project_iam_member" "k8s-infra-ii-sandbox" {

Copy link
Member

@spiffxp spiffxp left a 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")

@k8s-ci-robot
Copy link
Contributor

@bernokl: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Rerun command
pull-k8sio-terraform-prow-build-trusted 8fb21ed link /test pull-k8sio-terraform-prow-build-trusted
pull-k8sio-terraform-prow-build 8fb21ed link /test pull-k8sio-terraform-prow-build
pull-k8sio-terraform-kubernetes-public 8fb21ed link /test pull-k8sio-terraform-kubernetes-public

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.

@riaankleinhans
Copy link
Contributor

/close

@k8s-ci-robot
Copy link
Contributor

@Riaankl: Closed this PR.

In response to this:

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/access Define who has access to what via IAM bindings, role bindings, policy, etc. area/terraform Terraform modules, testing them, writing more of them, code in infra/gcp/clusters/ cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants