Skip to content

Add proper support for opendkim v2.11+ by updating config file#2

Open
cgunther wants to merge 1 commit intomasterfrom
opendkim-2.11+
Open

Add proper support for opendkim v2.11+ by updating config file#2
cgunther wants to merge 1 commit intomasterfrom
opendkim-2.11+

Conversation

@cgunther
Copy link
Member

Support was previously provided by reverting the service file to an old
version to match the old config, however the proper fix would have been
updating the service to match the new config.

autorestart and sender_headers attributes were removed as there's no
reference to them in the new config file.

The cookbook currently updates the service file, essentially undoing the
previous "fix", though in the long run we probably shouldn't touch the
service file ourselves, instead just relying on whatever the package
installs.

This likely now requires Ubuntu 18.04 or newer as the new config file
may not be compatible with versions of opendkim shipped with earlier
versions of Ubuntu.

The new path for keyfiles seems to be /etc/dkimkeys/, however I left the
cookbook default of /etc/mail/ so we don't have to deal with potentially
moving keys for instances that already have them in the cookbook
default.

@jimryan
Copy link
Member

jimryan commented Apr 26, 2021

@cgunther Thank you so much for this. A couple of questions:

  1. Is there some way to deprecate attributes in Chef? For example, if a user were using the autorestart or sender_headers settings, what happens after these changes are merged and they update the cookbook? Do they see an error, or do the attributes just silently not do anything? IMO failure or an explicit deprecation notice would be preferred over silent failure/no-op.
  2. Could we grep the service file for something that was only present in our old overridden version to determine if we should replace or not? That'd help reduce the chances of the cookbook overwriting an even newer service file with a now stale version.
  3. Is there a Chef-way to express that the cookbook is only compatible with Ubuntu 18+ (or more specifically, require some minimum opendkim version)? 14 and 16 are still in ESM, so it's not necessarily reasonable to assume all users are on 18+

Support was previously provided by reverting the service file to an old
version to match the old config, however the proper fix would have been
updating the service to match the new config.

`autorestart` and `sender_headers` attributes were removed as there's no
reference to them in the new config file.

If the service file looks old (likely had our previous "fix" applied),
we'll update the service file to what the package likely would have
installed to ensure compatibility with the new config file.

This likely now requires Ubuntu 18.04 or newer as the new config file
may not be compatible with versions of opendkim shipped with earlier
versions of Ubuntu.

The new path for keyfiles seems to be /etc/dkimkeys/, however I left the
cookbook default of /etc/mail/ so we don't have to deal with potentially
moving keys for instances that already have them in the cookbook
default.
@cgunther
Copy link
Member Author

  1. I'm not aware of a way to deprecate attributes. They're essentially an open-ended hash as opposed to a finite set of possible keys, so any keys not referenced elsewhere will silently do nothing. The config seems to suggest they're both still supported, though I'd imagine AutoRestart is more-so handled by the service, not opendkim itself. My project used neither of these, so I can't quite test them, but it certainly raises the question of how much configuration is supported through the cookbook, as it's clearly a tiny subset already.

  2. I tweaked the service updating to check for a reference to the old EnvironmentFile key, hoping that's a strong sign of an old "fixed" service. If you've got an 18.04+ project that was previously "fixed" the old way, give this a try as my projects were already properly fixed via the prior iteration of this PR.

  3. I added the supports setting to the metadata file, but I'm not sure if this is actually enforced, or just descriptive.

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.

2 participants