nward
commented
1 year ago Hi @deligatedgeek thanks for the contribution
I have reviewed it, and this code will need some adjustment - as you note, it works in your environment, but it will break some environments, and I think we need to consider the overall structure (using a define). I have not used FreeRADIUS' trigger mechanisms personally, so am not sure the best way forward.
Some quick notes on the code as written:
- You have used a
define, which can be called multiple times, but the file that is created would not be unique if it was called multiple times so the manifest would fail to compile radiusd.confis updated to includetrigger.confregardless of whether the config file exists- Only a couple of config params are passed - I think we need to include some more configurable items here
- If
ensureispresentthencmdis set to snmptrap - if it is absent, then it is set to echo - but the file resource would not be created because it also usesensure. I think this conditional should be removed, and instead there should simply be asnmp_trap_cmdparameter on the define (or class - see below).
Re the overall structure - the way this is structured really depends on whether you can have multiple instances of triggers defined - or can you only ever have one?
If you can only have one set of trigger configuration, then I think this makes sense to change this to be a class, or maybe just have it in the main freeradius class - which is where most config files like this are defined.
Alternatively, if you can have multiple trigger.conf files, or if you can have areas within trigger.conf repeated, then it makes sense to either have a unique file name or use a concat.
deligatedgeek
This includes the trigger.conf and allows configuration of trap destination and community string.
I think installing snmp OS packages, and the Freeradius bibs, which are installed by RHEL derivatives but not Debian, should be handled by module users, as instructed by Freeradius.
Thinking in the future to add a hash which would enable a set of traps