Conversation
hawkw
added a commit
that referenced
this pull request
Apr 26, 2023
TCP ports are [defined as 16-bit unsigned integers][1]. Currently, however, the Go controllers often represent them as other numeric types, such as `int` and `uint32`. We currently validate parsed ports by testing whether they are less than 65536. However, it's not always clear whether a given `int` represents a port number that has already been validated by this check or not. This commit changes the `PortRange` type and parsing code to represent ports as `uint16`, making invalid states harder to represent at the type level. We change port parsing from using `strconv.Atoi` (which returns an `int`, which may be negative or greater than 65535) to using `strconv.ParseUint` with a bit size of 16 --- this function will fail to parse the value if it is a negative number or greater than 65535, so we no longer have to perform additional validation when parsing ports. We can also remove some casting that is no longer necessary. It's still necessary to cast to `uint32` in a couple of places when constructing protobuf messages, because protobuf doesn't support 16-bit integers. Depends on #10827 [1]: https://en.wikipedia.org/wiki/Port_(computer_networking)#Port_number
adleong
approved these changes
Apr 26, 2023
| et.log.Errorf("failed to get opaque ports for pod %s/%s: %s", address.Pod.Namespace, address.Pod.Name, err) | ||
| } | ||
| opaquePorts = getAnnotatedOpaquePorts(address.Pod, et.defaultOpaquePorts) | ||
| wa, err = createWeightedAddr(address, opaquePorts, et.enableH2Upgrade, et.identityTrustDomain, et.controllerNS, et.log) |
Member
There was a problem hiding this comment.
Maybe it's outside the scope of this change, but we could also change createWeightedAddr to accept a slice of PortRanges instead of a set of ports. It needs to be able to determine if a given port is opaque and it could do this by checking to see if the port is contained in any of the opaque port ranges. On the other hand, it works fine today with an expanded set of ports so maybe there's not a good reason to change it. 🤷♀️
Contributor
Author
There was a problem hiding this comment.
I might address this in a follow-up, if that's okay?
mateiidavid
approved these changes
Apr 27, 2023
risingspiral
pushed a commit
that referenced
this pull request
May 4, 2023
Currently, the proxy injector will expand lists of opaque port ranges into lists of individual port numbers. This is because the proxy has historically not accepted port ranges in the `LINKERD2_PROXY_INBOUND_PORTS_DISABLE_PROTOCOL_DETECTION` environment variable. However, when very large ranges are used, the size of the injected manifest can be quite large, since each individual port number in a range must be listed separately. Proxy PR linkerd/linkerd2-proxy#2395 changed the proxy to accept ranges as well as individual port numbers in the opaque ports environment variable, and this change was included in the latest proxy release (v2.200.0). This means that the proxy-injector no longer needs to expand large port ranges into individual port numbers, and can now simply forward the list of ranges to the proxy. This branch changes the proxy injector to do this, resolving issues with manifest size due to large port ranges. Closes #9803
risingspiral
pushed a commit
that referenced
this pull request
May 5, 2023
Currently, the proxy injector will expand lists of opaque port ranges into lists of individual port numbers. This is because the proxy has historically not accepted port ranges in the `LINKERD2_PROXY_INBOUND_PORTS_DISABLE_PROTOCOL_DETECTION` environment variable. However, when very large ranges are used, the size of the injected manifest can be quite large, since each individual port number in a range must be listed separately. Proxy PR linkerd/linkerd2-proxy#2395 changed the proxy to accept ranges as well as individual port numbers in the opaque ports environment variable, and this change was included in the latest proxy release (v2.200.0). This means that the proxy-injector no longer needs to expand large port ranges into individual port numbers, and can now simply forward the list of ranges to the proxy. This branch changes the proxy injector to do this, resolving issues with manifest size due to large port ranges. Closes #9803 Signed-off-by: Eric Anderson <eric@buoyant.io>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Currently, the proxy injector will expand lists of opaque port ranges
into lists of individual port numbers. This is because the proxy has
historically not accepted port ranges in the
LINKERD2_PROXY_INBOUND_PORTS_DISABLE_PROTOCOL_DETECTIONenvironmentvariable. However, when very large ranges are used, the size of the
injected manifest can be quite large, since each individual port number
in a range must be listed separately.
Proxy PR linkerd/linkerd2-proxy#2395 changed the proxy to accept ranges
as well as individual port numbers in the opaque ports environment
variable, and this change was included in the latest proxy release
(v2.200.0). This means that the proxy-injector no longer needs to expand
large port ranges into individual port numbers, and can now simply
forward the list of ranges to the proxy. This branch changes the proxy
injector to do this, resolving issues with manifest size due to large
port ranges.
Closes #9803