Skip to content

feat:add tls support fot memcached #5471

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ozewr
Copy link

@ozewr ozewr commented Dec 27, 2024

Which issue does this PR close?

Closes #5419.

Rationale for this change

see #5419

What changes are included in this PR?

Modified the opendal::services::Memcached to support TLS connections.

Are there any user-facing changes?

Users can enable TLS using .tls() and provide the CA file using .cafile().
example:

let memcached = Memcached::default()
        .endpoint(r#"tcp://example.app.local:11211"#)
        .tls(true)
        .cafile(path);

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Would you like to add a test for memcached with tls enabled?

@ozewr
Copy link
Author

ozewr commented Dec 31, 2024

Would you like to add a test for memcached with tls enabled?

No problem, I will complete it later.

@ozewr ozewr requested a review from PsiACE as a code owner January 2, 2025 10:07
@ozewr
Copy link
Author

ozewr commented Jan 2, 2025

Does the Memcached service require any special configuration? During testing, I found many incorrect test results, and some tests occasionally pass while failing at other times during repeated tests. @Xuanwo
Here is my Memcached service configuration:

memcached --protocol=auto -p 11212 --enable-ssl -o ssl_chain_cert=./server.crt,ssl_key=./server.key,ssl_verify_mode=2,ssl_ca_cert=./client_ca/client_ca.crt

@ozewr ozewr requested a review from Xuanwo January 3, 2025 08:02
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thank you for working on this and sorry for the late.

Comment on lines +295 to +302
#[async_trait::async_trait]
pub trait Connect:
AsyncWrite + std::marker::Unpin + tokio::io::AsyncRead + std::marker::Send
{
}

impl Connect for TcpStream {}
impl Connect for TlsStream<TcpStream> {}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we don't this trait?

Comment on lines 100 to 102
pub struct Connection {
io: BufReader<TcpStream>,
io: BufReader<Box<dyn Connect>>,
}
Copy link
Member

Choose a reason for hiding this comment

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

How about using:

pub struct Connection<IO: AsyncRead + AsyncWrite>{
    io: IO,
}

So we don't need to Box<dyn Connect> here?

Copy link
Author

Choose a reason for hiding this comment

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

This might not be a good implementation.

impl bb8::ManageConnection for MemcacheConnectionManager {
type Connection = binary::Connection;
type Error = Error;

This implementation returns a concrete type Self::Connection. If we try to use generics here, it would cause a conflict where a single function attempts to return two different types.That's because IO is a concrete type.

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

Successfully merging this pull request may close these issues.

new feature: Memcached In Transit Encryption (TLS) support
2 participants