Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert BackwardCompatibilityInterceptor's behavior of changing /apis to /oapi URLs #2490

Conversation

rohanKanojia
Copy link
Member

@rohanKanojia rohanKanojia commented Sep 15, 2020

Revert #2373

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. Git bisect log:

kubernetes-client : $ git bisect log
git bisect start
# bad: [ea09419284b32e6da70aadfaf9022485f2bec594] Merge pull request #2478 from rohanKanojia/pr/migrate-github-actions-java8
git bisect bad ea09419284b32e6da70aadfaf9022485f2bec594
# good: [c5fe5488dc15ed3d4795a3b3d676549c24c2e744] Post v4.10.3 release chores
git bisect good c5fe5488dc15ed3d4795a3b3d676549c24c2e744
# bad: [262e35f09be835bf5f3b5fbc4c16791224fbc390] Merge pull request #2381 from rohanKanojia/pr/issue2308
git bisect bad 262e35f09be835bf5f3b5fbc4c16791224fbc390
# bad: [d13a86480f699735dcc8e0ece90c24d4e49079e8] Merge pull request #2393 from rohanKanojia/pr/fix-failing-resource-it
git bisect bad d13a86480f699735dcc8e0ece90c24d4e49079e8
# good: [f1d695d59ce50a9067639df0b79855f03096adc8] Merge pull request #2371 from manusa/fix/mockserver-post
git bisect good f1d695d59ce50a9067639df0b79855f03096adc8
# good: [0b1e0668788aaf2d7f0e706f111ca67456cbb36c] Merge pull request #2361 from fabric8io/dependabot/maven/org.jboss-jandex-2.2.1.Final
git bisect good 0b1e0668788aaf2d7f0e706f111ca67456cbb36c
# bad: [14cae945ee20ce1ac980d492e53d388aa93eb371] Merge pull request #2372 from rohanKanojia/pr/issue2292
git bisect bad 14cae945ee20ce1ac980d492e53d388aa93eb371
# bad: [f6d4e8fedd2d7c36a5608e5f3a4fd73070e51559] Merge pull request #2375 from rohanKanojia/pr/issue2373
git bisect bad f6d4e8fedd2d7c36a5608e5f3a4fd73070e51559
# bad: [293ab9d445291de389d438f4a857928722230dba] Fix #2373: Unable to create a Template on OCP3
git bisect bad 293ab9d445291de389d438f4a857928722230dba
# first bad commit: [293ab9d445291de389d438f4a857928722230dba] Fix #2373: Unable to create a Template on OCP3

Handling OpenShift old /oapi requests seem to be handled in OpenShiftOperation where we check config.isOpenshiftApiGroupsEnabled() to modify OperationContext.

#2373 can be fixed by adding apiVersion in OpenShiftOperation.

Description

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change
  • Chore (non-breaking change which doesn't affect codebase;
    test, version modification, documentation, etc.)

Checklist

  • Code contributed by me aligns with current project license: Apache 2.0
  • I Added CHANGELOG entry regarding this change
  • I have implemented unit tests to cover my changes
  • I have added/updated the javadocs and other documentation accordingly
  • No new bugs, code smells, etc. in SonarCloud report
  • I tested my code in Kubernetes
  • I tested my code in OpenShift

@manusa
Copy link
Member

manusa commented Sep 16, 2020

You're "reverting" too much, some of the fixes provided after (like the one for createOrReplace #2467) are reverted too.

@rohanKanojia
Copy link
Member Author

I reverted BackwardCompatibilityInterceptor to it's 4.10.3 state. I think #2467 was also caused by my change in #2375 . I haven't reverted tests for #2467 I added(they're passing with this change):

public void createOrReplace() {
// Given
DeploymentConfig deploymentConfig = client.deploymentConfigs().inNamespace(session.getNamespace()).withName("dc-createorreplace").get();
// When

public void createOrReplace() {
// Given
Route route = client.routes().inNamespace(currentNamespace).withName("route-createorreplace").get();
// When
route.getMetadata().setAnnotations(Collections.singletonMap("foo", "bar"));
route.getSpec().setHost("test.fabric8.io");
route = client.routes().inNamespace(currentNamespace).createOrReplace(route);

@manusa
Copy link
Member

manusa commented Sep 16, 2020

I reverted BackwardCompatibilityInterceptor to it's 4.10.3 state. I think #2467 was also caused by my change in #2375 .

I'm not so sure about that.

The thing is that with the "new" createOrReplace procedure, we are relying on a 409 Conflict HTTP status code response to trigger the replace request.

If you don't keep this check in the interceptor:

 if (!response.isSuccessful() && response.code() == HttpURLConnection.HTTP_NOT_FOUND)

The interceptor will "kick in" whenever a resource needs to be replaced (too).

Regarding tests not failing, I'm guessing the tests you implemented in #2467 fix did not include an OpenShift resource specific replace case scenario.

@rohanKanojia
Copy link
Member Author

rohanKanojia commented Sep 16, 2020

I think you're right. Right now BackwardsCompatibiityInterceptor(what is here in this PR) seems to be only converting /oapi URLs to /apis. Client sends /apis URLs for both OpenShift 3.10.0 and 3.11.0, this is the reason tests are passing on both. I should add that 404 check.

@rohanKanojia rohanKanojia force-pushed the pr/fix-backwardscompatibilityinterceptor-bug branch from f18e889 to e5d2aca Compare September 16, 2020 11:06
@rohanKanojia rohanKanojia force-pushed the pr/fix-backwardscompatibilityinterceptor-bug branch 3 times, most recently from c2c1ad4 to 98532d0 Compare September 21, 2020 08:12
…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 rohanKanojia force-pushed the pr/fix-backwardscompatibilityinterceptor-bug branch from 98532d0 to a9caeb1 Compare September 21, 2020 09:18
@sonarcloud
Copy link

sonarcloud bot commented Sep 21, 2020

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

17.6% 17.6% Coverage
0.0% 0.0% Duplication

warning The version of Java (1.8.0_265) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

@rohanKanojia
Copy link
Member Author

[merge]

@fusesource-ci fusesource-ci merged commit 707ca3b into fabric8io:master Sep 25, 2020
@tombentley
Copy link
Contributor

@rohanKanojia please could you tell me when there might be a release with this change in?

@manusa
Copy link
Member

manusa commented Oct 2, 2020

Hi @tombentley
If there are no impediments, we'll be cutting a release today.

@tombentley
Copy link
Contributor

Awesome, thanks!

@manusa
Copy link
Member

manusa commented Oct 2, 2020

Hi @tombentley
In case you missed it, new release is available https://github.com/fabric8io/kubernetes-client/releases/tag/v4.12.0 on Maven Central.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to create a Template on OCP3
5 participants