Skip to content

[release-3.5]pkg/types: Support Unix sockets in NewURLS#15940

Merged
ahrtr merged 3 commits into
etcd-io:release-3.5from
HubertZhang:backport-12469-3.5
Feb 20, 2024
Merged

[release-3.5]pkg/types: Support Unix sockets in NewURLS#15940
ahrtr merged 3 commits into
etcd-io:release-3.5from
HubertZhang:backport-12469-3.5

Conversation

@HubertZhang
Copy link
Copy Markdown
Contributor

@HubertZhang HubertZhang commented May 23, 2023

backport of #12469 to release 3.5, which enables using unix socket setup an embed server

@ahrtr
Copy link
Copy Markdown
Member

ahrtr commented May 23, 2023

Please signoff the commits.

https://github.com/etcd-io/etcd/pull/15940/checks?check_run_id=13690537425

@HubertZhang HubertZhang force-pushed the backport-12469-3.5 branch from acc308b to 007a338 Compare May 24, 2023 07:10
ahrtr
ahrtr approved these changes May 25, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks.

Please also add a item in 3.5.10 changelog

HubertZhang added a commit to HubertZhang/etcd that referenced this pull request May 26, 2023
@HubertZhang
update changelog for etcd-io#15940
d87bd32 
Signed-off-by: Hubert Zhang <hubert.zyk@gmail.com>
HubertZhang added a commit to HubertZhang/etcd that referenced this pull request May 30, 2023
@HubertZhang
update changelog for etcd-io#15940
018868f 
Signed-off-by: Hubert Zhang <hubert.zyk@gmail.com>
@HubertZhang HubertZhang mentioned this pull request May 30, 2023
Merged
@ahrtr
Copy link
Copy Markdown
Member

ahrtr commented Jun 5, 2023

ping @mitake @ptabor @serathius

HubertZhang added a commit to HubertZhang/etcd that referenced this pull request Jun 6, 2023
@HubertZhang
update changelog for etcd-io#15940
b187b82 
Signed-off-by: Hubert Zhang <hubert.zyk@gmail.com>
jmhbnz
jmhbnz approved these changes Jun 6, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

Backport itself looks correct - My only reservation is that it doesn't look like any follow-up e2e tests were added as requested in #12469 (review). Unsure if that should block this backport or not?

@chaochn47
Copy link
Copy Markdown
Member

chaochn47 commented Jun 7, 2023 •
edited
Loading

I guess e2e test needs to add UDS support here. Right now only http(s) schema is supported in e2e test.

I am thinking of adding those as part of verification of POC of #15145 (comment) in case that migration will break UDS users.. Is it the desired direction before I making these changes? @ahrtr @serathius @jmhbnz

Just to be clear: I meant the changes are only the test cases to verify authority header are sent properly in case of UDS in e2e test environment. Integration test already covers that though in https://github.com/etcd-io/etcd/blob/main/tests/integration/grpc_test.go#L31 without embed etcd or run etcd as process.

@chaochn47 chaochn47 mentioned this pull request Jun 8, 2023
Merged
@HubertZhang
Copy link
Copy Markdown
Contributor Author

HubertZhang commented Jun 20, 2023

Should I backport that #16037 in this PR also?

@chaochn47
Copy link
Copy Markdown
Member

chaochn47 commented Jun 20, 2023 •
edited
Loading

Should I backport that #16037 in this PR also?

Go for it. I just slimmed through 3.5 test code, it should be similar as the original change with minor adjustments.

Please also add a item in 3.5.10 changelog

BTW, this comment sounds like need to be addressed before the merge.

@ahrtr
Copy link
Copy Markdown
Member

ahrtr commented Jul 5, 2023

@HubertZhang Please follow comment above #15940 (comment), thx

@HubertZhang
Copy link
Copy Markdown
Contributor Author

HubertZhang commented Jul 28, 2023

@ahrtr @chaochn47 I've cherry picked #16037 into this PR. Since there is no tests/framework/e2e package in branch 3.5, I merged those change to tests/e2e/cluster_test.go

HubertZhang added a commit to HubertZhang/etcd that referenced this pull request Aug 23, 2023
@HubertZhang
update changelog for etcd-io#15940
892f0a1 
Signed-off-by: Hubert Zhang <hubert.zyk@gmail.com>
@chaochn47 chaochn47 mentioned this pull request Oct 13, 2023
Merged
@chaochn47
Copy link
Copy Markdown
Member

chaochn47 commented Oct 15, 2023

grpc: Dial("unix://relative-path") no longer works (grpc/grpc-go#4817).

This is never officially supported by gRPC. So users should not use this URL to dial etcd in the first place.

In addition, etcd 3.5 v3 client transforms this URL to unix:<relative-path> internally in the resolver.

@ahrtr
Copy link
Copy Markdown
Member

ahrtr commented Oct 16, 2023

This is never officially supported by gRPC. So users should not use this URL to dial etcd in the first place.

To be clearer,

  • In the form of "unix:path", the path may be relative or absolute based on naming.md#name-syntax
  • I still think it makes sense to support UDS in 3.5.

@ahrtr ahrtr mentioned this pull request Feb 18, 2024
Closed
4 tasks
@ahrtr
Copy link
Copy Markdown
Member

ahrtr commented Feb 18, 2024

Can you please rebase this PR?

Karen Almog and others added 3 commits February 19, 2024 12:21
@HubertZhang
pkg/types: Support Unix sockets in NewURLS
1a9062e 
Resolves etcd-io#12450
This commits adds support to unix/unixs socket URLs, which currently
fail with the message "URL address does not have the form "host:port".
It also replaces the work started in etcd-io#11747.

(cherry picked from commit d93b7c8)
Signed-off-by: Hubert Zhang <hubert.zyk@gmail.com>
@HubertZhang
Add unix socket test to TestNewURLsValue
80e2064 
(cherry picked from commit 75747f2)
Signed-off-by: Hubert Zhang <hubert.zyk@gmail.com>
@chaochn47 @HubertZhang
add uds test cases into e2e TestAuthority
a1aaa51 
Signed-off-by: Chao Chen <chaochn@amazon.com>
@HubertZhang
Copy link
Copy Markdown
Contributor Author

HubertZhang commented Feb 19, 2024

PR rebased @ahrtr

@ahrtr
Copy link
Copy Markdown
Member

ahrtr commented Feb 19, 2024

cc @fuweid @serathius

@HubertZhang Please raise a separate PR to update the 3.5 changelog.

HubertZhang added a commit to HubertZhang/etcd that referenced this pull request Feb 20, 2024
@HubertZhang
update changelog for etcd-io#15940
c0e1b86 
Signed-off-by: Hubert Zhang <hubert.zyk@gmail.com>
@HubertZhang
Copy link
Copy Markdown
Contributor Author

HubertZhang commented Feb 20, 2024

Changelog PR is #15983

fuweid
fuweid approved these changes Feb 20, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

@ahrtr ahrtr merged commit 5cc7239 into etcd-io:release-3.5 Feb 20, 2024
ahrtr added a commit that referenced this pull request Feb 20, 2024
@ahrtr
Merge pull request #15983 from HubertZhang/changelog-15940
7226d3e 
update changelog for #15940
@aojea aojea mentioned this pull request Apr 12, 2025
Merged
@pohly pohly mentioned this pull request Apr 13, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serathius serathius serathius approved these changes

@fuweid fuweid fuweid approved these changes

@ahrtr ahrtr ahrtr approved these changes

+1 more reviewer

@jmhbnz jmhbnz jmhbnz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@HubertZhang @ahrtr @chaochn47 @serathius @fuweid @jmhbnz