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 31, 2020
1 parent 0b1e066 commit 293ab9d
Show file tree
Hide file tree
Showing 7 changed files with 304 additions and 62 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
* Fix #2316: Cannot load resource from stream without apiVersion

#### Improvements
Expand Down
55 changes: 43 additions & 12 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,8 +132,8 @@ public int hashCode() {
public Response intercept(Chain chain) throws IOException {
Request request = chain.request();
Response response = chain.proceed(request);
if (isDeprecatedOpenshiftOapiRequest(request)) {
return handleOpenshiftOapiRequests(request, response, chain);
if (isOpenshiftApiRequest(request)) {
return handleOpenshiftRequests(request, response, chain);
} else if (!response.isSuccessful() && responseCodeToTransformations.keySet().contains(response.code())) {
String url = request.url().toString();
Matcher matcher = getMatcher(url);
Expand Down Expand Up @@ -169,21 +169,28 @@ 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 handleOpenshiftOapiRequests(Request request, Response response, Chain chain) throws IOException{
private static Response handleOpenshiftRequests(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);
ResourceKey target = getResourceKeyFromRequest(request);
if (target != null) {
requestUrl = requestUrl.replace("/oapi", "/apis/" + target.getGroup());
String requestUrl = request.url().toString();
requestUrl = isOpenShift4Request(requestUrl) ?
convertToOpenShiftOapiUrl(requestUrl, target) :
convertToOpenShift4Url(requestUrl, target);
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);
Expand All @@ -206,10 +213,34 @@ 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) {
String requestUrl = request.url().toString();
return isOpenshift3OapiRequest(requestUrl) || isOpenShift4Request(requestUrl);
}
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;
}
}
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();
}

}
97 changes: 59 additions & 38 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 All @@ -46,7 +47,7 @@ public class DeploymentConfigTest {
public OpenShiftServer server = new OpenShiftServer();

@Test
public void testList() {
void testList() {
server.expect().withPath("/apis/apps.openshift.io/v1/namespaces/test/deploymentconfigs").andReturn(200, new DeploymentConfigListBuilder().build()).once();
server.expect().withPath("/apis").andReturn(200, new APIGroupListBuilder()
.addNewGroup()
Expand Down Expand Up @@ -84,7 +85,7 @@ public void testList() {
}

@Test
public void testGet() {
void testGet() {
server.expect().withPath("/apis/apps.openshift.io/v1/namespaces/test/deploymentconfigs/dc1").andReturn(200, new DeploymentConfigBuilder()
.withNewMetadata().withName("dc1").endMetadata()
.build()).once();
Expand All @@ -108,7 +109,7 @@ public void testGet() {
}

@Test
public void testDelete() throws InterruptedException {
void testDelete() throws InterruptedException {
DeploymentConfig dc1 = new DeploymentConfigBuilder()
.withNewMetadata()
.withName("dc1")
Expand Down Expand Up @@ -165,7 +166,7 @@ public void testDelete() throws InterruptedException {
}

@Test
public void testDeleteWithPropagationPolicy() throws InterruptedException {
void testDeleteWithPropagationPolicy() throws InterruptedException {
server.expect().delete()
.withPath("/apis/apps.openshift.io/v1/namespaces/test/deploymentconfigs/dc1")
.andReturn(200, new DeploymentConfigBuilder().build())
Expand All @@ -179,7 +180,7 @@ public void testDeleteWithPropagationPolicy() throws InterruptedException {
}

@Test
public void testDeployingLatest() {
void testDeployingLatest() {
server.expect().withPath("/apis/apps.openshift.io/v1/namespaces/test/deploymentconfigs/dc1")
.andReturn(200, new DeploymentConfigBuilder().withNewMetadata().withName("dc1").endMetadata()
.withNewStatus().withLatestVersion(1L).endStatus().build())
Expand All @@ -194,11 +195,11 @@ public void testDeployingLatest() {

DeploymentConfig deploymentConfig = client.deploymentConfigs().withName("dc1").deployLatest();
assertNotNull(deploymentConfig);
assertEquals(new Long(2), deploymentConfig.getStatus().getLatestVersion());
assertEquals(Long.valueOf(2), deploymentConfig.getStatus().getLatestVersion());
}

@Test
public void testDeployingLatestHandlesMissingLatestVersion() {
void testDeployingLatestHandlesMissingLatestVersion() {
server.expect().withPath("/apis/apps.openshift.io/v1/namespaces/test/deploymentconfigs/dc1")
.andReturn(200, new DeploymentConfigBuilder().withNewMetadata().withName("dc1").endMetadata()
.withNewStatus().endStatus().build())
Expand All @@ -213,43 +214,15 @@ public void testDeployingLatestHandlesMissingLatestVersion() {

DeploymentConfig deploymentConfig = client.deploymentConfigs().withName("dc1").deployLatest();
assertNotNull(deploymentConfig);
assertEquals(new Long(1), deploymentConfig.getStatus().getLatestVersion());
assertEquals(Long.valueOf(1), deploymentConfig.getStatus().getLatestVersion());
}

//This is a test that verifies a recent fix (sundrio #135).
//According to this issue when editing a list of buildables using predicates, the object visitors get overwrriten.
@Test
public void testDeploymentConfigVisitor() {
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 @@ -268,6 +241,54 @@ public void visit(ContainerFluent<?> container) {

}
}).build();
assertNotNull(dc2);
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());
}

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();
}
}
47 changes: 47 additions & 0 deletions ...src/test/java/io/fabric8/openshift/client/server/mock/SecurityContextConstraintsTest.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 io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.openshift.api.model.SecurityContextConstraints;
import io.fabric8.openshift.api.model.SecurityContextConstraintsBuilder;
import io.fabric8.openshift.api.model.SecurityContextConstraintsList;
Expand All @@ -27,6 +28,9 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.migrationsupport.rules.EnableRuleMigrationSupport;

import java.net.HttpURLConnection;
import java.util.List;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
Expand All @@ -37,6 +41,49 @@ public class SecurityContextConstraintsTest {
@Rule
public OpenShiftServer server = new OpenShiftServer();

@Test
void testCreateOrReplace() {
// Given
SecurityContextConstraints scc = new SecurityContextConstraintsBuilder()
.withNewMetadata().withName("scc1").endMetadata()
.withAllowPrivilegedContainer(true)
.withNewRunAsUser().withType("RunAsAny").endRunAsUser()
.withNewSeLinuxContext().withType("RunAsAny").endSeLinuxContext()
.withUsers("admin")
.withGroups("admin-group")
.build();
server.expect().post().withPath("/apis/security.openshift.io/v1/securitycontextconstraints")
.andReturn(HttpURLConnection.HTTP_OK, scc)
.once();
OpenShiftClient client = server.getOpenshiftClient();

// When
scc = client.securityContextConstraints().createOrReplace(scc);

// Then
assertNotNull(scc);
assertEquals("scc1", scc.getMetadata().getName());
assertEquals(1, scc.getUsers().size());
assertEquals(1, scc.getGroups().size());
}

@Test
void testLoad() {
// Given
server.expect().post().withPath("/apis/security.openshift.io/v1/securitycontextconstraints")
.andReturn(HttpURLConnection.HTTP_OK, new SecurityContextConstraintsBuilder().build())
.once();
OpenShiftClient client = server.getOpenshiftClient();

// When
List<HasMetadata> items = client.load(getClass().getResourceAsStream("/test-scc.yml")).createOrReplace();

// Then
assertNotNull(items);
assertEquals(1, items.size());
assertTrue(items.get(0) instanceof SecurityContextConstraints);
}

@Test
public void testList() {
server.expect().withPath("/apis/security.openshift.io/v1/securitycontextconstraints").andReturn(200, new SecurityContextConstraintsListBuilder()
Expand Down
Loading

0 comments on commit 293ab9d

Please sign in to comment.