connect: validate and test more of the L7 config entries (#6156)

agent/config/runtime_test.go

@@ -3082,7 +3082,7 @@ func TestConfigFlagsAndEdgecases(t *testing.T) {
 												},
 												{
 													"name": "debug2",
-													"present": false,
+													"present": true,
 													"invert": true
 												},
 												{
@@ -3165,7 +3165,7 @@ func TestConfigFlagsAndEdgecases(t *testing.T) {
 											},
 											{
 												name = "debug2"
-												present = false
+												present = true
 												invert = true
 											},
 											{
@@ -3247,7 +3247,7 @@ func TestConfigFlagsAndEdgecases(t *testing.T) {
 											},
 											{
 												Name:    "debug2",
-												Present: false,
+												Present: true,
 												Invert:  true,
 											},
 											{

agent/structs/config_entry_discoverychain.go

@@ -3,8 +3,10 @@ package structs
 import (
 	"fmt"
 	"math"
+	"regexp"
 	"sort"
 	"strconv"
+	"strings"
 	"time"
 
 	"github.com/hashicorp/consul/acl"
@@ -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
 }
 
@@ -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 {
+		eligibleForPrefixRewrite := false
-			continue
+		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 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 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)
+			}
 
-		pathParts := 0
+			for j, hdr := range route.Match.HTTP.Header {
-		if route.Match.HTTP.PathExact != "" {
+				if hdr.Name == "" {
-			pathParts++
+					return fmt.Errorf("Route[%d] Header[%d] missing required Name field", i, j)
-		}
+				}
-		if route.Match.HTTP.PathPrefix != "" {
+				hdrParts := 0
-			pathParts++
+				if hdr.Present {
-		}
+					hdrParts++
-		if route.Match.HTTP.PathRegex != "" {
+				}
-			pathParts++
+				if hdr.Exact != "" {
-		}
+					hdrParts++
-		if pathParts > 1 {
+				}
-			return fmt.Errorf("Route[%d] should only contain at most one of PathExact, PathPrefix, or PathRegex", i)
+				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)
+				}
+			}
 
-		// TODO(rb): do some validation of PathExact and PathPrefix
+			for j, qm := range route.Match.HTTP.QueryParam {
-
+				if qm.Name == "" {
-		for j, hdr := range route.Match.HTTP.Header {
+					return fmt.Errorf("Route[%d] QueryParam[%d] missing required Name field", i, j)
-			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)
-			}
-		}
-
-		for j, qm := range route.Match.HTTP.QueryParam {
-			if qm.Name == "" {
-				return fmt.Errorf("Route[%d] QueryParam[%d] missing required Name field", i, j)
-			}
-		}
-
-		ineligibleForPrefixRewrite := false
-		if route.Match.HTTP.PathRegex != "" {
-			ineligibleForPrefixRewrite = true
 		}
 
 		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)
 			}
 		}
@@ -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)
@@ -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
 
@@ -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
 }
 
@@ -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)
@@ -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
 }
 
@@ -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)
+			}
 		}
 	}
 
@@ -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
 		}
 	}
 
@@ -647,8 +636,6 @@ 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
 				}
 			}
 
@@ -656,8 +643,6 @@ func (e *ServiceResolverConfigEntry) Validate() error {
 				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)
@@ -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
 }
 
@@ -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)
@@ -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
+}

agent/structs/config_entry_discoverychain_test.go

@@ -1,23 +1,265 @@
 package structs
 
 import (
+	"bytes"
+	"fmt"
+	"strings"
 	"testing"
 	"time"
 
+	"github.com/hashicorp/consul/acl"
 	"github.com/stretchr/testify/require"
 )
 
