From 5b88aae3b4119ccf20674d57907254cecdfaca65 Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" <4903+rboyer@users.noreply.github.com> Date: Tue, 22 Aug 2023 15:16:55 -0500 Subject: [PATCH] catalog: validating Protocol and Health enums on Service, Workload, and ServiceEndpoints (#18554) --- internal/catalog/internal/types/service.go | 14 ++++++- .../internal/types/service_endpoints.go | 23 ++++++++++- .../internal/types/service_endpoints_test.go | 35 ++++++++++++++++- .../catalog/internal/types/service_test.go | 38 +++++++++++++++++-- internal/catalog/internal/types/validators.go | 32 +++++++++++++++- .../catalog/internal/types/validators_test.go | 5 ++- internal/catalog/internal/types/workload.go | 14 ++++++- .../catalog/internal/types/workload_test.go | 31 +++++++++++++-- 8 files changed, 177 insertions(+), 15 deletions(-) diff --git a/internal/catalog/internal/types/service.go b/internal/catalog/internal/types/service.go index 9f933c53b9..91c0c732e2 100644 --- a/internal/catalog/internal/types/service.go +++ b/internal/catalog/internal/types/service.go @@ -6,10 +6,11 @@ package types import ( "math" + "github.com/hashicorp/go-multierror" + "github.com/hashicorp/consul/internal/resource" pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v1alpha1" "github.com/hashicorp/consul/proto-public/pbresource" - "github.com/hashicorp/go-multierror" ) const ( @@ -87,6 +88,17 @@ func ValidateService(res *pbresource.Resource) error { }) } + if protoErr := validateProtocol(port.Protocol); protoErr != nil { + err = multierror.Append(err, resource.ErrInvalidListElement{ + Name: "ports", + Index: idx, + Wrapped: resource.ErrInvalidField{ + Name: "protocol", + Wrapped: protoErr, + }, + }) + } + // validate the virtual port is within the allowed range - 0 is allowed // to signify that no virtual port should be used and the port will not // be available for transparent proxying within the mesh. diff --git a/internal/catalog/internal/types/service_endpoints.go b/internal/catalog/internal/types/service_endpoints.go index 294ca09fab..a8a591ef26 100644 --- a/internal/catalog/internal/types/service_endpoints.go +++ b/internal/catalog/internal/types/service_endpoints.go @@ -6,10 +6,11 @@ package types import ( "math" + "github.com/hashicorp/go-multierror" + "github.com/hashicorp/consul/internal/resource" pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v1alpha1" "github.com/hashicorp/consul/proto-public/pbresource" - "github.com/hashicorp/go-multierror" ) const ( @@ -128,6 +129,13 @@ func validateEndpoint(endpoint *pbcatalog.Endpoint, res *pbresource.Resource) er }) } + if healthErr := validateHealth(endpoint.HealthStatus); healthErr != nil { + err = multierror.Append(err, resource.ErrInvalidField{ + Name: "health_status", + Wrapped: healthErr, + }) + } + // Validate the endpoints ports for portName, port := range endpoint.Ports { // Port names must be DNS labels @@ -139,6 +147,17 @@ func validateEndpoint(endpoint *pbcatalog.Endpoint, res *pbresource.Resource) er }) } + if protoErr := validateProtocol(port.Protocol); protoErr != nil { + err = multierror.Append(err, resource.ErrInvalidMapValue{ + Map: "ports", + Key: portName, + Wrapped: resource.ErrInvalidField{ + Name: "protocol", + Wrapped: protoErr, + }, + }) + } + // As the physical port is the real port the endpoint will be bound to // it must be in the standard 1-65535 range. if port.Port < 1 || port.Port > math.MaxUint16 { @@ -146,7 +165,7 @@ func validateEndpoint(endpoint *pbcatalog.Endpoint, res *pbresource.Resource) er Map: "ports", Key: portName, Wrapped: resource.ErrInvalidField{ - Name: "phsical_port", + Name: "physical_port", Wrapped: errInvalidPhysicalPort, }, }) diff --git a/internal/catalog/internal/types/service_endpoints_test.go b/internal/catalog/internal/types/service_endpoints_test.go index ac835c9c70..9b45d7d2ca 100644 --- a/internal/catalog/internal/types/service_endpoints_test.go +++ b/internal/catalog/internal/types/service_endpoints_test.go @@ -6,11 +6,12 @@ package types import ( "testing" + "github.com/stretchr/testify/require" + "github.com/hashicorp/consul/internal/resource" rtest "github.com/hashicorp/consul/internal/resource/resourcetest" pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v1alpha1" "github.com/hashicorp/consul/proto-public/pbresource" - "github.com/stretchr/testify/require" ) var ( @@ -130,6 +131,20 @@ func TestValidateServiceEndpoints_EndpointInvalid(t *testing.T) { require.ErrorIs(t, err, resource.ErrEmpty) }, }, + "invalid-health-status": { + modify: func(endpoint *pbcatalog.Endpoint) { + endpoint.Ports["foo"] = &pbcatalog.WorkloadPort{ + Port: 42, + } + endpoint.HealthStatus = 99 + }, + validateErr: func(t *testing.T, err error) { + rtest.RequireError(t, err, resource.ErrInvalidField{ + Name: "health_status", + Wrapped: resource.NewConstError("not a supported enum value: 99"), + }) + }, + }, "invalid-port-name": { modify: func(endpoint *pbcatalog.Endpoint) { endpoint.Ports[""] = &pbcatalog.WorkloadPort{ @@ -144,6 +159,24 @@ func TestValidateServiceEndpoints_EndpointInvalid(t *testing.T) { }) }, }, + "invalid-port-protocol": { + modify: func(endpoint *pbcatalog.Endpoint) { + endpoint.Ports["foo"] = &pbcatalog.WorkloadPort{ + Port: 42, + Protocol: 99, + } + }, + validateErr: func(t *testing.T, err error) { + rtest.RequireError(t, err, resource.ErrInvalidMapValue{ + Map: "ports", + Key: "foo", + Wrapped: resource.ErrInvalidField{ + Name: "protocol", + Wrapped: resource.NewConstError("not a supported enum value: 99"), + }, + }) + }, + }, "port-0": { modify: func(endpoint *pbcatalog.Endpoint) { endpoint.Ports["foo"].Port = 0 diff --git a/internal/catalog/internal/types/service_test.go b/internal/catalog/internal/types/service_test.go index a2bdf2360a..4f53a5c0a6 100644 --- a/internal/catalog/internal/types/service_test.go +++ b/internal/catalog/internal/types/service_test.go @@ -6,12 +6,13 @@ package types import ( "testing" - "github.com/hashicorp/consul/internal/resource" - pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v1alpha1" - "github.com/hashicorp/consul/proto-public/pbresource" "github.com/stretchr/testify/require" "google.golang.org/protobuf/reflect/protoreflect" "google.golang.org/protobuf/types/known/anypb" + + "github.com/hashicorp/consul/internal/resource" + pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v1alpha1" + "github.com/hashicorp/consul/proto-public/pbresource" ) func createServiceResource(t *testing.T, data protoreflect.ProtoMessage) *pbresource.Resource { @@ -171,6 +172,37 @@ func TestValidateService_VirtualPortReused(t *testing.T) { require.EqualValues(t, 42, actual.Value) } +func TestValidateService_InvalidPortProtocol(t *testing.T) { + data := &pbcatalog.Service{ + Workloads: &pbcatalog.WorkloadSelector{ + Prefixes: []string{""}, + }, + Ports: []*pbcatalog.ServicePort{ + { + TargetPort: "foo", + Protocol: 99, + }, + }, + } + + res := createServiceResource(t, data) + + err := ValidateService(res) + + expected := resource.ErrInvalidListElement{ + Name: "ports", + Index: 0, + Wrapped: resource.ErrInvalidField{ + Name: "protocol", + Wrapped: resource.NewConstError("not a supported enum value: 99"), + }, + } + + var actual resource.ErrInvalidListElement + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) +} + func TestValidateService_VirtualPortInvalid(t *testing.T) { data := &pbcatalog.Service{ Workloads: &pbcatalog.WorkloadSelector{ diff --git a/internal/catalog/internal/types/validators.go b/internal/catalog/internal/types/validators.go index 602ad71174..94691107f9 100644 --- a/internal/catalog/internal/types/validators.go +++ b/internal/catalog/internal/types/validators.go @@ -4,15 +4,17 @@ package types import ( + "fmt" "net" "regexp" "strings" + "github.com/hashicorp/go-multierror" + "google.golang.org/protobuf/proto" + "github.com/hashicorp/consul/internal/resource" pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v1alpha1" "github.com/hashicorp/consul/proto-public/pbresource" - "github.com/hashicorp/go-multierror" - "google.golang.org/protobuf/proto" ) const ( @@ -135,6 +137,19 @@ func validatePortName(name string) error { return nil } +func validateProtocol(protocol pbcatalog.Protocol) error { + switch protocol { + case pbcatalog.Protocol_PROTOCOL_TCP, + pbcatalog.Protocol_PROTOCOL_HTTP, + pbcatalog.Protocol_PROTOCOL_HTTP2, + pbcatalog.Protocol_PROTOCOL_GRPC, + pbcatalog.Protocol_PROTOCOL_MESH: + return nil + default: + return resource.NewConstError(fmt.Sprintf("not a supported enum value: %v", protocol)) + } +} + // validateWorkloadAddress will validate the WorkloadAddress type. This involves validating // the Host within the workload address and the ports references. For ports references we // ensure that values in the addresses ports array are present in the set of map keys. @@ -207,3 +222,16 @@ func validateReference(allowedType *pbresource.Type, allowedTenancy *pbresource. return err } + +func validateHealth(health pbcatalog.Health) error { + switch health { + case pbcatalog.Health_HEALTH_ANY, + pbcatalog.Health_HEALTH_PASSING, + pbcatalog.Health_HEALTH_WARNING, + pbcatalog.Health_HEALTH_CRITICAL, + pbcatalog.Health_HEALTH_MAINTENANCE: + return nil + default: + return resource.NewConstError(fmt.Sprintf("not a supported enum value: %v", health)) + } +} diff --git a/internal/catalog/internal/types/validators_test.go b/internal/catalog/internal/types/validators_test.go index d1f5e85d2b..a3790e9f9e 100644 --- a/internal/catalog/internal/types/validators_test.go +++ b/internal/catalog/internal/types/validators_test.go @@ -8,11 +8,12 @@ import ( "strings" "testing" + "github.com/hashicorp/go-multierror" + "github.com/stretchr/testify/require" + "github.com/hashicorp/consul/internal/resource" pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v1alpha1" "github.com/hashicorp/consul/proto-public/pbresource" - "github.com/hashicorp/go-multierror" - "github.com/stretchr/testify/require" ) func TestIsValidDNSLabel(t *testing.T) { diff --git a/internal/catalog/internal/types/workload.go b/internal/catalog/internal/types/workload.go index 8560abfe87..b26506d175 100644 --- a/internal/catalog/internal/types/workload.go +++ b/internal/catalog/internal/types/workload.go @@ -7,10 +7,11 @@ import ( "math" "sort" + "github.com/hashicorp/go-multierror" + "github.com/hashicorp/consul/internal/resource" pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v1alpha1" "github.com/hashicorp/consul/proto-public/pbresource" - "github.com/hashicorp/go-multierror" ) const ( @@ -76,6 +77,17 @@ func ValidateWorkload(res *pbresource.Resource) error { }) } + if protoErr := validateProtocol(port.Protocol); protoErr != nil { + err = multierror.Append(err, resource.ErrInvalidMapValue{ + Map: "ports", + Key: portName, + Wrapped: resource.ErrInvalidField{ + Name: "protocol", + Wrapped: protoErr, + }, + }) + } + // Collect the list of mesh ports if port.Protocol == pbcatalog.Protocol_PROTOCOL_MESH { meshPorts = append(meshPorts, portName) diff --git a/internal/catalog/internal/types/workload_test.go b/internal/catalog/internal/types/workload_test.go index 6c4d6708c2..6763fba54c 100644 --- a/internal/catalog/internal/types/workload_test.go +++ b/internal/catalog/internal/types/workload_test.go @@ -6,12 +6,13 @@ package types import ( "testing" - "github.com/hashicorp/consul/internal/resource" - pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v1alpha1" - "github.com/hashicorp/consul/proto-public/pbresource" "github.com/stretchr/testify/require" "google.golang.org/protobuf/reflect/protoreflect" "google.golang.org/protobuf/types/known/anypb" + + "github.com/hashicorp/consul/internal/resource" + pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v1alpha1" + "github.com/hashicorp/consul/proto-public/pbresource" ) func createWorkloadResource(t *testing.T, data protoreflect.ProtoMessage) *pbresource.Resource { @@ -227,6 +228,30 @@ func TestValidateWorkload_InvalidPortName(t *testing.T) { require.Equal(t, expected, actual) } +func TestValidateWorkload_InvalidPortProtocol(t *testing.T) { + data := validWorkload() + data.Ports["foo"] = &pbcatalog.WorkloadPort{ + Port: 42, + Protocol: 99, + } + + res := createWorkloadResource(t, data) + + err := ValidateWorkload(res) + require.Error(t, err) + expected := resource.ErrInvalidMapValue{ + Map: "ports", + Key: "foo", + Wrapped: resource.ErrInvalidField{ + Name: "protocol", + Wrapped: resource.NewConstError("not a supported enum value: 99"), + }, + } + var actual resource.ErrInvalidMapValue + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) +} + func TestValidateWorkload_Port0(t *testing.T) { data := validWorkload() data.Ports["bar"] = &pbcatalog.WorkloadPort{Port: 0}