Snawoot / postfix-mta-sts-resolver

Daemon which provides TLS client policy for Postfix via socketmap, according to domain MTA-STS policy
MIT License
117 stars 23 forks source link

Add support for redis-sentinel #95

Closed acteru closed 1 year ago

acteru commented 1 year ago

Purpose of proposed changes

Closes #94

Essential steps taken

acteru commented 1 year ago

Does anyone have a suggestion how it would be possible to test this automatically? I don't really have much experience with testing, but would be willing to work my way into it.

Snawoot commented 1 year ago

Hello, @acteru!

Sorry for late response.

IIRC similar module for redis is tested by e2e tests with actual redis, so in this case we will need actual sentinel cluster in test env. It's doable, but quite laborious and not a priority because feature is quite isolated from other code and can be tested at least manually. Anyway, tests can be added later: feature with no tests is better than absence of feature.

About things that forgotten and possible corner cases. I'm not sure application will handle properly election of new master server by sentinel. Code stores connection pool for master once and very likely it will not be valid if master was switched to another server (e.g. if old server just become unavailable). Unfortunately, I don't know how to approach this properly: store once as it is already done, or evaluate before each interaction with redis or reevaluate it once in a while (regularly or using some caching decorator with small time to live). I haven't much of experience with sentinel and python redis driver. If possible please check how it behaves after master switch. I have a feeling it can be improved in follow up pull requests.

All in all, great job! Thank you for the contribution!

Snawoot commented 1 year ago

@acteru + if you don't mind, would be nice to update man page (man/mta-sts-daemon.yml.5.adoc) to advertise new kind of supported cache.

acteru commented 1 year ago

@Snawoot What I was able to find out so far is that the sentinel client supports a reconnect. I get an error that the current master cannot be found, but after the master selection everything works again. For testing I did a manual failover, as long as the master is not available I get the following error and the stacktrace 2023-02-13 10:46:22 ERROR STS: Cache set failed: No master found for 'mymaster' but the daemon keeps running, at least started over cli. Then after the failover i get: 2023-02-13 11:37:05 ERROR STS: Cache get failed: Connection closed by server. After checking with a few keys, everything still seems to be working correctly. Perhaps it would be necessary to handle the exceptions a bit nicer, but on the whole everything seems to be working

Snawoot commented 1 year ago

@acteru I think it's already good enough. Will make a new release as soon as I can.

Snawoot commented 1 year ago

Released in v1.2.0