Merge pull request #110 from Dstack-TEE/fix/ingress-renew-days-removable

fix(dstack-ingress): let RENEW_DAYS_BEFORE be unset again

Author: kvinwang

custom-domain/dstack-ingress/README.md

@@ -181,7 +181,7 @@ environment:
 | `DOH_RESOLVERS` | Google + Cloudflare | Comma-separated DoH endpoints used to verify records |
 | `TLSALPN_PORT` | `9443` | Loopback port lego's ACME responder binds to |
 | `TLS_TERMINATE_PORT` | `9444` | Loopback port the TLS frontend moves to in tls-alpn-01 mode |
-| `RENEW_DAYS_BEFORE` | client default | Days of remaining lifetime that trigger renewal. Applies to both modes: passed to lego as `--renew-days`, and written to certbot's `renew_before_expiry` |
+| `RENEW_DAYS_BEFORE` | client default | Days of remaining lifetime that trigger renewal. Applies to both modes: passed to lego as `--renew-days`, and written to certbot's `renew_before_expiry`. Unsetting it removes that setting again, so the client's own default applies |
 | `RENEW_INTERVAL` | `43200` | Seconds between successful certificate passes |
 | `DNS_SETTLE_SECONDS` | `30` | Wait after DNS verifies, so the gateway's own TXT cache expires |
 | `MAXCONN` | `4096` | HAProxy max connections |

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

@@ -482,6 +482,10 @@ def _lineage_name(domain: str) -> str:
         """certbot stores a wildcard lineage under the bare name."""
         return domain[2:] if domain.startswith("*.") else domain
 
+    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 apply_renewal_window(self, domain: str) -> None:
         """Make RENEW_DAYS_BEFORE mean the same thing here as it does for lego.
 
@@ -491,56 +495,103 @@ def apply_renewal_window(self, domain: str) -> None:
         runs. Without this the variable is silently tls-alpn-01 only, which also
         left the dns-01 renewal branch with no way to be exercised on demand.
 
-        Leaving it unset keeps certbot's own default (30 days).
+        Unsetting it has to remove the setting again, not merely stop writing
+        it: the lineage config lives in the certificate volume and outlives the
+        container, so a value written once would otherwise be permanent, and
+        the documented way to leave the renewal window -- unset the variable --
+        would do nothing while the certificate stayed permanently due.
+
+        This assumes the container owns the lineage config: clearing removes
+        any active renew_before_expiry, including one a human put there, since
+        nothing distinguishes the two. That holds for the volume this image
+        manages. Mounting a certbot directory maintained elsewhere is out of
+        scope -- set RENEW_DAYS_BEFORE to the value you want in that case,
+        rather than leaving it unset and expecting the file to be left alone.
         """
         days = os.environ.get("RENEW_DAYS_BEFORE", "").strip()
-        if not days:
-            return
-        if not days.isdigit() or int(days) < 1:
+        if days and (not days.isdigit() or int(days) < 1):
             print(
                 f"Warning: ignoring invalid RENEW_DAYS_BEFORE={days!r} "
                 f"(expected a positive number of days)",
                 file=sys.stderr,
             )
             return
 
-        path = f"/etc/letsencrypt/renewal/{self._lineage_name(domain)}.conf"
+        path = self._renewal_conf_path(domain)
         if not os.path.isfile(path):
             # No lineage yet: the first issuance has not happened, and certbot
             # writes this file itself. Nothing to do.
             return
 
-        setting = f"renew_before_expiry = {days} days"
+        setting = f"renew_before_expiry = {days} days" if days else None
         try:
             with open(path, encoding="utf-8") as fh:
                 lines = fh.read().splitlines()
         except OSError as exc:
             print(f"Warning: cannot read {path}: {exc}", file=sys.stderr)
             return
 
-        out, replaced = [], False
+        out, replaced, removed = [], False, False
         for line in lines:
-            # The key ships commented out, so match both forms.
-            if re.match(r"\s*#?\s*renew_before_expiry\s*=", line):
+            # The key ships commented out, so match both forms when writing.
+            # When clearing, drop only the active setting: the commented
+            # template is certbot's own and carries no value.
+            active = re.match(r"\s*renew_before_expiry\s*=", line)
+            if setting is not None and re.match(
+                r"\s*#?\s*renew_before_expiry\s*=", line
+            ):
                 if not replaced:
                     out.append(setting)
                     replaced = True
                 continue
