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

[Bugfix]: Shell runtime execution with security context #1862

Merged
merged 6 commits into from Oct 14, 2020

Conversation

quaark
Copy link
Contributor

@quaark quaark commented Oct 13, 2020

Shell runtime would need to chmod the run script to give it execute permissions.
If a security context is given, this fails.

Added a check to see if the handler is a script in the current working directory or a command from the PATH.
If it is a command, it is run with sh -c
If not, it is run with sh (without -c) which results in sh reading the file as a script and running it, so there's no need to chmod the file.

Benchmarked the changes in the event processing function to ensure no large loss of performance happens because of the string manipulations. The performance change is negligible.

Copy link
Contributor

@liranbg liranbg left a comment

Choose a reason for hiding this comment

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

Looking good and great addition!

some comment below, most important is that when changing delicate code in event processing, we must ensure the changes aligned / improving performance and not hurting them for stylish changes

pkg/processor/runtime/shell/runtime.go Outdated Show resolved Hide resolved
pkg/processor/runtime/shell/runtime.go Show resolved Hide resolved
pkg/processor/runtime/shell/runtime.go Outdated Show resolved Hide resolved
pkg/processor/runtime/shell/runtime.go Outdated Show resolved Hide resolved
@liranbg liranbg merged commit 7ce7619 into nuclio:development Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants