Extend the gc admission plugin to check ownerReference.blockOwnerDeletion by caesarxuchao · Pull Request #43876 · kubernetes/kubernetes

caesarxuchao · 2017-03-30T21:43:52Z

#Extend the gc admission plugin to prevent user who doesn't have delete permission of the owner from changing blockOwnerDeletion field of existing ownerReferences, or adding ownerReference with blockOwnerDeletion=true.

The plugin need a RESTMapper to translate ownerRef.Kind to Resource. It should be using a dynamic one. However, as discussed in #42615, such a RESTMapper will be built after watchable discovery API is implemented, so in this PR the plugin is using the api.Registry.RESTMapper(), which is also used by the garbage collector currently.

Extending the gc admission plugin so that a user who doesn't have delete permission of the *owner* cannot modify blockOwnerDeletion field of existing ownerReferences, or add new ownerReference with blockOwnerDeletion=true

cc @lavalamp

k8s-github-robot · 2017-03-30T21:44:03Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: caesarxuchao
We suggest the following additional approvers: @deads2k, @derekwaynecarr

Details

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kube-apiserver/OWNERS~~ [caesarxuchao]
federation/cmd/federation-apiserver/OWNERS
pkg/kubeapiserver/OWNERS
plugin/pkg/admission/OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-reviewable · 2017-03-30T21:44:59Z

This change is

caesarxuchao · 2017-03-31T19:44:14Z

@k8s-bot verify test this

deads2k · 2017-03-31T20:35:10Z

@sttts since I'm out next week

caesarxuchao · 2017-04-04T18:23:58Z

@sttts could you take a look? Thanks.

0xmichalis · 2017-04-04T19:00:03Z

+		}
+		allowed, reason, err := a.authorizer.Authorize(attribute)
+		if !allowed {
+			return admission.NewForbidden(attributes, fmt.Errorf("cannot set blockOwnerDeletion in an ownerReference refers to a resource you can't delete: %v, %v", reason, err))


sttts · 2017-04-05T07:00:41Z

+
+// Returns new blocking ownerReferences, and references whose blockOwnerDeletion field is changed.
+// Changes between nil and false are ignored.
+func changingBlockOwnerDeletion(newObj, oldObj runtime.Object) []metav1.OwnerReference {


Misleading name, sounds like a predicate. better: changedDeletionBlockingOwnerRefs.

sttts · 2017-04-05T07:07:55Z

+
+	var changedRefs []metav1.OwnerReference
+	indexedOldRefs := indexByUID(oldMeta.GetOwnerReferences())
+	for _, newRef := range newRefs {


if an OwnerRef goes away in newObj, this will go unnoticed. Might be intended, but makes this unsymmetric. The func names all sound symmetric (changingBlockOwnerDeletion).

It's intentional. If a user has permission on the dependent object, she should have the right to remove the ownerRef so that the object doesn't share life cycle with the owner object.

Currently this admission plugin also disallows a user setting ref.blockOwnerDeletion from to true to false if she doesn't have delete permission of the owner object. Probably we should just allow that since the user can change the deletion behavior of the owner in the same way by removing the ref.

These two cases make the function asymmetric. I'll change the function name.

Updated the code to match what I wrote in the comment above.

sttts · 2017-04-05T07:09:35Z

+	return authorizer.AttributesRecord{
+		User: attributes.GetUserInfo(),
+		Verb: "delete",
+		// ownerReference can only refer to an object in the same namespace, so attributes.GetnNamespace() equals to the owner's namespace


GetnNamespace

sttts · 2017-04-05T07:14:31Z

+	for _, ref := range changedRefs {
+		attribute, err := a.toDeleteAttribute(ref, attributes)
+		if err != nil {
+			// An error occurs if a RESTMapping cannot be found for


Is this a security issue? If the RESTMapper is not up-to-date, we will allow ObjectReferences which would otherwise be forbidden. Until we have watchable discovery this might be fine as this only applies to TPRs. Can you add a comment that we switch to safe default "Forbidden" when we have watchable discovery ?

Good point. Further, if we are making the default "Forbidden" in the future, we should make it "Forbidden" now. Making validation rules tighter is backwards incompatible. Updated.

caesarxuchao · 2017-04-05T21:12:27Z

Comments addresed. PTAL. Thanks.

deads2k · 2017-04-11T19:05:03Z

+		return nil
+	}
+	newRefs := newMeta.GetOwnerReferences()
+	blockingNewRefs := blockingOwnerRefs(newRefs)


if this list is empty, return.

deads2k · 2017-04-11T19:06:33Z

+	}
+
+	var ret []metav1.OwnerReference
+	indexedOldRefs := indexByUID(oldMeta.GetOwnerReferences())


I would actually bet on it being just as fast to iterate the n-squared loop, but I don't really care.

deads2k · 2017-04-11T19:08:32Z

+	var ret []metav1.OwnerReference
+	indexedOldRefs := indexByUID(oldMeta.GetOwnerReferences())
+	for _, ref := range blockingNewRefs {
+		if oldRef, ok := indexedOldRefs[ref.UID]; ok {


optional nit: reduce nesting depth like

oldRef, ok := indexOldRefs[ref.UID] if !ok{ ret = append(... continue } wasNotBlocking := ... if wasNotBlocking{ ret = append(... continue }

deads2k · 2017-04-11T19:10:48Z

 }

+func (a *gcPermissionsEnforcement) SetRESTMapper(restMapper meta.RESTMapper) {
+	a.restMapper = restMapper


check that you have this in Validate

deads2k · 2017-04-11T19:11:43Z

 }

+// translates ref to a DeleteAttribute deleting the object referred by the ref.
+func (a *gcPermissionsEnforcement) toDeleteAttribute(ref metav1.OwnerReference, attributes admission.Attributes) (authorizer.AttributesRecord, error) {


ownerRefToDeleteAttribute?

deads2k · 2017-04-11T19:12:51Z

+	if err != nil {
+		return authorizer.AttributesRecord{}, err
+	}
+	mapping, err := a.restMapper.RESTMapping(schema.GroupKind{Group: groupVersion.Group, Kind: ref.Kind}, groupVersion.Version)


ResourcesFor and check for any match?

ResourcesFor takes a Resource, not a Kind, as input: https://github.com/kubernetes/apimachinery/blob/master/pkg/api/meta/interfaces.go#L136

RESTMapping().Resource seems the only way to translate Kind to Resource.

RESTMapping().Resource seems the only way to translate Kind to Resource.

RESTMappings then. We should handle the multi-match case.

The behavior is undefined if there are multiple matches. The admission controller cannot determine to which resource the user is trying to set the ownerReference.
In the garbage collector, it only handles the first match.
I think it's a bug in the design phase that the ownerReference records the Kind instead of Resource. I don't think we should fix it by trying all restmappings, rather, we should add a Resource field to the ownerReference. WDYT?

I think it's a bug in the design phase that the ownerReference records the Kind instead of Resource. I don't think we should fix it by trying all restmappings, rather, we should add a Resource field to the ownerReference. WDYT?

I have long thought that, but its controversial. In other places, @liggitt wants to keep kind assumes a canonical mapping from one to the other. I can't tell what @lavalamp wants given comments in the TPR thread, but I previously thought he liked the one to one mapping.

Given that there is more than one mapping possible, we can start out strict (must have delete access to all) and make it less strict later if we find it to be a problem.

I would not start down the path of changing the API without broad agreement.

Regardless of whether Kind <-> Resource is a one-on-one mapping, recording Resource in ownerReference seems correct, because Resource indicates a unique way to retrieve the owner object.

I think the problem of adding ownerReference.Resource now is that we have to keep it alongside the Kind field, due to the deprecation policy. That can cause confusion.

Given that there is more than one mapping possible, we can start out strict (must have delete access to all) and make it less strict later if we find it to be a problem.

Ok.

deads2k · 2017-04-11T19:15:35Z

+			username:        "non-rc-deleter",
+			resource:        api.SchemeGroupVersion.WithResource("pods"),
+			newObj:          podWithOwnerRefs(blockRC1, notBlockRC2),
+			expectedAllowed: false,


We have multiple deny branches now. Please check the error message to make sure it's the failure you're expecting.

deads2k · 2017-04-11T19:16:38Z

Looks structurally sound. A few comments I'd like to see fixed up.

caesarxuchao · 2017-04-12T22:30:30Z

@deads2k i addressed all the comments except the one regarding calling ResourcesFor(). I replied to that comment inline. PTAL. Thanks.

deads2k · 2017-04-13T18:46:59Z

After the multiple mappings handled, this lgtm.

caesarxuchao · 2017-04-13T18:53:10Z

Multiple-mapping case handled. PTAL. Thanks.

deads2k · 2017-04-13T18:53:18Z

lgtm, don't forget to squash.

…rmission of the owner from setting blockOwnerDeletion

caesarxuchao · 2017-04-13T18:56:03Z

Thanks for the reviews. Squashed. Applying the labels per comment.

k8s-github-robot · 2017-04-14T06:18:05Z

Automatic merge from submit-queue (batch tested with PRs 44440, 44038, 44302, 44316, 43876)

@lavalamp

…ion-admission Automatic merge from submit-queue (batch tested with PRs 44440, 44038, 44302, 44316, 43876) Extend the gc admission plugin to check ownerReference.blockOwnerDeletion #Extend the gc admission plugin to prevent user who doesn't have delete permission of the *owner* from changing blockOwnerDeletion field of existing ownerReferences, or adding ownerReference with blockOwnerDeletion=true. The plugin need a RESTMapper to translate ownerRef.Kind to Resource. It should be using a dynamic one. However, as discussed in kubernetes#42615, such a RESTMapper will be built after watchable discovery API is implemented, so in this PR the plugin is using the `api.Registry.RESTMapper()`, which is also [used](https://github.com/kubernetes/kubernetes/blob/master/cmd/kube-controller-manager/app/core.go#L165-L166) by the garbage collector currently. ```release-note Extending the gc admission plugin so that a user who doesn't have delete permission of the *owner* cannot modify blockOwnerDeletion field of existing ownerReferences, or add new ownerReference with blockOwnerDeletion=true ``` cc @lavalamp

caesarxuchao assigned deads2k Mar 30, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 30, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Mar 30, 2017

caesarxuchao removed the release-note-label-needed label Mar 30, 2017

k8s-github-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Mar 30, 2017

deads2k assigned sttts Mar 31, 2017

0xmichalis reviewed Apr 4, 2017

View reviewed changes

sttts reviewed Apr 5, 2017

View reviewed changes

deads2k reviewed Apr 11, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 12, 2017

caesarxuchao force-pushed the blockOwnerDeletion-admission branch from 5f2be3c to 587926c Compare April 12, 2017 22:29

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 12, 2017

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 12, 2017

caesarxuchao force-pushed the blockOwnerDeletion-admission branch from 587926c to a956f7c Compare April 12, 2017 22:55

add gc admission plugin that prevents user who doesn't have delete pe…

9d7a8df

…rmission of the owner from setting blockOwnerDeletion

caesarxuchao force-pushed the blockOwnerDeletion-admission branch from fce1926 to 9d7a8df Compare April 13, 2017 18:55

caesarxuchao added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 13, 2017

k8s-github-robot merged commit 3b9eb1a into kubernetes:master Apr 14, 2017

coderabbitai Bot mentioned this pull request Jun 22, 2026

feat(kubernetes): bootstrap tenant workers with Talos via CABPT cozystack/cozystack#2931

Merged

Conversation

caesarxuchao commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-github-robot commented Mar 30, 2017

Uh oh!

k8s-reviewable commented Mar 30, 2017

Uh oh!

caesarxuchao commented Mar 31, 2017

Uh oh!

deads2k commented Mar 31, 2017

Uh oh!

caesarxuchao commented Apr 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caesarxuchao commented Apr 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Apr 11, 2017

Uh oh!

caesarxuchao commented Apr 12, 2017

Uh oh!

deads2k commented Apr 13, 2017

Uh oh!

caesarxuchao commented Apr 13, 2017

Uh oh!

deads2k commented Apr 13, 2017

Uh oh!

caesarxuchao commented Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-github-robot commented Apr 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

caesarxuchao commented Mar 30, 2017 •

edited

Loading

caesarxuchao commented Apr 13, 2017 •

edited

Loading