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.

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

Fix keyrotation warning #5199

Merged
merged 2 commits into from Jun 30, 2022

Conversation

irbekrm
Copy link
Collaborator

@irbekrm irbekrm commented Jun 9, 2022

This PR improves the warning that is thrown when a user has changed some key size or algorithm in Certificate's spec, but cert-manager is unable to regenerate the private key because .spec.rotationPolicy is set to Never or is unset.

At the moment user would see this warning on cert's spec:

  Warning  DecodeFailed  30m   cert-manager-certificates-key-manager  Existing private key in Secret "myib-prod-ca" does not match requirements on Certificate resource, mismatching fields: [spec.keySize]

I have changed it to:

Warning  CannotRegenerateKey  15s (x2 over 15s)  cert-manager-certificates-key-manager      User intervention required: existing private key in Secret "test1" does not match requirements on Certificate resource, mismatching fields: [spec.privateKey.size], but cert-manager cannot create new private key as the Certificate's .spec.privateKey.rotationPolicy is unset or set to Never. To allow cert-manager to create a new private key you can set .spec.privateKey.rotationPolicy to 'Always' (this will result in the private key being regenerated every time a cert is renewed)

(maybe too verbose?)

Fixes #5183

I have also considered that perhaps we could simply recreate the key in this case, however this would be a change in behaviour. At the moment the description of cert.spec.privateKey.rotationPolicy does explicitly state that user intervention is required if a key spec field has changed, but the policy is not 'Always'

Updates warning message that is thrown if issuance fails because private key does not match spec, but private key regeneration is disabled. See https://github.com/cert-manager/cert-manager/pull/5199.

/kind cleanup

…has changed

Signed-off-by: irbekrm <irbekrm@gmail.com>
@jetstack-bot jetstack-bot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 9, 2022
@@ -60,7 +60,7 @@ func PrivateKeyMatchesSpec(pk crypto.PrivateKey, spec cmapi.CertificateSpec) ([]
func rsaPrivateKeyMatchesSpec(pk crypto.PrivateKey, spec cmapi.CertificateSpec) ([]string, error) {
rsaPk, ok := pk.(*rsa.PrivateKey)
if !ok {
return []string{"spec.keyAlgorithm"}, nil
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

these are probably leftover from alpha/beta structure when the privateKey field did not exist yet

@irbekrm irbekrm force-pushed the fix_keyrotation_warning branch 2 times, most recently from 69f2a88 to a56068f Compare June 9, 2022 14:18
Signed-off-by: irbekrm <irbekrm@gmail.com>
@irbekrm
Copy link
Collaborator Author

irbekrm commented Jun 28, 2022

/milestone v1.9

@jetstack-bot jetstack-bot added this to the v1.9 milestone Jun 28, 2022
@irbekrm
Copy link
Collaborator Author

irbekrm commented Jun 28, 2022

/test pull-cert-manager-e2e-v1-24

Copy link
Member

@SgtCoDFish SgtCoDFish left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
/hold

I think this seems an obvious win and changing the error to reasonCannotRegenerateKey makes sense.

I think there's a small chance someone might be filtering logs based on the old error code specifically, right, making this a change in behaviour?

I think that's totally reasonable, but would it be worth maybe adding a release note to this PR just noting the change?

@jetstack-bot jetstack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 30, 2022
@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 30, 2022
@jetstack-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irbekrm, SgtCoDFish

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jetstack-bot jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jun 30, 2022
@irbekrm
Copy link
Collaborator Author

irbekrm commented Jun 30, 2022

I think there's a small chance someone might be filtering logs based on the old error code specifically, right, making this a change in behaviour?
I think that's totally reasonable, but would it be worth maybe adding a release note to this PR just noting the change?

Thanks @SgtCoDFish and thanks for the xkcd 😄 I've added a release note

@irbekrm
Copy link
Collaborator Author

irbekrm commented Jun 30, 2022

/hold cancel

@jetstack-bot jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 30, 2022
@jetstack-bot jetstack-bot merged commit d15d2d5 into cert-manager:master Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Existing private key is not up to date for spec: [spec.keySize] [spec.dnsNames]
3 participants