Backport ccm main

[Lorak-mmk]

@muzarski @wprzytula This is a draft of the CCM backport into main branch.

I mostly took the current state from branch-hackathon. One important change I made is to remove separate ccm-integration test target. Instead ccm tests are now a module in integration. This avoids the problem with sharing utils. It should also be quicker to compile - no need to link 2 separate binaries.

Apart from that I did not really modify CCM integration. Now the question is: what do we do with it. Are we satisfied with the API? Probably not. If not, what should the API be like?

This is fully internal to the crate, so we can change it freely, so there is no need to spend too much time on it - we can always improve later. Still, we should retain some reasonable level of quality, so I'd like to discuss this a bit.

I did not yet pick up @muzarski 's changes that integrated auth and TLS workflows into CCM. I can do that after we agree to the rest of this PR.

On the above matter @muzarski : How should I adapt CCM given that we now support multiple TLS backends? I see we have in mod.rs DB_TLS_CERT_PATH, DB_TLS_KEY_PATH and CA_TLS_CERT_PATH (which I removed for now because it was guarded by old feature name). I also see that on branch-hackathon you wrote a TLS test. We should probably make a test per backend, right? What about those vars? Do they make sense for all the tests? In that case we just need to change feature guard on CA_TLS_CERT_PATH to be activated when any backend is active - or to even make it always active because why not.

Pre-review checklist

• [x] I have split my patch into logically separate commits.
• [ ] All commit messages clearly explain what they change and why.
• [ ] I added relevant tests for new features and bug fixes.
• [ ] All commits compile, pass static checks and pass test.
• [ ] PR description sums up the changes and reasons why they should be introduced.
• [ ] I have provided docstrings for the public items that I want to introduce.
• [ ] I have adjusted the documentation in ./docs/source/.
• [ ] I added appropriate Fixes: annotations to PR description.

[github-actions[bot]]

cargo semver-checks found no API-breaking changes in this PR. Checked commit: 9701a5f84de4d22868937bd0a1611590dd388215

[Lorak-mmk]

Possible improvements / API changes after a brief glance at the code:

