Merge pull request #133466 from bart0sh/PR193-refactor-api-enablement

Refactor API group validation and error reporting

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

@@ -22,6 +22,7 @@ import (
 
 	"github.com/spf13/pflag"
 
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apiserver/pkg/server"
 	"k8s.io/apiserver/pkg/server/resourceconfig"
 	serverstore "k8s.io/apiserver/pkg/server/storage"
@@ -70,20 +71,22 @@ func (s *APIEnablementOptions) Validate(registries ...GroupRegistry) []error {
 	errors := []error{}
 	if s.RuntimeConfig[resourceconfig.APIAll] == "false" && len(s.RuntimeConfig) == 1 {
 		// Do not allow only set api/all=false, in such case apiserver startup has no meaning.
-		return append(errors, fmt.Errorf("invalid key with only %v=false", resourceconfig.APIAll))
+		return append(errors, fmt.Errorf("invalid runtime-config with only %v=false", resourceconfig.APIAll))
 	}
 
-	groups, err := resourceconfig.ParseGroups(s.RuntimeConfig)
+	groupVersions, err := resourceconfig.ParseGroupVersions(s.RuntimeConfig)
 	if err != nil {
 		return append(errors, err)
 	}
 
-	for _, registry := range registries {
-		// filter out known groups
-		groups = unknownGroups(groups, registry)
+	unknownGroupVersions := sets.New[string]()
+	for _, groupVersion := range groupVersions {
+		if !isGroupRegistered(groupVersion.Group, registries) {
+			unknownGroupVersions.Insert(groupVersion.String())
+		}
 	}
-	if len(groups) != 0 {
-		errors = append(errors, fmt.Errorf("unknown api groups %s", strings.Join(groups, ",")))
+	if len(unknownGroupVersions) != 0 {
+		errors = append(errors, fmt.Errorf("unknown api groups %s", strings.Join(sets.List(unknownGroupVersions), ",")))
 	}
 
 	return errors
@@ -129,18 +132,17 @@ func (s *APIEnablementOptions) ApplyTo(c *server.Config, defaultResourceConfig *
 	return err
 }
 
-func unknownGroups(groups []string, registry GroupRegistry) []string {
-	unknownGroups := []string{}
-	for _, group := range groups {
-		if !registry.IsGroupRegistered(group) {
-			unknownGroups = append(unknownGroups, group)
-		}
-	}
-	return unknownGroups
-}
-
 // GroupRegistry provides a method to check whether given group is registered.
 type GroupRegistry interface {
-	// IsRegistered returns true if given group is registered.
+	// IsGroupRegistered returns true if given group is registered.
 	IsGroupRegistered(group string) bool
 }
+
+func isGroupRegistered(group string, registries []GroupRegistry) bool {
+	for _, registry := range registries {
+		if registry.IsGroupRegistered(group) {
+			return true
+		}
+	}
+	return false
+}

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

@@ -48,9 +48,9 @@ func TestAPIEnablementOptionsValidate(t *testing.T) {
 			name: "test when options is nil",
 		},
 		{
-			name:          "test when invalid key with only api/all=false",
+			name:          "test when invalid runtime-config with only api/all=false",
 			runtimeConfig: cliflag.ConfigurationMap{"api/all": "false"},
-			expectErr:     "invalid key with only api/all=false",
+			expectErr:     "invalid runtime-config with only api/all=false",
 		},
 		{
 			name:          "test when ConfigurationMap key is invalid",
@@ -59,13 +59,17 @@ func TestAPIEnablementOptionsValidate(t *testing.T) {
 		},
 		{
 			name:          "test when unknown api groups",
-			runtimeConfig: cliflag.ConfigurationMap{"api/v1": "true"},
-			expectErr:     "unknown api groups",
+			runtimeConfig: cliflag.ConfigurationMap{"api/v1": "true", "api/v1beta2": "true"},
+			expectErr:     "unknown api groups api/v1,api/v1beta2",
 		},
 		{
 			name:          "test when valid api groups",
 			runtimeConfig: cliflag.ConfigurationMap{"apiregistration.k8s.io/v1beta1": "true"},
 		},
+		{
+			name:          "test when invalid api groups",
+			runtimeConfig: cliflag.ConfigurationMap{"apiregistration.k8s.io/v1beta1": "true"},
+		},
 	}
 	testGroupRegistry := fakeGroupRegistry{}
 

staging/src/k8s.io/apiserver/pkg/server/resourceconfig/helpers.go

@@ -221,24 +221,37 @@ func getRuntimeConfigValue(overrides cliflag.ConfigurationMap, apiKey string, de
 
 // ParseGroups takes in resourceConfig and returns parsed groups.
 func ParseGroups(resourceConfig cliflag.ConfigurationMap) ([]string, error) {
+	groupVersions, err := ParseGroupVersions(resourceConfig)
+	if err != nil {
+		return nil, err
+	}
 	groups := []string{}
+	for _, gv := range groupVersions {
+		groups = append(groups, gv.Group)
+	}
+	return groups, nil
+}
+
+// ParseGroupVersions takes in resourceConfig and returns parsed group versions.
+func ParseGroupVersions(resourceConfig cliflag.ConfigurationMap) ([]schema.GroupVersion, error) {
+	groupVersions := []schema.GroupVersion{}
 	for key := range resourceConfig {
 		if _, ok := groupVersionMatchers[key]; ok {
 			continue
 		}
 		tokens := strings.Split(key, "/")
 		if len(tokens) != 2 && len(tokens) != 3 {
-			return groups, fmt.Errorf("runtime-config invalid key %s", key)
+			return nil, fmt.Errorf("runtime-config invalid key %s", key)
 		}
 		groupVersionString := tokens[0] + "/" + tokens[1]
 		groupVersion, err := schema.ParseGroupVersion(groupVersionString)
 		if err != nil {
 			return nil, fmt.Errorf("runtime-config invalid key %s", key)
 		}
-		groups = append(groups, groupVersion.Group)
+		groupVersions = append(groupVersions, groupVersion)
 	}
 
-	return groups, nil
+	return groupVersions, nil
 }
 
 // EmulationForwardCompatibleResourceConfig creates a new ResourceConfig that besides all the enabled resources in resourceConfig,