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 installing and managing radsniff #157

Merged
merged 4 commits into from
May 25, 2021

Conversation

nward
Copy link
Collaborator

@nward nward commented May 12, 2021

This is quite lightweight - it installs a systemd service, and a sysconfig options file.

All configuration is passed in an "options" string, allowing the user to use it however they want.

In a future version we might want to consider exposing all the radsniff command line options back in to puppet, but I don't think we need to do that right away.

This currently only works on Redhat/Centos - though that should be easy to extend.. I just don't have debian etc. machines running FR so not sure exactly what's needed there.

I've been using this patch in my local copy of the module and it works fine.

Additional module requirement is camptocamp/systemd.
Change outside the scope of radsniff is allowing all users to read the dictionary files.

@nward nward requested a review from djjudas21 May 12, 2021 22:45
@nward nward added this to the Version 4 milestone May 12, 2021
djjudas21
djjudas21 previously approved these changes May 14, 2021
@amateo
Copy link
Collaborator

amateo commented May 14, 2021

I have ubuntu servers, so I can test it... I'm going to try it

@amateo
Copy link
Collaborator

amateo commented May 14, 2021

I think that i could run in Ubuntu systems with a few changes:

  • Instead of hardcoding pidfile with /var/run/radiusd/radsniff.pid you could use /var/run/${fr_service}/radsniff.pid
  • /etc/sysconfig/radsniff should be /etc/default/radsniff in Ubuntu

@nward
Copy link
Collaborator Author

nward commented May 14, 2021

How does that look @amateo ?

I've added support for looking at the freeradius params file, and have added optional 'envfile' and 'pidfile' options to extend support to more OSes or configurations.

@amateo
Copy link
Collaborator

amateo commented May 14, 2021

I looks ok

@amateo
Copy link
Collaborator

amateo commented May 14, 2021

Sorry... I think I have found some bugs, I'll send you a patch as soon as I get it.

@amateo
Copy link
Collaborator

amateo commented May 14, 2021

Just need this change:

diff --git a/manifests/radsniff.pp b/manifests/radsniff.pp
index 2183706..699707c 100644
--- a/manifests/radsniff.pp
+++ b/manifests/radsniff.pp
@@ -36,7 +36,7 @@ class freeradius::radsniff (
 
   $escaped_cmd = $options.regsubst('"','\\\\"','G')
 
-  file { $envfile:
+  file { $final_envfile:
     content => @("SYSCONFIG"),
       RADSNIFF_OPTIONS="${escaped_cmd}"
       | SYSCONFIG

@nward
Copy link
Collaborator Author

nward commented May 14, 2021

Thanks, good catch. Let me see if I can figure out why my unit test didn't catch that, and then update it to catch it and include your fix.

@nward nward force-pushed the radsniff branch 2 times, most recently from 73c847f to 687f70b Compare May 14, 2021 11:52
@nward
Copy link
Collaborator Author

nward commented May 14, 2021

OK. I really botched that up. I had named the spec file incorrectly - it has to have _spec.rb in the name, or it isn't included in the testing when you run pdk test unit, but it is included in pdk validate, and I had fixed a couple of validation errors there so thought it was being tested... I was sitting here thinking "wow, tests passed first time, doing great!". I was very, very wrong.

So, anyway. I've pushed a new commit - I'll squash all of the changes on this branch together before we actually merge this, but I have left it un-squashed so you could see the changes.

I'm not super happy with how all the tests work here with params etc. - If we do #162 then this can be significantly simpler, as everything comes in to the tests as parameters on the class under test.

@nward nward merged commit 7338d28 into djjudas21:master May 25, 2021
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