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

BuildConfig watch performs request on wrong endpoint #2668

Closed
manusa opened this issue Dec 16, 2020 · 8 comments
Closed

BuildConfig watch performs request on wrong endpoint #2668

manusa opened this issue Dec 16, 2020 · 8 comments
Assignees
Labels
Milestone

Comments

@manusa
Copy link
Member

manusa commented Dec 16, 2020

Description

The code introduced in #2490, more specifically the following lines

private void updateApiVersion() {
if (apiGroupName != null && apiGroupVersion != null) {
this.apiVersion = apiGroupName + "/" + apiGroupVersion;
} else if (apiGroupVersion != null) {
this.apiVersion = apiGroupVersion;
}
}

cause the watch connection to be performed to the following (invalid) endpoint:

/apis/build.openshift.io/v1/namespaces/test/builds/$resourceName?watch=true

instead of:

/apis/build.openshift.io/v1/namespaces/test/builds?fieldSelector=metadata.name%3D$resourceName&watch=true

This is originated in the following code:

if (baseOperation.isApiGroup()) {
httpUrlBuilder.addPathSegment(name);
} else {
if (fieldQueryString.length() > 0) {
fieldQueryString += ",";
}
fieldQueryString += "metadata.name=" + name;
}

Since isApiGroup() call returns true for OpenShift resources. In case of standard model types, apiVersion field is null, so method returns false.

The #2490 PR reverts #2373, however, those lines weren't part of that initial PR.

Relates to: openshift/origin#25759

@manusa manusa added the bug label Dec 16, 2020
@manusa manusa added this to the 5.0.0 milestone Dec 16, 2020
@manusa
Copy link
Member Author

manusa commented Dec 16, 2020

This issue is currently a blocker for the CI execution jobs in Eclipse JKube

@manusa
Copy link
Member Author

manusa commented Dec 16, 2020

Removing the updateApiVersion() and its invocation in the constructor fixed the issue in JKube (Using the code provided inhttps://github.com/eclipse-jkube/jkube/pull/524 and updating the dependency to 5.0-SNAPSHOT with the fixed client installed locally).

So this seems to be the fix, once we find out why those lines were added in the first place and we make sure we don't break anything else by removing them.

@rohanKanojia
Copy link
Member

I did a git bisect between v4.10.3 and v4.12.0 and it's pointing to this commit: a9caeb1

Here are git bisect logs:

kubernetes-client : $ git bisect log
git bisect start
# bad: [9dc84ecafba374b23324cff60bee56c53737315a] [RELEASE] Updated project version to v4.12.0
git bisect bad 9dc84ecafba374b23324cff60bee56c53737315a
# good: [9d05eb1fbb6134518c9b2991b3b372b13bd501d0] Updated project version to v4.10.3
git bisect good 9d05eb1fbb6134518c9b2991b3b372b13bd501d0
# good: [1621382068ac9cedcde4e710f01186389b1d38c6] Merge pull request #2438 from manusa/fix/clean-run
git bisect good 1621382068ac9cedcde4e710f01186389b1d38c6
# good: [62e54aeed88c4d2baf71d8b1ccc9fe2b0f44d61b] Merge pull request #2481 from fabric8io/dependabot/maven/sundrio.version-0.22.0
git bisect good 62e54aeed88c4d2baf71d8b1ccc9fe2b0f44d61b
# good: [6b20cb70eff324962d9e58b5d4c77f6642671894] Merge branch 'iss_2458' of github.com-personal:umutkocasarac/kubernetes-client into iss_2458
git bisect good 6b20cb70eff324962d9e58b5d4c77f6642671894
# bad: [71692850095527587abec4ed395bf47a9119371b] Merge pull request #2487 from umutkocasarac/iss_2458
git bisect bad 71692850095527587abec4ed395bf47a9119371b
# good: [3b271b39cecb500055b57920629b98f2a2a4aaa5] Merge pull request #2409 from anshlykov/gh-2391
git bisect good 3b271b39cecb500055b57920629b98f2a2a4aaa5
# good: [10f3a028aa6a3bc99cbac939c35f510283d96923] Merge remote-tracking branch 'origin/master'
git bisect good 10f3a028aa6a3bc99cbac939c35f510283d96923
# bad: [1551f10056ffc818eb3321ec6e6b43e95de33d85] Convert DeploymentIT#testWaitUntilReady() to a mock test
git bisect bad 1551f10056ffc818eb3321ec6e6b43e95de33d85
# bad: [a9caeb1a8be898df13f9ca03cf90398cac0876a2] Revert BackwardCompatibilityInterceptor's behavior of changing /apis to /oapi URLs
git bisect bad a9caeb1a8be898df13f9ca03cf90398cac0876a2
# first bad commit: [a9caeb1a8be898df13f9ca03cf90398cac0876a2] Revert BackwardCompatibilityInterceptor's behavior of changing /apis to /oapi URLs

I can confirm that reverting this commit seems to fix the issue. However, let me check why I did it and if it would break anything.

@rohanKanojia
Copy link
Member

When I remove updateApiVersion call this test[0] fails which seems to be related to #2373:

I checked the isApiGroup check in WatchConnectionManager and it seems to be added in this PR #821 to fix watch for OpenShift 3.6 resources. According to our compatibility matrix[1], we don't support OpenShift 3.6 anymore. Maybe we should remove this Openshift 3.6 specific workaround from WatchConnectionManager?

[0] https://github.com/fabric8io/kubernetes-client/blob/master/kubernetes-itests/src/test/java/io/fabric8/openshift/TemplateIT.java#L92
[1] https://github.com/fabric8io/kubernetes-client#openshift-compatibility-matrix

@manusa
Copy link
Member Author

manusa commented Dec 17, 2020

Maybe we should remove this Openshift 3.6 specific workaround from WatchConnectionManager?

This is what #2671 is doing (which I think is the right approach).

I'm still not sure if this custom raw updateApiVersion() method is right (especially given the test you've pointed out), or if it will backfire at us again in the future. However, we can consider #2671 to be the proper fix for this issue, since it tackles the root cause.

