Modularize - Ability for server to be used as a package #27

jordanschalm · 2016-12-10T05:03:00Z

…ored client handling code.

Remove old `server` package Refactor cmd package to work with new server Remove `models.go`, remove parts and refactor other parts into either `server.go` or `client.go` Change format of `allowed_hosts` to JSON array from comma separated string

Refactor NewServer and server.run into server file Remove requirement for servers to receive appconfig

…nning apps

Refactor MD5Hash method to not use pointers to strings (go strings are pointers under the hood already anyway) Refactor mysql+redis backend implementation to work with new backend interface and modified client struct Move concern of reading a configuration file to the command line interface

* tls sate always goes back to command state

flashmob · 2016-12-13T16:01:23Z

Thanks. Code looks very neat.
Well, currently the stuck on #19 - so would merge this first

Some feedback

There's some imports still pointing to your fork (serve.go)

parseHeaders() - maybe parsing headers could be left to the backends? If possible, looking to defer as much work as possible to the backends, or even further down the stack. Would also like to avoid regex where possible. The previous implementation used a simple string compare to find the subject only and stopped scanning once found it, but full header parsing might be better to avoid until later?

parseHeaders() - not all emails can have headers, and data can be unpredictable. The code would panic if the map was nil.

parseHeaders() - to detect header start / end, would it be easier to get a slice like this: headers := [0:strings.Index("\r\n\r\n")] (of course bounds checking before).

For parsing headers, these readers may be useful https://golang.org/pkg/net/textproto/ rather than the http package.

ClientStartTLS - found a bug that would cause an infinite loop if client closes connection during negotiation. (This was already there before)

ClientStartTLS - according to rfc3207, it should not respond with any 220 message after TLS negotiation, but go into expecting EHLO from the client. Your mod to reset the client is correct.

ClientStartTLS - If TLS negotiation failed, it might as well go back to command state without any additional message, the client can decide whenever to continue or not since it would know if TLS failed or not. (Interpreting rfc3207)

So... Committed some fixes to this PR & took the server for a spin on production!
Hmm.. It didn't go to well, after a few minutes, it started having trouble with "too many open file" errors and hogging all cpu. Had to shut it down and current master is handling the same traffic with no issues. Not sure what is causing it yet? Perhaps stuck in a loop. There were a lot of "Error reading data" messages before it went into "too many open file" mode..

jordanschalm · 2016-12-13T17:25:41Z

Thanks for the feedback. I'll move the header parsing to a util function and leave it to the backend and try to figure out what is causing those errors.

* added some kills for read errors

flashmob · 2016-12-14T00:42:37Z

Still not sure what is causing the above problem. It looks like the number of connected clients fills up quickly to the max (5000) and then the "too many open file" errors come in this form:

time="2016-12-13T16:47:28Z" level=debug msg="Waiting for a new client. Client ID: 5387"
time="2016-12-13T16:47:28Z" level=info msg="Error accepting client" error="accept tcp 192.99.19.220:25: accept4: too many open files"

Noticed that there is a bug when in DATA state, if the host is not allowed, it will still go to save the email & respond with two messages, 550 Error: Host not allowed: test.com and 250 OK : queued as 309ea3da1721ec4e96da2e7a0c082275

Making some fixes, will commit and send them up soon, then test again

flashmob · 2016-12-14T01:50:20Z

Still getting the errors after the bug fixes. Slight correction to the above message: server fills up to about 1000 clients before going into weird state.
Got a feeling that the backend can't keep up?
Looking at the guerrilla_db_redis.go file, it looks like the notification channel has been moved there, may be something with this.

…l workers from being created and causing deadlock in the Redis+MySQL backend

…into modularize

flashmob · 2016-12-20T22:56:15Z

Thanks for updates, including the Readme update - was going to ask you about it, but looks like we're on the same page!

We should be ready to merge this soon?

Regarding the c *Client, what would you say if we placed the c.Data, c.MailFrom, c.RcptTo, c.Helo, c.Address, c.TLS, in their own struct, eg. type MailData struct. The reason is that the backend should not depend on the *Client, mostly because it will make it easier to to work with it in the future, since the backends will not hold the pointer to Client. It may also make optimizations easier in the future, eg. c.Data to could be a bytes buffer instead of a string, and we don't have to worry about changing the API signiture if the Client changes. What do you say?

jordanschalm · 2016-12-20T23:01:17Z

I agree. Separating public Client data into its own struct is a good idea. I can do that later today. After that it should be ready to merge.

flashmob · 2016-12-20T23:58:38Z

Thanks! That will be another bounty earned!

Before merging, a thorough test will be done on Guerrilla Mail production.

Btw, In the future, considering to change Data to a []byte - the reason is because string assumes text, however Data is just a raw chunk of bytes that does not really represent text, eg it could be 8bitmime. The other reason, it would be more efficient to use with Reader / Writer to avoid the string to []byte conversion. What do you think?

…into modularize

jordanschalm · 2016-12-21T00:51:33Z

If we're leaving the interpretation of the DATA to backends, I think leaving Data as byte array is the right choice. It might make pulling the subject header out more complicated though.

…e. Move `Client#Hash` into sql+redis backend

flashmob · 2016-12-21T02:17:34Z

Thanks. That was quick!

Regarding MailData, do you think this would be the best name, or could there be more concise alternatives? Eg. Envelope or any other ideas?

…into modularize

flashmob · 2016-12-21T17:07:57Z

Last push fixed a bug that caused start tls to be advertised even though when it was off. Also, STARTTLS command will be ignored if it is disabled (lots of clients try it despite not advertised). Getting more stable to merge. Is there anything else you wanted to add?

jordanschalm · 2016-12-21T19:04:13Z

Nice. I would like to come up with a better name than MailData. You're right, that is pretty vague...

Maybe one of:
Envelope
Parcel
Message
MailPayload
Email

And do you want to change EmailParts to EmailAddress or MailAddress? I think it's a bit clearer. Email is such an overloaded term

jordanschalm · 2016-12-22T02:01:33Z

I'm happy to merge now, unless you want to change the Envelope/EmailParts naming.

…into modularize

flashmob · 2016-12-22T02:27:08Z

Envelope would be a good metaphor. Good point on the EmailParts, EmailAddress is better. Also, in the client struct, Address was changed to RemoteAddress, just to make things clear.

So testing is going great! This branch has been running on Guerrilla Mail for the last 12 hours, usually middle of the week is the most busiest time for a battle test.

Looks like ready to go. Thanks for another bounty earned!

jordanschalm added 16 commits November 27, 2016 20:30

Begin major refactoring. Collapsed subpackages into top level, refact…

4859e9d

…ored client handling code.

Remove go from repo and icon name

ce5f3a2

Remove old `server` package Refactor cmd package to work with new server Remove `models.go`, remove parts and refactor other parts into either `server.go` or `client.go` Change format of `allowed_hosts` to JSON array from comma separated string

Change repo name

ce366b9

add backend model to root package

e60264a

Fix ROOT in makefile

502939c

Refactor NewServer and server.run into server file Remove requirement for servers to receive appconfig

fix imports in cmd

393f946

Add comments

68c4eff

Add Guerrilla struct and global New/Run interface for starting and ru…

0068ee9

…nning apps

add message sent tracking field to clients, update backend interface

f52ca17

PR for modularize / code review

09b9110

remove changes file and remove from gitignore

4dfb974

add glide dependency management

148d911

make server private to guerrilla package

df865ee

Update sample config to use JSON array for allowed hosts

fcbede1

Fix loadconfig method of dummyconfig

9a1999f

jordanschalm changed the title ~~Modularize - Ability for server to be used as a package~~ WIP: Modularize - Ability for server to be used as a package Dec 10, 2016

flashmob added 4 commits December 14, 2016 01:16

* change imports back to flashmob's repo

295e455

* tls sate always goes back to command state

* don't reply with any response after switching to TLS

1f92198

* allocate the Headers map

8a9a0b3

* fix panic when saving to mysql (passed as a string)

603df7d

* do not allocate client until we have space for it

97334f5

* added some kills for read errors

flashmob added 2 commits December 14, 2016 11:56

do not process email if host is not allowed

4159692

changed greeting to show number of active clients

f8cb332

jordanschalm added 2 commits December 13, 2016 19:10

merge

692ec73

Fix issue where config file wasn't read correctly, preventing saveMai…

fae8e89

…l workers from being created and causing deadlock in the Redis+MySQL backend

jordanschalm and others added 6 commits December 16, 2016 16:03

add env var for vendor in go 1.5

c0e4eee

test travis

7712473

get travis working, revert import names back to flashmob

8a03136

update readme with BackendResult interfacce example;

b4d2847

Merge branch 'modularize' of https://github.com/flashmob/go-guerrilla …

18335c3

…into modularize

Add check that system open file limit is less then max clients at launch

6e9c577

Merge branch 'modularize' of https://github.com/flashmob/go-guerrilla …

c38928f

…into modularize

move public client fields to MailData struct and make client privat…

55a49d8

…e. Move `Client#Hash` into sql+redis backend

flashmob added 6 commits December 21, 2016 17:32

add ip to log message

cdd6a0e

Merge branch 'modularize' of https://github.com/flashmob/go-guerrilla …

638d9e9

…into modularize

renamed clientHandshake state to clientGreeting

291c982

don't advertise STARTTLS if turned off

e11c7ae

don't recognize STARTTLS command if turned off

00817c6

simplify

571069f

jordanschalm added 2 commits December 21, 2016 17:57

change names: EmailParts -> EmailAddress, MailData -> Envelope

b207f88

Fix naming in README

bbb4986

flashmob added 2 commits December 22, 2016 13:05

rename MailData to Envelope

8c14ef6

Merge branch 'modularize' of https://github.com/flashmob/go-guerrilla …

d1b9944

…into modularize

rename Address to RemoteAddress

9ebedb3

flashmob merged commit 88d73fe into master Dec 22, 2016

flashmob deleted the modularize branch December 22, 2016 03:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modularize - Ability for server to be used as a package #27

Modularize - Ability for server to be used as a package #27

jordanschalm commented Dec 10, 2016

flashmob commented Dec 13, 2016

jordanschalm commented Dec 13, 2016

flashmob commented Dec 14, 2016 •

edited

Loading

flashmob commented Dec 14, 2016

flashmob commented Dec 20, 2016

jordanschalm commented Dec 20, 2016

flashmob commented Dec 20, 2016

jordanschalm commented Dec 21, 2016

flashmob commented Dec 21, 2016

flashmob commented Dec 21, 2016

jordanschalm commented Dec 21, 2016 •

edited

Loading

jordanschalm commented Dec 22, 2016

flashmob commented Dec 22, 2016

Modularize - Ability for server to be used as a package #27

Modularize - Ability for server to be used as a package #27

Conversation

jordanschalm commented Dec 10, 2016

flashmob commented Dec 13, 2016

jordanschalm commented Dec 13, 2016

flashmob commented Dec 14, 2016 • edited Loading

flashmob commented Dec 14, 2016

flashmob commented Dec 20, 2016

jordanschalm commented Dec 20, 2016

flashmob commented Dec 20, 2016

jordanschalm commented Dec 21, 2016

flashmob commented Dec 21, 2016

flashmob commented Dec 21, 2016

jordanschalm commented Dec 21, 2016 • edited Loading

jordanschalm commented Dec 22, 2016

flashmob commented Dec 22, 2016

flashmob commented Dec 14, 2016 •

edited

Loading

jordanschalm commented Dec 21, 2016 •

edited

Loading