Revert BackwardCompatibilityInterceptor's behavior of changing /apis …

…to /oapi URLs This commit reverts 293ab9d which was added in order to fix fabric8io#2373. This behavior was causing problems with Strimzi's Kubernetes Client upgrade: strimzi/strimzi-kafka-operator#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, fabric8io#2373 can be fixed by adding apiVersion in OpenShiftOperation.
rohanKanojia · Sep 21, 2020 · cdba1f4 · cdba1f4
1 parent 509364b
commit cdba1f4
Show file tree

Hide file tree

Showing 6 changed files with 72 additions and 101 deletions.
diff --git a/...t/src/main/java/io/fabric8/kubernetes/client/utils/BackwardsCompatibilityInterceptor.java b/...t/src/main/java/io/fabric8/kubernetes/client/utils/BackwardsCompatibilityInterceptor.java
@@ -128,24 +128,24 @@ 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);
       ResourceKey target = responseCodeToTransformations.get(response.code()).get(key);
       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/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
@@ -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/...tes-tests/src/test/java/io/fabric8/openshift/client/server/mock/DeploymentConfigTest.java b/...tes-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
@@ -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/...ift-client/src/main/java/io/fabric8/openshift/client/dsl/internal/OpenShiftOperation.java b/...ift-client/src/main/java/io/fabric8/openshift/client/dsl/internal/OpenShiftOperation.java
@@ -41,6 +41,7 @@ public class OpenShiftOperation<T extends HasMetadata, L extends KubernetesResou
 
   public OpenShiftOperation(OperationContext ctx) {
     super(wrap(ctx));
+    updateApiVersion();
   }
 
   static OperationContext wrap(OperationContext context) {
@@ -106,7 +107,15 @@ public T waitUntilReady(long amount, TimeUnit timeUnit) throws InterruptedExcept
     return waitUntilCondition(resource -> Objects.nonNull(resource) && OpenShiftReadiness.isReady(resource), amount, timeUnit);
   }
 
-  protected Class<? extends Config> getConfigType()  {
+  private void updateApiVersion() {
+    if (apiGroupName != null && apiGroupVersion != null) {
+      this.apiVersion = apiGroupName + "/" + apiGroupVersion;
+    } else if (apiGroupVersion != null) {
+      this.apiVersion = apiGroupVersion;
+    }
+  }
+
+  protected Class<? extends Config> getConfigType() {
     return OpenShiftConfig.class;
   }
 }