@rohanKanojia
Copy link
Member

Let me try to explain why I added that updateApiVersion method.

If you try to create an object like this using oc create -f test-template:

apiVersion: v1
kind: Template
metadata:
  name: redis-template
...

OpenShift will automatically convert ApiVersion from v1 to new ApiVersions. Here is how object looks when I do oc get template redis-template:

kubernetes-resource-yamls : $ oc get template redis-template -o yaml
apiVersion: template.openshift.io/v1
kind: Template
labels:
  redis: master
metadata:
  annotations:
    description: Description
    iconClass: icon-redis
    tags: database,nosql
  creationTimestamp: 2020-12-17T06:53:17Z
  name: redis-template
...

Right now in OpenShiftClient we try to find out whether the cluster is an Old OpenShift cluster /oapi endpoints or newer clusters (/apis/*.openshift.io/v1) here during initialization, we basically hit an /apis endpoint and check whether any ApiGroup exists which ends with openshift.io:

Boolean enabled = OpenshiftAdapterSupport.isOpenShiftAPIGroups(httpClient, url);

So if you try to create an OpenShift object with apiGroup: v1 on newer OpenShift Clusters, this happens:

  • OpenShiftClient detects /apis endpoint and sets config.withOpenshiftApiGroupsEnabled(true). Which means it will use /apis/*.openshift.io/v1 endpoints
  • OpenShiftClient sends the payload to the API Server on /apis/*.openshift.io/v1 endpoint
  • Request fails since apiVersion in request body is still v1 and URL being requested is /apis/*.openshift.io/v1

@manusa
Copy link
Member Author

manusa commented Dec 17, 2020

Is there any definition of what openShiftApiGroups are exactly or if they are actually a "thing"?

I can only trace back to this #716

Right now in OpenShiftClient we try to find out whether the cluster

IIRC this is not exactly true, since instead of detecting what the cluster is or the APIs it provides, we fallback to other endpoints in case those are the right ones.

@rohanKanojia
Copy link
Member

Closed via #2671

rohanKanojia added a commit to rohanKanojia/kubernetes-client that referenced this issue Dec 22, 2020
We instantiate BuildConfig in Eclipse JKube while doing S2I
builds. Adding an integration test for that so that we don't break this
again in future.

Related to fabric8io#2668
rohanKanojia added a commit to rohanKanojia/kubernetes-client that referenced this issue Dec 23, 2020
We instantiate BuildConfig in Eclipse JKube while doing S2I
builds. Adding an integration test for that so that we don't break this
again in future.

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

No branches or pull requests

2 participants