Merge pull request #4531 from mposolda/crossdc-fix-tests

KEYCLOAK-5371 Fix ActionTokenCrossDCTest and BruteForceCrossDCTest

model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/InfinispanActionTokenStoreProvider.java

@@ -16,6 +16,7 @@
  */
 package org.keycloak.models.sessions.infinispan;
 
+import org.jboss.logging.Logger;
 import org.keycloak.cluster.ClusterProvider;
 import org.keycloak.common.util.Time;
 import org.keycloak.models.*;
@@ -33,6 +34,8 @@ import org.infinispan.Cache;
  */
 public class InfinispanActionTokenStoreProvider implements ActionTokenStoreProvider {
 
+    private static final Logger LOG = Logger.getLogger(InfinispanActionTokenStoreProvider.class);
+
     private final Cache<ActionTokenReducedKey, ActionTokenValueEntity> actionKeyCache;
     private final InfinispanKeycloakTransaction tx;
     private final KeycloakSession session;
@@ -58,6 +61,8 @@ public class InfinispanActionTokenStoreProvider implements ActionTokenStoreProvi
         ActionTokenReducedKey tokenKey = new ActionTokenReducedKey(key.getUserId(), key.getActionId(), key.getActionVerificationNonce());
         ActionTokenValueEntity tokenValue = new ActionTokenValueEntity(notes);
 
+        LOG.debugf("Adding used action token to actionTokens cache: %s", tokenKey.toString());
+
         this.tx.put(actionKeyCache, tokenKey, tokenValue, key.getExpiration() - Time.currentTime(), TimeUnit.SECONDS);
     }
 
@@ -68,7 +73,15 @@ public class InfinispanActionTokenStoreProvider implements ActionTokenStoreProvi
         }
 
         ActionTokenReducedKey key = new ActionTokenReducedKey(actionTokenKey.getUserId(), actionTokenKey.getActionId(), actionTokenKey.getActionVerificationNonce());
-        return this.actionKeyCache.getAdvancedCache().get(key);
+
+        ActionTokenValueModel value = this.actionKeyCache.getAdvancedCache().get(key);
+        if (value == null) {
+            LOG.debugf("Not found any value in actionTokens cache for key: %s", key.toString());
+        } else {
+            LOG.debugf("Found value in actionTokens cache for key: %s", key.toString());
+        }
+
+        return value;
     }
     
     @Override

model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/entities/ActionTokenValueEntity.java

@@ -45,6 +45,11 @@ public class ActionTokenValueEntity implements ActionTokenValueModel {
         return notes.get(name);
     }
 
+    @Override
+    public String toString() {
+        return String.format("ActionTokenValueEntity [ notes=%s ]", notes.toString());
+    }
+
     public static class ExternalizerImpl implements Externalizer<ActionTokenValueEntity> {
 
         private static final int VERSION_1 = 1;

testsuite/integration-arquillian/HOW-TO-RUN.md

@@ -460,13 +460,13 @@ The cross DC requires setting a profile specifying used cache server by specifyi
 
 a) First compile the Infinispan/JDG test server via the following command:
 
-  `mvn -Pcache-server-infinispan -f testsuite/integration-arquillian -DskipTests clean install`
+  `mvn -Pcache-server-infinispan,auth-servers-crossdc-undertow -f testsuite/integration-arquillian -DskipTests clean install`
 
 or
 
-  `mvn -Pcache-server-jdg -f testsuite/integration-arquillian -DskipTests clean install`
+  `mvn -Pcache-server-jdg,auth-servers-crossdc-undertow -f testsuite/integration-arquillian -DskipTests clean install`
 
-b) Then in case you want to use **JBoss-based** containers instead of containers on Embedded Undertow run following command:
+b) Then in case you want to use **JBoss-based** Keycloak backend containers instead of containers on Embedded Undertow run following command:
 
     `mvn -Pauth-servers-crossdc-jboss,auth-server-wildfly -f testsuite/integration-arquillian -DskipTests clean install`
 
@@ -476,7 +476,7 @@ By default JBoss-based containers use in-memory h2 database. It can be configure
 
   `mvn -Pauth-servers-crossdc-jboss,auth-server-wildfly,jpa -f testsuite/integration-arquillian -DskipTests clean install -Djdbc.mvn.groupId=org.mariadb.jdbc -Djdbc.mvn.artifactId=mariadb-java-client -Djdbc.mvn.version=2.0.3 -Dkeycloak.connectionsJpa.url=jdbc:mariadb://localhost:3306/keycloak -Dkeycloak.connectionsJpa.password=keycloak -Dkeycloak.connectionsJpa.user=keycloak`
 
-c1) Then you can run the tests using the following command (adjust the test specification according to your needs) for containers on **Undertow**:
+c1) Then you can run the tests using the following command (adjust the test specification according to your needs) for Keycloak backend containers on **Undertow**:
 
   `mvn -Pcache-server-infinispan,auth-servers-crossdc-undertow -Dtest=*.crossdc.* -pl testsuite/integration-arquillian/tests/base clean install`
 
@@ -484,7 +484,7 @@ or
 
   `mvn -Pcache-server-jdg,auth-servers-crossdc-undertow -Dtest=*.crossdc.* -pl testsuite/integration-arquillian/tests/base clean install`
 
-c2) For **JBoss-based** containers:
+c2) For **JBoss-based** Keycloak backend containers:
 
   `mvn -Pcache-server-infinispan,auth-servers-crossdc-jboss,auth-server-wildfly -Dtest=*.crossdc.* -pl testsuite/integration-arquillian/tests/base clean install`
 

testsuite/integration-arquillian/servers/auth-server/undertow/src/main/java/org/keycloak/testsuite/arquillian/undertow/lb/SimpleUndertowLoadBalancer.java

@@ -17,7 +17,12 @@
 
 package org.keycloak.testsuite.arquillian.undertow.lb;
 
+import java.lang.reflect.Field;
 import java.net.URI;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.LinkedList;
+import java.util.List;
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
 
@@ -34,6 +39,7 @@ import io.undertow.server.handlers.proxy.ProxyHandler;
 import io.undertow.util.AttachmentKey;
 import io.undertow.util.Headers;
 import org.jboss.logging.Logger;
+import org.keycloak.common.util.reflections.Reflections;
 import org.keycloak.services.managers.AuthenticationSessionManager;
 import java.util.LinkedHashMap;
 import java.util.StringTokenizer;
@@ -110,11 +116,12 @@ public class SimpleUndertowLoadBalancer {
             lb.removeHost(uri);
             lb.addHost(uri, route);
         });