+func TestConfigEntries_ListRelatedServices_AndACLs(t *testing.T) {
+	// This test tests both of these because they are related functions.
+	t.Parallel()
+
+	newServiceACL := func(t *testing.T, canRead, canWrite []string) acl.Authorizer {
+		var buf bytes.Buffer
+		for _, s := range canRead {
+			buf.WriteString(fmt.Sprintf("service %q { policy = %q }\n", s, "read"))
+		}
+		for _, s := range canWrite {
+			buf.WriteString(fmt.Sprintf("service %q { policy = %q }\n", s, "write"))
+		}
+
+		policy, err := acl.NewPolicyFromSource("", 0, buf.String(), acl.SyntaxCurrent, nil)
+		require.NoError(t, err)
+
+		authorizer, err := acl.NewPolicyAuthorizer(acl.DenyAll(), []*acl.Policy{policy}, nil)
+		require.NoError(t, err)
+		return authorizer
+	}
+
+	type testACL struct {
+		name       string
+		authorizer acl.Authorizer
+		canRead    bool
+		canWrite   bool
+	}
+
+	defaultDenyCase := testACL{
+		name:       "deny",
+		authorizer: newServiceACL(t, nil, nil),
+		canRead:    false,
+		canWrite:   false,
+	}
+	readTestCase := testACL{
+		name:       "can read test",
+		authorizer: newServiceACL(t, []string{"test"}, nil),
+		canRead:    true,
+		canWrite:   false,
+	}
+	writeTestCase := testACL{
+		name:       "can write test",
+		authorizer: newServiceACL(t, nil, []string{"test"}),
+		canRead:    true,
+		canWrite:   true,
+	}
+	writeTestCaseDenied := testACL{
+		name:       "cannot write test",
+		authorizer: newServiceACL(t, nil, []string{"test"}),
+		canRead:    true,
+		canWrite:   false,
+	}
+
+	for _, tc := range []struct {
+		name           string
+		entry          discoveryChainConfigEntry
+		expectServices []string
+		expectACLs     []testACL
+	}{
+		{
+			name: "resolver: self",
+			entry: &ServiceResolverConfigEntry{
+				Kind: ServiceResolver,
+				Name: "test",
+			},
+			expectServices: nil,
+			expectACLs: []testACL{
+				defaultDenyCase,
+				readTestCase,
+				writeTestCase,
+			},
+		},
+		{
+			name: "resolver: redirect",
+			entry: &ServiceResolverConfigEntry{
+				Kind: ServiceResolver,
+				Name: "test",
+				Redirect: &ServiceResolverRedirect{
+					Service: "other",
+				},
+			},
+			expectServices: []string{"other"},
+			expectACLs: []testACL{
+				defaultDenyCase,
+				readTestCase,
+				writeTestCaseDenied,
+				{
+					name:       "can write test (with other:read)",
+					authorizer: newServiceACL(t, []string{"other"}, []string{"test"}),
+					canRead:    true,
+					canWrite:   true,
+				},
+			},
+		},
+		{
+			name: "resolver: failover",
+			entry: &ServiceResolverConfigEntry{
+				Kind: ServiceResolver,
+				Name: "test",
+				Subsets: map[string]ServiceResolverSubset{
+					"foo": {OnlyPassing: true},
+					"bar": {OnlyPassing: true},
+				},
+				Failover: map[string]ServiceResolverFailover{
+					"foo": ServiceResolverFailover{
+						Service: "other1",
+					},
+					"bar": ServiceResolverFailover{
+						Service: "other2",
+					},
+				},
+			},
+			expectServices: []string{"other1", "other2"},
+			expectACLs: []testACL{
+				defaultDenyCase,
+				readTestCase,
+				writeTestCaseDenied,
+				{
+					name:       "can write test (with other1:read and other2:read)",
+					authorizer: newServiceACL(t, []string{"other1", "other2"}, []string{"test"}),
+					canRead:    true,
+					canWrite:   true,
+				},
+			},
+		},
+		{
+			name: "splitter: self",
+			entry: &ServiceSplitterConfigEntry{
+				Kind: ServiceSplitter,
+				Name: "test",
+				Splits: []ServiceSplit{
+					{Weight: 100},
+				},
+			},
+			expectServices: nil,
+			expectACLs: []testACL{
+				defaultDenyCase,
+				readTestCase,
+				writeTestCase,
+			},
+		},
+		{
+			name: "splitter: some",
+			entry: &ServiceSplitterConfigEntry{
+				Kind: ServiceSplitter,
+				Name: "test",
+				Splits: []ServiceSplit{
+					{Weight: 25, Service: "b"},
+					{Weight: 25, Service: "a"},
+					{Weight: 50, Service: "c"},
+				},
+			},
+			expectServices: []string{"a", "b", "c"},
+			expectACLs: []testACL{
+				defaultDenyCase,
+				readTestCase,
+				writeTestCaseDenied,
+				{
+					name:       "can write test (with a:read, b:read, and c:read)",
+					authorizer: newServiceACL(t, []string{"a", "b", "c"}, []string{"test"}),
+					canRead:    true,
+					canWrite:   true,
+				},
+			},
+		},
+		{
+			name: "router: self",
+			entry: &ServiceRouterConfigEntry{
+				Kind: ServiceRouter,
+				Name: "test",
+			},
+			expectServices: []string{"test"},
+			expectACLs: []testACL{
+				defaultDenyCase,
+				readTestCase,
+				writeTestCase,
+			},
+		},
+		{
+			name: "router: some",
+			entry: &ServiceRouterConfigEntry{
+				Kind: ServiceRouter,
+				Name: "test",
+				Routes: []ServiceRoute{
+					{
+						Match: &ServiceRouteMatch{HTTP: &ServiceRouteHTTPMatch{
+							PathPrefix: "/foo",
+						}},
+						Destination: &ServiceRouteDestination{
+							Service: "foo",
+						},
+					},
+					{
+						Match: &ServiceRouteMatch{HTTP: &ServiceRouteHTTPMatch{
+							PathPrefix: "/bar",
+						}},
+						Destination: &ServiceRouteDestination{
+							Service: "bar",
+						},
+					},
+				},
+			},
+			expectServices: []string{"bar", "foo", "test"},
+			expectACLs: []testACL{
+				defaultDenyCase,
+				readTestCase,
+				writeTestCaseDenied,
+				{
+					name:       "can write test (with foo:read and bar:read)",
+					authorizer: newServiceACL(t, []string{"foo", "bar"}, []string{"test"}),
+					canRead:    true,
+					canWrite:   true,
+				},
+			},
+		},
+	} {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			// sanity check inputs
+			require.NoError(t, tc.entry.Normalize())
+			require.NoError(t, tc.entry.Validate())
+
+			got := tc.entry.ListRelatedServices()
+			require.Equal(t, tc.expectServices, got)
+
+			for _, a := range tc.expectACLs {
+				a := a
+				t.Run(a.name, func(t *testing.T) {
+					require.Equal(t, a.canRead, tc.entry.CanRead(a.authorizer))
+					require.Equal(t, a.canWrite, tc.entry.CanWrite(a.authorizer))
+				})
+			}
+		})
+	}
+}
+
 func TestServiceResolverConfigEntry(t *testing.T) {
 	t.Parallel()
 
-	for _, tc := range []struct {
+	type testcase struct {
 		name         string
 		entry        *ServiceResolverConfigEntry
 		normalizeErr string
 		validateErr  string
 		// check is called between normalize and validate
 		check func(t *testing.T, entry *ServiceResolverConfigEntry)
-	}{
+	}
+
+	cases := []testcase{
 		{
 			name:         "nil",
 			entry:        nil,
@@ -251,7 +493,39 @@ func TestServiceResolverConfigEntry(t *testing.T) {
 			},
 			validateErr: "Bad ConnectTimeout",
 		},
-	} {
+	}
+
+	// Bulk add a bunch of similar validation cases.
+	for _, invalidSubset := range invalidSubsetNames {
+		tc := testcase{
+			name: "invalid subset name: " + invalidSubset,
+			entry: &ServiceResolverConfigEntry{
+				Kind: ServiceResolver,
+				Name: "test",
+				Subsets: map[string]ServiceResolverSubset{
+					invalidSubset: {OnlyPassing: true},
+				},
+			},
+			validateErr: fmt.Sprintf("Subset %q is invalid", invalidSubset),
+		}
+		cases = append(cases, tc)
+	}
+
+	for _, goodSubset := range validSubsetNames {
+		tc := testcase{
+			name: "valid subset name: " + goodSubset,
+			entry: &ServiceResolverConfigEntry{
+				Kind: ServiceResolver,
+				Name: "test",
+				Subsets: map[string]ServiceResolverSubset{
+					goodSubset: {OnlyPassing: true},
+				},
+			},
+		}
+		cases = append(cases, tc)
+	}
+
+	for _, tc := range cases {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
 			err := tc.entry.Normalize()
@@ -461,3 +735,369 @@ func TestServiceSplitterConfigEntry(t *testing.T) {
 		})
 	}
 }
+
+func TestServiceRouterConfigEntry(t *testing.T) {
+	t.Parallel()
+
+	httpMatch := func(http *ServiceRouteHTTPMatch) *ServiceRouteMatch {
+		return &ServiceRouteMatch{HTTP: http}
+	}
+	httpMatchHeader := func(headers ...ServiceRouteHTTPMatchHeader) *ServiceRouteMatch {
+		return httpMatch(&ServiceRouteHTTPMatch{
+			Header: headers,
+		})
+	}
+	httpMatchParam := func(params ...ServiceRouteHTTPMatchQueryParam) *ServiceRouteMatch {
+		return httpMatch(&ServiceRouteHTTPMatch{
+			QueryParam: params,
+		})
+	}
+	toService := func(svc string) *ServiceRouteDestination {
+		return &ServiceRouteDestination{Service: svc}
+	}
+	routeMatch := func(match *ServiceRouteMatch) ServiceRoute {
+		return ServiceRoute{
+			Match:       match,
+			Destination: toService("other"),
+		}
+	}
+	makerouter := func(routes ...ServiceRoute) *ServiceRouterConfigEntry {
+		return &ServiceRouterConfigEntry{
+			Kind:   ServiceRouter,
+			Name:   "test",
+			Routes: routes,
+		}
+	}
+
+	type testcase struct {
+		name         string
+		entry        *ServiceRouterConfigEntry
+		normalizeErr string
+		validateErr  string
+		// check is called between normalize and validate
+		check func(t *testing.T, entry *ServiceRouterConfigEntry)
+	}
+
+	cases := []testcase{
+		{
+			name:         "nil",
+			entry:        nil,
+			normalizeErr: "config entry is nil",
+		},
+		{
+			name:        "no name",
+			entry:       &ServiceRouterConfigEntry{},
+			validateErr: "Name is required",
+		},
+		{
+			name:  "empty",
+			entry: makerouter(),
+		},
+		{
+			name: "1 empty route",
+			entry: makerouter(
+				ServiceRoute{},
+			),
+		},
+
+		{
+			name: "route with path exact",
+			entry: makerouter(routeMatch(httpMatch(&ServiceRouteHTTPMatch{
+				PathExact: "/exact",
+			}))),
+		},
+		{
+			name: "route with bad path exact",
+			entry: makerouter(routeMatch(httpMatch(&ServiceRouteHTTPMatch{
+				PathExact: "no-leading-slash",
+			}))),
+			validateErr: "PathExact doesn't start with '/'",
+		},
+		{
+			name: "route with path prefix",
+			entry: makerouter(routeMatch(httpMatch(&ServiceRouteHTTPMatch{
+				PathPrefix: "/prefix",
+			}))),
+		},
+		{
+			name: "route with bad path prefix",
+			entry: makerouter(routeMatch(httpMatch(&ServiceRouteHTTPMatch{
+				PathPrefix: "no-leading-slash",
+			}))),
+			validateErr: "PathPrefix doesn't start with '/'",
+		},
+		{
+			name: "route with path regex",
+			entry: makerouter(routeMatch(httpMatch(&ServiceRouteHTTPMatch{
+				PathRegex: "/regex",
+			}))),
+		},
+		{
+			name: "route with path exact and prefix",
+			entry: makerouter(routeMatch(httpMatch(&ServiceRouteHTTPMatch{
+				PathExact:  "/exact",
+				PathPrefix: "/prefix",
+			}))),
+			validateErr: "should only contain at most one of PathExact, PathPrefix, or PathRegex",
+		},
+		{
+			name: "route with path exact and regex",
+			entry: makerouter(routeMatch(httpMatch(&ServiceRouteHTTPMatch{
+				PathExact: "/exact",
+				PathRegex: "/regex",
+			}))),
+			validateErr: "should only contain at most one of PathExact, PathPrefix, or PathRegex",
+		},
+		{
+			name: "route with path prefix and regex",
+			entry: makerouter(routeMatch(httpMatch(&ServiceRouteHTTPMatch{
+				PathPrefix: "/prefix",
+				PathRegex:  "/regex",
+			}))),
+			validateErr: "should only contain at most one of PathExact, PathPrefix, or PathRegex",
+		},
+		{
+			name: "route with path exact, prefix, and regex",
+			entry: makerouter(routeMatch(httpMatch(&ServiceRouteHTTPMatch{
+				PathExact:  "/exact",
+				PathPrefix: "/prefix",
+				PathRegex:  "/regex",
+			}))),
+			validateErr: "should only contain at most one of PathExact, PathPrefix, or PathRegex",
+		},
+
+		{
+			name: "route with no name header",
+			entry: makerouter(routeMatch(httpMatchHeader(ServiceRouteHTTPMatchHeader{
+				Present: true,
+			}))),
+			validateErr: "missing required Name field",
+		},
+		{
+			name: "route with header present",
+			entry: makerouter(routeMatch(httpMatchHeader(ServiceRouteHTTPMatchHeader{
+				Name:    "foo",
+				Present: true,
+			}))),
+		},
+		{
+			name: "route with header not present",
+			entry: makerouter(routeMatch(httpMatchHeader(ServiceRouteHTTPMatchHeader{
+				Name:    "foo",
+				Present: true,
+				Invert:  true,
+			}))),
+		},
+		{
+			name: "route with header exact",
+			entry: makerouter(routeMatch(httpMatchHeader(ServiceRouteHTTPMatchHeader{
+				Name:  "foo",
+				Exact: "bar",
+			}))),
+		},
+		{
+			name: "route with header regex",
+			entry: makerouter(routeMatch(httpMatchHeader(ServiceRouteHTTPMatchHeader{
+				Name:  "foo",
+				Regex: "bar",
+			}))),
+		},
+		{
+			name: "route with header prefix",
+			entry: makerouter(routeMatch(httpMatchHeader(ServiceRouteHTTPMatchHeader{
+				Name:   "foo",
+				Prefix: "bar",
+			}))),
+		},
+		{
+			name: "route with header suffix",
+			entry: makerouter(routeMatch(httpMatchHeader(ServiceRouteHTTPMatchHeader{
+				Name:   "foo",
+				Suffix: "bar",
+			}))),
+		},
+		{
+			name: "route with header present and exact",
+			entry: makerouter(routeMatch(httpMatchHeader(ServiceRouteHTTPMatchHeader{
+				Name:    "foo",
+				Present: true,
+				Exact:   "bar",
+			}))),
+			validateErr: "should only contain one of Present, Exact, Prefix, Suffix, or Regex",
+		},
+		{
+			name: "route with header present and regex",
+			entry: makerouter(routeMatch(httpMatchHeader(ServiceRouteHTTPMatchHeader{
+				Name:    "foo",
+				Present: true,
+				Regex:   "bar",
+			}))),
+			validateErr: "should only contain one of Present, Exact, Prefix, Suffix, or Regex",
+		},
+		{
+			name: "route with header present and prefix",
+			entry: makerouter(routeMatch(httpMatchHeader(ServiceRouteHTTPMatchHeader{
+				Name:    "foo",
+				Present: true,
+				Prefix:  "bar",
+			}))),
+			validateErr: "should only contain one of Present, Exact, Prefix, Suffix, or Regex",
+		},
+		{
+			name: "route with header present and suffix",
+			entry: makerouter(routeMatch(httpMatchHeader(ServiceRouteHTTPMatchHeader{
+				Name:    "foo",
+				Present: true,
+				Suffix:  "bar",
+			}))),
+			validateErr: "should only contain one of Present, Exact, Prefix, Suffix, or Regex",
+		},
+		// NOTE: Some combinatoric cases for header operators (some 5 choose 2,
+		// all 5 choose 3, all 5 choose 4, all 5 choose 5) are omitted from
+		// testing.
+
+		////////////////
+		{
+			name: "route with no name query param",
+			entry: makerouter(routeMatch(httpMatchParam(ServiceRouteHTTPMatchQueryParam{
+				Value: "foo",
+			}))),
+			validateErr: "missing required Name field",
+		},
+
+		////////////////
+		{
+			name: "route with no match and prefix rewrite",
+			entry: makerouter(ServiceRoute{
+				Match: nil,
+				Destination: &ServiceRouteDestination{
+					Service:       "other",
+					PrefixRewrite: "/new",
+				},
+			}),
+			validateErr: "cannot make use of PrefixRewrite without configuring either PathExact or PathPrefix",
+		},
+		{
+			name: "route with path prefix match and prefix rewrite",
+			entry: makerouter(ServiceRoute{
+				Match: httpMatch(&ServiceRouteHTTPMatch{
+					PathPrefix: "/api",
+				}),
+				Destination: &ServiceRouteDestination{
+					Service:       "other",
+					PrefixRewrite: "/new",
+				},
+			}),
+		},
+		{
+			name: "route with path exact match and prefix rewrite",
+			entry: makerouter(ServiceRoute{
+				Match: httpMatch(&ServiceRouteHTTPMatch{
+					PathExact: "/api",
+				}),
+				Destination: &ServiceRouteDestination{
+					Service:       "other",
+					PrefixRewrite: "/new",
+				},
+			}),
+		},
+		{
+			name: "route with path regex match and prefix rewrite",
+			entry: makerouter(ServiceRoute{
+				Match: httpMatch(&ServiceRouteHTTPMatch{
+					PathRegex: "/api",
+				}),
+				Destination: &ServiceRouteDestination{
+					Service:       "other",
+					PrefixRewrite: "/new",
+				},
+			}),
+			validateErr: "cannot make use of PrefixRewrite without configuring either PathExact or PathPrefix",
+		},
+		{
+			name: "route with header match and prefix rewrite",
+			entry: makerouter(ServiceRoute{
+				Match: httpMatchHeader(ServiceRouteHTTPMatchHeader{
+					Name:  "foo",
+					Exact: "bar",
+				}),
+				Destination: &ServiceRouteDestination{
+					Service:       "other",
+					PrefixRewrite: "/new",
+				},
+			}),
+			validateErr: "cannot make use of PrefixRewrite without configuring either PathExact or PathPrefix",
+		},
+		{
+			name: "route with header match and prefix rewrite",
+			entry: makerouter(ServiceRoute{
+				Match: httpMatchParam(ServiceRouteHTTPMatchQueryParam{
+					Name:  "foo",
+					Value: "bar",
+				}),
+				Destination: &ServiceRouteDestination{
+					Service:       "other",
+					PrefixRewrite: "/new",
+				},
+			}),
+			validateErr: "cannot make use of PrefixRewrite without configuring either PathExact or PathPrefix",
+		},
+	}
+
+	for _, tc := range cases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			err := tc.entry.Normalize()
+			if tc.normalizeErr != "" {
+				require.Error(t, err)
+				require.Contains(t, err.Error(), tc.normalizeErr)
+				return
+			}
+			require.NoError(t, err)
+
+			if tc.check != nil {
+				tc.check(t, tc.entry)
+			}
+
+			err = tc.entry.Validate()
+			if tc.validateErr != "" {
+				require.Error(t, err)
+				require.Contains(t, err.Error(), tc.validateErr)
+				return
+			}
+			require.NoError(t, err)
+		})
+	}
+}
+
+var validSubsetNames = []string{
+	"a", "aa", "2a", "a2", "a2a", "a22a",
+	"1", "11", "10", "01",
+	"a-a", "a--a", "a--a--a",
+	"0-0", "0--0", "0--0--0",
+	strings.Repeat("a", 63),
+}
+
+var invalidSubsetNames = []string{
+	"A", "AA", "2A", "A2", "A2A", "A22A",
+	"A-A", "A--A", "A--A--A",
+	" ", " a", "a ", "a a",
+	"_", "_a", "a_", "a_a",
+	".", ".a", "a.", "a.a",
+	"-", "-a", "a-",
+	strings.Repeat("a", 64),
+}
+
+func TestValidateServiceSubset(t *testing.T) {
+	for _, name := range validSubsetNames {
+		t.Run(name, func(t *testing.T) {
+			require.NoError(t, validateServiceSubset(name))
+		})
+	}
+
+	for _, name := range invalidSubsetNames {
+		t.Run(name, func(t *testing.T) {
+			require.Error(t, validateServiceSubset(name))
+		})
+	}
+}

agent/xds/routes.go

@@ -240,12 +240,6 @@ func makeRouteMatchForDiscoveryRoute(discoveryRoute *structs.DiscoveryRoute, pro
 				eh.HeaderMatchSpecifier = &envoyroute.HeaderMatcher_PresentMatch{
 					PresentMatch: true,
 				}
-			case hdr.Invert: // THIS HAS TO BE LAST
-				eh.HeaderMatchSpecifier = &envoyroute.HeaderMatcher_PresentMatch{
-					// We set this to the misleading value of 'true' here
-					// because we'll generically invert it next.
-					PresentMatch: true,
-				}
 			default:
 				continue // skip this impossible situation
 			}

agent/xds/routes_test.go

@@ -156,8 +156,9 @@ func TestRoutesFromSnapshot(t *testing.T) {
 							},
 							{
 								Match: httpMatchHeader(structs.ServiceRouteHTTPMatchHeader{
-									Name:   "x-debug",
+									Name:    "x-debug",
-									Invert: true,
+									Present: true,
+									Invert:  true,
 								}),
 								Destination: toService("hdr-not-present"),
 							},