Skip to content

Start test validator bound to localhost by default (rather than all interfaces) #5862

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

Merged
merged 1 commit into from
Apr 23, 2025

Conversation

alexpyattaev
Copy link

@alexpyattaev alexpyattaev commented Apr 17, 2025

Problem

Test-validator would bind to all interfaces by default which is insecure

Summary of Changes

  • Change test-validator to bind to localhost by default

Context

#5802 tried to do the same but was poorly scoped and as a result had a problematic change which made it possible to get binding of ports on top of already bound ones.
This resulted in #5856 to revert, but RCA there was not correct (test-validator binding had nothing to do with the issues there).

The logic changes to cluster-info moved into separate PR #5832

@alexpyattaev alexpyattaev force-pushed the test_validator_startup branch from a12cb91 to e9b1b87 Compare April 17, 2025 08:52
@alexpyattaev alexpyattaev marked this pull request as ready for review April 17, 2025 11:24
@codecov-commenter
Copy link

codecov-commenter commented Apr 17, 2025

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 82.9%. Comparing base (7f48e52) to head (a781797).
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #5862     +/-   ##
=========================================
- Coverage    82.9%    82.9%   -0.1%     
=========================================
  Files         830      830             
  Lines      377199   377199             
=========================================
- Hits       312917   312895     -22     
- Misses      64282    64304     +22     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@alexpyattaev alexpyattaev changed the title Test validator startup Start test validator bound to localhost by default (rather than all interfaces) Apr 17, 2025
Copy link

@gregcusack gregcusack left a comment

Choose a reason for hiding this comment

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

looks mostly good. you may have to rebase this once #5855 is merged. also would delete the extra line added after parsing bind_address

@alexpyattaev alexpyattaev force-pushed the test_validator_startup branch from d5a5b4d to d9193e8 Compare April 22, 2025 15:50
@alexpyattaev alexpyattaev force-pushed the test_validator_startup branch from d9193e8 to a781797 Compare April 22, 2025 15:52
@alexpyattaev alexpyattaev requested a review from gregcusack April 22, 2025 17:46
@alexpyattaev
Copy link
Author

Rebased, now its pretty trivial.

@alexpyattaev alexpyattaev merged commit de672d2 into anza-xyz:master Apr 23, 2025
30 checks passed
@joncinque
Copy link

Apologies for the fly-by review, but is this a breaking change? If so, can you add an entry to the changelog about this new behavior and how to restore the old behavior if needed?

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.

5 participants