Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions xrootd/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,11 @@ import (
// Concurrent requests are supported.
// Zero value is invalid, Client should be instantiated using NewClient.
type Client struct {
cancel context.CancelFunc
conn net.Conn
mux *mux.Mux
protocolVersion int32
cancel context.CancelFunc
conn net.Conn
mux *mux.Mux
protocolVersion int32
signRequirements protocol.SignRequirements
}

// NewClient creates a new xrootd client that connects to the given address.
Expand All @@ -54,7 +55,7 @@ func NewClient(ctx context.Context, address string) (*Client, error) {
return nil, err
}

client := &Client{cancel, conn, mux.New(), 0}
client := &Client{cancel: cancel, conn: conn, mux: mux.New()}

go client.consume(ctx)

Expand All @@ -63,6 +64,14 @@ func NewClient(ctx context.Context, address string) (*Client, error) {
return nil, err
}

protocolInfo, err := client.Protocol(ctx)
if err != nil {
client.Close()
return nil, err
}

client.signRequirements = protocol.NewSignRequirements(protocolInfo.SecurityLevel, protocolInfo.SecurityOverrides)

return client, nil
}

Expand Down
91 changes: 91 additions & 0 deletions xrootd/client/main_mock_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
// Copyright 2018 The go-hep Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

package client // import "go-hep.org/x/hep/xrootd/client"

import (
"context"
"encoding/binary"
"io"
"net"

"github.com/pkg/errors"
"go-hep.org/x/hep/xrootd/internal/mux"
"go-hep.org/x/hep/xrootd/protocol"
)

func testClientWithMockServer(serverFunc func(cancel func(), conn net.Conn), clientFunc func(cancel func(), client *Client)) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

server, conn := net.Pipe()
defer server.Close()
defer conn.Close()

client := &Client{cancel: cancel, conn: conn, mux: mux.New(), signRequirements: protocol.DefaultSignRequirements()}
defer client.Close()

go serverFunc(func() { client.Close() }, server)
go client.consume(ctx)

clientFunc(cancel, client)
}

func readRequest(conn net.Conn) ([]byte, error) {
// 16 is for the request options and 4 is for the data length
const requestSize = protocol.RequestHeaderLength + 16 + 4
var request = make([]byte, requestSize)
if _, err := io.ReadFull(conn, request); err != nil {
return nil, err
}

dataLength := binary.BigEndian.Uint32(request[protocol.RequestHeaderLength+16:])
if dataLength == 0 {
return request, nil
}

var data = make([]byte, dataLength)
if _, err := io.ReadFull(conn, data); err != nil {
return nil, err
}

return append(request, data...), nil
}

func writeResponse(conn net.Conn, data []byte) error {
n, err := conn.Write(data)
if err != nil {
return err
}
if n != len(data) {
return errors.Errorf("could not write all %d bytes: wrote %d", len(data), n)
}
return nil
}

// TODO: move marshalResponse outside of main_mock_test.go and use it for server implementation.
func marshalResponse(responseParts ...interface{}) ([]byte, error) {
var data []byte
for _, p := range responseParts {
pData, err := protocol.Marshal(p)
if err != nil {
return nil, err
}
data = append(data, pData...)
}
return data, nil
}

// TODO: move unmarshalRequest outside of main_mock_test.go and use it for server implementation.
func unmarshalRequest(data []byte, request interface{}) (protocol.RequestHeader, error) {
var header protocol.RequestHeader
if err := protocol.Unmarshal(data[:protocol.RequestHeaderLength], &header); err != nil {
return protocol.RequestHeader{}, err
}
if err := protocol.Unmarshal(data[protocol.RequestHeaderLength:], request); err != nil {
return protocol.RequestHeader{}, err
}

return header, nil
}
93 changes: 93 additions & 0 deletions xrootd/client/protocol.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
// Copyright 2018 The go-hep Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

package client // import "go-hep.org/x/hep/xrootd/client"

import (
"context"

xrdproto "go-hep.org/x/hep/xrootd/protocol"
"go-hep.org/x/hep/xrootd/protocol/protocol"
)

// ProtocolInfo is a response for the `Protocol` request. See details in the xrootd protocol specification.
type ProtocolInfo struct {
BinaryProtocolVersion int32
ServerType xrdproto.ServerType
IsManager bool
IsServer bool
IsMeta bool
IsProxy bool
IsSupervisor bool
SecurityVersion byte
ForceSecurity bool
SecurityLevel protocol.SecurityLevel
SecurityOverrides []protocol.SecurityOverride
}

// Protocol obtains the protocol version number, type of the server and security information, such as:
// the security version, the security options, the security level, and the list of alterations
// needed to the specified predefined security level.
func (client *Client) Protocol(ctx context.Context) (ProtocolInfo, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering whether Protocol should be a method on xrootd/client.Client or a function of xrootd/protocol/protocol...

if we keep Protocol a method of client.Client, then I suppose we'll have to have every single request/response as a method of client.Client as well (ie 32 methods)

I am thinking out loud here (so it may just be a completely bunker idea), but it seems we could just get away with 1 or 2 methods in client.Client and keep the rest in each of the xyz protocol packages.
it seems to me this would introduce less coupling between client.Client and each request/response.

for example:

  • xrootd/protocol/protocol:
type Request { ... }
func NewRequest(...) Request { ... }

type Response { ... } // essentially client.ProtocolInfo

func Send(ctx context.Context, cli *xrdclient.Client, req Request) (Response, error) {
    raw, err := cli.Call(ctx, req.ID(), req.Marshal(cli.Version()))
    // ...
    var resp Response
    err = xrdproto.Unmarshal(&resp)
    return resp, err
}

I thought that was what you were aiming at when you put each request-type into its own package.

that said, I am not completely happy with the Send function as it is.
Perhaps, Send should actually be a method of xrdclient.Client, but it would take a (yet to be defined) xrdproto.Request interface as argument:

func (cli *Client) Send(ctx context.Context, resp Response, req Request) error {
    raw, err := cli.marshal(req)
    recv, err := cli.call(ctx, req.ID(), raw)
    err = cli.unmarshal(raw, resp)
    return err
}

and used like so:

var resp protocol.Response
err := cli.Send(ctx, &resp, protocol.NewRequest(...))

from a user perspective, it would look better like so:

resp, err := cli.Send(ctx, protocol.NewRequest(...))

but one would have to type-assert to get the protocol.Response type...

I am still on the fence.
happy to get your input :)

Copy link
Contributor Author

@EgorMatirov EgorMatirov May 28, 2018

Choose a reason for hiding this comment

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

It sounds reasonable to me.
In fact, most of these Client.* request methods will be pretty straightforward - create request, send it, parse and return response.

However, from the user perspective it is:

dirs, err := client.Dirlist(ctx, "/tmp")

vs

var resp dirlist.Response
err := client.Send(ctx, &resp, dirlist.NewRequest("/tmp"))
dirs := resp.Dirs
// (or without predeclaration but with type-assertion)

So we have a lot more boilerplate code from user perspective IMHO.
Also, it's much easier to type "client." and get a list of code completions than be like "What was the name of that request again?".

Actually, I think, we can implement both ways:

  • After implementing a few more requests and the marshalling code generator, implement routine like this:
 raw, err := client.marshal(req)
 recv, err := client.call(...)
 err = client.unmarshal(...)
 return err
  • Make small helper methods of Client:
 func (client *Client) Dirlist(ctx context.Context, path string) ([]string, err) {
     var resp dirlist.Response
     err := client.Send(ctx, &resp, dirlist.NewRequest(path))
     if err != nil {
         return nil, err
     }
     return resp.Dirs, nil
 }

At that moment, there is something like that actually, but with more request-specific code in the Client methods due to the incompleteness of the un(marshaller).

What do you think about it?

resp, err := client.call(ctx, protocol.RequestID, protocol.NewRequest(client.protocolVersion, true))
if err != nil {
return ProtocolInfo{}, err
}

var generalResp protocol.GeneralResponse

if err = xrdproto.Unmarshal(resp, &generalResp); err != nil {
return ProtocolInfo{}, err
}

var securityInfo *protocol.SecurityInfo
if len(resp) > protocol.GeneralResponseLength {
securityInfo = &protocol.SecurityInfo{}
err = xrdproto.Unmarshal(resp[protocol.GeneralResponseLength:], securityInfo)
if err != nil {
return ProtocolInfo{}, err
}
}

var info = ProtocolInfo{
BinaryProtocolVersion: generalResp.BinaryProtocolVersion,
ServerType: extractServerType(generalResp.Flags),

// TODO: implement bit-encoded fields support in Unmarshal.
IsManager: generalResp.Flags&protocol.IsManager != 0,
IsServer: generalResp.Flags&protocol.IsServer != 0,
IsMeta: generalResp.Flags&protocol.IsMeta != 0,
IsProxy: generalResp.Flags&protocol.IsProxy != 0,
IsSupervisor: generalResp.Flags&protocol.IsSupervisor != 0,
}

if securityInfo != nil {
info.SecurityVersion = securityInfo.SecurityVersion
info.ForceSecurity = securityInfo.SecurityOptions&protocol.ForceSecurity != 0
info.SecurityLevel = securityInfo.SecurityLevel

if securityInfo.SecurityOverridesSize > 0 {
info.SecurityOverrides = make([]protocol.SecurityOverride, securityInfo.SecurityOverridesSize)

const offset = protocol.GeneralResponseLength + protocol.SecurityInfoLength
const elementSize = protocol.SecurityOverrideLength

for i := byte(0); i < securityInfo.SecurityOverridesSize; i++ {
err = xrdproto.Unmarshal(resp[offset+elementSize*int(i):], &info.SecurityOverrides[i])
if err != nil {
return ProtocolInfo{}, err
}
}
}
}

return info, nil
}

func extractServerType(flags protocol.Flags) xrdproto.ServerType {
if int32(flags)&int32(xrdproto.DataServer) != 0 {
return xrdproto.DataServer
}
return xrdproto.LoadBalancingServer
}
93 changes: 93 additions & 0 deletions xrootd/client/protocol_mock_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
// Copyright 2018 The go-hep Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

package client

import (
"context"
"net"
"reflect"
"testing"

xrdproto "go-hep.org/x/hep/xrootd/protocol"
"go-hep.org/x/hep/xrootd/protocol/protocol"
)

func TestClient_Protocol_WithSecurityInfo(t *testing.T) {
var protocolVersion int32 = 0x310

serverFunc := func(cancel func(), conn net.Conn) {
defer cancel()

data, err := readRequest(conn)
if err != nil {
t.Fatalf("could not read request: %v", err)
}

var gotRequest protocol.Request
gotHeader, err := unmarshalRequest(data, &gotRequest)
if err != nil {
t.Fatalf("could not unmarshal request: %v", err)
}

if gotHeader.RequestID != protocol.RequestID {
t.Fatalf("invalid request id was specified:\nwant = %d\ngot = %d\n", protocol.RequestID, gotHeader.RequestID)
}

if gotRequest.ClientProtocolVersion != protocolVersion {
t.Fatalf("invalid client protocol version was specified:\nwant = %d\ngot = %d\n", protocolVersion, gotRequest.ClientProtocolVersion)
}

flags := protocol.IsManager | protocol.IsServer | protocol.IsMeta | protocol.IsProxy | protocol.IsSupervisor

responseHeader := xrdproto.ResponseHeader{
StreamID: gotHeader.StreamID,
DataLength: protocol.GeneralResponseLength + protocol.SecurityInfoLength + protocol.SecurityOverrideLength,
}

protocolResponse := protocol.GeneralResponse{protocolVersion, flags}

protocolSecurityInfo := protocol.SecurityInfo{
SecurityOptions: protocol.None,
SecurityLevel: protocol.Pedantic,
SecurityOverridesSize: 1,
}

securityOverride := protocol.SecurityOverride{1, protocol.SignNeeded}

response, err := marshalResponse(responseHeader, protocolResponse, protocolSecurityInfo, securityOverride)
if err != nil {
t.Fatalf("could not marshal response: %v", err)
}

if err := writeResponse(conn, response); err != nil {
t.Fatalf("invalid write: %s", err)
}
}

var want = ProtocolInfo{
BinaryProtocolVersion: protocolVersion,
ServerType: xrdproto.DataServer,
IsManager: true,
IsServer: true,
IsMeta: true,
IsProxy: true,
IsSupervisor: true,
SecurityLevel: protocol.Pedantic,
SecurityOverrides: []protocol.SecurityOverride{{1, protocol.SignNeeded}},
}

clientFunc := func(cancel func(), client *Client) {
client.protocolVersion = protocolVersion
got, err := client.Protocol(context.Background())
if err != nil {
t.Fatalf("invalid protocol call: %v", err)
}
if !reflect.DeepEqual(got, want) {
t.Fatalf("protocol info does not match:\ngot = %v\nwant = %v", got, want)
}
}

testClientWithMockServer(serverFunc, clientFunc)
}
46 changes: 46 additions & 0 deletions xrootd/client/protocol_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// Copyright 2018 The go-hep Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

// +build xrootd_test_with_server

package client

import (
"context"
"reflect"
"testing"

"go-hep.org/x/hep/xrootd/protocol"
)

func testClient_Protocol(t *testing.T, addr string) {
var want = ProtocolInfo{
BinaryProtocolVersion: 784,
ServerType: protocol.DataServer,
IsServer: true,
}

client, err := NewClient(context.Background(), addr)
if err != nil {
t.Fatalf("could not create client: %v", err)
}

got, err := client.Protocol(context.Background())
if err != nil {
t.Fatalf("invalid protocol call: %v", err)
}
if !reflect.DeepEqual(got, want) {
t.Errorf("Client.Protocol()\ngot = %v\nwant = %v", got, want)
}

client.Close()
}

func TestClient_Protocol(t *testing.T) {
for _, addr := range testClientAddrs {
t.Run(addr, func(t *testing.T) {
testClient_Protocol(t, addr)
})
}
}
13 changes: 2 additions & 11 deletions xrootd/protocol/handshake/handshake.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,13 @@
// for handshake request (see XRootD specification).
package handshake // import "go-hep.org/x/hep/xrootd/protocol/handshake"

// ServerType is the general server type kept for compatibility
// with 2.0 protocol version (see xrootd protocol specification v3.1.0, p. 5).
type ServerType int32

const (
// LoadBalancingServer indicates whether this is a load-balancing server.
LoadBalancingServer ServerType = iota
// DataServer indicates whether this is a data server.
DataServer ServerType = iota
)
import "go-hep.org/x/hep/xrootd/protocol"

// Response is a response for the handshake request,
// which contains protocol version and server type.
type Response struct {
ProtocolVersion int32
ServerType ServerType
ServerType protocol.ServerType
}

// Request holds the handshake request parameters.
Expand Down
Loading