feat: add `RouteReasonNoMatchingParent` #1516

pmalek · 2022-11-08T13:24:17Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?:

- Add `RouteReasonNoMatchingParent` reason for `Accepted` condition set on routes when no matching parent has been found

k8s-ci-robot · 2022-11-08T13:24:25Z

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

conformance/tests/httproute-invalid-backendref-not-matching-listener-port.go

conformance/tests/httproute-invalid-backendref-not-matching-listener-port.yaml

shaneutt · 2022-11-08T14:14:58Z

Approved, couple small comments. I'll let Rob or Nick do the final lgtm.

robscott

Thanks @pmalek! This mostly LGTM, just a few nits.

conformance/tests/httproute-invalid-backendref-not-matching-listener-port.go

apis/v1beta1/shared_types.go

robscott · 2022-11-08T16:42:37Z

conformance/tests/httproute-invalid-backendref-not-matching-listener-port.yaml

+apiVersion: gateway.networking.k8s.io/v1beta1
+kind: Gateway
+metadata:
+  name: gateway-listener-not-matching-route-port
+  namespace: gateway-conformance-infra
+spec:
+  gatewayClassName: "{GATEWAY_CLASS_NAME}"
+  listeners:
+  - name: listener-1
+    port: 80
+    protocol: HTTP
+    allowedRoutes:
+      namespaces:
+        from: Same
+---


Every Gateway we use in conformance tests adds some real costs. I think we could just reuse one of our base Gateways instead of deploying a new one here, for example this one could just be referenced by the HTTPRoute below:

gateway-api/conformance/base/manifests.yaml

Lines 16 to 29 in 53e55ea

apiVersion: gateway.networking.k8s.io/v1beta1

kind: Gateway

metadata:

name: same-namespace

namespace: gateway-conformance-infra

spec:

gatewayClassName: "{GATEWAY_CLASS_NAME}"

listeners:

- name: http

port: 80

protocol: HTTP

allowedRoutes:

namespaces:

from: Same

Makes sense. PTAL.

robscott · 2022-11-08T16:44:41Z

/ok-to-test

robscott

Sorry I missed some key details in my first review, a few more comments now that I've taken a closer look. I this is really everything now though.

robscott · 2022-11-08T23:01:36Z

conformance/tests/httproute-invalid-backendref-not-matching-listener-port.yaml

+      # mismatched port here (81 is not an available gateway listener) triggers NoMatchingParent reason
+      port: 81


Not sure how I missed this, but this would only be invalid if it was on the parentRef, not the backendRef. We should be able to forward traffic to backends on any port.

Whoa, whoops! I did it too, zoomed right over it. Good catch.

Of course I missed it 🤦

apis/v1beta1/shared_types.go

robscott · 2022-11-08T23:06:41Z

conformance/tests/httproute-invalid-backendref-not-matching-listener-port.go

+)
+
+func init() {
+	ConformanceTests = append(ConformanceTests, HTTPRouteInvalidBackendRefNotMatchingListenerPort)


I think every instance of "InvalidBackendRef" in this PR should be renamed "InvalidParentRef" as a follow up to my other comment

robscott · 2022-11-08T23:10:04Z

conformance/tests/httproute-invalid-backendref-not-matching-listener-port.go

+var HTTPRouteInvalidBackendRefNotMatchingListenerPort = suite.ConformanceTest{
+	ShortName:   "HTTPRouteInvalidBackendRefNotMatchingListenerPort",
+	Description: "A single HTTPRoute in the gateway-conformance-infra namespace should set the Accepted status to False with reason NoMatchingParent when attempting to bind to a Gateway that does not have a matching ListenerPort.",
+	Manifests:   []string{"tests/httproute-invalid-backendref-not-matching-listener-port.yaml"},


Since this is covering an experimental feature (Port on ParentRef), we need to feature gate this test. Something like https://github.com/kubernetes-sigs/gateway-api/blob/main/conformance/tests/httproute-response-header-modifier.go#L36 and https://github.com/kubernetes-sigs/gateway-api/blob/main/conformance/utils/suite/suite.go#L50.

Got it. PTAL

robscott

Thanks @pmalek! This change LGTM, it looks like you need to run codegen (make generate) though. It would also be nice if you could squash this change into a single commit.

conformance/utils/suite/suite.go

robscott · 2022-11-14T22:34:27Z

Hey @pmalek, do you think you'll have time to take another look at this in the next ~day? We're just about ready to send v0.6.0 for API review and would love to include this in the release if possible.

pmalek · 2022-11-15T13:52:24Z

Hey @pmalek, do you think you'll have time to take another look at this in the next ~day? We're just about ready to send v0.6.0 for API review and would love to include this in the release if possible.

@robscott Sorry for the delay. I did manage to update the PR as you described. Apparently it needed a make fmt (make generate didn't produce any differences).

robscott

Thanks @pmalek!

/lgtm
/approve

k8s-ci-robot · 2022-11-15T17:16:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pmalek, robscott, shaneutt

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,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 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 Nov 8, 2022

k8s-ci-robot requested review from bowei and mikemorris November 8, 2022 13:24

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 8, 2022

pmalek marked this pull request as ready for review November 8, 2022 13:27

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 Nov 8, 2022

k8s-ci-robot requested review from keithmattix and robscott November 8, 2022 13:27

pmalek changed the title ~~feat: add RouteReasonNoMatchingParent~~ feat: add RouteReasonNoMatchingParent Nov 8, 2022

pmalek mentioned this pull request Nov 8, 2022

GEP-957: Add RouteConditionReason which could indicate a route not matching parentRefs Ports #1511

Closed

shaneutt approved these changes Nov 8, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2022

robscott reviewed Nov 8, 2022

View reviewed changes

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 8, 2022

pmalek requested review from robscott and removed request for bowei, keithmattix and mikemorris November 8, 2022 21:21

robscott reviewed Nov 8, 2022

View reviewed changes

pmalek requested a review from robscott November 9, 2022 21:23

robscott reviewed Nov 11, 2022

View reviewed changes

conformance/utils/suite/suite.go Outdated Show resolved Hide resolved

shaneutt linked an issue Nov 11, 2022 that may be closed by this pull request

Given GEP-957 how to mark status of routes not matching neither port nor hostname of parentRef? #1532

Closed

pmalek force-pushed the add-accepted-routereason-nomatchingparent branch from ca05e07 to 06bd152 Compare November 15, 2022 13:37

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 Nov 15, 2022

feat: add RouteReasonNoMatchingParent

e932a6c

pmalek force-pushed the add-accepted-routereason-nomatchingparent branch from 06bd152 to e932a6c Compare November 15, 2022 13:40

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

shaneutt requested a review from robscott November 15, 2022 16:22

robscott reviewed Nov 15, 2022

View reviewed changes

k8s-ci-robot assigned robscott Nov 15, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 15, 2022

k8s-ci-robot merged commit b3c8e2a into kubernetes-sigs:main Nov 15, 2022

pmalek mentioned this pull request Nov 29, 2022

fix: filter hostnames in HTTPRoute Kong/kubernetes-ingress-controller#3180

Merged

1 task

pmalek mentioned this pull request Mar 28, 2024

REQUEST: New membership for pmalek kubernetes/org#4855

Closed

9 tasks

	apiVersion: gateway.networking.k8s.io/v1beta1
	kind: Gateway
	metadata:
	name: same-namespace
	namespace: gateway-conformance-infra
	spec:
	gatewayClassName: "{GATEWAY_CLASS_NAME}"
	listeners:
	- name: http
	port: 80
	protocol: HTTP
	allowedRoutes:
	namespaces:
	from: Same

		# mismatched port here (81 is not an available gateway listener) triggers NoMatchingParent reason
		port: 81

feat: add RouteReasonNoMatchingParent #1516

feat: add RouteReasonNoMatchingParent #1516

Uh oh!

Conversation

pmalek commented Nov 8, 2022

Uh oh!

k8s-ci-robot commented Nov 8, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shaneutt commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robscott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

robscott Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

pmalek Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

robscott commented Nov 8, 2022

Uh oh!

robscott left a comment

Choose a reason for hiding this comment

Uh oh!

robscott Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

shaneutt Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmalek Nov 9, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

robscott Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

robscott Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

pmalek Nov 9, 2022

Choose a reason for hiding this comment

Uh oh!

robscott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

robscott commented Nov 14, 2022

Uh oh!

pmalek commented Nov 15, 2022

Uh oh!

robscott left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Nov 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

feat: add `RouteReasonNoMatchingParent` #1516

feat: add `RouteReasonNoMatchingParent` #1516

shaneutt commented Nov 8, 2022 •

edited

Loading

shaneutt Nov 9, 2022 •

edited

Loading