Improve the Gateway `ResolvedRefs` condition documentation #1419

mlavacca · 2022-09-30T09:36:37Z

What type of PR is this?

/kind documentation

What this PR does / why we need it:
The ListenerReasonRefNotPermitted documentation has been improved to specify that this reason can be triggered by either an unpermitted BackendRef or an unpermitted CertificateRef. Besides, the ListenerReasonInvalidCertificateRef documentation has been improved to specify better what makes a listener CertificateRef invalid

Which issue(s) this PR fixes:

Fixes #1362

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2022-09-30T09:36:46Z

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

robscott

Thanks @mlavacca! Would like to get @youngnick's review on this as well.

/cc @youngnick

robscott · 2022-09-30T18:40:38Z

apis/v1alpha2/gateway_types.go

+	// one of the Listener's Routes references an object in another namespace,
+	// where the object in the other namespace does not have a ReferenceGrant
+	// explicitly allowing the reference. The unpermitted reference can be
+	// either a BackendRef, or a CertificateRef.


A couple thoughts that are

It seems strange for this to extend to invalid references in attached routes. Maybe @youngnick has been thinking about this as part of the broader conditions update he's working on?

It seems like cross-namespace references that are not allowed by a ReferenceGrant should always receive the RefNotPermitted reason, regardless of if the Secret actually exists.

It seems like cross-namespace references that are not allowed by a ReferenceGrant should always receive the RefNotPermitted reason, regardless of if the Secret actually exists.

In this case, we should decide which reason has the highest severity: RefNotPermitted or InvalidCertificateRef. In my opinion, if no reference is allowed, the reason should beRefNotPermitted, no matter if the secret exists or is valid.
@robscott Do you agree/disagree or think that it is not clear by reading the description?

I agree with what you're suggesting in this comment, I think this PR still needs some work to make that clear in the spec.

It seems strange for this to extend to invalid references in attached routes. Maybe @youngnick has been thinking about this as part of the broader conditions update he's working on?

I just figured out that the previous comment here was copy-paste from the RouteReasonRefNotPermitted:

gateway-api/apis/v1beta1/shared_types.go

Lines 247 to 251 in 1e037bb

// This reason is used with the "ResolvedRefs" condition when

// one of the Listener's Routes has a BackendRef to an object in

// another namespace, where the object in the other namespace does

// not have a ReferenceGrant explicitly allowing the reference.

RouteReasonRefNotPermitted RouteConditionReason = "RefNotPermitted"

And it doesn't make any sense here; it misled me. The only reason to have RefNotPermitted should be the reference to an ungranted secret. So I'm going to fix it.

I addressed both your points, it should be clearer now.

apis/v1alpha2/gateway_types.go

robscott · 2022-09-30T23:35:11Z

@mlavacca Can you change the PR title to be a bit more descriptive? Easy to lose track of this PR when looking through the list.

robscott · 2022-10-03T20:14:53Z

/ok-to-test

mikemorris

The ListenerReasonRefNotPermitted changes LGTM, and I proposed a phrasing tweak to hopefully clarify ListenerReasonInvalidCertificateRef a bit more.

Thanks for your work on this @mlavacca, this feels pretty close now and a good clarification!

apis/v1alpha2/gateway_types.go

The ListenerReasonRefNotPermitted documentation has been improved to specify that this reason can be triggered by either an unpermitted BackendRef or an unpermitted CertificateRef. Besides, the ListenerReasonInvalidCertificateRef documentation has been improved to specify better what makes a listener CertificateRef invalid. Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>

mikemorris

/lgtm

robscott

Thanks @mlavacca, this is a great improvement!

/approve

k8s-ci-robot · 2022-10-05T22:50:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mikemorris, mlavacca, 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 kind/documentation Categorizes issue or PR as related to documentation. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 30, 2022

k8s-ci-robot requested review from shaneutt and youngnick September 30, 2022 09:36

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 Sep 30, 2022

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 30, 2022

mlavacca force-pushed the listener-not-permitted-docs branch 2 times, most recently from 614abc7 to 1144ce7 Compare September 30, 2022 09:38

mlavacca changed the title ~~Gateway API docs updated~~ Gateway API docs update Sep 30, 2022

robscott reviewed Sep 30, 2022

View reviewed changes

mlavacca changed the title ~~Gateway API docs update~~ Improve the Gateway ResolvedRefs condition documentation Oct 3, 2022

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 Oct 3, 2022

mlavacca force-pushed the listener-not-permitted-docs branch 2 times, most recently from 6195c10 to d2ea1f2 Compare October 4, 2022 13:01

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 4, 2022

mlavacca force-pushed the listener-not-permitted-docs branch from d2ea1f2 to fcf09f2 Compare October 4, 2022 13:04

mikemorris reviewed Oct 4, 2022

View reviewed changes

apis/v1alpha2/gateway_types.go Outdated Show resolved Hide resolved

mlavacca force-pushed the listener-not-permitted-docs branch from fcf09f2 to a9ff76e Compare October 5, 2022 08:28

mikemorris approved these changes Oct 5, 2022

View reviewed changes

k8s-ci-robot assigned mikemorris Oct 5, 2022

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

shaneutt requested a review from robscott October 5, 2022 18:51

robscott approved these changes Oct 5, 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 Oct 5, 2022

robscott added this to the v0.6.0 milestone Oct 5, 2022

k8s-ci-robot merged commit 38a295b into kubernetes-sigs:main Oct 5, 2022

mlavacca mentioned this pull request Oct 6, 2022

Improve the Gateway ResolvedRefs condition conformance tests #1437

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the Gateway `ResolvedRefs` condition documentation #1419

Improve the Gateway `ResolvedRefs` condition documentation #1419

mlavacca commented Sep 30, 2022

k8s-ci-robot commented Sep 30, 2022

robscott left a comment

robscott Sep 30, 2022

mlavacca Oct 3, 2022 •

edited

Loading

robscott Oct 3, 2022

mlavacca Oct 4, 2022

mlavacca Oct 4, 2022

robscott commented Sep 30, 2022

robscott commented Oct 3, 2022

mikemorris left a comment

mikemorris left a comment

robscott left a comment

k8s-ci-robot commented Oct 5, 2022

	// This reason is used with the "ResolvedRefs" condition when
	// one of the Listener's Routes has a BackendRef to an object in
	// another namespace, where the object in the other namespace does
	// not have a ReferenceGrant explicitly allowing the reference.
	RouteReasonRefNotPermitted RouteConditionReason = "RefNotPermitted"

Improve the Gateway ResolvedRefs condition documentation #1419

Improve the Gateway ResolvedRefs condition documentation #1419

Conversation

mlavacca commented Sep 30, 2022

k8s-ci-robot commented Sep 30, 2022

robscott left a comment

Choose a reason for hiding this comment

robscott Sep 30, 2022

Choose a reason for hiding this comment

mlavacca Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

robscott Oct 3, 2022

Choose a reason for hiding this comment

mlavacca Oct 4, 2022

Choose a reason for hiding this comment

mlavacca Oct 4, 2022

Choose a reason for hiding this comment

robscott commented Sep 30, 2022

robscott commented Oct 3, 2022

mikemorris left a comment

Choose a reason for hiding this comment

mikemorris left a comment

Choose a reason for hiding this comment

robscott left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 5, 2022

Improve the Gateway `ResolvedRefs` condition documentation #1419

Improve the Gateway `ResolvedRefs` condition documentation #1419

mlavacca Oct 3, 2022 •

edited

Loading