Skip to content

Feature: tight up the integration with extra eventSources #826

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

Open
andreaTP opened this issue Jan 13, 2022 · 8 comments
Open

Feature: tight up the integration with extra eventSources #826

andreaTP opened this issue Jan 13, 2022 · 8 comments
Labels
feature kind/feature Categorizes issue or PR as related to a new feature.

Comments

@andreaTP
Copy link
Collaborator

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
@csviri
Copy link
Collaborator

csviri commented Jan 13, 2022

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
Copy link
Collaborator Author

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

csviri commented Jan 13, 2022

@metacosm
Copy link
Collaborator

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

csviri commented Jan 13, 2022

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

csviri commented Jan 13, 2022

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

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
Copy link
Collaborator Author

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.

@jmrodri jmrodri added kind/feature Categorizes issue or PR as related to a new feature. feature labels Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

No branches or pull requests

4 participants