From 263a0430457670d9201504b431e58245e87db7da Mon Sep 17 00:00:00 2001 From: Ashwin Venkatesh Date: Fri, 6 Nov 2020 16:35:40 -0500 Subject: [PATCH 1/4] Add descriptions and missing validation for service-intentions --- api/v1alpha1/serviceintentions_types.go | 103 +++++++-- api/v1alpha1/serviceintentions_types_test.go | 214 ++++++++++++++++-- ...onsul.hashicorp.com_serviceintentions.yaml | 24 +- 3 files changed, 300 insertions(+), 41 deletions(-) diff --git a/api/v1alpha1/serviceintentions_types.go b/api/v1alpha1/serviceintentions_types.go index 3cca51bf38..a36a836124 100644 --- a/api/v1alpha1/serviceintentions_types.go +++ b/api/v1alpha1/serviceintentions_types.go @@ -2,6 +2,7 @@ package v1alpha1 import ( "encoding/json" + "strings" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" @@ -18,12 +19,21 @@ import ( // ServiceIntentionsSpec defines the desired state of ServiceIntentions type ServiceIntentionsSpec struct { - Destination Destination `json:"destination,omitempty"` - Sources SourceIntentions `json:"sources,omitempty"` + Destination Destination `json:"destination,omitempty"` + // Sources is the list of all intention sources and the authorization granted to those sources. + // The order of this list does not matter, but out of convenience Consul will always store this + // reverse sorted by intention precedence, as that is the order that they will be evaluated at enforcement time. + Sources SourceIntentions `json:"sources,omitempty"` } type Destination struct { - Name string `json:"name,omitempty"` + // Name is the destination of all intentions defined in this config entry. + // This may be set to the wildcard character (*) to match + // all services that don't otherwise have intentions defined. + Name string `json:"name,omitempty"` + // Namespace specifies the namespace the config entry will apply to. + // This may be set to the wildcard character (*) to match all services + // in all namespaces that don't otherwise have intentions defined. Namespace string `json:"namespace,omitempty"` } @@ -32,36 +42,63 @@ type IntentionPermissions []*IntentionPermission type IntentionHTTPHeaderPermissions []IntentionHTTPHeaderPermission type SourceIntention struct { - Name string `json:"name,omitempty"` - Namespace string `json:"namespace,omitempty"` - Action IntentionAction `json:"action,omitempty"` + // Name is the source of the intention. This is the name of a + // Consul service. The service doesn't need to be registered. + Name string `json:"name,omitempty"` + // Namespace is the namespace for the Name parameter. + Namespace string `json:"namespace,omitempty"` + // Action is required for an L4 intention, and should be set to one of + // "allow" or "deny" for the action that should be taken if this intention matches a request. + Action IntentionAction `json:"action,omitempty"` + // Permissions is the list of all additional L7 attributes that extend the intention match criteria. + // Permission precedence is applied top to bottom. For any given request the first permission to match + // in the list is terminal and stops further evaluation. As with L4 intentions, traffic that fails to + // match any of the provided permissions in this intention will be subject to the default intention + // behavior is defined by the default ACL policy. This should be omitted for an L4 intention + // as it is mutually exclusive with the Action field. Permissions IntentionPermissions `json:"permissions,omitempty"` - Description string `json:"description,omitempty"` + // Description for the intention. This is not used by Consul, but is presented in API responses to assist tooling. + Description string `json:"description,omitempty"` } type IntentionPermission struct { - Action IntentionAction `json:"action,omitempty"` - HTTP *IntentionHTTPPermission `json:"http,omitempty"` + // Action is one of "allow" or "deny" for the action that + // should be taken if this permission matches a request. + Action IntentionAction `json:"action,omitempty"` + // HTTP is a set of HTTP-specific authorization criteria. + HTTP *IntentionHTTPPermission `json:"http,omitempty"` } type IntentionHTTPPermission struct { - PathExact string `json:"pathExact,omitempty"` + // PathExact is the exact path to match on the HTTP request path. + PathExact string `json:"pathExact,omitempty"` + // PathPrefix is the path prefix to match on the HTTP request path. PathPrefix string `json:"pathPrefix,omitempty"` - PathRegex string `json:"pathRegex,omitempty"` - + // PathRegex is the regular expression to match on the HTTP request path. + PathRegex string `json:"pathRegex,omitempty"` + // Header is a set of criteria that can match on HTTP request headers. + // If more than one is configured all must match for the overall match to apply. Header IntentionHTTPHeaderPermissions `json:"header,omitempty"` - + // Methods is a list of HTTP methods for which this match applies. If unspecified + // all HTTP methods are matched. If provided the names must be a valid method. Methods []string `json:"methods,omitempty"` } type IntentionHTTPHeaderPermission struct { - Name string `json:"name,omitempty"` - Present bool `json:"present,omitempty"` - Exact string `json:"exact,omitempty"` - Prefix string `json:"prefix,omitempty"` - Suffix string `json:"suffix,omitempty"` - Regex string `json:"regex,omitempty"` - Invert bool `json:"invert,omitempty"` + // Name is the name of the header to match. + Name string `json:"name,omitempty"` + // Present matches if the header with the given name is present with any value. + Present bool `json:"present,omitempty"` + // Exact matches if the header with the given name is this value. + Exact string `json:"exact,omitempty"` + // Prefix matches if the header with the given name has this prefix. + Prefix string `json:"prefix,omitempty"` + // Suffix matches if the header with the given name has this suffix. + Suffix string `json:"suffix,omitempty"` + // Regex matches if the header with the given name matches this pattern. + Regex string `json:"regex,omitempty"` + // Invert inverts the logic of the match. + Invert bool `json:"invert,omitempty"` } // IntentionAction is the action that the intention represents. This @@ -196,13 +233,13 @@ func (in IntentionPermissions) toConsul() []*capi.IntentionPermission { for _, permission := range in { consulIntentionPermissions = append(consulIntentionPermissions, &capi.IntentionPermission{ Action: permission.Action.toConsul(), - HTTP: permission.HTTP.ToConsul(), + HTTP: permission.HTTP.toConsul(), }) } return consulIntentionPermissions } -func (in *IntentionHTTPPermission) ToConsul() *capi.IntentionHTTPPermission { +func (in *IntentionHTTPPermission) toConsul() *capi.IntentionHTTPPermission { if in == nil { return nil } @@ -296,12 +333,34 @@ func (in IntentionPermissions) validate(path *field.Path) field.ErrorList { func (in *IntentionHTTPPermission) validate(path *field.Path) field.ErrorList { var errs field.ErrorList + if (in.PathExact != "" && in.PathPrefix != "") || (in.PathExact != "" && in.PathRegex != "") || (in.PathPrefix != "" && in.PathRegex != "") { + errs = append(errs, field.Invalid(path, in, `At most only one of pathExact, pathPrefix, or pathRegex may be configured.`)) + } if invalidPathPrefix(in.PathPrefix) { errs = append(errs, field.Invalid(path.Child("pathPrefix"), in.PathPrefix, `must begin with a '/'`)) } if invalidPathPrefix(in.PathExact) { errs = append(errs, field.Invalid(path.Child("pathExact"), in.PathExact, `must begin with a '/'`)) } + for i, method := range in.Methods { + methods := []string{"GET", "HEAD", "POST", "PUT", "DELETE", "CONNECT", "OPTIONS", "TRACE", "PATCH"} + if !sliceContains(methods, strings.ToUpper(method)) { + errs = append(errs, field.Invalid(path.Child("methods").Index(i), method, notInSliceMessage(methods))) + } + } + errs = append(errs, in.Header.validate(path.Child("header"))...) + return errs +} +func (in IntentionHTTPHeaderPermissions) validate(path *field.Path) field.ErrorList { + var errs field.ErrorList + for i, permission := range in { + if (permission.Regex != "" && permission.Suffix != "") || (permission.Regex != "" && permission.Prefix != "") || (permission.Regex != "" && permission.Exact != "") || + (permission.Regex != "" && permission.Present) || (permission.Suffix != "" && permission.Prefix != "") || (permission.Suffix != "" && permission.Exact != "") || + (permission.Suffix != "" && permission.Present) || (permission.Prefix != "" && permission.Exact != "") || + (permission.Prefix != "" && permission.Present) || (permission.Exact != "" && permission.Present) { + errs = append(errs, field.Invalid(path.Index(i), in[i], `At most only one of exact, prefix, suffix, regex, or present may be configured.`)) + } + } return errs } diff --git a/api/v1alpha1/serviceintentions_types_test.go b/api/v1alpha1/serviceintentions_types_test.go index 831db4e7f2..2f29a11986 100644 --- a/api/v1alpha1/serviceintentions_types_test.go +++ b/api/v1alpha1/serviceintentions_types_test.go @@ -564,17 +564,11 @@ func TestServiceIntentions_Validate(t *testing.T) { { Action: "allow", HTTP: &IntentionHTTPPermission{ - PathExact: "/foo", - PathPrefix: "/bar", - PathRegex: "/baz", + PathExact: "/foo", Header: IntentionHTTPHeaderPermissions{ { Name: "header", Present: true, - Exact: "exact", - Prefix: "prefix", - Suffix: "suffix", - Regex: "regex", Invert: true, }, }, @@ -617,18 +611,12 @@ func TestServiceIntentions_Validate(t *testing.T) { { Action: "allow", HTTP: &IntentionHTTPPermission{ - PathExact: "/foo", - PathPrefix: "/bar", - PathRegex: "/baz", + PathRegex: "/baz", Header: IntentionHTTPHeaderPermissions{ { - Name: "header", - Present: true, - Exact: "exact", - Prefix: "prefix", - Suffix: "suffix", - Regex: "regex", - Invert: true, + Name: "header", + Regex: "regex", + Invert: true, }, }, Methods: []string{ @@ -719,6 +707,102 @@ func TestServiceIntentions_Validate(t *testing.T) { `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0].pathPrefix: Invalid value: "bar": must begin with a '/'`, }, }, + "invalid permissions.http pathPrefix,pathExact specified": { + input: &ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "does-not-matter", + }, + Spec: ServiceIntentionsSpec{ + Destination: Destination{ + Name: "dest-service", + Namespace: "namespace", + }, + Sources: SourceIntentions{ + { + Name: "svc-2", + Namespace: "bar", + Permissions: IntentionPermissions{ + { + Action: "allow", + HTTP: &IntentionHTTPPermission{ + PathPrefix: "/bar", + PathExact: "/foo", + }, + }, + }, + }, + }, + }, + }, + namespacesEnabled: true, + expectedErrMsgs: []string{ + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0]: Invalid value: v1alpha1.IntentionHTTPPermission{PathExact:"/foo", PathPrefix:"/bar", PathRegex:"", Header:v1alpha1.IntentionHTTPHeaderPermissions(nil), Methods:[]string(nil)}: At most only one of pathExact, pathPrefix, or pathRegex may be configured.`, + }, + }, + "invalid permissions.http pathPrefix,pathRegex specified": { + input: &ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "does-not-matter", + }, + Spec: ServiceIntentionsSpec{ + Destination: Destination{ + Name: "dest-service", + Namespace: "namespace", + }, + Sources: SourceIntentions{ + { + Name: "svc-2", + Namespace: "bar", + Permissions: IntentionPermissions{ + { + Action: "allow", + HTTP: &IntentionHTTPPermission{ + PathPrefix: "/bar", + PathRegex: "foo", + }, + }, + }, + }, + }, + }, + }, + namespacesEnabled: true, + expectedErrMsgs: []string{ + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0]: Invalid value: v1alpha1.IntentionHTTPPermission{PathExact:"", PathPrefix:"/bar", PathRegex:"foo", Header:v1alpha1.IntentionHTTPHeaderPermissions(nil), Methods:[]string(nil)}: At most only one of pathExact, pathPrefix, or pathRegex may be configured.`, + }, + }, + "invalid permissions.http pathRegex,pathExact specified": { + input: &ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "does-not-matter", + }, + Spec: ServiceIntentionsSpec{ + Destination: Destination{ + Name: "dest-service", + Namespace: "namespace", + }, + Sources: SourceIntentions{ + { + Name: "svc-2", + Namespace: "bar", + Permissions: IntentionPermissions{ + { + Action: "allow", + HTTP: &IntentionHTTPPermission{ + PathRegex: "bar", + PathExact: "/foo", + }, + }, + }, + }, + }, + }, + }, + namespacesEnabled: true, + expectedErrMsgs: []string{ + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0]: Invalid value: v1alpha1.IntentionHTTPPermission{PathExact:"/foo", PathPrefix:"", PathRegex:"bar", Header:v1alpha1.IntentionHTTPHeaderPermissions(nil), Methods:[]string(nil)}: At most only one of pathExact, pathPrefix, or pathRegex may be configured.`, + }, + }, "invalid permissions.http.pathExact": { input: &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ @@ -750,6 +834,102 @@ func TestServiceIntentions_Validate(t *testing.T) { `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0].pathExact: Invalid value: "bar": must begin with a '/'`, }, }, + "invalid permissions.http.methods": { + input: &ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "does-not-matter", + }, + Spec: ServiceIntentionsSpec{ + Destination: Destination{ + Name: "dest-service", + Namespace: "namespace", + }, + Sources: SourceIntentions{ + { + Name: "svc-2", + Namespace: "bar", + Permissions: IntentionPermissions{ + { + Action: "allow", + HTTP: &IntentionHTTPPermission{ + Methods: []string{ + "FOO", + "GET", + "BAR", + }, + }, + }, + }, + }, + }, + }, + }, + namespacesEnabled: true, + expectedErrMsgs: []string{ + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: [spec.sources[0].permissions[0].methods[0]: Invalid value: "FOO": must be one of "GET", "HEAD", "POST", "PUT", "DELETE", "CONNECT", "OPTIONS", "TRACE", "PATCH", spec.sources[0].permissions[0].methods[2]: Invalid value: "BAR": must be one of "GET", "HEAD", "POST", "PUT", "DELETE", "CONNECT", "OPTIONS", "TRACE", "PATCH"]`, + }, + }, + "invalid permissions.http.header": { + input: &ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "does-not-matter", + }, + Spec: ServiceIntentionsSpec{ + Destination: Destination{ + Name: "dest-service", + Namespace: "namespace", + }, + Sources: SourceIntentions{ + { + Name: "svc-2", + Namespace: "bar", + Permissions: IntentionPermissions{ + { + Action: "allow", + HTTP: &IntentionHTTPPermission{ + Header: IntentionHTTPHeaderPermissions{ + { + Name: "exact-present", + Present: true, + Exact: "foobar", + }, + { + Name: "prefix-exact", + Exact: "foobar", + Prefix: "barfood", + }, + { + Name: "suffix-prefix", + Prefix: "foo", + Suffix: "bar", + }, + { + Name: "suffix-regex", + Suffix: "bar", + Regex: "foo", + }, + { + Name: "regex-present", + Present: true, + Regex: "foobar", + }, + }, + }, + }, + }, + }, + }, + }, + }, + namespacesEnabled: true, + expectedErrMsgs: []string{ + `spec.sources[0].permissions[0].header[0]: Invalid value: v1alpha1.IntentionHTTPHeaderPermission{Name:"exact-present", Present:true, Exact:"foobar", Prefix:"", Suffix:"", Regex:"", Invert:false}: At most only one of exact, prefix, suffix, regex, or present may be configured.`, + `spec.sources[0].permissions[0].header[1]: Invalid value: v1alpha1.IntentionHTTPHeaderPermission{Name:"prefix-exact", Present:false, Exact:"foobar", Prefix:"barfood", Suffix:"", Regex:"", Invert:false}: At most only one of exact, prefix, suffix, regex, or present may be configured.`, + `spec.sources[0].permissions[0].header[2]: Invalid value: v1alpha1.IntentionHTTPHeaderPermission{Name:"suffix-prefix", Present:false, Exact:"", Prefix:"foo", Suffix:"bar", Regex:"", Invert:false}: At most only one of exact, prefix, suffix, regex, or present may be configured.`, + `spec.sources[0].permissions[0].header[3]: Invalid value: v1alpha1.IntentionHTTPHeaderPermission{Name:"suffix-regex", Present:false, Exact:"", Prefix:"", Suffix:"bar", Regex:"foo", Invert:false}: At most only one of exact, prefix, suffix, regex, or present may be configured.`, + `spec.sources[0].permissions[0].header[4]: Invalid value: v1alpha1.IntentionHTTPHeaderPermission{Name:"regex-present", Present:true, Exact:"", Prefix:"", Suffix:"", Regex:"foobar", Invert:false}: At most only one of exact, prefix, suffix, regex, or present may be configured.`, + }, + }, "invalid permissions.action": { input: &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ diff --git a/config/crd/bases/consul.hashicorp.com_serviceintentions.yaml b/config/crd/bases/consul.hashicorp.com_serviceintentions.yaml index db544e096c..6126384f7a 100644 --- a/config/crd/bases/consul.hashicorp.com_serviceintentions.yaml +++ b/config/crd/bases/consul.hashicorp.com_serviceintentions.yaml @@ -44,58 +44,78 @@ spec: destination: properties: name: + description: Name is the destination of all intentions defined in this config entry. This may be set to the wildcard character (*) to match all services that don't otherwise have intentions defined. type: string namespace: + description: Namespace specifies the namespace the config entry will apply to. This may be set to the wildcard character (*) to match all services in all namespaces that don't otherwise have intentions defined. type: string type: object sources: + description: Sources is the list of all intention sources and the authorization granted to those sources. The order of this list does not matter, but out of convenience Consul will always store this reverse sorted by intention precedence, as that is the order that they will be evaluated at enforcement time. items: properties: action: - description: IntentionAction is the action that the intention represents. This can be "allow" or "deny" to allowlist or denylist intentions. + description: Action is required for an L4 intention, and should be set to one of "allow" or "deny" for the action that should be taken if this intention matches a request. type: string description: + description: Description for the intention. This is not used by Consul, but is presented in API responses to assist tooling. type: string name: + description: Name is the source of the intention. This is the name of a Consul service. The service doesn't need to be registered. type: string namespace: + description: Namespace is the namespace for the Name parameter. type: string permissions: + description: Permissions is the list of all additional L7 attributes that extend the intention match criteria. Permission precedence is applied top to bottom. For any given request the first permission to match in the list is terminal and stops further evaluation. As with L4 intentions, traffic that fails to match any of the provided permissions in this intention will be subject to the default intention behavior is defined by the default ACL policy. This should be omitted for an L4 intention as it is mutually exclusive with the Action field. items: properties: action: - description: IntentionAction is the action that the intention represents. This can be "allow" or "deny" to allowlist or denylist intentions. + description: Action is one of "allow" or "deny" for the action that should be taken if this permission matches a request. type: string http: + description: HTTP is a set of HTTP-specific authorization criteria. properties: header: + description: Header is a set of criteria that can match on HTTP request headers. If more than one is configured all must match for the overall match to apply. items: properties: exact: + description: Exact matches if the header with the given name is this value. type: string invert: + description: Invert inverts the logic of the match. type: boolean name: + description: Name is the name of the header to match. type: string prefix: + description: Prefix matches if the header with the given name has this prefix. type: string present: + description: Present matches if the header with the given name is present with any value. type: boolean regex: + description: Regex matches if the header with the given name matches this pattern. type: string suffix: + description: Suffix matches if the header with the given name has this suffix. type: string type: object type: array methods: + description: Methods is a list of HTTP methods for which this match applies. If unspecified all HTTP methods are matched. If provided the names must be a valid method. items: type: string type: array pathExact: + description: PathExact is the exact path to match on the HTTP request path. type: string pathPrefix: + description: PathPrefix is the path prefix to match on the HTTP request path. type: string pathRegex: + description: PathRegex is the regular expression to match on the HTTP request path. type: string type: object type: object From e5be0be0154df4b02436f6f7410c3ce479a259cb Mon Sep 17 00:00:00 2001 From: Ashwin Venkatesh Date: Mon, 9 Nov 2020 11:51:52 -0500 Subject: [PATCH 2/4] Update PR based on review comments --- api/v1alpha1/serviceintentions_types.go | 67 +++++++++++++++---- api/v1alpha1/serviceintentions_types_test.go | 10 +-- ...onsul.hashicorp.com_serviceintentions.yaml | 1 + 3 files changed, 61 insertions(+), 17 deletions(-) diff --git a/api/v1alpha1/serviceintentions_types.go b/api/v1alpha1/serviceintentions_types.go index a36a836124..8dd51b70d4 100644 --- a/api/v1alpha1/serviceintentions_types.go +++ b/api/v1alpha1/serviceintentions_types.go @@ -2,7 +2,7 @@ package v1alpha1 import ( "encoding/json" - "strings" + "net/http" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" @@ -19,6 +19,7 @@ import ( // ServiceIntentionsSpec defines the desired state of ServiceIntentions type ServiceIntentionsSpec struct { + // Destination is the intention destination that will have the authorization granted to. Destination Destination `json:"destination,omitempty"` // Sources is the list of all intention sources and the authorization granted to those sources. // The order of this list does not matter, but out of convenience Consul will always store this @@ -333,20 +334,47 @@ func (in IntentionPermissions) validate(path *field.Path) field.ErrorList { func (in *IntentionHTTPPermission) validate(path *field.Path) field.ErrorList { var errs field.ErrorList - if (in.PathExact != "" && in.PathPrefix != "") || (in.PathExact != "" && in.PathRegex != "") || (in.PathPrefix != "" && in.PathRegex != "") { - errs = append(errs, field.Invalid(path, in, `At most only one of pathExact, pathPrefix, or pathRegex may be configured.`)) + pathParts := 0 + if in.PathRegex != "" { + pathParts++ } - if invalidPathPrefix(in.PathPrefix) { - errs = append(errs, field.Invalid(path.Child("pathPrefix"), in.PathPrefix, `must begin with a '/'`)) + if in.PathPrefix != "" { + pathParts++ + if invalidPathPrefix(in.PathPrefix) { + errs = append(errs, field.Invalid(path.Child("pathPrefix"), in.PathPrefix, `must begin with a '/'`)) + } + } + if in.PathExact != "" { + pathParts++ + if invalidPathPrefix(in.PathExact) { + errs = append(errs, field.Invalid(path.Child("pathExact"), in.PathExact, `must begin with a '/'`)) + } } - if invalidPathPrefix(in.PathExact) { - errs = append(errs, field.Invalid(path.Child("pathExact"), in.PathExact, `must begin with a '/'`)) + if pathParts > 1 { + asJSON, _ := json.Marshal(in) + errs = append(errs, field.Invalid(path, string(asJSON), `At most only one of pathExact, pathPrefix, or pathRegex may be configured.`)) } + + found := make(map[string]struct{}) for i, method := range in.Methods { - methods := []string{"GET", "HEAD", "POST", "PUT", "DELETE", "CONNECT", "OPTIONS", "TRACE", "PATCH"} - if !sliceContains(methods, strings.ToUpper(method)) { + methods := []string{ + http.MethodGet, + http.MethodHead, + http.MethodPost, + http.MethodPut, + http.MethodPatch, + http.MethodDelete, + http.MethodConnect, + http.MethodOptions, + http.MethodTrace, + } + if !sliceContains(methods, method) { errs = append(errs, field.Invalid(path.Child("methods").Index(i), method, notInSliceMessage(methods))) } + if _, ok := found[method]; ok { + errs = append(errs, field.Invalid(path.Child("methods").Index(i), method, `Method contained more than once.`)) + } + found[method] = struct{}{} } errs = append(errs, in.Header.validate(path.Child("header"))...) return errs @@ -354,10 +382,23 @@ func (in *IntentionHTTPPermission) validate(path *field.Path) field.ErrorList { func (in IntentionHTTPHeaderPermissions) validate(path *field.Path) field.ErrorList { var errs field.ErrorList for i, permission := range in { - if (permission.Regex != "" && permission.Suffix != "") || (permission.Regex != "" && permission.Prefix != "") || (permission.Regex != "" && permission.Exact != "") || - (permission.Regex != "" && permission.Present) || (permission.Suffix != "" && permission.Prefix != "") || (permission.Suffix != "" && permission.Exact != "") || - (permission.Suffix != "" && permission.Present) || (permission.Prefix != "" && permission.Exact != "") || - (permission.Prefix != "" && permission.Present) || (permission.Exact != "" && permission.Present) { + hdrParts := 0 + if permission.Present { + hdrParts++ + } + if permission.Exact != "" { + hdrParts++ + } + if permission.Regex != "" { + hdrParts++ + } + if permission.Prefix != "" { + hdrParts++ + } + if permission.Suffix != "" { + hdrParts++ + } + if hdrParts != 1 { errs = append(errs, field.Invalid(path.Index(i), in[i], `At most only one of exact, prefix, suffix, regex, or present may be configured.`)) } } diff --git a/api/v1alpha1/serviceintentions_types_test.go b/api/v1alpha1/serviceintentions_types_test.go index 2f29a11986..2eae827408 100644 --- a/api/v1alpha1/serviceintentions_types_test.go +++ b/api/v1alpha1/serviceintentions_types_test.go @@ -736,7 +736,7 @@ func TestServiceIntentions_Validate(t *testing.T) { }, namespacesEnabled: true, expectedErrMsgs: []string{ - `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0]: Invalid value: v1alpha1.IntentionHTTPPermission{PathExact:"/foo", PathPrefix:"/bar", PathRegex:"", Header:v1alpha1.IntentionHTTPHeaderPermissions(nil), Methods:[]string(nil)}: At most only one of pathExact, pathPrefix, or pathRegex may be configured.`, + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0]: Invalid value: "{\"pathExact\":\"/foo\",\"pathPrefix\":\"/bar\"}": At most only one of pathExact, pathPrefix, or pathRegex may be configured.`, }, }, "invalid permissions.http pathPrefix,pathRegex specified": { @@ -768,7 +768,7 @@ func TestServiceIntentions_Validate(t *testing.T) { }, namespacesEnabled: true, expectedErrMsgs: []string{ - `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0]: Invalid value: v1alpha1.IntentionHTTPPermission{PathExact:"", PathPrefix:"/bar", PathRegex:"foo", Header:v1alpha1.IntentionHTTPHeaderPermissions(nil), Methods:[]string(nil)}: At most only one of pathExact, pathPrefix, or pathRegex may be configured.`, + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0]: Invalid value: "{\"pathPrefix\":\"/bar\",\"pathRegex\":\"foo\"}": At most only one of pathExact, pathPrefix, or pathRegex may be configured.`, }, }, "invalid permissions.http pathRegex,pathExact specified": { @@ -800,7 +800,7 @@ func TestServiceIntentions_Validate(t *testing.T) { }, namespacesEnabled: true, expectedErrMsgs: []string{ - `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0]: Invalid value: v1alpha1.IntentionHTTPPermission{PathExact:"/foo", PathPrefix:"", PathRegex:"bar", Header:v1alpha1.IntentionHTTPHeaderPermissions(nil), Methods:[]string(nil)}: At most only one of pathExact, pathPrefix, or pathRegex may be configured.`, + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0]: Invalid value: "{\"pathExact\":\"/foo\",\"pathRegex\":\"bar\"}": At most only one of pathExact, pathPrefix, or pathRegex may be configured.`, }, }, "invalid permissions.http.pathExact": { @@ -856,6 +856,8 @@ func TestServiceIntentions_Validate(t *testing.T) { "FOO", "GET", "BAR", + "GET", + "POST", }, }, }, @@ -866,7 +868,7 @@ func TestServiceIntentions_Validate(t *testing.T) { }, namespacesEnabled: true, expectedErrMsgs: []string{ - `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: [spec.sources[0].permissions[0].methods[0]: Invalid value: "FOO": must be one of "GET", "HEAD", "POST", "PUT", "DELETE", "CONNECT", "OPTIONS", "TRACE", "PATCH", spec.sources[0].permissions[0].methods[2]: Invalid value: "BAR": must be one of "GET", "HEAD", "POST", "PUT", "DELETE", "CONNECT", "OPTIONS", "TRACE", "PATCH"]`, + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: [spec.sources[0].permissions[0].methods[0]: Invalid value: "FOO": must be one of "GET", "HEAD", "POST", "PUT", "PATCH", "DELETE", "CONNECT", "OPTIONS", "TRACE", spec.sources[0].permissions[0].methods[2]: Invalid value: "BAR": must be one of "GET", "HEAD", "POST", "PUT", "PATCH", "DELETE", "CONNECT", "OPTIONS", "TRACE", spec.sources[0].permissions[0].methods[3]: Invalid value: "GET": Method contained more than once.`, }, }, "invalid permissions.http.header": { diff --git a/config/crd/bases/consul.hashicorp.com_serviceintentions.yaml b/config/crd/bases/consul.hashicorp.com_serviceintentions.yaml index 6126384f7a..f5aaa2180d 100644 --- a/config/crd/bases/consul.hashicorp.com_serviceintentions.yaml +++ b/config/crd/bases/consul.hashicorp.com_serviceintentions.yaml @@ -42,6 +42,7 @@ spec: description: ServiceIntentionsSpec defines the desired state of ServiceIntentions properties: destination: + description: Destination is the intention destination that will have the authorization granted to. properties: name: description: Name is the destination of all intentions defined in this config entry. This may be set to the wildcard character (*) to match all services that don't otherwise have intentions defined. From c4985a77921b1ddbae31bfe5df3be1a991eee9e2 Mon Sep 17 00:00:00 2001 From: Ashwin Venkatesh Date: Mon, 9 Nov 2020 16:41:30 -0500 Subject: [PATCH 3/4] Make changes based on review comments --- api/v1alpha1/serviceintentions_types.go | 31 ++++++++++---------- api/v1alpha1/serviceintentions_types_test.go | 18 ++++++------ 2 files changed, 25 insertions(+), 24 deletions(-) diff --git a/api/v1alpha1/serviceintentions_types.go b/api/v1alpha1/serviceintentions_types.go index 8dd51b70d4..671c9ee721 100644 --- a/api/v1alpha1/serviceintentions_types.go +++ b/api/v1alpha1/serviceintentions_types.go @@ -352,7 +352,7 @@ func (in *IntentionHTTPPermission) validate(path *field.Path) field.ErrorList { } if pathParts > 1 { asJSON, _ := json.Marshal(in) - errs = append(errs, field.Invalid(path, string(asJSON), `At most only one of pathExact, pathPrefix, or pathRegex may be configured.`)) + errs = append(errs, field.Invalid(path, string(asJSON), `at most only one of pathExact, pathPrefix, or pathRegex may be configured.`)) } found := make(map[string]struct{}) @@ -372,13 +372,14 @@ func (in *IntentionHTTPPermission) validate(path *field.Path) field.ErrorList { errs = append(errs, field.Invalid(path.Child("methods").Index(i), method, notInSliceMessage(methods))) } if _, ok := found[method]; ok { - errs = append(errs, field.Invalid(path.Child("methods").Index(i), method, `Method contained more than once.`)) + errs = append(errs, field.Invalid(path.Child("methods").Index(i), method, `method listed more than once.`)) } found[method] = struct{}{} } errs = append(errs, in.Header.validate(path.Child("header"))...) return errs } + func (in IntentionHTTPHeaderPermissions) validate(path *field.Path) field.ErrorList { var errs field.ErrorList for i, permission := range in { @@ -386,20 +387,10 @@ func (in IntentionHTTPHeaderPermissions) validate(path *field.Path) field.ErrorL if permission.Present { hdrParts++ } - if permission.Exact != "" { - hdrParts++ - } - if permission.Regex != "" { - hdrParts++ - } - if permission.Prefix != "" { - hdrParts++ - } - if permission.Suffix != "" { - hdrParts++ - } + hdrParts += numNotEmpty(permission.Exact, permission.Regex, permission.Prefix, permission.Suffix) if hdrParts != 1 { - errs = append(errs, field.Invalid(path.Index(i), in[i], `At most only one of exact, prefix, suffix, regex, or present may be configured.`)) + asJson, _ := json.Marshal(in[i]) + errs = append(errs, field.Invalid(path.Index(i), string(asJson), `at most only one of exact, prefix, suffix, regex, or present may be configured.`)) } } return errs @@ -455,3 +446,13 @@ type ServiceIntentionsList struct { func init() { SchemeBuilder.Register(&ServiceIntentions{}, &ServiceIntentionsList{}) } + +func numNotEmpty(ss ...string) int { + count := 0 + for _, s := range ss { + if s != "" { + count++ + } + } + return count +} diff --git a/api/v1alpha1/serviceintentions_types_test.go b/api/v1alpha1/serviceintentions_types_test.go index 2eae827408..a37d848f3b 100644 --- a/api/v1alpha1/serviceintentions_types_test.go +++ b/api/v1alpha1/serviceintentions_types_test.go @@ -736,7 +736,7 @@ func TestServiceIntentions_Validate(t *testing.T) { }, namespacesEnabled: true, expectedErrMsgs: []string{ - `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0]: Invalid value: "{\"pathExact\":\"/foo\",\"pathPrefix\":\"/bar\"}": At most only one of pathExact, pathPrefix, or pathRegex may be configured.`, + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0]: Invalid value: "{\"pathExact\":\"/foo\",\"pathPrefix\":\"/bar\"}": at most only one of pathExact, pathPrefix, or pathRegex may be configured.`, }, }, "invalid permissions.http pathPrefix,pathRegex specified": { @@ -768,7 +768,7 @@ func TestServiceIntentions_Validate(t *testing.T) { }, namespacesEnabled: true, expectedErrMsgs: []string{ - `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0]: Invalid value: "{\"pathPrefix\":\"/bar\",\"pathRegex\":\"foo\"}": At most only one of pathExact, pathPrefix, or pathRegex may be configured.`, + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0]: Invalid value: "{\"pathPrefix\":\"/bar\",\"pathRegex\":\"foo\"}": at most only one of pathExact, pathPrefix, or pathRegex may be configured.`, }, }, "invalid permissions.http pathRegex,pathExact specified": { @@ -800,7 +800,7 @@ func TestServiceIntentions_Validate(t *testing.T) { }, namespacesEnabled: true, expectedErrMsgs: []string{ - `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0]: Invalid value: "{\"pathExact\":\"/foo\",\"pathRegex\":\"bar\"}": At most only one of pathExact, pathPrefix, or pathRegex may be configured.`, + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].permissions[0]: Invalid value: "{\"pathExact\":\"/foo\",\"pathRegex\":\"bar\"}": at most only one of pathExact, pathPrefix, or pathRegex may be configured.`, }, }, "invalid permissions.http.pathExact": { @@ -868,7 +868,7 @@ func TestServiceIntentions_Validate(t *testing.T) { }, namespacesEnabled: true, expectedErrMsgs: []string{ - `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: [spec.sources[0].permissions[0].methods[0]: Invalid value: "FOO": must be one of "GET", "HEAD", "POST", "PUT", "PATCH", "DELETE", "CONNECT", "OPTIONS", "TRACE", spec.sources[0].permissions[0].methods[2]: Invalid value: "BAR": must be one of "GET", "HEAD", "POST", "PUT", "PATCH", "DELETE", "CONNECT", "OPTIONS", "TRACE", spec.sources[0].permissions[0].methods[3]: Invalid value: "GET": Method contained more than once.`, + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: [spec.sources[0].permissions[0].methods[0]: Invalid value: "FOO": must be one of "GET", "HEAD", "POST", "PUT", "PATCH", "DELETE", "CONNECT", "OPTIONS", "TRACE", spec.sources[0].permissions[0].methods[2]: Invalid value: "BAR": must be one of "GET", "HEAD", "POST", "PUT", "PATCH", "DELETE", "CONNECT", "OPTIONS", "TRACE", spec.sources[0].permissions[0].methods[3]: Invalid value: "GET": method listed more than once.`, }, }, "invalid permissions.http.header": { @@ -925,11 +925,11 @@ func TestServiceIntentions_Validate(t *testing.T) { }, namespacesEnabled: true, expectedErrMsgs: []string{ - `spec.sources[0].permissions[0].header[0]: Invalid value: v1alpha1.IntentionHTTPHeaderPermission{Name:"exact-present", Present:true, Exact:"foobar", Prefix:"", Suffix:"", Regex:"", Invert:false}: At most only one of exact, prefix, suffix, regex, or present may be configured.`, - `spec.sources[0].permissions[0].header[1]: Invalid value: v1alpha1.IntentionHTTPHeaderPermission{Name:"prefix-exact", Present:false, Exact:"foobar", Prefix:"barfood", Suffix:"", Regex:"", Invert:false}: At most only one of exact, prefix, suffix, regex, or present may be configured.`, - `spec.sources[0].permissions[0].header[2]: Invalid value: v1alpha1.IntentionHTTPHeaderPermission{Name:"suffix-prefix", Present:false, Exact:"", Prefix:"foo", Suffix:"bar", Regex:"", Invert:false}: At most only one of exact, prefix, suffix, regex, or present may be configured.`, - `spec.sources[0].permissions[0].header[3]: Invalid value: v1alpha1.IntentionHTTPHeaderPermission{Name:"suffix-regex", Present:false, Exact:"", Prefix:"", Suffix:"bar", Regex:"foo", Invert:false}: At most only one of exact, prefix, suffix, regex, or present may be configured.`, - `spec.sources[0].permissions[0].header[4]: Invalid value: v1alpha1.IntentionHTTPHeaderPermission{Name:"regex-present", Present:true, Exact:"", Prefix:"", Suffix:"", Regex:"foobar", Invert:false}: At most only one of exact, prefix, suffix, regex, or present may be configured.`, + `spec.sources[0].permissions[0].header[0]: Invalid value: "{\"name\":\"exact-present\",\"present\":true,\"exact\":\"foobar\"}": at most only one of exact, prefix, suffix, regex, or present may be configured.`, + `spec.sources[0].permissions[0].header[1]: Invalid value: "{\"name\":\"prefix-exact\",\"exact\":\"foobar\",\"prefix\":\"barfood\"}": at most only one of exact, prefix, suffix, regex, or present may be configured.`, + `spec.sources[0].permissions[0].header[2]: Invalid value: "{\"name\":\"suffix-prefix\",\"prefix\":\"foo\",\"suffix\":\"bar\"}": at most only one of exact, prefix, suffix, regex, or present may be configured.`, + `spec.sources[0].permissions[0].header[3]: Invalid value: "{\"name\":\"suffix-regex\",\"suffix\":\"bar\",\"regex\":\"foo\"}": at most only one of exact, prefix, suffix, regex, or present may be configured.`, + `spec.sources[0].permissions[0].header[4]: Invalid value: "{\"name\":\"regex-present\",\"present\":true,\"regex\":\"foobar\"}": at most only one of exact, prefix, suffix, regex, or present may be configured.`, }, }, "invalid permissions.action": { From 4cfe655023f5849129ad1abc9526d9667f712aa5 Mon Sep 17 00:00:00 2001 From: Ashwin Venkatesh Date: Mon, 9 Nov 2020 17:18:52 -0500 Subject: [PATCH 4/4] Apply suggestions from code review Co-authored-by: Iryna Shustava --- api/v1alpha1/serviceintentions_types.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/v1alpha1/serviceintentions_types.go b/api/v1alpha1/serviceintentions_types.go index 671c9ee721..1c52d032a7 100644 --- a/api/v1alpha1/serviceintentions_types.go +++ b/api/v1alpha1/serviceintentions_types.go @@ -388,7 +388,7 @@ func (in IntentionHTTPHeaderPermissions) validate(path *field.Path) field.ErrorL hdrParts++ } hdrParts += numNotEmpty(permission.Exact, permission.Regex, permission.Prefix, permission.Suffix) - if hdrParts != 1 { + if hdrParts > 1 { asJson, _ := json.Marshal(in[i]) errs = append(errs, field.Invalid(path.Index(i), string(asJson), `at most only one of exact, prefix, suffix, regex, or present may be configured.`)) }