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

Fix Apple ECN dual stack behavior #35196

Merged
merged 11 commits into from
Aug 16, 2024
Merged

Conversation

martinduke
Copy link
Contributor

@martinduke martinduke commented Jul 15, 2024

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

@KBaichoo KBaichoo added the wait:review Waiting for (further) review label Jul 17, 2024
@KBaichoo
Copy link
Contributor

Test failure is a known flake that we're working on. @danzh2010 does this look good to you?

@danzh2010
Copy link
Contributor

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.

@KBaichoo
Copy link
Contributor

marked the comment unresolved

/wait

yanavlasov
yanavlasov previously approved these changes Jul 22, 2024
@yanavlasov yanavlasov enabled auto-merge (squash) July 22, 2024 23:58
@yanavlasov
Copy link
Contributor

@martinduke CI is stuck and I can not get it unstuck. Can you merge main to restart CI, please?

@soulxu
Copy link
Member

soulxu commented Jul 29, 2024

/wait for a merge main

auto-merge was automatically disabled July 29, 2024 14:10

Head branch was pushed to by a user without write access

@soulxu
Copy link
Member

soulxu commented Aug 1, 2024

@martinduke it seems you need to fix the DCO

@RyanTheOptimist
Copy link
Contributor

/wait
@martinduke it looks like your PR has a problem with DCO that you need to fix

Signed-off-by: Martin Duke <[email protected]>
Signed-off-by: Martin Duke <[email protected]>
@martinduke
Copy link
Contributor Author

/retest

@martinduke martinduke marked this pull request as ready for review August 15, 2024 16:05
@martinduke
Copy link
Contributor Author

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]>
@RyanTheOptimist
Copy link
Contributor

/wait

Signed-off-by: Martin Duke <[email protected]>
@martinduke
Copy link
Contributor Author

/retest

1 similar comment
@abeyad
Copy link
Contributor

abeyad commented Aug 15, 2024

/retest

@martinduke
Copy link
Contributor Author

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.

Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a 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!

Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a 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?

@martinduke martinduke changed the title Add ECN dual stack test Fix Apple ECN dual stack behavior Aug 15, 2024
@RyanTheOptimist RyanTheOptimist merged commit 9286760 into envoyproxy:main Aug 16, 2024
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wait:review Waiting for (further) review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants