Skip to content

Commit

Permalink
Merge pull request kubernetes#49133 from deads2k/gc-02-permissions
Browse files Browse the repository at this point in the history 
Automatic merge from submit-queue (batch tested with PRs 49133, 51557, 51749, 50842, 52018)

add controller permissions to set blockOwnerDeletion

fixes kubernetes#51970

`blockOwnerDeletion` requires delete permissions on the owner object.  This adds that permission for our controllers.

@kubernetes/sig-auth-misc 

```release-note
The OwnerReferencesPermissionEnforcement admission plugin now requires `update` permission on the `finalizers` subresource of the referenced owner in order to set `blockOwnerDeletion` on an owner reference.
```
  • Loading branch information
Kubernetes Submit Queue authored Sep 6, 2017
2 parents 7be29bd + 2572ea5 commit 0ed05f2
Show file tree
Hide file tree
Showing 4 changed files with 67 additions and 3 deletions.
5 changes: 3 additions & 2 deletions plugin/pkg/admission/gc/gc_admission.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ func (a *gcPermissionsEnforcement) Admit(attributes admission.Attributes) (err e
for _, record := range records {
allowed, reason, err := a.authorizer.Authorize(record)
if !allowed {
return admission.NewForbidden(attributes, fmt.Errorf("cannot set blockOwnerDeletion if an ownerReference refers to a resource you can't delete: %v, %v", reason, err))
return admission.NewForbidden(attributes, fmt.Errorf("cannot set blockOwnerDeletion if an ownerReference refers to a resource you can't set finalizers on: %v, %v", reason, err))
}
}
}
Expand Down Expand Up @@ -178,12 +178,13 @@ func (a *gcPermissionsEnforcement) ownerRefToDeleteAttributeRecords(ref metav1.O
for _, mapping := range mappings {
ret = append(ret, authorizer.AttributesRecord{
User: attributes.GetUserInfo(),
Verb: "delete",
Verb: "update",
// ownerReference can only refer to an object in the same namespace, so attributes.GetNamespace() equals to the owner's namespace
Namespace: attributes.GetNamespace(),
APIGroup: groupVersion.Group,
APIVersion: groupVersion.Version,
Resource: mapping.Resource,
Subresource: "finalizers",
Name: ref.Name,
ResourceRequest: true,
Path: "",
Expand Down
14 changes: 13 additions & 1 deletion plugin/pkg/admission/gc/gc_admission_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,20 +39,29 @@ func (fakeAuthorizer) Authorize(a authorizer.Attributes) (bool, string, error) {
if a.GetVerb() == "delete" {
return false, "", nil
}
if a.GetVerb() == "update" && a.GetSubresource() == "finalizers" {
return false, "", nil
}
return true, "", nil
}

if username == "non-pod-deleter" {
if a.GetVerb() == "delete" && a.GetResource() == "pods" {
return false, "", nil
}
if a.GetVerb() == "update" && a.GetResource() == "pods" && a.GetSubresource() == "finalizers" {
return false, "", nil
}
return true, "", nil
}

if username == "non-rc-deleter" {
if a.GetVerb() == "delete" && a.GetResource() == "replicationcontrollers" {
return false, "", nil
}
if a.GetVerb() == "update" && a.GetResource() == "replicationcontrollers" && a.GetSubresource() == "finalizers" {
return false, "", nil
}
return true, "", nil
}

Expand Down Expand Up @@ -326,7 +335,10 @@ func TestBlockOwnerDeletionAdmission(t *testing.T) {
return err == nil
}
expectCantSetBlockOwnerDeletionError := func(err error) bool {
return strings.Contains(err.Error(), "cannot set blockOwnerDeletion if an ownerReference refers to a resource you can't delete")
if err == nil {
return false
}
return strings.Contains(err.Error(), "cannot set blockOwnerDeletion if an ownerReference refers to a resource you can't set finalizers on")
}
tests := []struct {
name string
Expand Down
7 changes: 7 additions & 0 deletions plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ func buildControllerRoles() ([]rbac.ClusterRole, []rbac.ClusterRoleBinding) {
rbac.NewRule("get", "list", "watch", "update").Groups(batchGroup).Resources("cronjobs").RuleOrDie(),
rbac.NewRule("get", "list", "watch", "create", "update", "delete", "patch").Groups(batchGroup).Resources("jobs").RuleOrDie(),
rbac.NewRule("update").Groups(batchGroup).Resources("cronjobs/status").RuleOrDie(),
rbac.NewRule("update").Groups(batchGroup).Resources("cronjobs/finalizers").RuleOrDie(),
rbac.NewRule("list", "delete").Groups(legacyGroup).Resources("pods").RuleOrDie(),
eventsRule(),
},
Expand All @@ -83,6 +84,7 @@ func buildControllerRoles() ([]rbac.ClusterRole, []rbac.ClusterRoleBinding) {
Rules: []rbac.PolicyRule{
rbac.NewRule("get", "list", "watch").Groups(extensionsGroup, appsGroup).Resources("daemonsets").RuleOrDie(),
rbac.NewRule("update").Groups(extensionsGroup, appsGroup).Resources("daemonsets/status").RuleOrDie(),
rbac.NewRule("update").Groups(extensionsGroup, appsGroup).Resources("daemonsets/finalizers").RuleOrDie(),
rbac.NewRule("list", "watch").Groups(legacyGroup).Resources("nodes").RuleOrDie(),
rbac.NewRule("list", "watch", "create", "delete", "patch").Groups(legacyGroup).Resources("pods").RuleOrDie(),
rbac.NewRule("create").Groups(legacyGroup).Resources("pods/binding").RuleOrDie(),
Expand All @@ -95,6 +97,7 @@ func buildControllerRoles() ([]rbac.ClusterRole, []rbac.ClusterRoleBinding) {
Rules: []rbac.PolicyRule{
rbac.NewRule("get", "list", "watch", "update").Groups(extensionsGroup, appsGroup).Resources("deployments").RuleOrDie(),
rbac.NewRule("update").Groups(extensionsGroup, appsGroup).Resources("deployments/status").RuleOrDie(),
rbac.NewRule("update").Groups(extensionsGroup, appsGroup).Resources("deployments/finalizers").RuleOrDie(),
rbac.NewRule("get", "list", "watch", "create", "update", "patch", "delete").Groups(extensionsGroup).Resources("replicasets").RuleOrDie(),
// TODO: remove "update" once
// https://github.com/kubernetes/kubernetes/issues/36897 is resolved.
Expand Down Expand Up @@ -168,6 +171,7 @@ func buildControllerRoles() ([]rbac.ClusterRole, []rbac.ClusterRoleBinding) {
Rules: []rbac.PolicyRule{
rbac.NewRule("get", "list", "watch", "update").Groups(batchGroup).Resources("jobs").RuleOrDie(),
rbac.NewRule("update").Groups(batchGroup).Resources("jobs/status").RuleOrDie(),
rbac.NewRule("update").Groups(batchGroup).Resources("jobs/finalizers").RuleOrDie(),
rbac.NewRule("list", "watch", "create", "delete", "patch").Groups(legacyGroup).Resources("pods").RuleOrDie(),
eventsRule(),
},
Expand Down Expand Up @@ -225,6 +229,7 @@ func buildControllerRoles() ([]rbac.ClusterRole, []rbac.ClusterRoleBinding) {
Rules: []rbac.PolicyRule{
rbac.NewRule("get", "list", "watch", "update").Groups(extensionsGroup).Resources("replicasets").RuleOrDie(),
rbac.NewRule("update").Groups(extensionsGroup).Resources("replicasets/status").RuleOrDie(),
rbac.NewRule("update").Groups(extensionsGroup).Resources("replicasets/finalizers").RuleOrDie(),
rbac.NewRule("list", "watch", "patch", "create", "delete").Groups(legacyGroup).Resources("pods").RuleOrDie(),
eventsRule(),
},
Expand All @@ -235,6 +240,7 @@ func buildControllerRoles() ([]rbac.ClusterRole, []rbac.ClusterRoleBinding) {
// 1.0 controllers needed get, update, so without these old controllers break on new servers
rbac.NewRule("get", "list", "watch", "update").Groups(legacyGroup).Resources("replicationcontrollers").RuleOrDie(),
rbac.NewRule("update").Groups(legacyGroup).Resources("replicationcontrollers/status").RuleOrDie(),
rbac.NewRule("update").Groups(legacyGroup).Resources("replicationcontrollers/finalizers").RuleOrDie(),
rbac.NewRule("list", "watch", "patch", "create", "delete").Groups(legacyGroup).Resources("pods").RuleOrDie(),
eventsRule(),
},
Expand Down Expand Up @@ -278,6 +284,7 @@ func buildControllerRoles() ([]rbac.ClusterRole, []rbac.ClusterRoleBinding) {
rbac.NewRule("list", "watch").Groups(legacyGroup).Resources("pods").RuleOrDie(),
rbac.NewRule("get", "list", "watch").Groups(appsGroup).Resources("statefulsets").RuleOrDie(),
rbac.NewRule("update").Groups(appsGroup).Resources("statefulsets/status").RuleOrDie(),
rbac.NewRule("update").Groups(appsGroup).Resources("statefulsets/finalizers").RuleOrDie(),
rbac.NewRule("get", "create", "delete", "update", "patch").Groups(legacyGroup).Resources("pods").RuleOrDie(),
rbac.NewRule("get", "create", "delete", "update", "patch", "list", "watch").Groups(appsGroup).Resources("controllerrevisions").RuleOrDie(),
rbac.NewRule("get", "create").Groups(legacyGroup).Resources("persistentvolumeclaims").RuleOrDie(),
Expand Down
44 changes: 44 additions & 0 deletions plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/controller-roles.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,12 @@ items:
- cronjobs/status
verbs:
- update
- apiGroups:
- batch
resources:
- cronjobs/finalizers
verbs:
- update
- apiGroups:
- ""
resources:
Expand Down Expand Up @@ -165,6 +171,13 @@ items:
- daemonsets/status
verbs:
- update
- apiGroups:
- apps
- extensions
resources:
- daemonsets/finalizers
verbs:
- update
- apiGroups:
- ""
resources:
Expand Down Expand Up @@ -234,6 +247,13 @@ items:
- deployments/status
verbs:
- update
- apiGroups:
- apps
- extensions
resources:
- deployments/finalizers
verbs:
- update
- apiGroups:
- extensions
resources:
Expand Down Expand Up @@ -495,6 +515,12 @@ items:
- jobs/status
verbs:
- update
- apiGroups:
- batch
resources:
- jobs/finalizers
verbs:
- update
- apiGroups:
- ""
resources:
Expand Down Expand Up @@ -741,6 +767,12 @@ items:
- replicasets/status
verbs:
- update
- apiGroups:
- extensions
resources:
- replicasets/finalizers
verbs:
- update
- apiGroups:
- ""
resources:
Expand Down Expand Up @@ -784,6 +816,12 @@ items:
- replicationcontrollers/status
verbs:
- update
- apiGroups:
- ""
resources:
- replicationcontrollers/finalizers
verbs:
- update
- apiGroups:
- ""
resources:
Expand Down Expand Up @@ -958,6 +996,12 @@ items:
- statefulsets/status
verbs:
- update
- apiGroups:
- apps
resources:
- statefulsets/finalizers
verbs:
- update
- apiGroups:
- ""
resources:
Expand Down

0 comments on commit 0ed05f2

Please sign in to comment.