Validating HTTPRouteFilter type consistency #946

crmejia · 2021-11-23T03:11:18Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR adds validates that a HTTPRouterFilter Type matches it's value.

Which issue(s) this PR fixes:
Fixes #942

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2021-11-23T03:11:26Z

Hi @crmejia. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

crmejia · 2021-11-23T03:12:57Z

Hey @jpeach, this PR looks to address #942

jpeach · 2021-11-23T04:46:28Z

/ok-to-test

crmejia · 2021-11-24T02:12:54Z

/retest-required

Makefile

tokers · 2021-11-25T01:07:31Z

apis/v1alpha2/validation/httproute.go

+
+	switch filter.Type {
+	case gatewayv1a2.HTTPRouteFilterExtensionRef:
+		if filter.ExtensionRef == nil || filter.RequestHeaderModifier != nil || filter.RequestMirror != nil || filter.RequestRedirect != nil {


These if logics might not so feasible, if a new filter type is introduced, we have to change a few places, and it's easy to forget.

I see what you mean. It's hard to mantain. I've researched alternatives:

I started by looking at ways to implement discriminated unions in Go and they usually point to using interfaces(see Alternatives to sum types in Go, StackOverflow). Correct me If I'm wrong but I don't think it's right to add interfaces to the API just to solve this problem.

Something that occurred to me is to compare the json tags with the type but I think is "hacky".

Do you have any ideas or tips for the correct implementation? I have a hunch that there is a more elegant way to solve this and I just didn't look in the right place.

If we can relax the constraints, say unused filter config can still be reserved there, than the switch statement can be simple.

The best I could come up with was this:

func validateUnionField(discriminator string, name string, value interface{}) { var isNil bool switch value := value.(type) { case *gatewayv1a2.HTTPRequestHeaderFilter: isNil = (value == nil) case *gatewayv1a2.HTTPRequestMirrorFilter: isNil = (value == nil) case *gatewayv1a2.HTTPRequestRedirectFilter: isNil = (value == nil) case *gatewayv1a2.LocalObjectReference: isNil = (value == nil) default: // Puke an error so someone updates this code ... } if discriminator == name { if isNil { // Filter spec is not set, but it should be. } } else { if !isNil { // Filter spec is set, but it should not be. } } } validateUnionField(filter.Type, gatewayv1a2.HTTPRouteFilterExtensionRef, filter.ExtensionRef) validateUnionField(filter.Type, gatewayv1a2.HTTPRouteFilterRequestHeaderModifier, filter.RequestHeaderModifier)

I've relaxed the constraints to make the switch statement simpler. Also, I added a default case to make sure new types are added to the validation.

I think that func validateUnionField is not simpler and still has the issue of referencing the types so new types need to be added.

robscott

Thanks for the work on this @crmejia!

apis/v1alpha2/validation/httproute.go

apis/v1alpha2/validation/httproute_test.go

apis/v1alpha2/validation/httproute.go

crmejia · 2021-12-07T01:17:27Z

/retest

apis/v1alpha2/validation/httproute.go

robscott · 2021-12-10T22:15:59Z

/retest

robscott

Thanks for your work on this @crmejia!

robscott · 2021-12-10T22:10:24Z

apis/v1alpha2/validation/httproute.go

+		errs = append(errs, validateHTTPRouteUniqueFilters(rule.Filters, path.Child("rules").Index(i))...)
+		errs = append(errs, validateHTTPRouteFilterTypeMatchesValue(rule.Filters, path.Child("rules").Index(i))...)


This is already a great simplification, but can you take it one step further and just have a generic validateHTTPRouteFilters func instead that covers both of these? Would save us from looping through the same list of filters 2x.

I kept them separate to have a clear boundary for each validation but I can see the inefficiency. I'll simplify 👍🏾

apis/v1alpha2/validation/httproute.go

robscott · 2021-12-10T22:25:57Z

apis/v1alpha2/validation/httproute_test.go

+			name: "valid HTTPRouteFilterRequestHeaderModifier type filter with matching field",
+			routeFilter: []gatewayv1a2.HTTPRouteFilter{{
+				Type:                  gatewayv1a2.HTTPRouteFilterRequestHeaderModifier,
+				RequestHeaderModifier: &gatewayv1a2.HTTPRequestHeaderFilter{},


I wonder if we need to also make sure that the value of these fields is non empty? Even a simple check like filter.RequestRedirect == nil || (gatewayv1a2.HTTPRequestHeaderFilter{}) == *filter.RequestRedirect) might be helpful to ensure completely empty structs were also invalid.

Done. I had to use reflect.DeepEqual in some cases.

…onsistency

crmejia · 2021-12-12T14:34:56Z

/retest

apis/v1alpha2/validation/httproute.go

jpeach · 2021-12-12T22:11:35Z

Once the deep equal issue is done, this LGTM, pending Rob's concurrence.

/lgtm
/hold

apis/v1alpha2/validation/httproute.go

crmejia · 2021-12-16T02:23:43Z

After some discussion with Rob regarding validation we decided to check for nil instead of empty and to check these two cases for each filter:

if filter != nil && filter.Type != filter return some error
if filter == nil && filter.Type == filter return some other error
Please let me know what y'all think.

robscott

Thanks @crmejia! Just a couple tiny nits but otherwise LGTM.

apis/v1alpha2/validation/httproute.go

robscott · 2021-12-16T03:53:20Z

apis/v1alpha2/validation/httproute.go

+	if filter.Type == "" {
+		errs = append(errs, field.Required(path, "filter.Type cannot be empty"))
+	}


This is actually already covered by CRD validation, no need to also have it here: https://github.com/kubernetes-sigs/gateway-api/blob/master/apis/v1alpha2/httproute_types.go#L497-L498

robscott · 2021-12-17T05:23:30Z

Thanks @crmejia! Will leave final LGTM for someone else, but this looks great.

/approve

jpeach

/lgtm

\o/

k8s-ci-robot · 2021-12-20T03:30:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crmejia, jpeach, robscott

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jpeach,robscott]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jpeach · 2021-12-20T03:34:01Z

/remove-hold

* Validating HTTPRouteFilter type consistency * fixing Makefile regression * relaxing validation constraints to simplify switch statement * adding default type validation * refactoring validateHTTPRouteSpec * fixing paths * adding HTTPRouteFilter URLRewrite validation * validating route filters are not empty * simplying validation with validateHTTPRouteFilters * removing reflect.DeepEqual as is not necessary * validate route filters are not nil * small fixes * fixing empty filter test

Validating HTTPRouteFilter type consistency

99f76ab

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 23, 2021

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 23, 2021

k8s-ci-robot requested review from thockin and youngnick November 23, 2021 03:11

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 23, 2021

resolved merge conflict

b156924

jpeach reviewed Nov 24, 2021

View reviewed changes

Makefile Outdated Show resolved Hide resolved

fixing Makefile regression

0a7e6fc

tokers reviewed Nov 25, 2021

View reviewed changes

crmejia added 2 commits November 29, 2021 20:03

relaxing validation constraints to simplify switch statement

e59511b

adding default type validation

f0e3afa

robscott reviewed Dec 3, 2021

View reviewed changes

apis/v1alpha2/validation/httproute.go Outdated Show resolved Hide resolved

apis/v1alpha2/validation/httproute.go Outdated Show resolved Hide resolved

apis/v1alpha2/validation/httproute_test.go Outdated Show resolved Hide resolved

jpeach reviewed Dec 4, 2021

View reviewed changes

apis/v1alpha2/validation/httproute.go Outdated Show resolved Hide resolved

jpeach reviewed Dec 4, 2021

View reviewed changes

apis/v1alpha2/validation/httproute.go Outdated Show resolved Hide resolved

crmejia added 2 commits December 6, 2021 15:53

refactoring validateHTTPRouteSpec

db792d1

fixing paths

979ae2e

crmejia commented Dec 7, 2021

View reviewed changes

apis/v1alpha2/validation/httproute.go Outdated Show resolved Hide resolved

robscott reviewed Dec 10, 2021

View reviewed changes

crmejia added 2 commits December 10, 2021 18:12

Merge branch 'kubernetes-sigs:master' into validate_HTTPRouteFilter_c…

8394575

…onsistency

adding HTTPRouteFilter URLRewrite validation

d88e253

crmejia added 2 commits December 12, 2021 07:59

validating route filters are not empty

8dae8bd

simplying validation with validateHTTPRouteFilters

3d1d8e0

jpeach reviewed Dec 12, 2021

View reviewed changes

apis/v1alpha2/validation/httproute.go Outdated Show resolved Hide resolved

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 12, 2021

k8s-ci-robot assigned jpeach Dec 12, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 12, 2021

removing reflect.DeepEqual as is not necessary

0f76f94

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 14, 2021

jpeach reviewed Dec 15, 2021

View reviewed changes

apis/v1alpha2/validation/httproute.go Outdated Show resolved Hide resolved

jpeach reviewed Dec 15, 2021

View reviewed changes

apis/v1alpha2/validation/httproute.go Outdated Show resolved Hide resolved

jpeach reviewed Dec 15, 2021

View reviewed changes

apis/v1alpha2/validation/httproute.go Outdated Show resolved Hide resolved

validate route filters are not nil

2a5ecf5

robscott reviewed Dec 16, 2021

View reviewed changes

crmejia added 2 commits December 16, 2021 12:30

small fixes

68c9242

fixing empty filter test

3f87837

robscott added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Dec 17, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 17, 2021

jpeach approved these changes Dec 20, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 20, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 20, 2021

k8s-ci-robot merged commit ceaec64 into kubernetes-sigs:master Dec 20, 2021

crmejia deleted the validate_HTTPRouteFilter_consistency branch December 20, 2021 22:21

robscott mentioned this pull request Mar 24, 2022

Cherry pick webhook improvements and bug fixes into release-0.4 #1071

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validating HTTPRouteFilter type consistency #946

Validating HTTPRouteFilter type consistency #946

crmejia commented Nov 23, 2021

k8s-ci-robot commented Nov 23, 2021

crmejia commented Nov 23, 2021

jpeach commented Nov 23, 2021

crmejia commented Nov 24, 2021

tokers Nov 25, 2021

crmejia Nov 26, 2021

tokers Nov 26, 2021

jpeach Nov 29, 2021

crmejia Nov 30, 2021 •

edited

Loading

robscott left a comment

crmejia commented Dec 7, 2021

robscott commented Dec 10, 2021

robscott left a comment

robscott Dec 10, 2021

crmejia Dec 11, 2021

robscott Dec 10, 2021

crmejia Dec 12, 2021

crmejia commented Dec 12, 2021

jpeach commented Dec 12, 2021

crmejia commented Dec 16, 2021

robscott left a comment

robscott Dec 16, 2021

robscott commented Dec 17, 2021

jpeach left a comment

k8s-ci-robot commented Dec 20, 2021

jpeach commented Dec 20, 2021

		errs = append(errs, validateHTTPRouteUniqueFilters(rule.Filters, path.Child("rules").Index(i))...)
		errs = append(errs, validateHTTPRouteFilterTypeMatchesValue(rule.Filters, path.Child("rules").Index(i))...)

Validating HTTPRouteFilter type consistency #946

Validating HTTPRouteFilter type consistency #946

Conversation

crmejia commented Nov 23, 2021

k8s-ci-robot commented Nov 23, 2021

crmejia commented Nov 23, 2021

jpeach commented Nov 23, 2021

crmejia commented Nov 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crmejia Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

robscott left a comment

Choose a reason for hiding this comment

crmejia commented Dec 7, 2021

robscott commented Dec 10, 2021

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crmejia commented Dec 12, 2021

jpeach commented Dec 12, 2021

crmejia commented Dec 16, 2021

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott commented Dec 17, 2021

jpeach left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 20, 2021

jpeach commented Dec 20, 2021

crmejia Nov 30, 2021 •

edited

Loading