Skip to content

Add support of TLS connections#17

Merged
speedywizard merged 5 commits intoassembla:masterfrom
Onlinehead:master
Nov 22, 2016
Merged

Add support of TLS connections#17
speedywizard merged 5 commits intoassembla:masterfrom
Onlinehead:master

Conversation

@Onlinehead
Copy link
Copy Markdown
Contributor

@Onlinehead Onlinehead commented Nov 16, 2016

Just little change about support of TLS connections which sometimes useful.
Here is example of using with TLS:

amqp_url := "amqps://guest:guest@127.0.0.1:5671/"
cert, err := tls.LoadX509KeyPair("cert.pem", "key.pem")
tls_config := tls.Config{
			Certificates: []tls.Certificate{cert},
			InsecureSkipVerify: true}
cli := cony.NewClient(
		cony.URL(amqp_url),
		cony.Backoff(cony.DefaultBackoff),
		cony.TLS(&tls_config),
	)

@kron4eg
Copy link
Copy Markdown
Contributor

kron4eg commented Nov 17, 2016

@Onlinehead hello, and thank you for your PR!

I want to suggest couple of things. Please take a look at how amqp.Dial() and amqp.DialTLS() are implemented. They both use the same amqp.DialConfig(), which call actual dial function, and it can handle amqps:// scheme automatically, so there is no need in

if c.tlsConf != nil {

So what we can do instead of cony.TLS() function is cony.Config(), and pass initialized amqp.Config in it, which should be used in amqp.DialConfig replacing amqp.Dial in cony source.

Thank you!

@Onlinehead
Copy link
Copy Markdown
Contributor Author

Onlinehead commented Nov 18, 2016

@kron4eg Thanks for your tips.
I changed code for using DialConfig and yes, that is pretty better way:)
About checks, that is so strange, because on my PC checks passed without error:

PASS
ok      github.com/onlinehead/cony  0.015s

@kron4eg
Copy link
Copy Markdown
Contributor

kron4eg commented Nov 18, 2016

I wander if it's necessary to provide default Heartbeat in case it's 0, like in original amqp.Dial()... It's very easy to forget about it and get strange disconnects.

@kron4eg
Copy link
Copy Markdown
Contributor

kron4eg commented Nov 18, 2016

LGTM 👍

@speedywizard speedywizard merged commit fb14870 into assembla:master Nov 22, 2016
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.

3 participants