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

Use rsync to copy pack contents #415

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Use rsync to copy pack contents #415

wants to merge 4 commits into from

Conversation

cognifloyd
Copy link
Member

Closes #245

@cognifloyd cognifloyd added this to the v1.1.0 milestone Apr 12, 2024
@cognifloyd cognifloyd requested a review from a team April 12, 2024 00:40
@cognifloyd cognifloyd self-assigned this Apr 12, 2024
@pull-request-size pull-request-size bot added the size/M PR that changes 30-99 lines. Good size to review. label Apr 12, 2024
Copy link
Member Author

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

Either we do something like this, or we make the command configurable via values.

Maybe .utility.rsync = "rsync -rlp". That would mean the user would have to update this if they needed to switch to "cp". Or we keep both, but allow specifying the args.

utility:
  rsync: "rsync -a"
  cp: "cp -aR"
utility:
  rsync: "rsync -rlE"
  cp: "cp -dR --preserve=mode"

The gotcha with making it configurable is that people could forget to include recursive, symlinks, and preserving execute bits.

/bin/cp -aR /opt/stackstorm/virtualenvs/. /opt/stackstorm/virtualenvs-shared
- >
if command rsync; then
rsync -a /opt/stackstorm/packs/. /opt/stackstorm/packs-shared &&
Copy link
Member Author

Choose a reason for hiding this comment

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

-a is equivalent to -rlptgoD: https://www.mankier.com/1/rsync#--archive

  • -r: copy directories recursively
  • -l: recreate symlinks in destination
  • -p: permissions
  • -t: modification times
  • -g: copy gid (group id) by name
  • -o: copy oid (owner id) by name
  • -D: devices (character and block) and specials (sockets, fifos, etc)

Of those, I'm confident we need -rlp or -rlE (where -E is short for --executability, a subset of -p). Depending on which user is running this, we might also want -og. The other parts of -a are probably not necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I'll push a commit that switches to -rlptD based on rsync...Stealthii:stackstorm-k8s:rsync

rsync -a /opt/stackstorm/packs/. /opt/stackstorm/packs-shared &&
rsync -a /opt/stackstorm/virtualenvs/. /opt/stackstorm/virtualenvs-shared;
else
/bin/cp -aR /opt/stackstorm/packs/. /opt/stackstorm/packs-shared &&
Copy link
Member Author

@cognifloyd cognifloyd Apr 12, 2024

Choose a reason for hiding this comment

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

-a is equivalent to -dR --preserve=all:

  • -d: is equivalent to -P --preserve=links
  • -P: no dereference / never follow symbolic links in SOURCE
  • -R: copy directories recursively
  • --preserve=all: preserve file attributes
    • mode
    • ownership for user and group
    • timestamps
    • links for hard links
    • context for security context
    • xattr for extended attributes

Of these we need -dR --preserve=mode (we need mode to maintain the execute bits of action files). Depending on which user is copying the file, we might also need --preserve=ownership.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I'll push a commit that switches to -RP --preserve=mode,timestamps,links,xattr based on rsync...Stealthii:stackstorm-k8s:rsync

@Stealthii
Copy link

In an attempt to solve the ownership issue that blocks #400, we should only preserve the file attributes that matter for this purpose.

I've updated a fork of this branch with proposed changes that once verified, could be brought into this PR.

rsync...Stealthii:stackstorm-k8s:rsync

cognifloyd and others added 3 commits May 9, 2024 16:39
This change aims to preserve file attributes when copying via rsync or
cp, without ownership to enable environments where containers are ran
without root privileges.

Co-authored-by: Daniel Porter <[email protected]>
@cognifloyd cognifloyd marked this pull request as ready for review May 9, 2024 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature size/M PR that changes 30-99 lines. Good size to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make packs/virtualenv file copy more robust (use rsync w/ fallback to cp)
2 participants