-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Do not fail when no docker registry auth is available #2744
Conversation
this amends the behaviour introduced with hashicorp#2651 and allows pulling public images when docker.auth.helper is set
So, I guess this is a case of what "makes sense" vs. what "upstream" does. Personally, if we treat non-zero exit as null return, rather than exceptional, there isn't really a way for scripts to truly HALT the process or signal abnormal errors. Without actual definitions of expected statuses, I think all non-zero statuses should be considered exceptional. Although, there is naturally also a case for following the upstream precedent. I think this is a perfect example of that. You have my blessing. (But please do also prod the docker guys, so they actually DOCUMENT this) |
@rawler I don't mind diverging from upstream behavior. I think this is very specific config anyway. At least not stopping should be the default, otherwise there's no way to use dockerhub public images. |
Unless something else is clearly specified, I'd say that every exit-status
except 0 should be an error-code. (I.E. compare the early-termination of
shells in && and ||)
An example of this would be a creds-helper reading credentials from Vault.
If no valid-token is given - meaning the script can not check if vault has
credentials to use for this repo - it should return non-zero status, and an
error-message on stdout. (Possibly duplicated in JSON on stdout) In this
case, I think Nomad should fail the start, in the spirit of avoiding silent
errors*.
If, on the other hand, vault is consulted and verified to have no
credentials for the requested repo, it should return status 0, and an empty
JSON-doc on stdout.
Just my 5c
* It IS possible that the repo requires the auth and would have worked fine
without auth, but my personal preference leans towards explicit failures
rather than hidden errors. Logs are very rarely checked until long after an
error appears)
2017-06-27 12:43 GMT+02:00 Arvid E. Picciani <[email protected]>:
… @rawler <https://github.com/rawler> I don't mind diverging from upstream
behavior. I think this is very specific config anyway.
What would be your preferred way of signaling stopping vs
not stopping ? And why would you want to stop anyway?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2744 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAA13EPiX1uQNpzJamtDwj6AgHRObd-jks5sINzNgaJpZM4OGW3w>
.
|
I would personally prefer the current behaviour, i don't want my clients end up in a state where some public containers work, but all private containers fail - i would rather want the entire node to be failing hard and loud :) |
Thanks for the PR and great discussion! I'm going to merge this but then put the behavior behind a task config option for task's using the Docker driver. It will default to the existing behavior of hard failing as I think it's a more secure default to encourage people who have their own repo to rely on it for all of their images. (Update: This way if you have a task using a public image and you don't want to copy it over: you can just override the behavior for that task.) |
PR up with a test binary if anyone is willing to give it a spin/review: #2786 |
@schmichael this doesnt seem to be a global config option. setting this in every job configuration is fine but very confusing. A user would still have no idea what to do if an image simply doesn't download from dockerhub, and has no way to figure out that its from a global auth plugin. the docs don't say anything like "if you want to use public images in the same way as docker swarm or kubernetes, set auth_soft_fail". |
@aep Created a new issue #3030 to track this. Definitely is poorly documented and confusing! Implicit fallbacks from private-to-public repos makes me very nervous, but it's worth considering.
Update: We'll probably rework the |
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
this amends the behaviour introduced with #2651
and allows pulling public images when docker.auth.helper is set to https://github.com/awslabs/amazon-ecr-credential-helper