Skip to content

Add support for redis sentinel#763

Open
emilioah wants to merge 7 commits intocatalyst:MOODLE_39_STABLEfrom
emilioah:MOODLE_39_STABLE_redissentinel
Open

Add support for redis sentinel#763
emilioah wants to merge 7 commits intocatalyst:MOODLE_39_STABLEfrom
emilioah:MOODLE_39_STABLE_redissentinel

Conversation

@emilioah
Copy link
Copy Markdown

@emilioah emilioah commented Jun 5, 2023

Quick changes to support redis sentinel.

Configuration in moodle/config.php:

$CFG->auth_saml2_store = '\auth_saml2\redis_store';
// OPT 1: Single redis server
$CFG->auth_saml2_redis_server = '{{ moodle_config.redishost }}'; # Required for the redis_store above
// OPT 2: Redis Sentinel
$CFG->auth_saml2_redissentinel_servers = '1.1.1.1:26397,2.2.2.2:26397,3.3.3.3:26397';
$CFG->auth_saml2_redissentinel_group = 'rediscluster';

@danmarsden
Copy link
Copy Markdown
Member

I know the existing code pollutes the global $CFG to store auth_saml2 specific settings, but we should really migrate those to config_plugins table instead if possible, although we might not block merging based on that for now... I'll see if someone here can PR this properly - thanks!

@matt-catalyst
Copy link
Copy Markdown

SimpleSAML has builtin support for Redis Sentinel - could it be utilised by this patch?

https://simplesamlphp.org/docs/stable/simplesamlphp-maintenance.html#configuring-redis-storage

if (empty($CFG->auth_saml2_redis_server)) {
throw new \coding_exception('Redis connection string is not configured in $CFG->auth_saml2_redis_server');
}
if (!empty($CFG->auth_saml2_redissentinel_servers) && !empty($CFG->auth_saml2_redissentinel_group)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These really need to be plugin scope config items not globals please, its a very simple change. Get the config using get_config and set it in config.php like:

$CFG->forced_plugin_settings['auth_saml2']['foobar'] = 0;

This also needs to be documented somewhere like the readme file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants