add service controller for identity registry support#1951
add service controller for identity registry support#1951istio-merge-robot merged 2 commits intoistio:masterfrom crazytan:registry
Conversation
security/cmd/istio_ca/main.go
Outdated
There was a problem hiding this comment.
Is this WIP as all the handlers are empty?
Also it'd be strange if eventually all the handlers are defined here in main.go.
There was a problem hiding this comment.
I want to submit controller and registry in two PRs so I added an empty controller here.
Yes it is. In next PR registry will be created in its own package and the handlers will be defined there.
security/cmd/istio_ca/main.go
Outdated
There was a problem hiding this comment.
Just curious why this change is needed?
There was a problem hiding this comment.
I felt it's better to change it because the last line is unreachable. stopCh passed to cache controller is actually expected to be called by main(), not the controller. So <-stopCh here causes main to block forever, but I think it's clearer to just use select {} here to prevent future confusion about the stopCh usage. See https://github.com/kubernetes/client-go/blob/master/tools/cache/controller.go#L97:8
wattli
left a comment
There was a problem hiding this comment.
Thanks for working on this!
There was a problem hiding this comment.
What value does the stopCh expect? And how to send that value?
There was a problem hiding this comment.
An empty struct (struct{}{}) would suffice. Whoever is calling the Run can send the value to stopCh so that the underlying controllers are stopped. See original doc https://github.com/kubernetes/client-go/blob/master/tools/cache/controller.go#L97:8
security/cmd/istio_ca/main.go
Outdated
There was a problem hiding this comment.
Can we add some comments here to explain what it does? And if possible, please do the similar thing to the above NewSecretController as well, thanks!
There was a problem hiding this comment.
The docs are at the struct definition for both controllers.
Codecov Report
@@ Coverage Diff @@
## master #1951 +/- ##
==========================================
- Coverage 82.08% 81.77% -0.32%
==========================================
Files 198 191 -7
Lines 16320 15565 -755
==========================================
- Hits 13397 12728 -669
+ Misses 2480 2383 -97
- Partials 443 454 +11
Continue to review full report at Codecov.
|
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wattli The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. |
What this PR does / why we need it:
It adds a service controller, that is similar to secret controller and listens for events related to Service objects. Therefore another goroutine is started in CA which does nothing currently.
service_test.godemonstrates how a very simple registry should work with the controller.Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes istio/old_auth_repo#293Special notes for your reviewer:
I also removed unreachable code at the end of
main().Release note: