PS-9823 fix: mysql_migrate_keyring won't work with PS's components (8.0) by percona-ysorokin · Pull Request #5648 · percona/percona-server

percona-ysorokin · 2025-07-02T16:54:00Z

https://perconadev.atlassian.net/browse/PS-9823

Reworked keyring components to make sure their corresponding '.so' objects do not have unresolved symbols (from the 'dlopen(..., RTLD_NOW)' point of view). This change is needed to ensure that keyring components can be loaded not only from the 'mysqld' executable but from utilities like 'mysql_migrate_keyring' as well.

Keyring components' 'CMakeLists.txt' files fortified with aditional linking option '${LINK_FLAG_NO_UNDEFINED}' (-Wl,--no-undefined) which prevents building '.so' shared objects with unresolved sumbols.

Removed custom allocator from the 'components/keyrings/common/data/pfs_string.h' header to eliminate divergence from upstream code. 'pfs_string' kept as an alias to 'std::string' to minimize Percona code changes.

Removed 'DBUG_TRACE' calls from the 'component_keyring_kmip' code to get rid of 'mysys' library dependency.

Calls to 'mysql_components_handle_std_exception()' inside 'component_keyring_kmip' replaced with 'LogComponentErr()' to avoid dependency on 'minchassis'.

'memset_s()' Percona's extension function renamed to 'my_memset_s()'.

Added a series of 'component_keyring_xxx.dynamic_loading' MTR test cases (one for each keyring component: 'file', 'kmip', 'kms') that checks if the component's '.so' file does not have unresolved symbols in order to make sure that it can be loaded from auxiliary utilities (like 'mysql_migrate_keyring'). These MTR test cases internally build a helper utility from the '.cpp' file ('mysql-test/std_data/dlopen_checker.cpp') that simply performs an attempt to call 'dlopen(..., RTLD_NOW)' for the provided '.so' object.

Added 'keyring_vault.migrate_keyring' MTR test case that tests for keyring data migration from 'keyring_vault' plugin to 'keyring_file' plugin and back. Internally, it uses 'mysqld' executable in keyring data migration mode to perform key stansfer from one plugin to another.

Added 'have_keyring_file_plugin.inc' MTR include file that helps to identify if the server was build with 'keyring_file' plugin.

Removed 'have_keyring_kmip_plugin.inc' MTR include file added previously by mistake.

github-actions

⚠️ Clang-Tidy found issue(s) with the introduced code (1/2)

github-actions · 2025-07-02T17:02:08Z

mysql-test/include/keyring_tests/mats/dynamic_loading.inc

@@ -0,0 +1,27 @@
+# ==== Purpose ====


❌ clang-diagnostic-error ❌
invalid preprocessing directive

github-actions · 2025-07-02T17:02:08Z

mysql-test/include/keyring_tests/mats/dynamic_loading.inc

@@ -0,0 +1,27 @@
+# ==== Purpose ====
+#
+# Check if the provided library ('.so') can be successfully loaded with 'dlopen(..., RTLD_NOW)'


❌ clang-diagnostic-error ❌
invalid preprocessing directive

github-actions · 2025-07-02T17:02:08Z

mysql-test/include/keyring_tests/mats/dynamic_loading.inc

+#
+# Check if the provided library ('.so') can be successfully loaded with 'dlopen(..., RTLD_NOW)'
+#
+# ==== Usage ====


❌ clang-diagnostic-error ❌
invalid preprocessing directive

github-actions · 2025-07-02T17:02:08Z

mysql-test/include/keyring_tests/mats/dynamic_loading.inc

+#
+# ==== Usage ====
+#
+# --let $DLOPEN_CHECKER_LIBRARY_PATH = <path_to_the_library>


❌ clang-diagnostic-error ❌
invalid preprocessing directive

github-actions · 2025-07-02T17:02:08Z

mysql-test/include/keyring_tests/mats/dynamic_loading.inc

+# ==== Usage ====
+#
+# --let $DLOPEN_CHECKER_LIBRARY_PATH = <path_to_the_library>
+# --source include/keyring_tests/mats/dynamic_loading.inc


