refactor(google): use Get instead of List to find a specific image (#…

…3879)

I found this while looking for calls that needed pagination support. I
started to add pagination to the List call before I realized there was
really no point since we were applying a filter so that it would only
ever return a single image (or none).

I'm not sure why this didn't occur to me when I originally wrote the
code.

clouddriver-google/src/main/groovy/com/netflix/spinnaker/clouddriver/google/compute/Images.java

@@ -18,7 +18,7 @@
 
 import com.google.api.services.compute.Compute;
 import com.google.api.services.compute.ComputeRequest;
-import com.google.api.services.compute.model.ImageList;
+import com.google.api.services.compute.model.Image;
 import com.google.common.collect.ImmutableMap;
 import com.netflix.spectator.api.Registry;
 import com.netflix.spinnaker.clouddriver.google.GoogleExecutor;
@@ -38,11 +38,10 @@ public Images(GoogleNamedAccountCredentials credentials, Registry registry) {
     this.registry = registry;
   }
 
-  public GoogleComputeRequest<Compute.Images.List, ImageList> list(String project)
+  public GoogleComputeRequest<Compute.Images.Get, Image> get(String project, String image)
       throws IOException {
-
-    Compute.Images.List request = credentials.getCompute().images().list(project);
-    return wrapRequest(request, "list");
+    Compute.Images.Get request = credentials.getCompute().images().get(project, image);
+    return wrapRequest(request, "get");
   }
 
   private <RequestT extends ComputeRequest<ResponseT>, ResponseT>

clouddriver-google/src/main/groovy/com/netflix/spinnaker/clouddriver/google/deploy/ops/StatefullyUpdateBootImageAtomicOperation.java

@@ -23,12 +23,12 @@
 import com.google.api.client.googleapis.batch.json.JsonBatchCallback;
 import com.google.api.client.googleapis.json.GoogleJsonError;
 import com.google.api.client.http.HttpHeaders;
+import com.google.api.client.http.HttpStatusCodes;
 import com.google.api.client.util.Throwables;
 import com.google.api.services.compute.Compute;
 import com.google.api.services.compute.Compute.InstanceTemplates.Get;
 import com.google.api.services.compute.model.AttachedDisk;
 import com.google.api.services.compute.model.Image;
-import com.google.api.services.compute.model.ImageList;
 import com.google.api.services.compute.model.InstanceGroupManager;
 import com.google.api.services.compute.model.InstanceGroupManagerUpdatePolicy;
 import com.google.api.services.compute.model.InstanceTemplate;
@@ -173,13 +173,12 @@ private Image getImage(Task task, GoogleNamedAccountCredentials credentials) thr
     Images imagesApi = computeApiFactory.createImages(credentials);
 
     SettableFuture<Image> foundImage = SettableFuture.create();
-    String filter = "name eq " + description.getBootImage();
 
-    ComputeBatchRequest<Compute.Images.List, ImageList> batchRequest =
+    ComputeBatchRequest<Compute.Images.Get, Image> batchRequest =
         computeApiFactory.createBatchRequest(credentials);
     for (String project : getImageProjects(credentials)) {
-      GoogleComputeRequest<Compute.Images.List, ImageList> request = imagesApi.list(project);
-      request.getRequest().setFilter(filter);
+      GoogleComputeRequest<Compute.Images.Get, Image> request =
+          imagesApi.get(project, description.getBootImage());
       batchRequest.queue(request, new ImageListCallback(project, foundImage));
     }
     batchRequest.execute("findImage");
@@ -213,7 +212,7 @@ private ImmutableSet<String> getImageProjects(GoogleNamedAccountCredentials cred
         .build();
   }
 
