Skip to content

Commit

Permalink
Adjust CEL cost calculation and versioning for authorization library
Browse files Browse the repository at this point in the history
  • Loading branch information
@liggitt
liggitt committed Jul 19, 2024
1 parent be2e32f commit 83bd512
Show file tree
Hide file tree
Showing 12 changed files with 661 additions and 28 deletions.
78 changes: 63 additions & 15 deletions staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/filter_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,6 @@ import (
"context"
"errors"
"fmt"
"k8s.io/apimachinery/pkg/fields"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/selection"
genericfeatures "k8s.io/apiserver/pkg/features"
utilfeature "k8s.io/apiserver/pkg/util/feature"
featuregatetesting "k8s.io/component-base/featuregate/testing"
"reflect"
"strings"
"testing"
Expand All @@ -34,19 +28,25 @@ import (
celtypes "github.com/google/cel-go/common/types"
"github.com/stretchr/testify/require"

"k8s.io/utils/pointer"

corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/fields"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/selection"
"k8s.io/apimachinery/pkg/util/version"
"k8s.io/apiserver/pkg/admission"
celconfig "k8s.io/apiserver/pkg/apis/cel"
"k8s.io/apiserver/pkg/authentication/user"
"k8s.io/apiserver/pkg/authorization/authorizer"
apiservercel "k8s.io/apiserver/pkg/cel"
"k8s.io/apiserver/pkg/cel/environment"
genericfeatures "k8s.io/apiserver/pkg/features"
utilfeature "k8s.io/apiserver/pkg/util/feature"
featuregatetesting "k8s.io/component-base/featuregate/testing"
"k8s.io/utils/pointer"
)

type condition struct {
Expand Down Expand Up @@ -182,6 +182,9 @@ func TestFilter(t *testing.T) {
},
}

v130 := version.MajorMinor(1, 30)
v131 := version.MajorMinor(1, 31)

var nilUnstructured *unstructured.Unstructured
cases := []struct {
name string
Expand All @@ -195,6 +198,8 @@ func TestFilter(t *testing.T) {
namespaceObject *corev1.Namespace
strictCost bool
enableSelectors bool

compatibilityVersion *version.Version
}{
{
name: "valid syntax for object",
Expand Down Expand Up @@ -494,6 +499,38 @@ func TestFilter(t *testing.T) {
APIVersion: "*",
}),
},
{
name: "test authorizer error using fieldSelector with 1.30 compatibility",
validations: []ExpressionAccessor{
&condition{
Expression: "authorizer.group('apps').resource('deployments').fieldSelector('foo=bar').labelSelector('apple=banana').subresource('status').namespace('test').name('backend').check('create').allowed()",
},
},
attributes: newValidAttribute(&podObject, false),
results: []EvaluationResult{
{
Error: fmt.Errorf("fieldSelector"),
},
},
authorizer: newAuthzAllowMatch(authorizer.AttributesRecord{
ResourceRequest: true,
APIGroup: "apps",
Resource: "deployments",
Subresource: "status",
Namespace: "test",
Name: "backend",
Verb: "create",
APIVersion: "*",
FieldSelectorRequirements: fields.Requirements{
{Operator: "=", Field: "foo", Value: "bar"},
},
LabelSelectorRequirements: labels.Requirements{
*simpleLabelSelector,
},
}),
enableSelectors: true,
compatibilityVersion: v130,
},
{
name: "test authorizer allow resource check with all fields",
validations: []ExpressionAccessor{
Expand Down Expand Up @@ -523,7 +560,8 @@ func TestFilter(t *testing.T) {
*simpleLabelSelector,
},
}),
enableSelectors: true,
enableSelectors: true,
compatibilityVersion: v131,
},
{
name: "test authorizer allow resource check with parse failures",
Expand All @@ -550,7 +588,8 @@ func TestFilter(t *testing.T) {
FieldSelectorParsingErr: errors.New("invalid selector: 'foo badoperator bar'; can't understand 'foo badoperator bar'"),
LabelSelectorParsingErr: errors.New("unable to parse requirement: found 'badoperator', expected: in, notin, =, ==, !=, gt, lt"),
}),
enableSelectors: true,
enableSelectors: true,
compatibilityVersion: v131,
},
{
name: "test authorizer allow resource check with all fields, without gate",
Expand All @@ -575,6 +614,7 @@ func TestFilter(t *testing.T) {
Verb: "create",
APIVersion: "*",
}),
compatibilityVersion: v131,
},
{
name: "test authorizer not allowed resource check one incorrect field",
Expand Down Expand Up @@ -837,9 +877,13 @@ func TestFilter(t *testing.T) {
if tc.testPerCallLimit == 0 {
tc.testPerCallLimit = celconfig.PerCallLimit
}
env, err := environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), tc.strictCost).Extend(
compatibilityVersion := tc.compatibilityVersion
if compatibilityVersion == nil {
compatibilityVersion = environment.DefaultCompatibilityVersion()
}
env, err := environment.MustBaseEnvSet(compatibilityVersion, tc.strictCost).Extend(
environment.VersionedOptions{
IntroducedVersion: environment.DefaultCompatibilityVersion(),
IntroducedVersion: compatibilityVersion,
ProgramOptions: []celgo.ProgramOption{celgo.CostLimit(tc.testPerCallLimit)},
},
)
Expand Down Expand Up @@ -868,12 +912,16 @@ func TestFilter(t *testing.T) {
}
require.Equal(t, len(evalResults), len(tc.results))
for i, result := range tc.results {
if result.EvalResult != evalResults[i].EvalResult {
t.Errorf("Expected result '%v' but got '%v'", result.EvalResult, evalResults[i].EvalResult)
}
if result.Error != nil && !strings.Contains(evalResults[i].Error.Error(), result.Error.Error()) {
t.Errorf("Expected result '%v' but got '%v'", result.Error, evalResults[i].Error)
}
if result.Error == nil && evalResults[i].Error != nil {
t.Errorf("Expected result '%v' but got error '%v'", result.EvalResult, evalResults[i].Error)
continue
}
if result.EvalResult != evalResults[i].EvalResult {
t.Errorf("Expected result '%v' but got '%v'", result.EvalResult, evalResults[i].EvalResult)
}
}
})
}
Expand Down
35 changes: 35 additions & 0 deletions staging/src/k8s.io/apiserver/pkg/cel/environment/base.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"fmt"
"strconv"
"sync"
"sync/atomic"

