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

Allow callers to set log output #309

Merged
merged 1 commit into from
Feb 16, 2022
+44 −25
Merged

Allow callers to set log output #309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 1 addition & 2 deletions ecr-login/cli/docker-credential-ecr-login/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (
"os"

ecr "github.com/awslabs/amazon-ecr-credential-helper/ecr-login"
"github.com/awslabs/amazon-ecr-credential-helper/ecr-login/api"
"github.com/awslabs/amazon-ecr-credential-helper/ecr-login/config"
"github.com/awslabs/amazon-ecr-credential-helper/ecr-login/version"
"github.com/docker/docker-credential-helpers/credentials"
Expand All @@ -42,5 +41,5 @@ func main() {
}

config.SetupLogger()
credentials.Serve(ecr.ECRHelper{ClientFactory: api.DefaultClientFactory{}})
credentials.Serve(ecr.NewECRHelper())
}
46 changes: 37 additions & 9 deletions ecr-login/ecr.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"
"io"

"github.com/sirupsen/logrus"

Expand All @@ -26,7 +27,34 @@ import (
var notImplemented = errors.New("not implemented")

type ECRHelper struct {
ClientFactory api.ClientFactory
clientFactory api.ClientFactory
logger *logrus.Logger
Copy link
Contributor

Choose a reason for hiding this comment

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

How about using FieldLogger?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this give any flexibility to redirect/discard the output? I'm not very familiar with logrus personally, but it seems to only support log-writing and field-setting methods: https://pkg.go.dev/github.com/sirupsen/logrus#FieldLogger

Copy link
Contributor

Choose a reason for hiding this comment

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

You can either create a logrus.Logger and then set its Out as you did, or use https://github.com/sirupsen/logrus/blob/fdf1618bf7436ec3ee65753a6e2999c335e97221/hooks/test/test.go#L42.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. In either of those cases, my code has to be aware that logrus is involved, and take an explicit direct dependency on it.

go-containerregistry and many of its users don't depend on logrus directly today, and might only depend on it through ecr-login. Exposing that dependency through ecr-login's API means you can't change it in the future (if you want to) without breaking folks. By only exposing stdlib interfaces in the API, callers can keep a somewhat tidier dependency graph, and you retain some flexibility for the future. AFAIK nobody has expressed a desire to pass a different formatter.

Copy link
Contributor Author

@imjasonh imjasonh Feb 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

As an example, here's how I intend to use this affordance in ko to disable ecr-login's log message, without having to expose logrus as an implementation detail to ko.

https://github.com/google/ko/pull/586/files#diff-0924b0ef75727624e13d12e7f4b9362bbdb2b2e8ee8a9f5692f9be5bd8041f4fR46

If this PR is merged, I'd also make a similar change to tools like cosign, Tekton, Kaniko, etc., for the same reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. Sorry I was assuming you would be using logrus too.

}

type Option func(*ECRHelper)

func WithClientFactory(clientFactory api.ClientFactory) Option {
return func(e *ECRHelper) {
e.clientFactory = clientFactory
}
}

func WithLogOutput(w io.Writer) Option {
return func(e *ECRHelper) {
e.logger.Out = w
}
}

func NewECRHelper(opts ...Option) *ECRHelper {
e := &ECRHelper{
clientFactory: api.DefaultClientFactory{},
logger: logrus.StandardLogger(),
}
for _, o := range opts {
o(e)
}

return e
}

// ensure ECRHelper adheres to the credentials.Helper interface
Expand All @@ -45,7 +73,7 @@ func (ECRHelper) Delete(serverURL string) error {
func (self ECRHelper) Get(serverURL string) (string, string, error) {
registry, err := api.ExtractRegistry(serverURL)
if err != nil {
logrus.
self.logger.
WithError(err).
WithField("serverURL", serverURL).
Error("Error parsing the serverURL")
Expand All @@ -54,30 +82,30 @@ func (self ECRHelper) Get(serverURL string) (string, string, error) {

var client api.Client
if registry.FIPS {
client, err = self.ClientFactory.NewClientWithFipsEndpoint(registry.Region)
client, err = self.clientFactory.NewClientWithFipsEndpoint(registry.Region)
if err != nil {
logrus.WithError(err).Error("Error resolving FIPS endpoint")
self.logger.WithError(err).Error("Error resolving FIPS endpoint")
return "", "", credentials.NewErrCredentialsNotFound()
}
} else {
client = self.ClientFactory.NewClientFromRegion(registry.Region)
client = self.clientFactory.NewClientFromRegion(registry.Region)
}

auth, err := client.GetCredentials(serverURL)
if err != nil {
logrus.WithError(err).Error("Error retrieving credentials")
self.logger.WithError(err).Error("Error retrieving credentials")
return "", "", credentials.NewErrCredentialsNotFound()
}
return auth.Username, auth.Password, nil
}

func (self ECRHelper) List() (map[string]string, error) {
logrus.Debug("Listing credentials")
client := self.ClientFactory.NewClientWithDefaults()
self.logger.Debug("Listing credentials")
client := self.clientFactory.NewClientWithDefaults()

auths, err := client.ListCredentials()
if err != nil {
logrus.WithError(err).Error("Error listing credentials")
self.logger.WithError(err).Error("Error listing credentials")
return nil, fmt.Errorf("ecr: could not list credentials: %v", err)
}

Expand Down
20 changes: 6 additions & 14 deletions ecr-login/ecr_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import (
"testing"

ecr "github.com/awslabs/amazon-ecr-credential-helper/ecr-login/api"
"github.com/awslabs/amazon-ecr-credential-helper/ecr-login/mocks"
mock_api "github.com/awslabs/amazon-ecr-credential-helper/ecr-login/mocks"
"github.com/docker/docker-credential-helpers/credentials"
"github.com/stretchr/testify/assert"
)
Expand All @@ -36,9 +36,7 @@ func TestGetSuccess(t *testing.T) {
factory := &mock_api.MockClientFactory{}
client := &mock_api.MockClient{}

helper := &ECRHelper{
ClientFactory: factory,
}
helper := NewECRHelper(WithClientFactory(factory))

factory.NewClientFromRegionFn = func(_ string) ecr.Client { return client }
client.GetCredentialsFn = func(serverURL string) (*ecr.Auth, error) {
Expand All @@ -62,9 +60,7 @@ func TestGetError(t *testing.T) {
factory := &mock_api.MockClientFactory{}
client := &mock_api.MockClient{}

helper := &ECRHelper{
ClientFactory: factory,
}
helper := NewECRHelper(WithClientFactory(factory))

factory.NewClientFromRegionFn = func(_ string) ecr.Client { return client }
client.GetCredentialsFn = func(serverURL string) (*ecr.Auth, error) {
Expand All @@ -78,7 +74,7 @@ func TestGetError(t *testing.T) {
}

func TestGetNoMatch(t *testing.T) {
helper := &ECRHelper{}
helper := NewECRHelper(WithClientFactory(nil))

username, password, err := helper.Get("not-ecr-server-url")
assert.True(t, credentials.IsErrCredentialsNotFound(err))
Expand All @@ -90,9 +86,7 @@ func TestListSuccess(t *testing.T) {
factory := &mock_api.MockClientFactory{}
client := &mock_api.MockClient{}

helper := &ECRHelper{
ClientFactory: factory,
}
helper := NewECRHelper(WithClientFactory(factory))

factory.NewClientWithDefaultsFn = func() ecr.Client { return client }
client.ListCredentialsFn = func() ([]*ecr.Auth, error) {
Expand All @@ -113,9 +107,7 @@ func TestListFailure(t *testing.T) {
factory := &mock_api.MockClientFactory{}
client := &mock_api.MockClient{}

helper := &ECRHelper{
ClientFactory: factory,
}
helper := NewECRHelper(WithClientFactory(factory))

factory.NewClientWithDefaultsFn = func() ecr.Client { return client }
client.ListCredentialsFn = func() ([]*ecr.Auth, error) {
Expand Down