From f0e48977361a9e4f13b10b3ec0174322c4eae0d1 Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" <4903+rboyer@users.noreply.github.com> Date: Fri, 13 Oct 2023 11:31:56 -0500 Subject: [PATCH] mesh: ensure that xRoutes have ParentRefs that have matching Tenancy to the enclosing resource (#19176) We don't want an xRoute controlling traffic for a Service in another tenancy. --- .../mesh/internal/types/destinations_test.go | 30 +- internal/mesh/internal/types/grpc_route.go | 2 +- .../mesh/internal/types/grpc_route_test.go | 10 +- internal/mesh/internal/types/http_route.go | 2 +- .../mesh/internal/types/http_route_test.go | 327 +-------------- internal/mesh/internal/types/tcp_route.go | 2 +- .../mesh/internal/types/tcp_route_test.go | 11 +- internal/mesh/internal/types/xroute.go | 8 +- internal/mesh/internal/types/xroute_test.go | 378 ++++++++++++++++++ internal/resource/resourcetest/tenancy.go | 37 ++ internal/resource/tenancy.go | 21 + 11 files changed, 486 insertions(+), 342 deletions(-) create mode 100644 internal/resource/resourcetest/tenancy.go diff --git a/internal/mesh/internal/types/destinations_test.go b/internal/mesh/internal/types/destinations_test.go index f5745e4cb3..36ee71b086 100644 --- a/internal/mesh/internal/types/destinations_test.go +++ b/internal/mesh/internal/types/destinations_test.go @@ -61,19 +61,19 @@ func TestMutateUpstreams(t *testing.T) { }, }, "dest ref tenancy defaulting": { - tenancy: newTestTenancy("foo.bar"), + tenancy: resourcetest.Tenancy("foo.bar"), data: &pbmesh.Destinations{ Destinations: []*pbmesh.Destination{ - {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, newTestTenancy(""), "api")}, - {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, newTestTenancy(".zim"), "api")}, - {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, newTestTenancy("gir.zim"), "api")}, + {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, "", "api")}, + {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, ".zim", "api")}, + {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, "gir.zim", "api")}, }, }, expect: &pbmesh.Destinations{ Destinations: []*pbmesh.Destination{ - {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, newTestTenancy("foo.bar"), "api")}, - {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, newTestTenancy("foo.zim"), "api")}, - {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, newTestTenancy("gir.zim"), "api")}, + {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, "foo.bar", "api")}, + {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, "foo.zim", "api")}, + {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, "gir.zim", "api")}, }, }, }, @@ -138,7 +138,7 @@ func TestValidateUpstreams(t *testing.T) { skipMutate: true, data: &pbmesh.Destinations{ Destinations: []*pbmesh.Destination{ - {DestinationRef: newRefWithTenancy(pbcatalog.WorkloadType, nil, "api")}, + {DestinationRef: newRefWithTenancy(pbcatalog.WorkloadType, "default.default", "api")}, }, }, expectErr: `invalid element at index 0 of list "upstreams": invalid "destination_ref" field: invalid "type" field: reference must have type catalog.v2beta1.Service`, @@ -156,7 +156,7 @@ func TestValidateUpstreams(t *testing.T) { skipMutate: true, data: &pbmesh.Destinations{ Destinations: []*pbmesh.Destination{ - {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, newTestTenancy(".bar"), "api")}, + {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, ".bar", "api")}, }, }, expectErr: `invalid element at index 0 of list "upstreams": invalid "destination_ref" field: invalid "tenancy" field: invalid "partition" field: cannot be empty`, @@ -165,7 +165,7 @@ func TestValidateUpstreams(t *testing.T) { skipMutate: true, data: &pbmesh.Destinations{ Destinations: []*pbmesh.Destination{ - {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, newTestTenancy("foo"), "api")}, + {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, "foo", "api")}, }, }, expectErr: `invalid element at index 0 of list "upstreams": invalid "destination_ref" field: invalid "tenancy" field: invalid "namespace" field: cannot be empty`, @@ -174,7 +174,9 @@ func TestValidateUpstreams(t *testing.T) { skipMutate: true, data: &pbmesh.Destinations{ Destinations: []*pbmesh.Destination{ - {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, &pbresource.Tenancy{Partition: "foo", Namespace: "bar"}, "api")}, + {DestinationRef: resourcetest.Resource(pbcatalog.ServiceType, "api"). + WithTenancy(&pbresource.Tenancy{Partition: "foo", Namespace: "bar"}). + Reference("")}, }, }, expectErr: `invalid element at index 0 of list "upstreams": invalid "destination_ref" field: invalid "tenancy" field: invalid "peer_name" field: must be set to "local"`, @@ -182,9 +184,9 @@ func TestValidateUpstreams(t *testing.T) { "normal": { data: &pbmesh.Destinations{ Destinations: []*pbmesh.Destination{ - {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, newTestTenancy("foo.bar"), "api")}, - {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, newTestTenancy("foo.zim"), "api")}, - {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, newTestTenancy("gir.zim"), "api")}, + {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, "foo.bar", "api")}, + {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, "foo.zim", "api")}, + {DestinationRef: newRefWithTenancy(pbcatalog.ServiceType, "gir.zim", "api")}, }, }, }, diff --git a/internal/mesh/internal/types/grpc_route.go b/internal/mesh/internal/types/grpc_route.go index ab556c6947..630e416e61 100644 --- a/internal/mesh/internal/types/grpc_route.go +++ b/internal/mesh/internal/types/grpc_route.go @@ -64,7 +64,7 @@ func ValidateGRPCRoute(res *pbresource.Resource) error { } var merr error - if err := validateParentRefs(route.ParentRefs); err != nil { + if err := validateParentRefs(res.Id, route.ParentRefs); err != nil { merr = multierror.Append(merr, err) } diff --git a/internal/mesh/internal/types/grpc_route_test.go b/internal/mesh/internal/types/grpc_route_test.go index 0ca13e8563..26340bff77 100644 --- a/internal/mesh/internal/types/grpc_route_test.go +++ b/internal/mesh/internal/types/grpc_route_test.go @@ -81,6 +81,7 @@ func TestMutateGRPCRoute(t *testing.T) { WithTenancy(tc.routeTenancy). WithData(t, tc.route). Build() + resource.DefaultIDTenancy(res.Id, nil, resource.DefaultNamespacedTenancy()) err := MutateGRPCRoute(res) require.NoError(t, err) @@ -103,14 +104,17 @@ func TestMutateGRPCRoute(t *testing.T) { func TestValidateGRPCRoute(t *testing.T) { type testcase struct { - route *pbmesh.GRPCRoute - expectErr string + routeTenancy *pbresource.Tenancy + route *pbmesh.GRPCRoute + expectErr string } run := func(t *testing.T, tc testcase) { res := resourcetest.Resource(pbmesh.GRPCRouteType, "api"). + WithTenancy(tc.routeTenancy). WithData(t, tc.route). Build() + resource.DefaultIDTenancy(res.Id, nil, resource.DefaultNamespacedTenancy()) // Ensure things are properly mutated and updated in the inputs. err := MutateGRPCRoute(res) @@ -582,6 +586,7 @@ func TestValidateGRPCRoute(t *testing.T) { // Add common parent refs test cases. for name, parentTC := range getXRouteParentRefTestCases() { cases["parent-ref: "+name] = testcase{ + routeTenancy: parentTC.routeTenancy, route: &pbmesh.GRPCRoute{ ParentRefs: parentTC.refs, }, @@ -597,6 +602,7 @@ func TestValidateGRPCRoute(t *testing.T) { }) } cases["backend-ref: "+name] = testcase{ + routeTenancy: backendTC.routeTenancy, route: &pbmesh.GRPCRoute{ ParentRefs: []*pbmesh.ParentReference{ newParentRef(pbcatalog.ServiceType, "web", ""), diff --git a/internal/mesh/internal/types/http_route.go b/internal/mesh/internal/types/http_route.go index 847e23aed9..0ac2dcbf5c 100644 --- a/internal/mesh/internal/types/http_route.go +++ b/internal/mesh/internal/types/http_route.go @@ -75,7 +75,7 @@ func ValidateHTTPRoute(res *pbresource.Resource) error { } var merr error - if err := validateParentRefs(route.ParentRefs); err != nil { + if err := validateParentRefs(res.Id, route.ParentRefs); err != nil { merr = multierror.Append(merr, err) } diff --git a/internal/mesh/internal/types/http_route_test.go b/internal/mesh/internal/types/http_route_test.go index 5f5dad5460..81c8b5ab00 100644 --- a/internal/mesh/internal/types/http_route_test.go +++ b/internal/mesh/internal/types/http_route_test.go @@ -4,14 +4,10 @@ package types import ( - "strings" "testing" - "time" "github.com/stretchr/testify/require" "google.golang.org/protobuf/proto" - "google.golang.org/protobuf/types/known/durationpb" - "google.golang.org/protobuf/types/known/wrapperspb" "github.com/hashicorp/consul/internal/resource" "github.com/hashicorp/consul/internal/resource/resourcetest" @@ -35,6 +31,7 @@ func TestMutateHTTPRoute(t *testing.T) { WithTenancy(tc.routeTenancy). WithData(t, tc.route). Build() + resource.DefaultIDTenancy(res.Id, nil, resource.DefaultNamespacedTenancy()) err := MutateHTTPRoute(res) @@ -200,14 +197,17 @@ func TestMutateHTTPRoute(t *testing.T) { func TestValidateHTTPRoute(t *testing.T) { type testcase struct { - route *pbmesh.HTTPRoute - expectErr string + routeTenancy *pbresource.Tenancy + route *pbmesh.HTTPRoute + expectErr string } run := func(t *testing.T, tc testcase) { res := resourcetest.Resource(pbmesh.HTTPRouteType, "api"). + WithTenancy(tc.routeTenancy). WithData(t, tc.route). Build() + resource.DefaultIDTenancy(res.Id, nil, resource.DefaultNamespacedTenancy()) // Ensure things are properly mutated and updated in the inputs. err := MutateHTTPRoute(res) @@ -844,6 +844,7 @@ func TestValidateHTTPRoute(t *testing.T) { // Add common parent refs test cases. for name, parentTC := range getXRouteParentRefTestCases() { cases["parent-ref: "+name] = testcase{ + routeTenancy: parentTC.routeTenancy, route: &pbmesh.HTTPRoute{ ParentRefs: parentTC.refs, }, @@ -859,6 +860,7 @@ func TestValidateHTTPRoute(t *testing.T) { }) } cases["backend-ref: "+name] = testcase{ + routeTenancy: backendTC.routeTenancy, route: &pbmesh.HTTPRoute{ ParentRefs: []*pbmesh.ParentReference{ newParentRef(pbcatalog.ServiceType, "web", ""), @@ -878,319 +880,6 @@ func TestValidateHTTPRoute(t *testing.T) { } } -type xRouteParentRefMutateTestcase struct { - routeTenancy *pbresource.Tenancy - refs []*pbmesh.ParentReference - expect []*pbmesh.ParentReference -} - -func getXRouteParentRefMutateTestCases() map[string]xRouteParentRefMutateTestcase { - newRef := func(typ *pbresource.Type, tenancyStr, name string) *pbresource.Reference { - return resourcetest.Resource(typ, name). - WithTenancy(newTestTenancy(tenancyStr)). - Reference("") - } - - newParentRef := func(typ *pbresource.Type, tenancyStr, name, port string) *pbmesh.ParentReference { - return &pbmesh.ParentReference{ - Ref: newRef(typ, tenancyStr, name), - Port: port, - } - } - - return map[string]xRouteParentRefMutateTestcase{ - "parent ref tenancies defaulted": { - routeTenancy: newTestTenancy("foo.bar"), - refs: []*pbmesh.ParentReference{ - newParentRef(pbcatalog.ServiceType, "", "api", ""), - newParentRef(pbcatalog.ServiceType, ".zim", "api", ""), - newParentRef(pbcatalog.ServiceType, "gir.zim", "api", ""), - }, - expect: []*pbmesh.ParentReference{ - newParentRef(pbcatalog.ServiceType, "foo.bar", "api", ""), - newParentRef(pbcatalog.ServiceType, "foo.zim", "api", ""), - newParentRef(pbcatalog.ServiceType, "gir.zim", "api", ""), - }, - }, - } -} - -type xRouteBackendRefMutateTestcase struct { - routeTenancy *pbresource.Tenancy - refs []*pbmesh.BackendReference - expect []*pbmesh.BackendReference -} - -func getXRouteBackendRefMutateTestCases() map[string]xRouteBackendRefMutateTestcase { - newRef := func(typ *pbresource.Type, tenancyStr, name string) *pbresource.Reference { - return resourcetest.Resource(typ, name). - WithTenancy(newTestTenancy(tenancyStr)). - Reference("") - } - - newBackendRef := func(typ *pbresource.Type, tenancyStr, name, port string) *pbmesh.BackendReference { - return &pbmesh.BackendReference{ - Ref: newRef(typ, tenancyStr, name), - Port: port, - } - } - - return map[string]xRouteBackendRefMutateTestcase{ - "backend ref tenancies defaulted": { - routeTenancy: newTestTenancy("foo.bar"), - refs: []*pbmesh.BackendReference{ - newBackendRef(pbcatalog.ServiceType, "", "api", ""), - newBackendRef(pbcatalog.ServiceType, ".zim", "api", ""), - newBackendRef(pbcatalog.ServiceType, "gir.zim", "api", ""), - }, - expect: []*pbmesh.BackendReference{ - newBackendRef(pbcatalog.ServiceType, "foo.bar", "api", ""), - newBackendRef(pbcatalog.ServiceType, "foo.zim", "api", ""), - newBackendRef(pbcatalog.ServiceType, "gir.zim", "api", ""), - }, - }, - } -} - -type xRouteParentRefTestcase struct { - refs []*pbmesh.ParentReference - expectErr string -} - -func getXRouteParentRefTestCases() map[string]xRouteParentRefTestcase { - return map[string]xRouteParentRefTestcase{ - "no parent refs": { - expectErr: `invalid "parent_refs" field: cannot be empty`, - }, - "parent ref with nil ref": { - refs: []*pbmesh.ParentReference{ - newParentRef(pbcatalog.ServiceType, "api", ""), - { - Ref: nil, - Port: "http", - }, - }, - expectErr: `invalid element at index 1 of list "parent_refs": invalid "ref" field: missing required field`, - }, - "parent ref with bad type ref": { - refs: []*pbmesh.ParentReference{ - newParentRef(pbcatalog.ServiceType, "api", ""), - newParentRef(pbcatalog.WorkloadType, "api", ""), - }, - expectErr: `invalid element at index 1 of list "parent_refs": invalid "ref" field: invalid "type" field: reference must have type catalog.v2beta1.Service`, - }, - "parent ref with section": { - refs: []*pbmesh.ParentReference{ - newParentRef(pbcatalog.ServiceType, "api", ""), - { - Ref: resourcetest.Resource(pbcatalog.ServiceType, "web").Reference("section2"), - Port: "http", - }, - }, - expectErr: `invalid element at index 1 of list "parent_refs": invalid "ref" field: invalid "section" field: section cannot be set here`, - }, - "duplicate exact parents": { - refs: []*pbmesh.ParentReference{ - newParentRef(pbcatalog.ServiceType, "api", "http"), - newParentRef(pbcatalog.ServiceType, "api", "http"), - }, - expectErr: `invalid element at index 1 of list "parent_refs": invalid "port" field: parent ref "catalog.v2beta1.Service/default.local.default/api" for port "http" exists twice`, - }, - "duplicate wild parents": { - refs: []*pbmesh.ParentReference{ - newParentRef(pbcatalog.ServiceType, "api", ""), - newParentRef(pbcatalog.ServiceType, "api", ""), - }, - expectErr: `invalid element at index 1 of list "parent_refs": invalid "port" field: parent ref "catalog.v2beta1.Service/default.local.default/api" for wildcard port exists twice`, - }, - "duplicate parents via exact+wild overlap": { - refs: []*pbmesh.ParentReference{ - newParentRef(pbcatalog.ServiceType, "api", "http"), - newParentRef(pbcatalog.ServiceType, "api", ""), - }, - expectErr: `invalid element at index 1 of list "parent_refs": invalid "port" field: parent ref "catalog.v2beta1.Service/default.local.default/api" for ports [http] covered by wildcard port already`, - }, - "duplicate parents via exact+wild overlap (reversed)": { - refs: []*pbmesh.ParentReference{ - newParentRef(pbcatalog.ServiceType, "api", ""), - newParentRef(pbcatalog.ServiceType, "api", "http"), - }, - expectErr: `invalid element at index 1 of list "parent_refs": invalid "port" field: parent ref "catalog.v2beta1.Service/default.local.default/api" for port "http" covered by wildcard port already`, - }, - "good single parent ref": { - refs: []*pbmesh.ParentReference{ - newParentRef(pbcatalog.ServiceType, "api", "http"), - }, - }, - "good muliple parent refs": { - refs: []*pbmesh.ParentReference{ - newParentRef(pbcatalog.ServiceType, "api", "http"), - newParentRef(pbcatalog.ServiceType, "web", ""), - }, - }, - } -} - -type xRouteBackendRefTestcase struct { - refs []*pbmesh.BackendReference - expectErr string -} - -func getXRouteBackendRefTestCases() map[string]xRouteBackendRefTestcase { - return map[string]xRouteBackendRefTestcase{ - "no backend refs": { - expectErr: `invalid "backend_refs" field: cannot be empty`, - }, - "backend ref with nil ref": { - refs: []*pbmesh.BackendReference{ - newBackendRef(pbcatalog.ServiceType, "api", ""), - { - Ref: nil, - Port: "http", - }, - }, - expectErr: `invalid element at index 0 of list "rules": invalid element at index 1 of list "backend_refs": invalid "backend_ref" field: invalid "ref" field: missing required field`, - }, - "backend ref with bad type ref": { - refs: []*pbmesh.BackendReference{ - newBackendRef(pbcatalog.ServiceType, "api", ""), - newBackendRef(pbcatalog.WorkloadType, "api", ""), - }, - expectErr: `invalid element at index 0 of list "rules": invalid element at index 1 of list "backend_refs": invalid "backend_ref" field: invalid "ref" field: invalid "type" field: reference must have type catalog.v2beta1.Service`, - }, - "backend ref with section": { - refs: []*pbmesh.BackendReference{ - newBackendRef(pbcatalog.ServiceType, "api", ""), - { - Ref: resourcetest.Resource(pbcatalog.ServiceType, "web").Reference("section2"), - Port: "http", - }, - }, - expectErr: `invalid element at index 0 of list "rules": invalid element at index 1 of list "backend_refs": invalid "backend_ref" field: invalid "ref" field: invalid "section" field: section cannot be set here`, - }, - "backend ref with datacenter": { - refs: []*pbmesh.BackendReference{ - newBackendRef(pbcatalog.ServiceType, "api", ""), - { - Ref: newRef(pbcatalog.ServiceType, "db"), - Port: "http", - Datacenter: "dc2", - }, - }, - expectErr: `invalid element at index 0 of list "rules": invalid element at index 1 of list "backend_refs": invalid "backend_ref" field: invalid "datacenter" field: datacenter is not yet supported on backend refs`, - }, - "good backend ref": { - refs: []*pbmesh.BackendReference{ - newBackendRef(pbcatalog.ServiceType, "api", ""), - { - Ref: newRef(pbcatalog.ServiceType, "db"), - Port: "http", - }, - }, - }, - } -} - -type xRouteTimeoutsTestcase struct { - timeouts *pbmesh.HTTPRouteTimeouts - expectErr string -} - -func getXRouteTimeoutsTestCases() map[string]xRouteTimeoutsTestcase { - return map[string]xRouteTimeoutsTestcase{ - "bad request": { - timeouts: &pbmesh.HTTPRouteTimeouts{ - Request: durationpb.New(-1 * time.Second), - }, - expectErr: `invalid element at index 0 of list "rules": invalid "timeouts" field: invalid "request" field: timeout cannot be negative: -1s`, - }, - "bad idle": { - timeouts: &pbmesh.HTTPRouteTimeouts{ - Idle: durationpb.New(-1 * time.Second), - }, - expectErr: `invalid element at index 0 of list "rules": invalid "timeouts" field: invalid "idle" field: timeout cannot be negative: -1s`, - }, - "good all": { - timeouts: &pbmesh.HTTPRouteTimeouts{ - Request: durationpb.New(1 * time.Second), - Idle: durationpb.New(3 * time.Second), - }, - }, - } -} - -type xRouteRetriesTestcase struct { - retries *pbmesh.HTTPRouteRetries - expectErr string -} - -func getXRouteRetriesTestCases() map[string]xRouteRetriesTestcase { - return map[string]xRouteRetriesTestcase{ - "bad conditions": { - retries: &pbmesh.HTTPRouteRetries{ - OnConditions: []string{"garbage"}, - }, - expectErr: `invalid element at index 0 of list "rules": invalid "retries" field: invalid element at index 0 of list "on_conditions": not a valid retry condition: "garbage"`, - }, - "good all": { - retries: &pbmesh.HTTPRouteRetries{ - Number: wrapperspb.UInt32(5), - OnConditions: []string{"internal"}, - }, - }, - } -} - -func newRef(typ *pbresource.Type, name string) *pbresource.Reference { - return newRefWithTenancy(typ, nil, name) -} - -func newRefWithTenancy(typ *pbresource.Type, tenancy *pbresource.Tenancy, name string) *pbresource.Reference { - if tenancy == nil { - tenancy = resource.DefaultNamespacedTenancy() - } - return resourcetest.Resource(typ, name). - WithTenancy(tenancy). - Reference("") -} - -func newBackendRef(typ *pbresource.Type, name, port string) *pbmesh.BackendReference { - return &pbmesh.BackendReference{ - Ref: newRef(typ, name), - Port: port, - } -} - -func newParentRef(typ *pbresource.Type, name, port string) *pbmesh.ParentReference { - return newParentRefWithTenancy(typ, nil, name, port) -} - -func newParentRefWithTenancy(typ *pbresource.Type, tenancy *pbresource.Tenancy, name, port string) *pbmesh.ParentReference { - return &pbmesh.ParentReference{ - Ref: newRefWithTenancy(typ, tenancy, name), - Port: port, - } -} - -func newTestTenancy(s string) *pbresource.Tenancy { - parts := strings.Split(s, ".") - switch len(parts) { - case 0: - return resource.DefaultClusteredTenancy() - case 1: - v := resource.DefaultPartitionedTenancy() - v.Partition = parts[0] - return v - case 2: - v := resource.DefaultNamespacedTenancy() - v.Partition = parts[0] - v.Namespace = parts[1] - return v - default: - return &pbresource.Tenancy{Partition: "BAD", Namespace: "BAD", PeerName: "BAD"} - } -} - func TestHTTPRouteACLs(t *testing.T) { testXRouteACLs[*pbmesh.HTTPRoute](t, func(t *testing.T, parentRefs, backendRefs []*pbresource.Reference) *pbresource.Resource { data := &pbmesh.HTTPRoute{ diff --git a/internal/mesh/internal/types/tcp_route.go b/internal/mesh/internal/types/tcp_route.go index e8a7b361ee..c7470b14d5 100644 --- a/internal/mesh/internal/types/tcp_route.go +++ b/internal/mesh/internal/types/tcp_route.go @@ -64,7 +64,7 @@ func ValidateTCPRoute(res *pbresource.Resource) error { var merr error - if err := validateParentRefs(route.ParentRefs); err != nil { + if err := validateParentRefs(res.Id, route.ParentRefs); err != nil { merr = multierror.Append(merr, err) } diff --git a/internal/mesh/internal/types/tcp_route_test.go b/internal/mesh/internal/types/tcp_route_test.go index 10f30deb5c..7dd9a6a80d 100644 --- a/internal/mesh/internal/types/tcp_route_test.go +++ b/internal/mesh/internal/types/tcp_route_test.go @@ -81,6 +81,7 @@ func TestMutateTCPRoute(t *testing.T) { WithTenancy(tc.routeTenancy). WithData(t, tc.route). Build() + resource.DefaultIDTenancy(res.Id, nil, resource.DefaultNamespacedTenancy()) err := MutateTCPRoute(res) require.NoError(t, err) @@ -103,15 +104,17 @@ func TestMutateTCPRoute(t *testing.T) { func TestValidateTCPRoute(t *testing.T) { type testcase struct { - route *pbmesh.TCPRoute - expectErr string + routeTenancy *pbresource.Tenancy + route *pbmesh.TCPRoute + expectErr string } run := func(t *testing.T, tc testcase) { res := resourcetest.Resource(pbmesh.TCPRouteType, "api"). - WithTenancy(resource.DefaultNamespacedTenancy()). + WithTenancy(tc.routeTenancy). WithData(t, tc.route). Build() + resource.DefaultIDTenancy(res.Id, nil, resource.DefaultNamespacedTenancy()) // Ensure things are properly mutated and updated in the inputs. err := MutateTCPRoute(res) @@ -167,6 +170,7 @@ func TestValidateTCPRoute(t *testing.T) { // Add common parent refs test cases. for name, parentTC := range getXRouteParentRefTestCases() { cases["parent-ref: "+name] = testcase{ + routeTenancy: parentTC.routeTenancy, route: &pbmesh.TCPRoute{ ParentRefs: parentTC.refs, }, @@ -182,6 +186,7 @@ func TestValidateTCPRoute(t *testing.T) { }) } cases["backend-ref: "+name] = testcase{ + routeTenancy: backendTC.routeTenancy, route: &pbmesh.TCPRoute{ ParentRefs: []*pbmesh.ParentReference{ newParentRef(pbcatalog.ServiceType, "web", ""), diff --git a/internal/mesh/internal/types/xroute.go b/internal/mesh/internal/types/xroute.go index 449e58b393..1c60bdcb1c 100644 --- a/internal/mesh/internal/types/xroute.go +++ b/internal/mesh/internal/types/xroute.go @@ -60,7 +60,7 @@ func mutateXRouteRef(xrouteTenancy *pbresource.Tenancy, ref *pbresource.Referenc return !proto.Equal(orig, ref) } -func validateParentRefs(parentRefs []*pbmesh.ParentReference) error { +func validateParentRefs(id *pbresource.ID, parentRefs []*pbmesh.ParentReference) error { var merr error if len(parentRefs) == 0 { merr = multierror.Append(merr, resource.ErrInvalidField{ @@ -92,6 +92,12 @@ func validateParentRefs(parentRefs []*pbmesh.ParentReference) error { if err := catalog.ValidateLocalServiceRefNoSection(parent.Ref, wrapRefErr); err != nil { merr = multierror.Append(merr, err) } else { + if !resource.EqualTenancy(id.Tenancy, parent.Ref.Tenancy) { + merr = multierror.Append(merr, wrapRefErr(resource.ErrInvalidField{ + Name: "tenancy", + Wrapped: resource.ErrReferenceTenancyNotEqual, + })) + } prk := portedRefKey{ Key: resource.NewReferenceKey(parent.Ref), Port: parent.Port, diff --git a/internal/mesh/internal/types/xroute_test.go b/internal/mesh/internal/types/xroute_test.go index 67674aac14..09806bea8c 100644 --- a/internal/mesh/internal/types/xroute_test.go +++ b/internal/mesh/internal/types/xroute_test.go @@ -6,17 +6,365 @@ package types import ( "fmt" "testing" + "time" "github.com/stretchr/testify/require" + "google.golang.org/protobuf/types/known/durationpb" + "google.golang.org/protobuf/types/known/wrapperspb" "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/internal/resource" "github.com/hashicorp/consul/internal/resource/resourcetest" pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v2beta1" + pbmesh "github.com/hashicorp/consul/proto-public/pbmesh/v2beta1" "github.com/hashicorp/consul/proto-public/pbresource" ) +type xRouteParentRefMutateTestcase struct { + routeTenancy *pbresource.Tenancy + refs []*pbmesh.ParentReference + expect []*pbmesh.ParentReference +} + +func getXRouteParentRefMutateTestCases() map[string]xRouteParentRefMutateTestcase { + newRef := func(typ *pbresource.Type, tenancyStr, name string) *pbresource.Reference { + return resourcetest.Resource(typ, name). + WithTenancy(resourcetest.Tenancy(tenancyStr)). + Reference("") + } + + newParentRef := func(typ *pbresource.Type, tenancyStr, name, port string) *pbmesh.ParentReference { + return &pbmesh.ParentReference{ + Ref: newRef(typ, tenancyStr, name), + Port: port, + } + } + + return map[string]xRouteParentRefMutateTestcase{ + "parent ref tenancies defaulted": { + routeTenancy: resourcetest.Tenancy("foo.bar"), + refs: []*pbmesh.ParentReference{ + newParentRef(pbcatalog.ServiceType, "", "api", ""), + newParentRef(pbcatalog.ServiceType, ".zim", "api", ""), + newParentRef(pbcatalog.ServiceType, "gir.zim", "api", ""), + }, + expect: []*pbmesh.ParentReference{ + newParentRef(pbcatalog.ServiceType, "foo.bar", "api", ""), + newParentRef(pbcatalog.ServiceType, "foo.zim", "api", ""), + newParentRef(pbcatalog.ServiceType, "gir.zim", "api", ""), + }, + }, + } +} + +type xRouteBackendRefMutateTestcase struct { + routeTenancy *pbresource.Tenancy + refs []*pbmesh.BackendReference + expect []*pbmesh.BackendReference +} + +func getXRouteBackendRefMutateTestCases() map[string]xRouteBackendRefMutateTestcase { + newRef := func(typ *pbresource.Type, tenancyStr, name string) *pbresource.Reference { + return resourcetest.Resource(typ, name). + WithTenancy(resourcetest.Tenancy(tenancyStr)). + Reference("") + } + + newBackendRef := func(typ *pbresource.Type, tenancyStr, name, port string) *pbmesh.BackendReference { + return &pbmesh.BackendReference{ + Ref: newRef(typ, tenancyStr, name), + Port: port, + } + } + + return map[string]xRouteBackendRefMutateTestcase{ + "backend ref tenancies defaulted": { + routeTenancy: resourcetest.Tenancy("foo.bar"), + refs: []*pbmesh.BackendReference{ + newBackendRef(pbcatalog.ServiceType, "", "api", ""), + newBackendRef(pbcatalog.ServiceType, ".zim", "api", ""), + newBackendRef(pbcatalog.ServiceType, "gir.zim", "api", ""), + }, + expect: []*pbmesh.BackendReference{ + newBackendRef(pbcatalog.ServiceType, "foo.bar", "api", ""), + newBackendRef(pbcatalog.ServiceType, "foo.zim", "api", ""), + newBackendRef(pbcatalog.ServiceType, "gir.zim", "api", ""), + }, + }, + } +} + +type xRouteParentRefTestcase struct { + routeTenancy *pbresource.Tenancy + refs []*pbmesh.ParentReference + expectErr string +} + +func getXRouteParentRefTestCases() map[string]xRouteParentRefTestcase { + newRef := func(typ *pbresource.Type, tenancyStr, name string) *pbresource.Reference { + return resourcetest.Resource(typ, name). + WithTenancy(resourcetest.Tenancy(tenancyStr)). + Reference("") + } + + newParentRef := func(typ *pbresource.Type, tenancyStr, name, port string) *pbmesh.ParentReference { + return &pbmesh.ParentReference{ + Ref: newRef(typ, tenancyStr, name), + Port: port, + } + } + return map[string]xRouteParentRefTestcase{ + "no parent refs": { + routeTenancy: resource.DefaultNamespacedTenancy(), + expectErr: `invalid "parent_refs" field: cannot be empty`, + }, + "parent ref with nil ref": { + routeTenancy: resource.DefaultNamespacedTenancy(), + refs: []*pbmesh.ParentReference{ + newParentRef(pbcatalog.ServiceType, "", "api", ""), + { + Ref: nil, + Port: "http", + }, + }, + expectErr: `invalid element at index 1 of list "parent_refs": invalid "ref" field: missing required field`, + }, + "parent ref with bad type ref": { + routeTenancy: resource.DefaultNamespacedTenancy(), + refs: []*pbmesh.ParentReference{ + newParentRef(pbcatalog.ServiceType, "", "api", ""), + newParentRef(pbcatalog.WorkloadType, "", "api", ""), + }, + expectErr: `invalid element at index 1 of list "parent_refs": invalid "ref" field: invalid "type" field: reference must have type catalog.v2beta1.Service`, + }, + "parent ref with section": { + routeTenancy: resource.DefaultNamespacedTenancy(), + refs: []*pbmesh.ParentReference{ + newParentRef(pbcatalog.ServiceType, "", "api", ""), + { + Ref: resourcetest.Resource(pbcatalog.ServiceType, "web").Reference("section2"), + Port: "http", + }, + }, + expectErr: `invalid element at index 1 of list "parent_refs": invalid "ref" field: invalid "section" field: section cannot be set here`, + }, + "cross namespace parent": { + routeTenancy: resource.DefaultNamespacedTenancy(), + refs: []*pbmesh.ParentReference{ + newParentRef(pbcatalog.ServiceType, "default.foo", "api", ""), + }, + expectErr: `invalid element at index 0 of list "parent_refs": invalid "ref" field: invalid "tenancy" field: resource tenancy and reference tenancy differ`, + }, + "cross partition parent": { + routeTenancy: resource.DefaultNamespacedTenancy(), + refs: []*pbmesh.ParentReference{ + newParentRef(pbcatalog.ServiceType, "alpha.default", "api", ""), + }, + expectErr: `invalid element at index 0 of list "parent_refs": invalid "ref" field: invalid "tenancy" field: resource tenancy and reference tenancy differ`, + }, + "cross tenancy parent": { + routeTenancy: resource.DefaultNamespacedTenancy(), + refs: []*pbmesh.ParentReference{ + newParentRef(pbcatalog.ServiceType, "alpha.foo", "api", ""), + }, + expectErr: `invalid element at index 0 of list "parent_refs": invalid "ref" field: invalid "tenancy" field: resource tenancy and reference tenancy differ`, + }, + "duplicate exact parents": { + routeTenancy: resource.DefaultNamespacedTenancy(), + refs: []*pbmesh.ParentReference{ + newParentRef(pbcatalog.ServiceType, "", "api", "http"), + newParentRef(pbcatalog.ServiceType, "", "api", "http"), + }, + expectErr: `invalid element at index 1 of list "parent_refs": invalid "port" field: parent ref "catalog.v2beta1.Service/default.local.default/api" for port "http" exists twice`, + }, + "duplicate wild parents": { + routeTenancy: resource.DefaultNamespacedTenancy(), + refs: []*pbmesh.ParentReference{ + newParentRef(pbcatalog.ServiceType, "", "api", ""), + newParentRef(pbcatalog.ServiceType, "", "api", ""), + }, + expectErr: `invalid element at index 1 of list "parent_refs": invalid "port" field: parent ref "catalog.v2beta1.Service/default.local.default/api" for wildcard port exists twice`, + }, + "duplicate parents via exact+wild overlap": { + routeTenancy: resource.DefaultNamespacedTenancy(), + refs: []*pbmesh.ParentReference{ + newParentRef(pbcatalog.ServiceType, "", "api", "http"), + newParentRef(pbcatalog.ServiceType, "", "api", ""), + }, + expectErr: `invalid element at index 1 of list "parent_refs": invalid "port" field: parent ref "catalog.v2beta1.Service/default.local.default/api" for ports [http] covered by wildcard port already`, + }, + "duplicate parents via exact+wild overlap (reversed)": { + routeTenancy: resource.DefaultNamespacedTenancy(), + refs: []*pbmesh.ParentReference{ + newParentRef(pbcatalog.ServiceType, "", "api", ""), + newParentRef(pbcatalog.ServiceType, "", "api", "http"), + }, + expectErr: `invalid element at index 1 of list "parent_refs": invalid "port" field: parent ref "catalog.v2beta1.Service/default.local.default/api" for port "http" covered by wildcard port already`, + }, + "good single parent ref": { + routeTenancy: resource.DefaultNamespacedTenancy(), + refs: []*pbmesh.ParentReference{ + newParentRef(pbcatalog.ServiceType, "", "api", "http"), + }, + }, + "good muliple parent refs": { + routeTenancy: resource.DefaultNamespacedTenancy(), + refs: []*pbmesh.ParentReference{ + newParentRef(pbcatalog.ServiceType, "", "api", "http"), + newParentRef(pbcatalog.ServiceType, "", "web", ""), + }, + }, + } +} + +type xRouteBackendRefTestcase struct { + routeTenancy *pbresource.Tenancy + refs []*pbmesh.BackendReference + expectErr string +} + +func getXRouteBackendRefTestCases() map[string]xRouteBackendRefTestcase { + newRef := func(typ *pbresource.Type, tenancyStr, name string) *pbresource.Reference { + return resourcetest.Resource(typ, name). + WithTenancy(resourcetest.Tenancy(tenancyStr)). + Reference("") + } + + newBackendRef := func(typ *pbresource.Type, tenancyStr, name, port string) *pbmesh.BackendReference { + return &pbmesh.BackendReference{ + Ref: newRef(typ, tenancyStr, name), + Port: port, + } + } + return map[string]xRouteBackendRefTestcase{ + "no backend refs": { + routeTenancy: resource.DefaultNamespacedTenancy(), + expectErr: `invalid "backend_refs" field: cannot be empty`, + }, + "backend ref with nil ref": { + routeTenancy: resource.DefaultNamespacedTenancy(), + refs: []*pbmesh.BackendReference{ + newBackendRef(pbcatalog.ServiceType, "", "api", ""), + { + Ref: nil, + Port: "http", + }, + }, + expectErr: `invalid element at index 0 of list "rules": invalid element at index 1 of list "backend_refs": invalid "backend_ref" field: invalid "ref" field: missing required field`, + }, + "backend ref with bad type ref": { + routeTenancy: resource.DefaultNamespacedTenancy(), + refs: []*pbmesh.BackendReference{ + newBackendRef(pbcatalog.ServiceType, "", "api", ""), + newBackendRef(pbcatalog.WorkloadType, "", "api", ""), + }, + expectErr: `invalid element at index 0 of list "rules": invalid element at index 1 of list "backend_refs": invalid "backend_ref" field: invalid "ref" field: invalid "type" field: reference must have type catalog.v2beta1.Service`, + }, + "backend ref with section": { + routeTenancy: resource.DefaultNamespacedTenancy(), + refs: []*pbmesh.BackendReference{ + newBackendRef(pbcatalog.ServiceType, "", "api", ""), + { + Ref: resourcetest.Resource(pbcatalog.ServiceType, "web").Reference("section2"), + Port: "http", + }, + }, + expectErr: `invalid element at index 0 of list "rules": invalid element at index 1 of list "backend_refs": invalid "backend_ref" field: invalid "ref" field: invalid "section" field: section cannot be set here`, + }, + "backend ref with datacenter": { + routeTenancy: resource.DefaultNamespacedTenancy(), + refs: []*pbmesh.BackendReference{ + newBackendRef(pbcatalog.ServiceType, "", "api", ""), + { + Ref: newRef(pbcatalog.ServiceType, "", "db"), + Port: "http", + Datacenter: "dc2", + }, + }, + expectErr: `invalid element at index 0 of list "rules": invalid element at index 1 of list "backend_refs": invalid "backend_ref" field: invalid "datacenter" field: datacenter is not yet supported on backend refs`, + }, + "cross namespace backend": { + routeTenancy: resource.DefaultNamespacedTenancy(), + refs: []*pbmesh.BackendReference{ + newBackendRef(pbcatalog.ServiceType, "default.foo", "api", ""), + }, + }, + "cross partition backend": { + routeTenancy: resource.DefaultNamespacedTenancy(), + refs: []*pbmesh.BackendReference{ + newBackendRef(pbcatalog.ServiceType, "alpha.default", "api", ""), + }, + }, + "cross tenancy backend": { + routeTenancy: resource.DefaultNamespacedTenancy(), + refs: []*pbmesh.BackendReference{ + newBackendRef(pbcatalog.ServiceType, "alpha.foo", "api", ""), + }, + }, + "good backend ref": { + routeTenancy: resource.DefaultNamespacedTenancy(), + refs: []*pbmesh.BackendReference{ + newBackendRef(pbcatalog.ServiceType, "", "api", ""), + { + Ref: newRef(pbcatalog.ServiceType, "", "db"), + Port: "http", + }, + }, + }, + } +} + +type xRouteTimeoutsTestcase struct { + timeouts *pbmesh.HTTPRouteTimeouts + expectErr string +} + +func getXRouteTimeoutsTestCases() map[string]xRouteTimeoutsTestcase { + return map[string]xRouteTimeoutsTestcase{ + "bad request": { + timeouts: &pbmesh.HTTPRouteTimeouts{ + Request: durationpb.New(-1 * time.Second), + }, + expectErr: `invalid element at index 0 of list "rules": invalid "timeouts" field: invalid "request" field: timeout cannot be negative: -1s`, + }, + "bad idle": { + timeouts: &pbmesh.HTTPRouteTimeouts{ + Idle: durationpb.New(-1 * time.Second), + }, + expectErr: `invalid element at index 0 of list "rules": invalid "timeouts" field: invalid "idle" field: timeout cannot be negative: -1s`, + }, + "good all": { + timeouts: &pbmesh.HTTPRouteTimeouts{ + Request: durationpb.New(1 * time.Second), + Idle: durationpb.New(3 * time.Second), + }, + }, + } +} + +type xRouteRetriesTestcase struct { + retries *pbmesh.HTTPRouteRetries + expectErr string +} + +func getXRouteRetriesTestCases() map[string]xRouteRetriesTestcase { + return map[string]xRouteRetriesTestcase{ + "bad conditions": { + retries: &pbmesh.HTTPRouteRetries{ + OnConditions: []string{"garbage"}, + }, + expectErr: `invalid element at index 0 of list "rules": invalid "retries" field: invalid element at index 0 of list "on_conditions": not a valid retry condition: "garbage"`, + }, + "good all": { + retries: &pbmesh.HTTPRouteRetries{ + Number: wrapperspb.UInt32(5), + OnConditions: []string{"internal"}, + }, + }, + } +} + func testXRouteACLs[R XRouteData](t *testing.T, newRoute func(t *testing.T, parentRefs, backendRefs []*pbresource.Reference) *pbresource.Resource) { // Wire up a registry to generically invoke hooks registry := resource.NewRegistry() @@ -164,3 +512,33 @@ func testXRouteACLs[R XRouteData](t *testing.T, newRoute func(t *testing.T, pare assert(t, "2parents 2backends", rules, resTwoParentsTwoBackends, DENY, DENY) }) } + +func newRef(typ *pbresource.Type, name string) *pbresource.Reference { + return resourcetest.Resource(typ, name). + WithTenancy(resource.DefaultNamespacedTenancy()). + Reference("") +} + +func newRefWithTenancy(typ *pbresource.Type, tenancyStr, name string) *pbresource.Reference { + return resourcetest.Resource(typ, name). + WithTenancy(resourcetest.Tenancy(tenancyStr)). + Reference("") +} + +func newBackendRef(typ *pbresource.Type, name, port string) *pbmesh.BackendReference { + return &pbmesh.BackendReference{ + Ref: newRef(typ, name), + Port: port, + } +} + +func newParentRef(typ *pbresource.Type, name, port string) *pbmesh.ParentReference { + return newParentRefWithTenancy(typ, "default.default", name, port) +} + +func newParentRefWithTenancy(typ *pbresource.Type, tenancyStr string, name, port string) *pbmesh.ParentReference { + return &pbmesh.ParentReference{ + Ref: newRefWithTenancy(typ, tenancyStr, name), + Port: port, + } +} diff --git a/internal/resource/resourcetest/tenancy.go b/internal/resource/resourcetest/tenancy.go new file mode 100644 index 0000000000..838379cebb --- /dev/null +++ b/internal/resource/resourcetest/tenancy.go @@ -0,0 +1,37 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package resourcetest + +import ( + "strings" + + "github.com/hashicorp/consul/internal/resource" + "github.com/hashicorp/consul/proto-public/pbresource" +) + +// Tenancy constructs a pbresource.Tenancy from a concise string representation +// suitable for use in unit tests. +// +// - "" : partition="" namespace="" peerName="local" +// - "foo" : partition="foo" namespace="" peerName="local" +// - "foo.bar" : partition="foo" namespace="bar" peerName="local" +// - : partition="BAD" namespace="BAD" peerName="BAD" +func Tenancy(s string) *pbresource.Tenancy { + parts := strings.Split(s, ".") + switch len(parts) { + case 0: + return resource.DefaultClusteredTenancy() + case 1: + v := resource.DefaultPartitionedTenancy() + v.Partition = parts[0] + return v + case 2: + v := resource.DefaultNamespacedTenancy() + v.Partition = parts[0] + v.Namespace = parts[1] + return v + default: + return &pbresource.Tenancy{Partition: "BAD", Namespace: "BAD", PeerName: "BAD"} + } +} diff --git a/internal/resource/tenancy.go b/internal/resource/tenancy.go index 99ae0becf4..126e12413f 100644 --- a/internal/resource/tenancy.go +++ b/internal/resource/tenancy.go @@ -103,6 +103,27 @@ func DefaultNamespacedTenancy() *pbresource.Tenancy { } } +// DefaultIDTenancy will default/normalize the Tenancy of the provided +// ID in the context of some parent resource containing that ID. +// The default tenancy for the ID's type is also provided in cases where +// "default" is needed selectively or the parent is more precise than the +// child. +func DefaultIDTenancy(id *pbresource.ID, parentTenancy, scopeTenancy *pbresource.Tenancy) { + if id == nil { + return + } + if id.Tenancy == nil { + id.Tenancy = &pbresource.Tenancy{} + } + + if parentTenancy != nil { + dup := proto.Clone(parentTenancy).(*pbresource.Tenancy) + parentTenancy = dup + } + + defaultTenancy(id.Tenancy, parentTenancy, scopeTenancy) +} + // DefaultReferenceTenancy will default/normalize the Tenancy of the provided // Reference in the context of some parent resource containing that Reference. // The default tenancy for the Reference's type is also provided in cases where