Draft: Add Puppet proxy (CA) bulk actions by nadjaheitmann · Pull Request #429 · theforeman/foreman_puppet

nadjaheitmann · 2025-09-24T10:52:47Z

@adamruzicka @jeremylenz I have created a draft for the Puppet Proxy update bulk action. It is not more than a draft and I know there is a bunch of things missing (e.g. clearing proxy, CA proxy, tests, ...). I basically took code from Foreman and Katello and mixed it into this. Do you mind checking whether the general approach is the direction we want to go. UI wise, I think this is straight forward, but I was not sure whether

extending the BulkHostManager
adding a new API endpoint to the HostsBulkActionsController
adding the API endpoint itself to the Foreman rather than the foreman_puppet namespace

is the way to go.

I appreciate your opinions.

adamruzicka

Left some notes inline, can't speak much to the frontend

adamruzicka · 2025-09-24T12:01:14Z

+
+        api :PUT, "/hosts/bulk/change_puppet_proxy", N_("Change Puppet Proxy")
+        param_group :bulk_host_ids
+        param :proxy_id, :number, :required => true, :desc => N_("ID of the Puppet proxy to reassign the hosts to")


This being required means it can be used to reassign the host to a different proxy, but this cannot be used to unassign it, correct?

Correct. I have not yet implemented the unassign method, so the implementation details about that one are still open.

adamruzicka · 2025-09-24T12:02:31Z

+              host.puppet_ca_proxy = proxy
+            else
+              host.puppet_proxy = proxy
+            end
+            host.save(:validate => false)


Do we need callbacks to be run or could we take a shortcut and assign the proxy with a single update query?

Not sure, if we need callbacks to be honest. The original action does run some validations about validating the proxy I think: https://github.com/theforeman/foreman_puppet/blob/master/app/controllers/concerns/foreman_puppet/extensions/hosts_controller_extensions.rb#L143

Can the update command also take the validate => false parameter?

nadjaheitmann

Thanks @adamruzicka !

nadjaheitmann · 2025-09-24T12:21:18Z

+
+        api :PUT, "/hosts/bulk/change_puppet_proxy", N_("Change Puppet Proxy")
+        param_group :bulk_host_ids
+        param :proxy_id, :number, :required => true, :desc => N_("ID of the Puppet proxy to reassign the hosts to")


Correct. I have not yet implemented the unassign method, so the implementation details about that one are still open.

nadjaheitmann · 2025-09-24T12:28:00Z

+              host.puppet_ca_proxy = proxy
+            else
+              host.puppet_proxy = proxy
+            end
+            host.save(:validate => false)


Not sure, if we need callbacks to be honest. The original action does run some validations about validating the proxy I think: https://github.com/theforeman/foreman_puppet/blob/master/app/controllers/concerns/foreman_puppet/extensions/hosts_controller_extensions.rb#L143

Can the update command also take the validate => false parameter?

jeremylenz

Did a quick read of the frontend code, a couple comments below

nadjaheitmann · 2026-03-09T16:13:53Z

This PR uses theforeman/foreman@40938e4 , so it will only work with Foreman 3.19+

nadjaheitmann · 2026-04-24T12:45:21Z

@jeremylenz @adamruzicka Can you please re-review the PR? It is not completely in line with what the Bulk actions in Foreman core do/return but it could be implemented in a follow-up. (it does not return a nice link to the failed hosts)

jeremylenz

JS changes LGTM, did not test. 👍

adamruzicka · 2026-04-24T14:57:46Z

+        end
+
+        def find_smart_proxy
+          @proxy = SmartProxy.find_by(id: params[:proxy_id])


This should probably also check that the proxy has the right features

adamruzicka · 2026-04-24T14:59:01Z

+      selectAllHostsMode={selectAllHostsMode}
+      isOpen={isOpen}
+      closeModal={closeModal}
+      title={__('Change Puppet CA Proxy')}


I don't see the BulkChangeProxyCommon accepting a title prop?

It does not need one because title is the same as the changeMessage. I remove it for now.

adamruzicka · 2026-04-24T14:59:07Z

+    'bulk-change-puppet-ca-proxy'
+  );
+  return (
+    <BulkChangeProxyCommom


Suggested change

<BulkChangeProxyCommom

<BulkChangeProxyCommon

Nice catch, I wonder how this did not show up in my tests.

adamruzicka · 2026-04-24T14:59:42Z

+      selectAllHostsMode={selectAllHostsMode}
+      isOpen={isOpen}
+      closeModal={closeModal}
+      title={__('Change Puppet Proxy')}


Copilot

Pull request overview

Adds draft bulk actions for Puppet Proxy and Puppet CA Proxy on the Hosts index, including new React modals and a new backend bulk API/controller to apply the changes to selected hosts.

Changes:

Adds Hosts index “Puppet” bulk-action menu items plus four bulk-action modals (change/remove proxy + change/remove CA proxy).
Introduces shared React “common” components and Redux API actions for change/remove operations.
Adds a new ForemanPuppet API controller/routes plus a BulkHostsManager extension and accompanying Ruby tests.

Reviewed changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
webpack/src/Extends/Hosts/BulkActions/BulkRemovePuppetProxy/index.js	Scene wiring for “Remove Puppet proxy” modal.
webpack/src/Extends/Hosts/BulkActions/BulkRemovePuppetProxy/tests/index.test.js	Unit test ensuring correct props passed to common remove modal.
webpack/src/Extends/Hosts/BulkActions/BulkRemovePuppetCAProxy/index.js	Scene wiring for “Remove Puppet CA proxy” modal.
webpack/src/Extends/Hosts/BulkActions/BulkRemovePuppetCAProxy/tests/index.test.js	Unit test ensuring correct CA props/messages passed.
webpack/src/Extends/Hosts/BulkActions/BulkRemoveProxyCommon/index.js	Shared “remove proxy” confirmation modal + API dispatch.
webpack/src/Extends/Hosts/BulkActions/BulkRemoveProxyCommon/actions.js	Redux API action for bulk remove endpoint.
webpack/src/Extends/Hosts/BulkActions/BulkRemoveProxyCommon/tests/actions.test.js	Unit tests validating APIActions.put usage.
webpack/src/Extends/Hosts/BulkActions/BulkChangePuppetProxy/index.js	Scene wiring for “Change Puppet proxy” modal.
webpack/src/Extends/Hosts/BulkActions/BulkChangePuppetProxy/tests/index.test.js	Unit test ensuring correct props passed to common change modal.
webpack/src/Extends/Hosts/BulkActions/BulkChangePuppetCAProxy/index.js	Scene wiring for “Change Puppet CA proxy” modal.
webpack/src/Extends/Hosts/BulkActions/BulkChangePuppetCAProxy/tests/index.test.js	Unit test ensuring correct CA props/messages passed.
webpack/src/Extends/Hosts/BulkActions/BulkChangeProxyCommon/index.js	Shared “change proxy” modal with Smart Proxy select + bulk change dispatch.
webpack/src/Extends/Hosts/BulkActions/BulkChangeProxyCommon/actions.js	Redux API actions for fetching proxies + bulk change endpoint.
webpack/src/Extends/Hosts/BulkActions/BulkChangeProxyCommon/tests/actions.test.js	Unit tests validating APIActions.get/put usage.
webpack/src/Extends/Hosts/ActionsBar/index.js	Adds “Puppet” flyout under Hosts index kebab with 4 actions.
webpack/src/Extends/Hosts/ActionsBar/ActionsBar.scss	Styles intended for disabled menu description content.
webpack/global_index.js	Registers fills: Hosts index kebab section + modal fills.
test/services/foreman_puppet/bulk_hosts_manager_test.rb	Unit tests for BulkHostsManager puppet proxy updates.
test/controllers/foreman_puppet/api/v2/hosts_bulk_actions_controller_test.rb	Controller tests for change/remove endpoints and error cases.
lib/foreman_puppet/register.rb	Adds RBAC permission mappings for the new API actions.
lib/foreman_puppet/engine.rb	Includes the BulkHostsManager extension concern into core manager.
config/api_routes.rb	Adds new API routes for bulk change/remove puppet proxy.
app/services/concerns/foreman_puppet/extensions/bulk_hosts_manager.rb	Implements the bulk host update logic for puppet proxy fields.
app/controllers/foreman_puppet/api/v2/hosts_bulk_actions_controller.rb	Implements the bulk API endpoints and response handling.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-24T15:10:16Z

+import PropTypes from 'prop-types';
+import { Menu, MenuItem, MenuContent, MenuList } from '@patternfly/react-core';
+import { BanIcon } from '@patternfly/react-icons';
+import { translate as __ } from 'foremanReact/common/I18n';
+import { ForemanHostsIndexActionsBarContext } from 'foremanReact/components/HostsIndex';
+import { openBulkModal } from 'foremanReact/common/BulkModalStateHelper';
+import './ActionsBar.scss';
+
+const DisabledMenuItemDescription = ({ disabledReason }) => (
+  <span className="disabled-menu-item-span">
+    <span className="disabled-menu-item-icon">
+      <BanIcon />
+    </span>
+    <p className="disabled-menu-item-p">{disabledReason}</p>
+  </span>
+);
+
+DisabledMenuItemDescription.propTypes = {
+  disabledReason: PropTypes.string.isRequired,
+};
+


DisabledMenuItemDescription (and the related SCSS) is currently not used anywhere in this component (menu items are only disabled). Either wire it into the disabled state rendering (e.g., via a tooltip/flyout content) or remove it to avoid dead code.

Suggested change

import PropTypes from 'prop-types';

import { Menu, MenuItem, MenuContent, MenuList } from '@patternfly/react-core';

import { BanIcon } from '@patternfly/react-icons';

import { translate as __ } from 'foremanReact/common/I18n';

import { ForemanHostsIndexActionsBarContext } from 'foremanReact/components/HostsIndex';

import { openBulkModal } from 'foremanReact/common/BulkModalStateHelper';

import './ActionsBar.scss';

const DisabledMenuItemDescription = ({ disabledReason }) => (





<BanIcon />



{disabledReason}



);

DisabledMenuItemDescription.propTypes = {

disabledReason: PropTypes.string.isRequired,

};

import { Menu, MenuItem, MenuContent, MenuList } from '@patternfly/react-core';

import { translate as __ } from 'foremanReact/common/I18n';

import { ForemanHostsIndexActionsBarContext } from 'foremanReact/components/HostsIndex';

import { openBulkModal } from 'foremanReact/common/BulkModalStateHelper';

import './ActionsBar.scss';

Copilot · 2026-04-24T15:10:17Z

+      included: {
+        search: fetchBulkParams(),
+      },
+      proxy_id: smartProxyId.split('-')[0],
+      ca_proxy: isCAProxy,
+    };


smartProxyId is stored as a combined string ("<id>-<name>") and later parsed via split('-')[0]. This is brittle (depends on a delimiter and forces string parsing). Prefer using the proxy id itself as the Select value (and keep the label separately) so proxy_id can be sent without parsing.

Copilot · 2026-04-24T15:10:17Z

+Foreman::Application.routes.draw do
+  scope module: 'foreman_puppet' do
+    namespace :api, defaults: { format: 'json' } do
+      scope '(:apiv)', module: :v2, defaults: { apiv: 'v2' }, apiv: /v1|v2/, constraints: ApiConstraints.new(version: 2, default: true) do
+        match 'hosts/bulk/change_puppet_proxy', to: 'hosts_bulk_actions#change_puppet_proxy', via: [:put]
+        match 'hosts/bulk/remove_puppet_proxy', to: 'hosts_bulk_actions#remove_puppet_proxy', via: [:put]
+      end
+    end
+  end
+end
+
 ForemanPuppet::Engine.routes.draw do
  namespace :api, defaults: { format: 'json' } do
    scope '(:apiv)', module: :v2, defaults: { apiv: 'v2' }, apiv: /v1|v2/, constraints: ApiConstraints.new(version: 2, default: true) do


The new bulk routes are added to Foreman::Application.routes.draw, which creates /api/v2/hosts/bulk/... endpoints, but the frontend calls /foreman_puppet/api/v2/hosts/bulk/.... As a result, the UI requests won't hit these routes. Define these match 'hosts/bulk/... routes under ForemanPuppet::Engine.routes.draw (to provide /foreman_puppet/api/v2/...), and if you also want the deprecated core paths, add an explicit reroute/alias separately (similar to config/initializers/api_reroute.rb) rather than redefining application routes here.

Suggested change

Foreman::Application.routes.draw do

scope module: 'foreman_puppet' do

namespace :api, defaults: { format: 'json' } do

scope '(:apiv)', module: :v2, defaults: { apiv: 'v2' }, apiv: /v1|v2/, constraints: ApiConstraints.new(version: 2, default: true) do

match 'hosts/bulk/change_puppet_proxy', to: 'hosts_bulk_actions#change_puppet_proxy', via: [:put]

match 'hosts/bulk/remove_puppet_proxy', to: 'hosts_bulk_actions#remove_puppet_proxy', via: [:put]

end

end

end

end

ForemanPuppet::Engine.routes.draw do

namespace :api, defaults: { format: 'json' } do

scope '(:apiv)', module: :v2, defaults: { apiv: 'v2' }, apiv: /v1|v2/, constraints: ApiConstraints.new(version: 2, default: true) do

ForemanPuppet::Engine.routes.draw do

namespace :api, defaults: { format: 'json' } do

scope '(:apiv)', module: :v2, defaults: { apiv: 'v2' }, apiv: /v1|v2/, constraints: ApiConstraints.new(version: 2, default: true) do

match 'hosts/bulk/change_puppet_proxy', to: 'hosts_bulk_actions#change_puppet_proxy', via: [:put]

match 'hosts/bulk/remove_puppet_proxy', to: 'hosts_bulk_actions#remove_puppet_proxy', via: [:put]

The routes are on purpose still in the Foreman API. This is on purpose as the whole Puppet Proxy thing is still part of Foreman core actually. Any opinion on this @adamruzicka ?

