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.

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

filter local endpoints when set remote #52

Merged
merged 1 commit into from
Sep 14, 2024
Merged

Conversation

gojoy
Copy link
Contributor

@gojoy gojoy commented Aug 22, 2024

some etcd member has multiple clientURLs inculde local address,for example:
["http://10.7.7.7:2379","http://127.0.0.1:2379"]
in this case, excuting etcd-defrag with --cluster on remote machine would fail,because health check failed.
we should add flag --remote to filter local address

main.go Outdated Show resolved Hide resolved
endpoints.go Outdated Show resolved Hide resolved
@ahrtr
Copy link
Owner

ahrtr commented Aug 22, 2024

Thanks @gojoy for raising this PR. Overall it makes sense to me. Please resolve the two comments above.

@gojoy
Copy link
Contributor Author

gojoy commented Aug 22, 2024

Thanks @gojoy for raising this PR. Overall it makes sense to me. Please resolve the two comments above.

got it

@gojoy gojoy force-pushed the feat/remote branch 2 times, most recently from 157dc7a to 9689b72 Compare August 23, 2024 06:09
@ahrtr
Copy link
Owner

ahrtr commented Aug 23, 2024

Please also verify IPv6 loopback address in the test cases.

@gojoy
Copy link
Contributor Author

gojoy commented Aug 28, 2024

Please also verify IPv6 loopback address in the test cases.

done

@gojoy
Copy link
Contributor Author

gojoy commented Aug 30, 2024

Hi. Is there any problem that I should solve ? 😄 @ahrtr

@ahrtr
Copy link
Owner

ahrtr commented Aug 30, 2024

Hi. Is there any problem that I should solve ? 😄 @ahrtr

Please read #52 (comment). I remaindered you a couple of times, I am not sure why you keep marking it as resolved but actually not resolving it.

@gojoy
Copy link
Contributor Author

gojoy commented Aug 30, 2024

Hi. Is there any problem that I should solve ? 😄 @ahrtr

Please read #52 (comment). I remaindered you a couple of times, I am not sure why you keep marking it as resolved but actually not resolving it.

Sorry I misunderstood your meaning before. I didn't ignore it on purpose.
net.ParseIP cannot parse localhost and http:// url like http://127.0.0.1:2379,127.0.0.1:2379. So using ip.IsLoopback() cannot work well.

@ahrtr
Copy link
Owner

ahrtr commented Aug 30, 2024

net.ParseIP cannot parse localhost and http:// url like http://127.0.0.1:2379,127.0.0.1:2379. So using ip.IsLoopback() cannot work well.

The point is try not to hardcode the loopback address, and instead make it flexible. You can try to get the host part from the URL firstly, afterwards check whether or not it's a loopback address.

@gojoy
Copy link
Contributor Author

gojoy commented Sep 2, 2024

net.ParseIP cannot parse localhost and http:// url like http://127.0.0.1:2379,127.0.0.1:2379. So using ip.IsLoopback() cannot work well.

The point is try not to hardcode the loopback address, and instead make it flexible. You can try to get the host part from the URL firstly, afterwards check whether or not it's a loopback address.

I agree. I adjusted the code to make it flexible.thanks

endpoints.go Show resolved Hide resolved
endpoints.go Outdated Show resolved Hide resolved
endpoints.go Outdated Show resolved Hide resolved
endpoints_test.go Outdated Show resolved Hide resolved
endpoints.go Show resolved Hide resolved
endpoints.go Outdated Show resolved Hide resolved
endpoints.go Outdated Show resolved Hide resolved
@gojoy gojoy force-pushed the feat/remote branch 2 times, most recently from de74240 to 0cb8629 Compare September 13, 2024 12:44
endpoints_test.go Outdated Show resolved Hide resolved
endpoints_test.go Outdated Show resolved Hide resolved
endpoints_test.go Outdated Show resolved Hide resolved
endpoints_test.go Outdated Show resolved Hide resolved
endpoints_test.go Outdated Show resolved Hide resolved
endpoints_test.go Outdated Show resolved Hide resolved
endpoints_test.go Outdated Show resolved Hide resolved
endpoints_test.go Outdated Show resolved Hide resolved
endpoints_test.go Outdated Show resolved Hide resolved
endpoints.go Outdated Show resolved Hide resolved
@ahrtr
Copy link
Owner

ahrtr commented Sep 13, 2024

Thanks for all the effort. I will merge this PR once the above minor comments are resolved.

Signed-off-by: GitHub <noreply@github.com>
@ahrtr ahrtr merged commit 163fc0e into ahrtr:main Sep 14, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants