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 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.
  • Loading branch information
@rohanKanojia
rohanKanojia committed Sep 21, 2020
1 parent 509364b commit c2c1ad4
Show file tree
Hide file tree
Showing 6 changed files with 72 additions and 101 deletions.
65 changes: 17 additions & 48 deletions ...t/src/main/java/io/fabric8/kubernetes/client/utils/BackwardsCompatibilityInterceptor.java
View file
Expand Up @@ -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);
}
Expand All @@ -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);
Expand All @@ -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;
}
}
44 changes: 44 additions & 0 deletions kubernetes-itests/src/test/java/io/fabric8/openshift/TemplateIT.java
View file
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
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
19 changes: 1 addition & 18 deletions ...tes-tests/src/test/java/io/fabric8/openshift/client/server/mock/DeploymentConfigTest.java
View file
Expand Up @@ -49,7 +49,7 @@
import java.util.concurrent.atomic.AtomicBoolean;

@EnableRuleMigrationSupport
public class DeploymentConfigTest {
class DeploymentConfigTest {
@Rule
public OpenShiftServer server = new OpenShiftServer();

Expand Down Expand Up @@ -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
Expand Down
17 changes: 0 additions & 17 deletions kubernetes-tests/src/test/java/io/fabric8/openshift/client/server/mock/TemplateTest.java
View file
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
11 changes: 10 additions & 1 deletion ...ift-client/src/main/java/io/fabric8/openshift/client/dsl/internal/OpenShiftOperation.java
View file
Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
}
}

0 comments on commit c2c1ad4

Please sign in to comment.