operator-framework/java-operator-sdk

LocallyRunOperatorExtension issue with loading CRD from file

Opened this issue · 4 comments

Hi, I found something a bit unexpected while integration testing.

The use case:

We want to integrate with a third party, optionally setting up an Informer Event Source if the third party CRD is present in kubernetes. The third party (Strimzi) offers an API module that contains fabric8 CustomResources and CustomResourceDefinitionss, but not any CRD yaml manifests.

The problem

To integration test this I want to use LocallyRunOperatorExtension to additionally install one of these Custom Resource Definitions.

I cannot use LocallyRunOperatorExtension.builder().withAdditionalCustomResourceDefinition(Kafka.class) as their module does not contain CRD files in META-INF, and JOSDK looks like it would want them in a specific naming convention too.

I can take the relevant CustomResourceDefinition and serialize it to a temporary file and attempt to apply it with LocallyRunOperatorExtension.builder().withAdditionalCRD("/path/to/crd). But it appears that the LocallyRunOperatorExtension applies the CRD after it registers the Reconciler, so the CRD is not applied at the point we are trying to prepare the Event Sources.

Here's a minimal reproducer based off of io.javaoperatorsdk:bootstrapper:5.1.4:create

The reproducer also demonstates a workaround, using a fabric8 KubernetesClient from an @BeforeAll and @AfterAll to directly apply the CustomResourceDefinition from the module.

Ideas/Questions

  1. Would you be open to supporting the fabric8 CustomResourceDefinition directly? Like adding a withAdditionalCustomResourceDefinition(CustomResourceDefinition definition) type method and applying them at the same time as the Class based one.
  2. Why are the CRDs passed with withAdditionalCustomResourceDefinition and withAdditionalCRD applied at different times? Could they all be applied before the Reconciler is registered?

Thanks!
I'm happy to take a crack at making those changes

Hi @robobario , thank you for the issue!

  1. I'm totally fine with that, it would be great
  2. That does not seems to be right to me either, maybe @metacosm or @xstefank remembers the details why it was done like that. Feel free to improve that.

Thank you!

I wasn't around for 2. yet :). But both your points make sense to me and if you can contribute these changes, it would be great.

Thanks, there is a bit of other code around the withAdditionalCRD, like you can call applyCrd on the extension and it will pull the relevant CRD out of this collection and apply it. Makes me worry there are dark and ancient reasons it's set up like this. I'll look at point 1 first in any case.

I don't recall the details but the changes are related to #2561 and #2658 (and related issues) so whatever is done to address your issue should also take these ones into account.