Skip to content

Test connecting tenants via password over TLS through proxy#121

Closed
tbg wants to merge 1 commit intomasterfrom
pw
Closed

Test connecting tenants via password over TLS through proxy#121
tbg wants to merge 1 commit intomasterfrom
pw

Conversation

@tbg
Copy link
Member

@tbg tbg commented Sep 3, 2020

This commit adds infrastructure for testing secure connections through
the multi-tenancy proxy to a standalone SQL server. This works "out of
the box" for essentially those tests for which we previously had client
certificates working.

The tests that fail here are GoPG, Hibernate, Sequelize, Django and they
all fail for the basic reason that they don't support secure query
strings, at least not in the way they are supplied right now. To
really find out whether they have issues with multi-tenancy (or even
client certs), we need to customize the way they receive the connection
parameters. This is not something I plan to investigate.

Closes cockroachdb/cockroach#52413.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tbg tbg requested a review from rafiss September 3, 2020 13:21
@tbg
Copy link
Member Author

tbg commented Sep 8, 2020

Friendly ping.

// with this invocation?
//
// ./docker.sh make deps
// ./docker.sh go test -v -run ActiveRecord ./testing
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird.. mine also fails locally, but I get an error:

    main_test.go:296: Get "http://localhost:6543/ping/": dial tcp 127.0.0.1:6543: connect: connection refused
=== RUN   TestActiveRecord/SystemTenant/SecondRun
make: Entering directory '/home/rafiss/go/src/github.com/cockroachdb/examples-orms/ruby/activerecord'
panic: test timed out after 10m0s

rake aborted!
PG::ConnectionBad: could not connect to server: Connection refused
	Is the server running on host "localhost" (127.0.0.1) and accepting
	TCP/IP connections on port 34497?
could not connect to server: Cannot assign requested address
	Is the server running on host "localhost" (::1) and accepting
	TCP/IP connections on port 34497?

@tbg
Copy link
Member Author

tbg commented Sep 10, 2020

PTAL

Copy link
Contributor

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

LGTM, though perhaps we could change the TODO(during review) to a different phrasing. I'd like to keep your notes on the issue there, but just rephrased so it doesn't seem like the comment fell through the cracks during a review.

This commit adds infrastructure for testing secure connections through
the multi-tenancy proxy to a standalone SQL server. This works "out of
the box" for essentially those tests for which we previously had client
certificates working.

The tests that fail here are GoPG, Hibernate, Sequelize, Django and they
all fail for the basic reason that they don't support secure query
strings, at least not in the way they are supplied right now. To
*really* find out whether they have issues with multi-tenancy (or even
client certs), we need to customize the way they receive the connection
parameters. This is not something I plan to investigate.

Closes cockroachdb/cockroach#52413.
@tbg
Copy link
Member Author

tbg commented Oct 1, 2020

LGTM, though perhaps we could change the TODO(during review) to a different phrasing. I'd like to keep your notes on the issue there, but just rephrased so it doesn't seem like the comment fell through the cracks during a review.

Oh, yep, that was an oversight! Changed to a TODO(rafiss).

Copy link
Contributor

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

thanks for the work here!

@tbg
Copy link
Member Author

tbg commented Oct 2, 2020

Uh, the build has been running for 20h! I only changed a comment. Going to restart it and 🤞 but is this something you've seen before? It's sitting in

[12:36:05]
[TestActiveRecord/insecure/RegularTenant/SecondRun] TestActiveRecord/insecure/RegularTenant/SecondRun
[12:36:05]
[TestActiveRecord/insecure/RegularTenant/SecondRun] TestActiveRecord/insecure/RegularTenant/SecondRun/RetrieveFromAPIAfterRestart (running for 20h:38m:04s)
[12:36:05]
[TestActiveRecord/insecure/RegularTenant/SecondRun/RetrieveFromAPIAfterRestart] TestActiveRecord/insecure/RegularTenant/SecondRun/RetrieveFromAPIAfterRestart
[12:36:05]
[TestActiveRecord/insecure/RegularTenant/SecondRun/RetrieveFromAPIAfterRestart] TestActiveRecord/insecure/RegularTenant/SecondRun/RetrieveFromAPIAfterRestart/Customers
[12:36:05]
[TestActiveRecord/insecure/RegularTenant/SecondRun/RetrieveFromAPIAfterRestart] TestActiveRecord/insecure/RegularTenant/SecondRun/RetrieveFromAPIAfterRestart/Products
[12:36:05]
[TestActiveRecord/insecure/RegularTenant/SecondRun/RetrieveFromAPIAfterRestart] TestActiveRecord/insecure/RegularTenant/SecondRun/RetrieveFromAPIAfterRestart/Order

@tbg
Copy link
Member Author

tbg commented Oct 2, 2020

Hmm, hanging again. These tests pass on master right now, right?

@rafiss
Copy link
Contributor

rafiss commented Oct 2, 2020

Yikes... yes they are passing on master right now. So either (1) CockroachDB master has changed so that it no longer works with your PR, or (2) the versions of the libraries used by this PR have changed (e.g. always fetching latest) and the new versions don't work.

We have seen (2) happen before but AFAIK we have pinned all versions by now.

@rafiss
Copy link
Contributor

rafiss commented Oct 2, 2020

Here is an older passing test run for this PR: https://teamcity.cockroachdb.com/viewLog.html?buildId=2259861&buildTypeId=Cockroach_ExampleORMsUnitTests&tab=buildLog&branch_Cockroach=%3Cdefault%3E&_focus=325#_state=325

The hanging test is ActiveRecord. Both that previously passing test and the currently hanging test are using activerecord 5.2.4.3 and activerecord-cockroachdb-adapter 5.2.0 (and all other transitive dependencies are the same).

@tbg
Copy link
Member Author

tbg commented Oct 23, 2020

Hmm, this is annoying.

I've decided to abandon this PR for the time being. The hope that we would get some real coverage for multi tenancy from this hasn't materialized, due to most ORMs here not being configurable with SSL as it stands. Instead, @darinpp has tested various ORMs manually and I don't think he's going to lean on this PR at this point.

Sorry to have wasted your time here!

@tbg tbg closed this Oct 23, 2020
@rafiss
Copy link
Contributor

rafiss commented Oct 23, 2020

@tbg No problem!

Just a point of clarification to make sure I'm on the same page. Is this the testing you are referring to? https://docs.google.com/document/d/1cpqBG4fpMiytFMY2s7MD0l7iTamxUnoKok-EhvD9_WY/edit

That is mostly driver testing, not ORM testing. So my outsider opinion is that it would still be nice to test the user experience with ORMs, if possible.

@tbg
Copy link
Member Author

tbg commented Oct 23, 2020

👍 thanks for the clarification. cc @darinpp - if you (or someone on the CC end) wanted to pick this up, it's all yours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sql: add dummy sql proxy to 3node-tenant and Examples-ORMs

3 participants