Skip to content

Commit

Permalink
connect: validate and test more of the L7 config entries (#6156)
Browse files Browse the repository at this point in the history
  • Loading branch information
@rboyer
rboyer authored Jul 24, 2019
1 parent 483effd commit 1dbd92e
Show file tree
Hide file tree
Showing 5 changed files with 732 additions and 94 deletions.
6 changes: 3 additions & 3 deletions agent/config/runtime_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3082,7 +3082,7 @@ func TestConfigFlagsAndEdgecases(t *testing.T) {
},
{
"name": "debug2",
"present": false,
"present": true,
"invert": true
},
{
Expand Down Expand Up @@ -3165,7 +3165,7 @@ func TestConfigFlagsAndEdgecases(t *testing.T) {
},
{
name = "debug2"
present = false
present = true
invert = true
},
{
Expand Down Expand Up @@ -3247,7 +3247,7 @@ func TestConfigFlagsAndEdgecases(t *testing.T) {
},
{
Name: "debug2",
Present: false,
Present: true,
Invert: true,
},
{
Expand Down
163 changes: 83 additions & 80 deletions agent/structs/config_entry_discoverychain.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ package structs
import (
"fmt"
"math"
"regexp"
"sort"
"strconv"
"strings"
"time"

"github.com/hashicorp/consul/acl"
Expand Down Expand Up @@ -54,12 +56,8 @@ func (e *ServiceRouterConfigEntry) Normalize() error {
return fmt.Errorf("config entry is nil")
}

// TODO(rb): trim spaces

e.Kind = ServiceRouter

// TODO(rb): anything to normalize?

return nil
}

Expand All @@ -68,78 +66,69 @@ func (e *ServiceRouterConfigEntry) Validate() error {
return fmt.Errorf("Name is required")
}

// TODO(rb): enforce corresponding service has protocol=http

// TODO(rb): actually you can only define the HTTP section if protocol=http{,2}

// TODO(rb): validate the entire compiled chain? how?

// TODO(rb): validate more

// Technically you can have no explicit routes at all where just the
// catch-all is configured for you, but at that point maybe you should just
// delete it so it will default?

for i, route := range e.Routes {
if route.Match == nil || route.Match.HTTP == nil {
continue
}

pathParts := 0
if route.Match.HTTP.PathExact != "" {
pathParts++
}
if route.Match.HTTP.PathPrefix != "" {
pathParts++
}
if route.Match.HTTP.PathRegex != "" {
pathParts++
}
if pathParts > 1 {
return fmt.Errorf("Route[%d] should only contain at most one of PathExact, PathPrefix, or PathRegex", i)
}

// TODO(rb): do some validation of PathExact and PathPrefix

for j, hdr := range route.Match.HTTP.Header {
if hdr.Name == "" {
return fmt.Errorf("Route[%d] Header[%d] missing required Name field", i, j)
}
hdrParts := 0
if hdr.Present {
hdrParts++
}
if hdr.Exact != "" {
hdrParts++
}
if hdr.Regex != "" {
hdrParts++
eligibleForPrefixRewrite := false
if route.Match != nil && route.Match.HTTP != nil {
pathParts := 0
if route.Match.HTTP.PathExact != "" {
eligibleForPrefixRewrite = true
pathParts++
if !strings.HasPrefix(route.Match.HTTP.PathExact, "/") {
return fmt.Errorf("Route[%d] PathExact doesn't start with '/': %q", i, route.Match.HTTP.PathExact)
}
}
if hdr.Prefix != "" {
hdrParts++
if route.Match.HTTP.PathPrefix != "" {
eligibleForPrefixRewrite = true
pathParts++
if !strings.HasPrefix(route.Match.HTTP.PathPrefix, "/") {
return fmt.Errorf("Route[%d] PathPrefix doesn't start with '/': %q", i, route.Match.HTTP.PathPrefix)
}
}
if hdr.Suffix != "" {
hdrParts++
if route.Match.HTTP.PathRegex != "" {
pathParts++
}
// "absent" is the bare invert=true
if (hdrParts == 0 && !hdr.Invert) || (hdrParts > 1) {
return fmt.Errorf("Route[%d] Header[%d] should only contain one of Present, Exact, Prefix, Suffix, or Regex (or just Invert)", i, j)
if pathParts > 1 {
return fmt.Errorf("Route[%d] should only contain at most one of PathExact, PathPrefix, or PathRegex", i)
}
}

for j, qm := range route.Match.HTTP.QueryParam {
if qm.Name == "" {
return fmt.Errorf("Route[%d] QueryParam[%d] missing required Name field", i, j)
for j, hdr := range route.Match.HTTP.Header {
if hdr.Name == "" {
return fmt.Errorf("Route[%d] Header[%d] missing required Name field", i, j)
}
hdrParts := 0
if hdr.Present {
hdrParts++
}
if hdr.Exact != "" {
hdrParts++
}
if hdr.Regex != "" {
hdrParts++
}
if hdr.Prefix != "" {
hdrParts++
}
if hdr.Suffix != "" {
hdrParts++
}
if hdrParts != 1 {
return fmt.Errorf("Route[%d] Header[%d] should only contain one of Present, Exact, Prefix, Suffix, or Regex", i, j)
}
}
}

ineligibleForPrefixRewrite := false
if route.Match.HTTP.PathRegex != "" {
ineligibleForPrefixRewrite = true
for j, qm := range route.Match.HTTP.QueryParam {
if qm.Name == "" {
return fmt.Errorf("Route[%d] QueryParam[%d] missing required Name field", i, j)
}
}
}

if route.Destination != nil {
if route.Destination.PrefixRewrite != "" && ineligibleForPrefixRewrite {
if route.Destination.PrefixRewrite != "" && !eligibleForPrefixRewrite {
return fmt.Errorf("Route[%d] cannot make use of PrefixRewrite without configuring either PathExact or PathPrefix", i)
}
}
Expand Down Expand Up @@ -176,6 +165,10 @@ func (e *ServiceRouterConfigEntry) ListRelatedServices() []string {
}
}

if len(found) == 0 {
return nil
}

out := make([]string, 0, len(found))
for svc, _ := range found {
out = append(out, svc)
Expand Down Expand Up @@ -336,7 +329,6 @@ func (e *ServiceSplitterConfigEntry) Normalize() error {
if e == nil {
return fmt.Errorf("config entry is nil")
}
// TODO(rb): trim spaces

e.Kind = ServiceSplitter

Expand Down Expand Up @@ -398,10 +390,6 @@ func (e *ServiceSplitterConfigEntry) Validate() error {
return fmt.Errorf("the sum of all split weights must be 100, not %f", float32(sumScaled)/100)
}

// TODO(rb): enforce corresponding service has protocol=http

// TODO(rb): validate the entire compiled chain? how?

return nil
}

Expand Down Expand Up @@ -438,6 +426,10 @@ func (e *ServiceSplitterConfigEntry) ListRelatedServices() []string {
}
}

if len(found) == 0 {
return nil
}

out := make([]string, 0, len(found))
for svc, _ := range found {
out = append(out, svc)
Expand Down Expand Up @@ -568,12 +560,9 @@ func (e *ServiceResolverConfigEntry) Normalize() error {
if e == nil {
return fmt.Errorf("config entry is nil")
}
// TODO(rb): trim spaces

e.Kind = ServiceResolver

// TODO(rb): anything to normalize?

return nil
}

Expand All @@ -587,6 +576,9 @@ func (e *ServiceResolverConfigEntry) Validate() error {
if name == "" {
return fmt.Errorf("Subset defined with empty name")
}
if err := validateServiceSubset(name); err != nil {
return fmt.Errorf("Subset %q is invalid: %v", name, err)
}
}
}

Expand Down Expand Up @@ -623,12 +615,9 @@ func (e *ServiceResolverConfigEntry) Validate() error {
return fmt.Errorf("Redirect.Namespace defined without Redirect.Service")
}
} else if r.Service == e.Name {
// TODO(rb): prevent self loops?
if r.ServiceSubset != "" && !isSubset(r.ServiceSubset) {
return fmt.Errorf("Redirect.ServiceSubset %q is not a valid subset of %q", r.ServiceSubset, r.Service)
}
} else {
// TODO(rb): handle validating subsets for other services
}
}

Expand All @@ -647,17 +636,13 @@ func (e *ServiceResolverConfigEntry) Validate() error {
if !isSubset(f.ServiceSubset) {
return fmt.Errorf("Bad Failover[%q].ServiceSubset %q is not a valid subset of %q", subset, f.ServiceSubset, f.Service)
}
} else {
// TODO(rb): handle validating subsets for other services
}
}

if f.OverprovisioningFactor < 0 {
return fmt.Errorf("Bad Failover[%q].OverprovisioningFactor '%d', must be >= 0", subset, f.OverprovisioningFactor)
}

// TODO(rb): more extensive validation will require graph traversal

for _, dc := range f.Datacenters {
if dc == "" {
return fmt.Errorf("Bad Failover[%q].Datacenters: found empty datacenter", subset)
Expand All @@ -670,10 +655,6 @@ func (e *ServiceResolverConfigEntry) Validate() error {
return fmt.Errorf("Bad ConnectTimeout '%s', must be >= 0", e.ConnectTimeout)
}

// TODO(rb): validate the entire compiled chain? how?

// TODO(rb): validate more

return nil
}

Expand Down Expand Up @@ -710,6 +691,10 @@ func (e *ServiceResolverConfigEntry) ListRelatedServices() []string {
}
}

if len(found) == 0 {
return nil
}

out := make([]string, 0, len(found))
for svc, _ := range found {
out = append(out, svc)
Expand Down Expand Up @@ -1018,3 +1003,21 @@ func (e *ConfigEntryGraphError) Error() string {
}
return e.Message
}

var (
validServiceSubset = regexp.MustCompile(`^[a-z0-9]([a-z0-9-]*[a-z0-9])?$`)
serviceSubsetMaxLength = 63
)

// validateServiceSubset checks if the provided name can be used as an service
// subset. Because these are used in SNI headers they must a DNS label per
// RFC-1035/RFC-1123.
func validateServiceSubset(subset string) error {
if subset == "" || len(subset) > serviceSubsetMaxLength {
return fmt.Errorf("must be non-empty and 63 characters or fewer")
}
if !validServiceSubset.MatchString(subset) {
return fmt.Errorf("must be 63 characters or fewer, begin or end with lower case alphanumeric characters, and contain lower case alphanumeric characters or '-' in between")
}
return nil
}
Loading

0 comments on commit 1dbd92e

Please sign in to comment.