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.

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

Fix incorrect authentication latency metric #99944

Conversation

marseel
Copy link
Member

@marseel marseel commented Mar 8, 2021

What type of PR is this?

/kind bug

What this PR does / why we need it:

Authentication latency metric was incorrectly calculated. It took into account request duration.

Does this PR introduce a user-facing change?

Fixed authentication_duration_seconds metric. Previously it included whole apiserver request duration. 

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added release-note-none kind/bug size/XS cncf-cla: yes do-not-merge/needs-sig needs-triage needs-priority area/apiserver sig/api-machinery sig/instrumentation and removed do-not-merge/needs-sig labels Mar 8, 2021
@k8s-ci-robot k8s-ci-robot requested review from deads2k and soltysh Mar 8, 2021
@marseel
Copy link
Member Author

@marseel marseel commented Mar 8, 2021

/sig scalability
/cc @mborsz

@k8s-ci-robot k8s-ci-robot requested a review from mborsz Mar 8, 2021
@k8s-ci-robot k8s-ci-robot added the sig/scalability label Mar 8, 2021
@marseel marseel force-pushed the fix/fix_incorrect_authenticator_metrics branch 2 times, most recently from 2b00fc7 to d0feb44 Compare Mar 8, 2021
@marseel
Copy link
Member Author

@marseel marseel commented Mar 8, 2021

/assign @wojtek-t

@wojtek-t
Copy link
Member

@wojtek-t wojtek-t commented Mar 9, 2021

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added lgtm approved labels Mar 9, 2021
@wojtek-t
Copy link
Member

@wojtek-t wojtek-t commented Mar 9, 2021

/hold

@marseel - please add a release note
I would say that we should probably cherrypick this as the metrics were broken previously (@liggitt - for thoughts)

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold release-note and removed release-note-none labels Mar 9, 2021
@marseel
Copy link
Member Author

@marseel marseel commented Mar 9, 2021

I've added release note.
I think it makes sense to cherrypick it. Bug was introduced around ~1 year ago in #87631

@fedebongio
Copy link
Contributor

@fedebongio fedebongio commented Mar 9, 2021

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted and removed needs-triage labels Mar 9, 2021
@@ -47,7 +47,7 @@ func WithAuthentication(handler http.Handler, auth authenticator.Request, failed
req = req.WithContext(authenticator.WithAudiences(req.Context(), apiAuds))
}
resp, ok, err := auth.AuthenticateRequest(req)
defer recordAuthMetrics(req.Context(), resp, ok, err, apiAuds, authenticationStart)
recordAuthMetrics(req.Context(), resp, ok, err, apiAuds, authenticationStart)
Copy link
Member

@liggitt liggitt Mar 10, 2021

Choose a reason for hiding this comment

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

/hold

This breaks recording of the authentication audience error case on line 60. The fact that unit tests are green mean we have a test gap that should be closed as well.

Once this is correct, I would support merge and backport

Copy link
Member Author

@marseel marseel Mar 15, 2021

Choose a reason for hiding this comment

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

I've added tests. Also, this error on line 60 was not working before. When you do defer f(err) err is evaluated at the time of defer, not later.

@marseel marseel force-pushed the fix/fix_incorrect_authenticator_metrics branch from d0feb44 to f8d5970 Compare Mar 15, 2021
@k8s-ci-robot k8s-ci-robot added size/L and removed lgtm size/XS labels Mar 15, 2021
@marseel marseel force-pushed the fix/fix_incorrect_authenticator_metrics branch from f8d5970 to 9b87464 Compare Mar 15, 2021
@marseel marseel force-pushed the fix/fix_incorrect_authenticator_metrics branch from 9b87464 to 7dffc11 Compare Mar 15, 2021
@liggitt
Copy link
Member

@liggitt liggitt commented Mar 15, 2021

/lgtm
/approve
/hold cancel
/priority important-soon
/milestone v1.21
/kind regression

fixes regression in 1.18+ that breaks the ability to determine how long the authentication phase of the request is taking

@k8s-ci-robot k8s-ci-robot added priority/important-soon kind/regression and removed do-not-merge/hold labels Mar 15, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Mar 15, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-priority label Mar 15, 2021
@liggitt
Copy link
Member

@liggitt liggitt commented Mar 15, 2021

/retest

@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Mar 15, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, marseel, wojtek-t

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 15, 2021
@fejta-bot
Copy link

@fejta-bot fejta-bot commented Mar 15, 2021

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 067ab92 into kubernetes:master Mar 16, 2021
13 of 14 checks passed
@ialidzhikov
Copy link
Contributor

@ialidzhikov ialidzhikov commented Mar 16, 2021

@marseel , can you file the corresponding cherry-pick PRs?
Ref https://github.com/kubernetes/community/blob/master/contributors/devel/sig-release/cherry-picks.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/apiserver cncf-cla: yes kind/bug kind/regression lgtm priority/important-soon release-note sig/api-machinery sig/instrumentation sig/scalability size/L triage/accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants