-
Notifications
You must be signed in to change notification settings - Fork 690
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
Ensure certgen handles already-existing secrets correctly #2178
Conversation
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.
LGTM!
@@ -103,6 +104,10 @@ func writeCACertKube(client *kubernetes.Clientset, namespace string, cert []byte | |||
secret := newCertOnlySecret("cacert", namespace, "cacert.pem", cert) | |||
_, err := client.CoreV1().Secrets(namespace).Create(secret) | |||
if err != nil { | |||
if k8serrors.IsAlreadyExists(err) { |
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.
Nit: would this style be more canonical?
if _, err := client.CoreV1().Secrets(namespace).Create(secret); if err != nil {
...
}
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.
Won't hurt. I'll change it.
@youngnick Is there a place to document this? |
It's what we always said the behaviour was, and the inspiration behind having certgen being a Job in the first place. I've obviously missed a big test case when doing this originally, 🤦♂. I guess my answer is, "I'm not sure". |
9ef3cec
to
cf06214
Compare
On 30 Jan 2020, at 2:02 pm, Nick Young ***@***.***> wrote:
I guess my answer is, "I'm not sure".
Then should we file an issue to document certgen?
|
#2183 created. I'll merge this now and work on that. |
Fixes projectcontour#2150 Signed-off-by: Nick Young <[email protected]>
cf06214
to
679d061
Compare
Does certgen recreates existing certificates if expired? |
On Jan 31, 2020, at 4:28 PM, Ryan Elian ***@***.***> wrote:
Does certgen recreates existing certificates if expired?
No. To regenerate the certificates you would need to delete the secrets first.
|
Fixes #2150 by ensuring that the
contour certgen
Job will succeed if the secrets already exist.Signed-off-by: Nick Young [email protected]