Extensible Certificate Issuing Controller #2782
Conversation
jetstack-bot
added
release-note
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoshVanL 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 |
jetstack-bot
added
area/deploy
JoshVanL
force-pushed
the
certificate-issuing-controller
branch
2 times, most recently
from
16f4634
to
eb16d01
Compare
pkg/api/util/conditions.go
Outdated
|
|
||
| // Remove this condition from the condition slice | ||
| copy(crt.Status.Conditions[i:], crt.Status.Conditions[i+1:]) | ||
| crt.Status.Conditions = crt.Status.Conditions[:len(crt.Status.Conditions)-1] |
There was a problem hiding this comment.
Looping over a slice while modifying it is a tricky one - the i will not be correct after one element is removed. Instead, accumulate a new conditions slice that contains everything except conditions of the given type. This means you don't need i at all 😄
pkg/api/util/conditions.go
Outdated
| @@ -249,6 +264,24 @@ func CertificateRequestReadyReason(cr *cmapi.CertificateRequest) string { | |||
| return "" | |||
| } | |||
|
|
|||
| // This returns with the message if the CertificateRequest contains an | |||
| // Failed condition, and returns "" otherwise. | |||
| func CertificateRequestFailedMessage(cr *cmapi.CertificateRequest) string { | |||
There was a problem hiding this comment.
Hm, whilst we do have CertificateRequestReadyReason, this is because it is utilised at multiple places within the codebase and makes readability better. In this case, we're only really using it in one place (here: eb16d01#diff-528fa7c9a9cfce42c4813bd3207bb714R199)
Can we instead change this to be a call to GetCertificateRequestCondition, and then reference the appropriate within that accordingly? We can then remove this altogether 😄
| @@ -55,6 +55,7 @@ const ( | |||
| // Annotation names for CertificateRequests | |||
| const ( | |||
| CRPrivateKeyAnnotationKey = "cert-manager.io/private-key-secret-name" | |||
| CRRevisionAnnotationKey = "cert-manager.io/revision" | |||
There was a problem hiding this comment.
Can you add a comment? (and in v1alpha3 & internal)
There was a problem hiding this comment.
pkg/controller/expcertificates/util.go already defines this annotation: CertificateRevisionAnnotationKey = "cert-manager.io/certificate-revision".
- can you change this to be
certificate-revisioninstead (as per design doc) - can you remove duplicate definitions? 😄
| // secretData is a structure wrapping private key, certificate and CA data | ||
| type secretData struct { | ||
| sk, cert, ca []byte | ||
| } |
There was a problem hiding this comment.
nit: does this make more sense to define in secret.go?
| // obtain references to all the informers used by this controller | ||
| certificateInformer := cmFactory.Certmanager().V1alpha2().Certificates() | ||
| certificateRequestInformer := cmFactory.Certmanager().V1alpha2().CertificateRequests() | ||
| secretsInformer := factory.Core().V1().Secrets() |
There was a problem hiding this comment.
Missing event handler watching Secret resources named as status.nextPrivateKeySecretName
| return err | ||
| } | ||
|
|
||
| secretExists := true |
There was a problem hiding this comment.
nit: secretExists := (secret != nil) 🤷♂
| if err != nil { | ||
| return err | ||
| } | ||
| if reflect.DeepEqual(secret, newSecret) { |
There was a problem hiding this comment.
This behaviour does make sense, although I wonder if it will 'hide' real errors in our code/state machine.. perhaps one to try and tackle later, but for now maybe add a comment and possibly a log message in case this case occurs? (maybe at DebugLevel..)
There was a problem hiding this comment.
Hm, this check also won't work when P12/JKS support is enabled, as these formats use a random parameter when encrypting the keystore meaning the secretes will never be equal despite containing equal private keys/certs.
Given that, and given we don't have exhaustive testing around P12/JKS right now to identify how serious of an issue this is, an implementation that doesn't rely on this check would be best if possible!
| // If secret does not exist then create it | ||
| if !secretExists { | ||
| _, err = c.kubeClient.CoreV1().Secrets(newSecret.Namespace).Create(secretCtx, newSecret, metav1.CreateOptions{}) | ||
| if err != nil { |
There was a problem hiding this comment.
Replace with return err regardless of err != nil
| return err | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
nit: return err and remove the above check
| delete(s.Annotations, cmapi.IPSANAnnotationKey) | ||
| delete(s.Annotations, cmapi.URISANAnnotationKey) | ||
| } else { | ||
| x509Cert, err := utilpki.DecodeX509CertificateBytes(data.cert) |
There was a problem hiding this comment.
Just a note: this will be problematic in future once we begin natively support JKS and P12 as the DecodeX509CertificateBytes method will not be 'taught' how to decode these keystore formats (due to the complexity of manage encryption keys in this area of code).
Instead, we should be relying on the certificate data stored on the CertificateRequest and the private key data stored in the nextPrivateKeySecretName. Right now, in all cases, these are the same, so the code is safe.
But we're going to need to refactor all this a bit once we go to add P12 and JKS support 😄
Happy to merge without changes for now, but just a quick note! 😄
|
Some integration and unit tests to cover this controller would also be great 😄 |
jetstack-bot
added
the
needs-rebase
JoshVanL
force-pushed
the
certificate-issuing-controller
branch
2 times, most recently
from
689878c
to
0ccd364
Compare
JoshVanL
changed the title
jetstack-bot
removed
the
do-not-merge/work-in-progress
munnerz
force-pushed
the
certificate-issuing-controller
branch
from
2d375ee
to
544b028
Compare
jetstack-bot
removed
the
needs-rebase
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
Signed-off-by: James Munnelly <james@munnelly.eu>
munnerz
force-pushed
the
certificate-issuing-controller
branch
from
544b028
to
572e467
Compare
Signed-off-by: James Munnelly <james@munnelly.eu>
|
I'm going to be following up with additional PRs around this (+ additional controllers) 😄 /lgtm |
/cc @munnerz
This controller is used to implement the issuing controller, as described in the extensible certificates controller proposal here #2753. This controller is responsible for writing the resulting certificate, ca, and private key to the target certificate resource, in the correct fromat.