From 9b691e9bb7d715ef8bbd4b541bd59fbb2b935955 Mon Sep 17 00:00:00 2001 From: Andre Ponert Date: Thu, 5 Aug 2021 17:12:04 +0200 Subject: [PATCH 1/2] MODULES-8373 Fix mysql_grant resource to be idempodent on MySQL 8+ When the MySQL module is used against MySQL8, mysql_grant is no more idempodent. This is because MySQL 8+ no more returns "ALL" for SHOW GRANTS, when all privileges are granted. Instead, the separate privileges are shown. Unfortunately, the tests were just adapted to not error in this situation. This commit aims to fix the issue and also fixes the tests. --- lib/puppet/provider/mysql_grant/mysql.rb | 6 ++- spec/acceptance/00_mysql_server_spec.rb | 2 +- spec/acceptance/01_mysql_db_spec.rb | 4 ++ spec/acceptance/04_mysql_backup_spec.rb | 4 ++ spec/acceptance/types/mysql_database_spec.rb | 7 +-- spec/acceptance/types/mysql_grant_spec.rb | 45 ++------------------ spec/spec_helper_acceptance.rb | 7 +++ 7 files changed, 29 insertions(+), 46 deletions(-) diff --git a/lib/puppet/provider/mysql_grant/mysql.rb b/lib/puppet/provider/mysql_grant/mysql.rb index d26b0fb8d..157f8ed93 100644 --- a/lib/puppet/provider/mysql_grant/mysql.rb +++ b/lib/puppet/provider/mysql_grant/mysql.rb @@ -45,6 +45,10 @@ def self.instances (priv == 'ALL PRIVILEGES') ? 'ALL' : priv.strip end end + sorted_privileges = stripped_privileges.sort + if self.newer_than('mysql' => '8.0.0') + sorted_privileges = (sorted_privileges == ['ALTER', 'ALTER ROUTINE', 'CREATE', 'CREATE ROLE', 'CREATE ROUTINE', 'CREATE TABLESPACE', 'CREATE TEMPORARY TABLES', 'CREATE USER', 'CREATE VIEW', 'DELETE', 'DROP', 'DROP ROLE', 'EVENT', 'EXECUTE', 'FILE', 'INDEX', 'INSERT', 'LOCK TABLES', 'PROCESS', 'REFERENCES', 'RELOAD', 'REPLICATION CLIENT', 'REPLICATION SLAVE', 'SELECT', 'SHOW DATABASES', 'SHOW VIEW', 'SHUTDOWN', 'SUPER', 'TRIGGER', 'UPDATE']) ? ['ALL'] : sorted_privileges + end # Same here, but to remove OPTION leaving just GRANT. options = if %r{WITH\sGRANT\sOPTION}.match?(rest) ['GRANT'] @@ -57,7 +61,7 @@ def self.instances instances << new( name: "#{user}@#{host}/#{table}", ensure: :present, - privileges: stripped_privileges.sort, + privileges: sorted_privileges, table: table, user: "#{user}@#{host}", options: options, diff --git a/spec/acceptance/00_mysql_server_spec.rb b/spec/acceptance/00_mysql_server_spec.rb index e1538bd2d..b4c63c935 100644 --- a/spec/acceptance/00_mysql_server_spec.rb +++ b/spec/acceptance/00_mysql_server_spec.rb @@ -39,7 +39,7 @@ class { 'mysql::server': databases => { 'somedb' => { ensure => 'present', - charset => 'utf8', + charset => #{$charset}, }, } } diff --git a/spec/acceptance/01_mysql_db_spec.rb b/spec/acceptance/01_mysql_db_spec.rb index 15930a62f..25af5fcf9 100644 --- a/spec/acceptance/01_mysql_db_spec.rb +++ b/spec/acceptance/01_mysql_db_spec.rb @@ -4,6 +4,7 @@ describe 'mysql::db define' do describe 'creating a database' do + let(:pp) do <<-MANIFEST class { 'mysql::server': @@ -14,6 +15,7 @@ class { 'mysql::server': mysql::db { 'spec1': user => 'root1', password => 'password', + charset => #{$charset}, } MANIFEST end @@ -42,6 +44,7 @@ class { 'mysql::server': override_options => { 'root_password' => 'password' } } user => 'root1', password => 'password', sql => '/tmp/spec.sql', + charset => #{$charset}, } MANIFEST end @@ -66,6 +69,7 @@ class { 'mysql::server': override_options => { 'root_password' => 'password' } } user => 'root1', password => 'password', dbname => 'realdb', + charset => #{$charset}, } MANIFEST end diff --git a/spec/acceptance/04_mysql_backup_spec.rb b/spec/acceptance/04_mysql_backup_spec.rb index 34b4e456b..7a002641e 100644 --- a/spec/acceptance/04_mysql_backup_spec.rb +++ b/spec/acceptance/04_mysql_backup_spec.rb @@ -12,6 +12,7 @@ class { 'mysql::server': root_password => 'password' } ]: user => 'backup', password => 'secret', + charset => #{$charset}, } class { 'mysql::server::backup': @@ -72,6 +73,7 @@ class { 'mysql::server': root_password => 'password' } ]: user => 'backup', password => 'secret', + charset => #{$charset}, } class { 'mysql::server::backup': @@ -136,6 +138,7 @@ class { 'mysql::server': root_password => 'password' } ]: user => 'backup', password => 'secret', + charset => #{$charset}, } case $facts['os']['family'] { /Debian/: { @@ -260,6 +263,7 @@ class { 'mysql::server': root_password => 'password' } ]: user => 'backup', password => 'secret', + charset => #{$charset}, } case $facts['os']['family'] { /Debian/: { diff --git a/spec/acceptance/types/mysql_database_spec.rb b/spec/acceptance/types/mysql_database_spec.rb index 03579fe3d..47cfb946b 100644 --- a/spec/acceptance/types/mysql_database_spec.rb +++ b/spec/acceptance/types/mysql_database_spec.rb @@ -15,7 +15,8 @@ class { 'mysql::server': } describe 'creating database' do pp = <<-MANIFEST mysql_database { 'spec_db': - ensure => present, + ensure => present, + charset => #{$charset}, } MANIFEST it 'works without errors' do @@ -37,7 +38,7 @@ class { 'mysql::server': } collate => 'latin1_swedish_ci', } mysql_database { 'spec_utf8': - charset => 'utf8', + charset => #{$charset}, collate => 'utf8_general_ci', } MANIFEST @@ -54,7 +55,7 @@ class { 'mysql::server': } it 'finds utf8 db #stdout' do run_shell("mysql -NBe \"SHOW VARIABLES LIKE '%_database'\" spec_utf8") do |r| - expect(r.stdout).to match(%r{^character_set_database\tutf8\ncollation_database\tutf8_general_ci$}) + expect(r.stdout).to match(%r{^character_set_database\tutf8(mb3)?\ncollation_database\tutf8_general_ci$}) expect(r.stderr).to be_empty end end diff --git a/spec/acceptance/types/mysql_grant_spec.rb b/spec/acceptance/types/mysql_grant_spec.rb index 0318a553a..00c71678f 100644 --- a/spec/acceptance/types/mysql_grant_spec.rb +++ b/spec/acceptance/types/mysql_grant_spec.rb @@ -268,51 +268,13 @@ class { 'mysql::server': end end - # On Ubuntu 20.04 'ALL' now returns as the sum of it's constitute parts and so require a specific test - describe 'ALL privilege on newer MySQL versions', if: os[:family] == 'ubuntu' && os[:release] =~ %r{^20\.04} do - pp_one = <<-MANIFEST - mysql_user { 'all@localhost': - ensure => present, - } - mysql_grant { 'all@localhost/*.*': - user => 'all@localhost', - privileges => ['ALL'], - table => '*.*', - require => Mysql_user['all@localhost'], - } - MANIFEST - it "create ['ALL'] privs" do - apply_manifest(pp_one, catch_failures: true) - end - - pp_two = <<-MANIFEST - mysql_user { 'all@localhost': - ensure => present, - } - mysql_grant { 'all@localhost/*.*': - user => 'all@localhost', - privileges => ['ALTER', 'ALTER ROUTINE', 'CREATE', 'CREATE ROLE', 'CREATE ROUTINE', 'CREATE TABLESPACE', 'CREATE TEMPORARY TABLES', 'CREATE USER', 'CREATE VIEW', 'DELETE', 'DROP', 'DROP ROLE', 'EVENT', 'EXECUTE', 'FILE', 'INDEX', 'INSERT', 'LOCK TABLES', 'PROCESS', 'REFERENCES', 'RELOAD', 'REPLICATION CLIENT', 'REPLICATION SLAVE', 'SELECT', 'SHOW DATABASES', 'SHOW VIEW', 'SHUTDOWN', 'SUPER', 'TRIGGER', 'UPDATE'], - table => '*.*', - require => Mysql_user['all@localhost'], - } - MANIFEST - it "create ['ALL'] constitute parts privs" do - apply_manifest(pp_two, catch_changes: true) - end - end - describe 'complex test' do - # On Ubuntu 20.04 'ALL' now returns as the sum of it's constitute parts and so is no longer idempotent when set - privileges = if os[:family] == 'ubuntu' && os[:release] =~ %r{^20\.04} - "['SELECT', 'INSERT', 'UPDATE']" - else - "['ALL']" - end pp = <<-MANIFEST $dbSubnet = '10.10.10.%' mysql_database { 'foo': - ensure => present, + ensure => present, + charset => '#{$charset}', } exec { 'mysql-create-table': @@ -325,7 +287,7 @@ class { 'mysql::server': Mysql_grant { ensure => present, options => ['GRANT'], - privileges => #{privileges}, + privileges => ['ALL'], table => '*.*', require => [ Mysql_database['foo'], Exec['mysql-create-table'] ], } @@ -724,6 +686,7 @@ class { 'mysql::server': override_options => { 'root_password' => 'password' } } user => 'root1', password => 'password', sql => '/tmp/grant_spec_table.sql', + charset => #{$charset}, } MANIFEST it 'creates table' do diff --git a/spec/spec_helper_acceptance.rb b/spec/spec_helper_acceptance.rb index 4ac8d7e0f..fe32e2839 100644 --- a/spec/spec_helper_acceptance.rb +++ b/spec/spec_helper_acceptance.rb @@ -4,3 +4,10 @@ require 'spec_helper_acceptance_local' if File.file?(File.join(File.dirname(__FILE__), 'spec_helper_acceptance_local.rb')) PuppetLitmus.configure! + +# On Ubuntu 20.04 'utf8' charset now sets 'utf8mb3' internally and breaks idempotence +$charset = if os[:family] == 'ubuntu' && os[:release] =~ %r{^20\.04} + "utf8mb3" + else + "utf8" + end From 76e901ffc0732a34bbed827e1bfb6e0c6ab96236 Mon Sep 17 00:00:00 2001 From: Andre Ponert Date: Mon, 9 Aug 2021 14:59:31 +0200 Subject: [PATCH 2/2] Some refactoring to pass rubocup tests --- lib/puppet/provider/mysql_grant/mysql.rb | 7 +++++-- spec/acceptance/00_mysql_server_spec.rb | 2 +- spec/acceptance/01_mysql_db_spec.rb | 7 +++---- spec/acceptance/04_mysql_backup_spec.rb | 8 ++++---- spec/acceptance/types/mysql_database_spec.rb | 4 ++-- spec/acceptance/types/mysql_grant_spec.rb | 4 ++-- spec/spec_helper_acceptance.rb | 7 ------- spec/spec_helper_acceptance_local.rb | 8 ++++++++ 8 files changed, 25 insertions(+), 22 deletions(-) diff --git a/lib/puppet/provider/mysql_grant/mysql.rb b/lib/puppet/provider/mysql_grant/mysql.rb index 157f8ed93..b3e97b4e0 100644 --- a/lib/puppet/provider/mysql_grant/mysql.rb +++ b/lib/puppet/provider/mysql_grant/mysql.rb @@ -46,8 +46,11 @@ def self.instances end end sorted_privileges = stripped_privileges.sort - if self.newer_than('mysql' => '8.0.0') - sorted_privileges = (sorted_privileges == ['ALTER', 'ALTER ROUTINE', 'CREATE', 'CREATE ROLE', 'CREATE ROUTINE', 'CREATE TABLESPACE', 'CREATE TEMPORARY TABLES', 'CREATE USER', 'CREATE VIEW', 'DELETE', 'DROP', 'DROP ROLE', 'EVENT', 'EXECUTE', 'FILE', 'INDEX', 'INSERT', 'LOCK TABLES', 'PROCESS', 'REFERENCES', 'RELOAD', 'REPLICATION CLIENT', 'REPLICATION SLAVE', 'SELECT', 'SHOW DATABASES', 'SHOW VIEW', 'SHUTDOWN', 'SUPER', 'TRIGGER', 'UPDATE']) ? ['ALL'] : sorted_privileges + if newer_than('mysql' => '8.0.0') && sorted_privileges == ['ALTER', 'ALTER ROUTINE', 'CREATE', 'CREATE ROLE', 'CREATE ROUTINE', 'CREATE TABLESPACE', 'CREATE TEMPORARY TABLES', 'CREATE USER', + 'CREATE VIEW', 'DELETE', 'DROP', 'DROP ROLE', 'EVENT', 'EXECUTE', 'FILE', 'INDEX', 'INSERT', 'LOCK TABLES', 'PROCESS', 'REFERENCES', + 'RELOAD', 'REPLICATION CLIENT', 'REPLICATION SLAVE', 'SELECT', 'SHOW DATABASES', 'SHOW VIEW', 'SHUTDOWN', 'SUPER', 'TRIGGER', + 'UPDATE'] + sorted_privileges = ['ALL'] end # Same here, but to remove OPTION leaving just GRANT. options = if %r{WITH\sGRANT\sOPTION}.match?(rest) diff --git a/spec/acceptance/00_mysql_server_spec.rb b/spec/acceptance/00_mysql_server_spec.rb index b4c63c935..1da8281ba 100644 --- a/spec/acceptance/00_mysql_server_spec.rb +++ b/spec/acceptance/00_mysql_server_spec.rb @@ -39,7 +39,7 @@ class { 'mysql::server': databases => { 'somedb' => { ensure => 'present', - charset => #{$charset}, + charset => #{fetch_charset}, }, } } diff --git a/spec/acceptance/01_mysql_db_spec.rb b/spec/acceptance/01_mysql_db_spec.rb index 25af5fcf9..04586fe2a 100644 --- a/spec/acceptance/01_mysql_db_spec.rb +++ b/spec/acceptance/01_mysql_db_spec.rb @@ -4,7 +4,6 @@ describe 'mysql::db define' do describe 'creating a database' do - let(:pp) do <<-MANIFEST class { 'mysql::server': @@ -15,7 +14,7 @@ class { 'mysql::server': mysql::db { 'spec1': user => 'root1', password => 'password', - charset => #{$charset}, + charset => #{fetch_charset}, } MANIFEST end @@ -44,7 +43,7 @@ class { 'mysql::server': override_options => { 'root_password' => 'password' } } user => 'root1', password => 'password', sql => '/tmp/spec.sql', - charset => #{$charset}, + charset => #{fetch_charset}, } MANIFEST end @@ -69,7 +68,7 @@ class { 'mysql::server': override_options => { 'root_password' => 'password' } } user => 'root1', password => 'password', dbname => 'realdb', - charset => #{$charset}, + charset => #{fetch_charset}, } MANIFEST end diff --git a/spec/acceptance/04_mysql_backup_spec.rb b/spec/acceptance/04_mysql_backup_spec.rb index 7a002641e..d46fe771a 100644 --- a/spec/acceptance/04_mysql_backup_spec.rb +++ b/spec/acceptance/04_mysql_backup_spec.rb @@ -12,7 +12,7 @@ class { 'mysql::server': root_password => 'password' } ]: user => 'backup', password => 'secret', - charset => #{$charset}, + charset => #{fetch_charset}, } class { 'mysql::server::backup': @@ -73,7 +73,7 @@ class { 'mysql::server': root_password => 'password' } ]: user => 'backup', password => 'secret', - charset => #{$charset}, + charset => #{fetch_charset}, } class { 'mysql::server::backup': @@ -138,7 +138,7 @@ class { 'mysql::server': root_password => 'password' } ]: user => 'backup', password => 'secret', - charset => #{$charset}, + charset => #{fetch_charset}, } case $facts['os']['family'] { /Debian/: { @@ -263,7 +263,7 @@ class { 'mysql::server': root_password => 'password' } ]: user => 'backup', password => 'secret', - charset => #{$charset}, + charset => #{fetch_charset}, } case $facts['os']['family'] { /Debian/: { diff --git a/spec/acceptance/types/mysql_database_spec.rb b/spec/acceptance/types/mysql_database_spec.rb index 47cfb946b..247eb57df 100644 --- a/spec/acceptance/types/mysql_database_spec.rb +++ b/spec/acceptance/types/mysql_database_spec.rb @@ -16,7 +16,7 @@ class { 'mysql::server': } pp = <<-MANIFEST mysql_database { 'spec_db': ensure => present, - charset => #{$charset}, + charset => #{fetch_charset}, } MANIFEST it 'works without errors' do @@ -38,7 +38,7 @@ class { 'mysql::server': } collate => 'latin1_swedish_ci', } mysql_database { 'spec_utf8': - charset => #{$charset}, + charset => #{fetch_charset}, collate => 'utf8_general_ci', } MANIFEST diff --git a/spec/acceptance/types/mysql_grant_spec.rb b/spec/acceptance/types/mysql_grant_spec.rb index 00c71678f..c61cbb373 100644 --- a/spec/acceptance/types/mysql_grant_spec.rb +++ b/spec/acceptance/types/mysql_grant_spec.rb @@ -274,7 +274,7 @@ class { 'mysql::server': mysql_database { 'foo': ensure => present, - charset => '#{$charset}', + charset => '#{fetch_charset}', } exec { 'mysql-create-table': @@ -686,7 +686,7 @@ class { 'mysql::server': override_options => { 'root_password' => 'password' } } user => 'root1', password => 'password', sql => '/tmp/grant_spec_table.sql', - charset => #{$charset}, + charset => #{fetch_charset}, } MANIFEST it 'creates table' do diff --git a/spec/spec_helper_acceptance.rb b/spec/spec_helper_acceptance.rb index fe32e2839..4ac8d7e0f 100644 --- a/spec/spec_helper_acceptance.rb +++ b/spec/spec_helper_acceptance.rb @@ -4,10 +4,3 @@ require 'spec_helper_acceptance_local' if File.file?(File.join(File.dirname(__FILE__), 'spec_helper_acceptance_local.rb')) PuppetLitmus.configure! - -# On Ubuntu 20.04 'utf8' charset now sets 'utf8mb3' internally and breaks idempotence -$charset = if os[:family] == 'ubuntu' && os[:release] =~ %r{^20\.04} - "utf8mb3" - else - "utf8" - end diff --git a/spec/spec_helper_acceptance_local.rb b/spec/spec_helper_acceptance_local.rb index 2c1071121..267f5556f 100644 --- a/spec/spec_helper_acceptance_local.rb +++ b/spec/spec_helper_acceptance_local.rb @@ -32,6 +32,14 @@ def export_locales LitmusHelper.instance.run_shell('source ~/.bashrc') end +def fetch_charset + @charset ||= if os[:family] == 'ubuntu' && os[:release] =~ %r{^20\.04} + 'utf8mb3' + else + 'utf8' + end +end + RSpec.configure do |c| c.before :suite do if os[:family] == 'debian' || os[:family] == 'ubuntu'