From a9caeb1a8be898df13f9ca03cf90398cac0876a2 Mon Sep 17 00:00:00 2001 From: Rohan Kumar Date: Mon, 21 Sep 2020 19:37:24 +0530 Subject: [PATCH] Revert BackwardCompatibilityInterceptor's behavior of changing /apis to /oapi URLs This commit reverts 293ab9d445291de389d438f4a857928722230dba which was added in order to fix #2373. This behavior was causing problems with Strimzi's Kubernetes Client upgrade: https://github.com/strimzi/strimzi-kafka-operator/pull/3553 . On bisecting the failing test, I found this commit as culprit. Handling OpenShift old /oapi requests seem to be handled in OpenShiftOperation where we check `config.isOpenshiftApiGroupsEnabled()` to modify OperationContext. However, #2373 can be fixed by adding apiVersion in OpenShiftOperation. --- .../BackwardsCompatibilityInterceptor.java | 65 +++++-------------- .../fabric8/openshift/DeploymentConfigIT.java | 2 +- .../java/io/fabric8/openshift/TemplateIT.java | 44 +++++++++++++ .../client/server/mock/BuildConfigTest.java | 17 ----- .../server/mock/DeploymentConfigTest.java | 19 +----- .../client/server/mock/TemplateTest.java | 17 ----- .../dsl/internal/OpenShiftOperation.java | 11 +++- 7 files changed, 73 insertions(+), 102 deletions(-) diff --git a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/BackwardsCompatibilityInterceptor.java b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/BackwardsCompatibilityInterceptor.java index a5e1925fd0..db1205ebd4 100644 --- a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/BackwardsCompatibilityInterceptor.java +++ b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/BackwardsCompatibilityInterceptor.java @@ -128,14 +128,14 @@ public int hashCode() { openshiftOAPITransformations.put("imagestreams", new ResourceKey("ImageStream", "imagestreams", "image.openshift.io", "v1")); openshiftOAPITransformations.put("imagestreamtags", new ResourceKey("ImageStream", "imagestreamtags", "image.openshift.io", "v1")); openshiftOAPITransformations.put("securitycontextconstraints", new ResourceKey("SecurityContextConstraints", "securitycontextconstraints", "security.openshift.io", "v1")); -} + } public Response intercept(Chain chain) throws IOException { Request request = chain.request(); Response response = chain.proceed(request); - if (isOpenshiftApiRequest(request)) { - return handleOpenshiftRequests(request, response, chain); - } else if (!response.isSuccessful() && responseCodeToTransformations.containsKey(response.code())) { + if (isDeprecatedOpenshiftOapiRequest(request)) { + return handleOpenshiftOapiRequests(request, response, chain); + } else if (!response.isSuccessful() && responseCodeToTransformations.keySet().contains(response.code())) { String url = request.url().toString(); Matcher matcher = getMatcher(url); ResourceKey key = getKey(matcher); @@ -143,9 +143,9 @@ public Response intercept(Chain chain) throws IOException { if (target != null) { response.close(); // At this point, we know we won't reuse or return the response; so close it to avoid a connection leak. String newUrl = new StringBuilder(url) - .replace(matcher.start(API_VERSION), matcher.end(API_VERSION), target.version) // Order matters: We need to substitute right to left, so that former substitution don't affect the indexes of later. - .replace(matcher.start(API_GROUP), matcher.end(API_GROUP), target.group) - .toString(); + .replace(matcher.start(API_VERSION), matcher.end(API_VERSION), target.version) // Order matters: We need to substitute right to left, so that former substitution don't affect the indexes of later. + .replace(matcher.start(API_GROUP), matcher.end(API_GROUP), target.group) + .toString(); return handleNewRequestAndProceed(request, newUrl, target, chain); } @@ -170,28 +170,21 @@ private static ResourceKey getKey(Matcher m) { return m != null ? new ResourceKey(null, m.group(PATH), m.group(API_GROUP), m.group(API_VERSION)) : null; } - private static Response handleOpenshiftRequests(Request request, Response response, Chain chain) throws IOException{ + private static Response handleOpenshiftOapiRequests(Request request, Response response, Chain chain) throws IOException{ if (!response.isSuccessful() && response.code() == HttpURLConnection.HTTP_NOT_FOUND) { - ResourceKey target = getResourceKeyFromRequest(request); + String requestUrl = request.url().toString(); + // handle case when /oapi is not available + String[] parts = requestUrl.split("/"); + String resourcePath = parts[parts.length - 1]; + ResourceKey target = openshiftOAPITransformations.get(resourcePath); if (target != null) { - String requestUrl = request.url().toString(); - requestUrl = isOpenShift4Request(requestUrl) ? - convertToOpenShiftOapiUrl(requestUrl, target) : - convertToOpenShift4Url(requestUrl, target); + requestUrl = requestUrl.replace("/oapi", "/apis/" + target.getGroup()); return handleNewRequestAndProceed(request, requestUrl, target, chain); } } return response; } - private static String convertToOpenShift4Url(String requestUrl, ResourceKey target) { - return requestUrl.replace("/oapi", "/apis/" + target.getGroup()); - } - - private static String convertToOpenShiftOapiUrl(String requestUrl, ResourceKey target) { - return requestUrl.replace("/apis/" + target.getGroup() + "/" + target.getVersion(), "/oapi/v1"); - } - private static Response handleNewRequestAndProceed(Request request, String newUrl, ResourceKey target, Chain chain) throws IOException { Request.Builder newRequest = request.newBuilder() .url(newUrl); @@ -214,34 +207,10 @@ private static Response handleNewRequestAndProceed(Request request, String newUr return chain.proceed(newRequest.build()); } - private static boolean isOpenshiftApiRequest(Request request) { - if (request != null) { - String requestUrl = request.url().toString(); - return isOpenshift3OapiRequest(requestUrl) || isOpenShift4Request(requestUrl); + private static boolean isDeprecatedOpenshiftOapiRequest(Request request) { + if (request != null && request.url() != null) { + return request.url().toString().contains("oapi"); } return false; } - - private static boolean isOpenShift4Request(String requestUrl) { - return requestUrl.contains(".openshift.io"); - } - - private static boolean isOpenshift3OapiRequest(String requestUrl) { - return requestUrl.contains("oapi"); - } - - private static ResourceKey getResourceKeyFromRequest(Request request) { - String requestUrl = request.url().toString(); - String resourcePath; - String[] parts = requestUrl.split("/"); - if (parts.length > 2) { - if (request.method().equalsIgnoreCase("POST")) { - resourcePath = parts[parts.length - 1]; - } else { - resourcePath = parts[parts.length - 2]; - } - return openshiftOAPITransformations.get(resourcePath); - } - return null; - } } diff --git a/kubernetes-itests/src/test/java/io/fabric8/openshift/DeploymentConfigIT.java b/kubernetes-itests/src/test/java/io/fabric8/openshift/DeploymentConfigIT.java index 41c5b5b015..6f67c332c5 100644 --- a/kubernetes-itests/src/test/java/io/fabric8/openshift/DeploymentConfigIT.java +++ b/kubernetes-itests/src/test/java/io/fabric8/openshift/DeploymentConfigIT.java @@ -91,7 +91,7 @@ public void delete() { @Test public void testWaitUntilReady() throws InterruptedException { - DeploymentConfig deploymentConfig = client.deploymentConfigs().inNamespace(session.getNamespace()).withName("dc-waituntilready").waitUntilReady(2, TimeUnit.MINUTES); + DeploymentConfig deploymentConfig = client.deploymentConfigs().inNamespace(session.getNamespace()).withName("dc-waituntilready").waitUntilReady(15, TimeUnit.MINUTES); assertNotNull(deploymentConfig); assertEquals(1, deploymentConfig.getStatus().getAvailableReplicas().intValue()); assertTrue(deploymentConfig.getStatus().getConditions().stream().anyMatch(c -> c.getType().equals("Available"))); diff --git a/kubernetes-itests/src/test/java/io/fabric8/openshift/TemplateIT.java b/kubernetes-itests/src/test/java/io/fabric8/openshift/TemplateIT.java index f5c60632b7..b4012ea706 100644 --- a/kubernetes-itests/src/test/java/io/fabric8/openshift/TemplateIT.java +++ b/kubernetes-itests/src/test/java/io/fabric8/openshift/TemplateIT.java @@ -18,7 +18,10 @@ import io.fabric8.commons.ClusterEntity; import io.fabric8.commons.ReadyEntity; +import io.fabric8.kubernetes.api.model.Pod; +import io.fabric8.kubernetes.api.model.PodBuilder; import io.fabric8.openshift.api.model.Template; +import io.fabric8.openshift.api.model.TemplateBuilder; import io.fabric8.openshift.api.model.TemplateList; import io.fabric8.openshift.client.OpenShiftClient; import org.arquillian.cube.kubernetes.api.Session; @@ -84,4 +87,45 @@ public void delete() { boolean bDeleted = client.templates().inNamespace(session.getNamespace()).withName("template-delete").delete(); assertTrue(bDeleted); } + + @Test + public void testCreateWithVersionV1() { + // Given + Pod pod = new PodBuilder() + .withNewMetadata().withName("redis-master").endMetadata() + .withNewSpec() + .addNewContainer() + .addNewEnv().withName("REDIS_PASSWORD").withValue("${REDIS_PASSWORD}").endEnv() + .withImage("dockerfile/redis") + .addNewPort() + .withContainerPort(6379) + .withProtocol("TCP") + .endPort() + .endContainer() + .endSpec() + .build(); + Template template = new TemplateBuilder() + .withNewMetadata() + .withName("templateit-createv1") + .addToAnnotations("description", "Description") + .addToAnnotations("iconClass", "icon-redis") + .addToAnnotations("tags", "database,nosql") + .endMetadata() + .addToObjects(pod) + .addNewParameter() + .withDescription("Password used for Redis authentication") + .withFrom("[A-Z0-9]{8}") + .withGenerate("expression") + .withName("REDIS_PASSWORD") + .endParameter() + .addToLabels("redis", "master") + .build(); + + // When + // Set v1 Api + template.setApiVersion("v1"); + template = client.templates().inNamespace(session.getNamespace()).create(template); + assertNotNull(template); + assertEquals("template.openshift.io/v1", template.getApiVersion()); + } } diff --git a/kubernetes-tests/src/test/java/io/fabric8/openshift/client/server/mock/BuildConfigTest.java b/kubernetes-tests/src/test/java/io/fabric8/openshift/client/server/mock/BuildConfigTest.java index dc21ac0ca8..415f49b0b2 100644 --- a/kubernetes-tests/src/test/java/io/fabric8/openshift/client/server/mock/BuildConfigTest.java +++ b/kubernetes-tests/src/test/java/io/fabric8/openshift/client/server/mock/BuildConfigTest.java @@ -201,23 +201,6 @@ public void testDelete() { assertTrue(deleted); } - @Test - void testCreateOrReplaceOpenShift3() { - // Given - BuildConfig buildConfig = getBuildConfig(); - server.expect().post().withPath("/oapi/v1/namespaces/ns1/buildconfigs") - .andReturn(HttpURLConnection.HTTP_OK, buildConfig) - .once(); - OpenShiftClient client = server.getOpenshiftClient(); - - // When - buildConfig = client.buildConfigs().inNamespace("ns1").createOrReplace(buildConfig); - - // Then - assertNotNull(buildConfig); - assertEquals("ruby-sample-build", buildConfig.getMetadata().getName()); - } - private BuildConfig getBuildConfig() { return new BuildConfigBuilder() .withNewMetadata().withName("ruby-sample-build").endMetadata() diff --git a/kubernetes-tests/src/test/java/io/fabric8/openshift/client/server/mock/DeploymentConfigTest.java b/kubernetes-tests/src/test/java/io/fabric8/openshift/client/server/mock/DeploymentConfigTest.java index 259c2866d3..315f96966b 100644 --- a/kubernetes-tests/src/test/java/io/fabric8/openshift/client/server/mock/DeploymentConfigTest.java +++ b/kubernetes-tests/src/test/java/io/fabric8/openshift/client/server/mock/DeploymentConfigTest.java @@ -49,7 +49,7 @@ import java.util.concurrent.atomic.AtomicBoolean; @EnableRuleMigrationSupport -public class DeploymentConfigTest { +class DeploymentConfigTest { @Rule public OpenShiftServer server = new OpenShiftServer(); @@ -252,23 +252,6 @@ public void visit(ContainerFluent container) { assertTrue(visitedContainer.get()); } - @Test - void testCreateOrReplaceOnOpenShift3() { - // Given - DeploymentConfig deploymentConfig = getDeploymentConfig().build(); - server.expect().post().withPath("/oapi/v1/namespaces/ns1/deploymentconfigs") - .andReturn(HttpURLConnection.HTTP_OK, deploymentConfig) - .once(); - OpenShiftClient client = server.getOpenshiftClient(); - - // When - deploymentConfig = client.deploymentConfigs().inNamespace("ns1").createOrReplace(deploymentConfig); - - // Then - assertNotNull(deploymentConfig); - assertEquals("dc1", deploymentConfig.getMetadata().getName()); - } - @Test void testGetLog() { // Given diff --git a/kubernetes-tests/src/test/java/io/fabric8/openshift/client/server/mock/TemplateTest.java b/kubernetes-tests/src/test/java/io/fabric8/openshift/client/server/mock/TemplateTest.java index 808bd2ddb9..6ec9fe48f2 100644 --- a/kubernetes-tests/src/test/java/io/fabric8/openshift/client/server/mock/TemplateTest.java +++ b/kubernetes-tests/src/test/java/io/fabric8/openshift/client/server/mock/TemplateTest.java @@ -262,23 +262,6 @@ void testEmptyParameterMapValueShouldNotThrowNullPointerException() { assertNotNull(list); } - @Test - void testCreateOrReplaceOpenShift3() { - // Given - Template template = getTemplateBuilder().build(); - server.expect().post().withPath("/oapi/v1/namespaces/ns1/templates") - .andReturn(HttpURLConnection.HTTP_OK, template) - .once(); - - OpenShiftClient client = server.getOpenshiftClient(); - - // When - template = client.templates().inNamespace("ns1").createOrReplace(template); - - // Then - assertNotNull(template); - } - @Test void testCreateOrReplaceOpenShif4() { // Given diff --git a/openshift-client/src/main/java/io/fabric8/openshift/client/dsl/internal/OpenShiftOperation.java b/openshift-client/src/main/java/io/fabric8/openshift/client/dsl/internal/OpenShiftOperation.java index 45d9573c9c..46c2866c20 100644 --- a/openshift-client/src/main/java/io/fabric8/openshift/client/dsl/internal/OpenShiftOperation.java +++ b/openshift-client/src/main/java/io/fabric8/openshift/client/dsl/internal/OpenShiftOperation.java @@ -41,6 +41,7 @@ public class OpenShiftOperation Objects.nonNull(resource) && OpenShiftReadiness.isReady(resource), amount, timeUnit); } - protected Class getConfigType() { + private void updateApiVersion() { + if (apiGroupName != null && apiGroupVersion != null) { + this.apiVersion = apiGroupName + "/" + apiGroupVersion; + } else if (apiGroupVersion != null) { + this.apiVersion = apiGroupVersion; + } + } + + protected Class getConfigType() { return OpenShiftConfig.class; } }