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.

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

Cross-namespace parentRef reason for routes accepted condition #1670

Closed
mlavacca opened this issue Jan 23, 2023 · 9 comments · Fixed by #1672
Closed

Cross-namespace parentRef reason for routes accepted condition #1670

mlavacca opened this issue Jan 23, 2023 · 9 comments · Fixed by #1672
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature.
Milestone

Comments

@mlavacca
Copy link
Member

What would you like to be added:

A new RouteConditionReason should be added to state that the route is not accepted because a cross-namespace reference is not granted by a ReferenceGrant.

We already have a specular reason for the ResolvedRefs condition:

// 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"

That should be set in such a case, jointly with a failed accepted condition, but no reason for the accepted condition is suitable to express it.

Why this is needed:

As of now, the *route Accepted condition can fail with the following reasons :

  • NotAllowedByListeners
  • NoMatchingListenerHostname
  • NoMatchingParent
  • UnsupportedValue

When a route references a gateway (through a ParentRef) in another namespace, and no ReferenceGrant allows such a reference, there is no suitable reason to use for the Accepted condition to state it as failed.

@mlavacca mlavacca added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 23, 2023
@shaneutt shaneutt added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Jan 23, 2023
@Sajiyah-Salat
Copy link
Contributor

This is somewhat confusing for me as a beginner. Can you please break it down or provide me some resources to get started in this. Thank you.

@mlavacca
Copy link
Member Author

mlavacca commented Jan 24, 2023

This is somewhat confusing for me as a beginner. Can you please break it down or provide me some resources to get started in this. Thank you.

This issue is about adding a new reason to the set of already existing reasons for the accepted condition of the *route:

// This condition indicates whether the route has been accepted or rejected
// by a Gateway, and why.
//
// Possible reasons for this condition to be true are:
//
// * "Accepted"
//
// Possible reasons for this condition to be False are:
//
// * "NotAllowedByListeners"
// * "NoMatchingListenerHostname"
// * "UnsupportedValue"
//
// Possible reasons for this condition to be Unknown are:
//
// * "Pending"
//
// Controllers may raise this condition with other reasons,
// but should prefer to use the reasons listed above to improve
// interoperability.
RouteConditionAccepted RouteConditionType = "Accepted"
// This reason is used with the "Accepted" condition when the Route has been
// accepted by the Gateway.
RouteReasonAccepted RouteConditionReason = "Accepted"
// This reason is used with the "Accepted" condition when the route has not
// been accepted by a Gateway because the Gateway has no Listener whose
// allowedRoutes criteria permit the route
RouteReasonNotAllowedByListeners RouteConditionReason = "NotAllowedByListeners"
// This reason is used with the "Accepted" condition when the Gateway has no
// compatible Listeners whose Hostname matches the route
RouteReasonNoMatchingListenerHostname RouteConditionReason = "NoMatchingListenerHostname"
// This reason is used with the "Accepted" condition when there are
// no matching Parents. In the case of Gateways, this can occur when
// a Route ParentRef specifies a Port and/or SectionName that does not
// match any Listeners in the Gateway.
RouteReasonNoMatchingParent RouteConditionReason = "NoMatchingParent"
// This reason is used with the "Accepted" condition when a value for an Enum
// is not recognized.
RouteReasonUnsupportedValue RouteConditionReason = "UnsupportedValue"
// This reason is used with the "Accepted" when a controller has not yet
// reconciled the route.
RouteReasonPending RouteConditionReason = "Pending"

This reason should be used to express that the Route has not been accepted because it has a ParentRef that references a Gateway in another namespace, but no ReferenceGrant allows such a reference.

Therefore, simply adding the new reason to the API and providing the documentation is the scope of the issue. Such a reason will likely be used in the conformance tests in subsequent PRs.

@Sajiyah-Salat
Copy link
Contributor

// Possible reasons for this condition to be False are:

In this block there are only three condititons while @mlavacca says that there are four reasons and the extra reason is "NoMatchingParent"

RouteReasonRefNotPermitted RouteConditionReason = "RefNotPermitted"

Here in place of RefNoPermitted I should change it to accepted. and make changes in docs as well accordingly. right?

@mlavacca
Copy link
Member Author

@Sajiyah-Salat The documentation related to the Accepted condition has been recently updated. Here is the updated one:

// This condition indicates whether the route has been accepted or rejected
// by a Gateway, and why.
//
// Possible reasons for this condition to be true are:
//
// * "Accepted"
//
// Possible reasons for this condition to be False are:
//
// * "NotAllowedByListeners"
// * "NoMatchingListenerHostname"
// * "NoMatchingParent"
// * "UnsupportedValue"
//
// Possible reasons for this condition to be Unknown are:
//
// * "Pending"

As you can see, four different reasons can make the Accepted condition fail. The issue is about adding a fifth condition, similar to the RefNotPermitted one (which is related to the ResolvedRefs condition), that aims at stating that the route has not been accepted because a ParentRef references a Gateway in another namespace, but no ReferenceGrant allows such a reference.

@Sajiyah-Salat
Copy link
Contributor

#1672
Please review this pr and help me in adding a code line as I just added a comment now just to demonstrate what I have understood.Thank you.

@Sajiyah-Salat
Copy link
Contributor

Sajiyah-Salat commented Feb 2, 2023

Hello please close this issue.

@mlavacca
Copy link
Member Author

mlavacca commented Feb 2, 2023

Hello please close this issue.

👍 For the future, to make a merging PR automatically close the related issue, you need to write "Fixes #<issue_number>" in the PR description.

@mlavacca
Copy link
Member Author

mlavacca commented Feb 2, 2023

Closing as fixed by #1672

@mlavacca mlavacca closed this as completed Feb 2, 2023
@shaneutt shaneutt added this to the v0.6.1 milestone Feb 2, 2023
@shaneutt shaneutt reopened this Feb 7, 2023
@shaneutt shaneutt linked a pull request Feb 7, 2023 that will close this issue
@shaneutt
Copy link
Member

shaneutt commented Feb 7, 2023

Re-opened briefly to properly link the PR, closing again.

@shaneutt shaneutt closed this as completed Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants