Can you add:

• integration test
• update documentation (README)

@tomav I tried but I can't seem to get the tests working locally.
I rebased onto the current master to get green tests then I used:

$ make build-no-cache
$ make generate-accounts run generate-accounts-after-run fixtures tests

Which failed in the first fixture command:

# Setup sieve & create filtering folder (INBOX/spam)
docker cp "`pwd`/test/config/sieve/dovecot.sieve" mail:/var/mail/localhost.localdomain/user1/.dovecot.sieve
Error: No such container:path: mail:/var/mail/localhost.localdomain/user1
make: *** [Makefile:294: fixtures] Error 1

EDIT

Now this is visible in the travis log as well. The strangest thing is: my added code did not even run yet at this point.

Restarted build t see if it happens again. But the issue was before the tests started.

@tomav Seems we got no luck. Could you explain the issue in more detail, maybe I can debug it locally?

Okay, I debugged and my code did run at this point. There was an error in the start-mailserver.sh, hence the maildirs were not created.

Maybe we could add a test that makes it more explicit that start-mailserver.sh failed before "unrelated" tests trigger?

Still the same in Travis.

The code ip addr show | grep -E 'eth[0-9]+' may fail miserably as not all network interfaces use the ethx naming convention. Two of my interfaces are called enp6s0 and enp0s31f6, for example. I would rather pass in the networks as a parameter if we want to override the current behavior.

Sorry for the sluggish response, I am currently on holiday. So to answer you directly:

@tomav

Still the same in Travis.

I was a bit unclear. My previous post was just a status report that I could confirm the issue locally. I had not pushed anything new at this point.

@erik-wramner

The code ip addr show | grep -E 'eth[0-9]+' may fail miserably as not all network interfaces use the ethx naming convention. Two of my interfaces are called enp6s0 and enp0s31f6

I just copied over what was already there for PERMIT_DOCKER=host. After some research I found this:

This [enpXsY] is known as Predictable Network Interface naming and is part of systemd, [...]
Basic idea is that [...] interface name depends on physical location of hardware [...]

What I gather from this: The enp... naming scheme is only applicable to a hardware interface managed by systemd in contrast to docker's virtual network devices. So I'd assume inside containers we always have ethX.

Nevertheless, I still find it brittle to parse for ethX: docker might decide to change the naming after all and ip addr might change its output layout.
Additionally, I'd much prefer to have a manual override to allow for adding external (non docker) networks as well.

However, in the problem context if this PR, which comes from a docker-compose use case, we cannot know the subnets of the docker networks unless we force the user to define them statically. If we require the user to name the networks via their in container adapter names, we just shift the problem of unstable names onto the user. In the worst case, a container restart changes interface names.

Do you have any preferred choice here @erik-wramner? I find the existing options to be lacking:

• host -> allows for only one network and suffers from the same problem
• network -> only works with the default bridge network and breaks down in docker-compose settings

If I could wish I would like to have the following interface:

1. User sets up container with a list of connected networks
2. User defines which of these networks should be permitted, using their docker names, additionally can define subnets by hand
3. the daemon in the container automagically resolves these network names to subnets

Unfortunately, I see no way to do step 3 within the container, unless it gets access to the docker daemon for querying.

@erik-wramner
Alternatively we could use: ip -o -4 addr show type veth | egrep -o '[0-9\.]+/[0-9]+'

This would restrict the output to one line per IPv4 virtual ethernet device and parse only subnets from the output.

This is totally independent from the naming of the interfaces and could nearly grant my wish from above: I suspect the devices to be in the same order as the network definitions so using indices we can define which devices/networks to use.

I don't think you can rely on the order of the networks. I haven't seen anything about a guaranteed order and if custom networks (created manually or by other components and referenced with names) are mixed with managed networks that are created with the containers the creation order may vary, which can affect the presented order. Apart from that it seems fine.

Perhaps (enabled by an option) we could permit all the connected networks using this approach, i.e. no need to use names or indexes that we can't control?

Hm reading docker/compose#4645 I gather that the order with docker run is predefined but docker-compose orders by network name alphabetically and in some cases randomly.

Perhaps (enabled by an option) we could permit all the connected networks using this approach

Yeah, that is the initial idea of this PR. If you are fine with that approach I will update the code to use the ip command mentioned above.

I'm fine with that as it is opt-in provided that we can get the tests working. You could try to rebase your branch from master, I have spent quite some time trying to get the tests stable. Perhaps that helps. I can have a look later as well.

Please note that the tests only work on Linux, I've tried to run them on Mac and Windows without luck. If you still can't run locally, try on a Linux host.

I rebased as you said and still got a failing test. However, I got the impression that the make clean did not clean up everything. Even after that git status shows changes in test/config that I never did myself. Here is the last failing test:

 ✗ checking opendkim: /etc/opendkim/KeyTable should contain 2 entries
   (from function `assert_output' in file test/test_helper/bats-assert/src/assert.bash, line 239,
    in test file test/tests.bats, line 551)
     `assert_output 2' failed
   
   -- output differs --
   expected : 2
   actual   : 5
   --

I'll look into it when I get time, most likely by the end of the week. I don't have access to my build environment now.

@martin-schulze-vireso the tests in Travis passed, did you change something?

I wonder what happens with the cleanup step where we delete the network if we haven't created the network (i.e. that or an earlier step failed). Will it succeed or do we need to use force or handle the error?

Plus ideally I would like to avoid hard-coding the IP range for the custom network, but I can live with that to keep things simple.

@erik-wramner

the tests in Travis passed, did you change something?

I did some changes but the tests were not correct until now.

I wonder what happens with the cleanup step where we delete the network if we haven't created the network

The cleanup is aborted at this point. I hit this problem several times during development.

Plus ideally I would like to avoid hard-coding the IP range for the custom network

I could add some code to read out the subnet from docker directly but it makes the tests more complex.

I want to give some feedback for the development process with this project:

1. having everything in one big test suite is very ill suited to fast feedback cycles on the development of a single test. Ideally we could have some mechanism that allows to only do setup, run and teardown of a single test?
2. the cleanup seems to be incomplete, there are still things left in the test/config folder after make clean which seem to trigger errors in following test runs if forgotten to remove (via git checkout)
3. the setup steps are guarded with many sleep directives which lengthen the tests considerably and are very device specific: a slower machine might not get enough sleep and faster ones wait without benefit. If these sleeps are there solely to guarantee availability we might want to replace them with health checks and timeouts.
4. as mentioned above: there seems to be no test that simply checks if the server started successfully, thus giving a host of unhelpful failing tests when the root cause should be more visible.

This is the time output for the command sudo make generate-accounts run generate-accounts-after-run fixtures tests on my system:

real    17m20,333s
user    0m29,976s
sys     0m14,789s

Together these shortcomings lead to a very long feedback loop when developing new tests. I'd be willing to work on some of those, should I open up separate tickets for each of them?

One minor thing; the new docker container mounts the test folder. I've changed the others to mount the test/test-files folder read only instead. You should have that from the rebase. Can you do the same, please?

As for the cleanup, we could either do something advanced or we can simply ignore errors. Try to run:

docker network rm ... || true
Hopefully that allows the rest of the cleanup to proceed.

I fully agree with your feedback, I'm also new to this project and have also focused on the tests. In addition to your points I don't like starting all docker containers and leaving them running. Ideally there should be a test suite per container or group of containers.

I think you could just use one ticket and then use multiple PRs for that. No need for extra admin!