Exposing apigroups and apiresources

[manusa]

Description

Supersedes #3405
Fixes #3294
Fixes #3303

Fixes issue when compiling in JDK 8.

Sundrio has an issue when traversing the AST path for OkHttpClient (return type in previos Config#adaptClient).
The new commits on top of #3405 remove the adaptClient from the Config classes and places a similar method in the BaseClient class. The Config class is a little more anemic but at least OkHttpClient adaptation has been abstracted 🤷.

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

[centos-ci]

Can one of the admins verify this patch?

[shawkins]

LGTM

[manusa]

LGTM

Still getting some errors in the tests (I assume that the original PR would have those same errors, did you build it with tests locally?)

Errors are due to the OpenShiftAdapterSupport#isOpenShift call in the DefaultOpenShiftClient constructor, which fails in those unit tests that don't have a valid connection. I mocked that class for the openshift-client module tests, but there are still some failures in the Karaf module, still need to check those.

[manusa]

OK, I think it might be related with the removal of the negation:
if (config.isDisableApiGroupCheck()) {

This was initially implemented in #1761 -> #795

[shawkins]

OK, I think it might be related with the removal of the negation:

The problem should be with https://github.com/fabric8io/kubernetes-client/pull/3487/files#diff-7b405d7bf5ccb2d183a3bdd42f24aad3cf3e7d1bb419323b2a8a4d422bc6bac4R233

If there is no endpoint, then this call will fail. Previously the exception was swallowed in the OpenshiftAdapterSupport.

I guess karaf logic is expecting to construct the openshift client without a failure.

[shawkins]

After looking at this some more, I'm not sure it was ever working as intended. Between the boolean confusion, exception swallowing, and API_GROUPS_ENABLED_PER_URL.containsKey - rather than a get, it seems to have a very narrow path to work consistently.

I've tried in the next commit to satisfy #3184 as well. Essentially the behavior is:

if isDisableApiGroupCheck is true, then we'll report that it's openshift. That somewhat addresses #3184
if isOpenshiftApiGroupsEnabled is true, then we'll assume the use of api group / version in openshift calls.
if isOpenshiftApiGroupsEnabled is false, then we'll effectively rely on isDisableApiGroupCheck via the isOpenShift call. If it's false, we'll do the check. If it's true, then we'll assume it's openshift - that's a difference from old logic.

[manusa]

Your fix seems to be passing the tests now, thx.

Shall I remove this one e59f433? (It didn't seem to have any effect either) (We really need some proper unit tests to document the logic of the configuration)

I'm a little worried about possible regressions introduced by this PR when working on OpenShift. Shall we skip it from today's release and delay it to 5.9.0?

[shawkins]

I'm a little worried about possible regressions introduced by this PR when working on OpenShift. Shall we skip it from today's release and delay it to 5.9.0?

I can understand that.

On the one hand this wasn't functioning like it should. After the first time through the DefaultOpenShiftClient constructor, the check https://github.com/fabric8io/kubernetes-client/pull/3487/files#diff-7b405d7bf5ccb2d183a3bdd42f24aad3cf3e7d1bb419323b2a8a4d422bc6bac4L235 would assume "enabled" for any url previously checked. However since it's not altering the config in that path it's really not doing anything to the config flags at that point.

Only on the first time through this logic was there a possibility of checking the api groups.

If disableApiGroupCheck was false, then openshiftApiGroupsEnabled became false - regardless of what was set on the OpenShiftConfig (which is also confusing because in the OpenShiftConfig openshiftApiGroupsEnabled is treated as the primary property).

If disableApiGroupCheck was true (which appears to be only for mock logic), then it would perform the group check and use that result as openshiftApiGroupsEnabled - that seems completely contrary to the disable flag.

The proposed changes are trying somewhat to honor the original intent of the flags, and thus introducing the need to set the disable flag in more places. You could make the case to further change this logic for clarity.

Since the way we check for openshift is based upon the presence of openshift apigroups, in effect isOpenShift == isOpenshiftApiGroupsEnabled. Also since OpenShift 3.6 and earlier is no longer supported, at this point I'd say we don't need to preserve isOpenshiftApiGroupsEnabled at all. The rationale for keeping disableApiGroupCheck is for making the isAdaptable check work in mock scenarios.

I've added a commit that does this. The concern there is that it's actually behavioral different from before - what I can see/infer if you leave everything at the defaults, we'll only ever use oapi. With this change we'll use apis. @rohanKanojia is there anything that is required to use oapi that is an openshift.io group?

So it's probably best to get more eyes and testing on this.

[shawkins]

Also my apologizes for applying the "Whenever I run into a problem I can't solve, I always make it bigger" principle here.

[manusa]

So it's probably best to get more eyes and testing on this.

Since there is nothing critical here, I'll schedule this for 5.9.0 (due in < 3 weeks)

On the one hand this wasn't functioning like it should

I added a breaking change note in the changelog, at least to warn the users. It wasn't working as supposed, but has been like this for a while. The recent changes now enforce the check which might (or might not) break some user's current setup. The warning should help them figure out the issue.

[rohanKanojia]

As far as I remember /oapi path was used by OpenShift versions < 3.11 and got deprecated in favor of /apis . I had made this change in the scope of #1587 . Not 100% sure but I don't think there is any resource that'll use /oapi if it's in openshift.io apigroup.

[shawkins]

The warning should help them figure out the issue.

I've updated that to reflect these next changes.

[manusa]

TIL, I always run out of letters from the alphabet :)

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

35.9% Coverage
0.0% Duplication