Optionally ignore docker's request for storing / deleting credentials #315
Conversation
dotboris
changed the title
|
Let me check with Docker folks first. GCP is also having the same issue (see https://github.com/GoogleCloudPlatform/docker-credential-gcr/blob/e8a16dd0fa5bb661e304ad850a61ccfaa901045d/credhelper/helper.go#L62-L75). So I'd like to have a vendor-neutral solution. |
|
I'd rather like to see the feature implemented.. where it simply would raise a warning similar to... WARNING! Using --password via the CLI is insecure. Use --password-stdin. but rather WARNING! Using credentials from AWS ECR credential helper. Docker login ignored. |
|
Commenting to nudge this 😅 Something like this would be a big help for my team. We're trying to migrate our CI from using |
|
Would it be possible to add this change now and reconfigure if/when there is a central way to manage this? |
|
I'm guessing this project is no longer being developed. The last release was over a year ago. This would have been very helpful. |
|
In my opinion an environment variable shouldn't even be needed. The helper is caching the token anyway, so the By throwing errors (as it's doing currently), this helper is essentially not correctly implementing the docker credential helper spec, as can be seen when numerous people are having issues with this helper when |
|
Is there any movement to getting this PR reviewed? We ran into this problem today that |
These two operations are called when docker tries to store and delete credentials (docker login and docker logout) respectively. This change makes it so that both of these operations are implemented as nops instead of returning a "not implemented" error. The goal here is to ensure compatibilities with applications and tools that call docker login or docker logout for the user as part of their normal operations.
sparr
force-pushed
the
creds-add-delete-nop
branch
from
e2c53bc to
2c6ec84
Compare
Co-authored-by: Gavin Inglis <[email protected]>
| WithField("username", creds.Username). | ||
| WithField("serverURL", creds.ServerURL). | ||
| Warning("Ignoring request to store credentials. " + | ||
| "This is not supported in the context of the docker ecr-login helper.") |
There was a problem hiding this comment.
Wouldn't it be more useful to output this error message when shouldIgnoreCredsStorage() is not true? and add something like
To ignore this error, set env value `AWS_ECR_IGNORE_CREDS_STORAGE` to true
There was a problem hiding this comment.
We're interested in getting this change merged. I have forked and stood up this PR through here: #847 so we can get feedback addressed and hopefully merged soon. Can you please relay feedback for this PR there and I can look at resolving these.
In the meantime:
Can you elaborate on what you mean @swagatbora90? I am not quite sure on what the intention of this suggestion is.
There was a problem hiding this comment.
I did some thinking on this, and I think I get what you mean, though unfortunately this would maintain the status quo and not provide the outcome we're looking for here. If this storage of credentials was implemented, then yes, your change would make sense in this case. This is not the case though, we're trying to ignore an error.
To elaborate: My understanding is that we're preserving the notImplemented error if the environment variable isn't being passed, making this ignore capability a choice for the user. The current state of things is that we have no capabilities to just authenticate without storing the credential. Whether or not this is supported doesn't matter. The bottom line is: we want to get the auth done and move on.
The environment variable is meant to provide a means to skip the attempt at storing the credential and throw a non-blocking warning rather than an error. If we inverted the condition for checking shouldIgnoreCredsStorage, users will still get the blocking notImplemented error and doesn't satisfy what the change was intending to achieve. The test TestAddIgnored reflects this behaviour too.
|
Is there any hope that this will see some traction and get merged? |
|
@dotboris Are you still working on this? Looks like there is a lot of interest to get this feature in. |
|
I'm not. I haven't touched this in ages. I don't really have the bandwidth to work on this. If someone wants to take over, feel free to do so. |
|
@swagatbora90 I can pick this up if you're able to review? |
|
This PR can be closed in favour of #847 which has picked up the work and finished it off. |
|
Update for onlookers: The changes proposed in here, which were completed in #847 have now been merged in. Enjoy. |
Issue #, if available: #102 & #154
Description of changes:
Currently, both the
AddandDeleteoperations in this helper return anot implementederror. This can lead to issues wheredocker loginanddocker logoutstop working for the user while displaying a confusingnot implementederror.Following the suggestion in #154 (comment), this PR adds an environment variable (
AWS_ECR_IGNORE_CREDS_STORAGE) to optionally ignore these requests and return no error. WhenAWS_ECR_IGNORE_CREDS_STORAGE=true, instead of returning anot implementederror, we returnnil.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.