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

add container definition #215

Merged
merged 4 commits into from
Mar 28, 2024
Merged

add container definition #215

merged 4 commits into from
Mar 28, 2024

Conversation

clanktron
Copy link
Contributor

  • Commit(s) and code follow the repositories guidelines.
  • Test(s) have been added or updated to support these change(s).
  • Doc(s) have been added or updated to support these change(s).

Verification/Testing of Changes:
All commands were tested. Below is some sample usage.

docker run clanktron/hauler:stable
# Airgap Swiss Army Knife
# 
# Usage:
#   hauler [flags]
#   hauler [command]
# 
# Available Commands:
#   completion  Generates completion scripts for various shells
#   help        Help about any command
#   login       Log in to a registry
#   store       Interact with hauler's embedded content store
#   version     Print the current version
# 
# Flags:
#   -h, --help               help for hauler
#   -l, --log-level string    (default "info")
# 
# Use "hauler [command] --help" for more information about a command.
docker run -v ./store:/store clanktron/hauler:stable store serve fileserver
# 6:08AM INF copied artifacts to [store-files]
# 6:08AM INF starting file server on port [8080]

Additional Context:
Minimal rootless container.

@clanktron clanktron requested review from dweomer and amartin120 March 27, 2024 17:11
@zackbradys zackbradys added enhancement New feature or request size/M Denotes an issue/PR requiring a relatively moderate amount of work labels Mar 27, 2024
@zackbradys
Copy link
Member

zackbradys commented Mar 27, 2024

I like this! Thank you for putting it together. I think it may be useful to use a BCI based image for the builder and expose the ports that hauler would use? I added a directory for /tmp since `hauler requires it.

How would you feel about something like this?

# build the hauler binary
FROM registry.suse.com/bci/golang:1.21 AS builder

RUN zypper --non-interactive install make bash wget ca-certificates \
    && zypper clean -a

COPY . /build
WORKDIR /build
RUN make build

RUN echo "hauler:x:1001:1001::/home:" > /etc/passwd \
  && echo "hauler:x:1001:hauler" > /etc/group \
  && mkdir /store \
  && mkdir /store-files \
  && mkdir /registry

# build the minimal image
FROM scratch

COPY --from=builder /etc/ssl/certs/ /etc/ssl/certs/
COPY --from=builder /etc/passwd /etc/passwd
COPY --from=builder /etc/group /etc/group
COPY --from=builder --chown=hauler:hauler /tmp/. /tmp
COPY --from=builder --chown=hauler:hauler /home/. /home
COPY --from=builder --chown=hauler:hauler /store/. /store
COPY --from=builder --chown=hauler:hauler /registry/. /registry
COPY --from=builder --chown=hauler:hauler /store-files/. /store-files
COPY --from=builder --chown=hauler:hauler /build/bin/hauler /

USER hauler

EXPOSE 80 8080 5000

ENTRYPOINT [ "/hauler" ]

@zackbradys
Copy link
Member

zackbradys commented Mar 28, 2024

Would we want to add a directory for hauls, maybe like /hauls so we are able to mount a directory with exiting hauls (archives/tarballs)? CC: @amartin120

@amartin120
Copy link
Contributor

amartin120 commented Mar 28, 2024

@zackbradys I definitely like the idea of mounting in "hauls" like you were showing me in your example last week.

Also, boss man @dweomer should be along soon to provide his feedback. I'm kinda leaning on him for this specific effort.

Copy link
Contributor

@dweomer dweomer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zackbradys
Copy link
Member

Updated the purposed changes to the dockerfile here...

# build the hauler binary
FROM registry.suse.com/bci/golang:1.21 AS builder

RUN zypper --non-interactive install make bash wget ca-certificates \
    && zypper clean -a

COPY . /build
WORKDIR /build
RUN make build

RUN echo "hauler:x:1001:1001::/home:" > /etc/passwd \
  && echo "hauler:x:1001:hauler" > /etc/group \
  && mkdir /store \
  && mkdir /registry \
  && mkdir /store-files \
  && mkdir /hauls

# build the minimal image
FROM scratch

COPY --from=builder /etc/ssl/certs/ /etc/ssl/certs/
COPY --from=builder /etc/passwd /etc/passwd
COPY --from=builder /etc/group /etc/group
COPY --from=builder --chown=hauler:hauler /home/. /home
COPY --from=builder --chown=hauler:hauler /store/. /store
COPY --from=builder --chown=hauler:hauler /registry/. /registry
COPY --from=builder --chown=hauler:hauler /store-files/. /store-files
COPY --from=builder --chown=hauler:hauler /hauls/. /hauls
COPY --from=builder --chown=hauler:hauler /tmp/. /tmp
COPY --from=builder --chown=hauler:hauler /build/bin/hauler /

USER hauler

EXPOSE 80 8080 5000

ENTRYPOINT [ "/hauler" ]

@clanktron
Copy link
Contributor Author

Would we want to add a directory for hauls, maybe like /hauls so we are able to mount a directory with exiting hauls (archives/tarballs)?

@zackbradys you should still be able to mount volumes wherever you like in the container, even if the path doesn't already exist. For example both of following work:

$ docker run -v ./hauls:/hauls clanktron/hauler:stable store serve registry -s /hauls
7:37PM INF clanktron/carbide-images-api:latest
7:37PM INF copied artifacts to [127.0.0.1:35135]
7:37PM INF starting registry on port [5000]
$ docker run -v ./hauls:/doesnt/exist/hauls clanktron/hauler:stable store serve registry -s /doesnt/exist/hauls
7:37PM INF clanktron/carbide-images-api:latest
7:37PM INF copied artifacts to [127.0.0.1:35135]
7:37PM INF starting registry on port [5000]

I only made a point to create /registry and /store-files since they're created ephemerally with the fileserver/registry and won't necessarily be a mounted volume. Technically the /store creation is unnecessary.

@clanktron
Copy link
Contributor Author

clanktron commented Mar 28, 2024

it may be useful to use a BCI based image for the builder

Sure thing.

and expose the ports that hauler would use?

I feel like the EXPOSE directive is largely unnecessary since it doesn't actually affect anything (besides docker run -P, but you'd only want to expose one of those ports, not all).

I added a directory for /tmp since `hauler requires it.

