Add shared type `HeaderName` and use it for `GRPCHeaderName`, `HTTPHeaderName` and `QueryParamMatchName` #1796

gyohuangxin · 2023-03-09T01:55:28Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Add pattern for query param name in HTTPQueryParamMatch like other items' names to avoid illegal names.

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?:

Yes, illegal names like " " will be not allowed for query param name in HTTPQueryParamMatch.

gyohuangxin · 2023-03-09T02:05:48Z

@shaneutt Can you review it? Thanks.

shaneutt · 2023-03-10T15:23:06Z

apis/v1beta1/httproute_types.go

@@ -452,6 +452,7 @@ type HTTPQueryParamMatch struct {
 	//
 	// +kubebuilder:validation:MinLength=1
 	// +kubebuilder:validation:MaxLength=256
+	// +kubebuilder:validation:Pattern=`^[A-Za-z0-9!#$%&'*+\-.^_\x60|~]+$`


Was this pattern specifically pulled from a reference somewhere?

Yes, I pulled this pattern from other names properties likes:
https://github.com/kubernetes-sigs/gateway-api/blob/main/apis/v1alpha2/httproute_types.go#L117
https://github.com/kubernetes-sigs/gateway-api/blob/main/apis/v1beta1/httproute_types.go#L350
https://github.com/kubernetes-sigs/gateway-api/blob/main/apis/v1alpha2/grpcroute_types.go#L414

Can we use HTTPHeaderName as the type here instead of copying the regex? Would make this easier to maintain going forward.

robscott

Thanks @gyohuangxin! One tiny nit but otherwise LGTM.

robscott · 2023-03-15T17:58:08Z

apis/v1beta1/httproute_types.go

@@ -452,6 +452,7 @@ type HTTPQueryParamMatch struct {
 	//
 	// +kubebuilder:validation:MinLength=1
 	// +kubebuilder:validation:MaxLength=256
+	// +kubebuilder:validation:Pattern=`^[A-Za-z0-9!#$%&'*+\-.^_\x60|~]+$`


Can we use HTTPHeaderName as the type here instead of copying the regex? Would make this easier to maintain going forward.

sunjayBhatia

generally seems right, but agree with @robscott's idea to not have to repeat the same regex pattern many places, i don't know if adding another layer of type aliasing would be what we want to do/even work as we intend but that might be an idea to explore (e.g GRPCHeaderName, HTTPHeaderName and query param match name all use the same underlying more neutrally-named type)

gyohuangxin · 2023-03-16T09:12:07Z

Thanks for your comments. Agreed with @sunjayBhatia , the better solution is to introduce a shared type like HeaderName in apis/v1beta1/shared_types.go then any name properties (e.g GRPCHeaderName, HTTPHeaderName and QueryParamMatchName) can use them, so that we can reduce regular expression maintenance.
@robscott @shaneutt What do you think?

shaneutt · 2023-03-16T13:07:03Z

Thanks for your comments. Agreed with @sunjayBhatia , the better solution is to introduce a shared type like HeaderName in apis/v1beta1/shared_types.go then any name properties (e.g GRPCHeaderName, HTTPHeaderName and QueryParamMatchName) can use them, so that we can reduce regular expression maintenance. @robscott @shaneutt What do you think?

Yep sgtm, setting to draft to sort that out so this doesn't accidentally get merged 👍

Signed-off-by: Huang Xin <xin1.huang@intel.com>

…ame and QueryParamMatchName Signed-off-by: Xin Huang <xin1.huang@intel.com>

apis/v1beta1/shared_types.go

Signed-off-by: Huang Xin <xin1.huang@intel.com>

gyohuangxin · 2023-03-23T01:43:29Z

@robscott Could you review this? Thanks

shaneutt

Rob approved previously just with the caveat of the common type, we're all set thank you @gyohuangxin!!! 🥳

/lgtm

k8s-ci-robot · 2023-03-23T14:00:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gyohuangxin, shaneutt, sunjayBhatia

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~~ [shaneutt]

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

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 9, 2023

k8s-ci-robot requested review from bowei and robscott March 9, 2023 01:55

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 9, 2023

shaneutt self-requested a review March 9, 2023 02:16

shaneutt self-assigned this Mar 9, 2023

shaneutt added this to the v0.7.0 milestone Mar 9, 2023

shaneutt reviewed Mar 10, 2023

View reviewed changes

shaneutt approved these changes Mar 13, 2023

View reviewed changes

shaneutt requested a review from youngnick March 13, 2023 14:35

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 13, 2023

shaneutt requested review from mlavacca, skriss and sunjayBhatia March 13, 2023 14:36

robscott reviewed Mar 15, 2023

View reviewed changes

sunjayBhatia approved these changes Mar 15, 2023

View reviewed changes

shaneutt marked this pull request as draft March 16, 2023 13:07

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 16, 2023

gyohuangxin added 2 commits March 18, 2023 20:38

Add pattern for query param name in HTTPQueryParamMatch

160caf7

Signed-off-by: Huang Xin <xin1.huang@intel.com>

Add shared type HeaderName and use it for GRPCHeaderName, HTTPHeaderN…

d1202af

…ame and QueryParamMatchName Signed-off-by: Xin Huang <xin1.huang@intel.com>

gyohuangxin force-pushed the HTTPQueryParamMatch_validation branch from abafab3 to d1202af Compare March 18, 2023 14:00

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 18, 2023

gyohuangxin changed the title ~~Add pattern for query param name in HTTPQueryParamMatch~~ Add shared type HeaderName and use it for GRPCHeaderName, HTTPHeaderName and QueryParamMatchName Mar 20, 2023

gyohuangxin marked this pull request as ready for review March 20, 2023 06:31

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 20, 2023

k8s-ci-robot requested a review from shaneutt March 20, 2023 06:31

shaneutt requested changes Mar 20, 2023

View reviewed changes

apis/v1beta1/shared_types.go Outdated Show resolved Hide resolved

shaneutt requested a review from robscott March 20, 2023 12:37

k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 20, 2023

Fix a typo.

f655587

Signed-off-by: Huang Xin <xin1.huang@intel.com>

shaneutt approved these changes Mar 20, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 20, 2023

shaneutt approved these changes Mar 23, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 23, 2023

k8s-ci-robot merged commit 54a7bc4 into kubernetes-sigs:main Mar 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add shared type `HeaderName` and use it for `GRPCHeaderName`, `HTTPHeaderName` and `QueryParamMatchName` #1796

Add shared type `HeaderName` and use it for `GRPCHeaderName`, `HTTPHeaderName` and `QueryParamMatchName` #1796

gyohuangxin commented Mar 9, 2023 •

edited

Loading

gyohuangxin commented Mar 9, 2023

shaneutt Mar 10, 2023

gyohuangxin Mar 12, 2023

robscott Mar 15, 2023

robscott left a comment

robscott Mar 15, 2023

sunjayBhatia left a comment

gyohuangxin commented Mar 16, 2023

shaneutt commented Mar 16, 2023 •

edited

Loading

gyohuangxin commented Mar 23, 2023

shaneutt left a comment

k8s-ci-robot commented Mar 23, 2023

Add shared type HeaderName and use it for GRPCHeaderName, HTTPHeaderName and QueryParamMatchName #1796

Add shared type HeaderName and use it for GRPCHeaderName, HTTPHeaderName and QueryParamMatchName #1796

Conversation

gyohuangxin commented Mar 9, 2023 • edited Loading

gyohuangxin commented Mar 9, 2023

shaneutt Mar 10, 2023

Choose a reason for hiding this comment

gyohuangxin Mar 12, 2023

Choose a reason for hiding this comment

robscott Mar 15, 2023

Choose a reason for hiding this comment

robscott left a comment

Choose a reason for hiding this comment

robscott Mar 15, 2023

Choose a reason for hiding this comment

sunjayBhatia left a comment

Choose a reason for hiding this comment

gyohuangxin commented Mar 16, 2023

shaneutt commented Mar 16, 2023 • edited Loading

gyohuangxin commented Mar 23, 2023

shaneutt left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 23, 2023

Add shared type `HeaderName` and use it for `GRPCHeaderName`, `HTTPHeaderName` and `QueryParamMatchName` #1796

Add shared type `HeaderName` and use it for `GRPCHeaderName`, `HTTPHeaderName` and `QueryParamMatchName` #1796

gyohuangxin commented Mar 9, 2023 •

edited

Loading

shaneutt commented Mar 16, 2023 •

edited

Loading