🌱 Use caching authorizers per-workspace in initializingworkspaces/builder #2477

vincepri · 2022-12-12T17:27:20Z

This changeset is scoped to the virtual/initializingworkspace package.

Signed-off-by: Vince Prignano vince@prigna.com

Summary

Related issue(s)

Fixes #

pkg/virtual/initializingworkspaces/builder/build.go

pkg/virtual/initializingworkspaces/builder/caching_authorizer.go

pkg/virtual/initializingworkspaces/builder/build.go

pkg/virtual/initializingworkspaces/builder/caching_authorizer.go

hack/logcheck.out

pkg/virtual/initializingworkspaces/builder/caching_authorizer.go

ncdc

LGMT overall. Would like @s-urbaniak to review the authz/audit pieces

ncdc · 2022-12-12T19:00:42Z

pkg/virtual/initializingworkspaces/builder/caching_authorizer.go

+
+	authz, err := c.loadOrStore(workspace)
+	if err != nil {
+		return authorizer.DecisionNoOpinion, "error", err


@s-urbaniak would appreciate guidance on what to use here for the reason

As commented above, reason can be left empty string, however the error should be anonmyized.

ncdc · 2022-12-12T19:00:48Z

pkg/virtual/initializingworkspaces/builder/caching_authorizer.go

+	workspace, name, err := initialization.TypeFrom(tenancyv1alpha1.ClusterWorkspaceInitializer(dynamiccontext.APIDomainKeyFrom(ctx)))
+	if err != nil {
+		logger.V(2).Info(err.Error())
+		return authorizer.DecisionNoOpinion, "unable to determine initializer", fmt.Errorf("access not permitted")


@s-urbaniak PTAL

This is taken 1:1 from the current implementation, so if anything I suggest to take improvements as a separate PR. However, my comment for this (and thus the previous implementation):

specifying a reason in case of errors is not necessary. If an error occurs, the reason is discarded as per https://github.com/kubernetes/kubernetes/blob/00aae4c10c204d258e62a96f30970e9b91b29149/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authorization.go#L67-L71

We have to be careful of anonymizing here, so using the authorization decorator

kcp/pkg/authorization/decorator.go

Line 48 in ffc21ea

func NewDecorator(key string, target authorizer.Authorizer) *Decorator {

to anonymize is the way to go.

However, it's ok to address this as a follow-up as this PR introduces caching of delegated authorizers only.

vincepri · 2022-12-12T20:56:23Z

/test e2e-shared

pkg/virtual/initializingworkspaces/builder/caching_authorizer.go

s-urbaniak · 2023-01-02T10:12:16Z

As this is caching delegated authorizers and not authorize decisions per se I would be rather explicit about this in the godocs by saying that this caches authorizers per requested logical cluster and each cached authorizer additionally caches "allow" results (TTL being 5 minutes) and "deny" results (TTL being 30 seconds) per requested attributes. So it is rather a "cache of caches".

Additionally I suggest to add an additional constructor in the delegated authorizer and have the TTLs configurable so we know the exact timings here rather than on relying on central defaults (

kcp/pkg/authorization/delegated/authorizer.go

Lines 38 to 39 in 8d95d65

    
           AllowCacheTTL:             5 * time.Minute, 
        
           DenyCacheTTL:              30 * time.Second,

) so we have local overall predictable cache behavior here in the constructor.

pkg/authorization/delegated/caching_authorizer.go

s-urbaniak · 2023-01-10T09:13:21Z

/lgtm

ncdc · 2023-01-11T15:35:03Z

pkg/authorization/delegated/caching_authorizer.go

+// load loads the authorizer from the cache, if any.
+func (c *cachingAuthorizer) load(clusterName logicalcluster.Name) authorizer.Authorizer {
+	value, ok := c.cache.Get(clusterName)
+	if !ok && value == nil {


pkg/authorization/delegated/caching_authorizer.go

ncdc · 2023-01-11T15:43:17Z

pkg/authorization/delegated/caching_authorizer.go

+
+// NewCaching creates a new Authorizer that holds an internal cache of
+// Delegated Authorizer(s).
+func NewCaching(client kcpkubernetesclientset.ClusterInterface, auth CachingAuthorizerFunc, opts CachingOptions) *cachingAuthorizer {


Would call this NewCachingAuthorizer

This changeset is scoped to the virtual/initializingworkspace package. Signed-off-by: Vince Prignano <vince@prigna.com>

openshift-ci · 2023-01-11T15:50:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ncdc

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

vincepri · 2023-01-11T16:45:46Z

/test e2e-multiple-runs

vincepri · 2023-01-11T17:55:32Z

/test e2e-multiple-runs

vincepri · 2023-01-11T20:07:36Z

/retest

vincepri · 2023-01-11T21:51:23Z

/retest

vincepri commented Dec 12, 2022

View reviewed changes

pkg/virtual/initializingworkspaces/builder/build.go Outdated Show resolved Hide resolved

pkg/virtual/initializingworkspaces/builder/caching_authorizer.go Outdated Show resolved Hide resolved

openshift-ci bot requested review from ncdc and shawn-hurley December 12, 2022 17:29

vincepri commented Dec 12, 2022

View reviewed changes

pkg/virtual/initializingworkspaces/builder/caching_authorizer.go Outdated Show resolved Hide resolved

ncdc reviewed Dec 12, 2022

View reviewed changes

vincepri changed the title ~~🌱 Use caching authorizers per-workspace~~ 🌱 Use caching authorizers per-workspace in initializingworkspaces/builder Dec 12, 2022

vincepri commented Dec 12, 2022

View reviewed changes

hack/logcheck.out Outdated Show resolved Hide resolved

ncdc reviewed Dec 12, 2022

View reviewed changes

pkg/virtual/initializingworkspaces/builder/caching_authorizer.go Outdated Show resolved Hide resolved

ncdc reviewed Dec 12, 2022

View reviewed changes

pkg/virtual/initializingworkspaces/builder/caching_authorizer.go Outdated Show resolved Hide resolved

ncdc reviewed Dec 12, 2022

View reviewed changes

openshift-merge-robot 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 Dec 23, 2022

sttts reviewed Dec 23, 2022

View reviewed changes

pkg/virtual/initializingworkspaces/builder/caching_authorizer.go Outdated Show resolved Hide resolved

s-urbaniak reviewed Jan 2, 2023

View reviewed changes

pkg/virtual/initializingworkspaces/builder/caching_authorizer.go Outdated Show resolved Hide resolved

openshift-ci bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jan 3, 2023

vincepri commented Jan 3, 2023

View reviewed changes

pkg/authorization/delegated/caching_authorizer.go Outdated Show resolved Hide resolved

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 10, 2023

openshift-merge-robot 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 Jan 10, 2023

ncdc reviewed Jan 11, 2023

View reviewed changes

🌱 Use caching authorizers per-workspace

c1ef69c

This changeset is scoped to the virtual/initializingworkspace package. Signed-off-by: Vince Prignano <vince@prigna.com>

ncdc approved these changes Jan 11, 2023

View reviewed changes

openshift-ci bot assigned ncdc Jan 11, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 11, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 11, 2023

openshift-merge-robot merged commit 5d77df7 into kcp-dev:main Jan 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Use caching authorizers per-workspace in initializingworkspaces/builder #2477

🌱 Use caching authorizers per-workspace in initializingworkspaces/builder #2477

vincepri commented Dec 12, 2022

ncdc left a comment

ncdc Dec 12, 2022

s-urbaniak Jan 2, 2023

ncdc Dec 12, 2022

s-urbaniak Jan 2, 2023

vincepri commented Dec 12, 2022

s-urbaniak commented Jan 2, 2023

s-urbaniak commented Jan 10, 2023

ncdc Jan 11, 2023

ncdc Jan 11, 2023

openshift-ci bot commented Jan 11, 2023

vincepri commented Jan 11, 2023

vincepri commented Jan 11, 2023

vincepri commented Jan 11, 2023

vincepri commented Jan 11, 2023

🌱 Use caching authorizers per-workspace in initializingworkspaces/builder #2477

🌱 Use caching authorizers per-workspace in initializingworkspaces/builder #2477

Conversation

vincepri commented Dec 12, 2022

Summary

Related issue(s)

ncdc left a comment

Choose a reason for hiding this comment

ncdc Dec 12, 2022

Choose a reason for hiding this comment

s-urbaniak Jan 2, 2023

Choose a reason for hiding this comment

ncdc Dec 12, 2022

Choose a reason for hiding this comment

s-urbaniak Jan 2, 2023

Choose a reason for hiding this comment

vincepri commented Dec 12, 2022

s-urbaniak commented Jan 2, 2023

s-urbaniak commented Jan 10, 2023

ncdc Jan 11, 2023

Choose a reason for hiding this comment

ncdc Jan 11, 2023

Choose a reason for hiding this comment

openshift-ci bot commented Jan 11, 2023

vincepri commented Jan 11, 2023

vincepri commented Jan 11, 2023

vincepri commented Jan 11, 2023

vincepri commented Jan 11, 2023