feat(ME-3477): add private network flag and messages #25
Conversation
WalkthroughThe pull request introduces changes across multiple files, primarily focusing on the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
btoonk
requested review from
adrianosela,
atoonk,
pedroandrade,
th3wingman and
waltzofpearls
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
Makefile (1)
Line range hint
1-20: Suggestions for Makefile improvementsWhile the current Makefile structure is functional, consider the following improvements for better maintainability and adherence to Makefile best practices:
- Add a
.PHONYdeclaration for non-file targets to avoid conflicts with similarly named files and improve performance:.PHONY: all docker-run docker-build
- Consider adding a
cleantarget to remove generated files:clean: rm -rf $(COMPONENTS:=/*.pb.go) .PHONY: clean
- For better organization, consider grouping related variables together, e.g., all version-related variables at the top of the file.
These suggestions will enhance the Makefile's robustness and maintainability.
connector/connector.proto (1)
Line range hint
28-31: Correct misspellings of "Certificate" in message and field namesThere are multiple instances where "Certificate" is misspelled as "Certifcate" in message and field names:
- At line 28:
SshCertifcateSignRequest ssh_certificate_sign_request = 10;should beSshCertificateSignRequest ssh_certificate_sign_request = 10;- At line 30:
CertifcateSignRequest certifcate_sign_request = 12;should beCertificateSignRequest certificate_sign_request = 12;- At line 64:
message SshCertifcateSignRequest {should bemessage SshCertificateSignRequest {- At line 68:
message SshCertifcateSignResponse {should bemessage SshCertificateSignResponse {- At line 78:
message CertifcateSignRequest {should bemessage CertificateSignRequest {- At line 83:
message CertifcateSignResponse {should bemessage CertificateSignResponse {Apply these diffs to correct the misspellings:
// In ControlStreamRequest SshCertifcateSignRequest ssh_certificate_sign_request = 10; UploadRecording upload_recording = 11; - CertifcateSignRequest certifcate_sign_request = 12; + CertificateSignRequest certificate_sign_request = 12; // Message definitions -message SshCertifcateSignRequest { +message SshCertificateSignRequest { string request_id = 1; string socket_id = 2; string session_key = 3; string user_email = 4; bytes public_key = 5; bytes ticket = 6; } -message SshCertifcateSignResponse { +message SshCertificateSignResponse { string request_id = 1; bytes certificate = 2; } -message CertifcateSignRequest { +message CertificateSignRequest { string request_id = 1; bytes certificate_signing_request = 2; } -message CertifcateSignResponse { +message CertificateSignResponse { string request_id = 1; bytes certificate = 2; }Also, update any references to these messages and field names throughout the file.
Also applies to: 64-68, 78-79, 83-84
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
connector/connector.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (3)
- Makefile (1 hunks)
- connector/connector.proto (5 hunks)
- generate.sh (1 hunks)
🔇 Additional comments (4)
generate.sh (1)
3-3:⚠️ Potential issueVerify directory structure and resolve path inconsistencies
The change from
-I/app/prototo--proto_path=/app/protossuggests a directory structure change. However, there's an inconsistency in the paths used within the command:
- The new proto path is set to
/app/protos- The output paths and wildcard still use
/app/protoThis inconsistency could lead to errors in the protocol buffer compilation process.
Please verify the correct directory structure and update the paths consistently. If
/app/protosis the new correct path, consider updating the command as follows:-protoc -I/app/proto --proto_path=/app/protos --go_out=paths=source_relative:/app/proto --go-grpc_out=paths=source_relative:/app/proto /app/proto/*.proto +protoc --proto_path=/app/protos --go_out=paths=source_relative:/app/protos --go-grpc_out=paths=source_relative:/app/protos /app/protos/*.protoTo verify the directory structure, you can run the following script:
This script will help determine which directory actually contains the .proto files and ensure that the paths in the
protoccommand are correct.Makefile (1)
15-15: Expanded Docker volume mounts for broader file accessThe changes to the
docker-runtarget expand the volume mounts in the Docker run command. This modification allows for broader access to project files within the Docker container while maintaining the specific component directory mount.Pros:
- Provides access to the entire project directory, which may be necessary for certain build or generation processes.
- Maintains backwards compatibility by keeping the specific component directory mount.
Considerations:
- Ensure that mounting the entire project directory doesn't expose sensitive information or unnecessary files to the Docker container.
- Verify that the expanded access doesn't introduce any unintended side effects in the build process.
To ensure the changes don't introduce any issues, please run the following verification:
connector/connector.proto (2)
12-12: Import statement fordevice.protoadded correctlyThe import statement for
device/device.protois correctly added to include the device-related message definitions required for the new fields.
149-149: Addition ofprivate_network_enabledfield toConnectorConfigThe boolean field
private_network_enabledis correctly added to theConnectorConfigmessage.
connector/connector.proto
Outdated
|
|
||
| message DiscoveryDetailsMessage { | ||
| bool discoverable = 1; // whether the peer should be returned along with the networks its in | ||
| string endpoint_public_udp4 = 2; // the endpoint (IPv4 + port) to send packets to for the peer | ||
| string endpoint_public_udp6 = 3; // the endpoint (IPv6 + port) to send packets to for the peer | ||
| string public_key = 4; // the public key of the peer | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider renaming fields in DiscoveryDetailsMessage for clarity
In the DiscoveryDetailsMessage, consider renaming the fields endpoint_public_udp4 and endpoint_public_udp6 to public_udp4_endpoint and public_udp6_endpoint for better clarity and consistency with public_key.
Apply this diff to rename the fields:
message DiscoveryDetailsMessage {
bool discoverable = 1; // whether the peer should be returned along with the networks it's in
- string endpoint_public_udp4 = 2; // the endpoint (IPv4 + port) to send packets to for the peer
- string endpoint_public_udp6 = 3; // the endpoint (IPv6 + port) to send packets to for the peer
+ string public_udp4_endpoint = 2; // the endpoint (IPv4 + port) to send packets to for the peer
+ string public_udp6_endpoint = 3; // the endpoint (IPv6 + port) to send packets to for the peer
string public_key = 4; // the public key of the peer
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| message DiscoveryDetailsMessage { | |
| bool discoverable = 1; // whether the peer should be returned along with the networks its in | |
| string endpoint_public_udp4 = 2; // the endpoint (IPv4 + port) to send packets to for the peer | |
| string endpoint_public_udp6 = 3; // the endpoint (IPv6 + port) to send packets to for the peer | |
| string public_key = 4; // the public key of the peer | |
| } | |
| message DiscoveryDetailsMessage { | |
| bool discoverable = 1; // whether the peer should be returned along with the networks its in | |
| string public_udp4_endpoint = 2; // the endpoint (IPv4 + port) to send packets to for the peer | |
| string public_udp6_endpoint = 3; // the endpoint (IPv6 + port) to send packets to for the peer | |
| string public_key = 4; // the public key of the peer | |
| } |
connector/connector.proto
Outdated
| UploadRecording upload_recording = 11; | ||
| CertifcateSignRequest certifcate_sign_request = 12; | ||
| SessionEvent session_event = 13; | ||
| DiscoveryDetailsMessage discoveryDetailsMessage = 14; |
There was a problem hiding this comment.
Use consistent naming convention for field discoveryDetailsMessage
The field discoveryDetailsMessage in ControlStreamRequest uses camelCase. Other fields in the message use snake_case. For consistency, consider renaming it to discovery_details_message.
Apply this diff to fix the inconsistency:
SessionEvent session_event = 13;
- DiscoveryDetailsMessage discoveryDetailsMessage = 14;
+ DiscoveryDetailsMessage discovery_details_message = 14;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DiscoveryDetailsMessage discoveryDetailsMessage = 14; | |
| SessionEvent session_event = 13; | |
| DiscoveryDetailsMessage discovery_details_message = 14; |
There was a problem hiding this comment.
This is a good comment.
There was a problem hiding this comment.
@adrianosela, glad to hear that you found my comment helpful!
(^_^)/
| DiscoveryDetailsMessage discoveryDetailsMessage = 14; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Correct misspelling of message name ControlStreamReponse
The message name ControlStreamReponse is misspelled. It should be ControlStreamResponse.
Apply this diff to fix the typo:
-message ControlStreamReponse {
+message ControlStreamResponse {
oneof RequestType {
Init init = 1;
ConnectorConfig connector_config = 2;Also, update any references to this message throughout the file.
Committable suggestion was skipped due to low confidence.
connector/connector.proto
Outdated
| border0.device.v1.NetworkStateMessage network_state_message = 12; | ||
| border0.device.v1.PeerOnlineMessage peerOnlineMessage = 13; | ||
| border0.device.v1.PeerOfflineMessage peerOfflineMessage = 14; | ||
| border0.device.v1.DisconnectMessage disconnectMessage = 15; |
There was a problem hiding this comment.
Ensure consistent naming conventions for field names
Fields peerOnlineMessage, peerOfflineMessage, and disconnectMessage in ControlStreamResponse use camelCase, while other fields use snake_case. For consistency, consider renaming them to peer_online_message, peer_offline_message, and disconnect_message.
Apply this diff to fix the inconsistency:
CertifcateSignResponse certifcate_sign_response = 11;
- border0.device.v1.NetworkStateMessage network_state_message = 12;
- border0.device.v1.PeerOnlineMessage peerOnlineMessage = 13;
- border0.device.v1.PeerOfflineMessage peerOfflineMessage = 14;
- border0.device.v1.DisconnectMessage disconnectMessage = 15;
+ border0.device.v1.NetworkStateMessage network_state_message = 12;
+ border0.device.v1.PeerOnlineMessage peer_online_message = 13;
+ border0.device.v1.PeerOfflineMessage peer_offline_message = 14;
+ border0.device.v1.DisconnectMessage disconnect_message = 15;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| border0.device.v1.NetworkStateMessage network_state_message = 12; | |
| border0.device.v1.PeerOnlineMessage peerOnlineMessage = 13; | |
| border0.device.v1.PeerOfflineMessage peerOfflineMessage = 14; | |
| border0.device.v1.DisconnectMessage disconnectMessage = 15; | |
| border0.device.v1.NetworkStateMessage network_state_message = 12; | |
| border0.device.v1.PeerOnlineMessage peer_online_message = 13; | |
| border0.device.v1.PeerOfflineMessage peer_offline_message = 14; | |
| border0.device.v1.DisconnectMessage disconnect_message = 15; |
There was a problem hiding this comment.
@adrianosela, thank you! I'm glad you found my comment helpful.
(_/)
(^_^)
adrianosela
left a comment
adrianosela
left a comment
There was a problem hiding this comment.
I think we should make a new package for the device types and import it in both the connector and device packages. Otherwise its too easy to forget that connector imports it and accidentally make breaking changes? Maybe not since you would see the diff -- but i think its better.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
connector/connector.proto (1)
Line range hint
215-241: Correct misspelling of 'Certificate' in message and field namesThe term "Certificate" is misspelled as "Certifcate" in several message and field names. Please correct the spelling in the following instances:
SshCertifcateSignRequest→SshCertificateSignRequestSshCertifcateSignResponse→SshCertificateSignResponseCertifcateSignRequest→CertificateSignRequestCertifcateSignResponse→CertificateSignResponse- Field names such as
ssh_certificate_sign_request,ssh_certificate_sign_response,certifcate_sign_request, andcertifcate_sign_responseshould also be updated accordingly.Apply this diff to fix the typos and update references:
message ControlStreamRequest { oneof RequestType { Config config = 1; border0.common.v1.HeartbeatMessage heartbeat = 2; PluginDiscoveryResults plugin_discovery_results = 3; TunnelCertificateSignRequest tunnel_certificate_sign_request = 5; Log log = 6; ConnectorMetadata metadata = 7; AuthorizeRequest authorize = 8; SessionUpdateRequest session_update = 9; - SshCertifcateSignRequest ssh_certificate_sign_request = 10; + SshCertificateSignRequest ssh_certificate_sign_request = 10; UploadRecording upload_recording = 11; - CertifcateSignRequest certifcate_sign_request = 12; + CertificateSignRequest certificate_sign_request = 12; SessionEvent session_event = 13; border0.common.v1.DiscoveryDetailsMessage discovery_details = 14; } } message ControlStreamResponse { oneof RequestType { Init init = 1; ConnectorConfig connector_config = 2; UpdateConfig update_config = 3; border0.common.v1.HeartbeatMessage heartbeat = 4; TunnelCertificateSignResponse tunnel_certificate_sign_response = 5; Discover discover = 6; Stop stop = 7; Disconnect disconnect = 8; AuthorizeResponse authorize = 9; - SshCertifcateSignResponse ssh_certificate_sign_response = 10; + SshCertificateSignResponse ssh_certificate_sign_response = 10; - CertifcateSignResponse certifcate_sign_response = 11; + CertificateSignResponse certificate_sign_response = 11; border0.common.v1.NetworkStateMessage network_state = 12; border0.common.v1.PeerOnlineMessage peer_online = 13; border0.common.v1.PeerOfflineMessage peer_offline = 14; } } -message SshCertifcateSignRequest { +message SshCertificateSignRequest { string request_id = 1; string socket_id = 2; string session_key = 3; string user_email = 4; bytes public_key = 5; bytes ticket = 6; } -message SshCertifcateSignResponse { +message SshCertificateSignResponse { string request_id = 1; bytes certificate = 2; } -message CertifcateSignRequest { +message CertificateSignRequest { string request_id = 1; bytes certificate_signing_request = 2; } -message CertifcateSignResponse { +message CertificateSignResponse { string request_id = 1; bytes certificate = 2; }This correction ensures consistency and prevents potential confusion or errors arising from the misspelled term.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
connector/connector.pb.gois excluded by!**/*.pb.goconnector/connector_grpc.pb.gois excluded by!**/*.pb.godevice/device.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (4)
- Makefile (1 hunks)
- connector/connector.proto (4 hunks)
- device/device.proto (2 hunks)
- generate.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile
- generate.sh
🔇 Additional comments (15)
device/device.proto (7)
10-11: Appropriate import of common messages.The addition of
import "common/messages.proto";is appropriate to utilize shared message definitions, enhancing code modularity.
20-20: Field name updated to snake_case in DeviceToServerMessage.Renaming
authChallengeSolutionMessagetoauth_challenge_solutionimproves consistency with protobuf naming conventions.
21-21: Reference to commonDiscoveryDetailsMessageupdated.Updating to
border0.common.v1.DiscoveryDetailsMessagecorrectly references the common message definition.
22-22: Reference to commonHeartbeatMessageupdated.Changing to
border0.common.v1.HeartbeatMessageensures consistent use of the common heartbeat message.
29-29: Field name updated to snake_case in ServerToDeviceMessage.Renaming
authChallengeMessagetoauth_challengealigns with naming conventions.
30-34: References to common messages inServerToDeviceMessageupdated.Using
border0.common.v1prefixed messages promotes consistency and reuse across the codebase.
20-22: Verify that all references to moved messages are updated and backward compatibility is maintained.Moving message definitions to
common/messages.protoand updating namespaces enhances modularity. However, please verify:
- All references to these messages are updated throughout the codebase.
- There are no issues with serialization/deserialization due to namespace changes.
Run the following script to find any remaining references to old messages:
Also applies to: 29-34
connector/connector.proto (8)
12-12: [Approved]The addition of
import "common/messages.proto";is appropriate and ensures that common message definitions are available for use. This promotes reuse and consistency across the codebase.
15-15: Corrected RPC return type inControlStreamThe return type in the
ControlStreamRPC method has been corrected fromControlStreamReponsetoControlStreamResponse. This fixes the previous typo and aligns with the correct message definition.
21-21: Updatedheartbeatfield to use common messageUpdating the
heartbeatfield inControlStreamRequestto useborder0.common.v1.HeartbeatMessageis appropriate. This change leverages the common heartbeat message, promoting consistency and reducing duplication.
32-32: Addeddiscovery_detailsfield toControlStreamRequestThe addition of
border0.common.v1.DiscoveryDetailsMessage discovery_details = 14;toControlStreamRequestenhances the message with necessary discovery information. The field name follows the snake_case naming convention.
36-36: DefinedControlStreamResponsemessageThe introduction of the
ControlStreamResponsemessage provides the necessary structure for responses in the control stream. This addition is well-defined and logically structured.
41-41: Updatedheartbeatfield inControlStreamResponseUpdating the
heartbeatfield inControlStreamResponseto useborder0.common.v1.HeartbeatMessageensures consistency with the request message and utilizes the common definition.
49-51: Added new fields toControlStreamResponsefor enhanced communicationThe addition of the following fields enhances the capabilities of
ControlStreamResponse:
border0.common.v1.NetworkStateMessage network_state = 12;border0.common.v1.PeerOnlineMessage peer_online = 13;border0.common.v1.PeerOfflineMessage peer_offline = 14;These fields follow the snake_case naming convention and appropriately extend the message for improved status reporting.
145-145: Addedprivate_network_enabledfield toConnectorConfigIntroducing
bool private_network_enabled = 4;toConnectorConfigis a logical extension that allows configuration of the private network feature. The field numbering is sequential and follows protobuf conventions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
common/messages.proto (3)
11-11: Typo in comment: Correct "its" to "it is"In the comment on line 11, the phrase "the networks its in" should be "the networks it's in" or "the networks it is in" for clarity.
40-41: Consistency in capitalization of "WireGuard" in commentsIn the comments on lines 40 and 41, "wireguard" should be capitalized as "WireGuard" for consistency.
44-45: Consistency in capitalization of abbreviations in commentsPlease capitalize "udp", "ipv4", and "ipv6" as "UDP", "IPv4", and "IPv6" in the comments on lines 44 and 45 for consistency and clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
common/messages.pb.gois excluded by!**/*.pb.goconnector/connector.pb.gois excluded by!**/*.pb.goconnector/connector_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (3)
- Makefile (1 hunks)
- common/messages.proto (1 hunks)
- connector/connector.proto (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile
- connector/connector.proto
🔇 Additional comments (2)
common/messages.proto (2)
1-9: Syntax and package definitions are correctThe syntax version, Go package option, and protobuf package are correctly specified.
10-55: Overall message definitions are appropriateThe message and enum definitions are well-structured, and field types and numbers are correctly assigned. The use of Protocol Buffers best practices is evident.
|
|
||
| # add any components here after their directory has been created with .proto files | ||
| COMPONENTS:=connector device | ||
| COMPONENTS:=common connector device |
There was a problem hiding this comment.
don't need common here since its being mounted as a separate volume below.
There was a problem hiding this comment.
but we still need to built it to get the go code.
| @@ -0,0 +1,55 @@ | |||
| // sets the .proto file syntax version | |||
There was a problem hiding this comment.
device.proto more accurate?
There was a problem hiding this comment.
it's not just devices (eg. there is also healtcheck).
otherwise we should split it up. For now I will keep it like this.
adrianosela
left a comment
adrianosela
left a comment
There was a problem hiding this comment.
LGTM, some nit comments left.
Summary by CodeRabbit
New Features
Bug Fixes
Chores