[knox] branch master updated: KNOX-2904 - Mark the endpoint as failed when we failover AND request is non-idempotent (#752)

[mo...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

more pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/knox.git


The following commit(s) were added to refs/heads/master by this push:
     new 8d0564b1d KNOX-2904 -  Mark the endpoint as failed when we failover AND request is non-idempotent (#752)
8d0564b1d is described below

commit 8d0564b1d60214ef3e1e7bb9111ff50ea5bdf776
Author: Sandeep Moré <mo...@gmail.com>
AuthorDate: Fri Apr 28 06:54:27 2023 -0400

    KNOX-2904 -  Mark the endpoint as failed when we failover AND request is non-idempotent (#752)
    
    * KNOX-2904 -  Mark the endpoint as failed when we failover AND request is non-idempotent
    
    * KNOX-2904 - review comments
---
 .../ha/dispatch/ConfigurableHADispatch.java        | 42 +++++++++++++++-------
 1 file changed, 30 insertions(+), 12 deletions(-)

diff --git a/gateway-provider-ha/src/main/java/org/apache/knox/gateway/ha/dispatch/ConfigurableHADispatch.java b/gateway-provider-ha/src/main/java/org/apache/knox/gateway/ha/dispatch/ConfigurableHADispatch.java
index e42ab960f..826275bfe 100644
--- a/gateway-provider-ha/src/main/java/org/apache/knox/gateway/ha/dispatch/ConfigurableHADispatch.java
+++ b/gateway-provider-ha/src/main/java/org/apache/knox/gateway/ha/dispatch/ConfigurableHADispatch.java
@@ -79,7 +79,7 @@ public class ConfigurableHADispatch extends ConfigurableDispatch {
   private List<String> disableLoadBalancingForUserAgents = Arrays.asList(HaServiceConfigConstants.DEFAULT_DISABLE_LB_USER_AGENTS);
 
   /**
-   *  This activeURL is used to track urls when LB is turned off for some clients
+   *  This activeURL is used to track urls when LB is turned off for some clients.
    *  The problem we have with selectively turning off LB is that other clients
    *  that use LB can change the state from under the current session where LB is
    *  turned off.
@@ -219,6 +219,8 @@ public class ConfigurableHADispatch extends ConfigurableDispatch {
       /* if non-idempotent requests are not allowed to failover */
       if(!failoverNonIdempotentRequestEnabled && nonIdempotentRequests.stream().anyMatch(outboundRequest.getMethod()::equalsIgnoreCase)) {
         LOG.cannotFailoverNonIdempotentRequest(outboundRequest.getMethod(), e.toString());
+        /* mark endpoint as failed */
+        markEndpointFailed(outboundRequest, inboundRequest);
         throw e;
       } else {
         LOG.errorConnectingToServer(outboundRequest.getURI().toString(), e);
@@ -308,14 +310,10 @@ public class ConfigurableHADispatch extends ConfigurableDispatch {
       outboundResponse.sendError(HttpServletResponse.SC_BAD_GATEWAY, "Service connection error, HA failover disabled");
       return;
     }
-    haProvider.markFailedURL(getServiceRole(), outboundRequest.getURI().toString());
-    AtomicInteger counter = (AtomicInteger) inboundRequest.getAttribute(FAILOVER_COUNTER_ATTRIBUTE);
-    if ( counter == null ) {
-      counter = new AtomicInteger(0);
-    }
+    /* mark endpoint as failed */
+    final AtomicInteger counter = markEndpointFailed(outboundRequest, inboundRequest);
     inboundRequest.setAttribute(FAILOVER_COUNTER_ATTRIBUTE, counter);
-    if ( counter.incrementAndGet() <= maxFailoverAttempts ) {
-      setupUrlHashLookup(); // refresh the url hash after failing a url
+    if ( counter.get() <= maxFailoverAttempts ) {
       //null out target url so that rewriters run again
       inboundRequest.setAttribute(AbstractGatewayFilter.TARGET_REQUEST_URL_ATTRIBUTE_NAME, null);
       // Make sure to remove the cookie ha cookie from the request
@@ -331,10 +329,6 @@ public class ConfigurableHADispatch extends ConfigurableDispatch {
         }
       }
       LOG.failingOverRequest(outboundRequest.getURI().toString());
-
-      /* in case of failover update the activeURL variable */
-      activeURL.set(outboundRequest.getURI().toString());
-
       executeRequest(outboundRequest, inboundRequest, outboundResponse);
     } else {
       LOG.maxFailoverAttemptsReached(maxFailoverAttempts, getServiceRole());
@@ -346,6 +340,30 @@ public class ConfigurableHADispatch extends ConfigurableDispatch {
     }
   }
 
+  /**
+   * A helper method that marks an endpoint failed.
+   * Changes HA Provider state.
+   * Changes ActiveUrl state.
+   * Changes for inbound urls should be handled by calling functions.
+   * @param outboundRequest
+   * @param inboundRequest
+   * @return current failover counter
+   */
+  private synchronized AtomicInteger markEndpointFailed(final HttpUriRequest outboundRequest, final HttpServletRequest inboundRequest) {
+    haProvider.markFailedURL(getServiceRole(), outboundRequest.getURI().toString());
+    AtomicInteger counter = (AtomicInteger) inboundRequest.getAttribute(FAILOVER_COUNTER_ATTRIBUTE);
+    if ( counter == null ) {
+      counter = new AtomicInteger(0);
+    }
+
+    if ( counter.incrementAndGet() <= maxFailoverAttempts ) {
+      setupUrlHashLookup(); // refresh the url hash after failing a url
+      /* in case of failover update the activeURL variable */
+      activeURL.set(outboundRequest.getURI().toString());
+    }
+    return counter;
+  }
+
   private String hash(String url) {
     return DigestUtils.sha256Hex(url);
   }