Merge pull request #98147 from deads2k/system-masters-delete

add check to gc_admission to allow super users to skip RESTMapping

plugin/pkg/admission/gc/BUILD

@@ -18,6 +18,7 @@ go_library(
         "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/admission:go_default_library",
+        "//staging/src/k8s.io/apiserver/pkg/authentication/user:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/authorization/authorizer:go_default_library",
     ],
 )
@@ -31,6 +32,7 @@ go_test(
         "//pkg/kubeapiserver/admission:go_default_library",
         "//staging/src/k8s.io/api/apps/v1:go_default_library",
         "//staging/src/k8s.io/api/core/v1:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/api/meta:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",

plugin/pkg/admission/gc/gc_admission.go

@@ -28,6 +28,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apiserver/pkg/admission"
+	"k8s.io/apiserver/pkg/authentication/user"
 	"k8s.io/apiserver/pkg/authorization/authorizer"
 )
 
@@ -122,6 +123,18 @@ func (a *gcPermissionsEnforcement) Validate(ctx context.Context, attributes admi
 	// true. If so, only allows the change if the user has delete permission of
 	// the _OWNER_
 	newBlockingRefs := newBlockingOwnerDeletionRefs(attributes.GetObject(), attributes.GetOldObject())
+	if len(newBlockingRefs) == 0 {
+		return nil
+	}
+
+	// There can be a case where a restMapper tries to hit discovery endpoints and times out if the network is inaccessible.
+	// This can prevent creating the pod to run the network to be able to do discovery and it appears as a timeout, not a rejection.
+	// Because the timeout is wrapper on admission/request, we can run a single check to see if the user can finalize any
+	// possible resource.
+	if decision, _, _ := a.authorizer.Authorize(ctx, finalizeAnythingRecord(attributes.GetUserInfo())); decision == authorizer.DecisionAllow {
+		return nil
+	}
+
 	for _, ref := range newBlockingRefs {
 		records, err := a.ownerRefToDeleteAttributeRecords(ref, attributes)
 		if err != nil {
@@ -173,6 +186,20 @@ func isChangingOwnerReference(newObj, oldObj runtime.Object) bool {
 	return false
 }
 
+func finalizeAnythingRecord(userInfo user.Info) authorizer.AttributesRecord {
+	return authorizer.AttributesRecord{
+		User:            userInfo,
+		Verb:            "update",
+		APIGroup:        "*",
+		APIVersion:      "*",
+		Resource:        "*",
+		Subresource:     "finalizers",
+		Name:            "*",
+		ResourceRequest: true,
+		Path:            "",
+	}
+}
+
 // Translates ref to a DeleteAttribute deleting the object referred by the ref.
 // OwnerReference only records the object kind, which might map to multiple
 // resources, so multiple DeleteAttribute might be returned.

plugin/pkg/admission/gc/gc_admission_test.go

@@ -24,6 +24,7 @@ import (
 
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -50,6 +51,9 @@ func (fakeAuthorizer) Authorize(ctx context.Context, a authorizer.Attributes) (a
 		if a.GetVerb() == "update" && a.GetSubresource() == "finalizers" {
 			return authorizer.DecisionNoOpinion, "", nil
 		}
+		if a.GetAPIGroup() == "*" && a.GetResource() == "*" { // this user does not have full rights
+			return authorizer.DecisionNoOpinion, "", nil
+		}
 		return authorizer.DecisionAllow, "", nil
 	}
 
@@ -60,6 +64,9 @@ func (fakeAuthorizer) Authorize(ctx context.Context, a authorizer.Attributes) (a
 		if a.GetVerb() == "update" && a.GetResource() == "pods" && a.GetSubresource() == "finalizers" {
 			return authorizer.DecisionNoOpinion, "", nil
 		}
+		if a.GetAPIGroup() == "*" && a.GetResource() == "*" { // this user does not have full rights
+			return authorizer.DecisionNoOpinion, "", nil
+		}
 		return authorizer.DecisionAllow, "", nil
 	}
 
@@ -70,6 +77,9 @@ func (fakeAuthorizer) Authorize(ctx context.Context, a authorizer.Attributes) (a
 		if a.GetVerb() == "update" && a.GetResource() == "replicationcontrollers" && a.GetSubresource() == "finalizers" {
 			return authorizer.DecisionNoOpinion, "", nil
 		}
+		if a.GetAPIGroup() == "*" && a.GetResource() == "*" { // this user does not have full rights
+			return authorizer.DecisionNoOpinion, "", nil
+		}
 		return authorizer.DecisionAllow, "", nil
 	}
 
@@ -80,8 +90,12 @@ func (fakeAuthorizer) Authorize(ctx context.Context, a authorizer.Attributes) (a
 		if a.GetVerb() == "update" && a.GetResource() == "nodes" && a.GetSubresource() == "finalizers" {
 			return authorizer.DecisionNoOpinion, "", nil
 		}
+		if a.GetAPIGroup() == "*" && a.GetResource() == "*" { // this user does not have full rights
+			return authorizer.DecisionNoOpinion, "", nil
+		}
 		return authorizer.DecisionAllow, "", nil
 	}
+
 	return authorizer.DecisionAllow, "", nil
 }
 
@@ -134,6 +148,39 @@ func newGCPermissionsEnforcement() (*gcPermissionsEnforcement, error) {
 	return gcAdmit, nil
 }
 
+type neverReturningRESTMapper struct{}
+
+var _ meta.RESTMapper = &neverReturningRESTMapper{}
+
+func (r *neverReturningRESTMapper) KindFor(resource schema.GroupVersionResource) (schema.GroupVersionKind, error) {
+	// this ok because if the test works, this method should never be called.
+	panic("test failed")
+}
+func (r *neverReturningRESTMapper) KindsFor(resource schema.GroupVersionResource) ([]schema.GroupVersionKind, error) {
+	// this ok because if the test works, this method should never be called.
+	panic("test failed")
+}
+func (r *neverReturningRESTMapper) ResourceFor(input schema.GroupVersionResource) (schema.GroupVersionResource, error) {
+	// this ok because if the test works, this method should never be called.
+	panic("test failed")
+}
+func (r *neverReturningRESTMapper) ResourcesFor(input schema.GroupVersionResource) ([]schema.GroupVersionResource, error) {
+	// this ok because if the test works, this method should never be called.
+	panic("test failed")
+}
+func (r *neverReturningRESTMapper) RESTMapping(gk schema.GroupKind, versions ...string) (*meta.RESTMapping, error) {
+	// this ok because if the test works, this method should never be called.
+	panic("test failed")
+}
+func (r *neverReturningRESTMapper) RESTMappings(gk schema.GroupKind, versions ...string) ([]*meta.RESTMapping, error) {
+	// this ok because if the test works, this method should never be called.
+	panic("test failed")
+}
+func (r *neverReturningRESTMapper) ResourceSingularizer(resource string) (singular string, err error) {
+	// this ok because if the test works, this method should never be called.
+	panic("test failed")
+}
+
 func TestGCAdmission(t *testing.T) {
 	expectNoError := func(err error) bool {
 		return err == nil
@@ -420,6 +467,7 @@ func TestBlockOwnerDeletionAdmission(t *testing.T) {
 		subresource        string
 		oldObj             runtime.Object
 		newObj             runtime.Object
+		restMapperOverride meta.RESTMapper
 
 		checkError func(error) bool
 	}{
@@ -445,6 +493,14 @@ func TestBlockOwnerDeletionAdmission(t *testing.T) {
 			newObj:     podWithOwnerRefs(blockRC1, blockRC2, blockNode),
 			checkError: expectNoError,
 		},
+		{
+			name:               "super-user, create, some ownerReferences have blockOwnerDeletion=true, hangingRESTMapper",
+			username:           "super",
+			resource:           api.SchemeGroupVersion.WithResource("pods"),
+			newObj:             podWithOwnerRefs(blockRC1, blockRC2, blockNode),
+			restMapperOverride: &neverReturningRESTMapper{},
+			checkError:         expectNoError,
+		},
 		{
 			name:       "non-rc-deleter, create, no ownerReferences",
 			username:   "non-rc-deleter",
@@ -601,12 +657,17 @@ func TestBlockOwnerDeletionAdmission(t *testing.T) {
 			checkError: expectCantSetBlockOwnerDeletionError,
 		},
 	}
-	gcAdmit, err := newGCPermissionsEnforcement()
-	if err != nil {
-		t.Error(err)
-	}
 
 	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			gcAdmit, err := newGCPermissionsEnforcement()
+			if err != nil {
+				t.Fatal(err)
+			}
+			if tc.restMapperOverride != nil {
+				gcAdmit.restMapper = tc.restMapperOverride
+			}
+
 			operation := admission.Create
 			var options runtime.Object = &metav1.CreateOptions{}
 			if tc.oldObj != nil {
@@ -616,9 +677,10 @@ func TestBlockOwnerDeletionAdmission(t *testing.T) {
 			user := &user.DefaultInfo{Name: tc.username}
 			attributes := admission.NewAttributesRecord(tc.newObj, tc.oldObj, schema.GroupVersionKind{}, metav1.NamespaceDefault, "foo", tc.resource, tc.subresource, operation, options, false, user)
 
-		err := gcAdmit.Validate(context.TODO(), attributes, nil)
+			err = gcAdmit.Validate(context.TODO(), attributes, nil)
 			if !tc.checkError(err) {
-			t.Errorf("%v: unexpected err: %v", tc.name, err)
+				t.Fatal(err)
-		}
+			}
+		})
 	}
 }