Add Support for Unix Socket endpoints#19760
Conversation
|
Hi @SalehBorhani. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
... and 28 files with indirect coverage changes @@ Coverage Diff @@
## main #19760 +/- ##
==========================================
+ Coverage 66.85% 68.84% +1.98%
==========================================
Files 424 424
Lines 35756 35774 +18
==========================================
+ Hits 23905 24627 +722
+ Misses 10446 9729 -717
- Partials 1405 1418 +13 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
I have fixed the issues. Would you please check that |
|
Hey @SalehBorhani - Do you have capacity to add an e2e test to validate that connecting over unix socket works? This will help us ensure we don't accidentally break it in future, thanks! |
|
You're right @jmhbnz . I will provide that. |
k8s-ci-robot
added
area/clientv3
area/documentation
area/etcdctl
area/etcdutl
area/performance
area/robustness-testing
area/tooling
github_actions
tests/e2e/etcd_config_test.go
Outdated
| // Wait for the process to be ready | ||
| require.NoError(t, e2e.WaitReadyExpectProc(t.Context(), proc, e2e.EtcdServerReadyLines)) | ||
|
|
||
| // Additional validation can be added here if needed |
There was a problem hiding this comment.
can you please try to write a key/value and read it back using the unix socket?
There was a problem hiding this comment.
Yes. I forgot that :)
|
Would you please check that if is it ok for writing in etcd and read from it ? @ahrtr |
|
Thanks for your guidance. But i didn't understand where you mentioned to add changelog the link. What should i do exactly ? |
Please refer to the example #20045 . But you should update CHANGELOG-3.7.md: just add an item under the section "etcd server" |
|
I added to the changelog. @ahrtr |
|
I see that this PR is based on a very old commit, please rebase this PR. |
|
I rebased with main branch. @ahrtr |
ahrtr
changed the title
ivanvc
left a comment
ivanvc
left a comment
There was a problem hiding this comment.
Thanks for the pull request and great job, @SalehBorhani. I left a comment for a minor improvement.
tests/e2e/etcd_config_test.go
Outdated
| unixSocket := "unix:///tmp/etcd-client.sock" | ||
|
|
||
| // Clean up the socket file after test | ||
| defer os.Remove("/tmp/etcd-client.sock") |
There was a problem hiding this comment.
Could we change this to use a t.TempDir(), so we don't need to clean it up and ensure it's unique?
| unixSocket := "unix:///tmp/etcd-client.sock" | |
| // Clean up the socket file after test | |
| defer os.Remove("/tmp/etcd-client.sock") | |
| socketDir := t.TempDir() | |
| unixSocket := fmt.Sprintf("unix://%s/etcd-client.sock", socketDir) |
There was a problem hiding this comment.
Yes of course. I will check it now.
Signed-off-by: borhanisaleh6 <borhanisaleh6@gmail.com>
|
Would you please check that ? @ivanvc |
ivanvc
left a comment
ivanvc
left a comment
There was a problem hiding this comment.
LGTM. Thanks for your first contribution, @SalehBorhani!
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahrtr, ivanvc, SalehBorhani The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
5 participants
This pull request addresses #19704, which highlights the lack of support for connecting to etcd servers using the Unix socket format (unix://). The changes introduce support for Unix sockets, making it possible to leverage them in single-node etcd setups for local communication. The validation logic has been updated to allow Unix socket schemes while ensuring compatibility with the existing host:port format required for multi-node clusters.