Merge pull request #21362 from vespa-engine/mpolden/cli-compat

Warn on API incompatibility in Vespa CLI

14 files changed, 223 insertions, 16 deletions

diff --git a/client/go/cmd/command_tester.go b/client/go/cmd/command_tester.go
index 22f50b52cee..82682b3b355 100644
--- a/client/go/cmd/command_tester.go
+++ b/client/go/cmd/command_tester.go
@@ -65,6 +65,7 @@ func execute(cmd command, t *testing.T, client *mockHttpClient) (string, string)
 	rootCmd.Flags().VisitAll(resetFlag)
 	queryCmd.Flags().VisitAll(resetFlag)
 	documentCmd.Flags().VisitAll(resetFlag)
+	logCmd.Flags().VisitAll(resetFlag)
 
 	// Capture stdout and execute command
 	var capturedOut bytes.Buffer
diff --git a/client/go/cmd/helpers.go b/client/go/cmd/helpers.go
index 6cddfd09f54..905e51dda4f 100644
--- a/client/go/cmd/helpers.go
+++ b/client/go/cmd/helpers.go
@@ -13,20 +13,18 @@ import (
 	"strings"
 	"time"
 
+	"github.com/vespa-engine/vespa/client/go/build"
+	"github.com/vespa-engine/vespa/client/go/version"
 	"github.com/vespa-engine/vespa/client/go/vespa"
 )
 
 func printErrHint(err error, hints ...string) {
-	printErr(err)
+	fmt.Fprintln(stderr, color.Red("Error:"), err)
 	for _, hint := range hints {
 		fmt.Fprintln(stderr, color.Cyan("Hint:"), hint)
 	}
 }
 
-func printErr(err error) {
-	fmt.Fprintln(stderr, color.Red("Error:"), err)
-}
-
 func printSuccess(msg ...interface{}) {
 	log.Print(color.Green("Success: "), fmt.Sprint(msg...))
 }
