Skip to content

Commit

Permalink
connect: validate and test more of the L7 config entries
Browse files Browse the repository at this point in the history
  • Loading branch information
@rboyer
rboyer committed Jul 24, 2019
1 parent ef6b80b commit 26f9445
Show file tree
Hide file tree
Showing 2 changed files with 727 additions and 83 deletions.
164 changes: 84 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,70 @@ 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++
}
// "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)
}
}
}

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 +166,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 +330,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 +391,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 +427,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 +561,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 +577,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 +616,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 +637,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 +656,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 +692,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 +1004,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 26f9445

Please sign in to comment.