Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,9 @@ jobs:
if: matrix.security != 'none'
run: |
./forge security --mode ${{ matrix.security }}
- name: Setup TFTP server
run: |
./forge tftp
- name: Apply fapolicyd workarounds
# https://access.redhat.com/solutions/7072618 / https://issues.redhat.com/browse/RHEL-37912
# https://github.com/theforeman/foreman-fapolicyd/blob/develop/15-foreman-container.rules
Expand Down Expand Up @@ -152,6 +155,8 @@ jobs:
--add-feature google \
--add-feature remote-execution \
--add-feature bmc \
--add-feature tftp \
--tftp-servername 127.0.0.1 \
${{ matrix.iop == 'enabled' && '--add-feature iop' || '' }}
- name: Run tests
run: |
Expand Down
3 changes: 3 additions & 0 deletions development/playbooks/tftp/metadata.obsah.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
---
Comment thread
evgeni marked this conversation as resolved.
help: |
Setup TFTP server
20 changes: 20 additions & 0 deletions development/playbooks/tftp/tftp.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
- name: TFTP
hosts:
- quadlet
become: true
tasks:
- name: Install packages
ansible.builtin.package:
name:
- tftp-server
- tftp
state: present

- name: Start TFTP services
ansible.builtin.systemd:
name: "{{ item }}"
state: started
enabled: true
with_items:
- tftp.service
13 changes: 9 additions & 4 deletions docs/user/parameters.md
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,15 @@ There are multiple use cases from the users perspective that dictate what parame
| `--add-feature bmc` | Enable BMC feature | `--foreman-proxy-bmc` |
| `--bmc-ipmi-implementation` | IPMI implementation to use for BMC | `--foreman-proxy-bmc-default-provider` |
| `--bmc-redfish-verify-ssl` | Verify SSL certificates for Redfish BMC connections | `--foreman-proxy-bmc-redfish-verify-ssl` |
| `--add-feature tftp` | Enable TFTP feature | `--foreman-proxy-tftp` |
| `--tftp-servername` | IP address of the TFTP server (required when enabling TFTP) | `--foreman-proxy-tftp-servername` |
| `--tftp-root` | Directory to serve TFTP files from | `--foreman-proxy-tftp-root` |
Comment thread
Gauravtalreja1 marked this conversation as resolved.

### Unmapped

| foreman-installer Parameter | Description | Reason |
| --------------------------- | ----------- | ------ |
| `--foreman-proxy-tftp-managed` | Installer managed TFTP | not supported |

### Undetermined

Expand Down Expand Up @@ -144,10 +153,6 @@ There are multiple use cases from the users perspective that dictate what parame
| `--foreman-proxy-plugin-dns-route53-aws-access-key` | | foreman_proxy::plugin::dns_route53 | aws_access_key |
| `--foreman-proxy-plugin-dns-route53-aws-secret-key` | | foreman_proxy::plugin::dns_route53 | aws_secret_key |
| `--foreman-proxy-httpboot` | | foreman_proxy | httpboot |
| `--foreman-proxy-tftp` | | foreman_proxy | tftp |
| `--foreman-proxy-tftp-servername` | | foreman_proxy | tftp_servername |
| `--foreman-proxy-tftp-managed` | | foreman_proxy | tftp_managed |
| `--foreman-proxy-tftp-root` | | foreman_proxy | tftp_root |
| `--foreman-proxy-plugin-dhcp-remote-isc-dhcp-config` | | foreman_proxy::plugin::dhcp_remote_isc | config |
| `--foreman-proxy-plugin-dhcp-remote-isc-dhcp-leases` | | foreman_proxy::plugin::dhcp_remote_isc | dhcp_config |
| `--foreman-proxy-plugin-dhcp-remote-isc-key-name` | | foreman_proxy::plugin::dhcp_remote_isc | key_name |
Expand Down
4 changes: 4 additions & 0 deletions src/features.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -58,3 +58,7 @@ bmc:
description: Power management for bare metal hosts (IPMI, Redfish)
foreman_proxy:
plugin_name: bmc
tftp:
description: Enable TFTP feature on Foreman Proxy
foreman_proxy:
plugin_name: tftp
11 changes: 11 additions & 0 deletions src/playbooks/deploy/metadata.obsah.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,21 @@ variables:
parameter: --bmc-redfish-verify-ssl
help: Verify SSL certificates for Redfish BMC connections.
type: Boolean
foreman_proxy_tftp_root:
parameter: --tftp-root
help: Directory to serve TFTP files from, defaults to /var/lib/tftpboot if not specified.
type: AbsolutePath
foreman_proxy_tftp_servername:
parameter: --tftp-servername
help: Server name or IP address for TFTP. TFTP clients typically do not have DNS available, so an IP address is recommended.

