Use dynamic service discovery for client dns routing #266
Conversation
jakubno
force-pushed
the
use-dynamic-service-discovery-for-client-dns-routing-e2b-1503
branch
from
bd4841c to
ae81b8a
Compare
| dnsServer := dns.New() | ||
| intPort, convErr := strconv.Atoi(port) | ||
| if convErr != nil { | ||
| log.Fatalf("Failed to convert DNS_PORT to int: %v\n", convErr) |
There was a problem hiding this comment.
I was going to go through and change these log.Fatalf calls, but they call os.Exit which means defers don't run. (I'm generally of the opinion that log.Fatal should really only be used in the body of main() , and otherwise you should either panic or return an error). In any case, this function returns an error, so I think we should just do that here.
I'd also say it's probably fine to just default to 53, I'm not sure the case where this wouldn't be correct.
| dnsServer := dns.New() | ||
| intPort, convErr := strconv.Atoi(port) |
There was a problem hiding this comment.
in the start function (which takes an int) here we just convert it back to string (and if it's not a valid port number it'll error there.
Admittedly the Start method doesn't return in the foreground which means we either have to panic, or continue without a functioning DNS server (this has implications for #261).
While we're (I'm) being pedantic, a valid int isn't neccessarily a valid port number: port numbers have to be valid uint16 numbers and int is basically always an int64 (but sometimes an int32 though in practice we may be confident that we'll never need to run this software from a 32-bit build). So we could pass this and still have an invalid port number.
packages/nomad/api.hcl
Outdated
| @@ -73,6 +73,9 @@ job "api" { | |||
| REDIS_URL = "${redis_url}" | |||
| # This is here just because it is required in some part of our code which is transitively imported | |||
| TEMPLATE_BUCKET_NAME = "skip" | |||
| ADMIN_TOKEN = var.admin_token | |||
| REDIS_URL = var.redis_url | |||
There was a problem hiding this comment.
this duplicates line 72-73, which is either unnecessary or confusing 😅
There was a problem hiding this comment.
sorry, some mistake in merge conflict
packages/nomad/api.hcl
Outdated
| @@ -73,6 +73,9 @@ job "api" { | |||
| REDIS_URL = "${redis_url}" | |||
| # This is here just because it is required in some part of our code which is transitively imported | |||
| TEMPLATE_BUCKET_NAME = "skip" | |||
| ADMIN_TOKEN = var.admin_token | |||
| REDIS_URL = var.redis_url | |||
| DNS_PORT = 5353 | |||
There was a problem hiding this comment.
is there a particular reason to use a non-default DNS port?
There was a problem hiding this comment.
yes, there already running another DNS server on 53
Failed starting DNS server{error 26 0 failed to start DNS server: listen udp 0.0.0.0:53: bind: address already in use}
jakubno
force-pushed
the
use-dynamic-service-discovery-for-client-dns-routing-e2b-1503
branch
from
ae81b8a to
8cb2af0
Compare
Description
Use consul service discovery for sandbox DNS, this will allow us to swap API servers without problems with resolving DNS in client proxy