TFTP feature#533
Conversation
|
|
||
| # TFTP settings | ||
| foreman_proxy_tftp_root: /var/lib/tftpboot | ||
| foreman_proxy_tftp_servername: "{{ ansible_facts['fqdn'] }}" |
There was a problem hiding this comment.
Does this need a default? IIRC the code has a fallback on the Smart Proxy's hostname already so this doesn't add anything. In a remote setup it will also be guaranteed wrong.
There was a problem hiding this comment.
Hey, this is completely random (I was just browsing this repository) but:
TFTP clients usually do not have DNS stack available, therefore, this must be IP address if anything. I see fqdn so I assume this would be a hostname. I remember there was some FQDN>IP hack somewhere but using IP was always the best and the safest thing to do.
This has always been terrible name, servername. What it does is sets up next-server which is probably the root cause of all of this terminology. But what users typically need to set is an IP address.
There was a problem hiding this comment.
You're right, that is a better value. Ideally this value defaults to empty and we force users to provide the value when they enable the TFTP feature. Then input validation forces a valid IPv4 address.
There was a problem hiding this comment.
We could at least rename the parameter in foremactl.
Change it to tftp-ip and update the description.
WDYT?
evgeni
left a comment
evgeni
left a comment
There was a problem hiding this comment.
Can we get a test that sets up a tftp-server (on the quadlet machine), instructs the proxy to create a file in the tftp root and uses tftp to fetch that file?
| type: Boolean | ||
| foreman_proxy_tftp_root: | ||
| parameter: --tftp-root | ||
| help: Directory to serve TFTP files from. |
There was a problem hiding this comment.
Ewouds comment in jira made me realize that this description is wrong.
We also use /var/lib/tftpboot for the httpboot module (https://github.com/theforeman/smart-proxy/blob/3e249dd019dd36f41b546fe44c045ab055c1da6c/modules/httpboot/httpboot_plugin.rb#L13) and do not allow users to configure that in Puppet.
It is unclear to me how the HTTPBoot thing obtains the files (seems there is no code for that), so maybe it relies on the TFTP module to place the files and just exposes them over HTTP instead of TFTP?
(This is not blocking the feature here, just food for further thought going forward)
There was a problem hiding this comment.
I also faced similar doubts while just scaffolding httpboot feature, and i realised its use /var/lib/tftp which is not created by httpboot, which makes me think why we don't add tftp as dependency for httpboot. for full context you can refer #518 (comment)
There was a problem hiding this comment.
which makes me think why we don't add tftp as dependency for httpboot.
You may find theforeman/smart-proxy@db33bc6 interesting.
Perhaps we should call it the netboot directory? I already had https://github.com/theforeman/puppet-foreman_proxy/blob/master/manifests/tftp/netboot.pp in the old installer.
There was a problem hiding this comment.
"interesting" as in "wait, what!?"? ;-)
How is the directory populated, when the TFTP feature is off, but HTTPBoot is on?
There was a problem hiding this comment.
Magic? In theforeman/puppet-foreman_proxy@e4b7763 I wrote:
To get the netboot files, the TFTP feature must still be enabled.
It was a long discussion that was rather tedious. I'm not sure the whole use case of netbooting with HTTPBoot without TFTP was ever really well supported.
There was a problem hiding this comment.
How is the directory populated, when the TFTP feature is off, but HTTPBoot is on?
AFAIK, no population happens, just configuration, As of today, by default httpboot enablement will add :root_dir: /var/lib/tftpboot in config, and still be indirectly dependent on tftp to create and populate that directory
stejskalleos
force-pushed
the
ls/feat_tftp
branch
from
87e0ed6 to
507e60a
Compare
pr-processor
Bot
added
Needs re-review
and removed
Waiting on contributor
Needs re-review
labels
I'm still working on this test; I will add it in the upcoming commit. |
stejskalleos
force-pushed
the
ls/feat_tftp
branch
from
507e60a to
4fc9079
Compare
|
@ehelms applied comments from your review. |
| type: AbsolutePath | ||
| foreman_proxy_tftp_servername: | ||
| parameter: --tftp-servername | ||
| help: Server name to use for TFTP. |
There was a problem hiding this comment.
Given the validation message below, I'd include this line here in some form:
This should be the IP address of the TFTP server (TFTP clients typically do not have DNS available).
There was a problem hiding this comment.
Which I will also point out is slightly ironic we call it servername and then state it should be the IP address.
There was a problem hiding this comment.
Technically we're talking about https://www.rfc-editor.org/info/rfc2132/#section-9.4. DHCP option 66 is called TFTP server name. Sadly it doesn't describe at all what the content of it should be at the protocol level.
If we look at what we do for ISC DHCP (https://github.com/theforeman/smart-proxy/blob/3e249dd019dd36f41b546fe44c045ab055c1da6c/modules/dhcp_common/isc/omapi_provider.rb#L205-L217) then that's basically trying to send an IP, but ISC DHCP itself can also be configured with a hostname where the service does a lookup.
Kea's all-options.json actually has an example that is a hostname: https://github.com/isc-projects/kea/blob/f79874a1f22c29b96ee543f5bc905960818991f7/doc/examples/kea4/all-options.json#L787-L798.
So I'm not sure we need to be so strict in only accepting an IP. Apologies for not digging into that deep enough before.
stejskalleos
force-pushed
the
ls/feat_tftp
branch
from
4fc9079 to
738dbfe
Compare
stejskalleos
force-pushed
the
ls/feat_tftp
branch
2 times, most recently
from
f92121c to
bee64a8
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughThis pull request adds TFTP feature support to foremanctl, enabling Foreman Proxy to serve PXE boot files via TFTP. The changes declare a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/roles/foreman_proxy/templates/settings.d/tftp.yml.j2 (1)
3-3:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse container path instead of host path for
:tftproot.The Foreman Proxy runs inside a container where
{{ foreman_proxy_tftp_root }}(the host path) is mounted to/var/lib/tftpboot(seesrc/roles/foreman_proxy/tasks/feature/tftp.yamlline 26). The configuration must reference the container path, not the host path. When a user specifies a custom--tftp-root, the current template will render a path that doesn't exist inside the container, breaking TFTP functionality.🐛 Proposed fix
--- :enabled: {{ feature_enabled }} -:tftproot: {{ foreman_proxy_tftp_root }} +:tftproot: /var/lib/tftpboot :tftp_servername: {{ foreman_proxy_tftp_servername }} :tftp_connect_timeout: 10 :verify_server_cert: true🤖 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/templates/settings.d/tftp.yml.j2` at line 3, The template settings.d/tftp.yml.j2 is using the host path variable foreman_proxy_tftp_root for :tftproot:, which is wrong inside the container; change the template to reference the container TFTP root (/var/lib/tftpboot) instead of {{ foreman_proxy_tftp_root }} so the rendered config points to the path mounted inside the Foreman Proxy container (update settings.d/tftp.yml.j2 to use /var/lib/tftpboot for :tftproot:).
🧹 Nitpick comments (2)
docs/user/parameters.md (1)
117-117: ⚡ Quick winConsider using the more descriptive help text from metadata.
The description "Directory for TFTP" is brief. For consistency with the CLI help text defined in
src/playbooks/deploy/metadata.obsah.yaml, consider using "Directory to serve TFTP files from."📝 Suggested improvement
-| `--tftp-root` | Directory for TFTP | `--foreman-proxy-tftp-root` | +| `--tftp-root` | Directory to serve TFTP files from | `--foreman-proxy-tftp-root` |🤖 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` at line 117, The docs line for the CLI flag `--tftp-root` uses a terse description; update the help text to match the more descriptive metadata wording by replacing "Directory for TFTP" with "Directory to serve TFTP files from" to keep consistency with the CLI help defined in src/playbooks/deploy/metadata.obsah.yaml and ensure the parameter `--tftp-root` documentation mirrors that metadata description.src/playbooks/deploy/metadata.obsah.yaml (1)
59-65: ⚡ Quick winConsider clarifying that
--tftp-servernameshould be an IP address.The help text at line 65 says "Server name to use for TFTP" but doesn't mention that TFTP clients typically lack DNS, so this should be an IP address. The validation error message in
src/roles/foreman_proxy/tasks/feature/tftp.yamlline 9 does explain this, but users won't see that guidance until validation fails.📝 Suggested help text improvement
foreman_proxy_tftp_servername: parameter: --tftp-servername - help: Server name to use for TFTP. + help: Server name (typically an IP address) to use for TFTP. TFTP clients usually do not have DNS available.🤖 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/playbooks/deploy/metadata.obsah.yaml` around lines 59 - 65, Update the help text for the foreman_proxy_tftp_servername parameter (parameter: --tftp-servername) to explicitly state that it must be an IP address (TFTP clients typically lack DNS resolution), so users see this guidance before validation; modify the "help" value to mention "IP address (no DNS)" or similar and optionally include an example format to match the validation in src/roles/foreman_proxy/tasks/feature/tftp.yaml.
🤖 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 `@development/playbooks/tftp/metadata.obsah.yaml`:
- Line 1: The playbook currently located under development/playbooks/tftp/
(metadata.obsah.yaml) must be relocated into one of the approved subdirectories
under development/playbooks/ (vms/, test/, smoker/, deploy-dev/, security/,
sos/, or lock/); choose the most appropriate bucket for this playbook's purpose,
move metadata.obsah.yaml into that directory, and update any references or
automation that point at development/playbooks/tftp/ to the new path so
discovery/automation continues to work.
---
Duplicate comments:
In `@src/roles/foreman_proxy/templates/settings.d/tftp.yml.j2`:
- Line 3: The template settings.d/tftp.yml.j2 is using the host path variable
foreman_proxy_tftp_root for :tftproot:, which is wrong inside the container;
change the template to reference the container TFTP root (/var/lib/tftpboot)
instead of {{ foreman_proxy_tftp_root }} so the rendered config points to the
path mounted inside the Foreman Proxy container (update settings.d/tftp.yml.j2
to use /var/lib/tftpboot for :tftproot:).
---
Nitpick comments:
In `@docs/user/parameters.md`:
- Line 117: The docs line for the CLI flag `--tftp-root` uses a terse
description; update the help text to match the more descriptive metadata wording
by replacing "Directory for TFTP" with "Directory to serve TFTP files from" to
keep consistency with the CLI help defined in
src/playbooks/deploy/metadata.obsah.yaml and ensure the parameter `--tftp-root`
documentation mirrors that metadata description.
In `@src/playbooks/deploy/metadata.obsah.yaml`:
- Around line 59-65: Update the help text for the foreman_proxy_tftp_servername
parameter (parameter: --tftp-servername) to explicitly state that it must be an
IP address (TFTP clients typically lack DNS resolution), so users see this
guidance before validation; modify the "help" value to mention "IP address (no
DNS)" or similar and optionally include an example format to match the
validation in src/roles/foreman_proxy/tasks/feature/tftp.yaml.
🪄 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: ae1de4bc-3999-41dd-9bdb-dd5c320feaf7
📒 Files selected for processing (10)
.github/workflows/test.ymldevelopment/playbooks/tftp/metadata.obsah.yamldevelopment/playbooks/tftp/tftp.yamldocs/user/parameters.mdsrc/features.yamlsrc/playbooks/deploy/metadata.obsah.yamlsrc/roles/foreman_proxy/defaults/main.yamlsrc/roles/foreman_proxy/tasks/feature/tftp.yamlsrc/roles/foreman_proxy/templates/settings.d/tftp.yml.j2tests/foreman_proxy_test.py
stejskalleos
force-pushed
the
ls/feat_tftp
branch
2 times, most recently
from
7917de5 to
3463691
Compare
stejskalleos
force-pushed
the
ls/feat_tftp
branch
from
3463691 to
59aa338
Compare
stejskalleos
force-pushed
the
ls/feat_tftp
branch
3 times, most recently
from
ed3fa54 to
7119759
Compare
stejskalleos
force-pushed
the
ls/feat_tftp
branch
from
7119759 to
124b366
Compare
6 participants
Why are you introducing these changes? (Problem description, related links)
Support the TFTP Smart Proxy feature.
What are the changes introduced in this pull request?
foremanctl deploy ---add-feature tftpHow to test this pull request
./foremanctl deploy --help --tftp-root FOREMAN_PROXY_TFTP_ROOT Directory to serve TFTP files from. (persisted) --tftp-servername FOREMAN_PROXY_TFTP_SERVERNAME Server name to use for TFTP. (persisted)Go to https://quadlet.example.com/smart_proxies/2-quadlet-example-com, TFTP should be enabled.
cat /etc/foreman-proxy/settings.d/tftp.ymlGo to https://quadlet.example.com/smart_proxies/2-quadlet-example-com, TFTP should be disabled.
Checklist