Conversation
- Introduced DEFAULT_SWARM_DB_GOSSIP_PORT variable with a default value of 7946. - Updated usage function to include --swarm_db_gossip_port option. - Modified parameter parsing to handle the new gossip port argument. - Enhanced network settings to forward UDP and TCP traffic for the Swarm DB Gossip Port.
- Introduced DEFAULT_WG_PORT and DEFAULT_WG_PORT_RANGE variables for WireGuard configuration. - Updated usage function to include --wg_port and --wg_port_range options. - Enhanced parameter parsing to handle WireGuard port and range arguments. - Modified network settings to forward UDP traffic for the specified WireGuard port and range.
- Deleted DEFAULT_WG_PORT_RANGE variable and its associated logic from the script. - Updated usage function to remove --wg_port_range option. - Simplified network settings by eliminating the conditional handling for WireGuard port range.
- Introduced DEFAULT_ROOT_WRITABLE variable to manage root filesystem writability. - Updated usage function to include --root_writable option. - Enhanced parameter parsing to handle the new root writable argument. - Modified kernel command line and image drive settings based on the root writable option.
- Updated logic to create a qcow2 overlay when ROOT_WRITABLE is true, allowing for an immutable base image while enabling writes. - Added checks to create or use an existing overlay image, improving the management of writable root filesystems.
- Removed the check for existing overlay image and added logic to always create a new qcow2 overlay. - Simplified the overlay management by ensuring the previous overlay is removed before creation, enhancing the script's reliability.
- Removed unnecessary conditional statement to ensure the qcow2 overlay is always created after the previous one is removed. - Improved script reliability by streamlining the overlay management process.
|
|
||
| NETWORK_SETTINGS+=",hostfwd=udp:0.0.0.0:$WG_PORT-:51820" | ||
| NETWORK_SETTINGS+=",hostfwd=udp:0.0.0.0:$SWARM_DB_GOSSIP_PORT-:7946" | ||
| NETWORK_SETTINGS+=",hostfwd=tcp:0.0.0.0:$SWARM_DB_GOSSIP_PORT-:7946" |
There was a problem hiding this comment.
erroneous duplication?
There was a problem hiding this comment.
Different protos (tcp/udp)
scripts/start_super_protocol.sh
Outdated
|
|
||
| IMAGE_DRIVE=" -drive file=${IMAGE_PATH},if=virtio,format=raw" | ||
| if [[ "${ROOT_WRITABLE}" == "true" ]]; then | ||
| # Use qcow2 overlay to keep the base image immutable while allowing writes |
There was a problem hiding this comment.
It seems that this is not an overlay, but an addition image file.
WalkthroughAdds CLI flags and defaults for WireGuard, Swarm DB Gossip, PKI port, and an allow‑untrusted option; wires these ports into QEMU hostfwd rules (UDP 51820, UDP/TCP 7946, TCP 8443 if set); adds Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
scripts/start_super_protocol.sh(6 hunks)
🔇 Additional comments (6)
scripts/start_super_protocol.sh (6)
31-32: LGTM!The default port values are correct: 51820 is the standard WireGuard port and 7946 is the standard Docker Swarm gossip port.
121-121: LGTM!Variable initialization follows the established pattern and correctly uses the default values.
Also applies to: 124-124
151-151: LGTM!Argument parsing is correct and follows the established pattern.
Also applies to: 154-154
785-786: LGTM!Debug output formatting is consistent with existing debug messages and correctly displays the new port values.
943-943: Clarification: This is NOT a duplicate.Addressing the past review comment: Line 943 (TCP forwarding) is intentional and necessary. Docker Swarm's gossip protocol requires both UDP and TCP on port 7946 for proper operation—UDP for gossip messaging and TCP for failure detection and state synchronization.
941-943: Verify the IP binding choice for the new port forwards.The new port forwards use
0.0.0.0(all interfaces), while HTTP/HTTPS ports use$IP_ADDRESS(lines 935, 938). This difference might be intentional to allow WireGuard VPN and Swarm clustering to listen on all interfaces, but please confirm this is the desired behavior for your security model.
scripts/start_super_protocol.sh
Outdated
| echo " --wg_port <port> WireGuard port (default: ${DEFAULT_WG_PORT})" | ||
| echo " --http_port <port> HTTP port (default: no port forward)" | ||
| echo " --https_port <port> HTTPS port (default: no port forward)" | ||
| echo " --swarm_db_gossip_port <port> Swarm DB Gossip Port" |
There was a problem hiding this comment.
Add default value to help text for consistency.
The --swarm_db_gossip_port option is missing its default value in the help text, unlike other port options (e.g., line 88 for --wg_port).
🔎 Apply this diff to add the default value:
- echo " --swarm_db_gossip_port <port> Swarm DB Gossip Port"
+ echo " --swarm_db_gossip_port <port> Swarm DB Gossip Port (default: ${DEFAULT_SWARM_DB_GOSSIP_PORT})"📝 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.
| echo " --swarm_db_gossip_port <port> Swarm DB Gossip Port" | |
| echo " --swarm_db_gossip_port <port> Swarm DB Gossip Port (default: ${DEFAULT_SWARM_DB_GOSSIP_PORT})" |
🤖 Prompt for AI Agents
In scripts/start_super_protocol.sh around line 91, the help text for
"--swarm_db_gossip_port" lacks the default value; update the echo to include the
same default-value format used by other port options (e.g., append " (default:
<port>)") so the help output is consistent—set the default port to the same
value used in the script/config for swarm DB gossip.
* allow_untrusted flag * pki port * unified options style * no default pki port
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/start_super_protocol.sh (2)
843-847: Consider clarifying the error message.The error message could be more explicit about the requirement:
Suggested improvement
# Validate allow-untrusted requires swarm-init=true if [[ "${ALLOW_UNTRUSTED}" == "true" ]] && [[ "${SWARM_INIT}" == "false" ]]; then - echo "Error: --allow-untrusted can only be used with --swarm-init" + echo "Error: --allow-untrusted=true can only be used when --swarm-init=true" exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/start_super_protocol.sh` around lines 843 - 847, Update the error text in the ALLOW_UNTRUSTED / SWARM_INIT validation so it explicitly states the requirement and suggested fix: change the echo in the if block that checks [[ "${ALLOW_UNTRUSTED}" == "true" ]] && [[ "${SWARM_INIT}" == "false" ]] to something like "Error: --allow-untrusted requires --swarm-init (use --swarm-init to enable swarm mode)" so users know the exact flag to add; keep the same exit 1 behavior.
159-173: Inconsistent option naming convention.The new options mix underscore (
--wg_port,--pki_port,--swarm_db_gossip_port) and hyphen (--swarm-init,--allow-untrusted) conventions. Consider standardizing to one convention for better UX consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/start_super_protocol.sh` around lines 159 - 173, The CLI option names use mixed conventions (underscores like --wg_port, --pki_port, --swarm_db_gossip_port vs hyphens like --swarm-init, --allow-untrusted), so standardize them to one style (prefer hyphenated POSIX style) and update corresponding variable parsing and assignments (e.g., map --wg-port -> WG_PORT, --pki-port -> PKI_PORT, --swarm-db-gossip-port -> SWARM_DB_GOSSIP_PORT) while keeping existing variable names (WG_PORT, PKI_PORT, SWARM_DB_GOSSIP_PORT, SWARM_INIT, ALLOW_UNTRUSTED) so callers and downstream code keep working; ensure you update every occurrence in the argument parsing switch and any documentation or help text to match the chosen hyphenated names (--wg-port, --pki-port, --swarm-db-gossip-port) and keep the existing --swarm-init and --allow-untrusted names unchanged if you choose hyphens.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/start_super_protocol.sh`:
- Around line 981-987: The port-forward bindings are inconsistent: PKI_PORT uses
$IP_ADDRESS while WG_PORT and SWARM_DB_GOSSIP_PORT are bound to 0.0.0.0; update
the NETWORK_SETTINGS assembly in start_super_protocol.sh so WG_PORT and
SWARM_DB_GOSSIP_PORT use $IP_ADDRESS (same as the PKI case) unless you
intentionally want them on all interfaces—adjust the hostfwd entries that
reference WG_PORT and SWARM_DB_GOSSIP_PORT to use $IP_ADDRESS instead of
0.0.0.0, or add a clear comment explaining why 0.0.0.0 is required for those
ports.
- Around line 717-723: After the PKI_PORT availability check, add equivalent
port-binding checks for WG_PORT and SWARM_DB_GOSSIP_PORT: for each variable
(WG_PORT, SWARM_DB_GOSSIP_PORT) test whether it's non-empty, print a descriptive
"Checking ... port availability..." message, use nc -z "$IP_ADDRESS" "$WG_PORT"
(and similarly for SWARM_DB_GOSSIP_PORT) to detect an already-bound port, and if
bound echo which port is already bound and exit 1; place these checks
immediately after the existing PKI_PORT block so forwarding of WG_PORT and
SWARM_DB_GOSSIP_PORT is guarded the same way.
---
Nitpick comments:
In `@scripts/start_super_protocol.sh`:
- Around line 843-847: Update the error text in the ALLOW_UNTRUSTED / SWARM_INIT
validation so it explicitly states the requirement and suggested fix: change the
echo in the if block that checks [[ "${ALLOW_UNTRUSTED}" == "true" ]] && [[
"${SWARM_INIT}" == "false" ]] to something like "Error: --allow-untrusted
requires --swarm-init (use --swarm-init to enable swarm mode)" so users know the
exact flag to add; keep the same exit 1 behavior.
- Around line 159-173: The CLI option names use mixed conventions (underscores
like --wg_port, --pki_port, --swarm_db_gossip_port vs hyphens like --swarm-init,
--allow-untrusted), so standardize them to one style (prefer hyphenated POSIX
style) and update corresponding variable parsing and assignments (e.g., map
--wg-port -> WG_PORT, --pki-port -> PKI_PORT, --swarm-db-gossip-port ->
SWARM_DB_GOSSIP_PORT) while keeping existing variable names (WG_PORT, PKI_PORT,
SWARM_DB_GOSSIP_PORT, SWARM_INIT, ALLOW_UNTRUSTED) so callers and downstream
code keep working; ensure you update every occurrence in the argument parsing
switch and any documentation or help text to match the chosen hyphenated names
(--wg-port, --pki-port, --swarm-db-gossip-port) and keep the existing
--swarm-init and --allow-untrusted names unchanged if you choose hyphens.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
scripts/start_super_protocol.sh
| if [[ -n "$PKI_PORT" ]]; then | ||
| echo "Checking pki port aviability..." | ||
| if nc -z "$IP_ADDRESS" "$PKI_PORT"; then | ||
| echo "pki port $PKI_PORT already bound!" | ||
| exit 1 | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Missing availability checks for WireGuard and Swarm DB Gossip ports.
WG_PORT and SWARM_DB_GOSSIP_PORT are always forwarded but lack availability checks, unlike HTTP_PORT, HTTPS_PORT, and PKI_PORT. If these ports are already bound, QEMU will fail.
Suggested fix - add checks after PKI_PORT check
if [[ -n "$PKI_PORT" ]]; then
echo "Checking pki port aviability..."
if nc -z "$IP_ADDRESS" "$PKI_PORT"; then
echo "pki port $PKI_PORT already bound!"
exit 1
fi
fi
+
+ echo "Checking WireGuard port availability..."
+ if nc -z -u "0.0.0.0" "$WG_PORT"; then
+ echo "WireGuard port $WG_PORT already bound!"
+ exit 1
+ fi
+
+ echo "Checking Swarm DB Gossip port availability..."
+ if nc -z -u "0.0.0.0" "$SWARM_DB_GOSSIP_PORT"; then
+ echo "Swarm DB Gossip UDP port $SWARM_DB_GOSSIP_PORT already bound!"
+ exit 1
+ fi
+ if nc -z "0.0.0.0" "$SWARM_DB_GOSSIP_PORT"; then
+ echo "Swarm DB Gossip TCP port $SWARM_DB_GOSSIP_PORT already bound!"
+ exit 1
+ fi📝 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.
| if [[ -n "$PKI_PORT" ]]; then | |
| echo "Checking pki port aviability..." | |
| if nc -z "$IP_ADDRESS" "$PKI_PORT"; then | |
| echo "pki port $PKI_PORT already bound!" | |
| exit 1 | |
| fi | |
| fi | |
| if [[ -n "$PKI_PORT" ]]; then | |
| echo "Checking pki port aviability..." | |
| if nc -z "$IP_ADDRESS" "$PKI_PORT"; then | |
| echo "pki port $PKI_PORT already bound!" | |
| exit 1 | |
| fi | |
| fi | |
| echo "Checking WireGuard port availability..." | |
| if nc -z -u "0.0.0.0" "$WG_PORT"; then | |
| echo "WireGuard port $WG_PORT already bound!" | |
| exit 1 | |
| fi | |
| echo "Checking Swarm DB Gossip port availability..." | |
| if nc -z -u "0.0.0.0" "$SWARM_DB_GOSSIP_PORT"; then | |
| echo "Swarm DB Gossip UDP port $SWARM_DB_GOSSIP_PORT already bound!" | |
| exit 1 | |
| fi | |
| if nc -z "0.0.0.0" "$SWARM_DB_GOSSIP_PORT"; then | |
| echo "Swarm DB Gossip TCP port $SWARM_DB_GOSSIP_PORT already bound!" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/start_super_protocol.sh` around lines 717 - 723, After the PKI_PORT
availability check, add equivalent port-binding checks for WG_PORT and
SWARM_DB_GOSSIP_PORT: for each variable (WG_PORT, SWARM_DB_GOSSIP_PORT) test
whether it's non-empty, print a descriptive "Checking ... port availability..."
message, use nc -z "$IP_ADDRESS" "$WG_PORT" (and similarly for
SWARM_DB_GOSSIP_PORT) to detect an already-bound port, and if bound echo which
port is already bound and exit 1; place these checks immediately after the
existing PKI_PORT block so forwarding of WG_PORT and SWARM_DB_GOSSIP_PORT is
guarded the same way.
| if [[ -n "$PKI_PORT" ]]; then | ||
| NETWORK_SETTINGS+=",hostfwd=tcp:$IP_ADDRESS:$PKI_PORT-:8443" | ||
| fi | ||
|
|
||
| NETWORK_SETTINGS+=",hostfwd=udp:0.0.0.0:$WG_PORT-:51820" | ||
| NETWORK_SETTINGS+=",hostfwd=udp:0.0.0.0:$SWARM_DB_GOSSIP_PORT-:7946" | ||
| NETWORK_SETTINGS+=",hostfwd=tcp:0.0.0.0:$SWARM_DB_GOSSIP_PORT-:7946" |
There was a problem hiding this comment.
Inconsistent IP binding for port forwards.
PKI port uses $IP_ADDRESS (line 982), but WireGuard and Swarm DB Gossip ports are hardcoded to 0.0.0.0 (lines 985-987). This means these ports will always bind to all interfaces regardless of the --ip_address setting.
If this is intentional (e.g., WireGuard must be reachable from all interfaces), consider documenting this behavior. Otherwise, use $IP_ADDRESS consistently:
Suggested fix for consistency
- NETWORK_SETTINGS+=",hostfwd=udp:0.0.0.0:$WG_PORT-:51820"
- NETWORK_SETTINGS+=",hostfwd=udp:0.0.0.0:$SWARM_DB_GOSSIP_PORT-:7946"
- NETWORK_SETTINGS+=",hostfwd=tcp:0.0.0.0:$SWARM_DB_GOSSIP_PORT-:7946"
+ NETWORK_SETTINGS+=",hostfwd=udp:$IP_ADDRESS:$WG_PORT-:51820"
+ NETWORK_SETTINGS+=",hostfwd=udp:$IP_ADDRESS:$SWARM_DB_GOSSIP_PORT-:7946"
+ NETWORK_SETTINGS+=",hostfwd=tcp:$IP_ADDRESS:$SWARM_DB_GOSSIP_PORT-:7946"📝 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.
| if [[ -n "$PKI_PORT" ]]; then | |
| NETWORK_SETTINGS+=",hostfwd=tcp:$IP_ADDRESS:$PKI_PORT-:8443" | |
| fi | |
| NETWORK_SETTINGS+=",hostfwd=udp:0.0.0.0:$WG_PORT-:51820" | |
| NETWORK_SETTINGS+=",hostfwd=udp:0.0.0.0:$SWARM_DB_GOSSIP_PORT-:7946" | |
| NETWORK_SETTINGS+=",hostfwd=tcp:0.0.0.0:$SWARM_DB_GOSSIP_PORT-:7946" | |
| if [[ -n "$PKI_PORT" ]]; then | |
| NETWORK_SETTINGS+=",hostfwd=tcp:$IP_ADDRESS:$PKI_PORT-:8443" | |
| fi | |
| NETWORK_SETTINGS+=",hostfwd=udp:$IP_ADDRESS:$WG_PORT-:51820" | |
| NETWORK_SETTINGS+=",hostfwd=udp:$IP_ADDRESS:$SWARM_DB_GOSSIP_PORT-:7946" | |
| NETWORK_SETTINGS+=",hostfwd=tcp:$IP_ADDRESS:$SWARM_DB_GOSSIP_PORT-:7946" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/start_super_protocol.sh` around lines 981 - 987, The port-forward
bindings are inconsistent: PKI_PORT uses $IP_ADDRESS while WG_PORT and
SWARM_DB_GOSSIP_PORT are bound to 0.0.0.0; update the NETWORK_SETTINGS assembly
in start_super_protocol.sh so WG_PORT and SWARM_DB_GOSSIP_PORT use $IP_ADDRESS
(same as the PKI case) unless you intentionally want them on all
interfaces—adjust the hostfwd entries that reference WG_PORT and
SWARM_DB_GOSSIP_PORT to use $IP_ADDRESS instead of 0.0.0.0, or add a clear
comment explaining why 0.0.0.0 is required for those ports.
3 participants
Summary by CodeRabbit
New Features
Documentation
Diagnostics