From 61da52eb261c69c1018d69d32a287c569a0d9ee9 Mon Sep 17 00:00:00 2001 From: Allen Chen Date: Wed, 12 Aug 2026 10:06:58 -0700 Subject: [PATCH] Add command to close instance ports --- .agents/skills/brev-cli/SKILL.md | 5 + .agents/skills/brev-cli/reference/commands.md | 27 ++ pkg/cmd/ports/close.go | 231 +++++++++++++ pkg/cmd/ports/close_test.go | 314 ++++++++++++++++++ pkg/cmd/ports/ports.go | 29 +- 5 files changed, 598 insertions(+), 8 deletions(-) create mode 100644 pkg/cmd/ports/close.go create mode 100644 pkg/cmd/ports/close_test.go diff --git a/.agents/skills/brev-cli/SKILL.md b/.agents/skills/brev-cli/SKILL.md index 3b5f8b0c..4feb24a6 100644 --- a/.agents/skills/brev-cli/SKILL.md +++ b/.agents/skills/brev-cli/SKILL.md @@ -180,6 +180,11 @@ brev ports open my-instance 8080 brev ports open my-node 53 --protocol udp --allow 203.0.113.10/32 brev ports open my-instance 3000 --protocol http --public brev ports open my-instance 8888 --protocol http --authorize me@example.com + +# Close one port interactively, by exact ID, or close all ports +brev ports close my-instance +brev ports close my-instance --id nport-abc123 --approve +brev ports close my-node --all --approve ``` ### Listing Instances and Nodes diff --git a/.agents/skills/brev-cli/reference/commands.md b/.agents/skills/brev-cli/reference/commands.md index 4256d728..dbdcbc51 100644 --- a/.agents/skills/brev-cli/reference/commands.md +++ b/.agents/skills/brev-cli/reference/commands.md @@ -544,6 +544,33 @@ brev ports open my-instance 3000 --protocol http --public brev ports open my-instance 8888 --protocol http --authorize me@example.com ``` +#### Close ports + +Select and close one port interactively, close an exact mapping by its +`port_id`, or close every mapping on the target. `remove` is an alias for +`close`. + +```bash +brev ports close [flags] +``` + +**Flags:** +| Flag | Description | +|------|-------------| +| `--id` | Close the exact mapping with this `port_id` | +| `--all` | Close every port mapping on the target | +| `--approve` | Skip the confirmation prompt | + +Use `brev ports --json` to obtain stable `port_id` values +for automation. `--all` and `--id` cannot be combined. + +**Examples:** +```bash +brev ports close my-instance +brev ports close my-instance --id nport-abc123 --approve +brev ports remove my-node --all --approve +``` + ## Organization Commands ### brev org ls diff --git a/pkg/cmd/ports/close.go b/pkg/cmd/ports/close.go new file mode 100644 index 00000000..54e9fd34 --- /dev/null +++ b/pkg/cmd/ports/close.go @@ -0,0 +1,231 @@ +package ports + +import ( + "context" + "errors" + "fmt" + "io" + + devplanev1 "buf.build/gen/go/brevdev/devplane/protocolbuffers/go/devplaneapi/v1" + "connectrpc.com/connect" + "github.com/spf13/cobra" + + "github.com/brevdev/brev-cli/pkg/cmd/cmderrors" + "github.com/brevdev/brev-cli/pkg/cmd/register" + cmdutil "github.com/brevdev/brev-cli/pkg/cmd/util" + "github.com/brevdev/brev-cli/pkg/config" + breverrors "github.com/brevdev/brev-cli/pkg/errors" + "github.com/brevdev/brev-cli/pkg/terminal" +) + +type closeOptions struct { + portID string + all bool + approve bool +} + +type closePrompter interface { + terminal.Selector + terminal.Confirmer +} + +// NewCmdClosePort creates the `brev ports close` command. +func NewCmdClosePort(portStore Store) *cobra.Command { + return newCmdClosePort(portStore, register.TerminalPrompter{}) +} + +func newCmdClosePort(portStore Store, prompter closePrompter) *cobra.Command { + var opts closeOptions + + cmd := &cobra.Command{ + Annotations: map[string]string{"access": ""}, + Use: "close ", + Aliases: []string{"remove"}, + DisableFlagsInUseLine: true, + Short: "Close public ports on an instance or external node", + Example: ` + brev ports close my-instance + brev ports close my-instance --id nport-abc123 --approve + brev ports close my-node --all --approve`, + Args: cmderrors.TransformToValidationError(cobra.ExactArgs(1)), + RunE: func(cmd *cobra.Command, args []string) error { + if opts.all && opts.portID != "" { + return breverrors.NewValidationError("--all and --id cannot be used together") + } + if err := runClose(cmd.Context(), cmd.OutOrStdout(), portStore, prompter, args[0], opts); err != nil { + return breverrors.WrapAndTrace(err) + } + return nil + }, + } + + cmd.Flags().StringVar(&opts.portID, "id", "", "close the exact port mapping with this port_id") + cmd.Flags().BoolVar(&opts.all, "all", false, "close every port mapping on the target") + cmd.Flags().BoolVar(&opts.approve, "approve", false, "skip confirmation prompt (assume yes)") + return cmd +} + +func runClose( + ctx context.Context, + out io.Writer, + portStore Store, + prompter closePrompter, + nameOrID string, + opts closeOptions, +) error { + target, apiPorts, err := resolveTargetPorts(ctx, portStore, nameOrID) + if err != nil { + return breverrors.WrapAndTrace(err) + } + + removable := removablePorts(apiPorts) + if len(removable) == 0 { + return fmt.Errorf("no removable ports are open on %s", nameOrID) + } + + selected, err := selectPortsToClose(prompter, removable, opts) + if err != nil { + return breverrors.WrapAndTrace(err) + } + + if err := displayCloseConfirmation(out, nameOrID, selected); err != nil { + return breverrors.WrapAndTrace(err) + } + if !opts.approve && !prompter.ConfirmYesNo(closeConfirmationLabel(nameOrID, len(selected))) { + _, err := fmt.Fprintln(out, "No ports were closed.") + return breverrors.WrapAndTrace(err) + } + + return closePorts(ctx, out, portStore, target, nameOrID, selected) +} + +func removablePorts(apiPorts []*devplanev1.Port) []*devplanev1.Port { + ports := make([]*devplanev1.Port, 0, len(apiPorts)) + for _, port := range apiPorts { + if port != nil && port.GetPortId() != "" { + ports = append(ports, port) + } + } + return ports +} + +func selectPortsToClose( + prompter terminal.Selector, + ports []*devplanev1.Port, + opts closeOptions, +) ([]*devplanev1.Port, error) { + if opts.all { + return ports, nil + } + if opts.portID != "" { + for _, port := range ports { + if port.GetPortId() == opts.portID { + return []*devplanev1.Port{port}, nil + } + } + return nil, fmt.Errorf("port_id %q is not open on this target", opts.portID) + } + + labels := make([]string, len(ports)) + for i, port := range ports { + labels[i] = closeSelectionLabel(i, port) + } + chosen := prompter.Select("Select a port to close", labels) + for i, label := range labels { + if label == chosen { + return []*devplanev1.Port{ports[i]}, nil + } + } + return nil, fmt.Errorf("selected item did not match any open port") +} + +func closeSelectionLabel(index int, port *devplanev1.Port) string { + info := toPortInfos([]*devplanev1.Port{port})[0] + destinationPort := info.DestinationPort + if destinationPort == 0 { + destinationPort = info.PublicPort + } + return fmt.Sprintf( + "%d. %s %s public %s -> destination %s", + index+1, + info.Protocol, + valueOrDash(info.Endpoint), + portNumberLabel(info.PublicPort), + portNumberLabel(destinationPort), + ) +} + +func displayCloseConfirmation(out io.Writer, nameOrID string, ports []*devplanev1.Port) error { + if _, err := fmt.Fprintf(out, "The following port mapping(s) will be permanently removed from %s:\n\n", nameOrID); err != nil { + return breverrors.WrapAndTrace(err) + } + if err := displayTables(out, nameOrID, toPortInfos(ports)); err != nil { + return breverrors.WrapAndTrace(err) + } + _, err := fmt.Fprintln(out, "\nActive connections may be dropped and this action cannot be undone.") + return breverrors.WrapAndTrace(err) +} + +func closeConfirmationLabel(nameOrID string, count int) string { + portWord := "ports" + if count == 1 { + portWord = "port" + } + return fmt.Sprintf("Close %d %s on %s?", count, portWord, nameOrID) +} + +func closePorts( + ctx context.Context, + out io.Writer, + portStore Store, + target *cmdutil.WorkspaceOrNode, + nameOrID string, + ports []*devplanev1.Port, +) error { + var closeErrors []error + closed := 0 + for _, port := range ports { + if err := closePort(ctx, portStore, target, port.GetPortId()); err != nil { + closeErrors = append(closeErrors, fmt.Errorf("close port_id %q: %w", port.GetPortId(), err)) + continue + } + closed++ + } + + if closed > 0 { + portWord := "ports" + if closed == 1 { + portWord = "port" + } + if _, err := fmt.Fprintf(out, "Closed %d %s on %s.\n", closed, portWord, nameOrID); err != nil { + closeErrors = append(closeErrors, err) + } + } + if err := errors.Join(closeErrors...); err != nil { + return breverrors.WrapAndTrace(err) + } + return nil +} + +func closePort( + ctx context.Context, + portStore Store, + target *cmdutil.WorkspaceOrNode, + portID string, +) error { + if target.Workspace != nil { + client := register.NewEnvironmentServiceClient(portStore, config.GlobalConfig.GetBrevPublicAPIURL()) + _, err := client.ClosePort(ctx, connect.NewRequest(&devplanev1.EnvironmentServiceClosePortRequest{ + PortId: portID, + })) + return breverrors.WrapAndTrace(err) + } + if target.Node != nil { + client := register.NewNodeServiceClient(portStore, config.GlobalConfig.GetBrevPublicAPIURL()) + _, err := client.ClosePort(ctx, connect.NewRequest(&devplanev1.ClosePortRequest{ + PortId: portID, + })) + return breverrors.WrapAndTrace(err) + } + return fmt.Errorf("resolved target has no instance or external node") +} diff --git a/pkg/cmd/ports/close_test.go b/pkg/cmd/ports/close_test.go new file mode 100644 index 00000000..e87dfdb7 --- /dev/null +++ b/pkg/cmd/ports/close_test.go @@ -0,0 +1,314 @@ +package ports + +import ( + "bytes" + "context" + "errors" + "testing" + + devplanev1connect "buf.build/gen/go/brevdev/devplane/connectrpc/go/devplaneapi/v1/devplaneapiv1connect" + devplanev1 "buf.build/gen/go/brevdev/devplane/protocolbuffers/go/devplaneapi/v1" + "connectrpc.com/connect" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/brevdev/brev-cli/pkg/entity" +) + +type fakeClosePrompter struct { + selectIndex int + confirm bool + selectCalls int + confirmCalls int + items []string +} + +func (p *fakeClosePrompter) Select(_ string, items []string) string { + p.selectCalls++ + p.items = append([]string{}, items...) + if p.selectIndex < 0 || p.selectIndex >= len(items) { + return "" + } + return items[p.selectIndex] +} + +func (p *fakeClosePrompter) ConfirmYesNo(_ string) bool { + p.confirmCalls++ + return p.confirm +} + +type fakeCloseEnvironmentService struct { + devplanev1connect.UnimplementedEnvironmentServiceHandler + t *testing.T + expectedEnvID string + ports []*devplanev1.Port + closedPortIDs []string + failPortID string +} + +func (s *fakeCloseEnvironmentService) GetNetworkInfo( + _ context.Context, + req *connect.Request[devplanev1.EnvironmentServiceGetNetworkInfoRequest], +) (*connect.Response[devplanev1.EnvironmentServiceGetNetworkInfoResponse], error) { + s.t.Helper() + assert.Equal(s.t, s.expectedEnvID, req.Msg.GetEnvironmentId()) + return connect.NewResponse(&devplanev1.EnvironmentServiceGetNetworkInfoResponse{ + NetworkInfo: &devplanev1.EnvironmentNetworkInfo{ + Status: devplanev1.NetworkMemberStatus_NETWORK_MEMBER_STATUS_CONNECTED, + Ports: s.ports, + }, + }), nil +} + +func (s *fakeCloseEnvironmentService) ClosePort( + _ context.Context, + req *connect.Request[devplanev1.EnvironmentServiceClosePortRequest], +) (*connect.Response[devplanev1.EnvironmentServiceClosePortResponse], error) { + s.t.Helper() + if req.Msg.GetPortId() == s.failPortID { + return nil, connect.NewError(connect.CodeInternal, errors.New("close failed")) + } + s.closedPortIDs = append(s.closedPortIDs, req.Msg.GetPortId()) + return connect.NewResponse(&devplanev1.EnvironmentServiceClosePortResponse{}), nil +} + +type fakeCloseNodeService struct { + devplanev1connect.UnimplementedExternalNodeServiceHandler + t *testing.T + node *devplanev1.ExternalNode + closedPortIDs []string +} + +func (s *fakeCloseNodeService) ListNodes( + _ context.Context, + _ *connect.Request[devplanev1.ListNodesRequest], +) (*connect.Response[devplanev1.ListNodesResponse], error) { + return connect.NewResponse(&devplanev1.ListNodesResponse{ + Items: []*devplanev1.ExternalNode{s.node}, + }), nil +} + +func (s *fakeCloseNodeService) ClosePort( + _ context.Context, + req *connect.Request[devplanev1.ClosePortRequest], +) (*connect.Response[devplanev1.ClosePortResponse], error) { + s.t.Helper() + s.closedPortIDs = append(s.closedPortIDs, req.Msg.GetPortId()) + return connect.NewResponse(&devplanev1.ClosePortResponse{}), nil +} + +func newCloseEnvironmentStore() *fakeStore { + return &fakeStore{ + workspaces: []entity.Workspace{{ID: "env123", Name: "my-instance", CreatedByUserID: "user1"}}, + user: &entity.User{ID: "user1"}, + org: &entity.Organization{ID: "org1"}, + } +} + +func testTCPPort(id string, publicPort int32) *devplanev1.Port { + hostname := "global.prd.ga.run.brev.nvidia.com" + return &devplanev1.Port{ + PortId: id, + Protocol: devplanev1.PortProtocol_PORT_PROTOCOL_TCP, + PortNumber: publicPort, + ServerPort: 8080, + Hostname: &hostname, + Type: devplanev1.PortType_PORT_TYPE_USER, + } +} + +func TestCloseInteractivelySelectsOnePort(t *testing.T) { + service := &fakeCloseEnvironmentService{ + t: t, + expectedEnvID: "env123", + ports: []*devplanev1.Port{ + testTCPPort("nport-one", 41001), + testTCPPort("nport-two", 52002), + }, + } + _, handler := devplanev1connect.NewEnvironmentServiceHandler(service) + newTestServer(t, handler) + prompter := &fakeClosePrompter{selectIndex: 1, confirm: true} + var out bytes.Buffer + + err := runClose( + context.Background(), + &out, + newCloseEnvironmentStore(), + prompter, + "my-instance", + closeOptions{}, + ) + + require.NoError(t, err) + assert.Equal(t, []string{"nport-two"}, service.closedPortIDs) + assert.Equal(t, 1, prompter.selectCalls) + assert.Equal(t, 1, prompter.confirmCalls) + require.Len(t, prompter.items, 2) + assert.Contains(t, prompter.items[0], "public 41001 -> destination 8080") + assert.Contains(t, prompter.items[1], "public 52002 -> destination 8080") + assert.Contains(t, out.String(), "Closed 1 port on my-instance.") +} + +func TestCloseByExactIDOnExternalNode(t *testing.T) { + service := &fakeCloseNodeService{ + t: t, + node: &devplanev1.ExternalNode{ + ExternalNodeId: "unode123", + Name: "my-node", + Ports: []*devplanev1.Port{ + testTCPPort("nport-one", 41001), + testTCPPort("nport-two", 52002), + }, + }, + } + _, handler := devplanev1connect.NewExternalNodeServiceHandler(service) + newTestServer(t, handler) + prompter := &fakeClosePrompter{selectIndex: -1} + store := &fakeStore{ + user: &entity.User{ID: "user1"}, + org: &entity.Organization{ID: "org1"}, + } + var out bytes.Buffer + + err := runClose( + context.Background(), + &out, + store, + prompter, + "unode123", + closeOptions{portID: "nport-one", approve: true}, + ) + + require.NoError(t, err) + assert.Equal(t, []string{"nport-one"}, service.closedPortIDs) + assert.Zero(t, prompter.selectCalls) + assert.Zero(t, prompter.confirmCalls) + assert.Contains(t, out.String(), "global.prd.ga.run.brev.nvidia.com:41001") +} + +func TestCloseAllClosesSnapshot(t *testing.T) { + service := &fakeCloseEnvironmentService{ + t: t, + expectedEnvID: "env123", + ports: []*devplanev1.Port{ + testTCPPort("nport-one", 41001), + testTCPPort("nport-two", 52002), + }, + } + _, handler := devplanev1connect.NewEnvironmentServiceHandler(service) + newTestServer(t, handler) + prompter := &fakeClosePrompter{selectIndex: -1} + var out bytes.Buffer + + err := runClose( + context.Background(), + &out, + newCloseEnvironmentStore(), + prompter, + "my-instance", + closeOptions{all: true, approve: true}, + ) + + require.NoError(t, err) + assert.Equal(t, []string{"nport-one", "nport-two"}, service.closedPortIDs) + assert.Zero(t, prompter.selectCalls) + assert.Zero(t, prompter.confirmCalls) + assert.Contains(t, out.String(), "Closed 2 ports on my-instance.") +} + +func TestCloseCancellationDoesNotClosePort(t *testing.T) { + service := &fakeCloseEnvironmentService{ + t: t, + expectedEnvID: "env123", + ports: []*devplanev1.Port{testTCPPort("nport-one", 41001)}, + } + _, handler := devplanev1connect.NewEnvironmentServiceHandler(service) + newTestServer(t, handler) + prompter := &fakeClosePrompter{selectIndex: 0, confirm: false} + var out bytes.Buffer + + err := runClose( + context.Background(), + &out, + newCloseEnvironmentStore(), + prompter, + "my-instance", + closeOptions{}, + ) + + require.NoError(t, err) + assert.Empty(t, service.closedPortIDs) + assert.Contains(t, out.String(), "No ports were closed.") +} + +func TestCloseRejectsUnknownID(t *testing.T) { + service := &fakeCloseEnvironmentService{ + t: t, + expectedEnvID: "env123", + ports: []*devplanev1.Port{testTCPPort("nport-one", 41001)}, + } + _, handler := devplanev1connect.NewEnvironmentServiceHandler(service) + newTestServer(t, handler) + var out bytes.Buffer + + err := runClose( + context.Background(), + &out, + newCloseEnvironmentStore(), + &fakeClosePrompter{}, + "my-instance", + closeOptions{portID: "nport-missing", approve: true}, + ) + + assert.ErrorContains(t, err, `port_id "nport-missing" is not open on this target`) + assert.Empty(t, service.closedPortIDs) +} + +func TestCloseAllReportsPartialFailure(t *testing.T) { + service := &fakeCloseEnvironmentService{ + t: t, + expectedEnvID: "env123", + ports: []*devplanev1.Port{ + testTCPPort("nport-one", 41001), + testTCPPort("nport-two", 52002), + }, + failPortID: "nport-one", + } + _, handler := devplanev1connect.NewEnvironmentServiceHandler(service) + newTestServer(t, handler) + var out bytes.Buffer + + err := runClose( + context.Background(), + &out, + newCloseEnvironmentStore(), + &fakeClosePrompter{}, + "my-instance", + closeOptions{all: true, approve: true}, + ) + + assert.ErrorContains(t, err, `close port_id "nport-one"`) + assert.Equal(t, []string{"nport-two"}, service.closedPortIDs) + assert.Contains(t, out.String(), "Closed 1 port on my-instance.") +} + +func TestCloseCommandRejectsAllWithID(t *testing.T) { + cmd := newCmdClosePort(newCloseEnvironmentStore(), &fakeClosePrompter{}) + cmd.SetArgs([]string{"my-instance", "--all", "--id", "nport-one"}) + + err := cmd.Execute() + + assert.ErrorContains(t, err, "--all and --id cannot be used together") +} + +func TestRemovablePortsRequiresPortID(t *testing.T) { + got := removablePorts([]*devplanev1.Port{ + nil, + {PortNumber: 1234}, + testTCPPort("nport-one", 41001), + }) + + require.Len(t, got, 1) + assert.Equal(t, "nport-one", got[0].GetPortId()) +} diff --git a/pkg/cmd/ports/ports.go b/pkg/cmd/ports/ports.go index 90b61150..e0a44a08 100644 --- a/pkg/cmd/ports/ports.go +++ b/pkg/cmd/ports/ports.go @@ -70,15 +70,32 @@ func NewCmdPorts(portStore Store) *cobra.Command { cmd.Flags().BoolVar(&jsonOutput, "json", false, "output as JSON") cmd.AddCommand(NewCmdOpenPort(portStore)) + cmd.AddCommand(NewCmdClosePort(portStore)) return cmd } // Run resolves a managed instance or registered compute node and displays its ports. func Run(ctx context.Context, out io.Writer, portStore Store, nameOrID string, jsonOutput bool) error { - target, err := cmdutil.ResolveWorkspaceOrNodeWithContext(ctx, portStore, nameOrID) + _, apiPorts, err := resolveTargetPorts(ctx, portStore, nameOrID) if err != nil { return breverrors.WrapAndTrace(err) } + portInfos := toPortInfos(apiPorts) + if jsonOutput { + return writeJSON(out, portInfos) + } + return displayTables(out, nameOrID, portInfos) +} + +func resolveTargetPorts( + ctx context.Context, + portStore Store, + nameOrID string, +) (*cmdutil.WorkspaceOrNode, []*devplanev1.Port, error) { + target, err := cmdutil.ResolveWorkspaceOrNodeWithContext(ctx, portStore, nameOrID) + if err != nil { + return nil, nil, breverrors.WrapAndTrace(err) + } var apiPorts []*devplanev1.Port if target.Workspace != nil { @@ -87,7 +104,7 @@ func Run(ctx context.Context, out io.Writer, portStore Store, nameOrID string, j EnvironmentId: target.Workspace.ID, })) if err != nil { - return fmt.Errorf("get ports for instance %q: %w", nameOrID, err) + return nil, nil, fmt.Errorf("get ports for instance %q: %w", nameOrID, err) } var networkInfo *devplanev1.EnvironmentNetworkInfo if resp != nil && resp.Msg != nil { @@ -98,7 +115,7 @@ func Run(ctx context.Context, out io.Writer, portStore Store, nameOrID string, j if networkInfo == nil || (networkInfo.GetStatus() == devplanev1.NetworkMemberStatus_NETWORK_MEMBER_STATUS_UNSPECIFIED && len(networkInfo.GetPorts()) == 0) { - return breverrors.NewValidationError(fmt.Sprintf( + return nil, nil, breverrors.NewValidationError(fmt.Sprintf( "cannot list ports for instance %q: no Skybridge network member is available; "+ "the instance may still be provisioning or may use legacy network access. "+ "Try again when it is running, or view legacy secure links and firewall rules in the Brev console", @@ -109,11 +126,7 @@ func Run(ctx context.Context, out io.Writer, portStore Store, nameOrID string, j } else if target.Node != nil { apiPorts = target.Node.GetPorts() } - portInfos := toPortInfos(apiPorts) - if jsonOutput { - return writeJSON(out, portInfos) - } - return displayTables(out, nameOrID, portInfos) + return target, apiPorts, nil } func toPortInfos(apiPorts []*devplanev1.Port) []PortInfo {