Repository navigation
[ISSUE #8037] Validate sticky session configuration at the management plane and cover cross-replica verification - #8305
Conversation
…gement plane and cover cross-replica verification
wy471x
left a comment
wy471x
left a comment
There was a problem hiding this comment.
The direction is right: the validator sits next to CanaryConfigValidator, is wired into every RuleServiceImpl write path that previously checked canary only, and the docs state the management/runtime split clearly. Findings below are about cases where an "enabled but silently inert" configuration still passes the new validation, which is precisely what this PR set out to eliminate.
StickySessionConfigValidator#validateCookieName- explicitnullstill passes and silently disables affinity:
if (Objects.nonNull(cookieName) && StringUtils.isBlank(cookieName))Gson overwrites the field initializer with null for an explicit JSON null, and StickySessionAffinity#isActive treats a blank cookie name as inactive. So {"enabled":true,"secrets":["s"],"cookieName":null} is persisted and the gateway never writes a cookie - the exact failure mode the PR description rejects. StringUtils.isBlank is null-safe, so the guard can go and null can be rejected with the same message ("omit it to use the default" and "explicit null" are not equivalent here).
validateKeychecks onlyname, nottype. Gson maps an unrecognized enum constant tonull, andDeterministicKeyExtractor#extractfalls through to the header path for anything that is notCOOKIE:
if (DeterministicKeyConfig.KeyType.COOKIE == config.getType()) { ... } else { headers.getFirst(name) }So {"key":{"name":"X","type":"cookie"}} is accepted and then reads a request header, silently picking the wrong request location - the same class of degradation rejected for onUnavailable.
-
The documented "error naming the offending field (
sticky.…)" does not hold for type mismatches.RuleServiceImplroutes the handle throughCanaryConfigValidator.parseHandle, which validates JSON types forcanaryonly;"sticky":[],"maxAgeSeconds":1.5or"secrets":"s"therefore surface asInvalid Divide handle JSONinstead of namingsticky.maxAgeSeconds/sticky.secrets. Astickytype pre-check in the sameobject(root, "sticky")style ascanarywould close that gap, and would also give the validator aJsonObjectto validate against, the wayCanaryConfigValidatordeliberately checks types "before Gson can coerce values". -
Coverage of the paths this PR changes:
importData(List)andimportData(namespace, List)were both renamed and now validate sticky, buttestInvalidStickyNeverWritesOrPublishesasserts nothing about either, while the canary analogue does. The namespace overload is the more interesting one because it silently skips rules whose selector is unmapped, so a guard regression there would be invisible. -
StickySessionConfigValidatorre-encodes the activation predicate already implemented inStickySessionAffinity#isActive(enabled, positive lifetime, non-blank cookie name, at least one non-blank secret), kept in sync only by a comment. Adding a field means editing both places with no test asserting they still agree; a sharedisUsable()onStickySessionConfig(or moving it toshenyu-commonnext to the config) would give the predicate one owner. -
Minor naming:
validateDivideHandleandvalidateRuleHandleare near-synonyms called back to back in five methods, and theObjects.nonNull(divide)guard added around the sticky call is dead -parseHandlealready returns non-null or throws for any non-blank handle.
The two "another replica" tests are fine; the rollout one in particular exercises the one-directional key rotation the docs describe.
…ty or read the wrong location
|
Thanks @wy471x — all six points are addressed in the latest commit.
The docs now also narrow the runtime-tolerance claim: semantic values degrade to plain load balancing, while structural JSON type errors are rejected at parse time (as the canary section already did). Tests on this branch: common 682, divide 162, admin 2691 (the pre-existing flaky |
Aias00
left a comment
Aias00
left a comment
There was a problem hiding this comment.
Stream Agent analysis
FINAL cross-file review of PR #8305 (head 947c216) for sticky session config validation. Among 10 partitions, part-1 identified a HIGH severity regression: null check removed on DivideRuleHandle in RuleServiceImpl.java line 265, risking NPE. All other partitions passed. CI failed (passed=false). Therefore overall verdict is NEEDS_CHANGES.
Evidence
- part-1 review: NEEDS_CHANGES with HIGH finding at shenyu-admin/.../RuleServiceImpl.java line 265 RIGHT side: removed null check on parseHandle result
- CI status from attachment: complete=true, passed=false, check_count=67
- FINAL synthesis: all_parts_passed=false due to part-1 NEEDS_CHANGES; complete_diff=true but CI not passed
Recommended next steps
- Restore null check on 'divide' in RuleServiceImpl.java or ensure CanaryConfigValidator.parseHandle never returns null
- Investigate and fix failing CI checks (67 checks, passed=false)
- Re-review partition part-1 after fix to confirm resolution
Scope: captured diff and GitHub check metadata only; no local tests were executed.
Task: e8283956-5d10-494f-a0fb-c9e4c52e8705 · Commit: 947c216e0b010bb5afc5666a1357c9f8280343f7
Review verdict: NEEDS_CHANGES
FINAL scope synthesis of 10 partition reviews for PR #8305 at head 947c216. Partition part-1 reported NEEDS_CHANGES with a HIGH severity blocking finding (null check removed on DivideRuleHandle leading to potential NPE). All other partitions (part-0,2-9) returned PASS. CI passed=false. Per reduction rules, child NEEDS_CHANGES cannot be overridden and all_parts_passed is false; thus FINAL verdict is NEEDS_CHANGES. No local tests executed; no write tools used.
Publication: REQUEST_CHANGES (VERIFIED_BLOCKING_FINDINGS)
- shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/RuleServiceImpl.java:265 [RIGHT] Null check on 'divide' returned by CanaryConfigValidator.parseHandle was removed. If parseHandle can return null, divide.getCanary() (and later divide.getSticky() at line 269) will throw NullPointerException, not caught as IllegalArgumentException, causing unhandled exception.
Review details
This PR adds management-plane validation for sticky sessions but introduces a potential NullPointerException in RuleServiceImpl.java. The null check on the DivideRuleHandle returned by CanaryConfigValidator.parseHandle was removed; if parseHandle can return null, divide.getCanary()/getSticky() will throw an unhandled NPE. Please restore the guard or guarantee non-null, and resolve the failing CI before merge.
|
Thanks @Aias00 — the NPE path is not reachable, so I made the non-null contract explicit instead of restoring the caller guard.
The failing CI check is the pre-existing flaky |
Aias00
left a comment
Aias00
left a comment
There was a problem hiding this comment.
Stream Agent analysis
Cross-file synthesis of PR #8305 (head 1d600f3) for sticky session config validation. All 10 partition reviews passed with complete diff, but CI checks did not pass (passed=false, complete=true). Therefore final verdict cannot be PASS per policy.
Evidence
- Frozen attachment: PR cross-file review JSON shows all_parts_passed=true, complete_diff=true, ci.passed=false
- Child partitions part-0..part-9 all verdict PASS with no blocking findings
- Head sha 1d600f3, base 585f99d
Recommended next steps
- Investigate CI check failures (9 checks, passed=false) and ensure they pass
- Confirm cross-replica verification tests align with runtime semantics
- Re-run final review after CI green
Scope: captured diff and GitHub check metadata only; no local tests were executed.
Task: 564c9588-b082-4e75-8ec7-8406f3ef53fc · Commit: 1d600f3ed7d2ad5197934b40b666ddb283bca445
Review verdict: INCONCLUSIVE
FINAL synthesis scope: all 10 partition reviews returned PASS with complete_diff=true and no HIGH/CRITICAL findings. However CI status indicates passed=false (complete=true, check_count=9). Per review policy, a FINAL PASS requires all_parts_passed true, complete_diff true, and CI passed. Since CI did not pass, the verdict cannot be PASS; no concrete diff-anchored defects exist to force NEEDS_CHANGES, thus INCONCLUSIVE. Limitation: CI failure root cause not in supplied diff; cross-file runtime compatibility assumed from passing partition logic but not validated by green CI.
Publication: COMMENT (INCONCLUSIVE_REVIEW)
Review details
All supplied diff partitions review clean, but CI is failing. Please investigate CI failures before merging. No code-level blocking defects found in the reviewed partitions.
|
Thanks @Aias00 — the CI snapshot behind this verdict is stale; the same head is now fully green, and no code changed since it. At
The earlier If the stored review metadata still shows the old run, a manual re-trigger should pick up run 41971210351. |
Aias00
left a comment
Aias00
left a comment
There was a problem hiding this comment.
Stream Agent analysis
FINAL review of PR #8305 at head 1d600f3: cross-file synthesis of 10 partition reviews all PASS, complete diff supplied, CI passed (68 checks). No HIGH/CRITICAL defects; management-plane sticky session validation and tests are coherent.
Evidence
- Frozen attachment: all_parts_passed true, complete_diff true, global_complete_diff true
- CI passed: 68 checks complete, passed true
- 10 partition reviews (part-0..part-9) each verdict PASS with no blocking findings
Recommended next steps
- Maintainers to perform final merge decision per project process
- Optionally verify documentation matches released artifact
Scope: captured diff and GitHub check metadata only; no local tests were executed.
Task: 5ecdb40e-9a6e-447e-bc7f-e994015066b9 · Commit: 1d600f3ed7d2ad5197934b40b666ddb283bca445
Review verdict: PASS
SYNTHESIS/FINAL scope: assessed cross-file correctness from all child summaries. All 10 partitions reported PASS with empty blocking_findings. complete_diff and global_complete_diff true, CI passed (68 checks). No child NEEDS_CHANGES/INCONCLUSIVE overridden. Therefore FINAL PASS is justified. Limitation: no local test execution, but CI provides evidence.
Publication: APPROVE (EXPLICIT_PASS)
Review details
Recommendation: The PR is ready for maintainer review and merge given passing checks and complete review. No blocking issues found.
Part of #8037.
Problem
Phase 1 and 2 delivered the affinity itself (#8098, #8212), but the sticky section of a divide rule handle is never validated by the management plane: the admin rule service only inspects
canary.*, sostickypasses through Gson's lenient binding straight into the store — an unknownonUnavailablespelling silently behaves asREBINDat runtime, a non-positivemaxAgeSecondsor blank secrets silently disable the feature, and nothing tells the operator why. The test suite also exercised the affinity only through a single instance, leaving the operational assumption — that any gateway replica sharing the rule's secrets honors another replica's binding — documented but unverified.Change
This PR adds
StickySessionConfigValidator, a management-plane validator for the sticky section of an enabled configuration:onUnavailablemust be exactlyREBINDorSTRICT(the runtime compares case-sensitively, so other spellings are rejected instead of silently degrading),maxAgeSecondsmust be positive,cookieNamemust be a non-blank string (an explicitnullis not treated as equivalent to omitting it), an enabled configuration requires at least one non-blank secret, and a configured deterministic key requires a non-blank name and, whentypeis present,HEADERorCOOKIE(the runtime treats anything else as a header lookup). Structure is checked before Gson can coerce values, so a non-objectsticky, a non-integer lifetime, a non-array secret list or a non-string field fails with a message naming thesticky.*field instead of the generic parse error, and"enabled":1no longer escapes as a rawIllegalStateException. The admin rule service calls it for every divide rule write path —createOrUpdate,create,update,registerDefault, the configuration import and the namespace sync — so the save fails with a message naming the offendingsticky.*field.CanaryConfigValidator.parseHandlealso guarantees and documents a non-null result, so callers can dereference it directly without a redundant guard.A disabled configuration is exempt from value validation. The gateway activation checks ignore it entirely, so none of its values can affect a request, and rejecting it would stop an operator from editing an unrelated field of an already stored rule. Structural JSON type checks still run, because those shapes cannot be bound at all.
The gateway runtime keeps its tolerance for values: a semantically unusable configuration persisted before this validation still degrades to plain load balancing, so an upgrade does not turn a stored bad value into a request failure — the write path is where new mistakes are stopped. Structural JSON type errors are rejected when the handle is parsed, as the canary section already does. Documentation gains a management-validation section and a cross-replica operation section stating that affinity is portable only across replicas sharing the rule's secrets, and that a key rollout keeps working in one direction only: a replica that already holds the new key still accepts bindings signed with the old key, while a replica that still holds only the old key cannot verify a binding signed with the new one and starts a new session instead.
Tests
StickySessionConfigValidatorTestcovers every rejection branch (unknown and lower-cased policy, non-positive lifetime, blank and explicit-null cookie name, enabled without secrets, blank secrets, key without name, key without type) plus both valid key types, the valid case and the disabled pass-through case; an agreement test asserts that the validator accepts exactly the configurations the shared runtime activation predicate accepts.CanaryConfigValidatorTestcovers a nullstickysection, the structural type errors above, a valid structured section and the parser's non-null contract.RuleServiceTestadds the admin-side behavior: an invalid sticky section fails the create, the update, the registration and both configuration import overloads without writing a rule or publishing an event, a valid section is stored unchanged, and a disabled section does not block the write.DivideStickySessionAffinityTestadds the replica scenarios: a binding signed by one replica is honored by another replica sharing the secrets, a replica still signing with the old key is honored during a rotation window by a replica that already holds the new key, and a replica without the shared secrets rejects the foreign binding.Verification
mvn -pl shenyu-common,shenyu-plugin/shenyu-plugin-proxy/shenyu-plugin-divide,shenyu-admin -am clean test -DskipTests=falsepasses the common suite (683), the divide suite (162) and the admin rule service suite (58) clean; the full admin suite runs 2691 tests with the single pre-existing flakyShenyuClusterServiceTest.testSelectMasterErrorCountTrackingfailing (cluster-master error counting, unrelated to this change; it passes 9/9 in isolation). Checkstyle reports zero violations in the three modules. On the previous code the new validator and parse tests are red: the explicit-null cookie name, the nullkey.type, the activation-agreement test and the structural type-message test.