search

arquillian / arquillian-cube

Control (docker, kubernetes, openshift) containers in your tests with ease!
http://arquillian.org/arquillian-cube/
117 stars 98 forks source link

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

Closed fabiobrz closed 3 months ago

fabiobrz commented 7 months ago

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:

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

fabiobrz commented 5 months ago

  • 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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 4 months ago

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 commented 4 months ago

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 ?

  1. 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 commented 4 months ago

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 commented 3 months ago

@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 commented 3 months ago

@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 https://github.com/arquillian/arquillian-cube/issues/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.