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

Reintroduce: Fix for ipv6 link local with scope #10040

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

Conversation

gabekassel
Copy link

This reintroduces commit c1abc7f. Commit was reverted in #9375 and #9376 for a Google-internal compilation issue. Re-raising this commit as it fixes a critical issue for our use case, and prods Google team whether this is still an issue internally.

Please also advise if this should be back ported to any specific release branches if approved.

@ejona86
Copy link
Member

ejona86 commented Apr 17, 2023

Yes, this is still an issue. The internal issue is very ugly, and essentially means we should avoid Guava's net package. I did look to get alternative solutions here, because I still feel like there's other issues with this flow, but it was an android-specific issue which made testing to investigate details too onerous at the time.

@ejona86
Copy link
Member

ejona86 commented Apr 17, 2023

CC @temawi, @YifeiZhuang

@gabekassel
Copy link
Author

@ejona86 would it be possible to have an overload of configureTlsExtensions() that uses a build flag to enables us to use the isInetAddress check and an overload of that method that passes false by default? Hopefully this would make the current internal code follow its original path while whoever needs this check could just enforce it.

Does the codebase have patterns for flags like that we could follow?

@ejona86
Copy link
Member

ejona86 commented Apr 17, 2023

The internal problem is a build-time error, so a boolean is not sufficient. It might be possible to inject the reference to Guava instead, but that would have its own difficulties and short-term. Easiest solutions are to avoid Guava or to catch this problem earlier before the code gets to this point (which could allow using different libraries). To avoid Guava, easiest approach may be to just catch the exception from SNIHostName.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 23, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@gabekassel
Copy link
Author

@ejona86 we've gone with the approach of catching IllegalArgumentException to avoid the Guava import. Please let us know if there's anything else we should do and if this is acceptable.

@gabekassel gabekassel force-pushed the link-local-fix branch 2 times, most recently from 833b848 to 3858559 Compare May 24, 2023 16:10
@gabekassel
Copy link
Author

@ejona86 can you PTAL at this updated PR?

@gabekassel
Copy link
Author

@ejona86 any update?

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.

None yet

2 participants