feat(plugin-meetings): reachability- add detection of symmetric NAT #4156
Conversation
📝 WalkthroughWalkthroughThe update enhances the reachability modules by integrating NAT type detection. A new enumeration, Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it’s a critical failure. 🔧 ESLint
yarn install v1.22.22 (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
k-wasniowski
added
the
validated
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts (1)
2193-2194: Consider additional NAT type detection testsWhile the basic structure testing is comprehensive, consider adding specific tests that verify the NAT type detection logic and the handling of
natTypeUpdatedevents. This would improve test coverage for this new feature.packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts (2)
300-338: Consider preventing duplicate event emissionsThe NAT type detection logic is well-implemented, but the current code might emit the
natTypeUpdatedevent multiple times with the same value if multiple candidates confirm the symmetric NAT condition.Consider checking if the NAT type has changed before emitting the event:
private determineNatType(candidate: RTCIceCandidate) { this.srflxIceCandidates.push(candidate); if (this.srflxIceCandidates.length > 1) { const portsFound: {[key: string]: Set<number>} = {}; this.srflxIceCandidates.forEach((c) => { const key = `${c.address}:${c.relatedPort}`; if (!portsFound[key]) { portsFound[key] = new Set(); } portsFound[key].add(c.port); }); Object.entries(portsFound).forEach(([, ports]) => { if (ports.size > 1) { // Found candidates with the same address and relatedPort, but different ports + const previousNatType = this.result.natType; this.result.natType = NatType.SymmetricNat; + if (previousNatType !== NatType.SymmetricNat) { this.emit( { file: 'clusterReachability', function: 'determineNatType', }, Events.natTypeUpdated, { natType: NatType.SymmetricNat, } ); + } } }); } }
306-338: Add clarifying comment about relatedPortThe code correctly uses the
relatedPortproperty for NAT detection, but this terminology might not be immediately clear to all developers.Consider adding a clarifying comment about what
relatedPortrepresents:private determineNatType(candidate: RTCIceCandidate) { this.srflxIceCandidates.push(candidate); if (this.srflxIceCandidates.length > 1) { const portsFound: {[key: string]: Set<number>} = {}; + // Group candidates by address and relatedPort (the local/private interface port) + // In symmetric NAT, the same private endpoint maps to different public ports this.srflxIceCandidates.forEach((c) => { const key = `${c.address}:${c.relatedPort}`; if (!portsFound[key]) { portsFound[key] = new Set(); } portsFound[key].add(c.port); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts(6 hunks)packages/@webex/plugin-meetings/src/reachability/index.ts(5 hunks)packages/@webex/plugin-meetings/src/reachability/reachability.types.ts(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/reachability/clusterReachability.ts(12 hunks)packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts(24 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
packages/@webex/plugin-meetings/src/reachability/index.ts (1)
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts (3) (3)
Events(29:33)Events(35:35)NatTypeUpdatedEventData(25:27)
packages/@webex/plugin-meetings/test/unit/spec/reachability/clusterReachability.ts (1)
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts (5) (5)
Events(29:33)Events(35:35)ResultEventData(12:17)ClientMediaIpsUpdatedEventData(20:23)NatTypeUpdatedEventData(25:27)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test - Integration
🔇 Additional comments (22)
packages/@webex/plugin-meetings/src/reachability/reachability.types.ts (3)
10-13: Well-structured NAT type enumerationThe addition of the
NatTypeenum withUnknownandSymmetricNatvalues provides a clear, type-safe way to represent the NAT detection states. This is a clean implementation that allows for future expansion if additional NAT types need to be supported.
20-20: Appropriate addition of NAT type to cluster result typeAdding the
natTypeproperty to theClusterReachabilityResulttype ensures that NAT type information is properly tracked per cluster, which aligns well with the PR objective of detecting symmetric NAT.
36-36: Consistent inclusion of NAT type in metricsAdding the
natTypeproperty to theReachabilityMetricstype ensures that NAT information is available for analytics and reporting, which is important for monitoring network conditions across the user base.packages/@webex/plugin-meetings/src/reachability/index.ts (6)
26-26: Proper import of NatType enumThe import of the
NatTypeenum is correctly added to utilize the new type definition.
32-33: Import for new event data typeThe import of
NatTypeUpdatedEventDatais appropriately added to support handling the new NAT type update events.
310-310: Appropriate initialization of NAT type in metricsThe
natTypeproperty is correctly initialized toNatType.Unknownin the metrics object, providing a sensible default value.
326-328: Well-implemented NAT type update logicThe implementation only updates the
natTypewhen a non-unknown value is detected, which is a good approach to ensure that more specific information takes precedence. This handles the case where different clusters might detect different NAT types.
873-873: Proper initialization of NAT type in cluster resultsThe
natTypeproperty is correctly initialized toNatType.Unknownwhen setting up the initial cluster results. This ensures consistency with the type definition and provides a clear default state.
973-980: Well-implemented event listener for NAT type updatesThe event listener for
natTypeUpdatedfollows the same pattern as the existing event listeners, ensuring consistency in the codebase. It correctly updates the NAT type in the results object and persists the changes to storage.packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts (2)
552-553: Updated mock data with NAT typeThe addition of
natType: 'unknown'to mock cluster data ensures the tests remain compatible with the updated data structures.
588-589: Consistent NAT type in expected resultsThe expected results in assertions are properly updated to include the
natTypefield, ensuring the tests verify the complete result structure.packages/@webex/plugin-meetings/test/unit/spec/reachability/clusterReachability.ts (5)
12-14: Correct imports for new NAT-related typesThe necessary imports for
NatTypeUpdatedEventDataandNatTypeare properly added to support the updated testing code.
22-26: Updated event tracking for NAT type eventsThe
emittedEventsrecord is appropriately updated to trackNatTypeUpdatedEventDataevents, ensuring the test infrastructure can verify the emission of NAT type updates.
32-32: Updated event reset functionThe
resetEmittedEventsfunction is correctly updated to clear thenatTypeUpdatedevent array, ensuring a clean state between tests.
64-66: Added event listener for NAT type updatesThe event listener for
natTypeUpdatedevents is properly implemented, following the same pattern as the existing event listeners and storing the events for later verification.
462-498: Comprehensive test for symmetric NAT detectionThis new test case effectively verifies the core functionality of detecting symmetric NAT:
- It confirms no event is emitted with just one server reflexive candidate
- It verifies a
natTypeUpdatedevent is emitted when a second candidate with the same IP but different port is received- It checks that the final result correctly includes the detected NAT type
This is a well-constructed test that covers the key aspects of the new functionality.
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts (6)
9-9: LGTM: Import of NatType enumThe import is correctly added to include the new NatType enum alongside the existing ClusterReachabilityResult type.
25-27: LGTM: Well-designed event data typeThe NatTypeUpdatedEventData type follows the established pattern of other event data types in the file, with clear and concise structure.
32-32: LGTM: Event declarationThe new natTypeUpdated event is properly added to the Events constant with a descriptive comment.
49-49: LGTM: Storage for server reflexive candidatesGood choice to store server reflexive candidates for later analysis in the NAT type determination.
79-79: LGTM: Initializing natType propertyCorrectly initializing the natType to Unknown in the constructor as a good default state.
359-359: LGTM: Appropriate integration pointCalling
determineNatTypewhen server reflexive candidates are received is the perfect integration point for this feature.
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts
Outdated
Show resolved
Hide resolved
| Object.entries(portsFound).forEach(([, ports]) => { | ||
| if (ports.size > 1) { | ||
| // Found candidates with the same address and relatedPort, but different ports | ||
| this.result.natType = NatType.SymmetricNat; |
There was a problem hiding this comment.
we could also add support for other types of nat like full cone, right? (if we get multiple srflx candidates from different servers all with same port) It could be added in another PR...
…com/k-wasniowski/webex-js-sdk into feat-add-detection-of-symmetric-nat
…com/k-wasniowski/webex-js-sdk into feat-add-detection-of-symmetric-nat
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/@webex/plugin-meetings/test/unit/spec/reachability/clusterReachability.ts(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/@webex/plugin-meetings/test/unit/spec/reachability/clusterReachability.ts (1)
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts (5)
Events(29-33)Events(35-35)ResultEventData(12-17)ClientMediaIpsUpdatedEventData(20-23)NatTypeUpdatedEventData(25-27)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Packages
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (2)
packages/@webex/plugin-meetings/test/unit/spec/reachability/clusterReachability.ts (2)
22-25: LGTM! Event handling implementation for natTypeUpdated.The event handling for
natTypeUpdatedis well implemented, following the established pattern in the codebase with consistent typing, reset handling, and event listener setup.Also applies to: 32-32, 64-66
452-488: LGTM! Comprehensive test case for symmetric NAT detection.The test case correctly verifies the NAT detection logic by:
- Confirming no event is emitted with a single candidate
- Verifying the event is emitted when a second candidate with the same IP but different port is received
- Validating the correct NAT type value ('symmetric-nat') in the emitted event
- Ensuring the reachability results remain accurate after detection
This thoroughly tests the intended functionality of detecting symmetric NAT conditions.
packages/@webex/plugin-meetings/test/unit/spec/reachability/clusterReachability.ts
Show resolved
Hide resolved
COMPLETES #SPARK-637849
This pull request addresses
This PR introduces possibility of finding out if customer is behind a symmetric NAT during the reachability.
Change Type
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.