From 2ad516aeaf5fe460d8baf32298437ea4a59e9721 Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" Date: Mon, 1 Jul 2019 15:23:36 -0500 Subject: [PATCH] do some initial config entry graph validation during writes (#6047) --- agent/consul/discoverychain/compile.go | 101 ++- agent/consul/discoverychain/compile_test.go | 282 +++++- agent/consul/state/config_entry.go | 515 +++++++++++ .../state/config_entry_discoverychain.go | 287 ------ agent/consul/state/config_entry_test.go | 823 ++++++++++++++++++ agent/structs/config_entry.go | 9 + agent/structs/config_entry_discoverychain.go | 186 ++-- .../config_entry_discoverychain_test.go | 18 +- api/config_entry_discoverychain.go | 3 +- api/config_entry_discoverychain_test.go | 268 +++--- api/config_entry_test.go | 1 - 11 files changed, 1942 insertions(+), 551 deletions(-) delete mode 100644 agent/consul/state/config_entry_discoverychain.go diff --git a/agent/consul/discoverychain/compile.go b/agent/consul/discoverychain/compile.go index 5e79c2fde7..462c356e37 100644 --- a/agent/consul/discoverychain/compile.go +++ b/agent/consul/discoverychain/compile.go @@ -8,7 +8,13 @@ import ( "github.com/hashicorp/consul/agent/structs" ) -// TODO(rb): surface any specific errors that may matter during graph vetting at write-time (like mixing protocols) +type CompileRequest struct { + ServiceName string + CurrentNamespace string + CurrentDatacenter string + InferDefaults bool // TODO(rb): remove this? + Entries *structs.DiscoveryChainConfigEntries +} // Compile assembles a discovery chain in the form of a graph of nodes using // raw config entries and local context. @@ -19,13 +25,19 @@ import ( // Omitting router and splitter entries for services not using an L7 protocol // (like HTTP) happens during initial fetching, but for sanity purposes a quick // reinforcement of that happens here, too. -func Compile( - serviceName string, - currentNamespace string, - currentDatacenter string, - inferDefaults bool, - entries *structs.DiscoveryChainConfigEntries, -) (*structs.CompiledDiscoveryChain, error) { +// +// May return a *structs.ConfigEntryGraphError, but that is only expected when +// being used to validate modifications to the config entry graph. It should +// not be expected when compiling existing entries at runtime that are already +// valid. +func Compile(req CompileRequest) (*structs.CompiledDiscoveryChain, error) { + var ( + serviceName = req.ServiceName + currentNamespace = req.CurrentNamespace + currentDatacenter = req.CurrentDatacenter + inferDefaults = req.InferDefaults + entries = req.Entries + ) if serviceName == "" { return nil, fmt.Errorf("serviceName is required") } @@ -39,10 +51,6 @@ func Compile( return nil, fmt.Errorf("entries is required") } - // This shouldn't be necessary, but do it anyway. It is the one place input - // mutation will occur, but only if the caller forgot in the first place. - entries.Fixup() - c := &compiler{ serviceName: serviceName, currentNamespace: currentNamespace, @@ -85,6 +93,10 @@ type compiler struct { splitterNodes map[string]*structs.DiscoveryGraphNode groupResolverNodes map[structs.DiscoveryTarget]*structs.DiscoveryGraphNode // this is also an OUTPUT field + // usesAdvancedRoutingFeatures is set to true if config entries for routing + // or splitting appear in the compiled chain + usesAdvancedRoutingFeatures bool + // topNode is computed inside of assembleChain() // // This is an OUTPUT field. @@ -113,11 +125,10 @@ type compiler struct { } func (c *compiler) recordServiceProtocol(serviceName string) error { - if serviceDefault, ok := c.entries.Services[serviceName]; ok { + if serviceDefault := c.entries.GetService(serviceName); serviceDefault != nil { return c.recordProtocol(serviceName, serviceDefault.Protocol) - } else { - return c.recordProtocol(serviceName, "") } + return c.recordProtocol(serviceName, "") } func (c *compiler) recordProtocol(fromService, protocol string) error { @@ -130,8 +141,12 @@ func (c *compiler) recordProtocol(fromService, protocol string) error { if c.protocol == "" { c.protocol = protocol } else if c.protocol != protocol { - // TODO(rb): avoid this during config entry writes instead - return fmt.Errorf("discovery chain %q uses inconsistent protocols; service %q has %q != %q", c.serviceName, fromService, protocol, c.protocol) + return &structs.ConfigEntryGraphError{ + Message: fmt.Sprintf( + "discovery chain %q uses inconsistent protocols; service %q has %q which is not %q", + c.serviceName, fromService, protocol, c.protocol, + ), + } } return nil @@ -165,6 +180,15 @@ func (c *compiler) compile() (*structs.CompiledDiscoveryChain, error) { } } + if !enableAdvancedRoutingForProtocol(c.protocol) && c.usesAdvancedRoutingFeatures { + return nil, &structs.ConfigEntryGraphError{ + Message: fmt.Sprintf( + "discovery chain %q uses a protocol %q that does not permit advanced routing or splitting behavior", + c.serviceName, c.protocol, + ), + } + } + targets := make([]structs.DiscoveryTarget, 0, len(c.targets)) for target, _ := range c.targets { targets = append(targets, target) @@ -252,8 +276,8 @@ func (c *compiler) assembleChain() error { // The only router we consult is the one for the service name at the top of // the chain. - router, ok := c.entries.Routers[c.serviceName] - if !ok { + router := c.entries.GetRouter(c.serviceName) + if router == nil { // If no router is configured, move on down the line to the next hop of // the chain. node, err := c.getSplitterOrGroupResolverNode(c.newTarget(c.serviceName, "", "", "")) @@ -270,6 +294,7 @@ func (c *compiler) assembleChain() error { Name: router.Name, Routes: make([]*structs.DiscoveryRoute, 0, len(router.Routes)+1), } + c.usesAdvancedRoutingFeatures = true if err := c.recordServiceProtocol(router.Name); err != nil { return err } @@ -368,8 +393,8 @@ func (c *compiler) getSplitterNode(name string) (*structs.DiscoveryGraphNode, er } // Fetch the config entry. - splitter, ok := c.entries.Splitters[name] - if !ok { + splitter := c.entries.GetSplitter(name) + if splitter == nil { return nil, nil } @@ -413,6 +438,7 @@ func (c *compiler) getSplitterNode(name string) (*structs.DiscoveryGraphNode, er compiledSplit.Node = node } + c.usesAdvancedRoutingFeatures = true return splitNode, nil } @@ -439,6 +465,8 @@ RESOLVE_AGAIN: } // Handle redirects right up front. + // + // TODO(rb): What about a redirected subset reference? (web/v2, but web redirects to alt/"") if resolver.Redirect != nil { redirect := resolver.Redirect @@ -460,16 +488,19 @@ RESOLVE_AGAIN: goto RESOLVE_AGAIN } - // Since we're actually building a node with it, we can keep it. - // - // TODO(rb): maybe infer this from the keyspace of the groupresolvernodes slice. - c.retainResolvers[target.Service] = struct{}{} - - if target.Service != resolver.Name { - //TODO(rb): remove - panic("NOT POSSIBLE") + if target.ServiceSubset != "" && !resolver.SubsetExists(target.ServiceSubset) { + return nil, &structs.ConfigEntryGraphError{ + Message: fmt.Sprintf( + "service %q does not have a subset named %q", + target.Service, + target.ServiceSubset, + ), + } } + // Since we're actually building a node with it, we can keep it. + c.retainResolvers[target.Service] = struct{}{} + connectTimeout := resolver.ConnectTimeout if connectTimeout < 1 { connectTimeout = 5 * time.Second @@ -555,9 +586,6 @@ RESOLVE_AGAIN: if err != nil { return nil, err } - if failoverGroupResolverNode.Type != structs.DiscoveryGraphNodeTypeGroupResolver { - panic("TODO(rb)(remove): '" + failoverGroupResolverNode.Type + "' is not a group-resolver node") - } failoverTarget := failoverGroupResolverNode.GroupResolver.Target df.Targets = append(df.Targets, failoverTarget) } @@ -581,3 +609,12 @@ func defaultIfEmpty(val, defaultVal string) string { } return defaultVal } + +func enableAdvancedRoutingForProtocol(protocol string) bool { + switch protocol { + case "http", "http2", "grpc": + return true + default: + return false + } +} diff --git a/agent/consul/discoverychain/compile_test.go b/agent/consul/discoverychain/compile_test.go index e21dd62b41..04c213f890 100644 --- a/agent/consul/discoverychain/compile_test.go +++ b/agent/consul/discoverychain/compile_test.go @@ -11,14 +11,22 @@ import ( func TestCompile_NoEntries_NoInferDefaults(t *testing.T) { entries := newEntries() - res, err := Compile("main", "default", "dc1", false, entries) + res, err := Compile(CompileRequest{ + ServiceName: "main", + CurrentNamespace: "default", + CurrentDatacenter: "dc1", + InferDefaults: false, + Entries: entries, + }) require.NoError(t, err) require.Nil(t, res) } type compileTestCase struct { - entries *structs.DiscoveryChainConfigEntries - expect *structs.CompiledDiscoveryChain // the GroupResolverNodes map should have nil values + entries *structs.DiscoveryChainConfigEntries + expect *structs.CompiledDiscoveryChain // the GroupResolverNodes map should have nil values + expectErr string + expectGraphErr bool } func TestCompile(t *testing.T) { @@ -45,9 +53,19 @@ func TestCompile(t *testing.T) { "noop split to resolver with default subset": testcase_NoopSplit_WithDefaultSubset(), "resolver with default subset": testcase_Resolve_WithDefaultSubset(), "resolver with no entries and inferring defaults": testcase_DefaultResolver(), + // TODO(rb): handle this case better: "circular split": testcase_CircularSplit(), "all the bells and whistles": testcase_AllBellsAndWhistles(), "multi dc canary": testcase_MultiDatacenterCanary(), + + // various errors + "splitter requires valid protocol": testcase_SplitterRequiresValidProtocol(), + "router requires valid protocol": testcase_RouterRequiresValidProtocol(), + "split to unsplittable protocol": testcase_SplitToUnsplittableProtocol(), + "route to unroutable protocol": testcase_RouteToUnroutableProtocol(), + "failover crosses protocols": testcase_FailoverCrossesProtocols(), + "redirect crosses protocols": testcase_RedirectCrossesProtocols(), + "redirect to missing subset": testcase_RedirectToMissingSubset(), } for name, tc := range cases { @@ -69,26 +87,43 @@ func TestCompile(t *testing.T) { require.NoError(t, entry.Validate()) } - res, err := Compile("main", "default", "dc1", true, tc.entries) - require.NoError(t, err) - - // Avoid requiring unnecessary test boilerplate and inject these - // ourselves. - tc.expect.ServiceName = "main" - tc.expect.Namespace = "default" - tc.expect.Datacenter = "dc1" - - // These nodes are duplicated elsewhere in the results, so we only - // care that the keys are present. Walk the results and nil out the - // value payloads so that the require.Equal will still do the work - // for us. - if len(res.GroupResolverNodes) > 0 { - for target, _ := range res.GroupResolverNodes { - res.GroupResolverNodes[target] = nil + res, err := Compile(CompileRequest{ + ServiceName: "main", + CurrentNamespace: "default", + CurrentDatacenter: "dc1", + InferDefaults: true, + Entries: tc.entries, + }) + if tc.expectErr != "" { + require.Error(t, err) + require.Contains(t, err.Error(), tc.expectErr) + _, ok := err.(*structs.ConfigEntryGraphError) + if tc.expectGraphErr { + require.True(t, ok, "%T is not a *ConfigEntryGraphError", err) + } else { + require.False(t, ok, "did not expect a *ConfigEntryGraphError here: %v", err) } - } + } else { + require.NoError(t, err) - require.Equal(t, tc.expect, res) + // Avoid requiring unnecessary test boilerplate and inject these + // ourselves. + tc.expect.ServiceName = "main" + tc.expect.Namespace = "default" + tc.expect.Datacenter = "dc1" + + // These nodes are duplicated elsewhere in the results, so we only + // care that the keys are present. Walk the results and nil out the + // value payloads so that the require.Equal will still do the work + // for us. + if len(res.GroupResolverNodes) > 0 { + for target, _ := range res.GroupResolverNodes { + res.GroupResolverNodes[target] = nil + } + } + + require.Equal(t, tc.expect, res) + } }) } } @@ -365,7 +400,7 @@ func testcase_RouteBypassesSplit() compileTestCase { Name: "other", Subsets: map[string]structs.ServiceResolverSubset{ "bypass": { - Filter: "ServiceMeta.version == bypass", + Filter: "Service.Meta.version == bypass", }, }, }, @@ -554,10 +589,10 @@ func testcase_SubsetSplit() compileTestCase { Name: "main", Subsets: map[string]structs.ServiceResolverSubset{ "v1": { - Filter: "ServiceMeta.version == 1", + Filter: "Service.Meta.version == 1", }, "v2": { - Filter: "ServiceMeta.version == 2", + Filter: "Service.Meta.version == 2", }, }, }, @@ -718,10 +753,10 @@ func testcase_SplitBypassesSplit() compileTestCase { Name: "next", Subsets: map[string]structs.ServiceResolverSubset{ "bypassed": { - Filter: "ServiceMeta.version == bypass", + Filter: "Service.Meta.version == bypass", }, "not-bypassed": { - Filter: "ServiceMeta.version != bypass", + Filter: "Service.Meta.version != bypass", }, }, }, @@ -819,10 +854,10 @@ func testcase_ServiceAndSubsetRedirect() compileTestCase { Name: "other", Subsets: map[string]structs.ServiceResolverSubset{ "v1": { - Filter: "ServiceMeta.version == 1", + Filter: "Service.Meta.version == 1", }, "v2": { - Filter: "ServiceMeta.version == 2", + Filter: "Service.Meta.version == 2", }, }, }, @@ -949,7 +984,7 @@ func testcase_ServiceAndSubsetFailover() compileTestCase { Name: "main", Subsets: map[string]structs.ServiceResolverSubset{ "backup": { - Filter: "ServiceMeta.version == backup", + Filter: "Service.Meta.version == backup", }, }, Failover: map[string]structs.ServiceResolverFailover{ @@ -1059,8 +1094,8 @@ func testcase_NoopSplit_WithDefaultSubset() compileTestCase { Name: "main", DefaultSubset: "v2", Subsets: map[string]structs.ServiceResolverSubset{ - "v1": {Filter: "ServiceMeta.version == 1"}, - "v2": {Filter: "ServiceMeta.version == 2"}, + "v1": {Filter: "Service.Meta.version == 1"}, + "v2": {Filter: "Service.Meta.version == 2"}, }, }, ) @@ -1138,8 +1173,8 @@ func testcase_Resolve_WithDefaultSubset() compileTestCase { Name: "main", DefaultSubset: "v2", Subsets: map[string]structs.ServiceResolverSubset{ - "v1": {Filter: "ServiceMeta.version == 1"}, - "v2": {Filter: "ServiceMeta.version == 2"}, + "v1": {Filter: "Service.Meta.version == 1"}, + "v2": {Filter: "Service.Meta.version == 2"}, }, }, ) @@ -1394,9 +1429,9 @@ func testcase_AllBellsAndWhistles() compileTestCase { Name: "main", DefaultSubset: "default-subset", Subsets: map[string]structs.ServiceResolverSubset{ - "v1": {Filter: "ServiceMeta.version == 1"}, - "v2": {Filter: "ServiceMeta.version == 2"}, - "v3": {Filter: "ServiceMeta.version == 3"}, + "v1": {Filter: "Service.Meta.version == 1"}, + "v2": {Filter: "Service.Meta.version == 2"}, + "v3": {Filter: "Service.Meta.version == 3"}, "default-subset": {OnlyPassing: true}, }, }, @@ -1519,6 +1554,181 @@ func testcase_AllBellsAndWhistles() compileTestCase { return compileTestCase{entries: entries, expect: expect} } +func testcase_SplitterRequiresValidProtocol() compileTestCase { + entries := newEntries() + setServiceProtocol(entries, "main", "tcp") + + entries.AddSplitters( + &structs.ServiceSplitterConfigEntry{ + Kind: structs.ServiceSplitter, + Name: "main", + Splits: []structs.ServiceSplit{ + {Weight: 90, Namespace: "v1"}, + {Weight: 10, Namespace: "v2"}, + }, + }, + ) + + return compileTestCase{ + entries: entries, + expectErr: "does not permit advanced routing or splitting behavior", + expectGraphErr: true, + } +} + +func testcase_RouterRequiresValidProtocol() compileTestCase { + entries := newEntries() + setServiceProtocol(entries, "main", "tcp") + + entries.AddRouters( + &structs.ServiceRouterConfigEntry{ + Kind: structs.ServiceRouter, + Name: "main", + Routes: []structs.ServiceRoute{ + { + Match: &structs.ServiceRouteMatch{ + HTTP: &structs.ServiceRouteHTTPMatch{ + PathExact: "/other", + }, + }, + Destination: &structs.ServiceRouteDestination{ + Namespace: "other", + }, + }, + }, + }, + ) + return compileTestCase{ + entries: entries, + expectErr: "does not permit advanced routing or splitting behavior", + expectGraphErr: true, + } +} + +func testcase_SplitToUnsplittableProtocol() compileTestCase { + entries := newEntries() + setServiceProtocol(entries, "main", "tcp") + setServiceProtocol(entries, "other", "tcp") + + entries.AddSplitters( + &structs.ServiceSplitterConfigEntry{ + Kind: structs.ServiceSplitter, + Name: "main", + Splits: []structs.ServiceSplit{ + {Weight: 90}, + {Weight: 10, Service: "other"}, + }, + }, + ) + return compileTestCase{ + entries: entries, + expectErr: "does not permit advanced routing or splitting behavior", + expectGraphErr: true, + } +} + +func testcase_RouteToUnroutableProtocol() compileTestCase { + entries := newEntries() + setServiceProtocol(entries, "main", "tcp") + setServiceProtocol(entries, "other", "tcp") + + entries.AddRouters( + &structs.ServiceRouterConfigEntry{ + Kind: structs.ServiceRouter, + Name: "main", + Routes: []structs.ServiceRoute{ + { + Match: &structs.ServiceRouteMatch{ + HTTP: &structs.ServiceRouteHTTPMatch{ + PathExact: "/other", + }, + }, + Destination: &structs.ServiceRouteDestination{ + Service: "other", + }, + }, + }, + }, + ) + + return compileTestCase{ + entries: entries, + expectErr: "does not permit advanced routing or splitting behavior", + expectGraphErr: true, + } +} + +func testcase_FailoverCrossesProtocols() compileTestCase { + entries := newEntries() + setServiceProtocol(entries, "main", "grpc") + setServiceProtocol(entries, "other", "tcp") + + entries.AddResolvers( + &structs.ServiceResolverConfigEntry{ + Kind: structs.ServiceResolver, + Name: "main", + Failover: map[string]structs.ServiceResolverFailover{ + "*": structs.ServiceResolverFailover{ + Service: "other", + }, + }, + }, + ) + + return compileTestCase{ + entries: entries, + expectErr: "uses inconsistent protocols", + expectGraphErr: true, + } +} + +func testcase_RedirectCrossesProtocols() compileTestCase { + entries := newEntries() + setServiceProtocol(entries, "main", "grpc") + setServiceProtocol(entries, "other", "tcp") + + entries.AddResolvers( + &structs.ServiceResolverConfigEntry{ + Kind: structs.ServiceResolver, + Name: "main", + Redirect: &structs.ServiceResolverRedirect{ + Service: "other", + }, + }, + ) + return compileTestCase{ + entries: entries, + expectErr: "uses inconsistent protocols", + expectGraphErr: true, + } +} + +func testcase_RedirectToMissingSubset() compileTestCase { + entries := newEntries() + + entries.AddResolvers( + &structs.ServiceResolverConfigEntry{ + Kind: structs.ServiceResolver, + Name: "other", + ConnectTimeout: 33 * time.Second, + }, + &structs.ServiceResolverConfigEntry{ + Kind: structs.ServiceResolver, + Name: "main", + Redirect: &structs.ServiceResolverRedirect{ + Service: "other", + ServiceSubset: "v1", + }, + }, + ) + + return compileTestCase{ + entries: entries, + expectErr: `does not have a subset named "v1"`, + expectGraphErr: true, + } +} + func newSimpleRoute(name string, muts ...func(*structs.ServiceRoute)) structs.ServiceRoute { r := structs.ServiceRoute{ Match: &structs.ServiceRouteMatch{ diff --git a/agent/consul/state/config_entry.go b/agent/consul/state/config_entry.go index b56e8276cf..bc5e54f83a 100644 --- a/agent/consul/state/config_entry.go +++ b/agent/consul/state/config_entry.go @@ -3,6 +3,7 @@ package state import ( "fmt" + "github.com/hashicorp/consul/agent/consul/discoverychain" "github.com/hashicorp/consul/agent/structs" memdb "github.com/hashicorp/go-memdb" ) @@ -43,10 +44,79 @@ func configTableSchema() *memdb.TableSchema { Lowercase: true, }, }, + "link": &memdb.IndexSchema{ + Name: "link", + AllowMissing: true, + Unique: false, + Indexer: &ConfigEntryLinkIndex{}, + }, }, } } +type ConfigEntryLinkIndex struct { +} + +type discoveryChainConfigEntry interface { + structs.ConfigEntry + // ListRelatedServices returns a list of other names of services referenced + // in this config entry. + ListRelatedServices() []string +} + +func (s *ConfigEntryLinkIndex) FromObject(obj interface{}) (bool, [][]byte, error) { + entry, ok := obj.(structs.ConfigEntry) + if !ok { + return false, nil, fmt.Errorf("object is not a ConfigEntry") + } + + dcEntry, ok := entry.(discoveryChainConfigEntry) + if !ok { + return false, nil, nil + } + + linkedServices := dcEntry.ListRelatedServices() + + numLinks := len(linkedServices) + if numLinks == 0 { + return false, nil, nil + } + + vals := make([][]byte, 0, numLinks) + for _, linkedService := range linkedServices { + vals = append(vals, []byte(linkedService+"\x00")) + } + + return true, vals, nil +} + +func (s *ConfigEntryLinkIndex) FromArgs(args ...interface{}) ([]byte, error) { + if len(args) != 1 { + return nil, fmt.Errorf("must provide only a single argument") + } + arg, ok := args[0].(string) + if !ok { + return nil, fmt.Errorf("argument must be a string: %#v", args[0]) + } + // Add the null character as a terminator + arg += "\x00" + return []byte(arg), nil +} + +func (s *ConfigEntryLinkIndex) PrefixFromArgs(args ...interface{}) ([]byte, error) { + val, err := s.FromArgs(args...) + if err != nil { + return nil, err + } + + // Strip the null terminator, the rest is a prefix + n := len(val) + if n > 0 { + return val[:n-1], nil + } + return val, nil +} + func init() { registerSchema(configTableSchema) } @@ -173,6 +243,23 @@ func (s *Store) ensureConfigEntryTxn(tx *memdb.Txn, idx uint64, conf structs.Con } raftIndex.ModifyIndex = idx + var existingConf structs.ConfigEntry + if existing != nil { + existingConf = existing.(structs.ConfigEntry) + } + + err = s.validateProposedConfigEntryInGraph( + tx, + idx, + conf.GetKind(), + conf.GetName(), + existingConf, + conf, + ) + if err != nil { + return err // Err is already sufficiently decorated. + } + // Insert the config entry and update the index if err := tx.Insert(configTableName, conf); err != nil { return fmt.Errorf("failed inserting config entry: %s", err) @@ -232,6 +319,23 @@ func (s *Store) DeleteConfigEntry(idx uint64, kind, name string) error { return nil } + var existingConf structs.ConfigEntry + if existing != nil { + existingConf = existing.(structs.ConfigEntry) + } + + err = s.validateProposedConfigEntryInGraph( + tx, + idx, + kind, + name, + existingConf, + nil, + ) + if err != nil { + return err // Err is already sufficiently decorated. + } + // Delete the config entry from the DB and update the index. if err := tx.Delete(configTableName, existing); err != nil { return fmt.Errorf("failed removing check: %s", err) @@ -243,3 +347,414 @@ func (s *Store) DeleteConfigEntry(idx uint64, kind, name string) error { tx.Commit() return nil } + +// validateProposedConfigEntryInGraph can be used to verify graph integrity for +// a proposed graph create/update/delete. +// +// This must be called before any mutations occur on the config entries table! +// +// May return *ConfigEntryGraphValidationError if there is a concern to surface +// to the caller that they can correct. +func (s *Store) validateProposedConfigEntryInGraph( + tx *memdb.Txn, + idx uint64, + kind, name string, + prev, next structs.ConfigEntry, +) error { + switch kind { + case structs.ProxyDefaults: + return nil // no validation + case structs.ServiceDefaults: + fallthrough + case structs.ServiceRouter: + fallthrough + case structs.ServiceSplitter: + fallthrough + case structs.ServiceResolver: + return s.validateProposedConfigEntryInServiceGraph(tx, idx, kind, name, prev, next) + default: + return fmt.Errorf("unhandled kind %q during validation of %q", kind, name) + } +} + +func (s *Store) validateProposedConfigEntryInServiceGraph( + tx *memdb.Txn, + idx uint64, + kind, name string, + prev, next structs.ConfigEntry, +) error { + // Collect all of the chains that could be affected by this change + // including our own. + checkChains := map[string]struct{}{ + name: struct{}{}, + } + + iter, err := tx.Get(configTableName, "link", name) + for raw := iter.Next(); raw != nil; raw = iter.Next() { + entry := raw.(structs.ConfigEntry) + checkChains[entry.GetName()] = struct{}{} + } + if err != nil { + return err + } + + overrides := map[structs.ConfigEntryKindName]structs.ConfigEntry{ + {Kind: kind, Name: name}: next, + } + + for chainName, _ := range checkChains { + _, speculativeEntries, err := s.readDiscoveryChainConfigEntriesTxn(tx, nil, chainName, overrides) + if err != nil { + return err + } + // fmt.Printf("SPEC: %s/%s chain=%q, prev=%v, next=%v, ent=%+v\n", + // kind, name, + // chainName, + // prev != nil, + // next != nil, speculativeEntries) + + // TODO(rb): is this ok that we execute the compiler in the state store? + + // Note we use an arbitrary namespace and datacenter as those would not + // currently affect the graph compilation in ways that matter here. + req := discoverychain.CompileRequest{ + ServiceName: chainName, + CurrentNamespace: "default", + CurrentDatacenter: "dc1", + Entries: speculativeEntries, + } + if _, err := discoverychain.Compile(req); err != nil { + return err + } + } + + return nil +} + +// ReadDiscoveryChainConfigEntries will query for the full discovery chain for +// the provided service name. All relevant config entries will be recursively +// fetched and included in the result. +// +// Once returned, the caller still needs to assemble these into a useful graph +// structure. +func (s *Store) ReadDiscoveryChainConfigEntries( + ws memdb.WatchSet, + serviceName string, +) (uint64, *structs.DiscoveryChainConfigEntries, error) { + return s.readDiscoveryChainConfigEntries(ws, serviceName, nil) +} + +// readDiscoveryChainConfigEntries will query for the full discovery chain for +// the provided service name. All relevant config entries will be recursively +// fetched and included in the result. +// +// If 'overrides' is provided then it will use entries in that map instead of +// the database to simulate the entries that go into a modified discovery chain +// without actually modifying it yet. Nil values are tombstones to simulate +// deleting an entry. +// +// Overrides is not mutated. +func (s *Store) readDiscoveryChainConfigEntries( + ws memdb.WatchSet, + serviceName string, + overrides map[structs.ConfigEntryKindName]structs.ConfigEntry, +) (uint64, *structs.DiscoveryChainConfigEntries, error) { + tx := s.db.Txn(false) + defer tx.Abort() + return s.readDiscoveryChainConfigEntriesTxn(tx, ws, serviceName, overrides) +} + +func (s *Store) readDiscoveryChainConfigEntriesTxn( + tx *memdb.Txn, + ws memdb.WatchSet, + serviceName string, + overrides map[structs.ConfigEntryKindName]structs.ConfigEntry, +) (uint64, *structs.DiscoveryChainConfigEntries, error) { + res := &structs.DiscoveryChainConfigEntries{ + Routers: make(map[string]*structs.ServiceRouterConfigEntry), + Splitters: make(map[string]*structs.ServiceSplitterConfigEntry), + Resolvers: make(map[string]*structs.ServiceResolverConfigEntry), + Services: make(map[string]*structs.ServiceConfigEntry), + } + + // Note that below we always look up splitters and resolvers in pairs, even + // in some circumstances where both are not strictly necessary. + // + // For now we'll just eat the cost of fetching pairs of splitter/resolver + // config entries even though we may not always need both. In the common + // case we will need the pair so there's not a big drive to optimize this + // here at this time. + + // Both Splitters and Resolvers maps will contain placeholder nils until + // the end of this function to indicate "no such entry". + + var ( + idx uint64 + todoSplitters = make(map[string]struct{}) + todoResolvers = make(map[string]struct{}) + todoDefaults = make(map[string]struct{}) + ) + + // At every step we'll need service defaults. + todoDefaults[serviceName] = struct{}{} + + // first fetch the router, of which we only collect 1 per chain eval + _, router, err := s.getRouterConfigEntryTxn(tx, ws, serviceName, overrides) + if err != nil { + return 0, nil, err + } else if router != nil { + res.Routers[serviceName] = router + } + + if router != nil { + for _, svc := range router.ListRelatedServices() { + todoSplitters[svc] = struct{}{} + } + } else { + // Next hop in the chain is the splitter. + todoSplitters[serviceName] = struct{}{} + } + + for { + name, ok := anyKey(todoSplitters) + if !ok { + break + } + delete(todoSplitters, name) + + if _, ok := res.Splitters[name]; ok { + continue // already fetched + } + + // Yes, even for splitters. + todoDefaults[name] = struct{}{} + + _, splitter, err := s.getSplitterConfigEntryTxn(tx, ws, name, overrides) + if err != nil { + return 0, nil, err + } + + if splitter == nil { + res.Splitters[name] = nil + + // Next hop in the chain is the resolver. + todoResolvers[name] = struct{}{} + continue + } + + res.Splitters[name] = splitter + + todoResolvers[name] = struct{}{} + for _, svc := range splitter.ListRelatedServices() { + // If there is no splitter, this will end up adding a resolver + // after another iteration. + todoSplitters[svc] = struct{}{} + } + } + + for { + name, ok := anyKey(todoResolvers) + if !ok { + break + } + delete(todoResolvers, name) + + if _, ok := res.Resolvers[name]; ok { + continue // already fetched + } + + // And resolvers, too. + todoDefaults[name] = struct{}{} + + _, resolver, err := s.getResolverConfigEntryTxn(tx, ws, name, overrides) + if err != nil { + return 0, nil, err + } + + if resolver == nil { + res.Resolvers[name] = nil + continue + } + + res.Resolvers[name] = resolver + + for _, svc := range resolver.ListRelatedServices() { + todoResolvers[svc] = struct{}{} + } + } + + for { + name, ok := anyKey(todoDefaults) + if !ok { + break + } + delete(todoDefaults, name) + + if _, ok := res.Services[name]; ok { + continue // already fetched + } + + _, entry, err := s.getServiceConfigEntryTxn(tx, ws, name, overrides) + if err != nil { + return 0, nil, err + } + + if entry == nil { + res.Services[name] = nil + continue + } + + res.Services[name] = entry + } + + // Strip nils now that they are no longer necessary. + for name, entry := range res.Routers { + if entry == nil { + delete(res.Routers, name) + } + } + for name, entry := range res.Splitters { + if entry == nil { + delete(res.Splitters, name) + } + } + for name, entry := range res.Resolvers { + if entry == nil { + delete(res.Resolvers, name) + } + } + for name, entry := range res.Services { + if entry == nil { + delete(res.Services, name) + } + } + + return idx, res, nil +} + +// anyKey returns any key from the provided map if any exist. Useful for using +// a map as a simple work queue of sorts. +func anyKey(m map[string]struct{}) (string, bool) { + if len(m) == 0 { + return "", false + } + for k, _ := range m { + return k, true + } + return "", false +} + +// getServiceConfigEntryTxn is a convenience method for fetching a +// service-defaults kind of config entry. +// +// If an override is returned the index returned will be 0. +func (s *Store) getServiceConfigEntryTxn( + tx *memdb.Txn, + ws memdb.WatchSet, + serviceName string, + overrides map[structs.ConfigEntryKindName]structs.ConfigEntry, +) (uint64, *structs.ServiceConfigEntry, error) { + idx, entry, err := s.configEntryWithOverridesTxn(tx, ws, structs.ServiceDefaults, serviceName, overrides) + if err != nil { + return 0, nil, err + } else if entry == nil { + return idx, nil, nil + } + + service, ok := entry.(*structs.ServiceConfigEntry) + if !ok { + return 0, nil, fmt.Errorf("invalid service config type %T", entry) + } + return idx, service, nil +} + +// getRouterConfigEntryTxn is a convenience method for fetching a +// service-router kind of config entry. +// +// If an override is returned the index returned will be 0. +func (s *Store) getRouterConfigEntryTxn( + tx *memdb.Txn, + ws memdb.WatchSet, + serviceName string, + overrides map[structs.ConfigEntryKindName]structs.ConfigEntry, +) (uint64, *structs.ServiceRouterConfigEntry, error) { + idx, entry, err := s.configEntryWithOverridesTxn(tx, ws, structs.ServiceRouter, serviceName, overrides) + if err != nil { + return 0, nil, err + } else if entry == nil { + return idx, nil, nil + } + + router, ok := entry.(*structs.ServiceRouterConfigEntry) + if !ok { + return 0, nil, fmt.Errorf("invalid service config type %T", entry) + } + return idx, router, nil +} + +// getSplitterConfigEntryTxn is a convenience method for fetching a +// service-splitter kind of config entry. +// +// If an override is returned the index returned will be 0. +func (s *Store) getSplitterConfigEntryTxn( + tx *memdb.Txn, + ws memdb.WatchSet, + serviceName string, + overrides map[structs.ConfigEntryKindName]structs.ConfigEntry, +) (uint64, *structs.ServiceSplitterConfigEntry, error) { + idx, entry, err := s.configEntryWithOverridesTxn(tx, ws, structs.ServiceSplitter, serviceName, overrides) + if err != nil { + return 0, nil, err + } else if entry == nil { + return idx, nil, nil + } + + splitter, ok := entry.(*structs.ServiceSplitterConfigEntry) + if !ok { + return 0, nil, fmt.Errorf("invalid service config type %T", entry) + } + return idx, splitter, nil +} + +// getResolverConfigEntryTxn is a convenience method for fetching a +// service-resolver kind of config entry. +// +// If an override is returned the index returned will be 0. +func (s *Store) getResolverConfigEntryTxn( + tx *memdb.Txn, + ws memdb.WatchSet, + serviceName string, + overrides map[structs.ConfigEntryKindName]structs.ConfigEntry, +) (uint64, *structs.ServiceResolverConfigEntry, error) { + idx, entry, err := s.configEntryWithOverridesTxn(tx, ws, structs.ServiceResolver, serviceName, overrides) + if err != nil { + return 0, nil, err + } else if entry == nil { + return idx, nil, nil + } + + resolver, ok := entry.(*structs.ServiceResolverConfigEntry) + if !ok { + return 0, nil, fmt.Errorf("invalid service config type %T", entry) + } + return idx, resolver, nil +} + +func (s *Store) configEntryWithOverridesTxn( + tx *memdb.Txn, + ws memdb.WatchSet, + kind string, + name string, + overrides map[structs.ConfigEntryKindName]structs.ConfigEntry, +) (uint64, structs.ConfigEntry, error) { + if len(overrides) > 0 { + entry, ok := overrides[structs.ConfigEntryKindName{ + Kind: kind, Name: name, + }] + if ok { + return 0, entry, nil // a nil entry implies it should act like it is erased + } + } + + return s.configEntryTxn(tx, ws, kind, name) +} diff --git a/agent/consul/state/config_entry_discoverychain.go b/agent/consul/state/config_entry_discoverychain.go deleted file mode 100644 index ecd1bc4be1..0000000000 --- a/agent/consul/state/config_entry_discoverychain.go +++ /dev/null @@ -1,287 +0,0 @@ -package state - -import ( - "fmt" - - "github.com/hashicorp/consul/agent/structs" - memdb "github.com/hashicorp/go-memdb" -) - -// ReadDiscoveryChainConfigEntries will query for the full discovery chain for -// the provided service name. All relevant config entries will be recursively -// fetched and included in the result. -// -// Once returned, the caller still needs to assemble these into a useful graph -// structure. -func (s *Store) ReadDiscoveryChainConfigEntries( - ws memdb.WatchSet, - serviceName string, -) (uint64, *structs.DiscoveryChainConfigEntries, error) { - tx := s.db.Txn(false) - defer tx.Abort() - return s.readDiscoveryChainConfigEntriesTxn(tx, ws, serviceName) -} - -func allowDiscoveryChainL7Features(entry *structs.ServiceConfigEntry) bool { - if entry == nil { - return false // default is tcp - } - - return structs.EnableAdvancedRoutingForProtocol(entry.Protocol) -} - -func (s *Store) readDiscoveryChainConfigEntriesTxn( - tx *memdb.Txn, - ws memdb.WatchSet, - serviceName string, -) (uint64, *structs.DiscoveryChainConfigEntries, error) { - // TODO(rb): improve this so you can simulate changes to vet writes. - - res := &structs.DiscoveryChainConfigEntries{ - Routers: make(map[string]*structs.ServiceRouterConfigEntry), - Splitters: make(map[string]*structs.ServiceSplitterConfigEntry), - Resolvers: make(map[string]*structs.ServiceResolverConfigEntry), - Services: make(map[string]*structs.ServiceConfigEntry), - } - - // Note that below we always look up splitters and resolvers in pairs, even - // in some circumstances where both are not strictly necessary. - // - // For now we'll just eat the cost of fetching pairs of splitter/resolver - // config entries even though we may not always need both. In the common - // case we will need the pair so there's not a big drive to optimize this - // here at this time. - - // Both Splitters and Resolvers maps will contain placeholder nils until - // the end of this function to indicate "no such entry". - - var ( - idx uint64 - activateL7 = make(map[string]struct{}) - todoSplitters = make(map[string]struct{}) - todoResolvers = make(map[string]struct{}) - ) - - checkL7 := func(name string) (bool, error) { - if _, loaded := res.Services[name]; loaded { - _, ok := activateL7[name] - return ok, nil - } - - // first see if this is even a chain-aware protocol - thisIdx, entry, err := s.getServiceConfigEntryTxn(tx, ws, name) - if err != nil { - return false, err - } - - if idx == 0 { - idx = thisIdx - } - - res.Services[name] = entry // we'll strip the nil later - if allowDiscoveryChainL7Features(entry) { - activateL7[name] = struct{}{} - return true, nil - } - - return false, nil - } - - // first see if this is even a chain-aware protocol - if useL7, err := checkL7(serviceName); err != nil { - return 0, nil, err - - } else if useL7 { - // first fetch the router, of which we only collect 1 per chain eval - _, router, err := s.getRouterConfigEntryTxn(tx, ws, serviceName) - if err != nil { - return 0, nil, err - } else if router != nil { - res.Routers[serviceName] = router - } - - if router != nil { - for _, svc := range router.ListRelatedServices() { - todoSplitters[svc] = struct{}{} - } - } else { - // Next hop in the chain is the splitter. - todoSplitters[serviceName] = struct{}{} - } - - } else { - // Next hop in the chain is the resolver. - res.Splitters[serviceName] = nil - todoResolvers[serviceName] = struct{}{} - } - - for { - name, ok := anyKey(todoSplitters) - if !ok { - break - } - delete(todoSplitters, name) - - if _, ok := res.Splitters[name]; ok { - continue // already fetched - } - - var splitter *structs.ServiceSplitterConfigEntry - if useL7, err := checkL7(name); err != nil { - return 0, nil, err - - } else if useL7 { - _, splitter, err = s.getSplitterConfigEntryTxn(tx, ws, name) - if err != nil { - return 0, nil, err - } - - } else { - splitter = nil // sorry - } - - if splitter == nil { - res.Splitters[name] = nil - - // Next hop in the chain is the resolver. - todoResolvers[name] = struct{}{} - continue - } - - if len(splitter.Splits) == 0 { - return 0, nil, fmt.Errorf("found splitter config for %q that has no splits", name) - } - - res.Splitters[name] = splitter - - todoResolvers[name] = struct{}{} - for _, svc := range splitter.ListRelatedServices() { - // If there is no splitter, this will end up adding a resolver - // after another iteration. - todoSplitters[svc] = struct{}{} - } - } - - for { - name, ok := anyKey(todoResolvers) - if !ok { - break - } - delete(todoResolvers, name) - - if _, ok := res.Resolvers[name]; ok { - continue // already fetched - } - - _, resolver, err := s.getResolverConfigEntryTxn(tx, ws, name) - if err != nil { - return 0, nil, err - } - - if resolver == nil { - res.Resolvers[name] = nil - continue - } - - if len(resolver.Failover) > 0 { - for subset, failoverClause := range resolver.Failover { - if failoverClause.Service == "" && - failoverClause.ServiceSubset == "" && - failoverClause.Namespace == "" && - len(failoverClause.Datacenters) == 0 { - return 0, nil, fmt.Errorf("failover section for subset %q is errantly empty", subset) - } - } - } - - res.Resolvers[name] = resolver - - for _, svc := range resolver.ListRelatedServices() { - todoResolvers[svc] = struct{}{} - } - } - - res.Fixup() - - return idx, res, nil -} - -// anyKey returns any key from the provided map if any exist. Useful for using -// a map as a simple work queue of sorts. -func anyKey(m map[string]struct{}) (string, bool) { - if len(m) == 0 { - return "", false - } - for k, _ := range m { - return k, true - } - return "", false -} - -// getServiceConfigEntryTxn is a convenience method for fetching a -// service-defaults kind of config entry. -func (s *Store) getServiceConfigEntryTxn(tx *memdb.Txn, ws memdb.WatchSet, serviceName string) (uint64, *structs.ServiceConfigEntry, error) { - idx, entry, err := s.configEntryTxn(tx, ws, structs.ServiceDefaults, serviceName) - if err != nil { - return 0, nil, err - } else if entry == nil { - return idx, nil, nil - } - - service, ok := entry.(*structs.ServiceConfigEntry) - if !ok { - return 0, nil, fmt.Errorf("invalid service config type %T", entry) - } - return idx, service, nil -} - -// getRouterConfigEntryTxn is a convenience method for fetching a -// service-router kind of config entry. -func (s *Store) getRouterConfigEntryTxn(tx *memdb.Txn, ws memdb.WatchSet, serviceName string) (uint64, *structs.ServiceRouterConfigEntry, error) { - idx, entry, err := s.configEntryTxn(tx, ws, structs.ServiceRouter, serviceName) - if err != nil { - return 0, nil, err - } else if entry == nil { - return idx, nil, nil - } - - router, ok := entry.(*structs.ServiceRouterConfigEntry) - if !ok { - return 0, nil, fmt.Errorf("invalid service config type %T", entry) - } - return idx, router, nil -} - -// getSplitterConfigEntryTxn is a convenience method for fetching a -// service-splitter kind of config entry. -func (s *Store) getSplitterConfigEntryTxn(tx *memdb.Txn, ws memdb.WatchSet, serviceName string) (uint64, *structs.ServiceSplitterConfigEntry, error) { - idx, entry, err := s.configEntryTxn(tx, ws, structs.ServiceSplitter, serviceName) - if err != nil { - return 0, nil, err - } else if entry == nil { - return idx, nil, nil - } - - splitter, ok := entry.(*structs.ServiceSplitterConfigEntry) - if !ok { - return 0, nil, fmt.Errorf("invalid service config type %T", entry) - } - return idx, splitter, nil -} - -// getResolverConfigEntryTxn is a convenience method for fetching a -// service-resolver kind of config entry. -func (s *Store) getResolverConfigEntryTxn(tx *memdb.Txn, ws memdb.WatchSet, serviceName string) (uint64, *structs.ServiceResolverConfigEntry, error) { - idx, entry, err := s.configEntryTxn(tx, ws, structs.ServiceResolver, serviceName) - if err != nil { - return 0, nil, err - } else if entry == nil { - return idx, nil, nil - } - - resolver, ok := entry.(*structs.ServiceResolverConfigEntry) - if !ok { - return 0, nil, fmt.Errorf("invalid service config type %T", entry) - } - return idx, resolver, nil -} diff --git a/agent/consul/state/config_entry_test.go b/agent/consul/state/config_entry_test.go index 7824c8152c..038aa90e8d 100644 --- a/agent/consul/state/config_entry_test.go +++ b/agent/consul/state/config_entry_test.go @@ -2,6 +2,7 @@ package state import ( "testing" + "time" "github.com/hashicorp/consul/agent/structs" memdb "github.com/hashicorp/go-memdb" @@ -174,3 +175,825 @@ func TestStore_ConfigEntries(t *testing.T) { })) require.True(watchFired(ws)) } + +func TestStore_ConfigEntry_GraphValidation(t *testing.T) { + for _, tc := range []struct { + name string + entries []structs.ConfigEntry + op func(t *testing.T, s *Store) error + expectErr string + expectGraphErr bool + }{ + { + name: "splitter fails without default protocol", + entries: []structs.ConfigEntry{}, + op: func(t *testing.T, s *Store) error { + entry := &structs.ServiceSplitterConfigEntry{ + Kind: structs.ServiceSplitter, + Name: "main", + Splits: []structs.ServiceSplit{ + {Weight: 90, Namespace: "v1"}, + {Weight: 10, Namespace: "v2"}, + }, + } + return s.EnsureConfigEntry(0, entry) + }, + expectErr: "does not permit advanced routing or splitting behavior", + expectGraphErr: true, + }, + { + name: "splitter fails with tcp protocol", + entries: []structs.ConfigEntry{ + &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "main", + Protocol: "tcp", + }, + }, + op: func(t *testing.T, s *Store) error { + entry := &structs.ServiceSplitterConfigEntry{ + Kind: structs.ServiceSplitter, + Name: "main", + Splits: []structs.ServiceSplit{ + {Weight: 90, Namespace: "v1"}, + {Weight: 10, Namespace: "v2"}, + }, + } + return s.EnsureConfigEntry(0, entry) + }, + expectErr: "does not permit advanced routing or splitting behavior", + expectGraphErr: true, + }, + { + name: "router fails with tcp protocol", + entries: []structs.ConfigEntry{ + &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "main", + Protocol: "tcp", + }, + }, + op: func(t *testing.T, s *Store) error { + entry := &structs.ServiceRouterConfigEntry{ + Kind: structs.ServiceRouter, + Name: "main", + Routes: []structs.ServiceRoute{ + { + Match: &structs.ServiceRouteMatch{ + HTTP: &structs.ServiceRouteHTTPMatch{ + PathExact: "/other", + }, + }, + Destination: &structs.ServiceRouteDestination{ + Namespace: "other", + }, + }, + }, + } + return s.EnsureConfigEntry(0, entry) + }, + expectErr: "does not permit advanced routing or splitting behavior", + expectGraphErr: true, + }, + { + name: "router fails without default protocol", + entries: []structs.ConfigEntry{}, + op: func(t *testing.T, s *Store) error { + entry := &structs.ServiceRouterConfigEntry{ + Kind: structs.ServiceRouter, + Name: "main", + Routes: []structs.ServiceRoute{ + { + Match: &structs.ServiceRouteMatch{ + HTTP: &structs.ServiceRouteHTTPMatch{ + PathExact: "/other", + }, + }, + Destination: &structs.ServiceRouteDestination{ + Namespace: "other", + }, + }, + }, + } + return s.EnsureConfigEntry(0, entry) + }, + expectErr: "does not permit advanced routing or splitting behavior", + expectGraphErr: true, + }, + ///////////////////////////////////////////////// + { + name: "cannot remove default protocol after splitter created", + entries: []structs.ConfigEntry{ + &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "main", + Protocol: "http", + }, + &structs.ServiceSplitterConfigEntry{ + Kind: structs.ServiceSplitter, + Name: "main", + Splits: []structs.ServiceSplit{ + {Weight: 90, Namespace: "v1"}, + {Weight: 10, Namespace: "v2"}, + }, + }, + }, + op: func(t *testing.T, s *Store) error { + return s.DeleteConfigEntry(0, structs.ServiceDefaults, "main") + }, + expectErr: "does not permit advanced routing or splitting behavior", + expectGraphErr: true, + }, + { + name: "cannot change to tcp protocol after splitter created", + entries: []structs.ConfigEntry{ + &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "main", + Protocol: "http", + }, + &structs.ServiceSplitterConfigEntry{ + Kind: structs.ServiceSplitter, + Name: "main", + Splits: []structs.ServiceSplit{ + {Weight: 90, Namespace: "v1"}, + {Weight: 10, Namespace: "v2"}, + }, + }, + }, + op: func(t *testing.T, s *Store) error { + entry := &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "main", + Protocol: "tcp", + } + return s.EnsureConfigEntry(0, entry) + }, + expectErr: "does not permit advanced routing or splitting behavior", + expectGraphErr: true, + }, + { + name: "cannot remove default protocol after router created", + entries: []structs.ConfigEntry{ + &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "main", + Protocol: "http", + }, + &structs.ServiceRouterConfigEntry{ + Kind: structs.ServiceRouter, + Name: "main", + Routes: []structs.ServiceRoute{ + { + Match: &structs.ServiceRouteMatch{ + HTTP: &structs.ServiceRouteHTTPMatch{ + PathExact: "/other", + }, + }, + Destination: &structs.ServiceRouteDestination{ + Namespace: "other", + }, + }, + }, + }, + }, + op: func(t *testing.T, s *Store) error { + return s.DeleteConfigEntry(0, structs.ServiceDefaults, "main") + }, + expectErr: "does not permit advanced routing or splitting behavior", + expectGraphErr: true, + }, + { + name: "cannot change to tcp protocol after router created", + entries: []structs.ConfigEntry{ + &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "main", + Protocol: "http", + }, + &structs.ServiceRouterConfigEntry{ + Kind: structs.ServiceRouter, + Name: "main", + Routes: []structs.ServiceRoute{ + { + Match: &structs.ServiceRouteMatch{ + HTTP: &structs.ServiceRouteHTTPMatch{ + PathExact: "/other", + }, + }, + Destination: &structs.ServiceRouteDestination{ + Namespace: "other", + }, + }, + }, + }, + }, + op: func(t *testing.T, s *Store) error { + entry := &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "main", + Protocol: "tcp", + } + return s.EnsureConfigEntry(0, entry) + }, + expectErr: "does not permit advanced routing or splitting behavior", + expectGraphErr: true, + }, + ///////////////////////////////////////////////// + { + name: "cannot split to a service using tcp", + entries: []structs.ConfigEntry{ + &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "main", + Protocol: "http", + }, + &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "other", + Protocol: "tcp", + }, + }, + op: func(t *testing.T, s *Store) error { + entry := &structs.ServiceSplitterConfigEntry{ + Kind: structs.ServiceSplitter, + Name: "main", + Splits: []structs.ServiceSplit{ + {Weight: 90}, + {Weight: 10, Service: "other"}, + }, + } + return s.EnsureConfigEntry(0, entry) + }, + expectErr: "uses inconsistent protocols", + expectGraphErr: true, + }, + { + name: "cannot route to a service using tcp", + entries: []structs.ConfigEntry{ + &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "main", + Protocol: "http", + }, + &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "other", + Protocol: "tcp", + }, + }, + op: func(t *testing.T, s *Store) error { + entry := &structs.ServiceRouterConfigEntry{ + Kind: structs.ServiceRouter, + Name: "main", + Routes: []structs.ServiceRoute{ + { + Match: &structs.ServiceRouteMatch{ + HTTP: &structs.ServiceRouteHTTPMatch{ + PathExact: "/other", + }, + }, + Destination: &structs.ServiceRouteDestination{ + Service: "other", + }, + }, + }, + } + return s.EnsureConfigEntry(0, entry) + }, + expectErr: "uses inconsistent protocols", + expectGraphErr: true, + }, + ///////////////////////////////////////////////// + { + name: "cannot failover to a service using a different protocol", + entries: []structs.ConfigEntry{ + &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "main", + Protocol: "grpc", + }, + &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "other", + Protocol: "tcp", + }, + &structs.ServiceResolverConfigEntry{ + Kind: structs.ServiceResolver, + Name: "main", + ConnectTimeout: 33 * time.Second, + }, + }, + op: func(t *testing.T, s *Store) error { + entry := &structs.ServiceResolverConfigEntry{ + Kind: structs.ServiceResolver, + Name: "main", + Failover: map[string]structs.ServiceResolverFailover{ + "*": structs.ServiceResolverFailover{ + Service: "other", + }, + }, + } + return s.EnsureConfigEntry(0, entry) + }, + expectErr: "uses inconsistent protocols", + expectGraphErr: true, + }, + { + name: "cannot redirect to a service using a different protocol", + entries: []structs.ConfigEntry{ + &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "main", + Protocol: "grpc", + }, + &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "other", + Protocol: "tcp", + }, + &structs.ServiceResolverConfigEntry{ + Kind: structs.ServiceResolver, + Name: "main", + ConnectTimeout: 33 * time.Second, + }, + }, + op: func(t *testing.T, s *Store) error { + entry := &structs.ServiceResolverConfigEntry{ + Kind: structs.ServiceResolver, + Name: "main", + Redirect: &structs.ServiceResolverRedirect{ + Service: "other", + }, + } + return s.EnsureConfigEntry(0, entry) + }, + expectErr: "uses inconsistent protocols", + expectGraphErr: true, + }, + ///////////////////////////////////////////////// + { + name: "redirect to a subset that does exist is fine", + entries: []structs.ConfigEntry{ + &structs.ServiceResolverConfigEntry{ + Kind: structs.ServiceResolver, + Name: "other", + ConnectTimeout: 33 * time.Second, + Subsets: map[string]structs.ServiceResolverSubset{ + "v1": structs.ServiceResolverSubset{ + Filter: "Service.Meta.version == v1", + }, + }, + }, + }, + op: func(t *testing.T, s *Store) error { + entry := &structs.ServiceResolverConfigEntry{ + Kind: structs.ServiceResolver, + Name: "main", + Redirect: &structs.ServiceResolverRedirect{ + Service: "other", + ServiceSubset: "v1", + }, + } + return s.EnsureConfigEntry(0, entry) + }, + }, + { + name: "cannot redirect to a subset that does not exist", + entries: []structs.ConfigEntry{ + &structs.ServiceResolverConfigEntry{ + Kind: structs.ServiceResolver, + Name: "other", + ConnectTimeout: 33 * time.Second, + }, + }, + op: func(t *testing.T, s *Store) error { + entry := &structs.ServiceResolverConfigEntry{ + Kind: structs.ServiceResolver, + Name: "main", + Redirect: &structs.ServiceResolverRedirect{ + Service: "other", + ServiceSubset: "v1", + }, + } + return s.EnsureConfigEntry(0, entry) + }, + expectErr: `does not have a subset named "v1"`, + expectGraphErr: true, + }, + } { + tc := tc + + t.Run(tc.name, func(t *testing.T) { + s := testStateStore(t) + for _, entry := range tc.entries { + require.NoError(t, s.EnsureConfigEntry(0, entry)) + } + + err := tc.op(t, s) + if tc.expectErr != "" { + require.Error(t, err) + require.Contains(t, err.Error(), tc.expectErr) + _, ok := err.(*structs.ConfigEntryGraphError) + if tc.expectGraphErr { + require.True(t, ok, "%T is not a *ConfigEntryGraphError", err) + } else { + require.False(t, ok, "did not expect a *ConfigEntryGraphError here: %v", err) + } + } else { + require.NoError(t, err) + } + }) + } +} + +func TestStore_ReadDiscoveryChainConfigEntries_Overrides(t *testing.T) { + for _, tc := range []struct { + name string + entries []structs.ConfigEntry + expectBefore []structs.ConfigEntryKindName + overrides map[structs.ConfigEntryKindName]structs.ConfigEntry + expectAfter []structs.ConfigEntryKindName + expectAfterErr string + checkAfter func(t *testing.T, entrySet *structs.DiscoveryChainConfigEntries) + }{ + { + name: "mask service-defaults", + entries: []structs.ConfigEntry{ + &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "main", + Protocol: "tcp", + }, + }, + expectBefore: []structs.ConfigEntryKindName{ + {Kind: structs.ServiceDefaults, Name: "main"}, + }, + overrides: map[structs.ConfigEntryKindName]structs.ConfigEntry{ + {Kind: structs.ServiceDefaults, Name: "main"}: nil, + }, + expectAfter: []structs.ConfigEntryKindName{ + // nothing + }, + }, + { + name: "edit service-defaults", + entries: []structs.ConfigEntry{ + &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "main", + Protocol: "tcp", + }, + }, + expectBefore: []structs.ConfigEntryKindName{ + {Kind: structs.ServiceDefaults, Name: "main"}, + }, + overrides: map[structs.ConfigEntryKindName]structs.ConfigEntry{ + {Kind: structs.ServiceDefaults, Name: "main"}: &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "main", + Protocol: "grpc", + }, + }, + expectAfter: []structs.ConfigEntryKindName{ + {Kind: structs.ServiceDefaults, Name: "main"}, + }, + checkAfter: func(t *testing.T, entrySet *structs.DiscoveryChainConfigEntries) { + defaults := entrySet.GetService("main") + require.NotNil(t, defaults) + require.Equal(t, "grpc", defaults.Protocol) + }, + }, + + { + name: "mask service-router", + entries: []structs.ConfigEntry{ + &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "main", + Protocol: "http", + }, + &structs.ServiceRouterConfigEntry{ + Kind: structs.ServiceRouter, + Name: "main", + }, + }, + expectBefore: []structs.ConfigEntryKindName{ + {Kind: structs.ServiceDefaults, Name: "main"}, + {Kind: structs.ServiceRouter, Name: "main"}, + }, + overrides: map[structs.ConfigEntryKindName]structs.ConfigEntry{ + {Kind: structs.ServiceRouter, Name: "main"}: nil, + }, + expectAfter: []structs.ConfigEntryKindName{ + {Kind: structs.ServiceDefaults, Name: "main"}, + }, + }, + { + name: "edit service-router", + entries: []structs.ConfigEntry{ + &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "main", + Protocol: "http", + }, + &structs.ServiceResolverConfigEntry{ + Kind: structs.ServiceResolver, + Name: "main", + Subsets: map[string]structs.ServiceResolverSubset{ + "v1": {Filter: "Service.Meta.version == v1"}, + "v2": {Filter: "Service.Meta.version == v2"}, + "v3": {Filter: "Service.Meta.version == v3"}, + }, + }, + &structs.ServiceRouterConfigEntry{ + Kind: structs.ServiceRouter, + Name: "main", + Routes: []structs.ServiceRoute{ + { + Match: &structs.ServiceRouteMatch{ + HTTP: &structs.ServiceRouteHTTPMatch{ + PathExact: "/admin", + }, + }, + Destination: &structs.ServiceRouteDestination{ + ServiceSubset: "v2", + }, + }, + }, + }, + }, + expectBefore: []structs.ConfigEntryKindName{ + {Kind: structs.ServiceDefaults, Name: "main"}, + {Kind: structs.ServiceResolver, Name: "main"}, + {Kind: structs.ServiceRouter, Name: "main"}, + }, + overrides: map[structs.ConfigEntryKindName]structs.ConfigEntry{ + {Kind: structs.ServiceRouter, Name: "main"}: &structs.ServiceRouterConfigEntry{ + Kind: structs.ServiceRouter, + Name: "main", + Routes: []structs.ServiceRoute{ + { + Match: &structs.ServiceRouteMatch{ + HTTP: &structs.ServiceRouteHTTPMatch{ + PathExact: "/admin", + }, + }, + Destination: &structs.ServiceRouteDestination{ + ServiceSubset: "v3", + }, + }, + }, + }, + }, + expectAfter: []structs.ConfigEntryKindName{ + {Kind: structs.ServiceDefaults, Name: "main"}, + {Kind: structs.ServiceResolver, Name: "main"}, + {Kind: structs.ServiceRouter, Name: "main"}, + }, + checkAfter: func(t *testing.T, entrySet *structs.DiscoveryChainConfigEntries) { + router := entrySet.GetRouter("main") + require.NotNil(t, router) + require.Len(t, router.Routes, 1) + + expect := structs.ServiceRoute{ + Match: &structs.ServiceRouteMatch{ + HTTP: &structs.ServiceRouteHTTPMatch{ + PathExact: "/admin", + }, + }, + Destination: &structs.ServiceRouteDestination{ + ServiceSubset: "v3", + }, + } + require.Equal(t, expect, router.Routes[0]) + }, + }, + + { + name: "mask service-splitter", + entries: []structs.ConfigEntry{ + &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "main", + Protocol: "http", + }, + &structs.ServiceSplitterConfigEntry{ + Kind: structs.ServiceSplitter, + Name: "main", + Splits: []structs.ServiceSplit{ + {Weight: 100}, + }, + }, + }, + expectBefore: []structs.ConfigEntryKindName{ + {Kind: structs.ServiceDefaults, Name: "main"}, + {Kind: structs.ServiceSplitter, Name: "main"}, + }, + overrides: map[structs.ConfigEntryKindName]structs.ConfigEntry{ + {Kind: structs.ServiceSplitter, Name: "main"}: nil, + }, + expectAfter: []structs.ConfigEntryKindName{ + {Kind: structs.ServiceDefaults, Name: "main"}, + }, + }, + { + name: "edit service-splitter", + entries: []structs.ConfigEntry{ + &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "main", + Protocol: "http", + }, + &structs.ServiceSplitterConfigEntry{ + Kind: structs.ServiceSplitter, + Name: "main", + Splits: []structs.ServiceSplit{ + {Weight: 100}, + }, + }, + }, + expectBefore: []structs.ConfigEntryKindName{ + {Kind: structs.ServiceDefaults, Name: "main"}, + {Kind: structs.ServiceSplitter, Name: "main"}, + }, + overrides: map[structs.ConfigEntryKindName]structs.ConfigEntry{ + {Kind: structs.ServiceSplitter, Name: "main"}: &structs.ServiceSplitterConfigEntry{ + Kind: structs.ServiceSplitter, + Name: "main", + Splits: []structs.ServiceSplit{ + {Weight: 85, ServiceSubset: "v1"}, + {Weight: 15, ServiceSubset: "v2"}, + }, + }, + }, + expectAfter: []structs.ConfigEntryKindName{ + {Kind: structs.ServiceDefaults, Name: "main"}, + {Kind: structs.ServiceSplitter, Name: "main"}, + }, + checkAfter: func(t *testing.T, entrySet *structs.DiscoveryChainConfigEntries) { + splitter := entrySet.GetSplitter("main") + require.NotNil(t, splitter) + require.Len(t, splitter.Splits, 2) + + expect := []structs.ServiceSplit{ + {Weight: 85, ServiceSubset: "v1"}, + {Weight: 15, ServiceSubset: "v2"}, + } + require.Equal(t, expect, splitter.Splits) + }, + }, + + { + name: "mask service-resolver", + entries: []structs.ConfigEntry{ + &structs.ServiceResolverConfigEntry{ + Kind: structs.ServiceResolver, + Name: "main", + }, + }, + expectBefore: []structs.ConfigEntryKindName{ + {Kind: structs.ServiceResolver, Name: "main"}, + }, + overrides: map[structs.ConfigEntryKindName]structs.ConfigEntry{ + {Kind: structs.ServiceResolver, Name: "main"}: nil, + }, + expectAfter: []structs.ConfigEntryKindName{ + // nothing + }, + }, + { + name: "edit service-resolver", + entries: []structs.ConfigEntry{ + &structs.ServiceResolverConfigEntry{ + Kind: structs.ServiceResolver, + Name: "main", + }, + }, + expectBefore: []structs.ConfigEntryKindName{ + {Kind: structs.ServiceResolver, Name: "main"}, + }, + overrides: map[structs.ConfigEntryKindName]structs.ConfigEntry{ + {Kind: structs.ServiceResolver, Name: "main"}: &structs.ServiceResolverConfigEntry{ + Kind: structs.ServiceResolver, + Name: "main", + ConnectTimeout: 33 * time.Second, + }, + }, + expectAfter: []structs.ConfigEntryKindName{ + {Kind: structs.ServiceResolver, Name: "main"}, + }, + checkAfter: func(t *testing.T, entrySet *structs.DiscoveryChainConfigEntries) { + resolver := entrySet.GetResolver("main") + require.NotNil(t, resolver) + require.Equal(t, 33*time.Second, resolver.ConnectTimeout) + }, + }, + } { + tc := tc + + t.Run(tc.name, func(t *testing.T) { + s := testStateStore(t) + for _, entry := range tc.entries { + require.NoError(t, s.EnsureConfigEntry(0, entry)) + } + + t.Run("without override", func(t *testing.T) { + _, entrySet, err := s.readDiscoveryChainConfigEntries(nil, "main", nil) + require.NoError(t, err) + got := entrySetToKindNames(entrySet) + require.ElementsMatch(t, tc.expectBefore, got) + }) + + t.Run("with override", func(t *testing.T) { + _, entrySet, err := s.readDiscoveryChainConfigEntries(nil, "main", tc.overrides) + + if tc.expectAfterErr != "" { + require.Error(t, err) + require.Contains(t, err.Error(), tc.expectAfterErr) + } else { + require.NoError(t, err) + got := entrySetToKindNames(entrySet) + require.ElementsMatch(t, tc.expectAfter, got) + + if tc.checkAfter != nil { + tc.checkAfter(t, entrySet) + } + } + }) + }) + } +} + +func entrySetToKindNames(entrySet *structs.DiscoveryChainConfigEntries) []structs.ConfigEntryKindName { + var out []structs.ConfigEntryKindName + for _, entry := range entrySet.Routers { + out = append(out, structs.ConfigEntryKindName{ + Kind: entry.Kind, + Name: entry.Name, + }) + } + for _, entry := range entrySet.Splitters { + out = append(out, structs.ConfigEntryKindName{ + Kind: entry.Kind, + Name: entry.Name, + }) + } + for _, entry := range entrySet.Resolvers { + out = append(out, structs.ConfigEntryKindName{ + Kind: entry.Kind, + Name: entry.Name, + }) + } + for _, entry := range entrySet.Services { + out = append(out, structs.ConfigEntryKindName{ + Kind: entry.Kind, + Name: entry.Name, + }) + } + return out +} + +func TestStore_ReadDiscoveryChainConfigEntries_SubsetSplit(t *testing.T) { + s := testStateStore(t) + + entries := []structs.ConfigEntry{ + &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "main", + Protocol: "http", + }, + &structs.ServiceResolverConfigEntry{ + Kind: structs.ServiceResolver, + Name: "main", + Subsets: map[string]structs.ServiceResolverSubset{ + "v1": structs.ServiceResolverSubset{ + Filter: "Service.Meta.version == v1", + }, + "v2": structs.ServiceResolverSubset{ + Filter: "Service.Meta.version == v2", + }, + }, + }, + &structs.ServiceSplitterConfigEntry{ + Kind: structs.ServiceSplitter, + Name: "main", + Splits: []structs.ServiceSplit{ + {Weight: 90, ServiceSubset: "v1"}, + {Weight: 10, ServiceSubset: "v2"}, + }, + }, + } + + for _, entry := range entries { + require.NoError(t, s.EnsureConfigEntry(0, entry)) + } + + _, entrySet, err := s.ReadDiscoveryChainConfigEntries(nil, "main") + require.NoError(t, err) + + require.Len(t, entrySet.Routers, 0) + require.Len(t, entrySet.Splitters, 1) + require.Len(t, entrySet.Resolvers, 1) + require.Len(t, entrySet.Services, 1) +} diff --git a/agent/structs/config_entry.go b/agent/structs/config_entry.go index 6607d37bd3..9d523f01e3 100644 --- a/agent/structs/config_entry.go +++ b/agent/structs/config_entry.go @@ -542,3 +542,12 @@ func (c *ConfigEntryResponse) UnmarshalBinary(data []byte) error { return nil } + +// ConfigEntryKindName is a value type useful for maps. You can use: +// map[ConfigEntryKindName]Payload +// instead of: +// map[string]map[string]Payload +type ConfigEntryKindName struct { + Kind string + Name string +} diff --git a/agent/structs/config_entry_discoverychain.go b/agent/structs/config_entry_discoverychain.go index c77dc5872f..0d7174aeb9 100644 --- a/agent/structs/config_entry_discoverychain.go +++ b/agent/structs/config_entry_discoverychain.go @@ -9,15 +9,6 @@ import ( "github.com/hashicorp/consul/acl" ) -func EnableAdvancedRoutingForProtocol(protocol string) bool { - switch protocol { - case "http", "http2", "grpc": - return true - default: - return false - } -} - // ServiceRouterConfigEntry defines L7 (e.g. http) routing rules for a named // service exposed in Connect. // @@ -60,6 +51,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? @@ -74,6 +67,8 @@ func (e *ServiceRouterConfigEntry) Validate() error { // 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 @@ -82,6 +77,69 @@ func (e *ServiceRouterConfigEntry) Validate() error { // 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) + } + + 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) + } + } + + 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 { + return fmt.Errorf("Route[%d] cannot make use of PrefixRewrite without configuring either PathExact or PathPrefix", i) + } + } + } + return nil } @@ -136,6 +194,10 @@ type ServiceRouteMatch struct { // (gRPC, redis, etc) they can go here. } +func (m *ServiceRouteMatch) IsEmpty() bool { + return m.HTTP == nil || m.HTTP.IsEmpty() +} + // ServiceRouteHTTPMatch is a set of http-specific match criteria. type ServiceRouteHTTPMatch struct { PathExact string `json:",omitempty"` @@ -145,7 +207,17 @@ type ServiceRouteHTTPMatch struct { Header []ServiceRouteHTTPMatchHeader `json:",omitempty"` QueryParam []ServiceRouteHTTPMatchQueryParam `json:",omitempty"` - Methods []string `json:",omitempty"` + // TODO(rb): reenable Methods + // Methods []string `json:",omitempty"` +} + +func (m *ServiceRouteHTTPMatch) IsEmpty() bool { + return m.PathExact == "" && + m.PathPrefix == "" && + m.PathRegex == "" && + len(m.Header) == 0 && + len(m.QueryParam) == 0 + // && len(m.Methods) == 0 } type ServiceRouteHTTPMatchHeader struct { @@ -255,6 +327,7 @@ func (e *ServiceSplitterConfigEntry) Normalize() error { if e == nil { return fmt.Errorf("config entry is nil") } + // TODO(rb): trim spaces e.Kind = ServiceSplitter @@ -451,6 +524,17 @@ type ServiceResolverConfigEntry struct { RaftIndex } +func (e *ServiceResolverConfigEntry) SubsetExists(name string) bool { + if name == "" { + return true + } + if len(e.Subsets) == 0 { + return false + } + _, ok := e.Subsets[name] + return ok +} + func (e *ServiceResolverConfigEntry) IsDefault() bool { return e.DefaultSubset == "" && len(e.Subsets) == 0 && @@ -475,6 +559,7 @@ func (e *ServiceResolverConfigEntry) Normalize() error { if e == nil { return fmt.Errorf("config entry is nil") } + // TODO(rb): trim spaces e.Kind = ServiceResolver @@ -511,6 +596,12 @@ func (e *ServiceResolverConfigEntry) Validate() error { if e.Redirect != nil { r := e.Redirect + if len(e.Failover) > 0 { + return fmt.Errorf("Redirect and Failover cannot both be set") + } + + // TODO(rb): prevent subsets and default subsets from being defined? + if r.Service == "" && r.ServiceSubset == "" && r.Namespace == "" && r.Datacenter == "" { return fmt.Errorf("Redirect is empty") } @@ -538,8 +629,8 @@ func (e *ServiceResolverConfigEntry) Validate() error { return fmt.Errorf("Bad Failover[%q]: not a valid subset", subset) } - if f.Service == "" && f.ServiceSubset == "" && len(f.Datacenters) == 0 { - return fmt.Errorf("Bad Failover[%q] one of Service, ServiceSubset, or Datacenters is required", subset) + if f.Service == "" && f.ServiceSubset == "" && f.Namespace == "" && len(f.Datacenters) == 0 { + return fmt.Errorf("Bad Failover[%q] one of Service, ServiceSubset, Namespace, or Datacenters is required", subset) } if f.ServiceSubset != "" { @@ -750,64 +841,6 @@ type DiscoveryChainConfigEntries struct { Services map[string]*ServiceConfigEntry } -// Fixup ensures that the collection of entries make sense together. Nil maps -// are initialized, nil values are deleted, and advanced features are disabled -// if protocol dictates. -func (e *DiscoveryChainConfigEntries) Fixup() { - if e.Services == nil { - e.Services = make(map[string]*ServiceConfigEntry) - } - if e.Routers == nil { - e.Routers = make(map[string]*ServiceRouterConfigEntry) - } - if e.Splitters == nil { - e.Splitters = make(map[string]*ServiceSplitterConfigEntry) - } - if e.Resolvers == nil { - e.Resolvers = make(map[string]*ServiceResolverConfigEntry) - } - - for name, entry := range e.Routers { - if entry == nil { - delete(e.Routers, name) - } else { - if !e.allowAdvancedRouting(name) { - delete(e.Routers, name) - } - } - } - for name, entry := range e.Splitters { - if entry == nil { - delete(e.Splitters, name) - } else { - if !e.allowAdvancedRouting(name) { - delete(e.Splitters, name) - } - } - } - for name, entry := range e.Resolvers { - if entry == nil { - delete(e.Resolvers, name) - } - } - for name, entry := range e.Services { - if entry == nil { - delete(e.Services, name) - } - } -} - -func (e *DiscoveryChainConfigEntries) allowAdvancedRouting(name string) bool { - if e.Services == nil { - return false - } - entry, ok := e.Services[name] - if !ok || entry == nil { - return false - } - return EnableAdvancedRoutingForProtocol(entry.Protocol) -} - func (e *DiscoveryChainConfigEntries) GetRouter(name string) *ServiceRouterConfigEntry { if e.Routers != nil { return e.Routers[name] @@ -883,3 +916,16 @@ func (e *DiscoveryChainConfigEntries) IsEmpty() bool { func (e *DiscoveryChainConfigEntries) IsChainEmpty() bool { return len(e.Routers) == 0 && len(e.Splitters) == 0 && len(e.Resolvers) == 0 } + +type ConfigEntryGraphError struct { + // one of Message or Err should be set + Message string + Err error +} + +func (e *ConfigEntryGraphError) Error() string { + if e.Err != nil { + return e.Err.Error() + } + return e.Message +} diff --git a/agent/structs/config_entry_discoverychain_test.go b/agent/structs/config_entry_discoverychain_test.go index 9099fa10b3..f67f99aa62 100644 --- a/agent/structs/config_entry_discoverychain_test.go +++ b/agent/structs/config_entry_discoverychain_test.go @@ -53,7 +53,7 @@ func TestServiceResolverConfigEntry(t *testing.T) { Name: "test", DefaultSubset: "gone", Subsets: map[string]ServiceResolverSubset{ - "v1": {Filter: "ServiceMeta.version == v1"}, + "v1": {Filter: "Service.Meta.version == v1"}, }, }, validateErr: `DefaultSubset "gone" is not a valid subset`, @@ -65,7 +65,7 @@ func TestServiceResolverConfigEntry(t *testing.T) { Name: "test", DefaultSubset: "v1", Subsets: map[string]ServiceResolverSubset{ - "v1": {Filter: "ServiceMeta.version == v1"}, + "v1": {Filter: "Service.Meta.version == v1"}, }, }, }, @@ -122,7 +122,7 @@ func TestServiceResolverConfigEntry(t *testing.T) { ServiceSubset: "v1", }, Subsets: map[string]ServiceResolverSubset{ - "v1": {Filter: "ServiceMeta.version == v1"}, + "v1": {Filter: "Service.Meta.version == v1"}, }, }, }, @@ -157,7 +157,7 @@ func TestServiceResolverConfigEntry(t *testing.T) { Kind: ServiceResolver, Name: "test", Subsets: map[string]ServiceResolverSubset{ - "v1": {Filter: "ServiceMeta.version == v1"}, + "v1": {Filter: "Service.Meta.version == v1"}, }, Failover: map[string]ServiceResolverFailover{ "v1": ServiceResolverFailover{ @@ -172,13 +172,13 @@ func TestServiceResolverConfigEntry(t *testing.T) { Kind: ServiceResolver, Name: "test", Subsets: map[string]ServiceResolverSubset{ - "v1": {Filter: "ServiceMeta.version == v1"}, + "v1": {Filter: "Service.Meta.version == v1"}, }, Failover: map[string]ServiceResolverFailover{ "v1": ServiceResolverFailover{}, }, }, - validateErr: `Bad Failover["v1"] one of Service, ServiceSubset, or Datacenters is required`, + validateErr: `Bad Failover["v1"] one of Service, ServiceSubset, Namespace, or Datacenters is required`, }, { name: "failover to self using invalid subset", @@ -186,7 +186,7 @@ func TestServiceResolverConfigEntry(t *testing.T) { Kind: ServiceResolver, Name: "test", Subsets: map[string]ServiceResolverSubset{ - "v1": {Filter: "ServiceMeta.version == v1"}, + "v1": {Filter: "Service.Meta.version == v1"}, }, Failover: map[string]ServiceResolverFailover{ "v1": ServiceResolverFailover{ @@ -203,8 +203,8 @@ func TestServiceResolverConfigEntry(t *testing.T) { Kind: ServiceResolver, Name: "test", Subsets: map[string]ServiceResolverSubset{ - "v1": {Filter: "ServiceMeta.version == v1"}, - "v2": {Filter: "ServiceMeta.version == v2"}, + "v1": {Filter: "Service.Meta.version == v1"}, + "v2": {Filter: "Service.Meta.version == v2"}, }, Failover: map[string]ServiceResolverFailover{ "v1": ServiceResolverFailover{ diff --git a/api/config_entry_discoverychain.go b/api/config_entry_discoverychain.go index 18dcbca810..702b38b613 100644 --- a/api/config_entry_discoverychain.go +++ b/api/config_entry_discoverychain.go @@ -34,7 +34,8 @@ type ServiceRouteHTTPMatch struct { Header []ServiceRouteHTTPMatchHeader `json:",omitempty"` QueryParam []ServiceRouteHTTPMatchQueryParam `json:",omitempty"` - Methods []string `json:",omitempty"` + // TODO(rb): reenable Methods + // Methods []string `json:",omitempty"` } type ServiceRouteHTTPMatchHeader struct { diff --git a/api/config_entry_discoverychain_test.go b/api/config_entry_discoverychain_test.go index dcfaccefed..9667402eec 100644 --- a/api/config_entry_discoverychain_test.go +++ b/api/config_entry_discoverychain_test.go @@ -1,6 +1,7 @@ package api import ( + "fmt" "testing" "time" @@ -14,97 +15,48 @@ func TestAPI_ConfigEntry_DiscoveryChain(t *testing.T) { config_entries := c.ConfigEntries() - t.Run("Service Router", func(t *testing.T) { - // use one mega object to avoid multiple trips - makeEntry := func() *ServiceRouterConfigEntry { - return &ServiceRouterConfigEntry{ - Kind: ServiceRouter, - Name: "test", - Routes: []ServiceRoute{ - { - Match: &ServiceRouteMatch{ - HTTP: &ServiceRouteHTTPMatch{ - PathPrefix: "/prefix", - Header: []ServiceRouteHTTPMatchHeader{ - {Name: "x-debug", Exact: "1"}, - }, - QueryParam: []ServiceRouteHTTPMatchQueryParam{ - {Name: "debug", Value: "1"}, - }, - Methods: []string{"GET", "POST"}, - }, - }, - Destination: &ServiceRouteDestination{ - Service: "other", - ServiceSubset: "v2", - Namespace: "sec", - PrefixRewrite: "/", - RequestTimeout: 5 * time.Second, - NumRetries: 5, - RetryOnConnectFailure: true, - RetryOnStatusCodes: []uint32{500, 503, 401}, - }, - }, - }, - } - } + verifyResolver := func(t *testing.T, initial ConfigEntry) { + t.Helper() + require.IsType(t, &ServiceResolverConfigEntry{}, initial) + testEntry := initial.(*ServiceResolverConfigEntry) // set it - _, wm, err := config_entries.Set(makeEntry(), nil) + _, wm, err := config_entries.Set(testEntry, nil) require.NoError(t, err) require.NotNil(t, wm) require.NotEqual(t, 0, wm.RequestTime) // get it - entry, qm, err := config_entries.Get(ServiceRouter, "test", nil) + entry, qm, err := config_entries.Get(ServiceResolver, testEntry.Name, nil) require.NoError(t, err) require.NotNil(t, qm) require.NotEqual(t, 0, qm.RequestTime) // verify it - readRouter, ok := entry.(*ServiceRouterConfigEntry) + readResolver, ok := entry.(*ServiceResolverConfigEntry) require.True(t, ok) - readRouter.ModifyIndex = 0 // reset for Equals() - readRouter.CreateIndex = 0 // reset for Equals() + readResolver.ModifyIndex = 0 // reset for Equals() + readResolver.CreateIndex = 0 // reset for Equals() - goldenEntry := makeEntry() - require.Equal(t, goldenEntry, readRouter) + require.Equal(t, testEntry, readResolver) // TODO(rb): cas? // TODO(rb): list? - }) + } - t.Run("Service Splitter", func(t *testing.T) { - // use one mega object to avoid multiple trips - makeEntry := func() *ServiceSplitterConfigEntry { - return &ServiceSplitterConfigEntry{ - Kind: ServiceSplitter, - Name: "test", - Splits: []ServiceSplit{ - { - Weight: 90, - Service: "a", - ServiceSubset: "b", - Namespace: "c", - }, - { - Weight: 10, - Service: "x", - ServiceSubset: "y", - Namespace: "z", - }, - }, - } - } + verifySplitter := func(t *testing.T, initial ConfigEntry) { + t.Helper() + require.IsType(t, &ServiceSplitterConfigEntry{}, initial) + testEntry := initial.(*ServiceSplitterConfigEntry) // set it - _, wm, err := config_entries.Set(makeEntry(), nil) + _, wm, err := config_entries.Set(testEntry, nil) require.NoError(t, err) require.NotNil(t, wm) require.NotEqual(t, 0, wm.RequestTime) // get it - entry, qm, err := config_entries.Get(ServiceSplitter, "test", nil) + entry, qm, err := config_entries.Get(ServiceSplitter, testEntry.Name, nil) require.NoError(t, err) require.NotNil(t, qm) require.NotEqual(t, 0, qm.RequestTime) @@ -115,37 +67,76 @@ func TestAPI_ConfigEntry_DiscoveryChain(t *testing.T) { readSplitter.ModifyIndex = 0 // reset for Equals() readSplitter.CreateIndex = 0 // reset for Equals() - goldenEntry := makeEntry() - require.Equal(t, goldenEntry, readSplitter) + require.Equal(t, testEntry, readSplitter) // TODO(rb): cas? // TODO(rb): list? - }) + } - for name, tc := range map[string]func() *ServiceResolverConfigEntry{ - "with-redirect": func() *ServiceResolverConfigEntry { - return &ServiceResolverConfigEntry{ - Kind: ServiceResolver, - Name: "test", - Redirect: &ServiceResolverRedirect{ - Service: "a", - ServiceSubset: "b", - Namespace: "c", - Datacenter: "d", - }, - } - }, - "no-redirect": func() *ServiceResolverConfigEntry { - return &ServiceResolverConfigEntry{ + verifyRouter := func(t *testing.T, initial ConfigEntry) { + t.Helper() + require.IsType(t, &ServiceRouterConfigEntry{}, initial) + testEntry := initial.(*ServiceRouterConfigEntry) + + // set it + _, wm, err := config_entries.Set(testEntry, nil) + require.NoError(t, err) + require.NotNil(t, wm) + require.NotEqual(t, 0, wm.RequestTime) + + // get it + entry, qm, err := config_entries.Get(ServiceRouter, testEntry.Name, nil) + require.NoError(t, err) + require.NotNil(t, qm) + require.NotEqual(t, 0, qm.RequestTime) + + // verify it + readRouter, ok := entry.(*ServiceRouterConfigEntry) + require.True(t, ok) + readRouter.ModifyIndex = 0 // reset for Equals() + readRouter.CreateIndex = 0 // reset for Equals() + + require.Equal(t, testEntry, readRouter) + + // TODO(rb): cas? + // TODO(rb): list? + } + + // First set the necessary protocols to allow advanced routing features. + for _, service := range []string{ + "test-failover", + "test-redirect", + "alternate", + "test-split", + "test-route", + } { + serviceDefaults := &ServiceConfigEntry{ + Kind: ServiceDefaults, + Name: service, + Protocol: "http", + } + _, _, err := config_entries.Set(serviceDefaults, nil) + require.NoError(t, err) + } + + // NOTE: Due to service graph validation, these have to happen in a specific order. + for _, tc := range []struct { + name string + entry ConfigEntry + verify func(t *testing.T, initial ConfigEntry) + }{ + { + name: "failover", + entry: &ServiceResolverConfigEntry{ Kind: ServiceResolver, - Name: "test", + Name: "test-failover", DefaultSubset: "v1", Subsets: map[string]ServiceResolverSubset{ "v1": ServiceResolverSubset{ - Filter: "ServiceMeta.version == v1", + Filter: "Service.Meta.version == v1", }, "v2": ServiceResolverSubset{ - Filter: "ServiceMeta.version == v2", + Filter: "Service.Meta.version == v2", }, }, Failover: map[string]ServiceResolverFailover{ @@ -157,36 +148,83 @@ func TestAPI_ConfigEntry_DiscoveryChain(t *testing.T) { }, }, ConnectTimeout: 5 * time.Second, - } + }, + verify: verifyResolver, + }, + { + name: "redirect", + entry: &ServiceResolverConfigEntry{ + Kind: ServiceResolver, + Name: "test-redirect", + Redirect: &ServiceResolverRedirect{ + Service: "test-failover", + ServiceSubset: "v2", + Namespace: "c", + Datacenter: "d", + }, + }, + verify: verifyResolver, + }, + { + name: "mega splitter", // use one mega object to avoid multiple trips + entry: &ServiceSplitterConfigEntry{ + Kind: ServiceSplitter, + Name: "test-split", + Splits: []ServiceSplit{ + { + Weight: 90, + Service: "test-failover", + ServiceSubset: "v1", + Namespace: "c", + }, + { + Weight: 10, + Service: "test-redirect", + Namespace: "z", + }, + }, + }, + verify: verifySplitter, + }, + { + name: "mega router", // use one mega object to avoid multiple trips + entry: &ServiceRouterConfigEntry{ + Kind: ServiceRouter, + Name: "test-route", + Routes: []ServiceRoute{ + { + Match: &ServiceRouteMatch{ + HTTP: &ServiceRouteHTTPMatch{ + PathPrefix: "/prefix", + Header: []ServiceRouteHTTPMatchHeader{ + {Name: "x-debug", Exact: "1"}, + }, + QueryParam: []ServiceRouteHTTPMatchQueryParam{ + {Name: "debug", Value: "1"}, + }, + }, + }, + Destination: &ServiceRouteDestination{ + Service: "test-failover", + ServiceSubset: "v2", + Namespace: "sec", + PrefixRewrite: "/", + RequestTimeout: 5 * time.Second, + NumRetries: 5, + RetryOnConnectFailure: true, + RetryOnStatusCodes: []uint32{500, 503, 401}, + }, + }, + }, + }, + verify: verifyRouter, }, } { - // use one mega object to avoid multiple trips - makeEntry := tc - t.Run("Service Resolver - "+name, func(t *testing.T) { - - // set it - _, wm, err := config_entries.Set(makeEntry(), nil) - require.NoError(t, err) - require.NotNil(t, wm) - require.NotEqual(t, 0, wm.RequestTime) - - // get it - entry, qm, err := config_entries.Get(ServiceResolver, "test", nil) - require.NoError(t, err) - require.NotNil(t, qm) - require.NotEqual(t, 0, qm.RequestTime) - - // verify it - readResolver, ok := entry.(*ServiceResolverConfigEntry) - require.True(t, ok) - readResolver.ModifyIndex = 0 // reset for Equals() - readResolver.CreateIndex = 0 // reset for Equals() - - goldenEntry := makeEntry() - require.Equal(t, goldenEntry, readResolver) - - // TODO(rb): cas? - // TODO(rb): list? + tc := tc + name := fmt.Sprintf("%s:%s: %s", tc.entry.GetKind(), tc.entry.GetName(), tc.name) + ok := t.Run(name, func(t *testing.T) { + tc.verify(t, tc.entry) }) + require.True(t, ok, "subtest %q failed so aborting remainder", name) } } diff --git a/api/config_entry_test.go b/api/config_entry_test.go index 98e72d1d05..358c6549d2 100644 --- a/api/config_entry_test.go +++ b/api/config_entry_test.go @@ -170,7 +170,6 @@ func TestAPI_ConfigEntries(t *testing.T) { require.Equal(t, service2.Name, readService.Name) require.Equal(t, service2.Protocol, readService.Protocol) } - } // delete it