🆕 Devcontainer support, Docker support and IPv6 inside container fix#191
🆕 Devcontainer support, Docker support and IPv6 inside container fix#191aviadhahami wants to merge 11 commits intonfx:mainfrom
Conversation
aviadhahami
changed the title
|
You have problem with the UI. When go to http://localhost:8089/, we have "missing UI" |
|
|
||
| build-go-for-docker: | ||
| go mod vendor | ||
| CGO_ENABLED=0 GOOS=linux go build -a -installsuffix cgo -ldflags "-s -w" -o main main.go |
There was a problem hiding this comment.
We don't use cgo here at all, pls remove
There was a problem hiding this comment.
I agree, tho when running the project on Alpine it fails unless compiling w/ CGO.
The CGO is also used here: https://github.com/nfx/slrp/blob/main/.goreleaser.yaml#L13.
This topic specifically was previously discussed here: #90 (comment)
"...but slrp must be build with static linkage CGO_ENABLED=0 I believe..."
wdyt?
There was a problem hiding this comment.
Here's the situation where a code comment could have avoided a review comment 😉
There was a problem hiding this comment.
Will add the comment where needed ;)
.devcontainer/devcontainer.json
Outdated
| // "features": {}, | ||
|
|
||
| // Use 'forwardPorts' to make a list of ports inside the container available locally. | ||
| // "forwardPorts": [], |
There was a problem hiding this comment.
Can add them explicitly but note that once the project is running they are auto-forwarded by vscode.
Adding these two port will make them perma-accessible, which is what I wanted to avoid.
Would you like me to add them permanently?
Attaching a screenshot of how the vscode panel looks once the project is running in debug mode
.devcontainer/devcontainer.json
Outdated
| // Use 'postCreateCommand' to run commands after the container is created. | ||
| "postCreateCommand": ".devcontainer/postcreate.sh", | ||
|
|
||
| // Configure tool-specific properties. |
| // "customizations": {}, | ||
|
|
||
| // Uncomment to connect as root instead. More info: https://aka.ms/dev-containers-non-root. | ||
| "remoteUser": "root", |
There was a problem hiding this comment.
w/o the container can't init nvm (run source ~/.bashrc) properly.
Since this is only for the devcontainer, I think it's fine security-wise
.devcontainer/devcontainer.json
Outdated
| // For format details, see https://aka.ms/devcontainer.json. For config options, see the | ||
| // README at: https://github.com/devcontainers/templates/tree/main/src/go | ||
| { | ||
| "name": "Go", |
There was a problem hiding this comment.
| "name": "Go", | |
| "name": "slrp dev", |
Maybe this?
.devcontainer/devcontainer.json
Outdated
| "image": "mcr.microsoft.com/devcontainers/go:1-1.20-bookworm", | ||
|
|
||
| // Features to add to the dev container. More info: https://containers.dev/features. | ||
| // "features": {}, |
.devcontainer/postcreate.sh
Outdated
|
|
||
| # Check if we don't have nvm, if no - install it | ||
| echo "[ + ] Installing nvm..." | ||
| curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.38.0/install.sh | bash |
|
@nfx addressed your comments and rebased on top of your master :) take a look lmk what u think |
nfx
left a comment
nfx
left a comment
There was a problem hiding this comment.
Mostly lgtm, but I have to manually look at the introduced third party dependencies before merging.
Thanks for your help 🎊
|
@CyrilDesch You have to make a PR from your repo to this repo 😉 |
Update the docker to have multi-stage build and compile a static binary so we can land in Alpine later
|
update: rebased and addressed review comments. @nfx - @CyrilDesch already PRed to my repo (🎉 ), commit is 0a72c77 |
README.md
Outdated
| Run `make docker`. Once done, invoke with `docker run -p 8089:8089 -p 8090:8090 -v $HOME/.slrp/data:/data nfx/slrp:latest` | ||
|
|
||
| Once running, you can access the UI at [http://localhost:8089/](http://localhost:8089/) and the proxy at [http://localhost:8090/](http://localhost:8090/) | ||
| Test using a simple curl command `curl --proxy-insecure -D - -x http:// http://127.0.0.1:8090 -k http://httpbin.org/get` couple of times and see different origins and user agent headers. |
There was a problem hiding this comment.
There seems to be an extra http:// in the curl example command.
So instead of this:
curl --proxy-insecure -D - -x http:// http://127.0.0.1:8090 -k http://httpbin.org/get
It should be this:
curl --proxy-insecure -D - -x http://127.0.0.1:8090 -k http://httpbin.org/get
The -k curl option is only necessary if the target URL uses https, so either the -k can be dropped, or the target URL changed into https://httpbin.org/get.
There was a problem hiding this comment.
Using slrp as a proxy seems to require a https connection (to the proxy).
The --proxy-insecure option in the example suggests this as well.
But the example uses plain http:// for the proxy URL.
So the correct example is:
curl --proxy-insecure -D - -x "https://127.0.0.1:8090" "http://httpbin.org/get"
Or this:
curl --proxy-insecure -D - -x "https://127.0.0.1:8090" -k "https://httpbin.org/get"
pmux/proxy.go
Outdated
| // proxy package(or so). Hence we replace it with 0.0.0.0 | ||
| if strings.Contains(addr, "[::]") { | ||
| // Set it to 0.0.0.0 but maintain the port | ||
| fmt.Println("Encountered [::]: in address, replacing with 0.0.0.0") |
There was a problem hiding this comment.
Shouldn't this be a call to log.Info() instead of directly writing to stdout?
pmux/proxy.go
Outdated
|
|
||
| addrPort, err := netip.ParseAddrPort(addr) | ||
| if err != nil { | ||
| fmt.Println("Error parsing address:", err) |
There was a problem hiding this comment.
Shouldn't this be a call to log.Info() instead of directly writing to stdout?
Using log instead of stdout
|
Hey @nfx - any updates on this? :) |
|
Hey @nfx - any indications of when this might be merged pls ? |
5 participants
Heya! I wanted to fix some bugs in this project (ie #178 #181 ) but I wasn't able to consistently build this project.
So first - I added devcontainer support a580a8f
Then, I addressed #170 by updating the Dockerfile to fully build and ship the project as a linked binary inside an Alpine image.
I then also fixed #178.
So, the current status is that the code is usable via docker and we can also work on the same env via devcontainer.
Will update the readme with an example command to run via docker
Awesome project btw!
Follow-up work:
I am unable, as of now, to fix #181 and I keep hitting the same error. Any idea? might be around HTTPS