Skip to content

httpboot feature for proxy #518

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
arvind4501 wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

httpboot feature for proxy #518

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/features.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,7 @@ bmc:
description: Power management for bare metal hosts (IPMI, Redfish)
foreman_proxy:
plugin_name: bmc
httpboot:
description: UEFI HTTP Boot provisioning support
foreman_proxy:
plugin_name: httpboot
3 changes: 3 additions & 0 deletions src/roles/foreman_proxy/defaults/main.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,6 @@ foreman_proxy_foreman_server_url: "https://{{ ansible_facts['fqdn'] }}"
# BMC settings
foreman_proxy_bmc_ipmi_implementation: ipmitool
foreman_proxy_bmc_redfish_verify_ssl: true

# HTTPBoot settings

@ehelms ehelms May 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems redundant given the variable name.

foreman_proxy_httpboot_root_dir: /var/lib/tftpboot
19 changes: 19 additions & 0 deletions src/roles/foreman_proxy/tasks/feature/httpboot.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
---
- name: Check if httpboot root directory exists
ansible.builtin.stat:
path: "{{ foreman_proxy_httpboot_root_dir }}"
register: _foreman_proxy_httpboot_root_dir

- name: Mount httpboot root directory
ansible.builtin.copy:
dest: /etc/containers/systemd/foreman-proxy.container.d/httpboot-volume.conf
content: |
[Container]
Volume={{ foreman_proxy_httpboot_root_dir }}:{{ foreman_proxy_httpboot_root_dir }}:z
mode: '0644'
owner: root
group: root
when: _foreman_proxy_httpboot_root_dir.stat.exists
notify:
Comment on lines +7 to +17

@coderabbitai coderabbitai Bot May 28, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid enabling HTTPBoot with an unmounted root_dir.

When the directory is absent, this task skips the mount but the feature config still advertises :root_dir (from src/roles/foreman_proxy/templates/settings.d/httpboot.yml.j2), so HTTPBoot can be enabled in a broken state. Make this deterministic: either create the directory before mounting or fail explicitly if it does not exist.

Suggested fix (fail fast) 
 - name: Check if httpboot root directory exists
   ansible.builtin.stat:
     path: "{{ foreman_proxy_httpboot_root_dir }}"
   register: _foreman_proxy_httpboot_root_dir

+- name: Validate httpboot root directory exists when feature is enabled
+  ansible.builtin.fail:
+    msg: "HTTPBoot root directory '{{ foreman_proxy_httpboot_root_dir }}' does not exist on host."
+  when: not _foreman_proxy_httpboot_root_dir.stat.exists
+
 - name: Mount httpboot root directory
   ansible.builtin.copy:
     dest: /etc/containers/systemd/foreman-proxy.container.d/httpboot-volume.conf
     content: |
       [Container]
       Volume={{ foreman_proxy_httpboot_root_dir }}:{{ foreman_proxy_httpboot_root_dir }}:z
@@
-  when: _foreman_proxy_httpboot_root_dir.stat.exists
   notify:
     - Restart Foreman Proxy
     - Refresh Foreman Proxy
🤖 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/httpboot.yaml` around lines 7 - 17, Add
a deterministic guard before the "Mount httpboot root directory" task: insert a
new task named like "Fail if httpboot root dir missing" that uses
ansible.builtin.fail with a clear message and triggers when not
_foreman_proxy_httpboot_root_dir.stat.exists; keep the existing mount task
unchanged (it can remain conditional on
_foreman_proxy_httpboot_root_dir.stat.exists) so the playbook will fail fast
instead of allowing the httpboot template (settings.d/httpboot.yml.j2) to
advertise a non-existent foreman_proxy_httpboot_root_dir.

@arvind4501 arvind4501 May 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I like fail explicitly if it does not exist rather creating it as tftp feature will create that

@coderabbitai coderabbitai Bot May 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@arvind4501, agreed — keeping the responsibility clear (tftp creates the dir, httpboot only uses it) is the right design. Here is the updated task file reflecting the fail-fast approach:

---
- name: Check if httpboot root directory exists
  ansible.builtin.stat:
    path: "{{ foreman_proxy_httpboot_root_dir }}"
  register: _foreman_proxy_httpboot_root_dir

- name: Fail if httpboot root directory does not exist
  ansible.builtin.fail:
    msg: >-
      HTTPBoot root directory '{{ foreman_proxy_httpboot_root_dir }}' does not
      exist on the host. Enable the tftp feature first, or create the directory
      manually, before enabling httpboot.
  when: not _foreman_proxy_httpboot_root_dir.stat.exists

- name: Mount httpboot root directory
  ansible.builtin.copy:
    dest: /etc/containers/systemd/foreman-proxy.container.d/httpboot-volume.conf
    content: |
      [Container]
      Volume={{ foreman_proxy_httpboot_root_dir }}:{{ foreman_proxy_httpboot_root_dir }}:z
    mode: '0644'
    owner: root
    group: root
  notify:
    - Restart Foreman Proxy
    - Refresh Foreman Proxy

Key changes from the draft:

  • The fail task fires when the directory is absent, giving a clear error message pointing to the dependency on the tftp feature.
  • The when: _foreman_proxy_httpboot_root_dir.stat.exists guard is removed from the mount task — it's now redundant because the play will already have aborted before reaching it.

- Restart Foreman Proxy
- Refresh Foreman Proxy
3 changes: 3 additions & 0 deletions src/roles/foreman_proxy/templates/settings.d/httpboot.yml.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
---
:enabled: {{ feature_enabled }}
:root_dir: {{ foreman_proxy_httpboot_root_dir }}
4 changes: 4 additions & 0 deletions tests/foreman_proxy_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ def test_foreman_proxy_features(server, certificates, server_fqdn, enabled_featu
assert "bmc" in features
else:
assert "bmc" not in features
if 'httpboot' in enabled_features:
assert "httpboot" in features
else:
assert "httpboot" not in features


def test_foreman_proxy_service(server):
Expand Down
Loading