Implement ddns feature in dns-policy by weiiwang01 · Pull Request #475 · canonical/dns-operators

weiiwang01 · 2026-08-31T05:49:53Z

What this PR does

Implementing the ddns feature in the dns-policy charm. As agreed in the spec, the ddns feature should be implemented at the dns-policy level first.

A new charm configuration called ddns-domain is added to the dns-policy charm. When set, the ddns feature is enabled in the dns-policy charm. The ddns domain allocation is done in the dns-policy application and stored inside the database to ensure uniqueness across all dns requirers.

Why we need it

Checklist

I followed the contributing guide
I added or updated the documentation (if applicable)
I updated docs/changelog.md with user-relevant changes
I used AI to assist with preparing this PR
I added or updated tests as needed (unit and integration)
If integration test modules are used: I updated the workflow configuration
(e.g., in .github/workflows/integration_tests.yaml, ensure the modules list is correct)
If this PR involves Terraform: terraform fmt passes and tflint reports no errors

yhaliaw

Major changes needed

🤝 Human review with AI assistance.

yhaliaw · 2026-09-01T06:07:15Z

+        if not entries:
+            logger.debug("Reconciliation: no entry to publish upstream")
+            return
+        self._publish_upstream(entries)


The new if not entries: return guard sits on the combined approved-requests + ddns list, whereas the pre-PR code guarded on the requirer-requested entries and then unconditionally published approved_requests — even when empty.

This means a withdrawal is silently dropped: a requirer submits a record request, it's approved and published; the admin later denies/deletes it. On the next reconcile the relation still exists (so the if not relations guard at line 163 passes), get_approved_requests returns [], ddns is disabled, entries == [], and the function returns before calling _publish_upstream. Bind keeps serving the withdrawn record indefinitely. _clear_ddns_domains only clears the ddns-domain field on the provider relation, so it masks this rather than fixing it — the requirer-facing upstream relation is what goes stale.

Drop the early return and always call _publish_upstream(entries) so an empty list withdraws everything, same as a non-empty one publishes it. Worth adding a unit test for the non-empty → empty transition that asserts on the dns-record-requirer local app databag rather than just the provider-side ddns-domain field.

🤖 AI-assisted

Thank you for spotting the issue, fixed, thanks!

gregory-schiano · 2026-09-04T13:44:20Z

+                logger.error("No domain allocated for the relation %s", relation.id)
+                continue
+
+            domain = f"{label}.{ddns_domain}"


The size of ddns_domain is validated ensuring it's not more than 253 chars, but adding the prefix and dot may go over 253.
Shouldn't we validate the full domain ?

Yes, you are right. The current label length is 8 characters. I will limit the ddns-domain configuration to be less than 200 characters for future-proofing.

gregory-schiano · 2026-09-04T13:47:06Z

+        pytest.param("localhost", ["localhost"], id="already-allowed"),
+        pytest.param("", ["localhost"], id="empty"),


I'm curious how is it passing while DNS_POLICY_API_HOST is setting 127.0.0.1 and not localhost ?

Yes, the unit test is failing, I forget to update the unit test after changing the host, updated, thanks!

gregory-schiano · 2026-09-04T13:54:21Z

+                raise ApiError(str(e)) from e
+
+            try:
+                labels[relation_id] = str(req.json()["label"])


I think we should be stricted as null, collections will be accepted as labels. numbers too even if I don't thinks it's a pb

Added a more strict check, thanks!

gregory-schiano · 2026-09-04T14:00:34Z

+@pytest.mark.abort_on_fail
+def test_ddns_domain_is_allocated(
+    juju: jubilant.Juju,
+    dns_policy_name: str,
+    ddns_deployment,  # pylint: disable=unused-argument
+):
+    """
+    arrange: deploy the charms and set the ddns-domain configuration.
+    act: integrate a requirer on the dns-record-provider endpoint.
+    assert: the requirer is handed a domain under the configured suffix.
+    """
+    domain = _wait_for_ddns_domain(juju, f"{dns_policy_name}/0", published=True)
+
+    assert domain is not None
+    _, _, suffix = domain.partition(".")
+    assert suffix == DDNS_DOMAIN
+
+
+@pytest.mark.abort_on_fail
+def test_ddns_domain_is_resolvable(
+    juju: jubilant.Juju,
+    bind_name: str,
+    dns_policy_name: str,
+    dns_integrator_name: str,
+    ddns_deployment,  # pylint: disable=unused-argument
+):
+    """
+    arrange: deploy the charms and let a requirer be allocated a domain.
+    act: resolve that domain, and one of its subdomains, against the DNS provider.
+    assert: both resolve to the address of the requirer.
+    """
+    domain = _published_ddns_domain(juju, f"{dns_policy_name}/0")
+    assert domain is not None
+    address = _requirer_ingress_address(juju, f"{dns_policy_name}/0", f"{dns_integrator_name}/0")
+    bind_address = juju.status().get_units(bind_name)[f"{bind_name}/0"].public_address
+
+    assert _resolve(bind_address, domain) == [address]
+    assert _resolve(bind_address, f"anything.{domain}") == [address]
+
+
+@pytest.mark.abort_on_fail
+def test_ddns_domain_is_stable(
+    juju: jubilant.Juju,
+    dns_policy_name: str,
+    ddns_deployment,  # pylint: disable=unused-argument
+):
+    """
+    arrange: deploy the charms and let a requirer be allocated a domain.
+    act: reconfigure the charm.
+    assert: the requirer keeps the domain it was allocated.
+    """
+    domain = _published_ddns_domain(juju, f"{dns_policy_name}/0")
+
+    juju.config(dns_policy_name, {"debug": True})
+    juju.wait(
+        lambda status: jubilant.all_active(status, dns_policy_name),
+        error=jubilant.any_error,
+    )
+    time.sleep(120)  # let the reconciliation timer tick a couple of times
+
+    assert _published_ddns_domain(juju, f"{dns_policy_name}/0") == domain
+
+
+@pytest.mark.abort_on_fail
+def test_invalid_ddns_domain_blocks_the_charm(
+    juju: jubilant.Juju,
+    dns_policy_name: str,
+    ddns_deployment,  # pylint: disable=unused-argument
+):
+    """
+    arrange: deploy the charms and let a requirer be allocated a domain.
+    act: set an invalid ddns-domain configuration.
+    assert: the charm blocks and keeps the domain it already allocated.
+    """
+    domain = _published_ddns_domain(juju, f"{dns_policy_name}/0")
+
+    juju.config(dns_policy_name, {"ddns-domain": "not a domain"})
+    juju.wait(lambda status: jubilant.all_blocked(status, dns_policy_name))
+    time.sleep(120)  # let the reconciliation timer tick a couple of times
+
+    assert _published_ddns_domain(juju, f"{dns_policy_name}/0") == domain


These tests seems to rely on their order, someone making changes may break them. This should at least be explicit

Yes, that's a good idea, use a parametrized fixture for configuration instead, thanks!

gregory-schiano · 2026-09-04T14:03:27Z

+    """Allocate the label of the automatically allocated domain of a relation."""
+    permission_classes = [permissions.IsAuthenticated]
+
+    def get(self, request, instance, relation_id):


Since it's mutating the data shouldn't it be a PUT ?

This is a get-or-create function that will create the label if the corresponding label doesn't exist. The charm will also use this exact endpoint to get the corresponding labels. I am still undecided on whether or not this should be a GET or POST/PUT; I feel like both are valid.

gregory-schiano · 2026-09-04T14:07:35Z

+):
+    """Integrate a requirer with the dns-policy charm and enable the ddns feature."""
+    if not _workload_supports_ddns(juju, f"{dns_policy_name}/0"):
+        pytest.skip("The charmed-dns-policy snap has no ddns allocation API yet")


Shouldn't it be an xfail in case of 404 ? The skip will lead to succesful test suite despite the tests not been executed.

Also I'd reverse the logic of _workload_supports_ddns and explicitely look for 2XX
Because a 500 will be considered successful

I have updated the test to inject the dns-policy charm into the charm, so we don't need to wait for the next pull request to test it. This is no longer needed and removed, thanks!

gregory-schiano · 2026-09-04T14:09:12Z

+    except subprocess.CalledProcessError as exc:
+        raise OSError(f"Error packing charm: {exc}; Stderr:\n{exc.stderr}") from None
+
+    charms = [p.absolute() for p in directory.glob(f"{app_name}_*.charm")]


nit: If there's a pre-existing .charm (staled by a removed architecture for example) this will select it. Can't we get the artifacts produced ?

Updated the code to remove stale charms before build. Will convert this repository to charm-ci in the future to prevent this kind of issues. Thanks!

gregory-schiano · 2026-09-04T14:10:02Z

      default: "0.0.0.0"
+    ddns-domain:
+      type: string
+      description: >


Should specify that it takes precedence and that ingress-address is used as fallback

I think the field that uses ingress-address as default is the ddns-addresses field in the relation data, not the ddns-domain configuration used for the ddns-domain suffix.

weiiwang01 marked this pull request as ready for review August 31, 2026 06:11

weiiwang01 requested review from a team, DeeKay3, gregory-schiano and srbouffard as code owners August 31, 2026 06:11

weiiwang01 force-pushed the ddns/dns-policy branch 2 times, most recently from cdc67e8 to c0281d1 Compare September 1, 2026 06:04

yhaliaw reviewed Sep 1, 2026

View reviewed changes

weiiwang01 force-pushed the ddns/dns-policy branch from 0229b78 to 567e301 Compare September 4, 2026 06:00

gregory-schiano reviewed Sep 4, 2026

View reviewed changes

Base automatically changed from ddns/dns-record to main September 10, 2026 06:23

weiiwang01 added 3 commits September 10, 2026 14:23

Implement ddns feature in dns-policy

2c01ade

Fix integration test

b35b659

Withdraw upstream when empty

1bb179f

weiiwang01 force-pushed the ddns/dns-policy branch from 567e301 to 1bb179f Compare September 10, 2026 06:23

Apply suggestions from review comments

8cfe552

		pytest.param("localhost", ["localhost"], id="already-allowed"),
		pytest.param("", ["localhost"], id="empty"),

Conversation

weiiwang01 commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

Why we need it

Checklist

Uh oh!

yhaliaw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

weiiwang01 commented Aug 31, 2026 •

edited

Loading