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

Add sasl transport support #222

Merged
merged 8 commits into from
Mar 7, 2024
Merged

Add sasl transport support #222

merged 8 commits into from
Mar 7, 2024

Conversation

ecederstrand
Copy link
Contributor

Adds pure Python and Cython implementation of sasl transport.

@ethe ethe requested a review from aisk September 7, 2023 06:24
@ethe
Copy link
Member

ethe commented Sep 7, 2023

Thanks, I am reviewing it.

@ethe ethe added enhancement New feature or request good first issue Good for newcomers labels Sep 7, 2023
@ethe ethe self-requested a review September 7, 2023 06:26
setup.py Outdated Show resolved Hide resolved
@aisk
Copy link
Member

aisk commented Sep 7, 2023

I think this change need some test cases to ship with. And since this PR only implment the sasl transport client, maybe we can call some public thrift services with sasl support?

@ecederstrand
Copy link
Contributor Author

ecederstrand commented Sep 7, 2023

I agree that tests would be useful. I don't know of any public thrift services with sasl support, so would really like any suggestions here, or alternative methods for testing this code. We have an internal HBase installation that we tested this against, but a full HBase is quite a task to build for a test setup.

@aisk
Copy link
Member

aisk commented Sep 7, 2023

Fully implment the sasl server transport is the best way.

Another way is starting a TCP server before running this test case, and the server can be a dummpy server which only send a valid thrift response package while accept and read fixed length of packet.

You can dump a thrift packat via wiresharkl or something like it.

@aisk
Copy link
Member

aisk commented Mar 6, 2024

Since this is a new feature and it won't break current users' code, I think we can merge this without testing, if @ecederstrand doesn't have time to finish the work.

@ethe any concerns?

@ethe
Copy link
Member

ethe commented Mar 7, 2024

Since this is a new feature and it won't break current users' code, I think we can merge this without testing, if @ecederstrand doesn't have time to finish the work.

It looks good to me, maybe we could publish a pre-release version that contains this experimental feature.

@aisk aisk merged commit 91b28ea into Thriftpy:master Mar 7, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants