Add instructions for creating systemd units - #5
Open
jmeekhof wants to merge 23 commits into
Open
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 7efa8ee in 2 minutes and 28 seconds. Click for details.
- Reviewed
288lines of code in6files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. systemd/README.md:31
- Draft comment:
The README example passes '--config=%h/dnsscale.yaml' on ExecStart but the provided unit file (systemd/dnsscale.service) omits it. Please align the examples to avoid confusion. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. systemd/podman/Containerfile:9
- Draft comment:
Similar to the main Containerfile, consider explicitly providing an output flag (e.g. -o dnsscale) in the 'go build' command and ensure the ENTRYPOINT format is consistent. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The current code works fine without the explicit -o flag since Go will use the package name by default. The COPY command on line 12 shows it's already finding the binary correctly. The ENTRYPOINT format looks standard and correct. This seems like a style preference rather than a necessary change. Maybe there's a good reason to be explicit about the output location that I'm missing? The reference to "main Containerfile" suggests there might be consistency concerns across files. Even if there's another Containerfile with different conventions, this file works correctly as is. Being explicit about the output location doesn't add meaningful value here. The comment suggests a style change that doesn't improve functionality or clarity, and makes an unclear reference to ENTRYPOINT format issues that don't appear to exist.
3. systemd/podman/README.md:142
- Draft comment:
Minor grammatical correction: change 'allows' to 'allow' in the instructions regarding .bashrc. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and suggests a minor grammatical correction. It doesn't provide a code suggestion or address a potential issue in the code. According to the rules, purely informative comments should be removed.
4. systemd/podman/README.md:29
- Draft comment:
Consider standardizing configuration file paths between the systemd and Podman examples. The README for systemd uses '%h/dnsscale.yaml' while the Podman example mounts '%h/dnsscale/conf/dnsscale.yaml' to '/root/.dnsscale.yaml'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The inconsistency is real and could be confusing to users. However, there may be valid reasons for the different paths - Podman containers often need different file organization than native systemd services. The comment doesn't indicate a clear problem that needs fixing, just points out a difference. Per the rules, we should not make purely informative comments without a clear action needed. The paths being different might actually be intentional due to the different needs of containerized vs native services. Am I being too quick to assume this is a real issue? Good point - without understanding the architectural reasons for the different paths, this comment may be premature and potentially misleading. The comment should be deleted as it only points out a difference without clear evidence that standardization is needed or beneficial.
5. systemd/podman/README.md:3
- Draft comment:
Typo: In "using Quadlet's to manage services.", the apostrophe is likely unintentional. Suggest changing "Quadlet's" to "Quadlets". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is technically correct about the grammar, it's a very minor issue in documentation. The meaning is still clear despite the typo. The rules state we should not make purely informative comments or unimportant ones. This seems to fall into that category. The incorrect apostrophe could be seen as affecting readability and professional appearance of the documentation. Documentation quality is important. While documentation quality matters, this is an extremely minor issue that doesn't impact understanding. The rules specifically say not to make comments about obvious or unimportant issues. This comment should be deleted as it addresses a trivial grammatical issue that doesn't materially impact the documentation's usefulness.
6. systemd/podman/README.md:143
- Draft comment:
Typo: The phrase "This will allows the user" contains an unnecessary 's'. Consider changing it to "This will allow the user". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the grammar error is real, our rules emphasize focusing on code logic and important issues. Grammar fixes in documentation, while correct, are very minor and could be seen as noise in the review process. This is especially true in a README file where the meaning is still perfectly clear despite the minor grammatical error. The comment is technically correct, but am I being too strict by suggesting we remove a valid correction just because it's minor? The rules explicitly state to avoid obvious or unimportant comments. This grammatical fix, while correct, doesn't impact understanding or functionality. We should remove this comment as it's too minor and doesn't affect code functionality or documentation clarity.
Workflow ID: wflow_CBkUApvA00uK4XfW
You can customize 
| COPY go.mod go.sum ./ | ||
| RUN go mod download | ||
| COPY . . | ||
| RUN GOARCH=$TARGETARCH go build |
There was a problem hiding this comment.
Consider specifying the output binary name (using -o) and using the exec array form for ENTRYPOINT (as in the Podman variant) for consistency and to avoid shell issues.
Suggested change
| RUN GOARCH=$TARGETARCH go build | |
| RUN GOARCH=$TARGETARCH go build -o dnsscale |
| [Service] | ||
| Environment="PATH=/bin:/usr/bin:/usr/local/bin:%h/.local/bin:%h/go/bin" | ||
| EnvironmentFile=%h/dnsscale.env | ||
| ExecStart=%h/go/bin/dnsscale |
There was a problem hiding this comment.
Consider including a restart policy (e.g. Restart=on-failure) in the service file to improve resilience.
| @@ -0,0 +1,162 @@ | |||
| ## Podman Quadlet Instructions | |||
|
|
|||
| If you're running Podman, you also have the option of using Quadlet's to manage services. | |||
There was a problem hiding this comment.
Typo: change "Quadlet's" to "Quadlets" to use the correct plural form.
Suggested change
| If you're running Podman, you also have the option of using Quadlet's to manage services. | |
| If you're running Podman, you also have the option of using Quadlets to manage services. |
deleteNodeDNS identifies the records a node owns by looking for a TXT record carrying `node_id=<id>`, but the Route53 provider's ListRecords filtered the zone down to A and AAAA records only. The ownership TXT could therefore never appear in the list, so the loop never matched and every deleted node left its A, AAAA and TXT records behind forever. This is most visible with Tailscale containers: when a container loses its state volume it re-registers as `<name>-1`, and the records for the previous registration are never reclaimed. - Route53 ListRecords now returns TXT records alongside A/AAAA. - TXT quoting moves into the Route53 provider. The value was previously built with embedded quotes in main.go, which is the correct wire format for Route53 but is stored literally by Cloudflare, so the quotes ended up inside the Cloudflare TXT value. Providers now encode on write and decode on read, and the reconciler deals in plain values. Route53 deletes require an exact value match, so all writes share one code path to keep encoding from drifting between create and delete. - Route53 ListRecords skips alias record sets, which carry a nil TTL and no resource records; the old code would panic dereferencing TTL on a zone containing one. - deleteNodeDNS no longer stops at the first ownership record it finds. A node can own more than one name, and the early break meant only the first was reclaimed. - Delete failures are returned rather than swallowed, so the work item is retried instead of being marked done after a failed cleanup. Note that the Pi-hole provider cannot store TXT records at all (its custom DNS API is A/AAAA only), so node deletion remains a no-op there. Fixing that needs an ownership scheme that does not rely on TXT. Adds tests covering the delete path and the TXT encoding round trip.
dns.provider had no environment binding. viper.AutomaticEnv() was in
play but no key replacer was configured, so it looked for a variable
literally named "DNS.PROVIDER" and DNS_PROVIDER never mapped to the
nested key. Because Config.Validate() hard-requires dns.provider, a
configuration file was mandatory even when every other setting was
supplied through the environment - which is awkward for container
deployments that otherwise need no file at all.
The README's own "Environment Variables" example showed this: it had to
pass --dns-provider as a flag alongside the exported variables.
- Register a strings.NewReplacer(".", "_") key replacer so nested keys
are addressable as DNS_PROVIDER, LOGGING_LEVEL and so on.
- Bind every configuration key explicitly, including the app.* and
logging.* settings which were also unreachable. AutomaticEnv only
resolves keys viper already knows about, so relying on it alone makes
the set of settings that work from the environment hard to predict.
- The AWS settings keep AWS_PROFILE and AWS_REGION rather than moving to
the DNS_ROUTE53_* names the replacer would otherwise imply.
- Document the full key-to-variable mapping in the README.
Adds a test that unmarshals and validates a configuration built purely
from environment variables, with no file present.
nodesEqual compared name, online state and addresses, but not tags. Tags decide whether a node is managed at all via app.required_tags, so a node that gained or lost a relevant tag was never requeued unless one of its addresses happened to change at the same time. In practice that means tagging an existing device to bring it under dnsscale's management has no effect until the device's Tailscale address changes or the process restarts. Also switches the address comparison to slices.Equal, which is what the hand-rolled length check and loop were doing.
Aliases let one Tailscale node own additional names:
dns:
aliases:
fresno: [music, mealie, memory]
Each alias gets its own ownership TXT carrying the node's ID, so the
delete path reclaims aliases along with the node's own record rather
than orphaning them.
Tag-derived names do the same thing without an alias table, which suits
a layout where each service runs its own Tailscale sidecar carrying
--advertise-tags:
dns:
tag_aliases:
"tag:music": music
Static records cover names not derived from any node, most usefully a
wildcard:
dns:
static_records:
- name: "*"
value: "100.64.0.1"
Static records carry an ownership marker too, so dropping one from the
configuration reclaims it rather than leaving it behind.
Other changes:
- A zone sweep runs at startup and on --once. The per-node path cannot
see records left by a previous run - from a crash, or from the period
when the delete path was broken - or static records dropped from the
configuration. The sweep only ever touches names carrying a dnsscale
ownership marker, so unmanaged records in the zone are left alone. It
deliberately does not run on every poll, which would race with the
per-node delete path over the same records.
- --once runs a single pass and exits, for a timer or cron job.
- --dry-run wraps the provider and logs the writes it would make. Reads
pass through to the real provider, so the output reflects the actual
state of the zone.
- An empty app.required_tags now fails validation unless
app.manage_all_nodes is set. Empty meant "publish every authorized
device in the tailnet", which for a public zone publishes the name and
address of every personal laptop and phone that has ever joined, and
lets anyone enumerate the tailnet over DNS. It stays available, but
has to be asked for rather than arrived at by omission.
- Ownership matching compares the owner exactly instead of by substring.
Node IDs are not self-delimiting, so "abc" could previously claim the
records of "abcdef".
- Record names are normalized before comparison. Route53 returns names
with a trailing dot and octal escapes, so a wildcard comes back as
"\052.example.com." and would never match a generated name.
- Adds a .gitignore. The repository had none, so the built binary and
mise.local.toml - which holds real API credentials - were one
"git add -A" away from being committed.
A failed API call is not a usage error, but cobra printed the full help text for every one. Under Restart=on-failure that fills the container log with usage blocks instead of the error that caused the restart.
Tailscale API keys expire 90 days after creation. When one expires,
dnsscale does not fail in any visible way: it keeps running, keeps
polling on its interval, and every request returns 401. Reconciliation
stops, the process stays healthy, and the zone quietly goes stale until
someone happens to read the logs.
I hit this on a deployment where dnsscale had been running for over
seven months since its last successful DNS write - the key expired 90
days in, and the only symptom was a 401 every 30 seconds.
OAuth client credentials do not expire, and the token is refreshed
automatically, so this failure mode goes away.
- tailscale.oauth_client_id / tailscale.oauth_client_secret, also
settable as TAILSCALE_OAUTH_CLIENT_ID / TAILSCALE_OAUTH_CLIENT_SECRET
and via flags.
- The default scope is devices:core:read and nothing else. dnsscale
issues exactly one Tailscale call, GET /api/v2/tailnet/{tailnet}
/devices, and never writes, so there is no reason for a wider grant.
tailscale.oauth_scopes can override it.
- Validate() requires exactly one of the two auth methods. Accepting
both would leave which one actually authenticates up to the reader.
- The API-key path now warns at startup that the key will expire, and a
401 on that path says so explicitly rather than reporting the bare
status text. That is what made this failure mode so quiet.
golang.org/x/oauth2 was already in the module graph as an indirect
dependency; it is pinned at the version already present (v0.27.0) so
this does not move the go directive or the toolchain.
Tests cover the auth-selection rules and run the client-credentials
exchange against a stub server, asserting the grant type, the requested
scope, and that the access token reaches the devices call.
# Conflicts: # cmd.go
Reclaiming records whose owner no longer exists was on by default. That is the wrong default for a reconciler pointed at a zone that already has records in it: the first run can delete names nobody asked it to touch. Concretely, on the deployment this came from, a first run would have deleted seven live names - including an active service - because their owning nodes did not carry the configured tag. The records were all legitimately dnsscale-owned; they were simply created under an earlier, looser configuration. Reclamation now requires --prune (or app.prune: true). With it off, orphans are still detected and logged as a warning listing every name, so the drift is visible on every run and an operator can decide whether enabling it is safe. Static records are unaffected - only the destructive half is gated. Separately, the configuration tests were not hermetic: they inherited whatever the surrounding shell exported, so a developer with real credentials in their environment - via mise.local.toml, direnv or similar - got failures that had nothing to do with their changes, and worse, passes that did not mean what they appeared to. They now clear every bound variable before asserting, and pass identically with and without credentials present.
Every change under --dry-run was logged twice: once by the provider as "[dry-run] would update DNS record", and again by the reconciler as "Updated DNS record" - because the reconciler logs success as soon as the provider returns, and the dry-run provider always returns nil. The second line is false. Nothing was updated. A log asserting that a change happened when it did not is the same failure class as the silent 401 loop that made this tool look healthy while it reconciled nothing, and it is worse than saying nothing at all. Applied-change logging now goes through a helper that stays quiet in dry runs, covering record updates, static record writes, deletions and orphan reclamation. The provider's "would ..." lines are unaffected, so a dry run still reports exactly what it intends to do - once. Adds a test asserting no applied-change message is emitted during a dry run, and that the intent is still reported.
Two changes that make pruning safe enough to actually turn on. dns.protected_names lists names that are never deleted, whatever the ownership markers say. Ownership is inferred from a TXT record, and "owned" is not the same as "safe to delete": a record can carry the marker and still be one nobody wants reclaimed - written under an earlier configuration, belonging to a node that has stopped matching required_tags, or naming infrastructure whose whole purpose is to stay reachable when the rest of the estate is not. That last case is not hypothetical. On the deployment this came from, enabling --prune would have deleted the DNS name of an out-of-band recovery path: a router reachable over a VPN independent of Tailscale, which is precisely what you want working when everything else is not. It carried a dnsscale marker and its node no longer matched the tag filter, so by every rule the sweep applies it was an orphan. Without an exemption list, --prune is only safe in a zone dnsscale exclusively owns. Mixed zones are the common case, not an edge case. Entries match the fully qualified name and accept globs; skips are logged so protection is visible rather than a silent absence from the orphan list. Second, a tag filter that matches no nodes is now an error rather than a quiet success. It is the least visible way this tool can fail: it keeps polling, writes nothing, and reports that everything is fine. A host losing its tag during a rebuild looks exactly like this, and so does a typo in required_tags. RunOnce now logs at ERROR and returns a failure, so a timer or CI check notices. An empty tailnet is excluded, since that is a real state rather than a misconfiguration.
Upstream has had no commits since 2026-01-28 and carries pull requests open for over ten months, including a functional contribution from someone other than us. We run this in production, so the fork is where the work happens now, and it should be set up like something maintained rather than a temporary divergence. Adds a GitHub Actions workflow running gofmt, vet, build, tests and a container build on push and pull request. There was no CI anywhere, upstream or here: the only gate was vet and test running inside the deployment's image build, which means a broken commit was not caught until something tried to deploy it. The README now says what this is, what it fixes, and what it adds beyond upstream, with links to the five open PRs. Everything that generalises has been offered upstream and those PRs stay open - if the project wakes up, this fork should become unnecessary.
Ownership is tracked per name but deletion was performed per name too, so reclaiming an orphaned name removed every record under it regardless of type. A name dnsscale manages can legitimately carry records it never wrote, and mail records are exactly the kind that nobody notices are gone until delivery fails. Verified against an in-memory zone: pruning a name carrying an ownership marker alongside an MX, an SPF TXT and a CAA record removed all four. Reclamation is now per record. Only address records and TXT records carrying dnsscale's own marker are removed; MX, CNAME, CAA, SRV, NS and foreign TXT records survive, and the skip is logged. Both the sweep and the per-node delete path had the same defect and both are fixed. Also warns at startup when the configuration manages the zone apex. That is a different hazard the above does not cover: writes are upserts, and an upsert replaces the whole record set for a name and type, so managing the apex means the ownership TXT replaces whatever TXT is already there - an SPF record on most domains. It happens on the next poll, deletes nothing, and needs no --prune, so none of the deletion safeguards apply.
A report against fresno said dnsscale logged successful upserts for five game-server names that never appeared in Route53, with no error. The write path was not at fault, and there was nothing to fix there: ChangeResourceRecordSets is awaited, its error is checked, and "Updated DNS record" is only reached on a nil error. Every change is submitted as a single-change batch, so there is no batch for one bad change to poison. The five records exist in the zone today with byte-identical values and the correct ownership node_ids. What produced the report was the log. Two things combined: nodesEqual compared Online, which is derived from LastSeen rather than reported by the API, so it flips every time a device idles past five minutes and flips back when it checks in. Nothing dnsscale writes depends on it. On the reporting tailnet one laptop was requeued 241 times in a day, and a game-server node that flapped offline rewrote its three unchanged records four times in half an hour. syncNodes then queued and logged every one of those requeues at info, including nodes the tag filter would drop a moment later inside reconcile - silently, at debug. "Queuing node for reconciliation" was the only info line emitted per node, so five stale untagged registrations appeared on every poll under exactly the names that were missing from the zone. Read from outside, dnsscale looked busy publishing terraria while never intending to write it. An untagged node and a node whose write vanished are indistinguishable when the log shows the same thing for both. So: stop comparing Online, do not queue a node the filter will discard, say why at debug, and report the admitted set at info whenever it changes. That last line is the one that answers "is dnsscale even looking at this node", which the log previously could not. Filtered nodes are still cached. The cache is what notices a node has disappeared, and a node that was managed before it lost its tag still needs that to keep working.
167 requeues in a day, not 241. The larger number counted a day and a half of log, which is exactly the kind of casually-inflated evidence that makes the rest of a comment less trustworthy. The full-day count is striking enough.
The polling loop reports which nodes the tag filter admits, by name, whenever that set changes. RunOnce does not: it never calls syncNodes, so logManagedSet never runs, and the only trace of the filter in a --once pass was a count on the completion line. That count is not the useful part. "nodes_managed: 8" does not tell you whether terraria is one of the eight, and "is dnsscale looking at this node" is exactly the question a missing record raises — the question the previous commit existed to make answerable. Moving a deployment to --once on a timer silently gave that answer back up. So collect the admitted nodes by name, sorted, and put them on the completion line alongside the count. Same information the daemon prints, same reason.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces new containerization and systemd integration options for the
dnsscaleproject, along with comprehensive documentation for running the service under both standard systemd and Podman Quadlet environments. The changes provide example unit files, environment setup instructions, and scripts to streamline installation and deployment.Containerization and Build Improvements:
Containerfilefor building and packaging thednsscaleGo binary using Alpine Linux, enabling easier container-based deployments.Containerfilethat clones the repository, caches Go modules, and builds the binary for containerized environments.Systemd Integration and Documentation:
dnsscale.serviceunit file for user-level systemd, supporting environment files and custom paths for the executable.install_service.sh) to automate installation of the systemd unit file into the correct user configuration directory.systemd/README.mdfor configuring and runningdnsscaleas a user service, including environment and configuration file management.Podman Quadlet Support and Documentation:
systemd/podman/README.mdwith step-by-step instructions for runningdnsscaleusing Podman Quadlet, including example Quadlet unit files, environment setup, and advanced user management tips.Important
Adds systemd and Podman Quadlet support for
dnsscale, including containerization improvements and detailed documentation.Containerfilefor buildingdnsscaleGo binary using Alpine Linux.Containerfilefor cloning, caching, and building the binary.dnsscale.serviceunit file for user-level systemd with environment file support.install_service.shscript for automating systemd unit file installation.systemd/README.md.systemd/podman/README.mdwith instructions for Podman Quadlet setup.This description was created by
for 7efa8ee. You can customize this summary. It will automatically update as commits are pushed.