From 9d7a8df5eea3a6f06f9023fa191fb031e2fc483d Mon Sep 17 00:00:00 2001 From: Chao Xu Date: Fri, 24 Mar 2017 18:13:57 -0700 Subject: [PATCH] add gc admission plugin that prevents user who doesn't have delete permission of the owner from setting blockOwnerDeletion --- cmd/kube-apiserver/app/server.go | 4 +- .../cmd/federation-apiserver/app/server.go | 2 +- pkg/kubeapiserver/admission/BUILD | 1 + pkg/kubeapiserver/admission/init_test.go | 4 +- pkg/kubeapiserver/admission/initializer.go | 14 +- plugin/pkg/admission/gc/BUILD | 4 + plugin/pkg/admission/gc/gc_admission.go | 128 ++++- plugin/pkg/admission/gc/gc_admission_test.go | 471 ++++++++++++++---- .../admission/limitranger/admission_test.go | 2 +- .../namespace/autoprovision/admission_test.go | 2 +- .../namespace/exists/admission_test.go | 2 +- .../namespace/lifecycle/admission_test.go | 2 +- .../podnodeselector/admission_test.go | 2 +- .../admission_test.go | 2 +- 14 files changed, 520 insertions(+), 120 deletions(-) diff --git a/cmd/kube-apiserver/app/server.go b/cmd/kube-apiserver/app/server.go index 34088e888d6..16d05061210 100644 --- a/cmd/kube-apiserver/app/server.go +++ b/cmd/kube-apiserver/app/server.go @@ -384,7 +384,9 @@ func BuildAdmission(s *options.ServerRunOptions, plugins *admission.Plugins, cli glog.Fatalf("Error reading from cloud configuration file %s: %#v", s.CloudProvider.CloudConfigFile, err) } } - pluginInitializer := kubeapiserveradmission.NewPluginInitializer(client, sharedInformers, apiAuthorizer, cloudConfig) + // TODO: use a dynamic restmapper. See https://github.com/kubernetes/kubernetes/pull/42615. + restMapper := api.Registry.RESTMapper() + pluginInitializer := kubeapiserveradmission.NewPluginInitializer(client, sharedInformers, apiAuthorizer, cloudConfig, restMapper) admissionConfigProvider, err := admission.ReadAdmissionConfiguration(admissionControlPluginNames, s.GenericServerRunOptions.AdmissionControlConfigFile) if err != nil { return nil, fmt.Errorf("failed to read plugin config: %v", err) diff --git a/federation/cmd/federation-apiserver/app/server.go b/federation/cmd/federation-apiserver/app/server.go index 29b2268191e..9eee022a25f 100644 --- a/federation/cmd/federation-apiserver/app/server.go +++ b/federation/cmd/federation-apiserver/app/server.go @@ -194,7 +194,7 @@ func NonBlockingRun(s *options.ServerRunOptions, stopCh <-chan struct{}) error { glog.Fatalf("Error reading from cloud configuration file %s: %#v", s.CloudProvider.CloudConfigFile, err) } } - pluginInitializer := kubeapiserveradmission.NewPluginInitializer(client, sharedInformers, apiAuthorizer, cloudConfig) + pluginInitializer := kubeapiserveradmission.NewPluginInitializer(client, sharedInformers, apiAuthorizer, cloudConfig, nil) admissionConfigProvider, err := admission.ReadAdmissionConfiguration(admissionControlPluginNames, s.GenericServerRunOptions.AdmissionControlConfigFile) if err != nil { return fmt.Errorf("failed to read plugin config: %v", err) diff --git a/pkg/kubeapiserver/admission/BUILD b/pkg/kubeapiserver/admission/BUILD index d7279ca089a..c4a684760e2 100644 --- a/pkg/kubeapiserver/admission/BUILD +++ b/pkg/kubeapiserver/admission/BUILD @@ -29,6 +29,7 @@ go_library( deps = [ "//pkg/client/clientset_generated/internalclientset:go_default_library", "//pkg/client/informers/informers_generated/internalversion:go_default_library", + "//vendor:k8s.io/apimachinery/pkg/api/meta", "//vendor:k8s.io/apiserver/pkg/admission", "//vendor:k8s.io/apiserver/pkg/authorization/authorizer", ], diff --git a/pkg/kubeapiserver/admission/init_test.go b/pkg/kubeapiserver/admission/init_test.go index fd584385c03..7491413e2a0 100644 --- a/pkg/kubeapiserver/admission/init_test.go +++ b/pkg/kubeapiserver/admission/init_test.go @@ -51,7 +51,7 @@ var _ WantsAuthorizer = &WantAuthorizerAdmission{} // TestWantsAuthorizer ensures that the authorizer is injected when the WantsAuthorizer // interface is implemented. func TestWantsAuthorizer(t *testing.T) { - initializer := NewPluginInitializer(nil, nil, &TestAuthorizer{}, nil) + initializer := NewPluginInitializer(nil, nil, &TestAuthorizer{}, nil, nil) wantAuthorizerAdmission := &WantAuthorizerAdmission{} initializer.Initialize(wantAuthorizerAdmission) if wantAuthorizerAdmission.auth == nil { @@ -73,7 +73,7 @@ func (self *WantsCloudConfigAdmissionPlugin) Validate() error func TestCloudConfigAdmissionPlugin(t *testing.T) { cloudConfig := []byte("cloud-configuration") - initializer := NewPluginInitializer(nil, nil, &TestAuthorizer{}, cloudConfig) + initializer := NewPluginInitializer(nil, nil, &TestAuthorizer{}, cloudConfig, nil) wantsCloudConfigAdmission := &WantsCloudConfigAdmissionPlugin{} initializer.Initialize(wantsCloudConfigAdmission) diff --git a/pkg/kubeapiserver/admission/initializer.go b/pkg/kubeapiserver/admission/initializer.go index 4e591ef0d6c..1ca982d09cf 100644 --- a/pkg/kubeapiserver/admission/initializer.go +++ b/pkg/kubeapiserver/admission/initializer.go @@ -17,6 +17,7 @@ limitations under the License. package admission import ( + "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apiserver/pkg/admission" "k8s.io/apiserver/pkg/authorization/authorizer" "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset" @@ -48,22 +49,29 @@ type WantsCloudConfig interface { SetCloudConfig([]byte) } +// WantsRESTMapper defines a function which sets RESTMapper for admission plugins that need it. +type WantsRESTMapper interface { + SetRESTMapper(meta.RESTMapper) +} + type pluginInitializer struct { internalClient internalclientset.Interface informers informers.SharedInformerFactory authorizer authorizer.Authorizer cloudConfig []byte + restMapper meta.RESTMapper } var _ admission.PluginInitializer = pluginInitializer{} // NewPluginInitializer constructs new instance of PluginInitializer -func NewPluginInitializer(internalClient internalclientset.Interface, sharedInformers informers.SharedInformerFactory, authz authorizer.Authorizer, cloudConfig []byte) admission.PluginInitializer { +func NewPluginInitializer(internalClient internalclientset.Interface, sharedInformers informers.SharedInformerFactory, authz authorizer.Authorizer, cloudConfig []byte, restMapper meta.RESTMapper) admission.PluginInitializer { return pluginInitializer{ internalClient: internalClient, informers: sharedInformers, authorizer: authz, cloudConfig: cloudConfig, + restMapper: restMapper, } } @@ -85,4 +93,8 @@ func (i pluginInitializer) Initialize(plugin admission.Interface) { if wants, ok := plugin.(WantsCloudConfig); ok { wants.SetCloudConfig(i.cloudConfig) } + + if wants, ok := plugin.(WantsRESTMapper); ok { + wants.SetRESTMapper(i.restMapper) + } } diff --git a/plugin/pkg/admission/gc/BUILD b/plugin/pkg/admission/gc/BUILD index 0a686333d75..457587aa285 100644 --- a/plugin/pkg/admission/gc/BUILD +++ b/plugin/pkg/admission/gc/BUILD @@ -16,7 +16,10 @@ go_library( "//pkg/kubeapiserver/admission:go_default_library", "//vendor:k8s.io/apimachinery/pkg/api/equality", "//vendor:k8s.io/apimachinery/pkg/api/meta", + "//vendor:k8s.io/apimachinery/pkg/apis/meta/v1", "//vendor:k8s.io/apimachinery/pkg/runtime", + "//vendor:k8s.io/apimachinery/pkg/runtime/schema", + "//vendor:k8s.io/apimachinery/pkg/types", "//vendor:k8s.io/apiserver/pkg/admission", "//vendor:k8s.io/apiserver/pkg/authorization/authorizer", ], @@ -29,6 +32,7 @@ go_test( tags = ["automanaged"], deps = [ "//pkg/api:go_default_library", + "//pkg/kubeapiserver/admission:go_default_library", "//vendor:k8s.io/apimachinery/pkg/apis/meta/v1", "//vendor:k8s.io/apimachinery/pkg/runtime", "//vendor:k8s.io/apimachinery/pkg/runtime/schema", diff --git a/plugin/pkg/admission/gc/gc_admission.go b/plugin/pkg/admission/gc/gc_admission.go index eddb542d24e..80e53109a74 100644 --- a/plugin/pkg/admission/gc/gc_admission.go +++ b/plugin/pkg/admission/gc/gc_admission.go @@ -22,7 +22,10 @@ import ( apiequality "k8s.io/apimachinery/pkg/api/equality" "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" + "k8s.io/apimachinery/pkg/types" "k8s.io/apiserver/pkg/admission" "k8s.io/apiserver/pkg/authorization/authorizer" kubeapiserveradmission "k8s.io/kubernetes/pkg/kubeapiserver/admission" @@ -41,6 +44,8 @@ type gcPermissionsEnforcement struct { *admission.Handler authorizer authorizer.Authorizer + + restMapper meta.RESTMapper } func (a *gcPermissionsEnforcement) Admit(attributes admission.Attributes) (err error) { @@ -62,11 +67,32 @@ func (a *gcPermissionsEnforcement) Admit(attributes admission.Attributes) (err e Path: "", } allowed, reason, err := a.authorizer.Authorize(deleteAttributes) - if allowed { - return nil + if !allowed { + return admission.NewForbidden(attributes, fmt.Errorf("cannot set an ownerRef on a resource you can't delete: %v, %v", reason, err)) } - return admission.NewForbidden(attributes, fmt.Errorf("cannot set an ownerRef on a resource you can't delete: %v, %v", reason, err)) + // Further check if the user is setting ownerReference.blockOwnerDeletion to + // true. If so, only allows the change if the user has delete permission of + // the _OWNER_ + newBlockingRefs := newBlockingOwnerDeletionRefs(attributes.GetObject(), attributes.GetOldObject()) + for _, ref := range newBlockingRefs { + records, err := a.ownerRefToDeleteAttributeRecords(ref, attributes) + if err != nil { + return admission.NewForbidden(attributes, fmt.Errorf("cannot set blockOwnerDeletion in this case because cannot find RESTMapping for APIVersion %s Kind %s: %v, %v", ref.APIVersion, ref.Kind, reason, err)) + } + // Multiple records are returned if ref.Kind could map to multiple + // resources. User needs to have delete permission on all the + // matched Resources. + 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 nil + } func isChangingOwnerReference(newObj, oldObj runtime.Object) bool { @@ -100,13 +126,109 @@ func isChangingOwnerReference(newObj, oldObj runtime.Object) bool { return false } +// 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. +func (a *gcPermissionsEnforcement) ownerRefToDeleteAttributeRecords(ref metav1.OwnerReference, attributes admission.Attributes) ([]authorizer.AttributesRecord, error) { + var ret []authorizer.AttributesRecord + groupVersion, err := schema.ParseGroupVersion(ref.APIVersion) + if err != nil { + return ret, err + } + mappings, err := a.restMapper.RESTMappings(schema.GroupKind{Group: groupVersion.Group, Kind: ref.Kind}, groupVersion.Version) + if err != nil { + return ret, err + } + for _, mapping := range mappings { + ret = append(ret, authorizer.AttributesRecord{ + User: attributes.GetUserInfo(), + Verb: "delete", + // 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, + Name: ref.Name, + ResourceRequest: true, + Path: "", + }) + } + return ret, nil +} + +// only keeps the blocking refs +func blockingOwnerRefs(refs []metav1.OwnerReference) []metav1.OwnerReference { + var ret []metav1.OwnerReference + for _, ref := range refs { + if ref.BlockOwnerDeletion != nil && *ref.BlockOwnerDeletion == true { + ret = append(ret, ref) + } + } + return ret +} + +func indexByUID(refs []metav1.OwnerReference) map[types.UID]metav1.OwnerReference { + ret := make(map[types.UID]metav1.OwnerReference) + for _, ref := range refs { + ret[ref.UID] = ref + } + return ret +} + +// Returns new blocking ownerReferences, and references whose blockOwnerDeletion +// field is changed from nil or false to true. +func newBlockingOwnerDeletionRefs(newObj, oldObj runtime.Object) []metav1.OwnerReference { + newMeta, err := meta.Accessor(newObj) + if err != nil { + // if we don't have objectmeta, we don't have the object reference + return nil + } + newRefs := newMeta.GetOwnerReferences() + blockingNewRefs := blockingOwnerRefs(newRefs) + if len(blockingNewRefs) == 0 { + return nil + } + + if oldObj == nil { + return blockingNewRefs + } + oldMeta, err := meta.Accessor(oldObj) + if err != nil { + // if we don't have objectmeta, treat it as if all the ownerReference are newly created + return blockingNewRefs + } + + var ret []metav1.OwnerReference + indexedOldRefs := indexByUID(oldMeta.GetOwnerReferences()) + for _, ref := range blockingNewRefs { + oldRef, ok := indexedOldRefs[ref.UID] + if !ok { + // if ref is newly added, and it's blocking, then returns it. + ret = append(ret, ref) + continue + } + wasNotBlocking := oldRef.BlockOwnerDeletion == nil || *oldRef.BlockOwnerDeletion == false + if wasNotBlocking { + ret = append(ret, ref) + } + } + return ret +} + func (a *gcPermissionsEnforcement) SetAuthorizer(authorizer authorizer.Authorizer) { a.authorizer = authorizer } +func (a *gcPermissionsEnforcement) SetRESTMapper(restMapper meta.RESTMapper) { + a.restMapper = restMapper +} + func (a *gcPermissionsEnforcement) Validate() error { if a.authorizer == nil { return fmt.Errorf("missing authorizer") } + if a.restMapper == nil { + return fmt.Errorf("missing restMapper") + } return nil } diff --git a/plugin/pkg/admission/gc/gc_admission_test.go b/plugin/pkg/admission/gc/gc_admission_test.go index b37dbe17c6c..a714b8d21b1 100644 --- a/plugin/pkg/admission/gc/gc_admission_test.go +++ b/plugin/pkg/admission/gc/gc_admission_test.go @@ -17,6 +17,7 @@ limitations under the License. package gc import ( + "strings" "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -26,6 +27,7 @@ import ( "k8s.io/apiserver/pkg/authentication/user" "k8s.io/apiserver/pkg/authorization/authorizer" "k8s.io/kubernetes/pkg/api" + kubeadmission "k8s.io/kubernetes/pkg/kubeapiserver/admission" ) type fakeAuthorizer struct{} @@ -47,10 +49,33 @@ func (fakeAuthorizer) Authorize(a authorizer.Attributes) (bool, string, error) { return true, "", nil } + if username == "non-rc-deleter" { + if a.GetVerb() == "delete" && a.GetResource() == "replicationcontrollers" { + return false, "", nil + } + return true, "", nil + } + return true, "", nil } +// newGCPermissionsEnforcement returns the admission controller configured for testing. +func newGCPermissionsEnforcement() *gcPermissionsEnforcement { + gcAdmit := &gcPermissionsEnforcement{ + Handler: admission.NewHandler(admission.Create, admission.Update), + } + pluginInitializer := kubeadmission.NewPluginInitializer(nil, nil, fakeAuthorizer{}, nil, api.Registry.RESTMapper()) + pluginInitializer.Initialize(gcAdmit) + return gcAdmit +} + func TestGCAdmission(t *testing.T) { + expectNoError := func(err error) bool { + return err == nil + } + expectCantSetOwnerRefError := func(err error) bool { + return strings.Contains(err.Error(), "cannot set an ownerRef on a resource you can't delete") + } tests := []struct { name string username string @@ -58,143 +83,140 @@ func TestGCAdmission(t *testing.T) { oldObj runtime.Object newObj runtime.Object - expectedAllowed bool + checkError func(error) bool }{ { - name: "super-user, create, no objectref change", - username: "super", - resource: api.SchemeGroupVersion.WithResource("pods"), - newObj: &api.Pod{}, - expectedAllowed: true, + name: "super-user, create, no objectref change", + username: "super", + resource: api.SchemeGroupVersion.WithResource("pods"), + newObj: &api.Pod{}, + checkError: expectNoError, }, { - name: "super-user, create, objectref change", - username: "super", - resource: api.SchemeGroupVersion.WithResource("pods"), - newObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, - expectedAllowed: true, + name: "super-user, create, objectref change", + username: "super", + resource: api.SchemeGroupVersion.WithResource("pods"), + newObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, + checkError: expectNoError, }, { - name: "non-deleter, create, no objectref change", - username: "non-deleter", - resource: api.SchemeGroupVersion.WithResource("pods"), - newObj: &api.Pod{}, - expectedAllowed: true, + name: "non-deleter, create, no objectref change", + username: "non-deleter", + resource: api.SchemeGroupVersion.WithResource("pods"), + newObj: &api.Pod{}, + checkError: expectNoError, }, { - name: "non-deleter, create, objectref change", - username: "non-deleter", - resource: api.SchemeGroupVersion.WithResource("pods"), - newObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, - expectedAllowed: false, + name: "non-deleter, create, objectref change", + username: "non-deleter", + resource: api.SchemeGroupVersion.WithResource("pods"), + newObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, + checkError: expectCantSetOwnerRefError, }, { - name: "non-pod-deleter, create, no objectref change", - username: "non-pod-deleter", - resource: api.SchemeGroupVersion.WithResource("pods"), - newObj: &api.Pod{}, - expectedAllowed: true, + name: "non-pod-deleter, create, no objectref change", + username: "non-pod-deleter", + resource: api.SchemeGroupVersion.WithResource("pods"), + newObj: &api.Pod{}, + checkError: expectNoError, }, { - name: "non-pod-deleter, create, objectref change", - username: "non-pod-deleter", - resource: api.SchemeGroupVersion.WithResource("pods"), - newObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, - expectedAllowed: false, + name: "non-pod-deleter, create, objectref change", + username: "non-pod-deleter", + resource: api.SchemeGroupVersion.WithResource("pods"), + newObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, + checkError: expectCantSetOwnerRefError, }, { - name: "non-pod-deleter, create, objectref change, but not a pod", - username: "non-pod-deleter", - resource: api.SchemeGroupVersion.WithResource("not-pods"), - newObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, - expectedAllowed: true, + name: "non-pod-deleter, create, objectref change, but not a pod", + username: "non-pod-deleter", + resource: api.SchemeGroupVersion.WithResource("not-pods"), + newObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, + checkError: expectNoError, }, { - name: "super-user, update, no objectref change", - username: "super", - resource: api.SchemeGroupVersion.WithResource("pods"), - oldObj: &api.Pod{}, - newObj: &api.Pod{}, - expectedAllowed: true, + name: "super-user, update, no objectref change", + username: "super", + resource: api.SchemeGroupVersion.WithResource("pods"), + oldObj: &api.Pod{}, + newObj: &api.Pod{}, + checkError: expectNoError, }, { - name: "super-user, update, no objectref change two", - username: "super", - resource: api.SchemeGroupVersion.WithResource("pods"), - oldObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, - newObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, - expectedAllowed: true, + name: "super-user, update, no objectref change two", + username: "super", + resource: api.SchemeGroupVersion.WithResource("pods"), + oldObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, + newObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, + checkError: expectNoError, }, { - name: "super-user, update, objectref change", - username: "super", - resource: api.SchemeGroupVersion.WithResource("pods"), - oldObj: &api.Pod{}, - newObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, - expectedAllowed: true, + name: "super-user, update, objectref change", + username: "super", + resource: api.SchemeGroupVersion.WithResource("pods"), + oldObj: &api.Pod{}, + newObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, + checkError: expectNoError, }, { - name: "non-deleter, update, no objectref change", - username: "non-deleter", - resource: api.SchemeGroupVersion.WithResource("pods"), - oldObj: &api.Pod{}, - newObj: &api.Pod{}, - expectedAllowed: true, + name: "non-deleter, update, no objectref change", + username: "non-deleter", + resource: api.SchemeGroupVersion.WithResource("pods"), + oldObj: &api.Pod{}, + newObj: &api.Pod{}, + checkError: expectNoError, }, { - name: "non-deleter, update, no objectref change two", - username: "non-deleter", - resource: api.SchemeGroupVersion.WithResource("pods"), - oldObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, - newObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, - expectedAllowed: true, + name: "non-deleter, update, no objectref change two", + username: "non-deleter", + resource: api.SchemeGroupVersion.WithResource("pods"), + oldObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, + newObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, + checkError: expectNoError, }, { - name: "non-deleter, update, objectref change", - username: "non-deleter", - resource: api.SchemeGroupVersion.WithResource("pods"), - oldObj: &api.Pod{}, - newObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, - expectedAllowed: false, + name: "non-deleter, update, objectref change", + username: "non-deleter", + resource: api.SchemeGroupVersion.WithResource("pods"), + oldObj: &api.Pod{}, + newObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, + checkError: expectCantSetOwnerRefError, }, { - name: "non-deleter, update, objectref change two", - username: "non-deleter", - resource: api.SchemeGroupVersion.WithResource("pods"), - oldObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, - newObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}, {Name: "second"}}}}, - expectedAllowed: false, + name: "non-deleter, update, objectref change two", + username: "non-deleter", + resource: api.SchemeGroupVersion.WithResource("pods"), + oldObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, + newObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}, {Name: "second"}}}}, + checkError: expectCantSetOwnerRefError, }, { - name: "non-pod-deleter, update, no objectref change", - username: "non-pod-deleter", - resource: api.SchemeGroupVersion.WithResource("pods"), - oldObj: &api.Pod{}, - newObj: &api.Pod{}, - expectedAllowed: true, + name: "non-pod-deleter, update, no objectref change", + username: "non-pod-deleter", + resource: api.SchemeGroupVersion.WithResource("pods"), + oldObj: &api.Pod{}, + newObj: &api.Pod{}, + checkError: expectNoError, }, { - name: "non-pod-deleter, update, objectref change", - username: "non-pod-deleter", - resource: api.SchemeGroupVersion.WithResource("pods"), - oldObj: &api.Pod{}, - newObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, - expectedAllowed: false, + name: "non-pod-deleter, update, objectref change", + username: "non-pod-deleter", + resource: api.SchemeGroupVersion.WithResource("pods"), + oldObj: &api.Pod{}, + newObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, + checkError: expectCantSetOwnerRefError, }, { - name: "non-pod-deleter, update, objectref change, but not a pod", - username: "non-pod-deleter", - resource: api.SchemeGroupVersion.WithResource("not-pods"), - oldObj: &api.Pod{}, - newObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, - expectedAllowed: true, + name: "non-pod-deleter, update, objectref change, but not a pod", + username: "non-pod-deleter", + resource: api.SchemeGroupVersion.WithResource("not-pods"), + oldObj: &api.Pod{}, + newObj: &api.Pod{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "first"}}}}, + checkError: expectNoError, }, } - gcAdmit := &gcPermissionsEnforcement{ - Handler: admission.NewHandler(admission.Create, admission.Update), - authorizer: fakeAuthorizer{}, - } + gcAdmit := newGCPermissionsEnforcement() for _, tc := range tests { operation := admission.Create @@ -205,13 +227,250 @@ func TestGCAdmission(t *testing.T) { attributes := admission.NewAttributesRecord(tc.newObj, tc.oldObj, schema.GroupVersionKind{}, metav1.NamespaceDefault, "foo", tc.resource, "", operation, user) err := gcAdmit.Admit(attributes) - switch { - case err != nil && !tc.expectedAllowed: - case err != nil && tc.expectedAllowed: + if !tc.checkError(err) { + t.Errorf("%v: unexpected err: %v", tc.name, err) + } + } +} + +func TestBlockOwnerDeletionAdmission(t *testing.T) { + podWithOwnerRefs := func(refs ...metav1.OwnerReference) *api.Pod { + var refSlice []metav1.OwnerReference + for _, ref := range refs { + refSlice = append(refSlice, ref) + } + return &api.Pod{ + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: refSlice, + }, + } + } + + getTrueVar := func() *bool { + ret := true + return &ret + } + + getFalseVar := func() *bool { + ret := false + return &ret + } + blockRC1 := metav1.OwnerReference{ + APIVersion: "v1", + Kind: "ReplicationController", + Name: "rc1", + BlockOwnerDeletion: getTrueVar(), + } + blockRC2 := metav1.OwnerReference{ + APIVersion: "v1", + Kind: "ReplicationController", + Name: "rc2", + BlockOwnerDeletion: getTrueVar(), + } + notBlockRC1 := metav1.OwnerReference{ + APIVersion: "v1", + Kind: "ReplicationController", + Name: "rc1", + BlockOwnerDeletion: getFalseVar(), + } + notBlockRC2 := metav1.OwnerReference{ + APIVersion: "v1", + Kind: "ReplicationController", + Name: "rc2", + BlockOwnerDeletion: getFalseVar(), + } + nilBlockRC1 := metav1.OwnerReference{ + APIVersion: "v1", + Kind: "ReplicationController", + Name: "rc1", + } + nilBlockRC2 := metav1.OwnerReference{ + APIVersion: "v1", + Kind: "ReplicationController", + Name: "rc2", + } + blockDS1 := metav1.OwnerReference{ + APIVersion: "extensions/v1beta1", + Kind: "DaemonSet", + Name: "ds1", + BlockOwnerDeletion: getTrueVar(), + } + notBlockDS1 := metav1.OwnerReference{ + APIVersion: "extensions/v1beta1", + Kind: "DaemonSet", + Name: "ds1", + BlockOwnerDeletion: getFalseVar(), + } + + expectNoError := func(err error) bool { + 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") + } + tests := []struct { + name string + username string + resource schema.GroupVersionResource + oldObj runtime.Object + newObj runtime.Object + + checkError func(error) bool + }{ + // cases for create + { + name: "super-user, create, no ownerReferences", + username: "super", + resource: api.SchemeGroupVersion.WithResource("pods"), + newObj: podWithOwnerRefs(), + checkError: expectNoError, + }, + { + name: "super-user, create, all ownerReferences have blockOwnerDeletion=false", + username: "super", + resource: api.SchemeGroupVersion.WithResource("pods"), + newObj: podWithOwnerRefs(notBlockRC1, notBlockRC2), + checkError: expectNoError, + }, + { + name: "super-user, create, some ownerReferences have blockOwnerDeletion=true", + username: "super", + resource: api.SchemeGroupVersion.WithResource("pods"), + newObj: podWithOwnerRefs(blockRC1, blockRC2), + checkError: expectNoError, + }, + { + name: "non-rc-deleter, create, no ownerReferences", + username: "non-rc-deleter", + resource: api.SchemeGroupVersion.WithResource("pods"), + newObj: podWithOwnerRefs(), + checkError: expectNoError, + }, + { + name: "non-rc-deleter, create, all ownerReferences have blockOwnerDeletion=false or nil", + username: "non-rc-deleter", + resource: api.SchemeGroupVersion.WithResource("pods"), + newObj: podWithOwnerRefs(notBlockRC1, nilBlockRC2), + checkError: expectNoError, + }, + { + name: "non-rc-deleter, create, some ownerReferences have blockOwnerDeletion=true", + username: "non-rc-deleter", + resource: api.SchemeGroupVersion.WithResource("pods"), + newObj: podWithOwnerRefs(blockRC1, notBlockRC2), + checkError: expectCantSetBlockOwnerDeletionError, + }, + { + name: "non-rc-deleter, create, some ownerReferences have blockOwnerDeletion=true, but are pointing to daemonset", + username: "non-rc-deleter", + resource: api.SchemeGroupVersion.WithResource("pods"), + newObj: podWithOwnerRefs(blockDS1), + checkError: expectNoError, + }, + // cases are for update + { + name: "super-user, update, no ownerReferences change blockOwnerDeletion", + username: "super", + resource: api.SchemeGroupVersion.WithResource("pods"), + oldObj: podWithOwnerRefs(nilBlockRC1), + newObj: podWithOwnerRefs(notBlockRC1), + checkError: expectNoError, + }, + { + name: "super-user, update, some ownerReferences change to blockOwnerDeletion=true", + username: "super", + resource: api.SchemeGroupVersion.WithResource("pods"), + oldObj: podWithOwnerRefs(notBlockRC1), + newObj: podWithOwnerRefs(blockRC1), + checkError: expectNoError, + }, + { + name: "super-user, update, add new ownerReferences with blockOwnerDeletion=true", + username: "super", + resource: api.SchemeGroupVersion.WithResource("pods"), + oldObj: podWithOwnerRefs(), + newObj: podWithOwnerRefs(blockRC1), + checkError: expectNoError, + }, + { + name: "non-rc-deleter, update, no ownerReferences change blockOwnerDeletion", + username: "non-rc-deleter", + resource: api.SchemeGroupVersion.WithResource("pods"), + oldObj: podWithOwnerRefs(nilBlockRC1), + newObj: podWithOwnerRefs(notBlockRC1), + checkError: expectNoError, + }, + { + name: "non-rc-deleter, update, some ownerReferences change from blockOwnerDeletion=false to true", + username: "non-rc-deleter", + resource: api.SchemeGroupVersion.WithResource("pods"), + oldObj: podWithOwnerRefs(notBlockRC1), + newObj: podWithOwnerRefs(blockRC1), + checkError: expectCantSetBlockOwnerDeletionError, + }, + { + name: "non-rc-deleter, update, some ownerReferences change from blockOwnerDeletion=nil to true", + username: "non-rc-deleter", + resource: api.SchemeGroupVersion.WithResource("pods"), + oldObj: podWithOwnerRefs(nilBlockRC1), + newObj: podWithOwnerRefs(blockRC1), + checkError: expectCantSetBlockOwnerDeletionError, + }, + { + name: "non-rc-deleter, update, some ownerReferences change from blockOwnerDeletion=true to false", + username: "non-rc-deleter", + resource: api.SchemeGroupVersion.WithResource("pods"), + oldObj: podWithOwnerRefs(blockRC1), + newObj: podWithOwnerRefs(notBlockRC1), + checkError: expectNoError, + }, + { + name: "non-rc-deleter, update, some ownerReferences change blockOwnerDeletion, but all such references are to daemonset", + username: "non-rc-deleter", + resource: api.SchemeGroupVersion.WithResource("pods"), + oldObj: podWithOwnerRefs(notBlockDS1), + newObj: podWithOwnerRefs(blockDS1), + checkError: expectNoError, + }, + { + name: "non-rc-deleter, update, add new ownerReferences with blockOwnerDeletion=nil or false", + username: "non-rc-deleter", + resource: api.SchemeGroupVersion.WithResource("pods"), + oldObj: podWithOwnerRefs(), + newObj: podWithOwnerRefs(notBlockRC1, nilBlockRC2), + checkError: expectNoError, + }, + { + name: "non-rc-deleter, update, add new ownerReferences with blockOwnerDeletion=true", + username: "non-rc-deleter", + resource: api.SchemeGroupVersion.WithResource("pods"), + oldObj: podWithOwnerRefs(), + newObj: podWithOwnerRefs(blockRC1), + checkError: expectCantSetBlockOwnerDeletionError, + }, + { + name: "non-rc-deleter, update, add new ownerReferences with blockOwnerDeletion=true, but the references are to daemonset", + username: "non-rc-deleter", + resource: api.SchemeGroupVersion.WithResource("pods"), + oldObj: podWithOwnerRefs(), + newObj: podWithOwnerRefs(blockDS1), + checkError: expectNoError, + }, + } + + gcAdmit := newGCPermissionsEnforcement() + + for _, tc := range tests { + operation := admission.Create + if tc.oldObj != nil { + operation = admission.Update + } + user := &user.DefaultInfo{Name: tc.username} + attributes := admission.NewAttributesRecord(tc.newObj, tc.oldObj, schema.GroupVersionKind{}, metav1.NamespaceDefault, "foo", tc.resource, "", operation, user) + + err := gcAdmit.Admit(attributes) + if !tc.checkError(err) { t.Errorf("%v: unexpected err: %v", tc.name, err) - case err == nil && !tc.expectedAllowed: - t.Errorf("%v: missing err", tc.name) - case err == nil && tc.expectedAllowed: } } } diff --git a/plugin/pkg/admission/limitranger/admission_test.go b/plugin/pkg/admission/limitranger/admission_test.go index a651a2d502f..a3db23d88d1 100644 --- a/plugin/pkg/admission/limitranger/admission_test.go +++ b/plugin/pkg/admission/limitranger/admission_test.go @@ -595,7 +595,7 @@ func newHandlerForTest(c clientset.Interface) (admission.Interface, informers.Sh if err != nil { return nil, f, err } - pluginInitializer := kubeadmission.NewPluginInitializer(c, f, nil, nil) + pluginInitializer := kubeadmission.NewPluginInitializer(c, f, nil, nil, nil) pluginInitializer.Initialize(handler) err = admission.Validate(handler) return handler, f, err diff --git a/plugin/pkg/admission/namespace/autoprovision/admission_test.go b/plugin/pkg/admission/namespace/autoprovision/admission_test.go index 40001e366b6..f7429d02531 100644 --- a/plugin/pkg/admission/namespace/autoprovision/admission_test.go +++ b/plugin/pkg/admission/namespace/autoprovision/admission_test.go @@ -38,7 +38,7 @@ import ( func newHandlerForTest(c clientset.Interface) (admission.Interface, informers.SharedInformerFactory, error) { f := informers.NewSharedInformerFactory(c, 5*time.Minute) handler := NewProvision() - pluginInitializer := kubeadmission.NewPluginInitializer(c, f, nil, nil) + pluginInitializer := kubeadmission.NewPluginInitializer(c, f, nil, nil, nil) pluginInitializer.Initialize(handler) err := admission.Validate(handler) return handler, f, err diff --git a/plugin/pkg/admission/namespace/exists/admission_test.go b/plugin/pkg/admission/namespace/exists/admission_test.go index 6e211e1e66e..79f23bb3db2 100644 --- a/plugin/pkg/admission/namespace/exists/admission_test.go +++ b/plugin/pkg/admission/namespace/exists/admission_test.go @@ -37,7 +37,7 @@ import ( func newHandlerForTest(c clientset.Interface) (admission.Interface, informers.SharedInformerFactory, error) { f := informers.NewSharedInformerFactory(c, 5*time.Minute) handler := NewExists() - pluginInitializer := kubeadmission.NewPluginInitializer(c, f, nil, nil) + pluginInitializer := kubeadmission.NewPluginInitializer(c, f, nil, nil, nil) pluginInitializer.Initialize(handler) err := admission.Validate(handler) return handler, f, err diff --git a/plugin/pkg/admission/namespace/lifecycle/admission_test.go b/plugin/pkg/admission/namespace/lifecycle/admission_test.go index 026328ebb1d..4c8d679e12d 100644 --- a/plugin/pkg/admission/namespace/lifecycle/admission_test.go +++ b/plugin/pkg/admission/namespace/lifecycle/admission_test.go @@ -48,7 +48,7 @@ func newHandlerForTestWithClock(c clientset.Interface, cacheClock clock.Clock) ( if err != nil { return nil, f, err } - pluginInitializer := kubeadmission.NewPluginInitializer(c, f, nil, nil) + pluginInitializer := kubeadmission.NewPluginInitializer(c, f, nil, nil, nil) pluginInitializer.Initialize(handler) err = admission.Validate(handler) return handler, f, err diff --git a/plugin/pkg/admission/podnodeselector/admission_test.go b/plugin/pkg/admission/podnodeselector/admission_test.go index acdf65d6356..6e8bf093a5c 100644 --- a/plugin/pkg/admission/podnodeselector/admission_test.go +++ b/plugin/pkg/admission/podnodeselector/admission_test.go @@ -183,7 +183,7 @@ func TestHandles(t *testing.T) { func newHandlerForTest(c clientset.Interface) (*podNodeSelector, informers.SharedInformerFactory, error) { f := informers.NewSharedInformerFactory(c, 5*time.Minute) handler := NewPodNodeSelector(nil) - pluginInitializer := kubeadmission.NewPluginInitializer(c, f, nil, nil) + pluginInitializer := kubeadmission.NewPluginInitializer(c, f, nil, nil, nil) pluginInitializer.Initialize(handler) err := admission.Validate(handler) return handler, f, err diff --git a/plugin/pkg/admission/podtolerationrestriction/admission_test.go b/plugin/pkg/admission/podtolerationrestriction/admission_test.go index 5fd47146a46..138aa254a11 100644 --- a/plugin/pkg/admission/podtolerationrestriction/admission_test.go +++ b/plugin/pkg/admission/podtolerationrestriction/admission_test.go @@ -193,7 +193,7 @@ func newHandlerForTest(c clientset.Interface) (*podTolerationsPlugin, informers. return nil, nil, err } handler := NewPodTolerationsPlugin(pluginConfig) - pluginInitializer := kubeadmission.NewPluginInitializer(c, f, nil, nil) + pluginInitializer := kubeadmission.NewPluginInitializer(c, f, nil, nil, nil) pluginInitializer.Initialize(handler) err = admission.Validate(handler) return handler, f, err