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

[WIP] Add Rust secure examples #4127

Merged
merged 5 commits into from Dec 12, 2018

Conversation

Projects
None yet
6 participants
@couchand
Copy link
Contributor

couchand commented Dec 3, 2018

This adds the code for secure examples in Rust, in addition to making a few tweaks to the examples. Namely, an import that had a spurious self:: now refers directly to the imported postgres crate, and the function trait bound on the execute_txn method has been changed from FnMut to Fn to better reflect the semantics of this function.

couchand added some commits Dec 3, 2018

Change fn trait bound in rust transaction example
Previously, the trait bound on the `execute_txn` function was
`FnMut`, which allows the function to mutate external state
in the course of executing the transaction.  This is at odds
with the intended semantics of this retry loop, which expects
that the behavior is idempotent.

OTOH, there are valid use-cases for needing to mutate state,
for instance to keep track of the retry count.  But since the
purpose of this code sample is as an example for normal use,
this seems to be the right trait bound.

@couchand couchand requested review from tbg , nvanbenschoten , jseldess and sploiselle Dec 3, 2018

@knz knz added the in progress label Dec 3, 2018

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

cockroach-teamcity commented Dec 3, 2018

This change is Reviewable

@couchand couchand force-pushed the rust-secure-examples branch from 5c0d168 to cde1b5c Dec 3, 2018

@nvanbenschoten
Copy link
Member

nvanbenschoten left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


_includes/v2.1/app/basic-sample.rs, line 9 at r2 (raw file):

fn main() {
    let mut connector_builder = SslConnectorBuilder::new(SslMethod::tls()).unwrap();

nit: s/connection_builder/conn_builder/

Or change conn below.


_includes/v2.1/app/basic-sample.rs, line 13 at r2 (raw file):

    connector_builder.set_certificate_chain_file("certs/client.maxroach.crt");
    connector_builder.set_private_key_file("certs/client.maxroach.key", X509_FILETYPE_PEM);
    let mut ssl = OpenSsl::new().unwrap();

Can this be replaced by OpenSsl::with_connector(builder.build());?

@couchand
Copy link
Contributor

couchand left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


_includes/v2.1/app/basic-sample.rs, line 9 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: s/connection_builder/conn_builder/

Or change conn below.

It's not building the conn, it's building the connector field of the OpenSsl config.


_includes/v2.1/app/basic-sample.rs, line 13 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Can this be replaced by OpenSsl::with_connector(builder.build());?

I know I saw that with_connector method somewhere in my explorations of this, but wasn't able to get it work with the version dancing I was doing. But now I'm not even able to find a reference to that anywhere... Where is that defined?

@couchand couchand requested review from rmloveland and removed request for jseldess Dec 3, 2018

@sploiselle sploiselle self-assigned this Dec 3, 2018

@rmloveland
Copy link
Contributor

rmloveland left a comment

LGTM! (I admit I have not tested this code but I assume you have while writing it!) Once the code looks good to you and Nathan, I or someone else on the docs team can edit the surrounding text with the boilerplate text includes for setting up secure clusters, etc.

@nvanbenschoten
Copy link
Member

nvanbenschoten left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


_includes/v2.1/app/basic-sample.rs, line 9 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

It's not building the conn, it's building the connector field of the OpenSsl config.

Ah, good point.


_includes/v2.1/app/basic-sample.rs, line 13 at r2 (raw file):

Previously, couchand (Andrew Couch) wrote…

I know I saw that with_connector method somewhere in my explorations of this, but wasn't able to get it work with the version dancing I was doing. But now I'm not even able to find a reference to that anywhere... Where is that defined?

It's defined in the code. It looks like all of this is super new so https://docs.rs/postgres/0.15.2/postgres/ doesn't look like it's up to date.

@couchand
Copy link
Contributor

couchand left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


_includes/v2.1/app/basic-sample.rs, line 13 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

It's defined in the code. It looks like all of this is super new so https://docs.rs/postgres/0.15.2/postgres/ doesn't look like it's up to date.

Ah, I see, thanks. Yes, ten months of changes it looks like: sfackler/rust-postgres@0516cb9

Seems like (even though it's ugly currently), it should work with the latest API that's published to Cargo. Then we can update when the new version gets published, or perhaps it's worth adding a comment to the code notifying the reader to the change? Actually that's probably really wise...

@sploiselle

This comment has been minimized.

Copy link
Collaborator

sploiselle commented Dec 12, 2018

@couchand PTAL once TC puts up the AWS preview link

@couchand

This comment has been minimized.

Copy link
Contributor

couchand commented Dec 12, 2018

Ok I updated it with a comment referring to the impending API changes. Once that all settles I'll try to remember to come back and fix this up.

@sploiselle sploiselle changed the title Add Rust secure examples [WIP] Add Rust secure examples Dec 12, 2018

@nvanbenschoten
Copy link
Member

nvanbenschoten left a comment

Code LGTM

@couchand

This comment has been minimized.

Copy link
Contributor

couchand commented Dec 12, 2018

TFTR @nvanbenschoten

@sploiselle it's ready to go from my end, thanks!

@sploiselle sploiselle merged commit 23584d8 into master Dec 12, 2018

2 checks passed

GitHub CI (Docs) TeamCity build finished
Details
code-review/reviewable Review complete: 0 of 0 LGTMs obtained
Details

@sploiselle sploiselle removed the in progress label Dec 12, 2018

@sploiselle sploiselle deleted the rust-secure-examples branch Dec 12, 2018

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