Skip to content

Commit

Permalink
Merge pull request #4699 from gchq/gh-4693-session-leak
Browse files Browse the repository at this point in the history 
PR for #4693 - sessionList is very slow v7.5 on a cluster
  • Loading branch information
@at055612
at055612 authored Jan 15, 2025
2 parents 636a739 + 21d5e01 commit 9c46f4a
Show file tree
Hide file tree
Showing 14 changed files with 324 additions and 100 deletions.
92 changes: 58 additions & 34 deletions stroom-app/src/main/java/stroom/app/App.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
import stroom.config.app.AppConfig;
import stroom.config.app.Config;
import stroom.config.app.SecurityConfig;
import stroom.config.app.SessionConfig;
import stroom.config.app.SessionCookieConfig;
import stroom.config.app.StroomYamlUtil;
import stroom.config.global.impl.ConfigMapper;
import stroom.dropwizard.common.AdminServlets;
Expand Down Expand Up @@ -54,7 +56,9 @@
import stroom.util.logging.LambdaLoggerFactory;
import stroom.util.logging.LogUtil;
import stroom.util.shared.AbstractConfig;
import stroom.util.shared.ModelStringUtil;
import stroom.util.shared.ResourcePaths;
import stroom.util.time.StroomDuration;
import stroom.util.validation.ValidationModule;
import stroom.util.yaml.YamlUtil;

Expand All @@ -70,14 +74,14 @@
import jakarta.inject.Inject;
import jakarta.servlet.DispatcherType;
import jakarta.servlet.FilterRegistration;
import jakarta.servlet.SessionCookieConfig;
import jakarta.validation.ValidatorFactory;
import org.eclipse.jetty.server.session.SessionHandler;
import org.eclipse.jetty.servlets.CrossOriginFilter;

import java.io.IOException;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.time.Duration;
import java.util.EnumSet;
import java.util.Objects;

Expand Down Expand Up @@ -220,15 +224,14 @@ public void run(final Config configuration, final Environment environment) {
// We want Stroom to use the root path so we need to move Dropwizard's path.
environment.jersey().setUrlPattern(ResourcePaths.API_ROOT_PATH + "/*");

// Set up a session handler for Jetty
configureSessionHandling(environment);

// Ensure the session cookie that provides JSESSIONID is secure.
// Need to get it from ConfigMapper not AppConfig as ConfigMapper is now the source of
// truth for config.
// Need to get these config classed from ConfigMapper as the main appInjector is not created yet
// and configuration only holds the YAML view of the config, not the DB view.
final ConfigMapper configMapper = bootStrapInjector.getInstance(ConfigMapper.class);
final stroom.config.app.SessionCookieConfig sessionCookieConfig = configMapper.getConfigObject(
stroom.config.app.SessionCookieConfig.class);
final SessionCookieConfig sessionCookieConfig = configMapper.getConfigObject(SessionCookieConfig.class);
final SessionConfig sessionConfig = configMapper.getConfigObject(SessionConfig.class);

// Set up a session handler for Jetty
configureSessionHandling(environment, sessionConfig);
configureSessionCookie(environment, sessionCookieConfig);

// Configure Cross-Origin Resource Sharing.
Expand Down Expand Up @@ -273,11 +276,11 @@ public void run(final Config configuration, final Environment environment) {

private void showNodeInfo(final Config configuration) {
LOGGER.info(""
+ "\n********************************************************************************"
+ "\n Stroom home: " + homeDirProvider.get().toAbsolutePath().normalize()
+ "\n Stroom temp: " + tempDirProvider.get().toAbsolutePath().normalize()
+ "\n Node name: " + getNodeName(configuration.getYamlAppConfig())
+ "\n********************************************************************************");
+ "\n********************************************************************************"
+ "\n Stroom home: " + homeDirProvider.get().toAbsolutePath().normalize()
+ "\n Stroom temp: " + tempDirProvider.get().toAbsolutePath().normalize()
+ "\n Node name: " + getNodeName(configuration.getYamlAppConfig())
+ "\n********************************************************************************");
}

private void warnAboutDefaultOpenIdCreds(final Config configuration, final Injector injector) {
Expand All @@ -299,17 +302,17 @@ private void warnAboutDefaultOpenIdCreds(final Config configuration, final Injec
.getFullPathStr(AbstractOpenIdConfig.PROP_NAME_IDP_TYPE);

LOGGER.warn("\n" +
"\n -----------------------------------------------------------------------------" +
"\n " +
"\n WARNING!" +
"\n " +
"\n Using default and publicly available Open ID authentication credentials. " +
"\n This is insecure! These should only be used in test/demo environments. " +
"\n Set " + propPath + " to INTERNAL_IDP/EXTERNAL_IDP for production environments." +
"\n" +
"\n " + defaultOpenIdCredentials.getApiKey() +
"\n -----------------------------------------------------------------------------" +
"\n");
"\n -----------------------------------------------------------------------------" +
"\n " +
"\n WARNING!" +
"\n " +
"\n Using default and publicly available Open ID authentication credentials. " +
"\n This is insecure! These should only be used in test/demo environments. " +
"\n Set " + propPath + " to INTERNAL_IDP/EXTERNAL_IDP for production environments." +
"\n" +
"\n " + defaultOpenIdCredentials.getApiKey() +
"\n -----------------------------------------------------------------------------" +
"\n");
}
}

Expand Down Expand Up @@ -344,35 +347,56 @@ private void validateAppConfig(final Config config, final Path configFile) {

if (result.hasErrors() && appConfig.isHaltBootOnConfigValidationFailure()) {
LOGGER.error("Application configuration is invalid. Stopping Stroom. To run Stroom with invalid " +
"configuration, set {} to false, however this is not advised!",
"configuration, set {} to false, however this is not advised!",
appConfig.getFullPathStr(AppConfig.PROP_NAME_HALT_BOOT_ON_CONFIG_VALIDATION_FAILURE));
System.exit(1);
}
}

private static void configureSessionHandling(final Environment environment) {
SessionHandler sessionHandler = new SessionHandler();
private void configureSessionHandling(final Environment environment,
final SessionConfig sessionConfig) {

final SessionHandler sessionHandler = new SessionHandler();
// We need to give our session cookie a name other than JSESSIONID, otherwise it might
// clash with other services running on the same domain.
sessionHandler.setSessionCookie(SESSION_COOKIE_NAME);
long maxInactiveIntervalSecs = NullSafe.getOrElse(
sessionConfig.getMaxInactiveInterval(),
StroomDuration::getDuration,
Duration::toSeconds,
-1L);
if (maxInactiveIntervalSecs > Integer.MAX_VALUE) {
maxInactiveIntervalSecs = -1;
}
LOGGER.info("Setting session maxInactiveInterval to {} secs ({})",
ModelStringUtil.formatCsv(maxInactiveIntervalSecs),
(maxInactiveIntervalSecs > 0
? Duration.ofSeconds(maxInactiveIntervalSecs).toString()
: String.valueOf(maxInactiveIntervalSecs)));

// If we don't let sessions expire then the Map of HttpSession in SessionListListener
// will grow and grow
sessionHandler.setMaxInactiveInterval((int) maxInactiveIntervalSecs);

environment.servlets().setSessionHandler(sessionHandler);
environment.jersey().register(SessionFactoryProvider.class);
}

private static void configureSessionCookie(final Environment environment,
final stroom.config.app.SessionCookieConfig config) {
private void configureSessionCookie(final Environment environment,
final SessionCookieConfig sessionCookieConfig) {
// Ensure the session cookie that provides JSESSIONID is secure.
final SessionCookieConfig sessionCookieConfig = environment
final jakarta.servlet.SessionCookieConfig servletSessionCookieConfig = environment
.getApplicationContext()
.getServletContext()
.getSessionCookieConfig();
sessionCookieConfig.setSecure(config.isSecure());
sessionCookieConfig.setHttpOnly(config.isHttpOnly());
servletSessionCookieConfig.setSecure(sessionCookieConfig.isSecure());
servletSessionCookieConfig.setHttpOnly(sessionCookieConfig.isHttpOnly());
// TODO : Add `SameSite=Strict` when supported by JEE
}

private static void configureCors(io.dropwizard.core.setup.Environment environment) {
FilterRegistration.Dynamic cors = environment.servlets().addFilter("CORS", CrossOriginFilter.class);
final FilterRegistration.Dynamic cors = environment.servlets()
.addFilter("CORS", CrossOriginFilter.class);
cors.addMappingForUrlPatterns(EnumSet.allOf(DispatcherType.class), true, "/*");
cors.setInitParameter(CrossOriginFilter.ALLOWED_METHODS_PARAM, "GET,PUT,POST,DELETE,OPTIONS,PATCH");
cors.setInitParameter(CrossOriginFilter.ALLOWED_ORIGINS_PARAM, "*");
Expand Down
32 changes: 22 additions & 10 deletions stroom-config/stroom-config-app/src/main/java/stroom/config/app/AppConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ public class AppConfig extends AbstractConfig implements IsStroomConfig {
public static final String PROP_NAME_SECURITY = "security";
public static final String PROP_NAME_SERVICE_DISCOVERY = "serviceDiscovery";
public static final String PROP_NAME_SESSION_COOKIE = "sessionCookie";
public static final String PROP_NAME_SESSION = "session";
public static final String PROP_NAME_SOLR = "solr";
public static final String PROP_NAME_STATE = "state";
public static final String PROP_NAME_STATISTICS = "statistics";
Expand Down Expand Up @@ -161,6 +162,7 @@ public class AppConfig extends AbstractConfig implements IsStroomConfig {
private final SecurityConfig securityConfig;
private final ServiceDiscoveryConfig serviceDiscoveryConfig;
private final SessionCookieConfig sessionCookieConfig;
private final SessionConfig sessionConfig;
private final SolrConfig solrConfig;
private final StateConfig stateConfig;
private final StatisticsConfig statisticsConfig;
Expand Down Expand Up @@ -211,6 +213,7 @@ public AppConfig() {
new SecurityConfig(),
new ServiceDiscoveryConfig(),
new SessionCookieConfig(),
new SessionConfig(),
new SolrConfig(),
new StateConfig(),
new StatisticsConfig(),
Expand Down Expand Up @@ -260,6 +263,7 @@ public AppConfig(@JsonProperty(PROP_NAME_HALT_BOOT_ON_CONFIG_VALIDATION_FAILURE)
@JsonProperty(PROP_NAME_SECURITY) final SecurityConfig securityConfig,
@JsonProperty(PROP_NAME_SERVICE_DISCOVERY) final ServiceDiscoveryConfig serviceDiscoveryConfig,
@JsonProperty(PROP_NAME_SESSION_COOKIE) final SessionCookieConfig sessionCookieConfig,
@JsonProperty(PROP_NAME_SESSION) final SessionConfig sessionConfig,
@JsonProperty(PROP_NAME_SOLR) final SolrConfig solrConfig,
@JsonProperty(PROP_NAME_STATE) final StateConfig stateConfig,
@JsonProperty(PROP_NAME_STATISTICS) final StatisticsConfig statisticsConfig,
Expand Down Expand Up @@ -305,6 +309,7 @@ public AppConfig(@JsonProperty(PROP_NAME_HALT_BOOT_ON_CONFIG_VALIDATION_FAILURE)
this.securityConfig = securityConfig;
this.serviceDiscoveryConfig = serviceDiscoveryConfig;
this.sessionCookieConfig = sessionCookieConfig;
this.sessionConfig = sessionConfig;
this.solrConfig = solrConfig;
this.stateConfig = stateConfig;
this.statisticsConfig = statisticsConfig;
Expand All @@ -317,11 +322,12 @@ public AppConfig(@JsonProperty(PROP_NAME_HALT_BOOT_ON_CONFIG_VALIDATION_FAILURE)

@AssertTrue(
message = "stroom." + PROP_NAME_HALT_BOOT_ON_CONFIG_VALIDATION_FAILURE + " is set to false. If there is " +
"invalid configuration the system may behave in unexpected ways. This setting is not advised.",
"invalid configuration the system may behave in unexpected ways. This setting is not advised.",
payload = ValidationSeverity.Warning.class)
@JsonProperty(PROP_NAME_HALT_BOOT_ON_CONFIG_VALIDATION_FAILURE)
@JsonPropertyDescription("If true, Stroom will halt on start up if any errors are found in the YAML " +
"configuration file. If false, the errors will simply be logged. Setting this to false is not advised.")
"configuration file. If false, the errors will simply be logged." +
"Setting this to false is not advised.")
public boolean isHaltBootOnConfigValidationFailure() {
return haltBootOnConfigValidationFailure;
}
Expand Down Expand Up @@ -363,8 +369,9 @@ public ClusterLockConfig getClusterLockConfig() {

@JsonProperty(PROP_NAME_COMMON_DB_DETAILS)
@JsonPropertyDescription("Defines a set of common database connection details to use if no connection details " +
"are defined for a service area in stroom, e.g. core or config. This means you can have all service " +
"areas running in a single database, have each in their own database or a mixture.")
"are defined for a service area in stroom, e.g. core or config. This means you can " +
"have all service areas running in a single database, have each in their own " +
"database or a mixture.")
public CommonDbConfig getCommonDbConfig() {
return commonDbConfig;
}
Expand Down Expand Up @@ -448,10 +455,10 @@ public NodeConfig getNodeConfig() {
}

@JsonPropertyDescription("This is the base endpoint of the node for all inter-node communications, " +
"i.e. all cluster management and node info calls. " +
"This endpoint will typically be hidden behind a firewall and not be publicly available. " +
"The address must be resolvable from all other nodes in the cluster. " +
"This does not need to be set for a single node cluster.")
"i.e. all cluster management and node info calls. " +
"This endpoint will typically be hidden behind a firewall and not be " +
"publicly available. The address must be resolvable from all other nodes " +
"in the cluster. This does not need to be set for a single node cluster.")
@JsonProperty(PROP_NAME_NODE_URI)
public NodeUriConfig getNodeUri() {
return nodeUri;
Expand Down Expand Up @@ -479,7 +486,7 @@ public PropertyServiceConfig getPropertyServiceConfig() {
}

@JsonPropertyDescription("This is public facing URI of stroom which may be different from the local host if " +
"behind a proxy")
"behind a proxy")
@JsonProperty(PROP_NAME_PUBLIC_URI)
public PublicUriConfig getPublicUri() {
return publicUri;
Expand Down Expand Up @@ -537,6 +544,11 @@ public SessionCookieConfig getSessionCookieConfig() {
return sessionCookieConfig;
}

@JsonProperty(PROP_NAME_SESSION)
public SessionConfig getSessionConfig() {
return sessionConfig;
}

@JsonProperty(PROP_NAME_STATE)
@JsonPropertyDescription("Configuration for the stroom state service")
public StateConfig getStateConfig() {
Expand All @@ -555,7 +567,7 @@ public UiConfig getUiConfig() {
}

@JsonPropertyDescription("This is the URI where the UI is hosted if different to the public facing URI of the " +
"server, e.g. during development or some other deployments")
"server, e.g. during development or some other deployments")
@JsonProperty(PROP_NAME_UI_URI)
public UiUriConfig getUiUri() {
return uiUri;
Expand Down
51 changes: 51 additions & 0 deletions stroom-config/stroom-config-app/src/main/java/stroom/config/app/SessionConfig.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
package stroom.config.app;

import stroom.util.config.annotations.RequiresRestart;
import stroom.util.config.annotations.RequiresRestart.RestartScope;
import stroom.util.shared.AbstractConfig;
import stroom.util.shared.IsStroomConfig;
import stroom.util.time.StroomDuration;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonPropertyDescription;
import com.fasterxml.jackson.annotation.JsonPropertyOrder;


@JsonPropertyOrder(alphabetic = true)
public class SessionConfig extends AbstractConfig implements IsStroomConfig {

public static final String PROP_NAME_MAX_INACTIVE_INTERVAL = "maxInactiveInterval";

public static final StroomDuration DEFAULT_MAX_INACTIVE_INTERVAL = StroomDuration.ofDays(1);

private final StroomDuration maxInactiveInterval;

public SessionConfig() {
this.maxInactiveInterval = DEFAULT_MAX_INACTIVE_INTERVAL;
}

@SuppressWarnings("unused")
@JsonCreator
public SessionConfig(
@JsonProperty(PROP_NAME_MAX_INACTIVE_INTERVAL) final StroomDuration maxInactiveInterval) {
this.maxInactiveInterval = maxInactiveInterval;
}

@RequiresRestart(RestartScope.UI)
@JsonProperty(PROP_NAME_MAX_INACTIVE_INTERVAL)
@JsonPropertyDescription("The maximum time interval between the last access of a HTTP session and " +
"it being considered expired. Set to null for sessions that never expire, " +
"however this will causes sessions to be held and build up in memory indefinitely, " +
"so is best avoided.")
public StroomDuration getMaxInactiveInterval() {
return maxInactiveInterval;
}

@Override
public String toString() {
return "SessionConfig{" +
"maxInactiveInterval=" + maxInactiveInterval +
'}';
}
}
2 changes: 2 additions & 0 deletions stroom-config/stroom-config-app/src/test/resources/stroom/config/app/expected.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -926,6 +926,8 @@ appConfig:
servicesPort: 8080
zookeeperBasePath: "/stroom-services"
zookeeperUrl: "localhost:2181"
session:
maxInactiveInterval: "P1D"
sessionCookie:
httpOnly: true
secure: true
Expand Down
Loading

0 comments on commit 9c46f4a

Please sign in to comment.