diff --git a/agent/consul/catalog_endpoint.go b/agent/consul/catalog_endpoint.go index 0c1cbe3de7..5cb30b9c30 100644 --- a/agent/consul/catalog_endpoint.go +++ b/agent/consul/catalog_endpoint.go @@ -47,6 +47,24 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error // Handle a service registration. if args.Service != nil { + // Connect proxy specific logic + if args.Service.Kind == structs.ServiceKindConnectProxy { + // Name is optional, if it isn't set, we default to the + // proxy name. It actually MUST be this, but the validation + // below this will verify. + if args.Service.Service == "" { + args.Service.Service = fmt.Sprintf( + "%s-connect-proxy", args.Service.ProxyDestination) + } + } + + // Validate the service. This is in addition to the below since + // the above just hasn't been moved over yet. We should move it over + // in time. + if err := args.Service.Validate(); err != nil { + return err + } + // If no service id, but service name, use default if args.Service.ID == "" && args.Service.Service != "" { args.Service.ID = args.Service.Service diff --git a/agent/consul/catalog_endpoint_test.go b/agent/consul/catalog_endpoint_test.go index 572ff86bbf..2399e9b2f4 100644 --- a/agent/consul/catalog_endpoint_test.go +++ b/agent/consul/catalog_endpoint_test.go @@ -333,6 +333,87 @@ func TestCatalog_Register_ForwardDC(t *testing.T) { } } +func TestCatalog_Register_ConnectProxy(t *testing.T) { + t.Parallel() + + assert := assert.New(t) + dir1, s1 := testServer(t) + defer os.RemoveAll(dir1) + defer s1.Shutdown() + codec := rpcClient(t, s1) + defer codec.Close() + + args := structs.TestRegisterRequestProxy(t) + + // Register + var out struct{} + assert.Nil(msgpackrpc.CallWithCodec(codec, "Catalog.Register", &args, &out)) + + // List + req := structs.ServiceSpecificRequest{ + Datacenter: "dc1", + ServiceName: args.Service.Service, + } + var resp structs.IndexedServiceNodes + assert.Nil(msgpackrpc.CallWithCodec(codec, "Catalog.ServiceNodes", &req, &resp)) + assert.Len(resp.ServiceNodes, 1) + v := resp.ServiceNodes[0] + assert.Equal(structs.ServiceKindConnectProxy, v.ServiceKind) + assert.Equal(args.Service.ProxyDestination, v.ServiceProxyDestination) +} + +// Test an invalid ConnectProxy. We don't need to exhaustively test because +// this is all tested in structs on the Validate method. +func TestCatalog_Register_ConnectProxy_invalid(t *testing.T) { + t.Parallel() + + assert := assert.New(t) + dir1, s1 := testServer(t) + defer os.RemoveAll(dir1) + defer s1.Shutdown() + codec := rpcClient(t, s1) + defer codec.Close() + + args := structs.TestRegisterRequestProxy(t) + args.Service.ProxyDestination = "" + + // Register + var out struct{} + err := msgpackrpc.CallWithCodec(codec, "Catalog.Register", &args, &out) + assert.NotNil(err) + assert.Contains(err.Error(), "ProxyDestination") +} + +// Test registering a proxy with no name set, which should work. +func TestCatalog_Register_ConnectProxy_noName(t *testing.T) { + t.Parallel() + + assert := assert.New(t) + dir1, s1 := testServer(t) + defer os.RemoveAll(dir1) + defer s1.Shutdown() + codec := rpcClient(t, s1) + defer codec.Close() + + args := structs.TestRegisterRequestProxy(t) + args.Service.Service = "" + + // Register + var out struct{} + assert.Nil(msgpackrpc.CallWithCodec(codec, "Catalog.Register", &args, &out)) + + // List + req := structs.ServiceSpecificRequest{ + Datacenter: "dc1", + ServiceName: fmt.Sprintf("%s-connect-proxy", args.Service.ProxyDestination), + } + var resp structs.IndexedServiceNodes + assert.Nil(msgpackrpc.CallWithCodec(codec, "Catalog.ServiceNodes", &req, &resp)) + assert.Len(resp.ServiceNodes, 1) + v := resp.ServiceNodes[0] + assert.Equal(structs.ServiceKindConnectProxy, v.ServiceKind) +} + func TestCatalog_Deregister(t *testing.T) { t.Parallel() dir1, s1 := testServer(t) diff --git a/agent/structs/structs.go b/agent/structs/structs.go index 65ec87024e..e1ab91ab54 100644 --- a/agent/structs/structs.go +++ b/agent/structs/structs.go @@ -12,6 +12,7 @@ import ( "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/types" "github.com/hashicorp/go-msgpack/codec" + "github.com/hashicorp/go-multierror" "github.com/hashicorp/serf/coordinate" ) @@ -488,6 +489,31 @@ type NodeService struct { RaftIndex } +// Validate validates the node service configuration. +// +// NOTE(mitchellh): This currently only validates fields for a ConnectProxy. +// Historically validation has been directly in the Catalog.Register RPC. +// ConnectProxy validation was moved here for easier table testing, but +// other validation still exists in Catalog.Register. +func (s *NodeService) Validate() error { + var result error + + // ConnectProxy validation + if s.Kind == ServiceKindConnectProxy { + if strings.TrimSpace(s.ProxyDestination) == "" { + result = multierror.Append(result, fmt.Errorf( + "ProxyDestination must be non-empty for Connect proxy services")) + } + + if s.Port == 0 { + result = multierror.Append(result, fmt.Errorf( + "Port must be set for a Connect proxy")) + } + } + + return result +} + // IsSame checks if one NodeService is the same as another, without looking // at the Raft information (that's why we didn't call it IsEqual). This is // useful for seeing if an update would be idempotent for all the functional diff --git a/agent/structs/structs_test.go b/agent/structs/structs_test.go index dcb8e0c4ef..972146d93e 100644 --- a/agent/structs/structs_test.go +++ b/agent/structs/structs_test.go @@ -8,6 +8,7 @@ import ( "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/types" + "github.com/stretchr/testify/assert" ) func TestEncodeDecode(t *testing.T) { @@ -208,6 +209,60 @@ func TestStructs_ServiceNode_Conversions(t *testing.T) { } } +func TestStructs_NodeService_ValidateConnectProxy(t *testing.T) { + cases := []struct { + Name string + Modify func(*NodeService) + Err string + }{ + { + "valid", + func(x *NodeService) {}, + "", + }, + + { + "connect-proxy: no ProxyDestination", + func(x *NodeService) { x.ProxyDestination = "" }, + "ProxyDestination must be", + }, + + { + "connect-proxy: whitespace ProxyDestination", + func(x *NodeService) { x.ProxyDestination = " " }, + "ProxyDestination must be", + }, + + { + "connect-proxy: valid ProxyDestination", + func(x *NodeService) { x.ProxyDestination = "hello" }, + "", + }, + + { + "connect-proxy: no port set", + func(x *NodeService) { x.Port = 0 }, + "Port must", + }, + } + + for _, tc := range cases { + t.Run(tc.Name, func(t *testing.T) { + assert := assert.New(t) + ns := TestNodeServiceProxy(t) + tc.Modify(ns) + + err := ns.Validate() + assert.Equal(err != nil, tc.Err != "", err) + if err == nil { + return + } + + assert.Contains(strings.ToLower(err.Error()), strings.ToLower(tc.Err)) + }) + } +} + func TestStructs_NodeService_IsSame(t *testing.T) { ns := &NodeService{ ID: "node1", diff --git a/agent/structs/testing_catalog.go b/agent/structs/testing_catalog.go index 8a002d3803..4a55f1e3d3 100644 --- a/agent/structs/testing_catalog.go +++ b/agent/structs/testing_catalog.go @@ -11,12 +11,18 @@ func TestRegisterRequestProxy(t testing.T) *RegisterRequest { Datacenter: "dc1", Node: "foo", Address: "127.0.0.1", - Service: &NodeService{ - Kind: ServiceKindConnectProxy, - Service: ConnectProxyServiceName, - Address: "127.0.0.2", - Port: 2222, - ProxyDestination: "web", - }, + Service: TestNodeServiceProxy(t), + } +} + +// TestNodeServiceProxy returns a *NodeService representing a valid +// Connect proxy. +func TestNodeServiceProxy(t testing.T) *NodeService { + return &NodeService{ + Kind: ServiceKindConnectProxy, + Service: ConnectProxyServiceName, + Address: "127.0.0.2", + Port: 2222, + ProxyDestination: "web", } }