-
Notifications
You must be signed in to change notification settings - Fork 217
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.
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
NO-JIRA: certrotation: improve error messages #1720
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
// 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 | ||
|
@@ -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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a minor suggestions:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This error would be appended to the message with namespace/name: |
||
} 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,6 +68,7 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (* | |
if err != nil && !apierrors.IsNotFound(err) { | ||
return nil, false, err | ||
} | ||
var signerExists = true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
signingCertKeyPairSecret := originalSigningCertKeyPairSecret.DeepCopy() | ||
if apierrors.IsNotFound(err) { | ||
// create an empty one | ||
|
@@ -79,7 +80,7 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (* | |
), | ||
Type: corev1.SecretTypeTLS, | ||
} | ||
modified = true | ||
signerExists = false | ||
} | ||
|
||
applyFn := resourceapply.ApplySecret | ||
|
@@ -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 | ||
|
@@ -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 | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
} | ||
|
@@ -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) { | ||
|
@@ -114,6 +115,7 @@ func (c RotatedSelfSignedCertKeySecret) EnsureTargetCertKeyPair(ctx context.Cont | |
Type: corev1.SecretTypeTLS, | ||
} | ||
modified = true | ||
secretDoesntExist = true | ||
} | ||
|
||
applyFn := resourceapply.ApplySecret | ||
|
@@ -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 | ||
|
@@ -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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
} | ||
|
@@ -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. | ||
|
@@ -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 { | ||
|
@@ -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 | ||
} | ||
|
@@ -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 { | ||
|
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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