hubble: Fix reply state unknown being interpreted as false #13750
hubble: Fix reply state unknown being interpreted as false #13750
Conversation
This replaces the existing `reply` field, which suffers from false negatives due to protobuf not being able to distinguish between a false value and an absent value. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
There was a problem hiding this comment.
Reviewer note: This assumption is not presumed anymore with this change, so there is a slight change in behavior here: The reply filter will never report any Drop or Policy Verdict events anymore (as they have is_reply always nil).
If people feel strongly about this, I'm happy to add back an exception in the filter logic here to keep this previous behavior. But I'm not 100% confident that the exception is always correct (e.g. we could have dropped a spurious reply packet), which is why I removed it.
7c24be6 to
18dcdf3
Compare
|
Unfortunately we don't have proper CI for the metrics yet. But here are some stats from my local testing. Hubble with the connectivity-check.yaml running and the port-distribution metric enabled. Without the bugfix, after ~15 minutes: $ curl -s localhost:9091/metrics | grep hubble_port_distribution_total | wc -l
8343With the bug fix applied: $ curl -s localhost:9091/metrics | grep hubble_port_distribution_total | wc -l
11 |
18dcdf3 to
5132516
Compare
This fixes a long-standing issue with Hubble not being able to distingiush between the reply field being false and the reply field being absent. To address the issue, the old `reply` field is replaced with a tri-state `is_reply` field which can be either nil, true or false. All uses of the old field are removed in the source tree, but the field itself is preserved for backwards-compatibility. This incidentally also fixes a bug where the `port-distribution` metric suffered from the same confusion, which caused it to report source ports as destination ports. Ref: #13248 Fixes: #13744 Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
5132516 to
94e1c60
Compare
michi-covalent
left a comment
There was a problem hiding this comment.
lgtm 👍 one question around nil check
|
test-me-please |
|
Two CI flakes: |
|
retest-gke |
|
retest-4.9 |
|
retest-runtime |
This fixes a long-standing issue with Hubble not being able to
distingiush between the reply field being false and the reply field
being absent. To address the issue, the old
replyfield is replacedwith a tri-state
is_replyfield which can be either nil, true orfalse. All uses of the old field are removed in the source tree, but the
field itself is preserved for backwards-compatibility.
This incidentally also fixes a bug where the
port-distributionmetricsuffered from the same confusion, which caused it to report source ports
as destination ports.
Ref: #13248
Fixes: #13744