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

integration: test case for HOME #1072

Merged
merged 13 commits into from
Mar 5, 2020

Conversation

greut
Copy link
Contributor

@greut greut commented Feb 23, 2020

Description

According to #824, #1097 and #995, the $HOME variable should be set and correct when using USER.

Is it a limitation of the design? If not, do you see a way to get this fixed? If yes, any guidance would be appreciated to give a hand in the area.

Cheers

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes unit tests
  • Adds integration tests if needed.

See the contribution guide for more details.

Reviewer Notes

  • The code flow looks good.
  • Unit tests and or integration tests added.

Release Notes

Describe any changes here so maintainer can include it in the release notes, or delete this block.

- integration test to ensure the `HOME` variable is properly filled by `USER`

@cvgw
Copy link
Contributor

cvgw commented Feb 27, 2020

This is the same bug as #1082

@tejal29
Copy link
Member

tejal29 commented Feb 27, 2020

@greut Thank you for looking into this and submitting this PR.

I think the culprit here is addDefaultHOME function.

In this func at L106, we return immediately if HOME is set in the environmental variables there way ignoring the change in the USER config.
We should change that to

  1. Evaluate HOME from user string passed like L119
  2. Update the HOME variable if present in the env or append it.

Since this is a critical bug, we would like to get this in before next release. Let us know if you need any help finishing this PR.

Thanks
Tejal

@greut
Copy link
Contributor Author

greut commented Feb 27, 2020 via email

@cvgw cvgw self-requested a review February 27, 2020 20:58
@tejal29
Copy link
Member

tejal29 commented Feb 27, 2020

Iirc test is posix, not bash.

Ack. I meant assertions inside Dockerfile.

@greut greut marked this pull request as ready for review February 28, 2020 06:40
@tejal29
Copy link
Member

tejal29 commented Feb 28, 2020

@greut would you like to also add the fix to the code so your test pass?
You can follow the hints here #1072 (comment)

Let me know if you need more help.

greut added a commit to greut/kaniko that referenced this pull request Feb 28, 2020
Running USER didn't reset HOME which had to be explicitly set.

Closes GoogleContainerTools#1072

Signed-off-by: Yoan Blanc <yoan@dosimple.ch>
@greut greut force-pushed the integration-test-homedir branch 2 times, most recently from fa0f9c1 to 403c042 Compare February 28, 2020 16:57
@greut
Copy link
Contributor Author

greut commented Feb 28, 2020

touch: cannot touch '//hello': Permission denied better, we are close.

@greut
Copy link
Contributor Author

greut commented Feb 29, 2020

@tejal29 The issue at hands is being able to read the /etc/passwd from the current state of the filesystem. Any ideas how this is doable? I should have read the design docs way earlier 😉

pkg/util/command_util.go Outdated Show resolved Hide resolved
pkg/commands/user.go Outdated Show resolved Hide resolved
Copy link
Member

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

explanation for a corner case.

@tejal29
Copy link
Member

tejal29 commented Mar 4, 2020

Thanks @greut I am going to rebase your PR and resolve conflicts. Hopefully your branch will be green now.

Signed-off-by: Yoan Blanc <yoan@dosimple.ch>
Running USER didn't reset HOME which had to be explicitly set.

Closes GoogleContainerTools#1082

Signed-off-by: Yoan Blanc <yoan@dosimple.ch>
Signed-off-by: Yoan Blanc <yoan@dosimple.ch>
greut and others added 5 commits March 4, 2020 14:14
Signed-off-by: Yoan Blanc <yoan@dosimple.ch>
Signed-off-by: Yoan Blanc <yoan@dosimple.ch>
Signed-off-by: Yoan Blanc <yoan@dosimple.ch>
Signed-off-by: Yoan Blanc <yoan@dosimple.ch>
@greut
Copy link
Contributor Author

greut commented Mar 4, 2020

👍

@tejal29
Copy link
Member

tejal29 commented Mar 4, 2020

Now we are seeing the error where Group is not user group.

            INFO[0008] RUN touch $HOME/hello                        
            INFO[0008] cmd: /bin/sh                                 
            INFO[0008] args: [-c touch $HOME/hello]                 
            touch: cannot touch '//hello': Permission denied
            error building image: error building stage: failed to execute command: waiting for process to exit: exit status 1

@greut
Copy link
Contributor Author

greut commented Mar 5, 2020

@tejal29 I would have preferred a merge over that rebase to be frank.

Signed-off-by: Yoan Blanc <yoan@dosimple.ch>
Signed-off-by: Yoan Blanc <yoan@dosimple.ch>
Signed-off-by: Yoan Blanc <yoan@dosimple.ch>
Signed-off-by: Yoan Blanc <yoan@dosimple.ch>
Signed-off-by: Yoan Blanc <yoan@dosimple.ch>
@tejal29 tejal29 merged commit 7bf3f87 into GoogleContainerTools:master Mar 5, 2020
@greut greut deleted the integration-test-homedir branch March 5, 2020 19:39
@greut
Copy link
Contributor Author

greut commented Mar 5, 2020

wooooooooooohooo 😜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes CLA signed by all commit authors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants