Webhook: validate the combination of port, protocol, and hostname are unique for each listener. #1457

gyohuangxin · 2022-10-14T05:18:26Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add validation to validate the combination of port, protocol, and hostname are unique for each listener.

Which issue(s) this PR fixes:

Fixes #847

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2022-10-14T05:18:34Z

Hi @gyohuangxin. 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.

gyohuangxin · 2022-10-14T05:33:23Z

@robscott I make it a draft PR because I have some question:

As you mentioned in Webhook: Validate unique port, protocol, and hostname per listener #847:

The validating webhook should ensure that port, protocol, and hostname are unique among Gateway listeners

but then you give an example:

"2 listeners for foo.com would be valid if they were on different ports or protocols, but not if port and protocol were also identical."

They are a bit inconsistent, but my understanding is the webhook should separately validate unique port, protocol per hostname for gateway listeners. Is it correct? Is it needed to verify unique port, protocol across hostnames? Is it needed to verify unique hostname per listener?

I add validations in v1beta1 apis, should I add them in v1alpha1 apis too?

shaneutt · 2023-01-31T14:38:01Z

@gyohuangxin where are we at with this one? Is there anything we can do to help the progression here? Looks like maybe we're still waiting on some feedback requested from @robscott?

gyohuangxin · 2023-01-31T15:44:25Z

Yes, it's on hold because I'm still confused with the first question, I'm not sure my thought is close to the reality.

@robscott I make it a draft PR because I have some question:

As you mentioned in Webhook: Validate unique port, protocol, and hostname per listener #847:

The validating webhook should ensure that port, protocol, and hostname are unique among Gateway listeners

but then you give an example:

"2 listeners for foo.com would be valid if they were on different ports or protocols, but not if port and protocol were also identical."

They are a bit inconsistent, but my understanding is the webhook should separately validate unique port, protocol per hostname for gateway listeners. Is it correct? Is it needed to verify unique port, protocol across hostnames? Is it needed to verify unique hostname per listener?

robscott · 2023-03-23T22:47:46Z

apis/v1beta1/validation/gateway.go

+	hostnameProtocolMap := make(map[gatewayv1b1.Hostname][]gatewayv1b1.ProtocolType)
+	hostnamePortMap := make(map[gatewayv1b1.Hostname][]gatewayv1b1.PortNumber)


Hey @gyohuangxin, I think there's a simpler way to do this. All that we really want to do is ensure that the combination of port, protocol, and name are unique. So for example, the following would be valid:

- port: 80 hostname: foo.com protocol: HTTP - port: 443 hostname: foo.com protocol: HTTP

But the following would not be valid:

- port: 80 hostname: foo.com protocol: HTTP - port: 80 hostname: foo.com protocol: HTTP

I think what you have is slightly different than that, requiring that for the same hostname (foo.com), both port and protocol must be different for each listener. The spec as it's written today only requires that one of them is different.

A simple way to accomplish this would be to use something like fmt.Sprintf("%s:%s:%d", protocol, hostname, port) for each listener and then use k8s sets util to see if the value for that listener duplicated any listeners you'd already looked at within the same Gateway. Hopefully that makes sense, sorry for the delay on this!

@robscott Thanks for your clear comments, don't worry about the delay. I also like the simple implementation, thanks!

should this webhook attempt to reject one of these tuples (80, http, *.com) and (80, http, *.foo.com) :) ?

@arkodg I think it will very hard to accomplish. How would you decide if regexA (*.com in this case) is overlapping with regexB(*foo.com).

yeah its hard, but I wanted to highlight the case.
Since the API calls out any regex match to be implementation specific, even adding language in the API for this might be tricky
cc @robscott

robscott · 2023-03-23T22:52:05Z

Thanks for the work on this @gyohuangxin! Sorry I lost track of this, don't hesitate to ping me if I can help explain anything better.

hzxuzhonghu · 2023-03-24T01:53:07Z

apis/v1beta1/util/validation/gateway.go

@@ -0,0 +1,41 @@
+/*
+Copyright 2022 The Kubernetes Authors.


hzxuzhonghu · 2023-03-24T01:57:23Z

apis/v1beta1/util/validation/gateway.go

+
+// ContainsInPortSlice checks whether the provided Port
+// is in the target Port slice.
+func ContainsInPortSlice(items []gatewayv1b1.PortNumber, item *gatewayv1b1.PortNumber) bool {


pass value type of PortNumber

hzxuzhonghu · 2023-03-24T01:57:55Z

apis/v1beta1/util/validation/gateway.go

+
+// ContainsInProtocolSlice checks whether the provided Protocol
+// is in the target Protocol slice.
+func ContainsInProtocolSlice(items []gatewayv1b1.ProtocolType, item *gatewayv1b1.ProtocolType) bool {


value type instead of pointer

hzxuzhonghu · 2023-03-24T01:59:29Z

apis/v1beta1/validation/gateway.go

+		}
+		if listener.Port != 0 {
+			if portSlice, ok := hostnamePortMap[targetHostname]; ok {
+				if utils.ContainsInPortSlice(portSlice, &listener.Port) {


can use Slices.Contains from golang.org/x/exp/slices

…port, protocol, and name are unique for each listener. Signed-off-by: Huang Xin <xin1.huang@intel.com>

gyohuangxin · 2023-04-01T18:13:12Z

@robscott @shaneutt Can you review this PR? Thanks

apis/v1beta1/validation/gateway.go

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

LiorLieberman · 2023-04-04T08:06:50Z

Thanks @gyohuangxin ! This Looks good to me.
We probably want to change the PR description to match the new logic - ensuring Listener's port-protocol-hostname combinations are unique within an individual Gateway.

over to @robscott for a final review and approval.

gyohuangxin · 2023-04-04T08:34:31Z

Thanks @gyohuangxin ! This Looks good to me. We probably want to change the PR description to match the new logic - ensuring Listener's port-protocol-hostname combinations are unique within an individual Gateway.

over to @robscott for a final review and approval.

Sorry for missing the PR description, updated. Thanks for reminding 💯

arkodg

LGTM thanks !

dprotaso · 2023-04-04T19:52:51Z

Based on this comment it seems like we should take into consideration the tls properties attached to a listener when determing uniqueness

#1842 (comment)

gyohuangxin · 2023-04-05T13:49:59Z

Thanks for your comments @dprotaso . As said in @robscott 's #1842 (comment), the "Hostname" can be different for shared Port and Protocol, like the TLS example on the website: https://gateway-api.sigs.k8s.io/guides/tls/#wildcard-tls-listeners

So, I think it can cover the use cases. Please let me know if there are some use cases that use the same combination of port, protocol and hostname but use distinct tls fields.

robscott

Thanks @gyohuangxin! Would like a few more test cases but otherwise LGTM.

robscott · 2023-04-05T18:58:21Z

apis/v1beta1/validation/gateway_test.go

+			mutate: func(gw *gatewayv1b1.Gateway) {
+				hostnameFoo := gatewayv1b1.Hostname("foo.com")
+				gw.Spec.Listeners[0].Name = "foo"
+				gw.Spec.Listeners[0].Hostname = &hostnameFoo


Since hostname is optional can you add one more test case that shows that the same setup fails when both listeners have separate hostnames?

Similarly, it would be helpful to have additional test cases where everything was the same except for one of the fields (port, protocol, or hostname) to ensure those are all still valid.

Since hostname is optional can you add one more test case that shows that the same setup fails when both listeners have separate hostnames?

Do you mean to test the combination of port and protocol are unique when hostnames not set? If yes, I've add one: d75ab56#diff-e3c0a139e14037364db162e955588e8153e795f4cbe84b6fce32149d62ca2a73R170-R184

Similarly, it would be helpful to have additional test cases where everything was the same except for one of the fields (port, protocol, or hostname) to ensure those are all still valid.

Updated, thank you!

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

robscott

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

apis/v1beta1/validation/gateway_test.go

apis/v1beta1/validation/gateway.go

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

robscott · 2023-04-06T18:11:54Z

Thanks @gyohuangxin!

/approve
/ok-to-test

robscott · 2023-04-06T18:32:51Z

/lgtm
/approve

k8s-ci-robot · 2023-04-06T18:32:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arkodg, gyohuangxin, 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~~ [robscott]

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 do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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 Oct 14, 2022

k8s-ci-robot requested review from howardjohn and mikemorris October 14, 2022 05:18

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 14, 2022

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 14, 2022

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 20, 2022

gyohuangxin mentioned this pull request Nov 4, 2022

REQUEST: New membership for gyohuangxin kubernetes/org#3817

Closed

9 tasks

gyohuangxin force-pushed the unique_port_webhook branch from bb83aa7 to 91fe519 Compare November 15, 2022 08:59

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 15, 2022

shaneutt requested a review from robscott January 31, 2023 14:38

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

robscott reviewed Mar 23, 2023

View reviewed changes

hzxuzhonghu reviewed Mar 24, 2023

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 24, 2023

gyohuangxin force-pushed the unique_port_webhook branch from 91fe519 to 25dc9ca Compare March 28, 2023 01:46

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 28, 2023

Add validateHostnameProtocolPort to validate that the combination of …

a7b6059

…port, protocol, and name are unique for each listener. Signed-off-by: Huang Xin <xin1.huang@intel.com>

gyohuangxin force-pushed the unique_port_webhook branch from 25dc9ca to a7b6059 Compare March 29, 2023 17:16

LiorLieberman reviewed Apr 3, 2023

View reviewed changes

apis/v1beta1/validation/gateway.go Outdated Show resolved Hide resolved

Skip Insert if the set has the combination already

35c7329

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

arkodg approved these changes Apr 4, 2023

View reviewed changes

gyohuangxin changed the title ~~Webhook: validate the combination of port, protocol, and name are unique for each listener.~~ Webhook: validate the combination of port, protocol, and hostname are unique for each listener. Apr 5, 2023

robscott reviewed Apr 5, 2023

View reviewed changes

Add more unit tests.

d75ab56

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

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 6, 2023

robscott reviewed Apr 6, 2023

View reviewed changes

apis/v1beta1/validation/gateway_test.go Outdated Show resolved Hide resolved

apis/v1beta1/validation/gateway.go Outdated Show resolved Hide resolved

Fix nits.

820ec48

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

robscott added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 6, 2023

k8s-ci-robot assigned robscott Apr 6, 2023

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

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

k8s-ci-robot merged commit bb88d58 into kubernetes-sigs:main Apr 6, 2023

shaneutt mentioned this pull request Jul 24, 2023

shaneutt/1822 shaneutt/gateway-api#3

Closed

shaneutt mentioned this pull request Aug 7, 2023

update owner aliases shaneutt/gateway-api#4

Closed

shaneutt mentioned this pull request Sep 18, 2023

shaneutt/conformance tests for gateway addresses shaneutt/gateway-api#5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Webhook: validate the combination of port, protocol, and hostname are unique for each listener. #1457

Webhook: validate the combination of port, protocol, and hostname are unique for each listener. #1457

gyohuangxin commented Oct 14, 2022 •

edited

Loading

k8s-ci-robot commented Oct 14, 2022

gyohuangxin commented Oct 14, 2022

shaneutt commented Jan 31, 2023

gyohuangxin commented Jan 31, 2023

robscott Mar 23, 2023

gyohuangxin Mar 24, 2023

arkodg Mar 29, 2023

LiorLieberman Apr 4, 2023

arkodg Apr 4, 2023

robscott commented Mar 23, 2023

hzxuzhonghu Mar 24, 2023

hzxuzhonghu Mar 24, 2023

hzxuzhonghu Mar 24, 2023

hzxuzhonghu Mar 24, 2023

gyohuangxin commented Apr 1, 2023

LiorLieberman commented Apr 4, 2023

gyohuangxin commented Apr 4, 2023

arkodg left a comment

dprotaso commented Apr 4, 2023 •

edited

Loading

gyohuangxin commented Apr 5, 2023

robscott left a comment

robscott Apr 5, 2023

robscott Apr 5, 2023

gyohuangxin Apr 6, 2023

robscott left a comment

robscott commented Apr 6, 2023

robscott commented Apr 6, 2023

k8s-ci-robot commented Apr 6, 2023

		hostnameProtocolMap := make(map[gatewayv1b1.Hostname][]gatewayv1b1.ProtocolType)
		hostnamePortMap := make(map[gatewayv1b1.Hostname][]gatewayv1b1.PortNumber)

Webhook: validate the combination of port, protocol, and hostname are unique for each listener. #1457

Webhook: validate the combination of port, protocol, and hostname are unique for each listener. #1457

Conversation

gyohuangxin commented Oct 14, 2022 • edited Loading

k8s-ci-robot commented Oct 14, 2022

gyohuangxin commented Oct 14, 2022

shaneutt commented Jan 31, 2023

gyohuangxin commented Jan 31, 2023

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

robscott commented Mar 23, 2023

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

gyohuangxin commented Apr 1, 2023

LiorLieberman commented Apr 4, 2023

gyohuangxin commented Apr 4, 2023

arkodg left a comment

Choose a reason for hiding this comment

dprotaso commented Apr 4, 2023 • edited Loading

gyohuangxin commented Apr 5, 2023

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

robscott left a comment

Choose a reason for hiding this comment

robscott commented Apr 6, 2023

robscott commented Apr 6, 2023

k8s-ci-robot commented Apr 6, 2023

gyohuangxin commented Oct 14, 2022 •

edited

Loading

dprotaso commented Apr 4, 2023 •

edited

Loading