From 7c4c7596368b6fd50940a9e87ddd7550d768ffc7 Mon Sep 17 00:00:00 2001 From: Dharmjit Singh Date: Wed, 10 Jul 2024 14:44:59 +0530 Subject: [PATCH 1/4] Added EventPolicy Webhook Signed-off-by: Dharmjit Singh --- cmd/webhook/main.go | 3 ++ .../v1alpha1/eventpolicy_validation.go | 4 ++ .../v1alpha1/eventpolicy_validation_test.go | 51 ++++++++++++++++++- 3 files changed, 56 insertions(+), 2 deletions(-) diff --git a/cmd/webhook/main.go b/cmd/webhook/main.go index 902e4e4bb22..e21ec0e9956 100644 --- a/cmd/webhook/main.go +++ b/cmd/webhook/main.go @@ -26,6 +26,7 @@ import ( kubeclient "knative.dev/pkg/client/injection/kube/client" configmapinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/configmap/filtered" + eventingv1alpha1 "knative.dev/eventing/pkg/apis/eventing/v1alpha1" eventingv1beta3 "knative.dev/eventing/pkg/apis/eventing/v1beta3" "knative.dev/eventing/pkg/apis/feature" "knative.dev/eventing/pkg/apis/sinks" @@ -75,6 +76,8 @@ func init() { var ourTypes = map[schema.GroupVersionKind]resourcesemantics.GenericCRD{ // For group eventing.knative.dev. + // v1alpha1 + eventingv1alpha1.SchemeGroupVersion.WithKind("EventPolicy"): &eventingv1alpha1.EventPolicy{}, // v1beta1 eventingv1beta1.SchemeGroupVersion.WithKind("EventType"): &eventingv1beta1.EventType{}, // v1beta2 diff --git a/pkg/apis/eventing/v1alpha1/eventpolicy_validation.go b/pkg/apis/eventing/v1alpha1/eventpolicy_validation.go index 0c267b31968..756d5b50c0e 100644 --- a/pkg/apis/eventing/v1alpha1/eventpolicy_validation.go +++ b/pkg/apis/eventing/v1alpha1/eventpolicy_validation.go @@ -20,6 +20,7 @@ import ( "context" "strings" + "knative.dev/eventing/pkg/apis/feature" "knative.dev/pkg/apis" ) @@ -28,6 +29,9 @@ func (ep *EventPolicy) Validate(ctx context.Context) *apis.FieldError { } func (ets *EventPolicySpec) Validate(ctx context.Context) *apis.FieldError { + if !feature.FromContext(ctx).IsOIDCAuthentication() { + return apis.ErrGeneric("oidc-authentication feature not enabled") + } var err *apis.FieldError for i, f := range ets.From { if f.Ref == nil && (f.Sub == nil || *f.Sub == "") { diff --git a/pkg/apis/eventing/v1alpha1/eventpolicy_validation_test.go b/pkg/apis/eventing/v1alpha1/eventpolicy_validation_test.go index da103fd069f..708592736a7 100644 --- a/pkg/apis/eventing/v1alpha1/eventpolicy_validation_test.go +++ b/pkg/apis/eventing/v1alpha1/eventpolicy_validation_test.go @@ -21,11 +21,55 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "knative.dev/eventing/pkg/apis/feature" "knative.dev/pkg/apis" "knative.dev/pkg/ptr" ) -func TestEventPolicySpecValidation(t *testing.T) { +func TestEventPolicySpecValidationWithOIDCAuthenticationFeatureFlagDisabled(t *testing.T) { + tests := []struct { + name string + ep *EventPolicy + want *apis.FieldError + }{ + { + name: "valid, from.sub exactly '*'", + ep: &EventPolicy{ + Spec: EventPolicySpec{ + From: []EventPolicySpecFrom{{ + Sub: ptr.String("*"), + }}, + }, + }, + want: func() *apis.FieldError { + return apis.ErrGeneric("oidc-authentication feature not enabled") + }(), + }, + { + name: "invalid, missing from.ref and from.sub", + ep: &EventPolicy{ + Spec: EventPolicySpec{ + From: []EventPolicySpecFrom{{}}, + }, + }, + want: func() *apis.FieldError { + return apis.ErrGeneric("oidc-authentication feature not enabled") + }(), + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + ctx := feature.ToContext(context.TODO(), feature.Flags{ + feature.OIDCAuthentication: feature.Disabled, + }) + got := test.ep.Validate(ctx) + if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" { + t.Errorf("%s: Validate EventPolicySpec (-want, +got) = %v", test.name, diff) + } + }) + } +} +func TestEventPolicySpecValidationWithOIDCAuthenticationFeatureFlagEnabled(t *testing.T) { tests := []struct { name string ep *EventPolicy @@ -252,7 +296,10 @@ func TestEventPolicySpecValidation(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - got := test.ep.Validate(context.TODO()) + ctx := feature.ToContext(context.TODO(), feature.Flags{ + feature.OIDCAuthentication: feature.Enabled, + }) + got := test.ep.Validate(ctx) if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" { t.Errorf("%s: Validate EventPolicySpec (-want, +got) = %v", test.name, diff) } From 5cb354f2d6408671549cf67aae7c3dac766f49b7 Mon Sep 17 00:00:00 2001 From: Dharmjit Singh Date: Sat, 13 Jul 2024 08:21:24 +0530 Subject: [PATCH 2/4] eventpolicy webhook validation for oidc feature will be called for create or spec updates --- pkg/apis/eventing/v1alpha1/eventpolicy_validation.go | 10 +++++++--- .../eventing/v1alpha1/eventpolicy_validation_test.go | 2 ++ pkg/reconciler/channel/channel_test.go | 4 ++++ 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/pkg/apis/eventing/v1alpha1/eventpolicy_validation.go b/pkg/apis/eventing/v1alpha1/eventpolicy_validation.go index 756d5b50c0e..9fdc961d790 100644 --- a/pkg/apis/eventing/v1alpha1/eventpolicy_validation.go +++ b/pkg/apis/eventing/v1alpha1/eventpolicy_validation.go @@ -25,13 +25,17 @@ import ( ) func (ep *EventPolicy) Validate(ctx context.Context) *apis.FieldError { + // To not allow creation or spec updates of EventPolicy CRs + // if the oidc-authentication feature is not enabled + if apis.IsInCreate(ctx) || apis.IsInSpec(ctx) { + if !feature.FromContext(ctx).IsOIDCAuthentication() { + return apis.ErrGeneric("oidc-authentication feature not enabled") + } + } return ep.Spec.Validate(ctx).ViaField("spec") } func (ets *EventPolicySpec) Validate(ctx context.Context) *apis.FieldError { - if !feature.FromContext(ctx).IsOIDCAuthentication() { - return apis.ErrGeneric("oidc-authentication feature not enabled") - } var err *apis.FieldError for i, f := range ets.From { if f.Ref == nil && (f.Sub == nil || *f.Sub == "") { diff --git a/pkg/apis/eventing/v1alpha1/eventpolicy_validation_test.go b/pkg/apis/eventing/v1alpha1/eventpolicy_validation_test.go index 708592736a7..5be578ec3b9 100644 --- a/pkg/apis/eventing/v1alpha1/eventpolicy_validation_test.go +++ b/pkg/apis/eventing/v1alpha1/eventpolicy_validation_test.go @@ -62,6 +62,7 @@ func TestEventPolicySpecValidationWithOIDCAuthenticationFeatureFlagDisabled(t *t ctx := feature.ToContext(context.TODO(), feature.Flags{ feature.OIDCAuthentication: feature.Disabled, }) + ctx = apis.WithinCreate(ctx) got := test.ep.Validate(ctx) if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" { t.Errorf("%s: Validate EventPolicySpec (-want, +got) = %v", test.name, diff) @@ -69,6 +70,7 @@ func TestEventPolicySpecValidationWithOIDCAuthenticationFeatureFlagDisabled(t *t }) } } + func TestEventPolicySpecValidationWithOIDCAuthenticationFeatureFlagEnabled(t *testing.T) { tests := []struct { name string diff --git a/pkg/reconciler/channel/channel_test.go b/pkg/reconciler/channel/channel_test.go index 9835098b435..483939d1d1f 100644 --- a/pkg/reconciler/channel/channel_test.go +++ b/pkg/reconciler/channel/channel_test.go @@ -20,6 +20,7 @@ import ( "testing" eventingv1alpha1 "knative.dev/eventing/pkg/apis/eventing/v1alpha1" + "knative.dev/eventing/pkg/apis/feature" v1 "knative.dev/eventing/pkg/apis/messaging/v1" @@ -576,6 +577,9 @@ func TestReconcile(t *testing.T) { table.Test(t, MakeFactory(func(ctx context.Context, listers *Listers, cmw configmap.Watcher) controller.Reconciler { ctx = channelable.WithDuck(ctx) ctx = v1addr.WithDuck(ctx) + ctx = feature.ToContext(ctx, feature.Flags{ + feature.OIDCAuthentication: feature.Enabled, + }) r := &Reconciler{ dynamicClientSet: fakedynamicclient.Get(ctx), channelLister: listers.GetMessagingChannelLister(), From f7bc7ff2915e9a68e5a924f1e1675a935104c9ff Mon Sep 17 00:00:00 2001 From: Dharmjit Singh Date: Wed, 17 Jul 2024 13:40:10 +0530 Subject: [PATCH 3/4] fixed pr comments Signed-off-by: Dharmjit Singh --- pkg/apis/eventing/v1alpha1/eventpolicy_validation.go | 2 +- pkg/apis/eventing/v1alpha1/eventpolicy_validation_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/apis/eventing/v1alpha1/eventpolicy_validation.go b/pkg/apis/eventing/v1alpha1/eventpolicy_validation.go index 9fdc961d790..5f05c240df9 100644 --- a/pkg/apis/eventing/v1alpha1/eventpolicy_validation.go +++ b/pkg/apis/eventing/v1alpha1/eventpolicy_validation.go @@ -27,7 +27,7 @@ import ( func (ep *EventPolicy) Validate(ctx context.Context) *apis.FieldError { // To not allow creation or spec updates of EventPolicy CRs // if the oidc-authentication feature is not enabled - if apis.IsInCreate(ctx) || apis.IsInSpec(ctx) { + if apis.IsInCreate(ctx) || (apis.IsInUpdate(ctx) && apis.IsInSpec(ctx)) { if !feature.FromContext(ctx).IsOIDCAuthentication() { return apis.ErrGeneric("oidc-authentication feature not enabled") } diff --git a/pkg/apis/eventing/v1alpha1/eventpolicy_validation_test.go b/pkg/apis/eventing/v1alpha1/eventpolicy_validation_test.go index 5be578ec3b9..20034c8f907 100644 --- a/pkg/apis/eventing/v1alpha1/eventpolicy_validation_test.go +++ b/pkg/apis/eventing/v1alpha1/eventpolicy_validation_test.go @@ -146,7 +146,7 @@ func TestEventPolicySpecValidationWithOIDCAuthenticationFeatureFlagEnabled(t *te }(), }, { - name: "invalid, bot from.ref and from.sub set", + name: "invalid, both from.ref and from.sub set for the same list element", ep: &EventPolicy{ Spec: EventPolicySpec{ From: []EventPolicySpecFrom{{ @@ -175,7 +175,7 @@ func TestEventPolicySpecValidationWithOIDCAuthenticationFeatureFlagEnabled(t *te }(), }, { - name: "invalid, both to.ref and to.selector set", + name: "invalid, both to.ref and to.selector set for the same list element", ep: &EventPolicy{ Spec: EventPolicySpec{ To: []EventPolicySpecTo{ From 4212eecf0b6aa31a708084b480013ada6772eaf2 Mon Sep 17 00:00:00 2001 From: Dharmjit Singh Date: Fri, 19 Jul 2024 13:11:51 +0530 Subject: [PATCH 4/4] Include context in channel tests for oidc feature disabled Signed-off-by: Dharmjit Singh --- pkg/reconciler/channel/channel_test.go | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/pkg/reconciler/channel/channel_test.go b/pkg/reconciler/channel/channel_test.go index 483939d1d1f..7df891b7e8f 100644 --- a/pkg/reconciler/channel/channel_test.go +++ b/pkg/reconciler/channel/channel_test.go @@ -168,6 +168,9 @@ func TestReconcile(t *testing.T) { WithChannelAddress(&backingChannelAddressable), WithChannelEventPoliciesReadyBecauseOIDCDisabled()), }}, + Ctx: feature.ToContext(context.Background(), feature.Flags{ + feature.OIDCAuthentication: feature.Disabled, + }), }, { Name: "Already reconciled", Key: testKey, @@ -191,6 +194,9 @@ func TestReconcile(t *testing.T) { WithInMemoryChannelDLSUnknown(), WithInMemoryChannelEventPoliciesReady()), }, + Ctx: feature.ToContext(context.Background(), feature.Flags{ + feature.OIDCAuthentication: feature.Disabled, + }), }, { Name: "Backing channel created", Key: testKey, @@ -215,6 +221,9 @@ func TestReconcile(t *testing.T) { WithBackingChannelUnknown("BackingChannelNotConfigured", "BackingChannel has not yet been reconciled."), WithChannelEventPoliciesReadyBecauseOIDCDisabled()), }}, + Ctx: feature.ToContext(context.Background(), feature.Flags{ + feature.OIDCAuthentication: feature.Disabled, + }), }, { Name: "Backing channel created with delivery", Key: testKey, @@ -266,6 +275,9 @@ func TestReconcile(t *testing.T) { WithBackingChannelUnknown("BackingChannelNotConfigured", "BackingChannel has not yet been reconciled."), WithChannelEventPoliciesReadyBecauseOIDCDisabled()), }}, + Ctx: feature.ToContext(context.Background(), feature.Flags{ + feature.OIDCAuthentication: feature.Disabled, + }), }, { Name: "Generation Bump", Key: testKey, @@ -301,6 +313,9 @@ func TestReconcile(t *testing.T) { WithChannelObservedGeneration(42), WithChannelEventPoliciesReadyBecauseOIDCDisabled()), }}, + Ctx: feature.ToContext(context.Background(), feature.Flags{ + feature.OIDCAuthentication: feature.Disabled, + }), }, { Name: "Updating subscribers statuses", Key: testKey, @@ -336,6 +351,9 @@ func TestReconcile(t *testing.T) { WithChannelDLSUnknown(), WithChannelEventPoliciesReadyBecauseOIDCDisabled()), }}, + Ctx: feature.ToContext(context.Background(), feature.Flags{ + feature.OIDCAuthentication: feature.Disabled, + }), }, { Name: "Should provision applying EventPolicies", Key: testKey, @@ -577,9 +595,6 @@ func TestReconcile(t *testing.T) { table.Test(t, MakeFactory(func(ctx context.Context, listers *Listers, cmw configmap.Watcher) controller.Reconciler { ctx = channelable.WithDuck(ctx) ctx = v1addr.WithDuck(ctx) - ctx = feature.ToContext(ctx, feature.Flags{ - feature.OIDCAuthentication: feature.Enabled, - }) r := &Reconciler{ dynamicClientSet: fakedynamicclient.Get(ctx), channelLister: listers.GetMessagingChannelLister(),