Merge pull request fabric8io#4080 from shawkins/misc2

Minor changes to consider 0 replicas and disable the backwards compatibility interceptor by default

CHANGELOG.md

@@ -48,6 +48,7 @@ Please see the [migration guide](doc/MIGRATION-v6.md)
 * Fix #3936: Kubernetes Mock Server .metadata.generation field is an integer
 * Fix #3957: Lister `onOpen` should be called before marking the connection as open
 * Fix #4022: Reintroduce `Deletable` interface in `NonNamespaceOperation`
+* Fix #4009: updating readiness to consider 0 replicas
 
 #### _**Note**_:
 - `Config#autoConfigure(String context)`: Has been changed to only trigger the autoConfigure method once. Previously, providing a wrong context argument would not be a problem since an initial context-less autoConfigure would have already been invoked to provide a valid initial Config.

README.md

@@ -115,7 +115,7 @@ System properties are preferred over environment variables. The following system
 | `kubernetes.truststore.passphrase` / `KUBERNETES_TRUSTSTORE_PASSPHRASE` | | |
 | `kubernetes.keystore.file` / `KUBERNETES_KEYSTORE_FILE` | | |
 | `kubernetes.keystore.passphrase` / `KUBERNETES_KEYSTORE_PASSPHRASE` | | |
-| `kubernetes.backwardsCompatibilityInterceptor.disable` / `KUBERNETES_BACKWARDSCOMPATIBILITYINTERCEPTOR_DISABLE` | `Disable BackwardsCompatibilityInterceptor`| `false` |
+| `kubernetes.backwardsCompatibilityInterceptor.disable` / `KUBERNETES_BACKWARDSCOMPATIBILITYINTERCEPTOR_DISABLE` | `Disable BackwardsCompatibilityInterceptor`| `true` |
 
 Alternatively you can use the `ConfigBuilder` to create a config object for the Kubernetes client:
 

doc/MIGRATION-v6.md

@@ -1,6 +1,7 @@
 # Migration from 5.x to 6.x
 
 ## Contents:
