-
Notifications
You must be signed in to change notification settings - Fork 569
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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?
No problem, I will complete it later. |
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
|
There was a problem hiding this 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.
#[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> {} |
There was a problem hiding this comment.
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?
pub struct Connection { | ||
io: BufReader<TcpStream>, | ||
io: BufReader<Box<dyn Connect>>, | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
opendal/core/src/services/memcached/backend.rs
Lines 262 to 264 in f66cf1f
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.
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: