Skip to content

Commit

Permalink
certrotation: avoid using applySecret to catch racy cert updates
Browse files Browse the repository at this point in the history
  • Loading branch information
@vrutkovs
vrutkovs committed Aug 12, 2024
1 parent bc0e75f commit f468f49
Show file tree
Hide file tree
Showing 7 changed files with 122 additions and 307 deletions.
20 changes: 14 additions & 6 deletions pkg/operator/certrotation/cabundle.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (
"github.com/openshift/library-go/pkg/certs"
"github.com/openshift/library-go/pkg/crypto"
"github.com/openshift/library-go/pkg/operator/events"
"github.com/openshift/library-go/pkg/operator/resource/resourceapply"
"github.com/openshift/library-go/pkg/operator/resource/resourcehelper"
)

// CABundleConfigMap maintains a CA bundle config map, by adding new CA certs coming from RotatedSigningCASecret, and by removing expired old ones.
Expand All @@ -45,6 +45,7 @@ func (c CABundleConfigMap) EnsureConfigMapCABundle(ctx context.Context, signingC
// by this point we have current signing cert/key pair. We now need to make sure that the ca-bundle configmap has this cert and
// doesn't have any expired certs
modified := false
exists := true

originalCABundleConfigMap, err := c.Lister.ConfigMaps(c.Namespace).Get(c.Name)
if err != nil && !apierrors.IsNotFound(err) {
Expand All @@ -59,6 +60,7 @@ func (c CABundleConfigMap) EnsureConfigMapCABundle(ctx context.Context, signingC
c.AdditionalAnnotations,
)}
modified = true
exists = false
}

needsOwnerUpdate := false
Expand Down Expand Up @@ -88,14 +90,20 @@ func (c CABundleConfigMap) EnsureConfigMapCABundle(ctx context.Context, signingC
}

if modified {

actualCABundleConfigMap, updated, err := resourceapply.ApplyConfigMap(ctx, c.Client, c.EventRecorder, caBundleConfigMap)
var actualCABundleConfigMap *corev1.ConfigMap
var action string
if !exists {
actualCABundleConfigMap, err = c.Client.ConfigMaps(c.Namespace).Create(ctx, caBundleConfigMap, metav1.CreateOptions{})
action = "Create"
} else {
actualCABundleConfigMap, err = c.Client.ConfigMaps(c.Namespace).Update(ctx, caBundleConfigMap, metav1.UpdateOptions{})
action = "Update"
}
emitEvent(c.EventRecorder, "ConfigMap", action, resourcehelper.FormatResourceForCLIWithNamespace(actualCABundleConfigMap), err)
if err != nil {
return nil, err
}
if updated {
klog.V(2).Infof("Updated ca-bundle.crt configmap %s/%s with:\n%s", certs.CertificateBundleToString(updatedCerts), caBundleConfigMap.Namespace, caBundleConfigMap.Name)
}
klog.V(2).Infof("Updated ca-bundle.crt configmap %s/%s with:\n%s", certs.CertificateBundleToString(updatedCerts), caBundleConfigMap.Namespace, caBundleConfigMap.Name)
caBundleConfigMap = actualCABundleConfigMap
}