"github.com/google/cel-go/cel"
"github.com/google/cel-go/checker"
Expand All @@ -30,6 +31,8 @@ import (
"k8s.io/apimachinery/pkg/util/version"
celconfig "k8s.io/apiserver/pkg/apis/cel"
"k8s.io/apiserver/pkg/cel/library"
genericfeatures "k8s.io/apiserver/pkg/features"
utilfeature "k8s.io/apiserver/pkg/util/feature"
utilversion "k8s.io/apiserver/pkg/util/version"
)

Expand Down Expand Up @@ -146,6 +149,38 @@ var baseOptsWithoutStrictCost = []VersionedOptions{
library.Format(),
},
},
// Authz selectors
{
IntroducedVersion: version.MajorMinor(1, 31),
FeatureEnabled: func() bool {
enabled := utilfeature.DefaultFeatureGate.Enabled(genericfeatures.AuthorizeWithSelectors)
authzSelectorsLibraryInit.Do(func() {
// Record the first time feature enablement was checked for this library.
// This is checked from integration tests to ensure no cached cel envs
// are constructed before feature enablement is effectively set.
authzSelectorsLibraryEnabled.Store(enabled)
// Uncomment to debug where the first initialization is coming from if needed.
// debug.PrintStack()
})
return enabled
},
EnvOptions: []cel.EnvOption{
library.AuthzSelectors(),
},
},
}

var (
authzSelectorsLibraryInit sync.Once
authzSelectorsLibraryEnabled atomic.Value
)

// AuthzSelectorsLibraryEnabled returns whether the AuthzSelectors library was enabled when it was constructed.
// If it has not been contructed yet, this returns `false, false`.
// This is solely for the benefit of the integration tests making sure feature gates get correctly parsed before AuthzSelector ever has to check for enablement.
func AuthzSelectorsLibraryEnabled() (enabled, constructed bool) {
enabled, constructed = authzSelectorsLibraryEnabled.Load().(bool)
return
}

var StrictCostOpt = VersionedOptions{
Expand Down
29 changes: 25 additions & 4 deletions staging/src/k8s.io/apiserver/pkg/cel/environment/environment.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,15 @@ type VersionedOptions struct {
//
// Optional.
RemovedVersion *version.Version

// FeatureEnabled returns true if these options are enabled by feature gates,
// and returns false if these options are not enabled due to feature gates.
//
// This takes priority over IntroducedVersion / RemovedVersion for the NewExpressions environment.
//
// The StoredExpressions environment ignores this function.
//
// Optional.
FeatureEnabled func() bool
// EnvOptions provides CEL EnvOptions. This may be used to add a cel.Variable, a
// cel.Library, or to enable other CEL EnvOptions such as language settings.
//
Expand Down Expand Up @@ -210,15 +218,15 @@ type VersionedOptions struct {
// making multiple calls to Extend.
func (e *EnvSet) Extend(options ...VersionedOptions) (*EnvSet, error) {
if len(options) > 0 {
newExprOpts, err := e.filterAndBuildOpts(e.newExpressions, e.compatibilityVersion, options)
newExprOpts, err := e.filterAndBuildOpts(e.newExpressions, e.compatibilityVersion, true, options)
if err != nil {
return nil, err
}
p, err := e.newExpressions.Extend(newExprOpts)
if err != nil {
return nil, err
}
storedExprOpt, err := e.filterAndBuildOpts(e.storedExpressions, version.MajorMinor(math.MaxUint, math.MaxUint), options)
storedExprOpt, err := e.filterAndBuildOpts(e.storedExpressions, version.MajorMinor(math.MaxUint, math.MaxUint), false, options)
if err != nil {
return nil, err
}
Expand All @@ -231,13 +239,26 @@ func (e *EnvSet) Extend(options ...VersionedOptions) (*EnvSet, error) {
return e, nil
}

func (e *EnvSet) filterAndBuildOpts(base *cel.Env, compatVer *version.Version, opts []VersionedOptions) (cel.EnvOption, error) {
func (e *EnvSet) filterAndBuildOpts(base *cel.Env, compatVer *version.Version, honorFeatureGateEnablement bool, opts []VersionedOptions) (cel.EnvOption, error) {
var envOpts []cel.EnvOption
var progOpts []cel.ProgramOption
var declTypes []*apiservercel.DeclType

for _, opt := range opts {
var allowedByFeatureGate, allowedByVersion bool
if opt.FeatureEnabled != nil && honorFeatureGateEnablement {
// Feature-gate-enabled libraries must follow compatible default feature enablement.
// Enabling alpha features in their first release enables libraries the previous API server is unaware of.
allowedByFeatureGate = opt.FeatureEnabled()
if !allowedByFeatureGate {
continue
}
}
if compatVer.AtLeast(opt.IntroducedVersion) && (opt.RemovedVersion == nil || compatVer.LessThan(opt.RemovedVersion)) {
allowedByVersion = true
}

if allowedByFeatureGate || allowedByVersion {
envOpts = append(envOpts, opt.EnvOptions...)
progOpts = append(progOpts, opt.ProgramOptions...)
declTypes = append(declTypes, opt.DeclTypes...)
Expand Down
Loading

0 comments on commit 83bd512

Please sign in to comment.