Repository navigation
Conversation
f6e1db7 to
d2fc42d
Compare
vmishra22
left a comment
vmishra22
left a comment
There was a problem hiding this comment.
Overall LGTM, Replacing the panic! with a Result<Self, io::Error> + error! log + exit(1) is the right call, and I verified the error message actually reaches the user: in file log mode the stderr appender is ThresholdFilter(Error), and the mount helper pipes efs-proxy's stderr (proxy.py, stderr=subprocess.PIPE) and surfaces a non-zero exit via test_tunnel_process → fatal_error into mount.log. So the path described in the commit message holds end to end. Just a minor comment
Addresses review nit from vmishra22 on PR aws#355: the pid-file cleanup + exit(1) logic was duplicated across the TLS and plaintext branches. Extracted into a single bind_failure_exit helper.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you believe this PR is still relevant, please comment or push new changes to keep it open. |
|
Please address branch conflicts |
efs-proxy panicked with an unhelpful backtrace when it failed to bind to 127.0.0.1:<port>. This manifested as a hard crash with no actionable error message in environments like Bottlerocket/EKS containers where the loopback bind can fail (e.g. permission denied, address in use). Change Controller::new to return Result<Self, io::Error> and log the address and OS error on bind failure. main() now removes the stale pid file and exits with code 1 via bind_failure_exit instead of panicking. The mount helper already detects a non-zero efs-proxy exit and surfaces it to the user via mount.log. Add a unit test covering the bind-failure path. Fixes aws#323
7d6cf87 to
4790311
Compare
|
@samuhale Just rebased, please check again, thanks! |
|
/lgtm |
3 participants
Problem
efs-proxy panics with an unhelpful Rust backtrace when it cannot bind to
127.0.0.1:<port>. This is reproducible in Bottlerocket/EKS containers and surfaces as:The mount then fails with a
DeadlineExceededor timeout error, giving the user no actionable information about the root cause.Fixes #323
Change
Controller::newnow returnsResult<Self, io::Error>instead of panicking.error!log is emitted with the address and the OS error (e.g.permission denied,address already in use).main()removes the stale pid file and exits with code 1 through a sharedbind_failure_exithelper (used by both the TLS and plaintext branches). The non-zero exit is already detected by the mount helper and surfaced inmount.log.Testing
test_controller_new_bind_failure_returns_errcovers the bind-failure path.cargo testcompared tomaster. The same pre-existing tests fail on both:test_reload_certificate,test_remove_comments,test_secret_reader_matches_plain_reader,test_read_config_from_fileandtest_build_returns_error_with_illegal_bucket_name.