From abe2ca94c5f04ea8e70f00c6dc29512d9223daf2 Mon Sep 17 00:00:00 2001 From: Chris Piraino Date: Mon, 22 Jun 2020 15:14:12 -0400 Subject: [PATCH 1/2] Update gateway-services-nodes API endpoint to allow multiple addresses Previously, we were only returning a single ListenerPort for a single service. However, we actually allow a single service to be serviced over multiple ports, as well as allow users to define what hostnames they expect their services to be contacted over. When no hosts are defined, we return the default ingress domain for any configured DNS domain. To show this in the UI, we modify the gateway-services-nodes API to return a GatewayConfig.Addresses field, which is a list of addresses over which the specific service can be contacted. --- agent/dns.go | 6 ++- agent/dns_oss.go | 4 +- agent/dns_test.go | 6 +-- agent/structs/config_entry_gateways.go | 17 ++++++ agent/structs/config_entry_gateways_test.go | 58 +++++++++++++++++++++ agent/ui_endpoint.go | 52 +++++++++++++++--- agent/ui_endpoint_test.go | 55 ++++++++++++++++--- 7 files changed, 180 insertions(+), 18 deletions(-) diff --git a/agent/dns.go b/agent/dns.go index bbaae3582..c1c1d4f37 100644 --- a/agent/dns.go +++ b/agent/dns.go @@ -318,7 +318,11 @@ START: } func serviceNodeCanonicalDNSName(sn *structs.ServiceNode, domain string) string { - return serviceCanonicalDNSName(sn.ServiceName, sn.Datacenter, domain, &sn.EnterpriseMeta) + return serviceCanonicalDNSName(sn.ServiceName, "service", sn.Datacenter, domain, &sn.EnterpriseMeta) +} + +func serviceIngressDNSName(service, datacenter, domain string, entMeta *structs.EnterpriseMeta) string { + return serviceCanonicalDNSName(service, "ingress", datacenter, domain, entMeta) } // handlePtr is used to handle "reverse" DNS queries diff --git a/agent/dns_oss.go b/agent/dns_oss.go index 757dd660f..47b68bae0 100644 --- a/agent/dns_oss.go +++ b/agent/dns_oss.go @@ -26,6 +26,6 @@ func (d *DNSServer) parseDatacenterAndEnterpriseMeta(labels []string, _ *dnsConf return false } -func serviceCanonicalDNSName(name, datacenter, domain string, _ *structs.EnterpriseMeta) string { - return fmt.Sprintf("%s.service.%s.%s", name, datacenter, domain) +func serviceCanonicalDNSName(name, kind, datacenter, domain string, _ *structs.EnterpriseMeta) string { + return fmt.Sprintf("%s.%s.%s.%s", name, kind, datacenter, domain) } diff --git a/agent/dns_test.go b/agent/dns_test.go index fb6a5878b..6d5fd1e50 100644 --- a/agent/dns_test.go +++ b/agent/dns_test.go @@ -1041,7 +1041,7 @@ func TestDNS_ServiceReverseLookup(t *testing.T) { if !ok { t.Fatalf("Bad: %#v", in.Answer[0]) } - if ptrRec.Ptr != serviceCanonicalDNSName("db", "dc1", "consul", nil)+"." { + if ptrRec.Ptr != serviceCanonicalDNSName("db", "service", "dc1", "consul", nil)+"." { t.Fatalf("Bad: %#v", ptrRec) } } @@ -1089,7 +1089,7 @@ func TestDNS_ServiceReverseLookup_IPV6(t *testing.T) { if !ok { t.Fatalf("Bad: %#v", in.Answer[0]) } - if ptrRec.Ptr != serviceCanonicalDNSName("db", "dc1", "consul", nil)+"." { + if ptrRec.Ptr != serviceCanonicalDNSName("db", "service", "dc1", "consul", nil)+"." { t.Fatalf("Bad: %#v", ptrRec) } } @@ -1139,7 +1139,7 @@ func TestDNS_ServiceReverseLookup_CustomDomain(t *testing.T) { if !ok { t.Fatalf("Bad: %#v", in.Answer[0]) } - if ptrRec.Ptr != serviceCanonicalDNSName("db", "dc1", "custom", nil)+"." { + if ptrRec.Ptr != serviceCanonicalDNSName("db", "service", "dc1", "custom", nil)+"." { t.Fatalf("Bad: %#v", ptrRec) } } diff --git a/agent/structs/config_entry_gateways.go b/agent/structs/config_entry_gateways.go index 97140c6b0..d246755f4 100644 --- a/agent/structs/config_entry_gateways.go +++ b/agent/structs/config_entry_gateways.go @@ -376,6 +376,23 @@ type GatewayService struct { type GatewayServices []*GatewayService +func (g *GatewayService) Addresses(defaultHosts []string) []string { + if g.Port == 0 { + return nil + } + + hosts := g.Hosts + if len(hosts) == 0 { + hosts = defaultHosts + } + + var addresses []string + for _, h := range hosts { + addresses = append(addresses, fmt.Sprintf("%s:%d", h, g.Port)) + } + return addresses +} + func (g *GatewayService) IsSame(o *GatewayService) bool { return g.Gateway.Matches(&o.Gateway) && g.Service.Matches(&o.Service) && diff --git a/agent/structs/config_entry_gateways_test.go b/agent/structs/config_entry_gateways_test.go index 24fdc50d4..68cc2cf5f 100644 --- a/agent/structs/config_entry_gateways_test.go +++ b/agent/structs/config_entry_gateways_test.go @@ -562,3 +562,61 @@ func TestTerminatingConfigEntry_Validate(t *testing.T) { }) } } + +func TestGatewayService_Addresses(t *testing.T) { + cases := []struct { + name string + input GatewayService + argument []string + expected []string + }{ + { + name: "port is zero", + input: GatewayService{}, + expected: nil, + }, + { + name: "no hosts with empty array", + input: GatewayService{ + Port: 8080, + }, + expected: nil, + }, + { + name: "no hosts with default", + input: GatewayService{ + Port: 8080, + }, + argument: []string{ + "service.ingress.dc.domain", + "service.ingress.dc.alt.domain", + }, + expected: []string{ + "service.ingress.dc.domain:8080", + "service.ingress.dc.alt.domain:8080", + }, + }, + { + name: "user-defined hosts", + input: GatewayService{ + Port: 8080, + Hosts: []string{"*.test.example.com", "other.example.com"}, + }, + argument: []string{ + "service.ingress.dc.domain", + "service.ingress.alt.domain", + }, + expected: []string{"*.test.example.com:8080", "other.example.com:8080"}, + }, + } + + for _, test := range cases { + // We explicitly copy the variable for the range statement so that can run + // tests in parallel. + tc := test + t.Run(tc.name, func(t *testing.T) { + addresses := tc.input.Addresses(tc.argument) + require.ElementsMatch(t, tc.expected, addresses) + }) + } +} diff --git a/agent/ui_endpoint.go b/agent/ui_endpoint.go index 36297675b..202297d98 100644 --- a/agent/ui_endpoint.go +++ b/agent/ui_endpoint.go @@ -6,6 +6,7 @@ import ( "sort" "strings" + "github.com/hashicorp/consul/agent/config" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/api" ) @@ -16,7 +17,9 @@ import ( const metaExternalSource = "external-source" type GatewayConfig struct { - ListenerPort int + Addresses []string `json:",omitempty"` + // internal to track uniqueness + addressesSet map[string]struct{} } // ServiceSummary is used to summarize a service @@ -161,7 +164,7 @@ RPC: // Generate the summary // TODO (gateways) (freddy) Have Internal.ServiceDump return ServiceDump instead. Need to add bexpr filtering for type. - return summarizeServices(out.Nodes.ToServiceDump()), nil + return summarizeServices(out.Nodes.ToServiceDump(), s.agent.config), nil } // UIGatewayServices is used to query all the nodes for services associated with a gateway along with their gateway config @@ -195,10 +198,11 @@ RPC: } return nil, err } - return summarizeServices(out.Dump), nil + + return summarizeServices(out.Dump, s.agent.config), nil } -func summarizeServices(dump structs.ServiceDump) []*ServiceSummary { +func summarizeServices(dump structs.ServiceDump, cfg *config.RuntimeConfig) []*ServiceSummary { // Collect the summary information var services []structs.ServiceID summary := make(map[structs.ServiceID]*ServiceSummary) @@ -220,8 +224,9 @@ func summarizeServices(dump structs.ServiceDump) []*ServiceSummary { for _, csn := range dump { if csn.GatewayService != nil { - sum := getService(csn.GatewayService.Service.ToServiceID()) - sum.GatewayConfig.ListenerPort = csn.GatewayService.Port + gwsvc := csn.GatewayService + sum := getService(gwsvc.Service.ToServiceID()) + modifySummaryForGatewayService(cfg, sum, gwsvc) } // Will happen in cases where we only have the GatewayServices mapping @@ -299,3 +304,38 @@ func summarizeServices(dump structs.ServiceDump) []*ServiceSummary { } return output } + +func modifySummaryForGatewayService( + cfg *config.RuntimeConfig, + sum *ServiceSummary, + gwsvc *structs.GatewayService, +) { + var dnsAddresses []string + for _, domain := range []string{cfg.DNSDomain, cfg.DNSAltDomain} { + // If the domain is empty, do not use it to construct a valid DNS + // address + if domain == "" { + continue + } + dnsAddresses = append(dnsAddresses, serviceIngressDNSName( + gwsvc.Service.Name, + cfg.Datacenter, + domain, + &gwsvc.Service.EnterpriseMeta, + )) + } + + for _, addr := range gwsvc.Addresses(dnsAddresses) { + // check for duplicates, a service will have a ServiceInfo struct for + // every instance that is registered. + if _, ok := sum.GatewayConfig.addressesSet[addr]; !ok { + if sum.GatewayConfig.addressesSet == nil { + sum.GatewayConfig.addressesSet = make(map[string]struct{}) + } + sum.GatewayConfig.addressesSet[addr] = struct{}{} + sum.GatewayConfig.Addresses = append( + sum.GatewayConfig.Addresses, addr, + ) + } + } +} diff --git a/agent/ui_endpoint_test.go b/agent/ui_endpoint_test.go index 356fa2fcc..b139ba724 100644 --- a/agent/ui_endpoint_test.go +++ b/agent/ui_endpoint_test.go @@ -535,7 +535,7 @@ func TestUIGatewayServiceNodes_Terminating(t *testing.T) { func TestUIGatewayServiceNodes_Ingress(t *testing.T) { t.Parallel() - a := NewTestAgent(t, "") + a := NewTestAgent(t, `alt_domain = "alt.consul."`) defer a.Shutdown() // Register ingress gateway and a service that will be associated with it @@ -593,6 +593,18 @@ func TestUIGatewayServiceNodes_Ingress(t *testing.T) { } require.NoError(t, a.RPC("Catalog.Register", &arg, ®Output)) + // Set web protocol to http + svcDefaultsReq := structs.ConfigEntryRequest{ + Datacenter: "dc1", + Entry: &structs.ServiceConfigEntry{ + Name: "web", + Protocol: "http", + }, + } + var configOutput bool + require.NoError(t, a.RPC("ConfigEntry.Apply", &svcDefaultsReq, &configOutput)) + require.True(t, configOutput) + // Register ingress-gateway config entry, linking it to db and redis (does not exist) args := &structs.IngressGatewayConfigEntry{ Name: "ingress-gateway", @@ -609,13 +621,23 @@ func TestUIGatewayServiceNodes_Ingress(t *testing.T) { }, { Port: 8080, - Protocol: "tcp", + Protocol: "http", Services: []structs.IngressService{ { Name: "web", }, }, }, + { + Port: 8081, + Protocol: "http", + Services: []structs.IngressService{ + { + Name: "web", + Hosts: []string{"*.test.example.com"}, + }, + }, + }, }, } @@ -624,7 +646,6 @@ func TestUIGatewayServiceNodes_Ingress(t *testing.T) { Datacenter: "dc1", Entry: args, } - var configOutput bool require.NoError(t, a.RPC("ConfigEntry.Apply", &req, &configOutput)) require.True(t, configOutput) } @@ -636,11 +657,23 @@ func TestUIGatewayServiceNodes_Ingress(t *testing.T) { assert.Nil(t, err) assertIndex(t, resp) + // Construct expected addresses so that differences between OSS/Ent are handled by code + webDNS := serviceIngressDNSName("web", "dc1", "consul.", structs.DefaultEnterpriseMeta()) + webDNSAlt := serviceIngressDNSName("web", "dc1", "alt.consul.", structs.DefaultEnterpriseMeta()) + dbDNS := serviceIngressDNSName("db", "dc1", "consul.", structs.DefaultEnterpriseMeta()) + dbDNSAlt := serviceIngressDNSName("db", "dc1", "alt.consul.", structs.DefaultEnterpriseMeta()) + dump := obj.([]*ServiceSummary) expect := []*ServiceSummary{ { - Name: "web", - GatewayConfig: GatewayConfig{ListenerPort: 8080}, + Name: "web", + GatewayConfig: GatewayConfig{ + Addresses: []string{ + fmt.Sprintf("%s:8080", webDNS), + fmt.Sprintf("%s:8080", webDNSAlt), + "*.test.example.com:8081", + }, + }, EnterpriseMeta: *structs.DefaultEnterpriseMeta(), }, { @@ -651,9 +684,19 @@ func TestUIGatewayServiceNodes_Ingress(t *testing.T) { ChecksPassing: 1, ChecksWarning: 1, ChecksCritical: 0, - GatewayConfig: GatewayConfig{ListenerPort: 8888}, + GatewayConfig: GatewayConfig{ + Addresses: []string{ + fmt.Sprintf("%s:8888", dbDNS), + fmt.Sprintf("%s:8888", dbDNSAlt), + }, + }, EnterpriseMeta: *structs.DefaultEnterpriseMeta(), }, } + + // internal accounting that users don't see can be blown away + for _, sum := range dump { + sum.GatewayConfig.addressesSet = nil + } assert.ElementsMatch(t, expect, dump) } From 58eb3710fc2ad11af19c7e927f8adbad81e63071 Mon Sep 17 00:00:00 2001 From: Chris Piraino Date: Tue, 23 Jun 2020 13:48:30 -0400 Subject: [PATCH 2/2] remove obsolete comments about test parallelization --- agent/structs/config_entry_gateways_test.go | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/agent/structs/config_entry_gateways_test.go b/agent/structs/config_entry_gateways_test.go index 68cc2cf5f..e86d23302 100644 --- a/agent/structs/config_entry_gateways_test.go +++ b/agent/structs/config_entry_gateways_test.go @@ -77,10 +77,7 @@ func TestIngressConfigEntry_Normalize(t *testing.T) { }, } - for _, test := range cases { - // We explicitly copy the variable for the range statement so that can run - // tests in parallel. - tc := test + for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { err := tc.entry.Normalize() require.NoError(t, err) @@ -436,10 +433,7 @@ func TestIngressConfigEntry_Validate(t *testing.T) { }, } - for _, test := range cases { - // We explicitly copy the variable for the range statement so that can run - // tests in parallel. - tc := test + for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { err := tc.entry.Validate() if tc.expectErr != "" { @@ -546,10 +540,7 @@ func TestTerminatingConfigEntry_Validate(t *testing.T) { }, } - for _, test := range cases { - // We explicitly copy the variable for the range statement so that can run - // tests in parallel. - tc := test + for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { err := tc.entry.Validate() @@ -610,10 +601,7 @@ func TestGatewayService_Addresses(t *testing.T) { }, } - for _, test := range cases { - // We explicitly copy the variable for the range statement so that can run - // tests in parallel. - tc := test + for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { addresses := tc.input.Addresses(tc.argument) require.ElementsMatch(t, tc.expected, addresses)