PS: I fixed the UI issue that copilot mentions here.

Copilot · 2026-04-24T15:10:18Z

+            host.puppet_ca_proxy = proxy
+          else
+            host.puppet_proxy = proxy
+          end
+          host.save(validate: false)
+        rescue StandardError => e
+          message = format(_('Failed to set proxy for %{host}.'), host: host)
+          Foreman::Logging.exception(message, e)
+          error_hosts << host.id


host.save(validate: false) can return false (e.g., aborted callbacks) without raising, so failures may be silently treated as success and never added to error_hosts. Consider using save! (and rescuing) or explicitly checking the boolean return value and pushing the host id into error_hosts when the save fails.

One could argue that with validate: false this shouldn't really happen?

I would assume it just works. Not sure what would happen if you e.g. assign a proxy and it is deleted between the check in the API and call the to host.save.

Copilot · 2026-04-24T15:10:19Z

+        param :ca_proxy, :bool, required: true, desc: N_('True, if Puppet CA proxy should be changed instead of the Puppet proxy')
+        def change_puppet_proxy
+          error_hosts = ::BulkHostsManager.new(hosts: @hosts).change_puppet_proxy(@proxy, params[:ca_proxy])
+          process_bulk_puppet_proxy_response(


params[:ca_proxy] is used directly as a boolean. If a client sends ca_proxy=false as a string (common in form-encoded requests), it's truthy and will incorrectly update the CA proxy. Cast it explicitly (e.g., [true, 'true'].include?(params[:ca_proxy])) before passing it into change_puppet_proxy (same applies in remove_puppet_proxy).

Foreman has a helper for string->bool casting

Assisted by Codex <codex@openai.com>

pr-processor Bot added the Not yet reviewed label Sep 24, 2025

adamruzicka reviewed Sep 24, 2025

View reviewed changes

pr-processor Bot removed the Not yet reviewed label Sep 24, 2025

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch from e2b1c7f to c7dd83c Compare September 24, 2025 12:15

nadjaheitmann commented Sep 24, 2025

View reviewed changes

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch 2 times, most recently from ac62ab7 to e980546 Compare September 24, 2025 12:48

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch 3 times, most recently from 11c8919 to 4512e86 Compare October 13, 2025 07:52

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch from 4512e86 to 2eb2f1a Compare January 21, 2026 17:09

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch 3 times, most recently from 4bb7ef6 to 0faf7d6 Compare February 10, 2026 10:44

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch 3 times, most recently from 75e0f00 to 7d7c810 Compare February 18, 2026 13:40

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch from d68caaf to 0caaa68 Compare March 2, 2026 09:21

jeremylenz reviewed Mar 2, 2026

View reviewed changes

Comment thread webpack/src/Extends/Hosts/ActionsBar/index.js Outdated

Comment thread webpack/src/Extends/Hosts/ActionsBar/index.js Outdated

Comment thread webpack/src/Extends/Hosts/ActionsBar/index.js Outdated

Comment thread webpack/src/Extends/Hosts/ActionsBar/index.js Outdated

nadjaheitmann mentioned this pull request Mar 2, 2026

Fixes #39047 - replace useForemanModal in BulkActions Katello/katello#11626

Merged

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch 9 times, most recently from 98fdd79 to dfb9239 Compare April 10, 2026 12:35

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch from dfb9239 to 8bc4dc2 Compare April 10, 2026 13:32

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch 7 times, most recently from 9208a4c to d6fd7f8 Compare April 24, 2026 12:39

jeremylenz approved these changes Apr 24, 2026

View reviewed changes

adamruzicka requested changes Apr 24, 2026

View reviewed changes

pr-processor Bot added the Waiting on contributor label Apr 24, 2026

adamruzicka requested a review from Copilot April 24, 2026 15:03

Copilot started reviewing on behalf of adamruzicka April 24, 2026 15:03 View session

Copilot AI reviewed Apr 24, 2026

View reviewed changes

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

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch 2 times, most recently from 70bc32f to 1268415 Compare April 27, 2026 11:09

Add Puppet proxy (CA) bulk actions

7db72d3

Assisted by Codex <codex@openai.com>

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch from 1268415 to 7db72d3 Compare April 27, 2026 12:06

Conversation

nadjaheitmann commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamruzicka left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

nadjaheitmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jeremylenz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nadjaheitmann commented Mar 9, 2026

Uh oh!

nadjaheitmann commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

nadjaheitmann Apr 27, 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!

Uh oh!

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nadjaheitmann commented Sep 24, 2025 •

edited

Loading

nadjaheitmann commented Apr 24, 2026 •

edited

Loading

nadjaheitmann Apr 27, 2026 •

edited

Loading