❌ clang-diagnostic-error ❌
invalid preprocessing directive

github-actions · 2025-07-02T17:02:08Z

mysql-test/include/keyring_tests/mats/dynamic_loading.inc

+# --let $DLOPEN_CHECKER_LIBRARY_PATH = <path_to_the_library>
+# --source include/keyring_tests/mats/dynamic_loading.inc
+#
+# ==== Parameters ====


❌ clang-diagnostic-error ❌
invalid preprocessing directive

github-actions · 2025-07-02T17:02:08Z

mysql-test/include/keyring_tests/mats/dynamic_loading.inc

+#
+# ==== Parameters ====
+#
+# DLOPEN_CHECKER_LIBRARY_PATH


❌ clang-diagnostic-error ❌
invalid preprocessing directive

github-actions · 2025-07-02T17:02:08Z

mysql-test/include/keyring_tests/mats/dynamic_loading.inc

+# ==== Parameters ====
+#
+# DLOPEN_CHECKER_LIBRARY_PATH
+#    Full path to the library that needs to be checked for unresolved symbols ('.so')


❌ clang-diagnostic-error ❌
invalid preprocessing directive

github-actions · 2025-07-02T17:02:08Z

mysql-test/include/keyring_tests/mats/dynamic_loading.inc

+#    Full path to the library that needs to be checked for unresolved symbols ('.so')
+#
+
+--let $dlopen_checker_source = $MYSQL_TEST_DIR/std_data/dlopen_checker.cpp


❌ clang-diagnostic-error ❌
expected unqualified-id

github-actions · 2025-07-02T17:02:09Z

mysql-test/include/have_keyring_file_plugin.inc

@@ -0,0 +1,6 @@
+#
+# Check if the variable KEYRING_PLUGIN is set


❌ clang-diagnostic-error ❌
invalid preprocessing directive

github-actions

⚠️ Clang-Tidy found issue(s) with the introduced code (2/2)

github-actions · 2025-07-02T17:02:20Z

mysql-test/include/have_keyring_file_plugin.inc

+#
+# Check if the variable KEYRING_PLUGIN is set
+#
+if (!$KEYRING_PLUGIN) {


❌ clang-diagnostic-error ❌
expected unqualified-id

github-actions · 2025-07-02T17:02:21Z

mysys/my_malloc.cc


-#if !defined(HAVE_MEMSET_S)
-void memset_s(void *dest, size_t dest_max, int c, size_t n) {
+void my_memset_s(void *dest, size_t dest_max, int c, size_t n) {


⚠️ readability-identifier-length ⚠️
parameter name c is too short, expected at least 2 characters

github-actions · 2025-07-02T17:02:21Z

sql/binlog_crypt_data.cc

  if (key != nullptr) {
    assert(key_length == 16);
-    memset_s(key, 512, 0, key_length);
+    my_memset_s(key, 512, 0, key_length);


⚠️ readability-magic-numbers ⚠️
512 is a magic number; consider replacing it with a named constant

github-actions · 2025-07-02T17:02:21Z

include/my_sys.h

-#if !defined(HAVE_MEMSET_S)
-void memset_s(void *dest, size_t dest_max, int c, size_t n);
-#endif
+void my_memset_s(void *dest, size_t dest_max, int c, size_t n);


⚠️ readability-identifier-length ⚠️
parameter name c is too short, expected at least 2 characters

dlenev

Hello Yura!

I have only one fairly minor comment about using correct error numbers about this patch (the same as for 8.4).

Otherwise changes look OK to me.

dlenev · 2025-07-11T11:38:23Z

components/keyrings/keyring_kmip/backend/backend.cc

    }
    data.set_extension({id});
+  } catch (const std::exception &e) {
+    LogComponentErr(ERROR_LEVEL, ER_STD_UNKNOWN_EXCEPTION, e.what(), __func__);


Same comment as for 8.4 version. I think we should be using error codes for logging instead of ER_UNKNOWN_ERROR/ER_STD_UNKNOWN_EXCEPTION which are supposed to be client
errors. Probably the easiest way to do this is to use ER_LOG_PRINTF_MSG like it is done in
other places in this file.

https://perconadev.atlassian.net/browse/PS-9823 Reworked keyring components to make sure their corresponding '.so' objects do not have unresolved symbols (from the 'dlopen(..., RTLD_NOW)' point of view). This change is needed to ensure that keyring components can be loaded not only from the 'mysqld' executable but from utilities like 'mysql_migrate_keyring' as well. Keyring components' 'CMakeLists.txt' files fortified with aditional linking option '${LINK_FLAG_NO_UNDEFINED}' (-Wl,--no-undefined) which prevents building '.so' shared objects with unresolved sumbols. Removed custom allocator from the 'components/keyrings/common/data/pfs_string.h' header to eliminate divergence from upstream code. 'pfs_string' kept as an alias to 'std::string' to minimize Percona code changes. Removed 'DBUG_TRACE' calls from the 'component_keyring_kmip' code to get rid of 'mysys' library dependency. Calls to 'mysql_components_handle_std_exception()' inside 'component_keyring_kmip' replaced with 'LogComponentErr()' to avoid dependency on 'minchassis'. 'memset_s()' Percona's extension function renamed to 'my_memset_s()'. Added a series of 'component_keyring_xxx.dynamic_loading' MTR test cases (one for each keyring component: 'file', 'kmip', 'kms') that checks if the component's '.so' file does not have unresolved symbols in order to make sure that it can be loaded from auxiliary utilities (like 'mysql_migrate_keyring'). These MTR test cases internally build a helper utility from the '.cpp' file ('mysql-test/std_data/dlopen_checker.cpp') that simply performs an attempt to call 'dlopen(..., RTLD_NOW)' for the provided '.so' object. Added 'keyring_vault.migrate_keyring' MTR test case that tests for keyring data migration from 'keyring_vault' plugin to 'keyring_file' plugin and back. Internally, it uses 'mysqld' executable in keyring data migration mode to perform key stansfer from one plugin to another. Added 'have_keyring_file_plugin.inc' MTR include file that helps to identify if the server was build with 'keyring_file' plugin. Removed 'have_keyring_kmip_plugin.inc' MTR include file added previously by mistake.

…m percona-ysorokin/dev/PS-9823-8.0-mysql_migrate_keyring_unusable)

0511671' (Merge pull request #5648 from percona-ysorokin/dev/PS-9823-8.0-mysql_migrate_keyring_unusable)) into trunk

github-actions bot reviewed Jul 2, 2025

View reviewed changes

percona-ysorokin requested a review from dlenev July 4, 2025 12:28

dlenev requested changes Jul 11, 2025

View reviewed changes

percona-ysorokin force-pushed the dev/PS-9823-8.0-mysql_migrate_keyring_unusable branch from 296e149 to b5bcaba Compare July 14, 2025 23:28

percona-ysorokin force-pushed the dev/PS-9823-8.0-mysql_migrate_keyring_unusable branch from b5bcaba to b598edd Compare July 15, 2025 22:45

inikep merged commit 0511671 into percona:8.0 Jul 16, 2025
21 of 23 checks passed

inikep added a commit that referenced this pull request Jul 18, 2025

Null-Merge commit 'percona/8.0@0511671' (Merge pull request #5648 fro…

eab2f47

…m percona-ysorokin/dev/PS-9823-8.0-mysql_migrate_keyring_unusable)

inikep added a commit that referenced this pull request Jul 18, 2025

Null-merge commit 'percona/8.4@eab2f47' (Null-Merge commit 'percona/8.0@

eb21017

0511671' (Merge pull request #5648 from percona-ysorokin/dev/PS-9823-8.0-mysql_migrate_keyring_unusable)) into trunk

		@@ -0,0 +1,6 @@
		#
		# Check if the variable KEYRING_PLUGIN is set

Comments

Conversation

percona-ysorokin commented Jul 2, 2025

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

dlenev left a comment

Choose a reason for hiding this comment

Uh oh!

dlenev Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants