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 fe7925a88501..a980939a96f5 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 bbe87511e17e..3793dd07b0a5 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 fed65c4e71cd..509314a48f7c 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 d864d7acb1d3..5bc889a1f50b 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 da2c9de6f140..e2dc8accf3cb 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 2c8b428aecac..646578beb6e6 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 1f4b02269772..5eecda1f6095 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 3b331a9a6302..7ddbd00b4bee 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 0cd0d3b71bcf..2e9546b33663 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 0bf933a48262..84afa90ac007 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 4b243bf15272..456cacc93b60 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 91a5d1520776..4b3c6ef9f5b1 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 a5866fbbf0f2..2aa54ca0fcbc 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 a0ddad0b089c..f5501a27b070 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 282f4d2a84b7..55405c4e55bf 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, ValidateServicePortID("http")) }) t.Run("ok", func(t *testing.T) { - require.NoError(t, ValidatePortName("http")) + require.NoError(t, ValidateServicePortID("8080")) }) } diff --git a/internal/mesh/internal/controllers/explicitdestinations/controller.go b/internal/mesh/internal/controllers/explicitdestinations/controller.go index 58e2f615ad1b..0f69641efefd 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 75001296041e..e21c528f114b 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 000000000000..99a5ddb57308 --- /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 000000000000..fdf3073c8ea5 --- /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 9bfb2808e303..c45bd4488ca5 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 02ae0e0d435d..fc2d913f83fc 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 b860eede202d..a81317b8607b 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 71933a2a5d93..f383d4f05e0d 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 71b6b8cd5b0b..b1e838ed231e 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 a3c7dd460d62..78d7b2cc0171 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 596a4918d1fd..644ceaaf204c 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 515d46ab4302..9d9ec2756e82 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 4fe3062367cf..40f3c4aef10e 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 a128631195fe..301fdbf6d7a4 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 b97535df0ff1..a30e6b8f0d84 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 abbeb46a3ae8..8e150990b5a0 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 e019f7b60a0b..9092474dc7de 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 fd03768b907f..a43b251c96a9 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 d33cac4c0fe8..a24d97f58921 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 63d81ca5f728..1414647a8dd8 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.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.exp, c.service.FindServicePort(c.port)) + 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 18df155c05ef..25eca7bfa021 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 02ab5de3400d..cf8b2f26ce50 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 c1af9f011c54..e7d0706864a6 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 f9243bff4bc1..e2682139c437 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 7853384e19c5..2a1fa6b29eb3 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 0a18fd8c49b6..ff56a20920c0 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 0e6f8c545eed..9be3c1cf62fb 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 33c7ea919597..713ba978328c 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 5e85fbfb927b..9518f74818be 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 facdde6e1f5a..b3d1ed93f3cb 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 927a8ec7dd09..8ea21e012d2a 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 0651115baac2..50bc770a6954 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 79b3d468d4af..b1395ae78c8d 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"`