Upgrade grpc-go dependency and check if file in SocketPath is actually a socket #126
Conversation
k8s-ci-robot
added
the
release-note
k8s-ci-robot
added
cncf-cla: yes
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Jiawei0227, msau42 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
pkg/util/util_linux.go
Outdated
| if isSocket := (fi.Mode()&os.ModeSocket != 0); isSocket { | ||
| return true, nil | ||
| } | ||
| klog.Warningf("file exists in socketPath %s but it's not a socket.: %+v", socketPath, fi) |
There was a problem hiding this comment.
Does this fix #104? It seems like this will return false and nil for the error if it is not a socket.
There was a problem hiding this comment.
Yeah good point... I initially return error but then wonder if it will break things so I change it to warning. I think an Error here seems okay to me? It's just we will have to change the CleanUpSocket function as well.
There was a problem hiding this comment.
I vaguely recall the socket checking logic differs between linux and Windows. It might be worth double-checking.
There was a problem hiding this comment.
There is already a TestSocketRegularFile which I modified a little bit that test this behavior.
The limitation is that we will have different behaviors on linux and windows... Because according golang/go#33357, windows socket file are not returning the right mode somehow... So the test will behavior a little bit different based on the two platforms.
|
Do we have a unit test that asserts we should return an error if the socket path exists but is not a socket file? |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
What type of PR is this?
/kind bug
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes # #104
Fixes # #93
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
/assign @chrishenzie
/cc @msau42