Skip to content
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

channelz: stage 4 - add security and socket option info #2098

Merged
merged 8 commits into from
Jun 5, 2018

Conversation

lyuxuan
Copy link
Contributor

@lyuxuan lyuxuan commented May 22, 2018

fix the build tags that previously break users.
Socketoption is only supported on linux system, as GetsockoptTCPInfo is only defined for linux environment.


This change is Reviewable

@lyuxuan lyuxuan requested a review from menghanl May 22, 2018 23:34
@menghanl
Copy link
Contributor

Review status: 0 of 19 files reviewed at latest revision, all discussions resolved, some commit checks failed.


channelz/types_nonlinux.go, line 1 at r1 (raw file):

// +build !linux

Add an init() to log something.


Comments from Reviewable

@lyuxuan lyuxuan force-pushed the security_metric_fix branch from 5994dee to 4265c7d Compare May 29, 2018 18:23
@lyuxuan lyuxuan force-pushed the security_metric_fix branch from 4265c7d to 9637fb8 Compare May 29, 2018 19:41
@menghanl
Copy link
Contributor

Review status: all files reviewed at latest revision, all discussions resolved.


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

// +build linux
// +build go1.10

Why is this 1.10?


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

// +build linux

Only the socket related tests are platform specific, other tests should cover all platforms.


test/channelz_linux_go19_test.go, line 1 at r3 (raw file):

// +build go1.10,linux

The file name is go19.

And why is this go1.10 and above only?


Comments from Reviewable

@lyuxuan
Copy link
Contributor Author

lyuxuan commented May 30, 2018

Review status: all files reviewed at latest revision, 3 unresolved discussions.


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

Previously, menghanl (Menghan Li) wrote…

Why is this 1.10?

This is because SyscallConn() function for net.TCPListener is added after go1.10. golang/go@eed308d


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

Previously, menghanl (Menghan Li) wrote…

Only the socket related tests are platform specific, other tests should cover all platforms.

yeah, make sense. I split the test into 3 files, the base service_test.go which contains all the shared tests, service_linux_test.go, which contains linux necessary helper functions and tests, service_nonlinux_test.go, which contains nonlinux helper functions.


test/channelz_linux_go19_test.go, line 1 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

The file name is go19.

And why is this go1.10 and above only?

Wrong file name, should be go1.10. Good catch.


Comments from Reviewable

@dfawley dfawley assigned menghanl and unassigned lyuxuan May 31, 2018
@menghanl
Copy link
Contributor

menghanl commented Jun 1, 2018

Review status: 19 of 23 files reviewed at latest revision, 2 unresolved discussions.


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

Previously, lyuxuan wrote…

This is because SyscallConn() function for net.TCPListener is added after go1.10. golang/go@eed308d

Add comment to explain this.


channelz/service/service_linux_test.go, line 20 at r4 (raw file):

 *
 */

Add file level comment for what's different between this and non-linux.


test/channelz_linux_go19_test.go, line 1 at r3 (raw file):

Previously, lyuxuan wrote…

Wrong file name, should be go1.10. Good catch.

Is this 1.10 the same reason as above? Add a comment as well.


Comments from Reviewable

@lyuxuan
Copy link
Contributor Author

lyuxuan commented Jun 2, 2018

Review status: 19 of 23 files reviewed at latest revision, 3 unresolved discussions.


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

Previously, menghanl (Menghan Li) wrote…

Add comment to explain this.

done


channelz/service/service_linux_test.go, line 20 at r4 (raw file):

Previously, menghanl (Menghan Li) wrote…

Add file level comment for what's different between this and non-linux.

done


test/channelz_linux_go19_test.go, line 1 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

Is this 1.10 the same reason as above? Add a comment as well.

yes. done


Comments from Reviewable

@lyuxuan lyuxuan merged commit c1a21e2 into grpc:master Jun 5, 2018
lyuxuan added a commit that referenced this pull request Jun 6, 2018
menghanl pushed a commit that referenced this pull request Jun 6, 2018
)

Reverts #2098

Appengine will fail with the error below:
```
go-app-builder: Failed parsing input: parser: bad import "syscall" in google.golang.org/grpc/channelz/funcs.go from GOPATH
```

The root cause of it is in type_linux.go.
https://github.com/grpc/grpc-go/blob/629f6bc5e559f3a278d922c79f46678030797db3/channelz/types_linux.go#L21-L25
lyuxuan added a commit to lyuxuan/grpc-go that referenced this pull request Jun 7, 2018
lyuxuan added a commit to lyuxuan/grpc-go that referenced this pull request Jun 12, 2018
lyuxuan added a commit to lyuxuan/grpc-go that referenced this pull request Jun 12, 2018
lyuxuan added a commit to lyuxuan/grpc-go that referenced this pull request Jun 13, 2018
lyuxuan added a commit to lyuxuan/grpc-go that referenced this pull request Jun 13, 2018
lyuxuan added a commit to lyuxuan/grpc-go that referenced this pull request Jun 14, 2018
lyuxuan added a commit to lyuxuan/grpc-go that referenced this pull request Jun 21, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Dec 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants