tap: return an error when a `PortRange`'s min > max by hawkw · Pull Request #1044 · linkerd/linkerd2-proxy

hawkw · 2021-06-10T21:40:14Z

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.

Signed-off-by: Eliza Weisman <[email protected]>

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

Signed-off-by: Eliza Weisman <[email protected]>

changes

This release updates a wide variety of the proxy's dependencies. No user-facing changes are expected. --- * tap: Restore tap matching tests (linkerd/linkerd2-proxy#1042) * Add a 'rustfmt' feature to crates that use gRPC (linkerd/linkerd2-proxy#1041) * tap: return an error when a `PortRange`'s min > max (linkerd/linkerd2-proxy#1044) * retry: Enforce a cap on max replay buffer size (linkerd/linkerd2-proxy#1043) * Add codecov integration via cargo-tarpaulin (linkerd/linkerd2-proxy#1047) * Add a dependabot configuration (linkerd/linkerd2-proxy#1046) * Fixup dependabot configuration (linkerd/linkerd2-proxy#1049) * build(deps): bump actions/upload-artifact from 2.2.3 to 2.2.4 (linkerd/linkerd2-proxy#1050) * build(deps): bump softprops/action-gh-release from 0.1.5 to 1 (linkerd/linkerd2-proxy#1051) * build(deps): bump libfuzzer-sys in /linkerd/transport-header/fuzz (linkerd/linkerd2-proxy#1052) * build(deps): bump libfuzzer-sys from 0.4.0 to 0.4.2 in /linkerd/tls/fuzz (linkerd/linkerd2-proxy#1053) * build(deps): bump libfuzzer-sys in /linkerd/addr/fuzz (linkerd/linkerd2-proxy#1054) * build(deps): bump tokio from 1.5.0 to 1.7.0 in /linkerd/proxy/http/fuzz (linkerd/linkerd2-proxy#1056) * build(deps): bump tracing from 0.1.25 to 0.1.26 in /linkerd/dns/fuzz (linkerd/linkerd2-proxy#1055) * build(deps): bump libfuzzer-sys from 0.4.0 to 0.4.2 in /linkerd/dns/fuzz (linkerd/linkerd2-proxy#1058) * build(deps): bump tracing from 0.1.25 to 0.1.26 in /linkerd/addr/fuzz (linkerd/linkerd2-proxy#1059) * build(deps): bump libfuzzer-sys in /linkerd/proxy/http/fuzz (linkerd/linkerd2-proxy#1060) * build(deps): bump tracing in /linkerd/transport-header/fuzz (linkerd/linkerd2-proxy#1061) * build(deps): bump tokio from 1.5.0 to 1.7.0 in /linkerd/dns/fuzz (linkerd/linkerd2-proxy#1062) * build(deps): bump tokio in /linkerd/transport-header/fuzz (linkerd/linkerd2-proxy#1063) * build(deps): bump arbitrary in /linkerd/app/inbound/fuzz (linkerd/linkerd2-proxy#1064) * build(deps): bump tokio from 1.5.0 to 1.7.0 in /linkerd/app/inbound/fuzz (linkerd/linkerd2-proxy#1065) * build(deps): bump tracing in /linkerd/app/inbound/fuzz (linkerd/linkerd2-proxy#1066) * build(deps): bump hyper in /linkerd/app/inbound/fuzz (linkerd/linkerd2-proxy#1067) * build(deps): bump deflate from 0.7.20 to 0.9.1 (linkerd/linkerd2-proxy#1068) * build(deps): bump libfuzzer-sys in /linkerd/app/inbound/fuzz (linkerd/linkerd2-proxy#1069) * build(deps): bump tonic-build from 0.4.0 to 0.4.2 (linkerd/linkerd2-proxy#1070) * build(deps): bump linkerd2-proxy-api from `453ac1e` to `48f13d6` (linkerd/linkerd2-proxy#1071) * build(deps): bump ipnet from 2.3.0 to 2.3.1 (linkerd/linkerd2-proxy#1072) * build(deps): bump regex from 1.4.3 to 1.4.6 (linkerd/linkerd2-proxy#1073) * build(deps): bump arbitrary in /linkerd/transport-header/fuzz (linkerd/linkerd2-proxy#1057) * Update tonic to v0.4.3 (linkerd/linkerd2-proxy#1080) * ci: Skip coverage tests on dependencies-only changes (linkerd/linkerd2-proxy#1074) * build(deps): bump indexmap from 1.6.1 to 1.6.2 (linkerd/linkerd2-proxy#1075) * build(deps): bump rand from 0.8.3 to 0.8.4 (linkerd/linkerd2-proxy#1077) * build(deps): bump tokio-test from 0.4.0 to 0.4.2 (linkerd/linkerd2-proxy#1076) * build(deps): bump pin-project from 1.0.5 to 1.0.7 (linkerd/linkerd2-proxy#1078) * build(deps): bump arbitrary from 1.0.0 to 1.0.1 (linkerd/linkerd2-proxy#1083) * outbound: replace `sleep`s in test with `yield_now` (linkerd/linkerd2-proxy#1085) * build(deps): bump thiserror from 1.0.23 to 1.0.25 (linkerd/linkerd2-proxy#1081) * build(deps): bump async-stream from 0.3.0 to 0.3.2 (linkerd/linkerd2-proxy#1084) * build(deps): bump serde_json from 1.0.62 to 1.0.64 (linkerd/linkerd2-proxy#1086) * build(deps): bump tokio-stream from 0.1.5 to 0.1.6 (linkerd/linkerd2-proxy#1082) * build(deps): Skip coverage by PR title (linkerd/linkerd2-proxy#1087) * fuzz: Omit lockfiles from version control (linkerd/linkerd2-proxy#1088) * build(deps): bump tokio from 1.6.1 to 1.7.0 (linkerd/linkerd2-proxy#1079) * build(deps): bump mimalloc from 0.1.24 to 0.1.25 (linkerd/linkerd2-proxy#1089) * build(deps): bump tokio-util from 0.6.5 to 0.6.7 (linkerd/linkerd2-proxy#1090) * build(deps): bump libc from 0.2.86 to 0.2.97 (linkerd/linkerd2-proxy#1093) * build(deps): bump http from 0.2.3 to 0.2.4 (linkerd/linkerd2-proxy#1092) * build(deps): bump rustls from 0.19.0 to 0.19.1 (linkerd/linkerd2-proxy#1094) * build(deps): bump async-trait from 0.1.42 to 0.1.50 (linkerd/linkerd2-proxy#1095) * build(deps): bump tracing-subscriber from 0.2.17 to 0.2.18 (linkerd/linkerd2-proxy#1096) * build(deps): bump http-body from 0.4.0 to 0.4.2 (linkerd/linkerd2-proxy#1097) * build(deps): bump tracing from 0.1.25 to 0.1.26 (linkerd/linkerd2-proxy#1098) * build(deps): bump hdrhistogram from 7.2.0 to 7.3.0 (linkerd/linkerd2-proxy#1099) * build(deps): bump regex from 1.4.6 to 1.5.4 (linkerd/linkerd2-proxy#1100) * build(deps): bump tower from 0.4.7 to 0.4.8 (linkerd/linkerd2-proxy#1102) * build(deps): bump libfuzzer-sys from 0.4.0 to 0.4.2 (linkerd/linkerd2-proxy#1103) * build(deps): bump actions/download-artifact from 2.0.9 to 2.0.10 (linkerd/linkerd2-proxy#1104) * Remove unneeded uses of indexmap (linkerd/linkerd2-proxy#1048) * build(deps): Update trust-dns to v0.21.0-alpha.1 (linkerd/linkerd2-proxy#1105) * config: add a `PortSet` type (linkerd/linkerd2-proxy#1106) * replace `linkerd-concurrency-limit` with Tower (linkerd/linkerd2-proxy#1107) * build(deps): Update Rust to v1.53.0 (linkerd/linkerd2-proxy#1108)

hawkw added 2 commits June 10, 2021 14:32

tap: add test for min > max

1402438

Signed-off-by: Eliza Weisman <[email protected]>

hawkw requested a review from a team June 10, 2021 21:40

olix0r previously approved these changes Jun 10, 2021

View reviewed changes

hawkw added 2 commits June 10, 2021 15:22

oh wait apparently zero is special-cased

7d421c6

Signed-off-by: Eliza Weisman <[email protected]>

perhaps it is quickcheck that is wrong

4e49d55

Signed-off-by: Eliza Weisman <[email protected]>

olix0r added 3 commits June 11, 2021 14:57

fix quickcheck on port ranges

f0546a4

clippy

e7c88dc

touchup

df0f9dd

olix0r approved these changes Jun 11, 2021

View reviewed changes

olix0r merged commit 59b9ad3 into main Jun 11, 2021

olix0r deleted the eliza/bad-range branch June 11, 2021 17:57

olix0r mentioned this pull request Jun 18, 2021

proxy: v2.148.0 linkerd/linkerd2#6314

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tap: return an error when a `PortRange`'s min > max#1044

tap: return an error when a `PortRange`'s min > max#1044
olix0r merged 7 commits intomainfrom
eliza/bad-range

hawkw commented Jun 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

hawkw commented Jun 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants