Skip to content

fix(dstack-ingress): retry applying a renewed certificate until it sticks - #102

Merged
kvinwang merged 1 commit into
Dstack-TEE:mainfrom
pacoyang:fix/ingress-reload-only-on-renewal
Jul 28, 2026
Merged

fix(dstack-ingress): retry applying a renewed certificate until it sticks#102
kvinwang merged 1 commit into
Dstack-TEE:mainfrom
pacoyang:fix/ingress-reload-only-on-renewal

Conversation

@pacoyang

@pacoyang pacoyang commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Problem

Renewing a certificate and applying it were treated as one step:

if [ "$changed" -eq 1 ]; then
    collect_evidence     || echo "Evidence generation failed" >&2
    build_combined_pems  || echo "Combined PEM build failed" >&2
    haproxy_reload       || true
fi

Every failure there is swallowed, and none of them set failed, so the pass
still reports success. Twelve hours later the ACME client sees a fresh
certificate on disk, reports "nothing to renew", changed is 0, and the apply
is never attempted again.

HAProxy then goes on serving the pre-renewal certificate until it expires —
which is soon, because being close to expiry is why it renewed. The symptom
arrives weeks after the cause, as an expired certificate, with nothing in the
logs at that moment to connect the two.

Fix

Separate renewing from applying. APPLY_PENDING is set when something renewed
and cleared only once evidence, PEMs and the reload have all succeeded, so a
failed apply is retried on the next pass instead of waiting for the next
renewal.

Quiet passes stay quiet: with nothing renewed and nothing pending, the block
does not run at all. This does not reintroduce the reload-on-every-check
behaviour that used to replace HAProxy's worker twice a day.

Kept in both mode scripts rather than lifted into a library — the apply steps
are mode-specific, since certbot and lego lay certificates out differently, so a
shared helper would have to call back into functions its callers define.

Verification

The state machine, driven with stubbed apply steps:

Pass Input Result
1 renewed, reload fails APPLY_PENDING stays true
2 nothing renewed — where the old code gave up for good retries, still fails
3 nothing renewed, reload recovers applied, flag cleared
4 nothing renewed, nothing pending completely silent

Pass 2 is the bug. Pass 4 is the regression guard.

Container check: startup reaches placeholder → HAProxy → DNS wait as before, the
placeholder is served, and the container stays up (the startup pass runs bare
under set -e, so a stray non-zero there would kill it). Unit tests and the
sanitizer suite pass.

Note

Rebased onto the current main, which no longer has renew-certificate.sh or
renewal-daemon.sh#105 collapsed bootstrap and the renewal daemon into a
single owner per challenge type. Three of this PR's four original parts already
have equivalents there: the exit 2 propagation, the set -e handling at the
call site, and reloading only when something actually renewed. What remains is
the part that had no equivalent, and it is simpler here — the original needed an
on-disk stamp to coordinate two processes, whereas one process now owns the
whole lifecycle, so an in-memory flag covers it. A container restart rebuilds
the PEMs from disk anyway.

@pacoyang
pacoyang force-pushed the fix/ingress-reload-only-on-renewal branch 2 times, most recently from ad4ffa9 to 1cd22f3 Compare July 19, 2026 02:16
@kvinwang
kvinwang force-pushed the fix/ingress-reload-only-on-renewal branch from 1cd22f3 to 609a6bc Compare July 28, 2026 08:15
@kvinwang
kvinwang force-pushed the fix/ingress-reload-only-on-renewal branch from 609a6bc to 68382a7 Compare July 28, 2026 08:45
…icks

Renewing and applying were treated as one step:

    if [ "$changed" -eq 1 ]; then
        collect_evidence     || echo "Evidence generation failed" >&2
        build_combined_pems  || echo "Combined PEM build failed" >&2
        haproxy_reload       || true
    fi

Every failure there is swallowed, and none of them set `failed`, so the
pass still reports success. Twelve hours later the ACME client sees a
fresh certificate on disk, reports "nothing to renew", `changed` is 0,
and the apply is never attempted again. HAProxy goes on serving the
pre-renewal certificate until it expires -- which is soon, because being
close to expiry is why it renewed in the first place. The symptom arrives
weeks after the cause, as an expired certificate, with nothing in the
logs at that moment to connect the two.

Separate the two: `APPLY_PENDING` is set when something renewed and only
cleared once evidence, PEMs and the reload have all succeeded, so a
failed apply is retried on the next pass instead of waiting for the next
renewal.

Quiet passes stay quiet. With nothing renewed and nothing pending the
block does not run at all, so this does not bring back the reload-on-
every-check behaviour that made HAProxy replace its worker twice a day.

Kept in both mode scripts rather than lifted into a library: the apply
steps are mode-specific (certbot and lego lay certificates out
differently), so a shared helper would have to call back into functions
its callers define, which is harder to follow than the dozen lines it
would save.
@kvinwang
kvinwang force-pushed the fix/ingress-reload-only-on-renewal branch from 68382a7 to 26f4072 Compare July 28, 2026 08:46
@kvinwang kvinwang changed the title fix(dstack-ingress): reload haproxy only when a certificate was actually renewed fix(dstack-ingress): retry applying a renewed certificate until it sticks Jul 28, 2026
@kvinwang

Copy link
Copy Markdown
Collaborator

Test report

Two layers, because the first one alone would not have caught a typo in the
scripts: the state machine was first checked against a transcription of the
logic, which proves the design and nothing about the code that ships.

1. The real run_pass, driven directly

Extracted from the shipped scripts rather than retyped —
source <(sed -n '/^APPLY_PENDING=false/,/^}$/p' /scripts/dns01.sh) — with only
its dependencies stubbed, so the code under test is the code in the file.

Pass Input APPLY_PENDING after
1 renewed, reload fails true
2 nothing renewed — where the old code gave up permanently true (retried, still failing)
3 nothing renewed, reload recovers false
4 nothing renewed, nothing pending false, block does not run at all

Pass 2 is the bug this PR fixes. Pass 4 is the regression guard: it confirms the
change does not bring back the reload-on-every-check behaviour that used to
replace HAProxy's worker twice a day.

Run against both dns01.sh and tlsalpn.sh, with identical results. The two
copies of the block differ only in their surrounding comments.

2. A real container, with a real reload failure

dns-01 against Let's Encrypt staging with real Cloudflare credentials, real
HAProxy, RENEW_INTERVAL=60 and RENEW_DAYS_BEFORE=365 so every pass renews.
The reload was made to fail for real by pointing the PID file at a process that
does not exist, rather than by stubbing anything.

pass 1 (startup, HAProxy not yet exec'd)
    ✓ Certificate obtained successfully for r2.kvin.wang
    HAProxy is not running yet; certificates will be picked up at start
    [09:03:18] Next certificate check in 60s

    # echo 999999 > /var/run/haproxy/haproxy.pid

pass 2
    ✓ Certificate renewal completed
    HAProxy reload failed: SIGUSR2 to PID 999999 failed
    [09:03:18] Certificate apply incomplete; retrying on the next pass

    # echo 1 > /var/run/haproxy/haproxy.pid

pass 3
    ✓ Certificate renewal completed
    HAProxy reloaded with the new certificates
    [NOTICE]   (1) : Reloading HAProxy

The last line is HAProxy's own, so the reload actually happened rather than the
signal merely being sent successfully.

3. Startup safety

The startup run_pass is called bare under set -e, so any stray non-zero
inside it kills the container before HAProxy ever starts. Checked in
tls-alpn-01 wait mode, where the pass renews nothing and blocks on DNS:
placeholder generated and served (subject=CN = never.kvin.wang), container
running.

Worth noting because the first version of this used
[ "$changed" -eq 1 ] && APPLY_PENDING=true, which I suspected of tripping
set -e when nothing changed. It does not — in A && B a failing A is exempt,
only the final command counts — but the if form is unambiguous and stayed.

4. Suites

test_dnsguide.py (18) and test_sanitizers.sh pass; bash -n and ast.parse
clean across all scripts.

Not covered

  • tls-alpn-01 with a forced reload failure in a real container. Only dns-01
    was driven end to end. The block is identical in both files and the state
    machine was verified against both, but the integration itself was exercised on
    one path.
  • Failure of collect_evidence or build_combined_pems in a real container.
    Only the reload was made to fail; the other two feed the same applied flag
    and were covered in layer 1.

@kvinwang
kvinwang merged commit 5ed94d8 into Dstack-TEE:main Jul 28, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants