Skip creating sessions cache when Persistent Sessions is enabled

Re-order the configuration steps to avoid redundant warnings

Closes #32416

Signed-off-by: Pedro Ruivo <pruivo@redhat.com>
Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
Co-authored-by: Alexander Schwartz <aschwart@redhat.com>

model/infinispan/src/main/java/org/keycloak/connections/infinispan/DefaultInfinispanConnectionProviderFactory.java

@@ -65,6 +65,12 @@ import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.A
 import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.AUTHORIZATION_REVISIONS_CACHE_NAME;
 import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.CLIENT_SESSION_CACHE_NAME;
 import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.CLUSTERED_CACHE_NAMES;
+import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.JGROUPS_BIND_ADDR;
+import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.JGROUPS_UDP_MCAST_ADDR;
+import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.JMX_DOMAIN;
+import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.KEYS_CACHE_DEFAULT_MAX;
+import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.KEYS_CACHE_MAX_IDLE_SECONDS;
+import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.KEYS_CACHE_NAME;
 import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.LOGIN_FAILURE_CACHE_NAME;
 import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.OFFLINE_CLIENT_SESSION_CACHE_NAME;
 import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.OFFLINE_USER_SESSION_CACHE_NAME;
@@ -76,6 +82,7 @@ import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.U
 import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.USER_REVISIONS_CACHE_NAME;
 import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.USER_SESSION_CACHE_NAME;
 import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.WORK_CACHE_NAME;
+import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.skipSessionsCacheIfRequired;
 import static org.keycloak.connections.infinispan.InfinispanUtil.configureTransport;
 import static org.keycloak.connections.infinispan.InfinispanUtil.createCacheConfigurationBuilder;
 import static org.keycloak.connections.infinispan.InfinispanUtil.getActionTokenCacheConfig;
@@ -244,7 +251,7 @@ public class DefaultInfinispanConnectionProviderFactory implements InfinispanCon
         RemoteCacheManager remoteCacheManager = new RemoteCacheManager(builder.build());
 
         // establish connection to all caches
-        Arrays.stream(CLUSTERED_CACHE_NAMES).forEach(remoteCacheManager::getCache);
+        skipSessionsCacheIfRequired(Arrays.stream(CLUSTERED_CACHE_NAMES)).forEach(remoteCacheManager::getCache);
         return remoteCacheManager;
 
     }
@@ -256,7 +263,7 @@ public class DefaultInfinispanConnectionProviderFactory implements InfinispanCon
         defineRevisionCache(cacheManager, USER_CACHE_NAME, USER_REVISIONS_CACHE_NAME, USER_REVISIONS_CACHE_DEFAULT_MAX);
         defineRevisionCache(cacheManager, AUTHORIZATION_CACHE_NAME, AUTHORIZATION_REVISIONS_CACHE_NAME, AUTHORIZATION_REVISIONS_CACHE_DEFAULT_MAX);
 
-        cacheManager.getCache(InfinispanConnectionProvider.KEYS_CACHE_NAME, true);
+        cacheManager.getCache(KEYS_CACHE_NAME, true);
 
         this.topologyInfo = new TopologyInfo(cacheManager, config, false, getId());
 
@@ -275,14 +282,14 @@ public class DefaultInfinispanConnectionProviderFactory implements InfinispanCon
         this.topologyInfo = new TopologyInfo(cacheManager, config, true, getId());
 
         if (clustered) {
-            String jgroupsUdpMcastAddr = config.get("jgroupsUdpMcastAddr", System.getProperty(InfinispanConnectionProvider.JGROUPS_UDP_MCAST_ADDR));
+            String jgroupsUdpMcastAddr = config.get("jgroupsUdpMcastAddr", System.getProperty(JGROUPS_UDP_MCAST_ADDR));
-            String jgroupsBindAddr = config.get("jgroupsBindAddr", System.getProperty(InfinispanConnectionProvider.JGROUPS_BIND_ADDR));
+            String jgroupsBindAddr = config.get("jgroupsBindAddr", System.getProperty(JGROUPS_BIND_ADDR));
             configureTransport(gcb, topologyInfo.getMyNodeName(), topologyInfo.getMySiteName(), jgroupsUdpMcastAddr, jgroupsBindAddr,
                     "default-configs/default-keycloak-jgroups-udp.xml");
             gcb.jmx()
-              .domain(InfinispanConnectionProvider.JMX_DOMAIN + "-" + topologyInfo.getMyNodeName()).enable();
+              .domain(JMX_DOMAIN + "-" + topologyInfo.getMyNodeName()).enable();
         } else {
-            gcb.jmx().domain(InfinispanConnectionProvider.JMX_DOMAIN).enable();
+            gcb.jmx().domain(JMX_DOMAIN).enable();
         }
 
         Marshalling.configure(gcb);
