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.

Sign up for GitHub

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

Jump to bottom

API-1802: cert-rotation: allow specifying multiple target certs in CertRotationController #1722

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

API-1802: cert-rotation: allow specifying multiple target certs in CertRotationController #1722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2ec0b07
cert-rotation: allow specifying multiple target certs in CertRotation…
vrutkovs Apr 19, 2024
75f7411
certrotation: add certrotation controller unittests
vrutkovs Jul 11, 2024
26bcb48
certrotationcontroller: ensure secrets/configmaps are unique
vrutkovs Jul 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
130 changes: 105 additions & 25 deletions pkg/operator/certrotation/client_cert_rotation_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ import (
"time"

operatorv1 "github.com/openshift/api/operator/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/errors"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/util/wait"

"github.com/openshift/library-go/pkg/controller/factory"
Expand Down Expand Up @@ -65,9 +68,8 @@ type CertRotationController struct {
RotatedSigningCASecret RotatedSigningCASecret
// CABundleConfigMap maintains a CA bundle config map, by adding new CA certs coming from rotatedSigningCASecret, and by removing expired old ones.
CABundleConfigMap CABundleConfigMap
// RotatedSelfSignedCertKeySecret rotates a key and cert signed by a signing CA and stores it in a secret.
RotatedSelfSignedCertKeySecret RotatedSelfSignedCertKeySecret

// RotatedTargetSecrets contains a list of key and cert signed by a signing CA to rotate.
RotatedTargetSecrets []RotatedSelfSignedCertKeySecret
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will change the external API and clients would need to update if they're accessing this field directly.

Since you're already adding a new "Create" function for the multi rotation controller, would it make more sense to have two cert controllers? One for single rotation and one for multi.

Copy link
Member Author

@vrutkovs vrutkovs Jul 23, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will change the external API and clients would need to update if they're accessing this field directly

Correct, this would require users to update the code potentially. So far we identified that only cluster-kube-apiserver-operator and cluster-kube-control-manager-operator use multitarget controllers (and they don't need to access RotatedTargetSecrets directly)

would it make more sense to have two cert controllers? One for single rotation and one for multi.

It may be beneficial later to move common parts into functions and reuse them, however at this point only two different New... functions are used externally

// Plumbing:
StatusReporter StatusReporter
}
Expand All @@ -79,14 +81,29 @@ func NewCertRotationController(
rotatedSelfSignedCertKeySecret RotatedSelfSignedCertKeySecret,
recorder events.Recorder,
reporter StatusReporter,
controlledSecrets *[]metav1.ObjectMeta,
controlledConfigMaps *[]metav1.ObjectMeta,
) factory.Controller {
c := &CertRotationController{
Name: name,
RotatedSigningCASecret: rotatedSigningCASecret,
CABundleConfigMap: caBundleConfigMap,
RotatedSelfSignedCertKeySecret: rotatedSelfSignedCertKeySecret,
StatusReporter: reporter,
Name: name,
RotatedSigningCASecret: rotatedSigningCASecret,
CABundleConfigMap: caBundleConfigMap,
RotatedTargetSecrets: []RotatedSelfSignedCertKeySecret{rotatedSelfSignedCertKeySecret},
StatusReporter: reporter,
}
*controlledSecrets = append(*controlledSecrets, metav1.ObjectMeta{
Name: rotatedSigningCASecret.Name,
Namespace: rotatedSigningCASecret.Namespace,
})
*controlledConfigMaps = append(*controlledConfigMaps, metav1.ObjectMeta{
Name: caBundleConfigMap.Name,
Namespace: caBundleConfigMap.Namespace,
})
*controlledSecrets = append(*controlledSecrets, metav1.ObjectMeta{
Name: rotatedSelfSignedCertKeySecret.Name,
Namespace: rotatedSelfSignedCertKeySecret.Namespace,
})

return factory.New().
ResyncEvery(time.Minute).
WithSync(c.Sync).
Expand All @@ -101,6 +118,56 @@ func NewCertRotationController(
ToController("CertRotationController", recorder.WithComponentSuffix("cert-rotation-controller").WithComponentSuffix(name))
}

func NewCertRotationControllerMultipleTargets(
name string,
rotatedSigningCASecret RotatedSigningCASecret,
caBundleConfigMap CABundleConfigMap,
rotatedTargetSecrets []RotatedSelfSignedCertKeySecret,
recorder events.Recorder,
reporter StatusReporter,
controlledSecrets *[]metav1.ObjectMeta,
controlledConfigMaps *[]metav1.ObjectMeta,
) factory.Controller {
informers := sets.New[factory.Informer](
rotatedSigningCASecret.Informer.Informer(),
caBundleConfigMap.Informer.Informer(),
)

*controlledSecrets = append(*controlledSecrets, metav1.ObjectMeta{
Name: rotatedSigningCASecret.Name,
Namespace: rotatedSigningCASecret.Namespace,
})
*controlledConfigMaps = append(*controlledConfigMaps, metav1.ObjectMeta{
Name: caBundleConfigMap.Name,
Namespace: caBundleConfigMap.Namespace,
})
for _, target := range rotatedTargetSecrets {
informers = informers.Insert(target.Informer.Informer())
*controlledSecrets = append(*controlledSecrets, metav1.ObjectMeta{
Name: target.Name,
Namespace: target.Namespace,
})
}

c := &CertRotationController{
Name: name,
RotatedSigningCASecret: rotatedSigningCASecret,
CABundleConfigMap: caBundleConfigMap,
RotatedTargetSecrets: rotatedTargetSecrets,
StatusReporter: reporter,
}
return factory.New().
ResyncEvery(time.Minute).
WithSync(c.Sync).
WithInformers(
informers.UnsortedList()...,
).
WithPostStartHooks(
c.targetCertRecheckerPostRunHook,
).
ToController("MultipleTargetCertRotationController", recorder.WithComponentSuffix("cert-rotation-controller").WithComponentSuffix(name))
}

func (c CertRotationController) Sync(ctx context.Context, syncCtx factory.SyncContext) error {
syncErr := c.SyncWorker(ctx)

Expand All @@ -122,7 +189,15 @@ func (c CertRotationController) Sync(ctx context.Context, syncCtx factory.SyncCo
}

func (c CertRotationController) getSigningCertKeyPairLocation() string {
return fmt.Sprintf("%s/%s", c.RotatedSelfSignedCertKeySecret.Namespace, c.RotatedSelfSignedCertKeySecret.Name)
var firstSecret RotatedSelfSignedCertKeySecret
if c.RotatedTargetSecrets == nil {
return ""
}
if len(c.RotatedTargetSecrets) == 0 {
return ""
}
firstSecret = c.RotatedTargetSecrets[0]
return fmt.Sprintf("%s/%s", firstSecret.Namespace, firstSecret.Name)
}

func (c CertRotationController) SyncWorker(ctx context.Context) error {
Expand All @@ -136,30 +211,35 @@ func (c CertRotationController) SyncWorker(ctx context.Context) error {
return err
}

if _, err := c.RotatedSelfSignedCertKeySecret.EnsureTargetCertKeyPair(ctx, signingCertKeyPair, cabundleCerts); err != nil {
return err
var errs []error
for _, secret := range c.RotatedTargetSecrets {
if _, err := secret.EnsureTargetCertKeyPair(ctx, signingCertKeyPair, cabundleCerts); err != nil {
errs = append(errs, err)
}
}
if len(errs) != 0 {
return errors.NewAggregate(errs)
}

return nil
}

func (c CertRotationController) targetCertRecheckerPostRunHook(ctx context.Context, syncCtx factory.SyncContext) error {
// If we have a need to force rechecking the cert, use this channel to do it.
refresher, ok := c.RotatedSelfSignedCertKeySecret.CertCreator.(TargetCertRechecker)
if !ok {
return nil
}
targetRefresh := refresher.RecheckChannel()
go wait.Until(func() {
for {
select {
case <-targetRefresh:
syncCtx.Queue().Add(factory.DefaultQueueKey)
case <-ctx.Done():
return
}
for _, target := range c.RotatedTargetSecrets {
if refresher, ok := target.CertCreator.(TargetCertRechecker); ok {
go wait.Until(func() {
for {
select {
case <-refresher.RecheckChannel():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 with case _, 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

syncCtx.Queue().Add(factory.DefaultQueueKey)
case <-ctx.Done():
return
}
}
}, time.Minute, ctx.Done())
}
}, time.Minute, ctx.Done())
}

<-ctx.Done()
return nil
Expand Down