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

Switch to using CLI for everything except running the container #1421

Open
wants to merge 35 commits into
base: main
Choose a base branch
from

Conversation

yuvipanda
Copy link
Collaborator

@yuvipanda yuvipanda commented Feb 26, 2025

This PR originally was to try to convert just push to using the CLI, but expanded to
make some changes to how we were using docker-py for anything other than running
containers. Running containers should also be switched to using the CLI, but in a separate
PR.

  1. Remove find_images method in the ContainerEngine interface, as it was
    only being used inefficiently to iterate through all images to check if a single
    image exists. Instead we now use inspect_image (which can return a None
    if the image does not exist). This is a breaking change for other container engines
  2. Remove the separate push method from the ContainerEngine interface. This is a
    breaking change.
  3. Add push and load params to build. push will automatically push the resulting
    image to the registry, while load will ensure it's loaded onto the local image store and be
    ready to run. push never is called without a build in our codebase, so this is fine. It allows us
    to not load the image into the local image store when only pushing, as is the case with binderhub.
    load is only set if the image is to be run by repo2docker.
  4. Use the CLI for pushing images.
  5. Change mocked tests of find_image to 'real' tests
  6. Add a proper push test, including running an 'insecure' registry with a separate DIND to push to.
  7. Validate that registry_credentials is in the right form

For authentication, we preserve existing authentication information handling behavior:

  1. If registry_credentials are present, they are used but not leaked
    on to existing ~/.docker/config
  2. If registry_credentials are not present, they are not used
  3. Regardless of registry_credentials being present, we still will use
    existing authentication info in DOCKER_CONFIG

This authentication behavior is tested as well!

Removed functionality

This does remove the functionality of testing at start time if docker api is accessible, and failing if it is not. This is because we now have two ways to access the docker api - cli and python, and I don't want to repeatedly do it for both (plus it was causing the registry test to fail, even though that only used the cli). I propose we let this be, and add it back once we get rid of dockerpy completely.

TODO

  • Use stdin to pass password to docker login
  • Fix tests
  • Validate registry_credentials

Fixes #1414

Preserves existing authentication information handling behavior:

1. If registry_credentials are present, they are used but not leaked
   on to existing ~/.docker/config
2. If registry_credentials are not present, they are not used
3. Regardless of registry_credentials being present, we still will use
   existing authentication info in DOCKER_CONFIG
@yuvipanda yuvipanda changed the title [WIP] Use CLI to login and push Use CLI to push docker image Feb 26, 2025

env = os.environ.copy()
subprocess.check_call(
# FIXME: This should be using --password-stdin instead
Copy link
Member

@manics manics Feb 26, 2025

Choose a reason for hiding this comment

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

It might be easier to manipulate the config.json file directly:

json.dumps({"auths":{"<might need a scheme here://>registry.host":{"auth":b64encode(f"{user}:{token}".encode()).decode()}}})

It's also what we do to create the BinderHub secret:
https://github.com/jupyterhub/binderhub/blob/579dd78430422bae896c90d69ffad637d9fcfc26/helm-chart/binderhub/templates/_helpers.tpl#L40

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@manics I did consider that, but then we'll have to merge the existing ~/.docker/config file with the new setup carefully. I would rather just outsource it to docker login like this instead.

@yuvipanda
Copy link
Collaborator Author

The failing tests are because of the mock for the existing push. This is also why we couldn't catch #1413

@yuvipanda
Copy link
Collaborator Author

So, I'm just going to run a docker registry and test. Unfortunately while docker push works fine with registries on http, docker login does not without us changing docker daemon config to allow insecure-registries. So just for this test, I'm going to have to run a custom dind as well to test :D but totally doable though.

pre-commit-ci bot and others added 2 commits February 28, 2025 17:44
Also properly setup TLS for docker daemon to close security hole
@yuvipanda yuvipanda changed the title Use CLI to push docker image Remove docker-py dependency Feb 28, 2025
@yuvipanda
Copy link
Collaborator Author

ok, moving run to CLI is a lot more work haha so i'm going to hold off on that, and try to scope this down.

@yuvipanda yuvipanda marked this pull request as draft February 28, 2025 20:24
@yuvipanda
Copy link
Collaborator Author

I'm going to scope this back to just trying to implement docker push via the CLI again :)

@yuvipanda yuvipanda changed the title Remove docker-py dependency Switch to using CLI for everything except running the container Feb 28, 2025
@yuvipanda yuvipanda marked this pull request as ready for review February 28, 2025 22:07
@yuvipanda
Copy link
Collaborator Author

@minrk @manics ok, this is ready for review now! \o/

@yuvipanda yuvipanda requested a review from minrk February 28, 2025 23:08
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.

Refactor container runtime interface
2 participants