-
Notifications
You must be signed in to change notification settings - Fork 148
Add session persistence support for NGINX Plus users using the sticky cookie directive
#4305
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
base: feat/session-persistence
Are you sure you want to change the base?
Add session persistence support for NGINX Plus users using the sticky cookie directive
#4305
Conversation
sticky cookie directivesticky cookie directive
| units := []unit{ | ||
| {"ms", 1}, | ||
| {"s", 1000}, | ||
| {"m", 60 * 1000}, | ||
| {"h", 60 * 60 * 1000}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can switch its order if we want larger units first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why convert hours to seconds? nginx supports hours
622cc92 to
ded1099
Compare
sticky cookie directivesticky cookie directive
|
FYI, the release note in the PR description isn't going to be used at all since this is being merged into the feature branch. For the main PR that we merge at the end, we'll want a descriptive release note to discuss the different ways session persistence is supported. |
|
See my comment on the other PR, I think we have to rethink this a bit to support a backend being referenced multiple times, to prevent unintended behavior or blocking desired behavior. |
sticky cookie directivesticky cookie directive
9863bd1 to
758a284
Compare
1b4dc06 to
fc5c7cb
Compare
edd4d9e to
fcad961
Compare
sticky cookie directivesticky cookie directive
fcad961 to
864ddd8
Compare
| refsWithPolicies := createBackendRefs(createSPConfig("policies-sp"), "policies") | ||
|
|
||
| refsWithPolicies := createBackendRefs("policies") | ||
| getExpectedConfig := func() SessionPersistenceConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| getExpectedConfig := func() SessionPersistenceConfig { | |
| getExpectedSPConfig := func() SessionPersistenceConfig { |
| Name string | ||
| // Expiry is the expiration time of the session. | ||
| Expiry string | ||
| // Path is the path of the for which session is applied. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Path is the path of the for which session is applied. | |
| // Path is the path for which session is applied. |
| if !featureFlags.Plus && rule.SessionPersistence != nil { | ||
| ruleErrors = append(ruleErrors, field.Forbidden( | ||
| rulePath.Child("sessionPersistence"), | ||
| "SessionPersistence is only supported in NGINX Plus. This configuration will be ignored.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking we make this error more helpful for OSS users. Mention that ip_hash load balancing method can be set in an UpstreamSettingsPolicy for simple session persistence.
| ruleErrors = append(ruleErrors, field.Forbidden( | ||
| rulePath.Child("sessionPersistence"), | ||
| "SessionPersistence", | ||
| "SessionPersistence is only supported in experimental mode.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also mention NGINX plus here, so a user doesn't see this, enable experimental, and then get a new error.
| ) | ||
|
|
||
| createAllValidValidator := func() *validationfakes.FakeHTTPFieldsValidator { | ||
| createAllValidValidator := func(duration *v1.Duration) *validationfakes.FakeHTTPFieldsValidator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love that this function that was intended to be "all valid" now accepts a very specific arg that changes its behavior. Can we have separate validator function for duration?
| if sp.AbsoluteTimeout != nil { | ||
| if cookieLifetimeType == v1.SessionCookieLifetimeType { | ||
| expiry = "" | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if sp.AbsoluteTimeout != nil { | |
| if cookieLifetimeType == v1.SessionCookieLifetimeType { | |
| expiry = "" | |
| } | |
| } | |
| if sp.AbsoluteTimeout != nil && cookieLifetimeType == v1.SessionCookieLifetimeType { | |
| expiry = "" | |
| } |
|
|
||
| var errors routeRuleErrors | ||
| if sp.SessionName == nil { | ||
| errors.warn = append(errors.warn, field.Required(path.Child("sessionName"), "sessionName cannot be empty")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not? It's optional in the spec. Should we autogenerate a session name if not specified? Or do some implementations not need this, and that's why it's optional?
| } | ||
|
|
||
| if sp.IdleTimeout != nil { | ||
| errors.warn = append(errors.warn, field.Forbidden( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forbidden or not supported?
| path := getCookiePath(match) | ||
| paths = append(paths, path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| path := getCookiePath(match) | |
| paths = append(paths, path) | |
| paths = append(paths, getCookiePath(match)) |
| func TestProcessSessionPersistenceConfiguration(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| createAllValidValidator := func(duration *gatewayv1.Duration) *validationfakes.FakeHTTPFieldsValidator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as earlier about "all valid" validator.
|
Did you also verify where the same path is specified multiple times with different matching conditions and session persistence configs? |
| timeout = absoluteTimeout | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also need to validate if cookie type is Permanent, that AbsoluteTimeout must be specified? The spec states this.
|
|
||
| if sp.AbsoluteTimeout != nil { | ||
| if cookieLifetimeType == v1.SessionCookieLifetimeType { | ||
| expiry = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the timeout is just ignored in this case? The API spec confuses me on how we treat this.
|
Another thing, the |
Proposed changes
Write a clear and concise description that helps reviewers understand the purpose and impact of your changes. Use the
following format:
Problem: Users want to be able to specify session persistence for their upstreams.
Solution: Add support for session persistence using
sticky cookiedirectives which is only available for NGINX Plus usersTesting: Manual testing
Errors
Spec validation
NGF Errors
non-plus behaviour
non-experimental and non-plus behaviour
Configuration
HTTPRoutes
Case 1: path and expires specified
Case 2 : no expiry, only path specified
Regular expression with other path matches (no path set)
Multiple matches with common prefix
multiple matches with no common prefix
GRPCRoutes (path is always empty for GRPCRoutes
Hostname matching
Exact Method matching
Traffic Splitting with HTTPRoutes
Upstreams generated
Testing Session Persistence
Testing is done using a script by grabbing the cookie session id from the first request and using it in curl request to ensure each request goes to the same backend
HTTPRoutes (coffee -> sticky, tea -> regular)
GRPCRoutes (backend v1 --> normal, backend v2 --> sticky)
Each have 3 endpoints
Note:
RegularExpressionpath matches, we leave the cookie Path empty. A regex can match anywhere within the URL path (for example, /coffee/[A-Za-z]+/tea), so deriving a concrete cookie path from it would be misleading and could unintentionally restrict which requests send the cookie.Please focus on (optional): If you any specific areas where you would like reviewers to focus their attention or provide
specific feedback, add them here.
Closes #4231
Checklist
Before creating a PR, run through this checklist and mark each as complete.
Release notes
If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.