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

replace rust-crypto with rust-openssl #3886

Merged
merged 6 commits into from Sep 20, 2019

Conversation

@niedhui
Copy link
Collaborator

niedhui commented Dec 5, 2018

Signed-off-by: niedhui niedhui@gmail.com

What have you changed? (mandatory)

since rust-crypto hasn't been updated for couple of years, replace it with rust-openssl.
and will use it to implement other builtin encryption functions

What are the type of the changes? (mandatory)

Improvement

How has this PR been tested? (mandatory)

unit test

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

no

Does this PR affect tidb-ansible update? (mandatory)

no

Refer to a related PR or issue link (optional)

#3875 #3275

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Dec 5, 2018

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented Dec 6, 2018

Seems that the build fails on CI. Maybe need to install openssl in system?

@niedhui

This comment has been minimized.

Copy link
Collaborator Author

niedhui commented Dec 6, 2018

crate grpcio links BoringSSL, which is incompatible with OpenSSL. sfackler/rust-openssl#765

@overvenus

This comment has been minimized.

Copy link
Contributor

overvenus commented Dec 6, 2018

@niedhui FYI, we release grpcio 0.4.1 recently, which supports openssl.

@niedhui

This comment has been minimized.

Copy link
Collaborator Author

niedhui commented Dec 6, 2018

Good to know it. I'll take a look later. @overvenus @breeswish

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented Dec 6, 2018

Looks nice! Please resolve conflicts.

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented Dec 6, 2018

/run-integration-tests

@niedhui

This comment has been minimized.

Copy link
Collaborator Author

niedhui commented Dec 6, 2018

Ok, CI passed, seems openssl is alread installed on the CI server

some ontes:

  1. after this change, build TiKV will need openssl to be installed
  2. on MacOS, OPENSSL_ROOT_DIR need to be export to run cargo build, eg:
export OPENSSL_ROOT_DIR=$(brew --prefix openssl)

@overvenus can grpcio provide a 'vendor OpenSSL ' feature , like openssl crate?

@overvenus

This comment has been minimized.

Copy link
Contributor

overvenus commented Dec 7, 2018

@niedhui We can enable the vendor feature in TiKV, I think openssl will be statically linked. https://docs.rs/openssl/0.10.15/openssl/#vendored

@niedhui

This comment has been minimized.

Copy link
Collaborator Author

niedhui commented Dec 7, 2018

hi @overvenus
I tried this before, in Cargo.toml

grpcio = { version = "0.4", features = [ "openssl" ] }
openssl = { version = "0.10", features = ["vendored"] }

grpcio will complain to Could NOT find OpenSSL. if we want grpcio to use the static linked openssl, it will need to use openssl-src to tell it where to find openssl.

@ice1000

This comment has been minimized.

Copy link
Contributor

ice1000 commented Dec 7, 2018

@niedhui Is it possible to modify grpcio to achieve what you want?
It's maintained by us.

@niedhui

This comment has been minimized.

Copy link
Collaborator Author

niedhui commented Dec 10, 2018

@ice1000 zero experiences with golang & c . Mabybe I'll try this later.
If openssl as a prerequisite is acceptable, we can process this PR first

@niedhui

This comment has been minimized.

Copy link
Collaborator Author

niedhui commented Dec 11, 2018

had opened PR in grpcio tikv/grpc-rs#262

@Hijiao

This comment has been minimized.

Copy link
Contributor

Hijiao commented Dec 12, 2018

Hi, @niedhui could you please benchmark it and compare it to #3644 ?

@niedhui

This comment has been minimized.

Copy link
Collaborator Author

niedhui commented Dec 12, 2018

hi @Hijiao , we can use https://github.com/briansmith/crypto-bench to benchmark the rust-crypto and openssl. for digest, if input data is small, rust-crypto is a little faster, when input data grows, openssl wins.
below is a simple code I wrote for benchmark md5. on my Mac, when length of data is 2000, we got

MD5/crypto md5          time:   [4.6412 us 4.6596 us 4.6790 us]
MD5/openssl md5         time:   [3.1871 us 3.2029 us 3.2197 us]
#[macro_use]
extern crate criterion;
extern crate crypto;
extern crate hex;
extern crate openssl;

use criterion::{Criterion, Fun};
criterion_group!(benches, criterion_benchmark);
criterion_main!(benches);

fn md5_using_crypto(input: &[u8]) -> Vec<u8> {
    use crypto::{digest::Digest, md5::Md5};
    let mut hasher = Md5::new();
    let mut buff: [u8; 16] = [0; 16];
    hasher.input(input);
    hasher.result(&mut buff);
    hex::encode(buff).into_bytes()
}

fn md5_using_openssl(input: &[u8]) -> Vec<u8> {
    use openssl::hash::{self, MessageDigest};
    hash::hash(MessageDigest::md5(), input)
        .map(|digest| hex::encode(digest).into_bytes())
        .unwrap()
}

fn criterion_benchmark(c: &mut Criterion) {
    let crypto_md5 = Fun::new("crypto md5", |b, total| {
        b.iter(|| {
            let s = vec![0; *total];
            md5_using_crypto(&s);
        })
    });
    let openssl_md5 = Fun::new("openssl md5", |b, total| {
        b.iter(|| {
            let s = vec![0; *total];
            md5_using_openssl(&s);
        })
    });
    let functions = vec![crypto_md5, openssl_md5];
    c.bench_functions("MD5", functions, 2000);
}
Signed-off-by: niedhui <niedhui@gmail.com>
@niedhui niedhui force-pushed the niedhui:niedhui/rust-openssl branch from 09f2356 to abd9561 Sep 4, 2019
@niedhui

This comment has been minimized.

Copy link
Collaborator Author

niedhui commented Sep 4, 2019

friendly ping @AndreMouche, I had refined this PR, PTAL

…st-openssl

Signed-off-by: niedhui <niedhui@gmail.com>

# Conflicts:
#	Cargo.lock
Copy link
Contributor

overvenus left a comment

LGTM, thank you!

…st-openssl

Signed-off-by: niedhui <niedhui@gmail.com>

# Conflicts:
#	Cargo.lock
@niedhui niedhui requested review from overvenus and removed request for Hijiao Sep 12, 2019
@niedhui

This comment has been minimized.

Copy link
Collaborator Author

niedhui commented Sep 12, 2019

@overvenus resolve cargo.lock conflicts, please review again, thanks ~

Copy link
Contributor

overvenus left a comment

👍

@Hoverbear

This comment has been minimized.

Copy link
Member

Hoverbear commented Sep 19, 2019

Hey this fixes! #5313 , cool!

@Hoverbear

This comment has been minimized.

Copy link
Member

Hoverbear commented Sep 19, 2019

/bench

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Sep 20, 2019

@@                               Benchmark Diff                               @@
================================================================================
tidb: f9724dbf3bcb9fc58d0d854391c0ba9f87c3fbd4
--- tikv: f2bb322e6fffd72c26f0ef268f99e0d8441389aa
+++ tikv: a2adebfbf1bc5f7d2d57f6780301b6c9061d797e
pd: 4ca2265d180531191dcfc531e468ab8380737a33
================================================================================
test-1: < oltp_read_write >
    * QPS : 37335.97 ± 0.2965% (std=70.49) delta: 0.99%
    * AvgMs : 137.58 ± 0.3155% (std=0.25) delta: -1.04%
    * PercentileMs99 : 262.64 ± 0.0000% (std=0.00) delta: -1.78%
            
test-2: < oltp_point_select >
    * QPS : 82036.03 ± 0.8237% (std=447.75) delta: 1.21%
    * AvgMs : 3.12 ± 0.8969% (std=0.02) delta: -1.10%
    * PercentileMs99 : 6.60 ± 1.0912% (std=0.06) delta: -0.36%
            
test-3: < oltp_insert >
    * QPS : 21793.72 ± 0.5855% (std=77.86) delta: -0.30%
    * AvgMs : 11.74 ± 0.5963% (std=0.04) delta: 0.30%
    * PercentileMs99 : 23.95 ± 0.0000% (std=0.00) delta: 0.00%
            
test-4: < oltp_update_index >
    * QPS : 17049.35 ± 1.0835% (std=109.32) delta: -0.23%
    * AvgMs : 14.86 ± 0.3095% (std=0.03) delta: -0.08%
    * PercentileMs99 : 30.59 ± 1.0788% (std=0.27) delta: 0.00%
            
test-5: < oltp_update_non_index >
    * QPS : 29080.12 ± 0.6498% (std=121.90) delta: 0.11%
    * AvgMs : 8.80 ± 0.6818% (std=0.04) delta: 0.00%
    * PercentileMs99 : 18.48 ± 1.0715% (std=0.16) delta: -0.71%
            

https://perf.pingcap.com

@Hoverbear

This comment has been minimized.

Copy link
Member

Hoverbear commented Sep 20, 2019

@niedhui Thanks for this. :)

@Hoverbear

This comment has been minimized.

Copy link
Member

Hoverbear commented Sep 20, 2019

/merge

@sre-bot sre-bot added the S: CanMerge label Sep 20, 2019
@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Sep 20, 2019

/run-all-tests

@sre-bot sre-bot merged commit 15082b6 into tikv:master Sep 20, 2019
6 checks passed
6 checks passed
DCO All commits are signed off!
Details
idc-jenkins-ci-tikv/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-cop-push-down-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
@niedhui niedhui deleted the niedhui:niedhui/rust-openssl branch Sep 22, 2019
sticnarf pushed a commit to sticnarf/tikv that referenced this pull request Oct 27, 2019
Signed-off-by: niedhui <niedhui@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.