feat(BRE2-797): Allow SSH Port to be Specified#311
Open
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for specifying a target SSH port during SSH access granting/enabling flows, and forwards that port to the server via the node RPC request.
Changes:
- Prompt for an SSH port (defaulting to 22) in
register,grant-ssh, andenable-sshflows. - Extend
GrantSSHAccessToNode/GrantNodeSSHAccessRequestto include the selected port. - Update tests to avoid blocking on interactive input and bump the generated devplane protos dependency.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/cmd/register/sshkeys.go | Adds SSH port prompting + threads port into GrantNodeSSHAccessRequest. |
| pkg/cmd/register/register.go | Prompts for SSH port and passes it to GrantSSHAccessToNode. |
| pkg/cmd/register/register_test.go | Sets a fixed test port to prevent interactive prompts in affected tests. |
| pkg/cmd/grantssh/grantssh.go | Prompts for SSH port and passes it to GrantSSHAccessToNode. |
| pkg/cmd/grantssh/grantssh_test.go | Sets a fixed test port to prevent interactive prompts. |
| pkg/cmd/enablessh/enablessh.go | Prompts for SSH port and passes it to GrantSSHAccessToNode. |
| go.mod | Updates devplane protocolbuffers module version. |
| go.sum | Updates sums for the bumped devplane protocolbuffers module. |
Comments suppressed due to low confidence (3)
pkg/cmd/register/sshkeys.go:209
- The new SSH port prompting logic relies on package-level mutable state (testSSHPort) and exported SetTestSSHPort/ClearTestSSHPort helpers to make tests non-interactive. This leaks a test-only control surface into production code and introduces hidden global state that can become flaky if tests (or commands) ever run in parallel. Consider injecting an input/port provider via existing deps/prompter patterns (or passing the port down from the command layer) so production code stays deterministic and tests don’t need globals/exported test hooks.
const defaultSSHPort = 22
// testSSHPort is set by tests to avoid blocking on stdin. When non-nil,
// PromptSSHPort returns this value without prompting.
var testSSHPort *int32
// SetTestSSHPort sets the port returned by PromptSSHPort without prompting.
// Only for use in tests; call ClearTestSSHPort when done.
func SetTestSSHPort(port int32) { testSSHPort = &port }
// ClearTestSSHPort clears the test port override.
func ClearTestSSHPort() { testSSHPort = nil }
pkg/cmd/register/sshkeys.go:232
- PromptSSHPort hard-codes the default port value in multiple places (label/default string and defaultSSHPort constant). To avoid drift, reuse defaultSSHPort when building the prompt label/default. Also, strconv.ParseUint with bitSize=16 already constrains values to <= 65535, so the explicit upper-bound check is redundant (or switch to bitSize=32 and keep the range check).
portStr := terminal.PromptGetInput(terminal.PromptContent{
Label: " SSH port (default 22): ",
Default: "22",
AllowEmpty: true,
})
portStr = strings.TrimSpace(portStr)
if portStr == "" {
return defaultSSHPort, nil
}
n, err := strconv.ParseUint(portStr, 10, 16)
if err != nil {
return 0, fmt.Errorf("invalid port %q: %w", portStr, err)
}
if n < 1 || n > 65535 {
return 0, fmt.Errorf("port must be between 1 and 65535, got %d", n)
}
pkg/cmd/register/sshkeys.go:169
- The SSH port is now sent to the server via GrantNodeSSHAccessRequest.Port, but the updated tests only set a fixed port to avoid blocking; they don’t assert that the port value is actually propagated into the RPC request. Add an assertion in the existing register/grant-ssh flow tests (which already capture the request) that Port equals the chosen value to prevent regressions.
opToTry := func() error {
_, err := client.GrantNodeSSHAccess(ctx, connect.NewRequest(&nodev1.GrantNodeSSHAccessRequest{
ExternalNodeId: reg.ExternalNodeID,
UserId: targetUser.ID,
LinuxUser: osUser.Username,
Port: port,
}))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Specify:
Keep blank: