From 92aab7ea3127d64e9fc6acd5f938942512226464 Mon Sep 17 00:00:00 2001 From: Nitya Dhanushkodi Date: Mon, 29 Jan 2024 10:43:41 -0800 Subject: [PATCH] [NET-5586][rebased] v2: Support virtual port references in config (#20371) [OG Author: michael.zalimeni@hashicorp.com, rebase needed a separate PR] * v2: support virtual port in Service port references In addition to Service target port references, allow users to specify a port by stringified virtual port value. This is useful in environments such as Kubernetes where typical configuration is written in terms of Service virtual ports rather than workload (pod) target port names. Retaining the option of referencing target ports by name supports VMs, Nomad, and other use cases where virtual ports are not used by default. To support both uses cases at once, we will strictly interpret port references based on whether the value is numeric. See updated `ServicePort` docs for more details. * v2: update service ref docs for virtual port support Update proto and generated .go files with docs reflecting virtual port reference support. * v2: add virtual port references to L7 topo test Add coverage for mixed virtual and target port references to existing test. * update failover policy controller tests to work with computed failover policy and assert error conditions against FailoverPolicy and ComputedFailoverPolicy resources * accumulate services; don't overwrite them in enterprise --------- Co-authored-by: Michael Zalimeni Co-authored-by: R.B. Boyer --- .../v2beta1/foo-service-endpoints.json | 2 +- .../v2beta1/foo-service.json | 2 +- .../catalogtest/test_integration_v2beta1.go | 2 +- internal/catalog/exports.go | 4 +- .../controllers/failover/controller.go | 36 ++- .../controllers/failover/controller_test.go | 115 ++++++-- .../internal/controllers/failover/status.go | 17 +- internal/catalog/internal/types/errors.go | 4 +- .../catalog/internal/types/failover_policy.go | 35 ++- .../internal/types/failover_policy_test.go | 4 +- internal/catalog/internal/types/service.go | 4 +- .../internal/types/service_endpoints.go | 7 +- .../types/testdata/errNotDNSLabel.golden | 2 +- internal/catalog/internal/types/validators.go | 64 ++++- .../catalog/internal/types/validators_test.go | 163 ++++++++++- .../explicitdestinations/controller.go | 4 +- .../internal/controllers/routes/controller.go | 1 + .../routes/destination_policy_validation.go | 60 ++++ .../destination_policy_validation_test.go | 121 ++++++++ .../internal/controllers/routes/generate.go | 64 +++-- .../controllers/routes/generate_test.go | 269 ++++++++++++++++++ .../controllers/routes/ref_validation.go | 21 +- .../controllers/routes/ref_validation_test.go | 102 ++++--- .../internal/controllers/routes/status.go | 76 ++++- .../sidecarproxy/builder/destinations.go | 2 +- .../controllers/sidecarproxy/controller.go | 2 +- .../sidecarproxy/fetcher/data_fetcher.go | 4 +- .../mesh/internal/types/destination_policy.go | 44 +++ internal/mesh/internal/types/destinations.go | 2 +- .../pbcatalog/v2beta1/failover_policy.pb.go | 9 +- .../pbcatalog/v2beta1/failover_policy.proto | 10 +- proto-public/pbcatalog/v2beta1/service.pb.go | 7 + proto-public/pbcatalog/v2beta1/service.proto | 7 + .../pbcatalog/v2beta1/service_addon.go | 102 ++++++- .../pbcatalog/v2beta1/service_addon_test.go | 109 ++++++- proto-public/pbmesh/v2beta1/common.pb.go | 7 +- proto-public/pbmesh/v2beta1/common.proto | 7 +- .../pbmesh/v2beta1/computed_routes.pb.go | 6 + .../pbmesh/v2beta1/computed_routes.proto | 6 + .../pbmesh/v2beta1/destination_policy.pb.go | 8 +- .../pbmesh/v2beta1/destination_policy.proto | 8 +- .../pbmesh/v2beta1/destinations.pb.go | 5 +- .../pbmesh/v2beta1/destinations.proto | 5 +- .../v2beta1/destinations_configuration.pb.go | 9 +- .../v2beta1/destinations_configuration.proto | 9 +- .../explicit_destinations_l7_test.go | 72 +++-- testing/deployer/topology/compile.go | 17 +- testing/deployer/topology/topology.go | 5 +- 48 files changed, 1459 insertions(+), 182 deletions(-) create mode 100644 internal/mesh/internal/controllers/routes/destination_policy_validation.go create mode 100644 internal/mesh/internal/controllers/routes/destination_policy_validation_test.go diff --git a/internal/catalog/catalogtest/integration_test_data/v2beta1/foo-service-endpoints.json b/internal/catalog/catalogtest/integration_test_data/v2beta1/foo-service-endpoints.json index fe7925a885..a980939a96 100644 --- a/internal/catalog/catalogtest/integration_test_data/v2beta1/foo-service-endpoints.json +++ b/internal/catalog/catalogtest/integration_test_data/v2beta1/foo-service-endpoints.json @@ -35,7 +35,7 @@ } ], "ports": { - "external-service-port": { + "ext-svc-port": { "port": 9876, "protocol": "PROTOCOL_HTTP2" } diff --git a/internal/catalog/catalogtest/integration_test_data/v2beta1/foo-service.json b/internal/catalog/catalogtest/integration_test_data/v2beta1/foo-service.json index bbe87511e1..3793dd07b0 100644 --- a/internal/catalog/catalogtest/integration_test_data/v2beta1/foo-service.json +++ b/internal/catalog/catalogtest/integration_test_data/v2beta1/foo-service.json @@ -16,7 +16,7 @@ "@type": "hashicorp.consul.catalog.v2beta1.Service", "ports": [ { - "target_port": "external-service-port", + "target_port": "ext-svc-port", "protocol": "PROTOCOL_HTTP2" } ] diff --git a/internal/catalog/catalogtest/test_integration_v2beta1.go b/internal/catalog/catalogtest/test_integration_v2beta1.go index fed65c4e71..509314a48f 100644 --- a/internal/catalog/catalogtest/test_integration_v2beta1.go +++ b/internal/catalog/catalogtest/test_integration_v2beta1.go @@ -149,7 +149,7 @@ func expectedFooServiceEndpoints() *pbcatalog.ServiceEndpoints { {Host: "198.18.0.1"}, }, Ports: map[string]*pbcatalog.WorkloadPort{ - "external-service-port": { + "ext-svc-port": { Port: 9876, Protocol: pbcatalog.Protocol_PROTOCOL_HTTP2, }, diff --git a/internal/catalog/exports.go b/internal/catalog/exports.go index d864d7acb1..5bc889a1f5 100644 --- a/internal/catalog/exports.go +++ b/internal/catalog/exports.go @@ -86,8 +86,8 @@ func ValidateSelector(sel *pbcatalog.WorkloadSelector, allowEmpty bool) error { return types.ValidateSelector(sel, allowEmpty) } -func ValidatePortName(name string) error { - return types.ValidatePortName(name) +func ValidateServicePortID(id string) error { + return types.ValidateServicePortID(id) } func IsValidUnixSocketPath(host string) bool { diff --git a/internal/catalog/internal/controllers/failover/controller.go b/internal/catalog/internal/controllers/failover/controller.go index da2c9de6f1..e2dc8accf3 100644 --- a/internal/catalog/internal/controllers/failover/controller.go +++ b/internal/catalog/internal/controllers/failover/controller.go @@ -6,6 +6,9 @@ package failover import ( "context" + "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/types/known/anypb" + "github.com/hashicorp/consul/internal/catalog/internal/controllers/failover/expander" "github.com/hashicorp/consul/internal/catalog/internal/types" "github.com/hashicorp/consul/internal/controller" @@ -16,8 +19,6 @@ import ( pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v2beta1" pbmulticluster "github.com/hashicorp/consul/proto-public/pbmulticluster/v2beta1" "github.com/hashicorp/consul/proto-public/pbresource" - "google.golang.org/protobuf/proto" - "google.golang.org/protobuf/types/known/anypb" ) const ( @@ -89,6 +90,8 @@ func (r *failoverPolicyReconciler) Reconcile(ctx context.Context, rt controller. return nil } + // Capture original raw config for pre-normalization status conditions. + rawFailoverPolicy := failoverPolicy.Data // FailoverPolicy is name-aligned with the Service it controls. serviceID := &pbresource.ID{ @@ -149,13 +152,13 @@ func (r *failoverPolicyReconciler) Reconcile(ctx context.Context, rt controller. } } - conds := computeNewConditions(failoverPolicy.Resource, newComputedFailoverPolicy, service, destServices, missingSamenessGroups) + conds := computeNewConditions(rawFailoverPolicy, failoverPolicy.Resource, newComputedFailoverPolicy, service, destServices, missingSamenessGroups) if err := writeStatus(ctx, rt, failoverPolicy.Resource, conds); err != nil { rt.Logger.Error("error encountered when attempting to update the resource's failover policy status", "error", err) return err } - conds = computeNewConditions(computedFailoverResource, newComputedFailoverPolicy, service, destServices, missingSamenessGroups) + conds = computeNewConditions(rawFailoverPolicy, computedFailoverResource, newComputedFailoverPolicy, service, destServices, missingSamenessGroups) if err := writeStatus(ctx, rt, computedFailoverResource, conds); err != nil { rt.Logger.Error("error encountered when attempting to update the resource's computed failover policy status", "error", err) return err @@ -165,6 +168,7 @@ func (r *failoverPolicyReconciler) Reconcile(ctx context.Context, rt controller. } func computeNewConditions( + rawFailoverPolicy *pbcatalog.FailoverPolicy, fpRes *pbresource.Resource, fp *pbcatalog.ComputedFailoverPolicy, service *resource.DecodedResource[*pbcatalog.Service], @@ -182,11 +186,27 @@ func computeNewConditions( var conditions []*pbresource.Condition - for port, pc := range fp.GetPortConfigs() { - if _, ok := allowedPortProtocols[port]; !ok { - conditions = append(conditions, ConditionUnknownPort(port)) - } + if rawFailoverPolicy != nil { + // We need to validate port mappings on the raw input config due to the + // possibility of duplicate mappings, which will be normalized into one + // mapping by target port key. + usedTargetPorts := make(map[string]any) + for port := range rawFailoverPolicy.PortConfigs { + svcPort := service.Data.FindPortByID(port) + targetPort := svcPort.GetTargetPort() // svcPort could be nil + serviceRef := resource.NewReferenceKey(service.Id).ToReference() + if svcPort == nil { + conditions = append(conditions, ConditionUnknownPort(serviceRef, port)) + } else if _, ok := usedTargetPorts[targetPort]; ok { + conditions = append(conditions, ConditionConflictDestinationPort(serviceRef, svcPort)) + } else { + usedTargetPorts[targetPort] = struct{}{} + } + } + } + + for _, pc := range fp.GetPortConfigs() { for _, dest := range pc.Destinations { // We know from validation that a Ref must be set, and the type it // points to is a Service. diff --git a/internal/catalog/internal/controllers/failover/controller_test.go b/internal/catalog/internal/controllers/failover/controller_test.go index 2c8b428aec..646578beb6 100644 --- a/internal/catalog/internal/controllers/failover/controller_test.go +++ b/internal/catalog/internal/controllers/failover/controller_test.go @@ -67,8 +67,9 @@ func TestController(t *testing.T) { apiServiceData := &pbcatalog.Service{ Workloads: &pbcatalog.WorkloadSelector{Prefixes: []string{"api-"}}, Ports: []*pbcatalog.ServicePort{{ - TargetPort: "http", - Protocol: pbcatalog.Protocol_PROTOCOL_HTTP, + VirtualPort: 8080, + TargetPort: "http", + Protocol: pbcatalog.Protocol_PROTOCOL_HTTP, }}, } svc := rtest.Resource(pbcatalog.ServiceType, "api"). @@ -108,7 +109,60 @@ func TestController(t *testing.T) { t.Logf("reconciled to accepted") - // Update the failover to reference an unknown port + // Update the failover to reference a port twice (once by virtual, once by target port) + failoverData = &pbcatalog.FailoverPolicy{ + PortConfigs: map[string]*pbcatalog.FailoverConfig{ + "http": { + Destinations: []*pbcatalog.FailoverDestination{{ + Ref: apiServiceRef, + Port: "http", + }}, + }, + "8080": { + Destinations: []*pbcatalog.FailoverDestination{{ + Ref: apiServiceRef, + Port: "http", + }}, + }, + }, + } + failover = rtest.Resource(pbcatalog.FailoverPolicyType, "api"). + WithData(t, failoverData). + WithTenancy(tenancy). + Write(t, client) + + t.Cleanup(func() { client.MustDelete(t, failover.Id) }) + + // Assert that the FailoverPolicy has the conflict condition. + client.WaitForStatusCondition(t, failover.Id, ControllerID, ConditionConflictDestinationPort(apiServiceRef, &pbcatalog.ServicePort{ + VirtualPort: 8080, + TargetPort: "http", + })) + + // Assert that the ComputedFailoverPolicy has the conflict condition. + // The port normalization that occurs in the call to SimplifyFailoverPolicy results in the port being + // removed from the final FailoverPolicy and ComputedFailoverPolicy. + expFailoverData := &pbcatalog.FailoverPolicy{ + PortConfigs: map[string]*pbcatalog.FailoverConfig{ + "http": { + Destinations: []*pbcatalog.FailoverDestination{{ + Ref: apiServiceRef, + Port: "http", + }}, + }, + }, + } + expectedComputedFP = &pbcatalog.ComputedFailoverPolicy{ + PortConfigs: expFailoverData.PortConfigs, + BoundReferences: []*pbresource.Reference{apiServiceRef}, + } + waitAndAssertComputedFailoverPolicy(t, client, failover.Id, expectedComputedFP, ConditionConflictDestinationPort(apiServiceRef, &pbcatalog.ServicePort{ + VirtualPort: 8080, + TargetPort: "http", + })) + t.Logf("reconciled to using duplicate destination port") + + // Update the failover to fix the duplicate, but reference an unknown port failoverData = &pbcatalog.FailoverPolicy{ PortConfigs: map[string]*pbcatalog.FailoverConfig{ "http": { @@ -132,11 +186,27 @@ func TestController(t *testing.T) { t.Cleanup(func() { client.MustDelete(t, failover.Id) }) + // Assert that the FailoverPolicy has the unknown condition. + client.WaitForStatusCondition(t, failover.Id, ControllerID, ConditionUnknownPort(apiServiceRef, "admin")) + + // Assert that the ComputedFailoverPolicy has the unknown condition. + // The port normalization that occurs in the call to SimplifyFailoverPolicy results in the port being + // removed from the final FailoverPolicy and ComputedFailoverPolicy. + expFailoverData = &pbcatalog.FailoverPolicy{ + PortConfigs: map[string]*pbcatalog.FailoverConfig{ + "http": { + Destinations: []*pbcatalog.FailoverDestination{{ + Ref: apiServiceRef, + Port: "http", + }}, + }, + }, + } expectedComputedFP = &pbcatalog.ComputedFailoverPolicy{ - PortConfigs: failoverData.PortConfigs, + PortConfigs: expFailoverData.PortConfigs, BoundReferences: []*pbresource.Reference{apiServiceRef}, } - waitAndAssertComputedFailoverPolicy(t, client, failover.Id, expectedComputedFP, ConditionUnknownPort("admin")) + waitAndAssertComputedFailoverPolicy(t, client, failover.Id, expectedComputedFP, ConditionUnknownPort(apiServiceRef, "admin")) t.Logf("reconciled to unknown admin port") // update the service to fix the stray reference, but point to a mesh port @@ -144,15 +214,22 @@ func TestController(t *testing.T) { Workloads: &pbcatalog.WorkloadSelector{Prefixes: []string{"api-"}}, Ports: []*pbcatalog.ServicePort{ { - TargetPort: "http", - Protocol: pbcatalog.Protocol_PROTOCOL_HTTP, + TargetPort: "http", + VirtualPort: 8080, + Protocol: pbcatalog.Protocol_PROTOCOL_HTTP, }, { - TargetPort: "admin", - Protocol: pbcatalog.Protocol_PROTOCOL_MESH, + TargetPort: "admin", + VirtualPort: 10000, + Protocol: pbcatalog.Protocol_PROTOCOL_MESH, }, }, } + // update the expected ComputedFailoverPolicy to add back in the admin port as well + expectedComputedFP = &pbcatalog.ComputedFailoverPolicy{ + PortConfigs: failoverData.PortConfigs, + BoundReferences: []*pbresource.Reference{apiServiceRef}, + } svc = rtest.Resource(pbcatalog.ServiceType, "api"). WithData(t, apiServiceData). WithTenancy(tenancy). @@ -168,12 +245,14 @@ func TestController(t *testing.T) { Workloads: &pbcatalog.WorkloadSelector{Prefixes: []string{"api-"}}, Ports: []*pbcatalog.ServicePort{ { - TargetPort: "http", - Protocol: pbcatalog.Protocol_PROTOCOL_HTTP, + VirtualPort: 8080, + TargetPort: "http", + Protocol: pbcatalog.Protocol_PROTOCOL_HTTP, }, { - TargetPort: "admin", - Protocol: pbcatalog.Protocol_PROTOCOL_HTTP, + VirtualPort: 10000, + TargetPort: "admin", + Protocol: pbcatalog.Protocol_PROTOCOL_HTTP, }, }, } @@ -253,12 +332,14 @@ func TestController(t *testing.T) { Workloads: &pbcatalog.WorkloadSelector{Prefixes: []string{"other-"}}, Ports: []*pbcatalog.ServicePort{ { - TargetPort: "http", - Protocol: pbcatalog.Protocol_PROTOCOL_HTTP, + VirtualPort: 8080, + TargetPort: "http", + Protocol: pbcatalog.Protocol_PROTOCOL_HTTP, }, { - TargetPort: "admin", - Protocol: pbcatalog.Protocol_PROTOCOL_HTTP, + VirtualPort: 10000, + TargetPort: "admin", + Protocol: pbcatalog.Protocol_PROTOCOL_HTTP, }, }, } diff --git a/internal/catalog/internal/controllers/failover/status.go b/internal/catalog/internal/controllers/failover/status.go index 1f4b022697..5eecda1f60 100644 --- a/internal/catalog/internal/controllers/failover/status.go +++ b/internal/catalog/internal/controllers/failover/status.go @@ -5,6 +5,7 @@ package failover import ( "github.com/hashicorp/consul/internal/resource" + pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v2beta1" "github.com/hashicorp/consul/proto-public/pbresource" ) @@ -32,6 +33,9 @@ const ( MissingSamenessGroupReason = "MissingSamenessGroup" MissingSamenessGroupMessagePrefix = "referenced sameness group does not exist: " + + ConflictDestinationPortReason = "ConflictDestinationPort" + ConflictDestinationPortMessagePrefix = "multiple configs found for port on destination service: " ) var ( @@ -50,12 +54,12 @@ var ( } ) -func ConditionUnknownPort(port string) *pbresource.Condition { +func ConditionUnknownPort(ref *pbresource.Reference, port string) *pbresource.Condition { return &pbresource.Condition{ Type: StatusConditionAccepted, State: pbresource.Condition_STATE_FALSE, Reason: UnknownPortReason, - Message: UnknownPortMessagePrefix + port, + Message: UnknownPortMessagePrefix + port + " on " + resource.ReferenceToString(ref), } } @@ -94,3 +98,12 @@ func ConditionMissingSamenessGroup(ref *pbresource.Reference) *pbresource.Condit Message: MissingSamenessGroupMessagePrefix + resource.ReferenceToString(ref), } } + +func ConditionConflictDestinationPort(ref *pbresource.Reference, port *pbcatalog.ServicePort) *pbresource.Condition { + return &pbresource.Condition{ + Type: StatusConditionAccepted, + State: pbresource.Condition_STATE_FALSE, + Reason: ConflictDestinationPortReason, + Message: ConflictDestinationPortMessagePrefix + port.ToPrintableString() + " on " + resource.ReferenceToString(ref), + } +} diff --git a/internal/catalog/internal/types/errors.go b/internal/catalog/internal/types/errors.go index 3b331a9a63..7ddbd00b4b 100644 --- a/internal/catalog/internal/types/errors.go +++ b/internal/catalog/internal/types/errors.go @@ -9,11 +9,13 @@ import ( ) var ( - errNotDNSLabel = errors.New(fmt.Sprintf("value must match regex: %s", dnsLabelRegex)) + errNotDNSLabel = errors.New(fmt.Sprintf("value must be 1-63 characters and match regex: %s", dnsLabelRegex)) + errNotPortName = errors.New(fmt.Sprintf("value must be 1-15 characters, contain at least 1 alpha, and match regex: %s", ianaSvcNameRegex)) 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") + errInvalidPortID = errors.New(fmt.Sprintf("value must be in the range 1 to 65535 or be 1-15 characters, contain at least 1 alpha, and match regex: %s", ianaSvcNameRegex)) 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") diff --git a/internal/catalog/internal/types/failover_policy.go b/internal/catalog/internal/types/failover_policy.go index 0cd0d3b71b..2e9546b336 100644 --- a/internal/catalog/internal/types/failover_policy.go +++ b/internal/catalog/internal/types/failover_policy.go @@ -135,19 +135,19 @@ func validateCommonFailoverConfigs(res *pbcatalog.FailoverPolicy) error { } } - for portName, pc := range res.PortConfigs { + for portId, pc := range res.PortConfigs { wrapConfigErr := func(err error) error { return resource.ErrInvalidMapValue{ Map: "port_configs", - Key: portName, + Key: portId, Wrapped: err, } } - if portNameErr := ValidatePortName(portName); portNameErr != nil { + if portIdErr := ValidateServicePortID(portId); portIdErr != nil { merr = multierror.Append(merr, resource.ErrInvalidMapKey{ Map: "port_configs", - Key: portName, - Wrapped: portNameErr, + Key: portId, + Wrapped: portIdErr, }) } @@ -234,10 +234,10 @@ func validateFailoverPolicyDestination(dest *pbcatalog.FailoverDestination, port // assumed and will be reconciled. if dest.Port != "" { if ported { - if portNameErr := ValidatePortName(dest.Port); portNameErr != nil { + if portIdErr := ValidateServicePortID(dest.Port); portIdErr != nil { merr = multierror.Append(merr, wrapErr(resource.ErrInvalidField{ Name: "port", - Wrapped: portNameErr, + Wrapped: portIdErr, })) } } else { @@ -281,6 +281,27 @@ func SimplifyFailoverPolicy(svc *pbcatalog.Service, failover *pbcatalog.Failover failover.PortConfigs = make(map[string]*pbcatalog.FailoverConfig) } + // Normalize all port configs to use the target port of the corresponding service port. + normalizedPortConfigs := make(map[string]*pbcatalog.FailoverConfig) + for port, pc := range failover.PortConfigs { + svcPort := svc.FindPortByID(port) + + if svcPort != nil { + if _, ok := normalizedPortConfigs[svcPort.TargetPort]; ok { + // This is a duplicate virtual and target port mapping that will be reported as a status condition. + // Only update if this is the "canonical" mapping; otherwise, it's virtual, and we should ignore. + if port != svcPort.TargetPort { + continue + } + } + normalizedPortConfigs[svcPort.TargetPort] = pc + } + // Else this is an invalid reference that will be reported as a status condition. + // Drop for safety and simpler output. + } + + failover.PortConfigs = normalizedPortConfigs + for _, port := range svc.Ports { if port.Protocol == pbcatalog.Protocol_PROTOCOL_MESH { continue // skip diff --git a/internal/catalog/internal/types/failover_policy_test.go b/internal/catalog/internal/types/failover_policy_test.go index 0bf933a482..84afa90ac0 100644 --- a/internal/catalog/internal/types/failover_policy_test.go +++ b/internal/catalog/internal/types/failover_policy_test.go @@ -329,7 +329,7 @@ func getCommonFpCases() map[string]failoverTestcase { }, }, }, - expectErr: `invalid value of key "http" within port_configs: invalid element at index 0 of list "destinations": invalid "port" field: value must match regex: ^[a-z0-9]([a-z0-9\-_]*[a-z0-9])?$`, + expectErr: `invalid value of key "http" within port_configs: invalid element at index 0 of list "destinations": invalid "port" field: value must be in the range 1 to 65535 or be 1-15 characters, contain at least 1 alpha, and match regex: ^[a-zA-Z0-9]+(?:-?[a-zA-Z0-9]+)*$`, }, "ported config: bad ported in map": { failover: &pbcatalog.FailoverPolicy{ @@ -341,7 +341,7 @@ func getCommonFpCases() map[string]failoverTestcase { }, }, }, - expectErr: `map port_configs contains an invalid key - "$bad$": value must match regex: ^[a-z0-9]([a-z0-9\-_]*[a-z0-9])?$`, + expectErr: `map port_configs contains an invalid key - "$bad$": value must be in the range 1 to 65535 or be 1-15 characters, contain at least 1 alpha, and match regex: ^[a-zA-Z0-9]+(?:-?[a-zA-Z0-9]+)*$`, }, } return fpcases diff --git a/internal/catalog/internal/types/service.go b/internal/catalog/internal/types/service.go index 4b243bf152..456cacc93b 100644 --- a/internal/catalog/internal/types/service.go +++ b/internal/catalog/internal/types/service.go @@ -4,8 +4,6 @@ package types import ( - "math" - "github.com/hashicorp/go-multierror" "github.com/hashicorp/consul/internal/catalog/workloadselector" @@ -105,7 +103,7 @@ func validateService(res *DecodedService) error { // 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 { + if portErr := ValidateVirtualPort(port.VirtualPort); portErr != nil { err = multierror.Append(err, resource.ErrInvalidListElement{ Name: "ports", Index: idx, diff --git a/internal/catalog/internal/types/service_endpoints.go b/internal/catalog/internal/types/service_endpoints.go index 91a5d15207..4b3c6ef9f5 100644 --- a/internal/catalog/internal/types/service_endpoints.go +++ b/internal/catalog/internal/types/service_endpoints.go @@ -4,8 +4,6 @@ package types import ( - "math" - "github.com/hashicorp/go-multierror" "github.com/hashicorp/consul/acl" @@ -133,7 +131,6 @@ func validateEndpoint(endpoint *pbcatalog.Endpoint, res *pbresource.Resource) er // Validate the endpoints ports for portName, port := range endpoint.Ports { - // Port names must be DNS labels if portNameErr := ValidatePortName(portName); portNameErr != nil { err = multierror.Append(err, resource.ErrInvalidMapKey{ Map: "ports", @@ -155,13 +152,13 @@ func validateEndpoint(endpoint *pbcatalog.Endpoint, res *pbresource.Resource) er // 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 { + if portErr := ValidatePhysicalPort(port.Port); portErr != nil { err = multierror.Append(err, resource.ErrInvalidMapValue{ Map: "ports", Key: portName, Wrapped: resource.ErrInvalidField{ Name: "physical_port", - Wrapped: errInvalidPhysicalPort, + Wrapped: portErr, }, }) } diff --git a/internal/catalog/internal/types/testdata/errNotDNSLabel.golden b/internal/catalog/internal/types/testdata/errNotDNSLabel.golden index a5866fbbf0..2aa54ca0fc 100644 --- a/internal/catalog/internal/types/testdata/errNotDNSLabel.golden +++ b/internal/catalog/internal/types/testdata/errNotDNSLabel.golden @@ -1 +1 @@ -value must match regex: ^[a-z0-9]([a-z0-9\-_]*[a-z0-9])?$ \ No newline at end of file +value must be 1-63 characters and match regex: ^[a-z0-9]([a-z0-9\-_]*[a-z0-9])?$ \ No newline at end of file diff --git a/internal/catalog/internal/types/validators.go b/internal/catalog/internal/types/validators.go index a0ddad0b08..f5501a27b0 100644 --- a/internal/catalog/internal/types/validators.go +++ b/internal/catalog/internal/types/validators.go @@ -9,6 +9,7 @@ import ( "math" "net" "regexp" + "strconv" "strings" "github.com/hashicorp/go-multierror" @@ -23,11 +24,21 @@ 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 + + // IANA service name. Applies to non-numeric port names in Consul and Kubernetes. + // See https://datatracker.ietf.org/doc/html/rfc6335#section-5.1 for definition. + // Length and at-least-one-alpha requirements are checked separately since + // Go re2 does not have lookaheads and for pattern legibility. + ianaSvcNameRegex = `^[a-zA-Z0-9]+(?:-?[a-zA-Z0-9]+)*$` + atLeastOneAlphaRegex = `^.*[a-zA-Z].*$` ) var ( dnsLabelRegex = `^[a-z0-9]([a-z0-9\-_]*[a-z0-9])?$` dnsLabelMatcher = regexp.MustCompile(dnsLabelRegex) + + ianaSvcNameMatcher = regexp.MustCompile(ianaSvcNameRegex) + atLeastOneAlphaMatcher = regexp.MustCompile(atLeastOneAlphaRegex) ) func isValidIPAddress(host string) bool { @@ -57,6 +68,19 @@ func isValidDNSLabel(label string) bool { return dnsLabelMatcher.Match([]byte(label)) } +func isValidPortName(name string) bool { + if len(name) > 15 { + return false + } + + nameB := []byte(name) + return atLeastOneAlphaMatcher.Match(nameB) && ianaSvcNameMatcher.Match([]byte(name)) +} + +func isValidPhysicalPortNumber[V int | uint32](i V) bool { + return i > 0 && i <= math.MaxUint16 +} + func IsValidUnixSocketPath(host string) bool { if len(host) > maxUnixSocketPathLen || !strings.HasPrefix(host, "unix://") || strings.Contains(host, "\000") { return false @@ -145,8 +169,44 @@ func ValidatePortName(name string) error { return resource.ErrEmpty } - if !isValidDNSLabel(name) { - return errNotDNSLabel + if !isValidPortName(name) { + return errNotPortName + } + + return nil +} + +// ValidateServicePortID validates that the given string is a valid ID for referencing +// aservice port. This can be either a string virtual port number or target port name. +// See Service.ServicePort doc for more details. +func ValidateServicePortID(id string) error { + if id == "" { + return resource.ErrEmpty + } + + if !isValidPortName(id) { + // Unlike an unset ServicePort.VirtualPort, a _reference_ to a service virtual + // port must be a real port number. + if i, err := strconv.Atoi(id); err != nil || !isValidPhysicalPortNumber(i) { + return errInvalidPortID + } + } + + return nil +} + +func ValidateVirtualPort[V int | uint32](port V) error { + // Allow 0 for unset virtual port values. + if port != 0 && !isValidPhysicalPortNumber(port) { + return errInvalidVirtualPort + } + + return nil +} + +func ValidatePhysicalPort[V int | uint32](port V) error { + if !isValidPhysicalPortNumber(port) { + return errInvalidPhysicalPort } return nil diff --git a/internal/catalog/internal/types/validators_test.go b/internal/catalog/internal/types/validators_test.go index 282f4d2a84..55405c4e55 100644 --- a/internal/catalog/internal/types/validators_test.go +++ b/internal/catalog/internal/types/validators_test.go @@ -48,6 +48,10 @@ func TestIsValidDNSLabel(t *testing.T) { name: "1abc", valid: true, }, + "fully-numeric": { + name: "1234", + valid: true, + }, "underscore-start-not-allowed": { name: "_abc", valid: false, @@ -151,6 +155,56 @@ func TestIsValidIPAddress(t *testing.T) { } } +// TestIsValidPort tests both physical and virtual port validation using +// the same cases to ensure same coverage. +func TestIsValidPort(t *testing.T) { + type testCase struct { + port int + validVirtual bool + validPhysical bool + } + + cases := map[string]testCase{ + "negative": { + port: -1, + validPhysical: false, + validVirtual: false, + }, + "zero": { + port: 0, + validPhysical: false, + validVirtual: true, + }, + "min": { + port: 1, + validPhysical: true, + validVirtual: true, + }, + "8080": { + port: 8080, + validPhysical: true, + validVirtual: true, + }, + "max": { + port: 65535, + validPhysical: true, + validVirtual: true, + }, + "above-max": { + port: 65536, + validPhysical: false, + validVirtual: false, + }, + } + + for name, tcase := range cases { + t.Run(name, func(t *testing.T) { + require.Equal(t, tcase.validPhysical, isValidPhysicalPortNumber(tcase.port)) + require.Equal(t, tcase.validVirtual, ValidateVirtualPort(tcase.port) == nil) + }) + } +} + func TestIsValidUnixSocketPath(t *testing.T) { type testCase struct { name string @@ -354,22 +408,117 @@ func TestValidateIPAddress(t *testing.T) { } func TestValidatePortName(t *testing.T) { + type testCase struct { + name string + valid bool + } + + cases := map[string]testCase{ + "min-length": { + name: "a", + valid: true, + }, + "max-length": { + name: "a1b2c3d4e5f6g7h", + valid: true, + }, + "underscore-not-allowed": { + name: "has_underscores", + valid: false, + }, + "hyphenated": { + name: "has-hyphen3", + valid: true, + }, + "uppercase-allowed": { + name: "UPPERCASE", + valid: true, + }, + "numeric-start": { + name: "1abc", + valid: true, + }, + "numeric-start-with-hypen": { + name: "1-abc", + valid: true, + }, + "at-least-one-alpha-required": { + name: "1234", + valid: false, + }, + "hyphen-start-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("a", 16), + valid: false, + }, + "missing-name": { + name: "", + valid: false, + }, + } + + for name, tcase := range cases { + t.Run(name, func(t *testing.T) { + err := ValidatePortName(tcase.name) + if tcase.valid { + require.NoError(t, err) + } else { + require.Error(t, err) + if tcase.name == "" { + require.Equal(t, resource.ErrEmpty, err) + } else { + require.Equal(t, errNotPortName, err) + } + } + }) + } +} + +func TestValidatePortID(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. + // a valid port ID because it is a combination of ValidatePortName and + // ValidateVirtualPort. In general the criteria is that it must not + // be empty and must be either a valid DNS label or stringified port + // number between 1 and 65535. Extensive testing is performed within the + // tests for those functions. t.Run("empty", func(t *testing.T) { - require.Equal(t, resource.ErrEmpty, ValidatePortName("")) + require.Equal(t, resource.ErrEmpty, ValidateServicePortID("")) }) t.Run("invalid", func(t *testing.T) { - require.Equal(t, errNotDNSLabel, ValidatePortName("foo.com")) + require.Equal(t, errInvalidPortID, ValidateServicePortID("foo.com")) + }) + + t.Run("invalid", func(t *testing.T) { + require.Equal(t, errInvalidPortID, ValidateServicePortID("-1")) + }) + + t.Run("invalid", func(t *testing.T) { + require.Equal(t, errInvalidPortID, ValidateServicePortID("0")) + }) + + t.Run("invalid", func(t *testing.T) { + require.Equal(t, errInvalidPortID, ValidateServicePortID("65536")) }) t.Run("ok", func(t *testing.T) { - require.NoError(t, ValidatePortName("http")) + require.NoError(t, ValidateServicePortID("http")) + }) + + t.Run("ok", func(t *testing.T) { + require.NoError(t, ValidateServicePortID("8080")) }) } diff --git a/internal/mesh/internal/controllers/explicitdestinations/controller.go b/internal/mesh/internal/controllers/explicitdestinations/controller.go index 58e2f615ad..0f69641efe 100644 --- a/internal/mesh/internal/controllers/explicitdestinations/controller.go +++ b/internal/mesh/internal/controllers/explicitdestinations/controller.go @@ -213,8 +213,8 @@ func validate( return false, ConditionMeshProtocolNotFound(serviceRef) } - if service.GetData().FindServicePort(dest.DestinationPort) != nil && - service.GetData().FindServicePort(dest.DestinationPort).Protocol == pbcatalog.Protocol_PROTOCOL_MESH { + if service.GetData().FindPortByID(dest.DestinationPort) != nil && + service.GetData().FindPortByID(dest.DestinationPort).Protocol == pbcatalog.Protocol_PROTOCOL_MESH { return false, ConditionMeshProtocolDestinationPort(serviceRef, dest.DestinationPort) } diff --git a/internal/mesh/internal/controllers/routes/controller.go b/internal/mesh/internal/controllers/routes/controller.go index 7500129604..e21c528f11 100644 --- a/internal/mesh/internal/controllers/routes/controller.go +++ b/internal/mesh/internal/controllers/routes/controller.go @@ -83,6 +83,7 @@ func (r *routesReconciler) Reconcile(ctx context.Context, rt controller.Runtime, pending := make(PendingStatuses) ValidateXRouteReferences(related, pending) + ValidateDestinationPolicyPorts(related, pending) generatedResults := GenerateComputedRoutes(related, pending) diff --git a/internal/mesh/internal/controllers/routes/destination_policy_validation.go b/internal/mesh/internal/controllers/routes/destination_policy_validation.go new file mode 100644 index 0000000000..99a5ddb573 --- /dev/null +++ b/internal/mesh/internal/controllers/routes/destination_policy_validation.go @@ -0,0 +1,60 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package routes + +import ( + "github.com/hashicorp/consul/internal/mesh/internal/controllers/routes/loader" + "github.com/hashicorp/consul/internal/mesh/internal/types" + "github.com/hashicorp/consul/internal/resource" + pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v2beta1" + "github.com/hashicorp/consul/proto-public/pbresource" +) + +// ValidateDestinationPolicyPorts examines the ported configs of the policies provided +// and issues status conditions if anything is unacceptable. +func ValidateDestinationPolicyPorts(related *loader.RelatedResources, pending PendingStatuses) { + for rk, destPolicy := range related.DestinationPolicies { + conditions := computeNewDestPolicyPortConditions(related, rk, destPolicy) + pending.AddConditions(rk, destPolicy.Resource, conditions) + } +} + +func computeNewDestPolicyPortConditions( + related serviceGetter, + rk resource.ReferenceKey, + destPolicy *types.DecodedDestinationPolicy, +) []*pbresource.Condition { + var conditions []*pbresource.Condition + + // Since this is name-aligned, just switch the type to fetch the service. + service := related.GetService(resource.ReplaceType(pbcatalog.ServiceType, rk.ToID())) + if service != nil { + allowedPortProtocols := make(map[string]pbcatalog.Protocol) + for _, port := range service.Data.Ports { + if port.Protocol == pbcatalog.Protocol_PROTOCOL_MESH { + continue // skip + } + allowedPortProtocols[port.TargetPort] = port.Protocol + } + + usedTargetPorts := make(map[string]any) + for port := range destPolicy.Data.PortConfigs { + svcPort := service.Data.FindPortByID(port) + targetPort := svcPort.GetTargetPort() // svcPort could be nil + + serviceRef := resource.NewReferenceKey(service.Id) + if svcPort == nil { + conditions = append(conditions, ConditionUnknownDestinationPort(serviceRef.ToReference(), port)) + } else if _, ok := usedTargetPorts[targetPort]; ok { + conditions = append(conditions, ConditionConflictDestinationPort(serviceRef.ToReference(), svcPort)) + } else { + usedTargetPorts[targetPort] = struct{}{} + } + } + } else { + conditions = append(conditions, ConditionDestinationServiceNotFound(rk.ToReference())) + } + + return conditions +} diff --git a/internal/mesh/internal/controllers/routes/destination_policy_validation_test.go b/internal/mesh/internal/controllers/routes/destination_policy_validation_test.go new file mode 100644 index 0000000000..fdf3073c8e --- /dev/null +++ b/internal/mesh/internal/controllers/routes/destination_policy_validation_test.go @@ -0,0 +1,121 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package routes + +import ( + "testing" + + "github.com/stretchr/testify/require" + + pbmesh "github.com/hashicorp/consul/proto-public/pbmesh/v2beta1" + + "github.com/hashicorp/consul/internal/catalog" + "github.com/hashicorp/consul/internal/mesh/internal/types" + "github.com/hashicorp/consul/internal/resource" + rtest "github.com/hashicorp/consul/internal/resource/resourcetest" + pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v2beta1" + "github.com/hashicorp/consul/proto/private/prototest" +) + +func TestComputeNewDestPolicyPortConditions(t *testing.T) { + registry := resource.NewRegistry() + types.Register(registry) + catalog.RegisterTypes(registry) + + type protocolAndVirtualPort struct { + protocol pbcatalog.Protocol + virtualPort uint32 + } + + newService := func(name string, ports map[string]protocolAndVirtualPort) *types.DecodedService { + var portSlice []*pbcatalog.ServicePort + for targetPort, pv := range ports { + portSlice = append(portSlice, &pbcatalog.ServicePort{ + TargetPort: targetPort, + VirtualPort: pv.virtualPort, + Protocol: pv.protocol, + }) + } + svc := rtest.Resource(pbcatalog.ServiceType, name). + WithData(t, &pbcatalog.Service{Ports: portSlice}). + Build() + rtest.ValidateAndNormalize(t, registry, svc) + + dec, err := resource.Decode[*pbcatalog.Service](svc) + require.NoError(t, err) + return dec + } + + newDestPolicy := func(name string, portConfigs map[string]*pbmesh.DestinationConfig) *types.DecodedDestinationPolicy { + policy := rtest.Resource(pbmesh.DestinationPolicyType, name). + WithData(t, &pbmesh.DestinationPolicy{PortConfigs: portConfigs}). + Build() + rtest.ValidateAndNormalize(t, registry, policy) + + dec, err := resource.Decode[*pbmesh.DestinationPolicy](policy) + require.NoError(t, err) + return dec + } + + t.Run("with no service", func(t *testing.T) { + sg := newTestServiceGetter() + got := computeNewDestPolicyPortConditions(sg, resource.NewReferenceKey( + newRef(pbcatalog.ServiceType, "api", resource.DefaultNamespacedTenancy())), + newDestPolicy("dest", map[string]*pbmesh.DestinationConfig{ + "http": defaultDestConfig(), + })) + require.Len(t, got, 1) + prototest.AssertContainsElement(t, got, ConditionDestinationServiceNotFound( + newRef(pbcatalog.ServiceType, "api", resource.DefaultNamespacedTenancy()), + )) + }) + + t.Run("with service and using missing port", func(t *testing.T) { + sg := newTestServiceGetter(newService("api", map[string]protocolAndVirtualPort{ + "http": {pbcatalog.Protocol_PROTOCOL_HTTP, 8080}, + "mesh": {pbcatalog.Protocol_PROTOCOL_MESH, 20000}, + })) + got := computeNewDestPolicyPortConditions(sg, resource.NewReferenceKey( + newRef(pbcatalog.ServiceType, "api", resource.DefaultNamespacedTenancy())), + newDestPolicy("dest", map[string]*pbmesh.DestinationConfig{ + "grpc": defaultDestConfig(), + })) + require.Len(t, got, 1) + prototest.AssertContainsElement(t, got, ConditionUnknownDestinationPort( + newRef(pbcatalog.ServiceType, "api", resource.DefaultNamespacedTenancy()), + "grpc", + )) + }) + + t.Run("with service and using duplicate port", func(t *testing.T) { + sg := newTestServiceGetter(newService("api", map[string]protocolAndVirtualPort{ + "http": {pbcatalog.Protocol_PROTOCOL_HTTP, 8080}, + "mesh": {pbcatalog.Protocol_PROTOCOL_MESH, 20000}, + })) + got := computeNewDestPolicyPortConditions(sg, resource.NewReferenceKey( + newRef(pbcatalog.ServiceType, "api", resource.DefaultNamespacedTenancy())), + newDestPolicy("dest", map[string]*pbmesh.DestinationConfig{ + "http": defaultDestConfig(), + "8080": defaultDestConfig(), + })) + require.Len(t, got, 1) + prototest.AssertContainsElement(t, got, ConditionConflictDestinationPort( + newRef(pbcatalog.ServiceType, "api", resource.DefaultNamespacedTenancy()), + &pbcatalog.ServicePort{VirtualPort: 8080, TargetPort: "http"}, + )) + }) + + t.Run("with service and using correct port", func(t *testing.T) { + sg := newTestServiceGetter(newService("api", map[string]protocolAndVirtualPort{ + "http": {pbcatalog.Protocol_PROTOCOL_HTTP, 8080}, + "mesh": {pbcatalog.Protocol_PROTOCOL_MESH, 20000}, + })) + got := computeNewDestPolicyPortConditions(sg, resource.NewReferenceKey( + newRef(pbcatalog.ServiceType, "api", resource.DefaultNamespacedTenancy())), + newDestPolicy("dest", map[string]*pbmesh.DestinationConfig{ + "http": defaultDestConfig(), + })) + require.Empty(t, got) + }) +} diff --git a/internal/mesh/internal/controllers/routes/generate.go b/internal/mesh/internal/controllers/routes/generate.go index 9bfb2808e3..c45bd4488c 100644 --- a/internal/mesh/internal/controllers/routes/generate.go +++ b/internal/mesh/internal/controllers/routes/generate.go @@ -134,8 +134,13 @@ func compile( wildcardedPort = true break } - if _, ok := allowedPortProtocols[ref.Port]; ok { - ports = append(ports, ref.Port) + // Check for valid port reference and implicitly convert virtual + // port references to target port. From this point on, all port + // matching should be against a workload target port. The same + // normalization must be done for destination services below. + svcPort := parentServiceDec.Data.FindPortByID(ref.Port) + if _, ok := allowedPortProtocols[svcPort.GetTargetPort()]; ok { + ports = append(ports, svcPort.TargetPort) } } } @@ -330,9 +335,17 @@ func compile( // failover legs into here. for _, details := range mc.Targets { svcRef := details.BackendRef.Ref + + svc := related.GetService(svcRef) + if svc == nil { + panic("impossible at this point; should already have been handled before getting here") + } + // Already added to bound refs above, so skip needless check here + destPolicy := related.GetDestinationPolicyForService(svcRef) if destPolicy != nil { - portDestConfig, ok := destPolicy.Data.PortConfigs[details.BackendRef.Port] + simpleDestPolicy := types.SimplifyDestinationPolicy(svc.Data, destPolicy.Data) + portDestConfig, ok := simpleDestPolicy.PortConfigs[details.BackendRef.Port] if ok { boundRefCollector.AddRefOrID(destPolicy.Resource.Id) details.DestinationConfig = portDestConfig @@ -458,10 +471,12 @@ func compileFailoverConfig( } var backendTargetName string - ok, meshPort := shouldRouteTrafficToBackend(svc, backendRef) + rPorts, ok := shouldRouteTrafficToBackend(svc, backendRef) if !ok { continue // skip this leg of failover for now } + // Map virtual port to target port if used. + backendRef.Port = rPorts.targetPort destTargetName := types.BackendRefToComputedRoutesTarget(backendRef) @@ -470,7 +485,7 @@ func compileFailoverConfig( targets[destTargetName] = &pbmesh.BackendTargetDetails{ Type: pbmesh.BackendTargetDetailsType_BACKEND_TARGET_DETAILS_TYPE_INDIRECT, BackendRef: backendRef, - MeshPort: meshPort, + MeshPort: rPorts.meshPort, } } backendTargetName = destTargetName @@ -555,11 +570,13 @@ func compileHTTPRouteNode( if backendSvc != nil { brc.AddRefOrID(backendSvc.Resource.Id) } - if ok, meshPort := shouldRouteTrafficToBackend(backendSvc, backendRef.BackendRef); ok { + if rPorts, ok := shouldRouteTrafficToBackend(backendSvc, backendRef.BackendRef); ok { + // Map virtual port to target port if used. + backendRef.BackendRef.Port = rPorts.targetPort details := &pbmesh.BackendTargetDetails{ Type: pbmesh.BackendTargetDetailsType_BACKEND_TARGET_DETAILS_TYPE_DIRECT, BackendRef: backendRef.BackendRef, - MeshPort: meshPort, + MeshPort: rPorts.meshPort, } backendTarget = node.AddTarget(backendRef.BackendRef, details) } else { @@ -622,11 +639,13 @@ func compileGRPCRouteNode( if backendSvc != nil { brc.AddRefOrID(backendSvc.Resource.Id) } - if ok, meshPort := shouldRouteTrafficToBackend(backendSvc, backendRef.BackendRef); ok { + if rPorts, ok := shouldRouteTrafficToBackend(backendSvc, backendRef.BackendRef); ok { + // Map virtual port to target port if used. + backendRef.BackendRef.Port = rPorts.targetPort details := &pbmesh.BackendTargetDetails{ Type: pbmesh.BackendTargetDetailsType_BACKEND_TARGET_DETAILS_TYPE_DIRECT, BackendRef: backendRef.BackendRef, - MeshPort: meshPort, + MeshPort: rPorts.meshPort, } backendTarget = node.AddTarget(backendRef.BackendRef, details) } else { @@ -681,11 +700,13 @@ func compileTCPRouteNode( if backendSvc != nil { brc.AddRefOrID(backendSvc.Resource.Id) } - if ok, meshPort := shouldRouteTrafficToBackend(backendSvc, backendRef.BackendRef); ok { + if rPorts, ok := shouldRouteTrafficToBackend(backendSvc, backendRef.BackendRef); ok { + // Map virtual port to target port if used. + backendRef.BackendRef.Port = rPorts.targetPort details := &pbmesh.BackendTargetDetails{ Type: pbmesh.BackendTargetDetailsType_BACKEND_TARGET_DETAILS_TYPE_DIRECT, BackendRef: backendRef.BackendRef, - MeshPort: meshPort, + MeshPort: rPorts.meshPort, } backendTarget = node.AddTarget(backendRef.BackendRef, details) } else { @@ -705,15 +726,20 @@ func compileTCPRouteNode( return node } -func shouldRouteTrafficToBackend(backendSvc *types.DecodedService, backendRef *pbmesh.BackendReference) (bool, string) { +type routableBackendPorts struct { + meshPort, targetPort string +} + +func shouldRouteTrafficToBackend(backendSvc *types.DecodedService, backendRef *pbmesh.BackendReference) (*routableBackendPorts, bool) { if backendSvc == nil { - return false, "" + return nil, false } var ( - found = false - inMesh = false - meshPort string + found = false + inMesh = false + meshPort string + targetPort string ) for _, port := range backendSvc.Data.Ports { if port.Protocol == pbcatalog.Protocol_PROTOCOL_MESH { @@ -721,12 +747,14 @@ func shouldRouteTrafficToBackend(backendSvc *types.DecodedService, backendRef *p meshPort = port.TargetPort continue } - if port.TargetPort == backendRef.Port { + if port.MatchesPortId(backendRef.Port) { found = true + // Map virtual port to target port if used. + targetPort = port.TargetPort } } - return inMesh && found, meshPort + return &routableBackendPorts{meshPort, targetPort}, inMesh && found } func createDefaultRouteNode( diff --git a/internal/mesh/internal/controllers/routes/generate_test.go b/internal/mesh/internal/controllers/routes/generate_test.go index 02ae0e0d43..fc2d913f83 100644 --- a/internal/mesh/internal/controllers/routes/generate_test.go +++ b/internal/mesh/internal/controllers/routes/generate_test.go @@ -1682,5 +1682,274 @@ func TestGenerateComputedRoutes(t *testing.T) { }} run(t, related, expect, nil) }) + + // Same as above dest case, but tests various combinations of virtual and target port values + t.Run("http route with dest policy - virtual ports", func(t *testing.T) { + for _, parentRefPort := range []string{"http", "8080"} { + for _, backendRefPort := range []string{"http", "9090", ""} { + for _, configKeyPort := range []string{"http", "9090"} { + t.Run(fmt.Sprintf("%v, %v, %v", parentRefPort, backendRefPort, configKeyPort), func(t *testing.T) { + apiServiceData := &pbcatalog.Service{ + Workloads: &pbcatalog.WorkloadSelector{ + Prefixes: []string{"api-"}, + }, + Ports: []*pbcatalog.ServicePort{ + {TargetPort: "mesh", VirtualPort: 20000, Protocol: pbcatalog.Protocol_PROTOCOL_MESH}, + {TargetPort: "http", VirtualPort: 8080, Protocol: pbcatalog.Protocol_PROTOCOL_HTTP}, + }, + } + + fooServiceData := &pbcatalog.Service{ + Workloads: &pbcatalog.WorkloadSelector{ + Prefixes: []string{"foo-"}, + }, + Ports: []*pbcatalog.ServicePort{ + {TargetPort: "mesh", VirtualPort: 20000, Protocol: pbcatalog.Protocol_PROTOCOL_MESH}, + {TargetPort: "http", VirtualPort: 9090, Protocol: pbcatalog.Protocol_PROTOCOL_HTTP}, + }, + } + + httpRoute1 := &pbmesh.HTTPRoute{ + ParentRefs: []*pbmesh.ParentReference{ + newParentRef(newRef(pbcatalog.ServiceType, "api", tenancy), parentRefPort), + }, + Rules: []*pbmesh.HTTPRouteRule{{ + Matches: []*pbmesh.HTTPRouteMatch{{ + Path: &pbmesh.HTTPPathMatch{ + Type: pbmesh.PathMatchType_PATH_MATCH_TYPE_PREFIX, + Value: "/", + }, + }}, + BackendRefs: []*pbmesh.HTTPBackendRef{{ + BackendRef: newBackendRef(fooServiceRef, backendRefPort, ""), + }}, + }}, + } + + destPolicy := &pbmesh.DestinationPolicy{ + PortConfigs: map[string]*pbmesh.DestinationConfig{ + configKeyPort: { + ConnectTimeout: durationpb.New(55 * time.Second), + }, + }, + } + expectedPortDestConfig := &pbmesh.DestinationConfig{ + ConnectTimeout: durationpb.New(55 * time.Second), + } + + related := loader.NewRelatedResources(). + AddComputedRoutesIDs(apiComputedRoutesID). + AddResources( + newService("api", apiServiceData), + newService("foo", fooServiceData), + newHTTPRoute("api-http-route1", httpRoute1), + newDestPolicy("foo", destPolicy), + ) + + // Same result as non-virtual-port variant of test + expect := []*ComputedRoutesResult{{ + ID: apiComputedRoutesID, + OwnerID: apiServiceID, + Data: &pbmesh.ComputedRoutes{ + BoundReferences: []*pbresource.Reference{ + apiServiceRef, + fooServiceRef, + newRef(pbmesh.DestinationPolicyType, "foo", tenancy), + newRef(pbmesh.HTTPRouteType, "api-http-route1", tenancy), + }, + PortedConfigs: map[string]*pbmesh.ComputedPortRoutes{ + "http": { + Config: &pbmesh.ComputedPortRoutes_Http{ + Http: &pbmesh.ComputedHTTPRoute{ + Rules: []*pbmesh.ComputedHTTPRouteRule{ + { + Matches: defaultHTTPRouteMatches(), + BackendRefs: []*pbmesh.ComputedHTTPBackendRef{{ + BackendTarget: backendName("foo", "http"), + }}, + }, + { + Matches: defaultHTTPRouteMatches(), + BackendRefs: []*pbmesh.ComputedHTTPBackendRef{{ + BackendTarget: types.NullRouteBackend, + }}, + }, + }, + }, + }, + ParentRef: newParentRef(apiServiceRef, "http"), + Protocol: pbcatalog.Protocol_PROTOCOL_HTTP, + Targets: map[string]*pbmesh.BackendTargetDetails{ + backendName("foo", "http"): { + Type: pbmesh.BackendTargetDetailsType_BACKEND_TARGET_DETAILS_TYPE_DIRECT, + MeshPort: "mesh", + BackendRef: newBackendRef(fooServiceRef, "http", ""), + DestinationConfig: expectedPortDestConfig, + }, + }, + }, + }, + }, + }} + run(t, related, expect, nil) + }) + } + } + } + }) + + // Same as above failover case, but tests various combinations of virtual and target port values + t.Run("http route with failover policy - virtual ports", func(t *testing.T) { + for _, parentRefPort := range []string{"http", "8080"} { + for _, backendRefPortFoo := range []string{"http", "9090", ""} { + for _, backendRefPortBar := range []string{"http", "9091", ""} { + for _, configKeyPortFoo := range []string{"http", "9090", ""} { + t.Run(fmt.Sprintf("%v, %v, %v, %v", parentRefPort, backendRefPortFoo, backendRefPortBar, configKeyPortFoo), func(t *testing.T) { + apiServiceData := &pbcatalog.Service{ + Workloads: &pbcatalog.WorkloadSelector{ + Prefixes: []string{"api-"}, + }, + Ports: []*pbcatalog.ServicePort{ + {TargetPort: "mesh", VirtualPort: 20000, Protocol: pbcatalog.Protocol_PROTOCOL_MESH}, + {TargetPort: "http", VirtualPort: 8080, Protocol: pbcatalog.Protocol_PROTOCOL_HTTP}, + }, + } + + fooServiceData := &pbcatalog.Service{ + Workloads: &pbcatalog.WorkloadSelector{ + Prefixes: []string{"foo-"}, + }, + Ports: []*pbcatalog.ServicePort{ + {TargetPort: "mesh", VirtualPort: 20000, Protocol: pbcatalog.Protocol_PROTOCOL_MESH}, + {TargetPort: "http", VirtualPort: 9090, Protocol: pbcatalog.Protocol_PROTOCOL_HTTP}, + }, + } + + barServiceData := &pbcatalog.Service{ + Workloads: &pbcatalog.WorkloadSelector{ + Prefixes: []string{"bar-"}, + }, + Ports: []*pbcatalog.ServicePort{ + {TargetPort: "mesh", VirtualPort: 20000, Protocol: pbcatalog.Protocol_PROTOCOL_MESH}, + {TargetPort: "http", VirtualPort: 9091, Protocol: pbcatalog.Protocol_PROTOCOL_HTTP}, + }, + } + + httpRoute1 := &pbmesh.HTTPRoute{ + ParentRefs: []*pbmesh.ParentReference{ + newParentRef(newRef(pbcatalog.ServiceType, "api", tenancy), parentRefPort), + }, + Rules: []*pbmesh.HTTPRouteRule{{ + Matches: []*pbmesh.HTTPRouteMatch{{ + Path: &pbmesh.HTTPPathMatch{ + Type: pbmesh.PathMatchType_PATH_MATCH_TYPE_PREFIX, + Value: "/", + }, + }}, + BackendRefs: []*pbmesh.HTTPBackendRef{{ + BackendRef: newBackendRef(fooServiceRef, backendRefPortFoo, ""), + }}, + }}, + } + + failoverPolicy := &pbcatalog.FailoverPolicy{ + Config: &pbcatalog.FailoverConfig{ + Destinations: []*pbcatalog.FailoverDestination{ + {Ref: barServiceRef}, // port is not supported in non-ported config + {Ref: deadServiceRef}, // no service + }, + }, + } + // Test ported config if used in test case + if configKeyPortFoo != "" { + failoverPolicy = &pbcatalog.FailoverPolicy{ + PortConfigs: map[string]*pbcatalog.FailoverConfig{ + configKeyPortFoo: { + Destinations: []*pbcatalog.FailoverDestination{ + {Ref: barServiceRef, Port: backendRefPortBar}, + {Ref: deadServiceRef}, // no service + }, + }, + }, + } + } + expectedPortFailoverConfig := &pbmesh.ComputedFailoverConfig{ + Destinations: []*pbmesh.ComputedFailoverDestination{ + {BackendTarget: backendName("bar", "http")}, + // we skip the dead route + }, + } + + related := loader.NewRelatedResources(). + AddComputedRoutesIDs(apiComputedRoutesID). + AddResources( + newService("api", apiServiceData), + newService("foo", fooServiceData), + newService("bar", barServiceData), + newHTTPRoute("api-http-route1", httpRoute1), + newFailPolicy("foo", failoverPolicy), + ) + + // Same result as non-virtual-port variant of test + expect := []*ComputedRoutesResult{{ + ID: apiComputedRoutesID, + OwnerID: apiServiceID, + Data: &pbmesh.ComputedRoutes{ + BoundReferences: []*pbresource.Reference{ + newRef(pbcatalog.FailoverPolicyType, "foo", tenancy), + apiServiceRef, + barServiceRef, + fooServiceRef, + newRef(pbmesh.HTTPRouteType, "api-http-route1", tenancy), + }, + PortedConfigs: map[string]*pbmesh.ComputedPortRoutes{ + "http": { + Config: &pbmesh.ComputedPortRoutes_Http{ + Http: &pbmesh.ComputedHTTPRoute{ + Rules: []*pbmesh.ComputedHTTPRouteRule{ + { + Matches: defaultHTTPRouteMatches(), + BackendRefs: []*pbmesh.ComputedHTTPBackendRef{{ + BackendTarget: backendName("foo", "http"), + }}, + }, + { + Matches: defaultHTTPRouteMatches(), + BackendRefs: []*pbmesh.ComputedHTTPBackendRef{{ + BackendTarget: types.NullRouteBackend, + }}, + }, + }, + }, + }, + ParentRef: newParentRef(apiServiceRef, "http"), + Protocol: pbcatalog.Protocol_PROTOCOL_HTTP, + Targets: map[string]*pbmesh.BackendTargetDetails{ + backendName("foo", "http"): { + Type: pbmesh.BackendTargetDetailsType_BACKEND_TARGET_DETAILS_TYPE_DIRECT, + MeshPort: "mesh", + BackendRef: newBackendRef(fooServiceRef, "http", ""), + FailoverConfig: expectedPortFailoverConfig, + DestinationConfig: defaultDestConfig(), + }, + // Indirect target with unspecified port gets parent ref port + backendName("bar", "http"): { + Type: pbmesh.BackendTargetDetailsType_BACKEND_TARGET_DETAILS_TYPE_INDIRECT, + MeshPort: "mesh", + BackendRef: newBackendRef(barServiceRef, "http", ""), + DestinationConfig: defaultDestConfig(), + }, + }, + }, + }, + }, + }} + run(t, related, expect, nil) + }) + } + } + } + } + }) } } diff --git a/internal/mesh/internal/controllers/routes/ref_validation.go b/internal/mesh/internal/controllers/routes/ref_validation.go index b860eede20..a81317b860 100644 --- a/internal/mesh/internal/controllers/routes/ref_validation.go +++ b/internal/mesh/internal/controllers/routes/ref_validation.go @@ -4,12 +4,13 @@ package routes import ( - pbmesh "github.com/hashicorp/consul/proto-public/pbmesh/v2beta1" + "fmt" "github.com/hashicorp/consul/internal/mesh/internal/controllers/routes/loader" "github.com/hashicorp/consul/internal/mesh/internal/types" "github.com/hashicorp/consul/internal/resource" 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" ) @@ -43,6 +44,7 @@ func computeNewRouteRefConditions( // TODO(rb): handle port numbers here too if we are allowing those instead of the name? + usedParentTargetPorts := make(map[string]any) for _, parentRef := range parentRefs { if parentRef.Ref == nil || !resource.EqualType(parentRef.Ref.Type, pbcatalog.ServiceType) { continue // not possible due to xRoute validation @@ -58,11 +60,17 @@ func computeNewRouteRefConditions( if port.Protocol == pbcatalog.Protocol_PROTOCOL_MESH { hasMesh = true } - if port.TargetPort == parentRef.Port { + if port.MatchesPortId(parentRef.Port) { found = true + portRef := fmt.Sprintf("%s:%s", resource.ReferenceToString(parentRef.Ref), port.TargetPort) if port.Protocol == pbcatalog.Protocol_PROTOCOL_MESH { usingMesh = true } + if _, ok := usedParentTargetPorts[portRef]; ok { + conditions = append(conditions, ConditionConflictParentRefPort(parentRef.Ref, port.TargetPort)) + } else { + usedParentTargetPorts[portRef] = struct{}{} + } } } switch { @@ -80,6 +88,7 @@ func computeNewRouteRefConditions( } } + usedBackendTargetPorts := make(map[string]any) for _, backendRef := range backendRefs { if backendRef.Ref == nil || !resource.EqualType(backendRef.Ref.Type, pbcatalog.ServiceType) { continue // not possible due to xRoute validation @@ -95,11 +104,17 @@ func computeNewRouteRefConditions( if port.Protocol == pbcatalog.Protocol_PROTOCOL_MESH { hasMesh = true } - if port.TargetPort == backendRef.Port { + if port.MatchesPortId(backendRef.Port) { found = true + portRef := fmt.Sprintf("%s:%s", resource.ReferenceToString(backendRef.Ref), port.TargetPort) if port.Protocol == pbcatalog.Protocol_PROTOCOL_MESH { usingMesh = true } + if _, ok := usedBackendTargetPorts[portRef]; ok { + conditions = append(conditions, ConditionConflictBackendRefPort(backendRef.Ref, port.TargetPort)) + } else { + usedBackendTargetPorts[portRef] = struct{}{} + } } } switch { diff --git a/internal/mesh/internal/controllers/routes/ref_validation_test.go b/internal/mesh/internal/controllers/routes/ref_validation_test.go index 71933a2a5d..f383d4f05e 100644 --- a/internal/mesh/internal/controllers/routes/ref_validation_test.go +++ b/internal/mesh/internal/controllers/routes/ref_validation_test.go @@ -24,12 +24,18 @@ func TestComputeNewRouteRefConditions(t *testing.T) { types.Register(registry) catalog.RegisterTypes(registry) - newService := func(name string, ports map[string]pbcatalog.Protocol) *types.DecodedService { + type protocolAndVirtualPort struct { + protocol pbcatalog.Protocol + virtualPort uint32 + } + + newService := func(name string, ports map[string]protocolAndVirtualPort) *types.DecodedService { var portSlice []*pbcatalog.ServicePort - for name, proto := range ports { + for targetPort, pv := range ports { portSlice = append(portSlice, &pbcatalog.ServicePort{ - TargetPort: name, - Protocol: proto, + TargetPort: targetPort, + VirtualPort: pv.virtualPort, + Protocol: pv.protocol, }) } svc := rtest.Resource(pbcatalog.ServiceType, name). @@ -61,8 +67,8 @@ func TestComputeNewRouteRefConditions(t *testing.T) { }) t.Run("with service but no mesh port", func(t *testing.T) { - sg := newTestServiceGetter(newService("api", map[string]pbcatalog.Protocol{ - "http": pbcatalog.Protocol_PROTOCOL_HTTP, + sg := newTestServiceGetter(newService("api", map[string]protocolAndVirtualPort{ + "http": {pbcatalog.Protocol_PROTOCOL_HTTP, 8080}, })) got := computeNewRouteRefConditions(sg, []*pbmesh.ParentReference{ newParentRef(newRef(pbcatalog.ServiceType, "api", resource.DefaultNamespacedTenancy()), ""), @@ -74,9 +80,9 @@ func TestComputeNewRouteRefConditions(t *testing.T) { }) t.Run("with service but using mesh port", func(t *testing.T) { - sg := newTestServiceGetter(newService("api", map[string]pbcatalog.Protocol{ - "http": pbcatalog.Protocol_PROTOCOL_HTTP, - "mesh": pbcatalog.Protocol_PROTOCOL_MESH, + sg := newTestServiceGetter(newService("api", map[string]protocolAndVirtualPort{ + "http": {pbcatalog.Protocol_PROTOCOL_HTTP, 8080}, + "mesh": {pbcatalog.Protocol_PROTOCOL_MESH, 20000}, })) got := computeNewRouteRefConditions(sg, []*pbmesh.ParentReference{ newParentRef(newRef(pbcatalog.ServiceType, "api", resource.DefaultNamespacedTenancy()), "mesh"), @@ -89,9 +95,9 @@ func TestComputeNewRouteRefConditions(t *testing.T) { }) t.Run("with service and using missing port", func(t *testing.T) { - sg := newTestServiceGetter(newService("api", map[string]pbcatalog.Protocol{ - "http": pbcatalog.Protocol_PROTOCOL_HTTP, - "mesh": pbcatalog.Protocol_PROTOCOL_MESH, + sg := newTestServiceGetter(newService("api", map[string]protocolAndVirtualPort{ + "http": {pbcatalog.Protocol_PROTOCOL_HTTP, 8080}, + "mesh": {pbcatalog.Protocol_PROTOCOL_MESH, 20000}, })) got := computeNewRouteRefConditions(sg, []*pbmesh.ParentReference{ newParentRef(newRef(pbcatalog.ServiceType, "api", resource.DefaultNamespacedTenancy()), "web"), @@ -103,10 +109,26 @@ func TestComputeNewRouteRefConditions(t *testing.T) { )) }) + t.Run("with service and using duplicate port", func(t *testing.T) { + sg := newTestServiceGetter(newService("api", map[string]protocolAndVirtualPort{ + "http": {pbcatalog.Protocol_PROTOCOL_HTTP, 8080}, + "mesh": {pbcatalog.Protocol_PROTOCOL_MESH, 20000}, + })) + got := computeNewRouteRefConditions(sg, []*pbmesh.ParentReference{ + newParentRef(newRef(pbcatalog.ServiceType, "api", resource.DefaultNamespacedTenancy()), "http"), + newParentRef(newRef(pbcatalog.ServiceType, "api", resource.DefaultNamespacedTenancy()), "8080"), + }, nil) + require.Len(t, got, 1) + prototest.AssertContainsElement(t, got, ConditionConflictParentRefPort( + newRef(pbcatalog.ServiceType, "api", resource.DefaultNamespacedTenancy()), + "http", + )) + }) + t.Run("with service and using empty port", func(t *testing.T) { - sg := newTestServiceGetter(newService("api", map[string]pbcatalog.Protocol{ - "http": pbcatalog.Protocol_PROTOCOL_HTTP, - "mesh": pbcatalog.Protocol_PROTOCOL_MESH, + sg := newTestServiceGetter(newService("api", map[string]protocolAndVirtualPort{ + "http": {pbcatalog.Protocol_PROTOCOL_HTTP, 8080}, + "mesh": {pbcatalog.Protocol_PROTOCOL_MESH, 20000}, })) got := computeNewRouteRefConditions(sg, []*pbmesh.ParentReference{ newParentRef(newRef(pbcatalog.ServiceType, "api", resource.DefaultNamespacedTenancy()), ""), @@ -115,9 +137,9 @@ func TestComputeNewRouteRefConditions(t *testing.T) { }) t.Run("with service and using correct port", func(t *testing.T) { - sg := newTestServiceGetter(newService("api", map[string]pbcatalog.Protocol{ - "http": pbcatalog.Protocol_PROTOCOL_HTTP, - "mesh": pbcatalog.Protocol_PROTOCOL_MESH, + sg := newTestServiceGetter(newService("api", map[string]protocolAndVirtualPort{ + "http": {pbcatalog.Protocol_PROTOCOL_HTTP, 8080}, + "mesh": {pbcatalog.Protocol_PROTOCOL_MESH, 20000}, })) got := computeNewRouteRefConditions(sg, []*pbmesh.ParentReference{ newParentRef(newRef(pbcatalog.ServiceType, "api", resource.DefaultNamespacedTenancy()), "http"), @@ -139,8 +161,8 @@ func TestComputeNewRouteRefConditions(t *testing.T) { }) t.Run("with service but no mesh port", func(t *testing.T) { - sg := newTestServiceGetter(newService("api", map[string]pbcatalog.Protocol{ - "http": pbcatalog.Protocol_PROTOCOL_HTTP, + sg := newTestServiceGetter(newService("api", map[string]protocolAndVirtualPort{ + "http": {pbcatalog.Protocol_PROTOCOL_HTTP, 8080}, })) got := computeNewRouteRefConditions(sg, nil, []*pbmesh.BackendReference{ newBackendRef(newRef(pbcatalog.ServiceType, "api", resource.DefaultNamespacedTenancy()), "", ""), @@ -152,9 +174,9 @@ func TestComputeNewRouteRefConditions(t *testing.T) { }) t.Run("with service but using mesh port", func(t *testing.T) { - sg := newTestServiceGetter(newService("api", map[string]pbcatalog.Protocol{ - "http": pbcatalog.Protocol_PROTOCOL_HTTP, - "mesh": pbcatalog.Protocol_PROTOCOL_MESH, + sg := newTestServiceGetter(newService("api", map[string]protocolAndVirtualPort{ + "http": {pbcatalog.Protocol_PROTOCOL_HTTP, 8080}, + "mesh": {pbcatalog.Protocol_PROTOCOL_MESH, 20000}, })) got := computeNewRouteRefConditions(sg, nil, []*pbmesh.BackendReference{ newBackendRef(newRef(pbcatalog.ServiceType, "api", resource.DefaultNamespacedTenancy()), "mesh", ""), @@ -167,9 +189,9 @@ func TestComputeNewRouteRefConditions(t *testing.T) { }) t.Run("with service and using missing port", func(t *testing.T) { - sg := newTestServiceGetter(newService("api", map[string]pbcatalog.Protocol{ - "http": pbcatalog.Protocol_PROTOCOL_HTTP, - "mesh": pbcatalog.Protocol_PROTOCOL_MESH, + sg := newTestServiceGetter(newService("api", map[string]protocolAndVirtualPort{ + "http": {pbcatalog.Protocol_PROTOCOL_HTTP, 8080}, + "mesh": {pbcatalog.Protocol_PROTOCOL_MESH, 20000}, })) got := computeNewRouteRefConditions(sg, nil, []*pbmesh.BackendReference{ newBackendRef(newRef(pbcatalog.ServiceType, "api", resource.DefaultNamespacedTenancy()), "web", ""), @@ -181,10 +203,26 @@ func TestComputeNewRouteRefConditions(t *testing.T) { )) }) + t.Run("with service and using duplicate port", func(t *testing.T) { + sg := newTestServiceGetter(newService("api", map[string]protocolAndVirtualPort{ + "http": {pbcatalog.Protocol_PROTOCOL_HTTP, 8080}, + "mesh": {pbcatalog.Protocol_PROTOCOL_MESH, 20000}, + })) + got := computeNewRouteRefConditions(sg, nil, []*pbmesh.BackendReference{ + newBackendRef(newRef(pbcatalog.ServiceType, "api", resource.DefaultNamespacedTenancy()), "http", ""), + newBackendRef(newRef(pbcatalog.ServiceType, "api", resource.DefaultNamespacedTenancy()), "8080", ""), + }) + require.Len(t, got, 1) + prototest.AssertContainsElement(t, got, ConditionConflictBackendRefPort( + newRef(pbcatalog.ServiceType, "api", resource.DefaultNamespacedTenancy()), + "http", + )) + }) + t.Run("with service and using empty port", func(t *testing.T) { - sg := newTestServiceGetter(newService("api", map[string]pbcatalog.Protocol{ - "http": pbcatalog.Protocol_PROTOCOL_HTTP, - "mesh": pbcatalog.Protocol_PROTOCOL_MESH, + sg := newTestServiceGetter(newService("api", map[string]protocolAndVirtualPort{ + "http": {pbcatalog.Protocol_PROTOCOL_HTTP, 8080}, + "mesh": {pbcatalog.Protocol_PROTOCOL_MESH, 20000}, })) got := computeNewRouteRefConditions(sg, nil, []*pbmesh.BackendReference{ newBackendRef(newRef(pbcatalog.ServiceType, "api", resource.DefaultNamespacedTenancy()), "", ""), @@ -193,9 +231,9 @@ func TestComputeNewRouteRefConditions(t *testing.T) { }) t.Run("with service and using correct port", func(t *testing.T) { - sg := newTestServiceGetter(newService("api", map[string]pbcatalog.Protocol{ - "http": pbcatalog.Protocol_PROTOCOL_HTTP, - "mesh": pbcatalog.Protocol_PROTOCOL_MESH, + sg := newTestServiceGetter(newService("api", map[string]protocolAndVirtualPort{ + "http": {pbcatalog.Protocol_PROTOCOL_HTTP, 8080}, + "mesh": {pbcatalog.Protocol_PROTOCOL_MESH, 20000}, })) got := computeNewRouteRefConditions(sg, nil, []*pbmesh.BackendReference{ newBackendRef(newRef(pbcatalog.ServiceType, "api", resource.DefaultNamespacedTenancy()), "http", ""), diff --git a/internal/mesh/internal/controllers/routes/status.go b/internal/mesh/internal/controllers/routes/status.go index 71b6b8cd5b..b1e838ed23 100644 --- a/internal/mesh/internal/controllers/routes/status.go +++ b/internal/mesh/internal/controllers/routes/status.go @@ -7,6 +7,7 @@ import ( "fmt" "github.com/hashicorp/consul/internal/resource" + catalog "github.com/hashicorp/consul/proto-public/pbcatalog/v2beta1" "github.com/hashicorp/consul/proto-public/pbresource" ) @@ -28,10 +29,16 @@ const ( ParentRefUsingMeshPortReason = "ParentRefUsingMeshPort" BackendRefUsingMeshPortReason = "BackendRefUsingMeshPort" - UnknownParentRefPortReason = "UnknownParentRefPort" - UnknownBackendRefPortReason = "UnknownBackendRefPort" + UnknownParentRefPortReason = "UnknownParentRefPort" + UnknownBackendRefPortReason = "UnknownBackendRefPort" + UnknownDestinationPortReason = "UnknownDestinationPort" + ConflictParentRefPortReason = "ConflictParentRefPort" + ConflictBackendRefPortReason = "ConflictBackendRefPort" + ConflictDestinationPortReason = "ConflictDestinationPort" ConflictNotBoundToParentRefReason = "ConflictNotBoundToParentRef" + + DestinationServiceNotFoundReason = "DestinationServiceNotFound" ) var ( @@ -153,16 +160,79 @@ func conditionUnknownRefPort(ref *pbresource.Reference, port string, forBackend } } +func ConditionConflictParentRefPort(ref *pbresource.Reference, port string) *pbresource.Condition { + return conditionConflictRefPort(ref, port, false) +} + +func ConditionConflictBackendRefPort(ref *pbresource.Reference, port string) *pbresource.Condition { + return conditionConflictRefPort(ref, port, true) +} + +func conditionConflictRefPort(ref *pbresource.Reference, port string, forBackend bool) *pbresource.Condition { + reason := ConflictParentRefPortReason + short := "parent" + if forBackend { + reason = ConflictBackendRefPortReason + short = "backend" + } + return &pbresource.Condition{ + Type: StatusConditionAccepted, + State: pbresource.Condition_STATE_FALSE, + Reason: reason, + Message: fmt.Sprintf( + "multiple %s refs found for service %q on target port %q", + short, + resource.ReferenceToString(ref), + port, + ), + } +} + func ConditionConflictNotBoundToParentRef(ref *pbresource.Reference, port string, realType *pbresource.Type) *pbresource.Condition { return &pbresource.Condition{ Type: StatusConditionAccepted, State: pbresource.Condition_STATE_FALSE, Reason: ConflictNotBoundToParentRefReason, Message: fmt.Sprintf( - "Existing routes of type %q are bound to parent ref %q on port %q preventing this from binding", + "existing routes of type %q are bound to parent ref %q on port %q preventing this from binding", resource.TypeToString(realType), resource.ReferenceToString(ref), port, ), } } + +func ConditionDestinationServiceNotFound(serviceRef *pbresource.Reference) *pbresource.Condition { + return &pbresource.Condition{ + Type: StatusConditionAccepted, + State: pbresource.Condition_STATE_FALSE, + Reason: DestinationServiceNotFoundReason, + Message: fmt.Sprintf("service %q does not exist.", resource.ReferenceToString(serviceRef)), + } +} + +func ConditionUnknownDestinationPort(serviceRef *pbresource.Reference, port string) *pbresource.Condition { + return &pbresource.Condition{ + Type: StatusConditionAccepted, + State: pbresource.Condition_STATE_FALSE, + Reason: UnknownDestinationPortReason, + Message: fmt.Sprintf( + "port is not defined on service: %s on %s", + port, + resource.ReferenceToString(serviceRef), + ), + } +} + +func ConditionConflictDestinationPort(serviceRef *pbresource.Reference, port *catalog.ServicePort) *pbresource.Condition { + return &pbresource.Condition{ + Type: StatusConditionAccepted, + State: pbresource.Condition_STATE_FALSE, + Reason: ConflictDestinationPortReason, + Message: fmt.Sprintf( + "multiple configs found for port on destination service: %s on %s", + port.ToPrintableString(), + resource.ReferenceToString(serviceRef), + ), + } +} diff --git a/internal/mesh/internal/controllers/sidecarproxy/builder/destinations.go b/internal/mesh/internal/controllers/sidecarproxy/builder/destinations.go index a3c7dd460d..78d7b2cc01 100644 --- a/internal/mesh/internal/controllers/sidecarproxy/builder/destinations.go +++ b/internal/mesh/internal/controllers/sidecarproxy/builder/destinations.go @@ -67,7 +67,7 @@ func (b *Builder) buildDestination( var virtualPortNumber uint32 if destination.Explicit == nil { for _, port := range destination.Service.Data.Ports { - if port.TargetPort == cpr.ParentRef.Port { + if port.MatchesPortId(cpr.ParentRef.Port) { virtualPortNumber = port.VirtualPort } } diff --git a/internal/mesh/internal/controllers/sidecarproxy/controller.go b/internal/mesh/internal/controllers/sidecarproxy/controller.go index 596a4918d1..644ceaaf20 100644 --- a/internal/mesh/internal/controllers/sidecarproxy/controller.go +++ b/internal/mesh/internal/controllers/sidecarproxy/controller.go @@ -299,7 +299,7 @@ func (r *reconciler) workloadPortProtocolsFromService( inheritedProtocol := pbcatalog.Protocol_PROTOCOL_UNSPECIFIED for _, svc := range services { // Find workload's port as the target port. - svcPort := svc.GetData().FindServicePort(portName) + svcPort := svc.GetData().FindTargetPort(portName) // If this service doesn't select this port, go to the next service. if svcPort == nil { diff --git a/internal/mesh/internal/controllers/sidecarproxy/fetcher/data_fetcher.go b/internal/mesh/internal/controllers/sidecarproxy/fetcher/data_fetcher.go index 515d46ab43..9d9ec2756e 100644 --- a/internal/mesh/internal/controllers/sidecarproxy/fetcher/data_fetcher.go +++ b/internal/mesh/internal/controllers/sidecarproxy/fetcher/data_fetcher.go @@ -151,13 +151,13 @@ func (f *Fetcher) FetchComputedExplicitDestinationsData( } // Check if the desired port exists on the service and skip it doesn't. - if svc.GetData().FindServicePort(dest.DestinationPort) == nil { + if svc.GetData().FindPortByID(dest.DestinationPort) == nil { continue } // No destination port should point to a port with "mesh" protocol, // so check if destination port has the mesh protocol and skip it if it does. - if svc.GetData().FindServicePort(dest.DestinationPort).GetProtocol() == pbcatalog.Protocol_PROTOCOL_MESH { + if svc.GetData().FindPortByID(dest.DestinationPort).GetProtocol() == pbcatalog.Protocol_PROTOCOL_MESH { continue } diff --git a/internal/mesh/internal/types/destination_policy.go b/internal/mesh/internal/types/destination_policy.go index 4fe3062367..40f3c4aef1 100644 --- a/internal/mesh/internal/types/destination_policy.go +++ b/internal/mesh/internal/types/destination_policy.go @@ -8,9 +8,11 @@ import ( "fmt" "github.com/hashicorp/go-multierror" + "google.golang.org/protobuf/proto" "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/internal/resource" + 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" ) @@ -214,6 +216,48 @@ func validateDestinationPolicy(res *DecodedDestinationPolicy) error { return merr } +// SimplifyDestinationPolicy normalizes port references in the DestinationPolicy +// using the provided Service. +func SimplifyDestinationPolicy(svc *pbcatalog.Service, policy *pbmesh.DestinationPolicy) *pbmesh.DestinationPolicy { + if policy == nil { + panic("destination policy is required") + } + if svc == nil { + panic("service is required") + } + + // Copy so we can edit it. + dup := proto.Clone(policy) + policy = dup.(*pbmesh.DestinationPolicy) + + if policy.PortConfigs == nil { + policy.PortConfigs = make(map[string]*pbmesh.DestinationConfig) + } + + // Normalize all port configs to use the target port of the corresponding service port. + normalizedPortConfigs := make(map[string]*pbmesh.DestinationConfig) + for port, pc := range policy.PortConfigs { + svcPort := svc.FindPortByID(port) + + if svcPort != nil { + if _, ok := normalizedPortConfigs[svcPort.TargetPort]; ok { + // This is a duplicate virtual and target port mapping that will be reported as a status condition. + // Only update if this is the "canonical" mapping; otherwise, it's virtual, and we should ignore. + if port != svcPort.TargetPort { + continue + } + } + normalizedPortConfigs[svcPort.TargetPort] = pc + } + // Else this is an invalid reference that will be reported as a status condition. + // Drop for safety and simpler output. + } + + policy.PortConfigs = normalizedPortConfigs + + return policy +} + func aclReadHookDestinationPolicy(authorizer acl.Authorizer, authzContext *acl.AuthorizerContext, id *pbresource.ID, _ *pbresource.Resource) error { // DestinationPolicy is name-aligned with Service serviceName := id.Name diff --git a/internal/mesh/internal/types/destinations.go b/internal/mesh/internal/types/destinations.go index a128631195..301fdbf6d7 100644 --- a/internal/mesh/internal/types/destinations.go +++ b/internal/mesh/internal/types/destinations.go @@ -99,7 +99,7 @@ func validateDestinations(res *DecodedDestinations) error { merr = multierror.Append(merr, refErr) } - if portErr := catalog.ValidatePortName(dest.DestinationPort); portErr != nil { + if portErr := catalog.ValidateServicePortID(dest.DestinationPort); portErr != nil { merr = multierror.Append(merr, wrapDestErr(resource.ErrInvalidField{ Name: "destination_port", Wrapped: portErr, diff --git a/proto-public/pbcatalog/v2beta1/failover_policy.pb.go b/proto-public/pbcatalog/v2beta1/failover_policy.pb.go index b97535df0f..a30e6b8f0d 100644 --- a/proto-public/pbcatalog/v2beta1/failover_policy.pb.go +++ b/proto-public/pbcatalog/v2beta1/failover_policy.pb.go @@ -83,8 +83,11 @@ type FailoverPolicy struct { // Config defines failover for any named port not present in PortConfigs. Config *FailoverConfig `protobuf:"bytes,1,opt,name=config,proto3" json:"config,omitempty"` - // PortConfigs defines failover for a specific port on this service and takes + // PortConfigs defines failover for a specific port on a service and takes // precedence over Config. + // + // For more details on potential values of the service port identifier key, + // see documentation for Service.ServicePort. PortConfigs map[string]*FailoverConfig `protobuf:"bytes,2,rep,name=port_configs,json=portConfigs,proto3" json:"port_configs,omitempty" protobuf_key:"bytes,1,opt,name=key,proto3" protobuf_val:"bytes,2,opt,name=value,proto3"` } @@ -218,6 +221,10 @@ type FailoverDestination struct { // This must be a Service. Ref *pbresource.Reference `protobuf:"bytes,1,opt,name=ref,proto3" json:"ref,omitempty"` + // Port is the port of the destination service. + // + // For more details on potential values of the service port identifier key, + // see documentation for Service.ServicePort. // TODO: what should an empty port mean? Port string `protobuf:"bytes,2,opt,name=port,proto3" json:"port,omitempty"` Datacenter string `protobuf:"bytes,3,opt,name=datacenter,proto3" json:"datacenter,omitempty"` diff --git a/proto-public/pbcatalog/v2beta1/failover_policy.proto b/proto-public/pbcatalog/v2beta1/failover_policy.proto index abbeb46a3a..8e150990b5 100644 --- a/proto-public/pbcatalog/v2beta1/failover_policy.proto +++ b/proto-public/pbcatalog/v2beta1/failover_policy.proto @@ -15,8 +15,11 @@ message FailoverPolicy { // Config defines failover for any named port not present in PortConfigs. FailoverConfig config = 1; - // PortConfigs defines failover for a specific port on this service and takes + // PortConfigs defines failover for a specific port on a service and takes // precedence over Config. + // + // For more details on potential values of the service port identifier key, + // see documentation for Service.ServicePort. map port_configs = 2; } @@ -38,6 +41,11 @@ message FailoverConfig { message FailoverDestination { // This must be a Service. hashicorp.consul.resource.Reference ref = 1; + + // Port is the port of the destination service. + // + // For more details on potential values of the service port identifier key, + // see documentation for Service.ServicePort. // TODO: what should an empty port mean? string port = 2; string datacenter = 3; diff --git a/proto-public/pbcatalog/v2beta1/service.pb.go b/proto-public/pbcatalog/v2beta1/service.pb.go index e019f7b60a..9092474dc7 100644 --- a/proto-public/pbcatalog/v2beta1/service.pb.go +++ b/proto-public/pbcatalog/v2beta1/service.pb.go @@ -92,6 +92,13 @@ func (x *Service) GetVirtualIps() []string { return nil } +// ServicePort declares a port exposed by the service that can be used in config and xRoute +// references. +// +// For outside references to a service port by string identifier (e.g. in xRoutes and xPolicies), +// there are two forms supported: +// - A numeric value exclusively indicates a ServicePort.VirtualPort +// - A non-numeric value exclusively indicates a ServicePort.TargetPort type ServicePort struct { state protoimpl.MessageState sizeCache protoimpl.SizeCache diff --git a/proto-public/pbcatalog/v2beta1/service.proto b/proto-public/pbcatalog/v2beta1/service.proto index fd03768b90..a43b251c96 100644 --- a/proto-public/pbcatalog/v2beta1/service.proto +++ b/proto-public/pbcatalog/v2beta1/service.proto @@ -23,6 +23,13 @@ message Service { repeated string virtual_ips = 3; } +// ServicePort declares a port exposed by the service that can be used in config and xRoute +// references. +// +// For outside references to a service port by string identifier (e.g. in xRoutes and xPolicies), +// there are two forms supported: +// - A numeric value exclusively indicates a ServicePort.VirtualPort +// - A non-numeric value exclusively indicates a ServicePort.TargetPort message ServicePort { // virtual_port is the port that could only be used when transparent // proxy is used alongside a virtual IP or a virtual DNS address. diff --git a/proto-public/pbcatalog/v2beta1/service_addon.go b/proto-public/pbcatalog/v2beta1/service_addon.go index d33cac4c0f..a24d97f589 100644 --- a/proto-public/pbcatalog/v2beta1/service_addon.go +++ b/proto-public/pbcatalog/v2beta1/service_addon.go @@ -3,6 +3,11 @@ package catalogv2beta1 +import ( + "fmt" + "strconv" +) + func (s *Service) IsMeshEnabled() bool { for _, port := range s.GetPorts() { if port.Protocol == Protocol_PROTOCOL_MESH { @@ -12,11 +17,104 @@ func (s *Service) IsMeshEnabled() bool { return false } -func (s *Service) FindServicePort(name string) *ServicePort { +// FindTargetPort finds a ServicePort by its TargetPort value. +// +// Unlike FindPortByID, it will match a numeric TargetPort value. This is useful when +// looking up a service port by a workload port value, or when the data is known to +// be normalized to canonical target port values (e.g. computed routes). +func (s *Service) FindTargetPort(targetPort string) *ServicePort { + if s == nil || targetPort == "" { + return nil + } + for _, port := range s.GetPorts() { - if port.TargetPort == name { + if port.TargetPort == targetPort { return port } } + return nil } + +// FindPortByID finds a ServicePort by its VirtualPort or TargetPort value. +// +// Note that this will not match a target port if the given value is numeric. +// See Service.ServicePort doc for more information on how port IDs are matched. +func (s *Service) FindPortByID(id string) *ServicePort { + if s == nil || id == "" { + return nil + } + + // If a port reference is numeric, it must be considered a virtual port. + // See ServicePort doc for more information. + if p, ok := toVirtualPort(id); ok { + for _, port := range s.GetPorts() { + if int(port.VirtualPort) == p { + return port + } + } + } else { + for _, port := range s.GetPorts() { + if port.TargetPort == id { + return port + } + } + } + + return nil +} + +// MatchesPortId returns true if the given port ID is non-empty and matches the virtual +// or target port of the given ServicePort. See ServicePort doc for more information on +// how port IDs are matched. +// +// Note that this function does not validate the provided port ID. Configured service +// ports should be validated on write, prior to use of this function, which means any +// matching value is implicitly valid. +func (sp *ServicePort) MatchesPortId(id string) bool { + if sp == nil || id == "" { + return false + } + + // If a port reference is numeric, it must be considered a virtual port. + // See ServicePort doc for more information. + if p, ok := toVirtualPort(id); ok { + if int(sp.VirtualPort) == p { + return true + } + } else { + if sp.TargetPort == id { + return true + } + } + + return false +} + +// VirtualPortStr is a convenience helper for checking the virtual port against a port ID in config +// (e.g. keys in FailoverPolicy.PortConfigs). It returns the string representation of the virtual port. +func (sp *ServicePort) VirtualPortStr() string { + if sp == nil { + return "" + } + return fmt.Sprintf("%d", sp.VirtualPort) +} + +func (sp *ServicePort) ToPrintableString() string { + if sp == nil { + return "" + } + if sp.VirtualPort > 0 { + return fmt.Sprintf("%s (virtual %d)", sp.TargetPort, sp.VirtualPort) + } + return sp.TargetPort +} + +// isVirtualPort returns the numeric virtual port value and true if the given port string is fully numeric. +// Otherwise, returns 0 and false. See ServicePort doc for more information. +func toVirtualPort(port string) (int, bool) { + if p, err := strconv.Atoi(port); err == nil && p > 0 { + return p, true + } + return 0, false +} diff --git a/proto-public/pbcatalog/v2beta1/service_addon_test.go b/proto-public/pbcatalog/v2beta1/service_addon_test.go index 63d81ca5f7..1414647a8d 100644 --- a/proto-public/pbcatalog/v2beta1/service_addon_test.go +++ b/proto-public/pbcatalog/v2beta1/service_addon_test.go @@ -62,17 +62,19 @@ func TestServiceIsMeshEnabled(t *testing.T) { } } -func TestFindServicePort(t *testing.T) { +func TestFindPort(t *testing.T) { cases := map[string]struct { - service *Service - port string - exp *ServicePort + service *Service + port string + expById *ServicePort + expByTargetPort *ServicePort }{ - "nil": {service: nil, port: "foo", exp: nil}, + "nil": {service: nil, port: "foo", expById: nil, expByTargetPort: nil}, "no ports": { - service: &Service{}, - port: "foo", - exp: nil, + service: &Service{}, + port: "foo", + expById: nil, + expByTargetPort: nil, }, "non-existing port": { service: &Service{ @@ -87,8 +89,9 @@ func TestFindServicePort(t *testing.T) { }, }, }, - port: "not-found", - exp: nil, + port: "not-found", + expById: nil, + expByTargetPort: nil, }, "existing port": { service: &Service{ @@ -108,16 +111,98 @@ func TestFindServicePort(t *testing.T) { }, }, port: "bar", - exp: &ServicePort{ + expById: &ServicePort{ TargetPort: "bar", Protocol: Protocol_PROTOCOL_TCP, }, + expByTargetPort: &ServicePort{ + TargetPort: "bar", + Protocol: Protocol_PROTOCOL_TCP, + }, + }, + "existing port by virtual port": { + service: &Service{ + Ports: []*ServicePort{ + { + TargetPort: "foo", + VirtualPort: 8080, + Protocol: Protocol_PROTOCOL_HTTP, + }, + { + TargetPort: "bar", + VirtualPort: 8081, + Protocol: Protocol_PROTOCOL_TCP, + }, + { + TargetPort: "baz", + VirtualPort: 8081, + Protocol: Protocol_PROTOCOL_MESH, + }, + }, + }, + port: "8081", + expById: &ServicePort{ + TargetPort: "bar", + VirtualPort: 8081, + Protocol: Protocol_PROTOCOL_TCP, + }, + expByTargetPort: nil, }, } for name, c := range cases { t.Run(name, func(t *testing.T) { - require.Equal(t, c.exp, c.service.FindServicePort(c.port)) + require.Equal(t, c.expById, c.service.FindPortByID(c.port)) + require.Equal(t, c.expByTargetPort, c.service.FindTargetPort(c.port)) + }) + } +} + +func TestMatchesPortId(t *testing.T) { + testPort := &ServicePort{VirtualPort: 8080, TargetPort: "http"} + + cases := map[string]struct { + port *ServicePort + id string + expected bool + }{ + "nil": {port: nil, id: "foo", expected: false}, + "empty": {port: testPort, id: "", expected: false}, + "non-existing virtual port": { + port: testPort, + id: "9090", + expected: false, + }, + "non-existing target port": { + port: testPort, + id: "other-port", + expected: false, + }, + "existing virtual port": { + port: testPort, + id: "8080", + expected: true, + }, + "existing target port": { + port: testPort, + id: "http", + expected: true, + }, + "virtual and target mismatch": { + port: &ServicePort{VirtualPort: 8080, TargetPort: "9090"}, + id: "9090", + expected: false, + }, + "virtual and target match": { + port: &ServicePort{VirtualPort: 9090, TargetPort: "9090"}, + id: "9090", + expected: true, + }, + } + + for name, c := range cases { + t.Run(name, func(t *testing.T) { + require.Equal(t, c.expected, c.port.MatchesPortId(c.id)) }) } } diff --git a/proto-public/pbmesh/v2beta1/common.pb.go b/proto-public/pbmesh/v2beta1/common.pb.go index 18df155c05..25eca7bfa0 100644 --- a/proto-public/pbmesh/v2beta1/common.pb.go +++ b/proto-public/pbmesh/v2beta1/common.pb.go @@ -36,6 +36,9 @@ type ParentReference struct { // For east/west this is the name of the Consul Service port to direct traffic to // or empty to imply all. // For north/south this is TBD. + // + // For more details on potential values of this field, see documentation for + // Service.ServicePort. Port string `protobuf:"bytes,2,opt,name=port,proto3" json:"port,omitempty"` } @@ -94,8 +97,10 @@ type BackendReference struct { Ref *pbresource.Reference `protobuf:"bytes,1,opt,name=ref,proto3" json:"ref,omitempty"` // For east/west this is the name of the Consul Service port to direct traffic to // or empty to imply using the same value as the parent ref. - // // For north/south this is TBD. + // + // For more details on potential values of this field, see documentation for + // Service.ServicePort. Port string `protobuf:"bytes,2,opt,name=port,proto3" json:"port,omitempty"` Datacenter string `protobuf:"bytes,3,opt,name=datacenter,proto3" json:"datacenter,omitempty"` } diff --git a/proto-public/pbmesh/v2beta1/common.proto b/proto-public/pbmesh/v2beta1/common.proto index 02ab5de340..cf8b2f26ce 100644 --- a/proto-public/pbmesh/v2beta1/common.proto +++ b/proto-public/pbmesh/v2beta1/common.proto @@ -16,6 +16,9 @@ message ParentReference { // For east/west this is the name of the Consul Service port to direct traffic to // or empty to imply all. // For north/south this is TBD. + // + // For more details on potential values of this field, see documentation for + // Service.ServicePort. string port = 2; } @@ -25,8 +28,10 @@ message BackendReference { // For east/west this is the name of the Consul Service port to direct traffic to // or empty to imply using the same value as the parent ref. - // // For north/south this is TBD. + // + // For more details on potential values of this field, see documentation for + // Service.ServicePort. string port = 2; string datacenter = 3; } diff --git a/proto-public/pbmesh/v2beta1/computed_routes.pb.go b/proto-public/pbmesh/v2beta1/computed_routes.pb.go index c1af9f011c..e7d0706864 100644 --- a/proto-public/pbmesh/v2beta1/computed_routes.pb.go +++ b/proto-public/pbmesh/v2beta1/computed_routes.pb.go @@ -87,6 +87,12 @@ type ComputedRoutes struct { sizeCache protoimpl.SizeCache unknownFields protoimpl.UnknownFields + // PortedConfigs is the map of service ports to the ComputedPortRoutes for + // those ports. + // + // The port identifier key here is always normalized to the target (workload) + // port name regardless of whether a virtual or target port identifier was + // provided in input config. PortedConfigs map[string]*ComputedPortRoutes `protobuf:"bytes,1,rep,name=ported_configs,json=portedConfigs,proto3" json:"ported_configs,omitempty" protobuf_key:"bytes,1,opt,name=key,proto3" protobuf_val:"bytes,2,opt,name=value,proto3"` // BoundReferences is a slice of mixed type references of resources that were // involved in the formulation of this resource. diff --git a/proto-public/pbmesh/v2beta1/computed_routes.proto b/proto-public/pbmesh/v2beta1/computed_routes.proto index f9243bff4b..e2682139c4 100644 --- a/proto-public/pbmesh/v2beta1/computed_routes.proto +++ b/proto-public/pbmesh/v2beta1/computed_routes.proto @@ -21,6 +21,12 @@ import "pbresource/resource.proto"; message ComputedRoutes { option (hashicorp.consul.resource.spec) = {scope: SCOPE_NAMESPACE}; + // PortedConfigs is the map of service ports to the ComputedPortRoutes for + // those ports. + // + // The port identifier key here is always normalized to the target (workload) + // port name regardless of whether a virtual or target port identifier was + // provided in input config. map ported_configs = 1; // BoundReferences is a slice of mixed type references of resources that were diff --git a/proto-public/pbmesh/v2beta1/destination_policy.pb.go b/proto-public/pbmesh/v2beta1/destination_policy.pb.go index 7853384e19..2a1fa6b29e 100644 --- a/proto-public/pbmesh/v2beta1/destination_policy.pb.go +++ b/proto-public/pbmesh/v2beta1/destination_policy.pb.go @@ -191,8 +191,8 @@ func (HashPolicyField) EnumDescriptor() ([]byte, []int) { } // DestinationPolicy is the destination-controlled set of defaults that -// are used when similar controls defined in an UpstreamConfig are left -// unspecified. +// are used when similar controls defined in an DestinationsConfiguration are +// left unspecified. // // Users may wish to share commonly configured settings for communicating with // a service in one place, but yet retain the ability to tweak those on a @@ -205,6 +205,10 @@ type DestinationPolicy struct { sizeCache protoimpl.SizeCache unknownFields protoimpl.UnknownFields + // PortConfigs defines the destination policy for a specific port on a service. + // + // For more details on potential values of the service port identifier key, + // see documentation for Service.ServicePort. PortConfigs map[string]*DestinationConfig `protobuf:"bytes,1,rep,name=port_configs,json=portConfigs,proto3" json:"port_configs,omitempty" protobuf_key:"bytes,1,opt,name=key,proto3" protobuf_val:"bytes,2,opt,name=value,proto3"` } diff --git a/proto-public/pbmesh/v2beta1/destination_policy.proto b/proto-public/pbmesh/v2beta1/destination_policy.proto index 0a18fd8c49..ff56a20920 100644 --- a/proto-public/pbmesh/v2beta1/destination_policy.proto +++ b/proto-public/pbmesh/v2beta1/destination_policy.proto @@ -9,8 +9,8 @@ import "google/protobuf/duration.proto"; import "pbresource/annotations.proto"; // DestinationPolicy is the destination-controlled set of defaults that -// are used when similar controls defined in an UpstreamConfig are left -// unspecified. +// are used when similar controls defined in an DestinationsConfiguration are +// left unspecified. // // Users may wish to share commonly configured settings for communicating with // a service in one place, but yet retain the ability to tweak those on a @@ -21,6 +21,10 @@ import "pbresource/annotations.proto"; message DestinationPolicy { option (hashicorp.consul.resource.spec) = {scope: SCOPE_NAMESPACE}; + // PortConfigs defines the destination policy for a specific port on a service. + // + // For more details on potential values of the service port identifier key, + // see documentation for Service.ServicePort. map port_configs = 1; } diff --git a/proto-public/pbmesh/v2beta1/destinations.pb.go b/proto-public/pbmesh/v2beta1/destinations.pb.go index 0e6f8c545e..9be3c1cf62 100644 --- a/proto-public/pbmesh/v2beta1/destinations.pb.go +++ b/proto-public/pbmesh/v2beta1/destinations.pb.go @@ -100,8 +100,9 @@ type Destination struct { // DestinationRef is the reference to an destination service. This has to be pbcatalog.Service type. DestinationRef *pbresource.Reference `protobuf:"bytes,1,opt,name=destination_ref,json=destinationRef,proto3" json:"destination_ref,omitempty"` - // DestinationPort is the port name of the destination service. This should be the name - // of the service's target port. + // DestinationPort is the port of the destination service. + // + // For more details on potential values of this field, see documentation for Service.ServicePort. DestinationPort string `protobuf:"bytes,2,opt,name=destination_port,json=destinationPort,proto3" json:"destination_port,omitempty"` // Datacenter is the datacenter for where this destination service lives. Datacenter string `protobuf:"bytes,3,opt,name=datacenter,proto3" json:"datacenter,omitempty"` diff --git a/proto-public/pbmesh/v2beta1/destinations.proto b/proto-public/pbmesh/v2beta1/destinations.proto index 33c7ea9195..713ba97832 100644 --- a/proto-public/pbmesh/v2beta1/destinations.proto +++ b/proto-public/pbmesh/v2beta1/destinations.proto @@ -29,8 +29,9 @@ message Destination { // DestinationRef is the reference to an destination service. This has to be pbcatalog.Service type. hashicorp.consul.resource.Reference destination_ref = 1; - // DestinationPort is the port name of the destination service. This should be the name - // of the service's target port. + // DestinationPort is the port of the destination service. + // + // For more details on potential values of this field, see documentation for Service.ServicePort. string destination_port = 2; // Datacenter is the datacenter for where this destination service lives. diff --git a/proto-public/pbmesh/v2beta1/destinations_configuration.pb.go b/proto-public/pbmesh/v2beta1/destinations_configuration.pb.go index 5e85fbfb92..9518f74818 100644 --- a/proto-public/pbmesh/v2beta1/destinations_configuration.pb.go +++ b/proto-public/pbmesh/v2beta1/destinations_configuration.pb.go @@ -94,7 +94,7 @@ func (x *DestinationsConfiguration) GetConfigOverrides() []*DestinationConfigOve return nil } -// UpstreamConfigOverrides allow to override destination configuration per destination_ref/port/datacenter. +// DestinationConfigOverrides allow to override destination configuration per destination_ref/port/datacenter. // In that sense, those three fields (destination_ref, destination_port and datacenter) are treated // sort of like map keys and config is a like a map value for that key. type DestinationConfigOverrides struct { @@ -105,8 +105,11 @@ type DestinationConfigOverrides struct { // DestinationRef is the reference to an destination service that this configuration applies to. // This has to be pbcatalog.Service type. DestinationRef *pbresource.Reference `protobuf:"bytes,1,opt,name=destination_ref,json=destinationRef,proto3" json:"destination_ref,omitempty"` - // DestinationPort is the port name of the destination service. This should be the name - // of the service's target port. If not provided, this configuration will apply to all ports of an destination. + // DestinationPort is the port of the destination service. + // + // For more details on potential values of this field, see documentation for Service.ServicePort. + // + // If not provided, this configuration will apply to all ports of an destination. DestinationPort string `protobuf:"bytes,2,opt,name=destination_port,json=destinationPort,proto3" json:"destination_port,omitempty"` // Datacenter is the datacenter for where this destination service lives. Datacenter string `protobuf:"bytes,3,opt,name=datacenter,proto3" json:"datacenter,omitempty"` diff --git a/proto-public/pbmesh/v2beta1/destinations_configuration.proto b/proto-public/pbmesh/v2beta1/destinations_configuration.proto index facdde6e1f..b3d1ed93f3 100644 --- a/proto-public/pbmesh/v2beta1/destinations_configuration.proto +++ b/proto-public/pbmesh/v2beta1/destinations_configuration.proto @@ -28,7 +28,7 @@ message DestinationsConfiguration { repeated DestinationConfigOverrides config_overrides = 3; } -// UpstreamConfigOverrides allow to override destination configuration per destination_ref/port/datacenter. +// DestinationConfigOverrides allow to override destination configuration per destination_ref/port/datacenter. // In that sense, those three fields (destination_ref, destination_port and datacenter) are treated // sort of like map keys and config is a like a map value for that key. message DestinationConfigOverrides { @@ -36,8 +36,11 @@ message DestinationConfigOverrides { // This has to be pbcatalog.Service type. hashicorp.consul.resource.Reference destination_ref = 1; - // DestinationPort is the port name of the destination service. This should be the name - // of the service's target port. If not provided, this configuration will apply to all ports of an destination. + // DestinationPort is the port of the destination service. + // + // For more details on potential values of this field, see documentation for Service.ServicePort. + // + // If not provided, this configuration will apply to all ports of an destination. string destination_port = 2; // Datacenter is the datacenter for where this destination service lives. diff --git a/test-integ/catalogv2/explicit_destinations_l7_test.go b/test-integ/catalogv2/explicit_destinations_l7_test.go index 927a8ec7dd..8ea21e012d 100644 --- a/test-integ/catalogv2/explicit_destinations_l7_test.go +++ b/test-integ/catalogv2/explicit_destinations_l7_test.go @@ -124,6 +124,7 @@ func (c testSplitterFeaturesL7ExplicitDestinationsCreator) NewConfig(t *testing. Enterprise: utils.IsEnterprise(), Name: clusterName, Nodes: servers, + Services: make(map[topology.ID]*pbcatalog.Service), } lastNode := 0 @@ -181,6 +182,7 @@ func (c testSplitterFeaturesL7ExplicitDestinationsCreator) topologyConfigAddNode newID("static-server-v1", tenancy), topology.NodeVersionV2, func(wrk *topology.Workload) { + wrk.V2Services = []string{"static-server-v1", "static-server"} wrk.Meta = map[string]string{ "version": "v1", } @@ -200,6 +202,7 @@ func (c testSplitterFeaturesL7ExplicitDestinationsCreator) topologyConfigAddNode newID("static-server-v2", tenancy), topology.NodeVersionV2, func(wrk *topology.Workload) { + wrk.V2Services = []string{"static-server-v2", "static-server"} wrk.Meta = map[string]string{ "version": "v2", } @@ -219,6 +222,7 @@ func (c testSplitterFeaturesL7ExplicitDestinationsCreator) topologyConfigAddNode newID("static-client", tenancy), topology.NodeVersionV2, func(wrk *topology.Workload) { + wrk.V2Services = []string{"static-client"} for i, tenancy := range c.tenancies { wrk.Destinations = append(wrk.Destinations, &topology.Destination{ @@ -296,40 +300,58 @@ func (c testSplitterFeaturesL7ExplicitDestinationsCreator) topologyConfigAddNode }}, }) - staticServerService := sprawltest.MustSetResourceData(t, &pbresource.Resource{ - Id: &pbresource.ID{ - Type: pbcatalog.ServiceType, - Name: "static-server", - Tenancy: tenancy, - }, - }, &pbcatalog.Service{ - Workloads: &pbcatalog.WorkloadSelector{ - // This will result in a 50/50 uncontrolled split. - Prefixes: []string{"static-server-"}, - }, - Ports: []*pbcatalog.ServicePort{ + portsFunc := func(offset uint32) []*pbcatalog.ServicePort { + return []*pbcatalog.ServicePort{ { - TargetPort: "http", - Protocol: pbcatalog.Protocol_PROTOCOL_HTTP, + TargetPort: "http", + VirtualPort: 8005 + offset, + Protocol: pbcatalog.Protocol_PROTOCOL_HTTP, }, { - TargetPort: "http2", - Protocol: pbcatalog.Protocol_PROTOCOL_HTTP2, + TargetPort: "http2", + VirtualPort: 8006 + offset, + Protocol: pbcatalog.Protocol_PROTOCOL_HTTP2, }, { - TargetPort: "grpc", - Protocol: pbcatalog.Protocol_PROTOCOL_GRPC, + TargetPort: "grpc", + VirtualPort: 9005 + offset, + Protocol: pbcatalog.Protocol_PROTOCOL_GRPC, }, { - TargetPort: "tcp", - Protocol: pbcatalog.Protocol_PROTOCOL_TCP, + TargetPort: "tcp", + VirtualPort: 10005 + offset, + Protocol: pbcatalog.Protocol_PROTOCOL_TCP, }, { TargetPort: "mesh", Protocol: pbcatalog.Protocol_PROTOCOL_MESH, }, + } + } + + // Differ parent and backend virtual ports to verify we route to each correctly. + parentServicePorts := portsFunc(0) + backendServicePorts := portsFunc(100) + + // Explicitly define backend services s.t. they are not inferred from workload, + // which would assign random virtual ports. + cluster.Services[newID("static-client", tenancy)] = &pbcatalog.Service{ + Ports: []*pbcatalog.ServicePort{ + { + TargetPort: "mesh", + Protocol: pbcatalog.Protocol_PROTOCOL_MESH, + }, }, - }) + } + cluster.Services[newID("static-server", tenancy)] = &pbcatalog.Service{ + Ports: parentServicePorts, + } + cluster.Services[newID("static-server-v1", tenancy)] = &pbcatalog.Service{ + Ports: backendServicePorts, + } + cluster.Services[newID("static-server-v2", tenancy)] = &pbcatalog.Service{ + Ports: backendServicePorts, + } httpServerRoute := sprawltest.MustSetResourceData(t, &pbresource.Resource{ Id: &pbresource.ID{ @@ -345,7 +367,7 @@ func (c testSplitterFeaturesL7ExplicitDestinationsCreator) topologyConfigAddNode Name: "static-server", Tenancy: tenancy, }, - Port: "http", + Port: "8005", // use mix of target and virtual parent ports }, { Ref: &pbresource.Reference{ @@ -406,6 +428,7 @@ func (c testSplitterFeaturesL7ExplicitDestinationsCreator) topologyConfigAddNode Name: "static-server-v1", Tenancy: tenancy, }, + Port: "9105", // use mix of virtual and target (inferred from parent) ports }, Weight: 10, }, @@ -436,7 +459,7 @@ func (c testSplitterFeaturesL7ExplicitDestinationsCreator) topologyConfigAddNode Name: "static-server", Tenancy: tenancy, }, - Port: "tcp", + Port: "10005", // use virtual parent port }}, Rules: []*pbmesh.TCPRouteRule{{ BackendRefs: []*pbmesh.TCPBackendRef{ @@ -447,6 +470,7 @@ func (c testSplitterFeaturesL7ExplicitDestinationsCreator) topologyConfigAddNode Name: "static-server-v1", Tenancy: tenancy, }, + Port: "10105", // use explicit virtual port }, Weight: 10, }, @@ -457,6 +481,7 @@ func (c testSplitterFeaturesL7ExplicitDestinationsCreator) topologyConfigAddNode Name: "static-server-v2", Tenancy: tenancy, }, + Port: "tcp", // use explicit target port }, Weight: 90, }, @@ -471,7 +496,6 @@ func (c testSplitterFeaturesL7ExplicitDestinationsCreator) topologyConfigAddNode ) cluster.InitialResources = append(cluster.InitialResources, - staticServerService, v1TrafficPerms, v2TrafficPerms, httpServerRoute, diff --git a/testing/deployer/topology/compile.go b/testing/deployer/topology/compile.go index 0651115baa..50bc770a69 100644 --- a/testing/deployer/topology/compile.go +++ b/testing/deployer/topology/compile.go @@ -529,7 +529,7 @@ func compile(logger hclog.Logger, raw *Config, prev *Topology) (*Topology, error } if c.EnableV2 { - // Populate the VirtualPort field on all implied destinations. + // Populate the VirtualPort field on all destinations. for _, n := range c.Nodes { for _, wrk := range n.Workloads { for _, dest := range wrk.ImpliedDestinations { @@ -539,7 +539,20 @@ func compile(logger hclog.Logger, raw *Config, prev *Topology) (*Topology, error if sp.Protocol == pbcatalog.Protocol_PROTOCOL_MESH { continue } - if sp.TargetPort == dest.PortName { + if sp.MatchesPortId(dest.PortName) { + dest.VirtualPort = sp.VirtualPort + } + } + } + } + for _, dest := range wrk.Destinations { + res, ok := c.Services[dest.ID] + if ok { + for _, sp := range res.Ports { + if sp.Protocol == pbcatalog.Protocol_PROTOCOL_MESH { + continue + } + if sp.MatchesPortId(dest.PortName) { dest.VirtualPort = sp.VirtualPort } } diff --git a/testing/deployer/topology/topology.go b/testing/deployer/topology/topology.go index 79b3d468d4..b1395ae78c 100644 --- a/testing/deployer/topology/topology.go +++ b/testing/deployer/topology/topology.go @@ -1061,7 +1061,10 @@ type Destination struct { LocalPort int Peer string `json:",omitempty"` - // PortName is the named port of this Destination to route traffic to. + // PortName is the port of this Destination to route traffic to. + // + // For more details on potential values of this field, see documentation + // for Service.ServicePort. // // This only applies for multi-port (v2). PortName string `json:",omitempty"`