From 3a5d76f86ef20cb7278b7dc1e4e19ff4dc4319c4 Mon Sep 17 00:00:00 2001 From: Rohan Kumar Date: Mon, 8 Feb 2021 15:55:57 +0530 Subject: [PATCH] Fix #2672: WaitUntilReady for Service resource throws IllegalArgumentException 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 | 1 + .../client/internal/readiness/Readiness.java | 30 +++++++++++- .../internal/readiness/ReadinessTest.java | 12 +++++ .../java/io/fabric8/kubernetes/ServiceIT.java | 17 ++++++- .../src/test/resources/service-it.yml | 29 ++++++++++++ .../kubernetes/client/mock/ServiceTest.java | 41 ++++++++++++++--- .../readiness/OpenShiftReadiness.java | 12 ++++- .../readiness/OpenShiftReadinessTest.java | 46 +++++++++++++++++++ 8 files changed, 178 insertions(+), 10 deletions(-) create mode 100644 openshift-client/src/test/java/io/fabric8/openshift/client/internal/readiness/OpenShiftReadinessTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 53a03f065aa..a9770c1aca4 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/internal/readiness/Readiness.java b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/internal/readiness/Readiness.java index 4a271e0310b..347c97f4d24 100644 --- a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/internal/readiness/Readiness.java +++ b/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 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 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 getReadinessApplicableResourcesList() { + List 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); diff --git a/kubernetes-client/src/test/java/io/fabric8/kubernetes/client/internal/readiness/ReadinessTest.java b/kubernetes-client/src/test/java/io/fabric8/kubernetes/client/internal/readiness/ReadinessTest.java index f28de336511..18914a9f6d7 100644 --- a/kubernetes-client/src/test/java/io/fabric8/kubernetes/client/internal/readiness/ReadinessTest.java +++ b/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)); + } } diff --git a/kubernetes-itests/src/test/java/io/fabric8/kubernetes/ServiceIT.java b/kubernetes-itests/src/test/java/io/fabric8/kubernetes/ServiceIT.java index 963d609d5a1..5828372a978 100644 --- a/kubernetes-itests/src/test/java/io/fabric8/kubernetes/ServiceIT.java +++ b/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")); diff --git a/kubernetes-itests/src/test/resources/service-it.yml b/kubernetes-itests/src/test/resources/service-it.yml index a26c34ffd8f..75fb182ad65 100644 --- a/kubernetes-itests/src/test/resources/service-it.yml +++ b/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 diff --git a/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ServiceTest.java b/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ServiceTest.java index 5e50e12f79e..d15d4ea923c 100644 --- a/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ServiceTest.java +++ b/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,7 +174,6 @@ 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"); @@ -179,4 +181,31 @@ void testGetUrlFromClusterIPService() { // 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); + } } diff --git a/openshift-client/src/main/java/io/fabric8/openshift/client/internal/readiness/OpenShiftReadiness.java b/openshift-client/src/main/java/io/fabric8/openshift/client/internal/readiness/OpenShiftReadiness.java index 3e1b58b65f6..aab412cb583 100644 --- a/openshift-client/src/main/java/io/fabric8/openshift/client/internal/readiness/OpenShiftReadiness.java +++ b/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 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 getOpenShiftReadinessApplicableResourcesList() { + List openShiftReadinessApplicableResources = new ArrayList<>(Readiness.getReadinessApplicableResourcesList()); + openShiftReadinessApplicableResources.add("DeploymentConfig"); + + return openShiftReadinessApplicableResources; + } } diff --git a/openshift-client/src/test/java/io/fabric8/openshift/client/internal/readiness/OpenShiftReadinessTest.java b/openshift-client/src/test/java/io/fabric8/openshift/client/internal/readiness/OpenShiftReadinessTest.java new file mode 100644 index 00000000000..f3ff3c35881 --- /dev/null +++ b/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)); + } +}