Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow DI for condition and discriminator classes #720

Open
Javatar81 opened this issue Sep 21, 2023 · 13 comments
Open

Allow DI for condition and discriminator classes #720

Javatar81 opened this issue Sep 21, 2023 · 13 comments
Assignees

Comments

@Javatar81
Copy link

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.

@AleksanderBrzozowski
Copy link

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

@metacosm metacosm self-assigned this Feb 27, 2024
@metacosm
Copy link
Member

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?

@AleksanderBrzozowski
Copy link

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?

@Javatar81
Copy link
Author

Javatar81 commented Feb 27, 2024

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.

@csviri
Copy link
Contributor

csviri commented Feb 27, 2024

Just a small remark, discriminators are probably going away in v5:
operator-framework/java-operator-sdk#2252
operator-framework/java-operator-sdk#2253

@metacosm
Copy link
Member

@Javatar81 would the approach described in operator-framework/java-operator-sdk#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.

@Javatar81
Copy link
Author

Javatar81 commented Mar 11, 2024

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

@Javatar81
Copy link
Author

Javatar81 commented Mar 11, 2024

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

@metacosm
Copy link
Member

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

@metacosm
Copy link
Member

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
Copy link
Member

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
Copy link
Member

@Javatar81 @AleksanderBrzozowski is this still something you are interested in? If yes, could you provide more details as to what you'd like to be able to do more specifically, in particular with respect to how CDI would handle the fact that conditions all share the same base type?

@AleksanderBrzozowski
Copy link

So, what we do is we have a class which extends from BulkDependentResource (and some other types as well).
The class needs access to the application properties - which are currently injected.

For the condition to pass, the properties are not needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants