144 rocky8

[minorOffense]

See #144

[djjudas21]

Thanks for your contribution. This passes tests and looks good to me - I assume it's working in your environment @minorOffense? I'm not currently using FreeRADIUS at the moment so I don't have a test platform.

[minorOffense]

The service started with that in place yes on Rocky 8. I didn't test CentOS Stream or RHEL8.

[djjudas21]

Thanks @minorOffense

@nward or @amateo are either of you guys on CentOS/RHEL? I guess they're highly likely to be the same as Rocky 8, but you never know... 😄

[nward]

I have an open question on #144:

Does FreeRADIUS run in the default configuration without a certificate? Certificates are of course not required in all RADIUS use cases.

@minorOffense Are you in a position to answer this? I am concerned that it could stop FreeRADIUS's default configuration starting if certs are never generated?
I wonder if we should generate certs if none are provided - i.e. rather than relying on bootstrap running and us then purging the certs dir.

[nward]

I note that EL9 doesn't run bootstrap:

* Wed Mar 10 2021 Robbie Harwood <rharwood@redhat.com> - 3.0.21-11
- Disable automatic bootstrap

[minorOffense]

If we know the certs that bootstrap generate I’d add a series of file definitions to ensure present on those files. So the service doesn’t break on install.

That’s what we did in our internal puppet code. An exec on bootstrap then register the files it generates.

[nward]

Here's my thoughts, this looks good so far, nice work! I have a few tweaks suggested.

Do you want to have a stab at some tests for this? It would go in spec/classes/freeradius_spec.rb, and check out spec/classes/radsniff_spec.rb for some tests which run differently with different facts (osfamily in that case).

[nward]

Let's rename this to be unique and make the filename not RHEL8 specific, as this impacts any EL8 and maybe other distros too, perhaps something like:

Suggested change

	systemd::dropin_file { 'remove_bootstrap.conf':
	ensure => present,
	unit => 'radiusd.service', #@todo programmatically determine the service name
	content => template('freeradius/systemd_dropin_rhel8.erb'),
	}
	systemd::dropin_file { 'freeradius remove bootstrap':
	filename => 'remove_bootstrap.conf',
	ensure => present,
	unit => 'radiusd.service', #@todo programmatically determine the service name
	content => template('freeradius/systemd_dropin_remove_bootstrap.erb'),
	}

[nward]

Also - do you want to turn this todo in to an issue, I don't think we can solve that in this PR

[nward]

As a style thing, can we add a newline at the end of this file and potentially update the filename

[nward]

I think we need to be >=1.1.1 in order to get systemd::dropin_file with the params we're using (filename which I've suggested above is introduced in 1.1.1, systemd::dropin_file without it is in 1.0.0). I don't know if we want to test all the way back there, but I think if we can support older we should, as some users will be on older versions of puppet/systemd

[nward]

Also note that camptocamp/systemd is currently referenced here and in .fixtures.yml, so we would need to remove/update those references and ensure this is compatible (from memory, camptocamp/systemd became puppet/systemd?)

[nward]

@nward or @amateo are either of you guys on CentOS/RHEL? I guess they're highly likely to be the same as Rocky 8, but you never know... 😄

I actually do have some real world licensed RHEL8 boxes, but not running FreeRADIUS :( I am happy with this being tested on Rocky8 - I'm all over those RPMs and they're all the same in this regard!

[jnk0]

We would appreciate to see this PR merged. Already tested it on a couple of Rocky Linux 8 boxes.

[djjudas21]

Consensus seems to be that this works so it should be safe. @nward, you left some review comments last time you looked at it. Are you satisfied that they've been addressed?

[nward]

Consensus seems to be that this works so it should be safe. @nward, you left some review comments last time you looked at it. Are you satisfied that they've been addressed?

They don't appear to have been addressed.

I have some day job work doing FreeRADIUS 3.2 on EL9 with Puppet over the next 2 weeks, so I'll tidy it up and do any further EL9 and FR3.2 changes, if there's no objection.

[nward]

I've made changes to resolve these issues here: https://github.com/djjudas21/puppet-freeradius/tree/144-rocky8

It looks like @minorOffense didn't allow maintainers to push to their branch - so I think the best approach is to close this, and create a new PR?

[djjudas21]

Keen to get this merged - @nward and @minorOffense, please could you collaborate on this and hopefully we can get this merged and a new release tagged

[djjudas21]

I've created a new PR #194 which contains all of these commits plus @nward's fixes. Closing this in favour of #194