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

fix(session): detect canceled lookup correctly #626

Closed
wants to merge 1 commit into from

Conversation

mmatous
Copy link
Contributor

@mmatous mmatous commented Aug 24, 2023

Hi, this will prevent smtp: rDNS error {"reason":"","src_ip":"x.x.x.x:port"} for every incoming message.
Weirdly, it didn't start appearing before using milter/rspamd. It should be fixed regardless, but I don't really know what changed. Maybe timeout expiring while waiting for rspamd response? Or rspamd hogging DNS resolver time for it's own checks.

cancelation is not DNSError, so UnwrapDNSErr() returns "" as reason

Signed-off-by: Martin Matous <[email protected]>
@mmatous
Copy link
Contributor Author

mmatous commented Oct 3, 2023

I'm having trouble reproducing this error. I prepared the following scripts to simulate your CI and test various go versions

FROM ubuntu:20.04

RUN set -ex && \
    apt update -y && apt dist-upgrade -y && \
    apt install -y build-essential wget git libpam-dev && \
    wget 'https://github.com/mmatous/maddy/archive/refs/heads/dev.tar.gz' && \
    tar -xzf dev.tar.gz

RUN wget 'https://go.dev/dl/go__VER__.linux-amd64.tar.gz' && \
    tar -C /usr/local -xzf 'go__VER__.linux-amd64.tar.gz'

ENV PATH="${PATH}:/usr/local/go/bin"
WORKDIR /maddy-dev
RUN ./build.sh && \
    go test ./... -coverprofile=coverage.out -covermode=atomic
WORKDIR ./tests
ENTRYPOINT ./run.sh
#!/usr/bin/env bash

sed "s/__VER__/$1/g" ./Dockerfile.template > "./Dockerfile.$1"
docker build --tag "maddy-$1" --file "./Dockerfile.$1" .

Usage e.g.:

sudo ./dbuild.sh 1.18.9
sudo docker run --rm --interactive --tty maddy-1.18.9

I install seemingly unnecessary packages for cases where I wanted to look around manually and running integration tests is left outside of build phase so it can be called in a loop.
Anyway, I found that the tests almost always (~95 %) pass while intermittent errors can happen. For my changes I managed to get

--- FAIL: TestDNSBLConfig (0.05s)
    t.go:159: Using /tmp/maddy-tests-2406491360
    t.go:209: launching [./maddy.cover -config /tmp/maddy-tests-2406491360/maddy.conf -debug.smtpport 0 -debug.dnsoverride 127.0.0.1:36663 -log stderr -test.coverprofile /tmp/maddy-coverage-re
port.178a71122d6de9c7]
    t.go:254: maddy: dnsbl: List does not contain a test record for 127.0.0.2   {"list":"dnsbl.test"}
    t.go:254: maddy: dnsbl: List does not contain a test record for ::FFFF:7F00:2       {"list":"dnsbl.test"}
    t.go:254: maddy: dnsbl: List does not contain a test record for 'test' TLD  {"list":"dnsbl.test"}
    t.go:263: Log ended before all expected listeners are up. Start-up error?
FAIL
exit status 1
FAIL    github.com/foxcpp/maddy/tests   10.337s

Or a different run:

--- FAIL: TestIMAPEndpointAuthMap (0.11s)
    t.go:159: Using /tmp/maddy-tests-2317279703
    t.go:209: launching [./maddy.cover -config /tmp/maddy-tests-2317279703/maddy.conf -debug.smtpport 0 -debug.dnsoverride 127.0.0.1:42773 -log stderr -test.coverprofile /tmp/maddy-coverage-re
port.178a72c1edaef9dc]
    t.go:254: maddy: imap: listening on tcp://127.0.0.1:43717    (test runner>listener wait trigger<)
    t.go:254: maddy: imap: TLS is disabled, this is insecure configuration and should be used only for testing!
    imap_test.go:39: 47167 < imap: * OK [CAPABILITY IMAP4rev1 LITERAL+ SASL-IR CHILDREN UNSELECT MOVE IDLE APPENDLIMIT LOGINDISABLED COMPRESS] IMAP4rev1 Service Ready
    imap_test.go:40: 47167 > imap: . LOGIN [email protected] 123
    imap_test.go:41: 47167 < imap: . NO Authentication disabled
    imap_test.go:41: Response line not matching the expected pattern, want ". OK *"
    t.go:254: maddy: signal received (interrupt), next signal will force immediate shutdown.
FAIL
exit status 1
FAIL    github.com/foxcpp/maddy/tests   11.100s

And even on current dev HEAD without my changes:

smtp/server 2023/10/03 01:34:41 handler error: read tcp 127.0.0.1:49947->127.0.0.1:46800: read: connection reset by peer
smtp/server 2023/10/03 01:34:41 handler error: read tcp 127.0.0.1:50278->127.0.0.1:53322: read: connection reset by peer
--- FAIL: TestSMTPFlood_EnvelopeAbort_NoLimits_10Conns (0.05s)
    t.go:159: Using /tmp/maddy-tests-3724608267
    t.go:209: launching [./maddy.cover -config /tmp/maddy-tests-3724608267/maddy.conf -debug.smtpport 0 -debug.dnsoverride 127.0.0.1:34535 -log stderr -test.coverprofile /tmp/maddy-coverage-re
port.178a750869db92e1]
    t.go:263: Log ended before all expected listeners are up. Start-up error?
FAIL
exit status 1
FAIL    github.com/foxcpp/maddy/tests   12.543s

I only got consistent failures from go 1.20 onward and that was unit tests (likely expected for dev branch).
Unless there's a mistake in my approach, I'm currently inclined to chalk the failure up to brittle tests.
Do you think the PR is erroneous?

@foxcpp
Copy link
Owner

foxcpp commented Jan 21, 2024

Cherry-picked into master as c67955e. Thanks!

@foxcpp foxcpp closed this Jan 21, 2024
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.

2 participants