Don't throw exceptions from ECRHelper methods

[bertramn]

There are 2 comments in the Add and Delete method implementation of the Docker ECRHelper interface that these methods are not needed, followed by a throw error. Incidentally tools like packer DO call these methods resulting in our ECR based container builds failing.

Any chance you can log that the method was called but not throw an error? Something like this is sufficient.

func (ECRHelper) Add(creds *credentials.Credentials) error {
   // This is not implemented but called by packer
   logrus.
      WithField("serverURL", creds.ServerURL).
      WithField("username", creds.Username).
      Warn("credentials store method was called which is currently not implemented")
   return nil
}
func (ECRHelper) Delete(serverURL string) error {
   // This is not implemented but called by packer
   logrus.
      WithField("serverURL", serverURL).
      Warn("credentials erase method was called which is currently not implemented")
   return nil
}

[samuelkarp]

Hi @bertramn, thanks for opening this issue. I'm curious to know what code is expecting to call Add and Delete when using this helper; is it code that expects the methods to actually do something or is it just boilerplate and you're really okay with it being ignored?

I'm not sure how I feel about changing these to appear successful when they're not. In general, I'm not a fan of software that doesn't tell me when it's wrong, but I'm assuming you don't have full control over whatever is invoking the Add and Delete commands?

[bertramn]

I tried to use the ECR helper with packer to build docker images. When I look at the log record, it appears that Packer is calling these APIs after authenticating to delegate storing of the credentials (e.g. a mechanism you will find in the OSX keychain helper). Not an expert but the logic of calling the API makes sense, the part of how you mean to "chain" the cred helpers does not. If the Add and Delete methods fail, my packer builds fail.

[julienduchesne]

If I may chime in too. Having these commands crash makes it harder to migrate from a workflow where $(aws ecr get-login --no-include-email) is called to using this.

Our specific use case is this: We want to use this for Jenkins slave AMIs but our developers are using lots of logins in their pipelines. If we install this on AMIs, we will crash everyone's pipelines.

Maybe we shouldn't block a regular login even if we have this installed?

[julienduchesne]

I opened a PR that removes the error. However, the docker login now does nothing and it is hidden behavior as you say. Maybe a passthrough could be better? Fulfill the docker login requests if they are given, otherwise use the credsStore?

[samuelkarp]

@bertramn @julienduchesne Would it be a reasonable work-around to use a wrapper around the credential helper that discards those commands? Something like this:

$ cat docker-credential-ecr-login-no-error 
#!/bin/bash
# Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
# SPDX-License-Identifier: Apache-2.0

case "$1" in
  store|erase)
    exit 0
    ;;
esac

docker-credential-ecr-login "$@"

[julienduchesne]

Absolutely. However, packer seems too specific. Maybe no-error or something.

[samuelkarp]

Absolutely. However, packer seems too specific. Maybe no-error or something.

Sure, I was providing that as an example. Do either of you still want the credential helper itself changed or are you happy using a wrapper script like that one instead?

[julienduchesne]

I just tried it and it works fine. Personally, I'm ok with this solution. Thanks @samuelkarp

[bertramn]

I'd prefer the functionality to reside in the binary, wrapper scripts are just a hack and no one remembers why they are there and what they do exactly.

Can we not add a command arg like --disable-cred-storage to the ECRHelper to be "lenient" and not throw the error? This way folks (like myself) would have to deliberately instruct the helper to ignore any not implemented errors and it would otherwise be backward compatible with current functionality.

In the future it may be worth to somehow delegate these 2 method calls to another docker helper that manages the login in a cache or on osx the keychain. I have not seen chaining of helpers anywhere but it would appear to make sense to login to ECR and then have the creds stored.

[samuelkarp]

Can we not add a command arg like --disable-cred-storage to the ECRHelper to be "lenient" and not throw the error?

We can't add flags, as the credential helper is directly invoked by the Docker CLI and there's no way to customize what flags Docker will pass. However, we could add an environment variable as those will propagate through the fork/execve boundary.

I have not seen chaining of helpers anywhere but it would appear to make sense to login to ECR and then have the creds stored.

Chaining is really the right behavior and is something I've been interested in doing, but have lacked sufficient time to do so. I think the right mechanism to chain would be a change in the Docker CLI itself so that you could configure the chain in ~/.docker/config.json, but it would also be possible to implement a chaining credential helper that had its own configuration file.

[bertramn]

The longer I think about this, the less convinced I am that throwing an error in Add and Delete methods is a good idea. The credential helper is using the externally managed AWS context to produce the credentials for the login action. Why exactly do you need to store/remove the ECR login token? The ECR token expire reasonably quickly and the only way to get new ones is by asking the ecr-credential-helper to fetch new ones.

The only possible benefit that a chained credential helper could provide is to cache the token but would that not be an optimisation that the go SDK equivalent of aws ecr get-authorization-token should implement? I mean right now we do this token fetching business in shell scripts and crikey that script generates "a few" calls a day without any caching or like.

Just because the 2 functions are not implemented does not mean the cred helper does not work - BUT - throwing an error in those 2 unimplemented functions does break the otherwise functional ecr-credential-helper.

Easiest way to fix ecr-credential-helper is to remove the throw error statement in Add and Delete methods - low complexity and no breaking change in functionality.

[bertramn]

Any further thoughts on fixing this issue? We are a year in and still maintaining a fork of your project to get our packer builds to work. This is extremely annoying since we cannot use amazonlinux extras to just enable and install but need to maintain, build, release and then distribute this hack fork.

[bertramn]

@samuelkarp are we going to get any resolution to this issue?

[samuelkarp]

Hey @bertramn! I'd like to keep the default behavior as-is. I provided a work-around above (see https://github.com/awslabs/amazon-ecr-credential-helper/issues/154#issuecomment-472988526). I'd also be comfortable accepting a contribution adding a conditional behavior keyed off the presence of an environment variable.

[bertramn]

That is unfortunate - the hack with the shell script is not an option for us.