Skip to content

fix(config): treat allow_unverified_revocation_url as live in requires_reboot - #307

Merged
Bugs5382 merged 1 commit into
mainfrom
fix/301-requires-reboot-nil-empty
Sep 30, 2026
Merged

Bugs5382 merged 1 commit into
mainfrom
fix/301-requires-reboot-nil-empty

Conversation

@Bugs5382

@Bugs5382 Bugs5382 commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

What and why

config apply reported requires_reboot=true for changes that are actually live. The nil-versus-empty part of #301 (identical re-apply and profiles-only changes reporting a reboot) was fixed on main by #286, which compares configs by their stored YAML. This PR is rebased on that and covers what's left:

  • pki.allow_unverified_revocation_url is now classed as live. The signer reads it from the stored config on every issuance (applyRevocation in internal/node/signer.go, through the live configFunc in internal/init/run.go). Nothing reads it at boot: run.go only mentions it in a log line, and the revocation preflight and listener are built from revocation_base_url, which stays reboot-relevant. A change to the override alone now reports requires_reboot=false, next to pki.profiles and pki.root_leaf_issuance.
  • Tests drive NeedsReboot through the real apply path. The running config goes through Marshal/Parse like the store. The incoming config goes through ToProto, a protobuf wire round trip and FromProto like the API. They cover an identical re-apply (no reboot), a profiles-only change (no reboot) and a network change (reboot). On the pre-feat(config): switch ACME and EST on and off through machine config #286 main, the wire round trip reproduced the bug; an in-process FromProto(ToProto()) did not, because it keeps the empty slice.

Follow-up, not in this PR: pki.allow_unsynced_clock isn't on main yet (it arrives with the SNTP client work, #271). The docs already say the signer reads it live but the apply still reports a reboot for it. Once #271 merges, it should join the live list the same way, with a test.

The Reboot pending line in cryptosctl image status comes from image upgrades, not from ApplyConfig, so it isn't affected.

Docs: CryptOS-PKI/docs#59 lists allow_unverified_revocation_url among the live fields in the machine config reference, "Apply config" and "Declarative config". Merge the two together.

Closes #301

Verification

  • Lint clean
  • Tests pass
  • Build succeeds
  • Documentation updated (if behavior or API changed)

How this was verified

  • The new allow_unverified_revocation_url test failed on main (requires_reboot=true) and passes with the change.
  • The round-trip tests failed on the pre-feat(config): switch ACME and EST on and off through machine config #286 main and pass on current main and on this branch. The existing TestNeedsReboot* cases are unchanged and pass.
  • task ci (fmt, lint, vet, test, build) passes locally on the rebased branch.

Before merging: add a closing comment summarizing what was actually done in this PR
(not just the checked boxes).

@Bugs5382 Bugs5382 self-assigned this Sep 30, 2026
@Bugs5382
Bugs5382 marked this pull request as ready for review September 30, 2026 16:28
…s_reboot

The signer reads pki.allow_unverified_revocation_url from the stored
config on every issuance, and nothing reads it at boot, so a change to
it alone takes effect immediately. NeedsReboot still reported
requires_reboot=true for it. Class it as hot next to pki.profiles and
pki.root_leaf_issuance.

Add tests that drive NeedsReboot through the real apply path: the
running config read back from the store, and the incoming config after
a protobuf wire round trip. They cover an identical re-apply and a
profiles-only change (no reboot) and a network change (reboot).

Signed-off-by: Bugs5382 <12115015+Bugs5382@users.noreply.github.com>
@Bugs5382
Bugs5382 force-pushed the fix/301-requires-reboot-nil-empty branch from e2757a0 to 547fb28 Compare September 30, 2026 16:41
@github-actions github-actions Bot added the fix Bug fix (fix). Patch. label Sep 30, 2026
@Bugs5382 Bugs5382 changed the title fix(config): report requires_reboot only when a reboot-relevant field changed fix(config): treat allow_unverified_revocation_url as live in requires_reboot Sep 30, 2026
@Bugs5382

Copy link
Copy Markdown
Contributor Author

Merging for the v0.1.0 test run: CI green and mergeable at merge time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Bug fix (fix). Patch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(config): report requires_reboot only when a reboot-relevant field changed

1 participant