From 5b1fffa3e40b812e81ede244f671c90e3428e2ec Mon Sep 17 00:00:00 2001 From: Jiahui Feng Date: Wed, 28 Feb 2024 15:31:18 -0800 Subject: [PATCH] add resource filter to admission initializer. --- pkg/kubeapiserver/admission/config.go | 4 +- .../admission/exclusion/filter.go | 42 +++++++ .../admission/exclusion/resources.go | 66 +++++++++++ pkg/kubeapiserver/admission/initializer.go | 8 ++ .../admission/initializer_test.go | 7 +- .../pkg/admission/initializer/interfaces.go | 8 ++ .../admission/resourcefilter/interfaces.go | 29 +++++ .../apiserver/cel/excludedresources_test.go | 106 ++++++++++++++++++ 8 files changed, 266 insertions(+), 4 deletions(-) create mode 100644 pkg/kubeapiserver/admission/exclusion/filter.go create mode 100644 pkg/kubeapiserver/admission/exclusion/resources.go create mode 100644 staging/src/k8s.io/apiserver/pkg/admission/resourcefilter/interfaces.go create mode 100644 test/integration/apiserver/cel/excludedresources_test.go diff --git a/pkg/kubeapiserver/admission/config.go b/pkg/kubeapiserver/admission/config.go index 751d29f3c86..16f994093a0 100644 --- a/pkg/kubeapiserver/admission/config.go +++ b/pkg/kubeapiserver/admission/config.go @@ -29,13 +29,14 @@ import ( "k8s.io/apiserver/pkg/admission" webhookinit "k8s.io/apiserver/pkg/admission/plugin/webhook/initializer" genericapiserver "k8s.io/apiserver/pkg/server" - egressselector "k8s.io/apiserver/pkg/server/egressselector" + "k8s.io/apiserver/pkg/server/egressselector" "k8s.io/apiserver/pkg/util/webhook" cacheddiscovery "k8s.io/client-go/discovery/cached/memory" externalinformers "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" "k8s.io/client-go/restmapper" + "k8s.io/kubernetes/pkg/kubeapiserver/admission/exclusion" quotainstall "k8s.io/kubernetes/pkg/quota/v1/install" ) @@ -69,6 +70,7 @@ func (c *Config) New(proxyTransport *http.Transport, egressSelector *egressselec cloudConfig, discoveryRESTMapper, quotainstall.NewQuotaConfigurationForAdmission(), + exclusion.NewFilter(), ) admissionPostStartHook := func(context genericapiserver.PostStartHookContext) error { diff --git a/pkg/kubeapiserver/admission/exclusion/filter.go b/pkg/kubeapiserver/admission/exclusion/filter.go new file mode 100644 index 00000000000..d4db639c89a --- /dev/null +++ b/pkg/kubeapiserver/admission/exclusion/filter.go @@ -0,0 +1,42 @@ +/* +Copyright 2024 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 exclusion + +import ( + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apiserver/pkg/admission" + "k8s.io/apiserver/pkg/admission/resourcefilter" +) + +// NewFilter creates a resource filter with the built-in exclusion list. +func NewFilter() resourcefilter.Interface { + return &filter{excluded: sets.New[schema.GroupResource](excluded...)} +} + +type filter struct { + excluded sets.Set[schema.GroupResource] +} + +func (f *filter) ShouldHandle(a admission.Attributes) bool { + gvr := a.GetResource() + // ignore the version for the decision-making + // because putting different versions into different category + // is almost always a mistake. + gr := gvr.GroupResource() + return !f.excluded.Has(gr) +} diff --git a/pkg/kubeapiserver/admission/exclusion/resources.go b/pkg/kubeapiserver/admission/exclusion/resources.go new file mode 100644 index 00000000000..26cbdf629e1 --- /dev/null +++ b/pkg/kubeapiserver/admission/exclusion/resources.go @@ -0,0 +1,66 @@ +/* +Copyright 2024 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 exclusion + +import ( + "slices" + + "k8s.io/apimachinery/pkg/runtime/schema" +) + +// include is the list of resources that the expression-based admission controllers +// should intercept. +// The version is omitted, all versions of the same GroupResource are treated the same. +// If a resource is transient, i.e., not persisted in the storage, the resource must be +// in either include or excluded list. +var included = []schema.GroupResource{ + {Group: "", Resource: "bindings"}, + {Group: "", Resource: "pods/attach"}, + {Group: "", Resource: "pods/binding"}, + {Group: "", Resource: "pods/eviction"}, + {Group: "", Resource: "pods/exec"}, + {Group: "", Resource: "pods/portforward"}, + + // ref: https://github.com/kubernetes/kubernetes/issues/122205#issuecomment-1927390823 + {Group: "", Resource: "serviceaccounts/token"}, +} + +// excluded is the list of resources that the expression-based admission controllers +// should ignore. +// The version is omitted, all versions of the same GroupResource are treated the same. +var excluded = []schema.GroupResource{ + // BEGIN interception of these non-persisted resources may break the cluster + {Group: "authentication.k8s.io", Resource: "selfsubjectreviews"}, + {Group: "authentication.k8s.io", Resource: "tokenreviews"}, + {Group: "authorization.k8s.io", Resource: "localsubjectaccessreviews"}, + {Group: "authorization.k8s.io", Resource: "selfsubjectaccessreviews"}, + {Group: "authorization.k8s.io", Resource: "selfsubjectrulesreviews"}, + {Group: "authorization.k8s.io", Resource: "subjectaccessreviews"}, + // END interception of these non-persisted resources may break the cluster +} + +// Included returns a copy of the list of resources that the expression-based admission controllers +// should intercept. +func Included() []schema.GroupResource { + return slices.Clone(included) +} + +// Excluded returns a copy of the list of resources that the expression-based admission controllers +// should ignore. +func Excluded() []schema.GroupResource { + return slices.Clone(excluded) +} diff --git a/pkg/kubeapiserver/admission/initializer.go b/pkg/kubeapiserver/admission/initializer.go index 0ba97b5427b..536dfbf5ce8 100644 --- a/pkg/kubeapiserver/admission/initializer.go +++ b/pkg/kubeapiserver/admission/initializer.go @@ -20,6 +20,7 @@ import ( "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apiserver/pkg/admission" "k8s.io/apiserver/pkg/admission/initializer" + "k8s.io/apiserver/pkg/admission/resourcefilter" quota "k8s.io/apiserver/pkg/quota/v1" ) @@ -35,6 +36,7 @@ type PluginInitializer struct { cloudConfig []byte restMapper meta.RESTMapper quotaConfiguration quota.Configuration + resourceFilter resourcefilter.Interface } var _ admission.PluginInitializer = &PluginInitializer{} @@ -46,11 +48,13 @@ func NewPluginInitializer( cloudConfig []byte, restMapper meta.RESTMapper, quotaConfiguration quota.Configuration, + resourceFilter resourcefilter.Interface, ) *PluginInitializer { return &PluginInitializer{ cloudConfig: cloudConfig, restMapper: restMapper, quotaConfiguration: quotaConfiguration, + resourceFilter: resourceFilter, } } @@ -68,4 +72,8 @@ func (i *PluginInitializer) Initialize(plugin admission.Interface) { if wants, ok := plugin.(initializer.WantsQuotaConfiguration); ok { wants.SetQuotaConfiguration(i.quotaConfiguration) } + + if wants, ok := plugin.(initializer.WantsResourceFilter); ok { + wants.SetResourceFilter(i.resourceFilter) + } } diff --git a/pkg/kubeapiserver/admission/initializer_test.go b/pkg/kubeapiserver/admission/initializer_test.go index a9fdd5fcece..ead0a2e3f25 100644 --- a/pkg/kubeapiserver/admission/initializer_test.go +++ b/pkg/kubeapiserver/admission/initializer_test.go @@ -24,6 +24,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apiserver/pkg/admission" quota "k8s.io/apiserver/pkg/quota/v1" + "k8s.io/kubernetes/pkg/kubeapiserver/admission/exclusion" ) type doNothingAdmission struct{} @@ -49,7 +50,7 @@ func (p *WantsCloudConfigAdmissionPlugin) SetCloudConfig(cloudConfig []byte) { func TestCloudConfigAdmissionPlugin(t *testing.T) { cloudConfig := []byte("cloud-configuration") - initializer := NewPluginInitializer(cloudConfig, nil, nil) + initializer := NewPluginInitializer(cloudConfig, nil, nil, exclusion.NewFilter()) wantsCloudConfigAdmission := &WantsCloudConfigAdmissionPlugin{} initializer.Initialize(wantsCloudConfigAdmission) @@ -94,7 +95,7 @@ func (p *WantsRESTMapperAdmissionPlugin) SetRESTMapper(mapper meta.RESTMapper) { func TestRESTMapperAdmissionPlugin(t *testing.T) { mapper := doNothingRESTMapper{} - initializer := NewPluginInitializer(nil, mapper, nil) + initializer := NewPluginInitializer(nil, mapper, nil, exclusion.NewFilter()) wantsRESTMapperAdmission := &WantsRESTMapperAdmissionPlugin{} initializer.Initialize(wantsRESTMapperAdmission) @@ -121,7 +122,7 @@ func (p *WantsQuotaConfigurationAdmissionPlugin) SetQuotaConfiguration(config qu func TestQuotaConfigurationAdmissionPlugin(t *testing.T) { config := doNothingQuotaConfiguration{} - initializer := NewPluginInitializer(nil, nil, config) + initializer := NewPluginInitializer(nil, nil, config, exclusion.NewFilter()) wantsQuotaConfigurationAdmission := &WantsQuotaConfigurationAdmissionPlugin{} initializer.Initialize(wantsQuotaConfigurationAdmission) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/initializer/interfaces.go b/staging/src/k8s.io/apiserver/pkg/admission/initializer/interfaces.go index 6077c89de84..233d880062a 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/initializer/interfaces.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/initializer/interfaces.go @@ -19,6 +19,7 @@ package initializer import ( "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apiserver/pkg/admission" + "k8s.io/apiserver/pkg/admission/resourcefilter" "k8s.io/apiserver/pkg/authorization/authorizer" "k8s.io/apiserver/pkg/cel/openapi/resolver" quota "k8s.io/apiserver/pkg/quota/v1" @@ -89,3 +90,10 @@ type WantsSchemaResolver interface { SetSchemaResolver(resolver resolver.SchemaResolver) admission.InitializationValidator } + +// WantsResourceFilter defines a function which sets the ResourceFilter for +// an admission plugin that needs it. +type WantsResourceFilter interface { + SetResourceFilter(filter resourcefilter.Interface) + admission.InitializationValidator +} diff --git a/staging/src/k8s.io/apiserver/pkg/admission/resourcefilter/interfaces.go b/staging/src/k8s.io/apiserver/pkg/admission/resourcefilter/interfaces.go new file mode 100644 index 00000000000..23711b6dfff --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/admission/resourcefilter/interfaces.go @@ -0,0 +1,29 @@ +/* +Copyright 2024 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 resourcefilter + +import ( + "k8s.io/apiserver/pkg/admission" +) + +// Interface is a resource filter that takes an Attributes and +// check if it should be handled or ignored by the admission plugin. +type Interface interface { + // ShouldHandle returns true if the admission plugin should handle the request, + // considering the given Attributes, or false otherwise. + ShouldHandle(admission.Attributes) bool +} diff --git a/test/integration/apiserver/cel/excludedresources_test.go b/test/integration/apiserver/cel/excludedresources_test.go new file mode 100644 index 00000000000..be7fe221b59 --- /dev/null +++ b/test/integration/apiserver/cel/excludedresources_test.go @@ -0,0 +1,106 @@ +/* +Copyright 2024 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 cel + +import ( + "fmt" + "slices" + "strings" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/client-go/discovery" + apiservertesting "k8s.io/kubernetes/cmd/kube-apiserver/app/testing" + "k8s.io/kubernetes/pkg/kubeapiserver/admission/exclusion" + "k8s.io/kubernetes/test/integration/framework" +) + +// TestExcludedResources is an open-box test to ensure that a resource that is not persisted +// is either in the allow-list or block-list of the CEL admission. +// +// see staging/src/k8s.io/apiserver/pkg/admission/exclusion/exclusion.go +// for the lists that the test is about. +func TestExcludedResources(t *testing.T) { + server, err := apiservertesting.StartTestServer(t, + nil, // no extra options + // enable all APIs to cover all defined resources + // note that it does not require feature flags enabled to + // discovery GVRs of disabled features. + []string{"--runtime-config=api/all=true"}, + framework.SharedEtcd()) + if err != nil { + t.Fatal(err) + } + defer server.TearDownFn() + + config := server.ClientConfig + + discoveryClient, err := discovery.NewDiscoveryClientForConfig(config) + if err != nil { + t.Fatal(err) + } + _, resourceLists, _, err := discoveryClient.GroupsAndMaybeResources() + if err != nil { + t.Fatal(err) + } + interestedGRs := sets.New[schema.GroupResource]() + interestedVerbCombinations := []metav1.Verbs{ + {"create"}, + {"create", "get"}, + } + for _, rl := range resourceLists { + for _, r := range rl.APIResources { + slices.Sort(r.Verbs) + for _, c := range interestedVerbCombinations { + if slices.Equal(r.Verbs, c) { + gv, err := schema.ParseGroupVersion(rl.GroupVersion) + if err != nil { + t.Fatalf("internal error: cannot parse GV from %q: %v", rl.GroupVersion, err) + } + interestedGRs.Insert(gv.WithResource(r.Name).GroupResource()) + } + } + } + } + existing := sets.New[schema.GroupResource]() + existing.Insert(exclusion.Included()...) + existing.Insert(exclusion.Excluded()...) + shouldAdd, shouldRemove := interestedGRs.Difference(existing), existing.Difference(interestedGRs) + if shouldAdd.Len() > 0 { + t.Errorf("the following resources should either be in Included or Excluded in\n"+ + "pkg/kubeapiserver/admission/exclusion/resources.go\n%s", + formatGRs(shouldAdd.UnsortedList()), + ) + } + if shouldRemove.Len() > 0 { + t.Errorf("the following resources are in pkg/kubeapiserver/admission/exclusion/resources.go\n"+ + "but does not seem to be transient.\n%s", + formatGRs(shouldRemove.UnsortedList())) + } +} + +func formatGRs(grs []schema.GroupResource) string { + lines := make([]string, 0, len(grs)) + for _, gvr := range grs { + item := fmt.Sprintf("%#v,", gvr) + lines = append(lines, item) + } + slices.Sort(lines) + return strings.Join(lines, "\n") +}