constraints:
required_together:
- [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]]
required_in_list:
- [[[features, tftp]], [foreman_proxy_tftp_servername]]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we add a foreman-proxy feature as a requirement here as well?

I tried to enable tftp without foreman-proxy and it was enabled without any errors, but I know ideally it wouldn't work, so should we add a better validation here to check if tftp is present in required_in_list or do we want to enable foreman-proxy feature as dependancy to TFTP?

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.

@ekohl What is the correct behavior in this scenario:

User runs foremanctl deploy --add-feature tftp without the --add-feature foreman-proxy (in the current run or in the previous one).

What's the expected behavior?
a. Foremanctl should fail with an error about the required feature
b. Foremanctl should automatically add the smart proxy feature

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.

I don't think tftp should silently pull in foreman-proxy. Adding a whole separate service is something that must be done explicitly IMHO because you may also need to open up firewall ports. So I'd lean more to an error that a required feature is not enabled.

This will also apply to all other Smart Proxy features, like DNS and DHCP. I lean to tracking that as a follow up task because I doubt we fixed in other places.

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.


include:
- _certificate_source
Expand Down
5 changes: 5 additions & 0 deletions src/plugins/modules/migrate_answers.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ def cast_database_mode(value):
('foreman', 'server_ssl_key'): 'IGNORE',
('foreman', 'server_ssl_ca'): 'IGNORE',

# Foreman Proxy TFTP
('foreman_proxy', 'tftp_managed'): 'IGNORE',
('foreman_proxy', 'tftp_root'): 'foreman_proxy_tftp_root',
('foreman_proxy', 'tftp_servername'): 'foreman_proxy_tftp_servername',

# TODO: Add more mappings as discovered
}

Expand Down
4 changes: 4 additions & 0 deletions src/roles/foreman_proxy/defaults/main.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,7 @@ foreman_proxy_foreman_server_url: "https://{{ ansible_facts['fqdn'] }}"
# BMC settings
foreman_proxy_bmc_ipmi_implementation: ipmitool
foreman_proxy_bmc_redfish_verify_ssl: true

# TFTP settings
foreman_proxy_tftp_root: /var/lib/tftpboot
foreman_proxy_tftp_servername: "{{ undef(hint='You must specify a TFTP servername') }}"
23 changes: 23 additions & 0 deletions src/roles/foreman_proxy/tasks/feature/tftp.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
---
- name: TFTP root directory
notify:
- Restart Foreman Proxy
- Refresh Foreman Proxy
block:
- name: Create TFTP root directory
ansible.builtin.file:
path: "{{ foreman_proxy_tftp_root }}"
state: directory
mode: "0755"
owner: 998
group: 998

- name: Mount the directory into foreman-proxy container
ansible.builtin.copy:
dest: /etc/containers/systemd/foreman-proxy.container.d/tftp-root.conf
content: |
[Container]
Volume={{ foreman_proxy_tftp_root }}:/var/lib/tftpboot:rw
mode: '0644'
owner: root
group: root
2 changes: 2 additions & 0 deletions src/roles/foreman_proxy/tasks/main.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
sdnotify: true
network: host
hostname: "{{ ansible_facts['hostname'] }}.local"
security_opt:
- "label=disable"
Comment on lines +19 to +20

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IIUC, label=disable effectively disables SELinux for the container. Could you explain the rationale behind this change and the underlying issue it is meant to resolve?

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.

That's the only way to make the container read, write, and list data in the directory.

secrets:
- 'foreman-proxy-settings-yml,type=mount,target=/etc/foreman-proxy/settings.yml'
- 'foreman-proxy-ssl-ca,type=mount,target=/etc/foreman-proxy/ssl_ca.pem'
Expand Down
4 changes: 4 additions & 0 deletions src/roles/foreman_proxy/templates/settings.d/tftp.yml.j2
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
:enabled: {{ feature_enabled }}
:tftproot: /var/lib/tftpboot
:tftp_servername: {{ foreman_proxy_tftp_servername }}
Comment thread
stejskalleos marked this conversation as resolved.
21 changes: 21 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
SSH_CONFIG = './.tmp/ssh-config'
OBSAH_STATE = os.environ.get('OBSAH_STATE', '.var/lib/foremanctl')
PARAMETERS_FILE = os.path.join(OBSAH_STATE, 'parameters.yaml')
FOREMAN_PROXY_PORT = 8443


class UserParameters:
Expand Down Expand Up @@ -271,6 +272,26 @@ def get_connection_with_tls_context(self, request, verify, proxies=None, cert=No
return conn


@pytest.fixture(scope="module")
def proxy_request(server, certificates, server_fqdn):
Comment thread
stejskalleos marked this conversation as resolved.
def _request(path, method=None, data=None, return_body=False):
curl_opts = (
f"--cacert {certificates['server_ca_certificate']} "
f"--cert {certificates['client_certificate']} "
f"--key {certificates['client_key']} "
f"--silent "
)
if not return_body:
curl_opts += "--output /dev/null --write-out '%{http_code}' "
if method:
curl_opts += f"-X {method} "
if data:
curl_opts += f"-d '{data}' "
return server.run(f"curl {curl_opts}https://{server_fqdn}:{FOREMAN_PROXY_PORT}/{path}")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tried this manually, and whenever I invoke this API, it creates the directory structure on the Foreman side rather than on the TFTP server.

Additionally, when I run the tftp command from the test you added, it fails with a File not found error because the expected files are not present on the TFTP server.

Could you elaborate on the expected workflow here and how this test is supposed to work? Am I missing a step that synchronizes or publishes the generated content to the TFTP server?

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.

There is nothing else needed; just replicate the test steps, and it should work. The CI is passing, so it's working.

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.

The core design is that it will be created on the Foreman Proxy side (which may also run a Foreman). It's expected that the admin makes sure the TFTP server also uses the same directory. For example, using NFS as done in https://docs.theforeman.org/3.19/Integrating_Provisioning_Infrastructure_Services/index-foreman-el.html#integrating-a-generic-tftp-server.

Ideally we'd have an accompanying docs PR that explains the end-to-end workflow.


return _request


@pytest.fixture(scope="module")
def local_request(ssh_config, server_fqdn):
session = requests.Session()
Expand Down
31 changes: 21 additions & 10 deletions tests/foreman_proxy_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,18 @@

import pytest

FOREMAN_PROXY_PORT = 8443
from tests.conftest import FOREMAN_PROXY_PORT


@pytest.fixture(scope="module")
def proxy_v2_features(server, certificates, server_fqdn):
cmd = server.run(
f"curl --cacert {certificates['server_ca_certificate']} "
f"--cert {certificates['client_certificate']} "
f"--key {certificates['client_key']} "
f"--silent https://{server_fqdn}:{FOREMAN_PROXY_PORT}/v2/features"
)
def proxy_v2_features(proxy_request):
cmd = proxy_request("v2/features", return_body=True)
assert cmd.succeeded, f"Failed to query /v2/features: {cmd.stderr}"
return json.loads(cmd.stdout)


def test_foreman_proxy_features(server, certificates, server_fqdn, enabled_features):
cmd = server.run(f"curl --cacert {certificates['server_ca_certificate']} --silent https://{server_fqdn}:{FOREMAN_PROXY_PORT}/features")
def test_foreman_proxy_features(proxy_request, enabled_features):
cmd = proxy_request("features", return_body=True)
assert cmd.succeeded
features = json.loads(cmd.stdout)
assert "logs" in features
Expand All @@ -29,6 +24,10 @@ def test_foreman_proxy_features(server, certificates, server_fqdn, enabled_featu
assert "bmc" in features
else:
assert "bmc" not in features
if 'tftp' in enabled_features:
assert "tftp" in features
else:
assert "tftp" not in features


def test_foreman_proxy_service(server):
Expand Down Expand Up @@ -56,6 +55,18 @@ def test_foreman_proxy_client_auth_to_foreman(server, certificates, server_fqdn)
assert cmd.stdout == '201'


@pytest.mark.feature('tftp')
def test_tftp_write_and_fetch(proxy_request, server):
test_mac = "aa:bb:cc:dd:ee:ff"
cmd = proxy_request(f"tftp/{test_mac}", data="syslinux_config=foremanctl+test+probe")
assert cmd.succeeded
assert cmd.stdout == '200', f"Expected HTTP 200 when creating TFTP PXE config, got {cmd.stdout}"

Comment thread
stejskalleos marked this conversation as resolved.
cmd = server.run("tftp 127.0.0.1 -c get pxelinux.cfg/01-aa-bb-cc-dd-ee-ff /tmp/foremanctl_tftp_test_download")
Comment thread
stejskalleos marked this conversation as resolved.
assert cmd.succeeded, f"TFTP get failed: {cmd.stdout}"
assert server.file("/tmp/foremanctl_tftp_test_download").content_string == "foremanctl test probe"


@pytest.mark.feature('bmc')
def test_bmc_capabilities(proxy_v2_features):
assert 'bmc' in proxy_v2_features
Expand Down
Loading