operator-framework / java-operator-sdk

Java SDK for building Kubernetes Operators
https://javaoperatorsdk.io/
Apache License 2.0
776 stars 211 forks source link

Feature: tight up the integration with extra eventSources #826

Open andreaTP opened 2 years ago

andreaTP commented 2 years ago

A controller can define additional eventSources as shown e.g. here

there are 2 improvements we can investigate:

csviri commented 2 years ago

Hi @andreaTP 1.the watched resource is accessible in multiple way, if you hold reference to the informer (variable), you can just pass read it using the resource id of the custom resource, see: https://github.com/java-operator-sdk/java-operator-sdk/blob/001cbaf38bc91332f0bced22649e3e7a465857ee/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java#L152

or the CR:

https://github.com/java-operator-sdk/java-operator-sdk/blob/001cbaf38bc91332f0bced22649e3e7a465857ee/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java#L141

You can also use the get it using the context: <T> Optional<T> getSecondaryResource(Class<T> expectedType)

  1. this is now part of quarkus extension, not sure why was it implemented there. Maybe @metacosm can elaborate.
andreaTP commented 2 years ago

if you hold reference to the informer

have you already evaluated to expose this functionality from the Context? it might result in a nicer UX I think.

csviri commented 2 years ago

sure, currently its limited, since the dependent resources might have impact on this, for now see:

https://github.com/java-operator-sdk/java-operator-sdk/blob/001cbaf38bc91332f0bced22649e3e7a465857ee/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/Context.java#L9-L13

metacosm commented 2 years ago

Hi @andreaTP 1.the watched resource is accessible in multiple way, if you hold reference to the informer (variable), you can just pass read it using the resource id of the custom resource, see:

https://github.com/java-operator-sdk/java-operator-sdk/blob/001cbaf38bc91332f0bced22649e3e7a465857ee/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java#L152

or the CR:

https://github.com/java-operator-sdk/java-operator-sdk/blob/001cbaf38bc91332f0bced22649e3e7a465857ee/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java#L141

Generally speaking, I think it's a bad pattern to expose the underlying implementation if we can avoid it. We switched from Watcher to Informer, we might switch to something else again. We shouldn't have to expose this to the users.

Similarly, we now send a list of event sources to be registered and people shouldn't hold onto them unless really needed. Event sources are an implementation detail that people shouldn't really have to deal with most of the time. See the dependent resource work (#785) to see how things could be improved (imo, at least! 😅)

You can also use the get it using the context: <T> Optional<T> getSecondaryResource(Class<T> expectedType)

That's how it should secondary resources should be retrieved, imo.

2. this is now part of quarkus extension, not sure why was it implemented there. Maybe @metacosm can elaborate.

No this isn't at the moment.

csviri commented 2 years ago

No this isn't at the moment.

Maybe we should create a separate issue for this, and discuss where this should be done, not sure if this is quarkus extension, then rather here in the JOSDK, or actually by Operator SDK Plugin.

csviri commented 2 years ago

Generally speaking, I think it's a bad pattern to expose the underlying implementation if we can avoid it. We switched from Watcher to Informer, we might switch to something else again. We shouldn't have to expose this to the users.

This is true for K8S resource, we can make a good API to access those, the non-k8s resources is a different story, for now we created the event sources which should be the base for those, to see how we could make a unification. See the CachingEventSource and other event sources which extends it.

metacosm commented 2 years ago

No this isn't at the moment.

Maybe we should create a separate issue for this, and discuss where this should be done, not sure if this is quarkus extension, then rather here in the JOSDK, or actually by Operator SDK Plugin.

Generating RBACs here would be quite complicated and duplicate work that's already done elsewhere. Not sure about the plugin. RBACs are already generated mostly for "free" in the Quarkus extension, we already augment what is generated by default there. We could indeed extract more information from the event sources (and dependent resource definitions when we have them). Created quarkiverse/quarkus-operator-sdk#189 to investigate this.

andreaTP commented 2 years ago

I think it's a bad pattern to expose the underlying implementation if we can avoid it.

That's a good comment, we should be able to register "Watched resources" and they should be easily accessible from the public API without bothering about the implementation details.