expose DNS feature with nsupdate provider and a dedicated DNS server by evgeni · Pull Request #532 · theforeman/foremanctl

evgeni · 2026-06-01T12:51:29Z

Why are you introducing these changes? (Problem description, related links)

What are the changes introduced in this pull request?

How to test this pull request

Steps to reproduce:

Checklist

Tests added/updated (if applicable)
Documentation updated (if applicable)

ehelms · 2026-06-02T01:35:17Z

Let's see how smart my review tool is getting, I ran it over this draft as it's a good playground for it:

Ansible Role — copy module src path

development/roles/bind/tasks/main.yml:10 — src: files/rndc.key is wrong. Inside a role, copy resolves src relative to the role's files/ directory, so Ansible will look for
/files/files/rndc.key, which doesn't exist. Same bug on line 18 with src: files/example.test.db. Both should drop the files/ prefix: src: rndc.key and src: example.test.db.

Ansible Role — service not enabled

development/roles/bind/tasks/main.yml:48 — The Start BIND task starts the service but doesn't set enabled: true. After a VM reboot the service won't come back up, which will break any
test run that reboots the named VM.

Missing obsah metadata

development/playbooks/remote-named/ — No metadata.obsah.yaml. Without it, forge (via obsah) won't discover the remote-named subcommand, so it can't be invoked.

evgeni · 2026-06-02T05:49:49Z

Let's see how smart my review tool is getting, I ran it over this draft as it's a good playground for it:

Ansible Role — copy module src path

* development/roles/bind/tasks/main.yml:10 — src: files/rndc.key is wrong. Inside a role, copy resolves src relative to the role's files/ directory, so Ansible will look for
  /files/files/rndc.key, which doesn't exist. Same bug on line 18 with src: files/example.test.db. Both should drop the files/ prefix: src: rndc.key and src: example.test.db.

And yet it works… https://docs.ansible.com/projects/ansible/latest/playbook_guide/playbook_pathing.html#id3 is more complicated than "it will look in files/"

Ansible Role — service not enabled

* development/roles/bind/tasks/main.yml:48 — The Start BIND task starts the service but doesn't set enabled: true. After a VM reboot the service won't come back up, which will break any
  test run that reboots the named VM.

Technically correct, but nothing reboots the VM.

Missing obsah metadata

* development/playbooks/remote-named/ — No metadata.obsah.yaml. Without it, forge (via obsah) won't discover the remote-named subcommand, so it can't be invoked.

And yet it works… ;)

(Yes, those are all valid points, but their criticality and impact is way off)

evgeni · 2026-06-02T08:17:30Z

+    parameter: --dns-provider
+    help: DNS provider to use.
+    choices:
+      - nsupdate


The provider names are really dns_<name>, so dns_nsupdate, but it felt rather cumbersome to me to make the user add the (imho redundant) prefix here. Instead whenever foreman_proxy_dns_provider is used, it needs to be prefixed with dns_

evgeni · 2026-06-02T08:17:56Z

    - [certificates_custom_server_certificate, certificates_custom_server_key, certificates_custom_server_ca_certificate]
  forbidden_if:
    - [certificates_source, installer, [certificates_custom_server_certificate, certificates_custom_server_key, certificates_custom_server_ca_certificate]]
+  # FIXME: this only should trigger if the dns feature is on


obsah can't do required_if on a feature, but it should?

theforeman/obsah#119 WDYT

evgeni · 2026-06-02T08:18:56Z

+- name: Include tasks for {{ foreman_proxy_dns_provider }}
+  ansible.builtin.include_tasks: feature.yaml


is this too much of a "clever" hack? a feature (dns) enabling a "sub" feature (dns_nsupdate)?

In https://github.com/theforeman/puppet-foreman_proxy I made a distinction of modules and providers.

But what you're talking about here also has precedent: https://github.com/theforeman/puppet-foreman_proxy/blob/5e4fcc4200ec540cbe033e7258aecc60083554d8/manifests/config.pp#L19-L32. As the person who wrote that and is happy with it: not too clever in my book.

ekohl · 2026-06-02T10:23:13Z

+
+
+@pytest.mark.feature('dns')
+def test_dns_nsupdate(server, certificates, server_fqdn):


This effectively implements a subset of https://github.com/theforeman/forklift/blob/master/bats/fb-proxy-dns.bats. It's a good start but I'd love to see it expanded in the future

You're saying we should test all supported DNS types and also their deletion etc? Can do, sure.

BTW, you don't happen to have an Python implementation of Foreman's ProxyAPI implementation at hand? ;)

You're saying we should test all supported DNS types and also their deletion etc? Can do, sure.

I think that would be great. And for PTR both IPv4 and IPv6.

BTW, you don't happen to have an Python implementation of Foreman's ProxyAPI implementation at hand? ;)

Sorry, I don't.

git init proxyapy

Anyway, added all but PTR (was too lazy to add a .arpa domain to the setup)

ekohl · 2026-06-02T10:29:16Z

+- name: Include tasks for {{ foreman_proxy_dns_provider }}
+  ansible.builtin.include_tasks: feature.yaml


In https://github.com/theforeman/puppet-foreman_proxy I made a distinction of modules and providers.

But what you're talking about here also has precedent: https://github.com/theforeman/puppet-foreman_proxy/blob/5e4fcc4200ec540cbe033e7258aecc60083554d8/manifests/config.pp#L19-L32. As the person who wrote that and is happy with it: not too clever in my book.

evgeni · 2026-06-05T07:42:32Z

    - [certificates_source, installer, [certificates_custom_server_certificate, certificates_custom_server_key, certificates_custom_server_ca_certificate]]
-
+  required_in_list:
+    - [[[features, dns], [foreman_proxy_dns_provider, nsupdate]], [foreman_proxy_dns_keyfile, foreman_proxy_dns_server]]


this uses theforeman/obsah#119, but I am not 100% satisfied with it. partially because this only works when the user passes both --add-feature dns and --dns-provider nsupdate explicitly. Relying on the default dns provider being nsupdate doesn't work, as then the "required" check doesn't trigger.

That means only foremanctl deploy --add-feature dns would fail?

So I tested foremanctl deploy --add-feature foreman-proxy --add-feature dns it works --dns-provider nsupdate is selected by default.

But it fails with

The task includes an option with an undefined variable. The error was: 'foreman_proxy_dns_server' is undefined. 'foreman_proxy_dns_server' is undefined. 'foreman_proxy_dns_server' is undefined. 'foreman_proxy_dns_server' is undefined

Which is expected since I didn't pass --dns-server. But i think this validation should happen earlier not in the later stage. What do you think?

Also, why there are 4 undefined statements in the error?

ekohl · 2026-06-05T16:38:34Z

+    if dns_type == 'PTR':
+        expected = dns_name
+        query = dns_value
+    else:
+        expected = dns_value
+        query = dns_name


This reminds me of https://projects.theforeman.org/issues/18398. Actually the inspiration for me to expose capabilities in the registration protocol: signify the Smart Proxy supports a DNS v2 API. That never happened, but we have used the registration protocol v2.

ekohl

Not a blocker for this, but I wonder about downstream vs upstream supported providers. What if it differs? Perhaps a question for https://community.theforeman.org/t/container-based-downstreams/46492.

ekohl · 2026-06-08T11:56:47Z

+- name: Include tasks for {{ foreman_proxy_dns_provider_name }}
+  ansible.builtin.include_tasks: feature.yaml
+  vars:
+    feature_enabled: "true"


How about making this value dependent on whether the provider is selected? Then you can list every provider and include disable steps. Not a blocker for this PR, but thinking about the pattern here.

At first I was like "wth is he talking about", but then 💡 came.

You mean something like:

- name: Include tasks for {{ item }} ansible.builtin.include_tasks: feature.yaml vars: feature_enabled: "{{ foreman_proxy_dns_provider_name == item }}" loop: - every - possible - dns - provider

I didn't even think about a loop, but that's even nicer.

evgeni · 2026-06-08T12:31:09Z

Not a blocker for this, but I wonder about downstream vs upstream supported providers. What if it differs? Perhaps a question for https://community.theforeman.org/t/container-based-downstreams/46492.

My naïve reply would be:

foreman_proxy role defines a foreman_proxy_supported_dns_providers list in its defaults
any downstream can override this by providing a downstream.yaml vars file that is included in all playbooks

(no, this is not implemented right now, but would be possible without too much of a headache)

Edit: you'd also have to override the params definition to skip the unsupported ones. Which we can't do today. Ugh.

shubhamsg199 · 2026-06-08T14:58:21Z

+| `--foreman-proxy-dns-server` | Configure the target DNS server | `--foreman-proxy-dns-server` |
+| `--foreman-proxy-keyfile` | Confogure the key for the target DNS server | `--foreman-proxy-keyfile` |


shubhamsg199 · 2026-06-08T15:00:01Z

    - [certificates_source, installer, [certificates_custom_server_certificate, certificates_custom_server_key, certificates_custom_server_ca_certificate]]
-
+  required_in_list:
+    - [[[features, dns], [foreman_proxy_dns_provider, nsupdate]], [foreman_proxy_dns_keyfile, foreman_proxy_dns_server]]


That means only foremanctl deploy --add-feature dns would fail?

pr-processor Bot added the Not yet reviewed label Jun 1, 2026

evgeni force-pushed the rndc branch 4 times, most recently from a07bb03 to 809c5ed Compare June 2, 2026 08:15

evgeni changed the title ~~setup dns_nsupdate with external bind~~ expose DNS feature with nsupdate provider and a dedicated DNS server Jun 2, 2026

evgeni commented Jun 2, 2026

View reviewed changes

pr-processor Bot removed the Not yet reviewed label Jun 2, 2026

evgeni commented Jun 2, 2026

View reviewed changes

evgeni force-pushed the rndc branch from 9b466e5 to 314a643 Compare June 2, 2026 10:11

ekohl reviewed Jun 2, 2026

View reviewed changes

evgeni mentioned this pull request Jun 2, 2026

Add KEA DHCP provider feature for foreman-proxy #531

Open

2 tasks

evgeni force-pushed the rndc branch 2 times, most recently from edc8cba to 591b419 Compare June 2, 2026 14:08

evgeni mentioned this pull request Jun 3, 2026

allow specifying multiple triggers for required_if theforeman/obsah#119

Open

evgeni force-pushed the rndc branch 2 times, most recently from dfd0a70 to ed04896 Compare June 5, 2026 07:40

evgeni commented Jun 5, 2026

View reviewed changes

evgeni force-pushed the rndc branch from ed04896 to e98ef71 Compare June 5, 2026 07:49

ekohl reviewed Jun 5, 2026

View reviewed changes

Comment thread docs/user/parameters.md Outdated

evgeni force-pushed the rndc branch 3 times, most recently from 041d5d0 to 6fd746e Compare June 5, 2026 15:43

ekohl reviewed Jun 5, 2026

View reviewed changes

evgeni force-pushed the rndc branch from 6fd746e to 97c3434 Compare June 5, 2026 16:49

shubhamsg199 reviewed Jun 6, 2026

View reviewed changes

Comment thread development/roles/bind/files/rndc.key Outdated

evgeni force-pushed the rndc branch from 97c3434 to 838537d Compare June 6, 2026 09:41

evgeni force-pushed the rndc branch 3 times, most recently from e763aa3 to bfcb8fc Compare June 8, 2026 11:38

ekohl reviewed Jun 8, 2026

View reviewed changes

evgeni added 4 commits June 8, 2026 15:07

setup external bind that can be used for dns_nsupdate provider

b423381

expose DNS feature

7a6b54d

add tests for DNS feature

d901036

document new parameters

35f4dfe

evgeni force-pushed the rndc branch from bfcb8fc to 35f4dfe Compare June 8, 2026 13:24

shubhamsg199 reviewed Jun 8, 2026

View reviewed changes

		- name: Include tasks for {{ foreman_proxy_dns_provider }}
		ansible.builtin.include_tasks: feature.yaml



		@pytest.mark.feature('dns')
		def test_dns_nsupdate(server, certificates, server_fqdn):

		\| `--foreman-proxy-dns-server` \| Configure the target DNS server \| `--foreman-proxy-dns-server` \|
		\| `--foreman-proxy-keyfile` \| Confogure the key for the target DNS server \| `--foreman-proxy-keyfile` \|

Conversation

evgeni commented Jun 1, 2026

Why are you introducing these changes? (Problem description, related links)

What are the changes introduced in this pull request?

How to test this pull request

Checklist

Uh oh!

ehelms commented Jun 2, 2026

Uh oh!

evgeni commented Jun 2, 2026

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!

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!

shubhamsg199 Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ekohl 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!

evgeni commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

5 participants

shubhamsg199 Jun 8, 2026 •

edited

Loading

evgeni commented Jun 8, 2026 •

edited

Loading