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

Jump to bottom

conformance: missmatch SectionName and Port will trigger NoMatchingParent #2582

Merged
merged 1 commit into from
Nov 16, 2023
Merged

conformance: missmatch SectionName and Port will trigger NoMatchingParent #2582

merged 1 commit into from
Nov 16, 2023

Conversation

tao12345666333
Copy link
Member

@tao12345666333 tao12345666333 commented Nov 13, 2023 •
edited
Loading

What type of PR is this?

/kind test
/area conformance
What this PR does / why we need it:

According to the description, when both Port and SectionName are specified, the name and port of the selected listener
must match both specified values.

https://github.com/kubernetes-sigs/gateway-api/blob/e2f02aa0fc9dfa27be271397801f52f4e0a78acc/apis/v1/shared_types.go#L124C1-L130C38

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added kind/test do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. area/conformance labels Nov 13, 2023
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 13, 2023
mlavacca
mlavacca reviewed Nov 13, 2023
View reviewed changes
Copy link
Member

@mlavacca mlavacca left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @tao12345666333! The test looks good to me, I left a couple of comments below.

conformance/tests/httproute-invalid-parentref-section-name-not-matching-port.go Outdated Show resolved Hide resolved
conformance/tests/httproute-invalid-parentref-section-name-not-matching-port.go Show resolved Hide resolved
@mlavacca
Copy link
Member

/retest

@mlavacca
Copy link
Member

Also, the release note is missing here. Since this is a conformance test, NONE is good.

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 14, 2023
@tao12345666333 @mlavacca
conformance: missmatch SectionName and Port will trigger NoMatchingPa…
c56677f 
…rent

Signed-off-by: Jintao Zhang <zhangjintao9020@gmail.com>
Co-authored-by: Mattia Lavacca <lavacca.mattia@gmail.com>
@tao12345666333 tao12345666333 force-pushed the cmt-section-name-not-match-port-value branch from 297ee7e to c56677f Compare November 14, 2023 05:35
mlavacca
mlavacca reviewed Nov 14, 2023
View reviewed changes
Copy link
Member

@mlavacca mlavacca left a comment

Choose a reason for hiding this comment

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

Apart from a very picky little thing, it looks good to me.

conformance/tests/httproute-invalid-parentref-section-name-not-matching-port.go Show resolved Hide resolved
conformance/tests/httproute-invalid-parentref-section-name-not-matching-port.yaml Show resolved Hide resolved
conformance/tests/httproute-invalid-parentref-section-name-not-matching-port.go Show resolved Hide resolved
@czeslavo czeslavo mentioned this pull request Nov 14, 2023
Closed
1 task
mlavacca
mlavacca approved these changes Nov 14, 2023
View reviewed changes
Copy link
Member

@mlavacca mlavacca left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 14, 2023
@mlavacca
Copy link
Member

mlavacca commented Nov 16, 2023 •
edited
Loading

will defer to @arkodg, @sunjayBhatia for the final lgtm.

@arkodg
Copy link
Contributor

arkodg commented Nov 16, 2023 •
edited
Loading

this looks good @tao12345666333, have you tried this test out on any implementation ?

@tao12345666333
Copy link
Member Author

Thanks for your reply.

Yes, I have tried it in Kong Ingress locally.

Once this PR be merged, the KIC will enable this conformance test, and unblock Kong/kubernetes-ingress-controller#5147

@arkodg
Copy link
Contributor

arkodg commented Nov 16, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mlavacca, sunjayBhatia, tao12345666333

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:

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

sunjayBhatia
sunjayBhatia reviewed Nov 16, 2023
View reviewed changes
Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

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

fails with contour since the gateway name has a really long name and that causes a label we add to some resources to be > 63 chars

selfishly it would be great to have a shorter gateway name here but otherwise the logic of the test looks good

@sunjayBhatia
Copy link
Member

test passes with contour when i shorten the gateway name

@k8s-ci-robot k8s-ci-robot merged commit a919f8b into kubernetes-sigs:main Nov 16, 2023
8 checks passed
@tao12345666333 tao12345666333 deleted the cmt-section-name-not-match-port-value branch November 17, 2023 00:35
@tao12345666333
Copy link
Member Author

Thanks @sunjayBhatia

Good point. Let me shorten the name, prevent it from destroying other projects

@arkodg
Copy link
Contributor

arkodg commented Nov 17, 2023

Thanks @sunjayBhatia

Good point. Let me shorten the name, prevent it from destroying other projects

sounds like a feature, not a bug

@tao12345666333 tao12345666333 mentioned this pull request Nov 17, 2023
Closed
@sunjayBhatia sunjayBhatia mentioned this pull request Nov 17, 2023
Closed
@sunjayBhatia
Copy link
Member

sunjayBhatia commented Nov 17, 2023 •
edited
Loading

added #2592 since the long gw name has some wider implications, namely GEP-1762

@sjberman
Copy link
Contributor

@tao12345666333 This test is running as part of the core tests, but port is an extended feature. Shouldn't this test be run in the extended suite?

@tao12345666333
Copy link
Member Author

@sjberman oh, yes. Port is Extended, SectionName is Core.

@mlavacca mlavacca mentioned this pull request Nov 30, 2023
Merged
@ciarams87 ciarams87 mentioned this pull request Nov 30, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sunjayBhatia sunjayBhatia sunjayBhatia approved these changes

@mlavacca mlavacca mlavacca approved these changes

@michaelbeaumont michaelbeaumont Awaiting requested review from michaelbeaumont

Assignees

@arkodg arkodg

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/test lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tao12345666333 @mlavacca @arkodg @k8s-ci-robot @sunjayBhatia @sjberman