From c44a7552ea661b741d2802092c8be4b43d23876b Mon Sep 17 00:00:00 2001
From: Maxim Ivanov <ivanov.maxim@gmail.com>
Date: Wed, 11 Apr 2018 11:27:23 +0100
Subject: [PATCH] Check challenge before presenting it

With async challenge Check, it is often happens,
that solver.Check() fails on first run after solver.Present()

Cert-manager then tries again, but starts with solver.Present(),
which not being idempotent right now fails on certain DNS providers.

This change swaps order of solver.Check() and solver.Present().
Check is not returning error if propagation not happened, it then
allows Present() to run.

In the current form, Present() will be spamming with errors,
but this doesn't stop Check from happening on every attempt,
so eventually Challenge can be verified and accepted. In the future,
Present() should be made idempotent.
---
 pkg/issuer/acme/acme.go          |  4 ++++
 pkg/issuer/acme/dns/util/wait.go |  5 +++--
 pkg/issuer/acme/prepare.go       | 32 ++++++++++++++++++--------------
 3 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/pkg/issuer/acme/acme.go b/pkg/issuer/acme/acme.go
index 300066146..131db4079 100644
--- a/pkg/issuer/acme/acme.go
+++ b/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
 }
diff --git a/pkg/issuer/acme/dns/util/wait.go b/pkg/issuer/acme/dns/util/wait.go
index 5583bd8ed..739fd5eb8 100644
--- a/pkg/issuer/acme/dns/util/wait.go
+++ b/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
 		}
 	}
 
diff --git a/pkg/issuer/acme/prepare.go b/pkg/issuer/acme/prepare.go
index a5c117f0e..72ffaa44b 100644
--- a/pkg/issuer/acme/prepare.go
+++ b/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 {