From ad8a0544f2137f212e2defb1614143795204c1b1 Mon Sep 17 00:00:00 2001 From: Chris Piraino Date: Wed, 6 May 2020 16:09:24 -0500 Subject: [PATCH] Require individual services in ingress entry to match protocols (#7774) We require any non-wildcard services to match the protocol defined in the listener on write, so that we can maintain a consistent experience through ingress gateways. This also helps guard against accidental misconfiguration by a user. - Update tests that require an updated protocol for ingress gateways --- agent/consul/health_endpoint_test.go | 19 +++++ agent/consul/state/catalog_test.go | 40 --------- agent/consul/state/config_entry.go | 46 +++++++++++ agent/consul/state/config_entry_test.go | 82 +++++++++++++++++++ agent/dns_test.go | 19 +++++ api/config_entry_gateways_test.go | 15 +++- .../config_entries.hcl | 14 ++-- .../config_entries.hcl | 14 ++-- 8 files changed, 194 insertions(+), 55 deletions(-) diff --git a/agent/consul/health_endpoint_test.go b/agent/consul/health_endpoint_test.go index c0ad43c2c..3178e4f72 100644 --- a/agent/consul/health_endpoint_test.go +++ b/agent/consul/health_endpoint_test.go @@ -1286,6 +1286,25 @@ func TestHealth_ServiceNodes_Ingress_ACL(t *testing.T) { } require.Nil(t, msgpackrpc.CallWithCodec(codec, "Catalog.Register", &arg, &out)) + // Register proxy-defaults with 'http' protocol + { + req := structs.ConfigEntryRequest{ + Op: structs.ConfigEntryUpsert, + Datacenter: "dc1", + Entry: &structs.ProxyConfigEntry{ + Kind: structs.ProxyDefaults, + Name: structs.ProxyConfigGlobal, + Config: map[string]interface{}{ + "protocol": "http", + }, + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + var out bool + require.Nil(t, msgpackrpc.CallWithCodec(codec, "ConfigEntry.Apply", &req, &out)) + require.True(t, out) + } + // Register ingress-gateway config entry { args := &structs.IngressGatewayConfigEntry{ diff --git a/agent/consul/state/catalog_test.go b/agent/consul/state/catalog_test.go index 5d96d03aa..a9656f235 100644 --- a/agent/consul/state/catalog_test.go +++ b/agent/consul/state/catalog_test.go @@ -5433,46 +5433,6 @@ func TestStateStore_GatewayServices_IngressProtocolFiltering(t *testing.T) { require.Equal(uint64(8), idx) require.ElementsMatch(results, expected) }) - - // Relies on service defaults for service1 being set to grpc above - t.Run("only filters on gateway services from wildcards", func(t *testing.T) { - require := require.New(t) - expected := structs.GatewayServices{ - { - Gateway: structs.NewServiceID("ingress1", nil), - Service: structs.NewServiceID("service1", nil), - GatewayKind: structs.ServiceKindIngressGateway, - Port: 4444, - Protocol: "http", - RaftIndex: structs.RaftIndex{ - CreateIndex: 8, - ModifyIndex: 8, - }, - }, - } - - ingress1 := &structs.IngressGatewayConfigEntry{ - Kind: "ingress-gateway", - Name: "ingress1", - Listeners: []structs.IngressListener{ - { - Port: 4444, - Protocol: "http", - Services: []structs.IngressService{ - { - Name: "service1", - }, - }, - }, - }, - } - assert.NoError(t, s.EnsureConfigEntry(8, ingress1, nil)) - - idx, results, err := s.GatewayServices(nil, "ingress1", nil) - require.NoError(err) - require.Equal(uint64(8), idx) - require.ElementsMatch(results, expected) - }) } func setupIngressState(t *testing.T, s *Store) memdb.WatchSet { diff --git a/agent/consul/state/config_entry.go b/agent/consul/state/config_entry.go index 07ad61652..d0ffd07af 100644 --- a/agent/consul/state/config_entry.go +++ b/agent/consul/state/config_entry.go @@ -351,6 +351,10 @@ func (s *Store) validateProposedConfigEntryInGraph( if err != nil { return err } + err = s.validateProposedIngressProtocolsInServiceGraph(tx, next, entMeta) + if err != nil { + return err + } case structs.TerminatingGateway: err := s.checkGatewayClash(tx, name, structs.TerminatingGateway, structs.IngressGateway, entMeta) if err != nil { @@ -834,6 +838,48 @@ func (s *Store) configEntryWithOverridesTxn( return s.configEntryTxn(tx, ws, kind, name, entMeta) } +func (s *Store) validateProposedIngressProtocolsInServiceGraph( + tx *memdb.Txn, + next structs.ConfigEntry, + entMeta *structs.EnterpriseMeta, +) error { + // This is the case for deleting a config entry + if next == nil { + return nil + } + ingress, ok := next.(*structs.IngressGatewayConfigEntry) + if !ok { + return fmt.Errorf("type %T is not an ingress gateway config entry", next) + } + + validationFn := func(svc structs.ServiceID, expectedProto string) error { + _, svcProto, err := s.protocolForService(tx, nil, svc) + if err != nil { + return err + } + + if svcProto != expectedProto { + return fmt.Errorf("service %q has protocol %q, which does not match defined listener protocol %q", + svc.String(), svcProto, expectedProto) + } + + return nil + } + + for _, l := range ingress.Listeners { + for _, s := range l.Services { + if s.Name == structs.WildcardSpecifier { + continue + } + err := validationFn(s.ToServiceID(), l.Protocol) + if err != nil { + return err + } + } + } + return nil +} + // protocolForService returns the service graph protocol associated to the // provided service, checking all relevant config entries. func (s *Store) protocolForService( diff --git a/agent/consul/state/config_entry_test.go b/agent/consul/state/config_entry_test.go index 0255a9416..dbcd6971f 100644 --- a/agent/consul/state/config_entry_test.go +++ b/agent/consul/state/config_entry_test.go @@ -1284,3 +1284,85 @@ func TestStore_ValidateGatewayNamesCannotBeShared(t *testing.T) { // Cannot have 2 gateways with same service name require.Error(t, s.EnsureConfigEntry(5, ingress, nil)) } + +func TestStore_ValidateIngressGatewayErrorOnMismatchedProtocols(t *testing.T) { + s := testStateStore(t) + + ingress := &structs.IngressGatewayConfigEntry{ + Kind: structs.IngressGateway, + Name: "gateway", + Listeners: []structs.IngressListener{ + { + Port: 8080, + Protocol: "http", + Services: []structs.IngressService{ + {Name: "web"}, + }, + }, + }, + } + + t.Run("default to tcp", func(t *testing.T) { + err := s.EnsureConfigEntry(0, ingress, nil) + require.Error(t, err) + require.Contains(t, err.Error(), `service "web" has protocol "tcp"`) + }) + + t.Run("with proxy-default", func(t *testing.T) { + expected := &structs.ProxyConfigEntry{ + Kind: structs.ProxyDefaults, + Name: "global", + Config: map[string]interface{}{ + "protocol": "http2", + }, + } + require.NoError(t, s.EnsureConfigEntry(0, expected, nil)) + + err := s.EnsureConfigEntry(1, ingress, nil) + require.Error(t, err) + require.Contains(t, err.Error(), `service "web" has protocol "http2"`) + }) + + t.Run("with service-defaults override", func(t *testing.T) { + expected := &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "web", + Protocol: "grpc", + } + require.NoError(t, s.EnsureConfigEntry(1, expected, nil)) + err := s.EnsureConfigEntry(2, ingress, nil) + require.Error(t, err) + require.Contains(t, err.Error(), `service "web" has protocol "grpc"`) + }) + + t.Run("with service-defaults correct protocol", func(t *testing.T) { + expected := &structs.ServiceConfigEntry{ + Kind: structs.ServiceDefaults, + Name: "web", + Protocol: "http", + } + require.NoError(t, s.EnsureConfigEntry(2, expected, nil)) + require.NoError(t, s.EnsureConfigEntry(3, ingress, nil)) + }) + + t.Run("ignores wildcard specifier", func(t *testing.T) { + ingress := &structs.IngressGatewayConfigEntry{ + Kind: structs.IngressGateway, + Name: "gateway", + Listeners: []structs.IngressListener{ + { + Port: 8080, + Protocol: "http", + Services: []structs.IngressService{ + {Name: "*"}, + }, + }, + }, + } + require.NoError(t, s.EnsureConfigEntry(4, ingress, nil)) + }) + + t.Run("deleting a config entry", func(t *testing.T) { + require.NoError(t, s.DeleteConfigEntry(5, structs.IngressGateway, "gateway", nil)) + }) +} diff --git a/agent/dns_test.go b/agent/dns_test.go index 2d24b6391..3a1cb7146 100644 --- a/agent/dns_test.go +++ b/agent/dns_test.go @@ -1667,6 +1667,25 @@ func TestDNS_IngressServiceLookup(t *testing.T) { require.Nil(t, a.RPC("Catalog.Register", args, &out)) } + // Register proxy-defaults with 'http' protocol + { + req := structs.ConfigEntryRequest{ + Op: structs.ConfigEntryUpsert, + Datacenter: "dc1", + Entry: &structs.ProxyConfigEntry{ + Kind: structs.ProxyDefaults, + Name: structs.ProxyConfigGlobal, + Config: map[string]interface{}{ + "protocol": "http", + }, + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + var out bool + require.Nil(t, a.RPC("ConfigEntry.Apply", req, &out)) + require.True(t, out) + } + // Register ingress-gateway config entry { args := &structs.IngressGatewayConfigEntry{ diff --git a/api/config_entry_gateways_test.go b/api/config_entry_gateways_test.go index d8bf50c93..2d9f4207c 100644 --- a/api/config_entry_gateways_test.go +++ b/api/config_entry_gateways_test.go @@ -23,8 +23,21 @@ func TestAPI_ConfigEntries_IngressGateway(t *testing.T) { Name: "bar", } + global := &ProxyConfigEntry{ + Kind: ProxyDefaults, + Name: ProxyConfigGlobal, + Config: map[string]interface{}{ + "protocol": "http", + }, + } + // set default protocol to http so that ingress gateways pass validation + _, wm, err := config_entries.Set(global, nil) + require.NoError(t, err) + require.NotNil(t, wm) + require.NotEqual(t, 0, wm.RequestTime) + // set it - _, wm, err := config_entries.Set(ingress1, nil) + _, wm, err = config_entries.Set(ingress1, nil) require.NoError(t, err) require.NotNil(t, wm) require.NotEqual(t, 0, wm.RequestTime) diff --git a/test/integration/connect/envoy/case-ingress-gateway-http/config_entries.hcl b/test/integration/connect/envoy/case-ingress-gateway-http/config_entries.hcl index 31da183a8..f6d0bac6d 100644 --- a/test/integration/connect/envoy/case-ingress-gateway-http/config_entries.hcl +++ b/test/integration/connect/envoy/case-ingress-gateway-http/config_entries.hcl @@ -2,6 +2,13 @@ enable_central_service_config = true config_entries { bootstrap = [ + { + kind = "proxy-defaults" + name = "global" + config { + protocol = "http" + } + }, { kind = "ingress-gateway" name = "ingress-gateway" @@ -18,13 +25,6 @@ config_entries { } ] }, - { - kind = "proxy-defaults" - name = "global" - config { - protocol = "http" - } - }, { kind = "service-router" // This is a "virtual" service name and will not have a backing diff --git a/test/integration/connect/envoy/case-ingress-gateway-multiple-services/config_entries.hcl b/test/integration/connect/envoy/case-ingress-gateway-multiple-services/config_entries.hcl index f799a85a0..0d3177f41 100644 --- a/test/integration/connect/envoy/case-ingress-gateway-multiple-services/config_entries.hcl +++ b/test/integration/connect/envoy/case-ingress-gateway-multiple-services/config_entries.hcl @@ -2,6 +2,13 @@ enable_central_service_config = true config_entries { bootstrap = [ + { + kind = "proxy-defaults" + name = "global" + config { + protocol = "http" + } + }, { kind = "ingress-gateway" name = "ingress-gateway" @@ -27,13 +34,6 @@ config_entries { ] } ] - }, - { - kind = "proxy-defaults" - name = "global" - config { - protocol = "http" - } } ] }