[NET-5586][rebased] v2: Support virtual port references in config #20371
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In addition to Service target port references, allow users to specify a port by stringified virtual port value. This is useful in environments such as Kubernetes where typical configuration is written in terms of Service virtual ports rather than workload (pod) target port names. Retaining the option of referencing target ports by name supports VMs, Nomad, and other use cases where virtual ports are not used by default. To support both uses cases at once, we will strictly interpret port references based on whether the value is numeric. See updated `ServicePort` docs for more details.
Update proto and generated .go files with docs reflecting virtual port reference support.
Add coverage for mixed virtual and target port references to existing test.
…r policy and assert error conditions against FailoverPolicy and ComputedFailoverPolicy resources
ndhanushkodi
added
pr/no-backport
pr/no-changelog
rboyer
approved these changes
Jan 29, 2024
Merged
2 tasks
ndhanushkodi
added a commit
to hashicorp/consul-k8s
that referenced
this pull request
Jan 31, 2024
…#3528) - [This change in consul](hashicorp/consul#20371) involves now interpreting whether xRoute/FailoverPolicy/DestinationPolicy resource service references use either the service port (virtualPort in consul) or service target port (targetPort in consul). To make this decision unambiguously: > This change updates our interpretation of these reference fields/keys (parent, backend, destination), s.t.: > > * A numeric value will be exclusively interpreted to indicate a ServicePort.virtual_port > * A non-numeric value will be exclusively interpreted to indicate a ServicePort.target_port (this supports VMs/Nomad and other cases where network virtual ports are not used, and port names are expected to be in reference to workload ports, not service ports) - If a K8s service targetport is allowed to be the stringified version of a number, it will be ambiguous in consul what to interpret the string "portID" as. - This change makes it such that the string port can never be a number, and will always also have alpha characters by prefixing "cslport-" to the workload port if the workload port name is unspecified.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Rebased @zalimeni's PR #20327 on top of the computed failover policy changes and pushed here just to not lose the changes on the original PR in case something is wrong with this one.
I needed to change the failover controller and failover controller test to make it work with the new computed failover policy changes that got merged into main.
Original PR contents:
Currently, when referencing a service in a parent/backend ref in an xRoute (example), destination ref in a
Destinationsresource, or port in an xPolicy config key, we only allow operators to specify workload port name (i.e the Service'sspec.ports[0].targetPortin K8s terms). This is not an intuitive UX for K8s users, who will be thinking of the service ports (i.e. Servicespec.ports[0].portin K8s) rather than workload ports when creating these objects.This change updates our interpretation of these reference fields/keys (parent, backend, destination), s.t.:
ServicePort.virtual_portServicePort.target_port(this supports VMs/Nomad and other cases where network virtual ports are not used, and port names are expected to be in reference to workload ports, not service ports)The effect should be a superset of existing functionality, with the additional option of specifying a virtual port (numeric string) where a service target port was previously required. Note that this does not apply to workload port definitions and endpoints config (which references workload port names), as virtual ports are only a facility of the service abstraction.
Testing & Reproduction steps
Links
PR Checklist