Handle forbidden response from node-repo

3 files changed, 39 insertions, 11 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/HttpException.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/HttpException.java
index 0a2ae1bd426..d0f436d16b6 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/HttpException.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/HttpException.java
@@ -4,13 +4,26 @@ package com.yahoo.vespa.hosted.node.admin.configserver;
 import javax.ws.rs.core.Response;
 import java.util.Optional;
 
+/**
+ * @author hakonhall
+ */
 @SuppressWarnings("serial")
 public class HttpException extends RuntimeException {
+
     public static class NotFoundException extends HttpException {
-        private static final long serialVersionUID = 4791511887L;
+
         public NotFoundException(String message) {
             super(Response.Status.NOT_FOUND, message);
         }
+
+    }
+
+    public static class ForbiddenException extends HttpException {
+
+        public ForbiddenException(String message) {
+            super(Response.Status.FORBIDDEN, message);
+        }
+
     }
 
     /**
@@ -28,6 +41,8 @@ public class HttpException extends RuntimeException {
             case SUCCESSFUL: return Optional.empty();
             case CLIENT_ERROR:
                 switch (status) {
+                    case FORBIDDEN:
+                        throw new ForbiddenException(message);
                     case NOT_FOUND:
                         throw new NotFoundException(message);
                     case CONFLICT:
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/RealNodeRepository.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/RealNodeRepository.java
index d2e9339a012..b84adb1bdbb 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/RealNodeRepository.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/RealNodeRepository.java
@@ -71,7 +71,10 @@ public class RealNodeRepository implements NodeRepository {
                 return Optional.empty();
             }
             return Optional.of(createContainerNodeSpec(nodeResponse));
-        } catch (HttpException.NotFoundException e) {
+        } catch (HttpException.NotFoundException|HttpException.ForbiddenException e) {
+            // Return empty on 403 in addition to 404 as it likely means we're trying to access a node that
+            // has been deleted. When a node is deleted, the parent-child relationship no longer exists and
+            // authorization cannot be granted.
             return Optional.empty();
         }
     }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImplTest.java
index f39a64d2dee..7c0b1f748f9 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImplTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/configserver/ConfigServerApiImplTest.java
@@ -76,14 +76,14 @@ public class ConfigServerApiImplTest {
     }
 
     @Test
-    public void testBasicParsingSingleServer() throws Exception {
+    public void testBasicParsingSingleServer() {
         TestPojo answer = executor.get("/path", TestPojo.class);
         assertThat(answer.foo, is("bar"));
         assertLogStringContainsGETForAHost();
     }
 
     @Test(expected = HttpException.class)
-    public void testBasicFailure() throws Exception {
+    public void testBasicFailure() {
         // Server is returning 400, no retries.
         mockReturnCode = 400;
 
@@ -93,7 +93,7 @@ public class ConfigServerApiImplTest {
     }
 
     @Test
-    public void testBasicSuccessWithNoRetries() throws Exception {
+    public void testBasicSuccessWithNoRetries() {
         // Server is returning 201, no retries.
         mockReturnCode = 201;
 
@@ -103,7 +103,7 @@ public class ConfigServerApiImplTest {
     }
 
     @Test
-    public void testRetries() throws Exception {
+    public void testRetries() {
         // Client is throwing exception, should be retries.
         mockReturnCode = 100000;
         try {
@@ -118,7 +118,7 @@ public class ConfigServerApiImplTest {
     }
 
     @Test
-    public void testRetriesOnBadHttpResponseCode() throws Exception {
+    public void testRetriesOnBadHttpResponseCode() {
         // Client is throwing exception, should be retries.
         mockReturnCode = 503;
         try {
@@ -134,7 +134,19 @@ public class ConfigServerApiImplTest {
     }
 
     @Test
-    public void testNotFound() throws Exception {
+    public void testForbidden() {
+        mockReturnCode = 403;
+        try {
+            executor.get("/path", TestPojo.class);
+            fail("Expected exception");
+        } catch (HttpException.ForbiddenException e) {
+            // ignore
+        }
+        assertLogStringContainsGETForAHost();
+    }
+
+    @Test
+    public void testNotFound() {
         // Server is returning 404, special exception is thrown.
         mockReturnCode = 404;
         try {
@@ -147,7 +159,7 @@ public class ConfigServerApiImplTest {
     }
 
     @Test
-    public void testConflict() throws Exception {
+    public void testConflict() {
         // Server is returning 409, no exception is thrown.
         mockReturnCode = 409;
         executor.get("/path", TestPojo.class);
@@ -156,8 +168,6 @@ public class ConfigServerApiImplTest {
 
     private void assertLogStringContainsGETForAHost() {
         String logString = mockLog.toString();
-        //assertThat(logString, startsWith("GET http://host"));
-        //assertThat(logString, endsWith(":666/path  "));
         assertTrue("log does not contain expected entries:" + logString,
                    (logString.equals("GET http://host1:666/path  ") || logString.equals("GET http://host2:666/path  ")));
     }