Transport Client

[aligeti]

Client library that can find IP and port from node ids (implement dhclient and netclient)
PivotalTracker ID#158238491

---

This change is 

[aligeti]

Created a pull request and initial devlopment setup for the pivotal tracker ID#158238491

[kaloyan-raev]

@aligeti The ticket in PivotalTracker is about creating a client library that implements the netclient interface. I don't see anything like that in this PR. In contrast, I see some code related to the Minio Gateway that is not in scope of this ticket.

[kaloyan-raev]

Review status: 0 of 10 files reviewed, 8 unresolved discussions (waiting on @aligeti)

---

pkg/clients/netclient/iface.go, line 12 at r8 (raw file):

	"go.uber.org/zap"
	"google.golang.org/grpc"
	"storj.io/storj/pkg/dtypes"

Add an empty line before the storj.io import group.

---

pkg/clients/netclient/iface.go, line 15 at r8 (raw file):

)

// NetClient defines the interface to an overlay client.

This is not an interface to an overlay client, but to a network client.

---

pkg/clients/netclient/iface.go, line 18 at r8 (raw file):

type NetClient interface {
	DialUnauthenticated(ctx context.Context, address dtypes.Address) (*grpc.ClientConn, error)
	DialNode(ctx context.Context, nodeID dtypes.Node) (*grpc.ClientConn, error)

The nodeID should be of type NodeID instead of Node. This is what the light client will get from the Network State Pointer before calling the NetClient.

---

pkg/clients/netclient/iface.go, line 21 at r8 (raw file):

}

// Overlay is the overlay concrete implementation of the client interface

There is nothing related to overlay in this type.

---

pkg/clients/netclient/iface.go, line 22 at r8 (raw file):

// Overlay is the overlay concrete implementation of the client interface
type storjClient struct {

netClient would be a better name for the type.

---

pkg/clients/netclient/iface.go, line 23 at r8 (raw file):

// Overlay is the overlay concrete implementation of the client interface
type storjClient struct {
	nodeID dtypes.Node

Why do you need to store the nodeID here?

---

pkg/clients/netclient/iface.go, line 30 at r8 (raw file):

func (o *storjClient) DialNode(ctx context.Context, nodeID dtypes.Node) (*grpc.ClientConn, error) {
	/* TODO@ASK: call the DHT functions to open up a connection to the DHT (cache) servers */
	conn, err := grpc.Dial((nodeID.Address.Network + ":" + strconv.Itoa(nodeID.Address.Port)), grpc.WithInsecure())

You can't actually do it just like that. As commented above the nodeID is of type NodeID, but not Node. So you don't have access to the node's Address directly from the NodeID. You need to Lookup the Node struct from the Overlay Client (PR #91) passing the NodeID you have.

---

pkg/dtypes/common.go, line 4 at r8 (raw file):

// See LICENSE for copying information.

package dtypes

You should not define this types yourself in this PR. Instead you should use the Node, NodeID and Address types defined by the Overlay Client (PR #91).

---

Comments from Reviewable

[jtolio]

Review status: 0 of 10 files reviewed, 10 unresolved discussions (waiting on @aligeti)

---

pkg/clients/netclient/iface.go, line 18 at r8 (raw file):

Previously, kaloyan-raev (Kaloyan Raev) wrote…

The nodeID should be of type NodeID instead of Node. This is what the light client will get from the Network State Pointer before calling the NetClient.

actually! it should be of type dtypes.Node. here's why:

the DHT cache is going to return a list of 40 nodes to talk to. for each of the 40 nodes, we're going to call DialNode. If we only pass an id and not a full dtypes.Node, we don't have an address, and we have to look it up again, which means talking to the DHT cache 40 more times. shoot!

if we use a dtypes.Node, if the address is filled in, we'll use the provided address and confirm it's the right node id before returning the ClientConn (the confirmation thing is something bryan is working on). If the address is not filled in and types.Node only has a NodeID, then we have to look up the address.

i should have left a comment about this. sorry!

---

pkg/miniogw/gateway-storj.go, line 18 at r8 (raw file):

	"time"

	"storj.io/storj/pkg/overlay"

would it be okay if we kept all of the changes in this file in a separate review?

---

pkg/overlay/client_test.go, line 1 at r8 (raw file):

// Copyright (C) 2018 Storj Labs, Inc.

can you leave dennis's commits out of this review as well?

---

Comments from Reviewable

[kaloyan-raev]

Review status: 0 of 10 files reviewed, 10 unresolved discussions (waiting on @aligeti)

---

pkg/clients/netclient/iface.go, line 18 at r8 (raw file):

Previously, jtolds (JT Olio) wrote…

actually! it should be of type dtypes.Node. here's why:

the DHT cache is going to return a list of 40 nodes to talk to. for each of the 40 nodes, we're going to call DialNode. If we only pass an id and not a full dtypes.Node, we don't have an address, and we have to look it up again, which means talking to the DHT cache 40 more times. shoot!

if we use a dtypes.Node, if the address is filled in, we'll use the provided address and confirm it's the right node id before returning the ClientConn (the confirmation thing is something bryan is working on). If the address is not filled in and types.Node only has a NodeID, then we have to look up the address.

i should have left a comment about this. sorry!

Well, it seems I misunderstood the workflow. I thought that the NetClient will call the DHT cache as necessary instead of the caller looking up the Node from the DHT cache before calling NetClient's Dial.

In any case, my concern was that the name of the parameter does not reflect its type. If the type should be Node then the param name should be node instead of nodeID.

---

Comments from Reviewable

[jtolio]

Review status: 0 of 10 files reviewed, 10 unresolved discussions (waiting on @aligeti)

---

pkg/clients/netclient/iface.go, line 18 at r8 (raw file):

Previously, kaloyan-raev (Kaloyan Raev) wrote…

Well, it seems I misunderstood the workflow. I thought that the NetClient will call the DHT cache as necessary instead of the caller looking up the Node from the DHT cache before calling NetClient's Dial.

In any case, my concern was that the name of the parameter does not reflect its type. If the type should be Node then the param name should be node instead of nodeID.

No no, you're right, that is the workflow. The NetClient will call the DHT cache as necessary! When you're dialing a specific node, this will do the lookup if you don't already have the address.

There's one side workflow which is weird that this is optimizing for is when someone else has already asked the DHT cache for 40 random nodes. You say "give me 40 nodes, I don't care about the ids", and it gives you back 40 nodes. Those 40 nodes, if they have addresses, can give the address to the DialNode method to save a lookup.

---

Comments from Reviewable

[coyle]

Reviewed 1 of 6 files at r10, 1 of 3 files at r11, 2 of 5 files at r14, 3 of 3 files at r16.
Review status: all files reviewed, 11 unresolved discussions (waiting on @jtolds)

---

Comments from Reviewable

[coyle]

---

Review status: all files reviewed, 11 unresolved discussions (waiting on @jtolds)

---

Comments from Reviewable

[kaloyan-raev]

Review status: all files reviewed, 14 unresolved discussions (waiting on @jtolds and @aligeti)

---

pkg/transport/transport.go, line 32 at r16 (raw file):

	if node.Address == nil || node.Address.Address == "" {
		return nil, errors.New("No Address")

We defined an error class above, so we should use it when creating new errors.

---

pkg/transport/client.go, line 18 at r16 (raw file):

var (
	mon   = monkit.Package()
	Error = errs.Class("error")

We should initialize the error class with "transport error", not just "error", so we can distinguish it from other error classes.

---

pkg/transport/client.go, line 23 at r16 (raw file):

// client defines the interface to an network client.
type Client interface {
	DialUnauthenticated(ctx context.Context, node proto.Node) (*grpc.ClientConn, error)

This method should take NodeAddress, not Node.

I remember from the discussion on Wednesday that DialUnauthenticated just dials the provided address, while DialNode will in addition (in future, when Bryan add some security stuff) check that the client dialed the expected node. And this is why DialNode takes a Node, but DialUnathenticated only a NodeAddress.

---

Comments from Reviewable

[aligeti]

Review status: all files reviewed, 14 unresolved discussions (waiting on @jtolds and @aligeti)

---

pkg/transport/transport.go, line 32 at r16 (raw file):

Previously, kaloyan-raev (Kaloyan Raev) wrote…

We defined an error class above, so we should use it when creating new errors.

Am I missing something here? That is what I am using errors.New("...") to create new errors. Let me know what you want it to be ..... Also I reviewed with Dennis. he is fine.

---

pkg/clients/connectioncache/connectioncache_iface.go, line 26 at r13 (raw file):

Previously, jtolds (JT Olio) wrote…

i don't think you need this Client thing. grpc connections are grpc connections, independent of anything else

removed the file

---

pkg/clients/connectioncache/connectioncache_impl.go, line 20 at r13 (raw file):

Previously, jtolds (JT Olio) wrote…

you don't need to do this. all the connection cache does is return *grpc.ClientConns. why do you need an overlay client at all?

removed the file

---

pkg/clients/connectioncache/connectioncache_impl.go, line 26 at r13 (raw file):

Previously, jtolds (JT Olio) wrote…

you do need a transport client though

Done.

---

pkg/clients/connectioncache/connectioncache_impl.go, line 51 at r13 (raw file):

Previously, jtolds (JT Olio) wrote…

you should also be able to unindent a bunch of this logic by getting rid of elses. if you return in an if, you don't need an else!

Done.

---

pkg/clients/connectioncache/connectioncache_impl.go, line 74 at r13 (raw file):

Previously, jtolds (JT Olio) wrote…

else unneeded

Done.

---

pkg/transport/client.go, line 18 at r16 (raw file):

Previously, kaloyan-raev (Kaloyan Raev) wrote…

We should initialize the error class with "transport error", not just "error", so we can distinguish it from other error classes.

Will look into it ...

---

pkg/transport/client.go, line 23 at r16 (raw file):

Previously, kaloyan-raev (Kaloyan Raev) wrote…

This method should take NodeAddress, not Node.

I remember from the discussion on Wednesday that DialUnauthenticated just dials the provided address, while DialNode will in addition (in future, when Bryan add some security stuff) check that the client dialed the expected node. And this is why DialNode takes a Node, but DialUnathenticated only a NodeAddress.

kaloyan, there are lot of things unknown....we need to have a framework in place and thing will change as we figure out the stuff. Should it be a NodeAddress or NodeID or pointer... will take shape once we start putting all pieces together. So for now I will hold on to what it is there now.

If you see any error in the code implemented, I would be happy to change.

---

pkg/clients/transportclient/transportclient.go, line 4 at r12 (raw file):

Previously, coyle (Dennis Coyle) wrote…

still needs to be moved

renamed/moved as suggested

---

pkg/clients/transportclient/transportclient.go, line 17 at r13 (raw file):

Previously, kaloyan-raev (Kaloyan Raev) wrote…

The second parameter should be node proto.Node instead of addr string.

Done.

---

pkg/clients/transportclient/transportclient_iface.go, line 4 at r15 (raw file):

Previously, coyle (Dennis Coyle) wrote…

This is not the correct place for this. We should create a pkg/transport and place it in a file called client.go and rename the interface to Client

Done...

---

pkg/clients/transportclient/transportclient_impl.go, line 17 at r14 (raw file):

Previously, aligeti wrote…

used proto.Node

Done.

---

pkg/clients/transportclient/transportclient_impl.go, line 23 at r14 (raw file):

Previously, aligeti wrote…

changed as suggested

Done.

---

pkg/clients/transportclient/transportclient_impl.go, line 32 at r14 (raw file):

Previously, aligeti wrote…

removed the error/retry attempt code

Done.

---

pkg/clients/transportclient/transportclient_impl.go, line 10 at r15 (raw file):

Previously, coyle (Dennis Coyle) wrote…

there needs to be a line separator

Done.

---

pkg/clients/transportclient/transportclient_impl.go, line 22 at r15 (raw file):

Previously, coyle (Dennis Coyle) wrote…

you should first check that node.Address is not nil before indexing off it

Done.

---

Comments from Reviewable

[aligeti]

Review status: all files reviewed, 14 unresolved discussions (waiting on @jtolds and @kaloyan-raev)

---

pkg/transport/client.go, line 18 at r16 (raw file):

Previously, aligeti wrote…

Will look into it ...

Done.

---

pkg/transport/client.go, line 23 at r16 (raw file):

Previously, aligeti wrote…

kaloyan, there are lot of things unknown....we need to have a framework in place and thing will change as we figure out the stuff. Should it be a NodeAddress or NodeID or pointer... will take shape once we start putting all pieces together. So for now I will hold on to what it is there now.

If you see any error in the code implemented, I would be happy to change.

Done.

---

Comments from Reviewable

[aligeti]

Review status: all files reviewed, 14 unresolved discussions (waiting on @jtolds and @kaloyan-raev)

---

pkg/transport/transport.go, line 32 at r16 (raw file):

Previously, aligeti wrote…

Am I missing something here? That is what I am using errors.New("...") to create new errors. Let me know what you want it to be ..... Also I reviewed with Dennis. he is fine.

Done.

---

pkg/clients/connectioncache/connectioncache_iface.go, line 26 at r13 (raw file):

Previously, aligeti wrote…

removed the file

Done.

---

pkg/clients/connectioncache/connectioncache_impl.go, line 20 at r13 (raw file):

Previously, aligeti wrote…

removed the file

Done.

---

Comments from Reviewable

[kaloyan-raev]

Review status: all files reviewed, 15 unresolved discussions (waiting on @jtolds, @kaloyan-raev, and @aligeti)

---

pkg/transport/transport.go, line 15 at r16 (raw file):

)

// transportClient is the concrete implementation of the networkclient interface

Is this doc comment saying anything meaningful? At least I cannot understand it.

---

pkg/transport/transport.go, line 32 at r16 (raw file):

Previously, aligeti wrote…

Done.

errors.New() creates a raw error. You've defined a zeebo error class in the client.go file, so I guess the intention was to use it in places like this one?

---

pkg/transport/client.go, line 18 at r16 (raw file):

Previously, aligeti wrote…

Done.

I don't see any change you've done here. Why marking it as Done?

---

pkg/transport/client.go, line 23 at r16 (raw file):

Previously, aligeti wrote…

Done.

We've discussed this interface thoroughly on the Wednesday meeting and I think everyone agreed we now have a very clear understanding of it. If you still think there is any uncertainty then I suggest to simply follow the original definition of the interface from #86 instead of changing it for no reason. Interfaces are important contracts between components and we should define them with care.

---

Comments from Reviewable

[aligeti]

Review status: 0 of 3 files reviewed, 15 unresolved discussions (waiting on @coyle, @jtolds, @aligeti, and @kaloyan-raev)

---

pkg/transport/transport.go, line 15 at r16 (raw file):

Previously, kaloyan-raev (Kaloyan Raev) wrote…

Is this doc comment saying anything meaningful? At least I cannot understand it.

Changed the comment

---

pkg/transport/transport.go, line 32 at r16 (raw file):

Previously, kaloyan-raev (Kaloyan Raev) wrote…

errors.New() creates a raw error. You've defined a zeebo error class in the client.go file, so I guess the intention was to use it in places like this one?

modified using zeebos

---

Comments from Reviewable

[kaloyan-raev]

Review status: 0 of 3 files reviewed, 13 unresolved discussions (waiting on @coyle, @jtolds, @kaloyan-raev, and @aligeti)

---

pkg/transport/transport.go, line 15 at r16 (raw file):

Previously, aligeti wrote…

Changed the comment

There is no networkclient interface. Perhaps it should be transport.Client.

---

pkg/transport/client.go, line 22 at r17 (raw file):

)

// Client defines the interface to an network client.

There is no network client. We call it transport client now.

---

Comments from Reviewable

[aligeti]

Review status: 0 of 3 files reviewed, 13 unresolved discussions (waiting on @coyle, @jtolds, @kaloyan-raev, and @aligeti)

---

pkg/transport/transport.go, line 15 at r16 (raw file):

Previously, kaloyan-raev (Kaloyan Raev) wrote…

There is no networkclient interface. Perhaps it should be transport.Client.

Done.

---

Comments from Reviewable

[aligeti]

Review status: 0 of 3 files reviewed, 13 unresolved discussions (waiting on @coyle, @jtolds, and @kaloyan-raev)

---

pkg/transport/client.go, line 22 at r17 (raw file):

Previously, kaloyan-raev (Kaloyan Raev) wrote…

There is no network client. We call it transport client now.

Done.

---

Comments from Reviewable

[kaloyan-raev]

Review status: 0 of 3 files reviewed, 12 unresolved discussions (waiting on @coyle, @jtolds, and @kaloyan-raev)

---

pkg/transport/transport.go, line 15 at r16 (raw file):

Previously, aligeti wrote…

Done.

It still says "networkclient interface".

---

Comments from Reviewable

[kaloyan-raev]

---

Reviewed 1 of 6 files at r10, 1 of 3 files at r11, 2 of 5 files at r14, 1 of 3 files at r16, 1 of 3 files at r17, 2 of 2 files at r18.
Review status: all files reviewed, 11 unresolved discussions (waiting on @jtolds)

---

Comments from Reviewable