cmd/syncthing: Don't fail early on api setup error (fixes 7558) (#7591)

* cmd/syncthing: Don't fail early on api setup error (fixes 7558)

* switch to factory pattern

* refactor config command to show help on nothing

* wip

* wip

* already abort in before

cmd/syncthing/cli/client.go

@@ -17,33 +17,88 @@ import (
 	"strings"
 
 	"github.com/syncthing/syncthing/lib/config"
+	"github.com/syncthing/syncthing/lib/events"
+	"github.com/syncthing/syncthing/lib/locations"
+	"github.com/syncthing/syncthing/lib/protocol"
 )
 
-type APIClient struct {
+type APIClient interface {
+	Get(url string) (*http.Response, error)
+	Post(url, body string) (*http.Response, error)
+}
+
+type apiClient struct {
 	http.Client
 	cfg    config.GUIConfiguration
 	apikey string
 }
 
-func getClient(cfg config.GUIConfiguration) *APIClient {
+type apiClientFactory struct {
+	cfg config.GUIConfiguration
+}
+
+func (f *apiClientFactory) getClient() (APIClient, error) {
+	// Now if the API key and address is not provided (we are not connecting to a remote instance),
+	// try to rip it out of the config.
+	if f.cfg.RawAddress == "" && f.cfg.APIKey == "" {
+		var err error
+		f.cfg, err = loadGUIConfig()
+		if err != nil {
+			return nil, err
+		}
+	} else if f.cfg.Address() == "" || f.cfg.APIKey == "" {
+		return nil, errors.New("Both --gui-address and --gui-apikey should be specified")
+	}
+
 	httpClient := http.Client{
 		Transport: &http.Transport{
 			TLSClientConfig: &tls.Config{
 				InsecureSkipVerify: true,
 			},
 			DialContext: func(_ context.Context, _, _ string) (net.Conn, error) {
-				return net.Dial(cfg.Network(), cfg.Address())
+				return net.Dial(f.cfg.Network(), f.cfg.Address())
 			},
 		},
 	}
-	return &APIClient{
+	return &apiClient{
 		Client: httpClient,
-		cfg:    cfg,
-		apikey: cfg.APIKey,
+		cfg:    f.cfg,
+		apikey: f.cfg.APIKey,
+	}, nil
+}
+
+func loadGUIConfig() (config.GUIConfiguration, error) {
+	// Load the certs and get the ID
+	cert, err := tls.LoadX509KeyPair(
+		locations.Get(locations.CertFile),
+		locations.Get(locations.KeyFile),
+	)
+	if err != nil {
+		return config.GUIConfiguration{}, fmt.Errorf("reading device ID: %w", err)
 	}
+
+	myID := protocol.NewDeviceID(cert.Certificate[0])
+
+	// Load the config
+	cfg, _, err := config.Load(locations.Get(locations.ConfigFile), myID, events.NoopLogger)
+	if err != nil {
+		return config.GUIConfiguration{}, fmt.Errorf("loading config: %w", err)
+	}
+
+	guiCfg := cfg.GUI()
+
+	if guiCfg.Address() == "" {
+		return config.GUIConfiguration{}, errors.New("Could not find GUI Address")
+	}
+
+	if guiCfg.APIKey == "" {
+		return config.GUIConfiguration{}, errors.New("Could not find GUI API key")
+	}
+
+	return guiCfg, nil
 }
 
-func (c *APIClient) Endpoint() string {
+func (c *apiClient) Endpoint() string {
 	if c.cfg.Network() == "unix" {
 		return "http://unix/"
 	}
@@ -54,7 +109,7 @@ func (c *APIClient) Endpoint() string {
 	return url
 }
 
-func (c *APIClient) Do(req *http.Request) (*http.Response, error) {
+func (c *apiClient) Do(req *http.Request) (*http.Response, error) {
 	req.Header.Set("X-API-Key", c.apikey)
 	resp, err := c.Client.Do(req)
 	if err != nil {
@@ -63,7 +118,7 @@ func (c *APIClient) Do(req *http.Request) (*http.Response, error) {
 	return resp, checkResponse(resp)
 }
 
-func (c *APIClient) Get(url string) (*http.Response, error) {
+func (c *apiClient) Get(url string) (*http.Response, error) {
 	request, err := http.NewRequest("GET", c.Endpoint()+"rest/"+url, nil)
 	if err != nil {
 		return nil, err
@@ -71,7 +126,7 @@ func (c *APIClient) Get(url string) (*http.Response, error) {
 	return c.Do(request)
 }
 
-func (c *APIClient) Post(url, body string) (*http.Response, error) {
+func (c *apiClient) Post(url, body string) (*http.Response, error) {
 	request, err := http.NewRequest("POST", c.Endpoint()+"rest/"+url, bytes.NewBufferString(body))
 	if err != nil {
 		return nil, err

cmd/syncthing/cli/config.go

@@ -0,0 +1,87 @@
+// Copyright (C) 2021 The Syncthing Authors.
+//
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this file,
+// You can obtain one at https://mozilla.org/MPL/2.0/.
+
+package cli
+
+import (
+	"encoding/json"
+	"fmt"
+	"reflect"
+
+	"github.com/AudriusButkevicius/recli"
+	"github.com/pkg/errors"
+	"github.com/syncthing/syncthing/lib/config"
+	"github.com/urfave/cli"
+)
+
+type configHandler struct {
+	original, cfg config.Configuration
+	client        APIClient
+	err           error
+}
+
+func getConfigCommand(f *apiClientFactory) (cli.Command, error) {
+	h := new(configHandler)
+	h.client, h.err = f.getClient()
+	if h.err == nil {
+		h.cfg, h.err = getConfig(h.client)
+	}
+	h.original = h.cfg.Copy()
+
+	// Copy the config and set the default flags
+	recliCfg := recli.DefaultConfig
+	recliCfg.IDTag.Name = "xml"
+	recliCfg.SkipTag.Name = "json"
+
+	commands, err := recli.New(recliCfg).Construct(&h.cfg)
+	if err != nil {
+		return cli.Command{}, fmt.Errorf("config reflect: %w", err)
+	}
+
+	return cli.Command{
+		Name:        "config",
+		HideHelp:    true,
+		Usage:       "Configuration modification command group",
+		Subcommands: commands,
+		Before:      h.configBefore,
+		After:       h.configAfter,
+	}, nil
+}
+
+func (h *configHandler) configBefore(c *cli.Context) error {
+	for _, arg := range c.Args() {
+		if arg == "--help" || arg == "-h" {
+			return nil
+		}
+	}
+	return h.err
+}
+
+func (h *configHandler) configAfter(c *cli.Context) error {
+	if h.err != nil {
+		// Error was already returned in configBefore
+		return nil
+	}
+	if reflect.DeepEqual(h.cfg, h.original) {
+		return nil
+	}
+	body, err := json.MarshalIndent(h.cfg, "", "  ")
+	if err != nil {
+		return err
+	}
+	resp, err := h.client.Post("system/config", string(body))
+	if err != nil {
+		return err
+	}
+	if resp.StatusCode != 200 {
+		body, err := responseToBArray(resp)
+		if err != nil {
+			return err
+		}
+		return errors.New(string(body))
+	}
+	return nil
+}

cmd/syncthing/cli/errors.go

@@ -39,7 +39,7 @@ var errorsCommand = cli.Command{
 }
 
 func errorsPush(c *cli.Context) error {
-	client := c.App.Metadata["client"].(*APIClient)
+	client := c.App.Metadata["client"].(APIClient)
 	errStr := strings.Join(c.Args(), " ")
 	response, err := client.Post("system/error", strings.TrimSpace(errStr))
 	if err != nil {

cmd/syncthing/cli/main.go

@@ -8,14 +8,10 @@ package cli
 
 import (
 	"bufio"
-	"crypto/tls"
-	"encoding/json"
 	"io/ioutil"
 	"os"
-	"reflect"
 	"strings"
 
-	"github.com/AudriusButkevicius/recli"
 	"github.com/alecthomas/kong"
 	"github.com/flynn-archive/go-shlex"
 	"github.com/mattn/go-isatty"
@@ -24,9 +20,6 @@ import (
 
 	"github.com/syncthing/syncthing/cmd/syncthing/cmdutil"
 	"github.com/syncthing/syncthing/lib/config"
-	"github.com/syncthing/syncthing/lib/events"
-	"github.com/syncthing/syncthing/lib/locations"
-	"github.com/syncthing/syncthing/lib/protocol"
 )
 
 type preCli struct {
@@ -51,68 +44,16 @@ func Run() error {
 	if err != nil {
 		return errors.Wrap(err, "Command line options:")
 	}
-	guiCfg := config.GUIConfiguration{
-		RawAddress: c.GUIAddress,
-		APIKey:     c.GUIAPIKey,
-	}
-
-	// Now if the API key and address is not provided (we are not connecting to a remote instance),
-	// try to rip it out of the config.
-	if guiCfg.RawAddress == "" && guiCfg.APIKey == "" {
-		// Load the certs and get the ID
-		cert, err := tls.LoadX509KeyPair(
-			locations.Get(locations.CertFile),
-			locations.Get(locations.KeyFile),
-		)
-		if err != nil {
-			return errors.Wrap(err, "reading device ID")
-		}
-
-		myID := protocol.NewDeviceID(cert.Certificate[0])
-
-		// Load the config
-		cfg, _, err := config.Load(locations.Get(locations.ConfigFile), myID, events.NoopLogger)
-		if err != nil {
-			return errors.Wrap(err, "loading config")
-		}
-
-		guiCfg = cfg.GUI()
-	} else if guiCfg.Address() == "" || guiCfg.APIKey == "" {
-		return errors.New("Both --gui-address and --gui-apikey should be specified")
-	}
-
-	if guiCfg.Address() == "" {
-		return errors.New("Could not find GUI Address")
-	}
-
-	if guiCfg.APIKey == "" {
-		return errors.New("Could not find GUI API key")
+	clientFactory := &apiClientFactory{
+		cfg: config.GUIConfiguration{
+			RawAddress: c.GUIAddress,
+			APIKey:     c.GUIAPIKey,
+		},
 	}
 
-	client := getClient(guiCfg)
-
-	cfg, cfgErr := getConfig(client)
-	original := cfg.Copy()
-
-	// Copy the config and set the default flags
-	recliCfg := recli.DefaultConfig
-	recliCfg.IDTag.Name = "xml"
-	recliCfg.SkipTag.Name = "json"
-
-	configCommand := cli.Command{
-		Name:     "config",
-		HideHelp: true,
-		Usage:    "Configuration modification command group",
-	}
-	if cfgErr != nil {
-		configCommand.Action = func(*cli.Context) error {
-			return cfgErr
-		}
-	} else {
-		configCommand.Subcommands, err = recli.New(recliCfg).Construct(&cfg)
-		if err != nil {
-			return errors.Wrap(err, "config reflect")
-		}
+	configCommand, err := getConfigCommand(clientFactory)
+	if err != nil {
+		return err
 	}
 
 	// Implement the same flags at the upper CLI, but do nothing with them.
@@ -144,7 +85,7 @@ func Run() error {
 	app := cli.NewApp()
 	app.Author = "The Syncthing Authors"
 	app.Metadata = map[string]interface{}{
-		"client": client,
+		"clientFactory": clientFactory,
 	}
 	app.Commands = []cli.Command{{
 		Name:  "cli",
@@ -187,23 +128,6 @@ func Run() error {
 		}
 	}
 
-	if cfgErr == nil && !reflect.DeepEqual(cfg, original) {
-		body, err := json.MarshalIndent(cfg, "", "  ")
-		if err != nil {
-			return err
-		}
-		resp, err := client.Post("system/config", string(body))
-		if err != nil {
-			return err
-		}
-		if resp.StatusCode != 200 {
-			body, err := responseToBArray(resp)
-			if err != nil {
-				return err
-			}
-			return errors.New(string(body))
-		}
-	}
 	return nil
 }
 

cmd/syncthing/cli/operations.go

@@ -42,7 +42,10 @@ var operationCommand = cli.Command{
 }
 
 func foldersOverride(c *cli.Context) error {
-	client := c.App.Metadata["client"].(*APIClient)
+	client, err := getClientFactory(c).getClient()
+	if err != nil {
+		return err
+	}
 	cfg, err := getConfig(client)
 	if err != nil {
 		return err

cmd/syncthing/cli/utils.go

@@ -32,15 +32,21 @@ func responseToBArray(response *http.Response) ([]byte, error) {
 
 func emptyPost(url string) cli.ActionFunc {
 	return func(c *cli.Context) error {
-		client := c.App.Metadata["client"].(*APIClient)
-		_, err := client.Post(url, "")
+		client, err := getClientFactory(c).getClient()
+		if err != nil {
+			return err
+		}
+		_, err = client.Post(url, "")
 		return err
 	}
 }
 
 func indexDumpOutput(url string) cli.ActionFunc {
 	return func(c *cli.Context) error {
-		client := c.App.Metadata["client"].(*APIClient)
+		client, err := getClientFactory(c).getClient()
+		if err != nil {
+			return err
+		}
 		response, err := client.Get(url)
 		if err != nil {
 			return err
@@ -51,7 +57,10 @@ func indexDumpOutput(url string) cli.ActionFunc {
 
 func saveToFile(url string) cli.ActionFunc {
 	return func(c *cli.Context) error {
-		client := c.App.Metadata["client"].(*APIClient)
+		client, err := getClientFactory(c).getClient()
+		if err != nil {
+			return err
+		}
 		response, err := client.Get(url)
 		if err != nil {
 			return err
@@ -82,7 +91,7 @@ func saveToFile(url string) cli.ActionFunc {
 	}
 }
 
-func getConfig(c *APIClient) (config.Configuration, error) {
+func getConfig(c APIClient) (config.Configuration, error) {
 	cfg := config.Configuration{}
 	response, err := c.Get("system/config")
 	if err != nil {
@@ -147,3 +156,7 @@ func nulString(bs []byte) string {
 func normalizePath(path string) string {
 	return filepath.ToSlash(filepath.Clean(path))
 }
+
+func getClientFactory(c *cli.Context) *apiClientFactory {
+	return c.App.Metadata["clientFactory"].(*apiClientFactory)
+}