Factor-out metrics related logic from authentication logic. #87631

immutableT · 2020-01-28T23:56:29Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Improve readability of the filters/authenticator package by extracting metrics' related logic into a separate file.
Cover metrics' related logic by unit tests.

Does this PR introduce a user-facing change?:

NONE

immutableT · 2020-01-28T23:58:28Z

Reducing the diff of #85113

immutableT · 2020-01-29T00:01:33Z

/assign @timstclair
/assign @awly

staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication.go

immutableT · 2020-01-29T00:09:00Z

/assign @logicalhan

tallclair

Thanks for breaking this out into a separate PR.

staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication.go

tallclair · 2020-01-29T00:14:53Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication.go

-	// Anything else (custom service accounts, custom external identities, etc.)
-	default:
-		return "other"
+func audiencesIntersect(apiAuds, responseAudiences authenticator.Audiences) bool {


/cc @mikedanese

To double check security critical change

I agree this preserves the existing logic, but the method name is fairly misleading, since it short-circuits on empty parameters. Consider renaming to audiencesAreAcceptable() or something similar.

As a follow-up, given that (I think) we are now wrapping all audience-agnostic authenticators in ones that know how to return the API server audience in the response, I think we should probably drop the len(responseAudiences) == 0 short-circuit.

Renamed to audiencesAreAcceptable.

Will follow-up with a PR to remove the len(responseAudiences) == 0 short-circuit.

staging/src/k8s.io/apiserver/pkg/endpoints/filters/metrics.go

staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication.go

staging/src/k8s.io/apiserver/pkg/endpoints/filters/metrics.go

staging/src/k8s.io/apiserver/pkg/endpoints/filters/metrics_test.go

tallclair · 2020-01-29T00:53:02Z

/lgtm

immutableT · 2020-01-29T17:16:02Z

/test pull-kubernetes-kubemark-e2e-gce-big
/test pull-kubernetes-e2e-gce
/test pull-kubernetes-e2e-gce-100-performance

immutableT · 2020-01-29T18:48:41Z

/assign @liggitt

immutableT · 2020-01-29T19:29:29Z

/test pull-kubernetes-e2e-gce-100-performance

mikedanese · 2020-02-20T01:25:51Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/metrics.go

+	successLabel                 = "success"
+	failureLabel                 = "failure"
+	errorLabel                   = "error"
+	audiencesDoNotIntersectLabel = "non-intersecting-audiences"


Why is this a result?

Would not we want to know about such cases?
See, a comment by @liggitt comment here

Shouldn't we report it as an error and not as a new result type? This is a pretty old metric, so I don't want to break the semantics even if it's alpha. On top of that, "success"/"failure"/"error" seems like a reasonable breakdown for results and I'm not sure that "non-intersecting-audiences" is distinct enough from "error" to warrant introduction of another state.

Done - now classifying non-intersecting audiences as a failure.

mikedanese · 2020-02-21T02:44:18Z

Verify failure looks real.

immutableT · 2020-02-21T18:18:48Z

/test pull-kubernetes-node-e2e-containerd
/test pull-kubernetes-e2e-gce

mikedanese · 2020-02-21T19:09:06Z

/lgtm

jtslear · 2020-02-28T16:41:20Z

Hello @RainbowMango
Bug Triage team here for the 1.18 release. This is a friendly reminder that code freeze is scheduled for 5 March. This PR has been identified as a blocker for #85113 Can we get a review completed on this?

tallclair · 2020-02-29T00:15:01Z

/milestone v1.18

RainbowMango · 2020-02-29T01:19:00Z

@jtslear Seems @mikedanese want @LiGgit take a look.

staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication.go

liggitt · 2020-02-29T18:29:45Z

a couple comments on method name and logging, one follow-up requested, lgtm otherwise

liggitt · 2020-03-01T00:49:59Z

/lgtm
/approve

k8s-ci-robot · 2020-03-01T00:50:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: immutableT, liggitt, mikedanese

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:

~~staging/src/k8s.io/apiserver/OWNERS~~ [liggitt]

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

mikedanese · 2020-03-01T19:41:11Z

/retest

tallclair · 2020-03-02T18:25:41Z

Looks like this is good to go.
/hold cancel

immutableT mentioned this pull request Jan 29, 2020

Classify authentication related metrics based on the authenticator's type. #85113

Closed

k8s-ci-robot assigned awly and timstclair Jan 29, 2020

immutableT commented Jan 29, 2020

View reviewed changes

staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication.go Show resolved Hide resolved

immutableT requested a review from RainbowMango January 29, 2020 00:08

k8s-ci-robot assigned logicalhan Jan 29, 2020

tallclair reviewed Jan 29, 2020

View reviewed changes

k8s-ci-robot requested a review from mikedanese January 29, 2020 00:33

immutableT force-pushed the extract-auth-metrics branch from 38d1ef6 to ad5c741 Compare January 29, 2020 00:43

k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 29, 2020

immutableT force-pushed the extract-auth-metrics branch from ad5c741 to ca61066 Compare January 29, 2020 00:48

k8s-ci-robot assigned tallclair Jan 29, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 29, 2020

k8s-ci-robot assigned liggitt Jan 29, 2020

mikedanese reviewed Feb 20, 2020

View reviewed changes

mikedanese added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Feb 21, 2020

k8s-ci-robot removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Feb 21, 2020

immutableT force-pushed the extract-auth-metrics branch from 1ef51cf to c9aa603 Compare February 21, 2020 00:33

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2020

immutableT force-pushed the extract-auth-metrics branch from c9aa603 to 9562ee8 Compare February 21, 2020 00:43

immutableT force-pushed the extract-auth-metrics branch from 9562ee8 to a689048 Compare February 21, 2020 16:59

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2020

k8s-ci-robot added this to the v1.18 milestone Feb 29, 2020

liggitt reviewed Feb 29, 2020

View reviewed changes

staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication.go Show resolved Hide resolved

Factor-out metrics related logic from authentication logic.

c0bad80

immutableT force-pushed the extract-auth-metrics branch from a689048 to c0bad80 Compare March 1, 2020 00:39

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 1, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 1, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 1, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 2, 2020

k8s-ci-robot merged commit 7dccfd6 into kubernetes:master Mar 3, 2020

marseel mentioned this pull request Mar 9, 2021

Fix incorrect authentication latency metric #99944

Merged

melnikalex mentioned this pull request Mar 26, 2021

Improve coverage of authorization metrics in apiserver (authorization_attempts) #100588

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Factor-out metrics related logic from authentication logic. #87631

Factor-out metrics related logic from authentication logic. #87631

immutableT commented Jan 28, 2020 •

edited

immutableT commented Jan 28, 2020

immutableT commented Jan 29, 2020

immutableT commented Jan 29, 2020

tallclair left a comment

tallclair Jan 29, 2020

liggitt Feb 29, 2020

immutableT Mar 1, 2020 •

edited

tallclair commented Jan 29, 2020

immutableT commented Jan 29, 2020

immutableT commented Jan 29, 2020

immutableT commented Jan 29, 2020

mikedanese Feb 20, 2020

immutableT Feb 20, 2020

mikedanese Feb 20, 2020 •

edited

immutableT Feb 20, 2020 •

edited

mikedanese commented Feb 21, 2020

immutableT commented Feb 21, 2020

mikedanese commented Feb 21, 2020

jtslear commented Feb 28, 2020

tallclair commented Feb 29, 2020

RainbowMango commented Feb 29, 2020

liggitt commented Feb 29, 2020

liggitt commented Mar 1, 2020

k8s-ci-robot commented Mar 1, 2020

mikedanese commented Mar 1, 2020

tallclair commented Mar 2, 2020

Factor-out metrics related logic from authentication logic. #87631

Factor-out metrics related logic from authentication logic. #87631

Conversation

immutableT commented Jan 28, 2020 • edited

immutableT commented Jan 28, 2020

immutableT commented Jan 29, 2020

immutableT commented Jan 29, 2020

tallclair left a comment

Choose a reason for hiding this comment

tallclair Jan 29, 2020

Choose a reason for hiding this comment

liggitt Feb 29, 2020

Choose a reason for hiding this comment

immutableT Mar 1, 2020 • edited

Choose a reason for hiding this comment

tallclair commented Jan 29, 2020

immutableT commented Jan 29, 2020

immutableT commented Jan 29, 2020

immutableT commented Jan 29, 2020

mikedanese Feb 20, 2020

Choose a reason for hiding this comment

immutableT Feb 20, 2020

Choose a reason for hiding this comment

mikedanese Feb 20, 2020 • edited

Choose a reason for hiding this comment

immutableT Feb 20, 2020 • edited

Choose a reason for hiding this comment

mikedanese commented Feb 21, 2020

immutableT commented Feb 21, 2020

mikedanese commented Feb 21, 2020

jtslear commented Feb 28, 2020

tallclair commented Feb 29, 2020

RainbowMango commented Feb 29, 2020

liggitt commented Feb 29, 2020

liggitt commented Mar 1, 2020

k8s-ci-robot commented Mar 1, 2020

mikedanese commented Mar 1, 2020

tallclair commented Mar 2, 2020

immutableT commented Jan 28, 2020 •

edited

immutableT Mar 1, 2020 •

edited

mikedanese Feb 20, 2020 •

edited

immutableT Feb 20, 2020 •

edited