Add BMC feature to foreman-proxy by arvind4501 · Pull Request #484 · theforeman/foremanctl

arvind4501 · 2026-05-04T05:50:54Z

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

Enable BMC feature for proxy

What are the changes introduced in this pull request?

BMC feature for foreman-proxy
Default bmc provider ipmi

How to test this pull request

foremanctl deploy --add-feature bmc

Steps to reproduce:

Checklist

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

arvind4501 · 2026-05-04T09:26:17Z

To test this with foremanctl we need theforeman/foreman-packaging#13417

evgeni

needs a rebase please

evgeni · 2026-05-05T06:53:00Z

+  foreman_proxy_bmc_default_provider:
+    help: Default BMC provider.
+    choices:
+      - freeipmi
+      - ipmitool
+      - redfish


While the parameter is called "default provider", the overall behavior is a bit different.

The smart proxy code takes the provider from the request, and falls back to the default if unset (honoring the "default" definition):
https://github.com/theforeman/smart-proxy/blob/a5780bc6e0132d7e91fa3074845b2a3d1cc30f01/modules/bmc/bmc_api.rb#L392-L393

But then Foreman comes (who is the only consumer of this API) and always sets the provider unless the selected provider in Foreman is "IPMI":
https://github.com/theforeman/foreman/blob/f2d892f7585c61d71f671298c8158712daace8e8/app/models/nic/bmc.rb#L27

Which basically makes the setting "used IPMI implementation" (as one can't pick one in Foreman), not "fallback provider".

To add insult to injury, everyone disagrees what a provider is. Foreman defines IPMI, Redfish and SSH, but the Proxy knows about freeipmi, ipmitool, redfish, shell and ssh 🤯

I don't think we can (or even should) fix all of that mess here and now, but maybe calling this ipmi_implementation instead of default_provider and dropping redfish would slightly reduce the user confusion?

👀 @stejskalleos

PS: I couldn't find any users of the "Shell" provider, which is basically giving power control over the machine running the smart proxy. First I thought that'd be used in Discovery, but there we have an own implementation. It was added in https://projects.theforeman.org/issues/2387 but the explanation is rather… slim

But then Foreman comes (who is the only consumer of this API) and always sets the provider unless the selected provider in Foreman is "IPMI":
https://github.com/theforeman/foreman/blob/f2d892f7585c61d71f671298c8158712daace8e8/app/models/nic/bmc.rb#L27

In this case of user selected IPMI in foreman UI, the smart proxy does not get any bmc_provider in request and falls back to Proxy::BMC::Plugin.settings.bmc_default_provider , which is ipmitool if its installed via foreman-installer.

To add insult to injury, everyone disagrees what a provider is. Foreman defines IPMI, Redfish and SSH, but the Proxy knows about freeipmi, ipmitool, redfish, shell and ssh 🤯

Yes and also with ssh provider, i am not sure if its supported as provider by foreman, our doc does not say that and even i can't see any ssh key deployed at https://github.com/theforeman/puppet-foreman_proxy/blob/master/manifests/init.pp#L401 path

amolpati30 · 2026-05-08T10:04:05Z

+  foreman_proxy_bmc_ssh_poweron:
+    parameter: --bmc-ssh-poweron
+    help: BMC SSH power on command.
+  foreman_proxy_bmc_ssh_poweroff:
+    parameter: --bmc-ssh-poweroff
+    help: BMC SSH power off command.


Don’t you think we can create a single resource for both on and off, and provide those options as choices?
Like -

foreman_proxy_bmc_ssh_power_on/off: parameter: --bmc-ssh-power-action choices: - on - off help: BMC SSH power on/off command

the thing is it will only set value(on/off) for a single variable either foreman_proxy_bmc_ssh_power_on or foreman_proxy_bmc_ssh_poweroff. but anyway we are removing the support of these providers.

evgeni · 2026-05-08T11:31:14Z

tested this with ipmitool and virtualbmc, worked well

stejskalleos · 2026-05-11T10:23:03Z

    persist: false
+  foreman_proxy_bmc_default_provider:
+    parameter: --bmc-default-provider
+    help: Default BMC provider.


The description is basically the parameter name, so not much of an explanation there for me.

The default implies (to me) the existence of a non-default parameter, which I don't think we have. Why not name it --bmc-provider?

I see it's named the same as in the bmc.yml, which I think should also be named just :bmc_provider:. (not a problem of this PR)

The default implies (to me) the existence of a non-default parameter, which I don't think we have. Why not name it --bmc-provider?

I think this goes back to how we expose default provider in smart-proxy itself, if you see https://github.com/theforeman/smart-proxy/blob/a5780bc6e0132d7e91fa3074845b2a3d1cc30f01/modules/bmc/bmc_api.rb#L393
we look for bmc_default_provider in bmc.yml. i could change the option to --bmc-provider but that then does not change the fact that actually it affects bmc_default_provider.

I think for this change we need to make changes in smart-proxy as well, which IMHO can be looked sepratelyy. what do you think?

The description is basically the parameter name, so not much of an explanation there for me.

description is inherited from puppet
https://github.com/theforeman/puppet-foreman_proxy/blob/master/manifests/init.pp#L208

It's not even the BMC provider, it's the IPMI provider/implementation.

https://github.com/theforeman/foremanctl/pull/484/changes#r3186541821

bmc_default_provider, what's the default value?
The name IMHO does not make sense. But we are not really good at naming. We use anything except provider, see use_provider in https://github.com/theforeman/smart-proxy/blob/develop/config/settings.d/dns.yml.example#L10 and https://github.com/theforeman/smart-proxy/blob/develop/config/settings.d/dhcp.yml.example

So call it --bmc-ipmi-implementation and only allow freeipmi and ipmitool here?

but we also support redfish, so including ipmi in name itself is misleading

Right, but the only place the "default_bmc_provider" is used (effectively) is to select freeipmi or ipmitool when the user selects "IPMI".
redfish is always passed as "redfish" provider

ah.. yes, true in case of foreman is using smart-proxy. as smart-proxy providers APIs i think anyone can consume those(not only foreman), and that then makes me think smart-proxy implemtation of default-provider is fine in itself if foreman is not the only cosumer.

can, sure, but I don't think there actually are any other consumers.

i could make the options here in foremanctl as --bmc-ipmi-implementation and save it in bmc.yml as bmc_default_provider . should i do the changes in smart-proxy implemenation as well

evgeni

This matches the current Puppet implementation, so ACK.

However, I would prefer if we could take the chance to clear things up and rename --bmc-default-provider to --bmc-ipmi-implementation, only allowing freeipmi and ipmitool as values, leaving redfish to be set explicitly by Foreman. Not going to block on this though.

ekohl · 2026-05-12T08:11:30Z

However, I would prefer if we could take the chance to clear things up and rename --bmc-default-provider to --bmc-ipmi-implementation, only allowing freeipmi and ipmitool as values, leaving redfish to be set explicitly by Foreman. Not going to block on this though.

Agreed. IMHO the Puppet implementation was always a bit confusing.

ekohl

I think it is good to stick to https://peps.python.org/pep-0008/#blank-lines

Surround top-level function and class definitions with two blank lines.

And perhaps also enforce that via pylint.

evgeni · 2026-05-13T05:47:54Z

@stejskalleos you had requested changes in the past, were your concerns addressed?

stejskalleos

LGTM, my requested changes have been implemented.
(I have not tested the feature myself tho)

ekohl · 2026-05-13T07:56:54Z

 FOREMAN_PROXY_PORT = 8443

+def get_proxy_feature_setting(server, feature):
+    cmd = server.run(f"podman exec foreman-proxy cat /etc/foreman-proxy/settings.d/{feature}.yml")


Perhaps not for this PR, but I think we should avoid reading config files where possible (treat them as internal details), but there is a way to expose settings in the Smart Proxy. Then it shows up in /v2/features as an exposed setting and you can query it from the outside.

I tried to see /v2/features endpoint but could not see anything related settings, i am only able to query /features which gives me

[ "bmc", "logs" ]

but nothing in case of /v2/features

I think https://github.com/theforeman/smart-proxy/blob/develop/modules/bmc/bmc_plugin.rb does not expose any extra setting as done in https://github.com/theforeman/smart-proxy/blob/4830f1b0f4ba552d6c124e615722b80aa7846eaf/modules/tftp/tftp_plugin.rb#L14
or did i missinterpreted you?

I tried to see /v2/features endpoint but could not see anything related settings, i am only able to query /features which gives me

You need to be authenticated so see that since settings can contain confidential information. See https://theforeman.org/2019/04/smart-proxy-capabilities-explained.html for more information.

I think https://github.com/theforeman/smart-proxy/blob/develop/modules/bmc/bmc_plugin.rb does not expose any extra setting as done in https://github.com/theforeman/smart-proxy/blob/4830f1b0f4ba552d6c124e615722b80aa7846eaf/modules/tftp/tftp_plugin.rb#L14

Correct, which is why you can't test it in this PR. However, we are in control of the BMC plugin so if this is something we care about then perhaps we should expose it.

The larger point: how do we test functionality going forward? Reading the actual config files is IMHO more of a unit test level thing. However, I'd expect the end-to-end test suite to be more black box and verify behavior. So let Foreman Proxy actually parse the config and read the state of the service. This will also uncover any typos that you may have made in config files or other bugs.

Was that clearer?

Yes, thanks for the explanation. so we can merge this PR and tackle the tests sepreately as requires changes in smart-proxy repo

that's /features what you pasted, /v2/features needs auth and gives you:

{ "dynflow": { "http_enabled": false, "https_enabled": false, "settings": {}, "state": "disabled", "capabilities": [] }, "ansible": { "http_enabled": false, "https_enabled": false, "settings": {}, "state": "disabled", "capabilities": [] }, "script": { "http_enabled": false, "https_enabled": false, "settings": {}, "state": "disabled", "capabilities": [] }, "facts": { "http_enabled": false, "https_enabled": false, "settings": {}, "state": "disabled", "capabilities": [] }, "dns": { "http_enabled": false, "https_enabled": false, "settings": { "use_provider": null }, "state": "disabled", "capabilities": [] }, "templates": { "http_enabled": false, "https_enabled": false, "settings": { "template_url": null }, "state": "disabled", "capabilities": [] }, "tftp": { "http_enabled": false, "https_enabled": false, "settings": { "tftp_servername": null }, "state": "disabled", "capabilities": [ "bootloader_universe" ] }, "dhcp": { "http_enabled": false, "https_enabled": false, "settings": { "use_provider": null }, "state": "disabled", "capabilities": [] }, "puppetca": { "http_enabled": false, "https_enabled": false, "settings": { "use_provider": null }, "state": "disabled", "capabilities": [] }, "puppet": { "http_enabled": false, "https_enabled": false, "settings": { "use_provider": null }, "state": "disabled", "capabilities": [] }, "bmc": { "http_enabled": true, "https_enabled": true, "settings": {}, "state": "running", "capabilities": [ "freeipmi", "ipmitool", "power_action_v2", "redfish", "shell", "ssh" ] }, "realm": { "http_enabled": false, "https_enabled": false, "settings": { "use_provider": null }, "state": "disabled", "capabilities": [] }, "logs": { "http_enabled": true, "https_enabled": true, "settings": {}, "state": "running", "capabilities": [] }, "httpboot": { "http_enabled": false, "https_enabled": false, "settings": { "http_port": null, "https_port": null }, "state": "disabled", "capabilities": [] }, "registration": { "http_enabled": false, "https_enabled": false, "settings": { "registration_url": null }, "state": "disabled", "capabilities": [] } }

(via curl --cacert certificates/certs/ca.crt --cert certificates/certs/quadlet.example.com-client.crt --key certificates/private/quadlet.example.com-client.key -k -v https://$(hostname -f):8443/v2/features | python3 -m json.tool)

and there you can see in BMC what it can do, but sadly not what the default provider is.

and there you can see in BMC what it can do, but sadly not what the default provider is.

As I noted below (#484 (comment)) I'd still argue that asserting freeipmi and ipmitool as capabilities is a good test. It asserts that both tools are available in the container image and Foreman Proxy detects them as usable. Pretty sure it would uncover when theforeman/foreman-packaging#13417 isn't in the container image.

"bmc": { "http_enabled": true, "https_enabled": true, "settings": {}, "state": "running", "capabilities": [ "freeipmi", "ipmitool", "power_action_v2", "redfish", "shell", "ssh" ] },

IMHO this also uncovers another TODO item: http_enabled should IMHO be false so that we only expose it over HTTPS.

would you like me to set http_enabled false in bmc config, here or https://github.com/theforeman/smart-proxy/blob/4a468d730426ca561f9e2f088589427ae2f96843/lib/proxy/settings/plugin.rb#L4?

given we don't expose the http port today, this is not relevant, IMHO

Fair, but it's something to keep in mind. Within the provisioning context we will see it and need to track that work when we do expose the HTTP port.

theforeman/smart-proxy#943 for exposing default provider setting

ekohl · 2026-05-13T07:58:47Z

In case it wasn't clear: don't consider my review as blocking but as hints to further assert a correct implementation.

amolpati30

I have tested the following:

Verified the parameter behavior during and after deployment, and no failures were observed.
Tested the BMC functionality on the iDRAC server and performed power on/off actions, which worked as expected.

ekohl · 2026-05-13T13:17:40Z

    assert cmd.stdout == '201'
+
+
+@pytest.mark.skipif("not is_bmc_enabled()")


In theforeman/smoker@1f7fd3a I implemented a mechanism in smoker to dynamically read the plugins. Perhaps for a follow up to implement this for enabled features so you can use:

Suggested change

@pytest.mark.skipif("not is_bmc_enabled()")

@pytest.mark.feature("bmc")

That would provide a much more generic basis for testing features.

I couldn't help myself: #508.

evgeni · 2026-05-15T09:56:44Z

Let's continue this in #508

pr-processor Bot added the Not yet reviewed label May 4, 2026

arvind4501 force-pushed the bmc branch from f157107 to 1cf009c Compare May 4, 2026 05:55

stejskalleos self-assigned this May 4, 2026

evgeni reviewed May 5, 2026

View reviewed changes

pr-processor Bot removed the Not yet reviewed label May 5, 2026

arvind4501 force-pushed the bmc branch 2 times, most recently from edd8df9 to 602b4fd Compare May 5, 2026 07:25

evgeni reviewed May 6, 2026

View reviewed changes

Comment thread tests/bmc_test.py Outdated

evgeni reviewed May 6, 2026

View reviewed changes

Comment thread tests/bmc_test.py Outdated

stejskalleos reviewed May 6, 2026

View reviewed changes

Comment thread src/playbooks/deploy/metadata.obsah.yaml

evgeni reviewed May 8, 2026

View reviewed changes

Comment thread src/roles/foreman_proxy/templates/settings.d/bmc.yml.j2 Outdated

evgeni reviewed May 8, 2026

View reviewed changes

Comment thread src/playbooks/deploy/metadata.obsah.yaml

evgeni reviewed May 8, 2026

View reviewed changes

Comment thread tests/foreman_proxy_test.py Outdated

amolpati30 reviewed May 8, 2026

View reviewed changes

arvind4501 force-pushed the bmc branch from 68c51a9 to 20aee61 Compare May 8, 2026 11:35

evgeni reviewed May 8, 2026

View reviewed changes

Comment thread tests/foreman_proxy_test.py Outdated

Comment thread tests/foreman_proxy_test.py Outdated

Comment thread tests/foreman_proxy_test.py

arvind4501 force-pushed the bmc branch 2 times, most recently from 395b644 to adb1df7 Compare May 11, 2026 10:08

stejskalleos requested changes May 11, 2026

View reviewed changes

pr-processor Bot added the Waiting on contributor label May 11, 2026

evgeni reviewed May 11, 2026

View reviewed changes

Comment thread src/features.yaml Outdated

pr-processor Bot added Needs re-review and removed Waiting on contributor Needs re-review labels May 11, 2026

evgeni approved these changes May 12, 2026

View reviewed changes

Comment thread tests/foreman_proxy_test.py Outdated

ekohl reviewed May 12, 2026

View reviewed changes

Comment thread tests/conftest.py

arvind4501 force-pushed the bmc branch from 7b0e0c5 to 644609c Compare May 12, 2026 09:08

stejskalleos approved these changes May 13, 2026

View reviewed changes

ekohl reviewed May 13, 2026

View reviewed changes

amolpati30 approved these changes May 13, 2026

View reviewed changes

Arvind Jangir added 10 commits May 13, 2026 18:33

Add BMC feature to foreman-proxy

1263c65

Add Redfish configurations

2c9f0b6

tests

a331e56

update parameters doc

9d1c33f

refactor tests

59042b5

don't apply config for only default provider

babe207

fix tests and enable bmc in CI

690f9d7

don't mention ssh

7e6c784

Limit default provider to only ipmi implementation

cf2c560

rename test

97dce2e

arvind4501 force-pushed the bmc branch from 644609c to 97dce2e Compare May 13, 2026 13:05

ruff fixes

34cd628

ekohl reviewed May 13, 2026

View reviewed changes

test bmc capabilites

b9bb58b

arvind4501 force-pushed the bmc branch from 05a3ff2 to b9bb58b Compare May 14, 2026 05:50

ekohl mentioned this pull request May 14, 2026

Expose user enabled features as pytest markers #508

Merged

2 tasks

evgeni mentioned this pull request May 15, 2026

Pulp import export support #455

Merged

evgeni merged commit e531086 into theforeman:master May 15, 2026
49 of 57 checks passed

		assert cmd.stdout == '201'


		@pytest.mark.skipif("not is_bmc_enabled()")

	@pytest.mark.skipif("not is_bmc_enabled()")
	@pytest.mark.feature("bmc")

Conversation

arvind4501 commented May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

arvind4501 commented May 4, 2026

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evgeni commented May 8, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arvind4501 May 11, 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!

evgeni May 11, 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!

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!

evgeni left a comment

Choose a reason for hiding this comment

Uh oh!

ekohl commented May 12, 2026

Uh oh!

Uh oh!

ekohl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

evgeni commented May 13, 2026

Uh oh!

stejskalleos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arvind4501 May 13, 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!

arvind4501 commented May 4, 2026 •

edited

Loading

arvind4501 May 11, 2026 •

edited

Loading

evgeni May 11, 2026 •

edited

Loading

arvind4501 May 13, 2026 •

edited

Loading

evgeni May 13, 2026 •

edited

Loading

ekohl May 13, 2026 •

edited

Loading