From ff6684d90f5fd304bba1795b2bcac1df020c2141 Mon Sep 17 00:00:00 2001 From: David Eads Date: Mon, 18 Jan 2021 15:06:44 -0500 Subject: [PATCH] add check to gc_admission to allow super users to skip RESTMapping --- plugin/pkg/admission/gc/BUILD | 2 + plugin/pkg/admission/gc/gc_admission.go | 27 +++++ plugin/pkg/admission/gc/gc_admission_test.go | 106 +++++++++++++++---- 3 files changed, 113 insertions(+), 22 deletions(-) diff --git a/plugin/pkg/admission/gc/BUILD b/plugin/pkg/admission/gc/BUILD index 842479b5a9a..80f152b095b 100644 --- a/plugin/pkg/admission/gc/BUILD +++ b/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", diff --git a/plugin/pkg/admission/gc/gc_admission.go b/plugin/pkg/admission/gc/gc_admission.go index afcded01dee..db0f3c12c10 100644 --- a/plugin/pkg/admission/gc/gc_admission.go +++ b/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. diff --git a/plugin/pkg/admission/gc/gc_admission_test.go b/plugin/pkg/admission/gc/gc_admission_test.go index 27002f84d4d..30a174efb10 100644 --- a/plugin/pkg/admission/gc/gc_admission_test.go +++ b/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 @@ -414,12 +461,13 @@ func TestBlockOwnerDeletionAdmission(t *testing.T) { 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 - username string - resource schema.GroupVersionResource - subresource string - oldObj runtime.Object - newObj runtime.Object + name string + username string + resource schema.GroupVersionResource + 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,24 +657,30 @@ func TestBlockOwnerDeletionAdmission(t *testing.T) { checkError: expectCantSetBlockOwnerDeletionError, }, } - gcAdmit, err := newGCPermissionsEnforcement() - if err != nil { - t.Error(err) - } for _, tc := range tests { - operation := admission.Create - var options runtime.Object = &metav1.CreateOptions{} - if tc.oldObj != nil { - operation = admission.Update - options = &metav1.UpdateOptions{} - } - 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) + 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 + } - err := gcAdmit.Validate(context.TODO(), attributes, nil) - if !tc.checkError(err) { - t.Errorf("%v: unexpected err: %v", tc.name, err) - } + operation := admission.Create + var options runtime.Object = &metav1.CreateOptions{} + if tc.oldObj != nil { + operation = admission.Update + options = &metav1.UpdateOptions{} + } + 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) + if !tc.checkError(err) { + t.Fatal(err) + } + }) } }