From a912bd8488014d6cf03f70d1afc69edc5b1f693e Mon Sep 17 00:00:00 2001 From: Mike Spreitzer Date: Wed, 13 Nov 2019 10:32:50 -0500 Subject: [PATCH] Identify cluster scope by a boolean field rather than a special namespace --- pkg/apis/flowcontrol/types.go | 39 ++++++++---------- pkg/apis/flowcontrol/validation/validation.go | 18 +++----- .../flowcontrol/validation/validation_test.go | 13 +++--- .../k8s.io/api/flowcontrol/v1alpha1/types.go | 41 ++++++++----------- 4 files changed, 46 insertions(+), 65 deletions(-) diff --git a/pkg/apis/flowcontrol/types.go b/pkg/apis/flowcontrol/types.go index 22f95346e26..8baa3f43f3e 100644 --- a/pkg/apis/flowcontrol/types.go +++ b/pkg/apis/flowcontrol/types.go @@ -29,7 +29,6 @@ const ( NameAll = "*" NamespaceEvery = "*" // matches every particular namespace - NamespaceClusterScope = "Cluster Scope" // matches absence of namespace ) // System preset priority level names @@ -240,29 +239,23 @@ type ResourcePolicyRule struct { // +listType=set Resources []string + // `clusterScope` indicates whether to match requests that do not + // specify a namespace (which happens either because the resource + // is not namespaced or the request targets all namespaces). + // If this field is omitted or false then the `namespaces` field + // must contain a non-empty list. + // +optional + ClusterScope bool + // `namespaces` is a list of target namespaces that restricts - // matches. A request that does not specify a target namespace - // (which happens both when the resource is not namespaced and - // when the resource is namespaced and the request is for all - // namespaces) matches only if this list includes "Cluster Scope" - // (this string is not a valid namespace and thus can not be - // confused with an actual namespace). A request that specifies a - // target namespace matches only if either (a) this list contains - // that target namespace or (b) this list contains "*". - // - // This list may not be omitted or empty. If the list contains - // "*" then the only other allowed member is "Cluster Scope". - // Without "*", it is allowed to list "Cluster Scope" along with - // particular namespaces. - // - // Requests will match only if the values in this list are - // appropriate for the resource(s) involved. For example: for a - // cluster scoped resource (i.e., one not namespaced) a request - // can match only if this list contains "Cluster Scope". It is - // entirely up to the client to populate this list with - // appropriate values; the server-performed validation does not - // (at least in this alpha) address this issue. - // + // matches. A request that specifies a target namespace matches + // only if either (a) this list contains that target namespace or + // (b) this list contains "*". Note that "*" matches any + // specified namespace but does not match a request that _does + // not specify_ a namespace (see the `clusterScope` field for + // that). + // This list may be empty, but only if `clusterScope` is true. + // +optional // +listType=set Namespaces []string } diff --git a/pkg/apis/flowcontrol/validation/validation.go b/pkg/apis/flowcontrol/validation/validation.go index 6bd6fd3b88b..1fd6da9ca35 100644 --- a/pkg/apis/flowcontrol/validation/validation.go +++ b/pkg/apis/flowcontrol/validation/validation.go @@ -245,20 +245,14 @@ func ValidateFlowSchemaResourcePolicyRule(rule *flowcontrol.ResourcePolicyRule, allErrs = append(allErrs, field.Invalid(fldPath.Child("resources"), rule.Resources, "if '*' is present, must not specify other resources")) } - if len(rule.Namespaces) == 0 { - allErrs = append(allErrs, field.Required(fldPath.Child("namespaces"), "resource rules must supply at least one namespace")) - } else if memberInList(flowcontrol.NamespaceEvery, rule.Namespaces...) { - for _, tgtNS := range rule.Namespaces { - if tgtNS != flowcontrol.NamespaceEvery && tgtNS != flowcontrol.NamespaceClusterScope { - allErrs = append(allErrs, field.Invalid(fldPath.Child("namespaces"), rule.Namespaces, "'*' may be accompanied only by 'Cluster Scope'")) - break - } + if len(rule.Namespaces) == 0 && !rule.ClusterScope { + allErrs = append(allErrs, field.Required(fldPath.Child("namespaces"), "resource rules that are not cluster scoped must supply at least one namespace")) + } else if hasWildcard(rule.Namespaces) { + if len(rule.Namespaces) > 1 { + allErrs = append(allErrs, field.Invalid(fldPath.Child("namespaces"), rule.Namespaces, "if '*' is present, must not specify other namespaces")) } } else { for idx, tgtNS := range rule.Namespaces { - if tgtNS == flowcontrol.NamespaceClusterScope { - continue - } for _, msg := range apimachineryvalidation.ValidateNamespaceName(tgtNS, false) { allErrs = append(allErrs, field.Invalid(fldPath.Child("namespaces").Index(idx), tgtNS, nsErrIntro+msg)) } @@ -268,7 +262,7 @@ func ValidateFlowSchemaResourcePolicyRule(rule *flowcontrol.ResourcePolicyRule, return allErrs } -const nsErrIntro = "each member of this list must be '*', 'Cluster Scope', or a DNS-1123 label; " +const nsErrIntro = "each member of this list must be '*' or a DNS-1123 label; " // ValidateFlowSchemaStatus validates status for the flow-schema. func ValidateFlowSchemaStatus(status *flowcontrol.FlowSchemaStatus, fldPath *field.Path) field.ErrorList { diff --git a/pkg/apis/flowcontrol/validation/validation_test.go b/pkg/apis/flowcontrol/validation/validation_test.go index 77e015fde91..92c44225705 100644 --- a/pkg/apis/flowcontrol/validation/validation_test.go +++ b/pkg/apis/flowcontrol/validation/validation_test.go @@ -407,11 +407,11 @@ func TestFlowSchemaValidation(t *testing.T) { }, }, expectedErrors: field.ErrorList{ - field.Required(field.NewPath("spec").Child("rules").Index(0).Child("resourceRules").Index(0).Child("namespaces"), "resource rules must supply at least one namespace"), + field.Required(field.NewPath("spec").Child("rules").Index(0).Child("resourceRules").Index(0).Child("namespaces"), "resource rules that are not cluster scoped must supply at least one namespace"), }, }, { - name: "NamespaceClusterScope is allowed", + name: "ClusterScope is allowed, with no Namespaces", flowSchema: &flowcontrol.FlowSchema{ ObjectMeta: metav1.ObjectMeta{ Name: "system-foo", @@ -434,7 +434,7 @@ func TestFlowSchemaValidation(t *testing.T) { Verbs: []string{flowcontrol.VerbAll}, APIGroups: []string{flowcontrol.APIGroupAll}, Resources: []string{flowcontrol.ResourceAll}, - Namespaces: []string{flowcontrol.NamespaceClusterScope}, + ClusterScope: true, }, }, }, @@ -444,7 +444,7 @@ func TestFlowSchemaValidation(t *testing.T) { expectedErrors: field.ErrorList{}, }, { - name: "NamespaceClusterScope is allowed with NamespaceEvery", + name: "ClusterScope is allowed with NamespaceEvery", flowSchema: &flowcontrol.FlowSchema{ ObjectMeta: metav1.ObjectMeta{ Name: "system-foo", @@ -467,7 +467,8 @@ func TestFlowSchemaValidation(t *testing.T) { Verbs: []string{flowcontrol.VerbAll}, APIGroups: []string{flowcontrol.APIGroupAll}, Resources: []string{flowcontrol.ResourceAll}, - Namespaces: []string{flowcontrol.NamespaceClusterScope, flowcontrol.NamespaceEvery}, + ClusterScope: true, + Namespaces: []string{flowcontrol.NamespaceEvery}, }, }, }, @@ -508,7 +509,7 @@ func TestFlowSchemaValidation(t *testing.T) { }, }, expectedErrors: field.ErrorList{ - field.Invalid(field.NewPath("spec").Child("rules").Index(0).Child("resourceRules").Index(0).Child("namespaces"), []string{"foo", flowcontrol.NamespaceEvery}, "'*' may be accompanied only by 'Cluster Scope'"), + field.Invalid(field.NewPath("spec").Child("rules").Index(0).Child("resourceRules").Index(0).Child("namespaces"), []string{"foo", flowcontrol.NamespaceEvery}, "if '*' is present, must not specify other namespaces"), }, }, { diff --git a/staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go b/staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go index c6e8ea43e5e..136d1cf70d4 100644 --- a/staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go +++ b/staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go @@ -29,7 +29,6 @@ const ( NameAll = "*" NamespaceEvery = "*" // matches every particular namespace - NamespaceClusterScope = "Cluster Scope" // matches absence of namespace ) // System preset priority level names @@ -240,31 +239,25 @@ type ResourcePolicyRule struct { // +listType=set Resources []string `json:"resources" protobuf:"bytes,3,rep,name=resources"` + // `clusterScope` indicates whether to match requests that do not + // specify a namespace (which happens either because the resource + // is not namespaced or the request targets all namespaces). + // If this field is omitted or false then the `namespaces` field + // must contain a non-empty list. + // +optional + ClusterScope bool `json:"clusterScope,omitempty" protobuf:"varint,4,opt,name=clusterScope"` + // `namespaces` is a list of target namespaces that restricts - // matches. A request that does not specify a target namespace - // (which happens both when the resource is not namespaced and - // when the resource is namespaced and the request is for all - // namespaces) matches only if this list includes "Cluster Scope" - // (this string is not a valid namespace and thus can not be - // confused with an actual namespace). A request that specifies a - // target namespace matches only if either (a) this list contains - // that target namespace or (b) this list contains "*". - // - // This list may not be omitted or empty. If the list contains - // "*" then the only other allowed member is "Cluster Scope". - // Without "*", it is allowed to list "Cluster Scope" along with - // particular namespaces. - // - // Requests will match only if the values in this list are - // appropriate for the resource(s) involved. For example: for a - // cluster scoped resource (i.e., one not namespaced) a request - // can match only if this list contains "Cluster Scope". It is - // entirely up to the client to populate this list with - // appropriate values; the server-performed validation does not - // (at least in this alpha) address this issue. - // + // matches. A request that specifies a target namespace matches + // only if either (a) this list contains that target namespace or + // (b) this list contains "*". Note that "*" matches any + // specified namespace but does not match a request that _does + // not specify_ a namespace (see the `clusterScope` field for + // that). + // This list may be empty, but only if `clusterScope` is true. + // +optional // +listType=set - Namespaces []string `json:"namespaces" protobuf:"bytes,4,rep,name=namespaces"` + Namespaces []string `json:"namespaces" protobuf:"bytes,5,rep,name=namespaces"` } // NonResourcePolicyRule is a predicate that matches non-resource requests according to their verb and the