Skip to content

Commit

Permalink
tap: return an error when a PortRange's min > max
Browse files Browse the repository at this point in the history
The quickcheck tests for tap that were re-enabled in PR #1042 have
caught a bug: when a `PortMatch` message has a `min` value that's
greater than its `max` value, the tests expect that `TcpMatch::try_from`
should return an error (since the range is obviously invalid). However,
we don't currently do that.

This is causing CI to break on an unrelated change:
https://github.com/linkerd/linkerd2-proxy/runs/2797285941#step:3:907

This PR adds a new test reproducing the failing quickcheck inputs, and
fixes the bug (by making `TcpMatch::try_from` return an error in that
case).

Since quickcheck inputs are randomly generated and are not
deterministic, I thought it was probably a good idea to have a
hard-coded test to reproduce this case, just in case there are future
regressions. As a side note, we may want to consider switching from the
`quickcheck` crate to the [`proptest` crate] --- one nice feature of
`proptest` is that it automatically records failing inputs, which can be
checked in to git, and future test runs will try all the failing inputs
as well as randomly generating new ones.

[`proptest` crate]: https://github.com/AltSysrq/proptest
  • Loading branch information
hawkw committed Jun 10, 2021
1 parent 1402438 commit 48f7b17
Showing 1 changed file with 4 additions and 1 deletion.
5 changes: 4 additions & 1 deletion linkerd/proxy/tap/src/grpc/match_.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ impl TryFrom<observe_request::r#match::Tcp> for TcpMatch {
m.r#match.ok_or(InvalidMatch::Empty).and_then(|t| match t {
tcp::Match::Ports(range) => {
// If either a minimum or maximum is not specified, the range is considered to
// be over a discrete value.
// be over a discrete value.`
let min = if range.min == 0 { range.max } else { range.min };
let max = if range.max == 0 { range.min } else { range.max };
if min == 0 || max == 0 {
Expand All @@ -189,6 +189,9 @@ impl TryFrom<observe_request::r#match::Tcp> for TcpMatch {
if min > u32::from(::std::u16::MAX) || max > u32::from(::std::u16::MAX) {
return Err(InvalidMatch::InvalidPort);
}
if min > max {
return Err(InvalidMatch::InvalidPort);
}
Ok(TcpMatch::PortRange(min as u16, max as u16))
}

Expand Down

0 comments on commit 48f7b17

Please sign in to comment.