Merge pull request #21473 from vespa-engine/mpolden/check-status

Check status code from ZTS

16 files changed, 65 insertions, 62 deletions

diff --git a/client/go/cmd/command_tester_test.go b/client/go/cmd/command_tester_test.go
index d71cdde0e8f..efaca6a7258 100644
--- a/client/go/cmd/command_tester_test.go
+++ b/client/go/cmd/command_tester_test.go
@@ -53,7 +53,7 @@ func setEnv(env map[string]string) map[string]string {
 }
 
 func resetEnv(env map[string]string, original map[string]string) {
-	for k, _ := range env {
+	for k := range env {
 		if v, ok := original[k]; ok {
 			os.Setenv(k, v)
 		} else {
diff --git a/client/go/cmd/config.go b/client/go/cmd/config.go
index 0997be2c899..3891ef0524d 100644
--- a/client/go/cmd/config.go
+++ b/client/go/cmd/config.go
@@ -183,14 +183,14 @@ func (c *Config) X509KeyPair(app vespa.ApplicationID) (KeyPair, error) {
 }
 
 func (c *Config) APIKeyPath(tenantName string) string {
-	if override, err := c.Get(apiKeyFileFlag); err == nil && override != "" {
+	if override, ok := c.Get(apiKeyFileFlag); ok {
 		return override
 	}
 	return filepath.Join(c.Home, tenantName+".api-key.pem")
 }
 
 func (c *Config) ReadAPIKey(tenantName string) ([]byte, error) {
-	if override, err := c.Get(apiKeyFlag); err == nil && override != "" {
+	if override, ok := c.Get(apiKeyFlag); ok {
 		return []byte(override), nil
 	}
 	return ioutil.ReadFile(c.APIKeyPath(tenantName))
@@ -198,13 +198,12 @@ func (c *Config) ReadAPIKey(tenantName string) ([]byte, error) {
 
 // UseAPIKey checks if api key should be used be checking if api-key or api-key-file has been set.
 func (c *Config) UseAPIKey(system vespa.System, tenantName string) bool {
-	if _, err := c.Get(apiKeyFlag); err == nil {
+	if _, ok := c.Get(apiKeyFlag); ok {
 		return true
 	}
-	if _, err := c.Get(apiKeyFileFlag); err == nil {
+	if _, ok := c.Get(apiKeyFileFlag); ok {
 		return true
 	}
-
 	// If no Auth0 token is created, fall back to tenant api key, but warn that this functionality is deprecated
 	// TODO: Remove this when users have had time to migrate over to Auth0 device flow authentication
 	a, err := auth0.GetAuth0(c.AuthConfigPath(), system.Name, system.URL)
@@ -267,12 +266,12 @@ func (c *Config) load() error {
 	return err
 }
 
-func (c *Config) Get(option string) (string, error) {
+func (c *Config) Get(option string) (string, bool) {
 	value := viper.GetString(option)
 	if value == "" {
-		return "", fmt.Errorf("no such option: %q", option)
+		return "", false
 	}
-	return value, nil
+	return value, true
 }
 
 func (c *Config) Set(option, value string) error {
@@ -313,8 +312,8 @@ func (c *Config) Set(option, value string) error {
 }
 
 func printOption(cfg *Config, option string) {
-	value, err := cfg.Get(option)
-	if err != nil {
+	value, ok := cfg.Get(option)
+	if !ok {
 		value = color.Faint("<unset>").String()
 	} else {
 		value = color.Cyan(value).String()
diff --git a/client/go/cmd/curl.go b/client/go/cmd/curl.go
index 1ede2cccae3..65cac223309 100644
--- a/client/go/cmd/curl.go
+++ b/client/go/cmd/curl.go
@@ -57,7 +57,7 @@ $ vespa curl -- -v --data-urlencode "yql=select * from music where album contain
 			return err
 		}
 		switch curlService {
-		case "deploy":
+		case vespa.DeployService:
 			t, err := getTarget()
 			if err != nil {
 				return err
@@ -67,7 +67,7 @@ $ vespa curl -- -v --data-urlencode "yql=select * from music where album contain
 					return err
 				}
 			}
-		case "document", "query":
+		case vespa.DocumentService, vespa.QueryService:
 			privateKeyFile, err := cfg.PrivateKeyPath(app)
 			if err != nil {
 				return err
diff --git a/client/go/cmd/deploy.go b/client/go/cmd/deploy.go
index 4a7fae243a0..207a0177de8 100644
--- a/client/go/cmd/deploy.go
+++ b/client/go/cmd/deploy.go
@@ -173,7 +173,7 @@ var activateCmd = &cobra.Command{
 func waitForQueryService(sessionOrRunID int64) error {
 	if waitSecsArg > 0 {
 		log.Println()
-		return waitForService("query", sessionOrRunID)
+		return waitForService(vespa.QueryService, sessionOrRunID)
 	}
 	return nil
 }
diff --git a/client/go/cmd/document.go b/client/go/cmd/document.go
index 7c22a67a560..fa211aec108 100644
--- a/client/go/cmd/document.go
+++ b/client/go/cmd/document.go
@@ -143,7 +143,7 @@ var documentGetCmd = &cobra.Command{
 	},
 }
 
-func documentService() (*vespa.Service, error) { return getService("document", 0, "") }
+func documentService() (*vespa.Service, error) { return getService(vespa.DocumentService, 0, "") }
 
 func operationOptions() vespa.OperationOptions {
 	return vespa.OperationOptions{
diff --git a/client/go/cmd/helpers.go b/client/go/cmd/helpers.go
index ab47a0e6d88..9003a64b33b 100644
--- a/client/go/cmd/helpers.go
+++ b/client/go/cmd/helpers.go
@@ -123,8 +123,8 @@ func getApplication() (vespa.ApplicationID, error) {
 	if err != nil {
 		return vespa.ApplicationID{}, err
 	}
-	app, err := cfg.Get(applicationFlag)
-	if err != nil {
+	app, ok := cfg.Get(applicationFlag)
+	if !ok {
 		return vespa.ApplicationID{}, errHint(fmt.Errorf("no application specified: %w", err), "Try the --"+applicationFlag+" flag")
 	}
 	application, err := vespa.ApplicationFromString(app)
@@ -139,9 +139,9 @@ func getTargetType() (string, error) {
 	if err != nil {
 		return "", err
 	}
-	target, err := cfg.Get(targetFlag)
-	if err != nil {
-		return "", fmt.Errorf("invalid target: %w", err)
+	target, ok := cfg.Get(targetFlag)
+	if !ok {
+		return "", fmt.Errorf("target is unset")
 	}
 	return target, nil
 }
diff --git a/client/go/cmd/query.go b/client/go/cmd/query.go
index ed7541288b6..0823b4f830e 100644
--- a/client/go/cmd/query.go
+++ b/client/go/cmd/query.go
@@ -62,7 +62,7 @@ func printCurl(url string, service *vespa.Service) error {
 }
 
 func query(cmd *cobra.Command, arguments []string) error {
-	service, err := getService("query", 0, "")
+	service, err := getService(vespa.QueryService, 0, "")
 	if err != nil {
 		return err
 	}
diff --git a/client/go/cmd/root.go b/client/go/cmd/root.go
index 96521737ba9..114d6150284 100644
--- a/client/go/cmd/root.go
+++ b/client/go/cmd/root.go
@@ -92,11 +92,7 @@ func configureOutput() error {
 	if err != nil {
 		return err
 	}
-	colorValue, err := config.Get(colorFlag)
-	if err != nil {
-		return err
-	}
-
+	colorValue, _ := config.Get(colorFlag)
 	colorize := false
 	switch colorValue {
 	case "auto":
diff --git a/client/go/cmd/status.go b/client/go/cmd/status.go
index 711dba4aa9d..93316b7b6de 100644
--- a/client/go/cmd/status.go
+++ b/client/go/cmd/status.go
@@ -6,6 +6,7 @@ package cmd
 
 import (
 	"github.com/spf13/cobra"
+	"github.com/vespa-engine/vespa/client/go/vespa"
 )
 
 func init() {
@@ -23,7 +24,7 @@ var statusCmd = &cobra.Command{
 	SilenceUsage:      true,
 	Args:              cobra.MaximumNArgs(1),
 	RunE: func(cmd *cobra.Command, args []string) error {
-		return waitForService("query", 0)
+		return waitForService(vespa.QueryService, 0)
 	},
 }
 
@@ -35,7 +36,7 @@ var statusQueryCmd = &cobra.Command{
 	SilenceUsage:      true,
 	Args:              cobra.ExactArgs(0),
 	RunE: func(cmd *cobra.Command, args []string) error {
-		return waitForService("query", 0)
+		return waitForService(vespa.QueryService, 0)
 	},
 }
 
@@ -47,7 +48,7 @@ var statusDocumentCmd = &cobra.Command{
 	SilenceUsage:      true,
 	Args:              cobra.ExactArgs(0),
 	RunE: func(cmd *cobra.Command, args []string) error {
-		return waitForService("document", 0)
+		return waitForService(vespa.DocumentService, 0)
 	},
 }
 
@@ -59,6 +60,6 @@ var statusDeployCmd = &cobra.Command{
 	SilenceUsage:      true,
 	Args:              cobra.ExactArgs(0),
 	RunE: func(cmd *cobra.Command, args []string) error {
-		return waitForService("deploy", 0)
+		return waitForService(vespa.DeployService, 0)
 	},
 }
diff --git a/client/go/cmd/test.go b/client/go/cmd/test.go
index d12059a8d12..555a689c005 100644
--- a/client/go/cmd/test.go
+++ b/client/go/cmd/test.go
@@ -29,7 +29,7 @@ func init() {
 }
 
 var testCmd = &cobra.Command{
-	Use:   "test <tests directory or test file>",
+	Use:   "test test-directory-or-file",
 	Short: "Run a test suite, or a single test",
 	Long: `Run a test suite, or a single test
 
@@ -214,7 +214,7 @@ func verify(step step, defaultCluster string, defaultParameters map[string]strin
 		if err != nil {
 			return "", "", err
 		}
-		service, err = target.Service("query", 0, 0, cluster)
+		service, err = target.Service(vespa.QueryService, 0, 0, cluster)
 		if err != nil {
 			return "", "", err
 		}
diff --git a/client/go/util/http.go b/client/go/util/http.go
index f95d52dabad..bb70c3ec6db 100644
--- a/client/go/util/http.go
+++ b/client/go/util/http.go
@@ -8,7 +8,6 @@ import (
 	"crypto/tls"
 	"fmt"
 	"net/http"
-	"net/url"
 	"time"
 
 	"github.com/vespa-engine/vespa/client/go/build"
@@ -45,15 +44,6 @@ func CreateClient(timeout time.Duration) HttpClient {
 	}
 }
 
-// Convenience function for doing a HTTP GET
-func HttpGet(host string, path string, description string) (*http.Response, error) {
-	url, err := url.Parse(host + path)
-	if err != nil {
-		return nil, fmt.Errorf("invalid target url: %s: %w", host+path, err)
-	}
-	return HttpDo(&http.Request{URL: url}, time.Second*10, description)
-}
-
 func HttpDo(request *http.Request, timeout time.Duration, description string) (*http.Response, error) {
 	if request.Header == nil {
 		request.Header = make(http.Header)
diff --git a/client/go/util/http_test.go b/client/go/util/http_test.go
index e87a1e5ada4..ccb809d198b 100644
--- a/client/go/util/http_test.go
+++ b/client/go/util/http_test.go
@@ -41,11 +41,15 @@ func (c mockHttpClient) UseCertificate(certificates []tls.Certificate) {}
 func TestHttpRequest(t *testing.T) {
 	ActiveHttpClient = mockHttpClient{}
 
-	response, err := HttpGet("http://host", "/okpath", "description")
+	req, err := http.NewRequest("GET", "http://host/okpath", nil)
+	assert.Nil(t, err)
+	response, err := HttpDo(req, time.Second*10, "description")
 	assert.Nil(t, err)
 	assert.Equal(t, 200, response.StatusCode)
 
-	response, err = HttpGet("http://host", "/otherpath", "description")
+	req, err = http.NewRequest("GET", "http://host/otherpath", nil)
+	assert.Nil(t, err)
+	response, err = HttpDo(req, time.Second*10, "description")
 	assert.Nil(t, err)
 	assert.Equal(t, 500, response.StatusCode)
 }
diff --git a/client/go/vespa/deploy.go b/client/go/vespa/deploy.go
index fdc083631f7..480408f3861 100644
--- a/client/go/vespa/deploy.go
+++ b/client/go/vespa/deploy.go
@@ -76,7 +76,7 @@ func (d *DeploymentOptions) IsCloud() bool {
 }
 
 func (d *DeploymentOptions) url(path string) (*url.URL, error) {
-	service, err := d.Target.Service(deployService, 0, 0, "")
+	service, err := d.Target.Service(DeployService, 0, 0, "")
 	if err != nil {
 		return nil, err
 	}
@@ -367,7 +367,7 @@ func uploadApplicationPackage(url *url.URL, opts DeploymentOptions) (int64, erro
 		Header: header,
 		Body:   ioutil.NopCloser(zipReader),
 	}
-	service, err := opts.Target.Service(deployService, opts.Timeout, 0, "")
+	service, err := opts.Target.Service(DeployService, opts.Timeout, 0, "")
 	if err != nil {
 		return 0, err
 	}
diff --git a/client/go/vespa/target.go b/client/go/vespa/target.go
index f620f3b865c..2314cc06f33 100644
--- a/client/go/vespa/target.go
+++ b/client/go/vespa/target.go
@@ -35,9 +35,14 @@ const (
 	// A hosted Vespa target
 	TargetHosted = "hosted"
 
-	deployService   = "deploy"
-	queryService    = "query"
-	documentService = "document"
+	// A Vespa service that handles deployments, either a config server or a controller
+	DeployService = "deploy"
+
+	// A Vespa service that handles queries.
+	QueryService = "query"
+
+	// A Vespa service that handles feeding of document. This may point to the same service as QueryService.
+	DocumentService = "document"
 
 	retryInterval = 2 * time.Second
 )
@@ -131,9 +136,9 @@ func (s *Service) Do(request *http.Request, timeout time.Duration) (*http.Respon
 func (s *Service) Wait(timeout time.Duration) (int, error) {
 	url := s.BaseURL
 	switch s.Name {
-	case deployService:
+	case DeployService:
 		url += "/status.html" // because /ApplicationStatus is not publicly reachable in Vespa Cloud
-	case queryService, documentService:
+	case QueryService, DocumentService:
 		url += "/ApplicationStatus"
 	default:
 		return 0, fmt.Errorf("invalid service: %s", s.Name)
@@ -148,11 +153,11 @@ func (s *Service) Wait(timeout time.Duration) (int, error) {
 
 func (s *Service) Description() string {
 	switch s.Name {
-	case queryService:
+	case QueryService:
 		return "Container (query API)"
-	case documentService:
+	case DocumentService:
 		return "Container (document API)"
-	case deployService:
+	case DeployService:
 		return "Deploy API"
 	}
 	return fmt.Sprintf("No description of service %s", s.Name)
@@ -163,13 +168,13 @@ func (t *customTarget) Type() string { return t.targetType }
 func (t *customTarget) Deployment() Deployment { return Deployment{} }
 
 func (t *customTarget) Service(name string, timeout time.Duration, sessionOrRunID int64, cluster string) (*Service, error) {
-	if timeout > 0 && name != deployService {
+	if timeout > 0 && name != DeployService {
 		if err := t.waitForConvergence(timeout); err != nil {
 			return nil, err
 		}
 	}
 	switch name {
-	case deployService, queryService, documentService:
+	case DeployService, QueryService, DocumentService:
 		url, err := t.urlWithPort(name)
 		if err != nil {
 			return nil, err
@@ -195,9 +200,9 @@ func (t *customTarget) urlWithPort(serviceName string) (string, error) {
 	port := u.Port()
 	if port == "" {
 		switch serviceName {
-		case deployService:
+		case DeployService:
 			port = "19071"
-		case queryService, documentService:
+		case QueryService, DocumentService:
 			port = "8080"
 		default:
 			return "", fmt.Errorf("unknown service: %s", serviceName)
@@ -208,7 +213,7 @@ func (t *customTarget) urlWithPort(serviceName string) (string, error) {
 }
 
 func (t *customTarget) waitForConvergence(timeout time.Duration) error {
-	deployer, err := t.Service(deployService, 0, 0, "")
+	deployer, err := t.Service(DeployService, 0, 0, "")
 	if err != nil {
 		return err
 	}
@@ -284,15 +289,15 @@ func (t *cloudTarget) Type() string {
 func (t *cloudTarget) Deployment() Deployment { return t.deploymentOptions.Deployment }
 
 func (t *cloudTarget) Service(name string, timeout time.Duration, runID int64, cluster string) (*Service, error) {
-	if name != deployService && t.deploymentOptions.ClusterURLs == nil {
+	if name != DeployService && t.deploymentOptions.ClusterURLs == nil {
 		if err := t.waitForEndpoints(timeout, runID); err != nil {
 			return nil, err
 		}
 	}
 	switch name {
-	case deployService:
+	case DeployService:
 		return &Service{Name: name, BaseURL: t.apiOptions.System.URL, TLSOptions: t.apiOptions.TLSOptions, ztsClient: t.ztsClient}, nil
-	case queryService, documentService:
+	case QueryService, DocumentService:
 		url, err := t.resolveEndpoint(cluster)
 		if err != nil {
 			return nil, err
diff --git a/client/go/zts/zts.go b/client/go/zts/zts.go
index b1a47db8e48..538971ebdd8 100644
--- a/client/go/zts/zts.go
+++ b/client/go/zts/zts.go
@@ -44,6 +44,9 @@ func (c *Client) AccessToken(domain string, certificate tls.Certificate) (string
 	}
 	defer response.Body.Close()
 
+	if response.StatusCode != http.StatusOK {
+		return "", fmt.Errorf("got status %d from %s", response.StatusCode, c.tokenURL.String())
+	}
 	var ztsResponse struct {
 		AccessToken string `json:"access_token"`
 	}
diff --git a/client/go/zts/zts_test.go b/client/go/zts/zts_test.go
index f1bd9c1ba75..0eec085aadb 100644
--- a/client/go/zts/zts_test.go
+++ b/client/go/zts/zts_test.go
@@ -13,6 +13,11 @@ func TestAccessToken(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
+	httpClient.NextResponse(400, `{"message": "bad request"}`)
+	_, err = client.AccessToken("vespa.vespa", tls.Certificate{})
+	if err == nil {
+		t.Fatal("want error for non-ok response status")
+	}
 	httpClient.NextResponse(200, `{"access_token": "foo bar"}`)
 	token, err := client.AccessToken("vespa.vespa", tls.Certificate{})
 	if err != nil {