Merge pull request #49902 from enj/enj/i/gc_delete_rbac

pkg/registry/rbac/BUILD

@@ -5,14 +5,22 @@ licenses(["notice"])
 load(
     "@io_bazel_rules_go//go:def.bzl",
     "go_library",
+    "go_test",
 )
 
 go_library(
     name = "go_default_library",
-    srcs = ["escalation_check.go"],
+    srcs = [
+        "escalation_check.go",
+        "helpers.go",
+    ],
     tags = ["automanaged"],
     deps = [
+        "//pkg/api:go_default_library",
         "//pkg/apis/rbac:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/api/meta:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/conversion:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/runtime:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/authentication/user:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/authorization/authorizer:go_default_library",
@@ -41,3 +49,16 @@ filegroup(
     ],
     tags = ["automanaged"],
 )
+
+go_test(
+    name = "go_default_test",
+    srcs = ["helpers_test.go"],
+    library = ":go_default_library",
+    tags = ["automanaged"],
+    deps = [
+        "//pkg/api:go_default_library",
+        "//pkg/api/helper:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
+    ],
+)

pkg/registry/rbac/clusterrole/policybased/BUILD

@@ -12,6 +12,7 @@ go_library(
     srcs = ["storage.go"],
     tags = ["automanaged"],
     deps = [
+        "//pkg/api/helper:go_default_library",
         "//pkg/apis/rbac:go_default_library",
         "//pkg/registry/rbac:go_default_library",
         "//pkg/registry/rbac/validation:go_default_library",

pkg/registry/rbac/clusterrole/policybased/storage.go

@@ -22,6 +22,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/registry/rest"
+	kapihelper "k8s.io/kubernetes/pkg/api/helper"
 	"k8s.io/kubernetes/pkg/apis/rbac"
 	rbacregistry "k8s.io/kubernetes/pkg/registry/rbac"
 	rbacregistryvalidation "k8s.io/kubernetes/pkg/registry/rbac/validation"
@@ -60,6 +61,11 @@ func (s *Storage) Update(ctx genericapirequest.Context, name string, obj rest.Up
 	nonEscalatingInfo := rest.WrapUpdatedObjectInfo(obj, func(ctx genericapirequest.Context, obj runtime.Object, oldObj runtime.Object) (runtime.Object, error) {
 		clusterRole := obj.(*rbac.ClusterRole)
 
+		// if we're only mutating fields needed for the GC to eventually delete this obj, return
+		if rbacregistry.IsOnlyMutatingGCFields(obj, oldObj, kapihelper.Semantic) {
+			return obj, nil
+		}
+
 		rules := clusterRole.Rules
 		if err := rbacregistryvalidation.ConfirmNoEscalation(ctx, s.ruleResolver, rules); err != nil {
 			return nil, errors.NewForbidden(groupResource, clusterRole.Name, err)

pkg/registry/rbac/clusterrolebinding/policybased/BUILD

@@ -12,6 +12,7 @@ go_library(
     srcs = ["storage.go"],
     tags = ["automanaged"],
     deps = [
+        "//pkg/api/helper:go_default_library",
         "//pkg/apis/rbac:go_default_library",
         "//pkg/registry/rbac:go_default_library",
         "//pkg/registry/rbac/validation:go_default_library",

pkg/registry/rbac/clusterrolebinding/policybased/storage.go

@@ -24,6 +24,7 @@ import (
 	"k8s.io/apiserver/pkg/authorization/authorizer"
 	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/registry/rest"
+	kapihelper "k8s.io/kubernetes/pkg/api/helper"
 	"k8s.io/kubernetes/pkg/apis/rbac"
 	rbacregistry "k8s.io/kubernetes/pkg/registry/rbac"
 	rbacregistryvalidation "k8s.io/kubernetes/pkg/registry/rbac/validation"
@@ -71,6 +72,11 @@ func (s *Storage) Update(ctx genericapirequest.Context, name string, obj rest.Up
 	nonEscalatingInfo := rest.WrapUpdatedObjectInfo(obj, func(ctx genericapirequest.Context, obj runtime.Object, oldObj runtime.Object) (runtime.Object, error) {
 		clusterRoleBinding := obj.(*rbac.ClusterRoleBinding)
 
+		// if we're only mutating fields needed for the GC to eventually delete this obj, return
+		if rbacregistry.IsOnlyMutatingGCFields(obj, oldObj, kapihelper.Semantic) {
+			return obj, nil
+		}
+
 		// if we're explicitly authorized to bind this clusterrole, return
 		if rbacregistry.BindingAuthorized(ctx, clusterRoleBinding.RoleRef, metav1.NamespaceNone, s.authorizer) {
 			return obj, nil

pkg/registry/rbac/helpers.go

+/*
+Copyright 2017 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package rbac
+
+import (
+	"k8s.io/apimachinery/pkg/api/meta"
+	"k8s.io/apimachinery/pkg/conversion"
+	"k8s.io/apimachinery/pkg/runtime"
+	kapi "k8s.io/kubernetes/pkg/api"
+)
+
+// IsOnlyMutatingGCFields checks finalizers and ownerrefs which GC manipulates
+// and indicates that only those fields are changing
+func IsOnlyMutatingGCFields(obj, old runtime.Object, equalities conversion.Equalities) bool {
+	// make a copy of the newObj so that we can stomp for comparison
+	copied, err := kapi.Scheme.Copy(obj)
+	if err != nil {
+		// if we couldn't copy, don't fail, just make it do the check
+		return false
+	}
+	copiedMeta, err := meta.Accessor(copied)
+	if err != nil {
+		return false
+	}
+	oldMeta, err := meta.Accessor(old)
+	if err != nil {
+		return false
+	}
+	copiedMeta.SetOwnerReferences(oldMeta.GetOwnerReferences())
+	copiedMeta.SetFinalizers(oldMeta.GetFinalizers())
+	copiedMeta.SetSelfLink(oldMeta.GetSelfLink())
+
+	return equalities.DeepEqual(copied, old)
+}

pkg/registry/rbac/helpers_test.go

+/*
+Copyright 2017 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package rbac
+
+import (
+	"testing"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	kapi "k8s.io/kubernetes/pkg/api"
+	kapihelper "k8s.io/kubernetes/pkg/api/helper"
+)
+
+func newPod() *kapi.Pod {
+	return &kapi.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Annotations:     map[string]string{},
+			Name:            "foo",
+			OwnerReferences: []metav1.OwnerReference{},
+		},
+	}
+
+}
+
+func TestIsOnlyMutatingGCFields(t *testing.T) {
+	tests := []struct {
+		name     string
+		obj      func() runtime.Object
+		old      func() runtime.Object
+		expected bool
+	}{
+		{
+			name: "same",
+			obj: func() runtime.Object {
+				return newPod()
+			},
+			old: func() runtime.Object {
+				return newPod()
+			},
+			expected: true,
+		},
+		{
+			name: "only annotations",
+			obj: func() runtime.Object {
+				obj := newPod()
+				obj.Annotations["foo"] = "bar"
+				return obj
+			},
+			old: func() runtime.Object {
+				return newPod()
+			},
+			expected: false,
+		},
+		{
+			name: "only other",
+			obj: func() runtime.Object {
+				obj := newPod()
+				obj.Spec.RestartPolicy = kapi.RestartPolicyAlways
+				return obj
+			},
+			old: func() runtime.Object {
+				return newPod()
+			},
+			expected: false,
+		},
+		{
+			name: "only ownerRef",
+			obj: func() runtime.Object {
+				obj := newPod()
+				obj.OwnerReferences = append(obj.OwnerReferences, metav1.OwnerReference{Name: "foo"})
+				return obj
+			},
+			old: func() runtime.Object {
+				return newPod()
+			},
+			expected: true,
+		},
+		{
+			name: "ownerRef and finalizer",
+			obj: func() runtime.Object {
+				obj := newPod()
+				obj.OwnerReferences = append(obj.OwnerReferences, metav1.OwnerReference{Name: "foo"})
+				obj.Finalizers = []string{"final"}
+				return obj
+			},
+			old: func() runtime.Object {
+				return newPod()
+			},
+			expected: true,
+		},
+		{
+			name: "and annotations",
+			obj: func() runtime.Object {
+				obj := newPod()
+				obj.OwnerReferences = append(obj.OwnerReferences, metav1.OwnerReference{Name: "foo"})
+				obj.Annotations["foo"] = "bar"
+				return obj
+			},
+			old: func() runtime.Object {
+				return newPod()
+			},
+			expected: false,
+		},
+		{
+			name: "and other",
+			obj: func() runtime.Object {
+				obj := newPod()
+				obj.OwnerReferences = append(obj.OwnerReferences, metav1.OwnerReference{Name: "foo"})
+				obj.Spec.RestartPolicy = kapi.RestartPolicyAlways
+				return obj
+			},
+			old: func() runtime.Object {
+				return newPod()
+			},
+			expected: false,
+		},
+	}
+
+	for _, tc := range tests {
+		actual := IsOnlyMutatingGCFields(tc.obj(), tc.old(), kapihelper.Semantic)
+		if tc.expected != actual {
+			t.Errorf("%s: expected %v, got %v", tc.name, tc.expected, actual)
+		}
+	}
+}

pkg/registry/rbac/role/policybased/BUILD

@@ -12,6 +12,7 @@ go_library(
     srcs = ["storage.go"],
     tags = ["automanaged"],
     deps = [
+        "//pkg/api/helper:go_default_library",
         "//pkg/apis/rbac:go_default_library",
         "//pkg/registry/rbac:go_default_library",
         "//pkg/registry/rbac/validation:go_default_library",

pkg/registry/rbac/role/policybased/storage.go

@@ -22,6 +22,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/registry/rest"
+	kapihelper "k8s.io/kubernetes/pkg/api/helper"
 	"k8s.io/kubernetes/pkg/apis/rbac"
 	rbacregistry "k8s.io/kubernetes/pkg/registry/rbac"
 	rbacregistryvalidation "k8s.io/kubernetes/pkg/registry/rbac/validation"
@@ -60,6 +61,11 @@ func (s *Storage) Update(ctx genericapirequest.Context, name string, obj rest.Up
 	nonEscalatingInfo := rest.WrapUpdatedObjectInfo(obj, func(ctx genericapirequest.Context, obj runtime.Object, oldObj runtime.Object) (runtime.Object, error) {
 		role := obj.(*rbac.Role)
 
+		// if we're only mutating fields needed for the GC to eventually delete this obj, return
+		if rbacregistry.IsOnlyMutatingGCFields(obj, oldObj, kapihelper.Semantic) {
+			return obj, nil
+		}
+
 		rules := role.Rules
 		if err := rbacregistryvalidation.ConfirmNoEscalation(ctx, s.ruleResolver, rules); err != nil {
 			return nil, errors.NewForbidden(groupResource, role.Name, err)

pkg/registry/rbac/rolebinding/policybased/BUILD

@@ -12,6 +12,7 @@ go_library(
     srcs = ["storage.go"],
     tags = ["automanaged"],
     deps = [
+        "//pkg/api/helper:go_default_library",
         "//pkg/apis/rbac:go_default_library",
         "//pkg/registry/rbac:go_default_library",
         "//pkg/registry/rbac/validation:go_default_library",

pkg/registry/rbac/rolebinding/policybased/storage.go

@@ -23,6 +23,7 @@ import (
 	"k8s.io/apiserver/pkg/authorization/authorizer"
 	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/registry/rest"
+	kapihelper "k8s.io/kubernetes/pkg/api/helper"
 	"k8s.io/kubernetes/pkg/apis/rbac"
 	rbacregistry "k8s.io/kubernetes/pkg/registry/rbac"
 	rbacregistryvalidation "k8s.io/kubernetes/pkg/registry/rbac/validation"
@@ -84,6 +85,11 @@ func (s *Storage) Update(ctx genericapirequest.Context, name string, obj rest.Up
 
 		roleBinding := obj.(*rbac.RoleBinding)
 
+		// if we're only mutating fields needed for the GC to eventually delete this obj, return
+		if rbacregistry.IsOnlyMutatingGCFields(obj, oldObj, kapihelper.Semantic) {
+			return obj, nil
+		}
+
 		// if we're explicitly authorized to bind this role, return
 		if rbacregistry.BindingAuthorized(ctx, roleBinding.RoleRef, namespace, s.authorizer) {
 			return obj, nil