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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Feb 20, 2024
Merged

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

merged 3 commits into from
Feb 20, 2024

Conversation

HubertZhang
Copy link
Contributor

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

@ahrtr
Copy link
Member

ahrtr commented May 23, 2023

Please signoff the commits.

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

ahrtr
ahrtr approved these changes May 25, 2023
View reviewed changes
Copy link
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
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
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
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
Contributor Author

Should I backport that #16037 in this PR also?

@chaochn47
Copy link
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
Member

ahrtr commented Jul 5, 2023

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

@HubertZhang
Copy link
Contributor Author

@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
Member

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
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
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
Contributor Author

PR rebased @ahrtr

@ahrtr
Copy link
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
Contributor Author

Changelog PR is #15983

fuweid
fuweid approved these changes Feb 20, 2024
View reviewed changes
Copy link
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
17 checks passed
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
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

@jmhbnz jmhbnz jmhbnz approved these changes

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