Allow callers to set log output #309
Conversation
| @@ -26,7 +27,20 @@ import ( | |||
| var notImplemented = errors.New("not implemented") | |||
|
|
|||
| type ECRHelper struct { | |||
| ClientFactory api.ClientFactory | |||
| clientFactory api.ClientFactory | |||
| logger *logrus.Logger | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
If this PR is merged, I'd also make a similar change to tools like cosign, Tekton, Kaniko, etc., for the same reason.
There was a problem hiding this comment.
Right. Sorry I was assuming you would be using logrus too.
ecr-login/ecr.go
Outdated
| } | ||
| } | ||
|
|
||
| func (self ECRHelper) WithLogOutput(w io.Writer) ECRHelper { |
There was a problem hiding this comment.
I'm inclined to use FieldLogger here as well. The caller may want to customize the logger (e.g. use JSON logging)
There was a problem hiding this comment.
This would tie ecr-login to logrus as its logging implementation via its exported API. I'm not sure how important that flexibility is, but currently, and with this change, ecr-login could change to another logging library under the hood, without callers being aware or needing to make any changes.
AFAIK, if this only supported WithFieldLogger, a caller that wanted to disable ecr-login's logging entirely would have to implement a discarding FieldLogger implementation, or else take a dependency on logrus and call logrus.New().SetOutput(ioutil.Discard).
If you think it would be a useful addition as a separate method, I'm happy to add it though.
|
How about using https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis instead of the Builder pattern? So that most of clients can do |
|
Yeah I can do that too if you'd like. One sec. edit: Done, and updated |
|
Looks good! Can you squash the commits to make the history tidy? Please keep |
Signed-off-by: Jason Hall <jasonhall@redhat.com>
7ea3aa6 to
276ba67
Compare
|
Friendly ping. I've squashed the commits, let me know if there's anything else you'd like to see. |
Reported in GoogleContainerTools/kaniko#1955 Changes in awslabs#309 made it possible for callers depending on this code as a Go library to redirect log output to ioutil.Discard to avoid spurious log statements. Unfortunately, because in that change the logger being used was the shared global logger.StandardLogger(), redirecting its output to ioutil.Discard to quiet ecr-login also disabled all logging using the shared global logger (like Kaniko does).
Reported in GoogleContainerTools/kaniko#1955 Changes in awslabs#309 made it possible for callers depending on this code as a Go library to redirect log output to ioutil.Discard to avoid spurious log statements. Unfortunately, because in that change the logger being used was the shared global logger.StandardLogger(), redirecting its output to ioutil.Discard to quiet ecr-login also disabled all logging using the shared global logger (like Kaniko does). Signed-off-by: Jason Hall <jasonhall@redhat.com>
writes to the requested output , instead of changing the behavior of the standard logrus logger. Rename WithLogOutput to WithLogger to make this new behavior clearer. Use non-global logger to avoid unexpected global log discarding Reported in GoogleContainerTools/kaniko#1955 Changes in awslabs#309 made it possible for callers depending on this code as a Go library to redirect log output to ioutil.Discard to avoid spurious log statements. Unfortunately, because in that change the logger being used was the shared global logger.StandardLogger(), redirecting its output to ioutil.Discard to quiet ecr-login also disabled all logging using the shared global logger (like Kaniko does). Signed-off-by: Jason Hall <jasonhall@redhat.com>
Use a new logrus.Logger instance instead of the default logrus singleton. Relying on the default singleton is problematic when dependencies (or transitive dependencies) also use logrus. When code paths in these dependencies modify the default singleton, it also impacts Skaffold logging. I ran into this issue while trying to upgrade the `ko` dependency to v0.10, and there was unwanted error-level log output messages from `amazon-ecr-credential-helper`. Context: - ko-build/ko#586 - awslabs/amazon-ecr-credential-helper#309 This change also moves us closer to not leaking the logger implementation dependency outside the `output/log` package. Tracking: GoogleContainerTools#7131
Signed-off-by: Jason Hall jasonhall@redhat.com
Description of changes:
Though this code is typically consumed as a CLI cred helper configured for use with the
dockerCLI, we've also found it to be really useful as a Go client for generating ECR credentials from the environment.go-containerregistry, which is used by Tekton, Knative,ko,cosign, Kaniko, Buildpacks and more, provides an adapter between its auth-providingauthn.Keychaininterface, and Docker'scredential.Helperinterface, which you implement here. This means that these tools can (and some already do!) embed the cred helper's logic with:For example:
And this works great! 🎉 These clients don't have to install the cred helper CLI and configure it using
~/.docker/config.jsonto pick up any available ECR credentials; they Just Work™️.Except...
Through this usage, this library logs a harmless error when we check whether a particular possibly-non-ECR registry can be used with this code. In its normal CLI usage,
dockerwon't invoke the ecr-login cred helper unless it already knows the registry is ECR, so this logging is correct in identifying a weird situation. But in usage as a library, this leads to spammy, sometimes scary log messages, which we'd like to avoid.I tried all manner of terrible hacks to disable
os.Stderr, but due to how logrus works internally, and how this code uses it, it doesn't seem possible without making logrus-specific changes to all those downstream clients.Instead, this PR proposes adding a func that's unused by the CLI code (its logic remains unchanged), to let library consumers set the log output for the scope of ecr-login's usage, which downstream clients can set to
ioutil.Discardif they want. This is quite a bit less invasive than any alternative I came up with, and should impose almost no maintenance burden to you lovely maintainers ❤️This also slightly changes the API for instantiating an
ECRHelperusingNewECRHelper(), though AFAIK the CLI (and our users, whom I'll happily update) depends on that directly.