fix(gce): gracefully handle null port in GCE health checks (#5643)

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

clouddriver-google/src/main/groovy/com/netflix/spinnaker/clouddriver/google/provider/agent/GoogleHealthCheckCachingAgent.groovy

@@ -148,7 +148,7 @@ class GoogleHealthCheckCachingAgent extends AbstractGoogleCachingAgent {
       { HealthCheckList list -> list.getItems() },
       "compute.healthChecks.list", TAG_SCOPE, SCOPE_GLOBAL
     )
-    ret.addAll(healthChecks.collect { toGoogleHealthCheck(it, "global") })
+    ret.addAll(healthChecks.findResults { toGoogleHealthCheck(it, "global") })
     def cachingAgent = this
     credentials.regions.collect { it.name }.each { String region ->
       List<HealthCheck> regionHealthChecks = new PaginatedRequest<HealthCheckList>(cachingAgent) {
@@ -166,7 +166,7 @@ class GoogleHealthCheckCachingAgent extends AbstractGoogleCachingAgent {
           { HealthCheckList list -> list.getItems() },
           "compute.regionHealthChecks.list", TAG_SCOPE, SCOPE_REGIONAL, TAG_REGION, region
         )
-      ret.addAll(regionHealthChecks.collect { toGoogleHealthCheck(it, region) })
+      ret.addAll(regionHealthChecks.findResults { toGoogleHealthCheck(it, region) })
     }
     ret
   }
@@ -186,32 +186,63 @@ class GoogleHealthCheckCachingAgent extends AbstractGoogleCachingAgent {
     // Health checks of kind 'healthCheck' are all nested -- the actual health check is contained
     // in a field inside a wrapper HealthCheck object. The wrapper object specifies the type of nested
     // health check as a string, and the proper field is populated based on the type.
+    Integer port
     switch(hc.getType()) {
       case 'HTTP':
+        port = hc.getHttpHealthCheck().getPort()
+        if (port == null) {
+          log.warn("HTTP health check ${hc.getName()} has a null port, ignoring.")
+          return null
+        }
+
         newHC.healthCheckType = GoogleHealthCheck.HealthCheckType.HTTP
-        newHC.port = hc.getHttpHealthCheck().getPort()
+        newHC.port = port
         newHC.requestPath = hc.getHttpHealthCheck().getRequestPath()
         break
       case 'HTTPS':
+        port = hc.getHttpsHealthCheck().getPort()
+        if (port == null) {
+          log.warn("HTTPS health check ${hc.getName()} has a null port, ignoring.")
+          return null
+        }
+
         newHC.healthCheckType = GoogleHealthCheck.HealthCheckType.HTTPS
-        newHC.port = hc.getHttpsHealthCheck().getPort()
+        newHC.port = port
         newHC.requestPath = hc.getHttpsHealthCheck().getRequestPath()
         break
       case 'TCP':
+        port = hc.getTcpHealthCheck().getPort()
+        if (port == null) {
+          log.warn("TCP health check ${hc.getName()} has a null port, ignoring.")
+          return null
+        }
+
         newHC.healthCheckType = GoogleHealthCheck.HealthCheckType.TCP
-        newHC.port = hc.getTcpHealthCheck().getPort()
+        newHC.port = port
         break
       case 'SSL':
+        port = hc.getSslHealthCheck().getPort()
+        if (port == null) {
+          log.warn("SSL health check ${hc.getName()} has a null port, ignoring.")
+          return null
+        }
+
         newHC.healthCheckType = GoogleHealthCheck.HealthCheckType.SSL
-        newHC.port = hc.getSslHealthCheck().getPort()
+        newHC.port = port
         break
       case 'UDP':
+        port = hc.getUdpHealthCheck().getPort()
+        if (port == null) {
+          log.warn("UDP health check ${hc.getName()} has a null port, ignoring.")
+          return null
+        }
+
         newHC.healthCheckType = GoogleHealthCheck.HealthCheckType.UDP
-        newHC.port = hc.getUdpHealthCheck().getPort()
+        newHC.port = port
         break
       default:
         log.warn("Health check ${hc.getName()} has unknown type ${hc.getType()}.")
-        return
+        return null
         break
     }
     return newHC

clouddriver-google/src/test/groovy/com/netflix/spinnaker/clouddriver/google/provider/agent/GoogleHealthCheckCachingAgentTest.groovy

@@ -0,0 +1,109 @@
+/*
+ * Copyright 2019 Google, LLC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.netflix.spinnaker.clouddriver.google.provider.agent
+
+import com.netflix.spinnaker.clouddriver.google.model.GoogleHealthCheck
+
+import static org.assertj.core.api.Assertions.assertThat
+
+import com.fasterxml.jackson.databind.ObjectMapper
+import com.google.api.services.compute.Compute
+import com.google.api.services.compute.model.*
+import com.google.common.collect.ImmutableList
+import com.google.common.collect.ImmutableMap
+import com.netflix.spectator.api.DefaultRegistry
+import com.netflix.spinnaker.clouddriver.google.security.GoogleNamedAccountCredentials
+import org.junit.jupiter.api.BeforeEach
+import org.junit.jupiter.api.Test
+import org.junit.platform.runner.JUnitPlatform
+import org.junit.runner.RunWith
+
+@RunWith(JUnitPlatform.class)
+class GoogleHealthCheckCachingAgentTest {
+
+  private static final String ACCOUNT_NAME = "partypups"
+  private static final String PROJECT = "myproject"
+  private static final String REGION = "myregion"
+  private static final String REGION_URL = "http://compute/regions/" + REGION
+  private static final String ZONE = REGION + "-myzone"
+  private static final String ZONE_URL = "http://compute/zones/" + ZONE
+
+  private ObjectMapper objectMapper
+  private GoogleHealthCheckCachingAgent healthCheckAgent
+
+  @BeforeEach
+  void createTestObjects() {
+    objectMapper = new ObjectMapper()
+
+    Compute compute = new StubComputeFactory().create()
+    GoogleNamedAccountCredentials credentials =
+      new GoogleNamedAccountCredentials.Builder()
+        .project(PROJECT)
+        .name(ACCOUNT_NAME)
+        .compute(compute)
+        .regionToZonesMap(ImmutableMap.of(REGION, ImmutableList.of(ZONE)))
+        .build()
+    healthCheckAgent = new GoogleHealthCheckCachingAgent(
+      "app-name",
+      credentials,
+      objectMapper,
+      new DefaultRegistry(),
+    )
+  }
+
+  private static HealthCheck buildBaseHealthCheck(String name, String region) {
+    HealthCheck hc = new HealthCheck()
+    hc.setName(name)
+    hc.setSelfLink("http://selflink")
+    hc.setRegion(region)
+    hc.setCheckIntervalSec(60)
+    hc.setTimeoutSec(10)
+    hc.setHealthyThreshold(1)
+    hc.setUnhealthyThreshold(3)
+    return hc
+  }
+
+  @Test
+  void createsValidHttpHealthCheck() {
+    HTTPHealthCheck httpHealthCheck = new HTTPHealthCheck()
+    httpHealthCheck.setPort(1234)
+    httpHealthCheck.setRequestPath("/healthz")
+
+    HealthCheck hc = buildBaseHealthCheck("valid", REGION)
+    hc.setHttpHealthCheck(httpHealthCheck)
+    hc.setType("HTTP")
+
+    GoogleHealthCheck ghc = healthCheckAgent.toGoogleHealthCheck(hc, REGION)
+    assertThat(ghc.getPort()).isEqualTo(1234)
+    assertThat(ghc.getRegion()).isEqualTo(REGION)
+    assertThat(ghc.getRequestPath()).isEqualTo("/healthz")
+    assertThat(ghc.getHealthCheckType()).isEqualTo(GoogleHealthCheck.HealthCheckType.HTTP)
+  }
+
+  @Test
+  void handlesHttpHealthCheckWithoutPort() {
+    HTTPHealthCheck httpHealthCheck = new HTTPHealthCheck()
+    httpHealthCheck.setRequestPath("/healthz")
+
+    HealthCheck hc = buildBaseHealthCheck("no-port", REGION)
+    hc.setHttpHealthCheck(httpHealthCheck)
+    hc.setType("HTTP")
+
+    GoogleHealthCheck ghc = healthCheckAgent.toGoogleHealthCheck(hc, REGION)
+    assertThat(ghc).isNull()
+  }
+}