Good catch, just added that.

@clanktron
Copy link
Contributor Author

@amartin120 @zackbradys Gonna go ahead and merge unless you guys have some more suggestions.

@zackbradys
Copy link
Member

zackbradys commented Mar 28, 2024

Would we want to add a directory for hauls, maybe like /hauls so we are able to mount a directory with exiting hauls (archives/tarballs)?

@zackbradys you should still be able to mount volumes wherever you like in the container, even if the path doesn't already exist. For example both of following work:

$ docker run -v ./hauls:/hauls clanktron/hauler:stable store serve registry -s /hauls
7:37PM INF clanktron/carbide-images-api:latest
7:37PM INF copied artifacts to [127.0.0.1:35135]
7:37PM INF starting registry on port [5000]
$ docker run -v ./hauls:/doesnt/exist/hauls clanktron/hauler:stable store serve registry -s /doesnt/exist/hauls
7:37PM INF clanktron/carbide-images-api:latest
7:37PM INF copied artifacts to [127.0.0.1:35135]
7:37PM INF starting registry on port [5000]

I only made a point to create /registry and /store-files since they're created ephemerally with the fileserver/registry and won't necessarily be a mounted volume. Technically the /store creation is unnecessary.

I agree that /registry, /store-files, and /tmp need to be created since they won't be a mounted volume, but for /store and for /hauls, I like having them there for defaults and documentation so users would see how we recommend to use the container, but if we think that is unnecessary or we could show an example in hauler-docs, then I would say we should remove both of them.

Have you tried running the container with a mounted volume that contains a haul like longhorn.tar.zst and then hauler store load longhorn.tar.zst, then hauler store serve registry? @clanktron

@zackbradys
Copy link
Member

zackbradys commented Mar 28, 2024

it may be useful to use a BCI based image for the builder

Sure thing.

and expose the ports that hauler would use?

I feel like the EXPOSE directive is largely unnecessary since it doesn't actually affect anything (besides docker run -P, but you'd only want to expose one of those ports, not all).

I added a directory for /tmp since `hauler requires it.

Good catch, just added that.

Thank you for upgrading the image!

On the same note as my last note, I agree that EXPOSE is unnecessary and definitely not required, but it could be useful from the documentation side. We could probably also solve this with an example in the hauler-docs. @clanktron

@clanktron
Copy link
Contributor Author

clanktron commented Mar 28, 2024

Have you tried running the container with a mounted volume that contains a haul like longhorn.tar.zst and then hauler store load longhorn.tar.zst, then hauler store serve registry?

@zackbradys If you mean like this yea

docker run -v ./haul.tar.zst:/haul.tar.zst -v ./store:/store clanktron/hauler:stable store load haul.tar.zst
# 11:07PM INF loading content from [haul.tar.zst] to [store]
docker run -v ./store:/store clanktron/hauler:stable store serve registry
# 11:07PM INF neuvector/scanner:latest

We could probably also solve this with an example in the hauler-docs

Yea I say we just document it properly; default ports, CWD (aka /), the fact there's no shell, etc.

@clanktron clanktron merged commit 722851d into hauler-dev:main Mar 28, 2024
1 check passed
@clanktron clanktron deleted the container branch March 28, 2024 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size/M Denotes an issue/PR requiring a relatively moderate amount of work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants