Expose docker API to host by praveenkumar · Pull Request #3002 · crc-org/crc

praveenkumar · 2022-02-11T10:50:11Z

This is only tested on the mac. I will test it on the Linux also but we need someone who can test this windows. I think on windows unix:/// doesn't work, we do need to check if named pipe can be used.

Some of the commits can be removed once the forked repo not needed.

cfergeau · 2022-02-11T11:24:47Z

pkg/crc/constants/constants.go

 	return filepath.Join(MachineInstanceDir, DefaultName, "id_ecdsa")
 }

+func GetDockerSockOnHost() string {


maybe GetHostDockerSocketPath?

cfergeau · 2022-02-11T11:26:47Z

go.mod

 	k8s.io/kubectl => github.com/openshift/kubernetes-kubectl v0.0.0-20210730111826-9c6734b9d97d
 )
+
+replace github.com/containers/gvisor-tap-vsock => github.com/protosam/gvisor-tap-vsock v0.3.1-0.20220114153937-6d76545f46e2


I'd prefer that we delay this PR a bit until this lands upstream instead of relying on a forked repository we have no control over.

go.mod

cmd/crc/cmd/daemon_linux.go

pkg/crc/machine/vsock.go

praveenkumar · 2022-02-11T12:41:33Z

/hold

#3002 (comment)

praveenkumar · 2022-02-11T14:27:24Z

/unhold

cfergeau

Looks good, still one pending comment, I suspect Windows support will be more work.

cfergeau · 2022-02-11T15:09:59Z

Let's address crc-org/snc#506 (comment) before merging this could cause changes in this PR.

pkg/crc/machine/vsock.go

go.mod

cfergeau · 2022-02-15T11:19:24Z

pkg/crc/machine/vsock.go

 				Protocol: "unix",
 				Local:    constants.GetHostDockerSocketPath(),
-				Remote:   fmt.Sprintf("ssh-tunnel://core@%s:%d/run/podman/podman.sock?key=%s", virtualMachineIP, internalSSHPort, constants.GetPrivateKeyPath()),
+				Remote:   fmt.Sprintf("ssh-tunnel://core@%s:%s/run/podman/podman.sock?key=%s", virtualMachineIP, internalSSHPort, constants.GetPrivateKeyPath()),


This should be using net.JoinHostPort as well

I believe you're right. I would also recommend constructing a https://pkg.go.dev/net/url#URL and setting its fields as necessary then stringifying at API boundaries. It'll handle all the weird edge cases.

cfergeau · 2022-02-16T13:13:19Z

pkg/crc/machine/vsock.go

+		Host:       net.JoinHostPort(virtualMachineIP, internalSSHPort),
+		Path:       "/run/podman/podman.sock",
+		ForceQuery: false,
+		RawQuery:   fmt.Sprintf("key=%s", constants.GetPrivateKeyPath()),


better to run this through https://pkg.go.dev/net/url#QueryEscape, hopefully gvisor-tap-vsock accepts this.
A bit odd to introduce the fmt.Sprintf() in one commit, to change it to using url.URL in a commit which is not really related to this.

We are dropping our fork of mdlayher/vsock since it has the API we need in the version, we are switching to. Fixes crc-org#3001

Since now gvisor-tap-vsock have the different protocol implementation like `unixTotcp`, `unixTounix`, `tcpTotcp` so those details need to be part of protocol type so that when check for the open port happen it should match with expose request type.

With this now following will happen automatic ``` ✗ curl --unix-socket ~/.crc/crc-http.sock http:/unix/network/services/forwarder/all | jq . [ { "local": "/tmp/docker.sock", "remote": "ssh-tunnel://core@192.168.127.2:22/run/podman/podman.sock?key=/Users/prkumar/.crc/machines/crc/id_ecdsa", "protocol": "unix" } ] ```

cfergeau · 2022-02-16T14:42:09Z

pkg/crc/machine/vsock.go

+		ForceQuery: false,
+		RawQuery:   fmt.Sprintf("key=%s", constants.GetPrivateKeyPath()),
+	}
+	return url.QueryEscape(u.String())


