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

Feature: tight up the integration with extra eventSources

Open andreaTP opened this issue 3 years ago • 8 comments

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

there are 2 improvements we can investigate:

  • the watched resource should be easily accessible in the reconcile loop
  • the generated Roles/RoleBindings etc. should take into account those resources too

andreaTP avatar Jan 13 '22 12:01 andreaTP

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.

csviri avatar Jan 13 '22 12:01 csviri

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.

andreaTP avatar Jan 13 '22 12:01 andreaTP

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

csviri avatar Jan 13 '22 12:01 csviri

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.

metacosm avatar Jan 13 '22 13:01 metacosm

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 avatar Jan 13 '22 13:01 csviri

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.

csviri avatar Jan 13 '22 13:01 csviri

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.

metacosm avatar Jan 13 '22 13:01 metacosm

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.

andreaTP avatar Jan 13 '22 14:01 andreaTP