Skip to content

Comments

bullet proof smtp#2818

Merged
supersven merged 38 commits intodevelopfrom
sventennie/bullet_proof_smtp
Nov 23, 2022
Merged

bullet proof smtp#2818
supersven merged 38 commits intodevelopfrom
sventennie/bullet_proof_smtp

Conversation

@supersven
Copy link
Contributor

@supersven supersven commented Nov 2, 2022

Problem to be solved:
When an SMTP server was misconfigured in the brig configuration or was misbehaving itself, brig was hanging on startup without any useful logs.

Changes in this PR:

  • Add tests for sending via SMTP. The used SMTP server is postie. Unfortunately, the current version of postie wasn't compatible to our GHC version due to outdated dependencies. I've updated postie's dependencies and brought it upstream (alexbiehl/postie@c927023). However, these changes are neither on Hackage nor in nixpkgs, yet. That's why I had to add a Nix override...

  • The entry and exit (with result) are now logged. I've separated the log logic from other concerns.

  • In the initialization code, logs are flushed before any error is thrown.

  • I've added a timeout with a big tolerance to every action. This should prevent use from hanging due to endlessly delaying SMTP servers and other network obscurities.

Behaviour:

  • Despite logging and an additional exception on timeout, the observable behavior should be unchanged. All caught exceptions are logged and then re-thrown as is.

Output without and with this patch:

When the SMTP server isn't reachable on startup.
Before:

brig: Network.Socket.connect: <socket: 13>: does not exist (Connection refused)

Now:

brig: Failed to establish test connection with SMTP server: CaughtException Network.Socket.connect: <socket: 13>: does not exist (Connection refused)
CallStack (from HasCallStack):
  error, called at src/Brig/SMTP.hs:116:7 in brig-2.0-inplace:Brig.SMTP
[brig] W, Checking test connection to localhost on startup : Caught exception : Network.Socket.connect: <socket: 13>: does not exist (Connection refused)

Where [brig] is logged output and brig: stderr/stdout.

Ticket: SQPIT-497

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@supersven supersven force-pushed the sventennie/bullet_proof_smtp branch from 8790f65 to 422ff50 Compare November 2, 2022 15:58
@supersven supersven temporarily deployed to cachix November 2, 2022 15:58 Inactive
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Nov 2, 2022
@supersven supersven temporarily deployed to cachix November 2, 2022 16:14 Inactive
@supersven supersven temporarily deployed to cachix November 3, 2022 16:10 Inactive
@supersven supersven temporarily deployed to cachix November 3, 2022 16:54 Inactive
@supersven supersven temporarily deployed to cachix November 4, 2022 07:35 Inactive
@supersven supersven temporarily deployed to cachix November 4, 2022 10:48 Inactive
@supersven supersven temporarily deployed to cachix November 4, 2022 11:56 Inactive
@supersven supersven temporarily deployed to cachix November 4, 2022 12:00 Inactive
@supersven supersven marked this pull request as ready for review November 4, 2022 13:25
@supersven supersven force-pushed the sventennie/bullet_proof_smtp branch from b677666 to 573a0a4 Compare November 11, 2022 06:38
@supersven supersven temporarily deployed to cachix November 11, 2022 06:38 Inactive
@supersven supersven temporarily deployed to cachix November 11, 2022 06:38 Inactive
@supersven supersven requested a review from fisx November 16, 2022 10:47
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

i left some comments. looks good, but i think i want a guided tour on monday or so! :-) ping me!

@supersven supersven temporarily deployed to cachix November 21, 2022 12:46 Inactive
@supersven supersven temporarily deployed to cachix November 21, 2022 12:46 Inactive
@supersven supersven force-pushed the sventennie/bullet_proof_smtp branch from 977dd09 to f509136 Compare November 22, 2022 17:42
@supersven supersven temporarily deployed to cachix November 22, 2022 17:42 Inactive
@supersven supersven temporarily deployed to cachix November 22, 2022 17:42 Inactive
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

👍

Comment on lines +174 to +177
let resultLog = case e of
SMTPUnauthorized ->
("Failed to establish connection, check your credentials." :: String)
SMTPConnectionTimeout -> ("Connection timeout." :: String)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let resultLog = case e of
SMTPUnauthorized ->
("Failed to establish connection, check your credentials." :: String)
SMTPConnectionTimeout -> ("Connection timeout." :: String)
let resultLog = show e

? not sure about this one :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd argue that it's fine to be a bit more chatty in log messages. Finally, they are meant to be interpreted by Ops people outside of Wire.

Co-authored-by: fisx <mf@zerobuzz.net>
@supersven supersven temporarily deployed to cachix November 23, 2022 09:42 Inactive
@supersven supersven temporarily deployed to cachix November 23, 2022 09:42 Inactive
Co-authored-by: fisx <mf@zerobuzz.net>
@supersven supersven temporarily deployed to cachix November 23, 2022 09:42 Inactive
@supersven supersven temporarily deployed to cachix November 23, 2022 09:42 Inactive
@supersven supersven temporarily deployed to cachix November 23, 2022 10:07 Inactive
@supersven supersven temporarily deployed to cachix November 23, 2022 10:07 Inactive
@supersven supersven merged commit 5d9f018 into develop Nov 23, 2022
@supersven supersven deleted the sventennie/bullet_proof_smtp branch November 23, 2022 12:15
supersven added a commit that referenced this pull request Nov 24, 2022
@supersven supersven restored the sventennie/bullet_proof_smtp branch November 24, 2022 15:51
@supersven supersven temporarily deployed to cachix November 24, 2022 15:52 Inactive
@supersven supersven temporarily deployed to cachix November 24, 2022 15:52 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants