Add provided certificate checking before LE certificate generation with OnHostRule option #1772
Conversation
There was a problem hiding this comment.
Hey @nmengin !
Ideally, adding (integration) tests would be better on this, but overall, seems good :)
|
@emilevauge I am agree with you for the tests. We believe that it can be nice to upgrade the boulder image and create ACME tests into a new PR. |
integration/acme_test.go
Outdated
| "time" | ||
|
|
||
| "github.com/go-check/check" | ||
|
|
||
| "errors" | ||
|
|
acme/acme_test.go
Outdated
| @@ -9,6 +9,9 @@ import ( | |||
| "testing" | |||
| "time" | |||
|
|
|||
| "crypto/tls" | |||
integration/acme_test.go
Outdated
| ) | ||
|
|
||
| // ACME test suites (using libcompose) | ||
| type AcmeSuite struct { | ||
| BaseSuite | ||
| onDemand bool |
There was a problem hiding this comment.
I think it's better to create a dedicated struct.
There was a problem hiding this comment.
The others tests use one struct for BaseSuite and all the arguments they need.
I used only one to be homogeneous, but I can changed it if it's really necessary.
There was a problem hiding this comment.
they provide data shared between test (from the setup), not for tests data.
nmengin
force-pushed
the
fix_provided_certificates_checking_for_OnHostRule
branch
from
3cd5ec5
to
266fbd2
Compare
ldez
force-pushed
the
fix_provided_certificates_checking_for_OnHostRule
branch
from
266fbd2
to
d986a1e
Compare
…ificate when OnHostRule is activated - ADD TI to check the new behaviour with onHostRule and provided certificates - ADD TU on the getProvidedCertificate method
ldez
force-pushed
the
fix_provided_certificates_checking_for_OnHostRule
branch
from
d986a1e
to
bdab298
Compare
During the code review of PR #1260, it appears that the provided certificates are not checked before LE certificate checking with the
OnHostRuleoption.Adding this check allows to generate a LE certificate only if domains have no provided certificates.
Thanks to it, PR #1260 will be modified to apply filter on domains to check by provided certificate in place of domains to ignore during LE certificate generation.
cc/ @CyrilPeponnet