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

Finish Route Rule API design and update edge case behaviors. #2909

Merged
merged 6 commits into from
Apr 9, 2024

Conversation

ginayeh
Copy link
Contributor

@ginayeh ginayeh commented Mar 28, 2024

What type of PR is this?
/kind gep

Optionally add one or more of the following kinds if applicable:
N/A

What this PR does / why we need it:
The main goal of this PR is to get #1619 ready for implementable by answering the open questions in Graduation Criteria for Implementable Status:
4. Finish designing the Route Rule API and document edge cases in Edge Case Behavior for configuring session persistence on both BackendLBPolicy and route rules.

Which issue(s) this PR fixes:
Fixes #2634

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/gep PRs related to Gateway Enhancement Proposal(GEP) cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 28, 2024
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 28, 2024
@k8s-ci-robot
Copy link
Contributor

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

@k8s-ci-robot
Copy link
Contributor

Welcome @ginayeh!

It looks like this is your first PR to kubernetes-sigs/gateway-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/gateway-api has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@robscott
Copy link
Member

/cc @gcs278
/ok-to-test

@k8s-ci-robot k8s-ci-robot requested a review from gcs278 March 28, 2024 19:06
@k8s-ci-robot 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 Mar 28, 2024
@robscott robscott added this to the v1.1.0 milestone Mar 28, 2024
Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Thanks @ginayeh!

`BackendLBPolicy` API. However, this API should be reevaluated in a future update to this GEP, considering the
associated edge cases when configuring both routes and backends.
To support route rule level configuration, this GEP also introduces an API as inline fields within HTTPRouteRule and GRPCRouteRule.
Any configuration that is specified at Route Rule level MUST override configuration that is attached at the backend level because route rule have a more global view and responsibility for the overall traffic routing.
Copy link
Member

Choose a reason for hiding this comment

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

Since we're making this "extended" support and elsewhere in the GEP we recognize that not everyone can support route-level config, I think we need to provide some kind of standard way to communicate that an implementation does not support the config at this level. Something conceptually similar to the IncompatibleFilters reason might make sense here, but I'm not sure that implementations that can't support session persistence config will want to reject the route. I don't think we need to solve this before the first experimental release of the API, but I just want to make sure we don't lose track of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point. I added this as an open question in the GEP. Please take a look.

@robscott
Copy link
Member

Thanks @ginayeh! Will defer to @gcs278 or @youngnick for final LGTM.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ginayeh, 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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 29, 2024
@gcs278
Copy link
Contributor

gcs278 commented Mar 30, 2024

Thanks @ginayeh! I will take a look at it early next week.

Copy link
Contributor

@gcs278 gcs278 left a comment

Choose a reason for hiding this comment

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

Thanks! Looking good to me, just some minor nit picks.

use cases. Only a subset of implementations have already designed their data plane to incorporate route-level session
persistence, making it likely that route-level session persistence will be less widely implemented.
use cases. Only a subset of implementations have already designed their data plane to incorporate route rule level session
persistence, making it likely that route rule level session persistence will be less widely implemented.

### Traffic Splitting

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we dropped the weight 0 scenario, I think we should still place a call out for weight 0, as it could be a ambiguous situation. I think the Traffic Splitting section makes the most sense. You can use own judgement if you disagree. I can't suggest for this line, so commenting this nearby:

When a
persistent session is established and traffic splitting is configured across services, the persistence to a single
backend MUST be maintained across services, **even if the weight is set to 0.**

Copy link
Contributor Author

@ginayeh ginayeh Apr 1, 2024

Choose a reason for hiding this comment

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

Thanks for pointing out. I forgot to add more context about why I removed the weight 0 example and also want to confirm I'm not missing anything critical.

1. Curl to /a which establishes a persistent session with servicev1
2. Curl to /b routes to servicev1 due to route persistence despite weight: 0 configuration

The previous example we had is doing two route rules with path /a and /b, and path /b has traffic split with weight 0. Since each route rule should have its own persistent session, so applying cookies from persistent session /a to servicev1 should not affact /b traffic routing with servicev1. As a result, /b should be routed to servicev2 with weight: 100. Does it make sense?

Then, I was thinking about how a persistent session can be established with a weight: 0 backend? Maybe servicev1 had a non-zero weight and a persistent session established, and then route resource is re-deployed and servicev1 has a new weight: 0 configuration, but I am not sure if we expect session persistence to work after deployment, so I stopped adding an example with weight: 0. Is there other example with weight: 0 that we can include?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since each route rule should have its own persistent session, so applying cookies from persistent session /a to servicev1 should not affact /b traffic routing with servicev1.

So, to be clear, even if the session persistence is attached to the Service via BackendLBPolicy, each route rule should have different cookies / persistent sessions? Hmm. I was originally thinking that the cookie would be associated with the service when attached via BackendLBPolicy, so that it could be shared among route rules (in the weight 0 example).

If it isn't shared among route rules when using BackendLBPolicy, would that be problematic for setting the cookie name? For instance, ServiceA with cookie named "foo". There's two route rules for /x and /y, but both go just to ServiceA. The cookie "foo" will encode the backendID for ServiceA. Won't the cookie be re-used for /x and /y (two route paths)?

I think regardless, you bring up a good point that I think we should clarify in the GEP: does attaching to Service mean cookies are reused across route rules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think attaching BackendLBPolicy to a Service should NOT mean cookies are resued across route rules referenceing to the same service. Let me update PR with a clarification on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another commit was pushed for this. PTAL @gcs278

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification @ginayeh. LGTM

@k8s-ci-robot 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 Apr 9, 2024
@gcs278
Copy link
Contributor

gcs278 commented Apr 9, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 9, 2024
@robscott
Copy link
Member

robscott commented Apr 9, 2024

Thanks @ginayeh and @gcs278!

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 9, 2024
@k8s-ci-robot k8s-ci-robot merged commit 8345de3 into kubernetes-sigs:main Apr 9, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/gep PRs related to Gateway Enhancement Proposal(GEP) lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants