Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Validate structure #753

Merged
merged 4 commits into from Apr 30, 2021
Merged

Validate structure #753

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1b84528
Reject configs with unrecognized sections and properties
mattias-p Apr 16, 2021
dd900c6
Validate required properties
mattias-p Apr 19, 2021
24593d3
Match documented and implemented defaults in example config
mattias-p Apr 30, 2021
b16962a
Comment out more parameters
mattias-p Apr 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
150 changes: 117 additions & 33 deletions lib/Zonemaster/Backend/Config.pm
View file
Open in desktop
Expand Up @@ -76,7 +76,20 @@ Emits a log warning with a deprecation message for each deprecated property that
is present.

Throws an exception if the given configuration contains errors.
Unrecognized sections and properties are silently ignored.

In a valid config file:

=over 4

=item

all required properties are present, and

=item

all sections and properties are recognized.

=back

=cut

Expand All @@ -88,9 +101,26 @@ sub parse {
my $ini = Config::IniFiles->new( -file => \$text )
or die "Failed to parse config: " . join( '; ', @Config::IniFiles::errors ) . "\n";

my $get_and_clear = sub { # Read and clear a property from a Config::IniFiles object.
my ( $section, $param ) = @_;
my $value = $ini->val( $section, $param );
$ini->delval( $section, $param );
return $value;
};

# Validate section names
{
my %sections = map { $_ => 1 } ( 'DB', 'MYSQL', 'POSTGRESQL', 'SQLITE', 'LANGUAGE', 'PUBLIC PROFILES', 'PRIVATE PROFILES', 'ZONEMASTER' );
for my $section ( $ini->Sections ) {
if ( !exists $sections{$section} ) {
die "config: unrecognized section: $section\n";
}
}
}

# Validate, normalize, and apply default values
{
my $engine = $ini->val( 'DB', 'engine' );
my $engine = $get_and_clear->( 'DB', 'engine' );
eval {
$engine = $obj->check_db($engine);
};
Expand All @@ -100,25 +130,25 @@ sub parse {
$obj->{_DB_engine} = $engine;
}

$obj->{_DB_polling_interval} = $ini->val( 'DB', 'polling_interval', '0.5' );
$obj->{_MYSQL_host} = $ini->val( 'MYSQL', 'host', undef );
$obj->{_MYSQL_user} = $ini->val( 'MYSQL', 'user', undef );
$obj->{_MYSQL_password} = $ini->val( 'MYSQL', 'password', undef );
$obj->{_MYSQL_database} = $ini->val( 'MYSQL', 'database', undef );
$obj->{_POSTGRESQL_host} = $ini->val( 'POSTGRESQL', 'host', undef );
$obj->{_POSTGRESQL_user} = $ini->val( 'POSTGRESQL', 'user', undef );
$obj->{_POSTGRESQL_password} = $ini->val( 'POSTGRESQL', 'password', undef );
$obj->{_POSTGRESQL_database} = $ini->val( 'POSTGRESQL', 'database', undef );
$obj->{_SQLITE_database_file} = $ini->val( 'SQLITE', 'database_file', undef );
$obj->{_ZONEMASTER_max_zonemaster_execution_time} = $ini->val( 'ZONEMASTER', 'max_zonemaster_execution_time', '600' );
$obj->{_ZONEMASTER_maximal_number_of_retries} = $ini->val( 'ZONEMASTER', 'maximal_number_of_retries', '0' );
$obj->{_ZONEMASTER_number_of_processes_for_frontend_testing} = $ini->val( 'ZONEMASTER', 'number_of_processes_for_frontend_testing', '20' );
$obj->{_ZONEMASTER_number_of_processes_for_batch_testing} = $ini->val( 'ZONEMASTER', 'number_of_processes_for_batch_testing', '20' );
$obj->{_ZONEMASTER_lock_on_queue} = $ini->val( 'ZONEMASTER', 'lock_on_queue', '0' );
$obj->{_ZONEMASTER_age_reuse_previous_test} = $ini->val( 'ZONEMASTER', 'age_reuse_previous_test', '600' );
$obj->{_DB_polling_interval} = $get_and_clear->( 'DB', 'polling_interval' ) // '0.5';
$obj->{_MYSQL_host} = $get_and_clear->( 'MYSQL', 'host' ) // undef;
$obj->{_MYSQL_user} = $get_and_clear->( 'MYSQL', 'user' ) // undef;
$obj->{_MYSQL_password} = $get_and_clear->( 'MYSQL', 'password' ) // undef;
$obj->{_MYSQL_database} = $get_and_clear->( 'MYSQL', 'database' ) // undef;
$obj->{_POSTGRESQL_host} = $get_and_clear->( 'POSTGRESQL', 'host' ) // undef;
$obj->{_POSTGRESQL_user} = $get_and_clear->( 'POSTGRESQL', 'user' ) // undef;
$obj->{_POSTGRESQL_password} = $get_and_clear->( 'POSTGRESQL', 'password' ) // undef;
$obj->{_POSTGRESQL_database} = $get_and_clear->( 'POSTGRESQL', 'database' ) // undef;
$obj->{_SQLITE_database_file} = $get_and_clear->( 'SQLITE', 'database_file' ) // undef;
$obj->{_ZONEMASTER_max_zonemaster_execution_time} = $get_and_clear->( 'ZONEMASTER', 'max_zonemaster_execution_time' ) // '600';
$obj->{_ZONEMASTER_maximal_number_of_retries} = $get_and_clear->( 'ZONEMASTER', 'maximal_number_of_retries' ) // '0';
$obj->{_ZONEMASTER_number_of_processes_for_frontend_testing} = $get_and_clear->( 'ZONEMASTER', 'number_of_processes_for_frontend_testing' ) // '20';
$obj->{_ZONEMASTER_number_of_processes_for_batch_testing} = $get_and_clear->( 'ZONEMASTER', 'number_of_processes_for_batch_testing' ) // '20';
$obj->{_ZONEMASTER_lock_on_queue} = $get_and_clear->( 'ZONEMASTER', 'lock_on_queue' ) // '0';
$obj->{_ZONEMASTER_age_reuse_previous_test} = $get_and_clear->( 'ZONEMASTER', 'age_reuse_previous_test' ) // '600';
Copy link
Contributor

Choose a reason for hiding this comment

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

When there are default values in Config.pm the default configuration file should not have the same value.

[ZONEMASTER]
max_zonemaster_execution_time            = 300
number_of_processes_for_frontend_testing = 20
number_of_processes_for_batch_testing    = 20
#age_reuse_previous_test=600

The max_zonemaster_execution_time is different from the Config.pm. I suggest that the configuration file has the same values, but the key is commented, or that they are just removed from the configuration file.

Copy link
Member Author

Choose a reason for hiding this comment

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

The only place I can find the value 300 is in the Travis backend config files. Do you think those must use the default value too?

Copy link
Contributor

@matsduf matsduf Apr 29, 2021 •
edited

Choose a reason for hiding this comment

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

  1. If the default values are set in Config.pm, the keys in the default configuration file should be commented as age_reuse_previous_test is.
  2. In https://github.com/zonemaster/zonemaster-backend/blob/develop/share/backend_config.ini I find the the following:
[ZONEMASTER]
max_zonemaster_execution_time            = 300
number_of_processes_for_frontend_testing = 20
number_of_processes_for_batch_testing    = 20
#age_reuse_previous_test=600

I suggest that default backend_config.ini is updated to:

[ZONEMASTER]
# max_zonemaster_execution_time            = 600
# number_of_processes_for_frontend_testing = 20
# number_of_processes_for_batch_testing    = 20
# age_reuse_previous_test=600


$obj->{_LANGUAGE_locale} = {};
for my $locale_tag ( split /\s+/, $ini->val( 'LANGUAGE', 'locale' ) || 'en_US' ) {
for my $locale_tag ( split /\s+/, $get_and_clear->( 'LANGUAGE', 'locale' ) || 'en_US' ) {
$locale_tag =~ /^[a-z]{2}_[A-Z]{2}$/
or die "Illegal locale tag in LANGUAGE.locale: $locale_tag\n";

Expand All @@ -132,43 +162,49 @@ sub parse {
default => '',
};
for my $name ( $ini->Parameters( 'PUBLIC PROFILES' ) ) {
$obj->{_public_profiles}{lc $name} = $ini->val( 'PUBLIC PROFILES', $name );
$obj->{_public_profiles}{lc $name} = $get_and_clear->( 'PUBLIC PROFILES', $name );
}
$obj->{_private_profiles} = {};
for my $name ( $ini->Parameters( 'PRIVATE PROFILES' ) ) {
$obj->{_private_profiles}{lc $name} = $ini->val( 'PRIVATE PROFILES', $name );
$obj->{_private_profiles}{lc $name} = $get_and_clear->( 'PRIVATE PROFILES', $name );
}

# Check required propertys (part 1/2)
if ( !defined $obj->DB_engine ) {
die "config: missing required property DB.engine\n";
}

# Handle deprecated properties
if ( defined( my $value = $ini->val( 'DB', 'database_host' ) ) ) {
$log->warning( "Use of deprecated config property DB.database_host. Use MYSQL.host or POSTGRESQL.host instead." );
my @warnings;
if ( defined( my $value = $get_and_clear->( 'DB', 'database_host' ) ) ) {
push @warnings, "Use of deprecated config property DB.database_host. Use MYSQL.host or POSTGRESQL.host instead.";

$obj->{_MYSQL_host} = $value
if $obj->DB_engine eq 'MySQL' && !defined $obj->MYSQL_host;

$obj->{_POSTGRESQL_host} = $value
if $obj->DB_engine eq 'PostgreSQL' && !defined $obj->POSTGRESQL_host;
}
if ( defined( my $value = $ini->val( 'DB', 'user' ) ) ) {
$log->warning( "Use of deprecated config property DB.user. Use MYSQL.user or POSTGRESQL.user instead." );
if ( defined( my $value = $get_and_clear->( 'DB', 'user' ) ) ) {
push @warnings, "Use of deprecated config property DB.user. Use MYSQL.user or POSTGRESQL.user instead.";

$obj->{_MYSQL_user} = $value
if $obj->DB_engine eq 'MySQL' && !defined $obj->MYSQL_user;

$obj->{_POSTGRESQL_user} = $value
if $obj->DB_engine eq 'PostgreSQL' && !defined $obj->POSTGRESQL_user;
}
if ( defined( my $value = $ini->val( 'DB', 'password' ) ) ) {
$log->warning( "Use of deprecated config property DB.password. Use MYSQL.password or POSTGRESQL.password instead." );
if ( defined( my $value = $get_and_clear->( 'DB', 'password' ) ) ) {
push @warnings, "Use of deprecated config property DB.password. Use MYSQL.password or POSTGRESQL.password instead.";

$obj->{_MYSQL_password} = $value
if $obj->DB_engine eq 'MySQL' && !defined $obj->MYSQL_password;

$obj->{_POSTGRESQL_password} = $value
if $obj->DB_engine eq 'PostgreSQL' && !defined $obj->POSTGRESQL_password;
}
if ( defined( my $value = $ini->val( 'DB', 'database_name' ) ) ) {
$log->warning( "Use of deprecated config property DB.database_name. Use MYSQL.database, POSTGRESQL.database or SQLITE.database_file instead." );
if ( defined( my $value = $get_and_clear->( 'DB', 'database_name' ) ) ) {
push @warnings, "Use of deprecated config property DB.database_name. Use MYSQL.database, POSTGRESQL.database or SQLITE.database_file instead.";

$obj->{_MYSQL_database} = $value
if $obj->DB_engine eq 'MySQL' && !defined $obj->MYSQL_database;
Expand All @@ -179,19 +215,67 @@ sub parse {
$obj->{_SQLITE_database_file} = $value
if $obj->DB_engine eq 'SQLite' && !defined $obj->SQLITE_database_file;
}
if ( defined( my $value = $ini->val( 'ZONEMASTER', 'number_of_professes_for_frontend_testing' ) ) ) {
$log->warning( "Use of deprecated config property ZONEMASTER.number_of_professes_for_frontend_testing. Use ZONEMASTER.number_of_processes_for_frontend_testing instead." );
if ( defined( my $value = $get_and_clear->( 'ZONEMASTER', 'number_of_professes_for_frontend_testing' ) ) ) {
push @warnings, "Use of deprecated config property ZONEMASTER.number_of_professes_for_frontend_testing. Use ZONEMASTER.number_of_processes_for_frontend_testing instead.";

$obj->{_ZONEMASTER_number_of_processes_for_frontend_testing} = $value
if !defined $obj->NumberOfProcessesForFrontendTesting;
}
if ( defined( my $value = $ini->val( 'ZONEMASTER', 'number_of_professes_for_batch_testing' ) ) ) {
$log->warning( "Use of deprecated config property ZONEMASTER.number_of_professes_for_batch_testing. Use ZONEMASTER.number_of_processes_for_batch_testing instead." );
if ( defined( my $value = $get_and_clear->( 'ZONEMASTER', 'number_of_professes_for_batch_testing' ) ) ) {
push @warnings, "Use of deprecated config property ZONEMASTER.number_of_professes_for_batch_testing. Use ZONEMASTER.number_of_processes_for_batch_testing instead.";

$obj->{_ZONEMASTER_number_of_processes_for_batch_testing} = $value
if !defined $obj->NumberOfProcessesForBatchTesting;
}

# Check unknown property names
my @unrecognized;
for my $section ( $ini->Sections ) {
for my $param ( $ini->Parameters( $section ) ) {
push @unrecognized, "$section.$param";
}
}
if ( @unrecognized ) {
die "config: unrecognized property(s): " . join( ", ", sort @unrecognized ) . "\n";
}

# Check required propertys (part 2/2)
if ( $obj->DB_engine eq 'MySQL' ) {
die "config: missing required property MYSQL.host (required when DB.engine = MySQL and DB.database_host is unset)\n"
if !defined $obj->MYSQL_host;

die "config: missing required property MYSQL.user (required when DB.engine = MySQL and DB.user is unset)\n"
if !defined $obj->MYSQL_user;

die "config: missing required property MYSQL.password (required when DB.engine = MySQL and DB.password is unset)\n"
if !defined $obj->MYSQL_password;

die "config: missing required property MYSQL.database (required when DB.engine = MySQL and DB.database_name is unset)\n"
if !defined $obj->MYSQL_database;
}
elsif ( $obj->DB_engine eq 'PostgreSQL' ) {
die "config: missing required property POSTGRESQL.host (required when DB.engine = PostgreSQL and DB.database_host is unset)\n"
if !defined $obj->POSTGRESQL_host;

die "config: missing required property POSTGRESQL.user (required when DB.engine = PostgreSQL and DB.user is unset)\n"
if !defined $obj->POSTGRESQL_user;

die "config: missing required property POSTGRESQL.password (required when DB.engine = PostgreSQL and DB.password is unset)\n"
if !defined $obj->POSTGRESQL_password;

die "config: missing required property POSTGRESQL.database (required when DB.engine = PostgreSQL and DB.database_name is unset)\n"
if !defined $obj->POSTGRESQL_database;
}
elsif ( $obj->DB_engine eq 'SQLite' ) {
die "config: missing required property SQLITE.database_file (required when DB.engine = SQLite and DB.database_name is unset)\n"
if !defined $obj->SQLITE_database_file;
}

# Emit deprecation warnings
for my $message ( @warnings ) {
$log->warning( $message );
}

return $obj;
}

Expand Down
8 changes: 4 additions & 4 deletions share/backend_config.ini
View file
Open in desktop
Expand Up @@ -21,10 +21,10 @@ database = zonemaster
database_file = /var/lib/zonemaster/db.sqlite

[ZONEMASTER]
max_zonemaster_execution_time = 300
number_of_processes_for_frontend_testing = 20
number_of_processes_for_batch_testing = 20
#age_reuse_previous_test=600
#max_zonemaster_execution_time = 600
#number_of_processes_for_frontend_testing = 20
#number_of_processes_for_batch_testing = 20
#age_reuse_previous_test = 600

# WARNING: The following option is experimental and all edge cases are not fully tested.
# Do not use it (keep the default value "0"), or use it with care.
Expand Down
140 changes: 140 additions & 0 deletions t/config.t
View file
Open in desktop
Expand Up @@ -143,6 +143,34 @@ subtest 'Everything but NoWarnings' => sub {
}
qr/Failed to parse config/, 'die: Invalid INI format';

throws_ok {
my $text = q{
[DB]
engine = Excel

[SQLITE]
databse_file = /var/db/zonemaster.sqlite

[ZNMEOTAESR]
lock_on_queue = 1
};
Zonemaster::Backend::Config->parse( $text );
}
qr{section.*ZNMEOTAESR}, 'die: Invalid section name';

throws_ok {
my $text = q{
[DB]
engine = SQLite
pnlilog_iatnvrel = 0.5

[SQLITE]
database_file = /var/db/zonemaster.sqlite
};
Zonemaster::Backend::Config->parse( $text );
}
qr{property.*pnlilog_iatnvrel}, 'die: Invalid property name';

throws_ok {
my $text = q{
[DB]
Expand All @@ -152,6 +180,118 @@ subtest 'Everything but NoWarnings' => sub {
}
qr/DB\.engine.*Excel/, 'die: Invalid DB.engine value';

throws_ok {
my $text = q{
[DB]
engine = MySQL

[MYSQL]
user = zonemaster_user
password = zonemaster_password
database = zonemaster_database
};
Zonemaster::Backend::Config->parse( $text );
}
qr/MYSQL\.host/, 'die: Missing MYSQL.host value';

throws_ok {
my $text = q{
[DB]
engine = MySQL

[MYSQL]
host = zonemaster-host
password = zonemaster_password
database = zonemaster_database
};
Zonemaster::Backend::Config->parse( $text );
}
qr/MYSQL\.user/, 'die: Missing MYSQL.user value';

throws_ok {
my $text = q{
[DB]
engine = MySQL

[MYSQL]
host = zonemaster-host
user = zonemaster_user
database = zonemaster_database
};
Zonemaster::Backend::Config->parse( $text );
}
qr/MYSQL\.password/, 'die: Missing MYSQL.password value';

throws_ok {
my $text = q{
[DB]
engine = MySQL

[MYSQL]
host = zonemaster-host
user = zonemaster_user
password = zonemaster_password
};
Zonemaster::Backend::Config->parse( $text );
}
qr/MYSQL\.database/, 'die: Missing MYSQL.database value';

throws_ok {
my $text = q{
[DB]
engine = PostgreSQL

[POSTGRESQL]
user = zonemaster_user
password = zonemaster_password
database = zonemaster_database
};
Zonemaster::Backend::Config->parse( $text );
}
qr/POSTGRESQL\.host/, 'die: Missing POSTGRESQL.host value';

throws_ok {
my $text = q{
[DB]
engine = PostgreSQL

[POSTGRESQL]
host = zonemaster-host
password = zonemaster_password
database = zonemaster_database
};
Zonemaster::Backend::Config->parse( $text );
}
qr/POSTGRESQL\.user/, 'die: Missing POSTGRESQL.user value';

throws_ok {
my $text = q{
[DB]
engine = PostgreSQL

[POSTGRESQL]
host = zonemaster-host
user = zonemaster_user
database = zonemaster_database
};
Zonemaster::Backend::Config->parse( $text );
}
qr/POSTGRESQL\.password/, 'die: Missing POSTGRESQL.password value';

throws_ok {
my $text = q{
[DB]
engine = PostgreSQL

[POSTGRESQL]
host = zonemaster-host
user = zonemaster_user
password = zonemaster_password
};
Zonemaster::Backend::Config->parse( $text );
}
qr/POSTGRESQL\.database/, 'die: Missing POSTGRESQL.database value';

throws_ok {
my $text = q{
[DB]
Expand Down