quarkus-operator-sdk icon indicating copy to clipboard operation
quarkus-operator-sdk copied to clipboard

Allow DI for condition and discriminator classes

Open Javatar81 opened this issue 2 years ago • 13 comments

There are several attributes that refer to a singleton object represented by its Class, e.g. conditions and discriminators. Examples are:

@KubernetesDependent(resourceDiscriminator = MyDiscriminator.class)

@Dependent(reconcilePrecondition = MyReconcilepreondition.class)

@Dependent(readyPostcondition = MyReadyPostcondition.class)

@Dependent(deletePostcondition = MyDeletePostcondition.class)

There is no way to inject additional dependencies into these classes since they are always instantiated via their default constructor. It would be useful to support attribute and constructor injection via CDI.

One use case is to inject the OpenShiftClient into a reconcile precondition class to implement checks agains the OCP API.

Javatar81 avatar Sep 21 '23 16:09 Javatar81

It would be also useful to support constructor based injection 🙂 Another use case is to use config properties, that are exposed as a bean.

AleksanderBrzozowski avatar Feb 27 '24 09:02 AleksanderBrzozowski

It would be also useful to support constructor based injection 🙂 Another use case is to use config properties, that are exposed as a bean.

Could you elaborate, please?

metacosm avatar Feb 27 '24 14:02 metacosm

Yes, of course.

So, if we have a configuration like below:

Dependent(type = SomeResource::class)

Then, constructor based injection won't work (tested with quarkus-operator-sdk 6.3.0):

java.lang.RuntimeException: java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.arc.deployment.ArcProcessor#generateResources threw an exception: jakarta.enterprise.inject.spi.DeploymentException: It's not possible to automatically add a synthetic no-args constructor to an unproxyable bean class. You need to manually add a non-private no-args constructor to com.mypackage.SomeResource in order to fulfill the requirements for normal scoped/intercepted/decorated beans.
	at io.quarkus.arc.processor.Beans.cannotAddSyntheticNoArgsConstructor(Beans.java:957)
	at io.quarkus.arc.processor.Beans.validateBean(Beans.java:762)
	at io.quarkus.arc.processor.BeanInfo.validate(BeanInfo.java:605)
	at io.quarkus.arc.processor.BeanDeployment.validateBeans(BeanDeployment.java:1547)
	at io.quarkus.arc.processor.BeanDeployment.validate(BeanDeployment.java:481)
	at io.quarkus.arc.processor.BeanProcessor.validate(BeanProcessor.java:164)
	at io.quarkus.arc.deployment.ArcProcessor.validate(ArcProcessor.java:471)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:864)
	at io.quarkus.builder.BuildContext.run(BuildContext.java:282)
	at org.jboss.threads.ContextHandler$1.runWith(ContextHandler.java:18)
	at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2513)
	at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1538)
	at java.base/java.lang.Thread.run(Thread.java:833)
	at org.jboss.threads.JBossThread.run(JBossThread.java:501)

It seems to be failing even in build phase 🤔

And for the second part of the question - it is not possible to inject when using readyPostcondition. Even if there is an inject annotation, the property is still null.

So from my point of view, it would be nice if any property of @Dependent annotation that requires class would support injection (constructor based injection). Not sure if it is possible with the actual architecture of the Operator SDK.

Any thoughts?

AleksanderBrzozowski avatar Feb 27 '24 14:02 AleksanderBrzozowski

JOSDK should allow conditions and discriminators to be CDI beans, i.e. if the referenced class, let's say MyDiscriminator.class is annotated with e.g. @ApplicationScoped the @Dependent registration process should pick up the singleton instance provided by CDI and must not instantiate it itself (though this could still be the case if the class is not an CDI bean). This would enable field and constructor injection.

Javatar81 avatar Feb 27 '24 15:02 Javatar81

Just a small remark, discriminators are probably going away in v5: https://github.com/operator-framework/java-operator-sdk/pull/2252 https://github.com/operator-framework/java-operator-sdk/issues/2253

csviri avatar Feb 27 '24 19:02 csviri

@Javatar81 would the approach described in https://github.com/operator-framework/java-operator-sdk/pull/2252 work in your use case? I'm personally still on the fence on whether or not we should remove discriminators altogether in future releases because I think they might make sense in some optimization scenarios. However, an alternative could be to override the getSecondaryResource method on your dependent implementation so I'd be interested in hearing more about your use case and how you use discriminators.

metacosm avatar Feb 28 '24 10:02 metacosm

I am currently migrating all my discriminator to the ResourceIDMatcherDiscriminator approach. If that works, this will be a good sign that https://github.com/operator-framework/java-operator-sdk/pull/2252 would also work for me.

Javatar81 avatar Mar 11 '24 14:03 Javatar81

This makes me think that we could remove discriminators from the list for CDI managed beans. However, I think we still need it for the classes referenced in the following attributes of @Dependent:

  • reconcilePrecondition
  • readyPostcondition
  • deletePostcondition
  • type

Javatar81 avatar Mar 11 '24 15:03 Javatar81

Note that the dependent resources (the type attribute in your list) are already exposed as CDI beans.

metacosm avatar Mar 13 '24 10:03 metacosm

One issue with making the conditions beans, is that we wouldn't be able to completely resolve the workflows at build time as is currently the case, if I'm not mistaken… This is something I need to think about more.

metacosm avatar Mar 13 '24 14:03 metacosm

Another issue is that all these classes have the same type so I'm not sure how resolution would work since we would probably need to add a qualifier to distinguish them, which starts to make things rather complicated. Maybe I'm not just seeing well how things would work in that scenario…

metacosm avatar Mar 13 '24 16:03 metacosm