Merge pull request #13906 from skpratt/validate-port-agent-split

Separate port and socket path validation for local agent

agent/agent_endpoint.go

@@ -1123,8 +1123,8 @@ func (s *HTTPHandlers) AgentRegisterService(resp http.ResponseWriter, req *http.
 		return nil, HTTPError{StatusCode: http.StatusBadRequest, Reason: fmt.Sprintf("Invalid Service Meta: %v", err)}
 	}
 
-	// Run validation. This is the same validation that would happen on
+	// Run validation. This same validation would happen on the catalog endpoint,
-	// the catalog endpoint so it helps ensure the sync will work properly.
+	// so it helps ensure the sync will work properly.
 	if err := ns.Validate(); err != nil {
 		return nil, HTTPError{StatusCode: http.StatusBadRequest, Reason: fmt.Sprintf("Validation failed: %v", err.Error())}
 	}
@@ -1164,7 +1164,7 @@ func (s *HTTPHandlers) AgentRegisterService(resp http.ResponseWriter, req *http.
 		return nil, HTTPError{StatusCode: http.StatusBadRequest, Reason: fmt.Sprintf("Invalid SidecarService: %s", err)}
 	}
 	if sidecar != nil {
-		if err := sidecar.Validate(); err != nil {
+		if err := sidecar.ValidateForAgent(); err != nil {
 			return nil, HTTPError{StatusCode: http.StatusBadRequest, Reason: fmt.Sprintf("Failed Validation: %v", err.Error())}
 		}
 		// Make sure we are allowed to register the sidecar using the token

agent/sidecar_service_test.go

@@ -339,7 +339,7 @@ func TestAgent_sidecarServiceFromNodeService(t *testing.T) {
 			}
 
 			ns := tt.sd.NodeService()
-			err := ns.Validate()
+			err := ns.ValidateForAgent()
 			require.NoError(t, err, "Invalid test case - NodeService must validate")
 
 			gotNS, gotChecks, gotToken, err := a.sidecarServiceFromNodeService(ns, tt.token)

agent/structs/structs.go

@@ -1413,6 +1413,27 @@ func (s *NodeService) IsGateway() bool {
 func (s *NodeService) Validate() error {
 	var result error
 
+	if s.Kind == ServiceKindConnectProxy {
+		if s.Port == 0 && s.SocketPath == "" {
+			result = multierror.Append(result, fmt.Errorf("Port or SocketPath must be set for a %s", s.Kind))
+		}
+	}
+
+	commonValidation := s.ValidateForAgent()
+	if commonValidation != nil {
+		result = multierror.Append(result, commonValidation)
+	}
+
+	return result
+}
+
+// ValidateForAgent does a subset validation, with the assumption that a local agent can assist with missing values.
+//
+// I.e. in the catalog case, a local agent cannot be assumed to facilitate auto-assignment of port or socket path,
+// so additional checks are needed.
+func (s *NodeService) ValidateForAgent() error {
+	var result error
+
 	// TODO(partitions): remember to double check that this doesn't cross partition boundaries
 
 	// ConnectProxy validation
@@ -1428,10 +1449,6 @@ func (s *NodeService) Validate() error {
 					"services"))
 		}
 
-		if s.Port == 0 && s.SocketPath == "" {
-			result = multierror.Append(result, fmt.Errorf("Port or SocketPath must be set for a %s", s.Kind))
-		}
-
 		if s.Connect.Native {
 			result = multierror.Append(result, fmt.Errorf(
 				"A Proxy cannot also be Connect Native, only typical services"))

agent/structs/structs_test.go

@@ -1157,6 +1157,16 @@ func TestStructs_NodeService_ValidateConnectProxy(t *testing.T) {
 	}
 }
 
+func TestStructs_NodeService_ValidateConnectProxyWithAgentAutoAssign(t *testing.T) {
+	t.Run("connect-proxy: no port set", func(t *testing.T) {
+		ns := TestNodeServiceProxy(t)
+		ns.Port = 0
+
+		err := ns.ValidateForAgent()
+		assert.NoError(t, err)
+	})
+}
+
 func TestStructs_NodeService_ValidateConnectProxy_In_Partition(t *testing.T) {
 	cases := []struct {
 		Name   string