search

singularityhub / sregistry-cli

Singularity Global Client for container management
https://singularityhub.github.io/sregistry-cli/
Mozilla Public License 2.0
14 stars 18 forks source link

Docker hub base is None when fetching manifests (Regression) #237

Closed jackh726 closed 5 years ago

jackh726 commented 5 years ago

Describe the bug

MESSAGELEVEL=DEBUG sregistry pull docker://ubuntu:latest
VERBOSE Obtaining manifest: None/library/ubuntu/manifests/latest v1
DEBUG GET None/library/ubuntu/manifests/latest
VERBOSE Obtaining manifest: None/library/ubuntu/manifests/latest v2
DEBUG GET None/library/ubuntu/manifests/latest
VERBOSE Obtaining manifest: None/library/ubuntu/manifests/latest config
DEBUG GET None/library/ubuntu/manifests/latest
ERROR Failed to find manifest. Check image name and login.

This was caused by https://github.com/singularityhub/sregistry-cli/commit/091f90e65ea335e777ba01093e1649823884cfec#diff-6e2f85ce20d990b2553b906dd69214dbR44

Now that the super is valid, this causes the self.base set here https://github.com/singularityhub/sregistry-cli/blob/ab8c38747bc181ba67641f93ad884b8bd4c70702/sregistry/main/docker/__init__.py#L109 to be overriden here https://github.com/singularityhub/sregistry-cli/blob/ab8c38747bc181ba67641f93ad884b8bd4c70702/sregistry/main/base/__init__.py#L54

Version 0.2.12, which was released prior to that commit, works as expected.

To Reproduce Steps to reproduce the behavior:

MESSAGELEVEL=DEBUG sregistry pull docker://ubuntu:latest
vsoch commented 5 years ago

Taking a look!

vsoch commented 5 years ago

hey @jackh726 I have a fix for you here -> https://github.com/singularityhub/sregistry-cli/pull/238 basically I moved the function to set the base to after the super, where we check for None. Do you want to give it a test, and let me know if anything else needs work? Thanks!

jackh726 commented 5 years ago

@vsoch That fixes it for me!

jackh726 commented 5 years ago

@vsoch Unsure if related, but I'll bring it up now in case it is (still trying to figure out if it's me) I'm actually using a private docker hub. I've set SREGISTRY_DOCKERHUB_USERNAME and SREGISTRY_DOCKERHUB_PASSWORD environment variables. However, whenever I try to pull an image, I'm getting

ERROR Unrecognized authentication challenge, exiting.

It still ends with no manifests being found because of this.

jackh726 commented 5 years ago

Ok, it is indeed not related to the original issue. But the 401 response has the following headers:

server: nginx/1.14.0
date: Tue, 16 Jul 2019 21:52:14 GMT
content-type: application/json; charset=utf-8
content-length: 166
connection: keep-alive
docker-distribution-api-version: registry/2.0
www-authenticate: Basic realm="Registry Realm"
x-content-type-options: nosniff

So the regex here doesn't match: https://github.com/singularityhub/sregistry-cli/blob/54c0c3ed18a4bae005863ae706e1b07b37a6297f/sregistry/main/docker/api.py#L49

I can open a separate issue if you need.

Edit: Actually it is related. The Authorization headers also also getting reset in the super init call.

vsoch commented 5 years ago

Thanks for the detailed headers! No need to open up another, we can cover them both here. It's dinner time here so I'll do this first thing tomorrow.

jackh726 commented 5 years ago

No problem! To be clear, I think there are two separate issues here: 1) The Authorization headers are getting reset because ApiConnection.init is getting called after Client._reset_headers() 2) When there isn't Authorization headers, the 401 response isn't parsed correctly. I haven't looked if other headers are also missing that would change the 401 response to be as expected.

Let me know if there's anything else you need from me!

vsoch commented 5 years ago

Sorry I don't think I'll be able to fit this in today, too overloaded. :/

vsoch commented 5 years ago

Ok thank goodness finally working on this! Let's first try re-ordering the auth headers - it could be (I think) that we don't need to request any token because it's a Basic authorization (and how would you ask for a token without a scope and service?) But given that this doesn't work, we will need to form a request for a token (can you peek at what this looks like given the Basic www-Authorization with just a Realm? Or is it some base64 encoded derivation of the username/password?

vsoch commented 5 years ago

https://github.com/singularityhub/sregistry-cli/pull/238/commits/2b57a5330beb1d9cb255736886403266ad61fbbf#diff-6e2f85ce20d990b2553b906dd69214db

vsoch commented 5 years ago

Actually, it looks like we already do this in update_secrets so I just moved that one down too (after the super);

        if username is not None and password is not None:
            auth = basic_auth_header(username, password)
            self.headers.update(auth)
jackh726 commented 5 years ago

@vsoch just tested your pull request branch and it all works for me!

vsoch commented 5 years ago

Awesome! So ready for merge and release?