feat(server): warn when listener binds loopback inside a container - #4218
kunaldevxxx wants to merge 7 commits into
Conversation
|
Thanks for the PR. It is labeled Slash commands (own line, regular comment) move it around the queue:
See CONTRIBUTING.md for details. |
|
/request-review @hubcio |
|
Thanks @kunaldevxxx for this PR. I ran the validator tests on your branch and all 53 passed, so that behaviour looks to be unchanged. The warning(s) reach the log. A few things before a more thorough review:
|
|
Sure @justinmclean i done please review |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4218 +/- ##
=============================================
- Coverage 87.48% 73.81% -13.67%
Complexity 1575 1575
=============================================
Files 1280 1278 -2
Lines 223241 203672 -19569
Branches 186604 167036 -19568
=============================================
- Hits 195293 150344 -44949
- Misses 23237 48669 +25432
+ Partials 4711 4659 -52
🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| #[test] | ||
| fn given_dockerenv_file_when_checking_container_should_return_true() { |
There was a problem hiding this comment.
nit: every test injects its own path and value, so the marker branch has no positive coverage and a typo in any indicator at 477-483 fails nothing. add one test with a real docker cgroup line.
There was a problem hiding this comment.
Added given_cgroup_with_docker_marker_when_checking_container_should_return_true with a real Docker cgroup entry (0::/system.slice/docker-...scope), as well as a negative test given_host_cgroup_without_markers_when_checking_container_should_return_false.
|
two notes outside the diff.
dropping the |
| listener.key | ||
| ) | ||
| }; | ||
| warn!("{msg}"); |
There was a problem hiding this comment.
nit: the advice goes through warn!, so RUST_LOG=error hides it, while the 18 other operator messages in this file use eprintln!. print this one the same way.
|
Thanks for the thorough review, @hubcio! All points have been addressed in the latest commit:
All local checks passed ( |
|
it looks like you didn't push the changes @kunaldevxxx |
|
yes sorry it didnt got push give few min @hubcio |
|
please review now @hubcio sorry for last time |
|
CI is still failing. once you are done fixing it, please read #4218 (comment) and act accordingly - without that maintainers won't start checking this again :) |
|
/ready |
Which issue does this PR address?
Closes #4209
Rationale
When starting the server in a container with default settings (e.g.
docker run -p 8090:8090 apache/iggy), the client-facing listener binds127.0.0.1:8090, which only listens on the container's isolated loopback interface and is unreachable from the host.What changed?
In
validate_client_facing_address, check whether the client-facing listener binds a loopback IP inside a container. If so, emit a warning guiding the operator to setIGGY_TCP_ADDRESS=0.0.0.0:8090together withIGGY_NODE_ADVERTISED_ADDRESS(or bind a concrete address).Container detection inspects sentinel files (
/.dockerenv,/run/.containerenv), environment variables (container,KUBERNETES_SERVICE_HOST), and Linux cgroups.Tests were added to assert that the warning fires with the expected environment variables inside a container, does not fire outside a container, and detects container markers without reading ambient pod environments.
Local Execution
Ran:
cargo fmt --all -- --checkcargo clippy -p configs --all-features --all-targets -- -D warningscargo test -p configs(270 passed, 0 failed)AI Usage
Antigravity AI assistant.
Assisted in drafting unit test cases.
Ran
cargo fmt,cargo clippy --all-features --all-targets -- -D warnings, and the completeconfigstest suite verifying all 270 tests pass.Yes.