+            if setting is None and active:
+                removed = True
+                continue
             out.append(line)
 
-        if not replaced:
+        if setting is not None and not replaced:
             # Must land before the first section header; the key is top-level.
             insert_at = next(
                 (i for i, line in enumerate(out) if line.strip().startswith("[")),
                 len(out),
             )
             out.insert(insert_at, setting)
+            replaced = True
 
+        if out == lines:
+            # Nothing to do: already correct, or already absent. Rewriting the
+            # file every pass would only add noise and a chance to corrupt it.
+            return
+
+        # Write through a temporary file and rename over the original. Opening
+        # the live config with "w" truncates it before anything is written, so
+        # a crash or a full disk mid-write would leave certbot with a truncated
+        # lineage config -- and a lineage certbot cannot parse is one it cannot
+        # renew. os.replace is atomic, so a reader sees the old file or the new
+        # one, never a half-written one.
+        tmp = f"{path}.dstack-tmp"
         try:
-            with open(path, "w", encoding="utf-8") as fh:
+            with open(tmp, "w", encoding="utf-8") as fh:
                 fh.write("\n".join(out) + "\n")
+                fh.flush()
+                os.fsync(fh.fileno())
+            os.replace(tmp, path)
         except OSError as exc:
             print(f"Warning: cannot write {path}: {exc}", file=sys.stderr)
+            try:
+                os.unlink(tmp)
+            except OSError as cleanup_exc:
+                print(
+                    f"Warning: cleanup failed for temporary file {tmp}: {cleanup_exc}",
+                    file=sys.stderr,
+                )
+            return
+
+        if removed:
+            print(
+                f"Renewal window for {domain} cleared; "
+                f"certbot's default applies again"
+            )
             return
         print(f"Renewal window for {domain} set to {days} days before expiry")
 

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

@@ -0,0 +1,170 @@
+#!/usr/bin/env python3
+"""Unit tests for certman's renewal-window handling.
+
+Run: python3 scripts/tests/test_certman.py
+"""
+
+import os
+import shutil
+import sys
+import tempfile
+import unittest
+
+sys.path.insert(0, os.path.join(os.path.dirname(os.path.abspath(__file__)), ".."))
+
+import certman  # noqa: E402
+
+# A lineage config as certbot writes it: the key ships commented out, and
+# everything after the first section header is out of bounds for a top-level key.
+CERTBOT_CONF = """\
+version = 5.7.0
+archive_dir = /etc/letsencrypt/archive/example.com
+cert = /etc/letsencrypt/live/example.com/cert.pem
+# renew_before_expiry = 30 days
+
+[renewalparams]
+authenticator = dns-cloudflare
+"""
+
+
+def _manager(path: str) -> certman.CertManager:
+    """A CertManager that reads and writes one temp file.
+
+    __init__ builds a provider from the environment; none of that is involved
+    in rewriting a config file, so bypass it.
+    """
+    mgr = object.__new__(certman.CertManager)
+    mgr._renewal_conf_path = lambda domain: path  # noqa: SLF001
+    return mgr
+
+
+class RenewalWindowTest(unittest.TestCase):
+    def setUp(self):
+        self._saved = os.environ.get("RENEW_DAYS_BEFORE")
+        # Its own directory, so a test may make the directory unwritable
+        # without touching anything it does not own.
+        self.dir = tempfile.mkdtemp()
+        self.path = os.path.join(self.dir, "example.com.conf")
+        self.write(CERTBOT_CONF)
+
+    def tearDown(self):
+        os.chmod(self.dir, 0o700)
+        shutil.rmtree(self.dir, ignore_errors=True)
+        if self._saved is None:
+            os.environ.pop("RENEW_DAYS_BEFORE", None)
+        else:
+            os.environ["RENEW_DAYS_BEFORE"] = self._saved
+
+    def write(self, text):
+        with open(self.path, "w", encoding="utf-8") as fh:
+            fh.write(text)
+
+    def read(self):
+        with open(self.path, encoding="utf-8") as fh:
+            return fh.read()
+
+    def apply(self, value):
+        if value is None:
+            os.environ.pop("RENEW_DAYS_BEFORE", None)
+        else:
+            os.environ["RENEW_DAYS_BEFORE"] = value
+        _manager(self.path).apply_renewal_window("example.com")
+
+    def test_setting_is_written_above_the_first_section(self):
+        self.apply("365")
+        body = self.read()
+        self.assertIn("renew_before_expiry = 365 days", body)
+        self.assertLess(
+            body.index("renew_before_expiry"), body.index("[renewalparams]")
+        )
+
+    def test_setting_replaces_a_previous_value_without_duplicating(self):
+        self.apply("365")
+        self.apply("45")
+        body = self.read()
+        self.assertIn("renew_before_expiry = 45 days", body)
+        self.assertNotIn("365", body)
+        self.assertEqual(body.count("renew_before_expiry"), 1)
+
+    def test_unsetting_removes_a_previously_written_value(self):
+        """The regression: the lineage config outlives the container.
+
+        Leaving the setting behind kept the certificate permanently due, and
+        the documented way out -- unset the variable -- did nothing.
+        """
+        self.apply("365")
+        self.assertIn("renew_before_expiry = 365 days", self.read())
+        self.apply(None)
+        self.assertNotIn("renew_before_expiry = 365 days", self.read())
+
+    def test_unsetting_leaves_certbots_commented_template_alone(self):
+        self.apply(None)
+        self.assertIn("# renew_before_expiry = 30 days", self.read())
+
+    def test_unsetting_on_an_untouched_config_changes_nothing(self):
+        self.apply(None)
+        self.assertEqual(self.read(), CERTBOT_CONF)
+
+    def test_invalid_value_leaves_the_config_alone(self):
+        self.apply("365")
+        before = self.read()
+        for bad in ("0", "-1", "later", "30 days"):
+            self.apply(bad)
+            self.assertEqual(self.read(), before, f"{bad!r} should be ignored")
+
+    def test_missing_lineage_is_not_an_error(self):
+        os.unlink(self.path)
+        self.apply("365")
+        self.assertFalse(os.path.exists(self.path))
+
+    def test_write_is_atomic_and_leaves_no_scratch_file(self):
+        """The live config is renamed into place, never truncated in place.
+
+        Opening it with "w" would empty it before the replacement was written,
+        and a lineage config certbot cannot parse is one it cannot renew.
+        """
+        self.apply("365")
+        self.assertFalse(
+            os.path.exists(self.path + ".dstack-tmp"),
+            "temporary file left behind",
+        )
+        self.assertTrue(self.read().endswith("\n"))
+        self.assertIn("[renewalparams]", self.read())
+
+    def test_a_failed_write_leaves_the_original_intact(self):
+        original = self.read()
+        os.chmod(self.dir, 0o500)  # cannot create the temp file
+        try:
+            self.apply("365")
+        finally:
+            os.chmod(self.dir, 0o700)
+        self.assertEqual(self.read(), original)
+
+    def test_wildcard_uses_the_bare_lineage_name(self):
+        self.assertEqual(
+            certman.CertManager._lineage_name("*.example.com"), "example.com"
+        )
+        self.assertEqual(
+            certman.CertManager._lineage_name("example.com"), "example.com"
+        )
+
+
+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
+    passing tests. Same guard as test_dnsguide.py."""
+
+    def test_no_duplicate_definitions(self):
+        import ast, collections, pathlib
+
+        tree = ast.parse(pathlib.Path(__file__).read_text())
+        names = [n.name for n in tree.body if isinstance(n, ast.ClassDef)]
+        for cls in (n for n in tree.body if isinstance(n, ast.ClassDef)):
+            names += [f"{cls.name}.{m.name}" for m in cls.body
+                      if isinstance(m, (ast.FunctionDef, ast.AsyncFunctionDef))]
+        dupes = [n for n, c in collections.Counter(names).items() if c > 1]
+        self.assertEqual(dupes, [], f"defined more than once: {dupes}")
+
+
+if __name__ == "__main__":
+    unittest.main(verbosity=2)