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

Allow Sync for non-root containers-hotreload example #3680

Merged
merged 6 commits into from
Mar 4, 2020

Conversation

ibidani
Copy link
Contributor

@ibidani ibidani commented Feb 12, 2020

When deploying the hot-reload example to non-root containers, skaffold dev sync functionality fails with "permissions denied"
There has been multiple open issues referring to this error: #2041 #1950
This change will ensure examples will be compatible with more clusters with higher security requirements.

Description
chowning the target folder with the proper runtime UID so that tar xmf (Sync) command will not fail

User facing changes

Write n/a if not output or log lines changed and no behavior is changed

Before
The following error cases the Sync on save to break

WARN[0024] Skipping deploy due to sync error: copying files: Running [kubectl --context <context> exec node-56846577b4-c878r --namespace <namespace> -c node -i -- tar xmf - -C / --no-same-owner]
 - stdout: 
 - stderr: tar: removing leading '/' from member names
tar: can't remove old file app/index.js: Permission denied
command terminated with exit code 1
: exit status 1 

After
skaffold dev sync is working as expected on both node and python examples

Next PRs.

Submitter Checklist

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

  • Includes unit tests
  • Mentions any output changes.
  • Adds documentation as needed: user docs, YAML reference, CLI reference.
  • Adds integration tests if needed.

Reviewer Notes

Release Notes

  • hot-reload example skaffold dev sync working on non-root containers

@codecov
Copy link

codecov bot commented Feb 12, 2020

Codecov Report

Merging #3680 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted Files Coverage Δ
...affold/kubernetes/portforward/kubectl_forwarder.go 65.85% <0%> (-2.44%) ⬇️

@dgageot dgageot self-assigned this Feb 13, 2020
@dgageot dgageot added the kokoro:run runs the kokoro jobs on a PR label Feb 13, 2020
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Feb 13, 2020
Copy link
Contributor

@dgageot dgageot left a comment

Choose a reason for hiding this comment

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

I just realized those changes should be made in integration/examples/hot-reload too. Otherzise, next time we do a release, your changes will be lost.

@ibidani
Copy link
Contributor Author

ibidani commented Feb 13, 2020

Oops, I'll update the integration files today

@ibidani
Copy link
Contributor Author

ibidani commented Feb 14, 2020

I updated the integration files
Thanks @dgageot

@tejal29 tejal29 added the kokoro:run runs the kokoro jobs on a PR label Feb 26, 2020
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Feb 26, 2020
@dgageot dgageot self-requested a review February 28, 2020 17:55
Copy link
Contributor

@dgageot dgageot left a comment

Choose a reason for hiding this comment

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

@ibidani I've thought about your PR some more. And I wonder if you shouldn't be using COPY --chown instead, like in https://github.com/GoogleContainerTools/skaffold/blob/master/examples/nodejs/backend/Dockerfile. wdyt?

@ibidani
Copy link
Contributor Author

ibidani commented Feb 28, 2020

Thanks Good suggestion, I'll update the code tonight

@ibidani
Copy link
Contributor Author

ibidani commented Feb 29, 2020

@dgageot I just realized this PR inspired you to open #3683 which fixes this issue.
Should I close this PR?

@dgageot
Copy link
Contributor

dgageot commented Feb 29, 2020

I don’t think so. I’d love to see all our examples better represent real projects. I didn’t want to step on your feet and changed a different sample.

@ibidani
Copy link
Contributor Author

ibidani commented Feb 29, 2020

I've backported the great changes in #3683 into hot-reload example, per my tests on unprivileged cluster securityContext was necessary for the hot-load to succeed without the "tar: can't remove old file app/index.js: Permission denied" error mentioned above.
I've learned so much, thanks @dgageot

securityContext:
    runAsUser: 1000

@dgageot dgageot added the kokoro:run runs the kokoro jobs on a PR label Mar 1, 2020
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Mar 1, 2020
@dgageot dgageot merged commit c5b9e52 into GoogleContainerTools:master Mar 4, 2020
kelsk pushed a commit to kelsk/skaffold that referenced this pull request Apr 16, 2021
…Tools#3680)

* Allow Sync for non-root containers-hotreload example

* Allow Sync for non-root containers-hotreload example - integration dir

* backporting GoogleContainerTools#3683 changes for better examples

* integration files updates of hot-reload example

* newline at the end of file

* newline at the end of file
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.

5 participants