xrootd/{client,protocol}: add protocol request #195

EgorMatirov · 2018-05-21T17:57:47Z

Add Protocol request which queries for such info as:

Protocol version
Server type
Security version
Security options
Security level
Security overrides, that customize predefined security level

Information about security level and security overrides will further be used for signing requests when it's required by the server.

Client clean-up in the mock test is a bit awful due to not decided (yet) way of closing.
Response parsing is a bit awful too, since it copies data twice - from slice to the structs and then assembles these structs in more complicated response.

I want to make a generator of Marshal\Unmarshal functions and handle bit-wise encoding and support of nested structs, slice length and so on.
Something like:

type ProtocolInfo struct {
	BinaryProtocolVersion int32 `order 0`
	ServerType            protocol.ServerType // Haven't come up with idea for enums yet.
	IsManager             bool `order 4 bit 0x00000002`
	IsServer              bool `order 4 bit 0x00000001`
	IsMeta                bool `order 4 bit 0x00000100`
	IsProxy               bool `order 4 bit 0x00000200`
	IsSupervisor          bool `order 4 bit 0x00000400`
	SecurityVersion       byte `order 10 optional` // Note that it is 10 due to 2 preceeding fields of int32 (4+4) and two reserved fields of byte (1+1)
	ForceSecurity         bool `order 11 bit 0x02 optional`
	SecurityLevel         byte `order 12 optional`
	SecurityOverridesSize byte `order 13 optional`
	SecurityOverrides     []protocolRequest.SecurityOverride `order 14 size SecurityOverridesSize`
}

Since I'm unsure that it is worth keeping a runtime-based Marshal\Unmarshal after generator will be written, I haven't added these features in it as part of that PR (or before that PR).

Regarding implementing the generator - I would like to deal with few complicated request before that to have better idea about possible features, if it is ok.

sbinet

just a first pass.
I haven't completely reviewed the tests.

sbinet · 2018-05-22T13:00:53Z

xrootd/client/protocol_mock_test.go

+	}
+
+	var want = ProtocolInfo{
+		protocolVersion,


please add struct fields names here:

want := ProtocolInfo{ BinaryProtocolVersion: protocolVersion, ... }

sbinet · 2018-05-22T13:03:46Z

xrootd/client/protocol.go

+
+	"go-hep.org/x/hep/xrootd/encoder"
+	"go-hep.org/x/hep/xrootd/protocol"
+	protocolRequest "go-hep.org/x/hep/xrootd/protocol/protocol"


package names are usually not "camel-cased".
could we think about a better name in this unfortunate name collision?

xproto ? protoreq ?

or name the other import like:

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

?

sbinet · 2018-05-22T13:07:58Z

xrootd/client/protocol.go

+	var securityInfo *protocolRequest.SecurityInfo
+	if len(resp) > protocolRequest.GeneralResponseLength {
+		securityInfo = &protocolRequest.SecurityInfo{}
+		err = encoder.Unmarshal(resp[protocolRequest.GeneralResponseLength:], securityInfo)


it seems we are dropping this error on the floor.
shouldn't we return it here ?

sbinet · 2018-05-22T13:10:26Z

xrootd/client/protocol.go

+		err = encoder.Unmarshal(resp[protocolRequest.GeneralResponseLength:], securityInfo)
+	}
+
+	var info = ProtocolInfo{}


it seems we could create a ProtocolInfo value with much of its fields already initialized to the correct values, like so:

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

sbinet · 2018-05-22T13:13:38Z

xrootd/client/protocol_test.go

+
+func TestClient_Protocol(t *testing.T) {
+	var want = ProtocolInfo{
+		784,


please use struct field names here as well.

sbinet · 2018-05-22T13:16:30Z

xrootd/protocol/protocol/protocol.go

+*/
+package protocol // import "go-hep.org/x/hep/xrootd/protocol/protocol"
+
+// RequestID is the id of the request, it is sent as part of message. See xrootd protocol specification for details.


See xrootd protocol specification for details.

please add either a link and/or the name of the section that deals with these details.

sbinet · 2018-05-22T13:21:06Z

xrootd/protocol/protocol/protocol.go

+// Request is the struct that holds protocol request parameters.
+type Request struct {
+	ClientProtocolVersion int32
+	Reserved1             [11]byte


if it works with the current reflect-based code of the marshaling/unmarshaling protocol, we should rename these ReservedX fields into just _.

if this doesn't work with the reflect-based code, then leave it like that but add a FIXME stating we should rename these into _ when the automatically generated (un)marshaling code is in place.

sbinet · 2018-05-22T13:22:14Z

xrootd/protocol/protocol/protocol.go

+	if withSecurityRequirements {
+		options |= ReturnSecurityRequirements
+	}
+	return Request{protocolVersion, [11]byte{}, options, 0}


please use struct field names here.
you can drop the ReservedX fields:

return Request{ClientProtocolVersion: protocolVersion, Options:options}

EgorMatirov · 2018-05-27T14:27:21Z

@sbinet, PTAL.

The problem with that request was in the difference between specification and actual implementation:

// NOTE: That request differs from one specified in http://xrootd.org/doc/dev45/XRdv310.pdf, p. 71:
// order of Options and Reserved fields differs in the protocol specification and
// actual implementation which can be found at https://github.com/xrootd/xrootd/blob/master/src/XProtocol/XProtocol.hh
// (see ClientProtocolRequest definition).
type Request struct {
	ClientProtocolVersion int32
	Options               RequestOptions
	// FIXME: Rename Reserved* fields to _ when automatically generated (un)marshalling will be available.
	Reserved1 [11]byte
	Reserved2 int32
}

Right now, I used the order of implementation and added a NOTE:.
Should I do something else? (Probably create an issue for xrootd?)

sbinet · 2018-05-27T16:08:30Z

Yes I guess we should file an issue against xrootd. (Probably to change the specs to match the implementation :P)

sbinet

this should probably be rebased on top of 21c1f45

sbinet · 2018-05-28T06:49:45Z

xrootd/client/protocol_mock_test.go

+	serverFunc := func(cancel func(), conn net.Conn) {
+		data, err := readRequest(conn)
+		if err != nil {
+			defer cancel()


wouldn't be possible to just put one defer cancel() at the top of the function ? (instead of all these scattered defers?)

sbinet · 2018-05-28T06:51:07Z

xrootd/client/protocol_test.go

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


s/cannon/could not/

sbinet · 2018-05-28T06:52:26Z

xrootd/protocol/protocol/protocol.go

+Package protocol contains the structures describing request and response
+for protocol request (see XRootD specification).
+
+Response consist of 3 parts:


s/Reponse consist/A response consists/

sbinet · 2018-05-28T12:14:38Z

xrootd/protocol/signRequirements.go

@@ -0,0 +1,59 @@
+// Copyright 2018 The go-hep Authors.  All rights reserved.


by convention, we don't use very much camelCase names for files.
I would either rename signRequirements.go into signing.go or signed_requirements.go.

or, perhaps, just merge the content of that file at the bottom of protocol.go ?

I would like not to put it into protocol.go because I expect a lot of "configuration" code to be placed here to initialize the predefined levels. I'm fine with signing.go. :)

sbinet · 2018-05-28T12:15:10Z

xrootd/protocol/signRequirements.go

+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Package protocol contains the XRootD protocol specific types.


please remove this package-level comment as it may override the much nicer one that already exists in protocol.go.

sbinet · 2018-05-28T12:35:24Z

xrootd/protocol/protocol/protocol.go

+2) SecurityInfo - a response part that is added to general response
+if `ReturnSecurityRequirements` is provided and server supports it.
+It contains security version, security options, security level,
+and number of following security overrides, if any.


s/and number/and the number/

sbinet · 2018-05-28T12:35:59Z

xrootd/protocol/protocol/protocol.go

+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+/*


please use // instead of /* ... */ (for consistency's sake.)

sbinet · 2018-05-28T12:37:41Z

xrootd/protocol/protocol/protocol.go

+type Flags int32
+
+const (
+	// IsServer indicates whether this server has server role.


this whole block could be rewritten to look like:

const ( IsServer Flags = 0x00000001 // IsServer indicates whether this server has server role. IsManager Flags = 0x00000002 // ... ... )

sbinet · 2018-05-28T12:40:58Z

xrootd/protocol/protocol/protocol.go

+// SecurityOverrideLength is the length of SecurityOverride in bytes.
+const SecurityOverrideLength = 2
+
+// SecurityOverride is a alteration needed to the specified predefined security level.


s/is a/is an/

sbinet · 2018-05-28T12:41:43Z

xrootd/protocol/protocol/protocol.go

+const SecurityOverrideLength = 2
+
+// SecurityOverride is a alteration needed to the specified predefined security level.
+// It consist of the request index and the security requirement that associated request is to have.


s/It consist/It consists/
s/that associated/the associated/
s/is to have/should have/

sbinet · 2018-05-28T12:59:00Z

I've filed xrootd/xrootd#717

EgorMatirov · 2018-05-28T14:02:27Z

Thank you very much for the review!

I have rebased on top of the master and added suggested changes.

sbinet

probably this PR should also Updates go-hep/hep#170 (as it implements kXR_protocol)
(also, probably #171 should be closed, right?)

sbinet · 2018-05-28T14:48:59Z

xrootd/client/client.go

 	}

-	client := &Client{cancel, conn, mux.New(), 0}
+	client := &Client{cancel: cancel, conn: conn, mux: mux.New(), signRequirements: protocol.DefaultSignRequirements()}


could we get away with just creating the zero value of protocol.SignRequirements ?
it seems client.signRequirements is overwritten at line 73, below.

Yep, you are right.

sbinet · 2018-05-28T15:38:47Z

xrootd/client/protocol.go

+// 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) {


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 :)

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?

sbinet · 2018-05-29T09:02:15Z

(moving the discussion to the main thread)

I agree it's more verbose from the Client user.
and I agree we should devise something a bit more easy on the keyboard as a user-facing API.
I also agree having everything exploded into many small-ish packages (for the req/resp) doesn't help discoverability (although, that can be somewhat mitigated with examples).

but, perhaps Client isn't the best place to hang Dirlist?
what about a few helper types that regroup functionalities by "themes"?

Dirlist is clearly something related to a filesystem.
kXR_rm, kXR_rmdir, kXR_chmod, (to name a few) as well.
while kXR_read, kXR_readv and kXR_write are more related to a (remote) file.

what about, then:

type File struct {
    c *xrdclient.Client
    name string
}

func (f *File) Readdirnames(n int) ([]string, error) {
    var resp dirlist.Response
    err := f.c.Send(ctx, &resp, dirlist.NewRequest(f.name))
    return resp.Dirs, err
}

EgorMatirov · 2018-05-29T09:51:16Z

I like that idea.

There is a number of specific requests like:

kXR_admin
kXR_auth
kXR_bind
kXR_login
kXR_ping
kXR_protocol
kXR_query
kXR_set

that are more related to a "session" and they probably can reside inside Client.
I'm not sure, but probably most of them are so specific that they will be called only during Client connection routine (protocol, login, auth, bind) and at some point we can decide even unexport them.

Something like that for example:

type Client struct {
    // connection-wise info
}

func (c *Client) Ping(ctx context.Context) error {
	// ...
}

type Filesystem struct {
    c *Client
}

func (f *Filesystem) Dirlist(ctx context.Context) ([]string, error) {
    // ...
}

type File struct {
    c *Client
    path string
}

func NewFile(client *Client, path string) File {
    return File{client, path}
}

func (f *File) Read(ctx context.Context, offset int64, length int32) ([]byte, error) {
    // ...
}

However, Read bothers me because of "readahead list" (see protocol specification, p. 99-100). It looks a bit awful to call Read on the specific file and pass a list of readaheads to be done for other files.
Do you know how wide this feature is used (just in case)? I mean, if it is more "specific" scenario, it is ok to leave it to more "low-level" routine:

var resp read.Response
err := client.Send(ctx, &resp, read.NewRequest(...))

Anyway, even with that additional argument in Read call I really like that approach.

sbinet · 2018-05-29T10:14:05Z

wrt kXR_admin, ... kXR_set: agreed. they probably are bound to either Client or Session.

(just nitpicking but:

func (fs *Filesystem) Open(path string) (*File, error) { ... }
func (fs *Filesystem) Create(path string) (*File, error) { ... }
func (fs *Filesystem) OpenFile(path string, flag int, perm os.FileMode) (*File, error) { ... }

(ie: I wouldn't have NewFile as a function)

I think for the readahead list, we could as a first pass, just discard that feature.
eventually, we would send the read request to the client for that file, and have the client gather all these requests and merge into a single one with all the readaheads for all these files.

but we can table this for now :)

EgorMatirov · 2018-05-29T12:00:38Z

@sbinet, sorry, what about merging this one? It looks like that we agree that it can be placed in Client for now.

I have updated PR with the fixes of other issues (somehow github managed to put that commit above our conversation, so I'm unsure if you noticed it). Commit messsage now includes "Updates #170" too.

Regarding Client \ Filesystem \ File separation: I'll make that when I'll make a PRs for the corresponding requests.

sbinet

LGTM.

(modulo once small last nitpick)

sbinet · 2018-05-29T12:10:04Z

xrootd/protocol/protocol/protocol.go

+)
+
+// Request holds protocol request parameters.
+// NOTE: That request differs from one specified in http://xrootd.org/doc/dev45/XRdv310.pdf, p. 71:


the specs have been "fixed" to follow what's in the code :)
please remove.

Protocol request is used for obtaining the protocol version number, type of server and possible security requirements. These security requirements determine which requests should be signed. This info is provided to Client via protocol.SignRequirements. The Client will use sr.Needed to determine if the request should be signed via Sigver request. Actual sign requirements depending on the security level will be added as part of the request implementation in the following form: if level >= protocol.Pedantic { sr.requirements[dirlist.RequestID] = protocol.SignNeeded // ... } Updates go-hep#170.

sbinet · 2018-05-29T13:14:32Z

FYI, the test failure in master is coming from:

cmd/vet: "cannot import, possibly version skew" regression golang/go#25596
go/internal/gcimporter: error with embedded non-interface (possibly an incomplete type) golang/go#25615

sbinet reviewed May 22, 2018

View reviewed changes

sbinet mentioned this pull request May 25, 2018

xrootd/internal/mux: migrate from xrootd/mux #198

Merged

EgorMatirov force-pushed the xrootd-protocol branch from 35f6d09 to 922d570 Compare May 27, 2018 14:21

sbinet reviewed May 28, 2018

View reviewed changes

sbinet mentioned this pull request May 28, 2018

specs: ClientProtocolRequest binary layout not implemented as specs xrootd/xrootd#717

Closed

EgorMatirov force-pushed the xrootd-protocol branch from 922d570 to a5167b8 Compare May 28, 2018 13:59

sbinet reviewed May 28, 2018

View reviewed changes

EgorMatirov force-pushed the xrootd-protocol branch from a5167b8 to 6356063 Compare May 29, 2018 10:30

sbinet approved these changes May 29, 2018

View reviewed changes

EgorMatirov force-pushed the xrootd-protocol branch from 6356063 to c418617 Compare May 29, 2018 12:17

sbinet merged commit 849df9e into go-hep:master May 29, 2018

EgorMatirov deleted the xrootd-protocol branch May 29, 2018 13:54

EgorMatirov mentioned this pull request May 29, 2018

xrootd/client: fix test #206

Merged

		@@ -0,0 +1,59 @@
		// Copyright 2018 The go-hep Authors. All rights reserved.

xrootd/{client,protocol}: add protocol request #195

xrootd/{client,protocol}: add protocol request #195

Uh oh!

Conversation

EgorMatirov commented May 21, 2018

Uh oh!

sbinet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EgorMatirov commented May 27, 2018

Uh oh!

sbinet commented May 27, 2018

Uh oh!

sbinet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbinet commented May 28, 2018

Uh oh!

EgorMatirov commented May 28, 2018

Uh oh!

sbinet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EgorMatirov May 28, 2018 • edited by sbinet Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbinet commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EgorMatirov commented May 29, 2018

Uh oh!

sbinet commented May 29, 2018

Uh oh!

EgorMatirov May 28, 2018 •

edited by sbinet

Loading

sbinet commented May 29, 2018 •

edited

Loading

EgorMatirov commented May 29, 2018 •

edited

Loading