Expand Down
40 changes: 18 additions & 22 deletions pkg/operator/certrotation/cabundle_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,18 +46,14 @@ func TestEnsureConfigMapCABundle(t *testing.T) {
initialConfigMapFn: func() *corev1.ConfigMap { return nil },
verifyActions: func(t *testing.T, client *kubefake.Clientset) {
actions := client.Actions()
if len(actions) != 2 {
if len(actions) != 1 {
t.Fatal(spew.Sdump(actions))
}

if !actions[0].Matches("get", "configmaps") {
if !actions[0].Matches("create", "configmaps") {
t.Error(actions[0])
}
if !actions[1].Matches("create", "configmaps") {
t.Error(actions[1])
}

actual := actions[1].(clienttesting.CreateAction).GetObject().(*corev1.ConfigMap)
actual := actions[0].(clienttesting.CreateAction).GetObject().(*corev1.ConfigMap)
if certType, _ := CertificateTypeFromObject(actual); certType != CertificateTypeCABundle {
t.Errorf("expected certificate type 'ca-bundle', got: %v", certType)
}
Expand All @@ -73,7 +69,7 @@ func TestEnsureConfigMapCABundle(t *testing.T) {
},
initialConfigMapFn: func() *corev1.ConfigMap {
caBundleConfigMap := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "trust-bundle"},
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "trust-bundle", ResourceVersion: "10"},
Data: map[string]string{},
}
certs, err := newTestCACertificate(pkix.Name{CommonName: "signer-tests"}, int64(1), metav1.Duration{Duration: time.Hour * 24 * 60}, time.Now)
Expand All @@ -89,15 +85,15 @@ func TestEnsureConfigMapCABundle(t *testing.T) {
},
verifyActions: func(t *testing.T, client *kubefake.Clientset) {
actions := client.Actions()
if len(actions) != 2 {
if len(actions) != 1 {
t.Fatal(spew.Sdump(actions))
}

if !actions[1].Matches("update", "configmaps") {
t.Error(actions[1])
if !actions[0].Matches("update", "configmaps") {
t.Error(actions[0])
}

actual := actions[1].(clienttesting.UpdateAction).GetObject().(*corev1.ConfigMap)
actual := actions[0].(clienttesting.UpdateAction).GetObject().(*corev1.ConfigMap)
if len(actual.Data["ca-bundle.crt"]) == 0 {
t.Error(actual.Data)
}
Expand All @@ -120,7 +116,7 @@ func TestEnsureConfigMapCABundle(t *testing.T) {
},
initialConfigMapFn: func() *corev1.ConfigMap {
caBundleConfigMap := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "trust-bundle"},
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "trust-bundle", ResourceVersion: "10"},
Data: map[string]string{},
}
certs, err := newTestCACertificate(pkix.Name{CommonName: "signer-tests"}, int64(1), metav1.Duration{Duration: time.Hour * 24 * 60}, time.Now)
Expand All @@ -136,15 +132,15 @@ func TestEnsureConfigMapCABundle(t *testing.T) {
},
verifyActions: func(t *testing.T, client *kubefake.Clientset) {
actions := client.Actions()
if len(actions) != 2 {
if len(actions) != 1 {
t.Fatal(spew.Sdump(actions))
}

if !actions[1].Matches("update", "configmaps") {
t.Error(actions[1])
if !actions[0].Matches("update", "configmaps") {
t.Error(actions[0])
}

actual := actions[1].(clienttesting.UpdateAction).GetObject().(*corev1.ConfigMap)
actual := actions[0].(clienttesting.UpdateAction).GetObject().(*corev1.ConfigMap)
if len(actual.Data["ca-bundle.crt"]) == 0 {
t.Error(actual.Data)
}
Expand All @@ -167,7 +163,7 @@ func TestEnsureConfigMapCABundle(t *testing.T) {
},
initialConfigMapFn: func() *corev1.ConfigMap {
caBundleConfigMap := &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "trust-bundle"},
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "trust-bundle", ResourceVersion: "10"},
Data: map[string]string{},
}
certBytes, err := os.ReadFile("./testfiles/tls-expired.crt")
Expand All @@ -187,15 +183,15 @@ func TestEnsureConfigMapCABundle(t *testing.T) {
},
verifyActions: func(t *testing.T, client *kubefake.Clientset) {
actions := client.Actions()
if len(actions) != 2 {
if len(actions) != 1 {
t.Fatal(spew.Sdump(actions))
}

if !actions[1].Matches("update", "configmaps") {
t.Error(actions[1])
if !actions[0].Matches("update", "configmaps") {
t.Error(actions[0])
}

actual := actions[1].(clienttesting.UpdateAction).GetObject().(*corev1.ConfigMap)
actual := actions[0].(clienttesting.UpdateAction).GetObject().(*corev1.ConfigMap)
if len(actual.Data["ca-bundle.crt"]) == 0 {
t.Error(actual.Data)
}
Expand Down
19 changes: 19 additions & 0 deletions pkg/operator/certrotation/event.go
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package certrotation

import (
"fmt"

"github.com/openshift/library-go/pkg/operator/events"
)

func emitEvent(recorder events.Recorder, objType string, action string, obj string, err error) {
eventFn := recorder.Eventf
reason := fmt.Sprintf("%s%sd", objType, action)
message := fmt.Sprintf("%sd %s", action, obj)
if err != nil {
eventFn = recorder.Warningf
reason = fmt.Sprintf("%s%sFailed", objType, action)
message = fmt.Sprintf("Failed to %s %s: %v", objType, obj, err)
}
eventFn(reason, message)
}
22 changes: 16 additions & 6 deletions pkg/operator/certrotation/signer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (

"github.com/openshift/library-go/pkg/crypto"
"github.com/openshift/library-go/pkg/operator/events"
"github.com/openshift/library-go/pkg/operator/resource/resourceapply"
"github.com/openshift/library-go/pkg/operator/resource/resourcehelper"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -57,12 +57,12 @@ type RotatedSigningCASecret struct {
// EnsureSigningCertKeyPair manages the entire lifecycle of a signer cert as a secret, from creation to continued rotation.
// It always returns the currently used CA pair, a bool indicating whether it was created/updated within this function call and an error.
func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (*crypto.CA, bool, error) {
exists := true
modified := false
originalSigningCertKeyPairSecret, err := c.Lister.Secrets(c.Namespace).Get(c.Name)
if err != nil && !apierrors.IsNotFound(err) {
return nil, false, err
}
var signerExists = true
signingCertKeyPairSecret := originalSigningCertKeyPairSecret.DeepCopy()
if apierrors.IsNotFound(err) {
// create an empty one
Expand All @@ -74,7 +74,8 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (*
),
Type: corev1.SecretTypeTLS,
}
signerExists = false
modified = true
exists = false
}

// apply necessary metadata (possibly via delete+recreate) if secret exists
Expand All @@ -84,8 +85,8 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (*
modified = needsMetadataUpdate || needsTypeChange || modified

signerUpdated := false
if needed, reason := needNewSigningCertKeyPair(signingCertKeyPairSecret, c.Refresh, c.RefreshOnlyWhenExpired); needed || !signerExists {
if !signerExists {
if needed, reason := needNewSigningCertKeyPair(signingCertKeyPairSecret, c.Refresh, c.RefreshOnlyWhenExpired); needed || !exists {
if !exists {
reason = "secret doesn't exist"
}
c.EventRecorder.Eventf("SignerUpdateRequired", "%q in %q requires a new signing cert/key pair: %v", c.Name, c.Namespace, reason)
Expand All @@ -100,7 +101,16 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (*
}

if modified {
actualSigningCertKeyPairSecret, _, err := resourceapply.ApplySecret(ctx, c.Client, c.EventRecorder, signingCertKeyPairSecret)
var actualSigningCertKeyPairSecret *corev1.Secret
var action string
if !exists {
actualSigningCertKeyPairSecret, err = c.Client.Secrets(c.Namespace).Create(ctx, signingCertKeyPairSecret, metav1.CreateOptions{})
action = "Create"
} else {
actualSigningCertKeyPairSecret, err = c.Client.Secrets(c.Namespace).Update(ctx, signingCertKeyPairSecret, metav1.UpdateOptions{})
action = "Update"
}
emitEvent(c.EventRecorder, "Secret", action, resourcehelper.FormatResourceForCLIWithNamespace(actualSigningCertKeyPairSecret), err)
if err != nil {
return nil, false, err
}
Expand Down
Loading

0 comments on commit f468f49

Please sign in to comment.