Force token cache to support audit annotations #90140
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
the
release-note-none
k8s-ci-robot
added
sig/auth
|
Can you add an annotation to the benchmark? |
mikedanese
reviewed
Apr 15, 2020
staging/src/k8s.io/apiserver/pkg/authentication/token/cache/cached_token_authenticator.go
Outdated
Show resolved
Hide resolved
mikedanese
reviewed
Apr 15, 2020
staging/src/k8s.io/apiserver/pkg/authentication/token/cache/cached_token_authenticator.go
Outdated
Show resolved
Hide resolved
|
Can you add a test as well? |
|
/cc @lavalamp |
|
uh, is it OK for annotations for 1 request to get announced for all of them? what will be in these annotations? Is there a design I can read? |
Overall, yes, because the assumption is that for some period of time (cache TTL), all requests with the same API audiences and the same bearer token result in the same audit annotations. This may not be true if the authenticator sets an annotation based on the current time, but that may be okay since cache TTLs are generally small (seconds).
That depends on the authenticator but one of the initial use cases is to make it safer to use bound SA tokens (see below).
|
Can you state this in a comment? It'll be very helpful for future readers. |
|
If 20% of successful authentications set audit annotations, there does not seem to be any real difference in the benchmark: Old: New: Delta: |
enj
force-pushed
the
enj/i/token_cache_annotations
branch
from
f024f68
to
989dad7
Compare
k8s-ci-robot
added
size/L
|
@lavalamp @mikedanese comments addressed. I am a little surprised by the benchmark results so please confirm I updated the benchmark correctly. |
enj
force-pushed
the
enj/i/token_cache_annotations
branch
2 times, most recently
from
c4fcaa0
to
08dfe39
Compare
|
Timeout on scale test could be related though benchmark results seem to disagree. /retest |
|
/test all |
|
/retest |
|
@lavalamp @mikedanese bump. |
k8s-ci-robot
added
the
needs-rebase
mikedanese
added
the
priority/important-soon
k8s-ci-robot
removed
the
needs-priority
mikedanese
reviewed
Jun 15, 2020
| // add some realistic annotations on ~20% of successful authentications | ||
| if rr.Float64() < 0.2 { | ||
| r.annotations = map[string]string{ | ||
| "audience.authentication.kubernetes.io": "e8357258-88b1-11ea-bc55-0242ac130003", |
There was a problem hiding this comment.
embed the float64 in an annotation with Sprint.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enj, 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Signed-off-by: Monis Khan <mok@vmware.com>
enj
force-pushed
the
enj/i/token_cache_annotations
branch
from
08dfe39
to
6039451
Compare
k8s-ci-robot
removed
lgtm
diff --git a/staging/src/k8s.io/apiserver/pkg/authentication/token/cache/BUILD b/staging/src/k8s.io/apiserver/pkg/authentication/token/cache/BUILD
index 21d4b220677..10c715372ef 100644
--- a/staging/src/k8s.io/apiserver/pkg/authentication/token/cache/BUILD
+++ b/staging/src/k8s.io/apiserver/pkg/authentication/token/cache/BUILD
@@ -47,7 +47,7 @@ go_library(
"//staging/src/k8s.io/component-base/metrics:go_default_library",
"//staging/src/k8s.io/component-base/metrics/legacyregistry:go_default_library",
"//vendor/golang.org/x/sync/singleflight:go_default_library",
- "//vendor/k8s.io/klog:go_default_library",
+ "//vendor/k8s.io/klog/v2:go_default_library",
],
)
diff --git a/staging/src/k8s.io/apiserver/pkg/authentication/token/cache/cached_token_authenticator.go b/staging/src/k8s.io/apiserver/pkg/authentication/token/cache/cached_token_authenticator.go
index 146af6fe500..a10564f04d9 100644
--- a/staging/src/k8s.io/apiserver/pkg/authentication/token/cache/cached_token_authenticator.go
+++ b/staging/src/k8s.io/apiserver/pkg/authentication/token/cache/cached_token_authenticator.go
@@ -38,7 +38,7 @@ import (
"k8s.io/apiserver/pkg/audit"
"k8s.io/apiserver/pkg/authentication/authenticator"
"k8s.io/apiserver/pkg/endpoints/request"
- "k8s.io/klog"
+ "k8s.io/klog/v2"
)
var errAuthnCrash = apierrors.NewInternalError(errors.New("authentication failed unexpectedly"))
diff --git a/staging/src/k8s.io/apiserver/pkg/authentication/token/cache/cached_token_authenticator_test.go b/staging/src/k8s.io/apiserver/pkg/authentication/token/cache/cached_token_authenticator_test.go
index 75f4017963e..ed3abfc1d1f 100644
--- a/staging/src/k8s.io/apiserver/pkg/authentication/token/cache/cached_token_authenticator_test.go
+++ b/staging/src/k8s.io/apiserver/pkg/authentication/token/cache/cached_token_authenticator_test.go
@@ -480,10 +480,11 @@ func (s *singleBenchmark) makeTokens() {
r.err = nil
// add some realistic annotations on ~20% of successful authentications
- if rr.Float64() < 0.2 {
+ if f := rr.Float64(); f < 0.2 {
r.annotations = map[string]string{
"audience.authentication.kubernetes.io": "e8357258-88b1-11ea-bc55-0242ac130003",
"namespace.authentication.kubernetes.io": "kube-system",
+ "float.authentication.kubernetes.io": fmt.Sprint(f),
}
}
case choice < 0.99: |
|
/retest |
1 similar comment
|
/retest |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/retest Review the full test history for this PR. Silence the bot with an |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Monis Khan mok@vmware.com
/kind feature
@kubernetes/sig-auth-pr-reviews
/assign @liggitt @mikedanese
xref: #89305