chore(ME-3976): add allowed networks messages and stats #48
Conversation
btoonk
requested review from
adrianosela,
atoonk,
pedroandrade,
th3wingman and
waltzofpearls
|
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. 🔧 buf (1.47.2)common/messages.protoConfig file YAML parsing error: yaml: unmarshal errors: connector/connector.protoConfig file YAML parsing error: yaml: unmarshal errors: WalkthroughThe pull request introduces modifications to the protocol buffer definitions in Changes
Sequence DiagramsequenceDiagram
participant Client
participant Server
Client->>Server: ControlStreamRequest with AllowedNetworks
Server-->>Client: ControlStreamResponse with allowed_networks
Note over Server: Process and respond with allowed network configurations
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
CodeRabbit Configuration File (
|
![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 (1)
connector/connector.proto (1)
294-304: LGTM! Well-structured message hierarchyThe AllowedNetworks message hierarchy is logically organized using appropriate data structures. Consider adding comments to document the purpose of each message type and field.
Add documentation comments like this:
+// AllowedNetworks represents the network access configuration for devices message AllowedNetworks { map<string, AllowedNetworksSocketConfig> devices = 1; } +// AllowedNetworksSocketConfig represents the socket-specific network configuration for a device message AllowedNetworksSocketConfig { map<string, AllowedNetworksSubnets> sockets = 1; } +// AllowedNetworksSubnets represents the list of subnets allowed for a specific socket message AllowedNetworksSubnets { repeated string subnets = 1; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (12)
common/messages.pb.gois excluded by!**/*.pb.goconnector/connector.pb.gois excluded by!**/*.pb.gogen/kotlin/border0/common/v1/NetworkDeviceStatsMessageKt.ktis excluded by!**/gen/**gen/kotlin/border0/common/v1/WireGuardPeerKt.ktis excluded by!**/gen/**gen/kotlin/border0/v1/AllowedNetworksKt.ktis excluded by!**/gen/**gen/kotlin/border0/v1/AllowedNetworksSocketConfigKt.ktis excluded by!**/gen/**gen/kotlin/border0/v1/AllowedNetworksSubnetsKt.ktis excluded by!**/gen/**gen/kotlin/border0/v1/ControlStreamRequestKt.ktis excluded by!**/gen/**gen/kotlin/border0/v1/ControlStreamResponseKt.ktis excluded by!**/gen/**gen/kotlin/border0/v1/StatsKt.ktis excluded by!**/gen/**gen/swift/connector.pb.swiftis excluded by!**/gen/**gen/swift/messages.pb.swiftis excluded by!**/gen/**
📒 Files selected for processing (2)
common/messages.proto(1 hunks)connector/connector.proto(3 hunks)
🔇 Additional comments (4)
common/messages.proto (1)
33-33: LGTM! Well-structured field additionThe optional socket_id field is properly integrated into NetworkDeviceStatsMessage, maintaining backward compatibility and following protobuf best practices.
connector/connector.proto (3)
35-35: LGTM! Clean type replacementThe stats field type change maintains backward compatibility while introducing the new Stats message structure.
56-56: LGTM! Well-placed additionThe allowed_networks field is properly integrated into ControlStreamResponse, following the PR objectives.
306-308: LGTM! Well-designed stats aggregationThe Stats message appropriately uses a repeated field to collect multiple network device statistics, integrating well with the socket_id addition in NetworkDeviceStatsMessage.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
connector/connector.proto (2)
295-305: Add documentation for the network configuration hierarchy.The new message types create a well-structured hierarchy for network configurations, but documentation would help clarify:
- The purpose and relationship between devices, sockets, and subnets
- Expected format and validation rules for subnet strings
- Examples of typical configurations
Consider adding comments like:
+ // AllowedNetworks represents the network access configuration for multiple devices message AllowedNetworks { + // Map of device IDs to their socket configurations map<string, AllowedNetworksSocketConfig> devices = 1; } + // AllowedNetworksSocketConfig represents the socket-level network configuration for a device message AllowedNetworksSocketConfig { + // Map of socket IDs to their allowed subnets map<string, AllowedNetworksSubnets> sockets = 1; } + // AllowedNetworksSubnets represents the allowed subnets for a socket message AllowedNetworksSubnets { + // List of allowed subnets in CIDR notation (e.g., "192.168.1.0/24") repeated string subnets = 1; }
307-309: Consider a more generic field name for future extensibility.The field name
network_device_statsis very specific. If this message might need to support other types of stats in the future, consider using a more generic name likestatsordevice_stats.message Stats { - repeated border0.common.v1.StatsMessage network_device_stats = 1; + repeated border0.common.v1.StatsMessage stats = 1; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
common/messages.pb.gois excluded by!**/*.pb.goconnector/connector.pb.gois excluded by!**/*.pb.gogen/kotlin/border0/common/v1/NetworkDeviceStatsMessageKt.ktis excluded by!**/gen/**gen/kotlin/border0/common/v1/WireGuardPeerKt.ktis excluded by!**/gen/**gen/kotlin/border0/v1/AllowedNetworksKt.ktis excluded by!**/gen/**gen/kotlin/border0/v1/AllowedNetworksSocketConfigKt.ktis excluded by!**/gen/**gen/kotlin/border0/v1/AllowedNetworksSubnetsKt.ktis excluded by!**/gen/**gen/kotlin/border0/v1/ControlStreamRequestKt.ktis excluded by!**/gen/**gen/kotlin/border0/v1/StatsKt.ktis excluded by!**/gen/**gen/swift/connector.pb.swiftis excluded by!**/gen/**gen/swift/messages.pb.swiftis excluded by!**/gen/**
📒 Files selected for processing (2)
common/messages.proto(1 hunks)connector/connector.proto(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- common/messages.proto
🔇 Additional comments (2)
connector/connector.proto (2)
57-57: LGTM! Field addition follows proper sequencing.The
allowed_networksfield is well-placed within theControlStreamResponsemessage.
36-36: Verify the coexistence of both stats fields.The addition of
combined_statsalongside the existingstatsfield suggests a transition period. Consider:
- Adding documentation to clarify the relationship between these fields
- Whether both fields should be used simultaneously
✅ Verification successful
The coexistence of both stats fields is intentional and well-designed
The
combined_statsfield (typeStats) is designed to handle multiple stats messages, while the originalstatsfield handles single messages. This is a valid pattern where:
stats(field 17): SingleStatsMessagefor individual statscombined_stats(field 18): Collection of multiple stats messages via the newStatswrapper🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage patterns of both stats fields rg -A 5 "stats.*combined_stats|combined_stats.*stats"Length of output: 55
Script:
#!/bin/bash # Search for Stats message definition and its usage ast-grep --pattern 'message Stats { $$$ }' # Search for any comments or documentation about stats fields rg -B 2 -A 2 '(stats|combined_stats).*=.*(\d+);' # Search for imports or references to stats messages rg -l "Stats|StatsMessage"Length of output: 8840
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
common/messages.proto (2)
34-34: Document the purpose of socket_id field.Please add a comment explaining:
- The purpose of the
socket_idfield- Its relationship with other fields in the message
- When it's expected to be set/unset
uint64 packets_in = 4; uint64 packets_out = 5; - optional string socket_id = 6; + // Unique identifier for the socket connection associated with these stats. + // Set when stats are for a specific socket, unset for aggregate device stats. + optional string socket_id = 6;
37-38: Enhance CombinedNetworkDeviceStatsMessage with metadata.Consider adding metadata fields to make the combined stats more useful:
- Timestamp for when the stats were aggregated
- Source/purpose identifier
- Documentation explaining when to use combined vs individual stats
message CombinedNetworkDeviceStatsMessage { + // Timestamp when these stats were aggregated + google.protobuf.Timestamp timestamp = 1; + // Identifier for the source/purpose of these combined stats + string source = 2; + // Collection of network device stats repeated NetworkDeviceStatsMessage stats = 1; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
common/messages.pb.gois excluded by!**/*.pb.gogen/kotlin/border0/common/v1/NetworkDeviceStatsMessageKt.ktis excluded by!**/gen/**gen/kotlin/border0/common/v1/StatsMessageKt.ktis excluded by!**/gen/**gen/kotlin/border0/common/v1/WireGuardPeerKt.ktis excluded by!**/gen/**gen/kotlin/border0/v1/AllowedNetworksKt.ktis excluded by!**/gen/**gen/kotlin/border0/v1/AllowedNetworksSocketConfigKt.ktis excluded by!**/gen/**gen/kotlin/border0/v1/AllowedNetworksSubnetsKt.ktis excluded by!**/gen/**gen/kotlin/border0/v1/StatsKt.ktis excluded by!**/gen/**gen/swift/messages.pb.swiftis excluded by!**/gen/**
📒 Files selected for processing (2)
common/messages.proto(2 hunks)connector/connector.proto(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- connector/connector.proto
🔇 Additional comments (1)
common/messages.proto (1)
24-24: Consider using specific message types instead of combining stats.Based on previous discussions (see PR #41), the
StatsMessagewas designed to have different message types per type. Consider using specific message types (e.g.,SocketTrafficStatsMessage) instead of combining stats into a single message.Run this script to verify if there are any other specific stats message types that could be reused:
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
connector/connector.proto (1)
294-305: Add field documentation for the new message types.While the structure is clear, the new message types lack documentation explaining their purpose and the expected content of their fields.
Consider adding documentation:
message AllowedNetworks { + // Map of device IDs to their socket configurations map<string, AllowedNetworksSocketConfig> devices = 1; } message AllowedNetworksSocketConfig { + // Map of socket IDs to their allowed subnets map<string, AllowedNetworksSubnets> sockets = 1; } message AllowedNetworksSubnets { + // List of CIDR notation subnets allowed for this socket repeated string subnets = 1; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
common/messages.pb.gois excluded by!**/*.pb.gogen/kotlin/border0/common/v1/StatsMessageKt.ktis excluded by!**/gen/**gen/kotlin/border0/common/v1/WireGuardPeerKt.ktis excluded by!**/gen/**gen/kotlin/border0/v1/AllowedNetworksKt.ktis excluded by!**/gen/**gen/kotlin/border0/v1/AllowedNetworksSocketConfigKt.ktis excluded by!**/gen/**gen/kotlin/border0/v1/AllowedNetworksSubnetsKt.ktis excluded by!**/gen/**gen/kotlin/border0/v1/StatsKt.ktis excluded by!**/gen/**gen/swift/messages.pb.swiftis excluded by!**/gen/**
📒 Files selected for processing (2)
common/messages.proto(2 hunks)connector/connector.proto(2 hunks)
🔇 Additional comments (3)
common/messages.proto (2)
24-24: LGTM! Field addition follows existing pattern.The new field
sockets_statsin theStatsMessageoneof follows the established pattern and naming convention.
36-43: LGTM! Stats structure is consistent.The
SocketsStatsMessagefollows the same structure asNetworkDeviceStatsMessage, maintaining consistency in the codebase.connector/connector.proto (1)
56-56: LGTM! Field addition matches request structure.The new field
allowed_networksinControlStreamResponsealigns with field number 17 inControlStreamRequest, maintaining symmetry.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
common/messages.proto (1)
24-24: Consider performance implications of socket stats collection.The new socket stats structure allows tracking individual socket metrics. For systems with a large number of sockets:
- Consider implementing sampling or aggregation strategies.
- Document recommended collection frequency.
- Consider adding fields for configuration (e.g., sampling rate, aggregation period).
Also applies to: 36-43, 46-48
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
common/messages.pb.gois excluded by!**/*.pb.gogen/kotlin/border0/common/v1/CombinedNetworkDeviceStatsMessageKt.ktis excluded by!**/gen/**gen/kotlin/border0/common/v1/SocketStatsMessageKt.ktis excluded by!**/gen/**gen/kotlin/border0/common/v1/SocketsStatsMessageKt.ktis excluded by!**/gen/**gen/kotlin/border0/common/v1/StatsMessageKt.ktis excluded by!**/gen/**gen/kotlin/border0/common/v1/WireGuardPeerKt.ktis excluded by!**/gen/**gen/kotlin/border0/v1/AllowedNetworksKt.ktis excluded by!**/gen/**gen/kotlin/border0/v1/AllowedNetworksSocketConfigKt.ktis excluded by!**/gen/**gen/kotlin/border0/v1/AllowedNetworksSubnetsKt.ktis excluded by!**/gen/**gen/kotlin/border0/v1/StatsKt.ktis excluded by!**/gen/**gen/swift/messages.pb.swiftis excluded by!**/gen/**
📒 Files selected for processing (2)
common/messages.proto(2 hunks)connector/connector.proto(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- connector/connector.proto
🔇 Additional comments (3)
common/messages.proto (3)
24-24: LGTM! Field addition follows the discussed pattern.The addition of
sockets_statsfield aligns with the existing pattern of having different message types per stats type, as discussed in PR #41.
46-48: LGTM! Clear container message structure.The message structure follows protobuf best practices with a clear plural name for a collection of stats.
36-43: Consider adding validation for socket_id format.The
socket_idfield is defined as a string without any format constraints. Consider documenting the expected format in comments and adding validation in the implementation.Run this script to check if there's any existing socket_id validation or format documentation:
Summary by CodeRabbit
New Features
Refactor