Race condition in admission control / Param resolution during ValidatingAdmissionPolicy evaluation

Proof-of-concept (Poc) to reproduce the race and demonstrate the fix: Prerequisites: - A Kubernetes cluster with ValidatingAdmissionPolicy support (1.36 line or newer) enabled. - kubectl configured to talk to the cluster. Steps to reproduce (race condition in vulnerable version): 1) Create a ValidatingAdmissionPolicy that references a ConfigMap parameter named param-a in namespace default. 2) Create a ValidatingAdmissionPolicyBinding that binds the policy to ParamRef: - name: param-a - namespace: default - ParameterNotFoundAction: Deny 3) Create the ConfigMap (param-a) in namespace default rapidly after the binding is created, such that the informer cache has not yet observed the new Param object. 4) Attempt to create a resource that would be affected by the policy (e.g., a ConfigMap/user resource that the policy would deny if the param is not observed). 5) Observe that in the vulnerable version, the admission may be denied or mis-evaluated due to the NotFound path hitting the informer cache, depending on timing. Reproducing with the fix (what changes): - With the patch, CollectParams will fall back to a direct API get via DynamicClient when the cache misses, ensuring the Param is resolved correctly even if the informer cache is not yet updated. Minimal example manifests (illustrative; adapt to your cluster's exact API surface): 1) Param ConfigMap (param-a) in default: --- apiVersion: v1 kind: ConfigMap metadata: name: param-a namespace: default data: allowed: "true" --- 2) ValidatingAdmissionPolicy (policy-a) using the param-kind ConfigMap (simplified): --- apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingAdmissionPolicy metadata: name: policy-a spec: matchConstraints: - name: default failurePolicy: Fail paramKind: apiVersion: v1 kind: ConfigMap evaluation: | # In real scenarios this would be a CEL/rego expression referencing the param value true --- 3) Binding (binding-a) with ParameterNotFoundAction Deny: --- apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingAdmissionPolicyBinding metadata: name: binding-a spec: policyName: policy-a paramRef: name: param-a namespace: default # The field may be present depending on SDK; if supported, set the following: ParameterNotFoundAction: Deny --- 4) Attempt to create a resource (e.g., a configmap) under race condition timing: --- apiVersion: v1 kind: ConfigMap metadata: name: test-resource namespace: default data: key: value --- Expected outcome (with fix): The admission decision should correctly resolve the param via direct API fallback when the cache misses, and the resource creation will be admitted/denied based on the actual param value rather than a NotFound from the informer cache.

diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/policy_dispatcher.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/policy_dispatcher.go index 62214a3092e7d..781955eff0729 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/policy_dispatcher.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/policy_dispatcher.go @@ -22,7 +22,7 @@ import ( "fmt" "time" - "k8s.io/api/admissionregistration/v1" + v1 "k8s.io/api/admissionregistration/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -32,6 +32,7 @@ import ( "k8s.io/apiserver/pkg/admission" "k8s.io/apiserver/pkg/admission/plugin/policy/matching" webhookgeneric "k8s.io/apiserver/pkg/admission/plugin/webhook/generic" + "k8s.io/client-go/dynamic" "k8s.io/client-go/informers" "k8s.io/client-go/tools/cache" ) @@ -174,6 +175,8 @@ func (d *policyDispatcher[P, B, E]) Dispatch(ctx context.Context, a admission.At hook.ParamScope, bindingAccessor.GetParamRef(), a.GetNamespace(), + hook.DynamicClient, + hook.RESTMapper, ) if err != nil { // There was an error collecting params for this binding. @@ -264,12 +267,17 @@ func (d *policyDispatcher[P, B, E]) Dispatch(ctx context.Context, a admission.At // Returns params to use to evaluate a policy-binding with given param // configuration. If the policy-binding has no param configuration, it // returns a single-element list with a nil param. +// +// The dynamicClient and restMapper parameters enable direct API fallback when +// the informer cache hasn't received the watch event for a newly created param yet. func CollectParams( paramKind *v1.ParamKind, paramInformer informers.GenericInformer, paramScope meta.RESTScope, paramRef *v1.ParamRef, namespace string, + dynamicClient dynamic.Interface, + restMapper meta.RESTMapper, ) ([]runtime.Object, error) { // If definition has paramKind, paramRef is required in binding. // If definition has no paramKind, paramRef set in binding will be ignored. @@ -331,7 +339,20 @@ func CollectParams( return nil, fmt.Errorf("paramRef.name and paramRef.selector are mutually exclusive") } - switch param, err := paramStore.Get(paramRef.Name); { + // First attempt: try to get the param from the informer cache (fast path) + param, err := paramStore.Get(paramRef.Name) + + // If cache returns NotFound and we have a client, try direct API call as fallback. + // This handles the race condition where resources (ConfigMap, Policy, Binding, and Param) + // are created in quick succession. The informer cache may have completed its initial + // sync (checked by WaitForCacheSync above), but newly created params might not have + // propagated to the cache yet. The direct API call ensures we get the correct answer + // without timing-dependent retry logic. + if apierrors.IsNotFound(err) && dynamicClient != nil && restMapper != nil && paramKind != nil { + param, err = getParamDirectly(dynamicClient, restMapper, paramKind, paramRef, namespace, paramScope) + } + + switch { case err == nil: params = []runtime.Object{param} case apierrors.IsNotFound(err): @@ -381,6 +402,59 @@ func CollectParams( return params, nil } +// getParamDirectly performs a direct API call to retrieve a param when the informer +// cache doesn't have it yet. This handles the race condition where a param resource +// is created but the watch event hasn't propagated to the informer cache. +func getParamDirectly( + dynamicClient dynamic.Interface, + restMapper meta.RESTMapper, + paramKind *v1.ParamKind, + paramRef *v1.ParamRef, + namespace string, + paramScope meta.RESTScope, +) (runtime.Object, error) { + // Convert GVK to GVR using the RESTMapper + gv, err := schema.ParseGroupVersion(paramKind.APIVersion) + if err != nil { + return nil, fmt.Errorf("invalid paramKind APIVersion %q: %w", paramKind.APIVersion, err) + } + gvk := gv.WithKind(paramKind.Kind) + + mapping, err := restMapper.RESTMapping(gvk.GroupKind(), gvk.Version) + if err != nil { + // An unknown paramKind (CRD not registered or deleted) is reported as + // NotFound so the caller routes it through ParameterNotFoundAction + // instead of failing the admission request as an internal error. + if meta.IsNoMatchError(err) { + return nil, apierrors.NewNotFound(schema.GroupResource{Group: gvk.Group, Resource: gvk.Kind}, paramRef.Name) + } + return nil, fmt.Errorf("failed to find REST mapping for %v: %w", gvk, err) + } + + // Determine the namespace for the Get request + var targetNamespace string + if paramScope.Name() == meta.RESTScopeNameNamespace { + if len(paramRef.Namespace) > 0 { + targetNamespace = paramRef.Namespace + } else { + targetNamespace = namespace + } + } + + // Perform the direct API call with a timeout + ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) + defer cancel() + + var resourceClient dynamic.ResourceInterface + if targetNamespace != "" { + resourceClient = dynamicClient.Resource(mapping.Resource).Namespace(targetNamespace) + } else { + resourceClient = dynamicClient.Resource(mapping.Resource) + } + + return resourceClient.Get(ctx, paramRef.Name, metav1.GetOptions{}) +} + var _ webhookgeneric.VersionedAttributeAccessor = &versionedAttributeAccessor{} type versionedAttributeAccessor struct { diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/policy_source.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/policy_source.go index 09e4895e93385..0b7d8a168ce80 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/policy_source.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/policy_source.go @@ -99,6 +99,11 @@ type PolicyHook[P runtime.Object, B runtime.Object, E Evaluator] struct { ParamInformer informers.GenericInformer ParamScope meta.RESTScope + // DynamicClient is used for direct API calls to handle cache misses + DynamicClient dynamic.Interface + // RESTMapper is used to resolve GVK to GVR for direct API calls + RESTMapper meta.RESTMapper + Evaluator E ConfigurationError error } @@ -352,6 +357,8 @@ func (s *policySource[P, B, E]) calculatePolicyData() ([]PolicyHook[P, B, E], er Evaluator: s.compilePolicyLocked(policySpec), ParamInformer: paramInformer, ParamScope: paramScope, + DynamicClient: s.dynamicClient, + RESTMapper: s.restMapper, ConfigurationError: configurationError, }) diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/policy_source_test.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/policy_source_test.go index ed3a395b6be07..e54396e2161f8 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/policy_source_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/generic/policy_source_test.go @@ -23,8 +23,11 @@ import ( "github.com/stretchr/testify/require" v1 "k8s.io/api/admissionregistration/v1" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/apiserver/pkg/admission" "k8s.io/apiserver/pkg/admission/plugin/policy/generic" @@ -203,6 +206,7 @@ type FakeBinding struct { metav1.ObjectMeta PolicyName string + ParamRef *v1.ParamRef } var _ generic.BindingAccessor = &FakeBinding{} @@ -246,7 +250,7 @@ func (fb *FakeBinding) GetMatchResources() *v1.MatchResources { } func (fb *FakeBinding) GetParamRef() *v1.ParamRef { - return nil + return fb.ParamRef } func (fp *FakePolicy) DeepCopyObject() runtime.Object { @@ -262,3 +266,219 @@ func (fb *FakeBinding) DeepCopyObject() runtime.Object { *newFB = *fb return newFB } + +// TestCollectParamsHandlesCacheMiss tests the race condition where a param +// exists in the API server but hasn't propagated to the informer cache yet. +// This can happen when Policy, Binding, and Param (e.g., ConfigMap) resources +// are created in rapid succession. The test verifies that CollectParams falls +// back to a direct API call when the cache returns NotFound. +func TestCollectParamsHandlesCacheMiss(t *testing.T) { + testContext, testCancel, err := generic.NewPolicyTestContext( + t, + func(fp *FakePolicy) generic.PolicyAccessor { return fp }, + func(fb *FakeBinding) generic.BindingAccessor { return fb }, + func(fp *FakePolicy) generic.Evaluator { return nil }, + makeTestDispatcher, + nil, + nil, + ) + require.NoError(t, err) + defer testCancel() + require.NoError(t, testContext.Start()) + + // Create a ConfigMap directly in the dynamic client (simulating etcd) + // without going through the informer, to simulate the race condition + configMap := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{ + "name": "test-param", + "namespace": "default", + }, + "data": map[string]interface{}{ + "maxReplicas": "3", + }, + }, + } + + // Add ConfigMap directly to dynamic client's tracker (bypassing informer) + // This simulates the state where the object exists in etcd but the + // watch event hasn't reached the informer cache yet + ctx := context.Background() + _, err = testContext.DynamicClient.Resource(schema.GroupVersionResource{ + Group: "", + Version: "v1", + Resource: "configmaps", + }).Namespace("default").Create(ctx, configMap, metav1.CreateOptions{}) + require.NoError(t, err) + + // Now create policy and binding that reference this ConfigMap + policy := &FakePolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-policy", + }, + ParamKind: &v1.ParamKind{ + APIVersion: "v1", + Kind: "ConfigMap", + }, + } + + binding := &FakeBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-binding", + }, + PolicyName: "test-policy", + ParamRef: &v1.ParamRef{ + Name: "test-param", + Namespace: "default", + }, + } + + // Create policy and binding through normal path (they'll be in informers) + require.NoError(t, testContext.UpdateAndWait(policy, binding)) + + // Get the param informer for ConfigMap + hooks := testContext.Source.Hooks() + require.Len(t, hooks, 1, "should have one policy hook") + + hook := hooks[0] + require.NotNil(t, hook.ParamInformer, "param informer should be set") + + // Call CollectParams - this should succeed via client fallback + // even though the ConfigMap isn't in the informer cache + params, err := generic.CollectParams( + policy.GetParamKind(), + hook.ParamInformer, + hook.ParamScope, + binding.GetParamRef(), + "default", + hook.DynamicClient, + hook.RESTMapper, + ) + + // With the client fallback fix: this should succeed + require.NoError(t, err, "CollectParams should succeed by falling back to direct client Get") + require.Len(t, params, 1, "should have retrieved the param") + require.NotNil(t, params[0], "param should not be nil") + + // Without the client fallback fix: the above would fail with NotFound error + // because the informer cache doesn't have the ConfigMap yet +} + +// TestCollectParamsHandlesMissingCRD verifies that when a paramKind references +// a CR for a CRD that the RESTMapper does not know about, CollectParams treats +// the param as missing and routes the binding through ParameterNotFoundAction +// rather than returning an internal error. +func TestCollectParamsHandlesMissingCRD(t *testing.T) { + testContext, testCancel, err := generic.NewPolicyTestContext( + t, + func(fp *FakePolicy) generic.PolicyAccessor { return fp }, + func(fb *FakeBinding) generic.BindingAccessor { return fb }, + func(fp *FakePolicy) generic.Evaluator { return nil }, + makeTestDispatcher, + nil, + nil, + ) + require.NoError(t, err) + defer testCancel() + require.NoError(t, testContext.Start()) + + // Bootstrap a hook with a known paramKind (ConfigMap) so we get a + // non-nil ParamInformer/RESTMapper/DynamicClient. The CollectParams + // calls below substitute a different paramKind that the RESTMapper + // cannot resolve, which is the scenario under test. + policy := &FakePolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "test-policy"}, + ParamKind: &v1.ParamKind{ + APIVersion: "v1", + Kind: "ConfigMap", + }, + } + binding := &FakeBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "test-binding"}, + PolicyName: "test-policy", + ParamRef: &v1.ParamRef{ + Name: "ignored", + Namespace: "default", + }, + } + require.NoError(t, testContext.UpdateAndWait(policy, binding)) + + hooks := testContext.Source.Hooks() + require.Len(t, hooks, 1, "should have one policy hook") + hook := hooks[0] + require.NotNil(t, hook.ParamInformer, "param informer should be set") + require.NotNil(t, hook.RESTMapper, "restMapper should be set") + require.NotNil(t, hook.DynamicClient, "dynamicClient should be set") + + missingParamKind := &v1.ParamKind{ + APIVersion: "missing.example.com/v1", + Kind: "MissingCRD", + } + + t.Run("no ParameterNotFoundAction returns no params and no error", func(t *testing.T) { + paramRef := &v1.ParamRef{ + Name: "some-param", + Namespace: "default", + } + + params, err := generic.CollectParams( + missingParamKind, + hook.ParamInformer, + meta.RESTScopeNamespace, + paramRef, + "default", + hook.DynamicClient, + hook.RESTMapper, + ) + + require.NoError(t, err, "an unknown paramKind must not surface as an internal error") + require.Empty(t, params, "no params should be returned when the paramKind cannot be resolved") + }) + + t.Run("ParameterNotFoundAction=Deny returns deny error", func(t *testing.T) { + denyAction := v1.DenyAction + paramRef := &v1.ParamRef{ + Name: "some-param", + Namespace: "default", + ParameterNotFoundAction: &denyAction, + } + + params, err := generic.CollectParams( + missingParamKind, + hook.ParamInformer, + meta.RESTScopeNamespace, + paramRef, + "default", + hook.DynamicClient, + hook.RESTMapper, + ) + + require.Error(t, err, "Deny action should produce an error when the param is missing") + require.Contains(t, err.Error(), "no params found") + require.Empty(t, params) + }) + + t.Run("ParameterNotFoundAction=Allow returns no params and no error", func(t *testing.T) { + allowAction := v1.AllowAction + paramRef := &v1.ParamRef{ + Name: "some-param", + Namespace: "default", + ParameterNotFoundAction: &allowAction, + } + + params, err := generic.CollectParams( + missingParamKind, + hook.ParamInformer, + meta.RESTScopeNamespace, + paramRef, + "default", + hook.DynamicClient, + hook.RESTMapper, + ) + + require.NoError(t, err, "Allo ... [truncated]