roachtest: declare encryption support as part of TestSpec

[renatolabs]

This commit introduces the EncryptionSupport field to TestSpec. By
setting this field, tests can declare whether they support being run
in a cluster with encryption enabled. Three options are available:

• EncryptionAllowed: test can run with or without encryption.
• EncryptionRequired: test can only run if encryption is enabled.
• EncryptionDisabled: test can only run if encryption is disabled (default).

This replaces the EncryptAtRandom field added earlier, which only
allowed tests to opt-in to random encryption.

In addition to this change, the values accepted by the --encrypt
flag in roachtest also changes. The only two valid values are:

• --encrypt=auto (default). Tests that support encryption
  (EncryptionAllowed) will run with encryption enabled.
• --encrypt=random (CI). Tests that support encryption
  (EncryptionAllowed) will run on a cluster which may or may
  not have encryption enabled.

The EncryptionSupport field should be the only way tests indicate
their relationship with encryption. The DontEncrypt field was
removed (it was not being used anymore at the time of this commit),
and all tests that set EncryptedStores directly were updated to use
EncryptionSupport accordingly.

Resolves: #79265.

Release note: None

---

This commit should be sufficient for us to re-enable @nicktrav's tests that originated this issue (#79265). Should I do it in this commit/PR or is that generally done separately once the fix is merged?

[cockroach-teamcity]

This change is 

[nicktrav]

Thanks for cleaning this up! 🎉 Just a few comments.

Should I do it in this commit/PR or is that generally done separately once the fix is merged?

I don't have a preference. In the case that we don't, I can just open a PR with the one line change to revert the skip made in #81054 and backport to 22.1.

The main thing is we'll want to at least backport this PR to 22.1 (check out these docs if you haven't already, and consider adding the labels for Blathers).

Reviewed 18 of 18 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @renatolabs and @tbg)

---

-- commits line 20 at r1:
Looking at test_runner.go it seems like for tests that have EncryptionRequired, the randomness doesn't factor in at all (good!). Worth updating this to mention that? Also - perhaps update the doc on the flag to mention that its value doesn't affect EncryptionRequired (we'll always run with encryption).

---

pkg/cmd/roachtest/cluster.go line 74 at r1 (raw file):

	// without encryption.
	//
	// To force encryption off for a test supports encryption

nit: "for a test that supports"

---

pkg/cmd/roachtest/test_runner.go line 457 at r1 (raw file):

	}()

	random := rand.New(rand.NewSource(timeutil.Now().UnixNano()))

I believe there's a global rand that can be used here instead - that handles logging the seed, setting via env vars, etc. See pkg/util/randutil. This could probably use the NewTestRand function. Will defer to Tobi.

If we can't use that for whatever reason, we should at least log the seed to aid reproducibility.

---

pkg/cmd/roachtest/registry/encryption.go line 35 at r1 (raw file):

const (
	// EncryptionDisabled indicates that a test requires encryption to

This comment seems incorrect. Copypasta?

---

pkg/cmd/roachtest/tests/encryption.go line 52 at r1 (raw file):

		// Restart node with encryption turned on to verify old key works.
		c.Start(ctx, t.L(), option.DefaultStartOpts(), install.MakeClusterSettings(), c.Range(1, nodes))

nit: could we reuse startOpts here?

---

pkg/cmd/roachtest/tests/gossip.go line 361 at r1 (raw file):

	startOpts := option.DefaultStartOpts()
	c.Start(ctx, t.L(), option.DefaultStartOpts(), settings)

nit: use startOpts here? It looks like it's used further down in this method, so probably can't remove it entirely.

---

pkg/cmd/roachtest/test_test.go line 225 at r1 (raw file):

		if atomic.LoadInt32(&sawEncrypted) == 0 {
			// NB: since it's a 50% chance, hitting this reliably over 10k trials
			// has probability (0.5)^10000 which is for all intents and purposes

super nit: This should probably have originally read 1 - (0.5)^10000 (one minus the prob that all tests ran without encryption, which is ~1). In which case the s/one/zero diff can be reverted.

[srosenberg]

Reviewed 9 of 18 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nicktrav, @renatolabs, and @tbg)

---

-- commits line 10 at r1:
The asymmetry between the naming of EncryptionRequired and EncryptionDisabled threw me off since the former means always on and the latter means always off. Maybe we could rename them to capture that intuition,

• EncryptionMetamorphic: test can run with or without encryption.
• EncryptionAlwaysEnabled: test can only run if encryption is enabled.
• EncryptionAlwaysDisabled: test can only run if encryption is enabled.

Side-note: the metamorphic reference is akin to "metamorphic constants" which get random values; e.g., https://github.com/cockroachdb/cockroach/blob/master/pkg/util/constants.go#L41

---

pkg/cmd/roachtest/main.go line 135 at r1 (raw file):

	rootCmd.PersistentFlags().StringVar(
		&workload, "workload", "", "path to workload binary to use")
	rootCmd.PersistentFlags().StringVar(

I wonder if we should get rid of "auto" and use random with a probability which defaults to 0.5; i.e., "auto" is subsumed by passing 1.0 but we could also entertain other values, e.g., 0.75. Although, I admit I don't immediately see use-cases for anything other than 0, 0.5 or 1.0.

---

pkg/cmd/roachtest/test_runner.go line 457 at r1 (raw file):

Previously, nicktrav (Nick Travers) wrote…

I believe there's a global rand that can be used here instead - that handles logging the seed, setting via env vars, etc. See pkg/util/randutil. This could probably use the NewTestRand function. Will defer to Tobi.

If we can't use that for whatever reason, we should at least log the seed to aid reproducibility.

Yep, good call @nicktrav! The above should be replaced with randutil.NewPseudoRand() to theoretically leverage reproducibility via the initial seed.

---

pkg/cmd/roachtest/test_test.go line 225 at r1 (raw file):
Yep, it's definitely the phrasing which also threw me off. What if we paraphrase,

NB: since it's a 50% chance, the probability of not taking this branch is 1 - (0.5)^10000 which is essentially one.

[tbg]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nicktrav, @renatolabs, and @srosenberg)

---

-- commits line 10 at r1:

Previously, srosenberg (Stan Rosenberg) wrote…

The asymmetry between the naming of EncryptionRequired and EncryptionDisabled threw me off since the former means always on and the latter means always off. Maybe we could rename them to capture that intuition,

• EncryptionMetamorphic: test can run with or without encryption.
• EncryptionAlwaysEnabled: test can only run if encryption is enabled.
• EncryptionAlwaysDisabled: test can only run if encryption is enabled.

Side-note: the metamorphic reference is akin to "metamorphic constants" which get random values; e.g., https://github.com/cockroachdb/cockroach/blob/master/pkg/util/constants.go#L41

My preference would be to s/EncryptionDisabled/EncryptionUnsupported/ and leave it at that. That way, the names reflect what the test can support. Stan, your suggestion raises questions about what happens when we try to run a test that has, say, EncryptionAlwaysDisabled, under encryption. Would it just run without encryption? The current names just state that it is unsupported. It leaves it to the test runner to decide how to handle this, and we're explicitly sidestepping this case right now by not offering true/false flags. I think it's a good thing if the semantics of that case don't require us to change the enum.

[renatolabs]

I removed the Skip field on the encryption tests 👍

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nicktrav and @srosenberg)

a discussion (no related file):
I pushed an updated version of this with the "metamorphic encryption" idea @srosenberg suggested. Let me know what you think. We can always revert back if you think you liked the previous version better 🙂

---

---

-- commits line 10 at r1:

Previously, tbg (Tobias Grieger) wrote…

My preference would be to s/EncryptionDisabled/EncryptionUnsupported/ and leave it at that. That way, the names reflect what the test can support. Stan, your suggestion raises questions about what happens when we try to run a test that has, say, EncryptionAlwaysDisabled, under encryption. Would it just run without encryption? The current names just state that it is unsupported. It leaves it to the test runner to decide how to handle this, and we're explicitly sidestepping this case right now by not offering true/false flags. I think it's a good thing if the semantics of that case don't require us to change the enum.

I liked the metamorphic idea and pushed a possible implementation of that. What I liked about this is that, in the previous approach, we were recommending devs to checkout a local copy of the test in order to run a test that supported "EncryptionAllowed" (random). But if we pass a probability, we can say probability=0 without having to change the code. I also renamed --encryption to --metamorphic-encryption-probability to make the intention clear that this parameter only controls encryption for tests that opted-in to metamorphic encryption.

---

-- commits line 20 at r1:

Previously, nicktrav (Nick Travers) wrote…

Looking at test_runner.go it seems like for tests that have EncryptionRequired, the randomness doesn't factor in at all (good!). Worth updating this to mention that? Also - perhaps update the doc on the flag to mention that its value doesn't affect EncryptionRequired (we'll always run with encryption).

I rewrote the commit message for the updated approach, let me know what you think.

---

pkg/cmd/roachtest/main.go line 135 at r1 (raw file):

Previously, srosenberg (Stan Rosenberg) wrote…

I wonder if we should get rid of "auto" and use random with a probability which defaults to 0.5; i.e., "auto" is subsumed by passing 1.0 but we could also entertain other values, e.g., 0.75. Although, I admit I don't immediately see use-cases for anything other than 0, 0.5 or 1.0.

I agree there's no use currently for anything other than {0, 0.5, 1}, but I think in practice that's ok, it should be relatively straightforward to understand what this is.

---

pkg/cmd/roachtest/test_runner.go line 457 at r1 (raw file):

Previously, srosenberg (Stan Rosenberg) wrote…

Yep, good call @nicktrav! The above should be replaced with randutil.NewPseudoRand() to theoretically leverage reproducibility via the initial seed.

Good to know, updated.

---

pkg/cmd/roachtest/registry/encryption.go line 35 at r1 (raw file):

Previously, nicktrav (Nick Travers) wrote…

This comment seems incorrect. Copypasta?

Ops, yes, the comments in this file were outdated. Let me know if you still find inconsistencies.

---

pkg/cmd/roachtest/tests/encryption.go line 52 at r1 (raw file):

Previously, nicktrav (Nick Travers) wrote…

nit: could we reuse startOpts here?

startOpts was only used in this file to set EncryptedStores = true. Tests should now interact with encryption by setting EncryptionSupport on the TestSpec.

---

pkg/cmd/roachtest/tests/gossip.go line 361 at r1 (raw file):

Previously, nicktrav (Nick Travers) wrote…

nit: use startOpts here? It looks like it's used further down in this method, so probably can't remove it entirely.

Just as in the other file, startOpts was used here to force encryption by setting EncryptedStores. Tests shouldn't do that anymore, ideally.

---

pkg/cmd/roachtest/test_test.go line 225 at r1 (raw file):

Previously, srosenberg (Stan Rosenberg) wrote…

Yep, it's definitely the phrasing which also threw me off. What if we paraphrase,

NB: since it's a 50% chance, the probability of not taking this branch is 1 - (0.5)^10000 which is essentially one.

Fixed.

[nicktrav]

Left on small comment about the default, but otherwise

Reviewed 20 of 20 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @renatolabs and @srosenberg)

---

pkg/cmd/roachtest/registry/encryption.go line 38 at r2 (raw file):

	// encryption to be disabled. The test will only run on clusters
	// with encryption disabled.
	EncryptionAlwaysDisabled = EncryptionSupport(iota)

I don't have strong opinions on this, though wanted to mention - any objections to making the metamorphic variant the default? It would be good from a test coverage perspective to have tests using the encryption at rest code. For most tests, I'd assume encryption vs. not-encryption is just an implementation detail that shouldn't matter.

Given this is more of a "test philosophy" type thing, I'll defer to y'all.

[renatolabs]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @srosenberg and @tbg)

---

pkg/cmd/roachtest/registry/encryption.go line 38 at r2 (raw file):

Previously, nicktrav (Nick Travers) wrote…

I don't have strong opinions on this, though wanted to mention - any objections to making the metamorphic variant the default? It would be good from a test coverage perspective to have tests using the encryption at rest code. For most tests, I'd assume encryption vs. not-encryption is just an implementation detail that shouldn't matter.

Given this is more of a "test philosophy" type thing, I'll defer to y'all.

I'd definitely prefer if metamorphic/random were the default, but I don't have a sense of how many tests rely on not having encryption (since "encryption disabled" is the current default).

@tbg @srosenberg Do you have a sense of how many tests could break if we changed the default? I suppose we could change it anyway and when tests do fail because of this, they would have an easy fix: just set EncryptionSupport in the TestSpec. That would be a good thing, as it would make the assumption explicit.

[tbg]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @srosenberg and @tbg)

---

-- commits line 10 at r1:

Previously, renatolabs wrote…

I liked the metamorphic idea and pushed a possible implementation of that. What I liked about this is that, in the previous approach, we were recommending devs to checkout a local copy of the test in order to run a test that supported "EncryptionAllowed" (random). But if we pass a probability, we can say probability=0 without having to change the code. I also renamed --encryption to --metamorphic-encryption-probability to make the intention clear that this parameter only controls encryption for tests that opted-in to metamorphic encryption.

Yeah, you're right, that solves our main problem. I'm sold.

[srosenberg]

Reviewed 20 of 20 files at r2, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @tbg)

---

pkg/cmd/roachtest/registry/encryption.go line 38 at r2 (raw file):

Previously, renatolabs wrote…

I'd definitely prefer if metamorphic/random were the default, but I don't have a sense of how many tests rely on not having encryption (since "encryption disabled" is the current default).

@tbg @srosenberg Do you have a sense of how many tests could break if we changed the default? I suppose we could change it anyway and when tests do fail because of this, they would have an easy fix: just set EncryptionSupport in the TestSpec. That would be a good thing, as it would make the assumption explicit.

I don't have a good intuition for how many tests may break if we enable it. We could manually trigger a full run in GCP and one in AWS to sample the number of new failures. If it turns out to be high, we could do this piecemeal to reduce the rate of new issues created.

[renatolabs]

bors r=nicktrav,srosenberg

TFRT!

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from aac8302 to blathers/backport-release-22.1-81483: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}