-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Fix Apple ECN dual stack behavior #35196
Conversation
Test failure is a known flake that we're working on. @danzh2010 does this look good to you? |
I think this PR need more comments. I don't know how to mark resolved review comment unresolved. |
marked the comment unresolved /wait |
@martinduke CI is stuck and I can not get it unstuck. Can you merge main to restart CI, please? |
/wait for a merge main |
Head branch was pushed to by a user without write access
@martinduke it seems you need to fix the DCO |
/wait |
Signed-off-by: Martin Duke <[email protected]>
Signed-off-by: Martin Duke <[email protected]>
/retest |
Marking ready for review, not because it's ready for review, but in the hope that this triggers the skipped iOS tests. |
Signed-off-by: Martin Duke <[email protected]>
/wait |
Signed-off-by: Martin Duke <[email protected]>
Signed-off-by: Martin Duke <[email protected]>
/retest |
1 similar comment
/retest |
Thanks to @abeyad, we were able to confirm this works correctly for Apple. Modulo the weird CI failures (apparently unrelated to the PR?) this is ready for review, and ultimately merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting to the bottom of this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the PR description to make clear that this PR includes a behavior change, not just a new test?
Commit Message: Check the status of ECN in a DualStack socket
Additional Description: Linux dual-stack sockets require IP_PROTO, IP_RECVTOS due to report ECN on incoming v4 sockets. Apple returns an error; IPV6_RECVTCLASS is sufficient for both IP versions.
Risk Level: Low
Testing: New unit tests to verify the sockopt is set, and ECN is reported on dual stack sockets
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A