Merge pull request #115147 from alexzielenski/apiserver/policy/multiple-paramkind-bug

ValidatingAdmissionPolicy: fix bug preventing multiple policies from using same paramKind

staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/admission_test.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"fmt"
 	"sync"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -633,6 +634,12 @@ func TestBasicPolicyDefinitionFailure(t *testing.T) {
 	require.ErrorContains(t, err, `Denied`)
 }
 
+type validatorFunc func(a admission.Attributes, o admission.ObjectInterfaces, params runtime.Object, matchKind schema.GroupVersionKind) ([]policyDecision, error)
+
+func (f validatorFunc) Validate(a admission.Attributes, o admission.ObjectInterfaces, params runtime.Object, matchKind schema.GroupVersionKind) ([]policyDecision, error) {
+	return f(a, o, params, matchKind)
+}
+
 type testValidator struct {
 }
 
@@ -1075,3 +1082,130 @@ func TestEmptyParamSource(t *testing.T) {
 	require.ErrorContains(t, err, `Denied`)
 	require.Equal(t, 1, numCompiles)
 }
+
+// Shows what happens when multiple policies share one param type, then
+// one policy stops using the param. The expectation is the second policy
+// keeps behaving normally
+func TestMultiplePoliciesSharedParamType(t *testing.T) {
+	testContext, testContextCancel := context.WithCancel(context.Background())
+	defer testContextCancel()
+
+	compiler := &fakeCompiler{
+		// Match everything by default
+		DefaultMatch: true,
+	}
+	handler, paramTracker, tracker, controller := setupFakeTest(t, compiler)
+
+	// Use ConfigMap native-typed param
+	policy1 := *denyPolicy
+	policy1.Name = "denypolicy1.example.com"
+
+	policy2 := *denyPolicy
+	policy2.Name = "denypolicy2.example.com"
+
+	binding1 := *denyBinding
+	binding2 := *denyBinding
+
+	binding1.Name = "denybinding1.example.com"
+	binding1.Spec.PolicyName = policy1.Name
+	binding2.Name = "denybinding2.example.com"
+	binding2.Spec.PolicyName = policy2.Name
+
+	compiles1 := atomic.Int64{}
+	evaluations1 := atomic.Int64{}
+
+	compiles2 := atomic.Int64{}
+	evaluations2 := atomic.Int64{}
+
+	compiler.RegisterDefinition(&policy1, func(vap *v1alpha1.ValidatingAdmissionPolicy) Validator {
+		compiles1.Add(1)
+
+		return validatorFunc(func(a admission.Attributes, o admission.ObjectInterfaces, params runtime.Object, matchKind schema.GroupVersionKind) ([]policyDecision, error) {
+			evaluations1.Add(1)
+			return []policyDecision{
+				{
+					action: actionAdmit,
+				},
+			}, nil
+		})
+	}, nil)
+
+	compiler.RegisterDefinition(&policy2, func(vap *v1alpha1.ValidatingAdmissionPolicy) Validator {
+		compiles2.Add(1)
+
+		return validatorFunc(func(a admission.Attributes, o admission.ObjectInterfaces, params runtime.Object, matchKind schema.GroupVersionKind) ([]policyDecision, error) {
+			evaluations2.Add(1)
+			return []policyDecision{
+				{
+					action:  actionDeny,
+					message: "Policy2Denied",
+				},
+			}, nil
+		})
+	}, nil)
+
+	require.NoError(t, tracker.Create(definitionsGVR, &policy1, policy1.Namespace))
+	require.NoError(t, tracker.Create(bindingsGVR, &binding1, binding1.Namespace))
+	require.NoError(t, paramTracker.Add(fakeParams))
+
+	// Wait for controller to reconcile given objects
+	require.NoError(t,
+		waitForReconcile(
+			testContext, controller,
+			&binding1, &policy1, fakeParams))
+
+	// Make sure policy 1 is created and bound to the params type first
+	require.NoError(t, tracker.Create(definitionsGVR, &policy2, policy2.Namespace))
+	require.NoError(t, tracker.Create(bindingsGVR, &binding2, binding2.Namespace))
+
+	// Wait for controller to reconcile given objects
+	require.NoError(t,
+		waitForReconcile(
+			testContext, controller,
+			&binding1, &binding2, &policy1, &policy2, fakeParams))
+
+	err := handler.Validate(
+		testContext,
+		// Object is irrelevant/unchecked for this test. Just test that
+		// the evaluator is executed, and returns admit meaning the params
+		// passed was a configmap
+		attributeRecord(nil, fakeParams, admission.Create),
+		&admission.RuntimeObjectInterfaces{},
+	)
+
+	require.ErrorContains(t, err, `Denied`)
+	require.EqualValues(t, 1, compiles1.Load())
+	require.EqualValues(t, 1, evaluations1.Load())
+	require.EqualValues(t, 1, compiles2.Load())
+	require.EqualValues(t, 1, evaluations2.Load())
+
+	// Remove param type from policy1
+	// Show that policy2 evaluator is still being passed the configmaps
+	policy1.Spec.ParamKind = nil
+	policy1.ResourceVersion = "2"
+
+	binding1.Spec.ParamRef = nil
+	binding1.ResourceVersion = "2"
+
+	require.NoError(t, tracker.Update(definitionsGVR, &policy1, policy1.Namespace))
+	require.NoError(t, tracker.Update(bindingsGVR, &binding1, binding1.Namespace))
+	require.NoError(t,
+		waitForReconcile(
+			testContext, controller,
+			&binding1, &policy1))
+
+	err = handler.Validate(
+		testContext,
+		// Object is irrelevant/unchecked for this test. Just test that
+		// the evaluator is executed, and returns admit meaning the params
+		// passed was a configmap
+		attributeRecord(nil, fakeParams, admission.Create),
+		&admission.RuntimeObjectInterfaces{},
+	)
+
+	require.ErrorContains(t, err, `Policy2Denied`)
+	require.EqualValues(t, 2, compiles1.Load())
+	require.EqualValues(t, 2, evaluations1.Load())
+	require.EqualValues(t, 1, compiles2.Load())
+	require.EqualValues(t, 2, evaluations2.Load())
+}

staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go

@@ -229,8 +229,12 @@ func (c *policyController) reconcilePolicyDefinition(namespace, name string, def
 		return info.configurationError
 	}
 
-	// Start watching the param CRD
-	if _, ok := c.paramsCRDControllers[*paramSource]; !ok {
+	if info, ok := c.paramsCRDControllers[*paramSource]; ok {
+		// If a param controller is already active for this paramsource, make
+		// sure it is tracking this policy's dependency upon it
+		info.dependentDefinitions.Insert(nn)
+
+	} else {
 		instanceContext, instanceCancel := context.WithCancel(c.context)
 
 		// Watch for new instances of this policy