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

bridge networking #229

Merged
merged 12 commits into from
Aug 16, 2022
Merged

bridge networking #229

merged 12 commits into from
Aug 16, 2022

Conversation

vito
Copy link
Owner

@vito vito commented Aug 13, 2022

Currently thunks runs in the same network namespace as the host. Bass was never designed for running untrusted thunks, so this isn't necessarily a bad thing1, but it has a major downside when it comes to running services: they're all competing for the same ports.

This PR configures CNI for Buildkit such that each thunk will run in its own network namespace using a bridge network to reach the host and other thunk addresses. Thunks can listen on whatever port they want without worrying about conflicts. Thunk addr values will now resolve to their target thunk's hostname instead of 127.0.0.1, and this hostname will be mapped to the target container's IP via /etc/hosts2

I originally thought this wouldn't be possible until moby/buildkit#28 lands but it turns out Buildkit already has CNI support, it's just not configured in the moby/buildkit image. I've built a separate basslang/buildkit image instead which is just moby/buildkit with Bass's CNI config.

The trickiest part here was getting the container IP back from Buildkit. The current approach is to run a tiny stupid TCP server on the runtime host and have the container shim connect to it through the gateway and pipe its hostname over the socket. This doesn't feel incredible from a security standpoint but I've done my best to narrow any theoretical attack vector. Still a WIP. I think I landed on something pretty decent for this; see #229 (comment) for more details.

Footnotes

  1. I kind of like simplicity of it, more akin to Procfiles than Docker Compose

  2. Admittedly there isn't much point doing this because if the container IP changes the LLB will be different either way, but I like the 𝒶𝑒𝓈𝓉𝒽𝑒𝓉𝒾𝒸

buildkit has bridge networking already!

This reverts commit ce3fd7a.
@vito vito added enhancement New feature or request breaking Removes or replaces previously supported functionality labels Aug 13, 2022
@vito
Copy link
Owner Author

vito commented Aug 13, 2022

This introduces some maintenance burden: I'll have to build and push a basslang/buildkit image for every supported architecture.1

This burden could be alleviated if and when moby/buildkit comes with CNI out-of-the-box (moby/buildkit#28), but we might not want to rely on Buildkit's default cni.json forever. Currently the only thing special about Bass's cni.json is that its IP range is 64.55.0.0/16 10.64.0.0/16, but we might want other changes someday -- like port mapping, if I can figure out how it works. If Bass ever needs its own cni.json it'd need a custom image, so this seems like the right thing to do anyway, with the additional benefit of not being blocked on upstream changes.

Footnotes

  1. Side-quest: it'd be great to build it with Bass rather than a Dockerfile!

@vito
Copy link
Owner Author

vito commented Aug 13, 2022

Side note: it probably makes more sense to run services via the frontend gateway container flow. Running services using Solve doesn't really make sense: we're not trying to build and cache services, we need to run them every time.

Following that change we could submit a PR upstream to have NewContainerResponse include the container IP. Alternatively the shim could print its container IP to stdout or something and the runtime could parse it out. Still janky, but way less janky than the TCP host bus dance.

@vito
Copy link
Owner Author

vito commented Aug 13, 2022

Hm, backpedaling a bit, it's pretty nice that using Solve automatically dedupes services...

@vito
Copy link
Owner Author

vito commented Aug 14, 2022

Switched to a new method for discovering the container IP. It's a bit more complicated but way more portable. The previous approach required the shim to call back to the host which didn't work reliably in tests and wouldn't work with a remote buildkit server.

The new approach moves the container IP discovery server into the shim and runs it in a separate scratch container through the frontend gateway container interface. The thunk shim then connects to the server and passes its container IP over the connection. The IP is then read from the server's stdout.

this was obviously broken! we would check if the cache already existed
and return it, but then create the cache and start writing to it.

now we'll create a temporary file and then rename it over the cache
instead.

this should fix all the really weird errors hitting bass-loop whenever
hooks kick off concurrently.
@vito vito marked this pull request as ready for review August 16, 2022 23:13
@vito vito changed the title support bridge networking bridge networking Aug 16, 2022
@vito vito merged commit 81534a3 into main Aug 16, 2022
@vito vito deleted the bridge branch August 16, 2022 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Removes or replaces previously supported functionality enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant