Add retry import with no auth for IS if we get 401 error (public images case) #18012
Conversation
openshift-ci-robot
added
the
size/S
|
@bparees this one is other card you sent me. Fix works, but to solve this properly would require changes in either docker API in upstream (docker distribution library) or RT in k8s code... Both of them I dont feel taking on as a challenge :D |
| if image.Status.Code == http.StatusUnauthorized { | ||
| importCtx := importer.NewContext(r.transport, r.insecureTransport).WithCredentials(nil) | ||
| imports := r.importFn(importCtx) | ||
| if err := imports.Import(ctx.(gocontext.Context), isi, stream); err != nil { |
There was a problem hiding this comment.
one problem I see with this is if the image really is private, does this turn what was a legitimate 401 into a 404 error when you make the import call with no credentials? (because docker returns 404 for private registries?)
That is, if the scenario is: i'm trying to import from a private registry and I provide invalid credentials, previously I would get a proper 401 and know my credentials were (probably) wrong. Is that what will still happen with this change, or will I now see a 404 error and be confused?
There was a problem hiding this comment.
I did the bunch of tests and checking how this "awesome" docker registry API behaves and when we do this (spent way too much time on this...). This fix for me looked least intrusive and did the job.
I think your case is covered because it still gives 401 but it gives different error message so in importer.go it gets assigned different error message.
There is chance it might behave diferently if it will be used with non docker distribution based docker registry API's (Nexus in example) which will give different error messages. But in this case all our error handling logic is flawed.
The current behavior of our IS importer with private and public images with wrong secret:
-bash-4.4# oc create secret docker-registry dummy-secret1 --docker-server=docker.io --docker-username=dummy1 --docker-password=dummy1 [email protected]
secret "dummy-secret1" created
##### PUBLIC IMAGE
-bash-4.4# oc import-image example --from=openshift/hello-openshift --confirm
! error: Import failed (InternalError): Internal error occurred: Get https://registry-1.docker.io/v2/openshift/hello-openshift/manifests/latest: unauthorized: incorrect username or password
Less than a second ago
error: tag latest failed: Internal error occurred: Get https://registry-1.docker.io/v2/openshift/hello-openshift/manifests/latest: unauthorized: incorrect username or password
##### PRIVATE IMAGE
-bash-4.4# oc import-image example1 --from=mangirdas/kafka-producer:latest --confirm
The import completed with errors.
! error: Import failed (InternalError): Internal error occurred: Get https://registry-1.docker.io/v2/mangirdas/kafka-producer/manifests/latest: unauthorized: incorrect username or password
Less than a second ago
error: tag latest failed: Internal error occurred: Get https://registry-1.docker.io/v2/mangirdas/kafka-producer/manifests/latest: unauthorized: incorrect username or password
With fix:
-bash-4.4# oc create secret docker-registry dummy-secret1 --docker-server=docker.io --docker-username=dummy1 --docker-password=dummy1 [email protected]
secret "dummy-secret1" created
##### PUBLIC IMAGE
-bash-4.4# oc import-image example --from=openshift/hello-openshift --confirm
The import completed successfully.
latest
tagged from openshift/hello-openshift
* openshift/hello-openshift@sha256:1a7a40888035f95c10cde81f04c10aa5919a41766e4680452918fc5591d7b02a
Less than a second ago
##### PRIVATE IMAGE
-bash-4.4# oc import-image example1 --from=mangirdas/kafka-producer:latest --confirm
The import completed with errors.
latest
tagged from mangirdas/kafka-producer:latest
! error: Import failed (Unauthorized): you may not have access to the Docker image "mangirdas/kafka-producer:latest"
Less than a second ago
error: tag latest failed: you may not have access to the Docker image "mangirdas/kafka-producer:latest"
Without secret:
##### PUBLIC IMAGE
-bash-4.4# oc import-image example --from=openshift/hello-openshift --confirm
The import completed successfully.
latest
tagged from openshift/hello-openshift
* openshift/hello-openshift@sha256:1a7a40888035f95c10cde81f04c10aa5919a41766e4680452918fc5591d7b02a
Less than a second ago
PRIVATE IMAGE
-bash-4.4# oc import-image example1 --from=mangirdas/kafka-producer:latest --confirm
latest
tagged from mangirdas/kafka-producer:latest
! error: Import failed (Unauthorized): you may not have access to the Docker image "mangirdas/kafka-producer:latest"
Less than a second ago
error: tag latest failed: you may not have access to the Docker image "mangirdas/kafka-producer:latest"
My secret is OK but the image is in the private repo of other person account:
oc create secret docker-registry dummy-secret1 --docker-server=docker.io --docker-username=mangirdas --docker-password=dummy1 [email protected]
-bash-4.4# oc import-image example2 --from=juozasa/hellonode:latest --confirm
latest
tagged from juozasa/hellonode:latest
! error: Import failed (Unauthorized): you may not have access to the Docker image "juozasa/hellonode:latest"
Less than a second ago
error: tag latest failed: you may not have access to the Docker image "juozasa/hellonode:latest"
|
awesome, thank you for the detailed summary of the behavior! |
openshift-ci-robot
added
lgtm
|
/retest |
|
looks like my local dev env is out of sync with our build environment. Investigating where is delta and why I can see different run results. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest |
|
you've got a compile erorr: |
|
Problem is "Works on my laptop" type one :) will try to align my dev env to test env... ETA - dunno. few days. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@mjudeikis i think the importer package was just restructured. Do a rebase from master. |
|
Thanks @bparees will do on the weekend or today in the evening... . Sev1 on the client site... "Life of the consultant..." :'( |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
mjudeikis
force-pushed
the
bugzilla_1506175
branch
from
917e9ee to
52ff14a
Compare
openshift-merge-robot
removed
the
lgtm
mjudeikis
force-pushed
the
bugzilla_1506175
branch
2 times, most recently
from
ae8d1be to
43054c7
Compare
| // check imported images status. If we get authentication error (401), try import same image without authentication. | ||
| // Docker registry gives 401 on public images if you have wrong secret in your secret list. | ||
| for _, image := range isi.Status.Images { | ||
| if image.Status.Reason == metav1.StatusReasonUnauthorized && strings.Contains(image.Status.Message, "incorrect username or password") { |
There was a problem hiding this comment.
If I dont check for the specific message this breaks 'TestImageStreamImportAuthenticated` test. I still need to understand which behavior of the test I break and if this is the change for the test or change for logic. For now, by adding this it makes the test pass and at the same time solves the bug.
There was a problem hiding this comment.
this sort of check is definitely fragile, i'd prefer to understand what might need to change about the test over adding this contains check.
There was a problem hiding this comment.
we can put this on hold for now as I need to find few days to investigate this more.
There was a problem hiding this comment.
i'd still like to revisit this, but it's an improvement for now so i'm going to merge.
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
|
will try to do some work this week and let you know on Monday. Got stuck with our awesome storage offering these days. |
|
/retest |
|
/retest extended_conformance_gce |
|
/lgtm @mjudeikis you can /hold cancel if you're happy with this. |
|
@bparees I dont know what changes form the first view, but after rebase tests passes. Will check the code base, but It might be harder to find :) The only thing which I would like to add this to make a little bit more cleaner error message.
So now user see a message that cant access image as its secure, and no "hints" to check secret. Suggested message flow to construct error messages:
Still testing this is "chain of actions is even possible in real world"... |
|
you're right. even if we try to import via unauthenticated, if we fail, we should return the original error we got when we tried via authentication, i think. |
mjudeikis
force-pushed
the
bugzilla_1506175
branch
from
43054c7 to
a3c1f98
Compare
|
the only change was now to use previous error message form image status field where its being saved. |
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
| importCtx := registryclient.NewContext(r.transport, r.insecureTransport).WithCredentials(nil) | ||
| imports := r.importFn(importCtx) | ||
| if err := imports.Import(ctx.(gocontext.Context), isi, stream); err != nil { | ||
| return nil, kapierrors.NewInternalError(fmt.Errorf(image.Status.Message)) |
There was a problem hiding this comment.
on further analysis i don't think this is correct. "err" will still be nil, even if the import failed due to credentials, no?
just as the credentials import check above expects err to be nil even if the import was disallowed by auth.
So you're turning a more serious error that occurred during import, into a credential message. I don't think that's right.
There was a problem hiding this comment.
I think I overthinking this... I think let's leave previous error. Standart error when image credentials are wrong is for private image is:
! error: Import failed (Unauthorized): you may not have access to the Docker image "docker.io/juozasa/hellonode:latest"
Less than a second ago
Only when public image is being imported it gives the wrong password when trying with auth header. So only in this case (as much as I manage to test all possible cases), it defaults to this condition. Its just how docker distribution api behaves...
There was a problem hiding this comment.
or we can concatenate both errors to one string but I would better keep it simple... previous way.
There was a problem hiding this comment.
i agree the error should be returned as is.
i'm less clear on what image.status.message is going to end up being in this case (where you provided invalid credentials and the image is not public, but we tried to access it with nil credentials)?
|
@mjudeikis: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
mjudeikis
force-pushed
the
bugzilla_1506175
branch
from
a3c1f98 to
e8c97a8
Compare
openshift-ci-robot
added
size/M
…authentication. This is to eliminate case when we try pull public images with wrong/expired secret and it blocks all imports
mjudeikis
force-pushed
the
bugzilla_1506175
branch
from
e8c97a8 to
f3290cb
Compare
|
Status now. I confirm that cases 2-3 got passed to "second loop to retry without a secret" and got old messages defaulted back after failure. Private image from different API: Public image when we have a wrong secret: |
|
Did some more advanced use case test: Before code: after code: |
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci-robot
added
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bparees, mjudeikis The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
Automatic merge from submit-queue. |
If we try to import any image (even public ones) and we have a bad secret (wrong password or expired cloud token) in our project imports are failing.
This leads to all imports in the same project to fail with a random unclear error.
This could be fixed in RoundTripper and Smarter Authenticator. This might end up bigger change than this fix...
This fix just retries import with empty authentication.
If you know better place for this fix let me know :)
https://bugzilla.redhat.com/show_bug.cgi?id=1506175