diff --git a/go.mod b/go.mod index f4d7c1f1e..47344ab8d 100644 --- a/go.mod +++ b/go.mod @@ -43,9 +43,9 @@ require ( gopkg.in/natefinch/lumberjack.v2 v2.0.0 gopkg.in/square/go-jose.v2 v2.2.2 k8s.io/api v0.0.0-20230112183318-59fcd23597fd - k8s.io/apimachinery v0.0.0-20230117181045-35ae53239086 - k8s.io/client-go v0.0.0-20230118115900-e6998df99e84 - k8s.io/component-base v0.0.0-20230112192324-33f62c7b2818 + k8s.io/apimachinery v0.0.0-20230119000403-b39d3a97b9a6 + k8s.io/client-go v0.0.0-20230119002741-c33df1cccbfb + k8s.io/component-base v0.0.0-20230119004332-38b99c2c9e9a k8s.io/klog/v2 v2.80.1 k8s.io/kms v0.0.0-20230117203143-407acb40b4f8 k8s.io/kube-openapi v0.0.0-20230109183929-3758b55a6596 @@ -123,8 +123,8 @@ require ( replace ( k8s.io/api => k8s.io/api v0.0.0-20230112183318-59fcd23597fd - k8s.io/apimachinery => k8s.io/apimachinery v0.0.0-20230117181045-35ae53239086 - k8s.io/client-go => k8s.io/client-go v0.0.0-20230118115900-e6998df99e84 - k8s.io/component-base => k8s.io/component-base v0.0.0-20230112192324-33f62c7b2818 + k8s.io/apimachinery => k8s.io/apimachinery v0.0.0-20230119000403-b39d3a97b9a6 + k8s.io/client-go => k8s.io/client-go v0.0.0-20230119002741-c33df1cccbfb + k8s.io/component-base => k8s.io/component-base v0.0.0-20230119004332-38b99c2c9e9a k8s.io/kms => k8s.io/kms v0.0.0-20230117203143-407acb40b4f8 ) diff --git a/go.sum b/go.sum index 2c5ecd314..a7bb09aa6 100644 --- a/go.sum +++ b/go.sum @@ -992,12 +992,12 @@ honnef.co/go/tools v0.0.1-2020.1.3/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9 honnef.co/go/tools v0.0.1-2020.1.4/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9vFzvIQ3k= k8s.io/api v0.0.0-20230112183318-59fcd23597fd h1:Y/vljv797SrqMUZ4KwQaIDcz+lecu074ACcke2d8D9s= k8s.io/api v0.0.0-20230112183318-59fcd23597fd/go.mod h1:K1fbar68K91an+8RbH9eJWJRnoXQ5rugf6J5wsGMMWg= -k8s.io/apimachinery v0.0.0-20230117181045-35ae53239086 h1:7yhcU6Liv6A9zpgeLsSASfmgpBHz/4WT7WK0Y+szTsM= -k8s.io/apimachinery v0.0.0-20230117181045-35ae53239086/go.mod h1:Kq932aOItR+rI/aiAVDXxxdRFNf0eLnI0gZmXJ6hl3o= -k8s.io/client-go v0.0.0-20230118115900-e6998df99e84 h1:YcxNPmKn8JNmwxPqAbONbH0vCCM0G/IhBp4+KCd9LNw= -k8s.io/client-go v0.0.0-20230118115900-e6998df99e84/go.mod h1:q8YXsL0M5VV5aF6DprurfY/VjRi6ByqRrR22EmP4SUM= -k8s.io/component-base v0.0.0-20230112192324-33f62c7b2818 h1:sZ5awJx2PpXaOwNSEu8Pq6zovP1+YG/nWZpagmpDRcs= -k8s.io/component-base v0.0.0-20230112192324-33f62c7b2818/go.mod h1:6IeVfLNdhjJkJFQHYMuwk7dgkOLA3Pt+Y7j7Md1d+CA= +k8s.io/apimachinery v0.0.0-20230119000403-b39d3a97b9a6 h1:nxc7EioumBwPm/wfQ2OfTyKq7sSt7RSThMTheCqe7Gg= +k8s.io/apimachinery v0.0.0-20230119000403-b39d3a97b9a6/go.mod h1:mb1AP2xs7Ajs+OvXRynwIgKVID9/rOtI7lFxIixvFp0= +k8s.io/client-go v0.0.0-20230119002741-c33df1cccbfb h1:b86Po/QFXB7JQOSAb/fc/hBa33vGyhFChy7NyGZl31s= +k8s.io/client-go v0.0.0-20230119002741-c33df1cccbfb/go.mod h1:4LcNfL89QHAE9BINU66MM61I4GJ0BLuE8xSYu+OIEXg= +k8s.io/component-base v0.0.0-20230119004332-38b99c2c9e9a h1:zYjKaAlDW2q7W1t4W8ds+wn+AB09ZlA68rtcz+9EMyg= +k8s.io/component-base v0.0.0-20230119004332-38b99c2c9e9a/go.mod h1:ljIiNP+BVFJrmiMy7ELMvnp161IE9EPA7XL3ZkniZ7U= k8s.io/klog/v2 v2.80.1 h1:atnLQ121W371wYYFawwYx1aEY2eUfs4l3J72wtgAwV4= k8s.io/klog/v2 v2.80.1/go.mod h1:y1WjHnz7Dj687irZUWR/WLkLc5N1YHtjLdmgWjndZn0= k8s.io/kms v0.0.0-20230117203143-407acb40b4f8 h1:oYOLi+NfW2BoplN1w5lO0NdBaQpV5Oe8C2XmFkjFi8g= diff --git a/pkg/admission/plugin/validatingadmissionpolicy/admission_test.go b/pkg/admission/plugin/validatingadmissionpolicy/admission_test.go index 2ba862136..3e41a9684 100644 --- a/pkg/admission/plugin/validatingadmissionpolicy/admission_test.go +++ b/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()) +} diff --git a/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go b/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go index b3fdcaa9e..bb2289f1b 100644 --- a/pkg/admission/plugin/validatingadmissionpolicy/controller_reconcile.go +++ b/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