From 662f38c62521616a78e973441a574580f913f491 Mon Sep 17 00:00:00 2001
From: Mitchell Hashimoto <mitchell.hashimoto@gmail.com>
Date: Fri, 4 May 2018 14:10:03 -0700
Subject: [PATCH] agent/structs: validate service definitions, port required
 for proxy

---
 agent/config/builder.go                     |  7 +++
 agent/structs/service_definition.go         | 31 ++++++++++++
 agent/structs/service_definition_test.go    | 54 +++++++++++++++++++++
 agent/structs/testing_catalog.go            |  8 +++
 agent/structs/testing_service_definition.go |  7 +++
 5 files changed, 107 insertions(+)

diff --git a/agent/config/builder.go b/agent/config/builder.go
index cd851aaf3..88f65ec76 100644
--- a/agent/config/builder.go
+++ b/agent/config/builder.go
@@ -912,6 +912,13 @@ func (b *Builder) Validate(rt RuntimeConfig) error {
 		return b.err
 	}
 
+	// Check for errors in the service definitions
+	for _, s := range rt.Services {
+		if err := s.Validate(); err != nil {
+			return fmt.Errorf("service %q: %s", s.Name, err)
+		}
+	}
+
 	// ----------------------------------------------------------------
 	// warnings
 	//
diff --git a/agent/structs/service_definition.go b/agent/structs/service_definition.go
index be69a7b57..506015649 100644
--- a/agent/structs/service_definition.go
+++ b/agent/structs/service_definition.go
@@ -1,5 +1,11 @@
 package structs
 
+import (
+	"fmt"
+
+	"github.com/hashicorp/go-multierror"
+)
+
 // ServiceDefinition is used to JSON decode the Service definitions. For
 // documentation on specific fields see NodeService which is better documented.
 type ServiceDefinition struct {
@@ -68,6 +74,31 @@ func (s *ServiceDefinition) ConnectManagedProxy() (*ConnectManagedProxy, error)
 	return p, nil
 }
 
+// Validate validates the service definition. This also calls the underlying
+// Validate method on the NodeService.
+//
+// NOTE(mitchellh): This currently only validates fields related to Connect
+// and is incomplete with regards to other fields.
+func (s *ServiceDefinition) Validate() error {
+	var result error
+
+	if s.Kind == ServiceKindTypical {
+		if s.Connect != nil && s.Connect.Proxy != nil {
+			if s.Port == 0 {
+				result = multierror.Append(result, fmt.Errorf(
+					"Services with a Connect managed proxy must have a port set"))
+			}
+		}
+	}
+
+	// Validate the NodeService which covers a lot
+	if err := s.NodeService().Validate(); err != nil {
+		result = multierror.Append(result, err)
+	}
+
+	return result
+}
+
 func (s *ServiceDefinition) CheckTypes() (checks CheckTypes, err error) {
 	if !s.Check.Empty() {
 		err := s.Check.Validate()
diff --git a/agent/structs/service_definition_test.go b/agent/structs/service_definition_test.go
index d3bab4a08..c73cff217 100644
--- a/agent/structs/service_definition_test.go
+++ b/agent/structs/service_definition_test.go
@@ -2,10 +2,12 @@ package structs
 
 import (
 	"fmt"
+	"strings"
 	"testing"
 	"time"
 
 	"github.com/pascaldekloe/goe/verify"
+	"github.com/stretchr/testify/require"
 )
 
 func TestAgentStructs_CheckTypes(t *testing.T) {
@@ -54,3 +56,55 @@ func TestAgentStructs_CheckTypes(t *testing.T) {
 		}
 	}
 }
+
+func TestServiceDefinitionValidate(t *testing.T) {
+	cases := []struct {
+		Name   string
+		Modify func(*ServiceDefinition)
+		Err    string
+	}{
+		{
+			"valid",
+			func(x *ServiceDefinition) {},
+			"",
+		},
+
+		{
+			"managed proxy with a port set",
+			func(x *ServiceDefinition) {
+				x.Port = 8080
+				x.Connect = &ServiceDefinitionConnect{
+					Proxy: &ServiceDefinitionConnectProxy{},
+				}
+			},
+			"",
+		},
+
+		{
+			"managed proxy with no port set",
+			func(x *ServiceDefinition) {
+				x.Connect = &ServiceDefinitionConnect{
+					Proxy: &ServiceDefinitionConnectProxy{},
+				}
+			},
+			"must have a port",
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.Name, func(t *testing.T) {
+			require := require.New(t)
+			service := TestServiceDefinition(t)
+			tc.Modify(service)
+
+			err := service.Validate()
+			t.Logf("error: %s", err)
+			require.Equal(err != nil, tc.Err != "")
+			if err == nil {
+				return
+			}
+
+			require.Contains(strings.ToLower(err.Error()), strings.ToLower(tc.Err))
+		})
+	}
+}
diff --git a/agent/structs/testing_catalog.go b/agent/structs/testing_catalog.go
index 1394b7081..a274ced77 100644
--- a/agent/structs/testing_catalog.go
+++ b/agent/structs/testing_catalog.go
@@ -29,6 +29,14 @@ func TestRegisterRequestProxy(t testing.T) *RegisterRequest {
 	}
 }
 
+// TestNodeService returns a *NodeService representing a valid regular service.
+func TestNodeService(t testing.T) *NodeService {
+	return &NodeService{
+		Kind:    ServiceKindTypical,
+		Service: "web",
+	}
+}
+
 // TestNodeServiceProxy returns a *NodeService representing a valid
 // Connect proxy.
 func TestNodeServiceProxy(t testing.T) *NodeService {
diff --git a/agent/structs/testing_service_definition.go b/agent/structs/testing_service_definition.go
index b14e1e2ff..370458371 100644
--- a/agent/structs/testing_service_definition.go
+++ b/agent/structs/testing_service_definition.go
@@ -4,6 +4,13 @@ import (
 	"github.com/mitchellh/go-testing-interface"
 )
 
+// TestServiceDefinition returns a ServiceDefinition for a typical service.
+func TestServiceDefinition(t testing.T) *ServiceDefinition {
+	return &ServiceDefinition{
+		Name: "db",
+	}
+}
+
 // TestServiceDefinitionProxy returns a ServiceDefinition for a proxy.
 func TestServiceDefinitionProxy(t testing.T) *ServiceDefinition {
 	return &ServiceDefinition{