Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ce overrides validation #5730

Merged
merged 10 commits into from
Sep 17, 2021
Merged

Ce overrides validation #5730

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
921fbf1
added sourceSpec validation from pkg/duck/source to pingSource
gabo1208 Sep 10, 2021
b3903cd
bumped pkg version so source validation can be used on the pingsource
gabo1208 Sep 10, 2021
3167d6d
bumped up pkg version for the source tests to pass
gabo1208 Sep 15, 2021
a4be80b
removed logs conf
gabo1208 Sep 16, 2021
e69c2eb
added regression tests for pingsource ceOverrides validation
gabo1208 Sep 16, 2021
30f3a59
added regression tests for pingsource ceOverrides validation
gabo1208 Sep 17, 2021
1a8fdf3
added sinkbinding validation and test
gabo1208 Sep 17, 2021
e7c6d39
removed trailing spaces and added container sourceSpec validation
gabo1208 Sep 17, 2021
92e2539
added ceOVerrides validation on apiserver source spec
gabo1208 Sep 17, 2021
c9e3156
added pingsource ceOverrides validation and test on v1beta2
gabo1208 Sep 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ require (
k8s.io/utils v0.0.0-20201110183641-67b214c5f920
knative.dev/hack v0.0.0-20210806075220-815cd312d65c
knative.dev/hack/schema v0.0.0-20210806075220-815cd312d65c
knative.dev/pkg v0.0.0-20210914164111-4857ab6939e3
knative.dev/pkg v0.0.0-20210915183108-52b0e2938ecb
knative.dev/reconciler-test v0.0.0-20210820180205-a25de6a08087
sigs.k8s.io/yaml v1.2.0
)
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1131,8 +1131,8 @@ knative.dev/hack v0.0.0-20210806075220-815cd312d65c/go.mod h1:PHt8x8yX5Z9pPquBEf
knative.dev/hack/schema v0.0.0-20210806075220-815cd312d65c h1:YqFCmijfROO3rzIO8u1EMKZXBwAFJMmIoTXcr6wdBy8=
knative.dev/hack/schema v0.0.0-20210806075220-815cd312d65c/go.mod h1:ffjwmdcrH5vN3mPhO8RrF2KfNnbHeCE2C60A+2cv3U0=
knative.dev/pkg v0.0.0-20210803160015-21eb4c167cc5/go.mod h1:RPk5txNA3apR9X40D4MpUOP9/VqOG8CrtABWfOwGVS4=
knative.dev/pkg v0.0.0-20210914164111-4857ab6939e3 h1:45c2VIOBQP6jpRj+pEyciuzTmBwbZpv8jBfBEf/B5oM=
knative.dev/pkg v0.0.0-20210914164111-4857ab6939e3/go.mod h1:jMSqkNMsrzuy+XR4Yr/BMy7SDVbUOl3KKB6+5MR+ZU8=
knative.dev/pkg v0.0.0-20210915183108-52b0e2938ecb h1:D105ysprqq/uufMiOPUKNXoao6F1USYCEDN32UpJCv4=
knative.dev/pkg v0.0.0-20210915183108-52b0e2938ecb/go.mod h1:jMSqkNMsrzuy+XR4Yr/BMy7SDVbUOl3KKB6+5MR+ZU8=
knative.dev/reconciler-test v0.0.0-20210820180205-a25de6a08087 h1:HVcaI8P+CNHl5VgnqWgAzGoc9vbvf3D421rd2qEo3Wc=
knative.dev/reconciler-test v0.0.0-20210820180205-a25de6a08087/go.mod h1:+Kovy+G5zXZNcuO/uB+zfo37vFKZzsLIlWezt/nKMz0=
pgregory.net/rapid v0.3.3 h1:jCjBsY4ln4Atz78QoBWxUEvAHaFyNDQg9+WU62aCn1U=
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/sources/v1/apiserver_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,6 @@ func (cs *ApiServerSourceSpec) Validate(ctx context.Context) *apis.FieldError {
errs = errs.Also(apis.ErrMissingField("kind").ViaField("owner"))
}
}

errs = errs.Also(cs.SourceSpec.Validate(ctx))
return errs
}
26 changes: 26 additions & 0 deletions pkg/apis/sources/v1/apiserver_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,32 @@ func TestAPIServerValidation(t *testing.T) {
},
},
want: errors.New("missing field(s): resources"),
}, {
name: "invalid spec ceOverrides validation",
spec: ApiServerSourceSpec{
EventMode: "Resource",
Resources: []APIVersionKindSelector{{
APIVersion: "v1",
Kind: "Foo",
}},
SourceSpec: duckv1.SourceSpec{
CloudEventOverrides: &duckv1.CloudEventOverrides{
Extensions: map[string]string{"Invalid_type": "any value"},
},
Sink: duckv1.Destination{
Ref: &duckv1.KReference{
APIVersion: "v1",
Kind: "broker",
Name: "default",
},
},
},
},
want: apis.ErrInvalidKeyName(
"Invalid_type",
"ceOverrides.extensions",
"keys are expected to be alphanumeric",
),
}}

for _, test := range tests {
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/sources/v1/container_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ func (cs *ContainerSourceSpec) Validate(ctx context.Context) *apis.FieldError {
}
}
}
errs = errs.Also(cs.SourceSpec.Validate(ctx))
return errs
}

Expand Down
149 changes: 92 additions & 57 deletions pkg/apis/sources/v1/container_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,74 +31,109 @@ func TestContainerSourceValidation(t *testing.T) {
name string
spec ContainerSourceSpec
want *apis.FieldError
}{{
name: "missing container",
spec: ContainerSourceSpec{
Template: corev1.PodTemplateSpec{},
SourceSpec: duckv1.SourceSpec{
Sink: duckv1.Destination{
Ref: &duckv1.KReference{
APIVersion: "v1",
Kind: "Broker",
Name: "default",
}{
{
name: "missing container",
spec: ContainerSourceSpec{
Template: corev1.PodTemplateSpec{},
SourceSpec: duckv1.SourceSpec{
Sink: duckv1.Destination{
Ref: &duckv1.KReference{
APIVersion: "v1",
Kind: "Broker",
Name: "default",
},
},
},
},
},
want: func() *apis.FieldError {
var errs *apis.FieldError
fe := apis.ErrMissingField("containers")
errs = errs.Also(fe)
return errs
}(),
}, {
name: "missing container image",
spec: ContainerSourceSpec{
Template: corev1.PodTemplateSpec{
Spec: corev1.PodSpec{
Containers: []corev1.Container{{
Name: "test-name",
}},
want: func() *apis.FieldError {
var errs *apis.FieldError
fe := apis.ErrMissingField("containers")
errs = errs.Also(fe)
return errs
}(),
}, {
name: "missing container image",
spec: ContainerSourceSpec{
Template: corev1.PodTemplateSpec{
Spec: corev1.PodSpec{
Containers: []corev1.Container{{
Name: "test-name",
}},
},
},
},
SourceSpec: duckv1.SourceSpec{
Sink: duckv1.Destination{
Ref: &duckv1.KReference{
APIVersion: "eventing.knative.dev/v1",
Kind: "Broker",
Name: "default",
SourceSpec: duckv1.SourceSpec{
Sink: duckv1.Destination{
Ref: &duckv1.KReference{
APIVersion: "eventing.knative.dev/v1",
Kind: "Broker",
Name: "default",
},
},
},
},
},
want: func() *apis.FieldError {
var errs *apis.FieldError
fe := apis.ErrMissingField("containers[0].image")
errs = errs.Also(fe)
return errs
}(),
}, {
name: "empty sink",
spec: ContainerSourceSpec{
Template: corev1.PodTemplateSpec{
Spec: corev1.PodSpec{
Containers: []corev1.Container{{
Name: "name",
Image: "image",
}},
want: func() *apis.FieldError {
var errs *apis.FieldError
fe := apis.ErrMissingField("containers[0].image")
errs = errs.Also(fe)
return errs
}(),
}, {
name: "empty sink",
spec: ContainerSourceSpec{
Template: corev1.PodTemplateSpec{
Spec: corev1.PodSpec{
Containers: []corev1.Container{{
Name: "name",
Image: "image",
}},
},
},
SourceSpec: duckv1.SourceSpec{
Sink: duckv1.Destination{},
},
},
SourceSpec: duckv1.SourceSpec{
Sink: duckv1.Destination{},
want: func() *apis.FieldError {
var errs *apis.FieldError
fe := apis.ErrGeneric("expected at least one, got none", "sink.ref", "sink.uri")
errs = errs.Also(fe)
return errs
}(),
}, {
name: "invalid spec ceOverrides validation",
spec: ContainerSourceSpec{
Template: corev1.PodTemplateSpec{
Spec: corev1.PodSpec{
Containers: []corev1.Container{{
Name: "name",
Image: "image",
}},
},
},
SourceSpec: duckv1.SourceSpec{
CloudEventOverrides: &duckv1.CloudEventOverrides{
Extensions: map[string]string{"Invalid_type": "any value"},
},
Sink: duckv1.Destination{
Ref: &duckv1.KReference{
APIVersion: "v1",
Kind: "broker",
Name: "default",
},
},
},
},
want: func() *apis.FieldError {
var errs *apis.FieldError
fe := apis.ErrInvalidKeyName(
"Invalid_type",
"ceOverrides.extensions",
"keys are expected to be alphanumeric",
)
errs = errs.Also(fe)
return errs
}(),
},
want: func() *apis.FieldError {
var errs *apis.FieldError
fe := apis.ErrGeneric("expected at least one, got none", "sink.ref", "sink.uri")
errs = errs.Also(fe)
return errs
}(),
},
}

for _, test := range tests {
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/sources/v1/ping_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ func (cs *PingSourceSpec) Validate(ctx context.Context) *apis.FieldError {
}
}
}
errs = errs.Also(cs.SourceSpec.Validate(ctx))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do this for all other sources as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but, I don't know exactly where that should be. If you can give me a hand with this :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, we need to go through every file in pkg/apis/sources/<version>/*_validation.go, and check if the embedded SourceSpec is validated, if not then we add the same check that you added for the PingSource (line 97).

For example, for ApiServerSource, we don't validate SourceSpec [1], so we need to add the check errs = errs.Also(cs.SourceSpec.Validate(ctx)) here

eventing/pkg/apis/sources/v1/apiserver_validation.go

Line 75 in 407f3cb

Also, if we can add simple unit tests for each source to prevent regression that would be great too.

Hope this helps, thanks!

[1]

eventing/pkg/apis/sources/v1/apiserver_validation.go

Lines 39 to 77 in 407f3cb

func (cs *ApiServerSourceSpec) Validate(ctx context.Context) *apis.FieldError {
var errs *apis.FieldError
// Validate mode, if can be empty or set as certain value
switch cs.EventMode {
case ReferenceMode, ResourceMode:
// EventMode is valid.
default:
errs = errs.Also(apis.ErrInvalidValue(cs.EventMode, "mode"))
}
// Validate sink
errs = errs.Also(cs.Sink.Validate(ctx).ViaField("sink"))
if len(cs.Resources) == 0 {
errs = errs.Also(apis.ErrMissingField("resources"))
}
for i, res := range cs.Resources {
_, err := schema.ParseGroupVersion(res.APIVersion)
if err != nil {
errs = errs.Also(apis.ErrInvalidValue(res.APIVersion, "apiVersion").ViaFieldIndex("resources", i))
}
if strings.TrimSpace(res.Kind) == "" {
errs = errs.Also(apis.ErrMissingField("kind").ViaFieldIndex("resources", i))
}
}
if cs.ResourceOwner != nil {
_, err := schema.ParseGroupVersion(cs.ResourceOwner.APIVersion)
if err != nil {
errs = errs.Also(apis.ErrInvalidValue(cs.ResourceOwner.APIVersion, "apiVersion").ViaField("owner"))
}
if strings.TrimSpace(cs.ResourceOwner.Kind) == "" {
errs = errs.Also(apis.ErrMissingField("kind").ViaField("owner"))
}
}
return errs
}

return errs
}

Expand Down
38 changes: 34 additions & 4 deletions pkg/apis/sources/v1/ping_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ func TestPingSourceValidation(t *testing.T) {
return errs
}(),
}, {
name: "invalid DataBase64 is to big ",
name: "invalid DataBase64 is to big",
source: PingSource{
Spec: PingSourceSpec{
Schedule: "*/2 * * * *",
Expand All @@ -292,7 +292,7 @@ func TestPingSourceValidation(t *testing.T) {
return errs
}(),
}, {
name: "invalid Data is to big ",
name: "invalid Data is to big",
source: PingSource{
Spec: PingSourceSpec{
Schedule: "*/2 * * * *",
Expand Down Expand Up @@ -320,7 +320,7 @@ func TestPingSourceValidation(t *testing.T) {
return errs
}(),
}, {
name: "big data ok ",
name: "big data ok",
source: PingSource{

Spec: PingSourceSpec{
Expand All @@ -343,7 +343,7 @@ func TestPingSourceValidation(t *testing.T) {
},
want: nil,
}, {
name: "big data still ok ",
name: "big data still ok",
source: PingSource{

Spec: PingSourceSpec{
Expand Down Expand Up @@ -407,6 +407,36 @@ func TestPingSourceValidation(t *testing.T) {
errs = errs.Also(fe)
return errs
}(),
}, {
name: "invalid spec ceOverrides validation",
source: PingSource{
Spec: PingSourceSpec{
Schedule: "*/2 * * * *",
Timezone: "Europe/Paris",
SourceSpec: duckv1.SourceSpec{
CloudEventOverrides: &duckv1.CloudEventOverrides{
Extensions: map[string]string{"Invalid_type": "any value"},
},
Sink: duckv1.Destination{
Ref: &duckv1.KReference{
APIVersion: "v1",
Kind: "broker",
Name: "default",
},
},
},
},
},
want: func() *apis.FieldError {
var errs *apis.FieldError
fe := apis.ErrInvalidKeyName(
"Invalid_type",
"spec.ceOverrides.extensions",
"keys are expected to be alphanumeric",
)
errs = errs.Also(fe)
return errs
}(),
},
}

Expand Down
4 changes: 3 additions & 1 deletion pkg/apis/sources/v1/sinkbinding_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ func (fb *SinkBinding) Validate(ctx context.Context) *apis.FieldError {

// Validate implements apis.Validatable
func (fbs *SinkBindingSpec) Validate(ctx context.Context) *apis.FieldError {
return fbs.Subject.Validate(ctx).ViaField("subject").Also(
err := fbs.Subject.Validate(ctx).ViaField("subject").Also(
fbs.Sink.Validate(ctx).ViaField("sink"))
err = err.Also(fbs.SourceSpec.Validate(ctx))
return err
}
36 changes: 36 additions & 0 deletions pkg/apis/sources/v1/sinkbinding_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,42 @@ func TestSinkBindingValidation(t *testing.T) {
},
},
want: apis.ErrGeneric("expected at least one, got none", "spec.sink.ref", "spec.sink.uri"),
}, {
name: "invalid spec ceOverrides validation",
in: &SinkBinding{
ObjectMeta: metav1.ObjectMeta{
Name: "gabo",
Namespace: "test",
},
Spec: SinkBindingSpec{
BindingSpec: duckv1.BindingSpec{
Subject: tracker.Reference{
APIVersion: "apps/v1",
Kind: "Deployment",
Name: "jeanne",
Namespace: "test",
},
},
SourceSpec: duckv1.SourceSpec{
CloudEventOverrides: &duckv1.CloudEventOverrides{
Extensions: map[string]string{"Invalid_type": "any value"},
},
Sink: duckv1.Destination{
Ref: &duckv1.KReference{
APIVersion: "serving.knative.dev/v1",
Kind: "Service",
Name: "gemma",
Namespace: "test",
},
},
},
},
},
want: apis.ErrInvalidKeyName(
"Invalid_type",
"spec.ceOverrides.extensions",
"keys are expected to be alphanumeric",
),
}}

for _, test := range tests {
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/sources/v1beta2/ping_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ func (cs *PingSourceSpec) Validate(ctx context.Context) *apis.FieldError {
}
}
}
errs = errs.Also(cs.SourceSpec.Validate(ctx))
return errs
}

Expand Down
Loading