Do not advertise temporary/deprecated IPv6 addresses by minimal mDNS on Linux #22009
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…s is temporary/deprecated/...
pullapprove
bot
requested review from
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
erjiaqing,
franck-apple,
gjc13,
harimau-qirex,
harsha-rajendran,
hawk248,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
kghost,
kpschoedel,
lazarkov,
LuDuda,
mrjerryjohns,
msandstedt and
robszewczyk
pullapprove
bot
requested review from
saurabhst,
selissia,
tcarmelveilleux,
tecimovic,
turon,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21,
yufengwangca and
yunhanw-google
tcarmelveilleux
changed the title
Comment on lines
+927
to
+940
| if (mCurAddr->ifa_flags & IFA_F_OPTIMISTIC) | ||
| { | ||
| flags.Set(InterfaceAddressIterator::Flags::kNotFinal); | ||
| } | ||
|
|
||
| if (mCurAddr->ifa_flags & IFA_F_DADFAILED) | ||
| { | ||
| flags.Set(InterfaceAddressIterator::Flags::kNotFinal); | ||
| } | ||
|
|
||
| if (mCurAddr->ifa_flags & IFA_F_TENTATIVE) | ||
| { | ||
| flags.Set(InterfaceAddressIterator::Flags::kNotFinal); | ||
| } |
There was a problem hiding this comment.
Suggest to ifdef those flasg separately in case some platforms like Android don't have all of them
There was a problem hiding this comment.
I would expect CI to detect this. I believe for android we care about IFA_F_OPTIMISTIC and DADFAILED, however the flags kNotFinal and such can remain the same as if a platform does not have that concept, the flags are not set. This is what currently happen for all non-linux
| { | ||
| if (it.GetFlags().HasAny(InterfaceAddressIterator::Flags::kNotFinal, InterfaceAddressIterator::Flags::kTemporary, |
There was a problem hiding this comment.
Add comment here and in IPv6 that we will not advertise addresses that are not current and stable
tcarmelveilleux
approved these changes
Aug 18, 2022
| @@ -390,6 +390,13 @@ class InterfaceIterator | |||
| class DLL_EXPORT InterfaceAddressIterator | |||
| { | |||
| public: | |||
| enum class Flags : uint8_t | |||
| { | |||
| kNotFinal = (1 << 0), // Not yet valid: Optimistic/DAD failed/tentative | |||
There was a problem hiding this comment.
This seems like it's exposing Linux flags in an API that is mean to work across a range of platforms that may not have these granular flags.
Perhaps start with just one (since all of the logic below seems to be treating all of these the same), and expand in the future as needed?
There was a problem hiding this comment.
The flags here correspond to RFC 8981 concepts, so I think they're universally meaningful (even if not exposed on all platforms).
There was a problem hiding this comment.
I tried to make it somewhat conceptual by saying not final instead of providing linux flags. Beyond that, RFC concepts should be sufficient and we do not require all platforms to support them.
I was unsure if we can simplify more. We could in theory just have a kDoNotUse flag and let each platform pick what the rules are, but that seems a bit rough. We can go that route as a followup.
|
PR #22009: Size comparison from 51d2c95 to 5b5dcac Increases (25 builds for bl602, cc13x2_26x2, efr32, esp32, linux, psoc6, telink)
Decreases (7 builds for cc13x2_26x2, nrfconnect, psoc6)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, telink)
|
|
Converting to draft: getting IFA_F flags is a bit more involved it seems and since it is linux-specific it does not seem as well documented. |
|
Closing as the changes to support this are likely larger and original PR and review likely does not apply. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add support for linux address setting fetch - figure out if an address is temporary/deprecated/...
Problem
Linux often has deprecated IPv6 addresses when a OTBR advertises such routes. The longer they run, the more addresses would show up and this causes an exponential growth in IP addresses that are used and reported by minmdnds.
This makes passing TC-RR-1.1 very hard and highly depending on the system running chip-tool.
Previously, with 15 controllers, the announcement phase was >8 seconds of blocking network operations, which impacted test timing enough to flake some testing.
Change overview
Allow interface address iterators to set flags for specific IP addresses for their status. Specifically for linux implemented detection for temporary/deprecated/non-final addresses.
Use such info during minmdns processing.
This improves/helps implement #21736
Testing
Manual testing on a system with multiple deprecated addresses, minmdns behaviour significantly less spammy.