Never wait on 4xx for any target

7 files changed, 55 insertions, 27 deletions

diff --git a/client/go/internal/cli/cmd/root.go b/client/go/internal/cli/cmd/root.go
index 695d2eaca8f..88d43411983 100644
--- a/client/go/internal/cli/cmd/root.go
+++ b/client/go/internal/cli/cmd/root.go
@@ -465,7 +465,6 @@ func (c *CLI) createDeploymentOptions(pkg vespa.ApplicationPackage, target vespa
 		ApplicationPackage: pkg,
 		Target:             target,
 		Timeout:            timeout,
-		HTTPClient:         c.httpClient,
 	}, nil
 }
 
diff --git a/client/go/internal/vespa/deploy.go b/client/go/internal/vespa/deploy.go
index 82fd014b377..f633c8ed9ee 100644
--- a/client/go/internal/vespa/deploy.go
+++ b/client/go/internal/vespa/deploy.go
@@ -45,7 +45,6 @@ type DeploymentOptions struct {
 	ApplicationPackage ApplicationPackage
 	Timeout            time.Duration
 	Version            version.Version
-	HTTPClient         util.HTTPClient
 }
 
 type LogLinePrepareResponse struct {
@@ -130,7 +129,7 @@ func Prepare(deployment DeploymentOptions) (PrepareResult, error) {
 		return PrepareResult{}, err
 	}
 	serviceDescription := "Deploy service"
-	response, err := deployment.HTTPClient.Do(req, time.Second*30)
+	response, err := deployServiceDo(req, time.Second*30, deployment)
 	if err != nil {
 		return PrepareResult{}, err
 	}
@@ -171,7 +170,7 @@ func Activate(sessionID int64, deployment DeploymentOptions) error {
 		return err
 	}
 	serviceDescription := "Deploy service"
-	response, err := deployment.HTTPClient.Do(req, time.Second*30)
+	response, err := deployServiceDo(req, time.Second*30, deployment)
 	if err != nil {
 		return err
 	}
@@ -263,7 +262,7 @@ func Submit(opts DeploymentOptions) error {
 	}
 	request.Header.Set("Content-Type", writer.FormDataContentType())
 	serviceDescription := "Submit service"
-	response, err := opts.HTTPClient.Do(request, time.Minute*10)
+	response, err := deployServiceDo(request, time.Minute*10, opts)
 	if err != nil {
 		return err
 	}
@@ -271,6 +270,14 @@ func Submit(opts DeploymentOptions) error {
 	return checkResponse(request, response, serviceDescription)
 }
 
+func deployServiceDo(request *http.Request, timeout time.Duration, opts DeploymentOptions) (*http.Response, error) {
+	s, err := opts.Target.Service(DeployService, 0, 0, "")
+	if err != nil {
+		return nil, err
+	}
+	return s.Do(request, timeout)
+}
+
 func checkDeploymentOpts(opts DeploymentOptions) error {
 	if opts.Target.Type() == TargetCloud && !opts.ApplicationPackage.HasCertificate() {
 		return fmt.Errorf("%s: missing certificate in package", opts)
@@ -330,7 +337,6 @@ func uploadApplicationPackage(url *url.URL, opts DeploymentOptions) (PrepareResu
 	if err != nil {
 		return PrepareResult{}, err
 	}
-
 	response, err := service.Do(request, time.Minute*10)
 	if err != nil {
 		return PrepareResult{}, err
diff --git a/client/go/internal/vespa/deploy_test.go b/client/go/internal/vespa/deploy_test.go
index db3d17c432a..da2604282c0 100644
--- a/client/go/internal/vespa/deploy_test.go
+++ b/client/go/internal/vespa/deploy_test.go
@@ -24,7 +24,6 @@ func TestDeploy(t *testing.T) {
 	opts := DeploymentOptions{
 		Target:             target,
 		ApplicationPackage: ApplicationPackage{Path: appDir},
-		HTTPClient:         &httpClient,
 	}
 	_, err := Deploy(opts)
 	assert.Nil(t, err)
@@ -47,7 +46,6 @@ func TestDeployCloud(t *testing.T) {
 	opts := DeploymentOptions{
 		Target:             target,
 		ApplicationPackage: ApplicationPackage{Path: appDir},
-		HTTPClient:         &httpClient,
 	}
 	_, err := Deploy(opts)
 	require.Nil(t, err)
diff --git a/client/go/internal/vespa/target.go b/client/go/internal/vespa/target.go
index 6d5d7efad91..9f3fd7f5c65 100644
--- a/client/go/internal/vespa/target.go
+++ b/client/go/internal/vespa/target.go
@@ -138,20 +138,36 @@ func (s *Service) Description() string {
 	return fmt.Sprintf("No description of service %s", s.Name)
 }
 
-func isOK(status int) bool { return status/100 == 2 }
+func isOK(status int) (bool, error) {
+	class := status / 100
+	switch class {
+	case 2: // success
+		return true, nil
+	case 4: // client error
+		return false, fmt.Errorf("request failed with status %d", status)
+	default: // retry
+		return false, nil
+	}
+}
 
 type responseFunc func(status int, response []byte) (bool, error)
 
 type requestFunc func() *http.Request
 
-// waitForOK queries url and returns its status code. If the url returns a non-200 status code, it is repeatedly queried
+// waitForOK queries url and returns its status code. If response status is not 2xx or 4xx, it is repeatedly queried
 // until timeout elapses.
 func waitForOK(service *Service, url string, timeout time.Duration) (int, error) {
 	req, err := http.NewRequest("GET", url, nil)
 	if err != nil {
 		return 0, err
 	}
-	okFunc := func(status int, response []byte) (bool, error) { return isOK(status), nil }
+	okFunc := func(status int, response []byte) (bool, error) {
+		ok, err := isOK(status)
+		if err != nil {
+			return false, fmt.Errorf("failed to query %s at %s: %w", service.Description(), url, err)
+		}
+		return ok, err
+	}
 	return wait(service, okFunc, func() *http.Request { return req }, timeout)
 }
 
diff --git a/client/go/internal/vespa/target_cloud.go b/client/go/internal/vespa/target_cloud.go
index e9dca55f654..928bb788494 100644
--- a/client/go/internal/vespa/target_cloud.go
+++ b/client/go/internal/vespa/target_cloud.go
@@ -123,7 +123,7 @@ func (t *cloudTarget) Service(name string, timeout time.Duration, runID int64, c
 			if err != nil {
 				return nil, err
 			}
-			if !isOK(status) {
+			if ok, _ := isOK(status); !ok {
 				return nil, fmt.Errorf("got status %d from deploy service at %s", status, service.BaseURL)
 			}
 		}
@@ -209,7 +209,7 @@ func (t *cloudTarget) PrintLog(options LogOptions) error {
 		return req
 	}
 	logFunc := func(status int, response []byte) (bool, error) {
-		if ok, err := isCloudOK(status); !ok {
+		if ok, err := isOK(status); !ok {
 			return ok, err
 		}
 		logEntries, err := ReadLogEntries(bytes.NewReader(response))
@@ -272,7 +272,7 @@ func (t *cloudTarget) waitForRun(runID int64, timeout time.Duration) error {
 		return req
 	}
 	jobSuccessFunc := func(status int, response []byte) (bool, error) {
-		if ok, err := isCloudOK(status); !ok {
+		if ok, err := isOK(status); !ok {
 			return ok, err
 		}
 		var resp jobResponse
@@ -327,7 +327,7 @@ func (t *cloudTarget) discoverEndpoints(timeout time.Duration) error {
 	}
 	urlsByCluster := make(map[string]string)
 	endpointFunc := func(status int, response []byte) (bool, error) {
-		if ok, err := isCloudOK(status); !ok {
+		if ok, err := isOK(status); !ok {
 			return ok, err
 		}
 		var resp deploymentResponse
@@ -354,11 +354,3 @@ func (t *cloudTarget) discoverEndpoints(timeout time.Duration) error {
 	t.deploymentOptions.ClusterURLs = urlsByCluster
 	return nil
 }
-
-func isCloudOK(status int) (bool, error) {
-	if status == 401 {
-		// when retrying we should give up immediately if we're not authorized
-		return false, fmt.Errorf("status %d: invalid credentials", status)
-	}
-	return isOK(status), nil
-}
diff --git a/client/go/internal/vespa/target_custom.go b/client/go/internal/vespa/target_custom.go
index df50e90a55b..0a3a9d48fed 100644
--- a/client/go/internal/vespa/target_custom.go
+++ b/client/go/internal/vespa/target_custom.go
@@ -61,7 +61,7 @@ func (t *customTarget) Service(name string, timeout time.Duration, sessionOrRunI
 			if err != nil {
 				return nil, err
 			}
-			if !isOK(status) {
+			if ok, _ := isOK(status); !ok {
 				return nil, fmt.Errorf("got status %d from deploy service at %s", status, service.BaseURL)
 			}
 		} else {
@@ -111,8 +111,8 @@ func (t *customTarget) waitForConvergence(timeout time.Duration) error {
 	}
 	converged := false
 	convergedFunc := func(status int, response []byte) (bool, error) {
-		if !isOK(status) {
-			return false, nil
+		if ok, err := isOK(status); !ok {
+			return ok, err
 		}
 		var resp serviceConvergeResponse
 		if err := json.Unmarshal(response, &resp); err != nil {
diff --git a/client/go/internal/vespa/target_test.go b/client/go/internal/vespa/target_test.go
index d15001911d0..bf266e8f9ec 100644
--- a/client/go/internal/vespa/target_test.go
+++ b/client/go/internal/vespa/target_test.go
@@ -18,10 +18,16 @@ import (
 
 type mockVespaApi struct {
 	deploymentConverged bool
+	authFailure         bool
 	serverURL           string
 }
 
 func (v *mockVespaApi) mockVespaHandler(w http.ResponseWriter, req *http.Request) {
+	if v.authFailure {
+		response := `{"message":"unauthorized"}`
+		w.WriteHeader(401)
+		w.Write([]byte(response))
+	}
 	switch req.URL.Path {
 	case "/cli/v1/":
 		response := `{"minVersion":"8.0.0"}`
@@ -106,6 +112,9 @@ func TestCloudTargetWait(t *testing.T) {
 
 	var logWriter bytes.Buffer
 	target := createCloudTarget(t, srv.URL, &logWriter)
+	vc.authFailure = true
+	assertServiceWaitErr(t, 401, true, target, "deploy")
+	vc.authFailure = false
 	assertServiceWait(t, 200, target, "deploy")
 
 	_, err := target.Service("query", time.Millisecond, 42, "")
@@ -188,11 +197,19 @@ func assertServiceURL(t *testing.T, url string, target Target, service string) {
 }
 
 func assertServiceWait(t *testing.T, expectedStatus int, target Target, service string) {
+	assertServiceWaitErr(t, expectedStatus, false, target, service)
+}
+
+func assertServiceWaitErr(t *testing.T, expectedStatus int, expectErr bool, target Target, service string) {
 	s, err := target.Service(service, 0, 42, "")
 	assert.Nil(t, err)
 
 	status, err := s.Wait(0)
-	assert.Nil(t, err)
+	if expectErr {
+		assert.NotNil(t, err)
+	} else {
+		assert.Nil(t, err)
+	}
 	assert.Equal(t, expectedStatus, status)
 }