+- [Backwards Compatibility Interceptor](#backwards-compatibility-interceptor)
 - [Namespace Changes](#namespace-changes)
 - [API/Impl split](#api-impl-split)
 - [Deserialization Resolution](#deserialization-resolution)
@@ -17,6 +18,10 @@
 - [evict Changes](#evict-changes)
 - [Delete Behavior](#delete-behavior)
 
+## Backwards Compatibility Interceptor
+
+kubernetes.backwardsCompatibilityInterceptor.disable now defaults to true, rather than false.  If you need backwards compatibility support, please set this property to false.
+
 ## Namespace Changes
 
 To match the behavior of kubectl the client will now consider any call to inNamespace as the namespace to use regardless of what is on a passed in item.  

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

@@ -132,12 +132,16 @@ public static boolean isStatefulSetReady(StatefulSet ss) {
     StatefulSetSpec spec = ss.getSpec();
     StatefulSetStatus status = ss.getStatus();
 
-    if (status == null || status.getReplicas() == null || status.getReadyReplicas() == null) {
+    //Can be true in testing, so handle it to make test writing easier.
+    if (spec == null || spec.getReplicas() == null) {
       return false;
     }
 
-    //Can be true in testing, so handle it to make test writing easier.
-    if (spec == null || spec.getReplicas() == null) {
+    if (spec.getReplicas() == 0) {
+      return true;
+    }
+
+    if (status == null || status.getReplicas() == null || status.getReadyReplicas() == null) {
       return false;
     }
 
@@ -150,30 +154,40 @@ public static boolean isDeploymentReady(Deployment d) {
     DeploymentSpec spec = d.getSpec();
     DeploymentStatus status = d.getStatus();
 
-    if (status == null || status.getReplicas() == null || status.getAvailableReplicas() == null) {
+    //Can be true in testing, so handle it to make test writing easier.
+    if (spec == null || spec.getReplicas() == null) {
       return false;
     }
 
-    //Can be true in testing, so handle it to make test writing easier.
-    if (spec == null || spec.getReplicas() == null) {
+    if (spec.getReplicas() == 0) {
+      return true;
+    }
+
+    if (status == null || status.getReplicas() == null || status.getAvailableReplicas() == null) {
       return false;
     }
 
-    return spec.getReplicas().intValue() == status.getReplicas() &&
-        spec.getReplicas() <= status.getAvailableReplicas();
+    int replicas = Utils.getNonNullOrElse(spec.getReplicas(), 0);
+
+    return replicas == status.getReplicas() &&
+        replicas <= status.getAvailableReplicas();
   }
 
   public static boolean isExtensionsDeploymentReady(io.fabric8.kubernetes.api.model.extensions.Deployment d) {
     Utils.checkNotNull(d, "Deployment can't be null.");
     io.fabric8.kubernetes.api.model.extensions.DeploymentSpec spec = d.getSpec();
     io.fabric8.kubernetes.api.model.extensions.DeploymentStatus status = d.getStatus();
 
-    if (status == null || status.getReplicas() == null || status.getAvailableReplicas() == null) {
+    //Can be true in testing, so handle it to make test writing easier.
+    if (spec == null || spec.getReplicas() == null) {
       return false;
     }
 
-    //Can be true in testing, so handle it to make test writing easier.
-    if (spec == null || spec.getReplicas() == null) {
+    if (spec.getReplicas() == 0) {
+      return true;
+    }
+
+    if (status == null || status.getReplicas() == null || status.getAvailableReplicas() == null) {
       return false;
     }
 
@@ -186,14 +200,19 @@ public static boolean isReplicaSetReady(ReplicaSet r) {
     ReplicaSetSpec spec = r.getSpec();
     ReplicaSetStatus status = r.getStatus();
 
-    if (status == null || status.getReadyReplicas() == null) {
+    //Can be true in testing, so handle it to make test writing easier.
+    if (spec == null || spec.getReplicas() == null) {
       return false;
     }
 
-    //Can be true in testing, so handle it to make test writing easier.
-    if (spec == null || spec.getReplicas() == null) {
+    if (spec.getReplicas() == 0) {
+      return true;
+    }
+
+    if (status == null || status.getReadyReplicas() == null) {
       return false;
     }
+
     return spec.getReplicas().intValue() == status.getReadyReplicas();
   }
 
@@ -202,12 +221,16 @@ public static boolean isReplicationControllerReady(ReplicationController r) {
     ReplicationControllerSpec spec = r.getSpec();
     ReplicationControllerStatus status = r.getStatus();
 
-    if (status == null || status.getReadyReplicas() == null) {
+    //Can be true in testing, so handle it to make test writing easier.
+    if (spec == null || spec.getReplicas() == null) {
       return false;
     }
 
-    //Can be true in testing, so handle it to make test writing easier.
-    if (spec == null || spec.getReplicas() == null) {
+    if (spec.getReplicas() == 0) {
+      return true;
+    }
+
+    if (status == null || status.getReadyReplicas() == null) {
       return false;
     }
 
@@ -255,7 +278,7 @@ public static boolean isPodSucceeded(Pod pod) {
 
   /**
    * Returns the ready condition of the pod.
-   * 
+   *
    * @param pod The target pod.
    * @return The {@link PodCondition} or null if not found.
    */

kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/utils/HttpClientUtils.java

@@ -56,6 +56,7 @@ private HttpClientUtils() {
   private static Pattern VALID_IPV4_PATTERN = null;
   public static final String ipv4Pattern = "(http:\\/\\/|https:\\/\\/)?(([01]?\\d\\d?|2[0-4]\\d|25[0-5])\\.){3}([01]?\\d\\d?|2[0-4]\\d|25[0-5])(\\/[0-9]\\d|1[0-9]\\d|2[0-9]\\d|3[0-2]\\d)?";
   protected static final String KUBERNETES_BACKWARDS_COMPATIBILITY_INTERCEPTOR_DISABLE = "kubernetes.backwardsCompatibilityInterceptor.disable";
+  protected static final String BACKWARDS_COMPATIBILITY_DISABLE_DEFAULT = "true";
 
   static {
     try {
@@ -130,7 +131,8 @@ public void before(BasicBuilder builder, HttpHeaders headers) {
     interceptors.put(TokenRefreshInterceptor.NAME, new TokenRefreshInterceptor(config, factory));
     // Backwards Compatibility Interceptor
     String shouldDisableBackwardsCompatibilityInterceptor = Utils
-        .getSystemPropertyOrEnvVar(KUBERNETES_BACKWARDS_COMPATIBILITY_INTERCEPTOR_DISABLE, "false");
+        .getSystemPropertyOrEnvVar(KUBERNETES_BACKWARDS_COMPATIBILITY_INTERCEPTOR_DISABLE,
+            BACKWARDS_COMPATIBILITY_DISABLE_DEFAULT);
     if (!Boolean.parseBoolean(shouldDisableBackwardsCompatibilityInterceptor)) {
       interceptors.put(BackwardsCompatibilityInterceptor.NAME, new BackwardsCompatibilityInterceptor());
     }

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

@@ -16,10 +16,11 @@
 package io.fabric8.kubernetes.client.readiness;
 
 import io.fabric8.kubernetes.api.model.ServiceBuilder;
+import io.fabric8.kubernetes.api.model.apps.Deployment;
+import io.fabric8.kubernetes.api.model.apps.DeploymentBuilder;
 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 io.fabric8.kubernetes.client.readiness.Readiness;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
@@ -114,4 +115,11 @@ void testReadinessWithNonNullResource() {
   void testReadinessNullResource() {
     assertFalse(readiness.isReady(null));
   }
+
+  @Test
+  void testDeploymentWithNoReplicas() {
+    Deployment d = new DeploymentBuilder().withNewSpec().withReplicas(0).endSpec().build();
+
+    assertTrue(readiness.isReady(d));
+  }
 }

kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/utils/HttpClientUtilsTest.java

@@ -39,11 +39,10 @@ void testCreateApplicableInterceptors() {
 
     // Then
     assertThat(interceptorList)
-      .isNotNull()
-      .hasSize(4)
-      .hasAtLeastOneElementOfType(BackwardsCompatibilityInterceptor.class)
-      .hasAtLeastOneElementOfType(ImpersonatorInterceptor.class)
-      .hasAtLeastOneElementOfType(TokenRefreshInterceptor.class);
+        .isNotNull()
+        .hasSize(3)
+        .hasAtLeastOneElementOfType(ImpersonatorInterceptor.class)
+        .hasAtLeastOneElementOfType(TokenRefreshInterceptor.class);
   }
 
   @Test
@@ -57,44 +56,45 @@ void testCreateApplicableInterceptorsWithBackwardsCompatibilityDisabled() {
 
     // Then
     assertThat(interceptorList)
-      .isNotNull()
-      .hasSize(3)
-      .noneMatch(i -> i instanceof BackwardsCompatibilityInterceptor)
-      .hasAtLeastOneElementOfType(ImpersonatorInterceptor.class)
-      .hasAtLeastOneElementOfType(TokenRefreshInterceptor.class);
+        .isNotNull()
+        .hasSize(3)
+        .noneMatch(i -> i instanceof BackwardsCompatibilityInterceptor)
+        .hasAtLeastOneElementOfType(ImpersonatorInterceptor.class)
+        .hasAtLeastOneElementOfType(TokenRefreshInterceptor.class);
     System.clearProperty(KUBERNETES_BACKWARDS_COMPATIBILITY_INTERCEPTOR_DISABLE);
   }
 
   @Test
   void getProxyUrl_whenHttpsProxyUrlWithNoPort_shouldReturnValidProxyUrl() {
     // Given
     Config config = new ConfigBuilder()
-      .withMasterUrl("http://localhost")
-      .withHttpProxy("http://192.168.0.1")
-      .build();
+        .withMasterUrl("http://localhost")
+        .withHttpProxy("http://192.168.0.1")
+        .build();
 
     // When
-    final IllegalArgumentException illegalArgumentException = assertThrows(IllegalArgumentException.class, () -> HttpClientUtils.getProxyUrl(config));
+    final IllegalArgumentException illegalArgumentException = assertThrows(IllegalArgumentException.class,
+        () -> HttpClientUtils.getProxyUrl(config));
 
     // Then
     assertThat(illegalArgumentException)
-      .hasMessage("Failure in creating proxy URL. Proxy port is required!");
+        .hasMessage("Failure in creating proxy URL. Proxy port is required!");
   }
 
   @Test
   void getProxyUrl_whenHttpsProxyUrlWithPort_shouldReturnValidProxyUrl() throws MalformedURLException {
     // Given
     Config config = new ConfigBuilder()
-      .withMasterUrl("http://localhost")
-      .withHttpProxy("http://192.168.0.1:3128")
-      .build();
+        .withMasterUrl("http://localhost")
+        .withHttpProxy("http://192.168.0.1:3128")
+        .build();
 
     // When
     URL url = HttpClientUtils.getProxyUrl(config);
 
     // Then
     assertThat(url).isNotNull()
-      .hasPort(3128)
-      .hasHost("192.168.0.1");
+        .hasPort(3128)
+        .hasHost("192.168.0.1");
   }
 }