moresandeep commented on code in PR #912:
URL: https://github.com/apache/knox/pull/912#discussion_r1601848944
##########
gateway-provider-ha/src/main/java/org/apache/knox/gateway/ha/dispatch/ConfigurableHADispatch.java:
##########
@@ -121,9 +123,16 @@ public void init() {
/* setup the active URL for non-LB case */
activeURL.set(haProvider.getActiveURL(getServiceRole()));
- // Suffix the cookie name by the service to make it unique
- // The cookie path is NOT unique since Knox is stripping the service name.
- stickySessionCookieName = stickySessionCookieName + '-' + getServiceRole();
+ // TODO I'm not sure that appending the role to the cookie name (or the
alternative of setting the correct cookie path)
+ // is really necessary.
+ // Knox sets a single session cookie for a topology, and I don't see how
that would be used for multiple services.
+ // I think only a seriously broken client would mix up the sticky cookies,
and even then
+ // Knox would drop any sticky cookie that does not point to a proper
backend.
+ if (!useRoutesForStickyCookiePath) {
Review Comment:
i see what you are trying to do, if routes are set then scope the cookie
based on path. There are few problems with this approach.
We have default topology feature
(https://knox.apache.org/books/knox-1-1-0/user-guide.html#Default+Topology+URLs)
where services do not need to specify context, we do that for CM. Not sure
how this will be impacted.
Debugging would be tricky with this given cookies names won't be intuitive.
##########
gateway-provider-ha/src/main/java/org/apache/knox/gateway/ha/dispatch/ConfigurableHADispatch.java:
##########
@@ -121,9 +123,16 @@ public void init() {
/* setup the active URL for non-LB case */
activeURL.set(haProvider.getActiveURL(getServiceRole()));
- // Suffix the cookie name by the service to make it unique
- // The cookie path is NOT unique since Knox is stripping the service name.
- stickySessionCookieName = stickySessionCookieName + '-' + getServiceRole();
+ // TODO I'm not sure that appending the role to the cookie name (or the
alternative of setting the correct cookie path)
+ // is really necessary.
Review Comment:
This is needed because a topology can have multiple services and those
services can have sticky enabled and disabled. So, a topology can have one
service with sticky session ON and other with OFF. This is the reason for
appending the service name.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@knox.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
