Skip to content

Conversation

pasindutennage-da
Copy link
Contributor

Fixes #2017

Set localhost as the default in SV, localhost, and validator docker-compose in cluster/compose

For SV and Validator, there is an option to expose externally using -E flag, in the respective start.sh scripts.

For localnet, it is hardcoded, because no extra traffic is expected.

…on to expose externally.

[static]

Signed-off-by: Pasindu Tennage <[email protected]>
…to chose between localhost and 0.0.0.0

[static]

Signed-off-by: Pasindu Tennage <[email protected]>
…r deployment.

Hardcoded to localhost, with no option to run externally.

Since localnet only runs locally, no need to expose externally.

[ci]

Signed-off-by: Pasindu Tennage <[email protected]>
@martinflorian-da
Copy link
Contributor

Looks reasonable to me, nice work @pasindutennage-da !

  1. CI is unhappy, AFAICT due to the log checker. Ideally you can avoid the problematic log; it looks like the better option in this case (the other option being adding it to the right "ignore" file):
Found problems in the sbt output:
time="2025-10-08T15:17:21Z" level=warning msg="The \"HOST_BIND_IP\" variable is not set. Defaulting to a blank string."
time="2025-10-08T15:17:23Z" level=warning msg="The \"HOST_BIND_IP\" variable is not set. Defaulting to a blank string."
Total: 2 lines with problems.
  1. This looks to me like a change we should make more public. How about adding something to docs/src/release_notes.rst?

@pasindutennage-da
Copy link
Contributor Author

The CI error comes because the test is not exporting the HOST_BIND_IP correclty. I made a revision, and now if the variable is not set, it uses the default value.

@isegall-da
Copy link
Contributor

The CI error comes because the test is not exporting the HOST_BIND_IP correclty. I made a revision, and now if the variable is not set, it uses the default value.

The message was probably from stop.sh. You might want instead to just add HOST_BIND_IP here:

export VALIDATOR_NEW_PARTICIPANT_IDENTIFIER=""
(where we just export a bunch of env vars, for the exact purpose of avoiding these warnings)

@moritzkiefer-da
Copy link
Contributor

Thanks! Changes lgtm overall, definitely should go in the release notes including mentioning the option to turn it back on.

Also please ask someone on macos to test this (probably spin up localnet and check that you can still open the wallet or something like that) as we discussed in slack. Docker on macos can be a bit finicky

Added release notes.
Added LocalNet docker-compose the option to chose between 0.0.0.0 and 127.0.0.1
Added exports to stop.sh to avoie CI complains.

[ci]

Signed-off-by: Pasindu Tennage <[email protected]>
@pasindutennage-da pasindutennage-da marked this pull request as ready for review October 10, 2025 16:41
Copy link
Contributor

@martinflorian-da martinflorian-da left a comment

Choose a reason for hiding this comment

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

Nice job, thank you @pasindutennage-da !

Mainly to try it out (90% sure this doesn't break anything there), you might want to consider doing a (basic) cluster test as well before you merge: https://github.com/hyperledger-labs/splice/blob/main/TESTING.md#requesting-cluster-tests

@martinflorian-da
Copy link
Contributor

martinflorian-da commented Oct 10, 2025

...and actually, how annoying would it be to extend our integration tests for these flows to check that all opened ports are bound to localhost?

I.e., DockerComposeFullNetworkFrontendIntegrationTest and DockerComposeValidatorFrontendIntegrationTest.

@martinflorian-da
Copy link
Contributor

...and actually, how annoying would it be to extend our integration tests for these flows to check that all opened ports are bound to localhost?

I.e., DockerComposeFullNetworkFrontendIntegrationTest and DockerComposeValidatorFrontendIntegrationTest.

From a quick look at the code there it looks like a reasonable investment; let's do this so we guard against regressions?

…o only localhost, when used with docker compose.

Updated release_notes.

[ci]

/cluster_test

Signed-off-by: Pasindu Tennage <[email protected]>
@pasindutennage-da
Copy link
Contributor Author

...and actually, how annoying would it be to extend our integration tests for these flows to check that all opened ports are bound to localhost?
I.e., DockerComposeFullNetworkFrontendIntegrationTest and DockerComposeValidatorFrontendIntegrationTest.

From a quick look at the code there it looks like a reasonable investment; let's do this so we guard against regressions?

Done. DockerComposeFullNetworkFrontendIntegrationTest checks for both SV and validator

@pasindutennage-da
Copy link
Contributor Author

/cluster_test

Copy link

Deploy cluster test triggered for Commit bf1da1615f4f0df660f739c29f8f1cc4dbfbcbbf in , please contact a Contributor to approve it in CircleCI: https://app.circleci.com/pipelines/github/DACH-NY/canton-network-internal/36638

Copy link
Contributor

@martinflorian-da martinflorian-da left a comment

Choose a reason for hiding this comment

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

Nice work, thank you!

@pasindutennage-da
Copy link
Contributor Author

Copy link

Deploy cluster test triggered for Commit bf1da1615f4f0df660f739c29f8f1cc4dbfbcbbf in , please contact a Contributor to approve it in CircleCI: https://app.circleci.com/pipelines/github/DACH-NY/canton-network-internal/36741

@pasindutennage-da
Copy link
Contributor Author

Nice job, thank you @pasindutennage-da !

Mainly to try it out (90% sure this doesn't break anything there), you might want to consider doing a (basic) cluster test as well before you merge: https://github.com/hyperledger-labs/splice/blob/main/TESTING.md#requesting-cluster-tests

Basic cluster tests passes

Copy link
Contributor

@martinflorian-da martinflorian-da left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@pasindutennage-da pasindutennage-da merged commit c352705 into main Oct 17, 2025
57 checks passed
@pasindutennage-da pasindutennage-da deleted the pasindutennage-da/fix-docker-localhost/2017 branch October 17, 2025 10:57
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.

Change docker compose setup to only expose to localhost by default

4 participants