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.

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

Fix: behavior incompatiblity between (standalone) LS and LS in Docker #30

Merged
merged 7 commits into from
Jan 4, 2022

Conversation

kares
Copy link
Contributor

@kares kares commented Nov 18, 2021

Description of the problem including expected versus actual behavior (from #29):

In the case a command to execute is missing, one expects ENOENT system error :
Errno::ENOENT: No such file or directory - invalid_command for a missing command,

however the behavior is different in Docker - popen does not raise - we simply get a non-zero exit code:
"message"=>"", "process"=>{"command_line"=>"invalid_command 1 2 3", "exit_code"=>127}


PENDING suggestion from #28 (comment) :

If there are two different behaviours, depending on which platform it is run, I would prefer we clearly specify both behaviours instead of specifying one of the behaviours and conditionally executing that spec.

If possible, it could be helpful to emulate the underlying change in behaviour and define our specs so that we can validate our effective behavour in CI regardless of which platform it is executed on (It would be hard to stub IO::popen because we rely on its side-effect of setting $?, but can we stub run_command?).

So our specs output would look something like:

  • when command fails
    • on a platform where IO.popen raises ENOENT
      • it does not enqueue an event
    • on a platform where IO.popen does not raise ENOENT (Docker)
      • it enqueues an event tagged with the error code

Ideally we would rather want the same behavior in Docker vs non-Docker LS ...

By switching to Open3's popen the behavior is now consistent between LS in Docker and standalone LS.

Previously, when a user would run Docker-ized LS using the exec input and points to a wrong/missing executable to run, the plugin would generates (error) events with a non-zero exit status (127), in standalone mode no events would have been generated.

@kares kares changed the title popen not raising in Docker IO.popen not raising in Docker Nov 18, 2021
@kares kares linked an issue Nov 22, 2021 that may be closed by this pull request
@kares kares changed the title IO.popen not raising in Docker Fix: behavior incompatiblity between (standalone) LS and LS in Docker Nov 22, 2021
@kares kares marked this pull request as ready for review November 22, 2021 08:24
@kares kares requested a review from yaauie November 22, 2021 08:24
@kares kares requested review from jsvd and removed request for yaauie December 2, 2021 14:51
lib/logstash/inputs/exec.rb Outdated Show resolved Hide resolved
@kares
Copy link
Contributor Author

kares commented Jan 4, 2022

CI 🔴 only happens on 8.x SNAPSHOT and seems to be caused by elastic/logstash#13572 (further investigation needed)

@kares kares merged commit 04e8e2b into logstash-plugins:main Jan 4, 2022
This was referenced Jan 4, 2022
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.

plugin behavior difference in Docker vs non-Docker env
2 participants