[discovery] Add DNS discovery methods. Rabbit and Nats config to support DNS discovery #38

sgirones · 2017-10-02T14:22:14Z

Get a list of hosts from a DNS name
Use a DNS name for configuring Rabbit and Nats

rybit · 2017-10-02T16:12:24Z

discovery/main.go

+import (
+	"fmt"
+
+	"github.com/miekg/dns"


is there a reason to not use the builtin https://golang.org/pkg/net/#LookupSRV?

We aren't specifying a particular DNS server, just using the resolve.conf, meaning we gain nothing afaict

TBH I googled around and found that library to be used in some places. I will take a look at your suggestion

rybit · 2017-10-02T16:14:04Z

messaging/nats.go

-	Servers     []string    `mapstructure:"servers"`
-	LogsSubject string      `mapstructure:"log_subject"`
+	TLS            *tls.Config `mapstructure:"tls_conf"`
+	ServiceDNSName string      `mapstructure:"service_dnsname"`


can you add an envconfig:"dns_name" so that we can access it by envvars easily? It is redundant to say "service_" b/c it is embedded in a nats config already

I wanted to add "service" to make it explicit that it was a "discovery" DNS name, not a simple DNS name pointing to a host

rybit · 2017-10-02T16:15:36Z

messaging/nats.go

@@ -64,6 +66,23 @@ func ConfigureNatsConnection(config *NatsConfig, log *logrus.Entry) (*nats.Conn,

 // ConnectToNats will do a TLS connection to the nats servers specified
 func ConnectToNats(config *NatsConfig, errHandler nats.ErrHandler) (*nats.Conn, error) {
+	serversString := config.ServerString()
+
+	if config.ServiceDNSName != "" {


we should pull this into a method.

if config.ServiceDNSName != "" { config.Servers = discoverNatsService(config.ServicDNSName) }

… DNS discovery

rybit · 2017-10-03T16:46:55Z

discovery/main.go

+	Port uint16
+}
+
+func DiscoverEndpoints(serviceDNS string) ([]Endpoint, error) {


personally, I'd do something like this

func DiscoverEndpoints(serviceName string) ([]Endpoint, error) { _, remotes, err := net.LookupSRV("", "", serviceName) if err != nil { return nil, err } endpoints := []Endpoint{} for _, n := range remotes { endpoints = append(...) } return endpoints, nil }

a little different, mostly around some spacing and where I'd declare endpoints. That is mostly style though.

I am not sure if we need the distinction though. I mean the remote structure is pretty close to the Endpoint in general. I don't think we are getting much by doing this in commons really.

Initially I wanted to add more methods to Endpoint, but I guess it is worthless right now

rybit · 2017-10-03T16:47:45Z

messaging/nats.go

-	Servers     []string    `mapstructure:"servers"`
-	LogsSubject string      `mapstructure:"log_subject"`
+	TLS           *tls.Config `mapstructure:"tls_conf"`
+	DiscoveryName string      `mapstructure:"discovery_name"`


add a split_words:"true" so that the envconfig would be something like WHATEVER_NATS_DISCOVERY_NAME.

rybit · 2017-10-03T16:49:55Z

messaging/nats.go

@@ -92,3 +104,18 @@ func ErrorHandler(log *logrus.Entry) nats.ErrHandler {
 		}).Error("Error while consuming from " + sub.Subject)
 	}
 }
+
+func buildNatsServersString(serviceName string) (string, error) {
+	natsUrls := []string{}


/s/natsUrls/natsURLs/ ~ go uppercases any acronyms

rybit · 2017-10-03T16:49:57Z

messaging/nats.go

+		natsUrls = append(natsUrls, fmt.Sprintf("nats://%s:%d", endpoint.Name, endpoint.Port))
+	}
+
+	return strings.Join(natsUrls, ","), nil


I would return the nats urls directly

rybit · 2017-10-03T16:52:11Z

messaging/nats.go

@@ -64,6 +66,16 @@ func ConfigureNatsConnection(config *NatsConfig, log *logrus.Entry) (*nats.Conn,

 // ConnectToNats will do a TLS connection to the nats servers specified
 func ConnectToNats(config *NatsConfig, errHandler nats.ErrHandler) (*nats.Conn, error) {
+	var err error
+	serversString := config.ServerString()


personally, I'd choose to do this after the discover block. Something like:

if config.DiscoveryName != "" { var err error config.Servers, err = discoverNatsURLs(config.DiscoveryName) if err != nil { return nil, errors.Wrapf(err, "Failed to lookup servers for service %s", config.DiscoveryName) } } ... return nats.Connect(config.ServerString(), options...)

ah, I like that

rybit · 2017-10-03T16:55:11Z

messaging/rabbit.go

@@ -204,11 +206,20 @@ func ValidateRabbitConfigStruct(servers []string, exchange ExchangeDefinition, q

 // ConnectToRabbit will open a TLS connection to rabbit mq
 func ConnectToRabbit(config *RabbitConfig, log *logrus.Entry) (*Consumer, error) {
-	if err := ValidateRabbitConfig(config); err != nil {
+	var err error
+	if err = ValidateRabbitConfig(config); err != nil {


personally, I keep var scoped down in if statements. It stops from accidentally using the wrong err b/c of shadowing.

rybit · 2017-10-03T16:55:32Z

messaging/rabbit.go

@@ -204,11 +206,20 @@ func ValidateRabbitConfigStruct(servers []string, exchange ExchangeDefinition, q

 // ConnectToRabbit will open a TLS connection to rabbit mq
 func ConnectToRabbit(config *RabbitConfig, log *logrus.Entry) (*Consumer, error) {
-	if err := ValidateRabbitConfig(config); err != nil {
+	var err error


declare this in the if block you're using it in

The problem I found is, when I assign the result of discoverRabbitServers to servers and err, if I use :=, it fails because of:

servers declared and not used (build)

I guess I could have used a tmp var and then assign it to servers?

Declaring err outside the if, allowed me to do:

servers, err = ...

rybit · 2017-10-03T16:56:20Z

messaging/rabbit.go

-	conn, err := DialToRabbit(config.Servers, config.TLS, log)
+	servers := config.Servers
+	if config.DiscoveryName != "" {
+		servers, err = discoverRabbitServers(config.DiscoveryName)


again, I'd set the config.Servers this way when you inspect the object afterwards you don't have a difference from what it is connecting an what it says it is using

sgirones changed the title ~~[discovery] Add DNS discovery methods. Rabbit and Nats config support DNS discovery~~ [discovery] Add DNS discovery methods. Rabbit and Nats config to support DNS discovery Oct 2, 2017

sgirones force-pushed the salvador/add-dns-discovery-package branch from 90de812 to b1863e4 Compare October 2, 2017 15:59

rybit reviewed Oct 2, 2017

View reviewed changes

sgirones force-pushed the salvador/add-dns-discovery-package branch from 9b7a3c2 to 8fce4cb Compare October 3, 2017 11:13

sgirones added 2 commits October 3, 2017 13:14

[discovery] Add DNS discovery methods. Rabbit and Nats config support…

ea01df8

… DNS discovery

fix deps

7f02246

sgirones force-pushed the salvador/add-dns-discovery-package branch from 8fce4cb to bc46ca2 Compare October 3, 2017 11:14

address comments

5ddd599

sgirones force-pushed the salvador/add-dns-discovery-package branch from bc46ca2 to 5ddd599 Compare October 3, 2017 11:21

rybit reviewed Oct 3, 2017

View reviewed changes

address comments

31f5251

calavera merged commit 44fb358 into master Oct 4, 2017

calavera deleted the salvador/add-dns-discovery-package branch October 4, 2017 17:29

[discovery] Add DNS discovery methods. Rabbit and Nats config to support DNS discovery #38

[discovery] Add DNS discovery methods. Rabbit and Nats config to support DNS discovery #38

Uh oh!

Conversation

sgirones commented Oct 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgirones Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sgirones Oct 4, 2017 •

edited

Loading