From 570056f587af96e246aa2644088545ba0d1edff9 Mon Sep 17 00:00:00 2001 From: Alex Barganier Date: Sun, 7 Aug 2022 14:28:09 -0400 Subject: [PATCH] roachprod,roachtest: make `mt` roachtests run secure This PR updates the `acceptance/multitenant` and `multitenant-upgrade` roachtests to run in secure mode, now that the necessary infra. to run multitenant roachtests in secure mode has been added to roachprod. It also updates the roachprod setup to detect whether or not the cockroach binary supports tenant-scoped client certificates via the `--help` command's output. Release note: none --- pkg/cmd/roachtest/tests/acceptance.go | 1 - pkg/cmd/roachtest/tests/multitenant.go | 2 +- .../roachtest/tests/multitenant_upgrade.go | 2 +- pkg/roachprod/install/cluster_synced.go | 49 +++++++------------ 4 files changed, 21 insertions(+), 33 deletions(-) diff --git a/pkg/cmd/roachtest/tests/acceptance.go b/pkg/cmd/roachtest/tests/acceptance.go index 9cefbd36fea0..d669a91a2525 100644 --- a/pkg/cmd/roachtest/tests/acceptance.go +++ b/pkg/cmd/roachtest/tests/acceptance.go @@ -63,7 +63,6 @@ func registerAcceptance(r registry.Registry) { registry.OwnerMultiTenant: { { name: "multitenant", - skip: "https://github.com/cockroachdb/cockroach/issues/81506", fn: runAcceptanceMultitenant, }, }, diff --git a/pkg/cmd/roachtest/tests/multitenant.go b/pkg/cmd/roachtest/tests/multitenant.go index f39355b19d64..f0ede12aa272 100644 --- a/pkg/cmd/roachtest/tests/multitenant.go +++ b/pkg/cmd/roachtest/tests/multitenant.go @@ -25,7 +25,7 @@ import ( func runAcceptanceMultitenant(ctx context.Context, t test.Test, c cluster.Cluster) { c.Put(ctx, t.Cockroach(), "./cockroach") - c.Start(ctx, t.L(), option.DefaultStartOpts(), install.MakeClusterSettings(), c.All()) + c.Start(ctx, t.L(), option.DefaultStartOpts(), install.MakeClusterSettings(install.SecureOption(true)), c.All()) const tenantID = 123 { diff --git a/pkg/cmd/roachtest/tests/multitenant_upgrade.go b/pkg/cmd/roachtest/tests/multitenant_upgrade.go index d09db437c3f2..45e31c3bddc2 100644 --- a/pkg/cmd/roachtest/tests/multitenant_upgrade.go +++ b/pkg/cmd/roachtest/tests/multitenant_upgrade.go @@ -69,7 +69,7 @@ func runMultiTenantUpgrade(ctx context.Context, t test.Test, c cluster.Cluster, kvNodes := c.Node(1) - settings := install.MakeClusterSettings(install.BinaryOption(predecessorBinary)) + settings := install.MakeClusterSettings(install.BinaryOption(predecessorBinary), install.SecureOption(true)) c.Start(ctx, t.L(), option.DefaultStartOpts(), settings, kvNodes) tenantStartOpt := createTenantOtherTenantIDs([]int{11, 12, 13, 14}) diff --git a/pkg/roachprod/install/cluster_synced.go b/pkg/roachprod/install/cluster_synced.go index f5d98356699a..1f8a0ce73d14 100644 --- a/pkg/roachprod/install/cluster_synced.go +++ b/pkg/roachprod/install/cluster_synced.go @@ -43,7 +43,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/syncutil" "github.com/cockroachdb/cockroach/pkg/util/timeutil" - "github.com/cockroachdb/cockroach/pkg/util/version" "github.com/cockroachdb/errors" "golang.org/x/sync/errgroup" ) @@ -1138,17 +1137,8 @@ func (c *SyncedCluster) createTenantCertBundle( } defer sess.Close() - // NB: We compare against the alpha version here because in semver v22.2.0 > v22.2.0-alpha. - tenantScopedCertsVersion := version.MustParse("v22.2.0-alpha.00000000-746-gc030b8b6dc") - var useTenantScopedCerts bool - if v, err := getCockroachVersion(ctx, c, node); err != nil { - l.Printf("unknown cockroach binary version on host cluster, using tenant scoped certs by default") - useTenantScopedCerts = true - } else { - useTenantScopedCerts = v.AtLeast(tenantScopedCertsVersion) - } var tenantScopeArg string - if useTenantScopedCerts { + if c.cockroachBinSupportsTenantScope(ctx, node) { tenantScopeArg = fmt.Sprintf("--tenant-scope %d", tenantID) } @@ -1184,6 +1174,24 @@ tar cvf %[5]s $CERT_DIR }) } +// cockroachBinSupportsTenantScope is a hack to figure out if the version of +// cockroach on the node supports tenant scoped certificates. We can't use a +// version comparison here because we need to compare alpha build versions which +// are compared lexicographically. This is a problem because our alpha versions +// contain an integer count of commits, which does not sort correctly. Once +// this feature ships in a release, it will be easier to do a version comparison +// on whether this command line flag is supported. +func (c *SyncedCluster) cockroachBinSupportsTenantScope(ctx context.Context, node Node) bool { + sess, err := c.newSession(node) + if err != nil { + return false + } + defer sess.Close() + + cmd := fmt.Sprintf("%s cert create-client --help | grep '\\--tenant-scope'", cockroachNodeBinary(c, node)) + return sess.Run(ctx, cmd) == nil +} + // getFile retrieves the given file from the first node in the cluster. The // filename is assumed to be relative from the home directory of the node's // user. @@ -1331,25 +1339,6 @@ func (c *SyncedCluster) distributeLocalCertsTar( }) } -func getCockroachVersion( - ctx context.Context, c *SyncedCluster, node Node, -) (*version.Version, error) { - sess, err := c.newSession(node) - if err != nil { - return nil, err - } - defer sess.Close() - - cmd := fmt.Sprintf("%s version --build-tag", cockroachNodeBinary(c, node)) - out, err := sess.CombinedOutput(ctx, cmd) - if err != nil { - return nil, errors.Wrapf(err, "~ %s\n%s", cmd, out) - } - - verString := strings.TrimSpace(string(out)) - return version.Parse(verString) -} - const progressDone = "=======================================>" const progressTodo = "----------------------------------------"