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

AttachedRoutes spec does not clarify how to handle partially invalid states #1626

Closed
robscott opened this issue Jan 5, 2023 · 11 comments · Fixed by #2396
Closed

AttachedRoutes spec does not clarify how to handle partially invalid states #1626

robscott opened this issue Jan 5, 2023 · 11 comments · Fixed by #2396
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-blocker MUST be completed to complete the milestone
Milestone

Comments

@robscott
Copy link
Member

robscott commented Jan 5, 2023

What happened:
The current AttachedRoutes spec is rather vague and does not clarify how partially invalid states should be handled.

// AttachedRoutes represents the total number of Routes that have been
// successfully attached to this Listener.
AttachedRoutes int32 `json:"attachedRoutes"`

This came up in #1624 (comment).

What you expected to happen:
I expected clear guidance on how to set this value for partially invalid listeners or routes.

@robscott robscott added the kind/bug Categorizes issue or PR as related to a bug. label Jan 5, 2023
@robscott
Copy link
Member Author

robscott commented Jan 5, 2023

Here are a couple possible options:

1. AttachedRoutes should only represent Routes that have actually been programmed with the Gateway
In general, I think this would mean that the Listener would have to have reached the "Programmed" state and Routes would have had to reach the "Accepted" state. Importantly, if a Listener or a Route transitioned to an invalid state in the future but was still programmed in the underlying dataplane, it would still be considered "Attached".

2. AttachedRoutes should not care about the status of the Listener or Routes
If a Route is attempting to attach to a Listener and the Listener allows for that connection, it should be counted towards AttachedRoutes. This would not take into consideration the status of either Listener or Route. For example, even if a Route referred to a non-existent Service or a Listener referred to a non-existent TLS cert, AttachedRoutes would still represent the number of connected resources.

At first glance, 1 seems like it would be best for users but hardest to implement, and 2 seems like it would be simplest to implement. Also open to any other alternative approaches.

@shaneutt shaneutt added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jan 9, 2023
@mikemorris
Copy link
Contributor

mikemorris commented Jan 10, 2023

In response to conversation during the Gateway API weekly meeting, I don't think renaming or deprecating attachedRoutes would make sense semantically - "acceptance" is mostly a property in relation to the Gateway API controller, "attached" (to the extent that the docs still use that language) has become more a property in relation to a Gateway listener/dataplane and correlated with a "programmed" status if applicable (@shaneutt described this similarly) - it makes sense how this evolved historically, but I think they're two separate concepts now.

+1 to definition 1️⃣ , if there's a need to express a count of routes targeting a Gateway listener, we could possibly introduce a new field targetedRoutes or acceptedRoutes (I can't think of a better term that signifies the listener is being targeted by the routes rather than the listener targeting the routes without implying a child relationship which wouldn't be accurate, open to suggestions).

It would be important to define edge cases like if a route not permitted by AllowedRoutes or route type not present in the supportedKinds field should be included in an acceptedRoutes total - I think those would both be excluded if we restrict this count to only Accepted routes - this probably has some bearing on how Accepted status is defined in https://gateway-api.sigs.k8s.io/geps/gep-1364/#partial-validity-and-conditions

I'm unsure if/how these fields should be set if the listener itself is in an Accepted: false state for some reason (TLS certificate invalid, port unavailable, etc).

For operators, comparing these two counts might be a helpful first triage step - "are there some Accepted routes targeting this listener that are not successfully attached?"

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 10, 2023
@robscott robscott moved this from Triage to Backlog in Gateway API: The Road to GA Apr 10, 2023
@robscott robscott added this to the v0.7.1 milestone Apr 10, 2023
@robscott robscott moved this from Backlog to Next in Gateway API: The Road to GA Apr 10, 2023
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle rotten
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 10, 2023
@robscott
Copy link
Member Author

/lifecycle frozen
/help

@k8s-ci-robot
Copy link
Contributor

@robscott:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

  • Why are we solving this issue?
  • To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
  • Does this issue have zero to low barrier of entry?
  • How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/lifecycle frozen
/help

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.

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. and removed lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels May 18, 2023
@shaneutt shaneutt modified the milestones: v0.7.1, v1.0.0 May 18, 2023
@shaneutt shaneutt added the release-blocker MUST be completed to complete the milestone label May 18, 2023
@shaneutt
Copy link
Member

shaneutt commented Aug 15, 2023

Here is a proposed plan of attack, going with number 2 above ("AttachedRoutes should not care about the status of the Listener or Routes"):

  1. add an official definition of "attached" in this context, with a meaning aligning with not caring about listener or route status and being clear that the purpose of this is to track things like "if I delete this gateway, how many routes have I orphaned?"
  2. add documentation which explains what it means above
  3. create an issue to create conformance tests which check that programmed AND non-programmed routes get counted
  4. kick up an issue to add a ProgrammedRoutes later if the community finds that useful

@shaneutt
Copy link
Member

Talked to @sunjayBhatia about this, and he's up for taking this one on.

/assign @sunjayBhatia

We expect resolving this nearly "autoresolves" #1925 as well.

Thanks @sunjayBhatia! Let us know if you run into any blockers.

@shaneutt shaneutt moved this from Next to In Progress in Gateway API: The Road to GA Aug 24, 2023
@shaneutt shaneutt removed help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. labels Aug 24, 2023
@shaneutt
Copy link
Member

@sunjayBhatia just checking in, any trouble moving this one (and #1925 ) forward? Let us know if we can help!

@sunjayBhatia
Copy link
Member

Submitted #2396

@sunjayBhatia
Copy link
Member

sunjayBhatia commented Sep 15, 2023

Also added #2402

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-blocker MUST be completed to complete the milestone
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

6 participants