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

Two problems that compound each other.

`certbot renew` renews *every* lineage in /etc/letsencrypt/renewal unless
given --cert-name, and we never gave it one. run_pass calls this once per
domain, so N domains meant N runs each covering all N lineages -- and
renew_certificate then reported the result against whichever domain
happened to ask, so domain A could be recorded as renewed because domain
B was. Scope each run to the lineage it is named for.

The run timeout was a hard-coded 300s. The dominant term inside a run is
the DNS propagation wait, which the plugin sleeps through in-process --
and linode's own CERTBOT_PROPAGATION_SECONDS is 300, so on linode the
cap could never be met and dns-01 timed out every time, on issuance as
well as renewal. Before --cert-name it was worse still: the cap covered
all lineages at once, so three cloudflare domains (120s each) could not
fit either. Size it from the wait, and let CERTBOT_TIMEOUT override.

--cert-name does not change the no-op wording renew_certificate parses:
"No renewals were attempted." comes from _renew_describe_results, which
runs for `renew` whether or not the lineage set was filtered. Checked
against certbot 5.7.0, the version the image installs.

Tested: 11 new cases in scripts/tests/test_certman.py covering scope
(plain, wildcard, delegation, and certonly staying on -d) and the
timeout (per-provider, delegation, override, invalid override, and a
provider with no propagation setting). All five that assert the new
behaviour fail against the previous code.

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 | 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 |
 
 For DNS provider credentials, see [DNS_PROVIDERS.md](DNS_PROVIDERS.md).
 

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

@@ -24,6 +24,11 @@ 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
+
+
 class CertManager:
     """Certificate management using DNS provider infrastructure."""
 
@@ -303,6 +308,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 +349,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 +393,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 +426,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 +448,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 +484,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 +510,31 @@ 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.
+        """
+        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 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

@@ -123,6 +123,129 @@ 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=120)._certbot_timeout(),
+            120 + 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"] = "200"
+        self.assertEqual(
+            _command_manager()._certbot_timeout(),
+            200 + 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(),
+            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=120)._certbot_timeout(),
+                120 + certman.CERTBOT_TIMEOUT_HEADROOM,
+                f"{bad!r} should be ignored",
+            )
+
+    def test_provider_without_propagation_still_gets_headroom(self):
+        # route53 sets CERTBOT_PROPAGATION_SECONDS = None.
+        self.assertEqual(
+            _command_manager(propagation=None)._certbot_timeout(),
+            certman.CERTBOT_TIMEOUT_HEADROOM,
+        )
+
+
 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