Skip to content

Commit

Permalink
Fix #2373: Unable to create a Template on OCP3
Browse files Browse the repository at this point in the history 
Looks like code added in #1956 BackwardCompatibilityInterceptor wasn't handling
/oapi failures properly.

Fix BackwardCompatibilityInterceptor logic for routing to OpenShift 3 /oapi/v1
endpoints when OpenShift 4 /apis/{group}/v1 endpoints not found. Also added some
tests to assert desired behavior
  • Loading branch information
@rohanKanojia
rohanKanojia committed Jul 28, 2020
1 parent a0a4516 commit 1426c0b
Show file tree
Hide file tree
Showing 5 changed files with 169 additions and 37 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

### 4.10-SNAPSHOT
#### Bugs
* Fix #2373: Unable to create a Template on OCP3

#### Improvements

Expand Down
11 changes: 5 additions & 6 deletions ...t/src/main/java/io/fabric8/kubernetes/client/utils/BackwardsCompatibilityInterceptor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ public int hashCode() {
public Response intercept(Chain chain) throws IOException {
Request request = chain.request();
Response response = chain.proceed(request);
if (isDeprecatedOpenshiftOapiRequest(request)) {
if (isOpenshiftApiRequest(request)) {
return handleOpenshiftOapiRequests(request, response, chain);
} else if (!response.isSuccessful() && responseCodeToTransformations.keySet().contains(response.code())) {
String url = request.url().toString();
Expand Down Expand Up @@ -172,12 +172,11 @@ private static ResourceKey getKey(Matcher m) {
private static Response handleOpenshiftOapiRequests(Request request, Response response, Chain chain) throws IOException{
if (!response.isSuccessful()) {
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) {
requestUrl = requestUrl.replace("/oapi", "/apis/" + target.getGroup());
requestUrl = requestUrl.replace("/apis/" + target.getGroup() + "/" + target.getVersion(), "/oapi/v1");
return handleNewRequestAndProceed(request, requestUrl, target, chain);
}
}
Expand Down Expand Up @@ -206,9 +205,9 @@ private static Response handleNewRequestAndProceed(Request request, String newUr
return chain.proceed(newRequest.build());
}

private static boolean isDeprecatedOpenshiftOapiRequest(Request request) {
if (request != null && request.url() != null) {
return request.url().toString().contains("oapi");
private static boolean isOpenshiftApiRequest(Request request) {
if (request != null) {
return request.url().toString().contains(".openshift.io");
}
return false;
}
Expand Down
45 changes: 45 additions & 0 deletions kubernetes-tests/src/test/java/io/fabric8/openshift/client/server/mock/BuildConfigTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.net.HttpURLConnection;
import java.net.SocketTimeoutException;
import java.util.concurrent.TimeUnit;

Expand Down Expand Up @@ -200,4 +201,48 @@ 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()
.withNewSpec()
.withRunPolicy("Serial")
.addNewTrigger().withType("GitHub").withNewGithub().withSecret("secret101").endGithub().endTrigger()
.addNewTrigger().withType("Generic").withNewGeneric().withSecret("secret101").endGeneric().endTrigger()
.addNewTrigger().withType("ImageChange").endTrigger()
.withNewSource()
.withNewGit().withUri("https://github.com/openshift/ruby-hello-world").endGit()
.endSource()
.withNewStrategy()
.withNewSourceStrategy()
.withNewFrom()
.withKind("ImageStreamTag")
.withName("ruby-20-centos7:latest")
.endFrom()
.endSourceStrategy()
.endStrategy()
.withNewOutput()
.withNewTo().withKind("ImageStreamTag").withName("origin-ruby-sample:latest").endTo()
.endOutput()
.withNewPostCommit().withScript("bundle exec rake test").endPostCommit()
.endSpec()
.build();
}

}
78 changes: 49 additions & 29 deletions ...tes-tests/src/test/java/io/fabric8/openshift/client/server/mock/DeploymentConfigTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import io.fabric8.openshift.api.model.DeploymentConfigListBuilder;
import io.fabric8.openshift.client.OpenShiftClient;

import java.net.HttpURLConnection;
import java.util.concurrent.atomic.AtomicBoolean;

@EnableRuleMigrationSupport
Expand Down Expand Up @@ -221,35 +222,7 @@ public void testDeployingLatestHandlesMissingLatestVersion() {
@Test
public void testDeploymentConfigVisitor() {
AtomicBoolean visitedContainer = new AtomicBoolean();

DeploymentConfig dc1 = new DeploymentConfigBuilder()
.withNewMetadata()
.withName("dc1")
.endMetadata()
.withNewSpec()
.withReplicas(1)
.addToSelector("name", "dc1")
.addNewTrigger()
.withType("ImageChange")
.withNewImageChangeParams()
.withAutomatic(true)
.withContainerNames("container")
.withNewFrom()
.withKind("ImageStreamTag")
.withName("image:1.0")
.endFrom()
.endImageChangeParams()
.endTrigger()
.withNewTemplate()
.withNewSpec()
.addNewContainer()
.withName("container")
.withImage("image")
.endContainer()
.endSpec()
.endTemplate()
.endSpec()
.build();
DeploymentConfig dc1 = getDeploymentConfig().build();

DeploymentConfig dc2 = new DeploymentConfigBuilder(dc1)
.accept(new TypedVisitor<DeploymentConfigSpecFluent<?>>() {
Expand All @@ -270,4 +243,51 @@ public void visit(ContainerFluent<?> container) {
}).build();
assertTrue(visitedContainer.get());
}

@Test
public 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());
}

private DeploymentConfigBuilder getDeploymentConfig() {
return new DeploymentConfigBuilder()
.withNewMetadata()
.withName("dc1")
.endMetadata()
.withNewSpec()
.withReplicas(1)
.addToSelector("name", "dc1")
.addNewTrigger()
.withType("ImageChange")
.withNewImageChangeParams()
.withAutomatic(true)
.withContainerNames("container")
.withNewFrom()
.withKind("ImageStreamTag")
.withName("image:1.0")
.endFrom()
.endImageChangeParams()
.endTrigger()
.withNewTemplate()
.withNewSpec()
.addNewContainer()
.withName("container")
.withImage("image")
.endContainer()
.endSpec()
.endTemplate()
.endSpec();
}
}
71 changes: 69 additions & 2 deletions kubernetes-tests/src/test/java/io/fabric8/openshift/client/server/mock/TemplateTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package io.fabric8.openshift.client.server.mock;

import java.io.IOException;
import java.net.HttpURLConnection;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand All @@ -24,6 +25,8 @@
import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.api.model.KubernetesList;
import io.fabric8.kubernetes.api.model.KubernetesListBuilder;
import io.fabric8.kubernetes.api.model.Pod;
import io.fabric8.kubernetes.api.model.PodBuilder;
import io.fabric8.kubernetes.api.model.Service;
import io.fabric8.kubernetes.api.model.ServicePort;
import io.fabric8.kubernetes.api.model.ServiceSpec;
Expand Down Expand Up @@ -181,7 +184,7 @@ public void testProcess() {
}

@Test
public void shouldLoadTemplateWithNumberParameters() throws Exception {
public void shouldLoadTemplateWithNumberParameters() {
OpenShiftClient client = new DefaultOpenShiftClient(new OpenShiftConfigBuilder().build());
Map<String, String> map = new HashMap<>();
map.put("PORT", "8080");
Expand All @@ -205,7 +208,7 @@ protected static void assertListIsServiceWithPort8080(List<HasMetadata> items) {
List<ServicePort> ports = serviceSpec.getPorts();
assertEquals(1, ports.size());
ServicePort port = ports.get(0);
assertEquals(new Integer(8080), port.getPort());
assertEquals(Integer.valueOf(8080), port.getPort());
}

@Test
Expand Down Expand Up @@ -259,4 +262,68 @@ public 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
Template template = getTemplateBuilder().build();
server.expect().post().withPath("/apis/template.openshift.io/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);
}

private TemplateBuilder getTemplateBuilder() {
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();
return new TemplateBuilder()
.withNewMetadata()
.withName("redis-template")
.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");
}

}

0 comments on commit 1426c0b

Please sign in to comment.