Skip to content
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.

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

sql: use embedded certs #3310

Merged
merged 3 commits into from
Dec 4, 2015
Merged

sql: use embedded certs #3310

merged 3 commits into from
Dec 4, 2015

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Dec 3, 2015

Fixes nightly sqllogictest.

Review on Reviewable

@mberhault
Copy link
Contributor

have you verified that a standalone sqllogictest binary works?

tmpKeyPath := tempRestrictedCopy(t.T, keyPath, tempDir)
// Copy these assets to disk from embedded strings, so this test can
// run from a standalone binary.
tempCAPath, _ := tempRestrictedCopy(t.T, tempDir, caPath, "TestLogic_ca")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than passing in the prefix for the temp file, you could derive it from the source path using filepath.Base().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true, but in this case it's tagged with the test name, so I'd rather leave it as-is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see why that matters, but ok, this isn't very important.

@petermattis
Copy link
Collaborator

LGTM, though we might want something that requires a bit less boilerplate per test since other tests might need this in the future when we move to using pgwire more widely. I'm thinking of the acceptance tests as one example.

@tamird
Copy link
Contributor Author

tamird commented Dec 3, 2015

I think this is pretty close to the minimum boilerplate that can be had. We can export tempRestrictedCopy for other tests, but that's about it.

@petermattis
Copy link
Collaborator

I disagree about this being the minimum boilerplate. There could be a single function which creates a temporary directory and writes the embedded certs out to it with the appropriate permissions and returns the lib/pq parameters already encoded.

@tamird
Copy link
Contributor Author

tamird commented Dec 3, 2015

Oh, sure, if you wanted to reduce the boilerplate all the way out to getting a lib/pq-specific thing.

@tamird
Copy link
Contributor Author

tamird commented Dec 3, 2015

@mberhault testing this locally resulted in serious static-binary-related sadness. I'm still trying to get to the bottom of it with a minimal repro, but for now lib/pq calls user.Current() which panics in a static sql.test binary.

@tamird
Copy link
Contributor Author

tamird commented Dec 4, 2015

golang/go#13470

@tamird
Copy link
Contributor Author

tamird commented Dec 4, 2015

OK, So I think the segfault issue now only affects TestPGWire, which we don't run in a static binary, and logic test does pass in the static binary in my local docker container. I'm going to merge this and hope for the best.

@tbg
Copy link
Member

tbg commented Dec 4, 2015

LGTM

On Fri, Dec 4, 2015, 08:20 Tamir Duberstein notifications@github.com
wrote:

OK, So I think the segfault issue now only affects PGWire test, which we
don't run in a static binary, and logic test does pass in the static binary
in my local docker container. I'm going to merge this and hope for the best.


Reply to this email directly or view it on GitHub
#3310 (comment)
.

@petermattis
Copy link
Collaborator

@tamird FYI, I just noticed that the benchmarks in sql/bench_test.go are broken due to using non-SSL connections.

@tamird
Copy link
Contributor Author

tamird commented Dec 4, 2015

I'm about to get on a plane, but yeah that seems bad. Might be tricky to fix since local postgres will need certs. I'll take a look in the air.

tamird added a commit that referenced this pull request Dec 4, 2015
@tamird tamird merged commit 80e77b1 into cockroachdb:master Dec 4, 2015
@tamird tamird deleted the pgwire-logictest branch December 4, 2015 16:50
@tbg tbg mentioned this pull request Dec 15, 2015
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.

4 participants