Merge pull request #7 from redbaron/challenge-recheck

Check challenge before presenting it

pkg/issuer/acme/acme.go

@@ -71,6 +71,10 @@ type solver interface {
 	// needs to create any new resources, it can set the appropriate owner
 	// reference
 	Present(ctx context.Context, crt *v1alpha1.Certificate, ch v1alpha1.ACMEOrderChallenge) error
+
+	// Check should return Error only if propagation check cannot be performed.
+	// It MUST return `false, nil` if can contact all relevant services and all is
+	// doing is waiting for propagation
 	Check(ch v1alpha1.ACMEOrderChallenge) (bool, error)
 	CleanUp(ctx context.Context, crt *v1alpha1.Certificate, ch v1alpha1.ACMEOrderChallenge) error
 }

pkg/issuer/acme/dns/util/wait.go

@@ -86,7 +86,8 @@ func checkAuthoritativeNss(fqdn, value string, nameservers []string) (bool, erro
 			return false, err
 		}
 
-		if r.Rcode != dns.RcodeSuccess {
+		// NXDomain response is not really an error, just waiting for propagation to happen
+		if !(r.Rcode == dns.RcodeSuccess || r.Rcode == dns.RcodeNameError) {
 			return false, fmt.Errorf("NS %s returned %s for %s", ns, dns.RcodeToString[r.Rcode], fqdn)
 		}
 
@@ -102,7 +103,7 @@ func checkAuthoritativeNss(fqdn, value string, nameservers []string) (bool, erro
 		}
 
 		if !found {
-			return false, fmt.Errorf("nameserver %q did not return the expected TXT record for domain %q", ns, fqdn)
+			return false, nil
 		}
 	}
 

pkg/issuer/acme/prepare.go

@@ -170,11 +170,9 @@ func (a *Acme) presentOrder(ctx context.Context, cl client.Interface, crt *v1alp
 
 // presentChallenge will process a challenge by talking to the acme server and
 // obtaining up to date status information.
-// If the challenge is still in a pending state, it will 'present' the
-// challenge using the appropriate solver.
-// It will then run the 'Check' function in order to determine whether the
-// challenge has propegated (e.g. to DNS resolvers, or to the ingress
-// controller).
+// If the challenge is still in a pending state, it will first check propagation
+// status of a challenge from previous attempt, and if missing it will 'present' the
+// new challenge using the appropriate solver.
 // If the check fails, an error will be returned.
 // Otherwise, it will return nil.
 func (a *Acme) presentChallenge(ctx context.Context, cl client.Interface, crt *v1alpha1.Certificate, ch v1alpha1.ACMEOrderChallenge) error {
@@ -202,20 +200,26 @@ func (a *Acme) presentChallenge(ctx context.Context, cl client.Interface, crt *v
 		return err
 	}
 
+	ok, err := solver.Check(ch)
+	if err != nil {
+		return err
+	}
+
+	if ok {
+		return nil
+	}
+
+	// TODO: make sure that solver.Present is noop if challenge
+	//       is already present and all we do is waiting for propagation,
+        //       otherwise it is spamming with errors which are not really erros
+        //       as we are just waiting for propagation
 	err = solver.Present(ctx, crt, ch)
 	if err != nil {
 		return err
 	}
 
-	ok, err := solver.Check(ch)
-	if err != nil {
-		return err
-	}
-	if !ok {
-		return fmt.Errorf("%s self check failed for domain %q", ch.Type, ch.Domain)
-	}
-
-	return nil
+	// Error is only way to indicate that we are ready to ACME Accept challenge
+	return fmt.Errorf("% challenge was created for domain %q, waiting for propagation", ch.Type, ch.Domain)
 }
 
 func (a *Acme) cleanupLastOrder(ctx context.Context, crt *v1alpha1.Certificate) error {