Skip to content

Rspamd: add greylisting option & code refactoring#3206

Merged
georglauterbach merged 12 commits intomasterfrom
rspamd/greylisting
Apr 11, 2023
Merged

Rspamd: add greylisting option & code refactoring#3206
georglauterbach merged 12 commits intomasterfrom
rspamd/greylisting

Conversation

@georglauterbach
Copy link
Copy Markdown
Member

@georglauterbach georglauterbach commented Mar 28, 2023 •
edited
Loading

Description

  1. adds the option to easily enable greylisting
  2. refactors setup code (no change in functionality)
  3. changes order in which Redis & Rspamd are started - Redis now comes first (which makes more sense)

I highly advise reviewing the first commit separately! It is a pure refactoring.

Closes #3215

Type of change

  • New feature (non-breaking change which adds functionality)
  • Improvement (non-breaking change that does improve existing functionality)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (README.md or the documentation under docs/)
  • If necessary I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@georglauterbach
refactoring Rspamd setup
5501e85 
Stylisting changes:

1. Added some log messages, changed log level for some messages
2. Removed a superflous message
3. Streamlined "calling-convention", i.e. a function should _internally_
   check whether it needs to do setup (which was not the case for `__rspamd__setup_learning`)
4. Outsourced some functionality into separate functions (Redis & ClamAV
   setup)
5. Added a few comments
6. Changed the order in which functions appear in the file; the order
   now aligns with the other in which functions are called (helpers are
   at the top)
@georglauterbach
add functionality for enabling greylisting
ca49f06 
This should in the future replace Postgrey completely.
@georglauterbach
update docs
ebe074b
@georglauterbach
added simple test for greylisting
6733084
@georglauterbach
change order in which Rspamd & redis are started
1099f2a 
Redis should be started before Rspamd. I have no clue why I used another
order in the first place.
@georglauterbach georglauterbach added kind/new feature A new feature is requested in this issue or implemeted with this PR area/features kind/improvement Improve an existing feature, configuration file or the documentation service/security/rspamd labels Mar 28, 2023
@georglauterbach georglauterbach added this to the v12.1.0 milestone Mar 28, 2023
@georglauterbach georglauterbach self-assigned this Mar 28, 2023
@georglauterbach georglauterbach added the meta/feature freeze On hold due to upcoming release process label Mar 28, 2023
georglauterbach
georglauterbach commented Mar 28, 2023
View reviewed changes
Comment thread mailserver.env
@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Mar 28, 2023

Just FYI: Rspamd 3.5 landed on 17 Mar, so v12.0.0 will include the (currently) latest version of Rspamd 🥳🎉

This PR is intended for v12.1.0 though :)

georglauterbach
georglauterbach commented Mar 29, 2023
View reviewed changes
Comment thread target/scripts/startup/setup.d/security/rspamd.sh Outdated
This was referenced Apr 3, 2023
Closed
Closed
@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Apr 10, 2023

This PR is ready for review and merge now :)

@georglauterbach georglauterbach removed the meta/feature freeze On hold due to upcoming release process label Apr 10, 2023
@polarathene
Copy link
Copy Markdown
Member

polarathene commented Apr 10, 2023

PR is ready for review

At a glance there is nothing alarming, but I've not got the time to do a thorough review as is usually the case with the Rspamd feature work especially.

I did notice some concerns while collaborating on related docs, and I'll raise an issue about that for you to address separately.

I can provide an approval, but it won't mean too much 😅

@casperklein do you want to provide a review, or should @georglauterbach go ahead and merge?

@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Apr 10, 2023 •
edited
Loading

@polarathene thanks for the info, much appreciated! Would you like to merge #3231 or this PR first? I tend towards merging #3231 first because the conflicts should be easier to resolve (if there are conflicts).

@polarathene
Copy link
Copy Markdown
Member

polarathene commented Apr 10, 2023

first because the conflicts should be easier to resolve (if there are conflicts).

There will be conflicts with rspamd docs page. I agree that #3231 should be merged first.

@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Apr 10, 2023 •
edited
Loading

first because the conflicts should be easier to resolve (if there are conflicts).

There will be conflicts with rspamd docs page. I agree that #3231 should be merged first.

Go ahead and merge #3231 when you think the PR is good to go :)

@georglauterbach georglauterbach removed the request for review from jrpear April 10, 2023 11:23
@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Apr 10, 2023

I marked this with priority/high now since it's blocking another PR I want to bring out :) I also resolved merge conflicts.

casperklein
casperklein reviewed Apr 10, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@casperklein casperklein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just two minor things.

Comment thread target/scripts/startup/setup.d/security/rspamd.sh Outdated
Comment thread target/scripts/start-mailserver.sh
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 11, 2023

Documentation preview for this PR is ready! 🎉

Built with commit: e22688f

@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Apr 11, 2023

LGTM. Just two minor things.

❤️ I adjusted the PR according to the PR feedback and I'll be mergin this then :)

@georglauterbach georglauterbach merged commit 806d3ef into master Apr 11, 2023
@georglauterbach georglauterbach deleted the rspamd/greylisting branch April 11, 2023 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@casperklein casperklein casperklein left review comments

@polarathene polarathene Awaiting requested review from polarathene

Assignees

@georglauterbach georglauterbach

Labels

area/features kind/improvement Improve an existing feature, configuration file or the documentation kind/new feature A new feature is requested in this issue or implemeted with this PR priority/high service/security/rspamd

Projects

Rspamd Integration
Archived in project

Milestone

v12.1.0

Development

Successfully merging this pull request may close these issues.

[FR] implement intelligent grey listing

3 participants

@georglauterbach @polarathene @casperklein