Certificates: preventing CertificateRequest creation runaway #5567

JoshVanL · 2022-11-09T11:28:49Z

PR adds a new Condition type to Certificates DuplicateSecretName. This condition is managed by a new DuplicateSecretName controller, and sets this condition on Certificates which have the same spec.secretName as that as another Certificate in the same Namespace. This condition blocks issuance.

/kind feature
/milestone v1.11

Certificates: Adds a new DuplicateSecretName condition on Certificate resources which will be set when the Certificate has a `spec.secretName` value of that of another Certificate in the same Namespace. This condition will block issuance of that Certificate. The condition will be removed once the duplication of secretName has been fixed, and normal issuance can continue. This condition is used to both prevent CertificateRequest runaway, as well as make it clear what the problem is to users.

You can check that this PR works by installing the below manifests, and view it is currently broken on master but fixed with this PR.

$ cat <<EOF | kubectl apply -f -
apiVersion: cert-manager.io/v1
kind: Issuer
metadata:
  name: ca
spec:
  selfSigned: {}
---
apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
  name: foo
spec:
  commonName: test
  secretName: hello
  privateKey:
    algorithm: RSA
    rotationPolicy: Always
  issuerRef:
    name: ca
---
apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
  name: bar
spec:
  commonName: test2
  secretName: hello
  issuerRef:
    name: ca
  privateKey:
    algorithm: ECDSA
    rotationPolicy: Always
EOF
$ kubectl get cr --watch

JoshVanL · 2022-11-09T13:39:13Z

Implementation is still not quite right.. we shouldn't fail on a duplicate secret name, and instead just block on the issuance. Triggering a new issuance on a non-failed Certificate which no longer has a duplicate secret name should happen right away, so the e2e tests are correctly failing.

pkg/apis/certmanager/v1/types_certificate.go

pkg/controller/certificates/duplicatesecrets/duplicatesecrets_controller.go

jsoref · 2022-11-09T16:58:00Z

pkg/controller/certificates/issuing/issuing_controller.go

 		// If Certificate doesn't have Issuing=true condition then we should check
 		// to ensure all non-issuing related SecretData is correct on the
 		// Certificate's secret.
 		return c.ensureSecretData(ctx, log, crt)
 	}

+	// If the Certificate has a duplicate Secret name condition set, we should
+	// fail the issuance. If we discover that we have a duplicate Secret name set
+	// ourselves, we exit early and wait for that condition to be set.


Does this need a negation?

pkg/controller/certificates/issuing/issuing_controller.go

pkg/controller/certificates/trigger/trigger_controller.go

SgtCoDFish · 2023-01-04T14:02:03Z

This won't make 1.11. I was asked to take a look at this but haven't had a chance. Pushing to 1.12!

vinzent · 2023-03-29T08:51:20Z

Looking forward to this feature. We already faced the "certificate secret race" multiple times. Issued about 40'000 certificates from our on-premise ACME server.

maelvls · 2023-04-14T11:50:56Z

Hi @JoshVanL! Just to let you know, we will have a code freeze for 1.12 starting Wed 19 April 2023. The code freeze will be waived after the release of 1.12 on Wed 26 April 2023.

inteon · 2023-05-03T07:43:09Z

/retest

jetstack-bot · 2023-10-03T18:34:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from inteon. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

drewwells · 2023-10-03T18:39:57Z

Why does this need a controller? We can check if secret's ownerReference matches the certificates resource ID

inteon · 2023-10-03T19:14:24Z

@drewwells

Why does this need a controller? We can check if secret's ownerReference matches the certificates resource ID

ownerReferences can be disabled (and probably should be in a production environment): https://cert-manager.io/docs/installation/upgrading/#reinstalling-cert-manager

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

…ecretName conflict Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

inteon · 2023-10-04T08:54:23Z

I updated the PR:

The DuplicateSecretName condition is renamed ot InConflict.
Now the Ready condition is set to False when there is an InConflict condition.

The new behavior looks as follows:

$ kubectl -n test get cert | grep True | wc -l
0
$ kubectl -n test get cert -o yaml | grep "secretName: ca-key-pair$" | wc -l
31

$ _bin/cmctl/cmctl-linux-amd64 renew -n test a9
Manually triggered issuance of Certificate test/a9

$ kubectl -n test get cert a9 -o yaml
apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
  creationTimestamp: "2023-10-04T08:51:11Z"
  generation: 1
  name: a9
  namespace: test
  resourceVersion: "3469"
  uid: f7c8d2ff-d3c6-4489-b183-2d8329a494fc
spec:
  commonName: test
  issuerRef:
    name: ca
  privateKey:
    algorithm: RSA
    rotationPolicy: Always
  secretName: ca-key-pair
status:
  conditions:
  - lastTransitionTime: "2023-10-04T08:51:11Z"
    message: 'Certificate shares the same Secret name as the following Certificates
      in this Namespace: [ a1, a10, a11, a12, a13, a14, a15, a16, a17, a18, a19, a2,
      a20, a21, a22, a23, a24, a25, a26, a27, a28, a29, a3, a30, a31, a4, a5, a6,
      a7, a8 ]. Issuance will block until this is resolved to prevent CertificateRequest
      creation runaway.'
    observedGeneration: 1
    reason: a1,a10,a11,a12,a13,a14,a15,a16,a17,a18,a19,a2,a20,a21,a22,a23,a24,a25,a26,a27,a28,a29,a3,a30,a31,a4,a5,a6,a7,a8
    status: "True"
    type: InConflict
  - lastTransitionTime: "2023-10-04T08:51:11Z"
    message: Not ready because Certificate has a "InConflict" condition
    observedGeneration: 1
    reason: InConflict
    status: "False"
    type: Ready
  - lastTransitionTime: "2023-10-04T08:51:57Z"
    message: The certificate has been successfully issued
    observedGeneration: 1
    reason: Issued
    status: "False"
    type: Issuing
  notAfter: "2024-01-02T08:51:57Z"
  notBefore: "2023-10-04T08:51:57Z"
  renewalTime: "2023-12-03T08:51:57Z"
  revision: 1

$ kubectl -n test get cert a8 -oyaml
apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
  creationTimestamp: "2023-10-04T08:51:10Z"
  generation: 1
  name: a8
  namespace: test
  resourceVersion: "3436"
  uid: 6add6307-d79d-4e8e-a9e8-15cbfb275a4b
spec:
  commonName: test
  issuerRef:
    name: ca
  privateKey:
    algorithm: RSA
    rotationPolicy: Always
  secretName: ca-key-pair
status:
  conditions:
  - lastTransitionTime: "2023-10-04T08:51:10Z"
    message: 'Certificate shares the same Secret name as the following Certificates
      in this Namespace: [ a1, a10, a11, a12, a13, a14, a15, a16, a17, a18, a19, a2,
      a20, a21, a22, a23, a24, a25, a26, a27, a28, a29, a3, a30, a31, a4, a5, a6,
      a7, a9 ]. Issuance will block until this is resolved to prevent CertificateRequest
      creation runaway.'
    observedGeneration: 1
    reason: a1,a10,a11,a12,a13,a14,a15,a16,a17,a18,a19,a2,a20,a21,a22,a23,a24,a25,a26,a27,a28,a29,a3,a30,a31,a4,a5,a6,a7,a9
    status: "True"
    type: InConflict
  - lastTransitionTime: "2023-10-04T08:51:10Z"
    message: Not ready because Certificate has a "InConflict" condition
    observedGeneration: 1
    reason: InConflict
    status: "False"
    type: Ready
  notAfter: "2024-01-02T08:51:57Z"
  notBefore: "2023-10-04T08:51:57Z"
  renewalTime: "2023-12-03T08:51:57Z"

This is the script I used to generate the certs

#!/bin/bash

kubectl create ns test

cat <<EOF | kubectl apply -f -
apiVersion: cert-manager.io/v1
kind: Issuer
metadata:
    name: ca
    namespace: test
spec:
    selfSigned: {}
EOF

for i in {1..31}; do

cat <<EOF | kubectl apply -f -
apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
    name: a$i
    namespace: test
spec:
    commonName: test$i
    secretName: ca-key-pair
    privateKey:
        algorithm: RSA
        rotationPolicy: Always
    issuerRef:
        name: ca
EOF

done

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

maelvls · 2023-10-06T10:07:53Z

pkg/controller/certificates/inconflict/inconflict_controller.go

+	condition := buildInConflictCondition(duplicates)
+	crt = crt.DeepCopy()
+	if condition != nil {
+		apiutil.SetCertificateCondition(crt, crt.Generation, condition.Type, condition.Status, condition.Reason, condition.Message)


Note: In api-conventions.md, the reason field is described as "a one-word CamelCase reason for the condition's last transition"

Thanks, currently, the reason is used to store what certs are in conflict & this value is actually used by the controller.
As discussed in today's standup, we probably can implement a solution that does not require this.

pkg/apis/certmanager/v1/types_certificate.go

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

inteon · 2023-10-09T12:15:33Z

NOTE: the current implementation makes all conflicting certificates not ready.
A future improvement might be to have 1 certificate (the first created) that continues to work correctly, and only fail the others.

everything breaks (current master)
all conflicting certificates break (this PR)
only the additional certificates break (future PR)

inteon · 2023-10-18T08:43:42Z

fixed in #6406

jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Nov 9, 2022

jetstack-bot added this to the v1.11 milestone Nov 9, 2022

JoshVanL changed the title ~~Certificates: preventing CertificateRequest creation runaway~~ WIP: Certificates: preventing CertificateRequest creation runaway Nov 9, 2022

jetstack-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 9, 2022

jsoref reviewed Nov 9, 2022

View reviewed changes

SgtCoDFish modified the milestones: v1.11, v1.12 Jan 4, 2023

irbekrm mentioned this pull request Jan 9, 2023

Two certificates referencing the same secret causes cert manager to constantly modify the secret #5675

Closed

seh mentioned this pull request Jan 23, 2023

Stop infinitely reissuing certs with shared Secret #5303

Closed

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2023

inteon self-assigned this Apr 28, 2023

inteon force-pushed the controllers-certificates-duplicate-secret-name branch from 3cfd601 to d00373a Compare April 28, 2023 08:01

jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2023

inteon force-pushed the controllers-certificates-duplicate-secret-name branch from 9465bb8 to ca0e839 Compare May 3, 2023 06:58

inteon changed the title ~~WIP: Certificates: preventing CertificateRequest creation runaway~~ Certificates: preventing CertificateRequest creation runaway May 3, 2023

jetstack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 3, 2023

inteon force-pushed the controllers-certificates-duplicate-secret-name branch from dc4a643 to 6e5f587 Compare May 3, 2023 14:48

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2023

jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 3, 2023

jetstack-bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 3, 2023

inteon force-pushed the controllers-certificates-duplicate-secret-name branch 2 times, most recently from eecf685 to 4fc6a8f Compare October 3, 2023 18:59

inteon added 5 commits October 3, 2023 21:54

resolve merge issues

1636b5a

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

replace deprecated wait functions

1b17360

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

retry update on conflict

bf35a42

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

Fix remaining issues with duplicatesecrets controller.

0a52869

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

Don't trigger issuance if there is a Secret name conflict

a75cbb8

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

inteon force-pushed the controllers-certificates-duplicate-secret-name branch 2 times, most recently from dc7371f to 8320cc7 Compare October 4, 2023 07:54

don't mark the Certificate resouce as Ready if there is a duplicate S…

b90e20b

…ecretName conflict Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

inteon force-pushed the controllers-certificates-duplicate-secret-name branch from 8320cc7 to b90e20b Compare October 4, 2023 07:56

rename DuplicateSecretName condition to InConflict

28e298b

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

inteon force-pushed the controllers-certificates-duplicate-secret-name branch from fc85050 to 28e298b Compare October 4, 2023 08:55

update comment

d0cf4a2

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

maelvls reviewed Oct 6, 2023

View reviewed changes

jsoref reviewed Oct 6, 2023

View reviewed changes

pkg/apis/certmanager/v1/types_certificate.go Outdated Show resolved Hide resolved

pkg/apis/certmanager/v1/types_certificate.go Outdated Show resolved Hide resolved

simplify the solution based on feedback

132a779

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

inteon force-pushed the controllers-certificates-duplicate-secret-name branch from 2316af9 to 132a779 Compare October 9, 2023 08:04

update integration test

c5155a1

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

inteon force-pushed the controllers-certificates-duplicate-secret-name branch from f726d2c to c5155a1 Compare October 9, 2023 11:49

inteon mentioned this pull request Oct 10, 2023

Fix DuplicateSecretName issue #6406

Merged

inteon closed this Oct 18, 2023

Certificates: preventing CertificateRequest creation runaway #5567

Certificates: preventing CertificateRequest creation runaway #5567

Uh oh!

Conversation

JoshVanL commented Nov 9, 2022 • edited by inteon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoshVanL commented Nov 9, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsoref Nov 9, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SgtCoDFish commented Jan 4, 2023

Uh oh!

vinzent commented Mar 29, 2023

Uh oh!

maelvls commented Apr 14, 2023

Uh oh!

inteon commented May 3, 2023

Uh oh!

jetstack-bot commented Oct 3, 2023

Uh oh!

drewwells commented Oct 3, 2023

Uh oh!

inteon commented Oct 3, 2023

Uh oh!

inteon commented Oct 4, 2023

Uh oh!

maelvls Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inteon Oct 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

inteon commented Oct 9, 2023

Uh oh!

inteon commented Oct 18, 2023

Uh oh!

Uh oh!

JoshVanL commented Nov 9, 2022 •

edited by inteon

Loading

maelvls Oct 6, 2023 •

edited

Loading