roachtest: rudimentary support for secure clusters #68101
Conversation
|
This change is |
There was a problem hiding this comment.
the changes to the ORM tests lgtm!
there is one more test that maybe needs to get standardized: connectionlatency
cc @RichardJCai you might be interested in reviewing this
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, 3 of 3 files at r2, 1 of 1 files at r3, 21 of 21 files at r4.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @tbg)
a discussion (no related file):
pkg/cmd/roachtest/cluster.go, line 1759 at r2 (raw file):
return err } // Need to prevent world readable files or lib/pq will complain.
It looks like Get explicitly adds world-readable permission to files after the investigation over in #44843
One interesting follow-up might be to extend Get to take a particular target permission, which would also eliminate the race to "fix" the permission after download.
Rather than manually cobbling together an url, use `url.URL`. Also, add a `--certs-dir` parameter to `pgurl` that allows customizing the location of the certs represented in the returned string. Release note: None
roachtest does not offer real support for secure clusters. In light of cockroachdb#65830 this needs to change. It will be a larger project to make this truly seamless. This is just a first important step: roachtest, when starting a secure cluster, will download the certs so that it can generate the proper SQL connection string for use by `c.Conn`. Importantly, the test (mostly) doesn't have to care whether it's operating against a secure cluster, as long as it doesn't do anything fancy. Parts of roachtest's artifact collection assumes an insecure cluster, so artifacts will be degraded when the cluster is secure. The logs will still be collected, though. Release note: None
The impl on clusterImpl is still used, so it isn't removed yet. Release note: None
A number of ad-hoc mechanisms for handling secure clusters was added a while back for the node-postgres roachtest. Now that roachprod and roachtest have rudimentary support for dealing with secure clusters, we get to remove all of that. I verified that node-postgres passes with this commit. Release note: None
Now that `testuser` certs are created by `roachprod start --secure` we need to update this test to stop doing it itself. Release note: None
There was a problem hiding this comment.
Reviewable status:
a discussion (no related file):
Previously, stevendanna (Steven Danna) wrote…
:lgtm:
TFTRs! I fixed up the connection_latency test, I just needed to remove the certs creation for testuser to get it to pass.
pkg/cmd/roachtest/cluster.go, line 1759 at r2 (raw file):
Previously, stevendanna (Steven Danna) wrote…
It looks like Get explicitly adds world-readable permission to files after the investigation over in #44843
One interesting follow-up might be to extend Get to take a particular target permission, which would also eliminate the race to "fix" the permission after download.
Going to leave as is for now, but adding an option to change the perms seems reasonable.
|
bors r=stevendanna,richardjcai |
|
Build succeeded: |
In cockroachdb#68101, `roachtest` gained rudimentary support for using secure clusters, including setting up local TLS client certificates. However, these local certificates were not removed when a cluster was wiped and reused, causing the client to fail with an authentication error when attempting to contact the new insecure cluster (as seen in e.g. cockroachdb#68261). This patch makes sure the local certificates are removed whenever `roachtest` wipes a cluster. Release note: None
In cockroachdb#68101, `roachtest` gained rudimentary support for using secure clusters, including setting up local TLS client certificates. However, these local certificates were not removed when a cluster was wiped and reused, causing the client to fail with an authentication error when attempting to contact the new insecure cluster (as seen in e.g. cockroachdb#68261). This patch makes sure the local certificates are removed whenever `roachtest` wipes a cluster. Release note: None
In cockroachdb#68101, `roachtest` gained rudimentary support for using secure clusters, including setting up local TLS client certificates. However, these local certificates were not removed when a cluster was wiped and reused, causing the client to fail with an authentication error when attempting to contact the new insecure cluster (as seen in e.g. cockroachdb#68261). This patch makes sure the local certificates are removed whenever `roachtest` wipes a cluster. Release note: None
68272: roachtest: add repro link to roachtest stress CI build r=stevendanna a=erikgrinaker Release note: None --- Unfortunately, there doesn't appear to be a way to get a link to the actual "Custom Build" form in TeamCity, so we have to provide instructions for people to manually click and fill in stuff. 😕 We could instead provide a `curl` command that would trigger a build with appropriate parameters via a REST API call, but that would require the user to set up API authentication tokens, so it's not clear that it's any more convenient. Ideas for improvements are welcome. 68279: roachtest: remove local TLS certs during cluster wipe r=stevendanna a=erikgrinaker In #68101, `roachtest` gained rudimentary support for using secure clusters, including setting up local TLS client certificates. However, these local certificates were not removed when a cluster was wiped and reused, causing the client to fail with an authentication error when attempting to contact the new insecure cluster (as seen in e.g. #68261). This patch makes sure the local certificates are removed whenever `roachtest` wipes a cluster. Resolves #68261. Resolves #68264. Release note: None Co-authored-by: Erik Grinaker <[email protected]>
TL;DR: roachtest can now start a secure cluster and
c.Connwill "just work".