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

@@ -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{

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 {

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(

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))
+	})
+}

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{

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)

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

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"
-      }
     }
   ]
 }