Skip to content
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.

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

Add support for RedHat 9 #51

Merged
merged 7 commits into from
Apr 21, 2023
Merged

Add support for RedHat 9 #51

merged 7 commits into from
Apr 21, 2023

Conversation

kibahop
Copy link
Member

@kibahop kibahop commented Jan 20, 2023

This adds so far minimally tested support for RedHat 9. The most important change in IPA version 4.10, that is atm in RedHat 9, is that mod_nss is replaced with with mod_ssl for the WebUI. Installer will also clash if UID_MAX and GID_MAX in login.defs overlap with idstart. There is a new parameters to handle that 'adjust_login_defs'. By default this is false.

@mattock
Copy link
Member

mattock commented Jan 23, 2023

The GitHub Action failures are false positives. I added some commits on top that fix a few genuine issues, but the rest is just GHA runtime environment issues. The first and possibly the last is this one:

# IPA switched to mod_ssl as the crypto engine for Apache as of version 4.7.0
# see https://www.freeipa.org/page/Releases/4.7.0#Highlights_in_4.7.0
if versioncmp($facts['ipa_server_version'], '4.7.0') < 0 {
exec { 'semanage-port-http_port_t':
Copy link
Member

Choose a reason for hiding this comment

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

Does this whole block become unnecessary with FreeIPA > 4.7.0? Most of this does not look specific to a crypto library change.

;
'adjust uid max':
line => "UID_MAX\t11999",
match => '^UID_MAX\s*60000$',
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps the match should be more flexible, e.g. ^UID_MAX\s\d$**. That way if the value is not exactly 60000 file_line would not fail or worse, add additional lines because no match is was found.

Copy link
Member

Choose a reason for hiding this comment

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

Also, does this apply to both FreeIPA clients and servers?

;
'adjust gid max':
line => "GID_MAX\t11999",
match => '^GID_MAX\s*60000$',
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here as above.

# Note: SUB_* only affect user/group mapping in containers, so not of
# concern here
if $easy_ipa::adjust_login_defs {
if $easy_ipa::idstart < Integer($facts['uid_max']) {
Copy link
Member

Choose a reason for hiding this comment

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

I would just return an Integer from Ruby. Easier, cleaner and more intuitive.

if $easy_ipa::idstart < Integer($facts['uid_max']) {
$uid_max_value = $easy_ipa::idstart -1
}
if $easy_ipa::idstart < Integer($facts['gid_max']) {
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Facter.add(:gid_max) do
setcode do
lines = File.readlines("/etc/login.defs")
lines.find { |line| line.start_with?("GID_MAX") }.split("\s")[1].strip
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the line looks like this:

GID_MAX        50000

That is if there are multiple spaces between GID_MAX and the value? Will [1] resolve as " " (=a space)?

Also, what if there are multiple lines that start with GID_MAX? Normally that would not happen, but can't be ruled out. That would cause potentially hard to debug problems. Two options I can think of for handling that:

  • Error out with a reasonable error message ("ERROR: multiple GID_MAX lines found in /etc/login.defs")
  • Pick the first value (e.g. line.starts_with?("GID_MAX")[0]) and print an informal message ("WARNING: multiple GIT_MAX lines found in /etc/login.defs").

Copy link
Member

Choose a reason for hiding this comment

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

Based on testing this works as expected. The "find" method returns the first line that matches. The split("\s") considers multiple whitespace characters as one, so multiple spaces between GID_MAX and the value do not cause issues.

@gartemiev
Copy link

@mattock , can this be fixed regarding tests and merged?

@mattock
Copy link
Member

mattock commented Apr 16, 2023

@mattock , can this be fixed regarding tests and merged?

@kibahop needs to be pushed a bit.

@mattock
Copy link
Member

mattock commented Apr 17, 2023

I think this is mergeable once the commits have been squashed.

@gartemiev
Copy link

@mattock , could you please squash it? I don't have permissions for it:

remote: Permission to Puppet-Finland/puppet-ipa.git denied to gartemiev. fatal: unable to access 'https://github.com/Puppet-Finland/puppet-ipa.git/': The requested URL returned error: 403

To quash it you need to run:
git rebase -i HEAD~16

and then add squash to all commits except initial one.

@kibahop
Copy link
Member Author

kibahop commented Apr 20, 2023

Add support for RHEL9

@kibahop kibahop closed this Apr 20, 2023
@kibahop kibahop reopened this Apr 20, 2023
@kibahop kibahop force-pushed the support_rhel9 branch 3 times, most recently from 233cb0e to 753389b Compare April 20, 2023 08:28
@gartemiev
Copy link

@mattock , this is still an issue with merge - commits were squashed by @kibahop - can you please fix and merge it?

The old workflow was from voxpupuli and was not really PDK-compatible. It could
be made to work only by heavy modifications to the Gemfile, which made it
practically incompatible with "pdk update" and .sync.yml. So, use a
PDK-compatible workflow from puppetlabs-postgresql with trivial modifications.

Signed-off-by: Samuli Seppänen <[email protected]>
@mattock mattock merged commit 6a19ceb into master Apr 21, 2023
@gartemiev
Copy link

@mattock - thanks for fixing tests!!! Highly appreciate it!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants