From 456156ebec0a943e689e9748a01a786c8f5bfed8 Mon Sep 17 00:00:00 2001 From: Matt Keeler Date: Fri, 12 May 2023 09:24:55 -0400 Subject: [PATCH] Add type validations for the catalog resources (#17211) Also adding some common resource validation error types to the internal/resource package. --- internal/catalog/internal/types/dns_policy.go | 59 +- .../catalog/internal/types/dns_policy_test.go | 162 +++++ internal/catalog/internal/types/errors.go | 62 ++ .../catalog/internal/types/errors_test.go | 70 +++ .../catalog/internal/types/health_checks.go | 68 +- .../internal/types/health_checks_test.go | 197 ++++++ .../catalog/internal/types/health_status.go | 50 +- .../internal/types/health_status_test.go | 215 +++++++ internal/catalog/internal/types/node.go | 59 +- internal/catalog/internal/types/node_test.go | 128 ++++ internal/catalog/internal/types/service.go | 89 ++- .../internal/types/service_endpoints.go | 87 +++ .../internal/types/service_endpoints_test.go | 184 ++++++ .../catalog/internal/types/service_test.go | 200 ++++++ .../errDNSPassingWeightOutOfRange.golden | 1 + .../errDNSWarningWeightOutOfRange.golden | 1 + .../testdata/errInvalidNodeHostFormat.golden | 1 + .../testdata/errInvalidPhysicalPort.golden | 1 + .../testdata/errInvalidPortReference.golden | 1 + .../testdata/errInvalidVirtualPort.golden | 1 + .../errInvalidWorkloadHostFormat.golden | 1 + .../testdata/errLocalityZoneNoRegion.golden | 1 + .../types/testdata/errNotDNSLabel.golden | 1 + .../types/testdata/errNotIPAddress.golden | 1 + .../types/testdata/errTooMuchMesh.golden | 1 + .../testdata/errUnixSocketMultiport.golden | 1 + .../testdata/errVirtualPortReused.golden | 1 + internal/catalog/internal/types/validators.go | 209 ++++++ .../catalog/internal/types/validators_test.go | 595 ++++++++++++++++++ .../catalog/internal/types/virtual_ips.go | 26 +- .../internal/types/virtual_ips_test.go | 82 +++ internal/catalog/internal/types/workload.go | 118 +++- .../catalog/internal/types/workload_test.go | 281 +++++++++ internal/resource/errors.go | 114 ++++ internal/resource/errors_test.go | 124 ++++ .../resource/testdata/ErrDataParse.golden | 1 + internal/resource/testdata/ErrEmpty.golden | 1 + .../resource/testdata/ErrInvalidField.golden | 1 + .../testdata/ErrInvalidListElement.golden | 1 + .../resource/testdata/ErrInvalidMapKey.golden | 1 + .../testdata/ErrInvalidMapValue.golden | 1 + .../testdata/ErrInvalidReferenceType.golden | 1 + internal/resource/testdata/ErrMissing.golden | 1 + .../resource/testdata/ErrOwnerInvalid.golden | 1 + .../ErrReferenceTenancyNotEqual.golden | 1 + 45 files changed, 3195 insertions(+), 7 deletions(-) create mode 100644 internal/catalog/internal/types/dns_policy_test.go create mode 100644 internal/catalog/internal/types/errors.go create mode 100644 internal/catalog/internal/types/errors_test.go create mode 100644 internal/catalog/internal/types/health_checks_test.go create mode 100644 internal/catalog/internal/types/health_status_test.go create mode 100644 internal/catalog/internal/types/node_test.go create mode 100644 internal/catalog/internal/types/service_endpoints_test.go create mode 100644 internal/catalog/internal/types/service_test.go create mode 100644 internal/catalog/internal/types/testdata/errDNSPassingWeightOutOfRange.golden create mode 100644 internal/catalog/internal/types/testdata/errDNSWarningWeightOutOfRange.golden create mode 100644 internal/catalog/internal/types/testdata/errInvalidNodeHostFormat.golden create mode 100644 internal/catalog/internal/types/testdata/errInvalidPhysicalPort.golden create mode 100644 internal/catalog/internal/types/testdata/errInvalidPortReference.golden create mode 100644 internal/catalog/internal/types/testdata/errInvalidVirtualPort.golden create mode 100644 internal/catalog/internal/types/testdata/errInvalidWorkloadHostFormat.golden create mode 100644 internal/catalog/internal/types/testdata/errLocalityZoneNoRegion.golden create mode 100644 internal/catalog/internal/types/testdata/errNotDNSLabel.golden create mode 100644 internal/catalog/internal/types/testdata/errNotIPAddress.golden create mode 100644 internal/catalog/internal/types/testdata/errTooMuchMesh.golden create mode 100644 internal/catalog/internal/types/testdata/errUnixSocketMultiport.golden create mode 100644 internal/catalog/internal/types/testdata/errVirtualPortReused.golden create mode 100644 internal/catalog/internal/types/validators.go create mode 100644 internal/catalog/internal/types/validators_test.go create mode 100644 internal/catalog/internal/types/virtual_ips_test.go create mode 100644 internal/catalog/internal/types/workload_test.go create mode 100644 internal/resource/errors.go create mode 100644 internal/resource/errors_test.go create mode 100644 internal/resource/testdata/ErrDataParse.golden create mode 100644 internal/resource/testdata/ErrEmpty.golden create mode 100644 internal/resource/testdata/ErrInvalidField.golden create mode 100644 internal/resource/testdata/ErrInvalidListElement.golden create mode 100644 internal/resource/testdata/ErrInvalidMapKey.golden create mode 100644 internal/resource/testdata/ErrInvalidMapValue.golden create mode 100644 internal/resource/testdata/ErrInvalidReferenceType.golden create mode 100644 internal/resource/testdata/ErrMissing.golden create mode 100644 internal/resource/testdata/ErrOwnerInvalid.golden create mode 100644 internal/resource/testdata/ErrReferenceTenancyNotEqual.golden diff --git a/internal/catalog/internal/types/dns_policy.go b/internal/catalog/internal/types/dns_policy.go index d1d0375901..4c34560276 100644 --- a/internal/catalog/internal/types/dns_policy.go +++ b/internal/catalog/internal/types/dns_policy.go @@ -4,9 +4,12 @@ package types import ( + "math" + "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 ( @@ -27,6 +30,60 @@ func RegisterDNSPolicy(r resource.Registry) { r.Register(resource.Registration{ Type: DNSPolicyV1Alpha1Type, Proto: &pbcatalog.DNSPolicy{}, - Validate: nil, + Validate: ValidateDNSPolicy, }) } + +func ValidateDNSPolicy(res *pbresource.Resource) error { + var policy pbcatalog.DNSPolicy + + if err := res.Data.UnmarshalTo(&policy); err != nil { + return resource.NewErrDataParse(&policy, err) + } + + var err error + // Ensure that this resource isn't useless and is attempting to + // select at least one workload. + if selErr := validateSelector(policy.Workloads, false); selErr != nil { + err = multierror.Append(err, resource.ErrInvalidField{ + Name: "workloads", + Wrapped: selErr, + }) + } + + // Validate the weights + if weightErr := validateDNSPolicyWeights(policy.Weights); weightErr != nil { + err = multierror.Append(err, resource.ErrInvalidField{ + Name: "weights", + Wrapped: weightErr, + }) + } + + return err +} + +func validateDNSPolicyWeights(weights *pbcatalog.Weights) error { + // Non nil weights are required + if weights == nil { + return resource.ErrMissing + } + + var err error + if weights.Passing < 1 || weights.Passing > math.MaxUint16 { + err = multierror.Append(err, resource.ErrInvalidField{ + Name: "passing", + Wrapped: errDNSPassingWeightOutOfRange, + }) + } + + // Each weight is an unsigned integer so we don't need to + // check for negative weights. + if weights.Warning > math.MaxUint16 { + err = multierror.Append(err, resource.ErrInvalidField{ + Name: "warning", + Wrapped: errDNSWarningWeightOutOfRange, + }) + } + + return err +} diff --git a/internal/catalog/internal/types/dns_policy_test.go b/internal/catalog/internal/types/dns_policy_test.go new file mode 100644 index 0000000000..b4c2da3c78 --- /dev/null +++ b/internal/catalog/internal/types/dns_policy_test.go @@ -0,0 +1,162 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package types + +import ( + "fmt" + "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" +) + +func createDNSPolicyResource(t *testing.T, data protoreflect.ProtoMessage) *pbresource.Resource { + res := &pbresource.Resource{ + Id: &pbresource.ID{ + Type: DNSPolicyType, + Tenancy: &pbresource.Tenancy{ + Partition: "default", + Namespace: "default", + PeerName: "local", + }, + Name: "test-policy", + }, + } + + var err error + res.Data, err = anypb.New(data) + require.NoError(t, err) + return res +} + +func TestValidateDNSPolicy_Ok(t *testing.T) { + data := &pbcatalog.DNSPolicy{ + Workloads: &pbcatalog.WorkloadSelector{ + Prefixes: []string{""}, + }, + Weights: &pbcatalog.Weights{ + Passing: 3, + Warning: 0, + }, + } + + res := createDNSPolicyResource(t, data) + + err := ValidateDNSPolicy(res) + require.NoError(t, err) +} + +func TestValidateDNSPolicy_ParseError(t *testing.T) { + // Any type other than the DNSPolicy type would work + // to cause the error we are expecting + data := &pbcatalog.IP{Address: "198.18.0.1"} + + res := createDNSPolicyResource(t, data) + + err := ValidateDNSPolicy(res) + require.Error(t, err) + require.ErrorAs(t, err, &resource.ErrDataParse{}) +} + +func TestValidateDNSPolicy_MissingWeights(t *testing.T) { + data := &pbcatalog.DNSPolicy{ + Workloads: &pbcatalog.WorkloadSelector{ + Prefixes: []string{""}, + }, + } + + res := createDNSPolicyResource(t, data) + + err := ValidateDNSPolicy(res) + require.Error(t, err) + expected := resource.ErrInvalidField{ + Name: "weights", + Wrapped: resource.ErrMissing, + } + var actual resource.ErrInvalidField + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) +} + +func TestValidateDNSPolicy_InvalidPassingWeight(t *testing.T) { + for _, weight := range []uint32{0, 1000000} { + t.Run(fmt.Sprintf("%d", weight), func(t *testing.T) { + data := &pbcatalog.DNSPolicy{ + Workloads: &pbcatalog.WorkloadSelector{ + Prefixes: []string{""}, + }, + Weights: &pbcatalog.Weights{ + Passing: weight, + }, + } + + res := createDNSPolicyResource(t, data) + + err := ValidateDNSPolicy(res) + require.Error(t, err) + expected := resource.ErrInvalidField{ + Name: "passing", + Wrapped: errDNSPassingWeightOutOfRange, + } + var actual resource.ErrInvalidField + require.ErrorAs(t, err, &actual) + require.Equal(t, "weights", actual.Name) + err = actual.Unwrap() + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) + }) + } +} + +func TestValidateDNSPolicy_InvalidWarningWeight(t *testing.T) { + data := &pbcatalog.DNSPolicy{ + Workloads: &pbcatalog.WorkloadSelector{ + Prefixes: []string{""}, + }, + Weights: &pbcatalog.Weights{ + Passing: 1, + Warning: 1000000, + }, + } + + res := createDNSPolicyResource(t, data) + + err := ValidateDNSPolicy(res) + require.Error(t, err) + expected := resource.ErrInvalidField{ + Name: "warning", + Wrapped: errDNSWarningWeightOutOfRange, + } + var actual resource.ErrInvalidField + require.ErrorAs(t, err, &actual) + require.Equal(t, "weights", actual.Name) + err = actual.Unwrap() + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) +} + +func TestValidateDNSPolicy_EmptySelector(t *testing.T) { + data := &pbcatalog.DNSPolicy{ + Weights: &pbcatalog.Weights{ + Passing: 10, + Warning: 3, + }, + } + + res := createDNSPolicyResource(t, data) + + err := ValidateDNSPolicy(res) + require.Error(t, err) + expected := resource.ErrInvalidField{ + Name: "workloads", + Wrapped: resource.ErrEmpty, + } + var actual resource.ErrInvalidField + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) +} diff --git a/internal/catalog/internal/types/errors.go b/internal/catalog/internal/types/errors.go new file mode 100644 index 0000000000..39a7131ae0 --- /dev/null +++ b/internal/catalog/internal/types/errors.go @@ -0,0 +1,62 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package types + +import ( + "errors" + "fmt" +) + +var ( + errNotDNSLabel = errors.New(fmt.Sprintf("value must match regex: %s", dnsLabelRegex)) + errNotIPAddress = errors.New("value is not a valid IP address") + errUnixSocketMultiport = errors.New("Unix socket address references more than one port") + errInvalidPhysicalPort = errors.New("port number is outside the range 1 to 65535") + errInvalidVirtualPort = errors.New("port number is outside the range 0 to 65535") + errDNSWarningWeightOutOfRange = errors.New("DNS warning weight is outside the range 0 to 65535") + errDNSPassingWeightOutOfRange = errors.New("DNS passing weight is outside of the range 1 to 65535") + errLocalityZoneNoRegion = errors.New("locality region cannot be empty if the zone is set") + errInvalidHealth = errors.New("health status must be one of: passing, warning, critical or maintenance") +) + +type errInvalidWorkloadHostFormat struct { + Host string +} + +func (err errInvalidWorkloadHostFormat) Error() string { + return fmt.Sprintf("%q is not an IP address, Unix socket path or a DNS name.", err.Host) +} + +type errInvalidNodeHostFormat struct { + Host string +} + +func (err errInvalidNodeHostFormat) Error() string { + return fmt.Sprintf("%q is not an IP address or a DNS name.", err.Host) +} + +type errInvalidPortReference struct { + Name string +} + +func (err errInvalidPortReference) Error() string { + return fmt.Sprintf("port with name %q has not been defined", err.Name) +} + +type errVirtualPortReused struct { + Index int + Value uint32 +} + +func (err errVirtualPortReused) Error() string { + return fmt.Sprintf("virtual port %d was previously assigned at index %d", err.Value, err.Index) +} + +type errTooMuchMesh struct { + Ports []string +} + +func (err errTooMuchMesh) Error() string { + return fmt.Sprintf("protocol \"mesh\" was specified in more than 1 port: %+v", err.Ports) +} diff --git a/internal/catalog/internal/types/errors_test.go b/internal/catalog/internal/types/errors_test.go new file mode 100644 index 0000000000..d8249549d5 --- /dev/null +++ b/internal/catalog/internal/types/errors_test.go @@ -0,0 +1,70 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package types + +import ( + "flag" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +// update allows golden files to be updated based on the current output. +var update = flag.Bool("update", false, "update golden files") + +func goldenError(t *testing.T, name string, actual string) { + t.Helper() + + fpath := filepath.Join("testdata", name+".golden") + + if *update { + require.NoError(t, os.WriteFile(fpath, []byte(actual), 0644)) + } else { + expected, err := os.ReadFile(fpath) + require.NoError(t, err) + require.Equal(t, string(expected), actual) + } +} + +func TestErrorStrings(t *testing.T) { + type testCase struct { + err error + expected string + } + + cases := map[string]error{ + "errInvalidWorkloadHostFormat": errInvalidWorkloadHostFormat{ + Host: "-foo-bar-", + }, + "errInvalidNodeHostFormat": errInvalidNodeHostFormat{ + Host: "unix:///node.sock", + }, + "errInvalidPortReference": errInvalidPortReference{ + Name: "http", + }, + "errVirtualPortReused": errVirtualPortReused{ + Index: 3, + Value: 8080, + }, + "errTooMuchMesh": errTooMuchMesh{ + Ports: []string{"http", "grpc"}, + }, + "errNotDNSLabel": errNotDNSLabel, + "errNotIPAddress": errNotIPAddress, + "errUnixSocketMultiport": errUnixSocketMultiport, + "errInvalidPhysicalPort": errInvalidPhysicalPort, + "errInvalidVirtualPort": errInvalidVirtualPort, + "errDNSWarningWeightOutOfRange": errDNSWarningWeightOutOfRange, + "errDNSPassingWeightOutOfRange": errDNSPassingWeightOutOfRange, + "errLocalityZoneNoRegion": errLocalityZoneNoRegion, + } + + for name, err := range cases { + t.Run(name, func(t *testing.T) { + goldenError(t, name, err.Error()) + }) + } +} diff --git a/internal/catalog/internal/types/health_checks.go b/internal/catalog/internal/types/health_checks.go index e117088981..7df200ec28 100644 --- a/internal/catalog/internal/types/health_checks.go +++ b/internal/catalog/internal/types/health_checks.go @@ -7,6 +7,7 @@ import ( "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 ( @@ -27,6 +28,71 @@ func RegisterHealthChecks(r resource.Registry) { r.Register(resource.Registration{ Type: HealthChecksV1Alpha1Type, Proto: &pbcatalog.HealthChecks{}, - Validate: nil, + Validate: ValidateHealthChecks, }) } + +func ValidateHealthChecks(res *pbresource.Resource) error { + var checks pbcatalog.HealthChecks + + if err := res.Data.UnmarshalTo(&checks); err != nil { + return resource.NewErrDataParse(&checks, err) + } + + var err error + + // Validate the workload selector + if selErr := validateSelector(checks.Workloads, false); selErr != nil { + err = multierror.Append(err, resource.ErrInvalidField{ + Name: "workloads", + Wrapped: selErr, + }) + } + + // Validate each check + for idx, check := range checks.HealthChecks { + if checkErr := validateCheck(check); checkErr != nil { + err = multierror.Append(err, resource.ErrInvalidListElement{ + Name: "checks", + Index: idx, + Wrapped: checkErr, + }) + } + } + + return err +} + +func validateCheck(check *pbcatalog.HealthCheck) error { + var err error + // Validate the check name + if check.Name == "" { + err = multierror.Append(err, resource.ErrInvalidField{ + Name: "name", + Wrapped: resource.ErrMissing, + }) + } else if !isValidDNSLabel(check.Name) { + err = multierror.Append(err, resource.ErrInvalidField{ + Name: "name", + Wrapped: errNotDNSLabel, + }) + } + + // Validate the definition + if check.Definition == nil { + err = multierror.Append(err, resource.ErrInvalidField{ + Name: "definition", + Wrapped: resource.ErrMissing, + }) + } + + // In theory it would be nice to validate the individual check definition. + // However whether a check is valid will be up for interpretation by + // the check executor. The executor may default some addressing, + // allow for templating etc. Therefore we cannot really know at admission + // time whether the check will be executable. Therefore it is expected + // that check executors will update the status of the resource to note + // whether it was valid for that executor. + + return err +} diff --git a/internal/catalog/internal/types/health_checks_test.go b/internal/catalog/internal/types/health_checks_test.go new file mode 100644 index 0000000000..12c13d3c1a --- /dev/null +++ b/internal/catalog/internal/types/health_checks_test.go @@ -0,0 +1,197 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package types + +import ( + "testing" + "time" + + "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" + "google.golang.org/protobuf/types/known/durationpb" +) + +func createHealthChecksResource(t *testing.T, data protoreflect.ProtoMessage) *pbresource.Resource { + res := &pbresource.Resource{ + Id: &pbresource.ID{ + Type: HealthChecksType, + Tenancy: &pbresource.Tenancy{ + Partition: "default", + Namespace: "default", + PeerName: "local", + }, + Name: "test-checks", + }, + } + + var err error + res.Data, err = anypb.New(data) + require.NoError(t, err) + return res +} + +func TestValidateHealthChecks_Ok(t *testing.T) { + data := &pbcatalog.HealthChecks{ + Workloads: &pbcatalog.WorkloadSelector{ + Prefixes: []string{""}, + }, + HealthChecks: []*pbcatalog.HealthCheck{ + { + Name: "test-check", + Definition: &pbcatalog.HealthCheck_Tcp{ + Tcp: &pbcatalog.TCPCheck{ + Address: "198.18.0.1", + }, + }, + Interval: durationpb.New(30 * time.Second), + Timeout: durationpb.New(15 * time.Second), + }, + }, + } + + res := createHealthChecksResource(t, data) + + err := ValidateHealthChecks(res) + require.NoError(t, err) +} + +func TestValidateHealthChecks_ParseError(t *testing.T) { + // Any type other than the HealthChecks type would work + // to cause the error we are expecting + data := &pbcatalog.IP{Address: "198.18.0.1"} + + res := createHealthChecksResource(t, data) + + err := ValidateHealthChecks(res) + require.Error(t, err) + require.ErrorAs(t, err, &resource.ErrDataParse{}) +} + +func TestValidateHealthChecks_InvalidCheckName(t *testing.T) { + genData := func(name string) *pbcatalog.HealthChecks { + return &pbcatalog.HealthChecks{ + Workloads: &pbcatalog.WorkloadSelector{ + Prefixes: []string{""}, + }, + HealthChecks: []*pbcatalog.HealthCheck{ + { + Name: name, + Definition: &pbcatalog.HealthCheck_Tcp{ + Tcp: &pbcatalog.TCPCheck{ + Address: "198.18.0.1", + }, + }, + Interval: durationpb.New(30 * time.Second), + Timeout: durationpb.New(15 * time.Second), + }, + }, + } + } + + type testCase struct { + name string + err bool + expectedErr resource.ErrInvalidField + } + + // These checks are not exhaustive of the classes of names which + // would be accepted or rejected. The tests for the isValidDNSLabel + // function have more thorough testing. Here we just ensure that + // we can get the errNotDNSLabel error to indicate that calling + // that function returned false and was emitted by ValidateHealthChecks + cases := map[string]testCase{ + "basic": { + name: "foo-check", + }, + "missing-name": { + err: true, + expectedErr: resource.ErrInvalidField{ + Name: "name", + Wrapped: resource.ErrEmpty, + }, + }, + "invalid-dns-label": { + name: "UPPERCASE", + err: true, + expectedErr: resource.ErrInvalidField{ + Name: "name", + Wrapped: errNotDNSLabel, + }, + }, + } + + for name, tcase := range cases { + t.Run(name, func(t *testing.T) { + data := genData(tcase.name) + err := ValidateHealthChecks(createHealthChecksResource(t, data)) + + if tcase.err { + require.Error(t, err) + + } else { + require.NoError(t, err) + } + }) + } +} + +func TestValidateHealthChecks_MissingDefinition(t *testing.T) { + data := &pbcatalog.HealthChecks{ + Workloads: &pbcatalog.WorkloadSelector{ + Prefixes: []string{""}, + }, + HealthChecks: []*pbcatalog.HealthCheck{ + { + Name: "test-check", + Interval: durationpb.New(30 * time.Second), + Timeout: durationpb.New(15 * time.Second), + }, + }, + } + + res := createHealthChecksResource(t, data) + + err := ValidateHealthChecks(res) + require.Error(t, err) + expected := resource.ErrInvalidField{ + Name: "definition", + Wrapped: resource.ErrMissing, + } + var actual resource.ErrInvalidField + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) +} + +func TestValidateHealthChecks_EmptySelector(t *testing.T) { + data := &pbcatalog.HealthChecks{ + HealthChecks: []*pbcatalog.HealthCheck{ + { + Name: "test-check", + Definition: &pbcatalog.HealthCheck_Tcp{ + Tcp: &pbcatalog.TCPCheck{ + Address: "198.18.0.1", + }, + }, + Interval: durationpb.New(30 * time.Second), + Timeout: durationpb.New(15 * time.Second), + }, + }, + } + + res := createHealthChecksResource(t, data) + + err := ValidateHealthChecks(res) + require.Error(t, err) + expected := resource.ErrInvalidField{ + Name: "workloads", + Wrapped: resource.ErrEmpty, + } + var actual resource.ErrInvalidField + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) +} diff --git a/internal/catalog/internal/types/health_status.go b/internal/catalog/internal/types/health_status.go index b85c8036d8..0dc73f6320 100644 --- a/internal/catalog/internal/types/health_status.go +++ b/internal/catalog/internal/types/health_status.go @@ -7,6 +7,7 @@ import ( "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 ( @@ -27,6 +28,53 @@ func RegisterHealthStatus(r resource.Registry) { r.Register(resource.Registration{ Type: HealthStatusV1Alpha1Type, Proto: &pbcatalog.HealthStatus{}, - Validate: nil, + Validate: ValidateHealthStatus, }) } + +func ValidateHealthStatus(res *pbresource.Resource) error { + var hs pbcatalog.HealthStatus + + if err := res.Data.UnmarshalTo(&hs); err != nil { + return resource.NewErrDataParse(&hs, err) + } + + var err error + + // Should we allow empty types? I think for now it will be safest to require + // the type field is set and we can relax this restriction in the future + // if we deem it desirable. + if hs.Type == "" { + err = multierror.Append(err, resource.ErrInvalidField{ + Name: "type", + Wrapped: resource.ErrMissing, + }) + } + + switch hs.Status { + case pbcatalog.Health_HEALTH_PASSING, + pbcatalog.Health_HEALTH_WARNING, + pbcatalog.Health_HEALTH_CRITICAL, + pbcatalog.Health_HEALTH_MAINTENANCE: + default: + err = multierror.Append(err, resource.ErrInvalidField{ + Name: "status", + Wrapped: errInvalidHealth, + }) + } + + // Ensure that the HealthStatus' owner is a type that we want to allow. The + // owner is currently the resource that this HealthStatus applies to. If we + // change this to be a parent reference within the HealthStatus.Data then + // we could allow for other owners. + if res.Owner == nil { + err = multierror.Append(err, resource.ErrInvalidField{ + Name: "owner", + Wrapped: resource.ErrMissing, + }) + } else if !resource.EqualType(res.Owner.Type, WorkloadType) && !resource.EqualType(res.Owner.Type, NodeType) { + err = multierror.Append(err, resource.ErrOwnerInvalid{ResourceType: res.Id.Type, OwnerType: res.Owner.Type}) + } + + return err +} diff --git a/internal/catalog/internal/types/health_status_test.go b/internal/catalog/internal/types/health_status_test.go new file mode 100644 index 0000000000..5fa9eaa5af --- /dev/null +++ b/internal/catalog/internal/types/health_status_test.go @@ -0,0 +1,215 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +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" +) + +var ( + defaultHealthStatusOwnerTenancy = &pbresource.Tenancy{ + Partition: "default", + Namespace: "default", + PeerName: "local", + } + + defaultHealthStatusOwner = &pbresource.ID{ + Type: WorkloadType, + Tenancy: defaultHealthStatusOwnerTenancy, + Name: "foo", + } +) + +func createHealthStatusResource(t *testing.T, data protoreflect.ProtoMessage, owner *pbresource.ID) *pbresource.Resource { + res := &pbresource.Resource{ + Id: &pbresource.ID{ + Type: HealthStatusType, + Tenancy: &pbresource.Tenancy{ + Partition: "default", + Namespace: "default", + PeerName: "local", + }, + Name: "test-status", + }, + Owner: owner, + } + + var err error + res.Data, err = anypb.New(data) + require.NoError(t, err) + return res +} + +func TestValidateHealthStatus_Ok(t *testing.T) { + data := &pbcatalog.HealthStatus{ + Type: "tcp", + Status: pbcatalog.Health_HEALTH_PASSING, + Description: "Doesn't matter as this is user settable", + Output: "Health check executors are free to use this field", + } + + type testCase struct { + owner *pbresource.ID + } + + cases := map[string]testCase{ + "workload-owned": { + owner: &pbresource.ID{ + Type: WorkloadType, + Tenancy: defaultHealthStatusOwnerTenancy, + Name: "foo-workload", + }, + }, + "node-owned": { + owner: &pbresource.ID{ + Type: NodeType, + Tenancy: defaultHealthStatusOwnerTenancy, + Name: "bar-node", + }, + }, + } + + for name, tcase := range cases { + t.Run(name, func(t *testing.T) { + res := createHealthStatusResource(t, data, tcase.owner) + err := ValidateHealthStatus(res) + require.NoError(t, err) + }) + } +} + +func TestValidateHealthStatus_ParseError(t *testing.T) { + // Any type other than the HealthStatus type would work + // to cause the error we are expecting + data := &pbcatalog.IP{Address: "198.18.0.1"} + + res := createHealthStatusResource(t, data, defaultHealthStatusOwner) + + err := ValidateHealthStatus(res) + require.Error(t, err) + require.ErrorAs(t, err, &resource.ErrDataParse{}) +} + +func TestValidateHealthStatus_InvalidHealth(t *testing.T) { + // while this is a valid enum value it is not allowed to be used + // as the Status field. + data := &pbcatalog.HealthStatus{ + Type: "tcp", + Status: pbcatalog.Health_HEALTH_ANY, + } + + res := createHealthStatusResource(t, data, defaultHealthStatusOwner) + + err := ValidateHealthStatus(res) + require.Error(t, err) + expected := resource.ErrInvalidField{ + Name: "status", + Wrapped: errInvalidHealth, + } + var actual resource.ErrInvalidField + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) +} + +func TestValidateHealthStatus_MissingType(t *testing.T) { + data := &pbcatalog.HealthStatus{ + Status: pbcatalog.Health_HEALTH_PASSING, + } + + res := createHealthStatusResource(t, data, defaultHealthStatusOwner) + + err := ValidateHealthStatus(res) + require.Error(t, err) + expected := resource.ErrInvalidField{ + Name: "type", + Wrapped: resource.ErrMissing, + } + var actual resource.ErrInvalidField + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) +} + +func TestValidateHealthStatus_MissingOwner(t *testing.T) { + data := &pbcatalog.HealthStatus{ + Type: "tcp", + Status: pbcatalog.Health_HEALTH_PASSING, + } + + res := createHealthStatusResource(t, data, nil) + + err := ValidateHealthStatus(res) + require.Error(t, err) + expected := resource.ErrInvalidField{ + Name: "owner", + Wrapped: resource.ErrMissing, + } + var actual resource.ErrInvalidField + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) +} + +func TestValidateHealthStatus_InvalidOwner(t *testing.T) { + data := &pbcatalog.HealthStatus{ + Type: "tcp", + Status: pbcatalog.Health_HEALTH_PASSING, + } + + type testCase struct { + owner *pbresource.ID + } + + cases := map[string]testCase{ + "group-mismatch": { + owner: &pbresource.ID{ + Type: &pbresource.Type{ + Group: "fake", + GroupVersion: CurrentVersion, + Kind: WorkloadKind, + }, + Tenancy: defaultHealthStatusOwnerTenancy, + Name: "baz", + }, + }, + "group-version-mismatch": { + owner: &pbresource.ID{ + Type: &pbresource.Type{ + Group: GroupName, + GroupVersion: "v99", + Kind: WorkloadKind, + }, + Tenancy: defaultHealthStatusOwnerTenancy, + Name: "baz", + }, + }, + "kind-mismatch": { + owner: &pbresource.ID{ + Type: ServiceType, + Tenancy: defaultHealthStatusOwnerTenancy, + Name: "baz", + }, + }, + } + + for name, tcase := range cases { + t.Run(name, func(t *testing.T) { + res := createHealthStatusResource(t, data, tcase.owner) + err := ValidateHealthStatus(res) + require.Error(t, err) + expected := resource.ErrOwnerInvalid{ + ResourceType: HealthStatusType, + OwnerType: tcase.owner.Type, + } + var actual resource.ErrOwnerInvalid + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) + }) + } +} diff --git a/internal/catalog/internal/types/node.go b/internal/catalog/internal/types/node.go index 09d8cca1ae..0d2c795aba 100644 --- a/internal/catalog/internal/types/node.go +++ b/internal/catalog/internal/types/node.go @@ -7,6 +7,7 @@ import ( "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 ( @@ -27,6 +28,62 @@ func RegisterNode(r resource.Registry) { r.Register(resource.Registration{ Type: NodeV1Alpha1Type, Proto: &pbcatalog.Node{}, - Validate: nil, + Validate: ValidateNode, }) } + +func ValidateNode(res *pbresource.Resource) error { + var node pbcatalog.Node + + if err := res.Data.UnmarshalTo(&node); err != nil { + return resource.NewErrDataParse(&node, err) + } + + var err error + // Validate that the node has at least 1 address + if len(node.Addresses) < 1 { + err = multierror.Append(err, resource.ErrInvalidField{ + Name: "addresses", + Wrapped: resource.ErrEmpty, + }) + } + + // Validate each node address + for idx, addr := range node.Addresses { + if addrErr := validateNodeAddress(addr); addrErr != nil { + err = multierror.Append(err, resource.ErrInvalidListElement{ + Name: "addresses", + Index: idx, + Wrapped: addrErr, + }) + } + } + + return err +} + +// Validates a single node address +func validateNodeAddress(addr *pbcatalog.NodeAddress) error { + // Currently the only field needing validation is the Host field. If that + // changes then this func should get updated to use a multierror.Append + // to collect the errors and return the overall set. + + // Check that the host is empty + if addr.Host == "" { + return resource.ErrInvalidField{ + Name: "host", + Wrapped: resource.ErrMissing, + } + } + + // Check if the host represents an IP address or a DNS name. Unix socket paths + // are not allowed for Node addresses unlike workload addresses. + if !isValidIPAddress(addr.Host) && !isValidDNSName(addr.Host) { + return resource.ErrInvalidField{ + Name: "host", + Wrapped: errInvalidNodeHostFormat{Host: addr.Host}, + } + } + + return nil +} diff --git a/internal/catalog/internal/types/node_test.go b/internal/catalog/internal/types/node_test.go new file mode 100644 index 0000000000..93e5ecea32 --- /dev/null +++ b/internal/catalog/internal/types/node_test.go @@ -0,0 +1,128 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +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" +) + +func createNodeResource(t *testing.T, data protoreflect.ProtoMessage) *pbresource.Resource { + res := &pbresource.Resource{ + Id: &pbresource.ID{ + Type: NodeType, + Tenancy: &pbresource.Tenancy{ + Partition: "default", + Namespace: "default", + PeerName: "local", + }, + Name: "test-node", + }, + } + + var err error + res.Data, err = anypb.New(data) + require.NoError(t, err) + return res +} + +func TestValidateNode_Ok(t *testing.T) { + data := &pbcatalog.Node{ + Addresses: []*pbcatalog.NodeAddress{ + { + Host: "198.18.0.1", + }, + { + Host: "fe80::316a:ed5b:f62c:7321", + }, + { + Host: "foo.node.example.com", + External: true, + }, + }, + } + + res := createNodeResource(t, data) + + err := ValidateNode(res) + require.NoError(t, err) +} + +func TestValidateNode_ParseError(t *testing.T) { + // Any type other than the Node type would work + // to cause the error we are expecting + data := &pbcatalog.IP{Address: "198.18.0.1"} + + res := createNodeResource(t, data) + + err := ValidateNode(res) + require.Error(t, err) + require.ErrorAs(t, err, &resource.ErrDataParse{}) +} + +func TestValidateNode_EmptyAddresses(t *testing.T) { + data := &pbcatalog.Node{} + + res := createNodeResource(t, data) + + err := ValidateNode(res) + require.Error(t, err) + expected := resource.ErrInvalidField{ + Name: "addresses", + Wrapped: resource.ErrEmpty, + } + var actual resource.ErrInvalidField + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) +} + +func TestValidateNode_InvalidAddress(t *testing.T) { + data := &pbcatalog.Node{ + Addresses: []*pbcatalog.NodeAddress{ + { + Host: "unix:///node.sock", + }, + }, + } + + res := createNodeResource(t, data) + + err := ValidateNode(res) + require.Error(t, err) + expected := resource.ErrInvalidField{ + Name: "host", + Wrapped: errInvalidNodeHostFormat{Host: "unix:///node.sock"}, + } + + var actual resource.ErrInvalidField + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) +} + +func TestValidateNode_AddressMissingHost(t *testing.T) { + data := &pbcatalog.Node{ + Addresses: []*pbcatalog.NodeAddress{ + {}, + }, + } + + res := createNodeResource(t, data) + + err := ValidateNode(res) + require.Error(t, err) + expected := resource.ErrInvalidField{ + Name: "host", + Wrapped: resource.ErrMissing, + } + + var actual resource.ErrInvalidField + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) +} diff --git a/internal/catalog/internal/types/service.go b/internal/catalog/internal/types/service.go index 78486128d2..f4da3cd8e4 100644 --- a/internal/catalog/internal/types/service.go +++ b/internal/catalog/internal/types/service.go @@ -4,9 +4,12 @@ package types import ( + "math" + "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 ( @@ -27,6 +30,90 @@ func RegisterService(r resource.Registry) { r.Register(resource.Registration{ Type: ServiceV1Alpha1Type, Proto: &pbcatalog.Service{}, - Validate: nil, + Validate: ValidateService, }) } + +func ValidateService(res *pbresource.Resource) error { + var service pbcatalog.Service + + if err := res.Data.UnmarshalTo(&service); err != nil { + return resource.NewErrDataParse(&service, err) + } + + var err error + + // Validate the workload selector. We are allowing selectors with no + // selection criteria as it will allow for users to manually manage + // ServiceEndpoints objects for this service such as when desiring to + // configure endpoint information for external services that are not + // registered as workloads + if selErr := validateSelector(service.Workloads, true); selErr != nil { + err = multierror.Append(err, resource.ErrInvalidField{ + Name: "workloads", + Wrapped: selErr, + }) + } + + usedVirtualPorts := make(map[uint32]int) + + // Validate each port + for idx, port := range service.Ports { + if usedIdx, found := usedVirtualPorts[port.VirtualPort]; found { + err = multierror.Append(err, resource.ErrInvalidListElement{ + Name: "ports", + Index: idx, + Wrapped: resource.ErrInvalidField{ + Name: "virtual_port", + Wrapped: errVirtualPortReused{ + Index: usedIdx, + Value: port.VirtualPort, + }, + }, + }) + } else { + usedVirtualPorts[port.VirtualPort] = idx + } + + // validate the target port + if nameErr := validatePortName(port.TargetPort); nameErr != nil { + err = multierror.Append(err, resource.ErrInvalidListElement{ + Name: "ports", + Index: idx, + Wrapped: resource.ErrInvalidField{ + Name: "target_port", + Wrapped: nameErr, + }, + }) + } + + // 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. + if port.VirtualPort > math.MaxUint16 { + err = multierror.Append(err, resource.ErrInvalidListElement{ + Name: "ports", + Index: idx, + Wrapped: resource.ErrInvalidField{ + Name: "virtual_port", + Wrapped: errInvalidVirtualPort, + }, + }) + } + + // basic protobuf deserialization should enforce that only known variants of the protocol field are set. + } + + // Validate that the Virtual IPs are all IP addresses + for idx, vip := range service.VirtualIps { + if vipErr := validateIPAddress(vip); vipErr != nil { + err = multierror.Append(err, resource.ErrInvalidListElement{ + Name: "virtual_ips", + Index: idx, + Wrapped: vipErr, + }) + } + } + + return err +} diff --git a/internal/catalog/internal/types/service_endpoints.go b/internal/catalog/internal/types/service_endpoints.go index bef9ca27ee..9a2ce37230 100644 --- a/internal/catalog/internal/types/service_endpoints.go +++ b/internal/catalog/internal/types/service_endpoints.go @@ -4,9 +4,12 @@ package types import ( + "math" + "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 ( @@ -30,3 +33,87 @@ func RegisterServiceEndpoints(r resource.Registry) { Validate: nil, }) } + +func ValidateServiceEndpoints(res *pbresource.Resource) error { + var svcEndpoints pbcatalog.ServiceEndpoints + + if err := res.Data.UnmarshalTo(&svcEndpoints); err != nil { + return resource.NewErrDataParse(&svcEndpoints, err) + } + + var err error + for idx, endpoint := range svcEndpoints.Endpoints { + if endpointErr := validateEndpoint(endpoint, res); endpointErr != nil { + err = multierror.Append(err, resource.ErrInvalidListElement{ + Name: "endpoints", + Index: idx, + Wrapped: endpointErr, + }) + } + } + + return err +} + +func validateEndpoint(endpoint *pbcatalog.Endpoint, res *pbresource.Resource) error { + var err error + + // Validate the target ref if not nil. When it is nil we are assuming that + // the endpoints are being managed for an external service that has no + // corresponding workloads that Consul has knowledge of. + if endpoint.TargetRef != nil { + // Validate the target reference + if refErr := validateReference(WorkloadType, res.Id.GetTenancy(), endpoint.TargetRef); refErr != nil { + err = multierror.Append(err, resource.ErrInvalidField{ + Name: "target_ref", + Wrapped: refErr, + }) + } + } + + // Validate the endpoint Addresses + for addrIdx, addr := range endpoint.Addresses { + if addrErr := validateWorkloadAddress(addr, endpoint.Ports); addrErr != nil { + err = multierror.Append(err, resource.ErrInvalidListElement{ + Name: "addresses", + Index: addrIdx, + Wrapped: addrErr, + }) + } + } + + // Ensure that the endpoint has at least 1 port. + if len(endpoint.Ports) < 1 { + err = multierror.Append(err, resource.ErrInvalidField{ + Name: "ports", + Wrapped: resource.ErrEmpty, + }) + } + + // Validate the endpoints ports + for portName, port := range endpoint.Ports { + // Port names must be DNS labels + if portNameErr := validatePortName(portName); portNameErr != nil { + err = multierror.Append(err, resource.ErrInvalidMapKey{ + Map: "ports", + Key: portName, + Wrapped: portNameErr, + }) + } + + // 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 { + err = multierror.Append(err, resource.ErrInvalidMapValue{ + Map: "ports", + Key: portName, + Wrapped: resource.ErrInvalidField{ + Name: "phsical_port", + Wrapped: errInvalidPhysicalPort, + }, + }) + } + } + + return err +} diff --git a/internal/catalog/internal/types/service_endpoints_test.go b/internal/catalog/internal/types/service_endpoints_test.go new file mode 100644 index 0000000000..bd902d6246 --- /dev/null +++ b/internal/catalog/internal/types/service_endpoints_test.go @@ -0,0 +1,184 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +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" +) + +var ( + defaultEndpointTenancy = &pbresource.Tenancy{ + Partition: "default", + Namespace: "default", + PeerName: "local", + } +) + +func createServiceEndpointsResource(t *testing.T, data protoreflect.ProtoMessage) *pbresource.Resource { + res := &pbresource.Resource{ + Id: &pbresource.ID{ + Type: ServiceEndpointsType, + Tenancy: defaultEndpointTenancy, + Name: "test-service", + }, + } + + var err error + res.Data, err = anypb.New(data) + require.NoError(t, err) + return res +} + +func TestValidateServiceEndpoints_Ok(t *testing.T) { + data := &pbcatalog.ServiceEndpoints{ + Endpoints: []*pbcatalog.Endpoint{ + { + TargetRef: &pbresource.ID{ + Type: WorkloadType, + Tenancy: defaultEndpointTenancy, + Name: "foo", + }, + Addresses: []*pbcatalog.WorkloadAddress{ + { + Host: "198.18.0.1", + }, + }, + Ports: map[string]*pbcatalog.WorkloadPort{ + "foo": { + Port: 8443, + Protocol: pbcatalog.Protocol_PROTOCOL_MESH, + }, + }, + HealthStatus: pbcatalog.Health_HEALTH_PASSING, + }, + }, + } + + res := createServiceEndpointsResource(t, data) + + err := ValidateServiceEndpoints(res) + require.NoError(t, err) +} + +func TestValidateServiceEndpoints_ParseError(t *testing.T) { + // Any type other than the ServiceEndpoints type would work + // to cause the error we are expecting + data := &pbcatalog.IP{Address: "198.18.0.1"} + + res := createServiceEndpointsResource(t, data) + + err := ValidateServiceEndpoints(res) + require.Error(t, err) + require.ErrorAs(t, err, &resource.ErrDataParse{}) +} + +func TestValidateServiceEndpoints_EndpointInvalid(t *testing.T) { + genData := func() *pbcatalog.Endpoint { + return &pbcatalog.Endpoint{ + TargetRef: &pbresource.ID{ + Type: WorkloadType, + Tenancy: defaultEndpointTenancy, + Name: "foo", + }, + Addresses: []*pbcatalog.WorkloadAddress{ + { + Host: "198.18.0.1", + Ports: []string{"foo"}, + }, + }, + Ports: map[string]*pbcatalog.WorkloadPort{ + "foo": { + Port: 42, + }, + }, + HealthStatus: pbcatalog.Health_HEALTH_PASSING, + } + } + + type testCase struct { + modify func(*pbcatalog.Endpoint) + validateErr func(t *testing.T, err error) + } + + cases := map[string]testCase{ + "invalid-target": { + modify: func(endpoint *pbcatalog.Endpoint) { + endpoint.TargetRef.Type = NodeType + }, + validateErr: func(t *testing.T, err error) { + require.ErrorIs(t, err, resource.ErrInvalidReferenceType{AllowedType: WorkloadType}) + }, + }, + "invalid-address": { + modify: func(endpoint *pbcatalog.Endpoint) { + endpoint.Addresses[0].Ports = []string{"bar"} + }, + validateErr: func(t *testing.T, err error) { + require.ErrorIs(t, err, errInvalidPortReference{Name: "bar"}) + }, + }, + "no-ports": { + modify: func(endpoint *pbcatalog.Endpoint) { + endpoint.Ports = nil + }, + validateErr: func(t *testing.T, err error) { + require.ErrorIs(t, err, resource.ErrEmpty) + }, + }, + "invalid-port-name": { + modify: func(endpoint *pbcatalog.Endpoint) { + endpoint.Ports[""] = &pbcatalog.WorkloadPort{ + Port: 42, + } + }, + validateErr: func(t *testing.T, err error) { + var mapErr resource.ErrInvalidMapKey + require.ErrorAs(t, err, &mapErr) + require.Equal(t, "ports", mapErr.Map) + require.Equal(t, "", mapErr.Key) + require.Equal(t, resource.ErrEmpty, mapErr.Wrapped) + }, + }, + "port-0": { + modify: func(endpoint *pbcatalog.Endpoint) { + endpoint.Ports["foo"].Port = 0 + }, + validateErr: func(t *testing.T, err error) { + require.ErrorIs(t, err, errInvalidPhysicalPort) + }, + }, + "port-out-of-range": { + modify: func(endpoint *pbcatalog.Endpoint) { + endpoint.Ports["foo"].Port = 65536 + }, + validateErr: func(t *testing.T, err error) { + require.ErrorIs(t, err, errInvalidPhysicalPort) + }, + }, + } + + for name, tcase := range cases { + t.Run(name, func(t *testing.T) { + data := genData() + tcase.modify(data) + + res := createServiceEndpointsResource(t, &pbcatalog.ServiceEndpoints{ + Endpoints: []*pbcatalog.Endpoint{ + data, + }, + }) + + err := ValidateServiceEndpoints(res) + require.Error(t, err) + tcase.validateErr(t, err) + }) + } +} diff --git a/internal/catalog/internal/types/service_test.go b/internal/catalog/internal/types/service_test.go new file mode 100644 index 0000000000..241a49939d --- /dev/null +++ b/internal/catalog/internal/types/service_test.go @@ -0,0 +1,200 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +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" +) + +func createServiceResource(t *testing.T, data protoreflect.ProtoMessage) *pbresource.Resource { + res := &pbresource.Resource{ + Id: &pbresource.ID{ + Type: ServiceType, + Tenancy: &pbresource.Tenancy{ + Partition: "default", + Namespace: "default", + PeerName: "local", + }, + Name: "test-policy", + }, + } + + var err error + res.Data, err = anypb.New(data) + require.NoError(t, err) + return res +} + +func TestValidateService_Ok(t *testing.T) { + data := &pbcatalog.Service{ + Workloads: &pbcatalog.WorkloadSelector{ + Names: []string{"foo", "bar"}, + Prefixes: []string{"abc-"}, + }, + Ports: []*pbcatalog.ServicePort{ + { + TargetPort: "http-internal", + VirtualPort: 42, + Protocol: pbcatalog.Protocol_PROTOCOL_HTTP, + }, + }, + VirtualIps: []string{"198.18.0.1"}, + } + + res := createServiceResource(t, data) + + err := ValidateService(res) + require.NoError(t, err) +} + +func TestValidateService_ParseError(t *testing.T) { + // Any type other than the Service type would work + // to cause the error we are expecting + data := &pbcatalog.IP{Address: "198.18.0.1"} + + res := createServiceResource(t, data) + + err := ValidateService(res) + require.Error(t, err) + require.ErrorAs(t, err, &resource.ErrDataParse{}) +} + +func TestValidateService_EmptySelector(t *testing.T) { + data := &pbcatalog.Service{ + Ports: []*pbcatalog.ServicePort{ + { + TargetPort: "http-internal", + Protocol: pbcatalog.Protocol_PROTOCOL_HTTP, + }, + }, + } + + res := createServiceResource(t, data) + + err := ValidateService(res) + require.NoError(t, err) +} + +func TestValidateService_InvalidSelector(t *testing.T) { + data := &pbcatalog.Service{ + Workloads: &pbcatalog.WorkloadSelector{ + Names: []string{""}, + }, + Ports: []*pbcatalog.ServicePort{ + { + TargetPort: "http-internal", + Protocol: pbcatalog.Protocol_PROTOCOL_HTTP, + }, + }, + } + + res := createServiceResource(t, data) + + err := ValidateService(res) + expected := resource.ErrInvalidListElement{ + Name: "names", + Index: 0, + Wrapped: resource.ErrEmpty, + } + + var actual resource.ErrInvalidListElement + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) +} + +func TestValidateService_InvalidTargetPort(t *testing.T) { + data := &pbcatalog.Service{ + Workloads: &pbcatalog.WorkloadSelector{ + Prefixes: []string{""}, + }, + Ports: []*pbcatalog.ServicePort{ + { + TargetPort: "", + }, + }, + } + + res := createServiceResource(t, data) + + err := ValidateService(res) + require.Error(t, err) + var actual resource.ErrInvalidField + require.ErrorAs(t, err, &actual) + require.Equal(t, "target_port", actual.Name) + require.Equal(t, resource.ErrEmpty, actual.Wrapped) +} + +func TestValidateService_VirtualPortReused(t *testing.T) { + data := &pbcatalog.Service{ + Workloads: &pbcatalog.WorkloadSelector{ + Prefixes: []string{""}, + }, + Ports: []*pbcatalog.ServicePort{ + { + VirtualPort: 42, + TargetPort: "foo", + }, + { + VirtualPort: 42, + TargetPort: "bar", + }, + }, + } + + res := createServiceResource(t, data) + + err := ValidateService(res) + require.Error(t, err) + var actual errVirtualPortReused + require.ErrorAs(t, err, &actual) + require.EqualValues(t, 0, actual.Index) + require.EqualValues(t, 42, actual.Value) +} + +func TestValidateService_VirtualPortInvalid(t *testing.T) { + data := &pbcatalog.Service{ + Workloads: &pbcatalog.WorkloadSelector{ + Prefixes: []string{""}, + }, + Ports: []*pbcatalog.ServicePort{ + { + VirtualPort: 100000, + TargetPort: "foo", + }, + }, + } + + res := createServiceResource(t, data) + + err := ValidateService(res) + require.Error(t, err) + require.ErrorIs(t, err, errInvalidVirtualPort) +} + +func TestValidateService_InvalidVIP(t *testing.T) { + data := &pbcatalog.Service{ + Workloads: &pbcatalog.WorkloadSelector{ + Prefixes: []string{""}, + }, + Ports: []*pbcatalog.ServicePort{ + { + TargetPort: "foo", + }, + }, + VirtualIps: []string{"foo"}, + } + + res := createServiceResource(t, data) + + err := ValidateService(res) + require.Error(t, err) + require.ErrorIs(t, err, errNotIPAddress) +} diff --git a/internal/catalog/internal/types/testdata/errDNSPassingWeightOutOfRange.golden b/internal/catalog/internal/types/testdata/errDNSPassingWeightOutOfRange.golden new file mode 100644 index 0000000000..c364e6a175 --- /dev/null +++ b/internal/catalog/internal/types/testdata/errDNSPassingWeightOutOfRange.golden @@ -0,0 +1 @@ +DNS passing weight is outside of the range 1 to 65535 \ No newline at end of file diff --git a/internal/catalog/internal/types/testdata/errDNSWarningWeightOutOfRange.golden b/internal/catalog/internal/types/testdata/errDNSWarningWeightOutOfRange.golden new file mode 100644 index 0000000000..0457e418b4 --- /dev/null +++ b/internal/catalog/internal/types/testdata/errDNSWarningWeightOutOfRange.golden @@ -0,0 +1 @@ +DNS warning weight is outside the range 0 to 65535 \ No newline at end of file diff --git a/internal/catalog/internal/types/testdata/errInvalidNodeHostFormat.golden b/internal/catalog/internal/types/testdata/errInvalidNodeHostFormat.golden new file mode 100644 index 0000000000..6eb2e24ebd --- /dev/null +++ b/internal/catalog/internal/types/testdata/errInvalidNodeHostFormat.golden @@ -0,0 +1 @@ +"unix:///node.sock" is not an IP address or a DNS name. \ No newline at end of file diff --git a/internal/catalog/internal/types/testdata/errInvalidPhysicalPort.golden b/internal/catalog/internal/types/testdata/errInvalidPhysicalPort.golden new file mode 100644 index 0000000000..eb971cb8a3 --- /dev/null +++ b/internal/catalog/internal/types/testdata/errInvalidPhysicalPort.golden @@ -0,0 +1 @@ +port number is outside the range 1 to 65535 \ No newline at end of file diff --git a/internal/catalog/internal/types/testdata/errInvalidPortReference.golden b/internal/catalog/internal/types/testdata/errInvalidPortReference.golden new file mode 100644 index 0000000000..3a165011c5 --- /dev/null +++ b/internal/catalog/internal/types/testdata/errInvalidPortReference.golden @@ -0,0 +1 @@ +port with name "http" has not been defined \ No newline at end of file diff --git a/internal/catalog/internal/types/testdata/errInvalidVirtualPort.golden b/internal/catalog/internal/types/testdata/errInvalidVirtualPort.golden new file mode 100644 index 0000000000..2cabd9cb28 --- /dev/null +++ b/internal/catalog/internal/types/testdata/errInvalidVirtualPort.golden @@ -0,0 +1 @@ +port number is outside the range 0 to 65535 \ No newline at end of file diff --git a/internal/catalog/internal/types/testdata/errInvalidWorkloadHostFormat.golden b/internal/catalog/internal/types/testdata/errInvalidWorkloadHostFormat.golden new file mode 100644 index 0000000000..eb57dc2ef0 --- /dev/null +++ b/internal/catalog/internal/types/testdata/errInvalidWorkloadHostFormat.golden @@ -0,0 +1 @@ +"-foo-bar-" is not an IP address, Unix socket path or a DNS name. \ No newline at end of file diff --git a/internal/catalog/internal/types/testdata/errLocalityZoneNoRegion.golden b/internal/catalog/internal/types/testdata/errLocalityZoneNoRegion.golden new file mode 100644 index 0000000000..9a6ec59d08 --- /dev/null +++ b/internal/catalog/internal/types/testdata/errLocalityZoneNoRegion.golden @@ -0,0 +1 @@ +locality region cannot be empty if the zone is set \ No newline at end of file diff --git a/internal/catalog/internal/types/testdata/errNotDNSLabel.golden b/internal/catalog/internal/types/testdata/errNotDNSLabel.golden new file mode 100644 index 0000000000..a5866fbbf0 --- /dev/null +++ b/internal/catalog/internal/types/testdata/errNotDNSLabel.golden @@ -0,0 +1 @@ +value must match regex: ^[a-z0-9]([a-z0-9\-_]*[a-z0-9])?$ \ No newline at end of file diff --git a/internal/catalog/internal/types/testdata/errNotIPAddress.golden b/internal/catalog/internal/types/testdata/errNotIPAddress.golden new file mode 100644 index 0000000000..db33684367 --- /dev/null +++ b/internal/catalog/internal/types/testdata/errNotIPAddress.golden @@ -0,0 +1 @@ +value is not a valid IP address \ No newline at end of file diff --git a/internal/catalog/internal/types/testdata/errTooMuchMesh.golden b/internal/catalog/internal/types/testdata/errTooMuchMesh.golden new file mode 100644 index 0000000000..48ce0969d8 --- /dev/null +++ b/internal/catalog/internal/types/testdata/errTooMuchMesh.golden @@ -0,0 +1 @@ +protocol "mesh" was specified in more than 1 port: [http grpc] \ No newline at end of file diff --git a/internal/catalog/internal/types/testdata/errUnixSocketMultiport.golden b/internal/catalog/internal/types/testdata/errUnixSocketMultiport.golden new file mode 100644 index 0000000000..c28c2f0695 --- /dev/null +++ b/internal/catalog/internal/types/testdata/errUnixSocketMultiport.golden @@ -0,0 +1 @@ +Unix socket address references more than one port \ No newline at end of file diff --git a/internal/catalog/internal/types/testdata/errVirtualPortReused.golden b/internal/catalog/internal/types/testdata/errVirtualPortReused.golden new file mode 100644 index 0000000000..5a8c8b799b --- /dev/null +++ b/internal/catalog/internal/types/testdata/errVirtualPortReused.golden @@ -0,0 +1 @@ +virtual port 8080 was previously assigned at index 3 \ No newline at end of file diff --git a/internal/catalog/internal/types/validators.go b/internal/catalog/internal/types/validators.go new file mode 100644 index 0000000000..e9fc085627 --- /dev/null +++ b/internal/catalog/internal/types/validators.go @@ -0,0 +1,209 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package types + +import ( + "net" + "regexp" + "strings" + + "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 ( + // 108 characters is the max size that Linux (and probably other OSes) will + // allow for the length of the Unix socket path. + maxUnixSocketPathLen = 108 +) + +var ( + dnsLabelRegex = `^[a-z0-9]([a-z0-9\-_]*[a-z0-9])?$` + dnsLabelMatcher = regexp.MustCompile(dnsLabelRegex) +) + +func isValidIPAddress(host string) bool { + return net.ParseIP(host) != nil +} + +func isValidDNSName(host string) bool { + if len(host) > 256 { + return false + } + + labels := strings.Split(host, ".") + for _, label := range labels { + if !isValidDNSLabel(label) { + return false + } + } + + return true +} + +func isValidDNSLabel(label string) bool { + if len(label) > 64 { + return false + } + + return dnsLabelMatcher.Match([]byte(label)) +} + +func isValidUnixSocketPath(host string) bool { + if len(host) > maxUnixSocketPathLen || !strings.HasPrefix(host, "unix://") || strings.Contains(host, "\000") { + return false + } + + return true +} + +func validateWorkloadHost(host string) error { + // Check that the host is empty + if host == "" { + return errInvalidWorkloadHostFormat{Host: host} + } + + // Check if the host represents an IP address, unix socket path or a DNS name + if !isValidIPAddress(host) && !isValidUnixSocketPath(host) && !isValidDNSName(host) { + return errInvalidWorkloadHostFormat{Host: host} + } + + return nil +} + +func validateSelector(sel *pbcatalog.WorkloadSelector, allowEmpty bool) error { + if sel == nil { + if allowEmpty { + return nil + } + + return resource.ErrEmpty + } + + if len(sel.Names) == 0 && len(sel.Prefixes) == 0 { + if allowEmpty { + return nil + } + + return resource.ErrEmpty + } + + var err error + + // Validate that all the exact match names are non-empty. This is + // mostly for the sake of not admitting values that should always + // be meaningless and never actually cause selection of a workload. + // This is because workloads must have non-empty names. + for idx, name := range sel.Names { + if name == "" { + err = multierror.Append(err, resource.ErrInvalidListElement{ + Name: "names", + Index: idx, + Wrapped: resource.ErrEmpty, + }) + } + } + + return err +} + +func validateIPAddress(ip string) error { + if ip == "" { + return resource.ErrEmpty + } + + if !isValidIPAddress(ip) { + return errNotIPAddress + } + + return nil +} + +func validatePortName(name string) error { + if name == "" { + return resource.ErrEmpty + } + + if !isValidDNSLabel(name) { + return errNotDNSLabel + } + + return nil +} + +// 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. +// Additionally for UNIX socket addresses we ensure that they specify only 1 port either +// explicitly in their ports references or implicitly by omitting the references and there +// only being 1 value in the ports map. +func validateWorkloadAddress(addr *pbcatalog.WorkloadAddress, ports map[string]*pbcatalog.WorkloadPort) error { + var err error + + if hostErr := validateWorkloadHost(addr.Host); hostErr != nil { + err = multierror.Append(err, resource.ErrInvalidField{ + Name: "host", + Wrapped: hostErr, + }) + } + + // 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) && + (len(addr.Ports) > 1 || (len(addr.Ports) == 0 && len(ports) > 1)) { + err = multierror.Append(err, errUnixSocketMultiport) + } + + // Check that all referenced ports exist + for idx, port := range addr.Ports { + _, found := ports[port] + if !found { + err = multierror.Append(err, resource.ErrInvalidListElement{ + Name: "ports", + Index: idx, + Wrapped: errInvalidPortReference{Name: port}, + }) + } + } + return err +} + +func validateReferenceType(allowed *pbresource.Type, check *pbresource.Type) error { + if allowed.Group == check.Group && + allowed.GroupVersion == check.GroupVersion && + allowed.Kind == check.Kind { + return nil + } + + return resource.ErrInvalidReferenceType{ + AllowedType: allowed, + } +} + +func validateReferenceTenancy(allowed *pbresource.Tenancy, check *pbresource.Tenancy) error { + if proto.Equal(allowed, check) { + return nil + } + + return resource.ErrReferenceTenancyNotEqual +} + +func validateReference(allowedType *pbresource.Type, allowedTenancy *pbresource.Tenancy, check *pbresource.ID) error { + var err error + + // Validate the references type is the allowed type. + if typeErr := validateReferenceType(allowedType, check.GetType()); typeErr != nil { + err = multierror.Append(err, typeErr) + } + + // Validate the references tenancy matches the allowed tenancy. + if tenancyErr := validateReferenceTenancy(allowedTenancy, check.GetTenancy()); tenancyErr != nil { + err = multierror.Append(err, tenancyErr) + } + + return err +} diff --git a/internal/catalog/internal/types/validators_test.go b/internal/catalog/internal/types/validators_test.go new file mode 100644 index 0000000000..cd3a12c5c0 --- /dev/null +++ b/internal/catalog/internal/types/validators_test.go @@ -0,0 +1,595 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package types + +import ( + "fmt" + "strings" + "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/hashicorp/go-multierror" + "github.com/stretchr/testify/require" +) + +func TestIsValidDNSLabel(t *testing.T) { + type testCase struct { + name string + valid bool + } + + cases := map[string]testCase{ + "min-length": { + name: "a", + valid: true, + }, + "max-length": { + name: strings.Repeat("a1b2c3d4", 8), + valid: true, + }, + "underscores": { + name: "has_underscores", + valid: true, + }, + "hyphenated": { + name: "has-hyphen3", + valid: true, + }, + "uppercase-not-allowed": { + name: "UPPERCASE", + valid: false, + }, + "numeric-start": { + name: "1abc", + valid: true, + }, + "underscore-start-not-allowed": { + name: "_abc", + valid: false, + }, + "hyphen-start-not-allowed": { + name: "-abc", + valid: false, + }, + "underscore-end-not-allowed": { + name: "abc_", + valid: false, + }, + "hyphen-end-not-allowed": { + name: "abc-", + valid: false, + }, + "unicode-not allowed": { + name: "abc∑", + valid: false, + }, + "too-long": { + name: strings.Repeat("aaaaaaaaa", 8), + valid: false, + }, + "missing-name": { + name: "", + valid: false, + }, + } + + for name, tcase := range cases { + t.Run(name, func(t *testing.T) { + require.Equal(t, tcase.valid, isValidDNSLabel(tcase.name)) + }) + } +} + +func TestIsValidDNSName(t *testing.T) { + // TestIsValidDNSLabel tests all of the individual label matching + // criteria. This test therefore is more limited to just the extra + // validations that IsValidDNSName does. Mainly that it ensures + // the overall length is less than 256 and that generally is made + // up of DNS labels joined with '.' + + type testCase struct { + name string + valid bool + } + + cases := map[string]testCase{ + "valid": { + name: "foo-something.example3.com", + valid: true, + }, + "exceeds-max-length": { + name: strings.Repeat("aaaa.aaaa", 29), + valid: false, + }, + "invalid-label": { + name: "foo._bar.com", + valid: false, + }, + } + + for name, tcase := range cases { + t.Run(name, func(t *testing.T) { + require.Equal(t, tcase.valid, isValidDNSName(tcase.name)) + }) + } +} + +func TestIsValidIPAddress(t *testing.T) { + type testCase struct { + name string + valid bool + } + + cases := map[string]testCase{ + "ipv4": { + name: "192.168.1.2", + valid: true, + }, + "ipv6": { + name: "2001:db8::1", + valid: true, + }, + "ipv4-mapped-ipv6": { + name: "::ffff:192.0.2.128", + valid: true, + }, + "invalid": { + name: "foo", + valid: false, + }, + } + + for name, tcase := range cases { + t.Run(name, func(t *testing.T) { + require.Equal(t, tcase.valid, isValidIPAddress(tcase.name)) + }) + } +} + +func TestIsValidUnixSocketPath(t *testing.T) { + type testCase struct { + name string + valid bool + } + + cases := map[string]testCase{ + "valid": { + name: "unix:///foo/bar.sock", + valid: true, + }, + "missing-prefix": { + name: "/foo/bar.sock", + valid: false, + }, + "too-long": { + name: fmt.Sprintf("unix://%s/bar.sock", strings.Repeat("/aaaaaaaaaa", 11)), + valid: false, + }, + "nul-in-name": { + name: "unix:///foo/bar\000sock", + valid: false, + }, + } + + for name, tcase := range cases { + t.Run(name, func(t *testing.T) { + require.Equal(t, tcase.valid, isValidUnixSocketPath(tcase.name)) + }) + } +} + +func TestValidateHost(t *testing.T) { + type testCase struct { + name string + valid bool + } + + cases := map[string]testCase{ + "ip-address": { + name: "198.18.0.1", + valid: true, + }, + "unix-socket": { + name: "unix:///foo.sock", + valid: true, + }, + "dns-name": { + name: "foo.com", + valid: true, + }, + "host-empty": { + name: "", + valid: false, + }, + "host-invalid": { + name: "unix:///foo/bar\000sock", + valid: false, + }, + } + + for name, tcase := range cases { + t.Run(name, func(t *testing.T) { + err := validateWorkloadHost(tcase.name) + if tcase.valid { + require.NoError(t, err) + } else { + require.Error(t, err) + require.Equal(t, errInvalidWorkloadHostFormat{Host: tcase.name}, err) + } + }) + } +} + +func TestValidateSelector(t *testing.T) { + type testCase struct { + selector *pbcatalog.WorkloadSelector + allowEmpty bool + err error + } + + cases := map[string]testCase{ + "nil-disallowed": { + selector: nil, + allowEmpty: false, + err: resource.ErrEmpty, + }, + "nil-allowed": { + selector: nil, + allowEmpty: true, + err: nil, + }, + "empty-allowed": { + selector: &pbcatalog.WorkloadSelector{}, + allowEmpty: true, + err: nil, + }, + "empty-disallowed": { + selector: &pbcatalog.WorkloadSelector{}, + allowEmpty: false, + err: resource.ErrEmpty, + }, + "ok": { + selector: &pbcatalog.WorkloadSelector{ + Names: []string{"foo", "bar"}, + Prefixes: []string{"foo", "bar"}, + }, + allowEmpty: false, + err: nil, + }, + "empty-name": { + selector: &pbcatalog.WorkloadSelector{ + Names: []string{"", "bar", ""}, + Prefixes: []string{"foo", "bar"}, + }, + allowEmpty: false, + err: &multierror.Error{ + Errors: []error{ + resource.ErrInvalidListElement{ + Name: "names", + Index: 0, + Wrapped: resource.ErrEmpty, + }, + resource.ErrInvalidListElement{ + Name: "names", + Index: 2, + Wrapped: resource.ErrEmpty, + }, + }, + }, + }, + } + + for name, tcase := range cases { + t.Run(name, func(t *testing.T) { + err := validateSelector(tcase.selector, tcase.allowEmpty) + if tcase.err == nil { + require.NoError(t, err) + } else { + require.Error(t, err) + require.Equal(t, tcase.err, err) + } + }) + } +} + +func TestValidateIPAddress(t *testing.T) { + // this test does not perform extensive validation of what constitutes + // an IP address. Instead that is handled in the test for the + // isValidIPAddress function + + t.Run("empty", func(t *testing.T) { + require.Equal(t, resource.ErrEmpty, validateIPAddress("")) + }) + + t.Run("invalid", func(t *testing.T) { + require.Equal(t, errNotIPAddress, validateIPAddress("foo.com")) + }) + + t.Run("ok", func(t *testing.T) { + require.NoError(t, validateIPAddress("192.168.0.1")) + }) +} + +func TestValidatePortName(t *testing.T) { + // this test does not perform extensive validation of what constitutes + // a valid port name. In general the criteria is that it must not + // be empty and must be a valid DNS label. Therefore extensive testing + // of what it means to be a valid DNS label is performed within the + // test for the isValidDNSLabel function. + + t.Run("empty", func(t *testing.T) { + require.Equal(t, resource.ErrEmpty, validatePortName("")) + }) + + t.Run("invalid", func(t *testing.T) { + require.Equal(t, errNotDNSLabel, validatePortName("foo.com")) + }) + + t.Run("ok", func(t *testing.T) { + require.NoError(t, validatePortName("http")) + }) +} + +func TestValidateWorkloadAddress(t *testing.T) { + type testCase struct { + addr *pbcatalog.WorkloadAddress + ports map[string]*pbcatalog.WorkloadPort + validateErr func(*testing.T, error) + } + + cases := map[string]testCase{ + "invalid-host": { + addr: &pbcatalog.WorkloadAddress{ + Host: "-blarg", + }, + ports: map[string]*pbcatalog.WorkloadPort{}, + validateErr: func(t *testing.T, err error) { + var actual resource.ErrInvalidField + require.ErrorAs(t, err, &actual) + require.Equal(t, "host", actual.Name) + }, + }, + "unix-socket-multiport-explicit": { + addr: &pbcatalog.WorkloadAddress{ + Host: "unix:///foo.sock", + Ports: []string{"foo", "bar"}, + }, + ports: map[string]*pbcatalog.WorkloadPort{ + "foo": {}, + "bar": {}, + }, + validateErr: func(t *testing.T, err error) { + require.ErrorIs(t, err, errUnixSocketMultiport) + }, + }, + "unix-socket-multiport-implicit": { + addr: &pbcatalog.WorkloadAddress{ + Host: "unix:///foo.sock", + }, + ports: map[string]*pbcatalog.WorkloadPort{ + "foo": {}, + "bar": {}, + }, + validateErr: func(t *testing.T, err error) { + require.ErrorIs(t, err, errUnixSocketMultiport) + }, + }, + + "unix-socket-singleport": { + addr: &pbcatalog.WorkloadAddress{ + Host: "unix:///foo.sock", + Ports: []string{"foo"}, + }, + ports: map[string]*pbcatalog.WorkloadPort{ + "foo": {}, + "bar": {}, + }, + }, + "invalid-port-reference": { + addr: &pbcatalog.WorkloadAddress{ + Host: "198.18.0.1", + Ports: []string{"foo"}, + }, + ports: map[string]*pbcatalog.WorkloadPort{ + "http": {}, + }, + validateErr: func(t *testing.T, err error) { + var actual errInvalidPortReference + require.ErrorAs(t, err, &actual) + require.Equal(t, "foo", actual.Name) + }, + }, + "ok": { + addr: &pbcatalog.WorkloadAddress{ + Host: "198.18.0.1", + Ports: []string{"http", "grpc"}, + External: true, + }, + ports: map[string]*pbcatalog.WorkloadPort{ + "http": {}, + "grpc": {}, + }, + }, + } + + for name, tcase := range cases { + t.Run(name, func(t *testing.T) { + err := validateWorkloadAddress(tcase.addr, tcase.ports) + if tcase.validateErr != nil { + require.Error(t, err) + tcase.validateErr(t, err) + } else { + require.NoError(t, err) + } + }) + } +} + +func TestValidateReferenceType(t *testing.T) { + allowedType := &pbresource.Type{ + Group: "foo", + GroupVersion: "v1", + Kind: "Bar", + } + + type testCase struct { + check *pbresource.Type + err bool + } + + cases := map[string]testCase{ + "ok": { + check: allowedType, + err: false, + }, + "group-mismatch": { + check: &pbresource.Type{ + Group: "food", + GroupVersion: "v1", + Kind: "Bar", + }, + err: true, + }, + "group-version-mismatch": { + check: &pbresource.Type{ + Group: "foo", + GroupVersion: "v2", + Kind: "Bar", + }, + err: true, + }, + "kind-mismatch": { + check: &pbresource.Type{ + Group: "foo", + GroupVersion: "v1", + Kind: "Baz", + }, + err: true, + }, + } + + for name, tcase := range cases { + t.Run(name, func(t *testing.T) { + err := validateReferenceType(allowedType, tcase.check) + if tcase.err { + require.Equal(t, resource.ErrInvalidReferenceType{AllowedType: allowedType}, err) + } else { + require.NoError(t, err) + } + }) + } +} + +func TestValidateReferenceTenancy(t *testing.T) { + allowedTenancy := &pbresource.Tenancy{ + Partition: "default", + Namespace: "default", + PeerName: "local", + } + + type testCase struct { + check *pbresource.Tenancy + err bool + } + + cases := map[string]testCase{ + "ok": { + check: allowedTenancy, + err: false, + }, + "partition-mismatch": { + check: &pbresource.Tenancy{ + Partition: "food", + Namespace: "default", + PeerName: "local", + }, + err: true, + }, + "group-version-mismatch": { + check: &pbresource.Tenancy{ + Partition: "default", + Namespace: "v2", + PeerName: "local", + }, + err: true, + }, + "kind-mismatch": { + check: &pbresource.Tenancy{ + Partition: "default", + Namespace: "default", + PeerName: "Baz", + }, + err: true, + }, + } + + for name, tcase := range cases { + t.Run(name, func(t *testing.T) { + err := validateReferenceTenancy(allowedTenancy, tcase.check) + if tcase.err { + require.Equal(t, resource.ErrReferenceTenancyNotEqual, err) + } else { + require.NoError(t, err) + } + }) + } +} + +func TestValidateReference(t *testing.T) { + allowedTenancy := &pbresource.Tenancy{ + Partition: "default", + Namespace: "default", + PeerName: "local", + } + + allowedType := WorkloadType + + type testCase struct { + check *pbresource.ID + err error + } + + cases := map[string]testCase{ + "ok": { + check: &pbresource.ID{ + Type: allowedType, + Tenancy: allowedTenancy, + Name: "foo", + }, + }, + "type-err": { + check: &pbresource.ID{ + Type: NodeType, + Tenancy: allowedTenancy, + Name: "foo", + }, + err: resource.ErrInvalidReferenceType{AllowedType: allowedType}, + }, + "tenancy-mismatch": { + check: &pbresource.ID{ + Type: allowedType, + Tenancy: &pbresource.Tenancy{ + Partition: "foo", + Namespace: "bar", + PeerName: "baz", + }, + Name: "foo", + }, + err: resource.ErrReferenceTenancyNotEqual, + }, + } + + for name, tcase := range cases { + t.Run(name, func(t *testing.T) { + err := validateReference(allowedType, allowedTenancy, tcase.check) + if tcase.err != nil { + require.ErrorIs(t, err, tcase.err) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/internal/catalog/internal/types/virtual_ips.go b/internal/catalog/internal/types/virtual_ips.go index 4a114fc800..a27f08df0a 100644 --- a/internal/catalog/internal/types/virtual_ips.go +++ b/internal/catalog/internal/types/virtual_ips.go @@ -7,6 +7,7 @@ import ( "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 ( @@ -27,6 +28,29 @@ func RegisterVirtualIPs(r resource.Registry) { r.Register(resource.Registration{ Type: VirtualIPsV1Alpha1Type, Proto: &pbcatalog.VirtualIPs{}, - Validate: nil, + Validate: ValidateVirtualIPs, }) } + +func ValidateVirtualIPs(res *pbresource.Resource) error { + var vips pbcatalog.VirtualIPs + + if err := res.Data.UnmarshalTo(&vips); err != nil { + return resource.NewErrDataParse(&vips, err) + } + + var err error + for idx, ip := range vips.Ips { + if vipErr := validateIPAddress(ip.Address); vipErr != nil { + err = multierror.Append(err, resource.ErrInvalidListElement{ + Name: "ips", + Index: idx, + Wrapped: resource.ErrInvalidField{ + Name: "address", + Wrapped: vipErr, + }, + }) + } + } + return err +} diff --git a/internal/catalog/internal/types/virtual_ips_test.go b/internal/catalog/internal/types/virtual_ips_test.go new file mode 100644 index 0000000000..76d55e7ecd --- /dev/null +++ b/internal/catalog/internal/types/virtual_ips_test.go @@ -0,0 +1,82 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +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" +) + +func createVirtualIPsResource(t *testing.T, data protoreflect.ProtoMessage) *pbresource.Resource { + res := &pbresource.Resource{ + Id: &pbresource.ID{ + Type: VirtualIPsType, + Tenancy: &pbresource.Tenancy{ + Partition: "default", + Namespace: "default", + PeerName: "local", + }, + Name: "test-vip", + }, + } + + var err error + res.Data, err = anypb.New(data) + require.NoError(t, err) + return res +} + +func TestValidateVirtualIPs_Ok(t *testing.T) { + data := &pbcatalog.VirtualIPs{ + Ips: []*pbcatalog.IP{ + { + Address: "::ffff:192.0.2.128", + Generated: true, + }, + { + Address: "10.0.1.42", + Generated: false, + }, + }, + } + + res := createVirtualIPsResource(t, data) + + err := ValidateVirtualIPs(res) + require.NoError(t, err) +} + +func TestValidateVirtualIPs_ParseError(t *testing.T) { + // Any type other than the VirtualIPs type would work + // to cause the error we are expecting + data := &pbcatalog.IP{Address: "198.18.0.1"} + + res := createVirtualIPsResource(t, data) + + err := ValidateVirtualIPs(res) + require.Error(t, err) + require.ErrorAs(t, err, &resource.ErrDataParse{}) +} + +func TestValidateVirtualIPs_InvalidIP(t *testing.T) { + data := &pbcatalog.VirtualIPs{ + Ips: []*pbcatalog.IP{ + { + Address: "foo", + }, + }, + } + + res := createVirtualIPsResource(t, data) + + err := ValidateVirtualIPs(res) + require.Error(t, err) + require.ErrorIs(t, err, errNotIPAddress) +} diff --git a/internal/catalog/internal/types/workload.go b/internal/catalog/internal/types/workload.go index 0fa742a532..a0dc7142d1 100644 --- a/internal/catalog/internal/types/workload.go +++ b/internal/catalog/internal/types/workload.go @@ -4,9 +4,13 @@ package types import ( + "math" + "sort" + "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 ( @@ -27,6 +31,118 @@ func RegisterWorkload(r resource.Registry) { r.Register(resource.Registration{ Type: WorkloadV1Alpha1Type, Proto: &pbcatalog.Workload{}, - Validate: nil, + Validate: ValidateWorkload, }) } + +func ValidateWorkload(res *pbresource.Resource) error { + var workload pbcatalog.Workload + + if err := res.Data.UnmarshalTo(&workload); err != nil { + return resource.NewErrDataParse(&workload, err) + } + + var err error + + // Validate that the workload has at least one port + if len(workload.Ports) < 1 { + err = multierror.Append(err, resource.ErrInvalidField{ + Name: "ports", + Wrapped: resource.ErrEmpty, + }) + } + + var meshPorts []string + + // Validate the Workload Ports + for portName, port := range workload.Ports { + if portNameErr := validatePortName(portName); portNameErr != nil { + err = multierror.Append(err, resource.ErrInvalidMapKey{ + Map: "ports", + Key: portName, + Wrapped: portNameErr, + }) + } + + // disallow port 0 for now + if port.Port < 1 || port.Port > math.MaxUint16 { + err = multierror.Append(err, resource.ErrInvalidMapValue{ + Map: "ports", + Key: portName, + Wrapped: resource.ErrInvalidField{ + Name: "port", + Wrapped: errInvalidPhysicalPort, + }, + }) + } + + // Collect the list of mesh ports + if port.Protocol == pbcatalog.Protocol_PROTOCOL_MESH { + meshPorts = append(meshPorts, portName) + } + } + + if len(meshPorts) > 1 { + sort.Strings(meshPorts) + err = multierror.Append(err, resource.ErrInvalidField{ + Name: "ports", + Wrapped: errTooMuchMesh{ + Ports: meshPorts, + }, + }) + } + + // If the workload is mesh enabled then a valid identity must be provided. + // If not mesh enabled but a non-empty identity is provided then we still + // validate that its valid. + if len(meshPorts) > 0 && workload.Identity == "" { + err = multierror.Append(err, resource.ErrInvalidField{ + Name: "identity", + Wrapped: resource.ErrMissing, + }) + } else if workload.Identity != "" && !isValidDNSLabel(workload.Identity) { + err = multierror.Append(err, resource.ErrInvalidField{ + Name: "identity", + Wrapped: errNotDNSLabel, + }) + } + + // Validate workload locality + if workload.Locality != nil && workload.Locality.Region == "" && workload.Locality.Zone != "" { + err = multierror.Append(err, resource.ErrInvalidField{ + Name: "locality", + Wrapped: errLocalityZoneNoRegion, + }) + } + + // Node associations are optional but if present the name should + // be a valid DNS label. + if workload.NodeName != "" { + if !isValidDNSLabel(workload.NodeName) { + err = multierror.Append(err, resource.ErrInvalidField{ + Name: "node_name", + Wrapped: errNotDNSLabel, + }) + } + } + + if len(workload.Addresses) < 1 { + err = multierror.Append(err, resource.ErrInvalidField{ + Name: "addresses", + Wrapped: resource.ErrEmpty, + }) + } + + // Validate Workload Addresses + for idx, addr := range workload.Addresses { + if addrErr := validateWorkloadAddress(addr, workload.Ports); addrErr != nil { + err = multierror.Append(err, resource.ErrInvalidListElement{ + Name: "addresses", + Index: idx, + Wrapped: addrErr, + }) + } + } + + return err +} diff --git a/internal/catalog/internal/types/workload_test.go b/internal/catalog/internal/types/workload_test.go new file mode 100644 index 0000000000..03662472f2 --- /dev/null +++ b/internal/catalog/internal/types/workload_test.go @@ -0,0 +1,281 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +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" +) + +func createWorkloadResource(t *testing.T, data protoreflect.ProtoMessage) *pbresource.Resource { + res := &pbresource.Resource{ + Id: &pbresource.ID{ + Type: WorkloadType, + Tenancy: &pbresource.Tenancy{ + Partition: "default", + Namespace: "default", + PeerName: "local", + }, + Name: "api-1234", + }, + } + + var err error + res.Data, err = anypb.New(data) + require.NoError(t, err) + return res +} + +func validWorkload() *pbcatalog.Workload { + return &pbcatalog.Workload{ + Addresses: []*pbcatalog.WorkloadAddress{ + { + Host: "127.0.0.1", + }, + }, + Ports: map[string]*pbcatalog.WorkloadPort{ + "http": { + Port: 8443, + Protocol: pbcatalog.Protocol_PROTOCOL_HTTP2, + }, + }, + NodeName: "foo", + Identity: "api", + Locality: &pbcatalog.Locality{ + Region: "us-east-1", + Zone: "1a", + }, + } +} + +func TestValidateWorkload_Ok(t *testing.T) { + res := createWorkloadResource(t, validWorkload()) + + err := ValidateWorkload(res) + require.NoError(t, err) +} + +func TestValidateWorkload_ParseError(t *testing.T) { + // Any type other than the Workload type would work + // to cause the error we are expecting + data := &pbcatalog.IP{Address: "198.18.0.1"} + + res := createWorkloadResource(t, data) + + err := ValidateWorkload(res) + require.Error(t, err) + require.ErrorAs(t, err, &resource.ErrDataParse{}) +} + +func TestValidateWorkload_EmptyAddresses(t *testing.T) { + data := validWorkload() + data.Addresses = nil + + res := createWorkloadResource(t, data) + + err := ValidateWorkload(res) + require.Error(t, err) + expected := resource.ErrInvalidField{ + Name: "addresses", + Wrapped: resource.ErrEmpty, + } + var actual resource.ErrInvalidField + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) +} + +func TestValidateWorkload_InvalidAddress(t *testing.T) { + data := validWorkload() + data.Addresses[0].Host = "-not-a-host" + + res := createWorkloadResource(t, data) + + err := ValidateWorkload(res) + require.Error(t, err) + expected := resource.ErrInvalidField{ + Name: "host", + Wrapped: errInvalidWorkloadHostFormat{Host: "-not-a-host"}, + } + + var actual resource.ErrInvalidField + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) +} + +func TestValidateWorkload_MissingIdentity(t *testing.T) { + data := validWorkload() + data.Ports["mesh"] = &pbcatalog.WorkloadPort{ + Port: 9090, + Protocol: pbcatalog.Protocol_PROTOCOL_MESH, + } + data.Identity = "" + + res := createWorkloadResource(t, data) + + err := ValidateWorkload(res) + require.Error(t, err) + expected := resource.ErrInvalidField{ + Name: "identity", + Wrapped: resource.ErrMissing, + } + var actual resource.ErrInvalidField + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) +} + +func TestValidateWorkload_InvalidIdentity(t *testing.T) { + data := validWorkload() + data.Identity = "/foiujd" + + res := createWorkloadResource(t, data) + + err := ValidateWorkload(res) + require.Error(t, err) + expected := resource.ErrInvalidField{ + Name: "identity", + Wrapped: errNotDNSLabel, + } + var actual resource.ErrInvalidField + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) +} + +func TestValidateWorkload_InvalidNodeName(t *testing.T) { + data := validWorkload() + data.NodeName = "/foiujd" + + res := createWorkloadResource(t, data) + + err := ValidateWorkload(res) + require.Error(t, err) + expected := resource.ErrInvalidField{ + Name: "node_name", + Wrapped: errNotDNSLabel, + } + var actual resource.ErrInvalidField + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) +} + +func TestValidateWorkload_NoPorts(t *testing.T) { + data := validWorkload() + data.Ports = nil + + res := createWorkloadResource(t, data) + + err := ValidateWorkload(res) + require.Error(t, err) + expected := resource.ErrInvalidField{ + Name: "ports", + Wrapped: resource.ErrEmpty, + } + var actual resource.ErrInvalidField + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) +} + +func TestValidateWorkload_TooMuchMesh(t *testing.T) { + data := validWorkload() + data.Ports["mesh1"] = &pbcatalog.WorkloadPort{ + Port: 9090, + Protocol: pbcatalog.Protocol_PROTOCOL_MESH, + } + data.Ports["mesh2"] = &pbcatalog.WorkloadPort{ + Port: 9091, + Protocol: pbcatalog.Protocol_PROTOCOL_MESH, + } + + res := createWorkloadResource(t, data) + + err := ValidateWorkload(res) + require.Error(t, err) + expected := resource.ErrInvalidField{ + Name: "ports", + Wrapped: errTooMuchMesh{ + Ports: []string{"mesh1", "mesh2"}, + }, + } + var actual resource.ErrInvalidField + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) +} + +func TestValidateWorkload_InvalidPortName(t *testing.T) { + data := validWorkload() + data.Ports[""] = &pbcatalog.WorkloadPort{ + Port: 42, + } + + res := createWorkloadResource(t, data) + + err := ValidateWorkload(res) + require.Error(t, err) + expected := resource.ErrInvalidMapKey{ + Map: "ports", + Key: "", + Wrapped: resource.ErrEmpty, + } + var actual resource.ErrInvalidMapKey + 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} + + res := createWorkloadResource(t, data) + + err := ValidateWorkload(res) + require.Error(t, err) + expected := resource.ErrInvalidField{ + Name: "port", + Wrapped: errInvalidPhysicalPort, + } + var actual resource.ErrInvalidField + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) +} + +func TestValidateWorkload_PortTooHigh(t *testing.T) { + data := validWorkload() + data.Ports["bar"] = &pbcatalog.WorkloadPort{Port: 65536} + + res := createWorkloadResource(t, data) + + err := ValidateWorkload(res) + require.Error(t, err) + expected := resource.ErrInvalidField{ + Name: "port", + Wrapped: errInvalidPhysicalPort, + } + var actual resource.ErrInvalidField + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) +} + +func TestValidateWorkload_Locality(t *testing.T) { + data := validWorkload() + data.Locality = &pbcatalog.Locality{ + Zone: "1a", + } + + res := createWorkloadResource(t, data) + + err := ValidateWorkload(res) + require.Error(t, err) + expected := resource.ErrInvalidField{ + Name: "locality", + Wrapped: errLocalityZoneNoRegion, + } + var actual resource.ErrInvalidField + require.ErrorAs(t, err, &actual) + require.Equal(t, expected, actual) +} diff --git a/internal/resource/errors.go b/internal/resource/errors.go new file mode 100644 index 0000000000..55fbb95d6a --- /dev/null +++ b/internal/resource/errors.go @@ -0,0 +1,114 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package resource + +import ( + "errors" + "fmt" + + "github.com/hashicorp/consul/proto-public/pbresource" + "google.golang.org/protobuf/reflect/protoreflect" +) + +var ( + ErrMissing = errors.New("missing required field") + ErrEmpty = errors.New("cannot be empty") + ErrReferenceTenancyNotEqual = errors.New("resource tenancy and reference tenancy differ") +) + +type ErrDataParse struct { + TypeName string + Wrapped error +} + +func NewErrDataParse(msg protoreflect.ProtoMessage, err error) ErrDataParse { + return ErrDataParse{ + TypeName: string(msg.ProtoReflect().Descriptor().FullName()), + Wrapped: err, + } +} + +func (err ErrDataParse) Error() string { + return fmt.Sprintf("error parsing resource data as type %q: %s", err.TypeName, err.Wrapped.Error()) +} + +func (err ErrDataParse) Unwrap() error { + return err.Wrapped +} + +type ErrInvalidField struct { + Name string + Wrapped error +} + +func (err ErrInvalidField) Error() string { + return fmt.Sprintf("invalid %q field: %v", err.Name, err.Wrapped) +} + +func (err ErrInvalidField) Unwrap() error { + return err.Wrapped +} + +type ErrInvalidListElement struct { + Name string + Index int + Wrapped error +} + +func (err ErrInvalidListElement) Error() string { + return fmt.Sprintf("invalid element at index %d of list %q: %v", err.Index, err.Name, err.Wrapped) +} + +func (err ErrInvalidListElement) Unwrap() error { + return err.Wrapped +} + +type ErrInvalidMapValue struct { + Map string + Key string + Wrapped error +} + +func (err ErrInvalidMapValue) Error() string { + return fmt.Sprintf("invalid value of key %q within %s: %v", err.Key, err.Map, err.Wrapped) +} + +func (err ErrInvalidMapValue) Unwrap() error { + return err.Wrapped +} + +type ErrInvalidMapKey struct { + Map string + Key string + Wrapped error +} + +func (err ErrInvalidMapKey) Error() string { + return fmt.Sprintf("map %s contains an invalid key - %q: %v", err.Map, err.Key, err.Wrapped) +} + +func (err ErrInvalidMapKey) Unwrap() error { + return err.Wrapped +} + +type ErrOwnerInvalid struct { + ResourceType *pbresource.Type + OwnerType *pbresource.Type +} + +func (err ErrOwnerInvalid) Error() string { + return fmt.Sprintf( + "resources of type %s cannot be owned by resources with type %s", + ToGVK(err.ResourceType), + ToGVK(err.OwnerType), + ) +} + +type ErrInvalidReferenceType struct { + AllowedType *pbresource.Type +} + +func (err ErrInvalidReferenceType) Error() string { + return fmt.Sprintf("reference must have type %s", ToGVK(err.AllowedType)) +} diff --git a/internal/resource/errors_test.go b/internal/resource/errors_test.go new file mode 100644 index 0000000000..5705457d59 --- /dev/null +++ b/internal/resource/errors_test.go @@ -0,0 +1,124 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package resource + +import ( + "errors" + "flag" + "fmt" + "os" + "path/filepath" + "testing" + + "github.com/hashicorp/consul/proto-public/pbresource" + "github.com/stretchr/testify/require" +) + +// update allows golden files to be updated based on the current output. +var update = flag.Bool("update", false, "update golden files") + +func goldenError(t *testing.T, name string, actual string) { + t.Helper() + + fpath := filepath.Join("testdata", name+".golden") + + if *update { + require.NoError(t, os.WriteFile(fpath, []byte(actual), 0644)) + } else { + expected, err := os.ReadFile(fpath) + require.NoError(t, err) + require.Equal(t, string(expected), actual) + } +} + +func TestErrorStrings(t *testing.T) { + type testCase struct { + err error + expected string + } + + fakeWrappedErr := fmt.Errorf("fake test error") + + cond := &pbresource.Condition{} + + cases := map[string]error{ + "ErrDataParse": NewErrDataParse(cond, fakeWrappedErr), + "ErrInvalidField": ErrInvalidField{ + Name: "host", + Wrapped: fakeWrappedErr, + }, + "ErrInvalidListElement": ErrInvalidListElement{ + Name: "addresses", + Index: 42, + Wrapped: fakeWrappedErr, + }, + "ErrInvalidMapKey": ErrInvalidMapKey{ + Map: "ports", + Key: "http", + Wrapped: fakeWrappedErr, + }, + "ErrInvalidMapValue": ErrInvalidMapValue{ + Map: "ports", + Key: "http", + Wrapped: fakeWrappedErr, + }, + "ErrOwnerInvalid": ErrOwnerInvalid{ + ResourceType: &pbresource.Type{Group: "foo", GroupVersion: "v1", Kind: "bar"}, + OwnerType: &pbresource.Type{Group: "other", GroupVersion: "v2", Kind: "something"}, + }, + "ErrInvalidReferenceType": ErrInvalidReferenceType{ + AllowedType: &pbresource.Type{Group: "foo", GroupVersion: "v1", Kind: "bar"}, + }, + "ErrMissing": ErrMissing, + "ErrEmpty": ErrEmpty, + "ErrReferenceTenancyNotEqual": ErrReferenceTenancyNotEqual, + } + + for name, err := range cases { + t.Run(name, func(t *testing.T) { + goldenError(t, name, err.Error()) + }) + } +} + +func TestErrorUnwrap(t *testing.T) { + type testCase struct { + err error + expected string + } + + fakeWrappedErr := fmt.Errorf("fake test error") + + cases := map[string]error{ + "ErrDataParse": ErrDataParse{ + TypeName: "hashicorp.consul.catalog.v1alpha1.Service", + Wrapped: fakeWrappedErr, + }, + "ErrInvalidField": ErrInvalidField{ + Name: "host", + Wrapped: fakeWrappedErr, + }, + "ErrInvalidListElement": ErrInvalidListElement{ + Name: "addresses", + Index: 42, + Wrapped: fakeWrappedErr, + }, + "ErrInvalidMapKey": ErrInvalidMapKey{ + Map: "ports", + Key: "http", + Wrapped: fakeWrappedErr, + }, + "ErrInvalidMapValue": ErrInvalidMapValue{ + Map: "ports", + Key: "http", + Wrapped: fakeWrappedErr, + }, + } + + for name, err := range cases { + t.Run(name, func(t *testing.T) { + require.Equal(t, fakeWrappedErr, errors.Unwrap(err)) + }) + } +} diff --git a/internal/resource/testdata/ErrDataParse.golden b/internal/resource/testdata/ErrDataParse.golden new file mode 100644 index 0000000000..b980ddb94b --- /dev/null +++ b/internal/resource/testdata/ErrDataParse.golden @@ -0,0 +1 @@ +error parsing resource data as type "hashicorp.consul.resource.Condition": fake test error \ No newline at end of file diff --git a/internal/resource/testdata/ErrEmpty.golden b/internal/resource/testdata/ErrEmpty.golden new file mode 100644 index 0000000000..96d45cf55b --- /dev/null +++ b/internal/resource/testdata/ErrEmpty.golden @@ -0,0 +1 @@ +cannot be empty \ No newline at end of file diff --git a/internal/resource/testdata/ErrInvalidField.golden b/internal/resource/testdata/ErrInvalidField.golden new file mode 100644 index 0000000000..9b213c55b0 --- /dev/null +++ b/internal/resource/testdata/ErrInvalidField.golden @@ -0,0 +1 @@ +invalid "host" field: fake test error \ No newline at end of file diff --git a/internal/resource/testdata/ErrInvalidListElement.golden b/internal/resource/testdata/ErrInvalidListElement.golden new file mode 100644 index 0000000000..fa6530e677 --- /dev/null +++ b/internal/resource/testdata/ErrInvalidListElement.golden @@ -0,0 +1 @@ +invalid element at index 42 of list "addresses": fake test error \ No newline at end of file diff --git a/internal/resource/testdata/ErrInvalidMapKey.golden b/internal/resource/testdata/ErrInvalidMapKey.golden new file mode 100644 index 0000000000..25b72c9d31 --- /dev/null +++ b/internal/resource/testdata/ErrInvalidMapKey.golden @@ -0,0 +1 @@ +map ports contains an invalid key - "http": fake test error \ No newline at end of file diff --git a/internal/resource/testdata/ErrInvalidMapValue.golden b/internal/resource/testdata/ErrInvalidMapValue.golden new file mode 100644 index 0000000000..359d4e69d8 --- /dev/null +++ b/internal/resource/testdata/ErrInvalidMapValue.golden @@ -0,0 +1 @@ +invalid value of key "http" within ports: fake test error \ No newline at end of file diff --git a/internal/resource/testdata/ErrInvalidReferenceType.golden b/internal/resource/testdata/ErrInvalidReferenceType.golden new file mode 100644 index 0000000000..c8d57e1efb --- /dev/null +++ b/internal/resource/testdata/ErrInvalidReferenceType.golden @@ -0,0 +1 @@ +reference must have type foo.v1.bar \ No newline at end of file diff --git a/internal/resource/testdata/ErrMissing.golden b/internal/resource/testdata/ErrMissing.golden new file mode 100644 index 0000000000..dec5d632b4 --- /dev/null +++ b/internal/resource/testdata/ErrMissing.golden @@ -0,0 +1 @@ +missing required field \ No newline at end of file diff --git a/internal/resource/testdata/ErrOwnerInvalid.golden b/internal/resource/testdata/ErrOwnerInvalid.golden new file mode 100644 index 0000000000..c0921c4323 --- /dev/null +++ b/internal/resource/testdata/ErrOwnerInvalid.golden @@ -0,0 +1 @@ +resources of type foo.v1.bar cannot be owned by resources with type other.v2.something \ No newline at end of file diff --git a/internal/resource/testdata/ErrReferenceTenancyNotEqual.golden b/internal/resource/testdata/ErrReferenceTenancyNotEqual.golden new file mode 100644 index 0000000000..be53478d69 --- /dev/null +++ b/internal/resource/testdata/ErrReferenceTenancyNotEqual.golden @@ -0,0 +1 @@ +resource tenancy and reference tenancy differ \ No newline at end of file