• Should NodeStartOptions be a struct? In other words, does it make sense to enable e.g. no_wait and wait_other_notice at the same time? We need to know the exact semantics of the wait-related flags to know that (cc @fruch because I don't think this is documented anywhere in this cursed software).
• Same for NodeStopOptions
• We hold nodes in Arc<RwLock<>>, and methods that give user nodes return that. Maybe we could return refs / mut refs and get rid of Arc<RwLock<>>? I'm not sure.
• In the future when we have custom test runner, we could make new struct (ClusterPreferences + NodePreferences) or extend *Options structs. Why? Tests may not care about some parameters, which could help test runner to provide less clusters.

[muzarski]

On the above matter @muzarski : How should I adapt CCM given that we now support multiple TLS backends? I see we have in mod.rs DB_TLS_CERT_PATH, DB_TLS_KEY_PATH and CA_TLS_CERT_PATH (which I removed for now because it was guarded by old feature name). I also see that on branch-hackathon you wrote a TLS test. We should probably make a test per backend, right? What about those vars? Do they make sense for all the tests? In that case we just need to change feature guard on CA_TLS_CERT_PATH to be activated when any backend is active - or to even make it always active because why not.

Notice that I implemented all of this when we had old certificates in the repository. We had to update them, because of the errors thrown by rustls.

Why old certs worked for openssl but did not for rustls?

It's because rustls supports hostname verification by default, while openssl does not. The CN (common name) in db certificate was not matching the hostname, thus rustls was throwing an error.

What changed in the certificates, compared to the previous version?

I generated the db certificates assigned to static IP (172.44.0.2 - one we currently use in CI for TLS single-node cluster). In other words, the extensions to certificate request in openssl config looked like:

[v3_req]
# Extensions to add to a certificate request
basicConstraints = CA:FALSE
subjectAltName = IP:172.44.0.2
keyUsage = nonRepudiation, digitalSignature, keyEncipherment

Thanks to that, rustls is able to verify the hostname using node's IP. It checks whether the node that we try to connect to has the same IP as the one defined in certificate (under subjectAltName).

Current state

Currently, our SSL "tests" are limited to simply running the tls-openssl and tls-rustls examples in SSL CI workflow. On hackathon-branch, however, I removed the SSL workflow and migrated the example test to ccm. It obviously did it for openssl only, as rustls was not supported back then.

Implementing the corresponding ccm test for rustls backed and removing SSL CI workflow.

Well, this is a bit tricky. While, it was not an issue with openssl, for the reasons stated above (no hostname verification), it won't work for rustls. This is because we use dynamic IPs in ccm tests.

The temporary solution I see: migrate the tests from SSL workflow to ccm only for openssl and limit rustls "tests" to just running the example against the global cluster (as it is currently done in CI).

If we ever decide that we want to have ccm tests for rustls backend, I list the possible solutions to the dynamic IP problem:

• Generate certificates on the fly, during the test. We would firstly receive some IP from the IpAllocator (or some other mechanism in the future) and then we could generate the self-signed cert for this IP. Then we could ccm updateconf and provide the path to generated certificate. There are some crates we could use, e.g. https://docs.rs/rcgen/latest/rcgen/.
• Disable hostname verification during the tests. I think this can be configured via the trait: rustls::client::danger::ServerCertVerifier. I'm not entirely sure, though. This needs some research - I think this is placed in danger module for a reason. OTOH, we already use it in scylla::cloud::config and implement it for our NoCertificateVerification struct.

[Lorak-mmk]

Moving such workflows to CCM has 2 advantages:

• We get rid of custom images
• We get rid of a GHA workflow

If we move only part of it, we get neither. So I'll cherry pick commits that move auth, and skip TLS for now. We can do that in the future.

Btw is it possible to use domain names instead of ip addresses with scylla? In other words, can we have domain names instead of ip addresses in system.peers in driver-relevant columns? If it was possible, we could use certs with hostnames instead of ips.

[fruch]

Moving such workflows to CCM has 2 advantages:

• We get rid of custom images
• We get rid of a GHA workflow

If we move only part of it, we get neither. So I'll cherry pick commits that move auth, and skip TLS for now. We can do that in the future.

Btw is it possible to use domain names instead of ip addresses with scylla? In other words, can we have domain names instead of ip addresses in system.peers in driver-relevant columns? If it was possible, we could use certs with hostnames instead of ips.

scylla can use hostnames, but then you need a dns server to map them.

I think generating certs as needed is the best approach, and also give the flexibility to try more variants as needed. that's what we are doing in dtest, and in SCT.

[Lorak-mmk]

scylla can use hostnames, but then you need a dns server to map them.

I think generating certs as needed is the best approach, and also give the flexibility to try more variants as needed. that's what we are doing in dtest, and in SCT.

Is there functionality in CCM to generate certs? Or do we have to do it other way?

If Scylla can use hostnames, then we should test it too.

@fruch one other question for you. Could you describe (or point to documentation if such exists) what exactly wait-related flags do in CCM, and how do they interact if I specify more than one?

[fruch]

scylla can use hostnames, but then you need a dns server to map them. I think generating certs as needed is the best approach, and also give the flexibility to try more variants as needed. that's what we are doing in dtest, and in SCT.

Is there functionality in CCM to generate certs? Or do we have to do it other way?

If Scylla can use hostnames, then we should test it too.

@fruch one other question for you. Could you describe (or point to documentation if such exists) what exactly wait-related flags do in CCM, and how do they interact if I specify more than one?

you are more then welcome to document it.

[Lorak-mmk]

you are more then welcome to document it.

I'd be happy to make a PR that improves descriptions, but I would have to first understand those options myself. I don't know ccm's codebase at all, and it is not really friendly to new contributors, that's why I asked you to explain those options.

[Lorak-mmk]

I backported the commits that move auth to CCM. I also removed TLS support from CCM for now.

[Lorak-mmk]

Marking as ready. The way I see it the only improvement I can make here is better CCM API - which needs input from others, which is basically a review.

[Lorak-mmk]

• Addressed @muzarski 's comments
• Removed all the Arc<Mutex<Node>> stuff, now we just operate on Nodes.
• I decided to retain NodeList because I welcome any kind of separation and structure in this code. I made its method simpler using iterator methods.
• Method that adds node return mut reference to this node. I think it is more useful than id.

[Lorak-mmk]

I have one more idea: we can split off another file from cluster.rs, I would call it ccm_cmd.rs. It would be a simple wrapper over CCM, providing builder-style commands. The purpose of this module would be to provide convenient way to call CCM, and encode all its commands and flags into Rust types. cluster.rs would be responsible for providing user-facing API, handling config dirs etc. Its code would hopefully become cleaner.

[Lorak-mmk]

I did this for 2 commands as an experiment, in additional commit. I like the new version, so unless anyone has different opinion I'll convert the rest of the command to this.

@dkropachev I see that both ccm create and ccm populate accept ipprefix argument. Why? What are their respective semantics?

[muzarski]

I did this for 2 commands as an experiment, in additional commit. I like the new version, so unless anyone has different opinion I'll convert the rest of the command to this.

I love the idea. The code in cluster.rs looks much cleaner.

[dkropachev]

I did this for 2 commands as an experiment, in additional commit. I like the new version, so unless anyone has different opinion I'll convert the rest of the command to this.

@dkropachev I see that both ccm create and ccm populate accept ipprefix argument. Why? What are their respective semantics?

ccm create calls ccm populate, if you provide number of nodes. Flow of ccm create + ccm add + ccm start is broken, so you better make ccm create to call ccm populate to create a cluster in the single blow.

[wprzytula]

Flow of ccm create + ccm add + ccm start is broken

Could you elaborate? What bad happens in such scenario?

[Lorak-mmk]

New version of the PR. Finalized the move to a separate command builders and made a lot of other changes. I think there is a lot of room for improvement still, but I'd like to put this up for review now anyway - it will be easier to get it to something acceptable together.

[Lorak-mmk]

Addressed Mikolaj's comment.

[Lorak-mmk]

Addressed most @wprzytula comments, and responded to rest. Rebased on main.

[wprzytula]

Please update the cover letter.

[Lorak-mmk]

Dropped the commit with Makefile changes and rebased on main.

Opened an issue for mixed-shards clusters: https://github.com/scylladb/scylla-rust-driver/issues/1367