Fix #2672: WaitUntilReady for Service resource throws IllegalArgument…

…Exception

Refactor Readiness.isReady to log a warning for Non Readiness Applicable
resources instead of throwing IllegalArgumentException.

For Readiness check, these kind of resources will only be checked with
Objects.nonNull since they become ready as soon as they get created

CHANGELOG.md

@@ -5,6 +5,7 @@
 #### Bugs
 * Fix #2748: Pass custom headers in kubernetes-client to watch api by modify WatchConnectionManager
 * Fix #2745: Filtering Operations can't configure PropagationPolicy
+* Fix #2672: WaitUntilReady for Service resource throws IllegalArgumentException
 
 #### Improvements
 * Fix #2717: Remove edit() methods from RawCustomResourceOperationsImpl taking InputStream arguments

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/internal/readiness/Readiness.java

@@ -35,13 +35,19 @@
 import io.fabric8.kubernetes.api.model.apps.StatefulSetSpec;
 import io.fabric8.kubernetes.api.model.apps.StatefulSetStatus;
 import io.fabric8.kubernetes.client.utils.Utils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
 
 public class Readiness {
 
   private static final String POD_READY = "Ready";
   private static final String NODE_READY = "Ready";
   private static final String TRUE = "True";
-
+  private static final Logger logger = LoggerFactory.getLogger(Readiness.class);
 
   public static boolean isReadinessApplicable(Class<? extends HasMetadata> itemClass) {
     return Deployment.class.isAssignableFrom(itemClass)
@@ -59,10 +65,30 @@ public static boolean isReady(HasMetadata item) {
     if (isReadiableKubernetesResource(item)) {
       return isKubernetesResourceReady(item);
     } else {
-      throw new IllegalArgumentException("Item needs to be one of [Node, Deployment, ReplicaSet, StatefulSet, Pod, ReplicationController], but was: [" + (item != null ? item.getKind() : "Unknown (null)") + "]");
+      return handleNonReadinessApplicableResource(item, getReadinessApplicableResourcesList());
     }
   }
 
+  public static boolean handleNonReadinessApplicableResource(HasMetadata item, List<String> readinessApplicableResources) {
+    boolean doesItemExist = Objects.nonNull(item);
+    String readinessApplicableResourcesStr = String.join(",", readinessApplicableResources);
+    logger.warn("{} is not a Readiableresource. It needs to be one of [{}]",
+      doesItemExist ? item.getKind() : "Unknown", readinessApplicableResourcesStr);
+    return doesItemExist;
+  }
+
+  public static List<String> getReadinessApplicableResourcesList() {
+    List<String> readinessApplicableResources = new ArrayList<>();
+    readinessApplicableResources.add("Node");
+    readinessApplicableResources.add("Deployment");
+    readinessApplicableResources.add("ReplicaSet");
+    readinessApplicableResources.add("StatefulSet");
+    readinessApplicableResources.add("Pod");
+    readinessApplicableResources.add("ReplicationController");
+
+    return readinessApplicableResources;
+  }
+
   private static boolean isKubernetesResourceReady(HasMetadata item) {
     if (item instanceof Deployment) {
       return isDeploymentReady((Deployment) item);

kubernetes-client/src/test/java/io/fabric8/kubernetes/client/internal/readiness/ReadinessTest.java

@@ -15,6 +15,8 @@
  */
 package io.fabric8.kubernetes.client.internal.readiness;
 
+import io.fabric8.kubernetes.api.model.Service;
+import io.fabric8.kubernetes.api.model.ServiceBuilder;
 import io.fabric8.kubernetes.api.model.apps.StatefulSet;
 import io.fabric8.kubernetes.api.model.apps.StatefulSetSpec;
 import io.fabric8.kubernetes.api.model.apps.StatefulSetStatus;
@@ -94,4 +96,14 @@ public void testStatefulSetReadiness() {
     assertTrue(Readiness.isReady(statefulSet));
     assertTrue(Readiness.isStatefulSetReady(statefulSet));
   }
+
+  @Test
+  void testReadinessWithNonNullResource() {
+    assertTrue(Readiness.isReady(new ServiceBuilder().withNewMetadata().withName("svc1").endMetadata().build()));
+  }
+
+  @Test
+  void testReadinessNullResource() {
+    assertFalse(Readiness.isReady(null));
+  }
 }

kubernetes-itests/src/test/java/io/fabric8/kubernetes/ServiceIT.java

@@ -26,7 +26,6 @@
 import org.arquillian.cube.kubernetes.api.Session;
 import org.arquillian.cube.kubernetes.impl.requirement.RequiresKubernetes;
 import org.arquillian.cube.requirement.ArquillianConditionalRunner;
-import org.assertj.core.internal.bytebuddy.build.ToStringPlugin;
 import org.jboss.arquillian.test.api.ArquillianResource;
 import org.junit.AfterClass;
 import org.junit.BeforeClass;
@@ -225,6 +224,22 @@ public void testExternalNameServiceCreateOrReplace() {
     assertEquals("his.database.example.com", service.getSpec().getExternalName());
   }
 
+  @Test
+  public void waitUntilReady() throws InterruptedException {
+    // Given
+    String svcName = "service-wait-until-ready";
+
+    // When
+    Service service = client.services().inNamespace(session.getNamespace())
+      .withName(svcName)
+      .waitUntilReady(10, TimeUnit.SECONDS);
+
+    // Then
+    assertNotNull(service);
+    assertEquals(svcName, service.getMetadata().getName());
+    assertTrue(client.pods().inNamespace(session.getNamespace()).withName(svcName).delete());
+  }
+
   @AfterClass
   public static void cleanup() {
     ClusterEntity.remove(ServiceIT.class.getResourceAsStream("/service-it.yml"));

kubernetes-itests/src/test/resources/service-it.yml

@@ -95,3 +95,32 @@ spec:
       protocol: TCP
       port: 443
       targetPort: 9377
+---
+apiVersion: v1
+kind: Service
+metadata:
+  name: service-wait-until-ready
+spec:
+  selector:
+    app: service-wait-until-ready
+  ports:
+    - name: http
+      protocol: TCP
+      port: 8080
+      targetPort: 8080
+---
+apiVersion: v1
+kind: Pod
+metadata:
+  name: service-wait-until-ready
+  labels:
+    app: service-wait-until-ready
+spec:
+  containers:
+    - name: busybox
+      image: busybox
+      command: ["sleep", "36000"]
+      ports:
+        - containerPort: 8080
+          name: http
+          protocol: TCP

kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ServiceTest.java

@@ -15,6 +15,10 @@
  */
 package io.fabric8.kubernetes.client.mock;
 
+import io.fabric8.kubernetes.api.model.EndpointAddressBuilder;
+import io.fabric8.kubernetes.api.model.EndpointPortBuilder;
+import io.fabric8.kubernetes.api.model.EndpointSubsetBuilder;
+import io.fabric8.kubernetes.api.model.EndpointsBuilder;
 import io.fabric8.kubernetes.api.model.IntOrString;
 import io.fabric8.kubernetes.api.model.Service;
 import io.fabric8.kubernetes.api.model.ServiceBuilder;
@@ -29,6 +33,7 @@
 
 import java.net.HttpURLConnection;
 import java.util.Collections;
+import java.util.concurrent.TimeUnit;
 
 import static junit.framework.TestCase.assertNotNull;
 import static junit.framework.TestCase.assertTrue;
@@ -41,6 +46,8 @@ class ServiceTest {
 
   Service service;
 
+  private KubernetesClient client;
+
   @BeforeEach
   void prepareService() {
     service = new ServiceBuilder()
@@ -57,11 +64,11 @@ void prepareService() {
       .addToSelector("deploymentconfig", "httpbin")
       .endSpec()
       .build();
+    client = server.getClient();
   }
 
   @Test
   void testLoad() {
-    KubernetesClient client = server.getClient();
     Service svc = client.services().load(getClass().getResourceAsStream("/test-service.yml")).get();
     assertNotNull(svc);
     assertEquals("httpbin", svc.getMetadata().getName());
@@ -74,7 +81,6 @@ void testCreate() {
       .andReturn(200, service)
       .once();
 
-    KubernetesClient client = server.getClient();
     Service responseSvc = client.services().inNamespace("test").create(service);
     assertNotNull(responseSvc);
     assertEquals("httpbin", responseSvc.getMetadata().getName());
@@ -104,7 +110,6 @@ void testReplace() throws InterruptedException {
       .andReturn(HttpURLConnection.HTTP_OK, serviceUpdated)
       .once();
 
-    KubernetesClient client = server.getClient();
     Service responseSvc = client.services().inNamespace("test").createOrReplace(serviceUpdated);
     assertNotNull(responseSvc);
     assertEquals("httpbin", responseSvc.getMetadata().getName());
@@ -122,7 +127,6 @@ void testDelete() {
       .andReturn(200, service)
       .once();
 
-    KubernetesClient client = server.getClient();
     boolean isDeleted = client.services().inNamespace("test").withName("httpbin").delete();
     assertTrue(isDeleted);
   }
@@ -142,7 +146,6 @@ void testUpdate() {
       .andReturn(200, serviceFromServer)
       .once();
 
-    KubernetesClient client = server.getClient();
     Service responseFromServer = client.services().inNamespace("test").withName("httpbin").edit(s -> new ServiceBuilder(s)
       .editOrNewMetadata().addToLabels("foo", "bar").endMetadata()
       .build());
@@ -171,12 +174,38 @@ void testGetUrlFromClusterIPService() {
       .andReturn(HttpURLConnection.HTTP_OK, service).always();
     server.expect().get().withPath("/apis/extensions/v1beta1/namespaces/ns1/ingresses")
       .andReturn(HttpURLConnection.HTTP_OK, new IngressListBuilder().build()).always();
-    KubernetesClient client = server.getClient();
 
     // When
     String url = client.services().inNamespace("ns1").withName("svc1").getURL("http");
 
     // Then
     assertEquals("tcp://187.30.14.32:8080", url);
   }
+
+  @Test
+  void testWaitUntilReady() throws InterruptedException {
+    // Given
+    Service svc1 = new ServiceBuilder().withNewMetadata().withName("svc1").endMetadata().build();
+    server.expect().get().withPath("/api/v1/namespaces/ns1/endpoints/svc1")
+      .andReturn(HttpURLConnection.HTTP_OK, new EndpointsBuilder()
+        .withNewMetadata().withName("svc1").endMetadata()
+        .addToSubsets(new EndpointSubsetBuilder()
+          .addToAddresses(new EndpointAddressBuilder().withIp("192.168.64.13").build())
+          .addToPorts(new EndpointPortBuilder().withPort(8443).build())
+          .build())
+        .build())
+      .once();
+    server.expect().get().withPath("/api/v1/namespaces/ns1/services/svc1")
+      .andReturn(HttpURLConnection.HTTP_OK, svc1)
+      .times(2);
+
+    // When
+    Service service = client.services()
+      .inNamespace("ns1")
+      .withName("svc1")
+      .waitUntilReady(60, TimeUnit.SECONDS);
+
+    // Then
+    assertNotNull(service);
+  }
 }

openshift-client/src/main/java/io/fabric8/openshift/client/internal/readiness/OpenShiftReadiness.java

@@ -22,6 +22,9 @@
 import io.fabric8.openshift.api.model.DeploymentConfigSpec;
 import io.fabric8.openshift.api.model.DeploymentConfigStatus;
 
+import java.util.ArrayList;
+import java.util.List;
+
 public class OpenShiftReadiness extends Readiness {
   public static boolean isReadinessApplicable(Class<? extends HasMetadata> itemClass) {
     return Readiness.isReadinessApplicable(itemClass)
@@ -35,7 +38,7 @@ public static boolean isReady(HasMetadata item) {
     } else if (item instanceof DeploymentConfig) {
       return isDeploymentConfigReady((DeploymentConfig) item);
     } else {
-      throw new IllegalArgumentException("Item needs to be one of [Node, Deployment, ReplicaSet, StatefulSet, Pod, DeploymentConfig, ReplicationController], but was: [" + (item != null ? item.getKind() : "Unknown (null)") + "]");
+      return handleNonReadinessApplicableResource(item, getOpenShiftReadinessApplicableResourcesList());
     }
   }
 
@@ -56,4 +59,11 @@ public static boolean isDeploymentConfigReady(DeploymentConfig d) {
     return spec.getReplicas().intValue() == status.getReplicas() &&
       spec.getReplicas() <= status.getAvailableReplicas();
   }
+
+  private static List<String> getOpenShiftReadinessApplicableResourcesList() {
+    List<String> openShiftReadinessApplicableResources = new ArrayList<>(Readiness.getReadinessApplicableResourcesList());
+    openShiftReadinessApplicableResources.add("DeploymentConfig");
+
+    return openShiftReadinessApplicableResources;
+  }
 }

openshift-client/src/test/java/io/fabric8/openshift/client/internal/readiness/OpenShiftReadinessTest.java

@@ -0,0 +1,46 @@
+/**
+ * Copyright (C) 2015 Red Hat, Inc.
+ *
+ * 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 io.fabric8.openshift.client.internal.readiness;
+
+import io.fabric8.openshift.api.model.DeploymentConfig;
+import io.fabric8.openshift.api.model.DeploymentConfigBuilder;
+import io.fabric8.openshift.api.model.ImageStreamBuilder;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+class OpenShiftReadinessTest {
+  @Test
+  void testOpenShiftReadinessWithDeploymentConfig() {
+    DeploymentConfig dc = new DeploymentConfigBuilder()
+      .withNewSpec().withReplicas(2).endSpec()
+      .withNewStatus().withAvailableReplicas(2).withReplicas(2).endStatus()
+      .build();
+
+    assertTrue(OpenShiftReadiness.isReady(dc));
+  }
+
+  @Test
+  void testOpenShiftReadinessWithNonReadinessApplicableResource() {
+    assertTrue(OpenShiftReadiness.isReady(new ImageStreamBuilder().withNewMetadata().withName("is1").endMetadata().build()));
+  }
+
+  @Test
+  void testOpenShiftReadinessWithNullResource() {
+    assertFalse(OpenShiftReadiness.isReady(null));
+  }
+}