Create unix domain socket file in temp directory #9211
Conversation
Currently queue-proxy creates unix domain socket file in the "current" directory, where is /ko-app/ in upstream. It works when root user runs queue-proxy or when the directory has runtime user's permission. In other words, `/ko-app/` dir must have runtime user's permission when nonroot users run queue-proxy. It is a pain that the directory permission must be cared. To solve the permission problems, this patch changes to create the socket file under temp directory.
There was a problem hiding this comment.
@nak3: 0 warnings.
In response to this:
Currently queue-proxy creates unix domain socket file in the "current"
directory, where is /ko-app/ in upstream.It works when root user runs queue-proxy or when the directory has
runtime user's permission. In other words,/ko-app/dir must have
runtime user's permission when nonroot users run queue-proxy.It is a pain that the directory permission must be cared.
To solve the permission problem, this patch changes to create the
socket file under temp directory./lint
/cc @julz @mattmoor @vagababov
Release Note
NONE
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
|
For example, downstream (OpenShift) runs queue-proxy with nonroot user Our current permission is like this: I have verified that the permission issue does not happen when socket |
There was a problem hiding this comment.
couple nits but generally seems legit, I'm a little confused why the /ko-app/ directory isn't writable by the user.. shouldn't it be part of the ko contract that the user can write to its own home dir? Is the actual problem here that openshift is running as a non-root user but isn't actually building the images with the rootless ko base image?
cmd/queue/main.go
Outdated
| @@ -201,7 +201,7 @@ func main() { | |||
| // when we're actually in the same container. | |||
| transport := &http.Transport{ | |||
| DialContext: func(_ context.Context, _, _ string) (net.Conn, error) { | |||
| return net.Dial("unix", unixSocketPath) | |||
| return net.Dial("unix", os.TempDir()+unixSocketPath) | |||
There was a problem hiding this comment.
nit, but better to do filepath.Join here I think
cmd/queue/main.go
Outdated
| @@ -63,7 +63,7 @@ const ( | |||
| // reportingPeriod is the interval of time between reporting stats by queue proxy. | |||
| reportingPeriod = 1 * time.Second | |||
|
|
|||
| unixSocketPath = "queue.sock" | |||
| unixSocketPath = "/queue.sock" | |||
There was a problem hiding this comment.
we should probably call this unixSocketName since this isn't the path any more
There was a problem hiding this comment.
Yes, and I changed it to var unixSocketPath = filepath.Join(os.TempDir(), "queue.sock").
|
/hold Just saw @julz comments |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: julz, mattmoor, nak3 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 |
|
I think my comments are all addressed, I'm interested why this doesn't "just work" in openshift (guessing it's just changing user and not changing base image to match?), but it seems like a reasonable fix either way |
|
/unhold |
Yes, I think your guessing is currect. Our base image is this: And the binary is pushed by this: So I think we also could solve the issue by setting the permission to |
|
thanks for the follow-up @nak3, makes sense! |
* Create unix domain socket file in temp directory Currently queue-proxy creates unix domain socket file in the "current" directory, where is /ko-app/ in upstream. It works when root user runs queue-proxy or when the directory has runtime user's permission. In other words, `/ko-app/` dir must have runtime user's permission when nonroot users run queue-proxy. It is a pain that the directory permission must be cared. To solve the permission problems, this patch changes to create the socket file under temp directory. * Use filepath.Join
* Create unix domain socket file in temp directory Currently queue-proxy creates unix domain socket file in the "current" directory, where is /ko-app/ in upstream. It works when root user runs queue-proxy or when the directory has runtime user's permission. In other words, `/ko-app/` dir must have runtime user's permission when nonroot users run queue-proxy. It is a pain that the directory permission must be cared. To solve the permission problems, this patch changes to create the socket file under temp directory. * Use filepath.Join
Currently queue-proxy creates unix domain socket file in the "current"
directory, where is /ko-app/ in upstream.
It works when root user runs queue-proxy or when the directory has
runtime user's permission. In other words,
/ko-app/dir must haveruntime user's permission when nonroot users run queue-proxy.
It is a pain that the directory permission must be cared.
To solve the permission problem, this patch changes to create the
socket file under temp directory.
/lint
/cc @julz @mattmoor @vagababov
Release Note