amazon-ecr-credential-helper: Don't throw exceptions from ECRHelper methods

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
}

About this issue

Original URL
State: closed
Created 5 years ago
Comments: 15 (5 by maintainers)

Most upvoted comments

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 on Mar 30, 2019

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

julienduchesne on Mar 14, 2019

@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 "$@"

samuelkarp on Mar 14, 2019