Skip to content

Add --cable-driver-option join flag#1830

Open
sanek9 wants to merge 1 commit into
submariner-io:develfrom
sanek9:feat/cable-driver-options
Open

Add --cable-driver-option join flag#1830
sanek9 wants to merge 1 commit into
submariner-io:develfrom
sanek9:feat/cable-driver-options

Conversation

@sanek9

@sanek9 sanek9 commented Jul 20, 2026

Copy link
Copy Markdown

What this PR does / why we need it

Allows setting Submariner.spec.cableDriverOptions at join time:

subctl join broker-info.subm \
  --cable-driver amneziawg \
  --cable-driver-option jc=7 \
  --cable-driver-option h1=200000000-280000000

Same key=value pattern as --image-override.

Depends on

  • Requires operator PR that adds spec.cableDriverOptions (otherwise the field is dropped / unknown).

Related issues

Checklist

  • Unit/deploy tests updated if present
  • subctl join --help documents the flag

Cross-links (this series)

Made with Cursor

Summary by CodeRabbit

  • New Features

    • Added support for repeatable --cable-driver-option key=value settings when joining a cluster.
    • Cable driver options are now passed through to the deployed Submariner configuration.
    • Invalid option formats are detected and reported before deployment.
  • Bug Fixes

    • Improved validation and handling of cable driver configuration values.
  • Tests

    • Added coverage for valid, empty, and invalid cable driver options.

Allow setting Submariner.spec.cableDriverOptions at join time via
repeated key=value flags, following the same Options []string pattern
as --image-override.

Signed-off-by: sanek9 <sanya0996@gmail.com>
@submariner-bot

Copy link
Copy Markdown
Contributor

🤖 Created branch: z_pr1830/sanek9/feat/cable-driver-options
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

Adds repeatable --cable-driver-option inputs, parses them as key/value pairs, validates invalid entries, and propagates the resulting map into the deployed Submariner specification with test coverage.

Changes

Cable driver options

Layer / File(s) Summary
CLI option input
pkg/join/options.go, cmd/subctl/join.go
Adds the repeatable --cable-driver-option flag and validates its values during argument checks.
Option parsing and join wiring
pkg/join/join.go
Parses key=value entries, reports malformed options, and passes the parsed map into Submariner deployment options.
Submariner spec wiring and validation
pkg/deploy/submariner.go, pkg/join/join_test.go
Copies cable-driver options into the Submariner spec and tests parsing and deployment propagation.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: skitt, sridhargaddam, tpantelis

Sequence Diagram(s)

sequenceDiagram
  participant JoinCommand
  participant ClusterToBroker
  participant ParseCableDriverOptions
  participant deploy.Submariner
  participant SubmarinerSpec
  JoinCommand->>ClusterToBroker: CableDriverOptionArr
  ClusterToBroker->>ParseCableDriverOptions: parse key=value entries
  ParseCableDriverOptions-->>ClusterToBroker: CableDriverOptions map
  ClusterToBroker->>deploy.Submariner: SubmarinerOptions with cable driver options
  deploy.Submariner->>SubmarinerSpec: populate CableDriverOptions
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: adding the repeated --cable-driver-option flag to subctl join.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Actionable Comments Resolved ✅ Passed New flag/help, parsing, wiring, and tests are in place; no unresolved actionable comments are visible in the changed areas.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sanek9

sanek9 commented Jul 20, 2026

Copy link
Copy Markdown
Author

Part of the proposal overview: submariner-io/submariner#4101

@sanek9

sanek9 commented Jul 20, 2026

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@sanek9

sanek9 commented Jul 20, 2026

Copy link
Copy Markdown
Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
pkg/join/join.go (1)

266-273: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Trim whitespace and handle empty keys.

It's recommended to trim surrounding whitespace from the parsed key and ensure it is not empty (e.g., handling inputs like --cable-driver-option =value or --cable-driver-option " key=value"). This helps prevent subtle misconfigurations where the driver might ignore the option due to a leading space or an invalid empty key.

♻️ Proposed refactor
 	for _, s := range options {
 		key, value, found := strings.Cut(s, "=")
 		if !found {
 			return nil, fmt.Errorf("invalid option %q provided. Please use `key=value` syntax", s)
 		}
+
+		key = strings.TrimSpace(key)
+		if key == "" {
+			return nil, fmt.Errorf("invalid option %q provided. Key cannot be empty", s)
+		}
 
 		result[key] = value
 	}

If you apply this, consider adding a quick test case in join_test.go to cover the empty key scenario!

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/join/join.go` around lines 266 - 273, Update the option parsing loop to
trim surrounding whitespace from the key returned by strings.Cut before storing
it in result, and reject the option when the trimmed key is empty. Preserve the
existing key=value validation and value handling, and add coverage in the
relevant join parsing tests for whitespace and empty-key inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@pkg/join/join.go`:
- Around line 266-273: Update the option parsing loop to trim surrounding
whitespace from the key returned by strings.Cut before storing it in result, and
reject the option when the trimmed key is empty. Preserve the existing key=value
validation and value handling, and add coverage in the relevant join parsing
tests for whitespace and empty-key inputs.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8d362387-e3b5-4543-8b9c-e31136a8d7f5

📥 Commits

Reviewing files that changed from the base of the PR and between 50a1909 and ff4f0b8.

📒 Files selected for processing (5)
  • cmd/subctl/join.go
  • pkg/deploy/submariner.go
  • pkg/join/join.go
  • pkg/join/join_test.go
  • pkg/join/options.go

@sanek9

sanek9 commented Jul 21, 2026

Copy link
Copy Markdown
Author

Depends on submariner-io/submariner-operator#4142

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants