Merge #83703

83703: roachtest,roachprod: detect tenant-scope certs via help, unskip tests r=rimadeodhar,cucaroach a=stevendanna

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.

Fixes #82926

Release note: None

Co-authored-by: Steven Danna <[email protected]>

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 {

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,20 +81,26 @@ 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)
 	}
 	tn.createTenantCert(ctx, t, c)
 	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))

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",

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 = "----------------------------------------"