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-06-02 #2094

Merged
merged 1 commit into from
Jun 2, 2021
Merged

Conversation

cncf-ci
Copy link
Contributor

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

Audit Updates wg-k8s-infra

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 25, 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 the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 25, 2021
@k8s-ci-robot k8s-ci-robot requested review from dims and spiffxp May 25, 2021 01:12
@k8s-ci-robot k8s-ci-robot added area/audit Audit of project resources, audit followup issues, code in audit/ wg/k8s-infra size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 25, 2021
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 25, 2021
@spiffxp
Copy link
Member

spiffxp commented May 25, 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 May 25, 2021
Comment on lines 3 to 4
"kind": "bigquery#dataset",
"id": "k8s-artifacts-prod:gcs_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'm confused. #2089 landed 13h ago. It updated the audit script to dump these files, and preemptively included them from a manual run.

This is behaving like either:

  • the audit script hasn't been updated
  • the service account running this doesn't have sufficient bigquery read access to see these resources

Going to check log files

Copy link
Member

@spiffxp spiffxp May 26, 2021

Choose a reason for hiding this comment

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

#2100 should have fixed, but it broke the script's ability to run in CI

#2104 should fix those problems

@cncf-ci cncf-ci changed the title audit: update as of 2021-05-25 audit: update as of 2021-05-27 May 27, 2021
@cncf-ci cncf-ci force-pushed the autoaudit-prow branch 2 times, most recently from 3a62717 to 01aa4ab Compare May 27, 2021 22:43
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 27, 2021
@cncf-ci cncf-ci changed the title audit: update as of 2021-05-27 audit: update as of 2021-05-28 May 28, 2021
@cncf-ci cncf-ci force-pushed the autoaudit-prow branch 2 times, most recently from 672776b to f06dd2e Compare May 28, 2021 10:45
@cncf-ci cncf-ci force-pushed the autoaudit-prow branch 2 times, most recently from 93a808e to ce96069 Compare May 29, 2021 22:43
@cncf-ci cncf-ci changed the title audit: update as of 2021-05-29 audit: update as of 2021-05-30 May 30, 2021
@cncf-ci cncf-ci force-pushed the autoaudit-prow branch 4 times, most recently from 26828ac to 7fa5973 Compare May 30, 2021 22:45
@cncf-ci cncf-ci changed the title audit: update as of 2021-05-30 audit: update as of 2021-05-31 May 31, 2021
@cncf-ci cncf-ci force-pushed the autoaudit-prow branch 4 times, most recently from 7370f01 to ead8d21 Compare May 31, 2021 22:41
@cncf-ci cncf-ci changed the title audit: update as of 2021-05-31 audit: update as of 2021-06-01 Jun 1, 2021
@cncf-ci cncf-ci force-pushed the autoaudit-prow branch 4 times, most recently from 589d07b to c8b79a3 Compare June 1, 2021 22:46
@cncf-ci cncf-ci changed the title audit: update as of 2021-06-01 audit: update as of 2021-06-02 Jun 2, 2021
@cncf-ci cncf-ci force-pushed the autoaudit-prow branch 2 times, most recently from ecf4e94 to b65bb19 Compare June 2, 2021 10:47
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.

#2120 - reviewed using the script here

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 2, 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 Jun 2, 2021
@k8s-ci-robot k8s-ci-robot merged commit b889e4f into kubernetes:main Jun 2, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone Jun 2, 2021
@spiffxp
Copy link
Member

spiffxp commented Jun 2, 2021

This was a huge pain to review, my browser kept hanging on the "files changed" page. I used the script from #2120 to categorize changes, see that PR for everything checked specifically. But I'll try to sum up the followup issues:

  • buckets should not have ACLs enabled (they should be using IAM at the bucket level instead)
    • the vast majority of this is buckets created by kube-up.sh -> need to find out if kube-up.sh requires per-object ACLs
    • the next largest pattern is GCR buckets that our scripts didn't pre-provision
    • the kubernetes_public_billing bucket has them but I don't think it needs them
  • non-prod buckets shouldn't have retention enabled
  • there are some k8s-staging buckets that do
  • storage class should always be STANDARD
    • kubernetes-public has bucket(s) that are not
  • audit logs should always be present
    • there are some projects that lack system_event or had it removed by this audit PR, unclear if this is a visibility issue, or the log is truly missing
  • logs created by e2e tests should not be present
    • I thought we had disabled logging for these, but the resources are showing up. Would rather not allowlist logs resources, because we want to catch unexpected new logs; OTOH this would be an annoying denylist to maintain.

The manual changes leftover after running that script that I was ok with:

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants