From 28da9a684807fc233173195b086c849a148e6e49 Mon Sep 17 00:00:00 2001 From: David Eads Date: Wed, 9 Feb 2022 13:05:12 -0500 Subject: [PATCH] update the --runtime-config handling to ensure that user preferences always take priority over hardcoded preferences Kubernetes-commit: e378fd2bae0fec4756a8e755395193337d13caa2 --- pkg/server/resourceconfig/helpers.go | 108 +++++++--- pkg/server/resourceconfig/helpers_test.go | 240 +++++++++++++++++++--- pkg/server/storage/resource_config.go | 14 ++ 3 files changed, 308 insertions(+), 54 deletions(-) diff --git a/pkg/server/resourceconfig/helpers.go b/pkg/server/resourceconfig/helpers.go index bfcce54b8..1049ff85e 100644 --- a/pkg/server/resourceconfig/helpers.go +++ b/pkg/server/resourceconfig/helpers.go @@ -26,7 +26,6 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" serverstore "k8s.io/apiserver/pkg/server/storage" cliflag "k8s.io/component-base/cli/flag" - "k8s.io/klog/v2" ) // GroupVersionRegistry provides access to registered group versions. @@ -65,7 +64,7 @@ var ( betaPattern = regexp.MustCompile(`^v\d+beta\d+$`) alphaPattern = regexp.MustCompile(`^v\d+alpha\d+$`) - matchers = map[string]func(gv schema.GroupVersion) bool{ + groupVersionMatchers = map[string]func(gv schema.GroupVersion) bool{ // allows users to address all api versions APIAll: func(gv schema.GroupVersion) bool { return true }, // allows users to address all api versions in the form v[0-9]+ @@ -76,9 +75,28 @@ var ( APIAlpha: func(gv schema.GroupVersion) bool { return alphaPattern.MatchString(gv.Version) }, } - matcherOrder = []string{APIAll, APIGA, APIBeta, APIAlpha} + groupVersionMatchersOrder = []string{APIAll, APIGA, APIBeta, APIAlpha} + + groupVersionResourceMatchers = map[string]func(gvr schema.GroupVersionResource) bool{ + // allows users to address all api versions + APIAll: func(gvr schema.GroupVersionResource) bool { return true }, + // allows users to address all api versions in the form v[0-9]+ + APIGA: func(gvr schema.GroupVersionResource) bool { return gaPattern.MatchString(gvr.Version) }, + // allows users to address all beta api versions + APIBeta: func(gvr schema.GroupVersionResource) bool { return betaPattern.MatchString(gvr.Version) }, + // allows users to address all alpha api versions + APIAlpha: func(gvr schema.GroupVersionResource) bool { return alphaPattern.MatchString(gvr.Version) }, + } + + groupVersionResourceMatchersOrder = []string{APIAll, APIGA, APIBeta, APIAlpha} ) +func resourceMatcherForVersion(gv schema.GroupVersion) func(gvr schema.GroupVersionResource) bool { + return func(gvr schema.GroupVersionResource) bool { + return gv == gvr.GroupVersion() + } +} + // MergeAPIResourceConfigs merges the given defaultAPIResourceConfig with the given resourceConfigOverrides. // Exclude the groups not registered in registry, and check if version is // not registered in group, then it will fail. @@ -90,29 +108,44 @@ func MergeAPIResourceConfigs( resourceConfig := defaultAPIResourceConfig overrides := resourceConfigOverrides - for _, flag := range matcherOrder { + for _, flag := range groupVersionMatchersOrder { if value, ok := overrides[flag]; ok { if value == "false" { - resourceConfig.DisableMatchingVersions(matchers[flag]) + resourceConfig.DisableMatchingVersions(groupVersionMatchers[flag]) } else if value == "true" { - resourceConfig.EnableMatchingVersions(matchers[flag]) + resourceConfig.EnableMatchingVersions(groupVersionMatchers[flag]) } else { return nil, fmt.Errorf("invalid value %v=%v", flag, value) } + // remove individual resource preferences that were hardcoded into the default. The override trumps those settings. + resourceConfig.RemoveMatchingResourcePreferences(groupVersionResourceMatchers[flag]) } } + type versionEnablementPreference struct { + key string + enabled bool + groupVersion schema.GroupVersion + } + type resourceEnablementPreference struct { + key string + enabled bool + groupVersionResource schema.GroupVersionResource + } + versionPreferences := []versionEnablementPreference{} + resourcePreferences := []resourceEnablementPreference{} + // "={true|false} allows users to enable/disable API. // This takes preference over api/all, if specified. // Iterate through all group/version overrides specified in runtimeConfig. for key := range overrides { // Have already handled them above. Can skip them here. - if _, ok := matchers[key]; ok { + if _, ok := groupVersionMatchers[key]; ok { continue } tokens := strings.Split(key, "/") - if len(tokens) < 2 { + if len(tokens) < 2 || len(tokens) > 3 { continue } groupVersionString := tokens[0] + "/" + tokens[1] @@ -121,13 +154,6 @@ func MergeAPIResourceConfigs( return nil, fmt.Errorf("invalid key %s", key) } - // individual resource enablement/disablement is only supported in the extensions/v1beta1 API group for legacy reasons. - // all other API groups are expected to contain coherent sets of resources that are enabled/disabled together. - if len(tokens) > 2 && (groupVersion != schema.GroupVersion{Group: "extensions", Version: "v1beta1"}) { - klog.Warningf("ignoring invalid key %s, individual resource enablement/disablement is not supported in %s, and will prevent starting in future releases", key, groupVersion.String()) - continue - } - // Exclude group not registered into the registry. if !registry.IsGroupRegistered(groupVersion.Group) { continue @@ -141,22 +167,46 @@ func MergeAPIResourceConfigs( if err != nil { return nil, err } - if enabled { - // enable the groupVersion for "group/version=true" and "group/version/resource=true" - resourceConfig.EnableVersions(groupVersion) - } else if len(tokens) == 2 { - // disable the groupVersion only for "group/version=false", not "group/version/resource=false" - resourceConfig.DisableVersions(groupVersion) - } - if len(tokens) < 3 { - continue + switch len(tokens) { + case 2: + versionPreferences = append(versionPreferences, versionEnablementPreference{ + key: key, + enabled: enabled, + groupVersion: groupVersion, + }) + case 3: + if strings.ToLower(tokens[2]) != tokens[2] { + return nil, fmt.Errorf("invalid key %v: group/version/resource and resource is always lowercase plural, not %q", key, tokens[2]) + } + resourcePreferences = append(resourcePreferences, resourceEnablementPreference{ + key: key, + enabled: enabled, + groupVersionResource: groupVersion.WithResource(tokens[2]), + }) } - groupVersionResource := groupVersion.WithResource(tokens[2]) - if enabled { - resourceConfig.EnableResources(groupVersionResource) + } + + // apply version preferences first, so that we can remove the hardcoded resource preferences that are being overridden + for _, versionPreference := range versionPreferences { + // if a user has expressed a preference about a version, that preference takes priority over the hardcoded resources + resourceConfig.RemoveMatchingResourcePreferences(resourceMatcherForVersion(versionPreference.groupVersion)) + if versionPreference.enabled { + // enable the groupVersion for "group/version=true" and "group/version/resource=true" + resourceConfig.EnableVersions(versionPreference.groupVersion) + } else { - resourceConfig.DisableResources(groupVersionResource) + // disable the groupVersion only for "group/version=false", not "group/version/resource=false" + resourceConfig.DisableVersions(versionPreference.groupVersion) + } + } + + // apply resource preferences last, so they have the highest priority + for _, resourcePreference := range resourcePreferences { + if resourcePreference.enabled { + resourceConfig.EnableResources(resourcePreference.groupVersionResource) + } else { + resourceConfig.DisableResources(resourcePreference.groupVersionResource) } } @@ -182,7 +232,7 @@ func getRuntimeConfigValue(overrides cliflag.ConfigurationMap, apiKey string, de func ParseGroups(resourceConfig cliflag.ConfigurationMap) ([]string, error) { groups := []string{} for key := range resourceConfig { - if _, ok := matchers[key]; ok { + if _, ok := groupVersionMatchers[key]; ok { continue } tokens := strings.Split(key, "/") diff --git a/pkg/server/resourceconfig/helpers_test.go b/pkg/server/resourceconfig/helpers_test.go index f78550d52..9bd97e30c 100644 --- a/pkg/server/resourceconfig/helpers_test.go +++ b/pkg/server/resourceconfig/helpers_test.go @@ -20,6 +20,10 @@ import ( "reflect" "testing" + appsv1 "k8s.io/api/apps/v1" + + "k8s.io/apimachinery/pkg/runtime/schema" + "github.com/stretchr/testify/require" apiv1 "k8s.io/api/core/v1" @@ -32,13 +36,28 @@ func TestParseRuntimeConfig(t *testing.T) { scheme := newFakeScheme(t) apiv1GroupVersion := apiv1.SchemeGroupVersion testCases := []struct { + name string runtimeConfig map[string]string defaultResourceConfig func() *serverstore.ResourceConfig expectedAPIConfig func() *serverstore.ResourceConfig err bool }{ { - // everything default value. + name: "using-kind", + runtimeConfig: map[string]string{ + "apps/v1/Deployment": "false", + }, + defaultResourceConfig: func() *serverstore.ResourceConfig { + return newFakeAPIResourceConfigSource() + }, + expectedAPIConfig: func() *serverstore.ResourceConfig { + return newFakeAPIResourceConfigSource() + }, + expectedEnabledAPIs: defaultFakeEnabledResources(), + err: true, + }, + { + name: "everything-default-value", runtimeConfig: map[string]string{}, defaultResourceConfig: func() *serverstore.ResourceConfig { return newFakeAPIResourceConfigSource() @@ -49,7 +68,7 @@ func TestParseRuntimeConfig(t *testing.T) { err: false, }, { - // no runtimeConfig override. + name: "no-runtimeConfig-override", runtimeConfig: map[string]string{}, defaultResourceConfig: func() *serverstore.ResourceConfig { config := newFakeAPIResourceConfigSource() @@ -64,7 +83,7 @@ func TestParseRuntimeConfig(t *testing.T) { err: false, }, { - // version enabled by runtimeConfig override. + name: "version-enabled-by-runtimeConfig-override", runtimeConfig: map[string]string{ "apps/v1": "", }, @@ -79,7 +98,7 @@ func TestParseRuntimeConfig(t *testing.T) { err: false, }, { - // Disable v1. + name: "disable-v1", runtimeConfig: map[string]string{ "/v1": "false", }, @@ -94,7 +113,7 @@ func TestParseRuntimeConfig(t *testing.T) { err: false, }, { - // invalid runtime config + name: "invalid-runtime-config", runtimeConfig: map[string]string{ "invalidgroup/version": "false", }, @@ -107,7 +126,7 @@ func TestParseRuntimeConfig(t *testing.T) { err: false, }, { - // enable all + name: "enable-all", runtimeConfig: map[string]string{ "api/all": "true", }, @@ -117,12 +136,14 @@ func TestParseRuntimeConfig(t *testing.T) { expectedAPIConfig: func() *serverstore.ResourceConfig { config := newFakeAPIResourceConfigSource() config.EnableVersions(scheme.PrioritizedVersionsAllGroups()...) + // disabling groups of APIs removes the individual resource preferences from the default + config.RemoveMatchingResourcePreferences(matchAllExplicitResourcesForFake) return config }, err: false, }, { - // only enable v1 + name: "only-enable-v1", runtimeConfig: map[string]string{ "api/all": "false", "/v1": "true", @@ -132,13 +153,16 @@ func TestParseRuntimeConfig(t *testing.T) { }, expectedAPIConfig: func() *serverstore.ResourceConfig { config := newFakeAPIResourceConfigSource() + config.DisableVersions(appsv1.SchemeGroupVersion) config.DisableVersions(extensionsapiv1beta1.SchemeGroupVersion) + // disabling groups of APIs removes the individual resource preferences from the default + config.RemoveMatchingResourcePreferences(matchAllExplicitResourcesForFake) return config }, err: false, }, { - // enable specific extensions resources + name: "enable-specific-extensions-resources", runtimeConfig: map[string]string{ "extensions/v1beta1/deployments": "true", }, @@ -153,7 +177,7 @@ func TestParseRuntimeConfig(t *testing.T) { err: false, }, { - // disable specific extensions resources + name: "disable-specific-extensions-resources", runtimeConfig: map[string]string{ "extensions/v1beta1/ingresses": "false", }, @@ -168,7 +192,7 @@ func TestParseRuntimeConfig(t *testing.T) { err: false, }, { - // disable all extensions resources + name: "disable-all-extensions-resources", runtimeConfig: map[string]string{ "extensions/v1beta1": "false", }, @@ -178,12 +202,14 @@ func TestParseRuntimeConfig(t *testing.T) { expectedAPIConfig: func() *serverstore.ResourceConfig { config := newFakeAPIResourceConfigSource() config.DisableVersions(extensionsapiv1beta1.SchemeGroupVersion) + // disabling groups of APIs removes the individual resource preferences from the default + config.RemoveMatchingResourcePreferences(matchAllExplicitResourcesForFake) return config }, err: false, }, { - // disable a non-extensions resource + name: "disable-a-no-extensions-resources", runtimeConfig: map[string]string{ "apps/v1/deployments": "false", }, @@ -191,12 +217,14 @@ func TestParseRuntimeConfig(t *testing.T) { return newFakeAPIResourceConfigSource() }, expectedAPIConfig: func() *serverstore.ResourceConfig { - return newFakeAPIResourceConfigSource() + config := newFakeAPIResourceConfigSource() + config.DisableResources(appsv1.SchemeGroupVersion.WithResource("deployments")) + return config }, err: false, // no error for backwards compatibility }, { - // disable all beta resources + name: "disable-all-beta-resources", runtimeConfig: map[string]string{ "api/beta": "false", }, @@ -206,24 +234,172 @@ func TestParseRuntimeConfig(t *testing.T) { expectedAPIConfig: func() *serverstore.ResourceConfig { config := newFakeAPIResourceConfigSource() config.DisableVersions(extensionsapiv1beta1.SchemeGroupVersion) + // disabling groups of APIs removes the individual resource preferences from the default + config.RemoveMatchingResourcePreferences(matchAllExplicitResourcesForFake) + return config + }, + err: false, // no error for backwards compatibility + }, + { + name: "user-explicit-disable-resource-over-user-version-enable", + runtimeConfig: map[string]string{ + "apps/v1": "true", + "apps/v1/deployments": "false", + }, + defaultResourceConfig: func() *serverstore.ResourceConfig { + return newFakeAPIResourceConfigSource() + }, + expectedAPIConfig: func() *serverstore.ResourceConfig { + config := newFakeAPIResourceConfigSource() + config.DisableResources(appsv1.SchemeGroupVersion.WithResource("deployments")) + return config + }, + err: false, // no error for backwards compatibility + }, + { + name: "user-explicit-enable-resource-over-user-version-disable", + runtimeConfig: map[string]string{ + "apps/v1": "false", + "apps/v1/deployments": "true", + }, + defaultResourceConfig: func() *serverstore.ResourceConfig { + return newFakeAPIResourceConfigSource() + }, + expectedAPIConfig: func() *serverstore.ResourceConfig { + config := newFakeAPIResourceConfigSource() + config.DisableVersions(appsv1.SchemeGroupVersion) + config.EnableResources(appsv1.SchemeGroupVersion.WithResource("deployments")) + return config + }, + err: false, // no error for backwards compatibility + }, + { + name: "user-explicit-disable-resource-over-user-stability-enable", + runtimeConfig: map[string]string{ + "api/ga": "true", + "apps/v1/deployments": "false", + }, + defaultResourceConfig: func() *serverstore.ResourceConfig { + return newFakeAPIResourceConfigSource() + }, + expectedAPIConfig: func() *serverstore.ResourceConfig { + config := newFakeAPIResourceConfigSource() + config.DisableResources(appsv1.SchemeGroupVersion.WithResource("deployments")) + return config + }, + err: false, // no error for backwards compatibility + }, + { + name: "user-explicit-enable-resource-over-user-stability-disable", + runtimeConfig: map[string]string{ + "api/ga": "false", + "apps/v1/deployments": "true", + }, + defaultResourceConfig: func() *serverstore.ResourceConfig { + return newFakeAPIResourceConfigSource() + }, + expectedAPIConfig: func() *serverstore.ResourceConfig { + config := newFakeAPIResourceConfigSource() + config.DisableVersions(apiv1.SchemeGroupVersion) + config.DisableVersions(appsv1.SchemeGroupVersion) + config.EnableResources(appsv1.SchemeGroupVersion.WithResource("deployments")) + return config + }, + err: false, // no error for backwards compatibility + }, + { + name: "user-explicit-disable-resource-over-user-version-enable-over-user-stability-disable", + runtimeConfig: map[string]string{ + "api/ga": "false", + "apps/v1": "true", + "apps/v1/deployments": "false", + }, + defaultResourceConfig: func() *serverstore.ResourceConfig { + return newFakeAPIResourceConfigSource() + }, + expectedAPIConfig: func() *serverstore.ResourceConfig { + config := newFakeAPIResourceConfigSource() + config.DisableVersions(apiv1.SchemeGroupVersion) + config.EnableVersions(appsv1.SchemeGroupVersion) + config.DisableResources(appsv1.SchemeGroupVersion.WithResource("deployments")) + return config + }, + err: false, // no error for backwards compatibility + }, + { + name: "user-explicit-enable-resource-over-user-version-disable-over-user-stability-disable", + runtimeConfig: map[string]string{ + "api/ga": "false", + "apps/v1": "false", + "apps/v1/deployments": "true", + }, + defaultResourceConfig: func() *serverstore.ResourceConfig { + return newFakeAPIResourceConfigSource() + }, + expectedAPIConfig: func() *serverstore.ResourceConfig { + config := newFakeAPIResourceConfigSource() + config.DisableVersions(apiv1.SchemeGroupVersion) + config.DisableVersions(appsv1.SchemeGroupVersion) + config.EnableResources(appsv1.SchemeGroupVersion.WithResource("deployments")) + return config + }, + err: false, // no error for backwards compatibility + }, + { + name: "user-explicit-disable-resource-over-user-version-enable-over-user-stability-enable", + runtimeConfig: map[string]string{ + "api/ga": "true", + "apps/v1": "true", + "apps/v1/deployments": "false", + }, + defaultResourceConfig: func() *serverstore.ResourceConfig { + return newFakeAPIResourceConfigSource() + }, + expectedAPIConfig: func() *serverstore.ResourceConfig { + config := newFakeAPIResourceConfigSource() + config.EnableVersions(appsv1.SchemeGroupVersion) + config.DisableResources(appsv1.SchemeGroupVersion.WithResource("deployments")) + return config + }, + err: false, // no error for backwards compatibility + }, + { + name: "user-explicit-enable-resource-over-user-version-disable-over-user-stability-enable", + runtimeConfig: map[string]string{ + "api/ga": "true", + "apps/v1": "false", + "apps/v1/deployments": "true", + }, + defaultResourceConfig: func() *serverstore.ResourceConfig { + return newFakeAPIResourceConfigSource() + }, + expectedAPIConfig: func() *serverstore.ResourceConfig { + config := newFakeAPIResourceConfigSource() + config.DisableVersions(appsv1.SchemeGroupVersion) + config.EnableResources(appsv1.SchemeGroupVersion.WithResource("deployments")) return config }, err: false, // no error for backwards compatibility }, } - for index, test := range testCases { - t.Log(scheme.PrioritizedVersionsAllGroups()) - actualDisablers, err := MergeAPIResourceConfigs(test.defaultResourceConfig(), test.runtimeConfig, scheme) - if err == nil && test.err { - t.Fatalf("expected error for test case: %v", index) - } else if err != nil && !test.err { - t.Fatalf("unexpected error: %s, for test: %v", err, test) - } + for _, test := range testCases { + t.Run(test.name, func(t *testing.T) { + t.Log(scheme.PrioritizedVersionsAllGroups()) + actualDisablers, err := MergeAPIResourceConfigs(test.defaultResourceConfig(), test.runtimeConfig, scheme) + if err == nil && test.err { + t.Fatalf("expected error") + } else if err != nil && !test.err { + t.Fatalf("unexpected error: %s, for test: %v", err, test) + } + if err != nil { + return + } - expectedConfig := test.expectedAPIConfig() - if err == nil && !reflect.DeepEqual(actualDisablers, expectedConfig) { - t.Fatalf("%v: unexpected apiResourceDisablers. Actual: %v\n expected: %v", test.runtimeConfig, actualDisablers, expectedConfig) - } + expectedConfig := test.expectedAPIConfig() + if !reflect.DeepEqual(actualDisablers, expectedConfig) { + t.Fatalf("%v: unexpected apiResourceDisablers. Actual: %v\n expected: %v", test.runtimeConfig, actualDisablers, expectedConfig) + } + }) } } @@ -232,6 +408,7 @@ func newFakeAPIResourceConfigSource() *serverstore.ResourceConfig { // NOTE: GroupVersions listed here will be enabled by default. Don't put alpha versions in the list. ret.EnableVersions( apiv1.SchemeGroupVersion, + appsv1.SchemeGroupVersion, extensionsapiv1beta1.SchemeGroupVersion, ) ret.EnableResources( @@ -246,9 +423,22 @@ func newFakeAPIResourceConfigSource() *serverstore.ResourceConfig { return ret } +func matchAllExplicitResourcesForFake(gvr schema.GroupVersionResource) bool { + switch gvr { + case extensionsapiv1beta1.SchemeGroupVersion.WithResource("ingresses"), + extensionsapiv1beta1.SchemeGroupVersion.WithResource("deployments"), + extensionsapiv1beta1.SchemeGroupVersion.WithResource("replicasets"), + extensionsapiv1beta1.SchemeGroupVersion.WithResource("daemonsets"): + return true + } + return false + +} + func newFakeScheme(t *testing.T) *runtime.Scheme { ret := runtime.NewScheme() require.NoError(t, apiv1.AddToScheme(ret)) + require.NoError(t, appsv1.AddToScheme(ret)) require.NoError(t, extensionsapiv1beta1.AddToScheme(ret)) require.NoError(t, ret.SetVersionPriority(apiv1.SchemeGroupVersion)) diff --git a/pkg/server/storage/resource_config.go b/pkg/server/storage/resource_config.go index bd85ac230..cba20ee77 100644 --- a/pkg/server/storage/resource_config.go +++ b/pkg/server/storage/resource_config.go @@ -70,6 +70,20 @@ func (o *ResourceConfig) EnableMatchingVersions(matcher func(gv schema.GroupVers } } +// RemoveMatchingResourcePreferences removes individual resource preferences that match. This is useful when an override of a version or level enablement should +// override the previously individual preferences. +func (o *ResourceConfig) RemoveMatchingResourcePreferences(matcher func(gvr schema.GroupVersionResource) bool) { + keysToRemove := []schema.GroupVersionResource{} + for k := range o.ResourceConfigs { + if matcher(k) { + keysToRemove = append(keysToRemove, k) + } + } + for _, k := range keysToRemove { + delete(o.ResourceConfigs, k) + } +} + // DisableVersions disables the versions entirely. func (o *ResourceConfig) DisableVersions(versions ...schema.GroupVersion) { for _, version := range versions {