Feat: add internal cert to replace cert manager #265
Conversation
k8s-ci-robot
added
kind/feature
k8s-ci-robot
added
the
size/L
kerthcet
force-pushed
the
feat/add-internal-cert
branch
from
9b011a1
to
67c65c7
Compare
kerthcet
force-pushed
the
feat/add-internal-cert
branch
3 times, most recently
from
b7a6194
to
6bf71a4
Compare
apis/kueue/v1alpha1/webhook.go
Outdated
| serviceName = "kueue-webhook-service" | ||
| secretName = "kueue-webhook-server-cert" |
There was a problem hiding this comment.
you should be passing these through the component config that is populated by kustomize https://github.com/kubernetes-sigs/kueue/blob/main/config/manager/controller_manager_config.yaml
Although we can leave it for a follow up.
main.go
Outdated
| } | ||
|
|
||
| setupProbeEndpoints(mgr, certsReady) | ||
| go startControllers(mgr, cCache, queues, certsReady, config.ManageJobsWithoutQueueName) |
There was a problem hiding this comment.
controllers are blocked by certsReady
Lines 157 to 161 in 6bf71a4
There was a problem hiding this comment.
why not do that in the main routine? Otherwise, mgr.Start in the main routine could be called before all the controllers are registered on it.
There was a problem hiding this comment.
Cert won't be ready until manager starts, add a comment here.
Controllers can be added after manager starts and will started immediately. See here:
https://github.com/kubernetes-sigs/controller-runtime/blob/0c2effbc7eabc502eb79472b6c6ba8fbb6ec8b76/pkg/manager/runnable_group.go#L49-L69
There was a problem hiding this comment.
What's the point of starting an empty manager?
There was a problem hiding this comment.
We register a cert controller to the manager by cert.ManageCerts(mgr)
There was a problem hiding this comment.
I see. Does the cert controller need to run before the certs can be ready?
There was a problem hiding this comment.
ah ok, that's the comment you added. Thanks
main.go
Outdated
| queues.CleanUpOnContext(ctx) | ||
| }() | ||
|
|
||
| startScheduler(ctx, mgr, cCache, queues, certsReady) |
There was a problem hiding this comment.
it's just two lines, why a function?
There was a problem hiding this comment.
Just like startControllers, hiding the details about how to start scheduler, although we have only two lines.
main.go
Outdated
| setupProbeEndpoints(mgr) | ||
| // Cert won't be ready until manager starts, so start a goroutine here which | ||
| // will block until the cert is ready before setting up the controllers. | ||
| // Controllers register after manager starts will start directly. |
There was a problem hiding this comment.
| // Controllers register after manager starts will start directly. | |
| // Controllers register after manager starts and will start directly. |
|
lgtm, can you squash? |
Signed-off-by: kerthcet <kerthcet@gmail.com>
kerthcet
force-pushed
the
feat/add-internal-cert
branch
from
9bada19
to
3a06c10
Compare
squashed. |
|
/approve |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, kerthcet The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Signed-off-by: kerthcet kerthcet@gmail.com
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #232 (comment)
Special notes for your reviewer: