Bug 1731222: unwarranted SARs on foreground GC of objects with owner …

…refs copy/use kubernetes/kubernetes#90534 for IsMutationForGC check
openshift · May 1, 2020 · 418f0ce · 418f0ce
1 parent 7535fa2
commit 418f0ce
Show file tree

Hide file tree

Showing 4 changed files with 259 additions and 2 deletions.
diff --git a/pkg/build/apiserver/admission/strategyrestrictions/admission.go b/pkg/build/apiserver/admission/strategyrestrictions/admission.go
@@ -17,7 +17,6 @@ import (
 	authorizationclient "k8s.io/client-go/kubernetes/typed/authorization/v1"
 	"k8s.io/client-go/rest"
 	kapihelper "k8s.io/kubernetes/pkg/apis/core/helper"
-	rbacregistry "k8s.io/kubernetes/pkg/registry/rbac"
 
 	"github.com/openshift/api/build"
 	buildclient "github.com/openshift/client-go/build/clientset/versioned"
@@ -27,6 +26,7 @@ import (
 	"github.com/openshift/openshift-apiserver/pkg/bootstrappolicy"
 	buildapi "github.com/openshift/openshift-apiserver/pkg/build/apis/build"
 	buildv1helpers "github.com/openshift/openshift-apiserver/pkg/build/apis/build/v1"
+	rbacregistry "github.com/openshift/openshift-apiserver/pkg/registry/rbac"
 )
 
 // provides a way to convert between internal and external.  Please don't used this to serialize and deserialize

diff --git a/pkg/registry/rbac/helpers.go b/pkg/registry/rbac/helpers.go
@@ -0,0 +1,55 @@
+/*
+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 (
+	"reflect"
+
+	"k8s.io/apimachinery/pkg/api/meta"
+	"k8s.io/apimachinery/pkg/conversion"
+	"k8s.io/apimachinery/pkg/runtime"
+)
+
+/*
+When https://github.com/kubernetes/kubernetes/pull/90534 is in a version of k8s pulled in by openshift-apiserver
+we can remove this entire package in lieu of using k8s again
+*/
+
+// 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 {
+	if old == nil || reflect.ValueOf(old).IsNil() {
+		return false
+	}
+
+	// make a copy of the newObj so that we can stomp for comparison
+	copied := obj.DeepCopyObject()
+	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())
+	copiedMeta.SetManagedFields(oldMeta.GetManagedFields())
+
+	return equalities.DeepEqual(copied, old)
+}
diff --git a/pkg/registry/rbac/helpers_test.go b/pkg/registry/rbac/helpers_test.go
@@ -0,0 +1,202 @@
+/*
+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 (
+	"reflect"
+	"testing"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
+	kapi "k8s.io/kubernetes/pkg/apis/core"
+	kapihelper "k8s.io/kubernetes/pkg/apis/core/helper"
+
+	fuzz "github.com/google/gofuzz"
+)
+
+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: "different managedFields",
+			obj: func() runtime.Object {
+				return newPod()
+			},
+			old: func() runtime.Object {
+				obj := newPod()
+				obj.ManagedFields = []metav1.ManagedFieldsEntry{
+					{
+						Manager: "manager",
+					},
+				}
+				return obj
+			},
+			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,
+		},
+		{
+			name: "and nil",
+			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 (*kapi.Pod)(nil)
+			},
+			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)
+		}
+	}
+}
+
+func TestNewMetadataFields(t *testing.T) {
+	f := fuzz.New().NilChance(0.0).NumElements(1, 1)
+	for i := 0; i < 100; i++ {
+		objMeta := metav1.ObjectMeta{}
+		f.Fuzz(&objMeta)
+		objMeta.Name = ""
+		objMeta.GenerateName = ""
+		objMeta.Namespace = ""
+		objMeta.SelfLink = ""
+		objMeta.UID = types.UID("")
+		objMeta.ResourceVersion = ""
+		objMeta.Generation = 0
+		objMeta.CreationTimestamp = metav1.Time{}
+		objMeta.DeletionTimestamp = nil
+		objMeta.DeletionGracePeriodSeconds = nil
+		objMeta.Labels = nil
+		objMeta.Annotations = nil
+		objMeta.OwnerReferences = nil
+		objMeta.Finalizers = nil
+		objMeta.ClusterName = ""
+		objMeta.ManagedFields = nil
+
+		if !reflect.DeepEqual(metav1.ObjectMeta{}, objMeta) {
+			t.Fatalf(`A new field was introduced in ObjectMeta, add the field to
+IsOnlyMutatingGCFields if necessary, and update this test:
+%#v`, objMeta)
+		}
+	}
+}
diff --git a/pkg/template/apiserver/registry/templateinstance/strategy.go b/pkg/template/apiserver/registry/templateinstance/strategy.go
@@ -15,9 +15,9 @@ import (
 	authorizationclient "k8s.io/client-go/kubernetes/typed/authorization/v1"
 	"k8s.io/kubernetes/pkg/api/legacyscheme"
 	kapihelper "k8s.io/kubernetes/pkg/apis/core/helper"
-	rbacregistry "k8s.io/kubernetes/pkg/registry/rbac"
 
 	"github.com/openshift/library-go/pkg/authorization/authorizationutil"
+	rbacregistry "github.com/openshift/openshift-apiserver/pkg/registry/rbac"
 	templateapi "github.com/openshift/openshift-apiserver/pkg/template/apis/template"
 	"github.com/openshift/openshift-apiserver/pkg/template/apis/template/validation"
 )