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

Manage organization roles #1726

Merged
merged 13 commits into from
Mar 1, 2021
Merged

Conversation

spiffxp
Copy link
Member

@spiffxp spiffxp commented Feb 26, 2021

This addresses most of #1659

To do so I added a script to generate roles out of other roles/permissions (#1656).

A role spec looks like the following:

# role name, e.g.
name: "foo.bar"
# human readable title for the role, e.g.
title: "Foo Barrer"
# human readable description for the role, e.g. 
description: "Allows doing Bar on Foo resources"
# include permissions from the following...
include:
  # a list of permissions, e.g.
  permissions:
  - foo.bar.doSomething
  # a list of roles, e.g.
  roles:
  - roles/foo.bar
  # only include permissions matching any extended regex in this list, e.g.
  permissionRegxes:
  - ^foo.bar.(get|list)
exclude:
  # exclude any permissions matching any extended regex in this list, e.g.
  permissionRegexes:
  - SuperDangerousOperation$

Sadly, I did this with bash and yq. Redoing this with go would be a great help-wanted.

Updates the following managed org roles:

  • prow.viewer - Auditing roles/viewer from k8s-infra-e2e-prow-viewers@ made me realize they don't have view access to buckets in most e2e projects. Now they will.

Adds the following previously unmanaged org roles:

  • CustomRole - not a good name, but held off on migrating to better name until I get some historical context
  • StorageBucketLister - this can go away once audit.viewer is live

Adds the following new managed org roles:

  • audit.viewer - replace the sundry bindings used by k8s-infra-gcp-auditors@
  • organization.admin - replace the sundry non-roles/owner bindings used by k8s-infra-gcp-org-admins@
    • not running the script to deploy this until I've gotten review

There is a little more followup I would like to do to close out #1659, but not in this PR:

  • pull the ServiceAccountLister role out of projects and up into the org (but keep the binding(s) project level)
  • remove StorageBucketLister unless there's a reason to keep it around
  • remove the detritus cleanup code marked with TODO's
  • setup a periodic job to refresh the custom roles to pick up new permissions that are added over time

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 wg/k8s-infra approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 26, 2021
@spiffxp
Copy link
Member Author

spiffxp commented Feb 26, 2021

/uncc @bartsmykla @mikedanese
/cc @ameukam @cblecker @dims
/assign @thockin

@dims
Copy link
Member

dims commented Feb 26, 2021

I like the idea! but don't know enough GCP stuff to review thoroughly. +1 to ship and iterate too!

E_TOOMUCHBASH and depends on yq, but now we can define custom roles using
something that looks like a spec
need to substitute in full role name depending on whether creating as
project role or organization role

output diff of role
refresh to pick up new permissions added to predefined roles,
and grant read access to all buckets/objects within a project
this was reverse-engineered from a role that's already in the org,
presumably manually created a long time ago
this was reverse-engineered from a role that already exists in the org,
presumably manually created a long time ago
intended to supplement the roles/owner binding given to org admins, and
to replace the additional role bindings used for org admins

- removed redundant roles from the list used to generate this
- added role to administer folders
add two individual users to the redundant role bindings just
in case this ends up locking out the group
@spiffxp
Copy link
Member Author

spiffxp commented Feb 27, 2021

Rebased due to #1733

@ameukam
Copy link
Member

ameukam commented Feb 27, 2021

/lgtm
(I blindly trust Aaron as a Bash Fighter. 😃)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2021
@ameukam
Copy link
Member

ameukam commented Feb 27, 2021

/hold
Un-hold when you're ready.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 27, 2021
@spiffxp
Copy link
Member Author

spiffxp commented Mar 1, 2021

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 1, 2021
@k8s-ci-robot k8s-ci-robot merged commit 8f66dc6 into kubernetes:main Mar 1, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Mar 1, 2021
@spiffxp spiffxp deleted the manage-org-roles branch March 1, 2021 05:02
@spiffxp
Copy link
Member Author

spiffxp commented Mar 1, 2021

Using the same process as https://gist.github.com/spiffxp/7d2bfaa9c510de89a7f2ce1c31ec1a00, I removed the early exit and ran ./infra/gcp/ensure-organization.sh

The resulting diff for k8s-infra-gcp-org-admins' permissions looks expected; nothing lost, and resourecemanager.folder permissions gained.

$ diff org-admin-permissions-{before,after}.txt
3460a3461,3462
> - resourcemanager.folders.create
> - resourcemanager.folders.delete
3463a3466
> - resourcemanager.folders.move
3464a3468,3469
> - resourcemanager.folders.undelete
> - resourcemanager.folders.update

@spiffxp
Copy link
Member Author

spiffxp commented Mar 4, 2021

/kind feature
/kind cleanup
/area infra/auditing
tagging issues/PRs related to audit followup

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. area/audit Audit of project resources, audit followup issues, code in audit/ labels Mar 4, 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. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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.

5 participants