Merge pull request #111 from Dstack-TEE/fix/ingress-renew-scope-and-timeout

fix(dstack-ingress): renew one lineage per run, and size the timeout to the wait

Author: kvinwang

custom-domain/dstack-ingress/README.md

@@ -192,7 +192,8 @@ environment:
 | `EVIDENCE_PORT` | `80` | Internal port for evidence HTTP server |
 | `ALPN` | | TLS ALPN protocols (e.g. `h2,http/1.1`). Only set if backends support h2c |
 | `DELEGATION_ZONE` | | Zone this container writes into, so the DNS token needs no access to the served domain's own zone (see below) |
-| `DELEGATION_PROPAGATION_SECONDS` | `120` | Wait after writing the delegated challenge TXT before validation. Must outlast the record TTL (60s), or a resolver still serving the previous attempt's value fails validation. Keep well under ~250s — certbot is killed after a 300s per-run timeout |
+| `DELEGATION_PROPAGATION_SECONDS` | `120` | Wait after writing the delegated challenge TXT before validation. Must outlast the record TTL (60s), or a resolver still serving the previous attempt's value fails validation. The certbot run timeout is sized from this, so raising it is safe |
+| `CERTBOT_TIMEOUT` | propagation wait + 180s, never below 300s | Seconds a single certbot run may take. The default is derived from the provider's propagation wait (or `DELEGATION_PROPAGATION_SECONDS`), which is what dominates it. Set this only if a run needs longer still; an explicit value is used as given, floor included |
 
 For DNS provider credentials, see [DNS_PROVIDERS.md](DNS_PROVIDERS.md).
 

custom-domain/dstack-ingress/scripts/certman.py

@@ -24,6 +24,17 @@ def staging_enabled() -> bool:
     return value == "true"
 
 
+# Time a certbot run needs on top of the DNS propagation wait: ACME round
+# trips, plugin setup, the cleanup hook, and certbot's own bookkeeping.
+CERTBOT_TIMEOUT_HEADROOM = 180
+
+# Never allow less than the cap this replaced. route53 declares no propagation
+# wait at all, so sizing purely from the wait would cut its budget from 300s to
+# 180s and fail renewals that used to fit. The sizing exists to raise the cap
+# where a provider needs more, not to lower it anywhere.
+CERTBOT_TIMEOUT_FLOOR = 300
+
+
 class CertManager:
     """Certificate management using DNS provider infrastructure."""
 
@@ -303,6 +314,8 @@ def _build_certbot_command(self, action: str, domain: str, email: str) -> List[s
             # For `renew`, certbot reuses the authenticator + hooks saved in the
             # renewal config from the initial `certonly`, so we don't re-specify
             # them here (and must not fall back to the DNS plugin).
+            if action == "renew":
+                base_cmd.extend(["--cert-name", self._lineage_name(domain)])
             if staging_enabled():
                 base_cmd.append("--staging")
             masked = [a if not (i > 0 and base_cmd[i - 1] == "--email") else "<email>"
@@ -342,6 +355,12 @@ def _build_certbot_command(self, action: str, domain: str, email: str) -> List[s
             else:
                 base_cmd.append("--register-unsafely-without-email")
             base_cmd.extend(["-d", domain])
+        if action == "renew":
+            # Without this, `certbot renew` renews *every* lineage in
+            # /etc/letsencrypt/renewal. run_pass calls this once per domain, so
+            # N domains meant N passes over all N lineages, and the result was
+            # then reported against whichever domain happened to ask.
+            base_cmd.extend(["--cert-name", self._lineage_name(domain)])
         if staging_enabled():
             base_cmd.extend(["--staging"])
 
@@ -380,10 +399,11 @@ def obtain_certificate(self, domain: str, email: str) -> bool:
             return False
 
         cmd = self._build_certbot_command("certonly", domain, email)
+        timeout = self._certbot_timeout()
 
         try:
             result = subprocess.run(
-                cmd, capture_output=True, text=True, timeout=300)
+                cmd, capture_output=True, text=True, timeout=timeout)
 
             if result.returncode == 0:
                 print(f"✓ Certificate obtained successfully for {domain}")
@@ -412,7 +432,8 @@ def obtain_certificate(self, domain: str, email: str) -> bool:
                 return False
 
         except subprocess.TimeoutExpired:
-            print(f"Certbot command timed out after 300 seconds", file=sys.stderr)
+            print(f"Certbot command timed out after {timeout} seconds",
+                  file=sys.stderr)
             return False
         except Exception as e:
             print(f"Error running certbot: {e}", file=sys.stderr)
@@ -433,10 +454,11 @@ def renew_certificate(self, domain: str) -> Tuple[bool, bool]:
 
         self.apply_renewal_window(domain)
         cmd = self._build_certbot_command("renew", domain, "")
+        timeout = self._certbot_timeout()
 
         try:
             result = subprocess.run(
-                cmd, capture_output=True, text=True, timeout=300)
+                cmd, capture_output=True, text=True, timeout=timeout)
 
             stdout_output = result.stdout.strip() if result.stdout else ""
             error_output = result.stderr.strip() if result.stderr else ""
@@ -468,6 +490,14 @@ def renew_certificate(self, domain: str) -> Tuple[bool, bool]:
 
                 return False, False
 
+        except subprocess.TimeoutExpired:
+            print(
+                f"Certbot renew for {domain} timed out after {timeout} seconds. "
+                f"If this provider needs a longer DNS propagation wait, raise "
+                f"CERTBOT_TIMEOUT.",
+                file=sys.stderr,
+            )
+            return False, False
         except Exception as e:
             print(f"Error running certbot: {e}", file=sys.stderr)
             return False, False
@@ -486,6 +516,35 @@ def _renewal_conf_path(self, domain: str) -> str:
         """Where certbot keeps this lineage's renewal config."""
         return f"/etc/letsencrypt/renewal/{self._lineage_name(domain)}.conf"
 
+    def _certbot_timeout(self) -> int:
+        """How long one certbot run may take.
+
+        The dominant term is the DNS propagation wait, which the plugin -- or,
+        under delegation, the auth hook -- sleeps through inside the process.
+        A single fixed cap cannot fit every provider: linode's own default
+        propagation is 300s, so a 300s cap could never be met and dns-01 on
+        linode timed out every time, on issuance as well as renewal.
+
+        Size the cap from the wait instead, and let a deployment override it.
+        Never below CERTBOT_TIMEOUT_FLOOR: a provider that declares no
+        propagation wait (route53) still spends time on ACME round trips, and
+        this change should not shorten anyone's budget. An explicit
+        CERTBOT_TIMEOUT is taken literally -- that one is the operator's call.
+        """
+        override = os.environ.get("CERTBOT_TIMEOUT", "").strip()
+        if override.isdigit() and int(override) > 0:
+            return int(override)
+
+        if os.environ.get("DELEGATION_ZONE", "").strip():
+            # Kept in step with acme-dns-alias-hook.sh, which does the sleeping.
+            wait = os.environ.get("DELEGATION_PROPAGATION_SECONDS", "").strip()
+            propagation = int(wait) if wait.isdigit() else 120
+        else:
+            propagation = getattr(
+                self.provider, "CERTBOT_PROPAGATION_SECONDS", None) or 0
+
+        return max(CERTBOT_TIMEOUT_FLOOR, propagation + CERTBOT_TIMEOUT_HEADROOM)
+
     def apply_renewal_window(self, domain: str) -> None:
         """Make RENEW_DAYS_BEFORE mean the same thing here as it does for lego.
 

custom-domain/dstack-ingress/scripts/tests/test_certman.py

@@ -149,6 +149,146 @@ def test_wildcard_uses_the_bare_lineage_name(self):
         )
 
 
+class FakeProvider:
+    CERTBOT_PLUGIN = "dns-cloudflare"
+    CERTBOT_CREDENTIALS_FILE = None
+    CERTBOT_PROPAGATION_SECONDS = 120
+
+
+def _command_manager(propagation=120) -> certman.CertManager:
+    """A CertManager that can build commands without a real provider or venv."""
+    mgr = object.__new__(certman.CertManager)
+    provider = FakeProvider()
+    provider.CERTBOT_PROPAGATION_SECONDS = propagation
+    mgr.provider = provider
+    mgr.provider_type = "cloudflare"
+    mgr._get_certbot_command = lambda: ["certbot"]  # noqa: SLF001
+    return mgr
+
+
+class EnvTestCase(unittest.TestCase):
+    """Restore every environment variable a test touches."""
+
+    VARS = (
+        "CERTBOT_TIMEOUT",
+        "DELEGATION_ZONE",
+        "DELEGATION_PROPAGATION_SECONDS",
+        "ACME_STAGING",
+        "CERTBOT_STAGING",
+    )
+
+    def setUp(self):
+        self._saved = {v: os.environ.get(v) for v in self.VARS}
+        for v in self.VARS:
+            os.environ.pop(v, None)
+
+    def tearDown(self):
+        for v, old in self._saved.items():
+            if old is None:
+                os.environ.pop(v, None)
+            else:
+                os.environ[v] = old
+
+
+class RenewScopeTest(EnvTestCase):
+    """`certbot renew` renews every lineage unless told otherwise.
+
+    run_pass calls this once per domain, so without --cert-name N domains meant
+    N runs over all N lineages, and each run's result was reported against
+    whichever domain happened to ask for it.
+    """
+
+    def test_renew_is_scoped_to_one_lineage(self):
+        cmd = _command_manager()._build_certbot_command("renew", "a.example.com", "")
+        self.assertIn("--cert-name", cmd)
+        self.assertEqual(cmd[cmd.index("--cert-name") + 1], "a.example.com")
+
+    def test_renew_uses_the_bare_name_for_a_wildcard(self):
+        cmd = _command_manager()._build_certbot_command("renew", "*.example.com", "")
+        self.assertEqual(cmd[cmd.index("--cert-name") + 1], "example.com")
+
+    def test_certonly_is_scoped_by_d_not_cert_name(self):
+        cmd = _command_manager()._build_certbot_command(
+            "certonly", "a.example.com", "")
+        self.assertNotIn("--cert-name", cmd)
+        self.assertIn("-d", cmd)
+
+    def test_delegation_renew_is_scoped_too(self):
+        os.environ["DELEGATION_ZONE"] = "deleg.example.net"
+        cmd = _command_manager()._build_certbot_command("renew", "a.example.com", "")
+        self.assertEqual(cmd[cmd.index("--cert-name") + 1], "a.example.com")
+        # renew must not re-specify the authenticator; it is in the lineage.
+        self.assertNotIn("--manual", cmd)
+
+
+class CertbotTimeoutTest(EnvTestCase):
+    """A fixed 300s cap could not fit every provider's own propagation wait."""
+
+    def test_timeout_covers_the_propagation_wait(self):
+        self.assertEqual(
+            _command_manager(propagation=400)._certbot_timeout(),
+            400 + certman.CERTBOT_TIMEOUT_HEADROOM,
+        )
+
+    def test_linode_default_propagation_fits(self):
+        # linode's CERTBOT_PROPAGATION_SECONDS is 300, so the old 300s cap could
+        # never be met: issuance and renewal timed out every time.
+        timeout = _command_manager(propagation=300)._certbot_timeout()
+        self.assertGreater(timeout, 300)
+
+    def test_delegation_uses_its_own_propagation_setting(self):
+        os.environ["DELEGATION_ZONE"] = "deleg.example.net"
+        os.environ["DELEGATION_PROPAGATION_SECONDS"] = "240"
+        self.assertEqual(
+            _command_manager()._certbot_timeout(),
+            240 + certman.CERTBOT_TIMEOUT_HEADROOM,
+        )
+
+    def test_delegation_falls_back_to_the_hook_default(self):
+        os.environ["DELEGATION_ZONE"] = "deleg.example.net"
+        self.assertEqual(
+            _command_manager()._certbot_timeout(),
+            max(certman.CERTBOT_TIMEOUT_FLOOR,
+                120 + certman.CERTBOT_TIMEOUT_HEADROOM),
+        )
+
+    def test_explicit_override_wins(self):
+        os.environ["CERTBOT_TIMEOUT"] = "900"
+        self.assertEqual(_command_manager(propagation=300)._certbot_timeout(), 900)
+
+    def test_invalid_override_is_ignored(self):
+        for bad in ("0", "-5", "soon", ""):
+            os.environ["CERTBOT_TIMEOUT"] = bad
+            self.assertEqual(
+                _command_manager(propagation=400)._certbot_timeout(),
+                400 + certman.CERTBOT_TIMEOUT_HEADROOM,
+                f"{bad!r} should be ignored",
+            )
+
+    def test_an_override_below_the_floor_is_still_honoured(self):
+        os.environ["CERTBOT_TIMEOUT"] = "60"
+        self.assertEqual(_command_manager(propagation=300)._certbot_timeout(), 60)
+
+    def test_provider_without_propagation_keeps_the_old_budget(self):
+        """route53 declares no propagation wait.
+
+        Sizing purely from the wait would cut it from 300s to 180s and fail
+        renewals that used to fit -- a regression introduced by the fix.
+        """
+        self.assertEqual(
+            _command_manager(propagation=None)._certbot_timeout(),
+            certman.CERTBOT_TIMEOUT_FLOOR,
+        )
+
+    def test_no_provider_ends_up_below_the_previous_fixed_cap(self):
+        for propagation in (None, 0, 30, 120, 300):
+            self.assertGreaterEqual(
+                _command_manager(propagation=propagation)._certbot_timeout(),
+                300,
+                f"propagation={propagation} shortened the budget",
+            )
+
+
 class NoDuplicateDefinitionsTest(unittest.TestCase):
     """Python shadows a repeated class or method silently; unittest counts the
     later one and the total still goes up, so a duplicated block looks like