[issue 1269] (2.0.x) Bumping Fabric8 version to 6.9.2

[fabiobrz]

Short description of what this resolves:

Upgrading the Fabric8 Kubernetes client to 6.9.2, non-disruptive approach.
Can be improved after further analysis and experiments.

CC @rsearls, @jimma, @gaol

Changes proposed in this pull request:

• import packages refactoring
• changes to align the implementation that uses the component with the latest APIs

Fixes #1269
@rsearls this also required the fixes for many issues that you have logged separately. How do we deal with such changes?

Draft: still having some issues with the Istio extension (currently commented) and there are TODO - check occurrences, that need to be resolved.

Some remarks and trackers:

1. Internal changes regarding Istio integration: the Istio client implementation moved within the Fabric8 K8s Client project and evolved. So we had to adjust the integration in order to be aligned with the new semantics: IstioClientAdapter has been created to adapt the original K8s client. This adapter implements IstioClient and a concrete instance of it is injected by IstioAssistantCreatorinto the IstioAssistant instance. This last one represents the main entry point to the integration, and the APIs that exposes have been kept unchanged, apart for one caveat: resources can no longer be created with generic lists, but just with single definitions. A unit test has been added for the new IstioClientAdapter class.

2. How we deal with HttpClient: now that the OkHttpClient impl is no longer exposed by the Fabric8 K8s Client, we made some changes, e.g.: here, which need to be double checked

3. INameServicehas been added for ArqCubeNameService to implement it. It was used to be provided by ArqCubeNameServiceDescriptor with JDK 8, which was self registering via SPI by implementing sun.net.spi.nameservice.NameServiceDescriptor. This last one is internal and no longer available in JDK 11, so ArqCubeNameServiceDescriptor has been removed and ArqCubeNameService can be registered programmatically via INameService.install(new ArqCubeNameService()) - must be double checked and verified, see INameService Javadoc and the related test.

4. GitHub CI worlflow: only JDK 11 enabled, JDK 8 to be verified. Category left as is (i.e. "docker", we should experiment with others)

5. mvnw version: bumped to 3.9.6

6. Mockito unnecessary stubs: now (with newer Mockito dependencies) those are traced as error that break the build, so we had to mark all of them via Mockito.lenient(), and we might want to log a follow up issue for detailed evaluation and removal

7. arquillian.cube.docker.impl.client.config POJOs: (e.g.: ExposedPort.java, Link.java, etc.) have been modified to add a single argument ctor, otherwise YAML deserialization via newer jackson dependencies would fail.

8. The podman unix socket is now used by DockerHealthAwaitStrategyTest:, raher than the docker one.

9. MockTest.java: Changes have been made to the Kubernetes mock server in order to mock additional expectations required by the new client behavior

[jimma]

@fabiobrz Awesome work ! This looks good to me. Only some ideas/minor things about OKHttpClient:

• Is it possible that we use jdk http client or url connection in IstioAssistant, then we don't need this okhttpclient dependency anymore?
• For the okhttpclient in the test , can we simply replace it with `restassured' to easily check the json response ?

[fabiobrz]

• Is it possible that we use jdk http client or url connection in IstioAssistant, then we don't need this okhttpclient dependency anymore?

• For the okhttpclient in the test , can we simply replace it with `restassured' to easily check the json response ?

Thanks for your review @jimma! Sure. It makes sense to me, will have a look.

[fabiobrz]

Hi @jimma - some considerations/questions after a quick look:

* Is it possible that we use jdk http client or url connection in `IstioAssistant`, then we don't need this okhttpclient dependency anymore?

Are there any reasons for your question to address just IstioAssistant? We are creating instances via OkHttpClientFactoryin several places, what about them?

* For the  okhttpclient in the test , can we simply replace it with `restassured' to easily check the json response ?

Here too, is this just about some tests or all the tests?

[jimma]

Hi @jimma - some considerations/questions after a quick look:

* Is it possible that we use jdk http client or url connection in `IstioAssistant`, then we don't need this okhttpclient dependency anymore?

Are there any reasons for your question to address just IstioAssistant? We are creating instances via OkHttpClientFactoryin several places, what about them?

I think less dependency is better. I only looked at IstioAssitant and didn't go through all classes which imports OKHttpClient class.

* For the  okhttpclient in the test , can we simply replace it with `restassured' to easily check the json response ?

Here too, is this just about some tests or all the tests?

I mean all the tests which relies on okttpclient to check the response. Is it doable ?

[fabiobrz]

Hi @jimma - some considerations/questions after a quick look:

* Is it possible that we use jdk http client or url connection in `IstioAssistant`, then we don't need this okhttpclient dependency anymore?

Are there any reasons for your question to address just IstioAssistant? We are creating instances via OkHttpClientFactoryin several places, what about them?

I think less dependency is better. I only looked at IstioAssitant and didn't go through all classes which imports OKHttpClient class.

Ok, so I'll figure out what it means to replace it with another implementation (e.g.: the JDK HttpClient).

* For the  okhttpclient in the test , can we simply replace it with `restassured' to easily check the json response ?

Here too, is this just about some tests or all the tests?

I mean all the tests which relies on okttpclient to check the response. Is it doable ?

Sure thing, that's actually one straightforward use case.

Hopefully I'll be able to get back soon about this, thanks.

[fabiobrz]

Hi @jimma - I made an attempt to address your latest remarks:

1. OkHttpClient has been replaced by JdkHttpClient in functional code, we only have one TBD here, regarding cookies manipulation when using a proxy, see https://github.com/fabiobrz/arquillian-cube/blob/2.0.x-bump-fabric8-692/openshift/openshift/src/main/java/org/arquillian/cube/openshift/impl/fabric8/F8Proxy.java#L73
2. OkHttpClient has been replaced by rest-assured where possible, the only exception is istio tests that use IstioAssistant for awaiting responses, since its methods expect for java.net request/response arguments now.

[jimma]

@fabiobrz Thanks for this improvement. This looks good to merge.

[jimma]

Hi @jimma - I made an attempt to address your latest remarks:

1. OkHttpClient has been replaced by JdkHttpClient in functional code, we only have one TBD here, regarding cookies manipulation when using a proxy, see https://github.com/fabiobrz/arquillian-cube/blob/2.0.x-bump-fabric8-692/openshift/openshift/src/main/java/org/arquillian/cube/openshift/impl/fabric8/F8Proxy.java#L73

I think JDK http client can support this too. Is there test or function requires this cookie thing ?

2. OkHttpClient has been replaced by rest-assured where possible, the only exception is istio tests that use IstioAssistant for awaiting responses, since its methods expect for java.net request/response arguments now.

Could you please point me the test code for this exception ? Thanks.

[fabiobrz]

Hi @jimma

I think JDK http client can support this too. Is there test or function requires this cookie thing

Yes, there should be a CookieHandler/Manager or something similar.

Is there test or function requires this cookie thing ?

Yep, it seems to be used by the OpenShift client adapter via F8Proxy in the end... I'll have to look closer though.

Could you please point me the test code for this exception ?

Yup, see here, and here. As you can see the await would have to be modified to work with rest assured API, and it doesn't seem the case to me, since IstioAssistant is not a test component.

[jimma]

@fabiobrz sorry for the late reply. These are all very minor issues. We can create some todo task for these things and merge your change. WDYT ?

[fabiobrz]

@fabiobrz sorry for the late reply. These are all very minor issues. We can create some todo task for these things and merge your change. WDYT ?

+1, thanks @jimma - I have filed #1297 for the cookie configuration TBD, while I actually think we should not do anything regarding IstioAssistant APIs, since those -being part of the functional implementation - should really not use rest-assured.

Feel free to merge, in case you don't have any objections.