channelz: stage 4 - add security and socket option info

[lyuxuan]

This change is 

[menghanl]

Reviewed 10 of 20 files at r1, 4 of 7 files at r2.
Review status: 14 of 18 files reviewed at latest revision, all discussions resolved.

---

channelz/types.go, line 423 at r2 (raw file):

// Security defines the interface that security protocols should implement in order
// to provide security info to channelz.
type Security interface {

Move this to a sub package?

channelz package defines the API between the clientconn and the channelz store.
This is more like a helper function for the clientconn to get info from creds. It's not part of the API.

---

channelz/types_linux.go, line 24 at r2 (raw file):

	"syscall"

	"golang.org/x/sys/unix"

Will this import cause the build to fail on non-linux? Or just function calls will return error?

---

channelz/types_linux.go, line 30 at r2 (raw file):

// getter function to obtain info from fd.
type SocketOptionData struct {
	Linger      *unix.Linger

This field is not defined in _windows.go, but is referenced in other files (e.g. service.go), even on windows.

---

channelz/util_default.go, line 1 at r2 (raw file):

// +build !darwin,!dragonfly,!freebsd,!linux,!netbsd,!openbsd,!solaris !go1.9

Rename this file. default sounds too normal.

---

channelz/util_unix_go19.go, line 1 at r2 (raw file):

// +build darwin dragonfly freebsd linux netbsd openbsd solaris

Why do we need this build constraint?
And why is this not just windows or not?

---

channelz/util_unix_go19.go, line 29 at r2 (raw file):

// GetSocketOption gets the socket option info of the conn.
func GetSocketOption(socket interface{}) *SocketOptionData {

This is also a helper function, not part of the API.
In other words, this is not necessary for channelz, the clientconn could generate the same result without calling this function.

---

channelz/service/service.go, line 38 at r2 (raw file):

const (
	secToNano  = 1e9

Make a function to do the time conversion.

---

channelz/service/service.go, line 145 at r2 (raw file):

			LocalCertificate:  v.LocalCertificate,
			RemoteCertificate: v.RemoteCertificate,
		},

Move }'s to one line

---

credentials/credentials.go, line 35 at r2 (raw file):

	"google.golang.org/grpc/channelz"

Delete blank line.

---

credentials/credentials.go, line 172 at r2 (raw file):

		return nil, nil, ctx.Err()
	}
	return tlsConn{conn, rawConn}, TLSInfo{conn.ConnectionState()}, nil

Names of the fields.

---

credentials/credentials_util_go19.go, line 5 at r2 (raw file):

/*
 *
 * Copyright 2014 gRPC authors.

2018

---

credentials/credentials_util_go19.go, line 36 at r2 (raw file):

func (c tlsConn) SyscallConn() (syscall.RawConn, error) {
	conn, ok := c.rawConn.(syscall.Conn)

Since we only care about method SyscallConn, do

rawConn.(interface{
  SyscallConn() (syscall.RawConn, error)
})

So we don't need the _pre19.go file for this.

But this works only if syscall.RawConn is defined pre 1.9.

---

credentials/credentials_util_pre_go19.go, line 5 at r2 (raw file):

/*
 *
 * Copyright 2014 gRPC authors.

2018

---

credentials/credentials_util_pre_go19.go, line 30 at r2 (raw file):

type tlsConn struct {
	*tls.Conn
	rawConn net.Conn

Why keeping the rawConn?

---

test/channelz_test.go, line 125 at r2 (raw file):

			ccs = append(ccs, cc)
		}

Revert this.

---

Comments from Reviewable

[lyuxuan]

Review status: 3 of 17 files reviewed at latest revision, 15 unresolved discussions.

---

channelz/types.go, line 423 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

Move this to a sub package?

channelz package defines the API between the clientconn and the channelz store.
This is more like a helper function for the clientconn to get info from creds. It's not part of the API.

moved to credentials package.

---

channelz/util_unix_go19.go, line 1 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

Why do we need this build constraint?
And why is this not just windows or not?

https://github.com/golang/sys/blob/master/unix/syscall_unix.go contains definition for function like GetsockoptLinger, GetsockoptTimeval, and it has the build tag listed here. Therefore, I have to have the same build tag here. FYI, I've made some changes to the build tag.

---

channelz/util_unix_go19.go, line 29 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

This is also a helper function, not part of the API.
In other words, this is not necessary for channelz, the clientconn could generate the same result without calling this function.

If we move this function to transport package, the it will complicate the package by adding two more files for different go version.

---

channelz/service/service.go, line 38 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

Make a function to do the time conversion.

done

---

channelz/service/service.go, line 145 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

Move }'s to one line

done

---

credentials/credentials.go, line 35 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

Delete blank line.

done

---

credentials/credentials.go, line 172 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

Names of the fields.

done

---

credentials/credentials_util_go19.go, line 5 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

2018

done

---

credentials/credentials_util_go19.go, line 36 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

Since we only care about method SyscallConn, do

rawConn.(interface{
  SyscallConn() (syscall.RawConn, error)
})

So we don't need the _pre19.go file for this.

But this works only if syscall.RawConn is defined pre 1.9.

Unfortunately, syscall.RawConn is not defined pre 1.9

---

credentials/credentials_util_pre_go19.go, line 5 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

2018

done

---

credentials/credentials_util_pre_go19.go, line 30 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

Why keeping the rawConn?

That's how we get the socket options, i.e. by doing the SyscallConn(rawConn) and then Control()

---

channelz/types_linux.go, line 24 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

Will this import cause the build to fail on non-linux? Or just function calls will return error?

nice catch!

---

channelz/types_linux.go, line 30 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

This field is not defined in _windows.go, but is referenced in other files (e.g. service.go), even on windows.

I added some dummy struct definitions for SocketOptionData for non unix build

---

channelz/util_default.go, line 1 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

Rename this file. default sounds too normal.

renamed to util_nonlinux_pre_go19.go

---

test/channelz_test.go, line 125 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

Revert this.

ok

---

Comments from Reviewable

[menghanl]

Reviewed 1 of 23 files at r1, 10 of 19 files at r3.
Review status: 14 of 17 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

channelz/types_nonunixes.go, line 71 at r3 (raw file):

// Windows OS doesn't support Socket Option
type SocketOptionData struct {
	Linger      *lingerDummy

This is a strong assumption that non-unix systems will have similar concepts with unix.

Looking at the proto def for socket option, the real data field is a google.protobuf.Any. We should hide the Linger details in the platform specific files, and add a method that generates the Any proto (move MarshalAny function calls from service.go to platform specific files).

---

channelz/service/service.go, line 38 at r2 (raw file):

Previously, lyuxuan wrote…

done

Hmm, what I meant is a function that returns time.Duration, or even ptypes.DurationProto.
And one function should be enough.

---

credentials/credentials.go, line 237 at r3 (raw file):

// Security defines the interface that security protocols should implement in order
// to provide security info to channelz.
type Security interface {

The names for the interfaces are OK in channelz package, but inappropriate in credentials package.

Rename this or move it to channelz/subpackage

---

Comments from Reviewable

[menghanl]

Reviewed 1 of 23 files at r1, 3 of 19 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

channelz/util_test.go, line 31 at r3 (raw file):

	"google.golang.org/grpc/channelz"

Remove this blank line.

---

channelz/util_test.go, line 65 at r3 (raw file):

		t.Fatalf("net.Listen(%s,%s) failed with err: %v", network, addr, err)
	}
	done := startServer(ln)

something is wrong with this startServer function

---

channelz/util_test.go, line 73 at r3 (raw file):

	}

	l := &unix.Linger{Onoff: 1, Linger: 5}

The content of this struct will be modified by the syscall later, right?
Just leave it empty? Or use new.

---

channelz/util_test.go, line 110 at r3 (raw file):

	}

	ln.Close()

defer

---

channelz/service/service.go, line 153 at r3 (raw file):

		anyval, err := ptypes.MarshalAny(v.Value)
		if err != nil {
			return &pb.Security{Model: &pb.Security_Other{Other: &pb.Security_OtherSecurity{

Move this code around so you create one local variable and only set its field when error is not nil.

---

channelz/service/service.go, line 168 at r3 (raw file):

	var opts []*pb.SocketOption
	if skopts.Linger != nil {
		additional, err := ptypes.MarshalAny(&pb.SocketOptionLinger{Active: skopts.Linger.Onoff != 0, Duration: ptypes.DurationProto(time.Duration(secToNano(int64(skopts.Linger.Linger))))})

Move this and other code that accesses internal fields of SocketOptionData to _unix.go and _nonunix.go files. So you won't need the dummy non-unix types, either.

---

channelz/service/service.go, line 168 at r3 (raw file):

	var opts []*pb.SocketOption
	if skopts.Linger != nil {
		additional, err := ptypes.MarshalAny(&pb.SocketOptionLinger{Active: skopts.Linger.Onoff != 0, Duration: ptypes.DurationProto(time.Duration(secToNano(int64(skopts.Linger.Linger))))})

And put the fields of the struct to multiple lines.

---

channelz/service/service_test.go, line 35 at r3 (raw file):

	"golang.org/x/sys/unix"
	"google.golang.org/grpc/channelz"
	pb "google.golang.org/grpc/channelz/service_proto"

channelzpb "google.golang.org/grpc/channelz/"

---

channelz/service/service_test.go, line 41 at r3 (raw file):

func init() {
	proto.RegisterType((*OtherSecurityValue)(nil), "grpc.credentials.OtherSecurityValue")

Move this next to the definition of OtherSecurityValue, and add a comment that it's needed by UnmarshalAny

---

channelz/service/service_test.go, line 180 at r3 (raw file):

	if protoDuration := protoLinger.GetDuration(); protoDuration != nil {
		if dur, err := ptypes.Duration(protoDuration); err == nil {
			linger.Linger = int32(int64(dur) / 1e9)

Similar to previous comments on making a function to convert duration, this and the following 1e3, 1e9 could also be a function.

---

credentials/credentials_util_go19.go, line 35 at r3 (raw file):

}

func (c tlsConn) SyscallConn() (syscall.RawConn, error) {

Add a comment that this implements interface syscall.Conn

---

Comments from Reviewable

[lyuxuan]

Review status: all files reviewed at latest revision, 14 unresolved discussions, some commit checks failed.

---

channelz/types_nonunixes.go, line 71 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

This is a strong assumption that non-unix systems will have similar concepts with unix.

Looking at the proto def for socket option, the real data field is a google.protobuf.Any. We should hide the Linger details in the platform specific files, and add a method that generates the Any proto (move MarshalAny function calls from service.go to platform specific files).

Discussed offline: to generate any proto, it will add pb dependency to channelz package, which will eventually lead to a cyclic import path.

---

channelz/util_test.go, line 31 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

Remove this blank line.

done

---

channelz/util_test.go, line 65 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

something is wrong with this startServer function

fixed

---

channelz/util_test.go, line 73 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

The content of this struct will be modified by the syscall later, right?
Just leave it empty? Or use new.

the value here is to set the fd, and we will read from the fd to verify the set and get functionality.

---

channelz/util_test.go, line 110 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

defer

done

---

channelz/service/service.go, line 38 at r2 (raw file):

ptypes.DurationProto
done

---

channelz/service/service.go, line 153 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

Move this code around so you create one local variable and only set its field when error is not nil.

done

---

channelz/service/service.go, line 168 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

Move this and other code that accesses internal fields of SocketOptionData to _unix.go and _nonunix.go files. So you won't need the dummy non-unix types, either.

done

---

channelz/service/service.go, line 168 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

And put the fields of the struct to multiple lines.

done

---

channelz/service/service_test.go, line 35 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

channelzpb "google.golang.org/grpc/channelz/"

I think this is obsolete, right?

---

channelz/service/service_test.go, line 41 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

Move this next to the definition of OtherSecurityValue, and add a comment that it's needed by UnmarshalAny

done

---

channelz/service/service_test.go, line 180 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

Similar to previous comments on making a function to convert duration, this and the following 1e3, 1e9 could also be a function.

done

---

credentials/credentials.go, line 237 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

The names for the interfaces are OK in channelz package, but inappropriate in credentials package.

Rename this or move it to channelz/subpackage

rename to ExtraSecurityInfo. It's better to be in credentials package as people implementing credentials will notice it.

---

credentials/credentials_util_go19.go, line 35 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

Add a comment that this implements interface syscall.Conn

done

---

Comments from Reviewable

[menghanl]

Reviewed 10 of 13 files at r4.
Review status: 18 of 19 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

channelz/util_test.go, line 65 at r3 (raw file):

Previously, lyuxuan wrote…

fixed

But what's the purpose of function startServer and why the for loop plus the channel.

It seems to me what you actually want is just

ln, _ := net.Listen(network, addr)
go func() { ln.Accept() } ()

conn, _ := net.Dial()

---

Comments from Reviewable

[lyuxuan]

Review status: 11 of 19 files reviewed at latest revision, 1 unresolved discussion.

---

channelz/util_test.go, line 65 at r3 (raw file):

go func() { ln.Accept() } ()
yes. I was thinking something more complicated that probably need this and later change my idea. Fixed.

---

Comments from Reviewable

[menghanl]

Reviewed 1 of 13 files at r4, 8 of 8 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.

---

credentials/credentials.go, line 237 at r5 (raw file):

// ExtraSecurityInfo defines the interface that security protocols should implement in order
// to provide security info to channelz.
type ExtraSecurityInfo interface {

Nit: If there's no plan to use this and the following types for anything else, I would include the word "channelz" in all the names, like ChannelzSecurityInfo.

---

Comments from Reviewable

[menghanl]

Had one last nit on the names ExtraSecurityInfo vs ChannelzSecurityInfo.

LGTM otherwise.

[menghanl]

Breaks darwin builds. Revert in #2096.