Skip to content

Conversation

@miminar
Copy link
Contributor

@miminar miminar commented Sep 20, 2016

Docker can store auth entries with schema prefix or even path suffix.
See an example:

{
    "auths": {
        "10.3.10.88:5000": {
            ...
        },
        "http://10.3.10.88:5000/v2/": {
            ...
        },
        "https://10.3.10.88:5000": {
            ...
        },
        "https://index.docker.io/v1/": {
            ...
        }
    }
}

The entries were created using command docker login of upstream Docker
1.12. Let's normalize the auth keys before trying to match against
hostname.

Signed-off-by: Michal Minář [email protected]

@runcom
Copy link
Member

runcom commented Sep 20, 2016

yeah, that behavior was totally wrong and I've already fixed that upstream in 1.13-dev moby/moby#23100 - from now on only hostname[:port]

@runcom
Copy link
Member

runcom commented Sep 20, 2016

BUT, this is breaking CI in skopeo...

@runcom
Copy link
Member

runcom commented Sep 20, 2016

we need some unit test here also...

if err != nil {
continue
}
normalizedAuths[u.Host] = v
Copy link
Member

Choose a reason for hiding this comment

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

this should contain host[:post] - CI fails on localhost:5000 for instance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does. Nevertheless, I reworked it completely.

@miminar miminar force-pushed the benevolent-dockercfg-parsing branch 3 times, most recently from da53d12 to 6b3ac99 Compare October 6, 2016 12:43
@miminar
Copy link
Contributor Author

miminar commented Oct 6, 2016

Reworked according to @runcom's changes in moby/moby#23100.

And added unit tests.

@miminar
Copy link
Contributor Author

miminar commented Oct 6, 2016

Ahh, one more thing, I need to return nil for error on missing config files.

@runcom
Copy link
Member

runcom commented Oct 6, 2016

Ahh, one more thing, I need to return nil for error on missing config files.

right, that's how it works in docker - but we really need to have a way to specify the config location (not in this PR ofc)

}

// bad luck; let's normalize the entries first
registry = normalizeRegistry(registry)
Copy link
Member

Choose a reason for hiding this comment

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

wait, this whole method's logic comes from docker and I don't want to mess with it because it's actually working, can you move this code here and below to another function/method instead?

@runcom
Copy link
Member

runcom commented Oct 6, 2016

@miminar tests failing though:

--- FAIL: TestReferenceNewImage (0.00s)
    Error Trace:    docker_transport_test.go:164
    Error:      Received unexpected error:
            /home/travis/.docker/config.json - stat /home/travis/.docker/config.json: no such file or directory

Docker can store auth entries with schema prefix or even path suffix.
See an example:

    {
        "auths": {
            "10.3.10.88:5000": {
                ...
            },
            "http://10.3.10.88:5000/v2/": {
                ...
            },
            "https://10.3.10.88:5000": {
                ...
            },
            "https://index.docker.io/v1/": {
                ...
            }
        }
    }

The entries were created using command `docker login` of upstream Docker
1.12. Let's normalize the auth keys before trying to match against
hostname.

Signed-off-by: Michal Minář <[email protected]>
@miminar miminar force-pushed the benevolent-dockercfg-parsing branch from 6b3ac99 to d30079f Compare October 6, 2016 13:06
@miminar
Copy link
Contributor Author

miminar commented Oct 6, 2016

@runcom fixed.

@runcom
Copy link
Member

runcom commented Oct 6, 2016

LGTM thanks for the tests @miminar !

Approved with PullApprove

@mtrmac
Copy link
Collaborator

mtrmac commented Oct 6, 2016

👍 thanks!

Approved with PullApprove

@mtrmac mtrmac merged commit e62573c into containers:master Oct 6, 2016
@miminar miminar deleted the benevolent-dockercfg-parsing branch October 7, 2016 11:54
wking added a commit to wking/containers-image that referenced this pull request Feb 25, 2019
Shifted a number of auth-getting unit tests from docker_client_test.go
into config_test.go, since they only excercise config.go logic.
They'd been in their previous location since landing in d30079f (Be
benevolent to .docker/config.json file, 2016-09-20, containers#96).

Signed-off-by: W. Trevor King <[email protected]>
wking added a commit to wking/containers-image that referenced this pull request Feb 25, 2019
Shifted a number of auth-getting unit tests from docker_client_test.go
into config_test.go, since they only excercise config.go logic.
They'd been in their previous location since landing in d30079f (Be
benevolent to .docker/config.json file, 2016-09-20, containers#96).

Signed-off-by: W. Trevor King <[email protected]>
wking added a commit to wking/containers-image that referenced this pull request Feb 27, 2019
Shifted a number of auth-getting unit tests from docker_client_test.go
into config_test.go, since they only excercise config.go logic.
They'd been in their previous location since landing in d30079f (Be
benevolent to .docker/config.json file, 2016-09-20, containers#96).

Signed-off-by: W. Trevor King <[email protected]>
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.

3 participants