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

audit: update as of 2021-05-05 #2001

Merged
merged 1 commit into from
May 5, 2021
Merged

Conversation

cncf-ci
Copy link
Contributor

@cncf-ci cncf-ci commented May 5, 2021

Audit Updates wg-k8s-infra

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 5, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @cncf-ci. 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/audit Audit of project resources, audit followup issues, code in audit/ labels May 5, 2021
@k8s-ci-robot k8s-ci-robot added wg/k8s-infra size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 5, 2021
@cncf-ci cncf-ci force-pushed the autoaudit-prow branch 2 times, most recently from 5adeebf to 49c66c9 Compare May 5, 2021 13:02
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.

/approve
/lgtm
/ok-to-test
This is basically confirmation of all the things done by #1974, and the addition of a k8s-staging-test-infra project

},
{
"members": [
"group:k8s-infra-gcs-access-logs@kubernetes.io"
Copy link
Member

Choose a reason for hiding this comment

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

Expected, this was #1966 being deployed

Comment on lines +1 to +8
{
"displayName": "k8s-infra container image auditor",
"email": "k8s-infra-gcr-auditor@k8s-artifacts-prod.iam.gserviceaccount.com",
"name": "projects/k8s-artifacts-prod/serviceAccounts/k8s-infra-gcr-auditor@k8s-artifacts-prod.iam.gserviceaccount.com",
"oauth2ClientId": "113024649066440988760",
"projectId": "k8s-artifacts-prod",
"uniqueId": "113024649066440988760"
}
Copy link
Member

Choose a reason for hiding this comment

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

This account's existence and its bindings are expected, a result of #1998 being deployed

First part of #1730

@@ -9,6 +9,7 @@ cloudtrace.googleapis.com Cloud Trace API
compute.googleapis.com Compute Engine API
containeranalysis.googleapis.com Container Analysis API
containerregistry.googleapis.com Container Registry API
containerscanning.googleapis.com Container Scanning API
Copy link
Member

Choose a reason for hiding this comment

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

Yeah this popped back up. #1963 is the followup issue to remove this from our code

"role": "projects/k8s-infra-prow-build-trusted/roles/ServiceAccountLister"
"role": "organizations/758905017065/roles/iam.serviceAccountLister"
Copy link
Member

Choose a reason for hiding this comment

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

Expected (though surprising at the time it happened) this is the last part of #1737 being deployed

This happened while deploying #1952 (comment), so apparently I forget to run terraform apply for the appropriate clusters after merging #1737

"group:k8s-infra-prow-viewers@kubernetes.io"
"group:k8s-infra-cluster-admins@kubernetes.io"
Copy link
Member

Choose a reason for hiding this comment

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

This is a weird way of diffing it, but same expected change as above (this time for k8s-infra-prow-build instead of k8s-infra-prow-build-trusted)

],
"role": "roles/storage.admin"
},
{
"members": [
"group:k8s-infra-cluster-admins@kubernetes.io",
"projectEditor:kubernetes-public",
Copy link
Member

Choose a reason for hiding this comment

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

These are expected, this is #1974 being deployed, specifically the part that's dropping projectEditor bindings

@@ -116,6 +116,7 @@
{
"members": [
"group:gke-security-groups@kubernetes.io",
"serviceAccount:gke-nodes-aaa@kubernetes-public.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.

This is the result of running terraform apply for clusters/projects/kubernetes-public/aaa using the last commit of #1974. There was an authoritative google_iam_policy terraform resource that kept overwriting other non-authoritative add-iam-policy-binding equivalents in terraform or bash.

I opted to move the project level bindings for accounts/groups not managed by terraform over to ensure-main-project.sh, and re-ran terraform apply... these are all of the bindings that should be present

Comment on lines +149 to +152
"members": [
"serviceAccount:k8s-infra-gcp-auditor@kubernetes-public.iam.gserviceaccount.com"
],
"role": "roles/viewer"
Copy link
Member

Choose a reason for hiding this comment

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

Also expected from #1974

It's redundant, given a binding to custom org role audit.viewer at the org level, but ensure-main-project.sh as written needs to give the service account some kind of role at the project level, and it seemed best to avoid tangling with the org for bootstrapping purposes

@@ -1,5 +1,5 @@
{
"displayName": "k8s-infra dns updater",
"displayName": "k8s-infra-dns-updater",
Copy link
Member

Choose a reason for hiding this comment

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

Expected from #1974

The displayName seemed mostly unused, so to reduce the number of parameters to pass during provisioning, I opted to make it the same as name (the part before the @ in the service accounts e-mail address). More easily searchable within our source code.

@@ -2,8 +2,7 @@
"bindings": [
{
"members": [
"serviceAccount:k8s-infra-prow-build-trusted.svc.id.goog[test-pods/k8s-infra-gcp-auditor]",
"serviceAccount:kubernetes-public.svc.id.goog[test-pods/k8s-infra-gcp-auditor]"
Copy link
Member

Choose a reason for hiding this comment

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

Expected from #1974

This should have been removed ages ago, this was over a year ago when I was testing out a prow build cluster in the kubernetes-public project, and had noodled on getting k8s-infra-gcp-auditor sufficient privileges to run the audit scripts.

@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 May 5, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 5, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cncf-ci, spiffxp

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 5, 2021
@k8s-ci-robot k8s-ci-robot merged commit 2cfeed5 into kubernetes:main May 5, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/audit Audit of project resources, audit followup issues, code in audit/ cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants