-
Notifications
You must be signed in to change notification settings - Fork 597
atc_api requires bind information now. #174
Conversation
addr = net.JoinHostPort(addr, port) | ||
} | ||
_, port, _ := net.SplitHostPort(s.GetAddr()) | ||
addr = net.JoinHostPort(addr, port) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of our tests use empty string as the client address, so I removed the conditional for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough :)
} | ||
if data.Secondary != "" { | ||
data.Secondary = net.JoinHostPort(data.Secondary, info.Port) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can improve this bit but I am happy to do it in a separate diff so I get to mess around this part of the code.
Basically, if the connections is HTTP and port is 80, we dont need to happen the port number.
if the connections is HTTPS and port is 443, same story.
Also, at this stage, we assume we talk plain HTTP because we don't provide a hint of HTTP or HTTPS. Again, I am happy to deal with it in a separate diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing here is hardcoding us to HTTP vs HTTPS. And providing the ports here doesn't cause any issues. It just means the UI is going to be more specific than it needs to be with the AJAX calls. Given that, I don't think it buys us very much to put extra logic in here to prevent adding the port numbers if they're unnecessary. Having port numbers works in 100% of cases, so I don't see the isse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I agree on the port thing. It is just cosmetics.
For HTTP/HTTPS I guess we can work around it on the javascript side. e.g if the call was made over HTTPS, prepend https:// to the next call, otherwise, http.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, that's where this should be done.
atc_api requires bind information now.
@@ -32,9 +33,13 @@ func main() { | |||
api.Log.Println("Connected to atcd socket on", args.ThriftAddr) | |||
} | |||
|
|||
if args.IPv4 == "" || args.IPv6 == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, this will fatal if ether ipv4 or ipv6 is not set. It should be.
if args.IPv4 == "" && args.IPv6 == "" {
When running the
atc_api
process, you now specify the-4
and-6
options, whichtell the API which addresses it should expose to the client.
The javascript client uses this information to enable shaping on both IPv4 and IPv6
interfaces.
Better documentation for this workflow forthcoming.