diff --git a/.changelog/11257.txt b/.changelog/11257.txt new file mode 100644 index 000000000..1fadec18c --- /dev/null +++ b/.changelog/11257.txt @@ -0,0 +1,3 @@ +```release-note:security +consul/connect: Fixed a bug causing the Nomad agent to panic if a mesh gateway was registered without a `proxy` block. +``` diff --git a/command/agent/consul/service_client.go b/command/agent/consul/service_client.go index 246f38234..afd06d92e 100644 --- a/command/agent/consul/service_client.go +++ b/command/agent/consul/service_client.go @@ -1100,26 +1100,32 @@ func (c *ServiceClient) serviceRegs(ops *operations, service *structs.Service, w kind = api.ServiceKindIngressGateway case service.Connect.IsTerminating(): kind = api.ServiceKindTerminatingGateway - // set the default port if bridge / default listener set - if defaultBind, exists := service.Connect.Gateway.Proxy.EnvoyGatewayBindAddresses["default"]; exists { - portLabel := envoy.PortLabel(structs.ConnectTerminatingPrefix, service.Name, "") - if dynPort, ok := workload.Ports.Get(portLabel); ok { - defaultBind.Port = dynPort.Value + + if proxy := service.Connect.Gateway.Proxy; proxy != nil { + // set the default port if bridge / default listener set + if defaultBind, exists := proxy.EnvoyGatewayBindAddresses["default"]; exists { + portLabel := envoy.PortLabel(structs.ConnectTerminatingPrefix, service.Name, "") + if dynPort, ok := workload.Ports.Get(portLabel); ok { + defaultBind.Port = dynPort.Value + } } } case service.Connect.IsMesh(): kind = api.ServiceKindMeshGateway - // wan uses the service port label, which is typically on a discrete host_network - if wanBind, exists := service.Connect.Gateway.Proxy.EnvoyGatewayBindAddresses["wan"]; exists { - if wanPort, ok := workload.Ports.Get(service.PortLabel); ok { - wanBind.Port = wanPort.Value + + if proxy := service.Connect.Gateway.Proxy; proxy != nil { + // wan uses the service port label, which is typically on a discrete host_network + if wanBind, exists := proxy.EnvoyGatewayBindAddresses["wan"]; exists { + if wanPort, ok := workload.Ports.Get(service.PortLabel); ok { + wanBind.Port = wanPort.Value + } } - } - // lan uses a nomad generated dynamic port on the default network - if lanBind, exists := service.Connect.Gateway.Proxy.EnvoyGatewayBindAddresses["lan"]; exists { - portLabel := envoy.PortLabel(structs.ConnectMeshPrefix, service.Name, "lan") - if dynPort, ok := workload.Ports.Get(portLabel); ok { - lanBind.Port = dynPort.Value + // lan uses a nomad generated dynamic port on the default network + if lanBind, exists := proxy.EnvoyGatewayBindAddresses["lan"]; exists { + portLabel := envoy.PortLabel(structs.ConnectMeshPrefix, service.Name, "lan") + if dynPort, ok := workload.Ports.Get(portLabel); ok { + lanBind.Port = dynPort.Value + } } } } diff --git a/nomad/job_endpoint_hook_connect.go b/nomad/job_endpoint_hook_connect.go index 1fb961b9f..a06f475ac 100644 --- a/nomad/job_endpoint_hook_connect.go +++ b/nomad/job_endpoint_hook_connect.go @@ -279,16 +279,10 @@ func groupConnectHook(job *structs.Job, g *structs.TaskGroup) error { // a name of an injected gateway task service.Name = env.ReplaceEnv(service.Name) - // detect whether the group is in host networking mode, which will - // require tweaking the default gateway task config - netHost := g.Networks[0].Mode == "host" - - if !netHost && service.Connect.IsGateway() { - // Modify the gateway proxy service configuration to automatically - // do the correct envoy bind address plumbing when inside a net - // namespace, but only if things are not explicitly configured. - service.Connect.Gateway.Proxy = gatewayProxyForBridge(service.Connect.Gateway) - } + // Generate a proxy configuration, if one is not provided, that is + // most appropriate for the network mode being used. + netMode := g.Networks[0].Mode + service.Connect.Gateway.Proxy = gatewayProxy(service.Connect.Gateway, netMode) // Inject a port whether bridge or host network (if not already set). // This port is accessed by the magic of Connect plumbing so it seems @@ -317,6 +311,10 @@ func groupConnectHook(job *structs.Job, g *structs.TaskGroup) error { // inject the gateway task only if it does not yet already exist if !hasGatewayTaskForService(g, service.Name) { prefix := service.Connect.Gateway.Prefix() + + // detect whether the group is in host networking mode, which will + // require tweaking the default gateway task config + netHost := netMode == "host" task := newConnectGatewayTask(prefix, service.Name, netHost) g.Tasks = append(g.Tasks, task) @@ -356,10 +354,10 @@ func gatewayProxyIsDefault(proxy *structs.ConsulGatewayProxy) bool { return false } -// gatewayProxyForBridge scans an existing gateway proxy configuration and tweaks -// it given an associated configuration entry so that it works as intended from -// inside a network namespace. -func gatewayProxyForBridge(gateway *structs.ConsulGateway) *structs.ConsulGatewayProxy { +// gatewayProxy scans an existing gateway proxy configuration and tweaks it +// given an associated configuration entry so that it works as intended with +// the network mode specified. +func gatewayProxy(gateway *structs.ConsulGateway, mode string) *structs.ConsulGatewayProxy { if gateway == nil { return nil } @@ -383,40 +381,42 @@ func gatewayProxyForBridge(gateway *structs.ConsulGateway) *structs.ConsulGatewa proxy.ConnectTimeout = helper.TimeToPtr(defaultConnectTimeout) } - // magically configure bind address(es) for bridge networking, per gateway type - // non-default configuration is gated above - switch { - case gateway.Ingress != nil: - proxy.EnvoyGatewayNoDefaultBind = true - proxy.EnvoyGatewayBindTaggedAddresses = false - proxy.EnvoyGatewayBindAddresses = gatewayBindAddressesIngress(gateway.Ingress) - case gateway.Terminating != nil: - proxy.EnvoyGatewayNoDefaultBind = true - proxy.EnvoyGatewayBindTaggedAddresses = false - proxy.EnvoyGatewayBindAddresses = map[string]*structs.ConsulGatewayBindAddress{ - "default": { - Address: "0.0.0.0", - Port: -1, // filled in later with dynamic port - }} - case gateway.Mesh != nil: - proxy.EnvoyGatewayNoDefaultBind = true - proxy.EnvoyGatewayBindTaggedAddresses = false - proxy.EnvoyGatewayBindAddresses = map[string]*structs.ConsulGatewayBindAddress{ - "wan": { - Address: "0.0.0.0", - Port: -1, // filled in later with configured port - }, - "lan": { - Address: "0.0.0.0", - Port: -1, // filled in later with generated port - }, + if mode == "bridge" { + // magically configure bind address(es) for bridge networking, per gateway type + // non-default configuration is gated above + switch { + case gateway.Ingress != nil: + proxy.EnvoyGatewayNoDefaultBind = true + proxy.EnvoyGatewayBindTaggedAddresses = false + proxy.EnvoyGatewayBindAddresses = gatewayBindAddressesIngressForBridge(gateway.Ingress) + case gateway.Terminating != nil: + proxy.EnvoyGatewayNoDefaultBind = true + proxy.EnvoyGatewayBindTaggedAddresses = false + proxy.EnvoyGatewayBindAddresses = map[string]*structs.ConsulGatewayBindAddress{ + "default": { + Address: "0.0.0.0", + Port: -1, // filled in later with dynamic port + }} + case gateway.Mesh != nil: + proxy.EnvoyGatewayNoDefaultBind = true + proxy.EnvoyGatewayBindTaggedAddresses = false + proxy.EnvoyGatewayBindAddresses = map[string]*structs.ConsulGatewayBindAddress{ + "wan": { + Address: "0.0.0.0", + Port: -1, // filled in later with configured port + }, + "lan": { + Address: "0.0.0.0", + Port: -1, // filled in later with generated port + }, + } } } return proxy } -func gatewayBindAddressesIngress(ingress *structs.ConsulIngressConfigEntry) map[string]*structs.ConsulGatewayBindAddress { +func gatewayBindAddressesIngressForBridge(ingress *structs.ConsulIngressConfigEntry) map[string]*structs.ConsulGatewayBindAddress { if ingress == nil || len(ingress.Listeners) == 0 { return make(map[string]*structs.ConsulGatewayBindAddress) } diff --git a/nomad/job_endpoint_hook_connect_test.go b/nomad/job_endpoint_hook_connect_test.go index 14d49aa07..75253ce13 100644 --- a/nomad/job_endpoint_hook_connect_test.go +++ b/nomad/job_endpoint_hook_connect_test.go @@ -112,7 +112,7 @@ func TestJobEndpointConnect_groupConnectHook(t *testing.T) { require.Exactly(t, tgExp, job.TaskGroups[0]) } -func TestJobEndpointConnect_groupConnectHook_IngressGateway(t *testing.T) { +func TestJobEndpointConnect_groupConnectHook_IngressGateway_BridgeNetwork(t *testing.T) { t.Parallel() // Test that the connect ingress gateway task is inserted if a gateway service @@ -135,7 +135,39 @@ func TestJobEndpointConnect_groupConnectHook_IngressGateway(t *testing.T) { expTG.Networks[0].Canonicalize() // rewrite the service gateway proxy configuration - expTG.Services[0].Connect.Gateway.Proxy = gatewayProxyForBridge(expTG.Services[0].Connect.Gateway) + expTG.Services[0].Connect.Gateway.Proxy = gatewayProxy(expTG.Services[0].Connect.Gateway, "bridge") + + require.NoError(t, groupConnectHook(job, job.TaskGroups[0])) + require.Exactly(t, expTG, job.TaskGroups[0]) + + // Test that the hook is idempotent + require.NoError(t, groupConnectHook(job, job.TaskGroups[0])) + require.Exactly(t, expTG, job.TaskGroups[0]) +} + +func TestJobEndpointConnect_groupConnectHook_IngressGateway_HostNetwork(t *testing.T) { + t.Parallel() + + // Test that the connect ingress gateway task is inserted if a gateway service + // exists. In host network mode, the default values are used. + job := mock.ConnectIngressGatewayJob("host", false) + job.Meta = map[string]string{ + "gateway_name": "my-gateway", + } + job.TaskGroups[0].Services[0].Name = "${NOMAD_META_gateway_name}" + + // setup expectations + expTG := job.TaskGroups[0].Copy() + expTG.Tasks = []*structs.Task{ + // inject the gateway task + newConnectGatewayTask(structs.ConnectIngressPrefix, "my-gateway", true), + } + expTG.Services[0].Name = "my-gateway" + expTG.Tasks[0].Canonicalize(job, expTG) + expTG.Networks[0].Canonicalize() + + // rewrite the service gateway proxy configuration + expTG.Services[0].Connect.Gateway.Proxy = gatewayProxy(expTG.Services[0].Connect.Gateway, "host") require.NoError(t, groupConnectHook(job, job.TaskGroups[0])) require.Exactly(t, expTG, job.TaskGroups[0]) @@ -204,7 +236,7 @@ func TestJobEndpointConnect_groupConnectHook_IngressGateway_CustomTask(t *testin expTG.Networks[0].Canonicalize() // rewrite the service gateway proxy configuration - expTG.Services[0].Connect.Gateway.Proxy = gatewayProxyForBridge(expTG.Services[0].Connect.Gateway) + expTG.Services[0].Connect.Gateway.Proxy = gatewayProxy(expTG.Services[0].Connect.Gateway, "bridge") require.NoError(t, groupConnectHook(job, job.TaskGroups[0])) require.Exactly(t, expTG, job.TaskGroups[0]) @@ -237,7 +269,7 @@ func TestJobEndpointConnect_groupConnectHook_TerminatingGateway(t *testing.T) { expTG.Networks[0].Canonicalize() // rewrite the service gateway proxy configuration - expTG.Services[0].Connect.Gateway.Proxy = gatewayProxyForBridge(expTG.Services[0].Connect.Gateway) + expTG.Services[0].Connect.Gateway.Proxy = gatewayProxy(expTG.Services[0].Connect.Gateway, "bridge") require.NoError(t, groupConnectHook(job, job.TaskGroups[0])) require.Exactly(t, expTG, job.TaskGroups[0]) @@ -278,7 +310,7 @@ func TestJobEndpointConnect_groupConnectHook_MeshGateway(t *testing.T) { expTG.Networks[0].Canonicalize() // rewrite the service gateway proxy configuration - expTG.Services[0].Connect.Gateway.Proxy = gatewayProxyForBridge(expTG.Services[0].Connect.Gateway) + expTG.Services[0].Connect.Gateway.Proxy = gatewayProxy(expTG.Services[0].Connect.Gateway, "bridge") require.NoError(t, groupConnectHook(job, job.TaskGroups[0])) require.Exactly(t, expTG, job.TaskGroups[0]) @@ -692,22 +724,22 @@ func TestJobEndpointConnect_gatewayProxyIsDefault(t *testing.T) { }) } -func TestJobEndpointConnect_gatewayBindAddresses(t *testing.T) { +func TestJobEndpointConnect_gatewayBindAddressesForBridge(t *testing.T) { t.Parallel() t.Run("nil", func(t *testing.T) { - result := gatewayBindAddressesIngress(nil) + result := gatewayBindAddressesIngressForBridge(nil) require.Empty(t, result) }) t.Run("no listeners", func(t *testing.T) { - result := gatewayBindAddressesIngress(&structs.ConsulIngressConfigEntry{Listeners: nil}) + result := gatewayBindAddressesIngressForBridge(&structs.ConsulIngressConfigEntry{Listeners: nil}) require.Empty(t, result) }) t.Run("simple", func(t *testing.T) { - result := gatewayBindAddressesIngress(&structs.ConsulIngressConfigEntry{ + result := gatewayBindAddressesIngressForBridge(&structs.ConsulIngressConfigEntry{ Listeners: []*structs.ConsulIngressListener{{ Port: 3000, Protocol: "tcp", @@ -725,7 +757,7 @@ func TestJobEndpointConnect_gatewayBindAddresses(t *testing.T) { }) t.Run("complex", func(t *testing.T) { - result := gatewayBindAddressesIngress(&structs.ConsulIngressConfigEntry{ + result := gatewayBindAddressesIngressForBridge(&structs.ConsulIngressConfigEntry{ Listeners: []*structs.ConsulIngressListener{{ Port: 3000, Protocol: "tcp", @@ -759,16 +791,16 @@ func TestJobEndpointConnect_gatewayBindAddresses(t *testing.T) { }) } -func TestJobEndpointConnect_gatewayProxyForBridge(t *testing.T) { +func TestJobEndpointConnect_gatewayProxy(t *testing.T) { t.Parallel() t.Run("nil", func(t *testing.T) { - result := gatewayProxyForBridge(nil) + result := gatewayProxy(nil, "bridge") require.Nil(t, result) }) t.Run("nil proxy", func(t *testing.T) { - result := gatewayProxyForBridge(&structs.ConsulGateway{ + result := gatewayProxy(&structs.ConsulGateway{ Ingress: &structs.ConsulIngressConfigEntry{ Listeners: []*structs.ConsulIngressListener{{ Port: 3000, @@ -778,7 +810,7 @@ func TestJobEndpointConnect_gatewayProxyForBridge(t *testing.T) { }}, }}, }, - }) + }, "bridge") require.Equal(t, &structs.ConsulGatewayProxy{ ConnectTimeout: helper.TimeToPtr(defaultConnectTimeout), EnvoyGatewayNoDefaultBind: true, @@ -792,7 +824,7 @@ func TestJobEndpointConnect_gatewayProxyForBridge(t *testing.T) { }) t.Run("ingress set defaults", func(t *testing.T) { - result := gatewayProxyForBridge(&structs.ConsulGateway{ + result := gatewayProxy(&structs.ConsulGateway{ Proxy: &structs.ConsulGatewayProxy{ ConnectTimeout: helper.TimeToPtr(2 * time.Second), Config: map[string]interface{}{"foo": 1}, @@ -806,7 +838,7 @@ func TestJobEndpointConnect_gatewayProxyForBridge(t *testing.T) { }}, }}, }, - }) + }, "bridge") require.Equal(t, &structs.ConsulGatewayProxy{ ConnectTimeout: helper.TimeToPtr(2 * time.Second), Config: map[string]interface{}{"foo": 1}, @@ -821,7 +853,7 @@ func TestJobEndpointConnect_gatewayProxyForBridge(t *testing.T) { }) t.Run("ingress leave as-is", func(t *testing.T) { - result := gatewayProxyForBridge(&structs.ConsulGateway{ + result := gatewayProxy(&structs.ConsulGateway{ Proxy: &structs.ConsulGatewayProxy{ Config: map[string]interface{}{"foo": 1}, EnvoyGatewayBindTaggedAddresses: true, @@ -835,7 +867,7 @@ func TestJobEndpointConnect_gatewayProxyForBridge(t *testing.T) { }}, }}, }, - }) + }, "bridge") require.Equal(t, &structs.ConsulGatewayProxy{ ConnectTimeout: nil, Config: map[string]interface{}{"foo": 1}, @@ -846,7 +878,7 @@ func TestJobEndpointConnect_gatewayProxyForBridge(t *testing.T) { }) t.Run("terminating set defaults", func(t *testing.T) { - result := gatewayProxyForBridge(&structs.ConsulGateway{ + result := gatewayProxy(&structs.ConsulGateway{ Proxy: &structs.ConsulGatewayProxy{ ConnectTimeout: helper.TimeToPtr(2 * time.Second), EnvoyDNSDiscoveryType: "STRICT_DNS", @@ -860,7 +892,7 @@ func TestJobEndpointConnect_gatewayProxyForBridge(t *testing.T) { SNI: "", }}, }, - }) + }, "bridge") require.Equal(t, &structs.ConsulGatewayProxy{ ConnectTimeout: helper.TimeToPtr(2 * time.Second), EnvoyGatewayNoDefaultBind: true, @@ -876,7 +908,7 @@ func TestJobEndpointConnect_gatewayProxyForBridge(t *testing.T) { }) t.Run("terminating leave as-is", func(t *testing.T) { - result := gatewayProxyForBridge(&structs.ConsulGateway{ + result := gatewayProxy(&structs.ConsulGateway{ Proxy: &structs.ConsulGatewayProxy{ Config: map[string]interface{}{"foo": 1}, EnvoyGatewayBindTaggedAddresses: true, @@ -886,7 +918,7 @@ func TestJobEndpointConnect_gatewayProxyForBridge(t *testing.T) { Name: "service1", }}, }, - }) + }, "bridge") require.Equal(t, &structs.ConsulGatewayProxy{ ConnectTimeout: nil, Config: map[string]interface{}{"foo": 1}, @@ -896,15 +928,15 @@ func TestJobEndpointConnect_gatewayProxyForBridge(t *testing.T) { }, result) }) - t.Run("mesh set defaults", func(t *testing.T) { - result := gatewayProxyForBridge(&structs.ConsulGateway{ + t.Run("mesh set defaults in bridge", func(t *testing.T) { + result := gatewayProxy(&structs.ConsulGateway{ Proxy: &structs.ConsulGatewayProxy{ ConnectTimeout: helper.TimeToPtr(2 * time.Second), }, Mesh: &structs.ConsulMeshConfigEntry{ // nothing }, - }) + }, "bridge") require.Equal(t, &structs.ConsulGatewayProxy{ ConnectTimeout: helper.TimeToPtr(2 * time.Second), EnvoyGatewayNoDefaultBind: true, @@ -922,8 +954,22 @@ func TestJobEndpointConnect_gatewayProxyForBridge(t *testing.T) { }, result) }) + t.Run("mesh set defaults in host", func(t *testing.T) { + result := gatewayProxy(&structs.ConsulGateway{ + Proxy: &structs.ConsulGatewayProxy{ + ConnectTimeout: helper.TimeToPtr(2 * time.Second), + }, + Mesh: &structs.ConsulMeshConfigEntry{ + // nothing + }, + }, "host") + require.Equal(t, &structs.ConsulGatewayProxy{ + ConnectTimeout: helper.TimeToPtr(2 * time.Second), + }, result) + }) + t.Run("mesh leave as-is", func(t *testing.T) { - result := gatewayProxyForBridge(&structs.ConsulGateway{ + result := gatewayProxy(&structs.ConsulGateway{ Proxy: &structs.ConsulGatewayProxy{ Config: map[string]interface{}{"foo": 1}, EnvoyGatewayBindTaggedAddresses: true, @@ -931,7 +977,7 @@ func TestJobEndpointConnect_gatewayProxyForBridge(t *testing.T) { Mesh: &structs.ConsulMeshConfigEntry{ // nothing }, - }) + }, "bridge") require.Equal(t, &structs.ConsulGatewayProxy{ ConnectTimeout: nil, Config: map[string]interface{}{"foo": 1},