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

NO-JIRA: certrotation: improve error messages #1720

Merged
merged 1 commit into from
Jul 11, 2024
+42 −18
Merged

NO-JIRA: certrotation: improve error messages #1720

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 10 additions & 2 deletions pkg/operator/certrotation/cabundle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ type CABundleConfigMap struct {
EventRecorder events.Recorder
}

func (c CABundleConfigMap) EnsureConfigMapCABundle(ctx context.Context, signingCertKeyPair *crypto.CA) ([]*x509.Certificate, error) {
func (c CABundleConfigMap) EnsureConfigMapCABundle(ctx context.Context, signingCertKeyPair *crypto.CA, signingCertKeyPairLocation string) ([]*x509.Certificate, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

silly question while going through the breaking change in CEO, isn't that simply ca.Namespace/ca.Name at this point?

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be a location of the signer used to build this CA, so its secret.Namespace/secret.Name

// 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
Expand Down Expand Up @@ -73,7 +73,15 @@ func (c CABundleConfigMap) EnsureConfigMapCABundle(ctx context.Context, signingC
return nil, err
}
if originalCABundleConfigMap == nil || originalCABundleConfigMap.Data == nil || !equality.Semantic.DeepEqual(originalCABundleConfigMap.Data, caBundleConfigMap.Data) {
c.EventRecorder.Eventf("CABundleUpdateRequired", "%q in %q requires a new cert", c.Name, c.Namespace)
reason := ""
if originalCABundleConfigMap == nil {
reason = "configmap doesn't exist"
} else if originalCABundleConfigMap.Data == nil {
reason = "configmap is empty"
Copy link
Contributor

Choose a reason for hiding this comment

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

a minor suggestions:

  • can we show the name and namespace of the object that does not exist for reason "configmap doesn't exist" and "configmap is empty"

Copy link
Member Author

Choose a reason for hiding this comment

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

This error would be appended to the message with namespace/name: openshift-kube-apiserver-operator node-system-admin-signer requires a new signing cert/key pair: secret doesn't exist in SignerUpdateRequired event

} else if !equality.Semantic.DeepEqual(originalCABundleConfigMap.Data, caBundleConfigMap.Data) {
reason = fmt.Sprintf("signer update %s", signingCertKeyPairLocation)
}
c.EventRecorder.Eventf("CABundleUpdateRequired", "%q in %q requires a new cert: %s", c.Name, c.Namespace, reason)
Copy link
Contributor

Choose a reason for hiding this comment

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

question: what does the c.Name and c.Namespace mean in relation to the object in the reason, or are we saying that the CA bundle configmap located in c.Namespace/c.Name requires a new cert?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, correct, that would be object namespace / name of the target configmap/secret

LabelAsManagedConfigMap(caBundleConfigMap, CertificateTypeCABundle)

modified = true
Expand Down
2 changes: 1 addition & 1 deletion pkg/operator/certrotation/cabundle_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ func TestEnsureConfigMapCABundle(t *testing.T) {
if err != nil {
t.Fatal(err)
}
_, err = c.EnsureConfigMapCABundle(context.TODO(), newCA)
_, err = c.EnsureConfigMapCABundle(context.TODO(), newCA, "signer-secret")
switch {
case err != nil && len(test.expectedError) == 0:
t.Error(err)
Expand Down
6 changes: 5 additions & 1 deletion 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 @@ -121,13 +121,17 @@ func (c CertRotationController) Sync(ctx context.Context, syncCtx factory.SyncCo
return syncErr
}

func (c CertRotationController) getSigningCertKeyPairLocation() string {
return fmt.Sprintf("%s/%s", c.RotatedSelfSignedCertKeySecret.Namespace, c.RotatedSelfSignedCertKeySecret.Name)
}

func (c CertRotationController) SyncWorker(ctx context.Context) error {
signingCertKeyPair, _, err := c.RotatedSigningCASecret.EnsureSigningCertKeyPair(ctx)
if err != nil {
return err
}

cabundleCerts, err := c.CABundleConfigMap.EnsureConfigMapCABundle(ctx, signingCertKeyPair)
cabundleCerts, err := c.CABundleConfigMap.EnsureConfigMapCABundle(ctx, signingCertKeyPair, c.getSigningCertKeyPairLocation())
if err != nil {
return err
}
Expand Down
13 changes: 9 additions & 4 deletions pkg/operator/certrotation/signer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (*
if err != nil && !apierrors.IsNotFound(err) {
return nil, false, err
}
var signerExists = true
Copy link
Member Author

Choose a reason for hiding this comment

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

This is required to pass unit tests, which do delete+create (and don't set ResourceVersion afterwards). It would be reworked after ApplySecretDoNotUse is removed

signingCertKeyPairSecret := originalSigningCertKeyPairSecret.DeepCopy()
if apierrors.IsNotFound(err) {
// create an empty one
Expand All @@ -79,7 +80,7 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (*
),
Type: corev1.SecretTypeTLS,
}
modified = true
signerExists = false
}

applyFn := resourceapply.ApplySecret
Expand All @@ -94,7 +95,10 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (*
modified = needsMetadataUpdate || needsTypeChange || modified

signerUpdated := false
if needed, reason := needNewSigningCertKeyPair(signingCertKeyPairSecret.Annotations, c.Refresh, c.RefreshOnlyWhenExpired); needed {
if needed, reason := needNewSigningCertKeyPair(signingCertKeyPairSecret, c.Refresh, c.RefreshOnlyWhenExpired); needed || !signerExists {
if !signerExists {
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)
if err := setSigningCertKeyPairSecret(signingCertKeyPairSecret, c.Validity); err != nil {
return nil, false, err
Expand Down Expand Up @@ -139,7 +143,8 @@ func ensureOwnerReference(meta *metav1.ObjectMeta, owner *metav1.OwnerReference)
return false
}

func needNewSigningCertKeyPair(annotations map[string]string, refresh time.Duration, refreshOnlyWhenExpired bool) (bool, string) {
func needNewSigningCertKeyPair(secret *corev1.Secret, refresh time.Duration, refreshOnlyWhenExpired bool) (bool, string) {
annotations := secret.Annotations
notBefore, notAfter, reason := getValidityFromAnnotations(annotations)
if len(reason) > 0 {
return true, reason
Expand All @@ -156,7 +161,7 @@ func needNewSigningCertKeyPair(annotations map[string]string, refresh time.Durat
validity := notAfter.Sub(notBefore)
at80Percent := notAfter.Add(-validity / 5)
if time.Now().After(at80Percent) {
return true, fmt.Sprintf("past its latest possible time %v", at80Percent)
return true, fmt.Sprintf("past refresh time (80%% of validity): %v", at80Percent)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: do you think it will help if we also include the validity in the message?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this would show happy path, where certs are updated at 80%, as the cert can be force refreshed in the future

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is dead code actually, I haven't seen this message at all

}

developerSpecifiedRefresh := notBefore.Add(refresh)
Expand Down
27 changes: 17 additions & 10 deletions pkg/operator/certrotation/target.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ type TargetCertCreator interface {
// NewCertificate creates a new key-cert pair with the given signer.
NewCertificate(signer *crypto.CA, validity time.Duration) (*crypto.TLSCertificateConfig, error)
// NeedNewTargetCertKeyPair decides whether a new cert-key pair is needed. It returns a non-empty reason if it is the case.
NeedNewTargetCertKeyPair(currentCertSecret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired bool) string
NeedNewTargetCertKeyPair(currentCertSecret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired, secretDoesntExist bool) string
// SetAnnotations gives an option to override or set additional annotations
SetAnnotations(cert *crypto.TLSCertificateConfig, annotations map[string]string) map[string]string
}
Expand All @@ -97,6 +97,7 @@ func (c RotatedSelfSignedCertKeySecret) EnsureTargetCertKeyPair(ctx context.Cont
// and need to mint one
// TODO do the cross signing thing, but this shows the API consumers want and a very simple impl.

secretDoesntExist := false
modified := false
originalTargetCertKeyPairSecret, err := c.Lister.Secrets(c.Namespace).Get(c.Name)
if err != nil && !apierrors.IsNotFound(err) {
Expand All @@ -114,6 +115,7 @@ func (c RotatedSelfSignedCertKeySecret) EnsureTargetCertKeyPair(ctx context.Cont
Type: corev1.SecretTypeTLS,
}
modified = true
secretDoesntExist = true
}

applyFn := resourceapply.ApplySecret
Expand All @@ -127,7 +129,7 @@ func (c RotatedSelfSignedCertKeySecret) EnsureTargetCertKeyPair(ctx context.Cont
needsSecretTypeUpdate := ensureSecretTLSTypeSet(targetCertKeyPairSecret)
modified = needsMetadataUpdate || needsSecretTypeUpdate || modified

if reason := c.CertCreator.NeedNewTargetCertKeyPair(targetCertKeyPairSecret, signingCertKeyPair, caBundleCerts, c.Refresh, c.RefreshOnlyWhenExpired); len(reason) > 0 {
if reason := c.CertCreator.NeedNewTargetCertKeyPair(targetCertKeyPairSecret, signingCertKeyPair, caBundleCerts, c.Refresh, c.RefreshOnlyWhenExpired, secretDoesntExist); len(reason) > 0 {
c.EventRecorder.Eventf("TargetUpdateRequired", "%q in %q requires a new target cert/key pair: %v", c.Name, c.Namespace, reason)
if err := setTargetCertKeyPairSecret(targetCertKeyPairSecret, c.Validity, signingCertKeyPair, c.CertCreator, c.AdditionalAnnotations); err != nil {
return nil, err
Expand All @@ -149,7 +151,12 @@ func (c RotatedSelfSignedCertKeySecret) EnsureTargetCertKeyPair(ctx context.Cont
return targetCertKeyPairSecret, nil
}

func needNewTargetCertKeyPair(annotations map[string]string, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired bool) string {
func needNewTargetCertKeyPair(secret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired, secretDoesntExist bool) string {
if secretDoesntExist {
return "secret doesn't exist"
Copy link
Contributor

Choose a reason for hiding this comment

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

Relying on the "ResourceVersion" being empty to indicate that the object does not exist may lead to bugs in the future, sometimes we set the resource version of an existing object to empty string before we send it to the apiserver for update
The more reliable way for us to know whether an object exists is when we get a NotFoundError from the apiserver, and use the not found error to produce this error message seems more intuitive to me, thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, implemented

}

annotations := secret.Annotations
if reason := needNewTargetCertKeyPairForTime(annotations, signer, refresh, refreshOnlyWhenExpired); len(reason) > 0 {
return reason
}
Expand Down Expand Up @@ -206,7 +213,7 @@ func needNewTargetCertKeyPairForTime(annotations map[string]string, signer *cryp
validity := notAfter.Sub(notBefore)
at80Percent := notAfter.Add(-validity / 5)
if time.Now().After(at80Percent) {
return fmt.Sprintf("past its latest possible time %v", at80Percent)
return fmt.Sprintf("past refresh time (80%% of validity): %v", at80Percent)
}

// If Certificate is past its refresh time, we may have action to take. We only do this if the signer is old enough.
Expand Down Expand Up @@ -266,8 +273,8 @@ func (r *ClientRotation) NewCertificate(signer *crypto.CA, validity time.Duratio
return signer.MakeClientCertificateForDuration(r.UserInfo, validity)
}

func (r *ClientRotation) NeedNewTargetCertKeyPair(currentCertSecret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired bool) string {
return needNewTargetCertKeyPair(currentCertSecret.Annotations, signer, caBundleCerts, refresh, refreshOnlyWhenExpired)
func (r *ClientRotation) NeedNewTargetCertKeyPair(currentCertSecret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired, secretDoesntExist bool) string {
return needNewTargetCertKeyPair(currentCertSecret, signer, caBundleCerts, refresh, refreshOnlyWhenExpired, secretDoesntExist)
}

func (r *ClientRotation) SetAnnotations(cert *crypto.TLSCertificateConfig, annotations map[string]string) map[string]string {
Expand All @@ -291,8 +298,8 @@ func (r *ServingRotation) RecheckChannel() <-chan struct{} {
return r.HostnamesChanged
}

func (r *ServingRotation) NeedNewTargetCertKeyPair(currentCertSecret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired bool) string {
reason := needNewTargetCertKeyPair(currentCertSecret.Annotations, signer, caBundleCerts, refresh, refreshOnlyWhenExpired)
func (r *ServingRotation) NeedNewTargetCertKeyPair(currentCertSecret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired, secretDoesntExist bool) string {
reason := needNewTargetCertKeyPair(currentCertSecret, signer, caBundleCerts, refresh, refreshOnlyWhenExpired, secretDoesntExist)
if len(reason) > 0 {
return reason
}
Expand Down Expand Up @@ -337,8 +344,8 @@ func (r *SignerRotation) NewCertificate(signer *crypto.CA, validity time.Duratio
return crypto.MakeCAConfigForDuration(signerName, validity, signer)
}

func (r *SignerRotation) NeedNewTargetCertKeyPair(currentCertSecret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired bool) string {
return needNewTargetCertKeyPair(currentCertSecret.Annotations, signer, caBundleCerts, refresh, refreshOnlyWhenExpired)
func (r *SignerRotation) NeedNewTargetCertKeyPair(currentCertSecret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired, secretDoesntExist bool) string {
return needNewTargetCertKeyPair(currentCertSecret, signer, caBundleCerts, refresh, refreshOnlyWhenExpired, secretDoesntExist)
}

func (r *SignerRotation) SetAnnotations(cert *crypto.TLSCertificateConfig, annotations map[string]string) map[string]string {
Expand Down