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

docs: README could improve DX guidance in Develop section (Docker) #3918

Open
3 of 5 tasks
polarathene opened this issue Jan 5, 2025 · 1 comment
Open
3 of 5 tasks
Labels
bug Something is not working.

Comments

@polarathene
Copy link

Preflight checklist

Ory Network Project

No response

Describe the bug

Originally reported here: #3914 (comment)

The project README provides build instructions for make docker followed by using the quickstart.yml for contributors interested in developing for Hydra via Docker:

image

However the image built from make docker tags by default :latest-sqlite. There is no mention of adjusting IMAGE_TAG, nor that the quickstart.yml would need to be adjusted (configured to pull the the DockerHub v2.2.0 image):

export IMAGE_TAG := $(if $(IMAGE_TAG),$(IMAGE_TAG),latest)

hydra/Makefile

Lines 71 to 74 in 8e5fae4

# Build local docker images
.PHONY: docker
docker:
DOCKER_BUILDKIT=1 DOCKER_CONTENT_TRUST=1 docker build --progress=plain -f .docker/Dockerfile-build -t oryd/hydra:${IMAGE_TAG}-sqlite .

hydra/quickstart.yml

Lines 12 to 14 in 8e5fae4

services:
hydra:
image: oryd/hydra:v2.2.0


Minor inconsistency noticed

TL;DR:

  • It doesn't seem necessary for a separate Alpine image with the sqlite package? The functionality works fine without the package installed AFAIK. Perhaps in the past Hydra builds dynamically linked to the library which required that?
  • The distroless variant on the other hand lacks sqlite support in it's static binary builds (up until a recent PR in Aug 2024, so this should be resolved in future releases published).
  • Both images should technically use the same static binary, and ideally hydra version would be able to display any additional tags the binary was built with (or equivalent to inspect supported build-time features).

Also note that the image built is Dockerfile-build (two stage, Go image for the build stage, Google Distroless for final stage), not Dockerfile-sqlite (Alpine with sqlite package).

Actually there is no sqlite package for the Debian distroless image used instead, presumably because Hydra is doing a static build with SQLite, but prior to that PR there doesn't seem like there was any sqlite package used either?

I assume this is referring to the Go build with -tags sqlite, and that the Alpine Dockerfile with sqlite package doesn't actually need to include sqlite when Hydra is built without dynamically linking the lib.

Alpine has minimal dynamic linking, while the Debian distroless image is fully static:

services:
  hydra:
    image: localhost/hydra:v2.2.0
    environment:
      DSN: sqlite:///var/lib/sqlite/db.sqlite?_fk=true
    # Unlike the current quickstart.yml example which relies upon the Docker Compose `depends_on`
    # feature to apply DB migrations through another container instance, just run both commands here:
    entrypoint: ["ash", "-c"]
    command:
      - |
        /opt/hydra-alp --config /etc/hydra/config.yml migrate sql --read-from-env --yes
        /opt/hydra-alp --config /etc/hydra/config.yml serve all --dev --sqa-opt-out
    # Build image locally (no need to try pull from a remote registry):
    pull_policy: build
    build:
      dockerfile_inline: |
        FROM alpine
        RUN mkdir -p /var/lib/sqlite
        COPY --from=oryd/hydra:v2.2.0 /usr/bin/hydra /opt/hydra-alp
        COPY --from=oryd/hydra:v2.2.0-distroless /usr/bin/hydra /opt/hydra-deb
        ADD https://raw.githubusercontent.com/ory/hydra/refs/heads/master/contrib/quickstart/5-min/hydra.yml /etc/hydra/config.yml

The above compose.yaml example is self contained, it is a rough approximation of the current quickstart.yml but embeds a Dockerfile to build and run with the example config bundled into the image.

That's so I can also verify that it functions with sqlite correctly with docker compose up, but the two copies of Hydra are to inspect how they differ:

$ docker compose run --rm -it hydra ash
$ apk add file


# Both builds from the same commit:
$ /opt/hydra-alp version
Version:    v2.2.0
Git Hash:   57096be9befbde4a1436ef48338d253a248c91c4
Build Time: 2024-02-12T11:21:55Z
$ opt/hydra-deb version
Version:    v2.2.0
Git Hash:   57096be9befbde4a1436ef48338d253a248c91c4
Build Time: 2024-02-12T11:21:55Z


