Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update for bullseye debian-mail-overlay #37

Merged
merged 11 commits into from
Sep 19, 2021

Conversation

SaraSmiseth
Copy link
Member

Description

This PR is for the updated bullseye debian-mail-overlay which is here mailserver2/debian-mail-overlay#11. Once that is merged this PR will fix the failing tests and do other necessary changes.

I fixed a few tests that were broken, because logging messages changed. There are still 2 tests left that do not run successfully. I'm not sure why they fail at the moment.

The failing tests are:

@test "checking dovecot: piped ham message with sieve" {

@test "checking dovecot: piped spam message with sieve" {

Fixes

Type of change

  • Update base image
  • Fix broken tests

Status

  • Ready
  • In development
  • Hold

TODO List

  • Fix remaining failing Tests

How has this been tested?

I have updated my server to use an image based on the updated debian-mail-overlay for about 2 weeks.

Copy link
Collaborator

@sknight80 sknight80 left a comment

Choose a reason for hiding this comment

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

So far, looks good to me.

@SaraSmiseth SaraSmiseth changed the title [WIP] Prepare for bullseye debian-mail-overlay Update for bullseye debian-mail-overlay Aug 18, 2021
@SaraSmiseth SaraSmiseth marked this pull request as ready for review August 18, 2021 14:26
@SaraSmiseth
Copy link
Member Author

The 2 tests mentioned above still fail. I will look into them when I have time at the weekend.
It looks like Travis does not run.
I found an error message here: Could not authorize build request for mailserver2/mailserver.

@AndrewSav AndrewSav mentioned this pull request Aug 19, 2021
4 tasks
@AndrewSav
Copy link
Collaborator

It looks like Travis does not run.
I found an error message here: Could not authorize build request for mailserver2/mailserver.

I think I fixed this by selecting the free plan. However my build fails, due to EICAR, which I believe you already fixed?

Probably no worth fixing my PR here, so I will just close it, but feel free to pull the update of the travis link (unrelated) to your PR from mine as well.

@SaraSmiseth
Copy link
Member Author

The reason for both checking dovecot: piped ham message with sieve tests failing is that the dovecot logging for sieve has changed. If I run the container with DEBUG_MODE=true the messages that the tests are expecting are there. It seems that they were changed from INFO to DEBUG level.
I have to change the tests so that they check for the messages in a container that is started with DEBUG_MODE=true . I'm not exactly sure how. Maybe add a new container just for these 2 tests? I will have to check if that's possible and we don't run into problems having too much test containers running at the same time.

@AndrewSav
Copy link
Collaborator

Sorry, do not have anything useful to contribute, but everything you say makes sense to me. A separate container should work, but at the same time we already have quite a lot of them and they are quire heavy, so the concern about a new one is founded.

@SaraSmiseth
Copy link
Member Author

Tests are all working now. I added a new container for the sieve tests. The tests require about 1.7GB more ram now.

We have a new travis problem:

Owner mailserver2 user license exceeded.

I'll try to use GitHub Actions instead.

@SaraSmiseth
Copy link
Member Author

I have played around with github actions here and got it working.

The problem I had was that random rspamd tests failed because the runners do not have enough RAM. I got it working by splitting tests and running the new container and the tests for the new container after everything else. See this ugly commit.

I don't like that workaround. What do you think?

@AndrewSav
Copy link
Collaborator

@SaraSmiseth what worries you? If we do not have enough memory to run them all at the same time we have to run them in stages, and that's what you did. May be to get rid of the mental sense of ugliness we just need to accept that this is a multistage tests which require different set of containers running at each stage. Would that thinking help? Or is there anything else particularly ugly that does not sit well?

Add a target for each mailserver-test-container to Makefile.
This allows to run for example only ldap tests with `make ldap`.
Running only `make` will execute all tests.
The workflows will build the docker image and then run the tests.
For example: the workflow `.github/workflows/default.yml` will run
`make default` to run tests for the default container.
These workflows run only for pull requests into master.
@SaraSmiseth
Copy link
Member Author

SaraSmiseth commented Sep 11, 2021

May be to get rid of the mental sense of ugliness we just need to accept that this is a multistage tests which require different set of containers running at each stage. Would that thinking help?

Yes thanks.

I added a target for each mailserver-test-container to the Makefile.
This allows to run for example only ldap tests with make ldap.
Running only make will execute all tests.
I split the test.bats file into multiple bats-test-files.

I have also added a github actions workflow for each mailserver-test-container.
The workflows will build the docker image and then run the tests.
For example: the workflow .github/workflows/default.yml will run
make default to run tests for the default container.
These workflows run only for pull requests into master.

EDIT:
The workflows are not running here because they are not in master yet. I have tested them in my fork.

This should be all. Should we merge it?

@AndrewSav
Copy link
Collaborator

This should be all. Should we merge it?

How urgent do you feel? I can do my best to try and give at a spin next weekend. If you are fairly confident that this works, then by all means go ahead.

@AndrewSav
Copy link
Collaborator

Also can we then remove travis or something? (I'm referring to the "All check have failed" message) Because it does not look like it provides any value at this stage.

@SaraSmiseth
Copy link
Member Author

How urgent do you feel? I can do my best to try and give at a spin next weekend. If you are fairly confident that this works, then by all means go ahead.

It's not that urgent. Take your time.

Also can we then remove travis or something? (I'm referring to the "All check have failed" message) Because it does not look like it provides any value at this stage.

Yes I'll remove it.

@sknight80
Copy link
Collaborator

Sorry, I am a little bit late for the party. How can I help?

@AndrewSav
Copy link
Collaborator

@sknight80 by testing that the changes here are working fine for you. Thanks.

@AndrewSav
Copy link
Collaborator

I keep getting

✗ checking postfix: milter-reject - clamav virus found
   (from function `assert_success' in file test/test_helper/bats-assert/src/assert.bash, line 114,
    in test file test/default.bats, line 491)
     `assert_success' failed

   -- command failed --
   status : 1
   output :
   --

docker exec mailserver_default cat /var/log/mail.log | grep EIC yields:

2021-09-18T02:00:25.064261+00:00 mail clamd[1770]: instream(127.0.0.1@35030): Win.Test.EICAR_HDB-1(697ef5516dc83129c71542c60777da9c:790) FOUND
2021-09-18T02:00:25.330080+00:00 mail postfix/cleanup[2179]: 087FE810: milter-reject: END-OF-MESSAGE from localhost[127.0.0.1]: 5.7.1 clamav: virus found: "Win.Test.EICAR_HDB-1"; from=<virus@gmail.com> to=<john.doe@domain.tld> proto=SMTP helo=<mx.gmail.com>

Are they keeping changing virus found messages?

Copy link
Collaborator

@AndrewSav AndrewSav left a comment

Choose a reason for hiding this comment

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

There are three things:

  • Virus detection - not sure but I'm getting inconsistent "virus found" message, the virus name flip between what's currently in the code and what I commented with
  • On my machine a lot of things does not have time to start, so I needed additional delays
  • Ideally instead of sleep we should wait for the ready conditions but this is probably out of scope of this PR

Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
test/default.bats Outdated Show resolved Hide resolved
test/default.bats Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
@AndrewSav
Copy link
Collaborator

@SaraSmiseth what about sleeps? I think we should add them to make the test work on a bigger range of machines until such time someone has time to substitute them with proper waits for readiness?

@SaraSmiseth
Copy link
Member Author

I have added the sleeps and updated EICAR tests. I agree that the sleeps should be changed later.

Copy link
Collaborator

@AndrewSav AndrewSav left a comment

Choose a reason for hiding this comment

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

Ok, I think we are good. I also ran an instance of it in a lab for last couple of days it seems fine.

@AndrewSav
Copy link
Collaborator

@SaraSmiseth thank you for your work on this upgrade. A bit of unrelated question, I know you are not using rainloop, what mail client are you using with the mailserver?

@SaraSmiseth
Copy link
Member Author

@SaraSmiseth thank you for your work on this upgrade. A bit of unrelated question, I know you are not using rainloop, what mail client are you using with the mailserver?

Thank you for the review. I use thunderbird and no webmail at all.

@SaraSmiseth SaraSmiseth merged commit 639ec3b into mailserver2:master Sep 19, 2021
@SaraSmiseth SaraSmiseth deleted the fix_tests branch September 19, 2021 15:32
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.

Implement Security Update for Dovecot v2.3.12
3 participants