Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose needed services on the control socket #1828

Merged
merged 1 commit into from
Jan 9, 2017

Conversation

aaronlehmann
Copy link
Collaborator

For swarm mode to function without exposing a TCP port, we need services
such as the dispatcher and node CA to be exposed on the control socket
(i.e. a unix socket). This commit changes the manager to expose those
services, and changes the raft proxy to inject some information into the
context when calling the handler directly that identifies the local
node. The authorization code in "ca" is updated to check for this
information on the context and make use of it, instead of returning an
error from RemoteNode. Also, the CA server now renewing a certificate
over the control socket.

This is the first part of #1826, broken out of that PR as requested.

cc @LK4D4 @cyli @diogomonica

For swarm mode to function without exposing a TCP port, we need services
such as the dispatcher and node CA to be exposed on the control socket
(i.e. a unix socket). This commit changes the manager to expose those
services, and changes the raft proxy to inject some information into the
context when calling the handler directly that identifies the local
node. The authorization code in "ca" is updated to check for this
information on the context and make use of it, instead of returning an
error from RemoteNode. Also, the CA server now renewing a certificate
over the control socket.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
@codecov-io
Copy link

Current coverage is 54.58% (diff: 28.30%)

Merging #1828 into master will increase coverage by 0.03%

@@             master      #1828   diff @@
==========================================
  Files           102        102          
  Lines         17025      17094    +69   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           9286       9330    +44   
- Misses         6599       6617    +18   
- Partials       1140       1147     +7   

Sunburst

Powered by Codecov. Last update 3c82476...8e51d74

@cyli
Copy link
Contributor

cyli commented Jan 3, 2017

With the caveat that I don't really understand the functions such as genClientServerStreamingMethod and why they needed to be modified to return a wrapper object instead (the generated code does not seem to reflect this) the rest LGTM.

@aaronlehmann
Copy link
Collaborator Author

With the caveat that I don't really understand the functions such as genClientServerStreamingMethod

This function generates Go code that wraps a client/server streaming RPC handler with a proxy. The proxy will just invoke the handler locally when called on the leader, but when it's called on a follower, it invokes the method remotely on the leader. The goal is to have certain handlers always run on the leader for consistency.

and why they needed to be modified to return a wrapper object instead

We want to tell the RPC handler that it was invoked locally. Formerly, it didn't need to know this explicitly. If it was invoked by a proxy that was forwarding the request, the proxy added metadata headers to the request, so the RPC handler knew where the request originated. And if the request happened to hit the leader without going through a level of proxying, we'd go by the TLS metadata that GRPC provides for us. But now we want these handlers to support requests that came over a unix socket, where TLS isn't in use. So we need to include some information with the node ID and so on.

We do this by injecting a custom context into the handler when it's called locally. With the simple handlers, this is trivial, because we can just call context.WithValue and replace the context that gets passed into the handler. However, stream handlers don't take a context argument, and instead call a Context method on the stream. Thus we need to wrap the stream to return our own custom context.

(the generated code does not seem to reflect this)

You should see it for stream handlers, such as SubscribeLogs. The confusion may come from this only being used for stream handlers, not simple handlers.

@cyli
Copy link
Contributor

cyli commented Jan 3, 2017

You should see it for stream handlers, such as SubscribeLogs. The confusion may come from this only being used for stream handlers, not simple handlers.

Ah ok, thanks, yes I had only checked the simple handlers, and did not check further with log broker and dispatcher.

However, stream handlers don't take a context argument, and instead call a Context method on the stream. Thus we need to wrap the stream to return our own custom context.

Ok, that makes sense, thanks for the explanation!

@@ -49,21 +49,27 @@ func (g *raftProxyGen) genProxyConstructor(s *descriptor.ServiceDescriptorProto)
md["redirect"] = append(md["redirect"], addr)
return metadata.NewContext(ctx, md), nil
}
mods := []func(context.Context)(context.Context, error){redirectChecker}
mods = append(mods, ctxMod)
remoteMods := []func(context.Context)(context.Context, error){redirectChecker}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this two blocks look inconsistent. Not opposed to any way, but I think they should look "same"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

localCtxMod is optional, but remoteCtxMod is not. I think it makes sense for remoteCtxMod not to be optional, because if it was omitted, this would make the requests forwarded by the proxy appear to come from this manager, and that could be disastrous if done by mistake.

We could make localCtxMod non-optional as well, but then all the proxy instances that are bound to TCP would have to pass an empty dummy function.

What do you think we should do here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good as it is. I mostly meant append vs creating new slice. I think it's okay, though.

Copy link
Contributor

@LK4D4 LK4D4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@LK4D4 LK4D4 merged commit 80c66b0 into moby:master Jan 9, 2017
@aaronlehmann aaronlehmann deleted the expose-services-on-control-socket branch January 9, 2017 18:53
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.

4 participants