opened
asubiotto
username_0
The server would previously run in insecure mode. This change makes the test
environment more in line with what a production environment would be like.

This change is transparent to the client as the PGURL used is returned through
a TestServer level method, which now includes secure connection options.
testserver: run cockroach server in secure mode
issue
created
asubiotto
username_0
The only thing to be wary of is that a caller using a custom pgurl will need to change its behavior. This might justify the introduction of parameters to make insecure optional but I haven't seen a callsite where this is true yet.
comment
created
asubiotto
username_0
This package is currently written in order to have an easy way to spin up a full cockroach server, this change is in line with that goal. If we want to add this in-memory functionality, I would prefer have tests choose whether they want to use on-disk or in-memory storage rather than replacing the current code. In any case, I think this is beyond the scope of this PR and is an argument more about the purpose of this package.
comment
created
andreimatei
username_1
What I'm saying is that this patch is going in a wrong direction. Its scope doesn't matter.
comment
created
asubiotto
username_0
We want to verify that simple ORM interactions work in an end-to-end multitenant environment by using this package in `examples-orms`. It's possible that we're trying to do too much. @username_2 what do you think?

I think we both agree that this package is supposed to provide an easy way to test against a cockroach server, but we fundamentally disagree on the scope. Part of the problem is that the purpose of this package isn't clearly stated anywhere, so there is no alternative but read into the code. This package already does plenty of involved magic in order to achieve this, which makes me believe that this magic is the value proposition of this package. I think ultimately, the package owner should decide on what the scope is. @username_3 I think this is you, could you weigh in?
comment
created
tbg
username_2
I agree that we should leave the defaults unchanged to avoid breaking existing users of this package, but other than that I don't quite understand @username_1's point. This package exists to make testing easier. Testing secure connections is something you may want to do and greasing that is not a step in the wrong direction as far as I can tell. That we're adding tenancy to it (down the road) can be argued about as a matter of style, but note that our main cockroach binary does expose tenancy functionality for the same reason I think it makes sense here -- keeping things simple. There's just no need to make it more complicated by maintaining two pieces of code when one will always subsume the other.

Ultimately, I would say this is @username_3' call. I know [he's fine with TLS for examples-ORMs](https://github.com/cockroachdb/cockroach/issues/49109#issuecomment-632083285), but possibly there's a reservation against changing `cockroach-go` - I wouldn't assume so but let's see. @username_3?
comment
created
rafiss
username_3
I agree, and I would view as connecting in secure mode as a fundamental part of "query processing."
comment
created
andreimatei
username_1
I don't think we can break anybody, can we? There was no way to construct a URL by hand since you can't get your hands on the server's port, can you?
So if y'all love secure mode so much, I don't think that's the argument for not making it the default.

FWIW, I only buy the https://github.com/cockroachdb/docs/issues/7258 argument to a point. We're pretending like we're having the client test interact with a secure cluster, but we're generating the certificates for them. We're not actually asking the client to go through the steps normally required for producing and using a certificate so, for example, if another TLS change comes along and causes some client certs to not be accepted by the server, then these tests wouldn't tell you anything about it.
comment