Conversation
|
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
Ansible Role — service not enabled
Missing obsah metadata
|
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
Technically correct, but nothing reboots the VM.
And yet it works… ;) (Yes, those are all valid points, but their criticality and impact is way off) |
evgeni
force-pushed
the
rndc
branch
4 times, most recently
from
a07bb03 to
809c5ed
Compare
evgeni
changed the title
| parameter: --dns-provider | ||
| help: DNS provider to use. | ||
| choices: | ||
| - nsupdate |
There was a problem hiding this comment.
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_
| - [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 |
There was a problem hiding this comment.
obsah can't do required_if on a feature, but it should?
| - name: Include tasks for {{ foreman_proxy_dns_provider }} | ||
| ansible.builtin.include_tasks: feature.yaml |
There was a problem hiding this comment.
is this too much of a "clever" hack? a feature (dns) enabling a "sub" feature (dns_nsupdate)?
There was a problem hiding this comment.
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.
| - name: Include tasks for {{ foreman_proxy_dns_provider }} | ||
| ansible.builtin.include_tasks: feature.yaml |
There was a problem hiding this comment.
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
force-pushed
the
rndc
branch
2 times, most recently
from
edc8cba to
591b419
Compare
evgeni
force-pushed
the
rndc
branch
2 times, most recently
from
dfd0a70 to
ed04896
Compare
| - [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]] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That means only foremanctl deploy --add-feature dns would fail?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
That means only
foremanctl deploy --add-feature dnswould fail?
No.
--add-feature dns --dns-provider nsupdate will fail validation with "dns_server is required" and "dns_keyfile" is required. But if you don't pass --dns-provider it will fail only later during actual execution as you've observed it:
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 undefinedWhich 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?
Should it? Yes. But right now I have no good idea how to implement that :(
There was a problem hiding this comment.
Okay, after bit of talking with @ekohl, we ended up declaring dns_provider a required parameter (without a default value, as the nsupdate default wouldn't work out of the box anyway), which solves this particular issue.
evgeni
force-pushed
the
rndc
branch
3 times, most recently
from
041d5d0 to
6fd746e
Compare
| if dns_type == 'PTR': | ||
| expected = dns_name | ||
| query = dns_value | ||
| else: | ||
| expected = dns_value | ||
| query = dns_name |
There was a problem hiding this comment.
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.
evgeni
force-pushed
the
rndc
branch
3 times, most recently
from
e763aa3 to
bfcb8fc
Compare
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
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:
(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. |
| - [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]] |
There was a problem hiding this comment.
That means only foremanctl deploy --add-feature dns would fail?
| --- | ||
| - name: Create RNDC Key podman secret | ||
| containers.podman.podman_secret: | ||
| state: present |
There was a problem hiding this comment.
Taking it a step further, this should probably look at the enabled state too. Ideally we would clean up items (secrets, systemd drop-ins).
There was a problem hiding this comment.
Right now this file is not included when the feature is disabled, but we certainly can extend this.
| - name: Include tasks for {{ item }} | ||
| ansible.builtin.include_tasks: feature.yaml | ||
| vars: | ||
| feature_enabled: "{{ foreman_proxy_dns_provider_name == item }}" |
There was a problem hiding this comment.
Should this also look at the dns feature being enabled?
evgeni
force-pushed
the
rndc
branch
2 times, most recently
from
67264db to
137406c
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughThis PR adds DNS support to the Foreman Proxy using the nsupdate provider. It introduces a new DNS feature with configurable provider, server, and keyfile parameters; establishes infrastructure to provision a BIND test server via Vagrant and Ansible; implements configuration management through role defaults, feature tasks, and templated settings; securely mounts the RNDC authentication key into the Foreman Proxy container; and adds comprehensive test coverage including DNS record CRUD operations with dig verification. Documentation updates track the new installer parameters. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/user/parameters.md (1)
120-124: 💤 Low valueConsider introducing a "Dropped" category as previously suggested.
The current "Unmapped" section with reason "not supported" is functional, but as ekohl suggested in a previous comment, introducing a third category "Dropped" (alongside "Mapped" and "Unmapped") for parameters that will not be implemented might provide clearer semantics. This would distinguish between parameters that are unmapped temporarily vs. those that are intentionally not supported.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/user/parameters.md` around lines 120 - 124, Add a new "Dropped" subsection alongside the existing "Mapped" and "Unmapped" sections and move parameters that are intentionally not supported into it; specifically, remove `--foreman-proxy-dns-managed` from the "Unmapped" table and list it under the new "Dropped" heading with the same description ("Installer managed DNS") and a clear reason ("intentionally not supported / dropped"). Ensure the "Unmapped" section is reserved for temporarily unmapped items and update any introductory text that describes the semantics of each category to reflect the three categories: Mapped, Unmapped, and Dropped.src/roles/foreman_proxy/tasks/feature.yaml (1)
6-6: ⚡ Quick winNormalize
feature_enabledto boolean checks.
feature_enabledis compared to the string"false"here, butsrc/roles/foreman_proxy/tasks/feature/dns.yamlpasses a boolean. That mismatch will treat booleanfalseas enabled once multiple providers exist.Suggested fix
- data: "{{ lookup('ansible.builtin.template', 'settings.d/' + feature_name + '.yml.j2') if feature_enabled != 'false' else ':enabled: false' }}" + data: "{{ lookup('ansible.builtin.template', 'settings.d/' + feature_name + '.yml.j2') if (feature_enabled | bool) else ':enabled: false' }}" @@ - - feature_enabled != "false" + - feature_enabled | boolAlso applies to: 27-27
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/roles/foreman_proxy/tasks/feature.yaml` at line 6, The condition currently compares feature_enabled to the string "false" which fails when callers pass a boolean; change the ternary to coerce feature_enabled to a boolean (e.g. use feature_enabled | bool) so the lookup runs only when feature_enabled is truthy: replace the expression "lookup('ansible.builtin.template', 'settings.d/' + feature_name + '.yml.j2') if feature_enabled != 'false' else ':enabled: false'" with a boolean-coerced check like "lookup(...) if feature_enabled | bool else ':enabled: false'"; apply the same change to the other occurrence referenced (line with the same feature_enabled ternary).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/user/parameters.md`:
- Line 115: Fix the typo in the table row for the DNS feature: change "feauture"
to "feature" in the description for the `--add-feature dns` entry (the row that
references `--foreman-proxy-dns`) so the docs read "Enable DNS feature".
- Line 120: The heading level for "Unmapped" is incorrect (it is `#####
Unmapped` while the preceding heading is `### Mapped`); change the "Unmapped"
heading to the next incremental level (`#### Unmapped`) so headings progress by
one level (refer to the existing "### Mapped" and update the "Unmapped" heading
accordingly).
---
Nitpick comments:
In `@docs/user/parameters.md`:
- Around line 120-124: Add a new "Dropped" subsection alongside the existing
"Mapped" and "Unmapped" sections and move parameters that are intentionally not
supported into it; specifically, remove `--foreman-proxy-dns-managed` from the
"Unmapped" table and list it under the new "Dropped" heading with the same
description ("Installer managed DNS") and a clear reason ("intentionally not
supported / dropped"). Ensure the "Unmapped" section is reserved for temporarily
unmapped items and update any introductory text that describes the semantics of
each category to reflect the three categories: Mapped, Unmapped, and Dropped.
In `@src/roles/foreman_proxy/tasks/feature.yaml`:
- Line 6: The condition currently compares feature_enabled to the string "false"
which fails when callers pass a boolean; change the ternary to coerce
feature_enabled to a boolean (e.g. use feature_enabled | bool) so the lookup
runs only when feature_enabled is truthy: replace the expression
"lookup('ansible.builtin.template', 'settings.d/' + feature_name + '.yml.j2') if
feature_enabled != 'false' else ':enabled: false'" with a boolean-coerced check
like "lookup(...) if feature_enabled | bool else ':enabled: false'"; apply the
same change to the other occurrence referenced (line with the same
feature_enabled ternary).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f749d97-486e-4316-85ed-234defccdc56
📒 Files selected for processing (17)
.github/workflows/test.ymlVagrantfiledevelopment/playbooks/remote-named/metadata.obsah.yamldevelopment/playbooks/remote-named/remote-named.yamldevelopment/playbooks/test/test.yamldevelopment/roles/bind/files/empty.zonedevelopment/roles/bind/tasks/main.ymldocs/user/parameters.mdsrc/features.yamlsrc/playbooks/deploy/metadata.obsah.yamlsrc/roles/foreman_proxy/defaults/main.yamlsrc/roles/foreman_proxy/tasks/feature.yamlsrc/roles/foreman_proxy/tasks/feature/dns.yamlsrc/roles/foreman_proxy/tasks/feature/dns_nsupdate.yamlsrc/roles/foreman_proxy/templates/settings.d/dns.yml.j2src/roles/foreman_proxy/templates/settings.d/dns_nsupdate.yml.j2tests/foreman_proxy_test.py
| type: AbsolutePath | ||
| foreman_proxy_dns_server: | ||
| parameter: --dns-server | ||
| help: DNS server to be managed |
There was a problem hiding this comment.
I wonder how descriptive we can be. I think this helps:
| help: DNS server to be managed | |
| help: Hostname of the DNS server to be managed |
I'm worried about managed implying something more because in our old installer we fully managed the server. Now we just manage the zone contents.
There was a problem hiding this comment.
I first wanted to do "to send nsupdate requests to", but infoblox uses the same setting for its server, so ended up with "to communicate with".
| - name: Include tasks for {{ item }} | ||
| ansible.builtin.include_tasks: feature.yaml | ||
| vars: | ||
| feature_enabled: "{{ foreman_proxy_dns_provider_name == item }}" |
There was a problem hiding this comment.
Coderabbit (in a nitpick) pointed out that this is now a boolean, while the original intent of the variable was to map the whole spectrum of true/false/http/https that foreman proxy supports and with a boolean the != 'false' will be wrong. Sounds like a very valid concern to me, will think about it.
(Mostly commenting so it's not lost in the rabbit hole, hah)
5 participants
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