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

Add option to disable sending remote address in init #2285

Merged
merged 1 commit into from
May 31, 2022
Merged

Add option to disable sending remote address in init #2285

merged 1 commit into from
May 31, 2022

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented May 25, 2022

When running behind a load balancer that doesn't preserve the client source IP address, it doesn't make sense to include the remote-address tlv in our init message, it will contain an IP address that is completely unrelated to our peer.

Fixes #2256

@t-bast
Add option to disable sending remote addr in init
4a38cd1 
When running behind a load balancer that doesn't preserve the client source
IP address, it doesn't make sense to include the `remote-address` tlv in
our `init` message, it will contain an IP address that is completely
unrelated to our peer.

Fixes #2256
@t-bast t-bast changed the title Add option to disable sending remote addr in init Add option to disable sending remote address in init May 25, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #2285 (4a38cd1) into master (9c7ddcd) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2285      +/-   ##
==========================================
+ Coverage   84.32%   84.33%   +0.01%     
==========================================
  Files         194      194              
  Lines       14411    14412       +1     
  Branches      567      591      +24     
==========================================
+ Hits        12152    12155       +3     
+ Misses       2259     2257       -2
Impacted Files Coverage Δ
...re/src/main/scala/fr/acinq/eclair/NodeParams.scala 92.80% <100.00%> (+0.02%) ⬆️
...main/scala/fr/acinq/eclair/io/PeerConnection.scala 85.98% <100.00%> (ø)
...cinq/eclair/remote/EclairInternalsSerializer.scala 97.77% <100.00%> (ø)
...in/scala/fr/acinq/eclair/channel/fsm/Channel.scala 88.39% <0.00%> (ø)
...cala/fr/acinq/eclair/payment/relay/NodeRelay.scala 97.56% <0.00%> (+1.62%) ⬆️

pm47
pm47 approved these changes May 30, 2022
View reviewed changes
Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

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

I'm not sure this is really necessary (we could fix our deployment, it's as simple as advertising a different public ip), but it doesn't cost much to support not sending the tlv, so I'll leave it to you to decide.

eclair-core/src/main/scala/fr/acinq/eclair/remote/EclairInternalsSerializer.scala
@@ -108,7 +108,8 @@ object EclairInternalsSerializer {
("pingDisconnect" | bool(8)) ::
("maxRebroadcastDelay" | finiteDurationCodec) ::
("killIdleDelay" | finiteDurationCodec) ::
("maxOnionMessagesPerSecond" | int32)).as[PeerConnection.Conf]
("maxOnionMessagesPerSecond" | int32) ::
("sendRemoteAddressInit" | bool(8))).as[PeerConnection.Conf]
Copy link
Member

Choose a reason for hiding this comment

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

Should be bool8 but apparently we have other "violations" in that file.

Copy link
Member Author

Choose a reason for hiding this comment

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

I saw that, that's why I chose to be consistent instead of using bool8. I'll do a follow-up tiny refactoring to move to bool8 everywhere in another PR.

@t-bast
Copy link
Member Author

t-bast commented May 31, 2022

we could fix our deployment, it's as simple as advertising a different public ip

But if we advertise the IP that doesn't go through the load balancer, it negates the benefits of having a load balancer, doesn't it?
I think having the option to turn this off can be useful and doesn't add too much code, because we don't know yet what setup other node operators will use and they may not always be able to have access to the real client IP.

@t-bast t-bast merged commit 749d92b into master May 31, 2022
@t-bast t-bast deleted the optional-init-remote-addr branch May 31, 2022 15:21
evd0kim pushed a commit to standardsats/eclair that referenced this pull request Jul 17, 2023
@t-bast @pm47
Add option to disable sending remote addr in init (ACINQ#2285)
d11ec5b 
When running behind a load balancer that doesn't preserve the client source
IP address, it doesn't make sense to include the `remote-address` tlv in
our `init` message, it will contain an IP address that is completely
unrelated to our peer.

Fixes ACINQ#2256
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pm47 pm47 pm47 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fill in init.remote_addr from front machines
3 participants
@t-bast @codecov-commenter @pm47