Skip to content

roachtest: fix allow_unsafe_internals edge cases #156795

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 6, 2025
Merged

roachtest: fix allow_unsafe_internals edge cases #156795

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c0b0acf
roachtest: fix network/authentication pgurl
DarrylWong Nov 3, 2025
8913ec0
roachprod: allow opting out of allow_unsafe_internals
DarrylWong Nov 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions pkg/cmd/roachtest/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2721,9 +2721,6 @@ func (c *clusterImpl) InternalPGUrl(
return c.pgURLErr(ctx, l, nodes, opts)
}

// Silence unused warning.
var _ = (&clusterImpl{}).InternalPGUrl
Copy link
Member

@srosenberg srosenberg Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good riddance.


// ExternalPGUrl returns the external Postgres endpoint for the specified nodes.
func (c *clusterImpl) ExternalPGUrl(
ctx context.Context, l *logger.Logger, nodes option.NodeListOption, opts roachprod.PGURLOptions,
Expand Down
6 changes: 5 additions & 1 deletion pkg/cmd/roachtest/tests/copyfrom.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,11 @@ func runCopyFromCRDB(ctx context.Context, t test.Test, c cluster.Cluster, sf int
t.Fatal(err)
}
}
urls, err := c.InternalPGUrl(ctx, t.L(), c.Node(1), roachprod.PGURLOptions{Auth: install.AuthUserPassword})
// psql doesn't support the CRDB specific "allow_unsafe_internals" parameter.
urls, err := c.InternalPGUrl(ctx, t.L(), c.Node(1), roachprod.PGURLOptions{
Auth: install.AuthUserPassword,
DisallowUnsafeInternals: true,
})
require.NoError(t, err)
m := c.NewDeprecatedMonitor(ctx, c.All())
m.Go(func(ctx context.Context) error {
Expand Down
6 changes: 1 addition & 5 deletions pkg/cmd/roachtest/tests/network.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,6 @@ func runNetworkAuthentication(ctx context.Context, t test.Test, c cluster.Cluste
c.Start(ctx, t.L(), startOpts, settings, c.Node(1))
}

t.L().Printf("retrieving server addresses...")
serverUrls, err := c.InternalPGUrl(ctx, t.L(), serverNodes, roachprod.PGURLOptions{Auth: install.AuthUserPassword})
require.NoError(t, err)

t.L().Printf("fetching certs...")
certsDir := fmt.Sprintf("/home/ubuntu/%s", install.CockroachNodeCertsDir)
localCertsDir, err := filepath.Abs("./network-certs")
Expand Down Expand Up @@ -206,7 +202,7 @@ SELECT $1::INT = ALL (
}

// Construct a connection URL to server i.
url := serverUrls[server-1]
url := fmt.Sprintf("{pgurl:%d}", server)
Copy link
Member

@srosenberg srosenberg Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.


// Attempt a client connection to that server.
t.L().Printf("server %d, attempt %d; url: %s\n", server, attempt, url)
Expand Down
5 changes: 3 additions & 2 deletions pkg/cmd/roachtest/tests/pg_regress.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ func initPGRegress(ctx context.Context, t test.Test, c cluster.Cluster) {
}
}

urls, err := c.InternalPGUrl(ctx, t.L(), node, roachprod.PGURLOptions{})
urls, err := c.InternalPGUrl(ctx, t.L(), node, roachprod.PGURLOptions{DisallowUnsafeInternals: true})
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -149,7 +149,8 @@ func runPGRegress(ctx context.Context, t test.Test, c cluster.Cluster) {
initPGRegress(ctx, t, c)

node := c.Node(1)
urls, err := c.InternalPGUrl(ctx, t.L(), node, roachprod.PGURLOptions{})
// psql doesn't support the CRDB specific "allow_unsafe_internals" parameter.
urls, err := c.InternalPGUrl(ctx, t.L(), node, roachprod.PGURLOptions{DisallowUnsafeInternals: true})
if err != nil {
t.Fatal(err)
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/roachprod/failureinjection/failures/failure.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ func (f *GenericFailure) Conn(
if !c.Secure {
authMode = install.AuthRootCert
}
nodeURL := c.NodeURL(ip, desc.Port, "" /* virtualClusterName */, desc.ServiceMode, authMode, "" /* database */)
nodeURL := c.NodeURL(ip, desc.Port, "" /* virtualClusterName */, desc.ServiceMode, authMode, "" /* database */, false /* disallowUnsafeInternals */)
nodeURL = strings.Trim(nodeURL, "'")
pgurl, err := url.Parse(nodeURL)
if err != nil {
Expand Down
4 changes: 2 additions & 2 deletions pkg/roachprod/install/cluster_synced.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2304,7 +2304,7 @@ func (c *SyncedCluster) pgurls(
if err != nil {
return nil, err
}
m[node] = c.NodeURL(host, desc.Port, virtualClusterName, desc.ServiceMode, DefaultAuthMode(), "" /* database */)
m[node] = c.NodeURL(host, desc.Port, virtualClusterName, desc.ServiceMode, DefaultAuthMode(), "" /* database */, false /* disallowUnsafeInternals */)
}
return m, nil
}
Expand Down Expand Up @@ -2347,7 +2347,7 @@ func (c *SyncedCluster) loadBalancerURL(
if err != nil {
return "", err
}
loadBalancerURL := c.NodeURL(address.IP, address.Port, virtualClusterName, descs[0].ServiceMode, auth, "" /* database */)
loadBalancerURL := c.NodeURL(address.IP, address.Port, virtualClusterName, descs[0].ServiceMode, auth, "" /* database */, false /* disallowUnsafeInternals */)
return loadBalancerURL, nil
}

Expand Down
17 changes: 11 additions & 6 deletions pkg/roachprod/install/cockroach.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -805,6 +805,7 @@ func (c *SyncedCluster) NodeURL(
serviceMode ServiceMode,
auth PGAuthMode,
database string,
disallowUnsafeInternals bool,
) string {
var u url.URL
u.Scheme = "postgres"
Expand Down Expand Up @@ -836,7 +837,11 @@ func (c *SyncedCluster) NodeURL(
v.Add("sslmode", "disable")
}

v.Add("allow_unsafe_internals", "true")
// We usually want to allow unsafe internals for testing environments,
// but allow an escape hatch to disallow it, e.g. if we are using psql.
if !disallowUnsafeInternals {
v.Add("allow_unsafe_internals", "true")
}
// We only want to pass an explicit `cluster` name if the user provided one.
if virtualClusterName != "" {
// We can only pass the cluster parameter for shared processes, as SQL server
Expand Down Expand Up @@ -894,7 +899,7 @@ func (c *SyncedCluster) ExecOrInteractiveSQL(
if err != nil {
return err
}
url := c.NodeURL("localhost", desc.Port, virtualClusterName, desc.ServiceMode, authMode, database)
url := c.NodeURL("localhost", desc.Port, virtualClusterName, desc.ServiceMode, authMode, database, false /* disallowUnsafeInternals */)
binary := cockroachNodeBinary(c, c.Nodes[0])
allArgs := []string{binary, "sql", "--url", url}
allArgs = append(allArgs, ssh.Escape(args))
Expand Down Expand Up @@ -926,7 +931,7 @@ func (c *SyncedCluster) ExecSQL(
cmd = fmt.Sprintf(`cd %s ; `, c.localVMDir(node))
}
cmd += SuppressMetamorphicConstantsEnvVar() + " " + cockroachNodeBinary(c, node) + " sql --url " +
c.NodeURL("localhost", desc.Port, virtualClusterName, desc.ServiceMode, authMode, database) + " " +
c.NodeURL("localhost", desc.Port, virtualClusterName, desc.ServiceMode, authMode, database, false /* disallowUnsafeInternals */) + " " +
ssh.Escape(args)
return c.runCmdOnSingleNode(ctx, l, node, cmd, defaultCmdOpts("run-sql"))
})
Expand Down Expand Up @@ -1562,7 +1567,7 @@ func (c *SyncedCluster) generateClusterSettingCmd(
if err != nil {
return "", err
}
url := c.NodeURL("localhost", port, SystemInterfaceName /* virtualClusterName */, ServiceModeShared, AuthRootCert, "" /* database */)
url := c.NodeURL("localhost", port, SystemInterfaceName /* virtualClusterName */, ServiceModeShared, AuthRootCert, "" /* database */, false /* disallowUnsafeInternals */)

// We use `mkdir -p` here since the directory may not exist if an in-memory
// store is used.
Expand All @@ -1584,7 +1589,7 @@ func (c *SyncedCluster) generateInitCmd(ctx context.Context, node Node) (string,
if err != nil {
return "", err
}
url := c.NodeURL("localhost", port, SystemInterfaceName /* virtualClusterName */, ServiceModeShared, AuthRootCert, "" /* database */)
url := c.NodeURL("localhost", port, SystemInterfaceName /* virtualClusterName */, ServiceModeShared, AuthRootCert, "" /* database */, false /* disallowUnsafeInternals */)
binary := cockroachNodeBinary(c, node)
initCmd += fmt.Sprintf(`
if ! test -e %[1]s ; then
Expand Down Expand Up @@ -1802,7 +1807,7 @@ func (c *SyncedCluster) createFixedBackupSchedule(
serviceMode = ServiceModeExternal
}

url := c.NodeURL("localhost", port, startOpts.VirtualClusterName, serviceMode, AuthRootCert, "" /* database */)
url := c.NodeURL("localhost", port, startOpts.VirtualClusterName, serviceMode, AuthRootCert, "" /* database */, false /* disallowUnsafeInternals */)
fullCmd := fmt.Sprintf(`%s COCKROACH_CONNECT_TIMEOUT=%d %s sql --url %s -e %q`,
SuppressMetamorphicConstantsEnvVar(), startSQLTimeout, binary, url, createScheduleCmd)
// Instead of using `c.ExecSQL()`, use `c.runCmdOnSingleNode()`, which allows us to
Expand Down
17 changes: 9 additions & 8 deletions pkg/roachprod/roachprod.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1131,12 +1131,13 @@ func Get(ctx context.Context, l *logger.Logger, clusterName, src, dest string) e
}

type PGURLOptions struct {
Database string
Secure install.SecureOption
External bool
VirtualClusterName string
SQLInstance int
Auth install.PGAuthMode
Database string
Secure install.SecureOption
External bool
VirtualClusterName string
SQLInstance int
Auth install.PGAuthMode
DisallowUnsafeInternals bool
}

// PgURL generates pgurls for the nodes in a cluster.
Expand Down Expand Up @@ -1171,7 +1172,7 @@ func PgURL(
if ip == "" {
return nil, errors.Errorf("empty ip: %v", ips)
}
urls = append(urls, c.NodeURL(ip, desc.Port, opts.VirtualClusterName, desc.ServiceMode, opts.Auth, opts.Database))
urls = append(urls, c.NodeURL(ip, desc.Port, opts.VirtualClusterName, desc.ServiceMode, opts.Auth, opts.Database, opts.DisallowUnsafeInternals))
}
if len(urls) != len(nodes) {
return nil, errors.Errorf("have nodes %v, but urls %v from ips %v", nodes, urls, ips)
Expand Down Expand Up @@ -2951,7 +2952,7 @@ func LoadBalancerPgURL(
if err != nil {
return "", err
}
return c.NodeURL(addr.IP, port, opts.VirtualClusterName, serviceMode, opts.Auth, opts.Database), nil
return c.NodeURL(addr.IP, port, opts.VirtualClusterName, serviceMode, opts.Auth, opts.Database, opts.DisallowUnsafeInternals), nil
}

// LoadBalancerIP resolves the IP of a load balancer serving the
Expand Down
Loading