Skip to content

Commit

Permalink
Create validation function for cross namespace referencing (knative#7812
Browse files Browse the repository at this point in the history 
)

* added validation

* commented out struct

* updated interface

* deleted unnecessary comments

* updated contents of the interface

* changed some names

* Added checks for validating channel

* Updated error details

* Commented out unused variables

* aded GetCrossNamespaceRef() for subscription

* fixed type error

* added validation checks

* added condition for setting namespace

* added GetCrossNamespaceRef for triggers

* added validation for triggers

* updated trigger and subscription types

* got rid of unnecessary flag check

* fixed subscription type

* fixed types and validation

* added license

* added nil check; empty kreference will be returned

* comments cleanup

* added function comment

* comments cleanup

* description update

* linting fix

* linting fix

* unit test for subscription validation

* updated channel namespace for unit test

* Update pkg/crossnamespace/validation.go

Co-authored-by: Calum Murray <cmurray@redhat.com>

* added conditional check in checknamespace

* moved comment

* unit test error fix

* unit test error fix

* unit test

* unit test fix

* updated error messages

* trigger validation and unit tests

* updated validation logic

* new unit test

* comment cleanup

---------

Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
Co-authored-by: Calum Murray <cmurray@redhat.com>
  • Loading branch information
@yijie-04 @pierDipi @Cali0707
3 people committed Apr 28, 2024
1 parent 4a3f65d commit 217f7a3
Show file tree
Hide file tree
Showing 10 changed files with 596 additions and 8 deletions.
7 changes: 7 additions & 0 deletions config/core/roles/webhook-clusterrole.yaml
View file
Expand Up @@ -162,6 +162,13 @@ rules:
- "patch"
- "watch"

# For checking if user has permissions to make a cross namespace resource
- apiGroups:
- "authorization.k8s.io"
resources:
- "subjectaccessreviews"
verbs:
- "create"

# Necessary for conversion webhook. These are copied from the serving
# TODO: Do we really need all these permissions?
Expand Down
8 changes: 8 additions & 0 deletions pkg/apis/eventing/v1/trigger_types.go
View file
Expand Up @@ -226,3 +226,11 @@ type TriggerList struct {
func (t *Trigger) GetStatus() *duckv1.Status {
return &t.Status.Status
}

// GetCrossNamespaceRef returns the Broker reference for the Trigger. Implements the ResourceInfo interface.
func (t *Trigger) GetCrossNamespaceRef() duckv1.KReference {
if t.Spec.BrokerRef != nil {
return *t.Spec.BrokerRef
}
return duckv1.KReference{}
}
19 changes: 18 additions & 1 deletion pkg/apis/eventing/v1/trigger_validation.go
View file
Expand Up @@ -25,6 +25,7 @@ import (
cesqlparser "github.com/cloudevents/sdk-go/sql/v2/parser"
"go.uber.org/zap"
corev1 "k8s.io/api/core/v1"
cn "knative.dev/eventing/pkg/crossnamespace"
"knative.dev/pkg/apis"
"knative.dev/pkg/kmp"
"knative.dev/pkg/logging"
Expand All @@ -46,13 +47,29 @@ func (t *Trigger) Validate(ctx context.Context) *apis.FieldError {
original := apis.GetBaseline(ctx).(*Trigger)
errs = errs.Also(t.CheckImmutableFields(ctx, original))
}
if feature.FromContext(ctx).IsEnabled(feature.CrossNamespaceEventLinks) && t.Spec.BrokerRef != nil {
crossNamespaceError := cn.CheckNamespace(ctx, t)
if crossNamespaceError != nil {
errs = errs.Also(crossNamespaceError)
}
}
return errs
}

// Validate the TriggerSpec.
func (ts *TriggerSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
if ts.Broker == "" {
if ts.BrokerRef == nil && ts.Broker == "" {
errs = errs.Also(apis.ErrMissingField("broker"))
} else if ts.BrokerRef != nil && ts.Broker != "" {
errs = errs.Also(apis.ErrMultipleOneOf("broker", "brokerRef"))
}

if !feature.FromContext(ctx).IsEnabled(feature.CrossNamespaceEventLinks) && ts.BrokerRef != nil {
if ts.BrokerRef.Namespace != "" {
fe := apis.ErrDisallowedFields("namespace")
fe.Details = "only name, apiVersion and kind are supported fields when feature.CrossNamespaceEventLinks is disabled"
errs = errs.Also(fe)
}
}

return errs.Also(
Expand Down
204 changes: 204 additions & 0 deletions pkg/apis/eventing/v1/trigger_validation_test.go
View file
Expand Up @@ -475,6 +475,30 @@ func TestTriggerSpecValidation(t *testing.T) {
},
},
want: apis.ErrInvalidValue(invalidString, "delivery.backoffDelay"),
}, {
name: "empty Broker and empty BrokerRef",
ts: &TriggerSpec{
Broker: "",
BrokerRef: nil,
Filter: validTriggerFilter,
Subscriber: validSubscriber,
},
want: apis.ErrMissingField("broker"),
}, {
name: "BrokerRef has different namespace",
ts: &TriggerSpec{
BrokerRef: &duckv1.KReference{
Name: "test-broker",
Namespace: "test-broker-ns",
},
Filter: validTriggerFilter,
Subscriber: validSubscriber,
},
want: func() *apis.FieldError {
fe := apis.ErrDisallowedFields("namespace")
fe.Details = "only name, apiVersion and kind are supported fields when feature.CrossNamespaceEventLinks is disabled"
return fe
}(),
}}

for _, test := range tests {
Expand All @@ -487,6 +511,186 @@ func TestTriggerSpecValidation(t *testing.T) {
}
}

func TestTriggerSpecValidationWithCrossNamespaceEventLinksFeatureEnabled(t *testing.T) {
invalidString := "invalid time"
tests := []struct {
name string
ts *TriggerSpec
want *apis.FieldError
}{{
name: "invalid trigger spec",
ts: &TriggerSpec{},
want: func() *apis.FieldError {
var errs *apis.FieldError
fe := apis.ErrMissingField("broker")
errs = errs.Also(fe)
fe = apis.ErrGeneric("expected at least one, got none", "subscriber.ref", "subscriber.uri")
errs = errs.Also(fe)
return errs
}(),
}, {
name: "missing broker",
ts: &TriggerSpec{
Broker: "",
Filter: validTriggerFilter,
Subscriber: validSubscriber,
},
want: func() *apis.FieldError {
fe := apis.ErrMissingField("broker")
return fe
}(),
}, {
name: "missing attributes keys, match all",
ts: &TriggerSpec{
Broker: "test_broker",
Filter: validEmptyTriggerFilter,
Subscriber: validSubscriber,
},
want: &apis.FieldError{},
}, {
name: "invalid attribute name, start with number",
ts: &TriggerSpec{
Broker: "test_broker",
Filter: newTriggerFilter(
map[string]string{
"0invalid": "my-value",
}),
Subscriber: validSubscriber,
},
want: apis.ErrInvalidKeyName("0invalid", apis.CurrentField,
"Attribute name must start with a letter and can only contain "+
"lowercase alphanumeric").ViaFieldKey("attributes", "0invalid").ViaField("filter"),
}, {
name: "invalid attribute name, capital letters",
ts: &TriggerSpec{
Broker: "test_broker",
Filter: newTriggerFilter(
map[string]string{
"invALID": "my-value",
}),
Subscriber: validSubscriber,
},
want: apis.ErrInvalidKeyName("invALID", apis.CurrentField,
"Attribute name must start with a letter and can only contain "+
"lowercase alphanumeric").ViaFieldKey("attributes", "invALID").ViaField("filter"),
}, {
name: "missing subscriber",
ts: &TriggerSpec{
Broker: "test_broker",
Filter: validTriggerFilter,
},
want: apis.ErrGeneric("expected at least one, got none", "subscriber.ref", "subscriber.uri"),
}, {
name: "missing subscriber.ref.name",
ts: &TriggerSpec{
Broker: "test_broker",
Filter: validTriggerFilter,
Subscriber: invalidSubscriber,
},
want: apis.ErrMissingField("subscriber.ref.name"),
}, {
name: "missing broker",
ts: &TriggerSpec{
Broker: "",
Filter: validTriggerFilter,
Subscriber: validSubscriber,
},
want: apis.ErrMissingField("broker"),
}, {
name: "valid empty filter",
ts: &TriggerSpec{
Broker: "test_broker",
Filter: validEmptyTriggerFilter,
Subscriber: validSubscriber,
},
want: &apis.FieldError{},
}, {
name: "valid SourceAndType filter",
ts: &TriggerSpec{
Broker: "test_broker",
Filter: validTriggerFilter,
Subscriber: validSubscriber,
},
want: &apis.FieldError{},
}, {
name: "valid Attributes filter",
ts: &TriggerSpec{
Broker: "test_broker",
Filter: validTriggerFilter,
Subscriber: validSubscriber,
},
want: &apis.FieldError{},
}, {
name: "invalid delivery, invalid delay string",
ts: &TriggerSpec{
Broker: "test_broker",
Filter: validEmptyTriggerFilter,
Subscriber: validSubscriber,
Delivery: &eventingduckv1.DeliverySpec{
BackoffDelay: &invalidString,
},
},
want: apis.ErrInvalidValue(invalidString, "delivery.backoffDelay"),
}, {
name: "using BrokerRef",
ts: &TriggerSpec{
BrokerRef: &duckv1.KReference{
Name: "test-broker",
Namespace: "test-broker-ns",
},
Filter: validTriggerFilter,
Subscriber: validSubscriber,
},
want: nil,
}, {
name: "non-empty Broker but empty BrokerRef",
ts: &TriggerSpec{
BrokerRef: &duckv1.KReference{
Name: "test-broker",
Namespace: "test-broker-ns",
},
Filter: validTriggerFilter,
Subscriber: validSubscriber,
},
want: nil,
}, {
name: "empty Broker but non-empty BrokerRef",
ts: &TriggerSpec{
BrokerRef: &duckv1.KReference{
Name: "test-broker",
Namespace: "test-broker-ns",
},
Filter: validTriggerFilter,
Subscriber: validSubscriber,
},
want: nil,
}, {
name: "non-empty Broker and BrokerRef",
ts: &TriggerSpec{
Broker: "test-broker",
BrokerRef: &duckv1.KReference{
Name: "test-broker",
Namespace: "test-broker-ns",
},
Filter: validTriggerFilter,
Subscriber: validSubscriber,
},
want: apis.ErrMultipleOneOf("broker", "brokerRef"),
}}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
ctx := feature.ToContext(context.TODO(), feature.Flags{
feature.CrossNamespaceEventLinks: feature.Enabled,
})
got := test.ts.Validate(ctx)
if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" {
t.Errorf("Validate TriggerSpec (-want, +got) =\n%s", diff)
}
})
}
}

func TestFilterSpecValidation(t *testing.T) {
newTriggerFiltersEnabledCtx := feature.ToContext(context.TODO(), feature.Flags{
feature.NewTriggerFilters: feature.Enabled,
Expand Down
13 changes: 8 additions & 5 deletions pkg/apis/messaging/v1/subscribable_channelable_validation.go
View file
Expand Up @@ -20,6 +20,7 @@ import (
"context"

"k8s.io/apimachinery/pkg/api/equality"
"knative.dev/eventing/pkg/apis/feature"
"knative.dev/pkg/apis"
duckv1 "knative.dev/pkg/apis/duck/v1"
)
Expand All @@ -32,11 +33,13 @@ func isChannelEmpty(f duckv1.KReference) bool {
func isValidChannel(ctx context.Context, f duckv1.KReference) *apis.FieldError {
errs := f.Validate(ctx)

// Namespace field is disallowed
if f.Namespace != "" {
fe := apis.ErrDisallowedFields("namespace")
fe.Details = "only name, apiVersion and kind are supported fields"
errs = errs.Also(fe)
if !feature.FromContext(ctx).IsEnabled(feature.CrossNamespaceEventLinks) {
// Only name, apiVersion and kind are supported fields when feature.CrossNamespaceEventLinks is disabled
if f.Namespace != "" {
fe := apis.ErrDisallowedFields("namespace")
fe.Details = "only name, apiVersion and kind are supported fields when feature.CrossNamespaceEventLinks is disabled"
errs = errs.Also(fe)
}
}

return errs
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/messaging/v1/subscribable_channelable_validation_test.go
View file
Expand Up @@ -58,7 +58,7 @@ var validationTests = []struct {
},
want: func() *apis.FieldError {
fe := apis.ErrDisallowedFields("namespace")
fe.Details = "only name, apiVersion and kind are supported fields"
fe.Details = "only name, apiVersion and kind are supported fields when feature.CrossNamespaceEventLinks is disabled"
return fe
}(),
},
Expand Down
5 changes: 5 additions & 0 deletions pkg/apis/messaging/v1/subscription_types.go
View file
Expand Up @@ -182,3 +182,8 @@ func (s *Subscription) GetUntypedSpec() interface{} {
func (s *Subscription) GetStatus() *duckv1.Status {
return &s.Status.Status
}

// GetCrossNamespaceRef returns the Channel reference for the Subscription. Implements the ResourceInfo interface.
func (s *Subscription) GetCrossNamespaceRef() duckv1.KReference {
return s.Spec.Channel
}
9 changes: 9 additions & 0 deletions pkg/apis/messaging/v1/subscription_validation.go
View file
Expand Up @@ -21,6 +21,8 @@ import (

"github.com/google/go-cmp/cmp/cmpopts"
"k8s.io/apimachinery/pkg/api/equality"
"knative.dev/eventing/pkg/apis/feature"
cn "knative.dev/eventing/pkg/crossnamespace"
"knative.dev/pkg/apis"
duckv1 "knative.dev/pkg/apis/duck/v1"
"knative.dev/pkg/kmp"
Expand All @@ -32,6 +34,13 @@ func (s *Subscription) Validate(ctx context.Context) *apis.FieldError {
original := apis.GetBaseline(ctx).(*Subscription)
errs = errs.Also(s.CheckImmutableFields(ctx, original))
}
// s.Validate(ctx) because krshaped is defined on the entire subscription, not just the spec
if feature.FromContext(ctx).IsEnabled(feature.CrossNamespaceEventLinks) {
crossNamespaceError := cn.CheckNamespace(ctx, s)
if crossNamespaceError != nil {
errs = errs.Also(crossNamespaceError)
}
}
return errs
}

Expand Down

0 comments on commit 217f7a3

Please sign in to comment.