API-1802: cert-rotation: allow specifying multiple target certs in CertRotationController #1722
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
vrutkovs
force-pushed
the
cert-rotation-multiple-targets
branch
2 times, most recently
from
9130e0c
to
d10d787
Compare
vrutkovs
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
vrutkovs
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@vrutkovs: This pull request references API-1802 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.16.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/cc @tkashem @p0lyn0mial |
There was a problem hiding this comment.
a) RotatedSigningCASecret creates the signer CA secret
b) CABundleConfigMap creates a configmap, and
c) RotatedSelfSignedCertKeySecret creates a secret
I like the idea of a controller doing one thing, can we explore the idea of the individual controllers?
a) SignerCAController: this controller manager the signer secret object.
b) CABundleController: it watched the secret object from a and creates a single configmap and manages it.
c) CertKeySecretController: this can watch objects from a and b and creates a secret with cert/key and manages it.
With this, we can have N instances of CertKeySecretController, where each instance derives it cert/key from a single instance of a and b
| WithPostStartHooks( | ||
| c.targetCertRecheckerPostRunHook, | ||
| ). | ||
| ToController("CertRotationController", recorder.WithComponentSuffix("cert-rotation-controller").WithComponentSuffix(name)) |
There was a problem hiding this comment.
MultipleTargetCertRotationController, so we have two distinct names?
| } | ||
| }(ch) | ||
| } | ||
|
|
There was a problem hiding this comment.
I would like to avoid making any runtime behavioral changes to NewCertRotationController if possible, we have the following options:
a) completely separate implementations: CertRotationController for NewCertRotationController, and MultipleTargetCertRotationController for NewCertRotationControllerMultipleTargets
b) abstract out the targetCertRecheckerPostRunHook implementations: singleTargetCertRecheckerPostRunHook and multiTargetCertRecheckerPostRunHook. This way we can reuse CertRotationController for both single and multiple. NewCertRotationControllerMultipleTargets will use multiTargetCertRecheckerPostRunHook.
c) can we have a single channel <-chan time.Time to be shared by multiple instances of CertCreator (for example ServingRotation), then the logic inside targetCertRecheckerPostRunHook does not need to change at all.
I prefer c, if doable
There was a problem hiding this comment.
Reworked this to make it look like b). Added a test which verifies goroutines don't leak
I don't quite understand what c) is meant for - make controller accept a single channel reused across all CertCreators? Not sure what's the benefit of that
| targetRefresh := refresher.RecheckChannel() | ||
| aggregateTargetRefresher := make(chan struct{}) | ||
| for _, ch := range targetRefreshers { | ||
| go func(c <-chan struct{}) { |
There was a problem hiding this comment.
go func does not have the same guarantee as go wait.Until(func() {}, time.Minute, ctx.Done())
| for _, ch := range targetRefreshers { | ||
| go func(c <-chan struct{}) { | ||
| for msg := range c { | ||
| aggregateTargetRefresher <- msg |
There was a problem hiding this comment.
goroutine leaking: <-ctx.Done(), could be a problem for integration tests that check goroutine leakages?
There was a problem hiding this comment.
ctx.Done would close them iiuc, but yeah, worth adding a unit test which uses runhook and ensures no goroutines are leaking
That's possible, but the goal is to create target certs, signers and CA are merely prerequisites to it. We could have separate signer/ca controllers, but we might end up with signer certs not producing any target certs or CA bundles without any new signers etc. |
vrutkovs
force-pushed
the
cert-rotation-multiple-targets
branch
from
d10d787
to
a528215
Compare
|
New changes are detected. LGTM label has been removed. |
vrutkovs
force-pushed
the
cert-rotation-multiple-targets
branch
from
a528215
to
d3c0949
Compare
|
|
||
| // Ensure both target certs have been called exactly three times | ||
| // initial sync and two hook calls for target certs | ||
| // TODO[vrutkovs]: informers make unpredictable number of calls |
There was a problem hiding this comment.
Not sure how to tackle that - or how to make sure two hook syncs were included in regular informer sync. informerFactory and NewCertRotationControllerMultipleTargets promise to sync every minute, but it happens much more often
|
An alternative design would be to have a controller per certificate – this would actually preserve the current behaviour. I prefer having a single controller per certificate as it is easier to debug, report status, retry on error, and reason about. Thoughts? Our issue is that both the I think the least invasive change would be to make both Another idea that comes to mind (I think Abu suggested the same) is to turn both |
Similar to Abu's idea in #1722 (review)? That would prevent races, but would make sequence of events to do a proper rollout complicated (three different controllers would need to be properly synced so that CA bundle would be updated before target cert etc.). Also may lead to "orphan" controllers managing CA bundle without a target cert.
Yes, this issue still potentially remains. The PR focuses on solving a much more widespread issue of multiple target certs
I don't know if its feasible, as its not just thread-safety but also process-safety we're concerned about |
Don't we do it all the time? The aggregator controller waits until a service is created before it wires an HTTP handler. What I like about these and other controllers is that when you look inside, you will see that these controllers are reconciling a single resource. They are simply reading their prerequisites and reacting to any changes before reconciling. In our case, it would boil down to three separate controllers that reconcile their resources, where the second and the third controller read the crypto material from the lister before reconciling. For example: NewCertRotationController would: When there are issues with the signerCA you go to |
Yes - this is what this controller is doing in the end. So adding three more controllers for each resource won't magically solve anything.
It also needs
otherwise it would rely on "lets wait a minute until next sync to catch", which means signer is already updated by CA is not - so its unclear when other components are allowed to use the updated signer This won't qualify as a simple bugfix two weeks before feature freeze - this is a full blown rework tbf |
Having a single controller for reconciling a resource would solve the race condition we are facing.
yes, usually controllers react to changes made to other resources and we should do the same.
yes, I agree it would require more code. The race we are facing is not new, it has been introduced a long time ago and it wasn't obvious. I think that having a controller per resource leads to simpler program in the end. |
|
Discussed on the meeting - this looks good enough for 4.17, later need to be reworked into more granular controllers, CA bundle controller should support handling multiple signers, |
| go wait.Until(func() { | ||
| for { | ||
| select { | ||
| case <-refresher.RecheckChannel(): |
There was a problem hiding this comment.
so if we have N targets, that means N goroutines, each of which is waiting to receive from a channel to be notified of an event.
The producer ( the entity that writes to a channel) will probably write once when the host name changes. it is also possible that a channel is shared among the N instances depending on how we instantiate the controller.
Let's say we have three targets, {a, b, and c}; so goroutine for c when it receives from its channel it will add a key to the queue, which will trigger the sync method, and the sync method applies to all targets. It's not a huge issue, but it seems to be an unnecessary complexity we can avoid?
a couple of questions with this mechanism:
- what should happen when the channel is closed, should we treat this an a
event occurred? probably not, maybe we should check withcase _, ok := <-refresher.RecheckChannel()? - the controller syncs every
1m, do we really need this channel based mechanism to trigger sync, are we losing anything other than 'few seconds of promptness' if we don't have this channel based trigger?
If we really need this mechanism, i would suggest abstracting this mechanism:
// this abstracts the work queue used by the controller
type queueAdder interface {
Add()
}
type channelBasedTrigger struct {
ch chan time.Time
queue queueAdder // or we can even use 'f func() {}'
}
func (t channelBasedTrigger) Event() { t.ch <- time.Now() } // used by the producer
func (t channelBasedTrigger) Run(ctx context.Context) {
go wait.Until(func() {
for {
select {
case t, ok := <-t.ch:
if ok {
t.queue.Add() // or f()
}
case <-ctx.Done():
return
}
}
}, time.Minute, ctx.Done())
}
This way we can keep the logic of channel based trigger mechanism co-located, more testable, and more generic too. Also, we can use one instance of channelBasedTrigger to inform N controllers.
There might be some constructs in the apimachinery/util/wait package that will allow us to achieve this.
There was a problem hiding this comment.
what should happen when the channel is closed, should we treat this an a event occurred?
iiuc both struct implementing TargetCertRechecker interface and the controllers are created in NewCertRotationController, so the channel is never closed when the controller is still running.
If we really need this mechanism, i would suggest abstracting this mechanism:
So far we use it in just one ServingRotation struct, so probably there is an easier way out
|
@vrutkovs Do we know exactly where the race condition occurs? Also, it would be nice to have a unit test so that we can validate whether the fix we are going to apply works. I’ve opened #1753 as an alternative to this PR. |
Usually its various CA bundles being updated incorrectly, especially those which are used in several targets.
I don't think its alternative really, we can have both. 1753 alone won't solve it (you still need a process-safe way for it) |
@vrutkovs could you be more specific ? Do we know what exactly is broken ? I have checked the code for updating the CA bundles and it looks good. |
See this job history - it intermittently passes or fails. This job is a good example. Here kube-apiserver won't start as So and kube-apiserver log is full of
The code is correct, but when its not thread-safe and also doesn't protect from several processes updating the same CA configmap. This PR would eliminate process races, #1753 would ensure |
this is the part I would like to understand deeper, the updating code seems to be removing duplicates and expired certificates. I mean, I understand how the code can lose a new certificate but i don't understand how could it drop the old certificates (non expired). |
Both PR seem to be removing the race within a single process. I still thing the best way forward would be to create separate controllers. |
I would rather bet on the apply to be the culprit here: Just to also leave a passive aggressive (wink) reminder that I would also like to see our tests going green as a confirmation our side works correctly to expedite this whole story... My two cents on the overall discussion so far: we've tried to get away from the CertRotationController very early on because we needed multiple signers and multiple leaf certificates per signer in etcd. We figured it's also cheap enough to always re-create the leaf cert configuration entirely by node listing: Which invalidates the need for I personally find the procedural and explicit one-to-many easier to debug, I don't quite get why you need to multi-thread those with goroutines or even need multiple control loops. How many leaf certs do you expect per signer CA that requires to parallelize and lock the reconciliation across the bundles? |
Correct, this code removes expired signers - but previous signer has not expired yet. Its being refreshed at 80% lifetime, so it still has 20% of a lifetime to be active (its 2.5 month of a year validity).
Sure, this PR is just interim fix for us to make it until 4.17 feature freeze. Once we establish one way of process and thread safety - and have e2e tests passing - we can experiment with more substantial code rework |
We don't, no one is happy about current codebase. However I'd prefer to stabilize existing codebase before performing any significant rework |
It is not about multi-threading for performance reasons. It is about having a single control loop per resource. I think this is already a well-established pattern upstream. I think that having a single controller that manages a resource is easy to understand and debug.
This is a crucial piece of code, and I wouldn't rush it. Besides, we still need to fix kubelet, client-go, and tons of other things before the platform will be able to recover itself from expired certificates. Thus, I don't see the point in developing temporary solutions. We are not dealing with an escalation that requires an immediate fix. I would rather implement a proper fix or not fix it at all. |
so you propose to rewrite the entire codebase to fit some upstream pattern? :) sounds great 👍 |
I'm proposing wrapping The new Then, when we need to compose these controllers we would only have a singe instance of Does it make sense to you as well ? |
This is what we agreed to few weeks back and noone is debating that choice. The immediate question is why all of this is being discussed in an unrelated PR with a temporary fix for 4.17 - and why is it being stalled for several weeks already |
We kind of have to. If the rework is significant and 4.18 branches we won't be able to backport it.
For indefinite suspend period - yes. For 90 days / 1 year on SNO - no, not really, we already recover with approved manual steps.
We do, this feature (limited to 90 days etc.) is on 4.17 plan |
vrutkovs
force-pushed
the
cert-rotation-multiple-targets
branch
from
d3c0949
to
126e202
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dinhxuanvu, vrutkovs The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…Controller Instead of defining several controllers managing the same signer/CA bundle pair and different target certs the same controller can accept a list of target certs to create.
vrutkovs
force-pushed
the
cert-rotation-multiple-targets
branch
from
126e202
to
b2298d8
Compare
vrutkovs
force-pushed
the
cert-rotation-multiple-targets
branch
from
b2298d8
to
6a2734e
Compare
|
@vrutkovs: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Instead of defining several controllers managing the same signer/CA bundle pair and different target certs the same controller can accept a list of target certs to create.
Tested with
workflow-test openshift-e2e-cert-rotation-suspend-sno 4.17,https://github.com/openshift/cluster-kube-apiserver-operator/pull/1669,https://github.com/openshift/cluster-etcd-operator/pull/1284 "CLUSTER_AGE_DAYS=300","CLUSTER_AGE_STEP=300","PACKET_OS=rocky_9"workflow-test openshift-e2e-cert-rotation-suspend-sno 4.17,https://github.com/openshift/cluster-kube-apiserver-operator/pull/1669,https://github.com/openshift/cluster-etcd-operator/pull/1284 "CLUSTER_AGE_DAYS=600","CLUSTER_AGE_STEP=300","PACKET_OS=rocky_9"workflow-test openshift-e2e-cert-rotation-suspend-sno 4.17,https://github.com/openshift/cluster-kube-apiserver-operator/pull/1669,https://github.com/openshift/cluster-etcd-operator/pull/1284 "CLUSTER_AGE_DAYS=900","CLUSTER_AGE_STEP=300","PACKET_OS=rocky_9"workflow-test openshift-e2e-cert-rotation-suspend-sno 4.17,https://github.com/openshift/cluster-kube-apiserver-operator/pull/1669,https://github.com/openshift/cluster-etcd-operator/pull/1284 "CLUSTER_AGE_DAYS=1200","CLUSTER_AGE_STEP=300","PACKET_OS=rocky_9"workflow-test openshift-e2e-cert-rotation-suspend-sno 4.17,https://github.com/openshift/cluster-kube-apiserver-operator/pull/1669,https://github.com/openshift/cluster-etcd-operator/pull/1284 "CLUSTER_AGE_DAYS=1500","CLUSTER_AGE_STEP=300","PACKET_OS=rocky_9"workflow-test openshift-e2e-cert-rotation-suspend-sno 4.17,https://github.com/openshift/cluster-kube-apiserver-operator/pull/1669,https://github.com/openshift/cluster-etcd-operator/pull/1284 "CLUSTER_AGE_DAYS=1800","CLUSTER_AGE_STEP=300","PACKET_OS=rocky_9"workflow-test openshift-e2e-cert-rotation-suspend-sno 4.17,https://github.com/openshift/cluster-kube-apiserver-operator/pull/1669,https://github.com/openshift/cluster-etcd-operator/pull/1284 "CLUSTER_AGE_DAYS=2100","CLUSTER_AGE_STEP=300","PACKET_OS=rocky_9"workflow-test openshift-e2e-cert-rotation-suspend-sno 4.17,https://github.com/openshift/cluster-kube-apiserver-operator/pull/1669,https://github.com/openshift/cluster-etcd-operator/pull/1284 "CLUSTER_AGE_DAYS=2400","CLUSTER_AGE_STEP=300","PACKET_OS=rocky_9"