RPC and HTTP interfaces fully generically-sockified so Unix is supported... #587
Conversation
|
Awesome! Do you think you could add appropriate tests for all these changes? Where necessary we can guard them with a runtime.GOOS to make sure we don't run on Windows. |
|
In past conversation with you on IRC you indicated that, since in theory you'd want to run all tests with these parameters set, it would be acceptable to test the changes by setting them and then re-running the normal test suite. This implies a more manual process, but otherwise I'm not sure what the best way to go about it is (maybe munging the makefile that handles "make test"?) If you're thinking of more basic individual tests to simply ensure that Consul can be spun up and a client can successfully connect when using Unix sockets, then yes, I can do that, except that as we've previously discussed since nearly all unit tests (on master) fail for me right now I wouldn't have a good baseline for determining whether these would be an issue. |
|
We strive for as high of test coverage as possible, and I think there are lots of testable areas with this patch. All of the tests are currently passing for us, and I believe @ryanuber changed the tests that were failing for you so that they should be passing more reliably as well. |
|
Sadly, this is not the case. Half of the tests still fail on current master (as of a couple of minutes ago). See https://gist.github.com/jefferai/b9f366330f1fbbfbe3e8 for a log. |
|
FWIW, machine is Ubuntu 14.04 running in VirtualBox on an early 2013 MacBook Pro. Go 1.3.3. Shouldn't be anything super weird. |
|
More debugging: after setting GOMAXPROCS=2 and re-running "make test" all tests passed. When I ran "make test" again afterwards, some of the tests failed again. |
|
OK, so for right now I'm taking the approach of "my code shouldn't fail any harder than master does" and with that, I found an issue. In toHTTP in api.go there is a urlRaw parameter. You can see that I changed this in my commits in favor of using the path and later explicitly setting the scheme and host in the request. The reason is that urlRaw will always return a string with http:// even when it should be returning just a path. I went down a rabbit hole of Go code and basically this is a hardcoded assumption that is made. I believe, but can check, that simply converting "http" to "unix" when passing into http.NewRequest did not work. By making the change that I did, various commands I tested (such as "consul watch") were able to connect and pull data from the http interface over a Unix socket. Unfortunately this breaks the API unit tests rather badly. I'll be digging into this and looking for an alternate solution, but any insights would be helpful. |
|
Just as an update, I was unable to work on this over the weekend due to being quite sick, but I've knocked down one of the unit test failures and updated the branch accordingly. |
|
As of this point, as far as I can tell, this branch does not fail any more than the master branch. The master branch still fails (not deterministically) in api/ and consul/, which makes it hard to ensure the remaining failures are just due to race conditions. |
|
@jefferai The tests passing as often as master is a good sign. I do still think we need some general testing around some of the functions that are going in here though. For example, |
|
@ryanuber Sure. I haven't had time to get to new tests yet, just fixing the one regression I had against old tests and running over and over to determine what's likely to be a problem and what's just test brokenness. New tests are next. |
|
@jefferai sounds good, just let us know when you think its ready and we'll take another look! |
|
@ryanuber @armon in agent.go, in Create, a configuration is passed in; this configuration is then ignored by the agent.setupServer and agent.setupClient calls, which means my adjustments to the config to have it listen on Unix sockets have no effect (on either the server or client). I'd like to change setupServer and setupClient to honor config.Addresses if set to something other than the defaults. This feels slightly intrusive, but I don't see any other way. Comments? |
|
@jefferai Not sure why they would be impacted? Since the HTTP listener is in the agent level and not at the Consul level. |
|
OK, I think the issue is that I didn't notice that the config was being translated from an agent.Config to consul.Config. I will try to dig into the code more but I'm a bit unclear as to where the separations between "server" and "client" are. For instance, the RPC socket provided by the running Consul instance is provided by the agent, and when I start Consul normally it honors what I choose (Unix/TCP). However, in a test that calls makeAgent with conf.Server set to true, it starts the "server" part of Consul but does not actually pay any attention to the RPC bits of the config. In TestAgent_RPCPing, which seemed like a logical place to base a Unix socket test off of, it seems like it's actually just talking to the "server" on the backend and not through the normal RPC interface. I'll keep digging and trying to find a better place, but any guidance/pushes in the right direction would be useful. |
…ted. Client works for RPC; will honor CONSUL_RPC_ADDR. HTTP works via consul/api; honors CONSUL_HTTP_ADDR. The format of a Unix socket in configuration data is: "unix://[/path/to/socket];[username or uid];[gid];[mode]" Obviously, the user must have appropriate permissions to create the socket file in the given path and assign the requested uid/gid. Also note that Go does not support gid lookups from group name, so gid must be numeric. See https://codereview.appspot.com/101310044 When connecting from the client, the format is just the first part of the above line: "unix://[/path/to/socket]" This code is copyright 2014 Akamai Technologies, Inc. <[email protected]>
… major function, and basic socket listening tests. This code is copyright 2014 Akamai Technologies, Inc. <[email protected]>
…nctions to be cleaner. Start of runtime tests. This code is copyright 2014 Akamai Technologies, Inc. <[email protected]>
…ry to not make assumptions about the listening socket; all RPC tests still pass. Still to do: Unix socket HTTP test. This code is copyright 2014 Akamai Technologies, Inc. <[email protected]>
also required making some hardcoded values into more generic functionality, which is generally a good thing. I verified that each test function that I modified still passed.: This code is copyright 2014 Akamai Technologies, Inc. <[email protected]>
|
@jefferai I'll take a look at this in just a bit, thanks! |
|
|
||
| req.URL.Host = r.url.Host | ||
| req.URL.Scheme = r.url.Scheme | ||
| req.Host = r.url.Host | ||
|
|
||
| // Setup auth | ||
| if err == nil && r.config.HttpAuth != nil { |
There was a problem hiding this comment.
We can probably remove the nil part of this check now that we have already checked it / returned if non-nil above.
|
@jefferai I went through and added some comments about various things. Overall looking really good! I am wondering about the format of the UNIX socket URI since it includes |
items pointed out in the code review This code is copyright 2014 Akamai Technologies, Inc. <[email protected]>
|
@ryanuber OK, I've gone through the issues above. A typical UNIX socket path is unix:///path/to/socket/file. The user/group/mode are necessary parts of the specification though. The reason it's in that form is compatibility with the current configuration, which is a string. Most other applications have the owner/group/mode as separate configuration items. The only way to do it the "normal" way would be to change the configuration file format, or at a minimum add several more values for each of the rpc and http addresses, either in a sub-object or as extra optional keys. When I proposed this on the mailing list (https://groups.google.com/forum/#!topic/consul-tool/9m4iKt6PTgE) I got no indication from either armon or other users that this method was unsatisfactory, and it retains compatibility. I'm fine with a change to the configuration syntax instead, but that would require a separate discussion with more parties involved, I think. |
|
@jefferai I've read through the comments, thanks for elaborating on the implementation. I'm going to merge this in and update a few minor stylistic things, but I think the functionality is there. I'll link you to the changeset afterwards. Thanks for doing this! 👍 |
RPC and HTTP interfaces fully generically-sockified so Unix is supported...
....
Client works for RPC; will honor CONSUL_RPC_ADDR. HTTP works via consul/api;
honors CONSUL_HTTP_ADDR.
The format of a Unix socket in configuration data is:
"unix://[/path/to/socket];[username or uid];[gid];[mode]"
Obviously, the user must have appropriate permissions to create the socket
file in the given path and assign the requested uid/gid. Also note that Go does
not support gid lookups from group name, so gid must be numeric. See
https://codereview.appspot.com/101310044
When connecting from the client, the format is just the first part of the
above line:
"unix://[/path/to/socket]"
This code is copyright 2014 Akamai Technologies, Inc. [email protected]