Skip to content

Commit

Permalink
Revert BackwardCompatibilityInterceptor's behavior of changing /apis …
Browse files Browse the repository at this point in the history 
…to /oapi URLs

This commit reverts 293ab9d which was added in order
to fix #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, #2373 can be fixed by adding apiVersion in OpenShiftOperation.
  • Loading branch information
@rohanKanojia
rohanKanojia committed Sep 15, 2020
1 parent c68c3fe commit f18e889
Show file tree
Hide file tree
Showing 6 changed files with 71 additions and 101 deletions.
68 changes: 18 additions & 50 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 @@ -24,7 +24,6 @@
import okio.Buffer;

import java.io.IOException;
import java.net.HttpURLConnection;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
Expand Down Expand Up @@ -128,24 +127,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);
}
Expand All @@ -170,28 +169,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{
if (!response.isSuccessful() && response.code() == HttpURLConnection.HTTP_NOT_FOUND) {
ResourceKey target = getResourceKeyFromRequest(request);
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) {
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);
Expand All @@ -214,34 +206,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;
}
}
44 changes: 44 additions & 0 deletions kubernetes-itests/src/test/java/io/fabric8/openshift/TemplateIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
}
}
17 changes: 0 additions & 17 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 @@ -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()
Expand Down
17 changes: 0 additions & 17 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 @@ -245,23 +245,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());
}

private DeploymentConfigBuilder getDeploymentConfig() {
return new DeploymentConfigBuilder()
.withNewMetadata()
Expand Down
17 changes: 0 additions & 17 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 @@ -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
Expand Down
9 changes: 9 additions & 0 deletions ...ift-client/src/main/java/io/fabric8/openshift/client/dsl/internal/OpenShiftOperation.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public class OpenShiftOperation<T extends HasMetadata, L extends KubernetesResou

public OpenShiftOperation(OperationContext ctx) {
super(wrap(ctx));
updateApiVersion();
}

static OperationContext wrap(OperationContext context) {
Expand Down Expand Up @@ -98,6 +99,14 @@ public URL getRootUrl() {
}
}

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;
}
Expand Down

0 comments on commit f18e889

Please sign in to comment.