-  private static class ImageListCallback extends JsonBatchCallback<ImageList> {
+  private static class ImageListCallback extends JsonBatchCallback<Image> {
 
     final String project;
     final SettableFuture<Image> foundImage;
@@ -224,15 +223,18 @@ private static class ImageListCallback extends JsonBatchCallback<ImageList> {
     }
 
     @Override
-    public void onSuccess(ImageList imageList, HttpHeaders responseHeaders) {
-      if (imageList.getItems() != null && !imageList.getItems().isEmpty())
-        foundImage.set(imageList.getItems().get(0));
+    public void onSuccess(Image image, HttpHeaders responseHeaders) {
+      foundImage.set(image);
     }
 
     @Override
     public void onFailure(GoogleJsonError e, HttpHeaders responseHeaders) {
-      log.warn(
-          String.format("Error retrieving images from project %s: %s", project, e.getMessage()));
+      // Only a single project (of the many we query) will actually contain this image, so 404s are
+      // expected.
+      if (e.getCode() != HttpStatusCodes.STATUS_CODE_NOT_FOUND) {
+        log.warn(
+            String.format("Error retrieving images from project %s: %s", project, e.getMessage()));
+      }
     }
   }
 

clouddriver-google/src/test/groovy/com/netflix/spinnaker/clouddriver/google/compute/FakeComputeBatchRequest.java

@@ -18,6 +18,7 @@
 
 import com.google.api.client.googleapis.batch.json.JsonBatchCallback;
 import com.google.api.client.googleapis.json.GoogleJsonError;
+import com.google.api.client.googleapis.json.GoogleJsonResponseException;
 import com.google.api.client.http.HttpHeaders;
 import com.google.api.services.compute.ComputeRequest;
 import java.io.IOException;
@@ -40,6 +41,15 @@ public void execute(String batchContext) throws IOException {
     for (QueuedRequest request : requests) {
       try {
         request.callback.onSuccess(request.request.execute(), new HttpHeaders());
+      } catch (GoogleJsonResponseException e) {
+        // Exceptions created by GoogleJsonResponseExceptionFactoryTesting don't have details.
+        GoogleJsonError details = e.getDetails();
+        if (details == null) {
+          details = new GoogleJsonError();
+          details.setCode(e.getStatusCode());
+          details.setMessage(e.getMessage());
+        }
+        request.callback.onFailure(details, e.getHeaders());
       } catch (IOException | RuntimeException e) {
         request.callback.onFailure(new GoogleJsonError(), new HttpHeaders());
       }

clouddriver-google/src/test/groovy/com/netflix/spinnaker/clouddriver/google/compute/FakeGoogleComputeOperationRequest.java

@@ -32,7 +32,7 @@ public FakeGoogleComputeOperationRequest() {
   }
 
   public FakeGoogleComputeOperationRequest(Operation response) {
-    super(response);
+    super(response, /* request= */ null);
   }
 
   @Override

clouddriver-google/src/test/groovy/com/netflix/spinnaker/clouddriver/google/compute/FakeGoogleComputeRequest.java

@@ -16,28 +16,57 @@
 
 package com.netflix.spinnaker.clouddriver.google.compute;
 
+import static com.google.common.base.Preconditions.checkNotNull;
+
 import com.google.api.services.compute.ComputeRequest;
+import java.io.IOException;
+import javax.annotation.Nullable;
 
 public class FakeGoogleComputeRequest<RequestT extends ComputeRequest<ResponseT>, ResponseT>
     implements GoogleComputeRequest<RequestT, ResponseT> {
 
   private final RequestT request;
   private final ResponseT response;
+  private final IOException exception;
 
   private boolean executed = false;
 
-  public FakeGoogleComputeRequest(ResponseT response) {
-    this(null, response);
+  public static <RequestT extends ComputeRequest<ResponseT>, ResponseT>
+      FakeGoogleComputeRequest<RequestT, ResponseT> createWithResponse(ResponseT response) {
+    return createWithResponse(response, /* request= */ null);
+  }
+
+  public static <RequestT extends ComputeRequest<ResponseT>, ResponseT>
+      FakeGoogleComputeRequest<RequestT, ResponseT> createWithResponse(
+          ResponseT response, RequestT request) {
+    return new FakeGoogleComputeRequest<>(response, request);
+  }
+
+  public static <RequestT extends ComputeRequest<ResponseT>, ResponseT>
+      FakeGoogleComputeRequest<RequestT, ResponseT> createWithException(IOException exception) {
+    return new FakeGoogleComputeRequest<RequestT, ResponseT>(exception, /* request= */ null);
   }
 
-  public FakeGoogleComputeRequest(RequestT request, ResponseT response) {
+  FakeGoogleComputeRequest(ResponseT response, @Nullable RequestT request) {
+    checkNotNull(response);
     this.request = request;
     this.response = response;
+    this.exception = null;
+  }
+
+  FakeGoogleComputeRequest(IOException exception, @Nullable RequestT request) {
+    checkNotNull(exception);
+    this.request = request;
+    this.response = null;
+    this.exception = exception;
   }
 
   @Override
-  public ResponseT execute() {
+  public ResponseT execute() throws IOException {
     executed = true;
+    if (exception != null) {
+      throw exception;
+    }
     return response;
   }
 

clouddriver-google/src/test/groovy/com/netflix/spinnaker/clouddriver/google/compute/ImagesTest.java

@@ -24,7 +24,6 @@
 import com.google.api.client.testing.http.MockLowLevelHttpResponse;
 import com.google.api.services.compute.Compute;
 import com.google.api.services.compute.model.Image;
-import com.google.api.services.compute.model.ImageList;
 import com.netflix.spectator.api.BasicTag;
 import com.netflix.spectator.api.DefaultRegistry;
 import com.netflix.spectator.api.NoopRegistry;
@@ -34,42 +33,34 @@
 import com.netflix.spinnaker.clouddriver.google.security.FakeGoogleCredentials;
 import com.netflix.spinnaker.clouddriver.google.security.GoogleNamedAccountCredentials;
 import java.io.IOException;
-import java.util.List;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
+import org.junit.platform.runner.JUnitPlatform;
+import org.junit.runner.RunWith;
 
-public class ImagesTest {
+@RunWith(JUnitPlatform.class)
+final class ImagesTest {
 
   private static final int CLOCK_STEP_TIME_MS = 1234;
   private static final int CLOCK_STEP_TIME_NS = 1234 * 1000000;
 
   @Test
-  public void list_success() throws IOException {
+  void get_success() throws IOException {
 
     HttpTransport transport =
         new ComputeOperationMockHttpTransport(
             new MockLowLevelHttpResponse()
                 .setStatusCode(200)
-                .setContent(
-                    ""
-                        + "{"
-                        + "  \"items\": ["
-                        + "    { \"name\": \"centos\" },"
-                        + "    { \"name\": \"ubuntu\" }"
-                        + "  ]"
-                        + "}"));
+                .setContent("{ \"name\": \"my-wacky-image-name\" }"));
 
     Images imagesApi = createImages(transport);
 
-    ImageList imageList = imagesApi.list("my-project").execute();
+    Image image = imagesApi.get("my-project", "my-image").execute();
 
-    List<Image> images = imageList.getItems();
-    assertThat(images).hasSize(2);
-    assertThat(images.get(0).getName()).isEqualTo("centos");
-    assertThat(images.get(1).getName()).isEqualTo("ubuntu");
+    assertThat(image.getName()).isEqualTo("my-wacky-image-name");
   }
 
   @Test
-  public void list_error() {
+  public void get_error() {
 
     HttpTransport transport =
         new ComputeOperationMockHttpTransport(
@@ -78,12 +69,12 @@ public void list_error() {
     Images imagesApi = createImages(transport);
 
     assertThatIOException()
-        .isThrownBy(() -> imagesApi.list("my-project").execute())
+        .isThrownBy(() -> imagesApi.get("my-project", "my-image").execute())
         .withMessageContaining("404");
   }
 
   @Test
-  public void list_successMetrics() throws IOException {
+  public void get_successMetrics() throws IOException {
 
     Registry registry = new DefaultRegistry(new SteppingClock(CLOCK_STEP_TIME_MS));
     HttpTransport transport =
@@ -92,7 +83,7 @@ public void list_successMetrics() throws IOException {
 
     Images imagesApi = createImages(transport, registry);
 
-    imagesApi.list("my-project").execute();
+    imagesApi.get("my-project", "my-image").execute();
 
     assertThat(registry.timers().count()).isEqualTo(1);
     Timer timer = registry.timers().findFirst().orElseThrow(AssertionError::new);
@@ -101,15 +92,15 @@ public void list_successMetrics() throws IOException {
     //               global state) so that we can test for the account tags
     assertThat(timer.id().tags())
         .contains(
-            tag("api", "compute.images.list"),
+            tag("api", "compute.images.get"),
             tag("scope", "global"),
             tag("status", "2xx"),
             tag("success", "true"));
     assertThat(timer.totalTime()).isEqualTo(CLOCK_STEP_TIME_NS);
   }
 
   @Test
-  public void list_errorMetrics() {
+  public void get_errorMetrics() {
 
     Registry registry = new DefaultRegistry(new SteppingClock(CLOCK_STEP_TIME_MS));
     HttpTransport transport =
@@ -118,7 +109,7 @@ public void list_errorMetrics() {
 
     Images imagesApi = createImages(transport, registry);
 
-    assertThatIOException().isThrownBy(() -> imagesApi.list("my-project").execute());
+    assertThatIOException().isThrownBy(() -> imagesApi.get("my-project", "my-image").execute());
 
     assertThat(registry.timers().count()).isEqualTo(1);
     Timer timer = registry.timers().findFirst().orElseThrow(AssertionError::new);
@@ -127,7 +118,7 @@ public void list_errorMetrics() {
     //               global state) so that we can test for the account tags
     assertThat(timer.id().tags())
         .contains(
-            tag("api", "compute.images.list"),
+            tag("api", "compute.images.get"),
             tag("scope", "global"),
             tag("status", "4xx"),
             tag("success", "false"));

clouddriver-google/src/test/groovy/com/netflix/spinnaker/clouddriver/google/deploy/ops/SetStatefulDiskAtomicOperationUnitSpec.groovy

@@ -16,9 +16,7 @@
 
 package com.netflix.spinnaker.clouddriver.google.deploy.ops
 
-
 import com.google.api.services.compute.model.InstanceGroupManager
-import com.google.api.services.compute.model.Operation
 import com.netflix.spinnaker.clouddriver.data.task.Task
 import com.netflix.spinnaker.clouddriver.data.task.TaskRepository
 import com.netflix.spinnaker.clouddriver.google.compute.FakeGoogleComputeOperationRequest
@@ -73,7 +71,7 @@ class SetStatefulDiskAtomicOperationUnitSpec extends Specification {
       credentials: CREDENTIALS)
     def operation = new SetStatefulDiskAtomicOperation(clusterProvider, computeApiFactory, description)
     def updateOp = new FakeGoogleComputeOperationRequest<>()
-    def getManagerRequest = new FakeGoogleComputeRequest<>(new InstanceGroupManager())
+    def getManagerRequest = FakeGoogleComputeRequest.createWithResponse(new InstanceGroupManager())
     _ * serverGroupManagers.get() >> getManagerRequest
 
     when: