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

feat: adding support for CHALK_BYPASS environment variable #419

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

miki725
Copy link
Contributor

@miki725 miki725 commented Sep 5, 2024

Issue

there is no way to disable/enable chalk when chalk wraps a docker command

fixes #224

Description

this allows to completely skip chalk when wrapping docker. Whenever its enabled, chalk directly proxies the command to docker without invoking any of the chalk internal machinery such as loading configs/etc

Testing

➜ make tests args="test_docker.py::test_docker_default_command --logs"

@miki725 miki725 marked this pull request as ready for review September 5, 2024 17:03
@miki725 miki725 requested a review from viega as a code owner September 5, 2024 17:03
@miki725 miki725 requested a review from drraid September 5, 2024 18:15
@miki725 miki725 force-pushed the bypass branch 2 times, most recently from a815a73 to 393862d Compare September 6, 2024 12:26
@miki725
Copy link
Contributor Author

miki725 commented Sep 6, 2024

ee7
ee7 previously approved these changes Sep 6, 2024
Copy link
Contributor

@ee7 ee7 left a comment

Choose a reason for hiding this comment

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

LGTM, assuming that e.g. @drraid is happy with the mechanism here.

src/bypass.nim Outdated Show resolved Hide resolved
src/bypass.nim Outdated Show resolved Hide resolved
this allows to completely skip chalk when wrapping `docker`.
Whenever its enabled, chalk directly proxies the command
to `docker` without invoking any of the chalk internal machinery
such as loading configs/etc
Copy link
Contributor

@drraid drraid left a comment

Choose a reason for hiding this comment

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

If I understand the code correctly, the default behavior of this is to still use chalk, and only bypass if the env var CHALK_BYPASS is true (1, etc.), and I think this is the inversion of what we want: if someone has configured chalk to use bypass, it should default to bypass unless an env var specifies that it shouldn't. The recent request to implement this is for a scenario where the customer wants people to be able to opt-in by setting a variable, and also this opt-in approach prevents accidentally CRWDSTRIKEing their machines for replaced binaries.

Maybe we want something like PERFORM_WRAPPED_CHALKING? Or maybe even something more explicit to identify the docker scenario?

Additionally I'd like to do some analysis to figure out the code paths before this code is reached, the goal will be to minimize it as much as possible so that bugs elsewhere in the chalk code base can't cause a failure of our proxying of execution

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.

Support a NOP run mode beyond virtual chalking
3 participants