Merge pull request kubernetes#87313 from MikeSpreitzer/apf-validation

Update validation for API Priority and Fairness

Author: k8s-ci-robot

pkg/apis/flowcontrol/BUILD

@@ -29,6 +29,7 @@ filegroup(
     srcs = [
         ":package-srcs",
         "//pkg/apis/flowcontrol/install:all-srcs",
+        "//pkg/apis/flowcontrol/internalbootstrap:all-srcs",
         "//pkg/apis/flowcontrol/util:all-srcs",
         "//pkg/apis/flowcontrol/v1alpha1:all-srcs",
         "//pkg/apis/flowcontrol/validation:all-srcs",

pkg/apis/flowcontrol/internalbootstrap/BUILD

@@ -0,0 +1,41 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+
+go_library(
+    name = "go_default_library",
+    srcs = ["default-internal.go"],
+    importpath = "k8s.io/kubernetes/pkg/apis/flowcontrol/internalbootstrap",
+    visibility = ["//visibility:public"],
+    deps = [
+        "//pkg/apis/flowcontrol:go_default_library",
+        "//pkg/apis/flowcontrol/install:go_default_library",
+        "//staging/src/k8s.io/api/flowcontrol/v1alpha1:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/conversion:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
+        "//staging/src/k8s.io/apiserver/pkg/apis/flowcontrol/bootstrap:go_default_library",
+    ],
+)
+
+filegroup(
+    name = "package-srcs",
+    srcs = glob(["**"]),
+    tags = ["automanaged"],
+    visibility = ["//visibility:private"],
+)
+
+filegroup(
+    name = "all-srcs",
+    srcs = [":package-srcs"],
+    tags = ["automanaged"],
+    visibility = ["//visibility:public"],
+)
+
+go_test(
+    name = "go_default_test",
+    srcs = ["defaults_test.go"],
+    embed = [":go_default_library"],
+    deps = [
+        "//staging/src/k8s.io/api/flowcontrol/v1alpha1:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/api/equality:go_default_library",
+        "//staging/src/k8s.io/apiserver/pkg/apis/flowcontrol/bootstrap:go_default_library",
+    ],
+)

pkg/apis/flowcontrol/internalbootstrap/default-internal.go

@@ -0,0 +1,76 @@
+/*
+Copyright 2019 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 internalbootstrap
+
+import (
+	fcv1a1 "k8s.io/api/flowcontrol/v1alpha1"
+	"k8s.io/apimachinery/pkg/conversion"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apiserver/pkg/apis/flowcontrol/bootstrap"
+	"k8s.io/kubernetes/pkg/apis/flowcontrol"
+	"k8s.io/kubernetes/pkg/apis/flowcontrol/install"
+)
+
+// MandatoryFlowSchemas holds the untyped renditions of the mandatory
+// flow schemas.  In this map the key is the schema's name and the
+// value is the `*FlowSchema`.  Nobody should mutate anything
+// reachable from this map.
+var MandatoryFlowSchemas = internalizeFSes(bootstrap.MandatoryFlowSchemas)
+
+// MandatoryPriorityLevelConfigurations holds the untyped renditions of the
+// mandatory priority level configuration objects.  In this map the
+// key is the object's name and the value is the
+// `*PriorityLevelConfiguration`.  Nobody should mutate anything
+// reachable from this map.
+var MandatoryPriorityLevelConfigurations = internalizePLs(bootstrap.MandatoryPriorityLevelConfigurations)
+
+// NewAPFScheme constructs and returns a Scheme configured to handle
+// the API object types that are used to configure API Priority and
+// Fairness
+func NewAPFScheme() *runtime.Scheme {
+	scheme := runtime.NewScheme()
+	install.Install(scheme)
+	return scheme
+}
+
+func internalizeFSes(exts []*fcv1a1.FlowSchema) map[string]*flowcontrol.FlowSchema {
+	ans := make(map[string]*flowcontrol.FlowSchema, len(exts))
+	converter := NewAPFScheme().Converter()
+	for _, ext := range exts {
+		var untyped flowcontrol.FlowSchema
+		err := converter.Convert(ext, &untyped, 0, &conversion.Meta{})
+		if err != nil {
+			panic(err)
+		}
+		ans[ext.Name] = &untyped
+	}
+	return ans
+}
+
+func internalizePLs(exts []*fcv1a1.PriorityLevelConfiguration) map[string]*flowcontrol.PriorityLevelConfiguration {
+	ans := make(map[string]*flowcontrol.PriorityLevelConfiguration, len(exts))
+	converter := NewAPFScheme().Converter()
+	for _, ext := range exts {
+		var untyped flowcontrol.PriorityLevelConfiguration
+		err := converter.Convert(ext, &untyped, 0, &conversion.Meta{})
+		if err != nil {
+			panic(err)
+		}
+		ans[ext.Name] = &untyped
+	}
+	return ans
+}

pkg/apis/flowcontrol/internalbootstrap/defaults_test.go

@@ -0,0 +1,47 @@
+/*
+Copyright 2020 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 internalbootstrap
+
+import (
+	"testing"
+
+	fcv1a1 "k8s.io/api/flowcontrol/v1alpha1"
+	apiequality "k8s.io/apimachinery/pkg/api/equality"
+	"k8s.io/apiserver/pkg/apis/flowcontrol/bootstrap"
+)
+
+func TestMandatoryAlreadyDefaulted(t *testing.T) {
+	scheme := NewAPFScheme()
+	for _, obj := range bootstrap.MandatoryFlowSchemas {
+		obj2 := obj.DeepCopyObject().(*fcv1a1.FlowSchema)
+		scheme.Default(obj2)
+		if apiequality.Semantic.DeepEqual(obj, obj2) {
+			t.Logf("Defaulting makes no change to %#+v", *obj)
+		} else {
+			t.Errorf("Defaulting changed %#+v to %#+v", *obj, *obj2)
+		}
+	}
+	for _, obj := range bootstrap.MandatoryPriorityLevelConfigurations {
+		obj2 := obj.DeepCopyObject().(*fcv1a1.PriorityLevelConfiguration)
+		scheme.Default(obj2)
+		if apiequality.Semantic.DeepEqual(obj, obj2) {
+			t.Logf("Defaulting makes no change to %#+v", *obj)
+		} else {
+			t.Errorf("Defaulting changed %#+v to %#+v", *obj, *obj2)
+		}
+	}
+}

pkg/apis/flowcontrol/types.go

@@ -33,7 +33,10 @@ const (
 
 // System preset priority level names
 const (
-	PriorityLevelConfigurationNameExempt = "exempt"
+	PriorityLevelConfigurationNameExempt   = "exempt"
+	PriorityLevelConfigurationNameCatchAll = "catch-all"
+	FlowSchemaNameExempt                   = "exempt"
+	FlowSchemaNameCatchAll                 = "catch-all"
 )
 
 // Conditions

pkg/apis/flowcontrol/validation/BUILD

@@ -8,6 +8,8 @@ go_library(
     deps = [
         "//pkg/apis/core/validation:go_default_library",
         "//pkg/apis/flowcontrol:go_default_library",
+        "//pkg/apis/flowcontrol/internalbootstrap:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/api/equality:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/api/validation:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
@@ -23,6 +25,7 @@ go_test(
         "//pkg/apis/flowcontrol:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
+        "//staging/src/k8s.io/apiserver/pkg/authentication/user:go_default_library",
         "//vendor/github.com/google/go-cmp/cmp:go_default_library",
         "//vendor/github.com/stretchr/testify/assert:go_default_library",
     ],

pkg/apis/flowcontrol/validation/validation.go

@@ -20,13 +20,14 @@ import (
 	"fmt"
 	"strings"
 
-	"k8s.io/apimachinery/pkg/api/validation"
+	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/apiserver/pkg/util/shufflesharding"
 	apivalidation "k8s.io/kubernetes/pkg/apis/core/validation"
 	"k8s.io/kubernetes/pkg/apis/flowcontrol"
+	"k8s.io/kubernetes/pkg/apis/flowcontrol/internalbootstrap"
 )
 
 // ValidateFlowSchemaName validates name for flow-schema.
@@ -73,7 +74,17 @@ var supportedLimitResponseType = sets.NewString(
 // ValidateFlowSchema validates the content of flow-schema
 func ValidateFlowSchema(fs *flowcontrol.FlowSchema) field.ErrorList {
 	allErrs := apivalidation.ValidateObjectMeta(&fs.ObjectMeta, false, ValidateFlowSchemaName, field.NewPath("metadata"))
-	allErrs = append(allErrs, ValidateFlowSchemaSpec(&fs.Spec, field.NewPath("spec"))...)
+	specPath := field.NewPath("spec")
+	allErrs = append(allErrs, ValidateFlowSchemaSpec(fs.Name, &fs.Spec, specPath)...)
+	if mand, ok := internalbootstrap.MandatoryFlowSchemas[fs.Name]; ok {
+		// Check for almost exact equality.  This is a pretty
+		// strict test, and it is OK in this context because both
+		// sides of this comparison are intended to ultimately
+		// come from the same code.
+		if !apiequality.Semantic.DeepEqual(fs.Spec, mand.Spec) {
+			allErrs = append(allErrs, field.Invalid(specPath, fs.Spec, fmt.Sprintf("spec of '%s' must equal the fixed value", fs.Name)))
+		}
+	}
 	allErrs = append(allErrs, ValidateFlowSchemaStatus(&fs.Status, field.NewPath("status"))...)
 	return allErrs
 }
@@ -84,14 +95,17 @@ func ValidateFlowSchemaUpdate(old, fs *flowcontrol.FlowSchema) field.ErrorList {
 }
 
 // ValidateFlowSchemaSpec validates the content of flow-schema's spec
-func ValidateFlowSchemaSpec(spec *flowcontrol.FlowSchemaSpec, fldPath *field.Path) field.ErrorList {
+func ValidateFlowSchemaSpec(fsName string, spec *flowcontrol.FlowSchemaSpec, fldPath *field.Path) field.ErrorList {
 	var allErrs field.ErrorList
 	if spec.MatchingPrecedence <= 0 {
 		allErrs = append(allErrs, field.Invalid(fldPath.Child("matchingPrecedence"), spec.MatchingPrecedence, "must be a positive value"))
 	}
 	if spec.MatchingPrecedence > flowcontrol.FlowSchemaMaxMatchingPrecedence {
 		allErrs = append(allErrs, field.Invalid(fldPath.Child("matchingPrecedence"), spec.MatchingPrecedence, fmt.Sprintf("must not be greater than %v", flowcontrol.FlowSchemaMaxMatchingPrecedence)))
 	}
+	if (spec.MatchingPrecedence == 1) && (fsName != flowcontrol.FlowSchemaNameExempt) {
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("matchingPrecedence"), spec.MatchingPrecedence, "only the schema named 'exempt' may have matchingPrecedence 1"))
+	}
 	if spec.DistinguisherMethod != nil {
 		if !supportedDistinguisherMethods.Has(string(spec.DistinguisherMethod.Type)) {
 			allErrs = append(allErrs, field.NotSupported(fldPath.Child("distinguisherMethod").Child("type"), spec.DistinguisherMethod, supportedDistinguisherMethods.List()))
@@ -139,10 +153,28 @@ func ValidateFlowSchemaSubject(subject *flowcontrol.Subject, fldPath *field.Path
 	switch subject.Kind {
 	case flowcontrol.SubjectKindServiceAccount:
 		allErrs = append(allErrs, ValidateServiceAccountSubject(subject.ServiceAccount, fldPath.Child("serviceAccount"))...)
+		if subject.User != nil {
+			allErrs = append(allErrs, field.Forbidden(fldPath.Child("user"), "user is forbidden when subject kind is not 'User'"))
+		}
+		if subject.Group != nil {
+			allErrs = append(allErrs, field.Forbidden(fldPath.Child("group"), "group is forbidden when subject kind is not 'Group'"))
+		}
 	case flowcontrol.SubjectKindUser:
 		allErrs = append(allErrs, ValidateUserSubject(subject.User, fldPath.Child("user"))...)
+		if subject.ServiceAccount != nil {
+			allErrs = append(allErrs, field.Forbidden(fldPath.Child("serviceAccount"), "serviceAccount is forbidden when subject kind is not 'ServiceAccount'"))
+		}
+		if subject.Group != nil {
+			allErrs = append(allErrs, field.Forbidden(fldPath.Child("group"), "group is forbidden when subject kind is not 'Group'"))
+		}
 	case flowcontrol.SubjectKindGroup:
 		allErrs = append(allErrs, ValidateGroupSubject(subject.Group, fldPath.Child("group"))...)
+		if subject.ServiceAccount != nil {
+			allErrs = append(allErrs, field.Forbidden(fldPath.Child("serviceAccount"), "serviceAccount is forbidden when subject kind is not 'ServiceAccount'"))
+		}
+		if subject.User != nil {
+			allErrs = append(allErrs, field.Forbidden(fldPath.Child("user"), "user is forbidden when subject kind is not 'User'"))
+		}
 	default:
 		allErrs = append(allErrs, field.NotSupported(fldPath.Child("kind"), subject.Kind, supportedSubjectKinds.List()))
 	}
@@ -152,10 +184,13 @@ func ValidateFlowSchemaSubject(subject *flowcontrol.Subject, fldPath *field.Path
 // ValidateServiceAccountSubject validates subject of "ServiceAccount" kind
 func ValidateServiceAccountSubject(subject *flowcontrol.ServiceAccountSubject, fldPath *field.Path) field.ErrorList {
 	var allErrs field.ErrorList
+	if subject == nil {
+		return append(allErrs, field.Required(fldPath, "serviceAccount is required when subject kind is 'ServiceAccount'"))
+	}
 	if len(subject.Name) == 0 {
 		allErrs = append(allErrs, field.Required(fldPath.Child("name"), ""))
 	} else if subject.Name != flowcontrol.NameAll {
-		for _, msg := range validation.ValidateServiceAccountName(subject.Name, false) {
+		for _, msg := range apimachineryvalidation.ValidateServiceAccountName(subject.Name, false) {
 			allErrs = append(allErrs, field.Invalid(fldPath.Child("name"), subject.Name, msg))
 		}
 	}
@@ -174,6 +209,9 @@ func ValidateServiceAccountSubject(subject *flowcontrol.ServiceAccountSubject, f
 // ValidateUserSubject validates subject of "User" kind
 func ValidateUserSubject(subject *flowcontrol.UserSubject, fldPath *field.Path) field.ErrorList {
 	var allErrs field.ErrorList
+	if subject == nil {
+		return append(allErrs, field.Required(fldPath, "user is required when subject kind is 'User'"))
+	}
 	if len(subject.Name) == 0 {
 		allErrs = append(allErrs, field.Required(fldPath.Child("name"), ""))
 	}
@@ -183,6 +221,9 @@ func ValidateUserSubject(subject *flowcontrol.UserSubject, fldPath *field.Path)
 // ValidateGroupSubject validates subject of "Group" kind
 func ValidateGroupSubject(subject *flowcontrol.GroupSubject, fldPath *field.Path) field.ErrorList {
 	var allErrs field.ErrorList
+	if subject == nil {
+		return append(allErrs, field.Required(fldPath, "group is required when subject kind is 'Group'"))
+	}
 	if len(subject.Name) == 0 {
 		allErrs = append(allErrs, field.Required(fldPath.Child("name"), ""))
 	}
@@ -298,7 +339,17 @@ func ValidateFlowSchemaCondition(condition *flowcontrol.FlowSchemaCondition, fld
 // ValidatePriorityLevelConfiguration validates priority-level-configuration.
 func ValidatePriorityLevelConfiguration(pl *flowcontrol.PriorityLevelConfiguration) field.ErrorList {
 	allErrs := apivalidation.ValidateObjectMeta(&pl.ObjectMeta, false, ValidatePriorityLevelConfigurationName, field.NewPath("metadata"))
-	allErrs = append(allErrs, ValidatePriorityLevelConfigurationSpec(&pl.Spec, pl.Name, field.NewPath("spec"))...)
+	specPath := field.NewPath("spec")
+	allErrs = append(allErrs, ValidatePriorityLevelConfigurationSpec(&pl.Spec, pl.Name, specPath)...)
+	if mand, ok := internalbootstrap.MandatoryPriorityLevelConfigurations[pl.Name]; ok {
+		// Check for almost exact equality.  This is a pretty
+		// strict test, and it is OK in this context because both
+		// sides of this comparison are intended to ultimately
+		// come from the same code.
+		if !apiequality.Semantic.DeepEqual(pl.Spec, mand.Spec) {
+			allErrs = append(allErrs, field.Invalid(specPath, pl.Spec, fmt.Sprintf("spec of '%s' must equal the fixed value", pl.Name)))
+		}
+	}
 	allErrs = append(allErrs, ValidatePriorityLevelConfigurationStatus(&pl.Status, field.NewPath("status"))...)
 	return allErrs
 }
@@ -311,14 +362,17 @@ func ValidatePriorityLevelConfigurationUpdate(old, pl *flowcontrol.PriorityLevel
 // ValidatePriorityLevelConfigurationSpec validates priority-level-configuration's spec.
 func ValidatePriorityLevelConfigurationSpec(spec *flowcontrol.PriorityLevelConfigurationSpec, name string, fldPath *field.Path) field.ErrorList {
 	var allErrs field.ErrorList
+	if (name == flowcontrol.PriorityLevelConfigurationNameExempt) != (spec.Type == flowcontrol.PriorityLevelEnablementExempt) {
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("type"), spec.Type, "type must be 'Exempt' if and only if name is 'exempt'"))
+	}
 	switch spec.Type {
 	case flowcontrol.PriorityLevelEnablementExempt:
 		if spec.Limited != nil {
 			allErrs = append(allErrs, field.Forbidden(fldPath.Child("limited"), "must be nil if the type is not Limited"))
 		}
 	case flowcontrol.PriorityLevelEnablementLimited:
 		if spec.Limited == nil {
-			allErrs = append(allErrs, field.Required(fldPath.Child("limited"), "must not be empty"))
+			allErrs = append(allErrs, field.Required(fldPath.Child("limited"), "must not be empty when type is Limited"))
 		} else {
 			allErrs = append(allErrs, ValidateLimitedPriorityLevelConfiguration(spec.Limited, fldPath.Child("limited"))...)
 		}
@@ -344,11 +398,11 @@ func ValidateLimitResponse(lr flowcontrol.LimitResponse, fldPath *field.Path) fi
 	switch lr.Type {
 	case flowcontrol.LimitResponseTypeReject:
 		if lr.Queuing != nil {
-			allErrs = append(allErrs, field.Forbidden(fldPath.Child("queuing"), "must be nil if the type is not Limited"))
+			allErrs = append(allErrs, field.Forbidden(fldPath.Child("queuing"), "must be nil if limited.limitResponse.type is not Limited"))
 		}
 	case flowcontrol.LimitResponseTypeQueue:
 		if lr.Queuing == nil {
-			allErrs = append(allErrs, field.Required(fldPath.Child("queuing"), "must not be empty"))
+			allErrs = append(allErrs, field.Required(fldPath.Child("queuing"), "must not be empty if limited.limitResponse.type is Limited"))
 		} else {
 			allErrs = append(allErrs, ValidatePriorityLevelQueuingConfiguration(lr.Queuing, fldPath.Child("queuing"))...)
 		}