Merge pull request #51289 from p0lyn0mial/admission_options_default_plugins

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

exposes a method on AdmissionOptions

**What this PR does / why we need it**:
exposes a method on AdmissionOptions that will set default admission plugin names when none were provided from the command line.

**Release note**:

```
NONE
```

cmd/kube-apiserver/app/options/options_test.go

@@ -103,9 +103,10 @@ func TestAddFlags(t *testing.T) {
 			MinRequestTimeout:           1800,
 		},
 		Admission: &apiserveroptions.AdmissionOptions{
-			PluginNames: []string{"AlwaysDeny"},
-			ConfigFile:  "/admission-control-config",
-			Plugins:     s.Admission.Plugins,
+			RecommendedPluginOrder: []string{"NamespaceLifecycle"},
+			PluginNames:            []string{"AlwaysDeny"},
+			ConfigFile:             "/admission-control-config",
+			Plugins:                s.Admission.Plugins,
 		},
 		Etcd: &apiserveroptions.EtcdOptions{
 			StorageConfig: storagebackend.Config{

staging/src/k8s.io/apiserver/pkg/server/options/BUILD

@@ -1,28 +1,4 @@
-package(default_visibility = ["//visibility:public"])
-
-load(
-    "@io_bazel_rules_go//go:def.bzl",
-    "go_library",
-    "go_test",
-)
-
-go_test(
-    name = "go_default_test",
-    srcs = ["serving_test.go"],
-    data = glob(["testdata/**"]),
-    library = ":go_default_library",
-    deps = [
-        "//vendor/github.com/stretchr/testify/assert:go_default_library",
-        "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
-        "//vendor/k8s.io/apimachinery/pkg/runtime/serializer:go_default_library",
-        "//vendor/k8s.io/apimachinery/pkg/version:go_default_library",
-        "//vendor/k8s.io/apiserver/pkg/endpoints/request:go_default_library",
-        "//vendor/k8s.io/apiserver/pkg/server:go_default_library",
-        "//vendor/k8s.io/apiserver/pkg/util/flag:go_default_library",
-        "//vendor/k8s.io/client-go/discovery:go_default_library",
-        "//vendor/k8s.io/client-go/rest:go_default_library",
-    ],
-)
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 
 go_library(
     name = "go_default_library",
@@ -39,6 +15,7 @@ go_library(
         "server_run_options.go",
         "serving.go",
     ],
+    visibility = ["//visibility:public"],
     deps = [
         "//vendor/github.com/golang/glog:go_default_library",
         "//vendor/github.com/pborman/uuid:go_default_library",
@@ -52,6 +29,7 @@ go_library(
         "//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/admission:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/admission/initializer:go_default_library",
+        "//vendor/k8s.io/apiserver/pkg/admission/plugin/namespace/lifecycle:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/apis/audit/v1beta1:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/audit:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/audit/policy:go_default_library",
@@ -80,9 +58,25 @@ go_library(
     ],
 )
 
-filegroup(
-    name = "testdata",
-    srcs = glob(["testdata/*"]),
+go_test(
+    name = "go_default_test",
+    srcs = [
+        "admission_test.go",
+        "serving_test.go",
+    ],
+    data = glob(["testdata/**"]),
+    library = ":go_default_library",
+    deps = [
+        "//vendor/github.com/stretchr/testify/assert:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/runtime/serializer:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/version:go_default_library",
+        "//vendor/k8s.io/apiserver/pkg/endpoints/request:go_default_library",
+        "//vendor/k8s.io/apiserver/pkg/server:go_default_library",
+        "//vendor/k8s.io/apiserver/pkg/util/flag:go_default_library",
+        "//vendor/k8s.io/client-go/discovery:go_default_library",
+        "//vendor/k8s.io/client-go/rest:go_default_library",
+    ],
 )
 
 filegroup(
@@ -99,4 +93,5 @@ filegroup(
         "//staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig:all-srcs",
     ],
     tags = ["automanaged"],
+    visibility = ["//visibility:public"],
 )

staging/src/k8s.io/apiserver/pkg/server/options/admission.go

@@ -23,6 +23,7 @@ import (
 	"github.com/spf13/pflag"
 	"k8s.io/apiserver/pkg/admission"
 	"k8s.io/apiserver/pkg/admission/initializer"
+	"k8s.io/apiserver/pkg/admission/plugin/namespace/lifecycle"
 	"k8s.io/apiserver/pkg/server"
 	"k8s.io/client-go/informers"
 	"k8s.io/client-go/kubernetes"
@@ -30,19 +31,28 @@ import (
 
 // AdmissionOptions holds the admission options
 type AdmissionOptions struct {
-	PluginNames []string
-	ConfigFile  string
-	Plugins     *admission.Plugins
+	// RecommendedPluginOrder holds an ordered list of plugin names we recommend to use by default
+	RecommendedPluginOrder []string
+	// DefaultOffPlugins a list of plugin names that should be disabled by default
+	DefaultOffPlugins []string
+	PluginNames       []string
+	ConfigFile        string
+	Plugins           *admission.Plugins
 }
 
 // NewAdmissionOptions creates a new instance of AdmissionOptions
 // Note:
-// In addition it calls RegisterAllAdmissionPlugins to register
-// all generic admission plugins.
+//  In addition it calls RegisterAllAdmissionPlugins to register
+//  all generic admission plugins.
+//
+//  Provides the list of RecommendedPluginOrder that holds sane values
+//  that can be used by servers that don't care about admission chain.
+//  Servers that do care can overwrite/append that field after creation.
 func NewAdmissionOptions() *AdmissionOptions {
 	options := &AdmissionOptions{
-		Plugins:     &admission.Plugins{},
-		PluginNames: []string{},
+		Plugins:                &admission.Plugins{},
+		PluginNames:            []string{},
+		RecommendedPluginOrder: []string{lifecycle.PluginName},
 	}
 	server.RegisterAllAdmissionPlugins(options.Plugins)
 	return options
@@ -58,14 +68,20 @@ func (a *AdmissionOptions) AddFlags(fs *pflag.FlagSet) {
 		"File with admission control configuration.")
 }
 
-// ApplyTo adds the admission chain to the server configuration
-// the method lazily initializes a generic plugin that is appended to the list of pluginInitializers
+// ApplyTo adds the admission chain to the server configuration.
+// In case admission plugin names were not provided by a custer-admin they will be prepared from the recommended/default values.
+// In addition the method lazily initializes a generic plugin that is appended to the list of pluginInitializers
 // note this method uses:
 //  genericconfig.LoopbackClientConfig
 //  genericconfig.SharedInformerFactory
 //  genericconfig.Authorizer
 func (a *AdmissionOptions) ApplyTo(c *server.Config, informers informers.SharedInformerFactory, pluginInitializers ...admission.PluginInitializer) error {
-	pluginsConfigProvider, err := admission.ReadAdmissionConfiguration(a.PluginNames, a.ConfigFile)
+	pluginNames := a.PluginNames
+	if len(a.PluginNames) == 0 {
+		pluginNames = a.enabledPluginNames()
+	}
+
+	pluginsConfigProvider, err := admission.ReadAdmissionConfiguration(pluginNames, a.ConfigFile)
 	if err != nil {
 		return fmt.Errorf("failed to read plugin config: %v", err)
 	}
@@ -82,7 +98,7 @@ func (a *AdmissionOptions) ApplyTo(c *server.Config, informers informers.SharedI
 	pluginInitializers = append(pluginInitializers, genericInitializer)
 	initializersChain = append(initializersChain, pluginInitializers...)
 
-	admissionChain, err := a.Plugins.NewFromPlugins(a.PluginNames, pluginsConfigProvider, initializersChain)
+	admissionChain, err := a.Plugins.NewFromPlugins(pluginNames, pluginsConfigProvider, initializersChain)
 	if err != nil {
 		return err
 	}
@@ -95,3 +111,33 @@ func (a *AdmissionOptions) Validate() []error {
 	errs := []error{}
 	return errs
 }
+
+// enabledPluginNames makes use of RecommendedPluginOrder and DefaultOffPlugins fields
+// to prepare a list of plugin names that are enabled.
+//
+// TODO(p0lyn0mial): In the end we will introduce two new flags:
+// --disable-admission-plugin this would be a list of admission plugins that a cluster-admin wants to explicitly disable.
+// --enable-admission-plugin  this would be a list of admission plugins that a cluster-admin wants to explicitly enable.
+// both flags are going to be handled by this method
+func (a *AdmissionOptions) enabledPluginNames() []string {
+	//TODO(p0lyn0mial): first subtract plugins that a user wants to explicitly enable from allOffPlugins (DefaultOffPlugins)
+	//TODO(p0lyn0miial): then add/append plugins that a user wants to explicitly disable to allOffPlugins
+	//TODO(p0lyn0mial): so that --off=three --on=one,three default-off=one,two results in  "one" being enabled.
+	allOffPlugins := a.DefaultOffPlugins
+	onlyEnabledPluginNames := []string{}
+	for _, pluginName := range a.RecommendedPluginOrder {
+		disablePlugin := false
+		for _, disabledPluginName := range allOffPlugins {
+			if pluginName == disabledPluginName {
+				disablePlugin = true
+				break
+			}
+		}
+		if disablePlugin {
+			continue
+		}
+		onlyEnabledPluginNames = append(onlyEnabledPluginNames, pluginName)
+	}
+
+	return onlyEnabledPluginNames
+}

staging/src/k8s.io/apiserver/pkg/server/options/admission_test.go

@@ -0,0 +1,68 @@
+/*
+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 options
+
+import (
+	"fmt"
+	"testing"
+)
+
+func TestEnabledPluginNamesMethod(t *testing.T) {
+	scenarios := []struct {
+		expectedPluginNames       []string
+		setDefaultOffPluginNames  []string
+		setRecommendedPluginOrder []string
+	}{
+		// scenario 1: check if a call to enabledPluginNames sets expected values.
+		{
+			expectedPluginNames: []string{"NamespaceLifecycle"},
+		},
+
+		// scenario 2: overwrite RecommendedPluginOrder and set DefaultOffPluginNames
+		// make sure that plugins which are on DefaultOffPluginNames list do not get to PluginNames list.
+		{
+			expectedPluginNames:       []string{"pluginA"},
+			setRecommendedPluginOrder: []string{"pluginA", "pluginB"},
+			setDefaultOffPluginNames:  []string{"pluginB"},
+		},
+	}
+
+	// act
+	for index, scenario := range scenarios {
+		t.Run(fmt.Sprintf("scenario %d", index), func(t *testing.T) {
+			target := NewAdmissionOptions()
+
+			if len(scenario.setDefaultOffPluginNames) > 0 {
+				target.DefaultOffPlugins = scenario.setDefaultOffPluginNames
+			}
+			if len(scenario.setRecommendedPluginOrder) > 0 {
+				target.RecommendedPluginOrder = scenario.setRecommendedPluginOrder
+			}
+
+			actualPluginNames := target.enabledPluginNames()
+
+			if len(actualPluginNames) != len(scenario.expectedPluginNames) {
+				t.Errorf("incorrect number of items, got %d, expected = %d", len(actualPluginNames), len(scenario.expectedPluginNames))
+			}
+			for i := range actualPluginNames {
+				if scenario.expectedPluginNames[i] != actualPluginNames[i] {
+					t.Errorf("missmatch at index = %d, got = %s, expected = %s", i, actualPluginNames[i], scenario.expectedPluginNames[i])
+				}
+			}
+		})
+	}
+}