-
Notifications
You must be signed in to change notification settings - Fork 3
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
KUBE-644: use informers to watch CSRs #154
Conversation
2732a6e
to
8c898b4
Compare
internal/actions/csr/csr.go
Outdated
} | ||
}, | ||
DeleteFunc: func(obj interface{}) {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this method be ommited?
internal/actions/csr/csr.go
Outdated
if cert.Approved() { | ||
continue | ||
}, | ||
UpdateFunc: func(oldObj, newObj interface{}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need both add and updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, in theory reacting just on add
should be enough. Will try to remove the update and see how it behaves.
internal/actions/csr/svc.go
Outdated
go WatchCastAINodeCSRs(ctx, log, h.clientset, c) | ||
go func() { | ||
for { | ||
if err := WatchCastAINodeCSRs(ctx, log, h.clientset, c); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why infinite loop here? If watch broke informer will handle reconnect. Did you think of other reasons?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, the informers handle the reconnect logic, but the createInformer
and informer.AddEventHandler
might fail. In such case we would end up with a running cluster-controller, but without the auto approval working.
If the context is done, then no error is returned and this goroutine quits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we initialize informer in main func and pass it here? Because if informer can't be created then there is something wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be possible, sounds like a good idea
No description provided.