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

Emit Events for ClusterTriggerAuthentication #1647

Merged
merged 3 commits into from
Mar 16, 2021

Conversation

zroubalik
Copy link
Member

@zroubalik zroubalik commented Mar 2, 2021

Signed-off-by: Zbynek Roubalik zroubali@redhat.com

Kubernetes Events haven't been emitted for ClusterTriggerAuthentication, adding a controller the sam way it is done for TriggerAuthentication. Should be enough for now

Checklist

Fixes #1523

Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
@coderanger
Copy link
Contributor

I don't really understand why we would do this? Those events aren't really useful for anything? It doesn't hurt per se but this implies you're trying to use events instead of an audit webhook for logging/recording/announcing cluster data changes.

@tomkerkhove
Copy link
Member

This can be useful outside of the scope of Kubernetes where you want to extend it or react in some other apps.
http://k8s-event-grid-bridge.io/

That's more a general remark, not for these events (although I see its value), but why not be open; maybe somebody does cool stuff with them!

@coderanger
Copy link
Contributor

It is more code to maintain and some small amount of increased load on the cluster if you are creating and deleting objects a lot. And the audit webhook system provides that kind of info for every object type, and a lot more info too (who did it, for example).

@coderanger
Copy link
Contributor

Not strongly against these but it feels like it is redundant with stuff K8s already provides better via other means :)

@zroubalik
Copy link
Member Author

I can see your point, that these events aren't that imporant. But because we are already emitting the same events for the other objects, I think that we should have unified experience and this PR fixes that. The increased load and addition code is not that imho huge, we can think of this as a first spike to an additional related functionality.

@tomkerkhove
Copy link
Member

Agreed, we can still make it an opt-out if we have the need?

@coderanger
Copy link
Contributor

Probably not even worth that, the maintenance is the bigger thing and even that is minimal. If we end up doing this for a bunch of types, we could make a shared generic version instead. That said, I'm not 100% sure this works? I don't think objects get the reconcile on delete if there isn't a finalizer to handle?

@zroubalik zroubalik added this to the v2.2 milestone Mar 16, 2021
@ahmelsayed ahmelsayed merged commit 57ce756 into kedacore:main Mar 16, 2021
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

Successfully merging this pull request may close these issues.

4 participants