From 27e0fa95ff973b056e7eaf01fff18f95c918f74e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20K=C3=A4ldstr=C3=B6m?= Date: Tue, 26 May 2026 12:44:02 +0300 Subject: [PATCH] Address liggitt's comments --- .../authorization/authorizer/conditions.go | 56 ++----------- .../authorizer/conditions_test.go | 79 ++++++++----------- .../pkg/authorization/metrics/metrics.go | 4 +- .../pkg/authorization/metrics/metrics_test.go | 11 ++- 4 files changed, 51 insertions(+), 99 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/authorization/authorizer/conditions.go b/staging/src/k8s.io/apiserver/pkg/authorization/authorizer/conditions.go index 58b84b7b9da..9b287de69d2 100644 --- a/staging/src/k8s.io/apiserver/pkg/authorization/authorizer/conditions.go +++ b/staging/src/k8s.io/apiserver/pkg/authorization/authorizer/conditions.go @@ -95,10 +95,8 @@ func ConditionsAwareDecisionFromParts(unconditional Decision, reason string, err } } -// INVARIANT: Exactly one of Is* must return true at all times. - -// IsAllowed returns true if the decision is an unconditional Allow. -func (d ConditionsAwareDecision) IsAllowed() bool { +// IsAllow returns true if the decision is an unconditional Allow. +func (d ConditionsAwareDecision) IsAllow() bool { return d.unconditionalDecision == DecisionAllow } @@ -107,54 +105,14 @@ func (d ConditionsAwareDecision) IsNoOpinion() bool { return d.unconditionalDecision == DecisionNoOpinion } -// IsDenied returns true if the decision is an unconditional Deny. -func (d ConditionsAwareDecision) IsDenied() bool { +// IsDeny returns true if the decision is an unconditional Deny. +func (d ConditionsAwareDecision) IsDeny() bool { return d.unconditionalDecision == DecisionDeny // == 0 == zero value } -// IsUnconditional is true if d is Allowed, Denied or NoOpinion. +// IsUnconditional is true if d is Allow, Deny or NoOpinion. func (d ConditionsAwareDecision) IsUnconditional() bool { - return d.IsAllowed() || d.IsDenied() || d.IsNoOpinion() -} - -// UnconditionalParts turns a ConditionsAwareDecision into the -// triple that Authorizer.Authorize expects. If the decision is -// conditional, the returned condition is Deny if there were at least -// some Deny condition, otherwise NoOpinion. -// This function is meant to be called when IsUnconditional() == true. -// -// If the authorizer is conditions-aware, it can choose to only implement -// real business logic in the ConditionsAwareAuthorize method, and implement -// Authorize() as "return self.ConditionsAwareAuthorize(ctx, attrs).UnconditionalParts()" -func (d ConditionsAwareDecision) UnconditionalParts() (Decision, string, error) { - switch { - case d.IsAllowed(): - return DecisionAllow, d.Reason(), d.Error() - case d.IsDenied(): - return DecisionDeny, d.Reason(), d.Error() - case d.IsNoOpinion(): - return DecisionNoOpinion, d.Reason(), d.Error() - default: - // An error is not returned here, as that could yield a HTTP response code of 500 instead of 403. - // For the use-case described above with regards to calling this function in Authorize, not returning - // an error is important, as it is valid to always fail closed, as if this happens, no unconditional - // permissions were given the requestor. - return d.FailClosedDecision(), "failed closed: tried to return conditional decision to conditions-unaware authorizer", nil - } -} - -// FailClosedDecision returns either a Deny or NoOpinion decision to fail closed -// whenever processing a decision fails. If the decision contains one or -// more Deny decisions or conditions, one must fail closed with Deny, as that could or would -// have been the if the condition evaluation did not error. Otherwise, NoOpinion is returned. -func (d ConditionsAwareDecision) FailClosedDecision() Decision { - if d.IsAllowed() || d.IsNoOpinion() { - return DecisionNoOpinion - } - // TODO(luxas): In the follow-up PR, add the logic for ConditionsMap and Union here, which - // makes this function useful. - // => d.IsDenied() == true - return DecisionDeny + return d.IsAllow() || d.IsDeny() || d.IsNoOpinion() } // Reason returns the reason associated with the decision. @@ -182,7 +140,7 @@ func (d ConditionsAwareDecision) String() string { } return fmt.Sprintf("(%s)", strings.Join(params, ", ")) } - if d.IsAllowed() { + if d.IsAllow() { return fmt.Sprintf("Allow%s", paramsStr()) } if d.IsNoOpinion() { diff --git a/staging/src/k8s.io/apiserver/pkg/authorization/authorizer/conditions_test.go b/staging/src/k8s.io/apiserver/pkg/authorization/authorizer/conditions_test.go index e5223f4e0a6..8f3dca17586 100644 --- a/staging/src/k8s.io/apiserver/pkg/authorization/authorizer/conditions_test.go +++ b/staging/src/k8s.io/apiserver/pkg/authorization/authorizer/conditions_test.go @@ -33,17 +33,16 @@ func TestConditionsAwareDecision(t *testing.T) { sampleAttrs := authorizer.AttributesRecord{} tests := []struct { - name string - testDecisions []authorizer.ConditionsAwareDecision - wantIsAllowed bool - wantIsNoOpinion bool - wantIsDenied bool - wantIsUnconditional bool - wantFailClosedIsDeny bool - wantReason string - wantAnyError bool - wantErrorIs error - wantString string + name string + testDecisions []authorizer.ConditionsAwareDecision + wantIsAllowed bool + wantIsNoOpinion bool + wantIsDenied bool + wantIsUnconditional bool + wantReason string + wantAnyError bool + wantErrorIs error + wantString string }{ { name: "zero value", @@ -54,12 +53,11 @@ func TestConditionsAwareDecision(t *testing.T) { return }).ConditionsAwareAuthorize(ctx, sampleAttrs), }, - wantIsDenied: true, - wantIsUnconditional: true, - wantFailClosedIsDeny: true, - wantReason: "", - wantErrorIs: nil, - wantString: `Deny`, + wantIsDenied: true, + wantIsUnconditional: true, + wantReason: "", + wantErrorIs: nil, + wantString: `Deny`, }, { name: "deny constructor", @@ -70,12 +68,11 @@ func TestConditionsAwareDecision(t *testing.T) { return authorizer.DecisionDeny, "foo", unexpectedErr }).ConditionsAwareAuthorize(ctx, sampleAttrs), }, - wantIsDenied: true, - wantIsUnconditional: true, - wantFailClosedIsDeny: true, - wantReason: "foo", - wantErrorIs: unexpectedErr, - wantString: `Deny(reason="foo", err="unexpected things happened")`, + wantIsDenied: true, + wantIsUnconditional: true, + wantReason: "foo", + wantErrorIs: unexpectedErr, + wantString: `Deny(reason="foo", err="unexpected things happened")`, }, { name: "allow constructor", @@ -115,12 +112,11 @@ func TestConditionsAwareDecision(t *testing.T) { return 42, "", nil }).ConditionsAwareAuthorize(ctx, sampleAttrs), }, - wantIsDenied: true, - wantIsUnconditional: true, - wantFailClosedIsDeny: true, - wantReason: "", - wantAnyError: true, - wantString: `Deny(err="unknown unconditional decision type: 42")`, + wantIsDenied: true, + wantIsUnconditional: true, + wantReason: "", + wantAnyError: true, + wantString: `Deny(err="unknown unconditional decision type: 42")`, }, { name: "from parts: unsupported mode with other error", @@ -130,19 +126,18 @@ func TestConditionsAwareDecision(t *testing.T) { return 42, "foo", otherErr }).ConditionsAwareAuthorize(ctx, sampleAttrs), }, - wantIsDenied: true, - wantIsUnconditional: true, - wantFailClosedIsDeny: true, - wantReason: "foo", - wantErrorIs: otherErr, - wantString: `Deny(reason="foo", err="[other error, unknown unconditional decision type: 42]")`, + wantIsDenied: true, + wantIsUnconditional: true, + wantReason: "foo", + wantErrorIs: otherErr, + wantString: `Deny(reason="foo", err="[other error, unknown unconditional decision type: 42]")`, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { for i, d := range tt.testDecisions { t.Run(fmt.Sprint(i), func(t *testing.T) { - isAllowed := d.IsAllowed() + isAllowed := d.IsAllow() if isAllowed != tt.wantIsAllowed { t.Errorf("IsAllowed() = %v, want %v", isAllowed, tt.wantIsAllowed) } @@ -150,7 +145,7 @@ func TestConditionsAwareDecision(t *testing.T) { if isNoOpinion != tt.wantIsNoOpinion { t.Errorf("IsNoOpinion() = %v, want %v", isNoOpinion, tt.wantIsNoOpinion) } - isDenied := d.IsDenied() + isDenied := d.IsDeny() if isDenied != tt.wantIsDenied { t.Errorf("IsDenied() = %v, want %v", isDenied, tt.wantIsDenied) } @@ -172,16 +167,6 @@ func TestConditionsAwareDecision(t *testing.T) { t.Errorf("Error() = %v, want %v", gotError, tt.wantErrorIs) } } - failClosed := d.FailClosedDecision() - if tt.wantFailClosedIsDeny { - if failClosed != authorizer.DecisionDeny { - t.Errorf("want FailClosedDecision() == Deny; got %s", failClosed) - } - } else { - if failClosed != authorizer.DecisionNoOpinion { - t.Errorf("want FailClosedDecision() == NoOpinion; got %s", failClosed) - } - } gotString := d.String() if gotString != tt.wantString { diff --git a/staging/src/k8s.io/apiserver/pkg/authorization/metrics/metrics.go b/staging/src/k8s.io/apiserver/pkg/authorization/metrics/metrics.go index 904a94d497b..ca6ca86a353 100644 --- a/staging/src/k8s.io/apiserver/pkg/authorization/metrics/metrics.go +++ b/staging/src/k8s.io/apiserver/pkg/authorization/metrics/metrics.go @@ -99,10 +99,10 @@ func (a *instrumentedAuthorizer) ConditionsAwareAuthorize(ctx context.Context, a switch { case decision.IsNoOpinion(): // non-terminal, not reported - case decision.IsAllowed(): + case decision.IsAllow(): // matches SubjectAccessReview status.allowed field name RecordAuthorizationDecision(a.authorizerType, a.authorizerName, "allowed") - case decision.IsDenied(): + case decision.IsDeny(): // matches SubjectAccessReview status.denied field name RecordAuthorizationDecision(a.authorizerType, a.authorizerName, "denied") default: diff --git a/staging/src/k8s.io/apiserver/pkg/authorization/metrics/metrics_test.go b/staging/src/k8s.io/apiserver/pkg/authorization/metrics/metrics_test.go index 8afe8310285..d69466a1466 100644 --- a/staging/src/k8s.io/apiserver/pkg/authorization/metrics/metrics_test.go +++ b/staging/src/k8s.io/apiserver/pkg/authorization/metrics/metrics_test.go @@ -163,7 +163,16 @@ type dummyConditionalAuthorizer struct { } func (d *dummyConditionalAuthorizer) Authorize(ctx context.Context, attrs authorizer.Attributes) (authorizer.Decision, string, error) { - return d.ConditionsAwareAuthorize(ctx, attrs).UnconditionalParts() + switch { + case d.authorizeDecision.IsAllow(): + return authorizer.DecisionAllow, d.authorizeDecision.Reason(), d.authorizeDecision.Error() + case d.authorizeDecision.IsNoOpinion(): + return authorizer.DecisionNoOpinion, d.authorizeDecision.Reason(), d.authorizeDecision.Error() + case d.authorizeDecision.IsDeny(): + return authorizer.DecisionDeny, d.authorizeDecision.Reason(), d.authorizeDecision.Error() + default: // Conditional case + return authorizer.DecisionDeny, "failed closed: wanted to return a conditional decision, but called on the conditions-unaware method", nil + } } func (d *dummyConditionalAuthorizer) ConditionsAwareAuthorize(ctx context.Context, attrs authorizer.Attributes) authorizer.ConditionsAwareDecision {