Add SessionPersistence API as experimental by gcs278 · Pull Request #2969 · kubernetes-sigs/gateway-api

gcs278 · 2024-04-12T14:47:05Z

From GEP-1619, add BackendLBPolicy with SessionPersistence fields and add the SessionPersistence fields on HTTPRouteRule and GRPCRouteRule as experimental.

/kind feature

What this PR does / why we need it:
Makes SessionPersistence API experimental.

Which issue(s) this PR fixes:
Updates #713

Does this PR introduce a user-facing change?:

BackendLBPolicy has been added to enable session persistence configuration for backends.
A SessionPersistence field has been added to HTTPRoute and GRPCRoute to enable session persistence for routes.

k8s-ci-robot · 2024-04-12T14:47:07Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

gcs278 · 2024-04-12T17:10:49Z

/cc @robscott @shaneutt @ginayeh

k8s-ci-robot · 2024-04-12T17:10:53Z

@gcs278: GitHub didn't allow me to request PR reviews from the following users: ginayeh.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @robscott @shaneutt @ginayeh

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.

gcs278 · 2024-04-12T17:15:57Z

apis/v1/shared_types.go

+	//
+	// When set to "Permanent", AbsoluteTimeout indicates the
+	// cookie's lifetime via the Expires or Max-Age cookie attributes
+	// and is required.


Should this requirement of having SessionPersistence.AbsoluteTimeout when SessionPersistence.LifetimeType = Permanent be enforced by CEL?

Or should we just allow implementations to decide a valid default?

Good question, I think we should definitely aim to enforce this with CEL. Happy to help figure out what that looks like if it ends up being tricky.

Added it.

I tested the CEL by hand in my own cluster with the CRD. Are there CRD integration tests for validating CEL that I should add a test case to? I looked around and couldn't find anything.

Yep, sorry I missed this, these tests are part of https://github.com/kubernetes-sigs/gateway-api/tree/main/pkg/test/cel. Here's where we run them in presubmits:

gateway-api/hack/verify-crds-kind.sh

Line 60 in a101a23

go test -v -timeout=120s -count=1 --tags ${CHANNEL} sigs.k8s.io/gateway-api/pkg/test/cel || res=$?

Ah should I looked a little harder...added a CEL test.

robscott

Thanks @gcs278! Just took a quick look at this but so far mostly LGTM.

robscott · 2024-04-12T17:20:53Z

apis/v1/shared_types.go

+	//
+	// When set to "Permanent", AbsoluteTimeout indicates the
+	// cookie's lifetime via the Expires or Max-Age cookie attributes
+	// and is required.


Good question, I think we should definitely aim to enforce this with CEL. Happy to help figure out what that looks like if it ends up being tricky.

robscott · 2024-04-12T17:22:10Z

apis/v1/shared_types.go

+	// Support: Implementation-specific
+	//
+	// +optional
+	// +kubebuilder:validation:MaxLength=4096


This seems like an awfully long length, what kind of session name would be this long?

Good question...

I believe I got the max length from https://www.rfc-editor.org/rfc/rfc6265:

Practical user agent implementations have limits on the number and
size of cookies that they can store. General-use user agents SHOULD
provide each of the following minimum capabilities:

o At least 4096 bytes per cookie (as measured by the sum of the
length of the cookie's name, value, and attributes).

But this is the minimum capabilities, so I suppose that's the worst case (smallest supported cookie size)...of the worst case (the entire cookie size is the name...).

So yea 4096 is a somewhat arbitrary. Is it best practice to limit string lengths? Or is it okay to leave unspecified?

Is it best practice to limit string lengths?

Yep, it's been a strong requirement to avoid any kind of unbounded values in Gateway API.

as measured by the sum of the length of the cookie's name, value, and attributes

In the case of cookie-based persistence this would just be the cookie name right? (Not the value or attributes)

In the case of cookie-based persistence this would just be the cookie name right? (Not the value or attributes)

Correct. SessionName is just the cookie name. I was just trying to find established limits, and the only one I came across was this 4096 bytes value for the whole cookie.

Since I can't find any enforced value, should we start with something reasonable? Maybe 128 or 256?

Let's start with 128 and increase if/when necessary. It's hard for me to think of a use case that would require a longer session name, but I could be missing something.

128 seems reasonable to start with. I suppose increasing the limit is much easier than decreasing the limit if we need to change it later.

robscott · 2024-04-12T17:23:58Z

apis/v1alpha2/backendlbpolicy_types.go

+	// Currently, Backends (i.e. Service, ServiceImport, or any
+	// implementation-specific backendRef) are the only valid API
+	// target references.
+	TargetRef PolicyTargetReference `json:"targetRef"`


I know you're already on this, but just in case we forget, when #2966 merges, we'll need to update this to be both a list and a local reference.

candita · 2024-04-15T22:27:41Z

apis/applyconfiguration/apis/v1/sessionpersistence.go

As an experimental feature, does this belong in v1?

Discussed in meeting, but since this is an experimental field and not an entire resource, we have to add it to any API versions that exist for that resource, in this case that includes v1. Importantly we add the <gateway:experimental> tags around this field to ensure it's only accessible if you install the experimental version of this CRD. This is conceptually similar to upstream Kubernetes where new fields added to the v1 Service API are initially guarded by alpha feature gates (but if that feature gate is enabled, the field is accessible as part of the v1 Service API).

gcs278 · 2024-04-16T02:50:15Z

/cc @kflynn

@robscott Updated to include LocalPolicyTargetReference as provided by #2966

robscott

Thanks @gcs278!

robscott · 2024-04-16T22:13:05Z

apis/v1alpha2/backendlbpolicy_types.go

+	// Currently, Backends (i.e. Service, ServiceImport, or any
+	// implementation-specific backendRef) are the only valid API
+	// target references.
+	TargetRef LocalPolicyTargetReference `json:"targetRef"`


Suggested change

TargetRef LocalPolicyTargetReference `json:"targetRef"`

// +listType=map

// +listMapKey=group

// +listMapKey=kind

// +listMapKey=name

// +kubebuilder:validation:MinItems=1

// +kubebuilder:validation:MaxItems=16

TargetRefs []LocalPolicyTargetReference `json:"targetRefs"`

I think this should be a list of object references, and I think this list should be "atomic" as I'd expect to exclusively have one owner, but interested in what others think here.

cc @dprotaso for advice on list type

listType=map with keys [group, kind, name] would make sense here. Then different parts of the list could be 'owned' by different people

Sounds good, that likely is far more future proof. Somehow I'd convinced myself that these resources would always have a single owner, but there will likely be at least some edge cases where that's not true. I've updated the original suggestion to reflect your advice @dprotaso.

Ack. Updated to be a list.

robscott · 2024-04-16T22:26:14Z

apis/v1/shared_types.go

+	// Support: Implementation-specific
+	//
+	// +optional
+	// +kubebuilder:validation:MaxLength=4096


Let's start with 128 and increase if/when necessary. It's hard for me to think of a use case that would require a longer session name, but I could be missing something.

robscott · 2024-04-17T18:26:11Z

Thanks @gcs278! Will defer to someone else for final LGTM.

/approve

shaneutt

Seems like a great huge step to getting something out there that implementations can try to implement and start giving us some feedback 👍

/approve

@gcs278 out of curiosity do you have an implementation that you tested this out with? If you do, would you be opposed to putting some time on our meeting agenda to do a quick demo for us? Would be really cool!

(also in general if you've had a chance to implement this locally and sort of try it out, that could be very valuable)

shaneutt · 2024-04-17T20:27:13Z

apis/v1/shared_types.go

+// SessionPersistence defines the desired state of
+// SessionPersistence.


Suggested change

// SessionPersistence defines the desired state of

// SessionPersistence.

// SessionPersistence defines the desired state of SessionPersistence.

shaneutt · 2024-04-17T20:29:53Z

apis/v1/shared_types.go

+	// the use a header or cookie. Defaults to cookie based session
+	// persistence.
+	//
+	// Support: Core for "Cookie" type


shaneutt · 2024-04-17T20:29:58Z

apis/v1/shared_types.go

+	//
+	// Support: Core for "Cookie" type
+	//
+	// Support: Extended for "Header" type


shaneutt · 2024-04-17T20:30:05Z

apis/v1/shared_types.go

+	// CookieConfig provides configuration settings that are specific
+	// to cookie-based session persistence.
+	//
+	// Support: Core


k8s-ci-robot · 2024-04-17T20:35:56Z

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

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

From GEP-1619, add BackendLBPolicy with SessionPersistence fields and add the SessionPersistence fields on HTTPRouteRule and GRPCRouteRule as experimental.

Add CEL to ensure absoluteTimeout is set when cookie lifetimeType is Permanent.

gcs278 · 2024-04-19T04:20:09Z

Seems like a great huge step to getting something out there that implementations can try to implement and start giving us some feedback 👍

/approve

@gcs278 out of curiosity do you have an implementation that you tested this out with? If you do, would you be opposed to putting some time on our meeting agenda to do a quick demo for us? Would be really cool!

I do not. I would love for an implementation to give this a shot. I will advertise for it in our community meeting.

(also in general if you've had a chance to implement this locally and sort of try it out, that could be very valuable)

+1 - that's one of the reasons I brought up Blitx as a reference implementation. I wish I had a "proving ground" for testing the API. I suppose it can be a real implementation. One of my concerns with the experimental API, is that it's mostly theoretical. I'm open to suggestions.

- Add CEL validation tests - Update BackendLBPolicy to support list of TargetRefs - Change SessionName limit to 128 bytes

robscott · 2024-04-19T16:29:47Z

Thanks @gcs278!

/lgtm
/hold cancel

k8s-ci-robot requested review from danwinship and robscott April 12, 2024 14:47

gcs278 force-pushed the GEP1619-experimental branch 2 times, most recently from bf40849 to 56a0446 Compare April 12, 2024 15:22

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/gep PRs related to Gateway Enhancement Proposal(GEP) labels Apr 12, 2024

gcs278 changed the title ~~Add SessionPersistence API via BackendLBPolicy~~ Add SessionPersistence API as experimental Apr 12, 2024

gcs278 marked this pull request as ready for review April 12, 2024 15: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 Apr 12, 2024

k8s-ci-robot requested a review from howardjohn April 12, 2024 15:27

gcs278 mentioned this pull request Apr 12, 2024

Adding changelog for v1.1.0-rc1 #2967

Merged

gcs278 force-pushed the GEP1619-experimental branch 4 times, most recently from 144d3c6 to 377c4eb Compare April 12, 2024 16:12

k8s-ci-robot requested a review from shaneutt April 12, 2024 17:10

gcs278 commented Apr 12, 2024

View reviewed changes

robscott reviewed Apr 12, 2024

View reviewed changes

candita reviewed Apr 15, 2024

View reviewed changes

gcs278 force-pushed the GEP1619-experimental branch from 4be3138 to 72ec6cd Compare April 16, 2024 02:46

k8s-ci-robot requested a review from kflynn April 16, 2024 02:50

robscott reviewed Apr 16, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 17, 2024

shaneutt approved these changes Apr 17, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 19, 2024

gcs278 added 3 commits April 19, 2024 00:11

Add SessionPersistence API as experimental

29391c7

From GEP-1619, add BackendLBPolicy with SessionPersistence fields and add the SessionPersistence fields on HTTPRouteRule and GRPCRouteRule as experimental.

GEP-1619: Add CEL for absoluteTimeout requirement

8e529f5

Add CEL to ensure absoluteTimeout is set when cookie lifetimeType is Permanent.

Update to support LocalPolicyTargetReference

a245d54

gcs278 force-pushed the GEP1619-experimental branch from 4a6d109 to d2b23cf Compare April 19, 2024 04:14

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 19, 2024

Add CEL validation tests and BackendLBPolicy API updates

2f10b0a

- Add CEL validation tests - Update BackendLBPolicy to support list of TargetRefs - Change SessionName limit to 128 bytes

gcs278 force-pushed the GEP1619-experimental branch from d2b23cf to 2f10b0a Compare April 19, 2024 04:32

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 19, 2024

k8s-ci-robot assigned robscott Apr 19, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 19, 2024

k8s-ci-robot merged commit 3ec2d65 into kubernetes-sigs:main Apr 19, 2024

-	TargetRef LocalPolicyTargetReference `json:"targetRef"`
+	// +listType=map
+	// +listMapKey=group
+	// +listMapKey=kind
+	// +listMapKey=name
+	// +kubebuilder:validation:MinItems=1
+	// +kubebuilder:validation:MaxItems=16
+	TargetRefs []LocalPolicyTargetReference `json:"targetRefs"`

		// SessionPersistence defines the desired state of
		// SessionPersistence.

Conversation

gcs278 commented Apr 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Apr 12, 2024

Uh oh!

gcs278 commented Apr 12, 2024

Uh oh!

k8s-ci-robot commented Apr 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robscott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gcs278 commented Apr 16, 2024

Uh oh!

robscott left a comment

Choose a reason for hiding this comment

Uh oh!

robscott Apr 16, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robscott commented Apr 17, 2024

Uh oh!

shaneutt left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Apr 17, 2024

Uh oh!

gcs278 commented Apr 12, 2024 •

edited

Loading

robscott Apr 16, 2024 •

edited

Loading

shaneutt left a comment •

edited

Loading