Move ingress param to a new endpoint (#8081)

In discussion with team, it was pointed out that query parameters tend
to be filter mechanism, and that semantically the "/v1/health/connect"
endpoint should return "all healthy connect-enabled endpoints (e.g.
could be side car proxies or native instances) for this service so I can
connect with mTLS".

That does not fit an ingress gateway, so we remove the query parameter
and add a new endpoint "/v1/health/ingress" that semantically means
"all the healthy ingress gateway instances that I can connect to
to access this connect-enabled service without mTLS"

agent/health_endpoint.go

@@ -12,6 +12,12 @@ import (
 	"github.com/hashicorp/consul/api"
 )
 
+const (
+	serviceHealth = "service"
+	connectHealth = "connect"
+	ingressHealth = "ingress"
+)
+
 func (s *HTTPServer) HealthChecksInState(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
 	// Set default DC
 	args := structs.ChecksInStateRequest{}
@@ -154,15 +160,26 @@ RETRY_ONCE:
 	return out.HealthChecks, nil
 }
 
+// HealthIngressServiceNodes should return "all the healthy ingress gateway instances
+// that I can use to access this connect-enabled service without mTLS".
+func (s *HTTPServer) HealthIngressServiceNodes(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
+	return s.healthServiceNodes(resp, req, ingressHealth)
+}
+
+// HealthConnectServiceNodes should return "all healthy connect-enabled
+// endpoints (e.g. could be side car proxies or native instances) for this
+// service so I can connect with mTLS".
 func (s *HTTPServer) HealthConnectServiceNodes(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
-	return s.healthServiceNodes(resp, req, true)
+	return s.healthServiceNodes(resp, req, connectHealth)
 }
 
+// HealthServiceNodes should return "all the healthy instances of this service
+// registered so I can connect directly to them".
 func (s *HTTPServer) HealthServiceNodes(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
-	return s.healthServiceNodes(resp, req, false)
+	return s.healthServiceNodes(resp, req, serviceHealth)
 }
 
-func (s *HTTPServer) healthServiceNodes(resp http.ResponseWriter, req *http.Request, connect bool) (interface{}, error) {
+func (s *HTTPServer) healthServiceNodes(resp http.ResponseWriter, req *http.Request, healthType string) (interface{}, error) {
 	// Set default DC
 	args := structs.ServiceSpecificRequest{}
 	if err := s.parseEntMetaNoWildcard(req, &args.EnterpriseMeta); err != nil {
@@ -182,23 +199,17 @@ func (s *HTTPServer) healthServiceNodes(resp http.ResponseWriter, req *http.Requ
 	}
 
 	// Determine the prefix
-	prefix := "/v1/health/service/"
+	var prefix string
-	if connect {
+	switch healthType {
+	case connectHealth:
 		prefix = "/v1/health/connect/"
-
+		args.Connect = true
-		// Check for ingress request only when requesting connect services
+	case ingressHealth:
-		ingress, err := getBoolQueryParam(params, "ingress")
+		prefix = "/v1/health/ingress/"
-		if err != nil {
+		args.Ingress = true
-			resp.WriteHeader(http.StatusBadRequest)
+	default:
-			fmt.Fprint(resp, "Invalid value for ?ingress")
+		// serviceHealth is the default type
-			return nil, nil
+		prefix = "/v1/health/service/"
-		}
-
-		if ingress {
-			args.Ingress = true
-		} else {
-			args.Connect = true
-		}
 	}
 
 	// Pull out the service name

agent/health_endpoint_test.go

@@ -1139,7 +1139,7 @@ func TestHealthConnectServiceNodes(t *testing.T) {
 	assert.Len(nodes[0].Checks, 0)
 }
 
-func TestHealthConnectServiceNodes_Ingress(t *testing.T) {
+func TestHealthIngressServiceNodes(t *testing.T) {
 	t.Parallel()
 
 	a := NewTestAgent(t, "")
@@ -1179,12 +1179,12 @@ func TestHealthConnectServiceNodes_Ingress(t *testing.T) {
 	require.Nil(t, a.RPC("ConfigEntry.Apply", req, &outB))
 	require.True(t, outB)
 
-	t.Run("no_query_value", func(t *testing.T) {
+	t.Run("associated service", func(t *testing.T) {
 		assert := assert.New(t)
 		req, _ := http.NewRequest("GET", fmt.Sprintf(
-			"/v1/health/connect/%s?ingress", args.Service.Service), nil)
+			"/v1/health/ingress/%s", args.Service.Service), nil)
 		resp := httptest.NewRecorder()
-		obj, err := a.srv.HealthConnectServiceNodes(resp, req)
+		obj, err := a.srv.HealthIngressServiceNodes(resp, req)
 		assert.Nil(err)
 		assertIndex(t, resp)
 
@@ -1195,47 +1195,18 @@ func TestHealthConnectServiceNodes_Ingress(t *testing.T) {
 		require.Equal(t, gatewayArgs.Service.Proxy, nodes[0].Service.Proxy)
 	})
 
-	t.Run("true_value", func(t *testing.T) {
+	t.Run("non-associated service", func(t *testing.T) {
 		assert := assert.New(t)
-		req, _ := http.NewRequest("GET", fmt.Sprintf(
+		req, _ := http.NewRequest("GET",
-			"/v1/health/connect/%s?ingress=true", args.Service.Service), nil)
+			"/v1/health/connect/notexist", nil)
 		resp := httptest.NewRecorder()
-		obj, err := a.srv.HealthConnectServiceNodes(resp, req)
+		obj, err := a.srv.HealthIngressServiceNodes(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) {

agent/http_register.go

@@ -91,6 +91,7 @@ func init() {
 	registerEndpoint("/v1/health/state/", []string{"GET"}, (*HTTPServer).HealthChecksInState)
 	registerEndpoint("/v1/health/service/", []string{"GET"}, (*HTTPServer).HealthServiceNodes)
 	registerEndpoint("/v1/health/connect/", []string{"GET"}, (*HTTPServer).HealthConnectServiceNodes)
+	registerEndpoint("/v1/health/ingress/", []string{"GET"}, (*HTTPServer).HealthIngressServiceNodes)
 	registerEndpoint("/v1/internal/ui/nodes", []string{"GET"}, (*HTTPServer).UINodes)
 	registerEndpoint("/v1/internal/ui/node/", []string{"GET"}, (*HTTPServer).UINodeInfo)
 	registerEndpoint("/v1/internal/ui/services", []string{"GET"}, (*HTTPServer).UIServices)

api/health.go

@@ -17,6 +17,12 @@ const (
 	HealthMaint    = "maintenance"
 )
 
+const (
+	serviceHealth = "service"
+	connectHealth = "connect"
+	ingressHealth = "ingress"
+)
+
 const (
 	// NodeMaint is the special key set by a node in maintenance mode.
 	NodeMaint = "_node_maintenance"
@@ -269,11 +275,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, false)
+	return h.service(service, tags, passingOnly, q, serviceHealth)
 }
 
 func (h *Health) ServiceMultipleTags(service string, tags []string, passingOnly bool, q *QueryOptions) ([]*ServiceEntry, *QueryMeta, error) {
-	return h.service(service, tags, passingOnly, q, false, false)
+	return h.service(service, tags, passingOnly, q, serviceHealth)
 }
 
 // Connect is equivalent to Service except that it will only return services
@@ -286,25 +292,31 @@ func (h *Health) Connect(service, tag string, passingOnly bool, q *QueryOptions)
 	if tag != "" {
 		tags = []string{tag}
 	}
-	return h.service(service, tags, passingOnly, q, true, false)
+	return h.service(service, tags, passingOnly, q, connectHealth)
 }
 
 func (h *Health) ConnectMultipleTags(service string, tags []string, passingOnly bool, q *QueryOptions) ([]*ServiceEntry, *QueryMeta, error) {
-	return h.service(service, tags, passingOnly, q, true, false)
+	return h.service(service, tags, passingOnly, q, connectHealth)
 }
 
 // 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)
+	return h.service(service, tags, passingOnly, q, ingressHealth)
 }
 
-func (h *Health) service(service string, tags []string, passingOnly bool, q *QueryOptions, connect, ingress bool) ([]*ServiceEntry, *QueryMeta, error) {
+func (h *Health) service(service string, tags []string, passingOnly bool, q *QueryOptions, healthType string) ([]*ServiceEntry, *QueryMeta, error) {
-	path := "/v1/health/service/" + service
+	var path string
-	if connect || ingress {
+	switch healthType {
+	case connectHealth:
 		path = "/v1/health/connect/" + service
+	case ingressHealth:
+		path = "/v1/health/ingress/" + service
+	default:
+		path = "/v1/health/service/" + service
 	}
+
 	r := h.c.newRequest("GET", path)
 	r.setQueryOptions(q)
 	if len(tags) > 0 {
@@ -315,9 +327,6 @@ 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

api/health_test.go

@@ -539,7 +539,7 @@ func TestAPI_HealthConnect_Filter(t *testing.T) {
 	require.Len(t, services, 1)
 }
 
-func TestAPI_HealthConnect_Ingress(t *testing.T) {
+func TestAPI_HealthIngress(t *testing.T) {
 	t.Parallel()
 	c, s := makeClient(t)
 	defer s.Stop()