FLINK-36332 Add option to select the Fabric8 httpclient implemention

[SamBarker]

What is the purpose of the change

Enable the building of the operator using alternative Fabric8 http clients.

Brief change log

Enable the building of the operator using alternative Fabric8 http clients.

Verifying this change

This change is already covered by existing tests, such as (please describe tests).

The build should still work even when run with -Dfabric8.httpclinent.impl=vertx or jdk or jetty for that matter 😁

Does this pull request potentially affect one of the following parts:

• Dependencies (does it add or upgrade a dependency): yes.
• The public API, i.e., is any changes to the CustomResourceDescriptors: no
• Core observer or reconciler logic that is regularly executed: no

Documentation

• Does this pull request introduce a new feature? (yes / no)
• If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

[SamBarker]

By default kubernetes-httpclient-okhttp pulls in OkHttp 3.X which is reported to have problems with Kubernetes IPV6 stacks e.g. https://issues.apache.org/jira/browse/FLINK-31928

[gyfora]

Do you have any recommendation how we should test this and make sure it works? Maybe we can add a case to the e2e-s somehow?

[csviri]

Eventually we should move to Vertx as default, which is the preferred one in fabric8.

[SamBarker]

Do you have any recommendation how we should test this and make sure it works? Maybe we can add a case to the e2e-s somehow?

Hmm thats a good question. It looks fairly easy to add it to maven build matrix (though at what cost?) and thus test on every build. That is however of debatable value as there is no change to the projects code and thus all it would really be doing is testing a third party dependency and is that very interesting at unit/integration test level?

A more interesting angle as suggested is to include it in the e2e suite. Which again has a massive implication for the spread of tests but is probably more interesting as it validates it works against various different minikube setups.

I've added both options in different commits so we can easily drop them if they aren't considered helpful.

[SamBarker]

I think the e2e workflow is now correctly setup (and has passed on my fork

[SamBarker]

I should note I'm not convinced the workflow is doing what its expected to do I've documented my findings as FLINK-36392 which I'm happy to work on next.

I've also noticed a bunch of warnings about deprecated node version from the actions so have opened FLINK-36393 which I'm currently testing a PR for on my fork.

[SamBarker]

I think the e2e workflow is now correctly setup (and has passed on my fork

Nope its not. The includes with the http-client seem to be overriding tests and not adding an extra run as intended...

I'm tempted to suggest splitting out test_application_operations.sh as a separate "smoke test" workflow that could run with each http-client with a fixed combination of the other matrix args.

My other thought is to have a workflow per Java version as that controls the supported flink versions as well and possibly the test suite as well as some of the tests have version exclusions.

A workflow per Java version feels easier for people to reason about too, but I don't have a lot of project context or history to work from so could be missing something...

[gyfora]

@SamBarker I think it makes sense to split the test workflows. It has grown too much and it's getting a bit annoying as you can see :)

Please feel free to propose any reasonable split, I think your understanding is probably as good as anyone's at this point.

[SamBarker]

Please feel free to propose any reasonable split, I think your understanding is probably as good as anyone's at this point.

In the scope of this PR i've just pulled out a smoke test job which validates the HTTP clients can do the basics. I don't see that as being a great split for future testing of other combinations however I'd like to look at a wider restructure in a separate PR.

I'd also like to call out bb6c577 my primary motivation for adding it was to have a clearer picture of what was actually being built (in my fork) but I think its reasonable to merge. As I don't see any point in running the full job matrix if the basics aren't working (and thus saving actions minutes/ other resources). The same argument could be extended to making the smoke test dependent on the test_ci job as well, but I haven't done that here as it might slow down feedback from the edge to edge tests due to some un-related or flanky unit test failures.

[gyfora]

I think we should actually move the namespace , and java-version matrix also to the smoke_test and fix-it (remove-it) in the regular e2es.

The http-client, java-version, namespace matrix doesn't need all the different tests, it either works or it doesn't (famous last words).

[gyfora]

actually the namespace flink/default doesn't really need more than 1 test (regardless of other params...)

[SamBarker]

I think we should actually move the namespace , and java-version matrix also to the smoke_test and fix-it (remove-it) in the regular e2es.

I agree it's a bit strange. What I was intending to do was make the smallest change here and revisit it in a follow up hence the matrix of single values.

actually the namespace flink/default doesn't really need more than 1 test (regardless of other params...)

Thanks that’s the kind of insight I need for restructuring🧩.

[gyfora]

@SamBarker , how would you like to proceed with this? As long as we are adding the matrix changes I suggested in a followup , this is good from my side.

Should we squash it and merge it or would you want to add the restructuring here as a "second" commit

[SamBarker]

I'd like to get this merged and follow up with the restructuring.

Squashing seems wise 😆