Address liggitt's comments

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() {

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 {

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:

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 {