+        log.infof("Load balancer: enable all nodes. All enabled nodes: %s", lb.toString());
     }
 
     public void disableAllBackendNodes() {
-        log.debugf("Load balancer: disabling all nodes");
         backendNodes.values().forEach(lb::removeHost);
+        log.infof("Load balancer: disabling all nodes");
     }
 
     public void enableBackendNodeByName(String nodeName) {
@@ -122,8 +129,8 @@ public class SimpleUndertowLoadBalancer {
         if (uri == null) {
             throw new IllegalArgumentException("Invalid node: " + nodeName);
         }
-        log.debugf("Load balancer: enabling node %s", nodeName);
         lb.addHost(uri, nodeName);
+        log.infof("Load balancer: enabled node '%s', All enabled nodes: %s", nodeName, lb.toString());
     }
 
     public void disableBackendNodeByName(String nodeName) {
@@ -131,8 +138,8 @@ public class SimpleUndertowLoadBalancer {
         if (uri == null) {
             throw new IllegalArgumentException("Invalid node: " + nodeName);
         }
-        log.debugf("Load balancer: disabling node %s", nodeName);
         lb.removeHost(uri);
+        log.infof("Load balancer: disabled node '%s', All enabled nodes: %s", nodeName, lb.toString());
     }
 
     static Map<String, URI> parseNodes(String nodes) {
@@ -225,6 +232,19 @@ public class SimpleUndertowLoadBalancer {
         }
 
 
+        // For now, overriden just this "addHost" method to avoid adding duplicates
+        @Override
+        public synchronized LoadBalancingProxyClient addHost(URI host, String jvmRoute) {
+            List<String> current = getCurrentHostRoutes();
+            if (current.contains(jvmRoute)) {
+                log.infof("Route '%s' already present. Skip adding", jvmRoute);
+                return this;
+            } else {
+                return super.addHost(host, jvmRoute);
+            }
+        }
+
+
         @Override
         public void getConnection(ProxyTarget target, HttpServerExchange exchange, ProxyCallback<ProxyConnection> callback, long timeout, TimeUnit timeUnit) {
             long timeoutMs = timeUnit.toMillis(timeout);
@@ -233,6 +253,31 @@ public class SimpleUndertowLoadBalancer {
             super.getConnection(target, exchange, callbackDelegate, timeout, timeUnit);
         }
 
+
+        @Override
+        public String toString() {
+            return getCurrentHostRoutes().toString();
+        }
+
+
+        private List<String> getCurrentHostRoutes() {
+            Field hostsField = Reflections.findDeclaredField(LoadBalancingProxyClient.class, "hosts");
+            hostsField.setAccessible(true);
+            Host[] hosts = (Host[]) Reflections.getFieldValue(hostsField, this);
+
+            if (hosts == null) {
+                return Collections.emptyList();
+            }
+
+            List<String> hostRoutes = new LinkedList<>();
+            for (Host host : hosts) {
+                Field hostField = Reflections.findDeclaredField(Host.class, "jvmRoute");
+                hostField.setAccessible(true);
+                String route = Reflections.getFieldValue(hostField, host).toString();
+                hostRoutes.add(route);
+            }
+            return hostRoutes;
+        }
     }
 
 

testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/arquillian/DeploymentTargetModifier.java

@@ -22,6 +22,7 @@ import org.jboss.arquillian.container.spi.client.deployment.TargetDescription;
 import org.jboss.arquillian.container.test.impl.client.deployment.AnnotationDeploymentScenarioGenerator;
 import org.jboss.arquillian.test.spi.TestClass;
 import org.jboss.logging.Logger;
+import org.keycloak.common.util.StringPropertyReplacer;
 
 import java.util.List;
 
@@ -73,12 +74,23 @@ public class DeploymentTargetModifier extends AnnotationDeploymentScenarioGenera
             if (deployment.getTarget() != null) {
                 String containerQualifier = deployment.getTarget().getName();
                 if (AUTH_SERVER_CURRENT.equals(containerQualifier)) {
-                    String authServerQualifier = AuthServerTestEnricher.AUTH_SERVER_CONTAINER;
+                    String newAuthServerQualifier = AuthServerTestEnricher.AUTH_SERVER_CONTAINER;
-                    log.infof("Setting target container for deployment %s.%s: %s", testClass.getName(), deployment.getName(), authServerQualifier);
+                    updateAuthServerQualifier(deployment, testClass, newAuthServerQualifier);
-                    deployment.setTarget(new TargetDescription(authServerQualifier));
+                } else {
+                    String newAuthServerQualifier = StringPropertyReplacer.replaceProperties(containerQualifier);
+                    if (!newAuthServerQualifier.equals(containerQualifier)) {
+                        updateAuthServerQualifier(deployment, testClass, newAuthServerQualifier);
+                    }
                 }
+
+
             }
         }
     }
 
+    private void updateAuthServerQualifier(DeploymentDescription deployment, TestClass testClass, String newAuthServerQualifier) {
+        log.infof("Setting target container for deployment %s.%s: %s", testClass.getName(), deployment.getName(), newAuthServerQualifier);
+        deployment.setTarget(new TargetDescription(newAuthServerQualifier));
+    }
+
 }

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/crossdc/AbstractAdminCrossDCTest.java

@@ -95,8 +95,10 @@ public abstract class AbstractAdminCrossDCTest extends AbstractCrossDCTest {
             T newStat = (T) stats.getSingleStatistics(key);
 
             Matcher<? super T> matcherInstance = matcherOnOldStat.apply(oldStat);
+            
+            log.infof("assertSingleStatistics '%s' : oldStat: %s, newStat: %s", key, oldStat.toString(), newStat.toString());
             assertThat(newStat, matcherInstance);
-        }, 20, 200);
+        }, 50, 200);
     }
 
     protected void assertStatistics(InfinispanStatistics stats, Runnable testedCode, BiConsumer<Map<String, Object>, Map<String, Object>> assertionOnStats) {
@@ -108,7 +110,7 @@ public abstract class AbstractAdminCrossDCTest extends AbstractCrossDCTest {
         Retry.execute(() -> {
             Map<String, Object> newStat = stats.getStatistics();
             assertionOnStats.accept(oldStat, newStat);
-        }, 5, 200);
+        }, 50, 200);
     }
 
 }

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/crossdc/AbstractCrossDCTest.java

@@ -50,8 +50,8 @@ public abstract class AbstractCrossDCTest extends AbstractTestRealmKeycloakTest
 
     // Keep the following constants in sync with arquillian
     public static final String QUALIFIER_NODE_BALANCER = "auth-server-balancer-cross-dc";
-    public static final String QUALIFIER_JBOSS_DC_0_NODE_1 = "auth-server-jboss-cross-dc-0_1";
+    public static final String QUALIFIER_AUTH_SERVER_DC_0_NODE_1 = "auth-server-${node.name}-cross-dc-0_1";
-    public static final String QUALIFIER_JBOSS_DC_1_NODE_1 = "auth-server-jboss-cross-dc-1_1";
+    public static final String QUALIFIER_AUTH_SERVER_DC_1_NODE_1 = "auth-server-${node.name}-cross-dc-1_1";
 
     @ArquillianResource
     @LoadBalancer(value = QUALIFIER_NODE_BALANCER)
@@ -215,7 +215,9 @@ public abstract class AbstractCrossDCTest extends AbstractTestRealmKeycloakTest
     public void disableDcOnLoadBalancer(DC dc) {
         int dcIndex = dc.ordinal();
         log.infof("Disabling load balancer for dc=%d", dcIndex);
-        this.suiteContext.getDcAuthServerBackendsInfo().get(dcIndex).forEach(c -> loadBalancerCtrl.disableBackendNodeByName(c.getQualifier()));
+        this.suiteContext.getDcAuthServerBackendsInfo().get(dcIndex).forEach(containerInfo -> {
+            loadBalancerCtrl.disableBackendNodeByName(containerInfo.getQualifier());
+        });
     }
 
     /**
@@ -231,7 +233,9 @@ public abstract class AbstractCrossDCTest extends AbstractTestRealmKeycloakTest
         } else {
             dcNodes.stream()
               .filter(ContainerInfo::isStarted)
-              .forEach(c -> loadBalancerCtrl.enableBackendNodeByName(c.getQualifier()));
+              .forEach(containerInfo -> {
+                  loadBalancerCtrl.enableBackendNodeByName(containerInfo.getQualifier());
+              });
         }
     }
 

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/crossdc/ActionTokenCrossDCTest.java

@@ -42,6 +42,8 @@ import org.keycloak.testsuite.arquillian.annotation.JmxInfinispanChannelStatisti
 import org.keycloak.testsuite.arquillian.InfinispanStatistics;
 import org.keycloak.testsuite.arquillian.InfinispanStatistics.Constants;
 import org.keycloak.testsuite.pages.ProceedPage;
+
+import java.util.Map;
 import java.util.concurrent.TimeUnit;
 import org.hamcrest.Matchers;
 import static org.hamcrest.Matchers.greaterThan;
@@ -104,8 +106,6 @@ public class ActionTokenCrossDCTest extends AbstractAdminCrossDCTest {
 
         String link = MailUtils.getPasswordResetEmailLink(message);
 
-        Retry.execute(() -> channelStatisticsCrossDc.reset(), 3, 100);
-
         assertSingleStatistics(cacheDc0Node0Statistics, Constants.STAT_CACHE_NUMBER_OF_ENTRIES,
           () -> driver.navigate().to(link),
           Matchers::is
@@ -115,10 +115,21 @@ public class ActionTokenCrossDCTest extends AbstractAdminCrossDCTest {
         proceedPage.clickProceedLink();
         passwordUpdatePage.assertCurrent();
 
-        // Verify that there was at least one message sent via the channel
+        // Verify that there was at least one message sent via the channel - Even if we did the change on DC0, the message may be sent either from DC0 or DC1. Seems it depends on the actionTokens key ownership.
-        assertSingleStatistics(channelStatisticsCrossDc, Constants.STAT_CHANNEL_SENT_MESSAGES,
+        // In case that it was sent from DC1, we will receive it in DC0.
-          () -> passwordUpdatePage.changePassword("new-pass", "new-pass"),
+        assertStatistics(channelStatisticsCrossDc,
-          old -> greaterThan((Comparable) 0l)
+                () -> {
+                    passwordUpdatePage.changePassword("new-pass", "new-pass");
+                },
+                (Map<String, Object> oldStats, Map<String, Object> newStats) -> {
+                    int oldSent = ((Number) oldStats.get(Constants.STAT_CHANNEL_SENT_MESSAGES)).intValue();
+                    int newSent = ((Number) newStats.get(Constants.STAT_CHANNEL_SENT_MESSAGES)).intValue();
+                    int oldReceived = ((Number) oldStats.get(Constants.STAT_CHANNEL_RECEIVED_MESSAGES)).intValue();
+                    int newReceived = ((Number) newStats.get(Constants.STAT_CHANNEL_RECEIVED_MESSAGES)).intValue();
+
+                    log.infof("oldSent: %d, newSent: %d, oldReceived: %d, newReceived: %d", oldSent, newSent, oldReceived, newReceived);
+                    Assert.assertTrue(newSent - oldSent > 0 || newReceived - oldReceived > 0);
+                }
         );
 
         assertEquals("Your account has been updated.", driver.getTitle());

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/crossdc/BruteForceCrossDCTest.java

@@ -52,7 +52,7 @@ public class BruteForceCrossDCTest extends AbstractAdminCrossDCTest {
     private static final String REALM_NAME = "brute-force-test";
     
     @Deployment(name = "dc0")
-    @TargetsContainer(QUALIFIER_JBOSS_DC_0_NODE_1)
+    @TargetsContainer(QUALIFIER_AUTH_SERVER_DC_0_NODE_1)
     public static WebArchive deployDC0() {
         return RunOnServerDeployment.create(
                 BruteForceCrossDCTest.class,
@@ -64,7 +64,7 @@ public class BruteForceCrossDCTest extends AbstractAdminCrossDCTest {
     }
     
     @Deployment(name = "dc1")
-    @TargetsContainer(QUALIFIER_JBOSS_DC_1_NODE_1)
+    @TargetsContainer(QUALIFIER_AUTH_SERVER_DC_1_NODE_1)
     public static WebArchive deployDC1() {
         return RunOnServerDeployment.create(
                 BruteForceCrossDCTest.class,
@@ -248,7 +248,6 @@ public class BruteForceCrossDCTest extends AbstractAdminCrossDCTest {
     }
 
 
-    // TODO Having this working on Wildfly might be a challenge. Maybe require @Deployment with @TargetsContainer descriptor generated at runtime as we don't know the container qualifier at compile time... Maybe workaround by add endpoint to TestingResourceProvider if needed..
     // resolution on Wildfly: make deployment available on both dc0_1 and dc1_1, see @Deployment methods
     private void addUserLoginFailure(KeycloakTestingClient testingClient) throws URISyntaxException, IOException {
         testingClient.server().run(session -> {