Fix #2672: WaitUntilReady for Service resource throws IllegalArgumentException

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
@@ -35,7 +36,7 @@
     suffixed with the specification version, e.g. `mycrplural.group.example.com-v1.yml`
   - The CRD files are generated in the `target/META-INF/fabric8` directory of your project
 
-_**Note**_: Breaking changes in the API
+#### _**Note**_: Breaking changes in the API
 ##### DSL Changes:
 - `client.settings()` DSL has been removed since PodPreset v1alpha1 API is no longer present in Kubernetes 1.20.x
 - `client.customResourceDefinitions()` has been removed. Use `client.apiextensions().v1beta1().customResourceDefinitions()` instead

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/base/BaseOperation.java

@@ -254,7 +254,7 @@ public T edit(Visitor... visitors) {
     throw new KubernetesClientException("Cannot edit read-only resources");
   }
 
-  @Override 
+  @Override
   public <V> T edit(final Class<V> visitorType, final Visitor<V> visitor) {
     return edit(new TypedVisitor<V>() {
         @Override
@@ -1085,14 +1085,18 @@ public boolean isApiGroup() {
     return apiVersion != null && apiVersion.indexOf('/') > 0;
   }
 
+  public Readiness getReadiness() {
+    return Readiness.getInstance();
+  }
+
   @Override
-  public Boolean isReady() {
-    return Readiness.isReady(get());
+  public final Boolean isReady() {
+    return getReadiness().isReady(get());
   }
 
   @Override
   public T waitUntilReady(long amount, TimeUnit timeUnit) throws InterruptedException {
-    return waitUntilCondition(resource -> Objects.nonNull(resource) && Readiness.isReady(resource), amount, timeUnit);
+    return waitUntilCondition(resource -> Objects.nonNull(resource) && getReadiness().isReady(resource), amount, timeUnit);
   }
 
   @Override

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableImpl.java

@@ -242,9 +242,13 @@ public Watch watch(ListOptions options, Watcher<HasMetadata> watcher) {
     return h.watch(client, config, meta.getMetadata().getNamespace(), meta, options, watcher);
   }
 
+  protected Readiness getReadiness() {
+    return Readiness.getInstance();
+  }
+
   @Override
-  public Boolean isReady() {
-    return Readiness.isReady(get());
+  public final Boolean isReady() {
+    return getReadiness().isReady(get());
   }
 
   @Override

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl.java

@@ -206,7 +206,7 @@ private static void logAsNotReady(Throwable t, HasMetadata meta) {
   @Override
   public Boolean isReady() {
     for (final HasMetadata meta : acceptVisitors(get(), visitors)) {
-      if (!isResourceReady(meta)) {
+      if (!getReadiness().isReady(meta)) {
         return false;
       }
     }
@@ -403,11 +403,11 @@ public Deletable cascading(boolean cascading) {
     return new NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl(client, config, fallbackNamespace, explicitNamespace, fromServer, true, visitors, item, null, null, gracePeriodSeconds, propagationPolicy, cascading, watchRetryInitialBackoffMillis, watchRetryBackoffMultiplier);
   }
 
-  protected boolean isResourceReady(HasMetadata meta) {
-    return Readiness.isReady(meta);
+  protected Readiness getReadiness() {
+    return Readiness.getInstance();
   }
 
-  protected <T> List<HasMetadata> asHasMetadata(T item, Boolean enableProccessing) {
+  protected <T> List<HasMetadata> asHasMetadata(T item, Boolean enableProcessing) {
     List<HasMetadata> result = new ArrayList<>();
     if (item instanceof KubernetesList) {
       result.addAll(((KubernetesList) item).getItems());
@@ -417,7 +417,7 @@ protected <T> List<HasMetadata> asHasMetadata(T item, Boolean enableProccessing)
       result.add((HasMetadata) item);
     }  else if (item instanceof String) {
       try (InputStream is = new ByteArrayInputStream(((String)item).getBytes(StandardCharsets.UTF_8))) {
-        return asHasMetadata(unmarshal(is), enableProccessing);
+        return asHasMetadata(unmarshal(is), enableProcessing);
       } catch (IOException e) {
         throw KubernetesClientException.launderThrowable(e);
       }

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

@@ -35,35 +35,68 @@
 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.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);
+  protected static final String READINESS_APPLICABLE_RESOURCES =
+    "Node, Deployment, ReplicaSet, StatefulSet, Pod, ReplicationController";
 
+  private static Readiness instance;
 
-  public static boolean isReadinessApplicable(Class<? extends HasMetadata> itemClass) {
-    return Deployment.class.isAssignableFrom(itemClass)
-      || io.fabric8.kubernetes.api.model.extensions.Deployment.class.isAssignableFrom(itemClass)
-      || ReplicaSet.class.isAssignableFrom(itemClass)
-      || Pod.class.isAssignableFrom(itemClass)
-      || ReplicationController.class.isAssignableFrom(itemClass)
-      || Endpoints.class.isAssignableFrom(itemClass)
-      || Node.class.isAssignableFrom(itemClass)
-      || StatefulSet.class.isAssignableFrom(itemClass)
-      ;
+  public static Readiness getInstance() {
+    if (instance == null) {
+      synchronized (Readiness.class) {
+        instance = new Readiness();
+      }
+    }
+    return instance;
   }
 
-  public static boolean isReady(HasMetadata item) {
-    if (isReadiableKubernetesResource(item)) {
-      return isKubernetesResourceReady(item);
+  /**
+   * Checks if the provided {@link HasMetadata} is marked as ready by the cluster.
+   *
+   * <p> A "Readiable" resources is a subjective trait for Kubernetes Resources. Many Resources, such as ConfigMaps,
+   * Secrets, etc. become ready as soon as they've been created in the cluster.
+   *
+   * <p> However, other resources such as Pods, Endpoints, Deployments, and controllers in general, only become
+   * ready when their desired state matches their actual state.
+   *
+   * <p> This method returns true for those "Readiable" resources once they are considered ready (even if the resource
+   * exists in the cluster). For "non-Readiable" resources, this method returns true once the resources are created in
+   * the cluster (in addition it logs a warning stating that the given resource is not considered "Readiable").
+   *
+   * @param item resource to be checked for Readiness.
+   * @return true if it's a Readiable Resource and is ready, or it's a non-readiable resource and has been created. false
+   * otherwise.
+   */
+  public boolean isReady(HasMetadata item) {
+    if (isReadinessApplicable(item)) {
+      return isResourceReady(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);
     }
   }
 
-  private static boolean isKubernetesResourceReady(HasMetadata item) {
+  protected boolean isReadinessApplicable(HasMetadata item) {
+    return (item instanceof Deployment ||
+      item instanceof io.fabric8.kubernetes.api.model.extensions.Deployment ||
+      item instanceof ReplicaSet ||
+      item instanceof Pod ||
+      item instanceof ReplicationController ||
+      item instanceof Endpoints ||
+      item instanceof Node ||
+      item instanceof StatefulSet);
+  }
+
+  protected boolean isResourceReady(HasMetadata item) {
     if (item instanceof Deployment) {
       return isDeploymentReady((Deployment) item);
     } else if (item instanceof io.fabric8.kubernetes.api.model.extensions.Deployment) {
@@ -84,6 +117,17 @@ private static boolean isKubernetesResourceReady(HasMetadata item) {
     return false;
   }
 
+  protected String getReadinessResourcesList() {
+    return READINESS_APPLICABLE_RESOURCES;
+  }
+
+  final boolean handleNonReadinessApplicableResource(HasMetadata item) {
+    boolean doesItemExist = Objects.nonNull(item);
+    logger.warn("{} is not a Readiable resource. It needs to be one of [{}]",
+      doesItemExist ? item.getKind() : "Unknown", getReadinessResourcesList());
+    return doesItemExist;
+  }
+
   public static boolean isStatefulSetReady(StatefulSet ss) {
     Utils.checkNotNull(ss, "StatefulSet can't be null.");
     StatefulSetSpec spec = ss.getSpec();
@@ -235,7 +279,7 @@ public static boolean isNodeReady(Node node) {
 
   /**
    * Returns the ready condition of the node.
-   * 
+   *
    * @param node The target node.
    * @return The {@link NodeCondition} or null if not found.
    */
@@ -254,16 +298,6 @@ private static NodeCondition getNodeReadyCondition(Node node) {
     return null;
   }
 
-  protected static boolean isReadiableKubernetesResource(HasMetadata item) {
-    return (item instanceof Deployment ||
-      item instanceof io.fabric8.kubernetes.api.model.extensions.Deployment ||
-    item instanceof ReplicaSet ||
-    item instanceof Pod ||
-    item instanceof ReplicationController ||
-    item instanceof Endpoints ||
-    item instanceof Node ||
-    item instanceof StatefulSet);
-  }
 }
 
 

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

@@ -38,7 +38,7 @@ public ReadinessWatcher(T resource) {
   public void eventReceived(Action action, T resource) {
     switch (action) {
       case MODIFIED:
-        if (Readiness.isReady(resource)) {
+        if (Readiness.getInstance().isReady(resource)) {
           reference.set(resource);
           latch.countDown();
         }

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/KubernetesResourceUtil.java

@@ -356,7 +356,7 @@ public static List<EnvVar> convertMapToEnvVarList(Map<String, String> envVarMap)
    * @return boolean value indicating it's status
    */
   public static boolean isResourceReady(HasMetadata item) {
-    return Readiness.isReady(item);
+    return Readiness.getInstance().isReady(item);
   }
 
   /**

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

@@ -15,54 +15,63 @@
  */
 package io.fabric8.kubernetes.client.internal.readiness;
 
+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;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
-public class ReadinessTest {
+class ReadinessTest {
+
+  private Readiness readiness;
+
+  @BeforeEach
+  void setUp() {
+    readiness = Readiness.getInstance();
+  }
 
   @Test
-  public void testStatefulSetReadinessNoSpecNoStatus() {
+  void testStatefulSetReadinessNoSpecNoStatus() {
     StatefulSet statefulSet = new StatefulSet();
-    assertFalse(Readiness.isReady(statefulSet));
+    assertFalse(readiness.isReady(statefulSet));
     assertFalse(Readiness.isStatefulSetReady(statefulSet));
   }
 
   @Test
-  public void testStatefulSetReadinessNoSpec() {
+  void testStatefulSetReadinessNoSpec() {
     StatefulSetStatus status = new StatefulSetStatus();
 
     StatefulSet statefulSet = new StatefulSet();
     statefulSet.setStatus(status);
 
-    assertFalse(Readiness.isReady(statefulSet));
+    assertFalse(readiness.isReady(statefulSet));
     assertFalse(Readiness.isStatefulSetReady(statefulSet));
 
     status.setReadyReplicas(1);
 
-    assertFalse(Readiness.isReady(statefulSet));
+    assertFalse(readiness.isReady(statefulSet));
     assertFalse(Readiness.isStatefulSetReady(statefulSet));
   }
 
   @Test
-  public void testStatefulSetReadinessNoStatus() {
+  void testStatefulSetReadinessNoStatus() {
     StatefulSetSpec spec = new StatefulSetSpec();
     spec.setReplicas(1);
 
     StatefulSet statefulSet = new StatefulSet();
     statefulSet.setSpec(spec);
 
-    assertFalse(Readiness.isReady(statefulSet));
+    assertFalse(readiness.isReady(statefulSet));
     assertFalse(Readiness.isStatefulSetReady(statefulSet));
 
   }
 
   @Test
-  public void testStatefulSetReadinessNotEnoughReadyReplicas() {
+  void testStatefulSetReadinessNotEnoughReadyReplicas() {
     StatefulSetStatus status = new StatefulSetStatus();
     status.setReadyReplicas(1);
     status.setReplicas(2);
@@ -74,12 +83,12 @@ public void testStatefulSetReadinessNotEnoughReadyReplicas() {
     statefulSet.setStatus(status);
     statefulSet.setSpec(spec);
 
-    assertFalse(Readiness.isReady(statefulSet));
+    assertFalse(readiness.isReady(statefulSet));
     assertFalse(Readiness.isStatefulSetReady(statefulSet));
   }
 
   @Test
-  public void testStatefulSetReadiness() {
+  void testStatefulSetReadiness() {
     StatefulSetStatus status = new StatefulSetStatus();
     status.setReadyReplicas(2);
     status.setReplicas(2);
@@ -91,7 +100,17 @@ public void testStatefulSetReadiness() {
     statefulSet.setStatus(status);
     statefulSet.setSpec(spec);
 
-    assertTrue(Readiness.isReady(statefulSet));
+    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/DeploymentIT.java

@@ -90,7 +90,7 @@ public void waitTest() {
     await().atMost(120, TimeUnit.SECONDS).until(deploymentReady);
     Deployment deploymentOne = client.apps().deployments()
       .inNamespace(session.getNamespace()).withName("deployment-wait").get();
-    assertTrue(Readiness.isDeploymentReady(deploymentOne));
+    assertTrue(Readiness.getInstance().isDeploymentReady(deploymentOne));
   }
 
   @Test

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

@@ -166,7 +166,7 @@ public void evict() throws InterruptedException {
     // cant evict because only one left
     assertFalse(client.pods().inNamespace(session.getNamespace()).withName(pod1.getMetadata().getName()).evict());
     // ensure it really is still up
-    assertTrue(Readiness.isReady(client.pods().inNamespace(session.getNamespace()).withName(pod1.getMetadata().getName()).fromServer().get()));
+    assertTrue(Readiness.getInstance().isReady(client.pods().inNamespace(session.getNamespace()).withName(pod1.getMetadata().getName()).fromServer().get()));
 
     // create another pod to satisfy PDB
     client.pods().inNamespace(session.getNamespace()).createOrReplace(pod3);

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"));