implement renewal of pki certs with min_seconds_remaining argument #386
Conversation
ghost
added
joemiller
force-pushed
the
renew-expiring-cert
branch
from
92c651c
to
7e65908
Compare
|
tests are passing now. The issue was that the test case's pki role has a 1 hour max ttl, but min_seconds_remaining defaults to 7days. This caused the plan to show pending changes which the test was not expecting. I added |
|
Any feedback on this PR? thanks |
There was a problem hiding this comment.
Hi @joemiller , thanks for working on this! It's a really neat feature.
The main thing I'm thinking about as I read this is backwards compatibility. If folks were already using this resource and weren't expecting it to automatically renew the certificates, it could be a surprise when they adopted the next version of the provider and it suddenly started generating the certificates. Probably for some organizations, the person using Terraform for this isn't necessarily the person who should be creating new certificates.
Would it be possible to have this default to being off? And it could only be used by someone actively adding a new field like auto_renew_certs or something?
|
|
||
| renewTime := expireTime.Add(-time.Duration(minSeconds) * time.Second) | ||
| if time.Now().After(renewTime) { | ||
| log.Printf("[DEBUG] certificate %q is due for renewal, expires %s, renewal time %s", d.Id(), expireTime, renewTime) |
There was a problem hiding this comment.
It would be great to get some test coverage of this code path. I see that the rest of the code in this method is currently hit, but not in here.
There was a problem hiding this comment.
ok, i'll work on that
| func pkiSecretBackendCertRead(d *schema.ResourceData, meta interface{}) error { | ||
| return nil | ||
| } | ||
|
|
||
| func pkiSecretBackendCertUpdate(d *schema.ResourceData, m interface{}) error { | ||
| // If the certificate or private_key have been marked as changed by our custom diff function we need | ||
| // to create a new certificate and key | ||
| if d.HasChange("certificate") || d.HasChange("private_key") { |
There was a problem hiding this comment.
This code path doesn't appear to have test coverage yet. Would be great to get some here too.
| } | ||
|
|
||
| log.Printf("[DEBUG] certificate %q is not due for renewal, expires %s, renewal time %s", d.Id(), expireTime, renewTime) | ||
| return nil |
There was a problem hiding this comment.
It would be great to get test coverage on all the code changes in this file too - the new fields that have been added, and the certificate that's generated.
|
@tyrannosaurus-becks I am happy to make this opt-in for backwards compatibility. i'll work on the requested changes |
joemiller
force-pushed
the
renew-expiring-cert
branch
from
7e65908
to
524a225
Compare
|
@tyrannosaurus-becks I've added an I also include a commit with some doc fixes. 1) cleaned up some trailing whitespace and 2) "ttl - time to leave" I am assuming should be "time to live" per the related vault docs |
joemiller
force-pushed
the
renew-expiring-cert
branch
from
7bbe48e
to
e3f7c2a
Compare
joemiller
force-pushed
the
renew-expiring-cert
branch
from
e3f7c2a
to
d44dc12
Compare
|
updated to remove vendor/vendor.json which i noticed i had accidentally re-added in my most recent rebase from master |
Fixes: ``` go: extracting github.com/mitchellh/hashstructure v0.0.0-20160209213820-6b17d669fac5 verifying github.com/hashicorp/[email protected]: checksum mismatch downloaded: h1:9HVkPxOpo+yO93Ah4yrO67d/qh0fbLLWbKqhYjyHq9A= go.sum: h1:VBj0QYQ0u2MCJzBfeYXGexnAl17GsH1yidnoxCqqD9E= ```
implement renewal of pki certs with min_seconds_remaining argument
This implements renewal for certificates in the
vault_pki_secret_backend_certandvault_pki_secret_backend_signresources. Related issue: #353Some notes on the implementation:
Adds a new optional argument
min_seconds_remaining. The default is 7 days (604800s). If the certificate'sexpirationis within this many seconds a new certificate will be fetched from Vault. The reason I chose to go with seconds instead of days was to cover cases where short-lived (perhaps less than a day) certificates are desired. I suppose "hours" might be a reasonable choice here as well. Days would be too long.Adds a new computed attribute
expirationto these resources. This value was already being returned by the Vault API and now it will be stored in TF state.Based loosely off of the way in which the acme TF provider handles cert refresh, except they use
min_days_remaining. This seems fine for certs issued by public CAs but some internal CAs may desire shorter expiration - https://github.com/terraform-providers/terraform-provider-acme/blob/0077b51dadc8b9fc4485e5b2a2844dedb063d9ae/acme/resource_acme_certificate.go#L83-L109I bumped the
mitchellh/copystructureandmitchellh/reflectwalkvendor'd deps tov1.0.0. I ran into a bugpanic: interface conversion: interface {} is schema.schemaMap, not *schema.schemaMapwhen implementing the custom Diff funcs and this was the resolution.