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

Add exclusion list for environment variables #802

Merged
merged 16 commits into from
May 9, 2024
Merged

Add exclusion list for environment variables #802

merged 16 commits into from
May 9, 2024

Conversation

DavidGOrtega
Copy link
Contributor

@DavidGOrtega DavidGOrtega commented Nov 4, 2021

@DavidGOrtega DavidGOrtega temporarily deployed to internal November 4, 2021 09:47 Inactive
@0x2b3bfa0
Copy link
Member

@DavidGOrtega, I believe that $PATH is mandatory for runners to work. 🤔

@casperdcl
Copy link
Contributor

casperdcl commented Nov 4, 2021

The problem with whitelist-only is there are a few other things that a user would likely want preserved too...

  • NVIDIA_VISIBLE_DEVICES
  • CUDA_VISIBLE_DEVICES
  • LANG

I'd prefer blacklisting CML config vars instead (AWS_*ACCESS_KEY etc)

(vis. #721 (comment))

@casperdcl casperdcl mentioned this pull request Nov 4, 2021
7 tasks
@casperdcl casperdcl added cml-runner Subcommand discussion Waiting for team decision p1-important High priority security Sensitive flaws labels Nov 4, 2021
@dacbd
Copy link
Contributor

dacbd commented Nov 4, 2021

Consider using systemd-run to start the run.sh ? Should scrub vars set in the session with export but maintain system controlled ones like PATH

@dacbd
Copy link
Contributor

dacbd commented Nov 4, 2021

Consider using systemd-run to start the run.sh ? Should scrub vars set in the session with export but maintain system controlled ones like PATH

Ehh don't think this plays nice when executed in a docker container, so probably not the best

@DavidGOrtega
Copy link
Contributor Author

DavidGOrtega commented Nov 4, 2021

Im checking. It's happening something interesting.
I was counting with spawn( {shell:true, env: {} }) which should create its own shell with system vars being expanded by the node ones. Funny is that in Macos seems to be fine... however inside a linux container Its not very happy...

@DavidGOrtega DavidGOrtega marked this pull request as draft November 9, 2021 19:46
@dacbd
Copy link
Contributor

dacbd commented Nov 20, 2021

Confirm that this is really only an issue for cloud-based runners; the fix might be in tpi?

@casperdcl
Copy link
Contributor

I have this issue on on-prem as well as cloud...

@tasdomas
Copy link
Contributor

This branch is now almost a year old. Should we make a decision on this?

@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal November 1, 2022 15:09 Inactive
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal November 1, 2022 18:56 Inactive
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal November 1, 2022 18:56 Inactive
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal February 14, 2023 13:06 — with GitHub Actions Inactive
@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@0x2b3bfa0 0x2b3bfa0 requested a review from dacbd February 14, 2023 13:25
@0x2b3bfa0 0x2b3bfa0 removed the blocked Dependent on something else label Feb 14, 2023
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal February 14, 2023 15:48 — with GitHub Actions Inactive
@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal February 21, 2023 17:09 — with GitHub Actions Inactive
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal February 21, 2023 17:09 — with GitHub Actions Inactive
@0x2b3bfa0 0x2b3bfa0 enabled auto-merge (squash) February 21, 2023 17:10
@github-actions

This comment was marked as outdated.

1 similar comment
@github-actions

This comment was marked as outdated.

@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal February 21, 2023 17:14 — with GitHub Actions Inactive
@github-actions

This comment was marked as outdated.

@0x2b3bfa0 0x2b3bfa0 disabled auto-merge May 9, 2024 22:04
@0x2b3bfa0 0x2b3bfa0 merged commit e7d27a5 into master May 9, 2024
@0x2b3bfa0 0x2b3bfa0 deleted the no-envars branch May 9, 2024 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cml-runner Subcommand discussion Waiting for team decision p1-important High priority security Sensitive flaws
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants