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.

Sign up for GitHub

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

Jump to bottom

[pvc] run git commands as gitpod user when using pvc #13929

Merged
merged 1 commit into from
Oct 24, 2022
Merged

[pvc] run git commands as gitpod user when using pvc #13929

merged 1 commit into from
Oct 24, 2022

Conversation

sagor999
Copy link
Contributor

@sagor999 sagor999 commented Oct 17, 2022 •
edited
Loading

Description

When using PVC, we run content-init from inside the supervisor, which runs as root user.
This causes git clone to run as root, causing cloned repo to be owned by root.
Workaround was to do chown as the end of clone, but on big repos it is slow.

This allows to run git op as gitpod user instead, thus not needing to do chown at the end.

Related Issue(s)

Fixes #12892

How to test

Open and close all kind of various ways of opening a workspace.
This code should only affect PVC code path, so to properly test you need to enable PVC feature flag on your account first.

Release Notes

none

Documentation

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview with-large-vm=true
  • /werft with-integration-tests=workspace
    Valid options are all, workspace, webapp, ide

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-pavel-chown.4 because the annotations in the pull request description changed
(with .werft/ from main)

@sagor999
Copy link
Contributor Author

sagor999 commented Oct 20, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-pavel-chown.8
(with .werft/ from main)

@sagor999
Copy link
Contributor Author

sagor999 commented Oct 21, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-pavel-chown.12
(with .werft/ from main)

@sagor999 sagor999 changed the title wip [pvc] run git commands as gitpod user when using pvc Oct 21, 2022
@sagor999
Copy link
Contributor Author

sagor999 commented Oct 21, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-pavel-chown.14
(with .werft/ from main)

@roboquat roboquat added size/M and removed size/S labels Oct 21, 2022
@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-pavel-chown.16 because the annotations in the pull request description changed
(with .werft/ from main)

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-pavel-chown.17 because the annotations in the pull request description changed
(with .werft/ from main)

@sagor999 sagor999 marked this pull request as ready for review October 21, 2022 17:09
@sagor999 sagor999 requested a review from a team October 21, 2022 17:09
@github-actions github-actions bot added the team: workspace Issue belongs to the Workspace team label Oct 21, 2022
@jenting
Copy link
Contributor

jenting commented Oct 21, 2022

When using PVC, we run content-init from inside the supervisor, which runs as root user.

Can we move the content-init operation ring1 or ring2 that runs as the gitpod user? So we don't need to specify run git clone as gitpod user.

@sagor999
Copy link
Contributor Author

@jenting frankly speaking, I am not sure, as there are a lot of moving parts in between ring0 and ring2. 🤔
Also I think there is some expectation that when ring1 or ring2 executes, that content init was already done.
Basically, if content init were to fail in ring2, it might break something else somewhere. 🤷
But if this solution is a no go for some reason, then I guess we can explore the option of trying to run content init inside ring2. 🤔

@csweichel do you have any thoughts on this? ^

jenting
jenting reviewed Oct 22, 2022
View reviewed changes
components/content-service/pkg/git/git.go
cmd := exec.Command("git", fullArgs...)
cmdName := "git"
if c.RunAsGitpodUser {
cmdName = "sudo"
Copy link
Contributor

Choose a reason for hiding this comment

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

sudo is required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah. that is the whole point here, and assumes that this runs as root (which it is from ring0).
so it runs sudo -u gitpod git ...

Copy link
Contributor

Choose a reason for hiding this comment

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

At this code phase, is the permission root?

If yes, I am thinking using runuser is more suitable or not. 🤔
https://prasadlinuxblog.wordpress.com/2012/09/04/392/

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it safe from a security viewpoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jenting I am not sure if there is any meaningful difference in our case for sudo vs runuser. WDYT?

@utam0k I don't think so. That code was already running as root (supervisor) prior to this changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with you.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just want to confirm with you about it. 💯

@kylos101
Copy link
Contributor

👋 @sagor999 did you mean to link this to #14003, instead of #12892 ?

I ask because this PR uses chown still, but as gitpod user, instead of root.

Does this PR solve the underlying performance issue, where using chown is slow? Can you share a link to a branch that is super slow in prod now, that is really fast using the preview environment for this PR? In other words, so we can see this actually improves (reduces) the time it takes to open a workspace?

@sagor999
Copy link
Contributor Author

@kylos101 no. It is linked to the correct issue that it is fixing.
And yes, it uses chown on the folder only (non recursive), which might be removed after this PR is merged: #14096

jenting
jenting approved these changes Oct 24, 2022
View reviewed changes
Copy link
Contributor

@jenting jenting left a comment

Choose a reason for hiding this comment

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

LGTM.

I can't think of any concerns about this change. So I'm okay to 🚢 it.

@roboquat roboquat merged commit aba04a0 into main Oct 24, 2022
@roboquat roboquat deleted the pavel/chown branch October 24, 2022 22:15
@roboquat roboquat added deployed: workspace Workspace team change is running in production deployed Change is completely running in production labels Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@utam0k utam0k utam0k left review comments

@jenting jenting jenting approved these changes

Assignees
No one assigned
Labels
deployed: workspace Workspace team change is running in production deployed Change is completely running in production release-note-none size/M team: workspace Issue belongs to the Workspace team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[PVC] investigate if possible to remove chown
5 participants
@sagor999 @jenting @kylos101 @utam0k @roboquat