Fix : fix gid duplication, additional_gids etc not getting used in exec #3018
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Yashodhan Joshi <[email protected]>
Signed-off-by: Yashodhan Joshi <[email protected]>
YJDoc2
commented
Dec 10, 2024
Comment on lines
+455
to
+475
| let mut env_exists = false; | ||
| let mut env: Vec<String> = self | ||
| .env | ||
| .iter() | ||
| .map(|(k, v)| { | ||
| if k == "PATH" { | ||
| env_exists = true; | ||
| } | ||
| format!("{k}={v}") | ||
| }) | ||
| .collect(); | ||
| // It is not possible in normal flow that path is None. The original container | ||
| // creation would have failed if path was absent. However we use Option | ||
| // just as a caution, and if neither exec cmd not original spec has PATH, | ||
| // the container creation will fail later which is ok | ||
| if let Some(p) = path { | ||
| if !env_exists { | ||
| env.push(p); | ||
| } | ||
| } | ||
| env |
There was a problem hiding this comment.
- If there is a more semantic way, let me know, these multiple checks are not particularly idiomatic.
- If there are no envs at all (self.env is empty) should we simply clone from the original container? I feel that is what the user would expect.
There was a problem hiding this comment.
@YJDoc2 rename env_exists to path_exists
|
There are a couple of other fixes needed in exec impl as well, so probably split this PR into another one. duplicate gid is pretty small in scope, so not mixing it with exec changes. |
|
Opening another PR to split this into two. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR does following fixes:
PATHfrom original container in case not provided to exec : exec allows supplying env vars, but by default it does not set any. This causes issue when running exec directly via youki, as it would error withPATH is not set. Given that we are exec-ing into an existing container, the expectation would be the same env would be applied, or atleast the path would be taken from it. I guess any higher runtime (docker/podman) automatically sets envs from original container, but I have injected path if missing.I want to also add tests for these, but wanted to get a review on the changes before that.