Surface self-check errors in challenge resource #1244
Conversation
Signed-off-by: Daniel Morsing <dmo@jetstack.io>
Signed-off-by: Daniel Morsing <dmo@jetstack.io>
Signed-off-by: Daniel Morsing <dmo@jetstack.io>
Body read errors are just regular errors, so we can reclassify them all into absorb errors Since we only have absorb errors, flip the switch so that all errors are absorbed. This will make it easier to surface errors into the controller. Signed-off-by: Daniel Morsing <dmo@jetstack.io>
This makes the check function into a simple precondition Signed-off-by: Daniel Morsing <dmo@jetstack.io>
Signed-off-by: Daniel Morsing <dmo@jetstack.io>
jetstack-bot
added
release-note
jetstack-bot
added
the
area/acme
| if !ok { | ||
| ch.Status.Reason = fmt.Sprintf("Waiting for %s challenge propagation", ch.Spec.Type) | ||
| glog.Infof("propagation check failed: %v", err) | ||
| ch.Status.Reason = fmt.Sprintf("Waiting for %s challenge propagation: %s", ch.Spec.Type, err) |
There was a problem hiding this comment.
either will work here, since error can always be converted into a string
|
|
||
| providerConfig, err := issuer.GetSpec().ACME.DNS01.Provider(providerName) | ||
| fqdn, value, ttl, err := util.DNS01Record(ch.Spec.DNSName, ch.Spec.Key, s.DNS01Nameservers, false) |
There was a problem hiding this comment.
As discussed on Slack, the change from followCNAME(providerConfig.CNAMEStrategy) to false makes sense as we should be relying on the recursive DNS server to resolve CNAMEs for us anyway, so we needn't ever resolve CNAMEs ourselves as part of the DNS01 self check.
| fqdn, value, ttl, err := util.DNS01Record(ch.Spec.DNSName, ch.Spec.Key, s.DNS01Nameservers, followCNAME(providerConfig.CNAMEStrategy)) | ||
| if err != nil { | ||
| return false, err | ||
| return err | ||
| } | ||
|
|
||
| glog.Infof("Checking DNS propagation for %q using name servers: %v", ch.Spec.DNSName, s.Context.DNS01Nameservers) | ||
|
|
||
| ok, err := util.PreCheckDNS(fqdn, value, s.Context.DNS01Nameservers, |
There was a problem hiding this comment.
This call here is what performs the actual self check - I think we need to make a similar change to PreCheckDNS as we have done to this function, to make it only return an error type. I may be wrong though?!
Otherwise the only information we'll ever surface is "DNS record for 'example.com' not yet propagated" (instead of some helpful error surfaced from the actual checker)
There was a problem hiding this comment.
PreCheckDNS turns various conditions into errors, so the only time we'll get the "not yet propagated" error is when we had no errors but the TXT record was missing
pkg/issuer/acme/http/http_test.go
Outdated
| }, | ||
| expectedErr: true, | ||
| expectedErr: false, |
There was a problem hiding this comment.
I think the test case names here need updating? should error with expectedErr: false seems a weird combination to me.
There was a problem hiding this comment.
Also, is there ever a case where expectedErr == true now?
There was a problem hiding this comment.
Having another look at the tests. I think they should be valid, but they might be made simpler now that we aren't testing the OK logic
Signed-off-by: Daniel Morsing <dmo@jetstack.io>
Signed-off-by: Daniel Morsing <dmo@jetstack.io>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: munnerz 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
the
approved
|
/retest |
1 similar comment
|
/retest |
What this PR does / why we need it:
This PR surfaces self-check errors in the challenge resource, making it more obvious what is holding up issuance of a certificate.
Which issue this PR fixes:
fixes #1208
Special notes for your reviewer:
This PR works by changing the
Checkmethod on thesolverinterface. Instead of having a (retry, error) tuple, we eliminate errors from the various checkers, making it so that the only type of error returned is a retryable one. Because of this change, the PR looks like it changes a lot more things than it actually does. If a change seems weird, I suggest looking through the commits one-by-oneRelease note: