Skip to content

Fix and workaround DNS issues - #998

Open
jonathan-conder wants to merge 3 commits into
mainfrom
fix/configure-dns
Open

Fix and workaround DNS issues#998
jonathan-conder wants to merge 3 commits into
mainfrom
fix/configure-dns

Conversation

@jonathan-conder

Copy link
Copy Markdown
Contributor

Description

Adds a (possibly temporary) workaround for canonical/lxd#18871. This should make the tests less flaky but the issue could still cause any number of strange problems.

Fixes *.wp name resolution for users who require DNSSEC or DNS-over-TLS for general name resolution.

Self-review quick check

  • Make decisions that cost a lot to reverse explicit in the PR description.
  • Avoid nested conditions.
  • Delete dead code and redundant comments.
  • Normalise symmetries by sticking to doing identical things identically.
// one way to handle errors
if err := f(); err != nil {
   ...
}

// one way to handle multiple returns
val, err := f()
if err != nil {
   ...
}
...
  • Check that coupled code elements, files, and directories are adjacent. For example, test data is stored as close as possible to a test.
  • Put variable declaration and initialisation together.
  • Divide large expressions into digestable and self-explanatory ones. Use multiple variables if required.
  • Put a blank line between two logically different chunks of code.
  • Follow the style guide for new error messages.

Docs

Procedure:

  • I have checked and added or updated relevant documentation.
  • I have checked and added or updated relevant release notes.
  • I have included the technical author in the review.

Content:

  • Headings and titles accurately describe the content.
  • New and updated pages include correct metadata.
  • Documentation tests are added or updated where applicable (for tutorial/ and how-to/ sections).
  • Documentation follows the style guide.
  • If needed, docs/.coverage.yaml updated, coverage tags added (.. artefact).

Or:

  • I confirm the PR has no implications for documentation.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds DNS reliability fixes for Workshop’s bridge network and mitigates LXD PID-reuse failures.

Changes:

  • Restarts aborted DNS configuration.
  • Disables DNSSEC and DNS-over-TLS for .wp routing.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
snap/local/lib/snap.workshop.configure-dns.service Adds restart behavior for aborted configuration.
internal/dns/resolved.go Configures per-link DNS security policies.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread snap/local/lib/snap.workshop.configure-dns.service
Comment thread internal/dns/resolved.go
Comment thread internal/dns/resolved.go
My original rationale for using the configure hook was that post-refresh
doesn't support revert. Now Workshop doesn't support revert anyway, so
we might as well use the install and post-refresh hooks.

This means we can more easily distinguish whether files need to be
installed or modified. I've taken a simple approach of always
reinstalling things, with the exception of configure-dns which users can
choose to disable if they want. This solves the problem of being able to
update these files and propagating the change to existing users.
See canonical/lxd#18871. LXD kills old dnsmasq
PIDs at around the same time as:
1. starting a new dnsmasq process, and
2. running workshopd configure-dns

This makes it quite likely that the workshopd PID is the same as the old
dnsmasq PID. LXD kills it regardless of what process it actually is.
This caused some flakiness in tests/main/autostart, which restarts the
spread VM several times.

The LXD team already found and fixed the bug before I ran into it, but
the fix hasn't been released yet; retrying configure-dns should fix the
flakiness in the meantime (although all bets are off when any process
can be killed at boot time).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (1)

internal/dns/resolved_test.go:59

  • This only forces DNSSEC away from the expected final state. DNS-over-TLS normally already defaults to no, so the assertion below would still pass if the new SetLinkDNSOverTLS call were accidentally removed. Set dnsovertls to yes here as well before calling ConfigureDNS so both overrides are actually exercised.
	out, err := exec.Command("resolvectl", "dnssec", s.iface, "yes").Output()
	c.Assert(err, check.IsNil, check.Commentf("%s", out))

Came across this when playing with Workshop on my personal machine. Both
options need to be disabled for name resolution to work. The setting is
scoped to workshopbr0 so won't affect standard name resolution.
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