ungleich-public/cdist-contrib
ungleich-public/cdist-contrib
8
2
Fork 3
Code Issues 2 Pull requests 3 Projects Releases Wiki Activity

[__opendkim*] add debian support #32

Open
fnux wants to merge 2 commits from opendkim-debian into master
pull from: opendkim-debian
merge into: ungleich-public:master
Conversation 0 Commits 2 Files changed 7 +47 -3
4 changed files with 47 additions and 3 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit d97fb9a434 - Show all commits

8
type/__opendkim/files/opendkim.conf.sh
View file

@ -3,6 +3,9 @@
echo "# Managed remotely, manual changes will be lost."
# Used for OS-specific configuration.
os=$(cat "${__global:?}/explorer/os")
evilham commented 2024-05-24 11:48:13 +00:00
Review
Copy link

It seems like we are not using this really, can we remove it?

It seems like we are not using this really, can we remove it?
# Optional chdir(2)
if [ "$BASEDIR" ];
then
@ -63,3 +66,8 @@ if [ "$USERID" ];
then
printf "UserID %s\n" "$USERID"
fi
if [ "$PIDFILE" ];
then
printf "PidFile %s\n" "$PIDFILE"
fi

7
type/__opendkim/man.rst
View file

@ -14,8 +14,8 @@ installation and basic configuration of an instance of OpenDKIM.
Note that this type does not generate or ensure that a key is present: use
`cdist-type__opendkim-genkey(7)` for that.
Note that this type is currently only implemented for Alpine Linux and FreeBSD.
Please contribute an implementation if you can.
Note that this type is currently only implemented for Debian, Alpine Linux and
evilham commented 2024-05-24 11:46:48 +00:00
Review
Copy link

Nit-pick: could we write things alphabetically here? :-D

Nit-pick: could we write things alphabetically here? :-D
FreeBSD. Please contribute an implementation if you can.
REQUIRED PARAMETERS
@ -45,6 +45,9 @@ custom-config
The string following this parameter is appended as-is in the configuration, to
enable more complex configurations.
pidfile
Specifies the path to a file that should be created at process start
containing the process ID.
BOOLEAN PARAMETERS
------------------

34
type/__opendkim/manifest
View file

@ -21,13 +21,20 @@
os=$(cat "${__global:?}/explorer/os")
CFG_DIR="/etc/opendkim"
CFG_FILE="$CFG_DIR/opendkim.conf"
service="opendkim"
case "$os" in
'alpine')
:
;;
'debian')
CFG_DIR="/etc/dkimkeys"
CFG_FILE="/etc/opendkim.conf"
;;
'freebsd')
CFG_DIR="/usr/local/etc/mail"
CFG_FILE="$CFG_DIR/opendkim.conf"
evilham commented 2024-05-24 12:22:57 +00:00
Review
Copy link

This seems redundant as it is the default.

This seems redundant as it is the default.
service="milter-opendkim"
;;
*)
@ -70,12 +77,37 @@ if [ -f "${__object:?}/parameter/userid" ]; then
export USERID
fi
if [ -f "${__object:?}/parameter/pidfile" ]; then
PIDFILE="$(cat "${__object:?}/parameter/pidfile")"
export PIDFILE
fi
# Debian: set configuration specific to debian packaging if no explicit value
# is requested.
if [ "$os" = "debian" ]; then
# In Debian, opendkim runs as user "opendkim". A umask of 007 is required when
# using a local socket with MTAs that access the socket as a non-privileged
# user (for example, Postfix). You may need to add user "postfix" to group
# "opendkim" in that case.
if [ -z "$USERID" ]; then
export USERID="opendkim"
fi
evilham commented 2024-05-24 12:15:20 +00:00
Review
Copy link

Given that USERID exists for the --userid parameter, which was deprecated, I think this should instead (or additionally) check for the UserID being defined in --extra-config, and if not, appending the corresponding line; similar to --processid.

At least in theory, removing support for --userid should keep things working.

As a general rule for all these special cases (--umask and --userid, for --pidfile see comment below): they should be mentioned in man.rst as special default values.

Given that `USERID` exists for the `--userid` parameter, which was deprecated, I think this should instead (or additionally) check for the `UserID` being defined in `--extra-config`, and if not, appending the corresponding line; similar to `--processid`. At least in theory, removing support for `--userid` should keep things working. As a general rule for all these special cases (`--umask` and `--userid`, for `--pidfile` see comment below): they should be mentioned in `man.rst` as special default values.
if [ -z "$UMASK" ]; then
export UMASK="007"
fi
if [ -z "$PIDFILE" ]; then
export PIDFILE="/run/opendkim/opendkim.pid"
fi
fi
# Boolean parameters
[ -f "${__object:?}/parameter/syslog" ] && export SYSLOG=yes
# Generate and deploy configuration file.
source_file="${__object:?}/files/opendkim.conf"
target_file="${CFG_DIR}/opendkim.conf"
target_file="${CFG_FILE}"
mkdir -p "${__object:?}/files"

1
type/__opendkim/parameter/optional
View file

@ -4,3 +4,4 @@ subdomains
umask
userid
custom-config
pidfile
evilham commented 2024-05-24 12:35:14 +00:00
Review
Copy link

Rather than add more parameters to the type (we don't want to support 100% of the .conf), I'd propose we rely on --custom-config instead.

We can check the contents of --custom-config and adapt that as necessary for e.g. Debian.

This keeps the type smaller and still just as flexible and out-of-the-box.

Rather than add more parameters to the type (we don't want to support 100% of the `.conf`), I'd propose we rely on `--custom-config` instead. We can check the contents of `--custom-config` and adapt that as necessary for e.g. Debian. This keeps the type smaller and still just as flexible and out-of-the-box.