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

Test qmail-inject recipient address qualification. #232

Open
wants to merge 4 commits into
base: schmonz-qmail-qfilter-osf
Choose a base branch
from

Conversation

schmonz
Copy link
Member

@schmonz schmonz commented Jan 20, 2022

Using qmail-qfilter (proposed in #227), turn the minimal reproducer from #147 into an automated regression test. It's an integration test, not a unit test, because it executes qmail-inject[*] the same way our systems would. Thanks to qmail-qfilter, the test does not involve a real queue in any way.

#229 probably fixes #147, and these tests will help us be confident of that.

[*] Or rather, tests/testable/qmail-inject, which is identical modulo linking with a custom definition of auto_qmail that points into the source tree

@DerDakon
Copy link
Member

Almost there ;)

I would very much prefer to not have to create a separate inject binary. Would it be possible to let the filter grep for the expected "To: " line and return success or failure? Then it should be possible to just qmail-inject and look at it's return code to see if the filter has accepted the mail or not. At least that would be the solution with the least possible side effects I can think of.

@schmonz
Copy link
Member Author

schmonz commented Jan 21, 2022

Yes, I'd feel much better if we could directly test the real qmail-inject we're about to install. Trouble is, before we've installed, qmail-inject hits die_chdir() -- and if we have installed notqmail already, /var/qmail is still the wrong place to be looking. So at least in this one case so far, we need a way to override the value of auto_qmail for testing.

But how? Linker tricks aren't portable enough. Relinking a fake auto_qmail into the real qmail-inject is a no-no (people need to be able to run the tests and then install). Maybe we introduce an environment variable for the purpose of overriding auto_qmail, being careful to name it something strange enough that no admin would happen to already have it set? Other options?

@DerDakon
Copy link
Member

I wonder why all tests pass if this should detect the regression?

@schmonz schmonz force-pushed the schmonz-inject-recipient-qualification-tests branch from 7bae68d to 351bf60 Compare January 22, 2022 22:37
@schmonz
Copy link
Member Author

schmonz commented Jan 22, 2022

I've made the failing test fail the build, so the checks should look uniformly bad now :-)

In an attempt to avoid

  • hand-duplicating an entire rule just so I can hand-change a tiny part of it
  • relying on non-portable linker tricks
  • introducing a test-motivated environment variable (and function lookup) into production code
  • using chroot or another workaround that would require running tests as root

... tests can depend on testable. integrationtest_inject depends on it, causing the top-level build to be copied into a subdirectory of tests where it can be messed with in unsurprising ways.

Copying seems wasteful, but for our autobuilds the time consumed is in the noise, and for local development we don't need to be running the integrationtest target most of the time, just the unittest target.

This approach seems weird but also obvious enough that we'll be unlikely to screw things up.

@schmonz
Copy link
Member Author

schmonz commented Jan 22, 2022

Also, the filter no longer drops a sentinel-file turd to indicate the bug. Instead it uses the customerror API from #195. Perfect fit :-)

tests/Makefile Outdated Show resolved Hide resolved
@schmonz schmonz force-pushed the schmonz-qmail-qfilter-osf branch 2 times, most recently from 2f965c6 to f791a11 Compare January 24, 2022 12:54
@schmonz schmonz force-pushed the schmonz-inject-recipient-qualification-tests branch 2 times, most recently from 3f6b2a8 to a1dbda3 Compare January 25, 2022 15:31
@schmonz schmonz requested a review from DerDakon January 25, 2022 18:04
bruceg and others added 3 commits January 25, 2022 22:19
From qmail-qfilter.git at commit 68ef8ee3d0e546ff1fec9d49f66e1c88e63ac1ba
(the newest available on master, dated 2017-10-26).
Co-authored-by: Amitai Schleier <schmonz-web-git@schmonz.com>
Co-authored-by: Astrid Sawatsky <astrid.sawatzky@gmx.de>
Co-authored-by: Evan Theriault <evanontario@hotmail.com>
Co-authored-by: Jacqueline Bilston <jmasonlee@gmail.com>
Co-authored-by: Joel Samaroo <xsamarjoex@gmail.com>
Co-authored-by: Patrick Ledzian <LiveActionCactus@users.noreply.github.com>
Co-authored-by: Trung Vo <ttrung.vo@gmail.com>
Co-authored-by: Yevhen Viktorov <y3vg3nk0@gmail.com>
@schmonz schmonz force-pushed the schmonz-inject-recipient-qualification-tests branch from a1dbda3 to d46a02a Compare January 25, 2022 21:30
@schmonz schmonz force-pushed the schmonz-inject-recipient-qualification-tests branch from d46a02a to 5f2547a Compare January 25, 2022 21:39
Copy link

@josuah josuah left a comment

Choose a reason for hiding this comment

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

This branch does not touch to any existing file beyond the Makefile.
This means very easy to integrate into an existing QMAIL setup.
It also should be straightforward to resolve the merge conflicts thanks to this.


static const char* qqargv[2];

/* a replacement for setenv(3) for systems that don't have one */
Copy link

Choose a reason for hiding this comment

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

Given setenv(3) is in POSIX,
https://pubs.opengroup.org/onlinepubs/9699919799/functions/setenv.html

I am simply curious about which system does not support it.
Maybe you ran into an counterexample?

Copy link

Choose a reason for hiding this comment

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

The POSIX one also has no vallen, I see in the rest of the code why this was desired.
This also means using memcpy for '\0'-terminated strings, which could be error-prone, but it looks correctly handled in all invocations of mysetenv.

munmap((void*)env, env_len);
}

/* Create a temporary invisible file opened for read/write */
Copy link

Choose a reason for hiding this comment

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

I like it! Disk storage, no cleanup needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants