conformance: add a conformance test for cookie based session persistence #4350
Conversation
Signed-off-by: Arko Dasgupta <[email protected]>
k8s-ci-robot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: arkodg The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
size/L
|
tested with Envoy Gateway and it successfully passes |
There was a problem hiding this comment.
Pull request overview
This PR adds a conformance test for cookie-based session persistence in HTTPRoute, ensuring that requests with the same session cookie are consistently routed to the same backend pod. The implementation includes a new feature flag, test logic, YAML manifest, and documentation for running the test with Envoy Gateway.
Key Changes
- Introduces
SupportHTTPRouteSessionPersistenceCookiefeature flag marked as experimental - Implements test logic that verifies cookie-based session stickiness across 10 consecutive requests
- Adds example command for running the new test against Envoy Gateway
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
pkg/features/httproute.go |
Adds the SupportHTTPRouteSessionPersistenceCookie feature flag and metadata with experimental channel, integrates it into HTTPRouteExtendedFeatures |
conformance/tests/httproute-session-persistence-cookie.go |
Implements the conformance test that verifies cookie-based session persistence by checking that subsequent requests with the same cookie route to the same pod |
conformance/tests/httproute-session-persistence-cookie.yaml |
Defines the HTTPRoute manifest with session persistence configuration using Cookie type and Session lifetime |
hack/implementations/envoy-gateway/README.md |
Updates documentation with example command for running the session persistence conformance test |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // This option indicates support for HTTPRoute additional redirect status code 303 (extended conformance) | ||
| SupportHTTPRoute307RedirectStatusCode FeatureName = "HTTPRoute307RedirectStatusCode" | ||
|
|
||
| // This option indicates support for HTTPRoute additional redirect status code 303 (extended conformance) |
There was a problem hiding this comment.
The comment for SupportHTTPRoute308RedirectStatusCode incorrectly states "redirect status code 303" when it should state "redirect status code 308". This appears to be a copy-paste error from line 107.
| // This option indicates support for HTTPRoute additional redirect status code 303 (extended conformance) | |
| // This option indicates support for HTTPRoute additional redirect status code 308 (extended conformance) |
| for i := 0; i < 10; i++ { | ||
| cReq, cRes, err := suite.RoundTripper.CaptureRoundTrip(req) | ||
| if err != nil { | ||
| t.Fatalf("request %d with cookie failed: %v", i+1, err) |
There was a problem hiding this comment.
The error message says "request %d with cookie failed" but on the first iteration (i=0), the cookie hasn't been set yet. The error message should be more accurate, perhaps "request %d failed" and only mention the cookie in subsequent iterations after i > 0.
| t.Fatalf("request %d with cookie failed: %v", i+1, err) | |
| if i == 0 { | |
| t.Fatalf("request %d failed: %v", i+1, err) | |
| } else { | |
| t.Fatalf("request %d with cookie failed: %v", i+1, err) | |
| } |
| t.Fatalf("request %d with cookie failed: %v", i+1, err) | ||
| } | ||
| if err := http.CompareRoundTrip(t, &req, cReq, cRes, expected); err != nil { | ||
| t.Fatalf("request %d with cookie failed expectations: %v", i+1, err) |
There was a problem hiding this comment.
The error message says "request %d with cookie failed expectations" but on the first iteration (i=0), the cookie hasn't been set yet. The error message should be more accurate, perhaps "request %d failed expectations" and only mention the cookie in subsequent iterations after i > 0.
| t.Fatalf("request %d with cookie failed expectations: %v", i+1, err) | |
| if i == 0 { | |
| t.Fatalf("request %d failed expectations: %v", i+1, err) | |
| } else { | |
| t.Fatalf("request %d with cookie failed expectations: %v", i+1, err) | |
| } |
|
|
||
| req := http.MakeRequest(t, &expected, gwAddr, "HTTP", "http") | ||
| initialPod := "" | ||
| for i := 0; i < 10; i++ { |
There was a problem hiding this comment.
The test uses a hardcoded value of 10 iterations to verify session persistence. Consider extracting this as a named constant at the package or function level (e.g., const numSessionPersistenceRequests = 10) to make it easier to understand the test's intent and allow for easier modification if needed.
| parser := &stdhttp.Response{Header: stdhttp.Header(headers)} | ||
| cookies := parser.Cookies() | ||
| if len(cookies) == 0 { | ||
| return nil, fmt.Errorf("cookie not found: headers: %v", headers) | ||
| } | ||
| return cookies[0], nil |
There was a problem hiding this comment.
The parseCookie function returns the first cookie from the response without verifying that it's the session persistence cookie. If the backend sets multiple cookies (e.g., tracking cookies, CSRF tokens, etc.), this function might return the wrong cookie. Consider either: 1) validating that the returned cookie has expected characteristics (e.g., specific name pattern), 2) iterating through cookies to find one that matches session persistence criteria, or 3) documenting the assumption that the first cookie is always the session persistence cookie.
| parser := &stdhttp.Response{Header: stdhttp.Header(headers)} | |
| cookies := parser.Cookies() | |
| if len(cookies) == 0 { | |
| return nil, fmt.Errorf("cookie not found: headers: %v", headers) | |
| } | |
| return cookies[0], nil | |
| const sessionCookieName = "session-persistence" | |
| parser := &stdhttp.Response{Header: stdhttp.Header(headers)} | |
| cookies := parser.Cookies() | |
| if len(cookies) == 0 { | |
| return nil, fmt.Errorf("cookie not found: headers: %v", headers) | |
| } | |
| for _, cookie := range cookies { | |
| if cookie.Name == sessionCookieName { | |
| return cookie, nil | |
| } | |
| } | |
| return nil, fmt.Errorf("session persistence cookie %q not found: headers: %v", sessionCookieName, headers) |
2 participants
What type of PR is this?
/kind test
/area conformance-test
What this PR does / why we need it:
add a conformance test for cookie based session persistence
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: