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

Jump to bottom

[DO NOT MERGE] registry: Suppress unnecessary unauthorized errors #10715

Closed
wants to merge 1 commit into from
Closed

[DO NOT MERGE] registry: Suppress unnecessary unauthorized errors #10715

wants to merge 1 commit into from

Conversation

miminar
Copy link

@miminar miminar commented Aug 30, 2016 •
edited
Loading

During a cross-repo mount request, it's quite common to forbid access to a source repository picked by Docker daemon regardless of user having access there. This produces error messages that appear severe but in fact they're not. This patch delays their logging until the very last repository access check and lowers their severity.

Closes #9649

@miminar
Copy link
Author

miminar commented Aug 30, 2016

[test]

@liggitt PTAL

liggitt
liggitt reviewed Aug 30, 2016
View reviewed changes
pkg/dockerregistry/server/repositorymiddleware.go Outdated

logger.Debugf("Origin auth: found deferred error for %s/%s: %v", namespace, name, repoErr)
return repoErr
return checkPendingErrors(ctx, r.namespace, r.name)
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the difference between the ctx passed in here and the ctx held by the repo. it was being done this way to preserve the original logging, but I don't know what additional info is logged by the ctx held by the repo

Copy link
Author

Choose a reason for hiding this comment

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

what is the difference between the ctx passed in here and the ctx held by the repo. it was being done this way to preserve the original logging, but I don't know what additional info is logged by the ctx held by the repo

In general, the most up to date context should be used for logging. Theoretically, context held by the repo may be stale, which would result in incorrect logging statements. But most probably, the two context objects will be the same.

Using r.ctx is mostly a hold-over from times when middleware methods used to lack the context argument.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why you need the (repository).checkPendingErrors after this change?
Just a wrapper which adds two arguments ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@miminar still there.

Copy link
Author

Choose a reason for hiding this comment

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

Just a wrapper which adds two arguments ?

@legionus exactly. I've removed the wrapper.

@liggitt liggitt added this to the 1.3.1 milestone Aug 30, 2016
@miminar miminar force-pushed the registry-cross-mount-logging-fixes branch from 220a2ae to 45a2b28 Compare August 31, 2016 07:45
@miminar
Copy link
Author

miminar commented Aug 31, 2016

Thanks! Items hopefully addressed.

@miminar miminar force-pushed the registry-cross-mount-logging-fixes branch from 45a2b28 to 5ce125c Compare August 31, 2016 08:59
@miminar
Copy link
Author

miminar commented Aug 31, 2016

Flakes #10738 and #8427 in job https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8574/

re-[test]

@smarterclayton smarterclayton modified the milestones: 1.3.1, 1.4.0 Sep 19, 2016
@miminar
Copy link
Author

miminar commented Oct 5, 2016

@liggitt May I ask for one more pass?

@ncdc
Copy link
Contributor

ncdc commented Nov 22, 2016

Is this still for 1.4?

@miminar
Copy link
Author

miminar commented Nov 23, 2016

Is this still for 1.4?

Probably not. It's a low risk though.

@legionus would you please take a look?

@miminar miminar modified the milestones: 1.4.0, 1.5.0 Nov 23, 2016
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 2, 2017
legionus
legionus suggested changes Feb 2, 2017
View reviewed changes
pkg/dockerregistry/server/auth.go Outdated
// Default to openshift if not present
realm = "origin"
// Default to origin if not present
realm = defaultRealm
Copy link
Contributor

Choose a reason for hiding this comment

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

Use getStringOption ?

Copy link
Author

Choose a reason for hiding this comment

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

It's there now.

@miminar miminar force-pushed the registry-cross-mount-logging-fixes branch from 5ce125c to 4d42379 Compare February 7, 2017 12:47
@miminar
Copy link
Author

miminar commented Feb 7, 2017

@legionus PTAL one more time.

legionus
legionus approved these changes Feb 7, 2017
View reviewed changes
Copy link
Contributor

@legionus legionus left a comment

Choose a reason for hiding this comment

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

LGTM

@miminar miminar force-pushed the registry-cross-mount-logging-fixes branch from 4d42379 to dce3327 Compare February 7, 2017 14:54
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 7, 2017
registry: Suppress unnecessary unauthorized errors
18c554e 
During a cross-repo mount request, it's quite common to forbid access to
a source repository picked by Docker daemon regardless of user having
access there. This produces error messages that appear severe but
they're not. This patch delays their logging until the very last
repository access check and lowers their severity.

Signed-off-by: Michal Minář <miminar@redhat.com>
@miminar miminar force-pushed the registry-cross-mount-logging-fixes branch from dce3327 to 18c554e Compare February 8, 2017 15:03
@miminar
Copy link
Author

miminar commented Feb 8, 2017

Strange, I don't see an error in https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_conformance/11139/console. Just:

[ERROR] PID 7548: test/extended/conformance.sh:75: `return $out` exited with status 1.
[INFO] 		Stack Trace: 
[INFO] 		  1: test/extended/conformance.sh:75: `return $out`
[INFO]   Exiting with code 1.
Error while running ssh/sudo command:

Re-starting.

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 18c554e

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13710/) (Base Commit: 76141a4)

@miminar
Copy link
Author

miminar commented Feb 9, 2017

@mfojtik this seem to be good for merge.

@mfojtik
Copy link
Contributor

mfojtik commented Feb 9, 2017

@miminar this seems big, I would prefer fork_ami and QE sign off before merging. WDYT?

@miminar
Copy link
Author

miminar commented Feb 9, 2017

@mfojtik I don't find it necessary. But for your deep calm and sound sleep, I'll do it 😉.

@miminar
Copy link
Author

miminar commented Feb 21, 2017

Started fork_ami at https://ci.openshift.redhat.com/jenkins/job/fork_ami/329/

@miminar miminar changed the title registry: Suppress unnecessary unauthorized errors [DO NOT MERGE] registry: Suppress unnecessary unauthorized errors Mar 7, 2017
@miminar
Copy link
Author

miminar commented Mar 7, 2017

QE found out some unexpected entries in logs. It looks like this needs some more work.

@smarterclayton smarterclayton modified the milestones: 1.5.0, 1.6.0 Mar 12, 2017
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 12, 2017
@openshift-bot openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 5, 2017
@openshift-bot openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 16, 2017
@openshift-bot openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 25, 2017
@openshift-bot openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 9, 2017
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 16, 2017
@openshift-bot
Copy link
Contributor

Origin Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 16, 2017
@miminar miminar deleted the registry-cross-mount-logging-fixes branch November 8, 2017 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@legionus legionus legionus approved these changes

Assignees
No one assigned
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Milestone
3.6.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@miminar @ncdc @openshift-bot @mfojtik @liggitt @legionus @smarterclayton