Fixes #36940 - Add necessary migration for host_config tftp directory #949

sbernhard · 2024-05-31T10:25:51Z

The migration is necessary as part of the puppet_foreman_proxy change in theforeman/puppet-foreman_proxy#821

Belongs to the SecureBoot story: theforeman/foreman#9864

ehelms · 2024-05-31T13:02:36Z

A migration spec for this would be great to have. Example: https://github.com/theforeman/foreman-installer/blob/develop/spec/migrations/20240328125552_reset_puppetserver_ciphers_spec.rb

goarsna · 2024-07-16T21:16:39Z

FYI: @nofaralfasi and I had a discussion last week that resulted in some changes we want to implement on the Smart Proxy side which are summarized in the Smart Proxy PR. These changes also will affect this PR. Further updates will follow soonish / in the upcoming weeks (as soon as I have time to work on this again).

goarsna · 2024-08-05T13:42:09Z

One outcome of the above mentioned discussion was that the Bootloader Universe should not be configurable. Instead it will be a fixed folder inside the TFTP root that is created automatically during installation / upgrade. The other agreement was, that the Host Config directory will be written with a dash. Both have now been applied to this PR.

nofaralfasi · 2024-10-28T13:02:42Z

/packit build

packit-as-a-service · 2024-10-28T13:02:48Z

Account nofaralfasi has no write access nor is author of PR!

ehelms · 2024-10-28T13:07:47Z

/packit build

ehelms · 2024-10-28T13:09:08Z

I think this will need a rebase to pass tests.

ekohl

Why are there multiple migrations, one adding host-config and another adding bootloader-universe?

goarsna · 2024-11-07T07:50:52Z

Why are there multiple migrations, one adding host-config and another adding bootloader-universe?

There is no specific reason for that despite that the directories are independent of each other from a file system POV and that at the point I added the migration for the bootloader-universe directory to the migrations it looked cleaner to me to add it in a separate migration. But on the other hand the directories belong to each other as the one does not make sense without the other.

My current POV: We sould merge the migrations into one.

What do you think about it @nofaralfasi?

nofaralfasi · 2024-11-07T09:28:21Z

My current POV: We sould merge the migrations into one.

What do you think about it @nofaralfasi?

I also think that we should merge them into one.

nofaralfasi · 2024-11-11T08:36:12Z

Hi, when testing this PR, we're getting the following error: /usr/share/gems/gems/kafo-7.5.1/lib/kafo/puppet_command.rb:53:in format_command': wrong number of arguments (given 2, expected 1) (ArgumentError).
Any idea why this is happening?

goarsna · 2024-11-12T10:44:57Z

Hi, when testing this PR, we're getting the following error: /usr/share/gems/gems/kafo-7.5.1/lib/kafo/puppet_command.rb:53:in format_command': wrong number of arguments (given 2, expected 1) (ArgumentError). Any idea why this is happening?

Hey Nofar, that is strange... I do not encounter this error (or any other errors) on my test system I just rolled out. But I have to mention that my test setup is based on Foreman 3.11 with the changes from the Secure Boot PRs applied on top.
When exactly does the error occur? Could you share the complete log output?

nofaralfasi · 2024-11-12T10:49:29Z

Hey Nofar, that is strange... I do not encounter this error (or any other errors) on my test system I just rolled out. But I have to mention that my test setup is based on Foreman 3.11 with the changes from the Secure Boot PRs applied on top. When exactly does the error occur? Could you share the complete log output?

When we run the foreman-installer command:

