From b9afedbea1e1c85c97c97f1a7e41c17291ea7105 Mon Sep 17 00:00:00 2001 From: Suresh Sigera Date: Thu, 10 Sep 2026 23:07:15 -0400 Subject: [PATCH 1/2] fix: don't default null replicas/readyReplicas in Readiness checks isReplicaSetReady and isReplicationControllerReady silently treated a null spec.replicas as 1 and null status.readyReplicas as 0, which can misreport readiness. isStatefulSetReady already had this exact issue fixed by a maintainer in a prior review - this applies the same fix to the two remaining copy-pasted methods, plus regression tests. --- .../io/kubernetes/client/util/Readiness.java | 8 +-- .../kubernetes/client/util/ReadinessTest.java | 50 ++++++++++++++++++- 2 files changed, 53 insertions(+), 5 deletions(-) diff --git a/util/src/main/java/io/kubernetes/client/util/Readiness.java b/util/src/main/java/io/kubernetes/client/util/Readiness.java index 715320574e..90191f7cc4 100644 --- a/util/src/main/java/io/kubernetes/client/util/Readiness.java +++ b/util/src/main/java/io/kubernetes/client/util/Readiness.java @@ -206,10 +206,10 @@ public static boolean isReplicaSetReady(V1ReplicaSet replicaSet) { Integer readyReplicas = status.getReadyReplicas(); if (replicas == null) { - replicas = 1; + return false; } if (readyReplicas == null) { - readyReplicas = 0; + return false; } return replicas.equals(readyReplicas); @@ -380,10 +380,10 @@ public static boolean isReplicationControllerReady(V1ReplicationController repli Integer readyReplicas = status.getReadyReplicas(); if (replicas == null) { - replicas = 1; + return false; } if (readyReplicas == null) { - readyReplicas = 0; + return false; } return replicas.equals(readyReplicas); diff --git a/util/src/test/java/io/kubernetes/client/util/ReadinessTest.java b/util/src/test/java/io/kubernetes/client/util/ReadinessTest.java index e1683f0c44..f47a1709ac 100644 --- a/util/src/test/java/io/kubernetes/client/util/ReadinessTest.java +++ b/util/src/test/java/io/kubernetes/client/util/ReadinessTest.java @@ -222,6 +222,30 @@ void isReplicaSetReady_notAllReplicasReady_returnsFalse() { assertThat(Readiness.isReplicaSetReady(replicaSet)).isFalse(); } + @Test + void isReplicaSetReady_nullSpecReplicas_returnsFalse() { + // Regression test: a null spec.replicas must not be silently treated as "1". + V1ReplicaSet replicaSet = new V1ReplicaSet() + .metadata(new V1ObjectMeta().name("test")) + .spec(new V1ReplicaSetSpec()) + .status(new V1ReplicaSetStatus().readyReplicas(1)); + assertThat(Readiness.isReplicaSetReady(replicaSet)).isFalse(); + } + + @Test + void isReplicaSetReady_nullReadyReplicas_returnsFalse() { + // Regression test: a null status.readyReplicas must not be silently treated as "0". + // Uses spec.replicas(0) specifically: under the old buggy code, a null readyReplicas + // defaulted to 0, so 0.equals(0) would wrongly report "ready" here. A non-zero + // replicas value wouldn't actually catch that bug, since it wouldn't match the + // default of 0 either way. + V1ReplicaSet replicaSet = new V1ReplicaSet() + .metadata(new V1ObjectMeta().name("test")) + .spec(new V1ReplicaSetSpec().replicas(0)) + .status(new V1ReplicaSetStatus()); + assertThat(Readiness.isReplicaSetReady(replicaSet)).isFalse(); + } + // ========== DaemonSet Tests ========== @Test @@ -337,6 +361,30 @@ void isReplicationControllerReady_allReplicasReady_returnsTrue() { assertThat(Readiness.isReplicationControllerReady(rc)).isTrue(); } + @Test + void isReplicationControllerReady_nullSpecReplicas_returnsFalse() { + // Regression test: a null spec.replicas must not be silently treated as "1". + V1ReplicationController rc = new V1ReplicationController() + .metadata(new V1ObjectMeta().name("test")) + .spec(new io.kubernetes.client.openapi.models.V1ReplicationControllerSpec()) + .status(new V1ReplicationControllerStatus().readyReplicas(1)); + assertThat(Readiness.isReplicationControllerReady(rc)).isFalse(); + } + + @Test + void isReplicationControllerReady_nullReadyReplicas_returnsFalse() { + // Regression test: a null status.readyReplicas must not be silently treated as "0". + // Uses spec.replicas(0) specifically: under the old buggy code, a null readyReplicas + // defaulted to 0, so 0.equals(0) would wrongly report "ready" here. A non-zero + // replicas value wouldn't actually catch that bug, since it wouldn't match the + // default of 0 either way. + V1ReplicationController rc = new V1ReplicationController() + .metadata(new V1ObjectMeta().name("test")) + .spec(new io.kubernetes.client.openapi.models.V1ReplicationControllerSpec().replicas(0)) + .status(new V1ReplicationControllerStatus()); + assertThat(Readiness.isReplicationControllerReady(rc)).isFalse(); + } + // ========== PersistentVolumeClaim Tests ========== @Test @@ -447,4 +495,4 @@ void isReady_delegatesToCorrectMethod_forJob() { .status("True")))); assertThat(Readiness.isReady(job)).isTrue(); } -} +} \ No newline at end of file From 0c31c258009cf4a30320ec60d0fbeed6e11a6771 Mon Sep 17 00:00:00 2001 From: Suresh Sigera Date: Thu, 17 Sep 2026 01:01:03 -0400 Subject: [PATCH 2/2] fix: treat null readyReplicas as 0, matching client-go semantics readyReplicas is omitempty on a plain int in the real API, so it's omitted from JSON whenever it's genuinely 0 - not just when unreported. Treating null as 0 (rather than 'not ready') matches client-go's own WaitForReadyReplicaSet check and fixes a case flagged in review where a zero-replica ReplicaSet was incorrectly reported not-ready. --- .../io/kubernetes/client/util/Readiness.java | 20 ++++---- .../kubernetes/client/util/ReadinessTest.java | 46 +++++++++++++------ 2 files changed, 45 insertions(+), 21 deletions(-) diff --git a/util/src/main/java/io/kubernetes/client/util/Readiness.java b/util/src/main/java/io/kubernetes/client/util/Readiness.java index 90191f7cc4..ec8eafa476 100644 --- a/util/src/main/java/io/kubernetes/client/util/Readiness.java +++ b/util/src/main/java/io/kubernetes/client/util/Readiness.java @@ -208,11 +208,13 @@ public static boolean isReplicaSetReady(V1ReplicaSet replicaSet) { if (replicas == null) { return false; } - if (readyReplicas == null) { - return false; - } + // readyReplicas is `omitempty` in the real API (a plain int, not a pointer), so the + // API server omits it from the JSON whenever it's genuinely 0 - not just when it + // hasn't been reported yet. A null here means "zero", matching client-go's own + // comparison (*(rs.Spec.Replicas) == rs.Status.ReadyReplicas), not "unknown". + int effectiveReadyReplicas = readyReplicas == null ? 0 : readyReplicas; - return replicas.equals(readyReplicas); + return replicas.equals(effectiveReadyReplicas); } /** @@ -382,11 +384,13 @@ public static boolean isReplicationControllerReady(V1ReplicationController repli if (replicas == null) { return false; } - if (readyReplicas == null) { - return false; - } + // readyReplicas is `omitempty` in the real API (a plain int, not a pointer), so the + // API server omits it from the JSON whenever it's genuinely 0 - not just when it + // hasn't been reported yet. A null here means "zero", matching client-go's own + // semantics, not "unknown". + int effectiveReadyReplicas = readyReplicas == null ? 0 : readyReplicas; - return replicas.equals(readyReplicas); + return replicas.equals(effectiveReadyReplicas); } /** diff --git a/util/src/test/java/io/kubernetes/client/util/ReadinessTest.java b/util/src/test/java/io/kubernetes/client/util/ReadinessTest.java index f47a1709ac..6d7ba7f666 100644 --- a/util/src/test/java/io/kubernetes/client/util/ReadinessTest.java +++ b/util/src/test/java/io/kubernetes/client/util/ReadinessTest.java @@ -233,16 +233,27 @@ void isReplicaSetReady_nullSpecReplicas_returnsFalse() { } @Test - void isReplicaSetReady_nullReadyReplicas_returnsFalse() { - // Regression test: a null status.readyReplicas must not be silently treated as "0". - // Uses spec.replicas(0) specifically: under the old buggy code, a null readyReplicas - // defaulted to 0, so 0.equals(0) would wrongly report "ready" here. A non-zero - // replicas value wouldn't actually catch that bug, since it wouldn't match the - // default of 0 either way. + void isReplicaSetReady_zeroReplicasWithNullReadyReplicas_returnsTrue() { + // readyReplicas is `omitempty` on a plain int in the real API, so the API server + // omits it from the JSON whenever it's genuinely 0 - not just when it hasn't been + // reported yet. A null readyReplicas here should be treated as 0, matching + // client-go's own WaitForReadyReplicaSet check (spec.replicas == status.readyReplicas), + // which considers a zero-replica ReplicaSet ready once its status is all-zero. V1ReplicaSet replicaSet = new V1ReplicaSet() .metadata(new V1ObjectMeta().name("test")) .spec(new V1ReplicaSetSpec().replicas(0)) .status(new V1ReplicaSetStatus()); + assertThat(Readiness.isReplicaSetReady(replicaSet)).isTrue(); + } + + @Test + void isReplicaSetReady_nonZeroReplicasWithNullReadyReplicas_returnsFalse() { + // A null readyReplicas is treated as 0 (see above), so a non-zero desired replica + // count still correctly reports not-ready when readyReplicas is null. + V1ReplicaSet replicaSet = new V1ReplicaSet() + .metadata(new V1ObjectMeta().name("test")) + .spec(new V1ReplicaSetSpec().replicas(3)) + .status(new V1ReplicaSetStatus()); assertThat(Readiness.isReplicaSetReady(replicaSet)).isFalse(); } @@ -372,16 +383,25 @@ void isReplicationControllerReady_nullSpecReplicas_returnsFalse() { } @Test - void isReplicationControllerReady_nullReadyReplicas_returnsFalse() { - // Regression test: a null status.readyReplicas must not be silently treated as "0". - // Uses spec.replicas(0) specifically: under the old buggy code, a null readyReplicas - // defaulted to 0, so 0.equals(0) would wrongly report "ready" here. A non-zero - // replicas value wouldn't actually catch that bug, since it wouldn't match the - // default of 0 either way. + void isReplicationControllerReady_zeroReplicasWithNullReadyReplicas_returnsTrue() { + // readyReplicas is `omitempty` on a plain int in the real API, so it's omitted from + // the JSON whenever it's genuinely 0, not just when unreported. A null readyReplicas + // should be treated as 0, matching client-go's semantics for a zero-replica resource. V1ReplicationController rc = new V1ReplicationController() .metadata(new V1ObjectMeta().name("test")) .spec(new io.kubernetes.client.openapi.models.V1ReplicationControllerSpec().replicas(0)) .status(new V1ReplicationControllerStatus()); + assertThat(Readiness.isReplicationControllerReady(rc)).isTrue(); + } + + @Test + void isReplicationControllerReady_nonZeroReplicasWithNullReadyReplicas_returnsFalse() { + // A null readyReplicas is treated as 0 (see above), so a non-zero desired replica + // count still correctly reports not-ready when readyReplicas is null. + V1ReplicationController rc = new V1ReplicationController() + .metadata(new V1ObjectMeta().name("test")) + .spec(new io.kubernetes.client.openapi.models.V1ReplicationControllerSpec().replicas(3)) + .status(new V1ReplicationControllerStatus()); assertThat(Readiness.isReplicationControllerReady(rc)).isFalse(); } @@ -495,4 +515,4 @@ void isReady_delegatesToCorrectMethod_forJob() { .status("True")))); assertThat(Readiness.isReady(job)).isTrue(); } -} \ No newline at end of file +}