[Manager] Enable encryption in test_client_encryption tearDown #13650
[Manager] Enable encryption in test_client_encryption tearDown #13650mikliapko wants to merge 3 commits intoscylladb:masterfrom
Conversation
mikliapko
force-pushed
the
manager-fix-logs-collection
branch
from
5fb2814 to
3f08803
Compare
mgmt_cli_test.py
Outdated
| def tearDown(self): | ||
| if not self.db_cluster.nodes[0].is_client_encrypt: | ||
| self._enable_client_encryption() | ||
| super().tearDown() |
There was a problem hiding this comment.
This expects the cluster to not have client encryption at the start, shouldnt it just restore to the default?
Also shouldnt this be done in the test itself, if the test is the one that changes it?
There was a problem hiding this comment.
This expects the cluster to not have client encryption at the start, shouldnt it just restore to the default?
Yeah, there are two possible initial configurations the cluster may have entering this test - enabled or disabled encryption, but after tearDown cluster should have it enabled in both cases. Otherwise, SCT would fail with
"/usr/bin/cqlsh --no-color --ssl --request-timeout=120 --connect-timeout=60 -e "select JSON * from system.truncated" 10.12.2.249"
STDERR: Connection error: ('Unable to connect to any servers', {'10.12.2.249:9042': PermissionError(1, "Tried connecting to [('10.12.2.249', 9042)]. Last error: [SSL: WRONG_VERSION_NUMBER] wrong version number (_ssl.c:1032)")}
when running CQLSH commands to save schema files required by logs collector.
In Manager sanity test_client_encryption is always the last sub-test.
https://github.com/scylladb/scylla-cluster-tests/blob/master/mgmt_cli_test.py#L912
Also shouldnt this be done in the test itself, if the test is the one that changes it?
IMO tearDown is a pretty nice mechanism for that (considering the note above that the exit condition should be always enabled encryption) - I would prefer it to try ... finally ... block put in the test body.
In such case the test is not complicated with extra logic not really needed for the test itself.
There was a problem hiding this comment.
but probably you are right and I shouldn't change the test to make log collection work probably but rather make it work if the client encryption is disabled as well, let me double check it
There was a problem hiding this comment.
okay, here is the problem, the CQLSH command gets or doesn't get --ssl flag depending on self.parent_cluster.params.get("client_encrypt") parameter value, what doesn't consider the fact of client encryption change during the test.
https://github.com/scylladb/scylla-cluster-tests/blob/master/sdcm/cluster.py#L3268
We could either change it to get the current state of client_encrypt from Scylla nodes or change the value of self.params.get("client_encrypt") every time the test does a change of encryption. I'd prefer the first.
There was a problem hiding this comment.
Or third option, we can change the teardown to change it back to default, which is self.params.get("client_encrypt"), which would make it self contained and without further affect as well.
I will leave the choice to you
There was a problem hiding this comment.
Make sense, I'd rather go with your option because fixing it in _gen_cqlsh_cmd to dynamically check the encryption state would add ~0.5 second to every run_cqlsh and who knows what effect may have across all the tests we have in SCT :) I'd rather avoid it.
There was a problem hiding this comment.
Regardless of which direction you'll, I would avoid using tearDown() and use pytest fixture with function scope, to enable/disable as needed. (also kind of a context manager, but that fits with pytest)
There was a problem hiding this comment.
@fruch
I thought that Manager tests used to utilize unittest.
But now I see it's pytest. Was it changed some time ago?
In such case, yes, I'll definitely go with pytest fixtures
There was a problem hiding this comment.
okay, there is some complexity making it a pytest fixture - test_client_encryption is called as a subtest from test_manager_sanity. So, it such case pytest doesn't run the fixture assigned to the test. I'll proceed with tearDown implementation for now.
|
Okay, converting back to draft to rework it a bit as discussed here |
mikliapko
force-pushed
the
manager-fix-logs-collection
branch
from
3f08803 to
14a1f02
Compare
Sometimes, the client encryption settings may change during the test (we have such checks in Manager suite `test_client_encryption`). With previous solution, it was not possible to disable encryption using db_cluster.disable_client_encrypt method since client_encryption_options was returning None. Now, the property would explicitly return enabled = False in such case
Since the test changes the client encryption state of the cluster, and
in order to not affect other tests or SCT teardown logic, it needs to
change it back to the default state at the end of the test.
Otherwise, SCT may fail while running some CQLSH commands where
_gen_cqlsh_cmd decides on the fact of enabled/disabled client encryption
reading `self.parent_cluster.params.get("client_encrypt")` parameter.
Together with that, enable_client_encrypt was adjusted to create certs
only if it's needed (create_certificates parameter)
...instead of Ubuntu24 tests that are currently used as a high prio test and the only test in Manager test run on multiDC cluster. We'd like to run it with client encryption enabled
mikliapko
force-pushed
the
manager-fix-logs-collection
branch
from
14a1f02 to
cbe96b4
Compare
3 participants
Pull request overview
This PR adds a tearDown method to the ManagerEncryptionTests class to re-enable client encryption after the test_client_encryption test completes. The test disables encryption at the end to verify the cluster can operate without it, but this left the cluster in an undesired state for next operations (like reading schema using CQLSH).
Changes:
Testing
PR pre-checks (self review)
backportlabels