diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index e380a51b5..1170eef16 100644 --- a/agent/config/runtime_test.go +++ b/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, }, { diff --git a/agent/structs/config_entry_discoverychain.go b/agent/structs/config_entry_discoverychain.go index 369232381..1c929bc90 100644 --- a/agent/structs/config_entry_discoverychain.go +++ b/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 { - continue - } + 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 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 - 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) - } + 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) + } + } - // 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) + for j, qm := range route.Match.HTTP.QueryParam { + if qm.Name == "" { + return fmt.Errorf("Route[%d] QueryParam[%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 +} diff --git a/agent/structs/config_entry_discoverychain_test.go b/agent/structs/config_entry_discoverychain_test.go index f67f99aa6..8e3310e56 100644 --- a/agent/structs/config_entry_discoverychain_test.go +++ b/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)) + }) + } +} diff --git a/agent/xds/routes.go b/agent/xds/routes.go index 1954d7f97..d8569db86 100644 --- a/agent/xds/routes.go +++ b/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 } diff --git a/agent/xds/routes_test.go b/agent/xds/routes_test.go index 5c3d15c3f..23c241ce3 100644 --- a/agent/xds/routes_test.go +++ b/agent/xds/routes_test.go @@ -156,8 +156,9 @@ func TestRoutesFromSnapshot(t *testing.T) { }, { Match: httpMatchHeader(structs.ServiceRouteHTTPMatchHeader{ - Name: "x-debug", - Invert: true, + Name: "x-debug", + Present: true, + Invert: true, }), Destination: toService("hdr-not-present"), },