Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Log more details on cache warming errors #898

Merged
merged 4 commits into from
Jan 26, 2018

Conversation

marccarre
Copy link
Contributor

@marccarre marccarre commented Jan 12, 2018

Fixes #895.

With this PR, Flux now logs:

ts=2018-01-25T11:39:40.147644595Z \
  auth="{map[index.docker.io:<registry creds for marccarrecicd@index.docker.io, from default:secret/dockerhub-secret>]}" \
  caller=warming.go:154 \
  component=warmer \
  canonical_name=index.docker.io/marccarre/go-hello-world \
  err="requesting tags: Get https://index.docker.io/v2/marccarre/go-hello-world/tags/list: unauthorized: incorrect username or password"

Note that password is completely masked.

Not sure if we still want this PR though, given this comment and #897.

@squaremo
Copy link
Member

#897 puts the logging behind a --registry-trace flag, and I think there's still value in also logging when there's been an error.

However, I'd request that it just omits passwords entirely -- I think it's more important to say where the credentials came from (struct cred now has a String method which summarises that) so people can investigate for themselves. Emitting any part of a password (as well as the length) is not sound.

@marccarre
Copy link
Contributor Author

Thanks a lot for the feedback @squaremo, I'll rebase this branch on #897's one and make the suggested changes 👍

Copy link
Member

@squaremo squaremo left a comment

Choose a reason for hiding this comment

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

Code review inline and request for change in the comment above (re omitting passwords).

@@ -46,8 +46,8 @@ type backlogItem struct {
registry.Credentials
}

// Continuously get the images to populate the cache with, and
// populate the cache with them.
// Loop continuously get the images to populate the cache with,

This comment was marked as abuse.

This comment was marked as abuse.

@@ -151,7 +150,7 @@ func (w *Warmer) warm(ctx context.Context, logger log.Logger, id image.Name, cre
tags, err := client.Tags(ctx)
if err != nil {
if !strings.Contains(err.Error(), context.DeadlineExceeded.Error()) && !strings.Contains(err.Error(), "net/http: request canceled") {
logger.Log("err", errors.Wrap(err, "requesting tags"))
logger.Log("err", errors.Wrapf(err, "requesting tags for %v from %v using %v", id.CanonicalName(), creds.Hosts(), creds))

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@marccarre
Copy link
Contributor Author

marccarre commented Jan 25, 2018

Also note that we now log additional details even when there is no error, e.g. when the registry is made public and the credentials are removed from the manifest:

ts=2018-01-25T11:58:41.349125409Z \
  caller=warming.go:190 \
  component=warmer \
  canonical_name=index.docker.io/marccarre/go-hello-world \
  creds={map[]} \
  fetching=marccarre/go-hello-world \
  total=2 \
  expired=2 \
  missing=0

ts=2018-01-25T11:58:41.481967446Z \
  caller=warming.go:238 \
  component=warmer \
  canonical_name=index.docker.io/marccarre/go-hello-world \
  creds={map[]} \
  updated=marccarre/go-hello-world \
  count=2

If too verbose, I can change the PR to only log canonical_name and creds on errors.
Let me know.

EDIT: Too verbose indeed.

@@ -123,3 +123,7 @@ func (cs Credentials) Merge(c Credentials) {
cs.m[k] = v
}
}

func (cs Credentials) String() string {
return fmt.Sprintf("{%v}", cs.m)

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@marccarre marccarre force-pushed the issues/895-improve-logging branch 4 times, most recently from 8bdaaad to 0d3df07 Compare January 26, 2018 14:39
@marccarre marccarre merged commit 9203375 into master Jan 26, 2018
@marccarre marccarre deleted the issues/895-improve-logging branch January 26, 2018 14:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants