cockroach icon indicating copy to clipboard operation
cockroach copied to clipboard

roachprod: small fixes and refactors for pgurl

Open DarrylWong opened this issue 1 year ago • 3 comments

roachprod: create certs for DefaultUser instead of roach Roachprod start creates certs for the default admin user. Previously this user was named roach so the certs create call created certs for user roach. However, now that the user has changed to roachprod, this no longer works.

This change instead creates certs for DefaultUser, which should reflect any changes to the naming.

roachtest: remove roachtestutil.DefaultPGUrl This helper was created when most tests were running on insecure clusters. This fact led to most urls being the same as no auth had to be done. However, with secure mode being enabled on most tests, this is no longer true and most tests stopped using this helper.

In the rare case it is used, usage varies a lot depending on the opts passed. This defeats the purpose of it being the "default" url. This change replaces DefaultPGUrl calls with roachprod.PGUrl instead.

roachprod: create CockroachNodeCertsDir constant This change creates a constant for the default crdb certs directory and uses it throughout roachprod and roachtest instead of hardcoding "certs".

roachprod: fix internal pgurls to have correct certs-dir and host Previously, internal pgurls would specify the certs-dir as c.localCertsDir, a path is that is only present for the roachtest runner.

Additionally, internal pgurls now use localhost instead of the ip address.

This is done through a new PGURLOption, Localhost, which indicates if localhost or the internal IP should be used. While it would be nice to default to localhost for all internal pgurls, the way c.InternalIP works is by parsing the InternalPgUrl.

Release note: None Epic: None Fixes: None

DarrylWong avatar Feb 16 '24 22:02 DarrylWong

This change is Reviewable

cockroach-teamcity avatar Feb 16 '24 22:02 cockroach-teamcity

Ran all the tests that use internalPgUrl or internalAddr + a 0.2 test run here

😁 Gladly accepting alternative name ideas for the constant:

https://github.com/cockroachdb/cockroach/blob/5df2a4bbbb12d2849a116067eea8e5a04d7e9424/pkg/roachprod/install/cluster_synced.go#L1611

Most of the diff is trivial - just replacing certs with the new constant, but also happy to split this up into four different PRs since most of the work is independent of each other.

DarrylWong avatar Feb 16 '24 22:02 DarrylWong

Ahh, upon further testing, it appears that it's not always okay to default to localhost for InternalPgUrl. Specifically, when we have a workload only node, it doesn't recognize localhost and needs the internal IP. It's not an issue to use the internal IP there since it doesn't look like workload verifies the host name.

This isn't run into at all, since all workload calls use {pgurl}, but for correctness I'll leave it as just an option that the caller will have to specify instead of defaulting to it. InternalPgUrl is only used in two tests anyway.

Nothing should have changed but rerunning internalpgurl related tests just in case.

DarrylWong avatar Feb 21 '24 17:02 DarrylWong

Reran the affected tests.

TFTR!

bors r=renatolabs

DarrylWong avatar Feb 26 '24 15:02 DarrylWong

Build succeeded:

craig[bot] avatar Feb 26 '24 16:02 craig[bot]