no, you want RawQuery: url.QueryEscape(fmt.Sprintf("key=%s, ...)), and then return url.String(). Or is it RawQuery: fmt.Sprintf("key=%s", url.QueryEscape(constants.GetPrivateKeyPath())?

Ah my bad I misread QueryEscape escapes the string so it can be safely placed inside a URL query. this one :(

anjannath · 2022-02-17T06:13:58Z

Tried it on windows, currently it fails during crc start with the following error:

failed to expose port C:\Users\anath\.crc\machines\crc\docker.sock -> ssh-tunnel://core@192.168.127.2:22/run/podman/podman.sock?key%3DC%3A%5CUsers%5Canath%5C.crc%5Cmachines%5Ccrc%5Cid_ecdsa: key not provided for unix-ssh connection

Daemon logs:

PS C:\Users\anath\redhat\crc> crc daemon
INFO Checking if running in a shell with administrator rights
INFO Checking Windows 10 release
INFO Checking Windows edition
[...]
INFO listening vsock://00000400-FACB-11E6-BD58-64006A7986D3
INFO listening \\.\pipe\crc-http
\\.\pipe\crc-http - - [17/Feb/2022:11:40:01 +0530] "GET /api/version HTTP/1.1" 200 74
\\.\pipe\crc-http - - [17/Feb/2022:11:40:20 +0530] "GET /network/services/forwarder/all HTTP/1.1" 200 3
\\.\pipe\crc-http - - [17/Feb/2022:11:40:20 +0530] "POST /network/services/forwarder/expose HTTP/1.1" 200 0
\\.\pipe\crc-http - - [17/Feb/2022:11:40:20 +0530] "POST /network/services/forwarder/expose HTTP/1.1" 200 0
\\.\pipe\crc-http - - [17/Feb/2022:11:40:20 +0530] "POST /network/services/forwarder/expose HTTP/1.1" 500 41

cfergeau

Looks good to me, maybe wait for more feedback from Anjan on Windows?

openshift-ci · 2022-02-17T07:22:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau, praveenkumar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [cfergeau,praveenkumar]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

gbraad · 2022-02-17T07:31:59Z

@cfergeau Anjan already mentioned it failed due to the SSH key location. is that resolved @praveenkumar ? If so, @anjannath can test again.

cfergeau · 2022-02-17T07:39:38Z

@cfergeau Anjan already mentioned it failed due to the SSH key location. is that resolved @praveenkumar ? If so, @anjannath can test again.

Anjan tested, reported it's not working, Praveen force-pushed a change which could help with what Anjan was seeing, so I was asking if Anjan can test again.

praveenkumar · 2022-02-17T09:27:29Z

@cfergeau Anjan already mentioned it failed due to the SSH key location. is that resolved @praveenkumar ? If so, @anjannath can test again.

Anjan tested, reported it's not working, Praveen force-pushed a change which could help with what Anjan was seeing, so I was asking if Anjan can test again.

Me and @anjannath tested this PR and in windows also it does able to map the socket but this socket docker client can't consume since it only support named pipe so we need to proxy namedpipe <=> docker sock somehow.

For windows I created a new tracking issue and from this PR disable the DOCKER_HOST from podman-env command on windows.

With this patch following is possible for mac and linux. For windows we need to use named pipe and that needs bit more effort. ``` ✗ ./crc podman-env export PATH="/Users/prkumar/.crc/bin/oc:$PATH" export CONTAINER_SSHKEY="/Users/prkumar/.crc/machines/crc/id_ecdsa" export CONTAINER_HOST="ssh://core@127.0.0.1:2222/run/user/1000/podman/podman.sock" export DOCKER_HOST="unix:///Users/prkumar/.crc/machines/crc/docker.sock" x eval $(./crc podman-env) ✗ docker images REPOSITORY TAG IMAGE ID CREATED SIZE quay.io/crcont/gvisor-tap-vsock 3231aba53905468c22e394493a0debc1a6cc6392 f42b05e6b25e 5 months ago 6.65MB httpd 2.4 a8ea074f4566 2 weeks ago 148MB ```

It also support ipv6 if in future we need support.

openshift-ci · 2022-02-17T09:41:22Z

New changes are detected. LGTM label has been removed.

praveenkumar · 2022-02-17T10:34:25Z

Created following issues

openshift-ci bot requested review from anjannath and cfergeau February 11, 2022 10:50

openshift-ci bot added the approved label Feb 11, 2022

cfergeau reviewed Feb 11, 2022

View reviewed changes

openshift-ci bot added the do-not-merge/hold label Feb 11, 2022

praveenkumar force-pushed the expose_docker_unix branch from 28a9258 to abb3def Compare February 11, 2022 14:24

openshift-ci bot removed the do-not-merge/hold label Feb 11, 2022

cfergeau reviewed Feb 11, 2022

View reviewed changes

praveenkumar force-pushed the expose_docker_unix branch from abb3def to 1e63f68 Compare February 11, 2022 15:00

praveenkumar mentioned this pull request Feb 14, 2022

go.mod: upgrade to github.com/mdlayher/vsock@v1.1.0 #3004

Closed

praveenkumar force-pushed the expose_docker_unix branch from 1e63f68 to 6c79281 Compare February 14, 2022 15:53

mdlayher reviewed Feb 14, 2022

View reviewed changes

pkg/crc/machine/vsock.go Outdated Show resolved Hide resolved

praveenkumar force-pushed the expose_docker_unix branch from 6c79281 to 22d859f Compare February 15, 2022 06:46

cfergeau reviewed Feb 15, 2022

View reviewed changes

go.mod Show resolved Hide resolved

cfergeau reviewed Feb 15, 2022

View reviewed changes

praveenkumar force-pushed the expose_docker_unix branch 2 times, most recently from bb435a3 to dd99f84 Compare February 16, 2022 07:35

praveenkumar added the has-to-be-in-release This PR need to go in coming release. label Feb 16, 2022

cfergeau reviewed Feb 16, 2022

View reviewed changes

praveenkumar force-pushed the expose_docker_unix branch from dd99f84 to 12d7428 Compare February 16, 2022 14:06

praveenkumar added 3 commits February 16, 2022 19:37

Update gvisor-tap-vsock dependency

69e37d7

We are dropping our fork of mdlayher/vsock since it has the API we need in the version, we are switching to. Fixes crc-org#3001

praveenkumar force-pushed the expose_docker_unix branch from 12d7428 to d5609df Compare February 16, 2022 14:07

cfergeau reviewed Feb 16, 2022

View reviewed changes

praveenkumar force-pushed the expose_docker_unix branch from d5609df to cd3e65e Compare February 16, 2022 15:37

praveenkumar force-pushed the expose_docker_unix branch from cd3e65e to c6db610 Compare February 17, 2022 07:12

cfergeau approved these changes Feb 17, 2022

View reviewed changes

openshift-ci bot assigned cfergeau Feb 17, 2022

openshift-ci bot added the lgtm label Feb 17, 2022

praveenkumar added 2 commits February 17, 2022 15:07

Vsock: Use net.JoinHostPort to create url

a29fa5f

It also support ipv6 if in future we need support.

praveenkumar force-pushed the expose_docker_unix branch from c6db610 to a29fa5f Compare February 17, 2022 09:41

openshift-ci bot removed the lgtm label Feb 17, 2022

praveenkumar merged commit 8ef4336 into crc-org:dev Feb 17, 2022

anjannath mentioned this pull request Mar 16, 2022

[Windows] Create named pipe for docker client to connect #3018

Closed

Conversation

praveenkumar commented Feb 11, 2022

Uh oh!

cfergeau Feb 11, 2022

Choose a reason for hiding this comment

Uh oh!

cfergeau Feb 11, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

praveenkumar commented Feb 11, 2022

Uh oh!

praveenkumar commented Feb 11, 2022

Uh oh!

cfergeau left a comment

Choose a reason for hiding this comment

Uh oh!

cfergeau commented Feb 11, 2022

Uh oh!

Uh oh!

Uh oh!

cfergeau Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

mdlayher Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

cfergeau Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

cfergeau Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

praveenkumar Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

anjannath commented Feb 17, 2022

Uh oh!

cfergeau left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Feb 17, 2022

Uh oh!

gbraad commented Feb 17, 2022

Uh oh!

cfergeau commented Feb 17, 2022

Uh oh!

praveenkumar commented Feb 17, 2022

Uh oh!

openshift-ci bot commented Feb 17, 2022

Uh oh!

praveenkumar commented Feb 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants