From a32b156d25777b30aba027030ff28377e6718483 Mon Sep 17 00:00:00 2001 From: Tevin Flores Date: Thu, 29 May 2025 15:18:19 -0400 Subject: [PATCH 1/3] This commit adds the "findSingleUser" helper function that centralizes and standardizes user lookup operations across CLI commands. The function: Takes username and ID flags from commands Handles error checking and validation Ensures exactly one user is found Provides consistent error messages The function eliminates duplicate code across multiple command files (users, nodes, preauthkeys). --- cmd/headscale/cli/nodes.go | 115 ++++++++++++------------------ cmd/headscale/cli/preauthkeys.go | 49 ++++++------- cmd/headscale/cli/user_helpers.go | 84 ++++++++++++++++++++++ cmd/headscale/cli/users.go | 77 ++------------------ 4 files changed, 163 insertions(+), 162 deletions(-) create mode 100644 cmd/headscale/cli/user_helpers.go diff --git a/cmd/headscale/cli/nodes.go b/cmd/headscale/cli/nodes.go index 00d803b2..e6e90402 100644 --- a/cmd/headscale/cli/nodes.go +++ b/cmd/headscale/cli/nodes.go @@ -21,84 +21,58 @@ import ( func init() { rootCmd.AddCommand(nodeCmd) - listNodesCmd.Flags().StringP("user", "u", "", "Filter by user") listNodesCmd.Flags().BoolP("tags", "t", false, "Show tags") - - listNodesCmd.Flags().StringP("namespace", "n", "", "User") - listNodesNamespaceFlag := listNodesCmd.Flags().Lookup("namespace") - listNodesNamespaceFlag.Deprecated = deprecateNamespaceMessage - listNodesNamespaceFlag.Hidden = true + usernameAndIDFlag(listNodesCmd) nodeCmd.AddCommand(listNodesCmd) - - listNodeRoutesCmd.Flags().Uint64P("identifier", "i", 0, "Node identifier (ID)") + listNodeRoutesCmd.Flags().Uint64P("node-id", "N", 0, "Node identifier (ID)") nodeCmd.AddCommand(listNodeRoutesCmd) - - registerNodeCmd.Flags().StringP("user", "u", "", "User") - - registerNodeCmd.Flags().StringP("namespace", "n", "", "User") - registerNodeNamespaceFlag := registerNodeCmd.Flags().Lookup("namespace") - registerNodeNamespaceFlag.Deprecated = deprecateNamespaceMessage - registerNodeNamespaceFlag.Hidden = true - - err := registerNodeCmd.MarkFlagRequired("user") - if err != nil { - log.Fatal(err.Error()) - } + usernameAndIDFlag(registerNodeCmd) registerNodeCmd.Flags().StringP("key", "k", "", "Key") - err = registerNodeCmd.MarkFlagRequired("key") + err := registerNodeCmd.MarkFlagRequired("key") if err != nil { log.Fatal(err.Error()) } nodeCmd.AddCommand(registerNodeCmd) - expireNodeCmd.Flags().Uint64P("identifier", "i", 0, "Node identifier (ID)") - err = expireNodeCmd.MarkFlagRequired("identifier") + expireNodeCmd.Flags().Uint64P("node-id", "N", 0, "Node identifier (ID)") + err = expireNodeCmd.MarkFlagRequired("node-id") if err != nil { log.Fatal(err.Error()) } nodeCmd.AddCommand(expireNodeCmd) - renameNodeCmd.Flags().Uint64P("identifier", "i", 0, "Node identifier (ID)") - err = renameNodeCmd.MarkFlagRequired("identifier") + renameNodeCmd.Flags().Uint64P("node-id", "N", 0, "Node identifier (ID)") + err = renameNodeCmd.MarkFlagRequired("node-id") if err != nil { log.Fatal(err.Error()) } nodeCmd.AddCommand(renameNodeCmd) - deleteNodeCmd.Flags().Uint64P("identifier", "i", 0, "Node identifier (ID)") - err = deleteNodeCmd.MarkFlagRequired("identifier") + deleteNodeCmd.Flags().Uint64P("node-id", "N", 0, "Node identifier (ID)") + err = deleteNodeCmd.MarkFlagRequired("node-id") if err != nil { log.Fatal(err.Error()) } nodeCmd.AddCommand(deleteNodeCmd) - moveNodeCmd.Flags().Uint64P("identifier", "i", 0, "Node identifier (ID)") + moveNodeCmd.Flags().Uint64P("node-id", "N", 0, "Node identifier (ID)") - err = moveNodeCmd.MarkFlagRequired("identifier") + err = moveNodeCmd.MarkFlagRequired("node-id") if err != nil { log.Fatal(err.Error()) } - moveNodeCmd.Flags().Uint64P("user", "u", 0, "New user") + usernameAndIDFlag(moveNodeCmd, "Target user ID to move the node to", "Target username to move the node to") - moveNodeCmd.Flags().StringP("namespace", "n", "", "User") - moveNodeNamespaceFlag := moveNodeCmd.Flags().Lookup("namespace") - moveNodeNamespaceFlag.Deprecated = deprecateNamespaceMessage - moveNodeNamespaceFlag.Hidden = true - - err = moveNodeCmd.MarkFlagRequired("user") - if err != nil { - log.Fatal(err.Error()) - } nodeCmd.AddCommand(moveNodeCmd) - tagCmd.Flags().Uint64P("identifier", "i", 0, "Node identifier (ID)") - tagCmd.MarkFlagRequired("identifier") + tagCmd.Flags().Uint64P("node-id", "N", 0, "Node identifier (ID)") + tagCmd.MarkFlagRequired("node-id") tagCmd.Flags().StringSliceP("tags", "t", []string{}, "List of tags to add to the node") nodeCmd.AddCommand(tagCmd) - approveRoutesCmd.Flags().Uint64P("identifier", "i", 0, "Node identifier (ID)") - approveRoutesCmd.MarkFlagRequired("identifier") + approveRoutesCmd.Flags().Uint64P("node-id", "N", 0, "Node identifier (ID)") + approveRoutesCmd.MarkFlagRequired("node-id") approveRoutesCmd.Flags().StringSliceP("routes", "r", []string{}, `List of routes that will be approved (comma-separated, e.g. "10.0.0.0/8,192.168.0.0/24" or empty string to remove all approved routes)`) nodeCmd.AddCommand(approveRoutesCmd) @@ -116,15 +90,17 @@ var registerNodeCmd = &cobra.Command{ Short: "Registers a node to your network", Run: func(cmd *cobra.Command, args []string) { output, _ := cmd.Flags().GetString("output") - user, err := cmd.Flags().GetString("user") - if err != nil { - ErrorOutput(err, fmt.Sprintf("Error getting user: %s", err), output) - } ctx, client, conn, cancel := newHeadscaleCLIWithConfig() defer cancel() defer conn.Close() + user, err := findSingleUser(ctx, client, cmd, "register Node", output) + if err != nil { + // The helper already calls ErrorOutput, so we can just return + return + } + registrationID, err := cmd.Flags().GetString("key") if err != nil { ErrorOutput( @@ -136,7 +112,7 @@ var registerNodeCmd = &cobra.Command{ request := &v1.RegisterNodeRequest{ Key: registrationID, - User: user, + User: user.GetName(), } response, err := client.RegisterNode(ctx, request) @@ -163,10 +139,6 @@ var listNodesCmd = &cobra.Command{ Aliases: []string{"ls", "show"}, Run: func(cmd *cobra.Command, args []string) { output, _ := cmd.Flags().GetString("output") - user, err := cmd.Flags().GetString("user") - if err != nil { - ErrorOutput(err, fmt.Sprintf("Error getting user: %s", err), output) - } showTags, err := cmd.Flags().GetBool("tags") if err != nil { ErrorOutput(err, fmt.Sprintf("Error getting tags flag: %s", err), output) @@ -176,8 +148,20 @@ var listNodesCmd = &cobra.Command{ defer cancel() defer conn.Close() - request := &v1.ListNodesRequest{ - User: user, + // Check if user identifier flags are provided + id, _ := cmd.Flags().GetInt64("identifier") + username, _ := cmd.Flags().GetString("name") + + request := &v1.ListNodesRequest{} + + // Only filter by user if user flags are provided + if id >= 0 || username != "" { + user, err := findSingleUser(ctx, client, cmd, "list", output) + if err != nil { + // The helper already calls ErrorOutput, so we can just return + return + } + request.User = user.GetName() } response, err := client.ListNodes(ctx, request) @@ -193,7 +177,7 @@ var listNodesCmd = &cobra.Command{ SuccessOutput(response.GetNodes(), "", output) } - tableData, err := nodesToPtables(user, showTags, response.GetNodes()) + tableData, err := nodesToPtables(request.User, showTags, response.GetNodes()) if err != nil { ErrorOutput(err, fmt.Sprintf("Error converting to table: %s", err), output) } @@ -464,7 +448,7 @@ var moveNodeCmd = &cobra.Command{ Run: func(cmd *cobra.Command, args []string) { output, _ := cmd.Flags().GetString("output") - identifier, err := cmd.Flags().GetUint64("identifier") + identifier, err := cmd.Flags().GetUint64("node-id") if err != nil { ErrorOutput( err, @@ -475,21 +459,16 @@ var moveNodeCmd = &cobra.Command{ return } - user, err := cmd.Flags().GetUint64("user") - if err != nil { - ErrorOutput( - err, - fmt.Sprintf("Error getting user: %s", err), - output, - ) - - return - } - ctx, client, conn, cancel := newHeadscaleCLIWithConfig() defer cancel() defer conn.Close() + user, err := findSingleUser(ctx, client, cmd, "move Node", output) + if err != nil { + // The helper already calls ErrorOutput, so we can just return + return + } + getRequest := &v1.GetNodeRequest{ NodeId: identifier, } @@ -510,7 +489,7 @@ var moveNodeCmd = &cobra.Command{ moveRequest := &v1.MoveNodeRequest{ NodeId: identifier, - User: user, + User: user.GetId(), } moveResponse, err := client.MoveNode(ctx, moveRequest) diff --git a/cmd/headscale/cli/preauthkeys.go b/cmd/headscale/cli/preauthkeys.go index c0c08831..620a349c 100644 --- a/cmd/headscale/cli/preauthkeys.go +++ b/cmd/headscale/cli/preauthkeys.go @@ -20,20 +20,20 @@ const ( func init() { rootCmd.AddCommand(preauthkeysCmd) - preauthkeysCmd.PersistentFlags().Uint64P("user", "u", 0, "User identifier (ID)") - preauthkeysCmd.PersistentFlags().StringP("namespace", "n", "", "User") + preauthkeysCmd.PersistentFlags().String("namespace", "", "User") pakNamespaceFlag := preauthkeysCmd.PersistentFlags().Lookup("namespace") pakNamespaceFlag.Deprecated = deprecateNamespaceMessage pakNamespaceFlag.Hidden = true - err := preauthkeysCmd.MarkPersistentFlagRequired("user") - if err != nil { - log.Fatal().Err(err).Msg("") - } preauthkeysCmd.AddCommand(listPreAuthKeys) preauthkeysCmd.AddCommand(createPreAuthKeyCmd) preauthkeysCmd.AddCommand(expirePreAuthKeyCmd) + + usernameAndIDFlag(listPreAuthKeys) + usernameAndIDFlag(createPreAuthKeyCmd) + usernameAndIDFlag(expirePreAuthKeyCmd) + createPreAuthKeyCmd.PersistentFlags(). Bool("reusable", false, "Make the preauthkey reusable") createPreAuthKeyCmd.PersistentFlags(). @@ -57,17 +57,17 @@ var listPreAuthKeys = &cobra.Command{ Run: func(cmd *cobra.Command, args []string) { output, _ := cmd.Flags().GetString("output") - user, err := cmd.Flags().GetUint64("user") - if err != nil { - ErrorOutput(err, fmt.Sprintf("Error getting user: %s", err), output) - } - ctx, client, conn, cancel := newHeadscaleCLIWithConfig() defer cancel() defer conn.Close() + user, err := findSingleUser(ctx, client, cmd, "list", output) + if err != nil { + return + } + request := &v1.ListPreAuthKeysRequest{ - User: user, + User: user.GetId(), } response, err := client.ListPreAuthKeys(ctx, request) @@ -141,9 +141,13 @@ var createPreAuthKeyCmd = &cobra.Command{ Run: func(cmd *cobra.Command, args []string) { output, _ := cmd.Flags().GetString("output") - user, err := cmd.Flags().GetUint64("user") + ctx, client, conn, cancel := newHeadscaleCLIWithConfig() + defer cancel() + defer conn.Close() + + user, err := findSingleUser(ctx, client, cmd, "list", output) if err != nil { - ErrorOutput(err, fmt.Sprintf("Error getting user: %s", err), output) + return } reusable, _ := cmd.Flags().GetBool("reusable") @@ -151,7 +155,7 @@ var createPreAuthKeyCmd = &cobra.Command{ tags, _ := cmd.Flags().GetStringSlice("tags") request := &v1.CreatePreAuthKeyRequest{ - User: user, + User: user.GetId(), Reusable: reusable, Ephemeral: ephemeral, AclTags: tags, @@ -176,10 +180,6 @@ var createPreAuthKeyCmd = &cobra.Command{ request.Expiration = timestamppb.New(expiration) - ctx, client, conn, cancel := newHeadscaleCLIWithConfig() - defer cancel() - defer conn.Close() - response, err := client.CreatePreAuthKey(ctx, request) if err != nil { ErrorOutput( @@ -206,17 +206,18 @@ var expirePreAuthKeyCmd = &cobra.Command{ }, Run: func(cmd *cobra.Command, args []string) { output, _ := cmd.Flags().GetString("output") - user, err := cmd.Flags().GetUint64("user") - if err != nil { - ErrorOutput(err, fmt.Sprintf("Error getting user: %s", err), output) - } ctx, client, conn, cancel := newHeadscaleCLIWithConfig() defer cancel() defer conn.Close() + user, err := findSingleUser(ctx, client, cmd, "list", output) + if err != nil { + return + } + request := &v1.ExpirePreAuthKeyRequest{ - User: user, + User: user.GetId(), Key: args[0], } diff --git a/cmd/headscale/cli/user_helpers.go b/cmd/headscale/cli/user_helpers.go new file mode 100644 index 00000000..678cdabd --- /dev/null +++ b/cmd/headscale/cli/user_helpers.go @@ -0,0 +1,84 @@ +package cli + +import ( + "context" + "errors" + "fmt" + + v1 "github.com/juanfont/headscale/gen/go/headscale/v1" + "github.com/spf13/cobra" + "google.golang.org/grpc/status" +) + +// usernameAndIDFlag adds the common user identification flags to a command +func usernameAndIDFlag(cmd *cobra.Command, opts ...string) { + idHelp := "User identifier (ID)" + nameHelp := "Username" + + if len(opts) > 0 && opts[0] != "" { + idHelp = opts[0] + } + if len(opts) > 1 && opts[1] != "" { + nameHelp = opts[1] + } + cmd.PersistentFlags().Int64P("identifier", "i", -1, idHelp) + cmd.PersistentFlags().StringP("name", "n", "", nameHelp) +} + +// usernameAndIDFromFlag returns the username and ID from the flags of the command. +// If both are empty, it will exit the program with an error. +func usernameAndIDFromFlag(cmd *cobra.Command) (uint64, string) { + username, _ := cmd.Flags().GetString("name") + identifier, _ := cmd.Flags().GetInt64("identifier") + if username == "" && identifier < 0 { + err := errors.New("--name or --identifier flag is required") + ErrorOutput( + err, + fmt.Sprintf( + "User identification error: %s", + status.Convert(err).Message(), + ), + "", + ) + } + + return uint64(identifier), username +} + +// findSingleUser takes command flags and returns a single user +// It handles all error checking and ensures exactly one user is found +func findSingleUser( + ctx context.Context, + client v1.HeadscaleServiceClient, + cmd *cobra.Command, + operationName string, + output string, +) (*v1.User, error) { + id, username := usernameAndIDFromFlag(cmd) + listReq := &v1.ListUsersRequest{ + Name: username, + Id: id, + } + + users, err := client.ListUsers(ctx, listReq) + if err != nil { + ErrorOutput( + err, + fmt.Sprintf("Error: %s", status.Convert(err).Message()), + output, + ) + return nil, err + } + + if len(users.GetUsers()) != 1 { + err := fmt.Errorf("Unable to determine user to %s, query returned multiple users, use ID", operationName) + ErrorOutput( + err, + fmt.Sprintf("Error: %s", status.Convert(err).Message()), + output, + ) + return nil, err + } + + return users.GetUsers()[0], nil +} diff --git a/cmd/headscale/cli/users.go b/cmd/headscale/cli/users.go index b5f1bc49..e3d138fc 100644 --- a/cmd/headscale/cli/users.go +++ b/cmd/headscale/cli/users.go @@ -13,31 +13,6 @@ import ( "google.golang.org/grpc/status" ) -func usernameAndIDFlag(cmd *cobra.Command) { - cmd.Flags().Int64P("identifier", "i", -1, "User identifier (ID)") - cmd.Flags().StringP("name", "n", "", "Username") -} - -// usernameAndIDFromFlag returns the username and ID from the flags of the command. -// If both are empty, it will exit the program with an error. -func usernameAndIDFromFlag(cmd *cobra.Command) (uint64, string) { - username, _ := cmd.Flags().GetString("name") - identifier, _ := cmd.Flags().GetInt64("identifier") - if username == "" && identifier < 0 { - err := errors.New("--name or --identifier flag is required") - ErrorOutput( - err, - fmt.Sprintf( - "Cannot rename user: %s", - status.Convert(err).Message(), - ), - "", - ) - } - - return uint64(identifier), username -} - func init() { rootCmd.AddCommand(userCmd) userCmd.AddCommand(createUserCmd) @@ -133,36 +108,16 @@ var destroyUserCmd = &cobra.Command{ Run: func(cmd *cobra.Command, args []string) { output, _ := cmd.Flags().GetString("output") - id, username := usernameAndIDFromFlag(cmd) - request := &v1.ListUsersRequest{ - Name: username, - Id: id, - } - ctx, client, conn, cancel := newHeadscaleCLIWithConfig() defer cancel() defer conn.Close() - users, err := client.ListUsers(ctx, request) + user, err := findSingleUser(ctx, client, cmd, "rename", output) if err != nil { - ErrorOutput( - err, - fmt.Sprintf("Error: %s", status.Convert(err).Message()), - output, - ) + // The helper already calls ErrorOutput, so we can just return + return } - if len(users.GetUsers()) != 1 { - err := fmt.Errorf("Unable to determine user to delete, query returned multiple users, use ID") - ErrorOutput( - err, - fmt.Sprintf("Error: %s", status.Convert(err).Message()), - output, - ) - } - - user := users.GetUsers()[0] - confirm := false force, _ := cmd.Flags().GetBool("force") if !force { @@ -277,34 +232,16 @@ var renameUserCmd = &cobra.Command{ defer cancel() defer conn.Close() - id, username := usernameAndIDFromFlag(cmd) - listReq := &v1.ListUsersRequest{ - Name: username, - Id: id, - } - - users, err := client.ListUsers(ctx, listReq) + user, err := findSingleUser(ctx, client, cmd, "rename", output) if err != nil { - ErrorOutput( - err, - fmt.Sprintf("Error: %s", status.Convert(err).Message()), - output, - ) - } - - if len(users.GetUsers()) != 1 { - err := fmt.Errorf("Unable to determine user to delete, query returned multiple users, use ID") - ErrorOutput( - err, - fmt.Sprintf("Error: %s", status.Convert(err).Message()), - output, - ) + // The helper already calls ErrorOutput, so we can just return + return } newName, _ := cmd.Flags().GetString("new-name") renameReq := &v1.RenameUserRequest{ - OldId: id, + OldId: user.GetId(), NewName: newName, } From 78d89dff6b65fccec1a663390cd0f5a06694e129 Mon Sep 17 00:00:00 2001 From: Tevin Flores Date: Thu, 29 May 2025 15:42:23 -0400 Subject: [PATCH 2/3] Improve clarity and consistency of error messages by using descriptive operation names when calling findSingleUser across all CLI commands. --- cmd/headscale/cli/nodes.go | 18 +++++++++--------- cmd/headscale/cli/preauthkeys.go | 9 ++++++--- cmd/headscale/cli/users.go | 4 ++-- 3 files changed, 17 insertions(+), 14 deletions(-) diff --git a/cmd/headscale/cli/nodes.go b/cmd/headscale/cli/nodes.go index e6e90402..cf2914a3 100644 --- a/cmd/headscale/cli/nodes.go +++ b/cmd/headscale/cli/nodes.go @@ -95,7 +95,7 @@ var registerNodeCmd = &cobra.Command{ defer cancel() defer conn.Close() - user, err := findSingleUser(ctx, client, cmd, "register Node", output) + user, err := findSingleUser(ctx, client, cmd, "registerNodeCmd", output) if err != nil { // The helper already calls ErrorOutput, so we can just return return @@ -156,7 +156,7 @@ var listNodesCmd = &cobra.Command{ // Only filter by user if user flags are provided if id >= 0 || username != "" { - user, err := findSingleUser(ctx, client, cmd, "list", output) + user, err := findSingleUser(ctx, client, cmd, "listNodesCmd", output) if err != nil { // The helper already calls ErrorOutput, so we can just return return @@ -199,7 +199,7 @@ var listNodeRoutesCmd = &cobra.Command{ Aliases: []string{"lsr", "routes"}, Run: func(cmd *cobra.Command, args []string) { output, _ := cmd.Flags().GetString("output") - identifier, err := cmd.Flags().GetUint64("identifier") + identifier, err := cmd.Flags().GetUint64("node-id") if err != nil { ErrorOutput( err, @@ -267,7 +267,7 @@ var expireNodeCmd = &cobra.Command{ Run: func(cmd *cobra.Command, args []string) { output, _ := cmd.Flags().GetString("output") - identifier, err := cmd.Flags().GetUint64("identifier") + identifier, err := cmd.Flags().GetUint64("node-id") if err != nil { ErrorOutput( err, @@ -310,7 +310,7 @@ var renameNodeCmd = &cobra.Command{ Run: func(cmd *cobra.Command, args []string) { output, _ := cmd.Flags().GetString("output") - identifier, err := cmd.Flags().GetUint64("identifier") + identifier, err := cmd.Flags().GetUint64("node-id") if err != nil { ErrorOutput( err, @@ -359,7 +359,7 @@ var deleteNodeCmd = &cobra.Command{ Run: func(cmd *cobra.Command, args []string) { output, _ := cmd.Flags().GetString("output") - identifier, err := cmd.Flags().GetUint64("identifier") + identifier, err := cmd.Flags().GetUint64("node-id") if err != nil { ErrorOutput( err, @@ -463,7 +463,7 @@ var moveNodeCmd = &cobra.Command{ defer cancel() defer conn.Close() - user, err := findSingleUser(ctx, client, cmd, "move Node", output) + user, err := findSingleUser(ctx, client, cmd, "moveNodeCmd", output) if err != nil { // The helper already calls ErrorOutput, so we can just return return @@ -745,7 +745,7 @@ var tagCmd = &cobra.Command{ defer conn.Close() // retrieve flags from CLI - identifier, err := cmd.Flags().GetUint64("identifier") + identifier, err := cmd.Flags().GetUint64("node-id") if err != nil { ErrorOutput( err, @@ -802,7 +802,7 @@ var approveRoutesCmd = &cobra.Command{ defer conn.Close() // retrieve flags from CLI - identifier, err := cmd.Flags().GetUint64("identifier") + identifier, err := cmd.Flags().GetUint64("node-id") if err != nil { ErrorOutput( err, diff --git a/cmd/headscale/cli/preauthkeys.go b/cmd/headscale/cli/preauthkeys.go index 620a349c..80910663 100644 --- a/cmd/headscale/cli/preauthkeys.go +++ b/cmd/headscale/cli/preauthkeys.go @@ -61,8 +61,9 @@ var listPreAuthKeys = &cobra.Command{ defer cancel() defer conn.Close() - user, err := findSingleUser(ctx, client, cmd, "list", output) + user, err := findSingleUser(ctx, client, cmd, "listPreAuthKeys", output) if err != nil { + // The helper already calls ErrorOutput, so we can just return return } @@ -145,8 +146,9 @@ var createPreAuthKeyCmd = &cobra.Command{ defer cancel() defer conn.Close() - user, err := findSingleUser(ctx, client, cmd, "list", output) + user, err := findSingleUser(ctx, client, cmd, "createPreAuthKeyCmd", output) if err != nil { + // The helper already calls ErrorOutput, so we can just return return } @@ -211,8 +213,9 @@ var expirePreAuthKeyCmd = &cobra.Command{ defer cancel() defer conn.Close() - user, err := findSingleUser(ctx, client, cmd, "list", output) + user, err := findSingleUser(ctx, client, cmd, "expirePreAuthKeyCmd", output) if err != nil { + // The helper already calls ErrorOutput, so we can just return return } diff --git a/cmd/headscale/cli/users.go b/cmd/headscale/cli/users.go index e3d138fc..638c077d 100644 --- a/cmd/headscale/cli/users.go +++ b/cmd/headscale/cli/users.go @@ -112,7 +112,7 @@ var destroyUserCmd = &cobra.Command{ defer cancel() defer conn.Close() - user, err := findSingleUser(ctx, client, cmd, "rename", output) + user, err := findSingleUser(ctx, client, cmd, "destroyUserCmd", output) if err != nil { // The helper already calls ErrorOutput, so we can just return return @@ -232,7 +232,7 @@ var renameUserCmd = &cobra.Command{ defer cancel() defer conn.Close() - user, err := findSingleUser(ctx, client, cmd, "rename", output) + user, err := findSingleUser(ctx, client, cmd, "renameUserCmd", output) if err != nil { // The helper already calls ErrorOutput, so we can just return return From e54bb9763c5edfe8a9abe87d60940dfe9bf54de0 Mon Sep 17 00:00:00 2001 From: Tevin Flores Date: Wed, 4 Jun 2025 13:26:56 -0400 Subject: [PATCH 3/3] This change standardizes the command line flag naming conventions across the headscale CLI: User-related operations now consistently use -u/--user (changed from -n/--name) Node-related operations now consistently use -n/--node-id (changed from -N/--identifier) User ID operations continue to use -i/--identifier The error message for user identification has also been simplified for better clarity. Integration tests have been updated to use the new flag naming conventions, ensuring all test cases properly exercise the updated CLI interface. This includes modifying test fixtures and command invocations to align with the standardized flags. --- cmd/headscale/cli/debug.go | 21 +++++------ cmd/headscale/cli/nodes.go | 16 ++++----- cmd/headscale/cli/user_helpers.go | 8 ++--- cmd/headscale/cli/users.go | 2 +- integration/auth_key_test.go | 2 +- integration/cli_test.go | 58 +++++++++++++++---------------- integration/general_test.go | 6 ++-- integration/hsic/hsic.go | 14 ++++++-- integration/scenario.go | 4 +-- 9 files changed, 67 insertions(+), 64 deletions(-) diff --git a/cmd/headscale/cli/debug.go b/cmd/headscale/cli/debug.go index 41b46fb0..6f9eb865 100644 --- a/cmd/headscale/cli/debug.go +++ b/cmd/headscale/cli/debug.go @@ -27,17 +27,11 @@ func init() { if err != nil { log.Fatal().Err(err).Msg("") } - createNodeCmd.Flags().StringP("user", "u", "", "User") - + usernameAndIDFlag(createNodeCmd) createNodeCmd.Flags().StringP("namespace", "n", "", "User") createNodeNamespaceFlag := createNodeCmd.Flags().Lookup("namespace") createNodeNamespaceFlag.Deprecated = deprecateNamespaceMessage createNodeNamespaceFlag.Hidden = true - - err = createNodeCmd.MarkFlagRequired("user") - if err != nil { - log.Fatal().Err(err).Msg("") - } createNodeCmd.Flags().StringP("key", "k", "", "Key") err = createNodeCmd.MarkFlagRequired("key") if err != nil { @@ -61,15 +55,16 @@ var createNodeCmd = &cobra.Command{ Run: func(cmd *cobra.Command, args []string) { output, _ := cmd.Flags().GetString("output") - user, err := cmd.Flags().GetString("user") - if err != nil { - ErrorOutput(err, fmt.Sprintf("Error getting user: %s", err), output) - } - ctx, client, conn, cancel := newHeadscaleCLIWithConfig() defer cancel() defer conn.Close() + user, err := findSingleUser(ctx, client, cmd, "debug-createNodeCmd", output) + if err != nil { + // The helper already calls ErrorOutput, so we can just return + return + } + name, err := cmd.Flags().GetString("name") if err != nil { ErrorOutput( @@ -109,7 +104,7 @@ var createNodeCmd = &cobra.Command{ request := &v1.DebugCreateNodeRequest{ Key: registrationID, Name: name, - User: user, + User: user.GetName(), Routes: routes, } diff --git a/cmd/headscale/cli/nodes.go b/cmd/headscale/cli/nodes.go index cf2914a3..952b9a57 100644 --- a/cmd/headscale/cli/nodes.go +++ b/cmd/headscale/cli/nodes.go @@ -24,7 +24,7 @@ func init() { listNodesCmd.Flags().BoolP("tags", "t", false, "Show tags") usernameAndIDFlag(listNodesCmd) nodeCmd.AddCommand(listNodesCmd) - listNodeRoutesCmd.Flags().Uint64P("node-id", "N", 0, "Node identifier (ID)") + listNodeRoutesCmd.Flags().Uint64P("node-id", "n", 0, "Node identifier (ID)") nodeCmd.AddCommand(listNodeRoutesCmd) usernameAndIDFlag(registerNodeCmd) registerNodeCmd.Flags().StringP("key", "k", "", "Key") @@ -34,28 +34,28 @@ func init() { } nodeCmd.AddCommand(registerNodeCmd) - expireNodeCmd.Flags().Uint64P("node-id", "N", 0, "Node identifier (ID)") + expireNodeCmd.Flags().Uint64P("node-id", "n", 0, "Node identifier (ID)") err = expireNodeCmd.MarkFlagRequired("node-id") if err != nil { log.Fatal(err.Error()) } nodeCmd.AddCommand(expireNodeCmd) - renameNodeCmd.Flags().Uint64P("node-id", "N", 0, "Node identifier (ID)") + renameNodeCmd.Flags().Uint64P("node-id", "n", 0, "Node identifier (ID)") err = renameNodeCmd.MarkFlagRequired("node-id") if err != nil { log.Fatal(err.Error()) } nodeCmd.AddCommand(renameNodeCmd) - deleteNodeCmd.Flags().Uint64P("node-id", "N", 0, "Node identifier (ID)") + deleteNodeCmd.Flags().Uint64P("node-id", "n", 0, "Node identifier (ID)") err = deleteNodeCmd.MarkFlagRequired("node-id") if err != nil { log.Fatal(err.Error()) } nodeCmd.AddCommand(deleteNodeCmd) - moveNodeCmd.Flags().Uint64P("node-id", "N", 0, "Node identifier (ID)") + moveNodeCmd.Flags().Uint64P("node-id", "n", 0, "Node identifier (ID)") err = moveNodeCmd.MarkFlagRequired("node-id") if err != nil { @@ -66,12 +66,12 @@ func init() { nodeCmd.AddCommand(moveNodeCmd) - tagCmd.Flags().Uint64P("node-id", "N", 0, "Node identifier (ID)") + tagCmd.Flags().Uint64P("node-id", "n", 0, "Node identifier (ID)") tagCmd.MarkFlagRequired("node-id") tagCmd.Flags().StringSliceP("tags", "t", []string{}, "List of tags to add to the node") nodeCmd.AddCommand(tagCmd) - approveRoutesCmd.Flags().Uint64P("node-id", "N", 0, "Node identifier (ID)") + approveRoutesCmd.Flags().Uint64P("node-id", "n", 0, "Node identifier (ID)") approveRoutesCmd.MarkFlagRequired("node-id") approveRoutesCmd.Flags().StringSliceP("routes", "r", []string{}, `List of routes that will be approved (comma-separated, e.g. "10.0.0.0/8,192.168.0.0/24" or empty string to remove all approved routes)`) nodeCmd.AddCommand(approveRoutesCmd) @@ -150,7 +150,7 @@ var listNodesCmd = &cobra.Command{ // Check if user identifier flags are provided id, _ := cmd.Flags().GetInt64("identifier") - username, _ := cmd.Flags().GetString("name") + username, _ := cmd.Flags().GetString("user") request := &v1.ListNodesRequest{} diff --git a/cmd/headscale/cli/user_helpers.go b/cmd/headscale/cli/user_helpers.go index 678cdabd..6e5f6909 100644 --- a/cmd/headscale/cli/user_helpers.go +++ b/cmd/headscale/cli/user_helpers.go @@ -22,16 +22,16 @@ func usernameAndIDFlag(cmd *cobra.Command, opts ...string) { nameHelp = opts[1] } cmd.PersistentFlags().Int64P("identifier", "i", -1, idHelp) - cmd.PersistentFlags().StringP("name", "n", "", nameHelp) + cmd.PersistentFlags().StringP("user", "u", "", nameHelp) } // usernameAndIDFromFlag returns the username and ID from the flags of the command. // If both are empty, it will exit the program with an error. func usernameAndIDFromFlag(cmd *cobra.Command) (uint64, string) { - username, _ := cmd.Flags().GetString("name") + username, _ := cmd.Flags().GetString("user") identifier, _ := cmd.Flags().GetInt64("identifier") if username == "" && identifier < 0 { - err := errors.New("--name or --identifier flag is required") + err := errors.New("--user or --identifier flag is required") ErrorOutput( err, fmt.Sprintf( @@ -71,7 +71,7 @@ func findSingleUser( } if len(users.GetUsers()) != 1 { - err := fmt.Errorf("Unable to determine user to %s, query returned multiple users, use ID", operationName) + err := fmt.Errorf("Unable to determine user for %s", operationName) ErrorOutput( err, fmt.Sprintf("Error: %s", status.Convert(err).Message()), diff --git a/cmd/headscale/cli/users.go b/cmd/headscale/cli/users.go index 638c077d..e957ddba 100644 --- a/cmd/headscale/cli/users.go +++ b/cmd/headscale/cli/users.go @@ -168,7 +168,7 @@ var listUsersCmd = &cobra.Command{ request := &v1.ListUsersRequest{} id, _ := cmd.Flags().GetInt64("identifier") - username, _ := cmd.Flags().GetString("name") + username, _ := cmd.Flags().GetString("user") email, _ := cmd.Flags().GetString("email") // filter by one param at most diff --git a/integration/auth_key_test.go b/integration/auth_key_test.go index d54ff593..4e0d30ac 100644 --- a/integration/auth_key_test.go +++ b/integration/auth_key_test.go @@ -342,7 +342,7 @@ func TestAuthKeyLogoutAndReloginSameUserExpiredKey(t *testing.T) { []string{ "headscale", "preauthkeys", - "--user", + "--identifier", strconv.FormatUint(userMap[userName].GetId(), 10), "expire", key.Key, diff --git a/integration/cli_test.go b/integration/cli_test.go index 2cff0500..9a7fd113 100644 --- a/integration/cli_test.go +++ b/integration/cli_test.go @@ -18,8 +18,8 @@ import ( "github.com/juanfont/headscale/integration/tsic" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "tailscale.com/tailcfg" "golang.org/x/exp/slices" + "tailscale.com/tailcfg" ) func executeAndUnmarshal[T any](headscale ControlServer, command []string, result T) error { @@ -128,7 +128,7 @@ func TestUserCommand(t *testing.T) { "list", "--output", "json", - "--name=user1", + "--user=user1", }, &listByUsername, ) @@ -219,7 +219,7 @@ func TestUserCommand(t *testing.T) { "users", "destroy", "--force", - "--name=newname", + "--user=newname", }, ) assert.Nil(t, err) @@ -272,7 +272,7 @@ func TestPreAuthKeyCommand(t *testing.T) { []string{ "headscale", "preauthkeys", - "--user", + "-i", "1", "create", "--reusable", @@ -298,7 +298,7 @@ func TestPreAuthKeyCommand(t *testing.T) { []string{ "headscale", "preauthkeys", - "--user", + "-i", "1", "list", "--output", @@ -355,7 +355,7 @@ func TestPreAuthKeyCommand(t *testing.T) { []string{ "headscale", "preauthkeys", - "--user", + "-i", "1", "expire", listedPreAuthKeys[1].GetKey(), @@ -369,7 +369,7 @@ func TestPreAuthKeyCommand(t *testing.T) { []string{ "headscale", "preauthkeys", - "--user", + "-i", "1", "list", "--output", @@ -409,7 +409,7 @@ func TestPreAuthKeyCommandWithoutExpiry(t *testing.T) { []string{ "headscale", "preauthkeys", - "--user", + "-i", "1", "create", "--reusable", @@ -426,7 +426,7 @@ func TestPreAuthKeyCommandWithoutExpiry(t *testing.T) { []string{ "headscale", "preauthkeys", - "--user", + "-i", "1", "list", "--output", @@ -471,7 +471,7 @@ func TestPreAuthKeyCommandReusableEphemeral(t *testing.T) { []string{ "headscale", "preauthkeys", - "--user", + "-i", "1", "create", "--reusable=true", @@ -488,7 +488,7 @@ func TestPreAuthKeyCommandReusableEphemeral(t *testing.T) { []string{ "headscale", "preauthkeys", - "--user", + "-i", "1", "create", "--ephemeral=true", @@ -508,7 +508,7 @@ func TestPreAuthKeyCommandReusableEphemeral(t *testing.T) { []string{ "headscale", "preauthkeys", - "--user", + "-i", "1", "list", "--output", @@ -559,7 +559,7 @@ func TestPreAuthKeyCorrectUserLoggedInCommand(t *testing.T) { []string{ "headscale", "preauthkeys", - "--user", + "-i", strconv.FormatUint(u2.GetId(), 10), "create", "--reusable", @@ -860,7 +860,7 @@ func TestNodeTagCommand(t *testing.T) { "headscale", "nodes", "tag", - "-i", "1", + "-n", "1", "-t", "tag:test", "--output", "json", }, @@ -875,7 +875,7 @@ func TestNodeTagCommand(t *testing.T) { "headscale", "nodes", "tag", - "-i", "2", + "-n", "2", "-t", "wrong-tag", "--output", "json", }, @@ -913,8 +913,6 @@ func TestNodeTagCommand(t *testing.T) { ) } - - func TestNodeAdvertiseTagCommand(t *testing.T) { IntegrationSkip(t) t.Parallel() @@ -1250,7 +1248,7 @@ func TestNodeCommand(t *testing.T) { "headscale", "nodes", "delete", - "--identifier", + "--node-id", // Delete the last added machine "4", "--output", @@ -1376,7 +1374,7 @@ func TestNodeExpireCommand(t *testing.T) { "headscale", "nodes", "expire", - "--identifier", + "--node-id", fmt.Sprintf("%d", listAll[idx].GetId()), }, ) @@ -1503,7 +1501,7 @@ func TestNodeRenameCommand(t *testing.T) { "headscale", "nodes", "rename", - "--identifier", + "--node-id", fmt.Sprintf("%d", listAll[idx].GetId()), fmt.Sprintf("newnode-%d", idx+1), }, @@ -1541,7 +1539,7 @@ func TestNodeRenameCommand(t *testing.T) { "headscale", "nodes", "rename", - "--identifier", + "--node-id", fmt.Sprintf("%d", listAll[4].GetId()), strings.Repeat("t", 64), }, @@ -1642,9 +1640,9 @@ func TestNodeMoveCommand(t *testing.T) { "headscale", "nodes", "move", - "--identifier", + "--node-id", strconv.FormatUint(node.GetId(), 10), - "--user", + "--identifier", strconv.FormatUint(userMap["new-user"].GetId(), 10), "--output", "json", @@ -1680,9 +1678,9 @@ func TestNodeMoveCommand(t *testing.T) { "headscale", "nodes", "move", - "--identifier", + "--node-id", nodeID, - "--user", + "--identifier", "999", "--output", "json", @@ -1691,7 +1689,7 @@ func TestNodeMoveCommand(t *testing.T) { assert.ErrorContains( t, err, - "user not found", + "Unable to determine user", ) assert.Equal(t, node.GetUser().GetName(), "new-user") @@ -1701,9 +1699,9 @@ func TestNodeMoveCommand(t *testing.T) { "headscale", "nodes", "move", - "--identifier", + "--node-id", nodeID, - "--user", + "--identifier", strconv.FormatUint(userMap["old-user"].GetId(), 10), "--output", "json", @@ -1720,9 +1718,9 @@ func TestNodeMoveCommand(t *testing.T) { "headscale", "nodes", "move", - "--identifier", + "--node-id", nodeID, - "--user", + "--identifier", strconv.FormatUint(userMap["old-user"].GetId(), 10), "--output", "json", diff --git a/integration/general_test.go b/integration/general_test.go index 292eb5ca..772533e4 100644 --- a/integration/general_test.go +++ b/integration/general_test.go @@ -597,7 +597,7 @@ func TestUpdateHostnameFromClient(t *testing.T) { "node", "rename", givenName, - "--identifier", + "--node-id", strconv.FormatUint(node.GetId(), 10), }) assertNoErr(t, err) @@ -693,7 +693,7 @@ func TestExpireNode(t *testing.T) { // TODO(kradalby): This is Headscale specific and would not play nicely // with other implementations of the ControlServer interface result, err := headscale.Execute([]string{ - "headscale", "nodes", "expire", "--identifier", "1", "--output", "json", + "headscale", "nodes", "expire", "--node-id", "1", "--output", "json", }) assertNoErr(t, err) @@ -1032,7 +1032,7 @@ func Test2118DeletingOnlineNodePanics(t *testing.T) { "headscale", "nodes", "delete", - "--identifier", + "--node-id", // Delete the last added machine fmt.Sprintf("%d", nodeList[0].GetId()), "--output", diff --git a/integration/hsic/hsic.go b/integration/hsic/hsic.go index 35550c65..0f922402 100644 --- a/integration/hsic/hsic.go +++ b/integration/hsic/hsic.go @@ -447,6 +447,16 @@ func New( } } + // Create necessary directories in the container + _, _, err = dockertestutil.ExecuteCommand( + container, + []string{"mkdir", "-p", "/tmp/mapresponses"}, + []string{}, + ) + if err != nil { + log.Printf("Warning: Could not create /tmp/mapresponses directory: %v", err) + } + return hsic, nil } @@ -727,7 +737,7 @@ func (t *HeadscaleInContainer) CreateAuthKey( ) (*v1.PreAuthKey, error) { command := []string{ "headscale", - "--user", + "-i", strconv.FormatUint(user, 10), "preauthkeys", "create", @@ -997,7 +1007,7 @@ func (t *HeadscaleInContainer) ApproveRoutes(id uint64, routes []netip.Prefix) ( command := []string{ "headscale", "nodes", "approve-routes", "--output", "json", - "--identifier", strconv.FormatUint(id, 10), + "--node-id", strconv.FormatUint(id, 10), fmt.Sprintf("--routes=%s", strings.Join(util.PrefixesToString(routes), ",")), } diff --git a/integration/scenario.go b/integration/scenario.go index 0af1956b..61283f00 100644 --- a/integration/scenario.go +++ b/integration/scenario.go @@ -438,13 +438,13 @@ func (s *Scenario) CreatePreAuthKey( if headscale, err := s.Headscale(); err == nil { key, err := headscale.CreateAuthKey(user, reusable, ephemeral) if err != nil { - return nil, fmt.Errorf("failed to create user: %w", err) + return nil, fmt.Errorf("failed to create preauthkey: %w", err) } return key, nil } - return nil, fmt.Errorf("failed to create user: %w", errNoHeadscaleAvailable) + return nil, fmt.Errorf("failed to create preauthkey: %w", errNoHeadscaleAvailable) } // CreateUser creates a User to be created in the