# foreman-installer
/usr/share/gems/gems/kafo-7.5.1/lib/kafo/puppet_command.rb:53:in `format_command': wrong number of arguments (given 2, expected 1) (ArgumentError)
	from /usr/share/foreman-installer/hooks/boot/01-kafo-hook-extensions.rb:132:in `execute_command'
	from /usr/share/foreman-installer/hooks/boot/01-kafo-hook-extensions.rb:124:in `execute'
	from /usr/share/foreman-installer/hooks/boot/03-foreman-maintain-extensions.rb:4:in `foreman_maintain'
	from /usr/share/foreman-installer/hooks/boot/09-version_locking.rb:2:in `package_lock_feature?'
	from /usr/share/foreman-installer/hooks/boot/09-version_locking.rb:5:in `block (4 levels) in load'
	from /usr/share/gems/gems/kafo-7.5.1/lib/kafo/hooking.rb:36:in `instance_eval'
	from /usr/share/gems/gems/kafo-7.5.1/lib/kafo/hooking.rb:36:in `block (4 levels) in load'
	from /usr/share/gems/gems/kafo-7.5.1/lib/kafo/hook_context.rb:19:in `instance_eval'
	from /usr/share/gems/gems/kafo-7.5.1/lib/kafo/hook_context.rb:19:in `execute'
	from /usr/share/gems/gems/kafo-7.5.1/lib/kafo/hooking.rb:67:in `block in execute'
	from /usr/share/gems/gems/kafo-7.5.1/lib/kafo/hooking.rb:65:in `each'
	from /usr/share/gems/gems/kafo-7.5.1/lib/kafo/hooking.rb:65:in `execute'
	from /usr/share/gems/gems/kafo-7.5.1/lib/kafo/kafo_configure.rb:144:in `initialize'
	from /usr/share/gems/gems/clamp-1.3.2/lib/clamp/command.rb:140:in `new'
	from /usr/share/gems/gems/clamp-1.3.2/lib/clamp/command.rb:140:in `run'
	from /usr/share/gems/gems/kafo-7.5.1/lib/kafo/kafo_configure.rb:54:in `run'
	from /usr/sbin/foreman-installer:8:in `<main>'

goarsna · 2024-11-12T11:42:13Z

Thanks for the log! I assume the error is not related to the changes in the PR. But I do not know what is causing the error... :/

goarsna · 2024-11-12T13:44:25Z

@nofar Rebased as discussed in DM. Maybe this solves the errors you encounter :)

goarsna · 2024-11-12T14:31:38Z

I squashed the migrations as discussed :)

stejskalleos · 2024-11-19T12:36:13Z

theforeman/smart-proxy#877 and theforeman/foreman#9864 have been merged

ehelms

Good to go once we merge theforeman/puppet-foreman_proxy#821

nofaralfasi · 2024-11-28T10:03:34Z

theforeman/puppet-foreman_proxy#821 has been merged. Could we have this included as well?

ekohl · 2024-11-28T13:26:19Z

config/foreman-proxy-content.migrations/240531091300_foreman_proxy_tftp_host_config.rb

@@ -0,0 +1,10 @@
+if answers['foreman_proxy']


I always like to think about the data types and this is a Variant[Boolean, Hash]. In other words, this can be true as well and cause the later code to crash. true means use all defaults and you don't need a migration. That's why most migrations use .is_a?(Hash)

Suggested change

if answers['foreman_proxy']

if answers['foreman_proxy'].is_a?(Hash)

ekohl · 2024-11-28T13:28:37Z

config/foreman-proxy-content.migrations/240531091300_foreman_proxy_tftp_host_config.rb

+  root = answers['foreman_proxy']['tftp_root']
+  if answers['foreman_proxy']['tftp_dirs']


This can fail because the data type of tftp_root is Optional[Stdlib::Absolutepath] (https://github.com/theforeman/puppet-foreman_proxy/blob/b73685cebc4efad1c23844843c0f98e1e6454138/manifests/init.pp#L340). It's usually set and I think it's safe enough to not migrate if it's not set.

Suggested change

root = answers['foreman_proxy']['tftp_root']

if answers['foreman_proxy']['tftp_dirs']

root = answers['foreman_proxy']['tftp_root']

if root && answers['foreman_proxy']['tftp_dirs']

And done. Thanks @ekohl!

ekohl

I didn't explicitly say it, but please keep all migrations the same.

goarsna · 2024-12-02T09:32:45Z

🙈 Is there a face palm emoji on GitHub? 😅 Fixed it

goarsna · 2024-12-02T15:22:22Z

Thanks a lot @ekohl for merging this! And thanks to everyone who was involved in getting this done 🎉

pr-processor bot added the Not yet reviewed label May 31, 2024

sbernhard force-pushed the fix_36940 branch 2 times, most recently from c3e3495 to f115512 Compare May 31, 2024 14:14

goarsna force-pushed the fix_36940 branch from f115512 to f1b99e6 Compare August 5, 2024 13:24

goarsna force-pushed the fix_36940 branch from f1b99e6 to 13e1bfb Compare August 8, 2024 11:35

ekohl reviewed Oct 28, 2024

View reviewed changes

pr-processor bot removed the Not yet reviewed label Oct 28, 2024

This was referenced Nov 12, 2024

Fixes #36833 - Add SecureBoot support for arbitrary operating systems to "Grub2 UEFI" PXE loaders theforeman/smart-proxy#877

Merged

Fixes #36834 - Add SecureBoot support for arbitrary operating systems to "Grub2 UEFI" PXE loaders theforeman/foreman#9864

Merged

goarsna force-pushed the fix_36940 branch from 13e1bfb to e0d91c2 Compare November 12, 2024 13:38

goarsna force-pushed the fix_36940 branch from e0d91c2 to bb192c7 Compare November 12, 2024 14:30

nofaralfasi approved these changes Nov 17, 2024

View reviewed changes

ehelms approved these changes Nov 21, 2024

View reviewed changes

ekohl requested changes Nov 28, 2024

View reviewed changes

pr-processor bot added the Waiting on contributor label Nov 28, 2024

goarsna force-pushed the fix_36940 branch from bb192c7 to 562729a Compare November 28, 2024 14:31

pr-processor bot added Needs re-review and removed Waiting on contributor Needs re-review labels Nov 28, 2024

ekohl requested changes Nov 28, 2024

View reviewed changes

pr-processor bot added the Waiting on contributor label Nov 28, 2024

Fixes #36940 - Add necessary migration for host_config tftp directory

46d8d90

goarsna force-pushed the fix_36940 branch from 562729a to 46d8d90 Compare December 2, 2024 09:24

pr-processor bot added Needs re-review and removed Waiting on contributor labels Dec 2, 2024

ekohl approved these changes Dec 2, 2024

View reviewed changes

pr-processor bot removed the Needs re-review label Dec 2, 2024

ekohl merged commit 57e5af7 into theforeman:develop Dec 2, 2024
7 of 8 checks passed

goarsna deleted the fix_36940 branch December 2, 2024 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #36940 - Add necessary migration for host_config tftp directory #949

Fixes #36940 - Add necessary migration for host_config tftp directory #949

sbernhard commented May 31, 2024 •

edited

Loading

ehelms commented May 31, 2024

goarsna commented Jul 16, 2024

goarsna commented Aug 5, 2024

nofaralfasi commented Oct 28, 2024

packit-as-a-service bot commented Oct 28, 2024

ehelms commented Oct 28, 2024

ehelms commented Oct 28, 2024

ekohl left a comment

goarsna commented Nov 7, 2024

nofaralfasi commented Nov 7, 2024

nofaralfasi commented Nov 11, 2024

goarsna commented Nov 12, 2024

nofaralfasi commented Nov 12, 2024

goarsna commented Nov 12, 2024

goarsna commented Nov 12, 2024 •

edited

Loading

goarsna commented Nov 12, 2024

stejskalleos commented Nov 19, 2024

ehelms left a comment

nofaralfasi commented Nov 28, 2024

ekohl Nov 28, 2024

goarsna Nov 28, 2024

ekohl Nov 28, 2024

goarsna Nov 28, 2024

ekohl left a comment

goarsna commented Dec 2, 2024

goarsna commented Dec 2, 2024

	if answers['foreman_proxy']
	if answers['foreman_proxy'].is_a?(Hash)

		root = answers['foreman_proxy']['tftp_root']
		if answers['foreman_proxy']['tftp_dirs']

Fixes #36940 - Add necessary migration for host_config tftp directory #949

Fixes #36940 - Add necessary migration for host_config tftp directory #949

Conversation

sbernhard commented May 31, 2024 • edited Loading

ehelms commented May 31, 2024

goarsna commented Jul 16, 2024

goarsna commented Aug 5, 2024

nofaralfasi commented Oct 28, 2024

packit-as-a-service bot commented Oct 28, 2024

ehelms commented Oct 28, 2024

ehelms commented Oct 28, 2024

ekohl left a comment

Choose a reason for hiding this comment

goarsna commented Nov 7, 2024

nofaralfasi commented Nov 7, 2024

nofaralfasi commented Nov 11, 2024

goarsna commented Nov 12, 2024

nofaralfasi commented Nov 12, 2024

goarsna commented Nov 12, 2024

goarsna commented Nov 12, 2024 • edited Loading

goarsna commented Nov 12, 2024

stejskalleos commented Nov 19, 2024

ehelms left a comment

Choose a reason for hiding this comment

nofaralfasi commented Nov 28, 2024

ekohl Nov 28, 2024

Choose a reason for hiding this comment

goarsna Nov 28, 2024

Choose a reason for hiding this comment

ekohl Nov 28, 2024

Choose a reason for hiding this comment

goarsna Nov 28, 2024

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

goarsna commented Dec 2, 2024

goarsna commented Dec 2, 2024

sbernhard commented May 31, 2024 •

edited

Loading

goarsna commented Nov 12, 2024 •

edited

Loading