# Alpine slightly larger:
$ du -bh /opt/hydra-alp
32.4M   /opt/hydra-alp
$ du -bh /opt/hydra-deb
30.6M   /opt/hydra-deb


# Alpine is oddly not a static build:
$ file /opt/hydra-alp
/opt/hydra-alp: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib/ld-musl-x86_64.so.1, Go BuildID=O5hYJHtx1bh63uS-jF6k/o42cMO9pv_BWZftOYIW3/z2CZ8b0W7k1DNVKp0d8Q/XF36JaeS6xco_aBezXNI, stripped


# While distroless is:
$ file /opt/hydra-deb
/opt/hydra-deb: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), statically linked, Go BuildID=CwRDstsTehsBpHBDR85C/rFz5SIrsj4c3dMZ9RgQq/iu_-uQSL6JFAHc3fJghg/ACGx2ItWVzu0ZSVXefF8, stripped


# Alpine build only links to musl:
$ ldd /opt/hydra-alp
  /lib/ld-musl-x86_64.so.1 (0x7f2dd5ab5000)
  libc.so => /lib/ld-musl-x86_64.so.1 (0x7f2dd5ab5000)

These binaries were published in Feb 2024, while the Dockerfile build change to get proper static linking while still using CGO_ENABLED=1 was later in Aug 2024. I've not tried building for comparison, but assume these two binaries may have been built slightly differently for some reason? Their Dockerfiles just COPY the hydra binary into the image, but AFAIK provided the build wasn't for glibc with hidden dlopen() calls (which do require dynamic linking, but that Aug 2024 PR should opt-out of), then the images should probably use the same static binary in future releases.

Sure enough, with the current Debian binary built, despite the static linking reported, it fails with sqlite:

$ docker compose -f quickstart.yaml run --rm -it hydra ash
/opt/hydra-deb --config /etc/hydra/config.yml migrate sql --read-from-env --yes 2>&1 | grep -vE 'migration_file|Applied|Pending'

Error: the DSN connection string looks like a SQLite connection, but SQLite support was not built into the binary. Please check if you have downloaded the correct binary or are using the correct Docker Image. Binary archives and Docker Images indicate SQLite support by appending the -sqlite suffix

I don't know if there's a command for Hydra that reports what tags it was built with, perhaps that could be added to the hydra version command?

Reproducing the bug

Follow the instructions from README, make docker && docker compose -f quickstart.yml up, the image built won't match the image tag in quickstart.yml, thus it won't be used.

Setting IMAGE_TAG for make docker isn't sufficient since it appends the -sqlite suffix, so manually editing quickstart.yml is required (Docker Compose does support ENV interpolation with default values, but due to the -sqlite suffix in the Makefile you could not share IMAGE_TAG and would need another ENV).

Relevant log output

No response

Relevant configuration

No response

Version

2.2.0

On which operating system are you observing this issue?

None

In which environment are you deploying?

None

Additional Context

As this is only a minor inconvenience that most will resolve locally without issue.

Ory could consider improving the DX here though, even if that's just a minor hint that quickstart.yml would need to be modify the image names.

@polarathene polarathene added the bug Something is not working. label Jan 5, 2025
@aeneasr
Copy link
Member

aeneasr commented Jan 7, 2025

However the image built from make docker tags by default :latest-sqlite. There is no mention of adjusting IMAGE_TAG, nor that the quickstart.yml would need to be adjusted (configured to pull the the DockerHub v2.2.0 image)
It doesn't seem necessary for a separate Alpine image with the sqlite package? The functionality works fine without the package installed AFAIK. Perhaps in the past Hydra builds dynamically linked to the library which required that?

Adjusting the image tag is fine by me, we separate -sqlite images from non-sqlite images because the former needs CGO while the latter doesn't. Usually, you don't need SQLite support and thus CGO in prod images but certainly do need it in development.

The idea behind publishing two images for that is to clearly separate CGO with non-CGO builds.

The distroless variant on the other hand lacks sqlite support in it's static binary builds (up until a recent PR in Aug 2024, so this should be resolved in future releases published).

We can add sqlite support to distroless, but in my view there's little use in prod images for SQLite as it doesn't scale and can't handle competing transactions (thus will cause a lot of 500s)

Both images should technically use the same static binary, and ideally hydra version would be able to display any additional tags the binary was built with (or equivalent to inspect supported build-time features).

That would indeed be nice!

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

No branches or pull requests

2 participants