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

Projects
None yet
4 participants
@aaronlehmann
Collaborator

aaronlehmann commented Dec 21, 2016

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

Expose needed services on the control socket
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>

@aaronlehmann aaronlehmann referenced this pull request Dec 21, 2016

Merged

Allow managers not to expose a remote API port #1826

6 of 6 tasks complete
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Dec 21, 2016

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

codecov-io commented Dec 21, 2016

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

This comment has been minimized.

Show comment
Hide comment
@cyli

cyli Jan 3, 2017

Contributor

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.

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

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Jan 3, 2017

Collaborator

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.

Collaborator

aaronlehmann commented Jan 3, 2017

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

This comment has been minimized.

Show comment
Hide comment
@cyli

cyli Jan 3, 2017

Contributor

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!

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}

This comment has been minimized.

@LK4D4

LK4D4 Jan 9, 2017

Contributor

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

@LK4D4

LK4D4 Jan 9, 2017

Contributor

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

This comment has been minimized.

@aaronlehmann

aaronlehmann Jan 9, 2017

Collaborator

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?

@aaronlehmann

aaronlehmann Jan 9, 2017

Collaborator

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?

This comment has been minimized.

@LK4D4

LK4D4 Jan 9, 2017

Contributor

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

@LK4D4

LK4D4 Jan 9, 2017

Contributor

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

@LK4D4

LK4D4 approved these changes Jan 9, 2017

LGTM

@LK4D4 LK4D4 merged commit 80c66b0 into docker:master Jan 9, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/project 54.58% (target 0.00%)
Details
dco-signed All commits are signed

@aaronlehmann aaronlehmann deleted the aaronlehmann:expose-services-on-control-socket branch Jan 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment