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 /auth for docker compatibility #9589

Merged

Conversation

troyready
Copy link
Contributor

@troyready troyready force-pushed the add_compat_auth_endpoint branch 10 times, most recently from c93ed4c to 0228bc7 Compare March 3, 2021 09:34
@troyready
Copy link
Contributor Author

The core element of this is working now (tested and verified with AWS ECR).

I'm not sure how to proceed with a unit test for it. If I understand correctly, it would mean setting up a docker registry.

Would appreciate a second set of eyes on the testing for an idea of how to go forward.

@mheon
Copy link
Member

mheon commented Mar 3, 2021

@edsantiago might have an answer on testing

@edsantiago
Copy link
Member

Ugh. This is a solved problem for our system tests in CI, but not for apiv2 tests. It's probably about a 2- to 3-hour effort to set it up for apiv2 tests; unfortunately I won't have that time until mid-March.

@rhatdan
Copy link
Member

rhatdan commented Mar 5, 2021

LGTM
I am fine with getting this in, without testing in CI, since it is a feature we are missing.

@rhatdan
Copy link
Member

rhatdan commented Mar 5, 2021

@containers/podman-maintainers PTAL
@baude @jwhonce PTAL

@edsantiago
Copy link
Member

Quick comment: I'm working on a testing framework for this. It's not ready, but one thing I've found is that this might not be compatible with docker. The docker API documentation implies that serveraddress should be a full URL, of the form https://host:port/v2. That doesn't work with your code; your code works if I pass in just host:port. Can you look into this? (I might be mistaken).

@edsantiago
Copy link
Member

Also: the tlsVerify(false) code is never triggering; I can't figure out how to make it trigger (possibly because serveraddress is getting set internally somehow to https://localhost which will never match http://localhost)

Also: on successful auth I get {"IdentityToken":"","Status":"Login Succeeded"} - shouldn't IdentityToken be non-null?

@edsantiago
Copy link
Member

@troyready try this:

$ make          ! (in your source tree, with these changes)
$ git pr 9669   ! requires git-extras
$ ./test/apiv2/test-apiv2 auth
Generating a RSA private key
.......................................++++
......++++
writing new private key to '/dev/shm/test-apiv2.tmp.33vdZT/registry/auth/domain.key'
-----
ok 1 [60-auth] POST /v1.40/auth [-d {"username":"uRIU5jdv","password":"WrOnGPassWord","serveraddress":"localhost:5066/"}] : status=400
ok 2 [60-auth] POST /v1.40/auth [-d {"username":"uRIU5jdv","password":"WrOnGPassWord","serveraddress":"localhost:5066/"}] : .Status ('login attempt to localhost:5066/ failed with status: unable to retrieve auth token: invalid username/password: unauthorized') ~ .* invalid username/password
ok 3 [60-auth] POST /v1.40/auth [-d {"username":"uRIU5jdv","password":"pVhgy0Bi","serveraddress":"localhost:5066/"}] : status=200
ok 4 [60-auth] POST /v1.40/auth [-d {"username":"uRIU5jdv","password":"pVhgy0Bi","serveraddress":"localhost:5066/"}] : .Status=Login Succeeded
not ok 5 [60-auth] POST /v1.40/auth [-d {"username":"uRIU5jdv","password":"pVhgy0Bi","serveraddress":"localhost:5066/"}] : .IdentityToken
#  expected: ~ [a-zA-Z0-9]
#    actual:
1..5

That will run my tests against your PR, with a local registry. It will fail, because See Above. To iterate:

$ git co your-working-branch
edit-edit-edit
$ make
$ git co pr/9669

HTH.

(Postscript: the results above partly-succeed only because I hardcoded skipTLS=false)

@troyready
Copy link
Contributor Author

Thanks for looking into it @edsantiago !

The docker API documentation implies that serveraddress should be a full URL, of the form https://host:port/v2. That doesn't work with your code; your code works if I pass in just host:port. Can you look into this? (I might be mistaken).

I think I ran into the same issue, where getting the http (no s) through from the api wasn't working (mangling it like pre-pending a https// or something akin to that). So as is, the tlsVerify(false) conditional isn't really useful.

To be clear, this seems to be specific to testing (which I think is the only scenario where TLS would be avoided -- this doesn't appear to be an issue when using it with a trusted https:// address (my actual integration tests were run against an AWS ECR).

Also: on successful auth I get {"IdentityToken":"","Status":"Login Succeeded"} - shouldn't IdentityToken be non-null?

The IdentityToken appears to be optional; when testing the same API using Docker against AWS ECR, I get the same empty string.

@troyready troyready force-pushed the add_compat_auth_endpoint branch 4 times, most recently from 250395b to 629bf36 Compare March 11, 2021 08:47
@troyready
Copy link
Contributor Author

Thanks again @edsantiago -- with the new test setup I was able to figure out what was going wrong. Latest commit fixes the issue: tests are working now, and I've confirmed it still works with a normal live (trusted TLS) repo as well.

(I'm pretty sure the zuul build failure is unrelated to changes here)

@edsantiago
Copy link
Member

/hold

@troyready could you please rebase, then update your added .at file to reflect the new parameter format (space-separated)? Now that #9686 has merged, if your PR merges it will break all of CI for everyone. I apologize for the inconvenience but hope that the new POST param format makes things easier.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 12, 2021
@troyready troyready force-pushed the add_compat_auth_endpoint branch from 629bf36 to 955aacc Compare March 12, 2021 18:51
@troyready
Copy link
Contributor Author

Sure thing, done @edsantiago

@edsantiago
Copy link
Member

Tests LGTM but I'll leave final approval to @jwhonce or @baude. Thank you for your efforts on this.

IdentityToken appears to be optional

I still find this weird, and pointless, but hey, if it matches docker, shrug. Thank you for testing against docker.

@TomSweeneyRedHat
Copy link
Member

LGTM

@rhatdan
Copy link
Member

rhatdan commented Mar 16, 2021

Great work @troyready Thanks.
/lgtm
/approve
/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 16, 2021
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 16, 2021
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, troyready

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 16, 2021
@openshift-merge-robot openshift-merge-robot merged commit d9f8469 into containers:master Mar 16, 2021
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

compatibility api should implement /auth endpoint
7 participants