From 5c341d47454f88406214942a7c818b4a1f39ba36 Mon Sep 17 00:00:00 2001 From: yusing Date: Sun, 25 Jan 2026 19:18:14 +0800 Subject: [PATCH] refactor: improve error handling, validation and proper cleanup --- .gitignore | 6 +++++- internal/acl/config.go | 2 +- internal/api/v1/docker/logs.go | 2 +- internal/api/v1/proxmox/common.go | 2 +- internal/api/v1/proxmox/journalctl.go | 31 ++++++++++++++++----------- internal/api/v1/route/route.go | 2 +- internal/proxmox/README.md | 10 ++++----- internal/proxmox/client.go | 3 +++ internal/proxmox/config.go | 13 ++++++----- internal/proxmox/node_command.go | 3 ++- internal/route/route.go | 4 ++-- internal/route/rules/do.go | 4 ++++ 12 files changed, 52 insertions(+), 30 deletions(-) diff --git a/.gitignore b/.gitignore index 8298d645..a8262858 100755 --- a/.gitignore +++ b/.gitignore @@ -40,4 +40,8 @@ tsconfig.tsbuildinfo !agent.compose.yml !agent/pkg/** -dev-data/ \ No newline at end of file +dev-data/ + +RELEASE_NOTES.md +CLAUDE.md +.kilocode/** \ No newline at end of file diff --git a/internal/acl/config.go b/internal/acl/config.go index d25ba8d6..b608a621 100644 --- a/internal/acl/config.go +++ b/internal/acl/config.go @@ -106,7 +106,7 @@ func (c *Config) Validate() gperr.Error { c.allowLocal = true } - if c.Notify.Interval < 0 { + if c.Notify.Interval <= 0 { c.Notify.Interval = defaultNotifyInterval } diff --git a/internal/api/v1/docker/logs.go b/internal/api/v1/docker/logs.go index 6b4a82bf..ee277b02 100644 --- a/internal/api/v1/docker/logs.go +++ b/internal/api/v1/docker/logs.go @@ -23,7 +23,7 @@ type LogsQueryParams struct { Since string `form:"from"` Until string `form:"to"` Levels string `form:"levels"` - Limit int `form:"limit,default=100" binding:"omitempty,min=1,max=1000"` + Limit int `form:"limit,default=100" binding:"min=1,max=1000"` } // @name LogsQueryParams // @x-id "logs" diff --git a/internal/api/v1/proxmox/common.go b/internal/api/v1/proxmox/common.go index 10477d0b..18864910 100644 --- a/internal/api/v1/proxmox/common.go +++ b/internal/api/v1/proxmox/common.go @@ -3,4 +3,4 @@ package proxmoxapi type ActionRequest struct { Node string `uri:"node" binding:"required"` VMID int `uri:"vmid" binding:"required"` -} +} // @name ProxmoxVMActionRequest diff --git a/internal/api/v1/proxmox/journalctl.go b/internal/api/v1/proxmox/journalctl.go index caabd067..99cf9632 100644 --- a/internal/api/v1/proxmox/journalctl.go +++ b/internal/api/v1/proxmox/journalctl.go @@ -11,18 +11,18 @@ import ( ) type JournalctlRequest struct { - Node string `uri:"node" binding:"required"` - VMID *int `uri:"vmid"` // optional - if not provided, streams node journalctl - Service string `uri:"service"` - Limit int `query:"limit" binding:"omitempty,min=1,max=1000"` -} + Node string `uri:"node" binding:"required"` // Node name + VMID *int `uri:"vmid"` // Container VMID (optional - if not provided, streams node journalctl) + Service string `uri:"service"` // Service name (e.g., 'pveproxy' for node, 'container@.service' format for LXC) + Limit int `query:"limit" default:"100" binding:"min=1,max=1000"` // Limit output lines (1-1000) +} // @name ProxmoxJournalctlRequest // @x-id "journalctl" // @BasePath /api/v1 // @Summary Get journalctl output // @Description Get journalctl output for node or LXC container. If vmid is not provided, streams node journalctl. // @Tags proxmox,websocket -// @Accept json +// @Accept json // @Produce application/json // @Param node path string true "Node name" // @Param vmid path int false "Container VMID (optional - if not provided, streams node journalctl)" @@ -42,6 +42,10 @@ func Journalctl(c *gin.Context) { c.JSON(http.StatusBadRequest, apitypes.Error("invalid request", err)) return } + if err := c.ShouldBindQuery(&request); err != nil { + c.JSON(http.StatusBadRequest, apitypes.Error("invalid request", err)) + return + } node, ok := proxmox.Nodes.Get(request.Node) if !ok { @@ -49,14 +53,10 @@ func Journalctl(c *gin.Context) { return } - manager, err := websocket.NewManagerWithUpgrade(c) - if err != nil { - c.Error(apitypes.InternalServerError(err, "failed to upgrade to websocket")) - return - } - defer manager.Close() + c.Status(http.StatusContinue) var reader io.ReadCloser + var err error if request.VMID == nil { reader, err = node.NodeJournalctl(c.Request.Context(), request.Service, request.Limit) } else { @@ -68,6 +68,13 @@ func Journalctl(c *gin.Context) { } defer reader.Close() + manager, err := websocket.NewManagerWithUpgrade(c) + if err != nil { + c.Error(apitypes.InternalServerError(err, "failed to upgrade to websocket")) + return + } + defer manager.Close() + writer := manager.NewWriter(websocket.TextMessage) _, err = io.Copy(writer, reader) if err != nil { diff --git a/internal/api/v1/route/route.go b/internal/api/v1/route/route.go index ed1a205a..bd6ab1be 100644 --- a/internal/api/v1/route/route.go +++ b/internal/api/v1/route/route.go @@ -37,5 +37,5 @@ func Route(c *gin.Context) { c.JSON(http.StatusOK, route) return } - c.JSON(http.StatusNotFound, nil) + c.JSON(http.StatusNotFound, apitypes.Error("route not found")) } diff --git a/internal/proxmox/README.md b/internal/proxmox/README.md index 6e1ee881..563cb805 100644 --- a/internal/proxmox/README.md +++ b/internal/proxmox/README.md @@ -52,11 +52,11 @@ graph TD ```go type Config struct { URL string `json:"url" validate:"required,url"` - Username string `json:"username" validate:"required_without=TokenID Secret"` - Password strutils.Redacted `json:"password" validate:"required_without=TokenID Secret"` - Realm string `json:"realm" validate:"required_without=TokenID Secret"` - TokenID string `json:"token_id" validate:"required_without=Username Password"` - Secret strutils.Redacted `json:"secret" validate:"required_without=Username Password"` + Username string `json:"username" validate:"required_without_all=TokenID Secret"` + Password strutils.Redacted `json:"password" validate:"required_without_all=TokenID Secret"` + Realm string `json:"realm"` + TokenID string `json:"token_id" validate:"required_without_all=Username Password"` + Secret strutils.Redacted `json:"secret" validate:"required_without_all=Username Password"` NoTLSVerify bool `json:"no_tls_verify"` client *Client diff --git a/internal/proxmox/client.go b/internal/proxmox/client.go index 55c1e384..9a17067e 100644 --- a/internal/proxmox/client.go +++ b/internal/proxmox/client.go @@ -65,6 +65,9 @@ func (c *Client) UpdateClusterInfo(ctx context.Context) (err error) { } func (c *Client) UpdateResources(ctx context.Context) error { + if c.Cluster == nil { + return errors.New("cluster not initialized, call UpdateClusterInfo first") + } resourcesSlice, err := c.Cluster.Resources(ctx, "vm") if err != nil { return err diff --git a/internal/proxmox/config.go b/internal/proxmox/config.go index 52b250cf..331003b7 100644 --- a/internal/proxmox/config.go +++ b/internal/proxmox/config.go @@ -18,12 +18,12 @@ import ( type Config struct { URL string `json:"url" validate:"required,url"` - Username string `json:"username" validate:"required_without=TokenID Secret"` - Password strutils.Redacted `json:"password" validate:"required_without=TokenID Secret"` - Realm string `json:"realm" validate:"required_without=TokenID Secret"` + Username string `json:"username" validate:"required_without_all=TokenID Secret"` + Password strutils.Redacted `json:"password" validate:"required_without_all=TokenID Secret"` + Realm string `json:"realm"` // default is "pam" - TokenID string `json:"token_id" validate:"required_without=Username Password"` - Secret strutils.Redacted `json:"secret" validate:"required_without=Username Password"` + TokenID string `json:"token_id" validate:"required_without_all=Username Password"` + Secret strutils.Redacted `json:"secret" validate:"required_without_all=Username Password"` NoTLSVerify bool `json:"no_tls_verify" yaml:"no_tls_verify,omitempty"` @@ -65,6 +65,9 @@ func (c *Config) Init(ctx context.Context) gperr.Error { } useCredentials := false if c.Username != "" && c.Password != "" { + if c.Realm == "" { + c.Realm = "pam" + } opts = append(opts, proxmox.WithCredentials(&proxmox.Credentials{ Username: c.Username, Password: c.Password.String(), diff --git a/internal/proxmox/node_command.go b/internal/proxmox/node_command.go index 6042a70f..804d1bff 100644 --- a/internal/proxmox/node_command.go +++ b/internal/proxmox/node_command.go @@ -50,6 +50,7 @@ func (n *Node) NodeCommand(ctx context.Context, command string) (io.ReadCloser, // Send command cmd := []byte(command + "\n") if err := handleSend(cmd); err != nil { + closeFn() return nil, err } @@ -70,6 +71,7 @@ func (n *Node) NodeCommand(ctx context.Context, command string) (io.ReadCloser, for { select { case <-ctx.Done(): + _ = pw.CloseWithError(ctx.Err()) return case msg := <-recv: // skip the header message like @@ -106,7 +108,6 @@ func (n *Node) NodeCommand(ctx context.Context, command string) (io.ReadCloser, case err := <-errs: if err != nil { if websocket.IsUnexpectedCloseError(err, websocket.CloseGoingAway, websocket.CloseAbnormalClosure) { - _ = pw.Close() return } _ = pw.CloseWithError(err) diff --git a/internal/route/route.go b/internal/route/route.go index 9277dd29..67534908 100644 --- a/internal/route/route.go +++ b/internal/route/route.go @@ -218,7 +218,7 @@ func (r *Route) validate() gperr.Error { r.Proxmox.VMName = res.Name if r.Host == DefaultHost { - containerName := r.Idlewatcher.ContainerName() + containerName := res.Name // get ip addresses of the vmid ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) @@ -336,7 +336,7 @@ func (r *Route) validate() gperr.Error { } } - if r.Proxmox == nil && r.Container == nil { + if r.Proxmox == nil && r.Container == nil && r.ProxyURL != nil { proxmoxProviders := config.WorkingState.Load().Value().Providers.Proxmox if len(proxmoxProviders) > 0 { // it's fine if ip is nil diff --git a/internal/route/rules/do.go b/internal/route/rules/do.go index 65477fc3..f0343d0c 100644 --- a/internal/route/rules/do.go +++ b/internal/route/rules/do.go @@ -79,6 +79,10 @@ var commands = map[string]struct { }, build: func(args any) CommandHandler { return NonTerminatingCommand(func(w http.ResponseWriter, r *http.Request) error { + if authHandler == nil { + http.Error(w, "Auth handler not initialized", http.StatusInternalServerError) + return errTerminated + } if !authHandler(w, r) { return errTerminated }