From c1202a8dd6d583c716c32b8443649563d6323574 Mon Sep 17 00:00:00 2001 From: Steven Danna Date: Fri, 1 Jul 2022 16:35:43 +0100 Subject: [PATCH] roachtest,roachprod: detect tenant-scope certs via help, unskip tests The version comparison code to detect tenant scoped certificates was still incorrect because the "alpha" portion of semver versions is compare lexicographically. Since our alpha versions contain a count of commits, this broke when we hit commit 1000 since the last tag. Here, we search the help on the command to see if it supports the tenant-scoped certs flag. If it does, we assume we will need them. There is duplication between the multitenant tests and roachprod still that we should clean up. Release note: None --- .../roachtest/tests/multitenant_fairness.go | 10 ++-- pkg/cmd/roachtest/tests/multitenant_utils.go | 21 +++++--- pkg/roachprod/install/BUILD.bazel | 1 - pkg/roachprod/install/cluster_synced.go | 49 +++++++------------ 4 files changed, 35 insertions(+), 46 deletions(-) diff --git a/pkg/cmd/roachtest/tests/multitenant_fairness.go b/pkg/cmd/roachtest/tests/multitenant_fairness.go index d0d3b4495036..8a035dc2e8c1 100644 --- a/pkg/cmd/roachtest/tests/multitenant_fairness.go +++ b/pkg/cmd/roachtest/tests/multitenant_fairness.go @@ -64,9 +64,7 @@ func registerMultiTenantFairness(r registry.Registry) { s.maxLoadOps = 100_000 r.Add(registry.TestSpec{ - Name: fmt.Sprintf("multitenant/fairness/kv/%s/%s", s.name, acStr[s.acEnabled]), - // TODO(cucaroach): remove this once #82926 is resolved. - Skip: "#82926", + Name: fmt.Sprintf("multitenant/fairness/kv/%s/%s", s.name, acStr[s.acEnabled]), Cluster: r.MakeClusterSpec(5), Owner: registry.OwnerSQLQueries, NonReleaseBlocker: false, @@ -95,9 +93,7 @@ func registerMultiTenantFairness(r registry.Registry) { s.maxLoadOps = 1000 r.Add(registry.TestSpec{ - Name: fmt.Sprintf("multitenant/fairness/store/%s/%s", s.name, acStr[s.acEnabled]), - // TODO(cucaroach): remove this once #82926 is resolved. - Skip: "#82926", + Name: fmt.Sprintf("multitenant/fairness/store/%s/%s", s.name, acStr[s.acEnabled]), Cluster: r.MakeClusterSpec(5), Owner: registry.OwnerSQLQueries, NonReleaseBlocker: false, @@ -162,7 +158,7 @@ func runMultiTenantFairness( const ( tenantBaseID = 11 tenantBaseHTTPPort = 8081 - tenantBaseSQLPort = 26257 + tenantBaseSQLPort = 26259 ) tenantHTTPPort := func(offset int) int { diff --git a/pkg/cmd/roachtest/tests/multitenant_utils.go b/pkg/cmd/roachtest/tests/multitenant_utils.go index 6d81200e0117..2cf2fea5d8fc 100644 --- a/pkg/cmd/roachtest/tests/multitenant_utils.go +++ b/pkg/cmd/roachtest/tests/multitenant_utils.go @@ -25,7 +25,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/roachprod" "github.com/cockroachdb/cockroach/pkg/security/username" "github.com/cockroachdb/cockroach/pkg/testutils" - "github.com/cockroachdb/cockroach/pkg/util/version" "github.com/stretchr/testify/require" ) @@ -82,13 +81,7 @@ func createTenantNode( node: node, sqlPort: sqlPort, } - n := c.Node(1) - versionStr, err := fetchCockroachVersion(ctx, t.L(), c, n[0]) - v := version.MustParse(versionStr) - require.NoError(t, err) - // Tenant scoped certificates were introduced in version 22.2. - tenantScopeRequiredVersion := version.MustParse("v22.2.0-alpha.00000000-746-gc030b8b6dc") - if v.AtLeast(tenantScopeRequiredVersion) { + if tn.cockroachBinSupportsTenantScope(ctx, c) { err := tn.recreateClientCertsWithTenantScope(ctx, c, createOptions.otherTenantIDs) require.NoError(t, err) } @@ -96,6 +89,18 @@ func createTenantNode( return tn } +// 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 (tn *tenantNode) cockroachBinSupportsTenantScope(ctx context.Context, c cluster.Cluster) bool { + err := c.RunE(ctx, c.Node(tn.node), "./cockroach cert create-client --help | grep '\\--tenant-scope'") + return err == nil +} + func (tn *tenantNode) createTenantCert(ctx context.Context, t test.Test, c cluster.Cluster) { var names []string eips, err := c.ExternalIP(ctx, t.L(), c.Node(tn.node)) diff --git a/pkg/roachprod/install/BUILD.bazel b/pkg/roachprod/install/BUILD.bazel index ad25e3ca9d5c..6d599709c136 100644 --- a/pkg/roachprod/install/BUILD.bazel +++ b/pkg/roachprod/install/BUILD.bazel @@ -36,7 +36,6 @@ go_library( "//pkg/util/retry", "//pkg/util/syncutil", "//pkg/util/timeutil", - "//pkg/util/version", "@com_github_alessio_shellescape//:shellescape", "@com_github_cockroachdb_errors//:errors", "@org_golang_x_sync//errgroup", diff --git a/pkg/roachprod/install/cluster_synced.go b/pkg/roachprod/install/cluster_synced.go index ae75398735ab..3f09950faed7 100644 --- a/pkg/roachprod/install/cluster_synced.go +++ b/pkg/roachprod/install/cluster_synced.go @@ -44,7 +44,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/retry" "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" ) @@ -1182,17 +1181,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) } @@ -1228,6 +1218,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. @@ -1375,25 +1383,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 = "----------------------------------------"