releng: Enable elevated privileges for Release Managers on SIG Release GCP projects #434

justaugustus · 2019-10-28T06:54:44Z

Add groups to delegate IAM access to Release Managers
- k8s-infra-release-admins@: Partial Admin
- k8s-infra-release-editors@: Editor
- k8s-infra-release-viewers@: Viewer
Add ensure-release-projects to grant rights to Release Managers
Establishes rights to GCS, GCB, and KMS for Release Managers

Required for prototyping build/stage/release process on k8s-infra (kubernetes/release#911).

/assign @dims @thockin @cblecker
cc: @kubernetes/sig-release-admins @kubernetes/release-engineering
/sig release
/area release-eng

k8s-ci-robot · 2019-10-28T06:54:46Z

@justaugustus: The label(s) area/release-eng cannot be applied. These labels are supported: api-review, community/discussion, community/maintenance, community/question, cuj/build-train-deploy, cuj/multi-user, platform/aws, platform/azure, platform/gcp, platform/minikube, platform/other

In response to this:

Add release-managers-admins@ group

Grants elevated privileges to SIG Release GCP projects.
Currently, this is restricted to SIG Release Chairs.

Enable reconciliation of Release Manager groups

Add ensure-release-projects to grant rights to Release Managers

Establishes rights to GCS, GCB, and KMS for Release Managers

Admins: release-managers-admins@

Writers: release-managers-private@

Viewers: release-managers@

Required for prototyping build/stage/release process on k8s-infra (#911).

/assign @dims @thockin @cblecker
cc: @kubernetes/sig-release-admins @kubernetes/release-engineering
/sig release
/area release-eng

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.

cblecker · 2019-10-28T16:58:30Z

/hold
Explicit hold for review.

hoegaarden

Some nits and questions, but generally /lgtm.

hoegaarden · 2019-10-28T19:43:43Z

infra/gcp/ensure-release-projects.sh

+function usage() {
+    echo "usage: $0 [repo...]" > /dev/stderr
+    echo "example:" > /dev/stderr
+    echo "  $0 # do all staging repos" > /dev/stderr
+    echo "  $0 coredns # just do one" > /dev/stderr
+    echo > /dev/stderr
+}


When/how/where would that be called?

it serves as docs, at least :)

hoegaarden · 2019-10-28T19:56:24Z

infra/gcp/ensure-release-projects.sh

+WRITERS="release-managers-private@kubernetes.io"
+VIEWERS="release-managers@kubernetes.io"
+
+for REPO; do


TIL: you can omit the in WORDS ...

From help for:

for: for NAME [in WORDS ... ] ; do COMMANDS; done
[...]
If in WORDS ...;' is not present, then in "$@"' is assumed.
[...]

How would you feel about changing that to explicitly use $@ for clarity?

for REPO in "$@"; do ... done

I would even go further and do

for REPO in "${@:-${PROJECTS[@]}}"; do ... done

instead of doing the set -- ... and overriding global state. But that is not that easy to read/understand if you are not used to reading stuff like that and probably more importantly it is just my personal preference.

Once I learned to embrace "$@" I started to prefer the simpler syntax, and this PR follows wstablished patterns

hoegaarden · 2019-10-28T20:06:02Z

infra/gcp/ensure-release-projects.sh

+    color 3 "Configuring: ${REPO}"
+
+    # The GCP project name.
+    PROJECT="${REPO}"


Can we maybe just use PROJECT instead of REPO? As far as I can tell, we never use REPO further down, so can we just do for PROJECT ...?

yes, the loop can be for PROJECT - "repo" is a holdover from the script he copied, I assume :)

hoegaarden · 2019-10-28T20:08:41Z

infra/gcp/lib.sh

+# $1: The GCP project
+# $2: The group email
+function empower_group_for_kms() {
+    if [ $# -lt 2 -o -z "$1" -o -z "$2" ]; then


Should we make that stricter by doing something like the below?

if [ $# -ne 2 -o -z "$1" -o -z "$2" ]; then

I saw, that other funcs here allow additional arguments, but that might just be a c&p "issue" from cases where additional optional arguments are allowed (e.g. empower_group_to_gcr or empower_gcr_admins).

this is probably a good idea, but I'm OK to let this PR go in and fix it up across the board.

thockin

Mostly I question the overall factoring - these don't feel like "staging" and "prod" projects (in the sense that we have scripts that exist) any more. Maybe they need to be totally distinct (removed from other scripts) and even renamed?

thockin · 2019-10-28T20:14:38Z

infra/gcp/ensure-release-projects.sh

+    echo "usage: $0 [repo...]" > /dev/stderr
+    echo "example:" > /dev/stderr
+    echo "  $0 # do all staging repos" > /dev/stderr
+    echo "  $0 coredns # just do one" > /dev/stderr


change "coredns" to a real (or realer-looking) example?

thockin · 2019-10-28T20:16:17Z

infra/gcp/ensure-release-projects.sh

+    set -- "${PROJECTS[@]}"
+fi
+
+ADMINS="release-managers-admins@kubernetes.io"


Casually looking at this, the distinctions are not clear. Why are release-managers just viewers? Do we need to rename groups? Can this be solved with docs here? Why do we need 3 distinct groups?

Here's what each group would do:

release-managers@: Includes all Release Managers (including Release Manager Associates), so this needs to be least-privilege (allow Associates to view, but not create, edit, delete)

release-managers-private@: Only includes Patch Release Team, Branch Managers, Build Admins, SIG Chairs (the set of users we want working on GCP, so we need edit here)

release-managers-admins@: Only includes SIG Release Chairs and grants additional access to administer KMS (limited set of users, security event mitigation, key mgmt (store GitHub tokens, deb/rpm signing key))

We should make distinctions here between "Groups that provide IAM roles" and "E-mail distribution lists". I would be concerned with someone getting access to the release artifacts simply because they become a SIG chair and need access to an e-mail list.

I agree with @cblecker - there are the social groups (email) and the permissions groups, and I am not sure they are the same. Should being a SIG-chair automatically grant you access to GCP?

thockin · 2019-10-28T20:17:50Z

infra/gcp/ensure-release-projects.sh

+
+# NB: Please keep this sorted.
+PROJECTS=(
+    k8s-staging-release-test


currently these are covered by the other ensure-* scripts. Should we a) drop them from those, b) add this logic to those, c) refactor to make the overlap clear?

justaugustus · 2019-10-28T21:38:17Z

@thockin --

Mostly I question the overall factoring - these don't feel like "staging" and "prod" projects (in the sense that we have scripts that exist) any more.

Yeah, totally agreed. This felt awkward to do, as it felt like shimming something into a model it doesn't really fit.
Each of the SIG Release GCP projects are essentially near-prod/prod, as various areas of the project depend on the artifacts:

k8s-staging-release-test/ci: Prow/intrepid users testing nightlies
k8s-staging-release-test/stage: Validated prior to promotion (but wider area of effect than other staging projects)
k8s-release-test-prod/release: All Kubernetes users

I think this projects would be more accurately named:

k8s-release-staging
k8s-release-prod

Maybe they need to be totally distinct (removed from other scripts) and even renamed?
currently these are covered by the other ensure-* scripts. Should we a) drop them from those, b) add this logic to those, c) refactor to make the overlap clear?

In an ideal world/future refactor, perhaps there could be one script, configure-projects.sh?
It would include three arrays, STAGING_PROJECTS, PROD_PROJECTS, and RELEASE_PROJECTS and maybe ensure behavior/project config/principal access on a set of case statements?

The overlap here is primarily because of copy/pasta.

I think the future should also include yaml config for custom roles, instead of adhoc IAM attaches.

justaugustus · 2019-10-28T21:52:21Z

(Just also noting that I went the copy/pasta route instead of refactoring because it wouldn't interfere with the other projects.)

justaugustus · 2019-10-28T22:01:23Z

(@hoegaarden -- I'll come back and address your review once we land on an approach.)

cblecker

Some initial thoughts on direction. These are in addition to @thockin 's other comments.

cblecker · 2019-10-28T22:59:10Z

groups/groups.yaml

+    name: release-managers-admins
+    description: |-
+      Grants elevated privileges to SIG Release GCP projects.
+      Currently, this is restricted to SIG Release Chairs.


I've mentioned this before in other contexts, but tying privileges to "chairpersonship" of a SIG/WG is an anti-pattern we should try to avoid. These privileges should be tied to A) a unique role defined by the SIG (like "Release Admins" or whatever you want to call it) or B) ownership of a subproject, or even C) a list of people who are just "keyholders".

cblecker · 2019-10-28T23:04:09Z

infra/gcp/ensure-release-projects.sh

+    set -- "${PROJECTS[@]}"
+fi
+
+ADMINS="release-managers-admins@kubernetes.io"


We should make distinctions here between "Groups that provide IAM roles" and "E-mail distribution lists". I would be concerned with someone getting access to the release artifacts simply because they become a SIG chair and need access to an e-mail list.

groups/groups.yaml

justaugustus · 2019-10-29T17:50:13Z

@cblecker --

I've mentioned this before in other contexts, but tying privileges to "chairpersonship" of a SIG/WG is an anti-pattern we should try to avoid. These privileges should be tied to A) a unique role defined by the SIG (like "Release Admins" or whatever you want to call it) or B) ownership of a subproject, or even C) a list of people who are just "keyholders".

Yep, yep. I think the last time we chatted about it was here: kubernetes/org#1205 (comment).
SIG Chairs are also Release Engineering subproject owners, so I occasionally interchange the two in documentation. I've updated the verbiage there.

We should make distinctions here between "Groups that provide IAM roles" and "E-mail distribution lists". I would be concerned with someone getting access to the release artifacts simply because they become a SIG chair and need access to an e-mail list.

Combining the two (communication and access) is exactly what we wanted to do for the Release Manager lists. We also moved to kubernetes.io for a more "official" address and more importantly, to get the benefit of being able to manage this access via config/PR.

More details can be found here: kubernetes/sig-release#732

thockin · 2019-10-29T18:13:35Z

groups/groups.yaml

+    owners:
+      - caselim@gmail.com
+      - cmiles@pivotal.io
+      - saugustus@vmware.com


why are we adding two accounts for Stephen and Tim? This means twice as many credentials that could leak. I'd like to make a policy that we explicitly NOT do this.

thockin · 2019-10-29T18:14:39Z

infra/gcp/lib.sh

+# $1: The GCP project
+# $2: The group email
+function empower_group_for_kms() {
+    if [ $# -lt 2 -o -z "$1" -o -z "$2" ]; then


this is probably a good idea, but I'm OK to let this PR go in and fix it up across the board.

thockin · 2019-10-29T18:16:51Z

infra/gcp/ensure-release-projects.sh

+function usage() {
+    echo "usage: $0 [repo...]" > /dev/stderr
+    echo "example:" > /dev/stderr
+    echo "  $0 # do all staging repos" > /dev/stderr


s/staging repos/release projects

thockin · 2019-10-29T18:17:20Z

infra/gcp/ensure-release-projects.sh

+function usage() {
+    echo "usage: $0 [repo...]" > /dev/stderr
+    echo "example:" > /dev/stderr
+    echo "  $0 # do all staging repos" > /dev/stderr
+    echo "  $0 coredns # just do one" > /dev/stderr
+    echo > /dev/stderr
+}


it serves as docs, at least :)

thockin · 2019-10-29T18:21:44Z

infra/gcp/ensure-release-projects.sh

+    set -- "${PROJECTS[@]}"
+fi
+
+ADMINS="release-managers-admins@kubernetes.io"


I agree with @cblecker - there are the social groups (email) and the permissions groups, and I am not sure they are the same. Should being a SIG-chair automatically grant you access to GCP?

thockin · 2019-10-29T18:22:57Z

infra/gcp/ensure-release-projects.sh

+WRITERS="release-managers-private@kubernetes.io"
+VIEWERS="release-managers@kubernetes.io"
+
+for REPO; do


Once I learned to embrace "$@" I started to prefer the simpler syntax, and this PR follows wstablished patterns

thockin · 2019-10-29T18:23:43Z

infra/gcp/ensure-release-projects.sh

+    color 3 "Configuring: ${REPO}"
+
+    # The GCP project name.
+    PROJECT="${REPO}"


yes, the loop can be for PROJECT - "repo" is a holdover from the script he copied, I assume :)

thockin · 2019-10-29T18:30:59Z

infra/gcp/ensure-release-projects.sh

+    PROJECT="${REPO}"
+
+    # The names of the buckets
+    STAGING_BUCKET="gs://${PROJECT}" # used by humans


This is the piece I want to discuss most - Do we want these projects "governed" by 2 ensure scripts? This overlaps directly with the other scripts, which means it will drift.

There's a particular set of semantics that we ascribe to a "staging project" and a "prod project". You are adding on to that, clearly, but it's not clear if you are changing more than that. In other words - should we make a clean break and remove these release projects from the other scripts? Or should we build on those?

For example, this script could do something like:

# First ensure that the project is a normal staging project "${ROOT}/ensure-staging-project.sh" release-test # Now apply customizations for release staging

If it helps, we can refactor the existing scripts to make this more obvious. I knew this was likely to come up, so we it should not be hard - we left room.

- `k8s-infra-release-admins@`: Partial Admin - `k8s-infra-release-editors@`: Editor - `k8s-infra-release-viewers@`: Viewer Signed-off-by: Stephen Augustus <saugustus@vmware.com>

Establishes rights to GCS, GCB, and KMS for Release Managers Signed-off-by: Stephen Augustus <saugustus@vmware.com>

k8s-ci-robot · 2019-10-29T23:52:12Z

@justaugustus: The label(s) area/release-eng cannot be applied. These labels are supported: api-review, community/discussion, community/maintenance, community/question, cuj/build-train-deploy, cuj/multi-user, platform/aws, platform/azure, platform/gcp, platform/minikube, platform/other

In response to this:

Add groups to delegate IAM access to Release Managers

k8s-infra-release-admins@: Partial Admin

k8s-infra-release-editors@: Editor

k8s-infra-release-viewers@: Viewer

Add ensure-release-projects to grant rights to Release Managers
Establishes rights to GCS, GCB, and KMS for Release Managers

Required for prototyping build/stage/release process on k8s-infra (#911).

/assign @dims @thockin @cblecker
cc: @kubernetes/sig-release-admins @kubernetes/release-engineering
/sig release
/area release-eng

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.

thockin

I will look at refactoring the shell code. I have an idea.

thockin · 2019-10-30T00:21:00Z

Thanks!

/lgtm
/approve

k8s-ci-robot · 2019-10-30T00:21:29Z

@justaugustus: The label(s) area/release-eng cannot be applied. These labels are supported: api-review, community/discussion, community/maintenance, community/question, cuj/build-train-deploy, cuj/multi-user, platform/aws, platform/azure, platform/gcp, platform/minikube, platform/other

In response to this:

Add groups to delegate IAM access to Release Managers

k8s-infra-release-admins@: Partial Admin

k8s-infra-release-editors@: Editor

k8s-infra-release-viewers@: Viewer

Add ensure-release-projects to grant rights to Release Managers
Establishes rights to GCS, GCB, and KMS for Release Managers

Required for prototyping build/stage/release process on k8s-infra (kubernetes/release#911).

/assign @dims @thockin @cblecker
cc: @kubernetes/sig-release-admins @kubernetes/release-engineering
/sig release
/area release-eng

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.

justaugustus · 2019-10-30T00:22:53Z

Awesome. Thanks y'all!
/hold cancel

cblecker · 2019-10-30T00:24:49Z

/approve

k8s-ci-robot · 2019-10-30T00:25:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, thockin

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:

~~OWNERS~~ [cblecker]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cblecker · 2019-10-30T00:35:44Z

Groups have been created.

k8s-ci-robot assigned cblecker, dims and thockin Oct 28, 2019

k8s-ci-robot added the sig/release Categorizes an issue or PR as relevant to SIG Release. label Oct 28, 2019

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 28, 2019

k8s-ci-robot requested review from dims and mikedanese October 28, 2019 06:54

k8s-ci-robot added the wg/k8s-infra label Oct 28, 2019

This was referenced Oct 28, 2019

Establish Kubernetes build/stage/release process on k8s-infra kubernetes/release#911

Open

[prototype] Stage/release Kubernetes on k8s-infra - Phase 0 kubernetes/release#909

Merged

justaugustus force-pushed the grant-releng branch from 4a47c3c to 7beff2d Compare October 28, 2019 12:10

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 28, 2019

hoegaarden reviewed Oct 28, 2019

View reviewed changes

thockin reviewed Oct 28, 2019

View reviewed changes

cblecker requested changes Oct 28, 2019

View reviewed changes

justaugustus force-pushed the grant-releng branch from 7beff2d to b9c4628 Compare October 29, 2019 17:46

thockin reviewed Oct 29, 2019

View reviewed changes

justaugustus force-pushed the grant-releng branch from b9c4628 to 305528a Compare October 29, 2019 21:53

justaugustus added 2 commits October 29, 2019 19:42

releng: Add groups to delegate IAM access to Release Managers

c217209

- `k8s-infra-release-admins@`: Partial Admin - `k8s-infra-release-editors@`: Editor - `k8s-infra-release-viewers@`: Viewer Signed-off-by: Stephen Augustus <saugustus@vmware.com>

releng: Add ensure-release-projects to grant rights to Release Managers

984f719

Establishes rights to GCS, GCB, and KMS for Release Managers Signed-off-by: Stephen Augustus <saugustus@vmware.com>

justaugustus force-pushed the grant-releng branch from 305528a to 984f719 Compare October 29, 2019 23:49

thockin reviewed Oct 30, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 30, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 30, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2019

k8s-ci-robot merged commit 25889d0 into kubernetes:master Oct 30, 2019

This was referenced Oct 30, 2019

Fix unbound variable in ensure-release-project.sh #439

Merged

Add cloudkms.cryptoKeyEncrypterDecrypter role to empower_group_for_kms #441

Merged

cpanato mentioned this pull request Dec 7, 2019

Release Manager access for @cpanato kubernetes/sig-release#892

Closed

13 tasks

justaugustus mentioned this pull request Dec 11, 2019

Clarification on new SIG Release GCP projects and image promotion kubernetes/release#964

Closed

releng: Enable elevated privileges for Release Managers on SIG Release GCP projects #434

releng: Enable elevated privileges for Release Managers on SIG Release GCP projects #434

Conversation

justaugustus commented Oct 28, 2019 • edited Loading

k8s-ci-robot commented Oct 28, 2019

cblecker commented Oct 28, 2019

hoegaarden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justaugustus commented Oct 28, 2019

justaugustus commented Oct 28, 2019

justaugustus commented Oct 28, 2019

cblecker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justaugustus commented Oct 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 29, 2019

thockin left a comment

Choose a reason for hiding this comment

thockin commented Oct 30, 2019

k8s-ci-robot commented Oct 30, 2019

justaugustus commented Oct 30, 2019

cblecker commented Oct 30, 2019

k8s-ci-robot commented Oct 30, 2019

cblecker commented Oct 30, 2019

justaugustus commented Oct 28, 2019 •

edited

Loading