From 36d4a2c610ff784e9e9327aea095458548066e41 Mon Sep 17 00:00:00 2001 From: Iryna Shustava Date: Fri, 13 Oct 2023 10:55:03 -0600 Subject: [PATCH] mesh: add more validations to Destinations resource --- internal/catalog/exports.go | 8 + .../catalog/internal/types/failover_policy.go | 4 +- internal/catalog/internal/types/service.go | 2 +- .../internal/types/service_endpoints.go | 2 +- internal/catalog/internal/types/validators.go | 8 +- .../catalog/internal/types/validators_test.go | 8 +- internal/catalog/internal/types/workload.go | 2 +- internal/mesh/internal/types/destinations.go | 91 ++++++++- .../mesh/internal/types/destinations_test.go | 175 ++++++++++++++++-- internal/mesh/internal/types/errors.go | 2 + internal/resource/errors.go | 1 + 11 files changed, 274 insertions(+), 29 deletions(-) diff --git a/internal/catalog/exports.go b/internal/catalog/exports.go index be1445edbcfe0..9d2733ababb86 100644 --- a/internal/catalog/exports.go +++ b/internal/catalog/exports.go @@ -105,3 +105,11 @@ func ValidateLocalServiceRefNoSection(ref *pbresource.Reference, wrapErr func(er func ValidateSelector(sel *pbcatalog.WorkloadSelector, allowEmpty bool) error { return types.ValidateSelector(sel, allowEmpty) } + +func ValidatePortName(name string) error { + return types.ValidatePortName(name) +} + +func IsValidUnixSocketPath(host string) bool { + return types.IsValidUnixSocketPath(host) +} diff --git a/internal/catalog/internal/types/failover_policy.go b/internal/catalog/internal/types/failover_policy.go index 4dc8b1bd8eb07..620c3f590e977 100644 --- a/internal/catalog/internal/types/failover_policy.go +++ b/internal/catalog/internal/types/failover_policy.go @@ -145,7 +145,7 @@ func ValidateFailoverPolicy(res *pbresource.Resource) error { Wrapped: err, } } - if portNameErr := validatePortName(portName); portNameErr != nil { + if portNameErr := ValidatePortName(portName); portNameErr != nil { merr = multierror.Append(merr, resource.ErrInvalidMapKey{ Map: "port_configs", Key: portName, @@ -245,7 +245,7 @@ func validateFailoverPolicyDestination(dest *pbcatalog.FailoverDestination, port // assumed and will be reconciled. if dest.Port != "" { if ported { - if portNameErr := validatePortName(dest.Port); portNameErr != nil { + if portNameErr := ValidatePortName(dest.Port); portNameErr != nil { merr = multierror.Append(merr, wrapErr(resource.ErrInvalidField{ Name: "port", Wrapped: portNameErr, diff --git a/internal/catalog/internal/types/service.go b/internal/catalog/internal/types/service.go index 4cefb362e78f0..9fa703641c7f2 100644 --- a/internal/catalog/internal/types/service.go +++ b/internal/catalog/internal/types/service.go @@ -89,7 +89,7 @@ func ValidateService(res *pbresource.Resource) error { } // validate the target port - if nameErr := validatePortName(port.TargetPort); nameErr != nil { + if nameErr := ValidatePortName(port.TargetPort); nameErr != nil { err = multierror.Append(err, resource.ErrInvalidListElement{ Name: "ports", Index: idx, diff --git a/internal/catalog/internal/types/service_endpoints.go b/internal/catalog/internal/types/service_endpoints.go index 8008ada845b4f..938d92b79239a 100644 --- a/internal/catalog/internal/types/service_endpoints.go +++ b/internal/catalog/internal/types/service_endpoints.go @@ -126,7 +126,7 @@ func validateEndpoint(endpoint *pbcatalog.Endpoint, res *pbresource.Resource) er // Validate the endpoints ports for portName, port := range endpoint.Ports { // Port names must be DNS labels - if portNameErr := validatePortName(portName); portNameErr != nil { + if portNameErr := ValidatePortName(portName); portNameErr != nil { err = multierror.Append(err, resource.ErrInvalidMapKey{ Map: "ports", Key: portName, diff --git a/internal/catalog/internal/types/validators.go b/internal/catalog/internal/types/validators.go index 43446b34cfe16..08a8425770a81 100644 --- a/internal/catalog/internal/types/validators.go +++ b/internal/catalog/internal/types/validators.go @@ -56,7 +56,7 @@ func isValidDNSLabel(label string) bool { return dnsLabelMatcher.Match([]byte(label)) } -func isValidUnixSocketPath(host string) bool { +func IsValidUnixSocketPath(host string) bool { if len(host) > maxUnixSocketPathLen || !strings.HasPrefix(host, "unix://") || strings.Contains(host, "\000") { return false } @@ -71,7 +71,7 @@ func validateWorkloadHost(host string) error { } // Check if the host represents an IP address, unix socket path or a DNS name - if !isValidIPAddress(host) && !isValidUnixSocketPath(host) && !isValidDNSName(host) { + if !isValidIPAddress(host) && !IsValidUnixSocketPath(host) && !isValidDNSName(host) { return errInvalidWorkloadHostFormat{Host: host} } @@ -126,7 +126,7 @@ func validateIPAddress(ip string) error { return nil } -func validatePortName(name string) error { +func ValidatePortName(name string) error { if name == "" { return resource.ErrEmpty } @@ -171,7 +171,7 @@ func validateWorkloadAddress(addr *pbcatalog.WorkloadAddress, ports map[string]* // Ensure that unix sockets reference exactly 1 port. They may also indirectly reference 1 port // by the workload having only a single port and omitting any explicit port assignment. - if isValidUnixSocketPath(addr.Host) && + if IsValidUnixSocketPath(addr.Host) && (len(addr.Ports) > 1 || (len(addr.Ports) == 0 && len(ports) > 1)) { err = multierror.Append(err, errUnixSocketMultiport) } diff --git a/internal/catalog/internal/types/validators_test.go b/internal/catalog/internal/types/validators_test.go index 952621adff70b..0d953b829043d 100644 --- a/internal/catalog/internal/types/validators_test.go +++ b/internal/catalog/internal/types/validators_test.go @@ -177,7 +177,7 @@ func TestIsValidUnixSocketPath(t *testing.T) { for name, tcase := range cases { t.Run(name, func(t *testing.T) { - require.Equal(t, tcase.valid, isValidUnixSocketPath(tcase.name)) + require.Equal(t, tcase.valid, IsValidUnixSocketPath(tcase.name)) }) } } @@ -322,15 +322,15 @@ func TestValidatePortName(t *testing.T) { // test for the isValidDNSLabel function. t.Run("empty", func(t *testing.T) { - require.Equal(t, resource.ErrEmpty, validatePortName("")) + require.Equal(t, resource.ErrEmpty, ValidatePortName("")) }) t.Run("invalid", func(t *testing.T) { - require.Equal(t, errNotDNSLabel, validatePortName("foo.com")) + require.Equal(t, errNotDNSLabel, ValidatePortName("foo.com")) }) t.Run("ok", func(t *testing.T) { - require.NoError(t, validatePortName("http")) + require.NoError(t, ValidatePortName("http")) }) } diff --git a/internal/catalog/internal/types/workload.go b/internal/catalog/internal/types/workload.go index 961a85346c4ff..89308667d2c15 100644 --- a/internal/catalog/internal/types/workload.go +++ b/internal/catalog/internal/types/workload.go @@ -44,7 +44,7 @@ func ValidateWorkload(res *pbresource.Resource) error { // Validate the Workload Ports for portName, port := range workload.Ports { - if portNameErr := validatePortName(portName); portNameErr != nil { + if portNameErr := ValidatePortName(portName); portNameErr != nil { err = multierror.Append(err, resource.ErrInvalidMapKey{ Map: "ports", Key: portName, diff --git a/internal/mesh/internal/types/destinations.go b/internal/mesh/internal/types/destinations.go index ee602de346755..5dc5d3dd343ed 100644 --- a/internal/mesh/internal/types/destinations.go +++ b/internal/mesh/internal/types/destinations.go @@ -4,6 +4,8 @@ package types import ( + "net" + "github.com/hashicorp/go-multierror" "google.golang.org/protobuf/proto" @@ -73,10 +75,24 @@ func ValidateDestinations(res *pbresource.Resource) error { var merr error + if selErr := catalog.ValidateSelector(destinations.Workloads, false); selErr != nil { + merr = multierror.Append(merr, resource.ErrInvalidField{ + Name: "workloads", + Wrapped: selErr, + }) + } + + if destinations.GetPqDestinations() != nil { + merr = multierror.Append(merr, resource.ErrInvalidField{ + Name: "pq_destinations", + Wrapped: resource.ErrUnsupported, + }) + } + for i, dest := range destinations.Destinations { wrapDestErr := func(err error) error { return resource.ErrInvalidListElement{ - Name: "upstreams", + Name: "destinations", Index: i, Wrapped: err, } @@ -93,11 +109,78 @@ func ValidateDestinations(res *pbresource.Resource) error { merr = multierror.Append(merr, refErr) } - // TODO(v2): validate port name using catalog validator - // TODO(v2): validate ListenAddr + if portErr := catalog.ValidatePortName(dest.DestinationPort); portErr != nil { + merr = multierror.Append(merr, wrapDestErr(resource.ErrInvalidField{ + Name: "destination_port", + Wrapped: portErr, + })) + } + + if dest.GetDatacenter() != "" { + merr = multierror.Append(merr, wrapDestErr(resource.ErrInvalidField{ + Name: "datacenter", + Wrapped: resource.ErrUnsupported, + })) + } + + if listenAddrErr := validateListenAddr(dest); listenAddrErr != nil { + merr = multierror.Append(merr, wrapDestErr(listenAddrErr)) + } + } + + return merr +} + +func validateListenAddr(dest *pbmesh.Destination) error { + var merr error + + if dest.GetListenAddr() == nil { + return multierror.Append(merr, resource.ErrInvalidFields{ + Names: []string{"ip_port", "unix"}, + Wrapped: resource.ErrMissingOneOf, + }) + } + + switch dest.GetListenAddr().(type) { + case *pbmesh.Destination_IpPort: + listenAddr := dest.GetListenAddr().(*pbmesh.Destination_IpPort).IpPort + + if ipPortErr := validateIPPort(listenAddr); ipPortErr != nil { + merr = multierror.Append(merr, resource.ErrInvalidField{ + Name: "ip_port", + Wrapped: ipPortErr, + }) + } + case *pbmesh.Destination_Unix: + listenAddr := dest.GetListenAddr().(*pbmesh.Destination_Unix).Unix + + if !catalog.IsValidUnixSocketPath(listenAddr.GetPath()) { + merr = multierror.Append(merr, resource.ErrInvalidField{ + Name: "unix", + Wrapped: resource.ErrInvalidField{ + Name: "path", + Wrapped: errInvalidUnixSocketPath, + }, + }) + } + } + + return merr +} + +func validateIPPort(ipPort *pbmesh.IPPortAddress) error { + var merr error + + if listenPortErr := validatePort(ipPort.GetPort(), "port"); listenPortErr != nil { + merr = multierror.Append(merr, listenPortErr) } - // TODO(v2): validate workload selectors + if net.ParseIP(ipPort.GetIp()) == nil { + merr = multierror.Append(merr, resource.ErrInvalidField{ + Name: "ip", + Wrapped: errInvalidIP, + }) + } return merr } diff --git a/internal/mesh/internal/types/destinations_test.go b/internal/mesh/internal/types/destinations_test.go index f5745e4cb3a07..9abe54f638a6b 100644 --- a/internal/mesh/internal/types/destinations_test.go +++ b/internal/mesh/internal/types/destinations_test.go @@ -17,7 +17,7 @@ import ( "github.com/hashicorp/consul/sdk/testutil" ) -func TestMutateUpstreams(t *testing.T) { +func TestMutateDestinations(t *testing.T) { type testcase struct { tenancy *pbresource.Tenancy data *pbmesh.Destinations @@ -86,7 +86,7 @@ func TestMutateUpstreams(t *testing.T) { } } -func TestValidateUpstreams(t *testing.T) { +func TestValidateDestinations(t *testing.T) { type testcase struct { data *pbmesh.Destinations skipMutate bool @@ -123,68 +123,219 @@ func TestValidateUpstreams(t *testing.T) { cases := map[string]testcase{ // emptiness "empty": { - data: &pbmesh.Destinations{}, + data: &pbmesh.Destinations{}, + expectErr: `invalid "workloads" field: cannot be empty`, }, "dest/nil ref": { skipMutate: true, data: &pbmesh.Destinations{ + Workloads: &pbcatalog.WorkloadSelector{Names: []string{"foo"}}, Destinations: []*pbmesh.Destination{ {DestinationRef: nil}, }, }, - expectErr: `invalid element at index 0 of list "upstreams": invalid "destination_ref" field: missing required field`, + expectErr: `invalid element at index 0 of list "destinations": invalid "destination_ref" field: missing required field`, }, "dest/bad type": { skipMutate: true, data: &pbmesh.Destinations{ + Workloads: &pbcatalog.WorkloadSelector{Names: []string{"foo"}}, Destinations: []*pbmesh.Destination{ {DestinationRef: newRefWithTenancy(pbcatalog.WorkloadType, nil, "api")}, }, }, - expectErr: `invalid element at index 0 of list "upstreams": invalid "destination_ref" field: invalid "type" field: reference must have type catalog.v2beta1.Service`, + expectErr: `invalid element at index 0 of list "destinations": invalid "destination_ref" field: invalid "type" field: reference must have type catalog.v2beta1.Service`, }, "dest/nil tenancy": { skipMutate: true, data: &pbmesh.Destinations{ + Workloads: &pbcatalog.WorkloadSelector{Names: []string{"foo"}}, Destinations: []*pbmesh.Destination{ {DestinationRef: &pbresource.Reference{Type: pbcatalog.ServiceType, Name: "api"}}, }, }, - expectErr: `invalid element at index 0 of list "upstreams": invalid "destination_ref" field: invalid "tenancy" field: missing required field`, + expectErr: `invalid element at index 0 of list "destinations": invalid "destination_ref" field: invalid "tenancy" field: missing required field`, }, "dest/bad dest tenancy/partition": { skipMutate: true, data: &pbmesh.Destinations{ + Workloads: &pbcatalog.WorkloadSelector{Names: []string{"foo"}}, Destinations: []*pbmesh.Destination{ {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, newTestTenancy(".bar"), "api")}, }, }, - expectErr: `invalid element at index 0 of list "upstreams": invalid "destination_ref" field: invalid "tenancy" field: invalid "partition" field: cannot be empty`, + expectErr: `invalid element at index 0 of list "destinations": invalid "destination_ref" field: invalid "tenancy" field: invalid "partition" field: cannot be empty`, }, "dest/bad dest tenancy/namespace": { skipMutate: true, data: &pbmesh.Destinations{ + Workloads: &pbcatalog.WorkloadSelector{Names: []string{"foo"}}, Destinations: []*pbmesh.Destination{ {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, newTestTenancy("foo"), "api")}, }, }, - expectErr: `invalid element at index 0 of list "upstreams": invalid "destination_ref" field: invalid "tenancy" field: invalid "namespace" field: cannot be empty`, + expectErr: `invalid element at index 0 of list "destinations": invalid "destination_ref" field: invalid "tenancy" field: invalid "namespace" field: cannot be empty`, }, "dest/bad dest tenancy/peer_name": { skipMutate: true, data: &pbmesh.Destinations{ + Workloads: &pbcatalog.WorkloadSelector{Names: []string{"foo"}}, Destinations: []*pbmesh.Destination{ {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, &pbresource.Tenancy{Partition: "foo", Namespace: "bar"}, "api")}, }, }, - expectErr: `invalid element at index 0 of list "upstreams": invalid "destination_ref" field: invalid "tenancy" field: invalid "peer_name" field: must be set to "local"`, + expectErr: `invalid element at index 0 of list "destinations": invalid "destination_ref" field: invalid "tenancy" field: invalid "peer_name" field: must be set to "local"`, + }, + "unsupported pq_destinations": { + skipMutate: true, + data: &pbmesh.Destinations{ + Workloads: &pbcatalog.WorkloadSelector{Names: []string{"foo"}}, + PqDestinations: []*pbmesh.PreparedQueryDestination{ + {Name: "foo-query"}, + }, + }, + expectErr: `invalid "pq_destinations" field: field is currently not supported`, + }, + "missing destination port": { + skipMutate: true, + data: &pbmesh.Destinations{ + Workloads: &pbcatalog.WorkloadSelector{Names: []string{"foo"}}, + Destinations: []*pbmesh.Destination{ + { + DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, newTestTenancy("foo.bar"), "api"), + ListenAddr: &pbmesh.Destination_IpPort{ + IpPort: &pbmesh.IPPortAddress{ + Ip: "127.0.0.1", + Port: 1234, + }, + }, + }, + }, + }, + expectErr: `invalid element at index 0 of list "destinations": invalid "destination_port" field: cannot be empty`, + }, + "unsupported datacenter": { + skipMutate: true, + data: &pbmesh.Destinations{ + Workloads: &pbcatalog.WorkloadSelector{Names: []string{"foo"}}, + Destinations: []*pbmesh.Destination{ + { + DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, newTestTenancy("foo.bar"), "api"), + DestinationPort: "p1", + Datacenter: "dc2", + ListenAddr: &pbmesh.Destination_IpPort{ + IpPort: &pbmesh.IPPortAddress{ + Ip: "127.0.0.1", + Port: 1234, + }, + }, + }, + }, + }, + expectErr: `invalid element at index 0 of list "destinations": invalid "datacenter" field: field is currently not supported`, + }, + "missing listen addr": { + skipMutate: true, + data: &pbmesh.Destinations{ + Workloads: &pbcatalog.WorkloadSelector{Names: []string{"foo"}}, + Destinations: []*pbmesh.Destination{ + { + DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, newTestTenancy("foo.bar"), "api"), + DestinationPort: "p1", + }, + }, + }, + expectErr: `invalid "ip_port,unix" fields: missing one of the required fields`, + }, + "invalid ip for listen addr": { + skipMutate: true, + data: &pbmesh.Destinations{ + Workloads: &pbcatalog.WorkloadSelector{Names: []string{"foo"}}, + Destinations: []*pbmesh.Destination{ + { + DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, newTestTenancy("foo.bar"), "api"), + DestinationPort: "p1", + ListenAddr: &pbmesh.Destination_IpPort{ + IpPort: &pbmesh.IPPortAddress{ + Ip: "invalid", + Port: 1234, + }, + }, + }, + }, + }, + expectErr: `invalid "ip" field: IP address is not valid`, + }, + "invalid port for listen addr": { + skipMutate: true, + data: &pbmesh.Destinations{ + Workloads: &pbcatalog.WorkloadSelector{Names: []string{"foo"}}, + Destinations: []*pbmesh.Destination{ + { + DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, newTestTenancy("foo.bar"), "api"), + DestinationPort: "p1", + ListenAddr: &pbmesh.Destination_IpPort{ + IpPort: &pbmesh.IPPortAddress{ + Ip: "127.0.0.1", + Port: 0, + }, + }, + }, + }, + }, + expectErr: `invalid "port" field: port number is outside the range 1 to 65535`, + }, + "invalid unix path for listen addr": { + skipMutate: true, + data: &pbmesh.Destinations{ + Workloads: &pbcatalog.WorkloadSelector{Names: []string{"foo"}}, + Destinations: []*pbmesh.Destination{ + { + DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, newTestTenancy("foo.bar"), "api"), + DestinationPort: "p1", + ListenAddr: &pbmesh.Destination_Unix{ + Unix: &pbmesh.UnixSocketAddress{ + Path: "foo", + }, + }, + }, + }, + }, + expectErr: `invalid "unix" field: invalid "path" field: unix socket path is not valid`, }, "normal": { data: &pbmesh.Destinations{ + Workloads: &pbcatalog.WorkloadSelector{Names: []string{"foo"}}, Destinations: []*pbmesh.Destination{ - {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, newTestTenancy("foo.bar"), "api")}, - {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, newTestTenancy("foo.zim"), "api")}, - {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, newTestTenancy("gir.zim"), "api")}, + { + DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, newTestTenancy("foo.bar"), "api"), + DestinationPort: "p1", + ListenAddr: &pbmesh.Destination_IpPort{ + IpPort: &pbmesh.IPPortAddress{ + Ip: "127.0.0.1", + Port: 1234, + }, + }, + }, + { + DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, newTestTenancy("foo.zim"), "api"), + DestinationPort: "p2", + ListenAddr: &pbmesh.Destination_IpPort{ + IpPort: &pbmesh.IPPortAddress{ + Ip: "127.0.0.1", + Port: 1235, + }, + }, + }, + { + DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, newTestTenancy("gir.zim"), "api"), + DestinationPort: "p3", + ListenAddr: &pbmesh.Destination_Unix{ + Unix: &pbmesh.UnixSocketAddress{ + Path: "unix://foo/bar", + }, + }, + }, }, }, }, diff --git a/internal/mesh/internal/types/errors.go b/internal/mesh/internal/types/errors.go index b234e8e73c034..a1fdcf629a5be 100644 --- a/internal/mesh/internal/types/errors.go +++ b/internal/mesh/internal/types/errors.go @@ -6,6 +6,8 @@ import ( var ( errInvalidPort = errors.New("port number is outside the range 1 to 65535") + errInvalidIP = errors.New("IP address is not valid") + errInvalidUnixSocketPath = errors.New("unix socket path is not valid") errInvalidExposePathProtocol = errors.New("invalid protocol: only HTTP and HTTP2 protocols are allowed") errMissingProxyConfigData = errors.New("at least one of \"bootstrap_config\" or \"dynamic_config\" fields must be set") ) diff --git a/internal/resource/errors.go b/internal/resource/errors.go index 2003d86cbf71b..2602e8c5b6ca7 100644 --- a/internal/resource/errors.go +++ b/internal/resource/errors.go @@ -14,6 +14,7 @@ import ( var ( ErrMissing = NewConstError("missing required field") + ErrMissingOneOf = NewConstError("missing one of the required fields") ErrEmpty = NewConstError("cannot be empty") ErrReferenceTenancyNotEqual = NewConstError("resource tenancy and reference tenancy differ") ErrUnsupported = NewConstError("field is currently not supported")