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.

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

Add an option to set a custom SSL pem files directory in test. #1293

Merged
merged 1 commit into from
Jan 31, 2023

Conversation

junaruga
Copy link
Contributor

In the Fedora project, we are running the mysql2 tests on the build environment with a user permission, without root permission and without sudo.

For the change in MariaDB 10.5.18 on Fedora, we need to set up the SSL pem files manually to run the SSL tests.

In this case, we couldn't set up the pem files required to run SSL tests in the /etc/mysql. This custom SSL directory option gives an option to run the SSL tests executed in the environment.

How to use:

$ TEST_RUBY_MYSQL2_SSL_DIR=/tmp/mysql2 \
  bundle exec rake spec

@junaruga
Copy link
Contributor Author

junaruga commented Dec 22, 2022

I can see there is one code style failure in rubocop 1.41.1 on the CI lint test. But I don't think it is related to this PR.

https://github.com/brianmario/mysql2/actions/runs/3758719510/jobs/6387395566#step:4:12

 lib/mysql2/result.rb:2:3: C: Style/Documentation: Missing top-level documentation comment for class Mysql2::Result.
  class Result
  ^^^^^^^^^^^^

The rubocop test was ok with rubocop 1.41.0.
https://github.com/brianmario/mysql2/actions/runs/3751714242/jobs/6373041330#step:3:69

@@ -16,7 +16,7 @@ jobs:
# Fedora latest stable version
- {distro: fedora, image: 'fedora:latest'}
# Fedora development version
- {distro: fedora, image: 'fedora:rawhide'}
- {distro: fedora, image: 'fedora:rawhide', ssl_dir: '/tmp/mysql2'}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the ssl_dir option to test this feature.
You can see the my.cnf is set up with the environment variable.
https://github.com/brianmario/mysql2/actions/runs/3758719509/jobs/6387395753#step:4:85

You can also see the SSL tests are not skipped. If the pem files don't exist, the tests are skipped.
https://github.com/brianmario/mysql2/actions/runs/3758719509/jobs/6387395753#step:4:664

-t \
-e TEST_RUBY_MYSQL2_SSL_DIR="${{ matrix.ssl_dir || '' }}" \
--cap-add=SYS_PTRACE --security-opt seccomp=unconfined \
mysql2
Copy link
Contributor Author

@junaruga junaruga Dec 22, 2022

Choose a reason for hiding this comment

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

I changed the 1 line command to the YAML block syntax, as I saw one YAML syntax error in the one line with adding the -e option. Maybe it's good to time to change to the YAML block syntax for better visibility.

@junaruga
Copy link
Contributor Author

It seems that the failures in MacOS mariadb and mysql CI cases come from the failure of installing a dependency Python. I don't think it is related to this PR.

@junaruga junaruga marked this pull request as ready for review December 22, 2022 15:02
@junaruga
Copy link
Contributor Author

Note that ideally I wanted to use the managed pem files without copying to another directory with the patch below. However I observed an error in the sudo service mysql restart. in only Ubuntu MariaDB 10.3 CI case. And, I couldn't find the reason. So, I didn't do that way.

diff --git a/ci/ssl.sh b/ci/ssl.sh
index e98f27a..663d8b5 100644
--- a/ci/ssl.sh
+++ b/ci/ssl.sh
@@ -2,11 +2,7 @@
 
 set -eux
 
-# Make sure there is an /etc/mysql
-mkdir -p /etc/mysql
-
-# Copy the local certs to /etc/mysql
-cp spec/ssl/*pem /etc/mysql/
+ROOT_DIR="$(pwd)"
 
 # Wherever MySQL configs live, go there (this is for cross-platform)
 cd $(my_print_defaults --help | grep my.cnf | xargs find 2>/dev/null | xargs dirname)
@@ -14,7 +10,7 @@ cd $(my_print_defaults --help | grep my.cnf | xargs find 2>/dev/null | xargs dir
 # Put the configs into the server
 echo "
 [mysqld]
-ssl-ca=/etc/mysql/ca-cert.pem
-ssl-cert=/etc/mysql/server-cert.pem
-ssl-key=/etc/mysql/server-key.pem
+ssl-ca=${ROOT_DIR}/spec/ssl/ca-cert.pem
+ssl-cert=${ROOT_DIR}/spec/ssl/server-cert.pem
+ssl-key=${ROOT_DIR}/spec/ssl/server-key.pem
 " >> my.cnf
diff --git a/spec/mysql2/client_spec.rb b/spec/mysql2/client_spec.rb
index ede316b..78d2f3d 100644
--- a/spec/mysql2/client_spec.rb
+++ b/spec/mysql2/client_spec.rb
@@ -154,9 +154,9 @@ RSpec.describe Mysql2::Client do # rubocop:disable Metrics/BlockLength
     let(:option_overrides) do
       {
         'host'     => 'mysql2gem.example.com', # must match the certificates
-        :sslkey    => '/etc/mysql/client-key.pem',
-        :sslcert   => '/etc/mysql/client-cert.pem',
-        :sslca     => '/etc/mysql/ca-cert.pem',
+        :sslkey    => './spec/ssl/client-key.pem',
+        :sslcert   => './spec/ssl/client-cert.pem',
+        :sslca     => './spec/ssl/ca-cert.pem',
         :sslcipher => 'DHE-RSA-AES256-SHA',
         :sslverify => true,
       }

def ssl_dir
return @ssl_dir if @ssl_dir

@ssl_dir = ENV['TEST_RUBY_MYSQL2_SSL_DIR'] || '/etc/mysql'
Copy link
Contributor Author

@junaruga junaruga Dec 22, 2022

Choose a reason for hiding this comment

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

The name of the TEST_RUBY_MYSQL2_SSL_DIR is because there is one environment variable RUBY_MYSQL2_LIBMYSQL_DLL. So, I referred to the name.

$ grep -r RUBY_MYSQL2
README.md:* Environment variable `RUBY_MYSQL2_LIBMYSQL_DLL=C:\path\to\libmysql.dll`
tmp/x86_64-linux/stage/README.md:* Environment variable `RUBY_MYSQL2_LIBMYSQL_DLL=C:\path\to\libmysql.dll`
tmp/x86_64-linux/stage/lib/mysql2.rb:  dll_path = if ENV['RUBY_MYSQL2_LIBMYSQL_DLL']
tmp/x86_64-linux/stage/lib/mysql2.rb:    ENV['RUBY_MYSQL2_LIBMYSQL_DLL']
lib/mysql2.rb:  dll_path = if ENV['RUBY_MYSQL2_LIBMYSQL_DLL']
lib/mysql2.rb:    ENV['RUBY_MYSQL2_LIBMYSQL_DLL']

ci/ssl.sh Outdated
ssl-key=/etc/mysql/server-key.pem
ssl-ca=${SSL_DIR}/ca-cert.pem
ssl-cert=${SSL_DIR}/server-cert.pem
ssl-key=${SSL_DIR}/server-key.pem
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a note, we are applying the patch below in the mysql2 gem RPM package in the Fedora project. We are using the *.pem files in the original repository directory without copying.

diff --git a/spec/mysql2/client_spec.rb b/spec/mysql2/client_spec.rb
index 5861882..3f5cda8 100644
--- a/spec/mysql2/client_spec.rb
+++ b/spec/mysql2/client_spec.rb
@@ -153,9 +153,9 @@ RSpec.describe Mysql2::Client do # rubocop:disable Metrics/B
lockLength
     let(:option_overrides) do
       {
         'host'     => 'mysql2gem.example.com', # must match the certificates
-        :sslkey    => '/etc/mysql/client-key.pem',
-        :sslcert   => '/etc/mysql/client-cert.pem',
-        :sslca     => '/etc/mysql/ca-cert.pem',
+        :sslkey    => 'spec/ssl/client-key.pem',
+        :sslcert   => 'spec/ssl/client-cert.pem',
+        :sslca     => 'spec/ssl/ca-cert.pem',
         :sslcipher => 'DHE-RSA-AES256-SHA',
         :sslverify => true,
       }
---
2.38.1

@sodabrew
Copy link
Collaborator

This looks good with latest update. Ready to land?

@junaruga
Copy link
Contributor Author

Just moment. I think the key word ssl_cert_dir is better than ssl_dir in the .github/workflows/container.yml - matrix. Because the ssl_dir makes me imagine it is a top directory of the installed openssl. Let me rebase again.

… test.

In the Fedora project, we are running the mysql2 tests on the build environment
with a user permission, without root permission and without `sudo`.

In this case, we couldn't set up the pem files required to run SSL tests in the
`/etc/mysql`. This custom SSL directory option gives an option to run the SSL
tests executed in the environment.

How to use:

```
$ TEST_RUBY_MYSQL2_SSL_CERT_DIR=/tmp/mysql2 \
  bundle exec rake spec
```
@junaruga
Copy link
Contributor Author

junaruga commented Jan 26, 2023

Yes, I am ready to land!

I rebased by replacing the ssl_dir with ssl_cert_dir, and TEST_RUBY_MYSQL2_SSL_DIR with TEST_RUBY_MYSQL2_SSL_CERT_DIR and updating the commit message on the latest master branch. I confirmed the intended value is actually used on my forked repository.

@junaruga
Copy link
Contributor Author

What do you think? Was the previous ssl_dir better than the current ssl_cert_dir for you?

@sodabrew sodabrew merged commit 28145a6 into brianmario:master Jan 31, 2023
@sodabrew
Copy link
Collaborator

Merged! I do like your last change, it is clearer that the directory is for SSL certs and not libraries or anything else.

@junaruga junaruga deleted the wip/ssl-dir-custom branch February 28, 2023 15:54
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.

2 participants