roachtest: encryption at rest configurations aren't respected when creating clusters

[nicktrav]

Describe the problem

A recent change in #76669 altered the way that tests were opted into using encryption at rest (EAR).

Consider a test that should always run with EAR (like this one). After the refactor, the test is required to explicitly opt into random EAR by setting EncryptAtRandom on the TestSpec (a bit of an API wrinkle, as this test always wants EAR, not randomly).

However, even if that flag were true, EAR is only on 50% of the time (see here), which makes the test flaky.

Expected behavior

A roachtest that always wants EAR should be have its settings respected.

Epic: CRDB-10428
Jira issue: CRDB-14664

[nicktrav]

Related to #78125.

[srosenberg]

The current logic is still too convoluted,

• encrypt is a global and defaults to random (for nightlies)
• we use it to set encAtRest [1] per cluster
• then, we use encAtRest to override startOpts.RoachprodOpts.EncryptedStores for the same cluster

It bears simplification. Why not lift encAtRest into TestSpec where EncryptAtRandom already lives?

[1] https://github.com/cockroachdb/cockroach/blob/master/pkg/cmd/roachtest/test_runner.go#L602
[2] https://github.com/cockroachdb/cockroach/blob/master/pkg/cmd/roachtest/cluster.go#L1796

[tbg]

encrypt is not only a global variable, it is a flag. This is so that you can run roachtest run --encrypt ....

It bears simplification. Why not lift encAtRest into TestSpec where EncryptAtRandom already lives?

because then you can't run roachtest run --encrypt=X any more, since now the encryption is hard-coded per test. Maybe that is fine - we don't seem to be using the --encrypt flag anywhere - but at least when running the test it is highly desirable to stick to one. It would be annoying to have to repro a test that specifies random encryption but only fails in one of the two cases. A local code modification would be necessary and that would create hoops to jump through in order to let CI stress the test for you.

I think there generally is merit to letting tests decide (and we do already do that until I guess I broke it with this issue), as long as it's deterministic. Maybe what we ought to do is to let the test decide between

• allow both
• allow only on (tests that verify something about encryption at rest would chose this, even if they could run without encryption, for example we would not want to run tpcc/enc=true/... without encryption since the name says it's encrypted)
• allow only off

where the actual determination of randomness (if supported) is made on a test-by-test basis behind roachtest run --encrypt=random, which CI would have to pass automatically. (with "on", tests that don't support encryption would be skipped; with off vice versa).

[renatolabs]

@tbg's suggestion makes sense to me. If anyone has another suggestion they'd like to discuss, let me know.

[renatolabs]

@tbg and I talked about this issue earlier today, and I'm documenting some of the things we talked about, and what our conclusion was.

TL;DR: --encrypt will no longer support true|false as it currently does. Instead, the only two possible values will be --encrypt=auto and --encrypt=random. The latter will be used in CI (as is the case today), and the former will be mostly used by developers (and is the default).

Behavior-wise, --auto means that if a test supports encryption, encryption will be enabled (and the reverse for tests that don't support encryption). random means we'll randomly select whether encryption is enabled for tests that indicate they support both.

If a dev wants to force encryption on/off on a test that allows both, they can checkout the relevant branch, change the TestSpec accordingly, and run roachtest with --encrypt=auto.

---

Longer description:

Previously (before #76669), --encrypt=random would try to enable encryption on the cluster randomly; tests that required encryption to be always enabled or disabled would then set RoachprodOpts.EncryptedStores (or RoachtestOpts.DontEncrypt) manually at some point during the test (as was the case in the test @nicktrav mentioned).

In #76669, Tobi then tried to reconcile the --encrypt flag with what tests declare by making sure the --encrypt flag has the final say on whether encryption is enabled on the cluster or not. That broke Nick's test because it relied on encryption always being enabled.

Some other approaches we considered to deal with this were:

• --encrypt=true and --encrypt=random would automatically skip tests that can only run with encryption disabled (and vice-versa for --encrypt=false). This was the original plan, but the downside is that we would then need to invoke roachtest twice in CI scripts, which we don't want.

• --encrypt=true means that roachtest will fail if the filter passed includes tests that don't support encryption (and vice-versa for --encrypt=false). The main reason we don't support this is that we don't foresee a lot of use for this feature, so we might not want to maintain it. For cases where the we do want to force encryption on tests that support both, the branch can be checked out and modified locally. We could add support for this if at some point doing this type of manual change becomes common.