Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ignore non-QUIC packets in trace analysis (#421) #422

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

koujaz
Copy link

@koujaz koujaz commented Jan 27, 2025

Non-QUIC packet can slip through in certain cases.
This change is to log the message instead of crashing whole test.

@marten-seemann
Copy link
Collaborator

I don't think we need any log output. These packets are expected in the migration test case.

@koujaz
Copy link
Author

koujaz commented Jan 27, 2025

Understand; will change it.
The idea to log message was that trace.py is generic and such packets in other test cases may not be expected. So we at least have some observability.

@marten-seemann
Copy link
Collaborator

Makes sense. Can we log the type of the packet somehow? Might be useful to see if it's an ICMP or ARP packet.

@koujaz
Copy link
Author

koujaz commented Jan 27, 2025

Yes, logging packets's __repr__ instead of __str__ to get nice and small message:

Captured packet without quic layer: <ICMPV6 Packet>

Note __str__ in this case can get very long and would not be appropriate for this case:

Captured packet without quic layer: Packet (Length: 142)
Layer ETH
:	Destination: 02:42:c1:a7:64:64
	.... ..1. .... .... .... .... = LG bit: Locally administered address (this is NOT the factory default)
	.... ...0 .... .... .... .... = IG bit: Individual address (unicast)
	Source: 02:42:c1:a7:64:02
	.... ..1. .... .... .... .... = LG bit: Locally administered address (this is NOT the factory default)
	.... ...0 .... .... .... .... = IG bit: Individual address (unicast)
	Type: IPv6 (0x86dd)
	Stream index: 6
Layer IPV6
:	0110 .... = Version: 6
	.... 0000 0000 .... .... .... .... .... = Traffic Class: 0x00 (DSCP: CS0, ECN: Not-ECT)
	.... 0000 00.. .... .... .... .... .... = Differentiated Services Codepoint: Default (0)
	.... .... ..00 .... .... .... .... .... = Explicit Congestion Notification: Not ECN-Capable Transport (0)
	.... 0000 1001 0101 0000 0101 = Flow Label: 0x09505
	Payload Length: 88
	Next Header: ICMPv6 (58)
...
(many lines stripped)

@marten-seemann
Copy link
Collaborator

Yes, logging packets's __repr__ instead of __str__ to get nice and small message:

Captured packet without quic layer: <ICMPV6 Packet>

Sounds good to me!

@marten-seemann
Copy link
Collaborator

@koujaz Can you update the PR then? Please also check why the linter is unhappy.

@marten-seemann
Copy link
Collaborator

@larseggert Does this PR work for you?

@larseggert
Copy link
Contributor

Nope, this logs but still crashes:

Captured packet without quic layer: <ICMPV6 Packet>
Captured packet without quic layer: <ICMPV6 Packet>
Traceback (most recent call last):
  File "/Users/lars/Documents/Code/quic-interop-runner/./run.py", line 183, in <module>
    sys.exit(main())
             ~~~~^^
  File "/Users/lars/Documents/Code/quic-interop-runner/./run.py", line 179, in main
    ).run()
      ~~~^^
  File "/Users/lars/Documents/Code/quic-interop-runner/interop.py", line 553, in run
    status = self._run_testcase(server, client, testcase)
  File "/Users/lars/Documents/Code/quic-interop-runner/interop.py", line 369, in _run_testcase
    return self._run_test(server, client, None, test)[0]
           ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/lars/Documents/Code/quic-interop-runner/interop.py", line 469, in _run_test
    status = testcase.check()
  File "/Users/lars/Documents/Code/quic-interop-runner/testcases.py", line 1463, in check
    super().check()
    ~~~~~~~~~~~~~^^
  File "/Users/lars/Documents/Code/quic-interop-runner/testcases.py", line 1352, in check
    super().check()
    ~~~~~~~~~~~~~^^
  File "/Users/lars/Documents/Code/quic-interop-runner/testcases.py", line 1281, in check
    cur = self._path(p)
  File "/Users/lars/Documents/Code/quic-interop-runner/testcases.py", line 1258, in _path
    (TestCasePortRebinding._addr(p, "src"), int(getattr(p["udp"], "srcport"))),
                                                        ~^^^^^^^
  File "/opt/homebrew/lib/python3.13/site-packages/pyshark/packet/packet.py", line 52, in __getitem__
    raise KeyError('Layer does not exist in packet')
KeyError: 'Layer does not exist in packet'

@koujaz
Copy link
Author

koujaz commented Jan 29, 2025

#423 really looks like duplicate where just different test crashes test via different code path.
I'm OK w/ rejecting this PR and having just non-QUIC packets silently filtered out as implemented in #423.
(You can asses better that these packets are generally not of any use for the scenarios covered by this suite).

@koujaz
Copy link
Author

koujaz commented Feb 3, 2025

Updated PR to discard non-QUIC packets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants