Make sure http2 is not tried

[geoand]

This is prevented because it breaks the mock-webserver in Java 11

The actual problem with mock-webserver seems to be an okhttp issue, but we can easily work around it here by ensuring that http2 is never tried (kubernetes-api server isn't http2 anyway so there no point in even attempting to use http2).

This is not a problem in Java 8 because the default installations of Java 8 don't support http2

[centos-ci]

Can one of the admins verify this patch?

[iocanel]

It somehow feels wrong, to disable something, just because it breaks a testing tool or framework.

I would conditionally disable it, when its used along with the mockwebserver.
The wrapper objects that do create the mock server for Kubernetes and Openshift are also responsible for initializing the client instance. So, it should be trivial to use that option there.

AFAIK, you can pass an instance of the Okhttp to the client constructor, so we could use it for this purpose.

[geoand]

How would we conditionally disable it?

As for the other instances, we could make the change to the other uses of OkHttpClient.Builder.

[iocanel]

See this constructor: https://github.com/fabric8io/kubernetes-client/blob/master/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/DefaultKubernetesClient.java#L96 ?

Use this constructor to pass an instance of okhttp when creating an instnace of the client here: https://github.com/fabric8io/kubernetes-client/blob/master/kubernetes-server-mock/src/main/java/io/fabric8/kubernetes/client/server/mock/KubernetesMockServer.java#L85

[iocanel]

Repeat for all kinds of mock servers we provide.

[geoand]

Thanks for the pointers! I'll update the PR tomorrow hopefully

[geoand]

@iocanel it seems like limiting this to only clients from the mock is not enough.

For instance in SCK there is a client being by the framework itself. Should we perhaps add a value to Config that would be used to exclude http2 (HttpClientUtils would need to take this property into account obviously)?
That way just setting a system property would be enough to make all clients work.

WDYT?

[iocanel]

That sounds great! But why do we care about sck? Isn't the issue limited to the mockserver?

…

On Tue, Jul 30, 2019 at 10:30 AM Georgios Andrianakis < ***@***.***> wrote: @iocanel <https://github.com/iocanel> it seems like limiting this to only test clients is not enough. For instance in SCK there is a client being by the framework itself. Should we perhaps add a value to Config that would be used to exclude http2 (HttpClientUtils would need to take this property into account obviously)? That way just setting a system property would be enough to make all clients work. WDYT? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1671?email_source=notifications&email_token=AADCEWFMNJAZGJ6UOTT6AQLQB7UZVA5CNFSM4IHUUFXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3DB6MI#issuecomment-516300593>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADCEWATAS3GZ5KTOYLIGE3QB7UZVANCNFSM4IHUUFXA> .

-- *Ioannis Canellos <http://iocanel.com>*

[geoand]

The issue is limited to the mockserver, but it manifests itself in SCK.
Basically when any of the clients tries to establish a connection and http2 is checked, the mockserver seems to fail completely.

[geoand]

@iocanel PR updated

[iocanel]

This pull request allows, disabling http2 for user-managed clients which is great.

For mock server managed clients, http2 can only be indirectly disabled using system properties.

This either needs to be somehow documented or needs to be the default (since we know that the mockwebserver doesn't currently support http2).

[geoand]

Great point. I'll do tha latter if that's OK with you .

I had initially fixed the configuration for the mock, but for some stupid reason I though it would be better to remove it :(

[geoand]

@iocanel PR updated again

[geoand]

SCK tested with this PR (and adding the new property to the tests) and all tests now pass with Java 11

[geoand]

@iocanel any idea why test and license-check CI jobs seem to be stuck?

[rohanKanojia]

ok to test

[rohanKanojia]

we need to whitelist PRs on fabric8 ci

[geoand]

thanks @rohanKanojia !

[iocanel]

add to whitelist

[geoand]

CI is green 🎉

[rohanKanojia]

[merge]