Add Prometheus metrics for authentication attempts #15794
Conversation
openshift-merge-robot
added
the
size/M
|
/unassign |
pkg/auth/prometheus/metrics.go
Outdated
| } | ||
|
|
||
| func UpdateAuthCounters(user, path, result string) { | ||
| authCounterTotal.WithLabelValues().Inc() |
There was a problem hiding this comment.
The cardinality of metrics that are labeled by username are going to be really high. What's the lifetime of these metrics?
pkg/auth/prometheus/metrics.go
Outdated
| Subsystem: AuthSubsystem, | ||
| Name: "auth_count_user_path", | ||
| Help: "Counts total authentication attempts, by user and request path", | ||
| }, []string{"user", "path"}, |
There was a problem hiding this comment.
cardinality of this is too high. Either use api request info and bucket by resource and verb, or get rid of this.
There was a problem hiding this comment.
Basically we cannot have any unbounded cardinality metrics (like path) in the system.
|
I removed the high cardinality label metrics. |
pkg/auth/server/login/login.go
Outdated
| user, ok, err := l.auth.AuthenticatePassword(username, password) | ||
| if err != nil { | ||
| glog.Errorf(`Error authenticating %q with provider %q: %v`, username, l.provider, err) | ||
| failed(errorpage.AuthenticationErrorCode(err), w, req) | ||
| result = "failure" |
There was a problem hiding this comment.
Should probably distinguish this result as "error"
mrogers950
force-pushed
the
auth_prom
branch
2 times, most recently
from
ecf26d4
to
d0ef2a3
Compare
|
@liggitt @smarterclayton lmk if there are any other changes needed. |
|
@liggitt @mfojtik @smarterclayton Could you please take a look at this, this is adressing a 3.7 commited card. |
pkg/auth/prometheus/metrics.go
Outdated
| @@ -0,0 +1,36 @@ | |||
| package prometheus | |||
There was a problem hiding this comment.
Call this package "metrics", we have no plans to support an alternate metrics sysem
pkg/auth/prometheus/metrics.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| AuthSubsystem = "auth_subsystem" |
There was a problem hiding this comment.
Private unless there is another reason.
Use "openshift_auth" to better describe (subsystem is redundant in public metrics
pkg/auth/prometheus/metrics.go
Outdated
| prometheus.MustRegister(authCounterResult) | ||
| } | ||
|
|
||
| func UpdateAuthCounters(result string) { |
There was a problem hiding this comment.
Generally we don't abstract metrics recording like this. Call this method "record"
Signed-off-by: Matt Rogers <mrogers@redhat.com>
mrogers950
force-pushed
the
auth_prom
branch
from
d0ef2a3
to
cc5aa91
Compare
|
@smarterclayton updated, and rebased. |
|
/retest |
1 similar comment
|
/retest |
|
flake #15900 |
|
/retest |
|
/test all |
|
flake #16273 |
|
/retest |
| user, ok, err := l.auth.AuthenticatePassword(username, password) | ||
| if err != nil { | ||
| glog.Errorf(`Error authenticating %q with provider %q: %v`, username, l.provider, err) | ||
| failed(errorpage.AuthenticationErrorCode(err), w, req) | ||
| result = "error" |
There was a problem hiding this comment.
I 'd prefer the result be a constant
There was a problem hiding this comment.
Generally we don't create constants for single use variables.
| var result string | ||
| defer func() { | ||
| metrics.Record(result) | ||
| }() | ||
| user, ok, err := l.auth.AuthenticatePassword(username, password) | ||
| if err != nil { | ||
| glog.Errorf(`Error authenticating %q with provider %q: %v`, username, l.provider, err) |
There was a problem hiding this comment.
Note, this should be utilruntime.HandleError(...), not glog.Errorf().
There was a problem hiding this comment.
I opened a separate PR for that.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mrogers950, smarterclayton The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
openshift-merge-robot
added
the
approved
|
Automatic merge from submit-queue (batch tested with PRs 16411, 16139, 16430, 16435, 15794) |
Automatic merge from submit-queue (batch tested with PRs 16411, 16139, 16430, 16435, 15794) Add Prometheus metrics for authentication attempts @openshift/sig-security
@openshift/sig-security