@@ -308,8 +315,8 @@ public class DefaultInfinispanConnectionProviderFactory implements InfinispanCon
         defineLocalCache(cacheManager, AUTHORIZATION_CACHE_NAME, AUTHORIZATION_REVISIONS_CACHE_NAME, localConfiguration, AUTHORIZATION_REVISIONS_CACHE_DEFAULT_MAX);
         defineLocalCache(cacheManager, USER_CACHE_NAME, USER_REVISIONS_CACHE_NAME, localConfiguration, USER_REVISIONS_CACHE_DEFAULT_MAX);
 
-        cacheManager.defineConfiguration(InfinispanConnectionProvider.KEYS_CACHE_NAME, getKeysCacheConfig());
+        cacheManager.defineConfiguration(KEYS_CACHE_NAME, getKeysCacheConfig());
-        cacheManager.getCache(InfinispanConnectionProvider.KEYS_CACHE_NAME, true);
+        cacheManager.getCache(KEYS_CACHE_NAME, true);
 
         var builder = createCacheConfigurationBuilder();
         if (clustered) {
@@ -459,9 +466,9 @@ public class DefaultInfinispanConnectionProviderFactory implements InfinispanCon
 
         cb.memory()
                 .whenFull(EvictionStrategy.REMOVE)
-                .maxCount(InfinispanConnectionProvider.KEYS_CACHE_DEFAULT_MAX);
+                .maxCount(KEYS_CACHE_DEFAULT_MAX);
 
-        cb.expiration().maxIdle(InfinispanConnectionProvider.KEYS_CACHE_MAX_IDLE_SECONDS, TimeUnit.SECONDS);
+        cb.expiration().maxIdle(KEYS_CACHE_MAX_IDLE_SECONDS, TimeUnit.SECONDS);
 
         return cb.build();
     }

model/infinispan/src/main/java/org/keycloak/connections/infinispan/InfinispanConnectionProvider.java

@@ -21,11 +21,13 @@ import java.util.Arrays;
 import java.util.concurrent.CompletionStage;
 import java.util.concurrent.Executor;
 import java.util.concurrent.ScheduledExecutorService;
+import java.util.function.Predicate;
 import java.util.stream.Stream;
 
 import org.infinispan.Cache;
 import org.infinispan.client.hotrod.RemoteCache;
 import org.infinispan.util.concurrent.BlockingManager;
+import org.keycloak.common.util.MultiSiteUtils;
 import org.keycloak.models.KeycloakSessionFactory;
 import org.keycloak.provider.Provider;
 
@@ -177,4 +179,15 @@ public interface InfinispanConnectionProvider extends Provider {
      */
     BlockingManager getBlockingManager();
 
+    static Stream<String> skipSessionsCacheIfRequired(Stream<String> caches) {
+        if (!MultiSiteUtils.isPersistentSessionsEnabled()) {
+            return caches;
+        }
+        // persistent-user-sessions enabled, we do not need the sessions caches from external Infinispan.
+        return caches
+                .filter(Predicate.isEqual(USER_SESSION_CACHE_NAME).negate())
+                .filter(Predicate.isEqual(OFFLINE_USER_SESSION_CACHE_NAME).negate())
+                .filter(Predicate.isEqual(CLIENT_SESSION_CACHE_NAME).negate())
+                .filter(Predicate.isEqual(OFFLINE_CLIENT_SESSION_CACHE_NAME).negate());
+    }
 }

model/infinispan/src/main/java/org/keycloak/connections/infinispan/remote/RemoteInfinispanConnectionProvider.java

@@ -33,6 +33,8 @@ import org.infinispan.util.concurrent.BlockingManager;
 import org.keycloak.connections.infinispan.InfinispanConnectionProvider;
 import org.keycloak.connections.infinispan.TopologyInfo;
 
+import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.skipSessionsCacheIfRequired;
+
 public record RemoteInfinispanConnectionProvider(EmbeddedCacheManager embeddedCacheManager,
                                                  RemoteCacheManager remoteCacheManager,
                                                  TopologyInfo topologyInfo) implements InfinispanConnectionProvider {
@@ -63,7 +65,7 @@ public record RemoteInfinispanConnectionProvider(EmbeddedCacheManager embeddedCa
         // Only the CacheStore (persistence) stores data in binary format and needs to be deleted.
         // We assume rolling-upgrade between KC 25 and KC 26 is not available, in other words, KC 25 and KC 26 servers are not present in the same cluster.
         var stage = CompletionStages.aggregateCompletionStage();
-        Arrays.stream(CLUSTERED_CACHE_NAMES)
+        skipSessionsCacheIfRequired(Arrays.stream(CLUSTERED_CACHE_NAMES))
                 .map(this::getRemoteCache)
                 .map(RemoteCache::clearAsync)
                 .forEach(stage::dependsOn);

model/infinispan/src/main/java/org/keycloak/connections/infinispan/remote/RemoteLoadBalancerCheckProviderFactory.java

@@ -17,6 +17,16 @@
 
 package org.keycloak.connections.infinispan.remote;
 
+import java.lang.invoke.MethodHandles;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CompletionStage;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import java.util.function.BiFunction;
+import java.util.stream.Collectors;
+
 import org.infinispan.client.hotrod.impl.InternalRemoteCache;
 import org.infinispan.client.hotrod.impl.operations.PingResponse;
 import org.infinispan.commons.util.concurrent.CompletableFutures;
@@ -36,18 +46,9 @@ import org.keycloak.provider.EnvironmentDependentProviderFactory;
 import org.keycloak.provider.ProviderConfigProperty;
 import org.keycloak.provider.ProviderConfigurationBuilder;
 
-import java.lang.invoke.MethodHandles;
-import java.util.Arrays;
-import java.util.List;
-import java.util.concurrent.Callable;
-import java.util.concurrent.CompletionStage;
-import java.util.concurrent.ScheduledFuture;
-import java.util.concurrent.TimeUnit;
-import java.util.function.BiFunction;
-import java.util.stream.Collectors;
-
 import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.CLUSTERED_CACHE_NAMES;
 import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.LOCAL_CACHE_NAMES;
+import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.skipSessionsCacheIfRequired;
 
 public class RemoteLoadBalancerCheckProviderFactory implements LoadBalancerCheckProviderFactory, EnvironmentDependentProviderFactory {
 
@@ -87,7 +88,7 @@ public class RemoteLoadBalancerCheckProviderFactory implements LoadBalancerCheck
             }
             this.connectionProvider = provider;
 
-            var remoteCacheChecks = Arrays.stream(CLUSTERED_CACHE_NAMES)
+            var remoteCacheChecks = skipSessionsCacheIfRequired(Arrays.stream(CLUSTERED_CACHE_NAMES))
                     .map(s -> new RemoteCacheCheck(s, provider))
                     .collect(Collectors.toList());
             var sequencer = new ActionSequencer(connectionProvider.getExecutor("load-balancer-check"), false, null);
@@ -205,8 +206,9 @@ public class RemoteLoadBalancerCheckProviderFactory implements LoadBalancerCheck
 
         @Override
         public Void apply(PingResponse response, Throwable throwable) {
-            logger.debugf("Received Ping response for cache '%s'. Success=%s, Throwable=%s", name, response.isSuccess(), throwable);
+            var successPing = response != null && response.isSuccess();
-            isDown = throwable != null || !response.isSuccess();
+            logger.debugf("Received Ping response for cache '%s'. Success=%s, Throwable=%s", name, successPing, throwable);
+            isDown = throwable != null || !successPing;
             return null;
         }
     }

quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/storage/legacy/infinispan/CacheManagerFactory.java

@@ -26,6 +26,7 @@ import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
+import java.util.stream.Stream;
 
 import io.micrometer.core.instrument.Metrics;
 import org.infinispan.client.hotrod.RemoteCache;
@@ -84,6 +85,7 @@ import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.L
 import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.OFFLINE_CLIENT_SESSION_CACHE_NAME;
 import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.OFFLINE_USER_SESSION_CACHE_NAME;
 import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.USER_SESSION_CACHE_NAME;
+import static org.keycloak.connections.infinispan.InfinispanConnectionProvider.skipSessionsCacheIfRequired;
 import static org.wildfly.security.sasl.util.SaslMechanismInformation.Names.SCRAM_SHA_512;
 
 public class CacheManagerFactory {
@@ -177,7 +179,7 @@ public class CacheManagerFactory {
 
         // establish connection to all caches
         if (isStartEagerly()) {
-            Arrays.stream(CLUSTERED_CACHE_NAMES).forEach(remoteCacheManager::getCache);
+            skipSessionsCacheIfRequired(Arrays.stream(CLUSTERED_CACHE_NAMES)).forEach(remoteCacheManager::getCache);
         }
         return remoteCacheManager;
     }
@@ -187,7 +189,7 @@ public class CacheManagerFactory {
         logger.warn("Creating remote cache in external Infinispan server. It should not be used in production!");
         var baseConfig = defaultRemoteCacheBuilder().build();
 
-        Arrays.stream(CLUSTERED_CACHE_NAMES)
+        skipSessionsCacheIfRequired(Arrays.stream(CLUSTERED_CACHE_NAMES))
                 .forEach(name -> builder.remoteCache(name).configuration(baseConfig.toStringConfiguration(name)));
     }
 
@@ -267,36 +269,6 @@ public class CacheManagerFactory {
         logger.info("Starting Infinispan embedded cache manager");
         ConfigurationBuilderHolder builder = new ParserRegistry().parse(config);
 
-        if (builder.getNamedConfigurationBuilders().entrySet().stream().anyMatch(c -> c.getValue().clustering().cacheMode().isClustered())) {
-            configureTransportStack(builder);
-            configureRemoteStores(builder);
-        }
-
-        Arrays.stream(CLUSTERED_CACHE_NAMES).forEach(cacheName -> {
-            if (cacheName.equals(USER_SESSION_CACHE_NAME) || cacheName.equals(CLIENT_SESSION_CACHE_NAME) || cacheName.equals(OFFLINE_USER_SESSION_CACHE_NAME) || cacheName.equals(OFFLINE_CLIENT_SESSION_CACHE_NAME)) {
-                ConfigurationBuilder configurationBuilder = builder.getNamedConfigurationBuilders().get(cacheName);
-                if (MultiSiteUtils.isPersistentSessionsEnabled()) {
-                    if (configurationBuilder.memory().maxCount() == -1) {
-                        logger.infof("Persistent user sessions enabled and no memory limit found in configuration. Setting max entries for %s to 10000 entries.", cacheName);
-                        configurationBuilder.memory().maxCount(10000);
-                    }
-                    /* The number of owners for these caches then need to be set to `1` to avoid backup owners with inconsistent data.
-                     As primary owner evicts a key based on its locally evaluated maxCount setting, it wouldn't tell the backup owner about this, and then the backup owner would be left with a soon-to-be-outdated key.
-                     While a `remove` is forwarded to the backup owner regardless if the key exists on the primary owner, a `computeIfPresent` is not, and it would leave a backup owner with an outdated key.
-                     With the number of owners set to `1`, there will be no backup owners, so this is the setting to choose with persistent sessions enabled to ensure consistent data in the caches. */
-                    configurationBuilder.clustering().hash().numOwners(1);
-                } else {
-                    if (configurationBuilder.memory().maxCount() != -1) {
-                        logger.warnf("Persistent user sessions NOT enabled and memory limit found in configuration for cache %s. This might be a misconfiguration!", cacheName);
-                    }
-                    if (configurationBuilder.clustering().hash().attributes().attribute(HashConfiguration.NUM_OWNERS).get() == 1
-                        && configurationBuilder.persistence().stores().isEmpty()) {
-                        logger.warnf("Number of owners is one for cache %s, and no persistence is configured. This might be a misconfiguration as you will lose data when a single node is restarted!", cacheName);
-                    }
-                }
-            }
-        });
-
         if (Configuration.isTrue(MetricsOptions.METRICS_ENABLED)) {
             builder.getGlobalConfigurationBuilder().addModule(MicrometerMeterRegisterConfigurationBuilder.class);
             builder.getGlobalConfigurationBuilder().module(MicrometerMeterRegisterConfigurationBuilder.class).meterRegistry(Metrics.globalRegistry);
@@ -322,12 +294,19 @@ public class CacheManagerFactory {
                      .findFirst();
 
                if (remoteStore.isPresent())
-                  logger.warnf("remote-store configuration detected for cache '%s'. Explicit cache configuration ignored when using '%s' Feature", cacheName, Profile.Feature.REMOTE_CACHE.getKey());
+                  logger.warnf("remote-store configuration detected for cache '%s'. Explicit cache configuration ignored when using '%s' or '%s' Features.", cacheName, Profile.Feature.REMOTE_CACHE.getKey(), Profile.Feature.MULTI_SITE.getKey());
                builders.remove(cacheName);
             }
             // Disable JGroups, not required when the data is stored in the Remote Cache.
             // The existing caches are local and do not require JGroups to work properly.
             builder.getGlobalConfigurationBuilder().nonClusteredDefault();
+        } else {
+            // embedded mode!
+            if (builder.getNamedConfigurationBuilders().entrySet().stream().anyMatch(c -> c.getValue().clustering().cacheMode().isClustered())) {
+                configureTransportStack(builder);
+                configureRemoteStores(builder);
+            }
+            configureSessionsCaches(builder);
         }
 
         var start = isStartEagerly();
@@ -367,7 +346,7 @@ public class CacheManagerFactory {
         return Integer.getInteger("kc.cache-ispn-start-timeout", 120);
     }
 
-    private void configureTransportStack(ConfigurationBuilderHolder builder) {
+    private static void configureTransportStack(ConfigurationBuilderHolder builder) {
         String transportStack = Configuration.getRawValue("kc.cache-stack");
 
         var transportConfig = builder.getGlobalConfigurationBuilder().transport();
@@ -392,7 +371,7 @@ public class CacheManagerFactory {
         }
     }
 
-    private void validateTlsAvailable(GlobalConfiguration config) {
+    private static void validateTlsAvailable(GlobalConfiguration config) {
         var stackName = config.transport().stack();
         if (stackName == null) {
             // unable to validate
@@ -410,7 +389,7 @@ public class CacheManagerFactory {
 
     }
 
-    private void configureRemoteStores(ConfigurationBuilderHolder builder) {
+    private static void configureRemoteStores(ConfigurationBuilderHolder builder) {
         //if one of remote store command line parameters is defined, some other are required, otherwise assume it'd configured via xml only
         if (Configuration.getOptionalKcValue(CACHE_REMOTE_HOST_PROPERTY).isPresent()) {
 
@@ -463,6 +442,32 @@ public class CacheManagerFactory {
         }
     }
 
+    private static void configureSessionsCaches(ConfigurationBuilderHolder builder) {
+        Stream.of(USER_SESSION_CACHE_NAME, CLIENT_SESSION_CACHE_NAME, OFFLINE_USER_SESSION_CACHE_NAME, OFFLINE_CLIENT_SESSION_CACHE_NAME)
+                .forEach(cacheName -> {
+                    var configurationBuilder = builder.getNamedConfigurationBuilders().get(cacheName);
+                    if (MultiSiteUtils.isPersistentSessionsEnabled()) {
+                        if (configurationBuilder.memory().maxCount() == -1) {
+                            logger.infof("Persistent user sessions enabled and no memory limit found in configuration. Setting max entries for %s to 10000 entries.", cacheName);
+                            configurationBuilder.memory().maxCount(10000);
+                        }
+                        /* The number of owners for these caches then need to be set to `1` to avoid backup owners with inconsistent data.
+                         As primary owner evicts a key based on its locally evaluated maxCount setting, it wouldn't tell the backup owner about this, and then the backup owner would be left with a soon-to-be-outdated key.
+                         While a `remove` is forwarded to the backup owner regardless if the key exists on the primary owner, a `computeIfPresent` is not, and it would leave a backup owner with an outdated key.
+                         With the number of owners set to `1`, there will be no backup owners, so this is the setting to choose with persistent sessions enabled to ensure consistent data in the caches. */
+                        configurationBuilder.clustering().hash().numOwners(1);
+                    } else {
+                        if (configurationBuilder.memory().maxCount() != -1) {
+                            logger.warnf("Persistent user sessions NOT enabled and memory limit found in configuration for cache %s. This might be a misconfiguration!", cacheName);
+                        }
+                        if (configurationBuilder.clustering().hash().attributes().attribute(HashConfiguration.NUM_OWNERS).get() == 1
+                                && configurationBuilder.persistence().stores().isEmpty()) {
+                            logger.warnf("Number of owners is one for cache %s, and no persistence is configured. This might be a misconfiguration as you will lose data when a single node is restarted!", cacheName);
+                        }
+                    }
+                });
+    }
+
     private static String requiredStringProperty(String propertyName) {
         return Configuration.getOptionalKcValue(propertyName).orElseThrow(() -> new RuntimeException("Property " + propertyName + " required but not specified"));
     }