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

feat: replace native-tls with rustls #701

Merged
merged 4 commits into from
Mar 9, 2023
Merged

Conversation

dust1
Copy link
Contributor

@dust1 dust1 commented Mar 6, 2023

Which issue does this PR close?

Closes #662

Rationale for this change

Replace native-tls with rustls

What changes are included in this PR?

edit Cargo.toml

Are there any user-facing changes?

none

How does this change test

none

@dust1
Copy link
Contributor Author

dust1 commented Mar 7, 2023

@jiacai2050 I tried to modify the reqwest dependency of the meta_client module. It looks like it will compile.
But I found that the table_kv module's reqwest comes from the obkv-table-client-rs module.
Do I also need to submit a PR in the obkv-table-client-rs project?

@ShiKaiWi
Copy link
Member

ShiKaiWi commented Mar 7, 2023

@jiacai2050 I tried to modify the reqwest dependency of the meta_client module. It looks like it will compile. But I found that the table_kv module's reqwest comes from the obkv-table-client-rs module. Do I also need to submit a PR in the obkv-table-client-rs project?

Actually, oceanbase/obkv-table-client-rs#12 has been filed. It will be appreciated if you are willing to address that issue.

meta_client/Cargo.toml Outdated Show resolved Hide resolved
@dust1
Copy link
Contributor Author

dust1 commented Mar 8, 2023

I have a problem.after I modified obkv-table-client-rs's reqwest, it can't find native-tls:

error: package ID specification `native-tls` did not match any packages

and change to rustls:

rustls v0.20.8
├── hyper-rustls v0.23.2
│   └── reqwest v0.11.14
│       └── obkv-table-client-rs v0.1.0 (/Users/kous/myProjects/rustproject/obkv-table-client-rs)
├── reqwest v0.11.14 (*)
└── tokio-rustls v0.23.4
    ├── hyper-rustls v0.23.2 (*)
    └── reqwest v0.11.14 (*)

But when I changed rev in ceresdb‘s obkv-table-client-rs. It depends on the native-tls.

native-tls v0.2.10
├── hyper-tls v0.5.0
│   └── reqwest v0.11.13
│       ├── obkv-table-client-rs v0.1.0 (https://github.com/oceanbase/obkv-table-client-rs.git?rev=f823d35880022402e816dc3d4a4cb64056f39bff#f823d358)
│       │   └── table_kv v1.0.0 (/Users/kous/myProjects/rustproject/ceresdb/components/table_kv)
...

do I need to add [features] in obkv-table-rs?

@jiacai2050
Copy link
Contributor

jiacai2050 commented Mar 9, 2023

I have a problem.after I modified obkv-table-client-rs's reqwest, it can't find native-tls:

Isn't this expected? your PR remove native-tls.

do I need to add [features] in obkv-table-rs?

No need.

Could you try cargo clean before cargo tree?

@dust1
Copy link
Contributor Author

dust1 commented Mar 9, 2023

Could you try cargo clean before cargo tree?

yes, i try do it. But obkv-table-client-rs still relies on native tls.😰

@jiacai2050
Copy link
Contributor

I will pull your branch to check in my local dev.

@jiacai2050 jiacai2050 changed the title edit: Remove native-tls dependence feat: replace native-tls with rustls Mar 9, 2023
@jiacai2050 jiacai2050 marked this pull request as ready for review March 9, 2023 09:46
@jiacai2050
Copy link
Contributor

jiacai2050 commented Mar 9, 2023

I remove native-tls from oss-rust-sdk, then native-tls is completely gone in CeresDB. 🚀

cargo tree -i -p native-tls
error: package ID specification `native-tls` did not match any packages

Merge when CI pass. Thanks for your contribution. 👍

Copy link
Contributor

@jiacai2050 jiacai2050 left a comment

Choose a reason for hiding this comment

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

LGTM

@jiacai2050 jiacai2050 added this pull request to the merge queue Mar 9, 2023
Merged via the queue into apache:main with commit 49fadb1 Mar 9, 2023
@dust1
Copy link
Contributor Author

dust1 commented Mar 9, 2023

It seems that I was so focused on this problem that I didn't eliminate the others first. thank you😃

@dust1 dust1 deleted the issue662 branch March 9, 2023 10:50
chunshao90 pushed a commit to chunshao90/ceresdb that referenced this pull request May 15, 2023
* edit: try to open rustls in meta_client/request

* edit

* edit: remove reqwest dep in meta_client

* bump oss-sdk, remove native-tls

---------

Co-authored-by: jiacai2050 <dev@liujiacai.net>
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.

Remove native-tls dependence
3 participants