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

Fix daemon-set-controller bootstrap RBAC policy #62146

Merged
merged 1 commit into from Apr 13, 2018

Conversation

@frodenas
Contributor

frodenas commented Apr 4, 2018

What this PR does / why we need it:

Adds a get permission for the controllerrevisions resource to the daemon-set-controller cluster role.

Which issue(s) this PR fixes :

Fixes #62145

Special notes for your reviewer:

The daemon-sets controller constructs the history of the daemon-set, so it needs to check the controller revisions for the daemon-set app. See issue above.

Release note:

Resolves forbidden error when the `daemon-set-controller` cluster role access `controllerrevisions` resources.
Fix daemon-set-controller bootstrap RBAC policy
Signed-off-by: Ferran Rodenas <rodenasf@vmware.com>
@frodenas

This comment has been minimized.

Contributor

frodenas commented Apr 4, 2018

/kind bug
/sig auth

@ericchiang

This comment has been minimized.

Member

ericchiang commented Apr 4, 2018

/lgtm

Is this newly introduced behavior or will it require a cherry pick? Based on the bug report it seems like this would be extremely disruptive bug. Surprised this only surfaced recently.

@frodenas

This comment has been minimized.

Contributor

frodenas commented Apr 4, 2018

@ericchiang It will require a cherry-pick, at least for 1.9. I ran into this randomly, I'm still unsure what scenarios trigger the issue.

@ericchiang

This comment has been minimized.

Member

ericchiang commented Apr 4, 2018

/assign @liggitt
/assign @deads2k

@liggitt

This comment has been minimized.

Member

liggitt commented Apr 4, 2018

Can you add a test that exercises the function that fails without this permission?

Are there other controllers that need similar permissions/tests?

@frodenas

This comment has been minimized.

Contributor

frodenas commented Apr 5, 2018

/test pull-kubernetes-e2e-gce-100-performance

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Apr 5, 2018

@frodenas: you can't request testing unless you are a kubernetes member.

In response to this:

/test pull-kubernetes-e2e-gce-100-performance

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.

@liggitt

This comment has been minimized.

Member

liggitt commented Apr 5, 2018

/assign @kow3ns
can you give feedback on what would be needed to exercise this in CI?

@dims

This comment has been minimized.

Member

dims commented Apr 6, 2018

/ok-to-test
/test pull-kubernetes-e2e-gce-100-performance

@janetkuo

This comment has been minimized.

Member

janetkuo commented Apr 10, 2018

/lgtm

@janetkuo

This comment has been minimized.

Member

janetkuo commented Apr 10, 2018

/retest

@janetkuo

This comment has been minimized.

Member

janetkuo commented Apr 10, 2018

/assign @deads2k
for approval

@liggitt

This comment has been minimized.

Member

liggitt commented Apr 10, 2018

Is RBAC unit test not enough? We can add an integration test or e2e test to have a DaemonSet with a ControllerRevision who doesn't match current DaemonSet template but has name collision with the ControllerRevision that the DaemonSet is going to create.

It's preferred to be done in integration test, but if integration test doesn't cover RBAC it needs to be tested in e2e.

The unit test doesn't demonstrate that the permission is sufficient, it just records the fact that the permission was added to the role.

This type of externally visible behavior seems like an ideal candidate for the e2e suite (and possibly even a conformance test, to ensure that in a conformant cluster, the daemonset controller handles this case)

@kow3ns

This comment has been minimized.

Member

kow3ns commented Apr 10, 2018

@liggit I don't think we should add ControllerRevision collision as an e2e test suite. The RBAC permissions associated with the DaemonSet controller are an implementation detail of the controller. The behavior with respect to the resolution of collisions for controller revisions is not visible to end users. With respect to conformance, I don't think its fair to say that a Kubenetes cluster that does not have RBAC configured to allow its DaemonSet controller to perform a get against ControllerHistory is not a valid Kubernetes cluster.
I would like to see the code exercised via an integration test, but if integration doesn't currently allow for RBAC I'd prefer to just validate the configuration at the unit level.

@liggitt

This comment has been minimized.

Member

liggitt commented Apr 11, 2018

I don't think we should add ControllerRevision collision as an e2e test suite. The RBAC permissions associated with the DaemonSet controller are an implementation detail of the controller. The behavior with respect to the resolution of collisions for controller revisions is not visible to end users.

the net effect of the bug was definitely end-user visible (create/delete a few times hung the controller). That's a test gap I would want to ensure was covered in a actual cluster to flush out unexpected interactions (e.g. create, delete, create, delete, create, and ensure the expected pods materialize)

Phrasing it more in black box terms rather than trying to drive a hash collision makes sense to me. I think we'd want the scenario that broke to actually be tested, but if you want to merge this without addressing the test gap, I won't block on it.

/approve
policy change is approved

/hold
@kow3ns can unhold when satisfied with test coverage

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Apr 11, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ericchiang, frodenas, janetkuo, liggitt

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

@janetkuo

This comment has been minimized.

Member

janetkuo commented Apr 11, 2018

I was not able to reproduce this by creating and deleting DaemonSets, and it's not clear to me how to reproduce it. Hash collision (i.e. ControllerRevision name collision) is different from what we saw in #62145, although it can trigger the same permission error. An e2e test for hash collision handling would have failed without this RBAC permission (it's more suitable to be an integration test, but integration test doesn't check RBAC), but it doesn't really cover the scenario described in this bug.

OTOH this PR adds a missing RBAC permission that DaemonSet controller clearly needs.

@janetkuo

This comment has been minimized.

Member

janetkuo commented Apr 12, 2018

Actually we should enable RBAC in DaemonSet integration tests instead of testing this in e2e just for RBAC. Otherwise all integration tests lose RBAC permission related coverage.

@kow3ns

This comment has been minimized.

Member

kow3ns commented Apr 12, 2018

/unhold
@janetkuo when you are satisfied I'm okay with merging this, can we open an issue to add RBAC to add the default RBAC for the workloads controller to the API server constructed for the integration framework.

@frodenas

This comment has been minimized.

Contributor

frodenas commented Apr 12, 2018

@janetkuo Thanks for taking a look at the tests, I haven't had time yet to look into this and probably I'll take me some time as I'm new to this codebase.

@janetkuo

This comment has been minimized.

Member

janetkuo commented Apr 13, 2018

Filed #62518 to track adding RBAC in integration
Filed #62519 to track adding DaemonSet hash collision test

Remove hold manually. @kow3ns already put unhold, but somehow the bot didn't react.

@janetkuo

This comment has been minimized.

Member

janetkuo commented Apr 13, 2018

/retest

@janetkuo

This comment has been minimized.

Member

janetkuo commented Apr 13, 2018

@frodenas thanks! Let's add the integration test in a follow-up PR

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Apr 13, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 39194c1 into kubernetes:master Apr 13, 2018

15 of 17 checks passed

pull-kubernetes-e2e-gce-100-performance Job failed.
Details
Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation frodenas authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@frodenas frodenas deleted the frodenas:fix-dsc-rbac branch Apr 13, 2018

@frodenas frodenas restored the frodenas:fix-dsc-rbac branch Apr 13, 2018

@frodenas frodenas deleted the frodenas:fix-dsc-rbac branch Apr 13, 2018

@janetkuo

This comment has been minimized.

Member

janetkuo commented Apr 13, 2018

@frodenas would you cherrypick this fix to 1.10 also? Thanks!

@frodenas

This comment has been minimized.

Contributor

frodenas commented Apr 13, 2018

@janetkuo Done

k8s-merge-robot added a commit that referenced this pull request Apr 15, 2018

Merge pull request #62524 from frodenas/automated-cherry-pick-of-#62146
…-upstream-release-1.9

Automatic merge from submit-queue.

Automated cherry pick of #62146: Fix daemon-set-controller bootstrap RBAC policy

Cherry pick of #62146 on release-1.9.

#62146: Fix daemon-set-controller bootstrap RBAC policy

k8s-merge-robot added a commit that referenced this pull request Apr 19, 2018

Merge pull request #62526 from frodenas/automated-cherry-pick-of-#62146
…-upstream-release-1.8

Automatic merge from submit-queue.

Automated cherry pick of #62146: Fix daemon-set-controller bootstrap RBAC policy

Cherry pick of #62146 on release-1.8.

#62146: Fix daemon-set-controller bootstrap RBAC policy

k8s-merge-robot added a commit that referenced this pull request Apr 19, 2018

Merge pull request #62562 from frodenas/automated-cherry-pick-of-#62146
…-upstream-release-1.10

Automatic merge from submit-queue.

Automated cherry pick of #62146: Fix daemon-set-controller bootstrap RBAC policy

Cherry pick of #62146 on release-1.10.

#62146: Fix daemon-set-controller bootstrap RBAC policy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment