Improve validation

4 files changed, 27 insertions, 7 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/OsController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/OsController.java
index 1c77efe095c..6ad7a005036 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/OsController.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/OsController.java
@@ -54,9 +54,7 @@ public record OsController(Controller controller) {
      * @param pin     Pin this version. This prevents automatic scheduling of upgrades until version is unpinned
      */
     public void upgradeTo(Version version, CloudName cloud, boolean force, boolean pin) {
-        if (version.isEmpty()) {
-            throw new IllegalArgumentException("Invalid version '" + version.toFullString() + "'");
-        }
+        requireNonEmpty(version);
         requireCloud(cloud);
         Instant scheduledAt = controller.clock().instant();
         try (Mutex lock = curator().lockOsVersions()) {
@@ -124,6 +122,8 @@ public record OsController(Controller controller) {
 
     /** Certify an OS version as compatible with given Vespa version */
     public CertifiedOsVersion certify(Version version, CloudName cloud, Version vespaVersion) {
+        requireNonEmpty(version);
+        requireNonEmpty(vespaVersion);
         requireCloud(cloud);
         try (Mutex lock = curator().lockCertifiedOsVersions()) {
             OsVersion osVersion = new OsVersion(version, cloud);
@@ -150,7 +150,7 @@ public record OsController(Controller controller) {
                                                                      .filter(cv -> cv.osVersion().equals(osVersion))
                                                                      .findFirst();
             if (existing.isEmpty()) {
-                throw new IllegalArgumentException(version + " is not certified");
+                throw new IllegalArgumentException(osVersion + " is not certified");
             }
             certifiedVersions = new HashSet<>(certifiedVersions);
             certifiedVersions.remove(existing.get());
@@ -187,6 +187,12 @@ public record OsController(Controller controller) {
         }
     }
 
+    private void requireNonEmpty(Version version) {
+        if (version.isEmpty()) {
+            throw new IllegalArgumentException("Invalid version '" + version.toFullString() + "'");
+        }
+    }
+
     private CuratorDb curator() {
         return controller.curator();
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/os/OsApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/os/OsApiHandler.java
index fe1fb979abc..b3bb2797a89 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/os/OsApiHandler.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/os/OsApiHandler.java
@@ -110,7 +110,11 @@ public class OsApiHandler extends AuditLoggingRequestHandler {
     private HttpResponse certifyVersion(HttpRequest request, String versionString, String cloudName) {
         Version version = Version.fromString(versionString);
         CloudName cloud = CloudName.from(cloudName);
-        Version vespaVersion = Version.fromString(asString(request.getData()));
+        String vespaVersionString = asString(request.getData());
+        if (vespaVersionString.isEmpty()) {
+            throw new IllegalArgumentException("Missing Vespa version in request body");
+        }
+        Version vespaVersion = Version.fromString(vespaVersionString);
         CertifiedOsVersion certified = controller.os().certify(version, cloud, vespaVersion);
         if (certified.vespaVersion().equals(vespaVersion)) {
             return new MessageResponse("Certified " + version.toFullString() + " in cloud " + cloud +
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsVersionStatusUpdaterTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsVersionStatusUpdaterTest.java
index 6644c3013ff..ce121029aaa 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsVersionStatusUpdaterTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsVersionStatusUpdaterTest.java
@@ -64,7 +64,9 @@ public class OsVersionStatusUpdaterTest {
         assertTrue(osVersions.get(new OsVersion(version1, otherCloud)).isEmpty(), "No nodes on current target");
 
         // Updating status cleans up stale certifications
-        Set<OsVersion> knownVersions = osVersions.keySet();
+        Set<OsVersion> knownVersions = osVersions.keySet().stream()
+                                                 .filter(osVersion -> !osVersion.version().isEmpty())
+                                                 .collect(Collectors.toSet());
         List<OsVersion> versionsToCertify = new ArrayList<>(knownVersions);
         versionsToCertify.addAll(List.of(new OsVersion(Version.fromString("95.0.1"), cloud),
                                          new OsVersion(Version.fromString("98.0.2"), cloud)));
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/os/OsApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/os/OsApiTest.java
index 15505dc3e95..7e9cbdec2df 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/os/OsApiTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/os/OsApiTest.java
@@ -130,7 +130,7 @@ public class OsApiTest extends ControllerContainerTest {
         assertResponse(new Request("http://localhost:8080/os/v1/", "{\"version\": \"0.0.0\", \"cloud\": \"cloud1\"}", Request.Method.PATCH),
                 "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Invalid version '0.0.0'\"}", 400);
         assertResponse(new Request("http://localhost:8080/os/v1/", "{\"version\": \"foo\", \"cloud\": \"cloud1\"}", Request.Method.PATCH),
-                "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Invalid version 'foo': For input string: \\\"foo\\\"\"}", 400);
+                "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Invalid version 'foo': Invalid version component in 'foo': For input string: \\\"foo\\\"\"}", 400);
 
         // Error: Invalid cloud
         assertResponse(new Request("http://localhost:8080/os/v1/", "{\"version\": \"7.6\", \"cloud\": \"foo\"}", Request.Method.PATCH),
@@ -160,6 +160,14 @@ public class OsApiTest extends ControllerContainerTest {
         assertResponse(new Request("http://localhost:8080/os/v1/firmware/dev/", "", Request.Method.DELETE),
                 "{\"error-code\":\"NOT_FOUND\",\"message\":\"No zones at path '/os/v1/firmware/dev/'\"}", 404);
 
+        // Error: Missing or invalid versions to certify
+        assertResponse(new Request("http://localhost:8080/os/v1/certify/cloud1/7.5.2", "", Request.Method.POST),
+                       "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Missing Vespa version in request body\"}", 400);
+        assertResponse(new Request("http://localhost:8080/os/v1/certify/cloud1/7.5.2", "foo", Request.Method.POST),
+                       "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Invalid version component in 'foo': For input string: \\\"foo\\\"\"}", 400);
+        assertResponse(new Request("http://localhost:8080/os/v1/certify/cloud1/bar", "1.2.3", Request.Method.POST),
+                       "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Invalid version component in 'bar': For input string: \\\"bar\\\"\"}", 400);
+
         assertFalse(tester.controller().auditLogger().readLog().entries().isEmpty(), "Actions are logged to audit log");
     }