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

Revert "Shrink uptermd Docker image by 5 MB, 20% (#225)" #228

Merged
merged 1 commit into from
Feb 3, 2024

Conversation

owenthereal
Copy link
Owner

This reverts commit bb763d6.

@bcspragu I have to revert #225 because the production community server shares the same image and needs sh to compute fly.io node address at boot time:

upterm/fly.toml

Lines 11 to 12 in 2e48f73

cmd = ["uptermd --ssh-addr 0.0.0.0:2222 --ws-addr 0.0.0.0:8080 --node-addr $(echo ${FLY_ALLOC_ID} | cut -f1 -d '-').vm.${FLY_APP_NAME}.internal:2222 --hostname uptermd.upterm.dev"]
entrypoint = ["/bin/sh", "-c"]
. I think we might have to stick with the Alpine image or if you have a suggestion that a smaller image that has a shell in it.

@owenthereal owenthereal merged commit 29231fc into master Feb 3, 2024
7 checks passed
@owenthereal owenthereal deleted the owenthereal/revert_docker_changes branch February 3, 2024 20:25
@bcspragu
Copy link
Contributor

bcspragu commented Feb 3, 2024

Ah, apologies for breaking things! As for adding a shell to SCRATCH, I think that defeats most of the purpose (and some of the size benefits) of it. Some alternatives, from what I think are best to worst:

  • Add FLY_* "auto-detection" directly to uptermd. If FLY_ALLOC_ID and FLY_APP_NAME are set, use those variables
    • Can optionally hide this behind a --autodetect-fly flag to make it opt-in
    • I'm suggesting this because it looks like the only usage of sh here is variable substitution (and some string munging)
    • Am also happy to do this work
  • Make a separate Docker image for Fly that includes a shell
    • Makes sense if there's other sh usage in the Fly setup
  • Copy the shell over from the builder step
    • This probably won't actually work, with dynamic libs and whatnot, and probably isn't a good approach.

Not having a shell is also a useful security invariant for some modalities of compromise, gives an attacker less of a toolkit for introspecting things.

@owenthereal
Copy link
Owner Author

I think the FLY_* shouldn't be built into uptermd because it's designed to be cloud agnostic. Making a separate Docker image for Fly is the way to go if we want to have a minimum image for self-hosting while keeping a shell for the fly.io deployment.

@bcspragu
Copy link
Contributor

bcspragu commented Feb 6, 2024

One more idea because I think even Fly users would benefit from the lack of shell: A small, auxiliary flycfg binary that does the "env lookup to uptermd flag" conversion. That way, Fly still has a separate, Fly-specific image, uptermd has no Fly-specific flags, and both images benefit from lack of shell for size + security

owenthereal added a commit that referenced this pull request May 21, 2024
#228 was reverted due to
deployment to Fly requires `bash`. The bash script is replaced with a Go binary
(`uptermd-fly`). Dockerfile.upterm is adjusted to accomondate for Fly deployment and
self-hosting with different build targets.
owenthereal added a commit that referenced this pull request May 21, 2024
* Use minimum distroless base image

#228 was reverted due to
deployment to Fly requires `bash`. The bash script is replaced with a Go binary
(`uptermd-fly`). Dockerfile.upterm is adjusted to accomondate for Fly deployment and
self-hosting with different build targets.

* Use Docker GitHub Actions
@owenthereal
Copy link
Owner Author

A small, auxiliary flycfg binary that does the "env lookup to uptermd flag" conversion. That way, Fly still has a separate, Fly-specific image, uptermd has no Fly-specific flags, and both images benefit from lack of shell for size + security

Thanks for the feedback. I created a binary uptermd-fly for this purpose and use Docker's build target for Fly's deployment: #261

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants