[Manager] Run Manager tests with enabled client encryption #9960

mikliapko · 2025-01-30T11:01:59Z

Closes scylladb/scylla-manager#4164

Changes:

Updated ensure_and_get_cluster method to apply --force_non_ssl_session_port flag in add cluster command if client encryption is enabled. Together with that, updated add_cluster method in upgrade and installation tests as well.
Updated test_client_encryption to make it independent of initial cluster state.
Enabled client encryption for most Manager tests, except of one sanity (ubuntu24-manager-sanity) and one upgrade test (ubuntu24-manager-upgrade).

Testing

ubuntu22-upgrade-test, enabled encryption
ubuntu24-upgrade-test, disabled encryption
sct-manager-backup-test, enabled encryption
rocky9-installation-test, enabled encryption
ubuntu22-sanity-test, enabled encryption

PR pre-checks (self review)

I added the relevant backport labels
I didn't leave commented-out/debugging code

soyacz

besides few comments, worth case to test would be using dns names instead of ip addresses

soyacz · 2025-02-03T08:15:05Z

mgmt_cli_test.py

    def test_client_encryption(self):
        self.log.info('starting test_client_encryption')
+        # Encryption should be disabled at the beginning of the test


why needs to be disabled?

As the test aims to check both cases - with client encryption enabled and with encryption disabled.
Moreover, it makes the test independent on the initial cluster encryption state - since the same test_client_encryption (which is part of test_manager_sanity) can be executed against cluster with enabled or disabled encryption.

soyacz · 2025-02-03T08:15:57Z

mgmt_cli_test.py

-        healthcheck_task = mgr_cluster.get_healthcheck_task()
-
-        self.db_cluster.enable_client_encrypt()
+        self._set_scylla_client_encryption(is_enable=True)


this won't make sense if client_encrypt setting is false, should error/warn about it

Ah, that's true, will fix it

I decided to change an approach for this test a bit.
First of all, disabling encryption at the beginning of the test might be redundant since from now, the majority of tests will have it set by default. It will result in extra steps to disable it.
Secondly, SM verification with enabled encryption has higher priority than with disabled, thus, it's a right thing to check it firstly.
Thus, the stages were swapped - the first test stage will check the Manager functionality with ENABLED encryption, and only after that with DISABLED.

retesting it here:

enabled by default encryption

disabled encryption

both tests have passed successfully

mikliapko · 2025-02-03T09:22:11Z

worth case to test would be using dns names instead of ip addresses

Could you please elaborate on this?
Where DNS names might be used instead of IP addressed?

soyacz · 2025-02-03T09:36:53Z

worth case to test would be using dns names instead of ip addresses

Could you please elaborate on this? Where DNS names might be used instead of IP addressed?

SM using dns names instead of IP's

Update `ensure_and_get_cluster` method to `--force_non_ssl_session_port` flag in add cluster command if client encryption is enabled. Update add cluster method in upgrade and installation tests as well. In addition to that, 'ensure_and_get_cluster' method was refactored to get 'add_cluster_extra_params' (not required) which can be used as extra params (for example, 'client_encrypt') for add_cluster method.

Test reworked to check the current state of Scylla cluster encryption at the beginning and, if disabled, to enable it. It makes the test independent on the initial cluster state. In addition to that, changed the order of steps - at first, the test verify SM with enabled encryption, at second, with disabled. It makes sense since the majority of tests now will have the cluster encryption set to enabled at the beginning of the test. Thus, it will help to save time on disabling encryption that might have been required otherwise. Additionally, introduced _disable_client_encryption method to disable encryption since disable_client_encrypt method of BaseScyllaCluster class doesn't work in case when "client_encryption_options" block is already part of scylla.yaml (haven't investigated why it behaves so).

Until now, all Manager basic tests were run with disabled client encryption. This change adds `client_encrypt: true` to all active Manager yaml configurations, thus, enabling Scylla client encryption in tests. Encryption is kept disabled for only two jobs (ubuntu24-manager-sanity, ubuntu24-manager-upgrade) to have at least one job running with such configuration. To disable encryption for these specific tests a separate configuration yaml was introduced `disable_client_encrypt.yaml`.

mikliapko · 2025-02-03T11:32:35Z

worth case to test would be using dns names instead of ip addresses

Could you please elaborate on this? Where DNS names might be used instead of IP addressed?

SM using dns names instead of IP's

Yeah, it's not covered anywhere in Manager SCT tests.
As it's not directly related to this PR, let me issue a separate task for this and resolve it out of this PR.
@soyacz Are you okay with that?

soyacz · 2025-02-03T12:00:53Z

worth case to test would be using dns names instead of ip addresses

Could you please elaborate on this? Where DNS names might be used instead of IP addressed?

SM using dns names instead of IP's

Yeah, it's not covered anywhere in Manager SCT tests. As it's not directly related to this PR, let me issue a separate task for this and resolve it out of this PR. @soyacz Are you okay with that?

Yes, I'm ok, it was just my note that would be good to test it.

soyacz · 2025-02-03T12:04:52Z

mgmt_cli_test.py

+        self.log.info('ENABLED client encryption checks')
+        if not self.db_cluster.nodes[0].is_client_encrypt:
+            self.db_cluster.enable_client_encrypt()


just setting to enabled won't fix the problem. Test without client_encrypt set to True at test config time (or even provision) will end with encryption enabled without certificates - possibly failing.
I think this part should be skipped then and jump to DISABLED client encryption check if that's possible

This method puts the whole client_encryption_options block into config:

client_encryption_options: certificate: /etc/scylla/ssl_conf/client-facing.crt enabled: true keyfile: /etc/scylla/ssl_conf/client-facing.key truststore: /etc/scylla/ssl_conf/ca.pem

If nothing is set before test, this block will be put there, otherwise, just skipped, and the rest of the test will make sense.

true, I mixed it with the method above.

In that case, it's up to you to decide on the test flow.

soyacz

LGTM

soyacz · 2025-02-03T12:19:54Z

mgmt_cli_test.py

+        self.log.info('ENABLED client encryption checks')
+        if not self.db_cluster.nodes[0].is_client_encrypt:
+            self.db_cluster.enable_client_encrypt()


true, I mixed it with the method above.

In that case, it's up to you to decide on the test flow.

mikliapko · 2025-02-04T11:34:35Z

@karol-kokoszka @Michal-Leszczynski Guys, could you please take a look?

soyacz · 2025-02-04T12:51:12Z

still need one QA more to approve

mikliapko · 2025-02-04T13:53:47Z

still need one QA more to approve

@fruch @vponomaryov

mikliapko self-assigned this Jan 30, 2025

mikliapko force-pushed the enable-encryption-for-manager-tests branch 4 times, most recently from b25ca7a to 5bda33d Compare January 31, 2025 10:18

mikliapko added the backport/none Backport is not required label Jan 31, 2025

mikliapko force-pushed the enable-encryption-for-manager-tests branch from 5bda33d to 93f8969 Compare January 31, 2025 12:25

mikliapko changed the title ~~[Manager] Improve cluster addition methods and test coverage for enabled client encryption~~ [Manager] Run Manager tests with enabled client encryption Jan 31, 2025

mikliapko force-pushed the enable-encryption-for-manager-tests branch from 93f8969 to 5c86a06 Compare January 31, 2025 13:20

mikliapko marked this pull request as ready for review January 31, 2025 15:11

mikliapko requested a review from rayakurl as a code owner January 31, 2025 15:11

mikliapko requested review from karol-kokoszka, Michal-Leszczynski and a team and removed request for rayakurl January 31, 2025 15:11

soyacz reviewed Feb 3, 2025

View reviewed changes

mikliapko added 3 commits February 3, 2025 11:29

mikliapko force-pushed the enable-encryption-for-manager-tests branch from 5c86a06 to 883e1dc Compare February 3, 2025 11:15

soyacz reviewed Feb 3, 2025

View reviewed changes

soyacz approved these changes Feb 3, 2025

View reviewed changes

mikliapko mentioned this pull request Feb 3, 2025

[SCT] Reconfigure some tests to use hostname instead of ip addresses while adding the cluster scylladb/scylla-manager#4239

Open

Michal-Leszczynski approved these changes Feb 4, 2025

View reviewed changes

karol-kokoszka approved these changes Feb 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Manager] Run Manager tests with enabled client encryption #9960

[Manager] Run Manager tests with enabled client encryption #9960

mikliapko commented Jan 30, 2025 •

edited

Loading

soyacz left a comment

soyacz Feb 3, 2025

mikliapko Feb 3, 2025

soyacz Feb 3, 2025

mikliapko Feb 3, 2025

mikliapko Feb 3, 2025

mikliapko Feb 3, 2025

mikliapko Feb 3, 2025

mikliapko commented Feb 3, 2025

soyacz commented Feb 3, 2025

mikliapko commented Feb 3, 2025

soyacz commented Feb 3, 2025

soyacz Feb 3, 2025

mikliapko Feb 3, 2025 •

edited

Loading

soyacz Feb 3, 2025

soyacz left a comment

soyacz Feb 3, 2025

mikliapko commented Feb 4, 2025

soyacz commented Feb 4, 2025

mikliapko commented Feb 4, 2025

[Manager] Run Manager tests with enabled client encryption #9960

Are you sure you want to change the base?

[Manager] Run Manager tests with enabled client encryption #9960

Conversation

mikliapko commented Jan 30, 2025 • edited Loading

Testing

PR pre-checks (self review)

soyacz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikliapko commented Feb 3, 2025

soyacz commented Feb 3, 2025

mikliapko commented Feb 3, 2025

soyacz commented Feb 3, 2025

Choose a reason for hiding this comment

mikliapko Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soyacz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikliapko commented Feb 4, 2025

soyacz commented Feb 4, 2025

mikliapko commented Feb 4, 2025

mikliapko commented Jan 30, 2025 •

edited

Loading

mikliapko Feb 3, 2025 •

edited

Loading