@@ -151,6 +149,21 @@ func getApiURL() string {
 }
 
 func getTarget() (vespa.Target, error) {
+	clientVersion, err := version.Parse(build.Version)
+	if err != nil {
+		return nil, err
+	}
+	target, err := createTarget()
+	if err != nil {
+		return nil, err
+	}
+	if err := target.CheckVersion(clientVersion); err != nil {
+		printErrHint(err, "This is not a fatal error, but this version may not work as expected", "Try 'vespa version' to check for a new version")
+	}
+	return target, nil
+}
+
+func createTarget() (vespa.Target, error) {
 	targetType, err := getTargetType()
 	if err != nil {
 		return nil, err
diff --git a/client/go/cmd/log_test.go b/client/go/cmd/log_test.go
index 4b32927ca17..9d895d1f244 100644
--- a/client/go/cmd/log_test.go
+++ b/client/go/cmd/log_test.go
@@ -6,6 +6,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/vespa-engine/vespa/client/go/build"
 )
 
 func TestLog(t *testing.T) {
@@ -26,3 +27,21 @@ func TestLog(t *testing.T) {
 	_, errOut := execute(command{homeDir: homeDir, args: []string{"log", "--from", "2021-09-27T13:12:49Z", "--to", "2021-09-27T13:15:00", "1h"}}, t, httpClient)
 	assert.Equal(t, "Error: invalid period: cannot combine --from/--to with relative value: 1h\n", errOut)
 }
+
+func TestLogOldClient(t *testing.T) {
+	buildVersion := build.Version
+	build.Version = "7.0.0"
+	homeDir := filepath.Join(t.TempDir(), ".vespa")
+	pkgDir := mockApplicationPackage(t, false)
+	httpClient := &mockHttpClient{}
+	httpClient.NextResponse(200, `{"minVersion": "8.0.0"}`)
+	execute(command{homeDir: homeDir, args: []string{"config", "set", "application", "t1.a1.i1"}}, t, httpClient)
+	execute(command{homeDir: homeDir, args: []string{"config", "set", "target", "cloud"}}, t, httpClient)
+	execute(command{homeDir: homeDir, args: []string{"api-key"}}, t, httpClient)
+	execute(command{homeDir: homeDir, args: []string{"cert", pkgDir}}, t, httpClient)
+	out, errOut := execute(command{homeDir: homeDir, args: []string{"log"}}, t, httpClient)
+	assert.Equal(t, "", out)
+	expected := "Error: client version 7.0.0 is less than the minimum supported version: 8.0.0\nHint: This is not a fatal error, but this version may not work as expected\nHint: Try 'vespa version' to check for a new version\n"
+	assert.Equal(t, expected, errOut)
+	build.Version = buildVersion
+}
diff --git a/client/go/cmd/prod.go b/client/go/cmd/prod.go
index 89f401a356e..8c40eb969bf 100644
--- a/client/go/cmd/prod.go
+++ b/client/go/cmd/prod.go
@@ -362,7 +362,7 @@ func prompt(r *bufio.Reader, question, defaultAnswer string, validator func(inpu
 		}
 
 		if err := validator(input); err != nil {
-			printErr(err)
+			printErrHint(err)
 			fmt.Fprintln(stderr)
 			input = ""
 		}
diff --git a/client/go/cmd/root.go b/client/go/cmd/root.go
index e25c5cb2d0d..0f4bef5595f 100644
--- a/client/go/cmd/root.go
+++ b/client/go/cmd/root.go
@@ -134,7 +134,7 @@ func Execute() error {
 				printErrHint(cliErr, cliErr.hints...)
 			}
 		} else {
-			printErr(err)
+			printErrHint(err)
 		}
 	}
 	return err
diff --git a/client/go/version/version.go b/client/go/version/version.go
index 00e26d25135..b27529fa5e1 100644
--- a/client/go/version/version.go
+++ b/client/go/version/version.go
@@ -15,6 +15,11 @@ type Version struct {
 	Label string
 }
 
+// IsZero returns whether v is the zero version, 0.0.0.
+func (v Version) IsZero() bool {
+	return v.Major == 0 && v.Minor == 0 && v.Patch == 0
+}
+
 func (v Version) String() string {
 	var sb strings.Builder
 	sb.WriteString(strconv.Itoa(v.Major))
diff --git a/client/go/version/version_test.go b/client/go/version/version_test.go
index 759b1c1d0c1..1caf99a1dd4 100644
--- a/client/go/version/version_test.go
+++ b/client/go/version/version_test.go
@@ -25,6 +25,20 @@ func TestParse(t *testing.T) {
 	assert.Equal(t, "1.2.3-foo", v.String())
 }
 
+func TestIsZero(t *testing.T) {
+	v, err := Parse("0.0.0")
+	assert.Nil(t, err)
+	assert.True(t, v.IsZero())
+
+	v, err = Parse("0.0.0-foo")
+	assert.Nil(t, err)
+	assert.True(t, v.IsZero())
+
+	v, err = Parse("1.2.3")
+	assert.Nil(t, err)
+	assert.False(t, v.IsZero())
+}
+
 func TestCompare(t *testing.T) {
 	assertComparison(t, "1.2.3", '>', "1.0.0")
 	assertComparison(t, "1.0.0", '<', "1.2.3")
diff --git a/client/go/vespa/deploy.go b/client/go/vespa/deploy.go
index 6ef4995e181..cac03067a69 100644
--- a/client/go/vespa/deploy.go
+++ b/client/go/vespa/deploy.go
@@ -333,7 +333,7 @@ func Submit(opts DeploymentOpts) error {
 	request.Header.Set("Content-Type", writer.FormDataContentType())
 	serviceDescription := "Submit service"
 	sigKeyId := opts.Deployment.Application.SerializedForm()
-	if err := opts.Target.PrepareApiRequest(request, sigKeyId); err != nil {
+	if err := opts.Target.SignRequest(request, sigKeyId); err != nil {
 		return err
 	}
 	response, err := util.HttpDo(request, time.Minute*10, sigKeyId)
@@ -371,7 +371,7 @@ func uploadApplicationPackage(url *url.URL, opts DeploymentOpts) (int64, error)
 	}
 	serviceDescription := "Deploy service"
 	sigKeyId := opts.Deployment.Application.SerializedForm()
-	if err := opts.Target.PrepareApiRequest(request, sigKeyId); err != nil {
+	if err := opts.Target.SignRequest(request, sigKeyId); err != nil {
 		return 0, err
 	}
 
diff --git a/client/go/vespa/target.go b/client/go/vespa/target.go
index f50716a5a3a..ae4934d788d 100644
--- a/client/go/vespa/target.go
+++ b/client/go/vespa/target.go
@@ -20,6 +20,7 @@ import (
 
 	"github.com/vespa-engine/vespa/client/go/auth0"
 	"github.com/vespa-engine/vespa/client/go/util"
+	"github.com/vespa-engine/vespa/client/go/version"
 )
 
 const (
@@ -52,7 +53,11 @@ type Target interface {
 	// PrintLog writes the logs of this deployment using given options to control output.
 	PrintLog(options LogOptions) error
 
-	PrepareApiRequest(req *http.Request, sigKeyId string) error
+	// SignRequest signs request with given keyID as required by the implementation of this target.
+	SignRequest(request *http.Request, keyID string) error
+
+	// CheckVersion verifies whether clientVersion is compatible with this target.
+	CheckVersion(clientVersion version.Version) error
 }
 
 // TLSOptions configures the certificate to use for service requests.
@@ -85,7 +90,9 @@ type customTarget struct {
 	baseURL    string
 }
 
-func (t *customTarget) PrepareApiRequest(req *http.Request, sigKeyId string) error { return nil }
+func (t *customTarget) SignRequest(req *http.Request, sigKeyId string) error { return nil }
+
+func (t *customTarget) CheckVersion(version version.Version) error { return nil }
 
 // Do sends request to this service. Any required authentication happens automatically.
 func (s *Service) Do(request *http.Request, timeout time.Duration) (*http.Response, error) {
@@ -265,7 +272,7 @@ func (t *cloudTarget) Service(name string, timeout time.Duration, runID int64, c
 	return nil, fmt.Errorf("unknown service: %s", name)
 }
 
-func (t *cloudTarget) PrepareApiRequest(req *http.Request, sigKeyId string) error {
+func (t *cloudTarget) SignRequest(req *http.Request, sigKeyId string) error {
 	if Auth0AccessTokenEnabled() {
 		if t.cloudAuth == "access-token" {
 			if err := t.addAuth0AccessToken(req); err != nil {
@@ -289,6 +296,36 @@ func (t *cloudTarget) PrepareApiRequest(req *http.Request, sigKeyId string) erro
 	return nil
 }
 
+func (t *cloudTarget) CheckVersion(clientVersion version.Version) error {
+	if clientVersion.IsZero() { // development version is always fine
+		return nil
+	}
+	req, err := http.NewRequest("GET", fmt.Sprintf("%s/cli/v1/", t.apiURL), nil)
+	if err != nil {
+		return err
+	}
+	response, err := util.HttpDo(req, 10*time.Second, "")
+	if err != nil {
+		return err
+	}
+	defer response.Body.Close()
+	var cliResponse struct {
+		MinVersion string `json:"minVersion"`
+	}
+	dec := json.NewDecoder(response.Body)
+	if err := dec.Decode(&cliResponse); err != nil {
+		return err
+	}
+	minVersion, err := version.Parse(cliResponse.MinVersion)
+	if err != nil {
+		return err
+	}
+	if clientVersion.Less(minVersion) {
+		return fmt.Errorf("client version %s is less than the minimum supported version: %s", clientVersion, minVersion)
+	}
+	return nil
+}
+
 func (t *cloudTarget) addAuth0AccessToken(request *http.Request) error {
 	a, err := auth0.GetAuth0(t.authConfigPath, t.systemName, t.apiURL)
 	if err != nil {
@@ -324,7 +361,7 @@ func (t *cloudTarget) PrintLog(options LogOptions) error {
 			q.Set("to", strconv.FormatInt(toMillis, 10))
 		}
 		req.URL.RawQuery = q.Encode()
-		t.PrepareApiRequest(req, t.deployment.Application.SerializedForm())
+		t.SignRequest(req, t.deployment.Application.SerializedForm())
 		return req
 	}
 	logFunc := func(status int, response []byte) (bool, error) {
@@ -380,7 +417,7 @@ func (t *cloudTarget) waitForRun(runID int64, timeout time.Duration) error {
 		q := req.URL.Query()
 		q.Set("after", strconv.FormatInt(lastID, 10))
 		req.URL.RawQuery = q.Encode()
-		if err := t.PrepareApiRequest(req, t.deployment.Application.SerializedForm()); err != nil {
+		if err := t.SignRequest(req, t.deployment.Application.SerializedForm()); err != nil {
 			panic(err)
 		}
 		return req
@@ -439,7 +476,7 @@ func (t *cloudTarget) discoverEndpoints(timeout time.Duration) error {
 	if err != nil {
 		return err
 	}
-	if err := t.PrepareApiRequest(req, t.deployment.Application.SerializedForm()); err != nil {
+	if err := t.SignRequest(req, t.deployment.Application.SerializedForm()); err != nil {
 		return err
 	}
 	urlsByCluster := make(map[string]string)
diff --git a/client/go/vespa/target_test.go b/client/go/vespa/target_test.go
index 0cfe9f1962c..5aa20b22465 100644
--- a/client/go/vespa/target_test.go
+++ b/client/go/vespa/target_test.go
@@ -13,6 +13,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/vespa-engine/vespa/client/go/version"
 )
 
 type mockVespaApi struct {
@@ -22,6 +23,9 @@ type mockVespaApi struct {
 
 func (v *mockVespaApi) mockVespaHandler(w http.ResponseWriter, req *http.Request) {
 	switch req.URL.Path {
+	case "/cli/v1/":
+		response := `{"minVersion":"8.0.0"}`
+		w.Write([]byte(response))
 	case "/application/v4/tenant/t1/application/a1/instance/i1/environment/dev/region/us-north-1":
 		response := "{}"
 		if v.deploymentConverged {
@@ -137,6 +141,25 @@ func TestLog(t *testing.T) {
 	assert.Equal(t, expected, buf.String())
 }
 
+func TestCheckVersion(t *testing.T) {
+	vc := mockVespaApi{}
+	srv := httptest.NewServer(http.HandlerFunc(vc.mockVespaHandler))
+	defer srv.Close()
+
+	target := createCloudTarget(t, srv.URL, ioutil.Discard)
+	assert.Nil(t, target.CheckVersion(mustVersion("8.0.0")))
+	assert.Nil(t, target.CheckVersion(mustVersion("8.1.0")))
+	assert.NotNil(t, target.CheckVersion(mustVersion("7.0.0")))
+}
+
+func mustVersion(s string) version.Version {
+	v, err := version.Parse(s)
+	if err != nil {
+		panic(err)
+	}
+	return v
+}
+
 func createCloudTarget(t *testing.T, url string, logWriter io.Writer) Target {
 	kp, err := CreateKeyPair()
 	assert.Nil(t, err)
diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/role/PathGroup.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/role/PathGroup.java
index b34d113d331..994eb1d7532 100644
--- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/role/PathGroup.java
+++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/role/PathGroup.java
@@ -211,7 +211,8 @@ enum PathGroup {
     /** Paths providing public information. */
     publicInfo("/user/v1/user",     // Information about who you are.
                "/badge/v1/{*}",     // Badges for deployment jobs.
-               "/zone/v1/{*}"),     // Lists environment and regions.
+               "/zone/v1/{*}",      // Lists environment and regions.
+               "/cli/v1/{*}"),      // Public information for Vespa CLI.
 
     /** Paths used for deploying system-wide feature flags. */
     systemFlagsDeploy("/system-flags/v1/deploy"),
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/deployment/CliApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/deployment/CliApiHandler.java
new file mode 100644
index 00000000000..ab8b6a1d26f
--- /dev/null
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/deployment/CliApiHandler.java
@@ -0,0 +1,65 @@
+// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.hosted.controller.restapi.deployment;
+
+import com.yahoo.component.Version;
+import com.yahoo.container.jdisc.HttpRequest;
+import com.yahoo.container.jdisc.HttpResponse;
+import com.yahoo.container.jdisc.ThreadedHttpRequestHandler;
+import com.yahoo.restapi.ErrorResponse;
+import com.yahoo.restapi.Path;
+import com.yahoo.restapi.SlimeJsonResponse;
+import com.yahoo.slime.Cursor;
+import com.yahoo.slime.Slime;
+import com.yahoo.yolean.Exceptions;
+
+import java.util.logging.Level;
+
+/**
+ * This handler implements the /cli/v1/ API. The API allows Vespa CLI to retrieve information about the system, without
+ * authorization. One example of such information is the minimum Vespa CLI version supported by our APIs.
+ *
+ * @author mpolden
+ */
+public class CliApiHandler extends ThreadedHttpRequestHandler {
+
+    /**
+     * The minimum version of Vespa CLI which is considered compatible with our APIs. If a version of Vespa CLI below
+     * this version tries to use our APIs, Vespa CLI will print a warning instructing the user to upgrade.
+     */
+    private static final Version MIN_CLI_VERSION = Version.fromString("7.547.18");
+
+    public CliApiHandler(Context context) {
+        super(context);
+    }
+
+    @Override
+    public HttpResponse handle(HttpRequest request) {
+        try {
+            switch (request.getMethod()) {
+                case GET: return get(request);
+                default: return ErrorResponse.methodNotAllowed("Method '" + request.getMethod() + "' is not supported");
+            }
+        }
+        catch (IllegalArgumentException e) {
+            return ErrorResponse.badRequest(Exceptions.toMessageString(e));
+        }
+        catch (RuntimeException e) {
+            log.log(Level.WARNING, "Unexpected error handling '" + request.getUri() + "'", e);
+            return ErrorResponse.internalServerError(Exceptions.toMessageString(e));
+        }
+    }
+
+    private HttpResponse get(HttpRequest request) {
+        Path path = new Path(request.getUri());
+        if (path.matches("/cli/v1/")) return root();
+        return ErrorResponse.notFoundError("Nothing at " + path);
+    }
+
+    private HttpResponse root() {
+        Slime slime = new Slime();
+        Cursor root = slime.setObject();
+        root.setString("minVersion", MIN_CLI_VERSION.toFullString());
+        return new SlimeJsonResponse(slime);
+    }
+
+}
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ControllerContainerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ControllerContainerTest.java
index 621a3272918..6b13e6c951a 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ControllerContainerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ControllerContainerTest.java
@@ -78,6 +78,9 @@ public class ControllerContainerTest {
                "  <handler id='com.yahoo.vespa.hosted.controller.restapi.deployment.BadgeApiHandler'>\n" +
                "    <binding>http://*/badge/v1/*</binding>\n" +
                "  </handler>\n" +
+               "  <handler id='com.yahoo.vespa.hosted.controller.restapi.deployment.CliApiHandler'>\n" +
+               "    <binding>http://*/cli/v1/*</binding>\n" +
+               "  </handler>\n" +
                "  <handler id='com.yahoo.vespa.hosted.controller.restapi.controller.ControllerApiHandler'>\n" +
                "    <binding>http://*/controller/v1/*</binding>\n" +
                "  </handler>\n" +
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/CliApiHandlerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/CliApiHandlerTest.java
new file mode 100644
index 00000000000..5a4d47e0db5
--- /dev/null
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/CliApiHandlerTest.java
@@ -0,0 +1,26 @@
+// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.hosted.controller.restapi.application;
+
+import com.yahoo.vespa.hosted.controller.restapi.ContainerTester;
+import com.yahoo.vespa.hosted.controller.restapi.ControllerContainerTest;
+import org.junit.Before;
+import org.junit.Test;
+
+/**
+ * @author mpolden
+ */
+public class CliApiHandlerTest extends ControllerContainerTest {
+
+    private ContainerTester tester;
+
+    @Before
+    public void before() {
+        tester = new ContainerTester(container, "src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/");
+    }
+
+    @Test
+    public void root() {
+        tester.assertResponse(authenticatedRequest("http://localhost:8080/cli/v1/"), "{\"minVersion\":\"7.547.18\"}");
+    }
+
+}