Skip to content
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.

Sign up for GitHub

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

Optionally ignore docker's request for storing / deleting credentials #315

Closed
wants to merge 5 commits into from
Closed

Optionally ignore docker's request for storing / deleting credentials #315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
39e8b95
Implement both the Add and Delete operations as nops
dotboris Mar 11, 2022
6eb364e
Fix typo in add's nop log
dotboris Mar 11, 2022
2540a1c
Use WithField when logging warning
dotboris Mar 14, 2022
2c6ec84
Gate docker creds ignore feature behind env var
dotboris Mar 14, 2022
c2915f7
Apply suggestions from code review
dotboris Jul 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ Docker to work with the helper.
To build and install the Amazon ECR Docker Credential Helper, we suggest Go
1.19 or later, `git` and `make` installed on your system.

If you just installed Go, make sure you also have added it to your PATH or
If you just installed Go, make sure you also have added it to your PATH or
Environment Vars (Windows). For example:

```
Expand Down Expand Up @@ -213,7 +213,7 @@ include:
* An [IAM role for Amazon EC2](http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/iam-roles-for-amazon-ec2.html)

To use credentials associated with a different named profile in the shared credentials file (`~/.aws/credentials`), you
may set the `AWS_PROFILE` environment variable.
may set the `AWS_PROFILE` environment variable.

The Amazon ECR Docker Credential Helper reads and supports some configuration options specified in the AWS
shared configuration file (`~/.aws/config`). To disable these options, you must set the `AWS_SDK_LOAD_CONFIG` environment
Expand All @@ -236,12 +236,13 @@ in the *AWS Command Line Interface User Guide*.
The credentials must have a policy applied that
[allows access to Amazon ECR](http://docs.aws.amazon.com/AmazonECR/latest/userguide/ecr_managed_policies.html).

### Amazon ECR Docker Credential Helper
### Amazon ECR Docker Credential Helper

| Environment Variable | Sample Value | Description |
| --------------------- | ------------- | ------------------------------------------------------------------ |
| AWS_ECR_DISABLE_CACHE | true | Disables the local file auth cache if set to a non-empty value |
| AWS_ECR_CACHE_DIR | ~/.ecr | Specifies the local file auth cache directory location |
| Environment Variable | Sample Value | Description |
| ---------------------------- | ------------- | ------------------------------------------------------------------ |
| AWS_ECR_DISABLE_CACHE | true | Disables the local file auth cache if set to a non-empty value |
| AWS_ECR_CACHE_DIR | ~/.ecr | Specifies the local file auth cache directory location |
| AWS_ECR_IGNORE_CREDS_STORAGE | true | Ignore calls to docker login or logout and pretend they succeeded |

## Usage

Expand Down
36 changes: 30 additions & 6 deletions ecr-login/ecr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"errors"
"fmt"
"io"
"os"

"github.com/sirupsen/logrus"

Expand Down Expand Up @@ -70,14 +71,37 @@ func NewECRHelper(opts ...Option) *ECRHelper {
// ensure ECRHelper adheres to the credentials.Helper interface
var _ credentials.Helper = (*ECRHelper)(nil)

func (ECRHelper) Add(creds *credentials.Credentials) error {
// This does not seem to get called
return notImplemented
func shouldIgnoreCredsStorage() bool {
return os.Getenv("AWS_ECR_IGNORE_CREDS_STORAGE") == "true"
}

func (ECRHelper) Delete(serverURL string) error {
// This does not seem to get called
return notImplemented
// Add tries to store credentials when docker requests it. This usually happens during `docker login` calls. In our context,
// storing arbitrary user given credentials makes no sense.
func (self ECRHelper) Add(creds *credentials.Credentials) error {
if shouldIgnoreCredsStorage() {
self.logger.
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.")
Comment on lines +83 to +86
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@therealvio therealvio Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@therealvio therealvio Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return nil
} else {
return notImplemented
}
}

// Delete tries to delete credentials when docker requests it. This usually happens during `docker logout` calls. In our context, we
// don't store arbitrary user given credentials so deleting them makes no sense.
func (self ECRHelper) Delete(serverURL string) error {
if shouldIgnoreCredsStorage() {
self.logger.
WithField("serverURL", serverURL).
Warning("Ignoring request to delete credentials. " +
"This is not supported in the context of the docker ecr-login helper.")
return nil
} else {
return notImplemented
}
}

func (self ECRHelper) Get(serverURL string) (string, string, error) {
Expand Down
81 changes: 81 additions & 0 deletions ecr-login/ecr_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package ecr
import (
"errors"
"fmt"
"os"
"testing"

ecr "github.com/awslabs/amazon-ecr-credential-helper/ecr-login/api"
Expand Down Expand Up @@ -118,3 +119,83 @@ func TestListFailure(t *testing.T) {
assert.Error(t, err)
assert.Len(t, serverList, 0)
}

func TestAddIgnored(t *testing.T) {
factory := &mock_api.MockClientFactory{}

helper := NewECRHelper(WithClientFactory(factory))

os.Setenv("AWS_ECR_IGNORE_CREDS_STORAGE", "true")
err := helper.Add(&credentials.Credentials{
ServerURL: proxyEndpoint,
Username: "AWS",
Secret: "supersecret",
})

assert.Nil(t, err)
}

func TestAddNotImplemented(t *testing.T) {
tests := []struct {
name string
setEnv func()
}{
{"unset", func() { os.Unsetenv("AWS_ECR_IGNORE_CREDS_STORAGE") }},
{"false", func() { os.Setenv("AWS_ECR_IGNORE_CREDS_STORAGE", "false") }},
{"0", func() { os.Setenv("AWS_ECR_IGNORE_CREDS_STORAGE", "0") }},
{"empty string", func() { os.Setenv("AWS_ECR_IGNORE_CREDS_STORAGE", "") }},
}

for _, test := range tests {
t.Run(test.name, func(tt *testing.T) {
factory := &mock_api.MockClientFactory{}

helper := NewECRHelper(WithClientFactory(factory))

test.setEnv()
err := helper.Add(&credentials.Credentials{
ServerURL: proxyEndpoint,
Username: "AWS",
Secret: "supersecret",
})

assert.Error(t, err, "not implemented")
})
}
}

func TestDeleteIgnored(t *testing.T) {
factory := &mock_api.MockClientFactory{}

helper := NewECRHelper(WithClientFactory(factory))

os.Setenv("AWS_ECR_IGNORE_CREDS_STORAGE", "true")
err := helper.Delete(proxyEndpoint)

assert.Nil(t, err)
}

func TestDeleteNotImplemented(t *testing.T) {
tests := []struct {
name string
setEnv func()
}{
{"unset", func() { os.Unsetenv("AWS_ECR_IGNORE_CREDS_STORAGE") }},
{"false", func() { os.Setenv("AWS_ECR_IGNORE_CREDS_STORAGE", "false") }},
{"0", func() { os.Setenv("AWS_ECR_IGNORE_CREDS_STORAGE", "0") }},
{"empty string", func() { os.Setenv("AWS_ECR_IGNORE_CREDS_STORAGE", "") }},
}

for _, test := range tests {
t.Run(test.name, func(tt *testing.T) {
factory := &mock_api.MockClientFactory{}

helper := NewECRHelper(WithClientFactory(factory))

test.setEnv()
err := helper.Delete(proxyEndpoint)

assert.Error(t, err, "not implemented")
})
}
}
Loading