From 3c037d9b961128d762d01e4a3219158118f6b411 Mon Sep 17 00:00:00 2001 From: Chris Piraino Date: Tue, 9 Jun 2020 11:44:31 -0500 Subject: [PATCH 1/5] Add ?ingress query parameter on /v1/health/connect Refactor boolean query parameter logic from ?passing value to re-use with ingress --- agent/health_endpoint.go | 63 +++++++++++++++------- agent/health_endpoint_test.go | 99 +++++++++++++++++++++++++++++++++++ 2 files changed, 143 insertions(+), 19 deletions(-) diff --git a/agent/health_endpoint.go b/agent/health_endpoint.go index 49027ffffe..7ece2b06e4 100644 --- a/agent/health_endpoint.go +++ b/agent/health_endpoint.go @@ -3,6 +3,7 @@ package agent import ( "fmt" "net/http" + "net/url" "strconv" "strings" @@ -186,6 +187,20 @@ func (s *HTTPServer) healthServiceNodes(resp http.ResponseWriter, req *http.Requ prefix = "/v1/health/connect/" } + // Check for ingress request only when requesting connect services + if connect { + ingress, err := getBoolQueryParam(params, "ingress") + if err != nil { + resp.WriteHeader(http.StatusBadRequest) + fmt.Fprint(resp, "Invalid value for ?ingress") + return nil, nil + } + if ingress { + args.Connect = false + args.Ingress = true + } + } + // Pull out the service name args.ServiceName = strings.TrimPrefix(req.URL.Path, prefix) if args.ServiceName == "" { @@ -224,26 +239,15 @@ func (s *HTTPServer) healthServiceNodes(resp http.ResponseWriter, req *http.Requ out.ConsistencyLevel = args.QueryOptions.ConsistencyLevel() // Filter to only passing if specified - if _, ok := params[api.HealthPassing]; ok { - val := params.Get(api.HealthPassing) - // Backwards-compat to allow users to specify ?passing without a value. This - // should be removed in Consul 0.10. - var filter bool - if val == "" { - filter = true - } else { - var err error - filter, err = strconv.ParseBool(val) - if err != nil { - resp.WriteHeader(http.StatusBadRequest) - fmt.Fprint(resp, "Invalid value for ?passing") - return nil, nil - } - } + filter, err := getBoolQueryParam(params, api.HealthPassing) + if err != nil { + resp.WriteHeader(http.StatusBadRequest) + fmt.Fprint(resp, "Invalid value for ?passing") + return nil, nil + } - if filter { - out.Nodes = filterNonPassing(out.Nodes) - } + if filter { + out.Nodes = filterNonPassing(out.Nodes) } // Translate addresses after filtering so we don't waste effort. @@ -273,6 +277,27 @@ func (s *HTTPServer) healthServiceNodes(resp http.ResponseWriter, req *http.Requ return out.Nodes, nil } +func getBoolQueryParam(params url.Values, key string) (bool, error) { + var param bool + if _, ok := params[key]; ok { + val := params.Get(key) + // Orginally a comment declared this check should be removed after Consul + // 0.10, to no longer support using ?passing without a value. However, I + // think this is a reasonable experience for a user and so am keeping it + // here. + if val == "" { + param = true + } else { + var err error + param, err = strconv.ParseBool(val) + if err != nil { + return false, err + } + } + } + return param, nil +} + // filterNonPassing is used to filter out any nodes that have check that are not passing func filterNonPassing(nodes structs.CheckServiceNodes) structs.CheckServiceNodes { n := len(nodes) diff --git a/agent/health_endpoint_test.go b/agent/health_endpoint_test.go index 59ef97679d..03fa9c7479 100644 --- a/agent/health_endpoint_test.go +++ b/agent/health_endpoint_test.go @@ -1139,6 +1139,105 @@ func TestHealthConnectServiceNodes(t *testing.T) { assert.Len(nodes[0].Checks, 0) } +func TestHealthConnectServiceNodes_Ingress(t *testing.T) { + t.Parallel() + + a := NewTestAgent(t, "") + defer a.Shutdown() + testrpc.WaitForLeader(t, a.RPC, "dc1") + + // Register gateway + gatewayArgs := structs.TestRegisterIngressGateway(t) + gatewayArgs.Service.Address = "127.0.0.27" + var out struct{} + require.NoError(t, a.RPC("Catalog.Register", gatewayArgs, &out)) + + args := structs.TestRegisterRequest(t) + require.NoError(t, a.RPC("Catalog.Register", args, &out)) + + // Associate service to gateway + cfgArgs := &structs.IngressGatewayConfigEntry{ + Name: "ingress-gateway", + Kind: structs.IngressGateway, + Listeners: []structs.IngressListener{ + { + Port: 8888, + Protocol: "tcp", + Services: []structs.IngressService{ + {Name: args.Service.Service}, + }, + }, + }, + } + + req := structs.ConfigEntryRequest{ + Op: structs.ConfigEntryUpsert, + Datacenter: "dc1", + Entry: cfgArgs, + } + var outB bool + require.Nil(t, a.RPC("ConfigEntry.Apply", req, &outB)) + require.True(t, outB) + + t.Run("no_query_value", func(t *testing.T) { + assert := assert.New(t) + req, _ := http.NewRequest("GET", fmt.Sprintf( + "/v1/health/connect/%s?ingress", args.Service.Service), nil) + resp := httptest.NewRecorder() + obj, err := a.srv.HealthConnectServiceNodes(resp, req) + assert.Nil(err) + assertIndex(t, resp) + + nodes := obj.(structs.CheckServiceNodes) + require.Len(t, nodes, 1) + require.Equal(t, structs.ServiceKindIngressGateway, nodes[0].Service.Kind) + require.Equal(t, gatewayArgs.Service.Address, nodes[0].Service.Address) + require.Equal(t, gatewayArgs.Service.Proxy, nodes[0].Service.Proxy) + }) + + t.Run("true_value", func(t *testing.T) { + assert := assert.New(t) + req, _ := http.NewRequest("GET", fmt.Sprintf( + "/v1/health/connect/%s?ingress=true", args.Service.Service), nil) + resp := httptest.NewRecorder() + obj, err := a.srv.HealthConnectServiceNodes(resp, req) + assert.Nil(err) + assertIndex(t, resp) + + nodes := obj.(structs.CheckServiceNodes) + require.Len(t, nodes, 1) + require.Equal(t, structs.ServiceKindIngressGateway, nodes[0].Service.Kind) + require.Equal(t, gatewayArgs.Service.Address, nodes[0].Service.Address) + require.Equal(t, gatewayArgs.Service.Proxy, nodes[0].Service.Proxy) + }) + + t.Run("false_value", func(t *testing.T) { + assert := assert.New(t) + req, _ := http.NewRequest("GET", fmt.Sprintf( + "/v1/health/connect/%s?ingress=false", args.Service.Service), nil) + resp := httptest.NewRecorder() + obj, err := a.srv.HealthConnectServiceNodes(resp, req) + assert.Nil(err) + assertIndex(t, resp) + + nodes := obj.(structs.CheckServiceNodes) + require.Len(t, nodes, 0) + }) + + t.Run("invalid_value", func(t *testing.T) { + assert := assert.New(t) + req, _ := http.NewRequest("GET", fmt.Sprintf( + "/v1/health/connect/%s?ingress=notabool", args.Service.Service), nil) + resp := httptest.NewRecorder() + _, err := a.srv.HealthConnectServiceNodes(resp, req) + assert.Equal(400, resp.Code) + + body, err := ioutil.ReadAll(resp.Body) + assert.Nil(err) + assert.True(bytes.Contains(body, []byte("Invalid value for ?ingress"))) + }) +} + func TestHealthConnectServiceNodes_Filter(t *testing.T) { t.Parallel() From 4837069fe08823d52c094e3db9e1d42dd3634b1b Mon Sep 17 00:00:00 2001 From: Chris Piraino Date: Tue, 9 Jun 2020 12:09:59 -0500 Subject: [PATCH 2/5] api: update api module with health.Ingress() method --- api/health.go | 22 +++++++++++----- api/health_test.go | 63 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 6 deletions(-) diff --git a/api/health.go b/api/health.go index 052c82902f..37d25d000b 100644 --- a/api/health.go +++ b/api/health.go @@ -269,11 +269,11 @@ func (h *Health) Service(service, tag string, passingOnly bool, q *QueryOptions) if tag != "" { tags = []string{tag} } - return h.service(service, tags, passingOnly, q, false) + return h.service(service, tags, passingOnly, q, false, false) } func (h *Health) ServiceMultipleTags(service string, tags []string, passingOnly bool, q *QueryOptions) ([]*ServiceEntry, *QueryMeta, error) { - return h.service(service, tags, passingOnly, q, false) + return h.service(service, tags, passingOnly, q, false, false) } // Connect is equivalent to Service except that it will only return services @@ -286,16 +286,23 @@ func (h *Health) Connect(service, tag string, passingOnly bool, q *QueryOptions) if tag != "" { tags = []string{tag} } - return h.service(service, tags, passingOnly, q, true) + return h.service(service, tags, passingOnly, q, true, false) } func (h *Health) ConnectMultipleTags(service string, tags []string, passingOnly bool, q *QueryOptions) ([]*ServiceEntry, *QueryMeta, error) { - return h.service(service, tags, passingOnly, q, true) + return h.service(service, tags, passingOnly, q, true, false) } -func (h *Health) service(service string, tags []string, passingOnly bool, q *QueryOptions, connect bool) ([]*ServiceEntry, *QueryMeta, error) { +// Ingress is equivalent to Connect except that it will only return associated +// ingress gateways for the requested service. +func (h *Health) Ingress(service string, passingOnly bool, q *QueryOptions) ([]*ServiceEntry, *QueryMeta, error) { + var tags []string + return h.service(service, tags, passingOnly, q, false, true) +} + +func (h *Health) service(service string, tags []string, passingOnly bool, q *QueryOptions, connect, ingress bool) ([]*ServiceEntry, *QueryMeta, error) { path := "/v1/health/service/" + service - if connect { + if connect || ingress { path = "/v1/health/connect/" + service } r := h.c.newRequest("GET", path) @@ -308,6 +315,9 @@ func (h *Health) service(service string, tags []string, passingOnly bool, q *Que if passingOnly { r.params.Set(HealthPassing, "1") } + if ingress { + r.params.Set("ingress", "1") + } rtt, resp, err := requireOK(h.c.doRequest(r)) if err != nil { return nil, nil, err diff --git a/api/health_test.go b/api/health_test.go index 858d63bed9..e097ff2d6a 100644 --- a/api/health_test.go +++ b/api/health_test.go @@ -546,6 +546,69 @@ func TestAPI_HealthConnect_Filter(t *testing.T) { require.Len(t, services, 1) } +func TestAPI_HealthConnect_Ingress(t *testing.T) { + t.Parallel() + c, s := makeClient(t) + defer s.Stop() + + agent := c.Agent() + health := c.Health() + + s.WaitForSerfCheck(t) + + // Make a service with a proxy + reg := &AgentServiceRegistration{ + Name: "foo", + Port: 8000, + } + err := agent.ServiceRegister(reg) + require.NoError(t, err) + defer agent.ServiceDeregister("foo") + + // Register the gateway + gatewayReg := &AgentServiceRegistration{ + Name: "foo-gateway", + Port: 8001, + Kind: ServiceKindIngressGateway, + } + err = agent.ServiceRegister(gatewayReg) + require.NoError(t, err) + defer agent.ServiceDeregister("foo-gateway") + + // Associate service and gateway + gatewayConfig := &IngressGatewayConfigEntry{ + Kind: IngressGateway, + Name: "foo-gateway", + Listeners: []IngressListener{ + { + Port: 2222, + Protocol: "tcp", + Services: []IngressService{ + { + Name: "foo", + }, + }, + }, + }, + } + _, wm, err := c.ConfigEntries().Set(gatewayConfig, nil) + require.NoError(t, err) + require.NotNil(t, wm) + + retry.Run(t, func(r *retry.R) { + services, meta, err := health.Ingress("foo", true, nil) + require.NoError(r, err) + + require.NotZero(r, meta.LastIndex) + + // Should be exactly 1 service - the original shouldn't show up as a connect + // endpoint, only it's proxy. + require.Len(r, services, 1) + require.Equal(r, services[0].Node.Datacenter, "dc1") + require.Equal(r, services[0].Service.Service, gatewayReg.Name) + }) +} + func TestAPI_HealthState(t *testing.T) { t.Parallel() c, s := makeClient(t) From ca41f80493b673749c799ed6a97c4560f8f2e8fc Mon Sep 17 00:00:00 2001 From: Chris Piraino Date: Tue, 9 Jun 2020 14:45:21 -0500 Subject: [PATCH 3/5] Set connect or ingress boolean after checking for query param --- agent/health_endpoint.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/agent/health_endpoint.go b/agent/health_endpoint.go index 7ece2b06e4..9573ae691b 100644 --- a/agent/health_endpoint.go +++ b/agent/health_endpoint.go @@ -164,7 +164,7 @@ func (s *HTTPServer) HealthServiceNodes(resp http.ResponseWriter, req *http.Requ func (s *HTTPServer) healthServiceNodes(resp http.ResponseWriter, req *http.Request, connect bool) (interface{}, error) { // Set default DC - args := structs.ServiceSpecificRequest{Connect: connect} + args := structs.ServiceSpecificRequest{} if err := s.parseEntMetaNoWildcard(req, &args.EnterpriseMeta); err != nil { return nil, err } @@ -185,19 +185,19 @@ func (s *HTTPServer) healthServiceNodes(resp http.ResponseWriter, req *http.Requ prefix := "/v1/health/service/" if connect { prefix = "/v1/health/connect/" - } - // Check for ingress request only when requesting connect services - if connect { + // Check for ingress request only when requesting connect services ingress, err := getBoolQueryParam(params, "ingress") if err != nil { resp.WriteHeader(http.StatusBadRequest) fmt.Fprint(resp, "Invalid value for ?ingress") return nil, nil } + if ingress { - args.Connect = false args.Ingress = true + } else { + args.Connect = true } } From 5e0cd7ede56506fdf651abba9ec4796b0144b678 Mon Sep 17 00:00:00 2001 From: Chris Piraino Date: Tue, 9 Jun 2020 14:45:57 -0500 Subject: [PATCH 4/5] Remove unnecessary defer from api.health_test.go We do not need to deregister services because every test gets its own instance of the client agent and the tmp directories are all deleted at the end. --- api/health_test.go | 9 --------- 1 file changed, 9 deletions(-) diff --git a/api/health_test.go b/api/health_test.go index e097ff2d6a..b0670c58c1 100644 --- a/api/health_test.go +++ b/api/health_test.go @@ -211,7 +211,6 @@ func TestAPI_HealthChecks(t *testing.T) { if err := agent.ServiceRegister(reg); err != nil { t.Fatalf("err: %v", err) } - defer agent.ServiceDeregister("foo") retry.Run(t, func(r *retry.R) { checks := HealthChecks{ @@ -264,7 +263,6 @@ func TestAPI_HealthChecks_NodeMetaFilter(t *testing.T) { if err := agent.ServiceRegister(reg); err != nil { t.Fatalf("err: %v", err) } - defer agent.ServiceDeregister("foo") retry.Run(t, func(r *retry.R) { checks, meta, err := health.Checks("foo", &QueryOptions{NodeMeta: meta}) @@ -354,7 +352,6 @@ func TestAPI_HealthService_SingleTag(t *testing.T) { }, } require.NoError(t, agent.ServiceRegister(reg)) - defer agent.ServiceDeregister("foo1") retry.Run(t, func(r *retry.R) { services, meta, err := health.Service("foo", "bar", true, nil) require.NoError(r, err) @@ -390,7 +387,6 @@ func TestAPI_HealthService_MultipleTags(t *testing.T) { }, } require.NoError(t, agent.ServiceRegister(reg)) - defer agent.ServiceDeregister("foo1") reg2 := &AgentServiceRegistration{ Name: "foo", @@ -402,7 +398,6 @@ func TestAPI_HealthService_MultipleTags(t *testing.T) { }, } require.NoError(t, agent.ServiceRegister(reg2)) - defer agent.ServiceDeregister("foo2") // Test searching with one tag (two results) retry.Run(t, func(r *retry.R) { @@ -488,7 +483,6 @@ func TestAPI_HealthConnect(t *testing.T) { } err := agent.ServiceRegister(reg) require.NoError(t, err) - defer agent.ServiceDeregister("foo") // Register the proxy proxyReg := &AgentServiceRegistration{ @@ -501,7 +495,6 @@ func TestAPI_HealthConnect(t *testing.T) { } err = agent.ServiceRegister(proxyReg) require.NoError(t, err) - defer agent.ServiceDeregister("foo-proxy") retry.Run(t, func(r *retry.R) { services, meta, err := health.Connect("foo", "", true, nil) @@ -563,7 +556,6 @@ func TestAPI_HealthConnect_Ingress(t *testing.T) { } err := agent.ServiceRegister(reg) require.NoError(t, err) - defer agent.ServiceDeregister("foo") // Register the gateway gatewayReg := &AgentServiceRegistration{ @@ -573,7 +565,6 @@ func TestAPI_HealthConnect_Ingress(t *testing.T) { } err = agent.ServiceRegister(gatewayReg) require.NoError(t, err) - defer agent.ServiceDeregister("foo-gateway") // Associate service and gateway gatewayConfig := &IngressGatewayConfigEntry{ From c1d329c5ddb7b617286f7a6d3734aa4bb41cf7b2 Mon Sep 17 00:00:00 2001 From: Chris Piraino Date: Tue, 9 Jun 2020 14:58:30 -0500 Subject: [PATCH 5/5] Remove TODO note about ingress API, it is done! --- agent/structs/structs.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/agent/structs/structs.go b/agent/structs/structs.go index 23a392a08f..8142b024b0 100644 --- a/agent/structs/structs.go +++ b/agent/structs/structs.go @@ -513,9 +513,6 @@ type ServiceSpecificRequest struct { // Connect if true will only search for Connect-compatible services. Connect bool - // TODO(ingress): Add corresponding API changes after figuring out what the - // HTTP endpoint looks like - // Ingress if true will only search for Ingress gateways for the given service. Ingress bool