Page MenuHomePhabricator

Fix ownership on artifact copies
ClosedPublic

Authored by dduvall on Mar 9 2018, 11:56 PM.

Details

Reviewers
thcipriani
mmodell
hashar
demon
Group Reviewers
Release-Engineering-Team
Commits
rGBLBR50c5793952a7: Fix ownership on artifact copies
Patch without arc
git checkout -b D1002 && curl -L https://phabricator.wikimedia.org/D1002?download=true | git apply
Summary

The implementation of D984 did not include enforcing ownership for
build.CopyFrom instruction and so artifacts copied from one image to
another via copies: were problematically owned as root.

In order to fix this behavior:

  1. config.ArtifactConfig build.CopyFrom instructions are now injected duration build.PhaseInstall
  2. config.VariantConfig calls build.ApplyUser for these artifact instructions as well using the runs.as user
  3. build.CopyAs was refactored to wrap any build.Instruction which should only really be used with build.Copy or build.CopyFrom.
Test Plan

Run go test ./.... Run blubber against configuration with a variant that
uses copies and verify that the COPY --from instructions also include a
--chown flag.

Diff Detail

Repository
rGBLBR Blubber
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

dduvall created this revision.Mar 9 2018, 11:56 PM
Restricted Application added a reviewer: Release-Engineering-Team. · View Herald TranscriptMar 9 2018, 11:56 PM
Restricted Application added a project: Release-Engineering-Team. · View Herald Transcript
dduvall requested review of this revision.Mar 9 2018, 11:56 PM
dduvall updated this revision to Diff 2653.Mar 22 2018, 5:34 PM
dduvall edited the summary of this revision. (Show Details)

Refactored build.CopyAs to use a generic Instruction anonymouse field instread of separate fields for Copy and CopyFrom usage.

thcipriani accepted this revision.Mar 22 2018, 5:50 PM

Works. Refactor makes building a CopyAs cleaner than having 2 embedded structs.

This revision is now accepted and ready to land.Mar 22 2018, 5:50 PM
This revision was automatically updated to reflect the committed changes.