Skip to content

Commit

Permalink
Fix TODO: de-spaghettify run mounts
Browse files Browse the repository at this point in the history 
Code Cleanup: Buildkit run mount setup functions no longer have 13
arguments and are slightly more readable. Use structs instead.

[NO NEW TESTS NEEDED]

Signed-off-by: Ashley Cui <acui@redhat.com>
  • Loading branch information
@ashley-cui
ashley-cui committed Jun 6, 2022
1 parent 468b130 commit 236c0ba
Show file tree
Hide file tree
Showing 3 changed files with 94 additions and 49 deletions.
32 changes: 16 additions & 16 deletions imagebuildah/stage_executor.go
View file
Expand Up @@ -597,30 +597,30 @@ func (s *StageExecutor) Run(run imagebuilder.Run, config docker.Config) error {
stdin = devNull
}
options := buildah.RunOptions{
Logger: s.executor.logger,
Hostname: config.Hostname,
Runtime: s.executor.runtime,
Args: s.executor.runtimeArgs,
Cmd: config.Cmd,
ContextDir: s.executor.contextDir,
Entrypoint: config.Entrypoint,
Env: config.Env,
Hostname: config.Hostname,
Logger: s.executor.logger,
Mounts: append([]Mount{}, s.executor.transientMounts...),
NamespaceOptions: s.executor.namespaceOptions,
NoHosts: s.executor.noHosts,
NoPivot: os.Getenv("BUILDAH_NOPIVOT") != "",
Mounts: append([]Mount{}, s.executor.transientMounts...),
Env: config.Env,
User: config.User,
WorkingDir: config.WorkingDir,
Entrypoint: config.Entrypoint,
ContextDir: s.executor.contextDir,
Cmd: config.Cmd,
Stdin: stdin,
Stdout: s.executor.out,
Stderr: s.executor.err,
Quiet: s.executor.quiet,
NamespaceOptions: s.executor.namespaceOptions,
Terminal: buildah.WithoutTerminal,
RunMounts: run.Mounts,
Runtime: s.executor.runtime,
Secrets: s.executor.secrets,
SSHSources: s.executor.sshsources,
RunMounts: run.Mounts,
StageMountPoints: stageMountPoints,
Stderr: s.executor.err,
Stdin: stdin,
Stdout: s.executor.out,
SystemContext: s.executor.systemContext,
Terminal: buildah.WithoutTerminal,
User: config.User,
WorkingDir: config.WorkingDir,
}
if config.NetworkDisabled {
options.ConfigureNetwork = buildah.NetworkDisabled
Expand Down
25 changes: 25 additions & 0 deletions run.go
View file
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/containers/buildah/pkg/sshagent"
"github.com/containers/image/v5/types"
"github.com/opencontainers/runtime-spec/specs-go"
spec "github.com/opencontainers/runtime-spec/specs-go"
"github.com/sirupsen/logrus"
)

Expand Down Expand Up @@ -178,3 +179,27 @@ type runMountArtifacts struct {
// LockedTargets to be unlocked if there are any.
LockedTargets []string
}

// RunMountInfo are the available run mounts for this run
type runMountInfo struct {
// ContextDir is the root directory for the source location for bind mounts.
ContextDir string
// Secrets are the available secrets to use in a RUN
Secrets map[string]define.Secret
// SSHSources is the available ssh agents to use in a RUN
SSHSources map[string]*sshagent.Source `json:"-"`
// Map of stages and container mountpoint if any from stage executor
StageMountPoints map[string]internal.StageMountDetails
// System context of current build
SystemContext *types.SystemContext
}

// IDMaps are the UIDs, GID, and maps for the run
type IDMaps struct {
uidmap []spec.LinuxIDMapping
gidmap []spec.LinuxIDMapping
rootUID int
rootGID int
processUID int
processGID int
}
86 changes: 53 additions & 33 deletions run_linux.go
View file
Expand Up @@ -321,7 +321,16 @@ rootless=%d

bindFiles["/run/.containerenv"] = containerenvPath
}
runArtifacts, err := b.setupMounts(options.SystemContext, mountPoint, spec, path, options.Mounts, bindFiles, volumes, b.CommonBuildOpts.Volumes, options.Secrets, options.SSHSources, options.RunMounts, options.ContextDir, options.StageMountPoints)

runMountInfo := runMountInfo{
ContextDir: options.ContextDir,
Secrets: options.Secrets,
SSHSources: options.SSHSources,
StageMountPoints: options.StageMountPoints,
SystemContext: options.SystemContext,
}

runArtifacts, err := b.setupMounts(mountPoint, spec, path, options.Mounts, bindFiles, volumes, b.CommonBuildOpts.Volumes, options.RunMounts, runMountInfo)
if err != nil {
return errors.Wrapf(err, "error resolving mountpoints for container %q", b.ContainerID)
}
Expand Down Expand Up @@ -478,7 +487,7 @@ func runSetupBuiltinVolumes(mountLabel, mountPoint, containerDir string, builtin
return mounts, nil
}

func (b *Builder) setupMounts(context *imagetypes.SystemContext, mountPoint string, spec *specs.Spec, bundlePath string, optionMounts []specs.Mount, bindFiles map[string]string, builtinVolumes, volumeMounts []string, secrets map[string]define.Secret, sshSources map[string]*sshagent.Source, runFileMounts []string, contextDir string, stageMountPoints map[string]internal.StageMountDetails) (*runMountArtifacts, error) {
func (b *Builder) setupMounts(mountPoint string, spec *specs.Spec, bundlePath string, optionMounts []specs.Mount, bindFiles map[string]string, builtinVolumes, volumeMounts []string, runFileMounts []string, runMountInfo runMountInfo) (*runMountArtifacts, error) {
// Start building a new list of mounts.
var mounts []specs.Mount
haveMount := func(destination string) bool {
Expand Down Expand Up @@ -521,9 +530,16 @@ func (b *Builder) setupMounts(context *imagetypes.SystemContext, mountPoint stri
// Get the list of subscriptions mounts.
subscriptionMounts := subscriptions.MountsWithUIDGID(b.MountLabel, cdir, b.DefaultMountsFilePath, mountPoint, int(rootUID), int(rootGID), unshare.IsRootless(), false)

idMaps := IDMaps{
uidmap: spec.Linux.UIDMappings,
gidmap: spec.Linux.GIDMappings,
rootUID: int(rootUID),
rootGID: int(rootGID),
processUID: int(processUID),
processGID: int(processGID),
}
// Get the list of mounts that are just for this Run() call.
// TODO: acui: de-spaghettify run mounts
runMounts, mountArtifacts, err := b.runSetupRunMounts(context, runFileMounts, secrets, stageMountPoints, sshSources, cdir, contextDir, spec.Linux.UIDMappings, spec.Linux.GIDMappings, int(rootUID), int(rootGID), int(processUID), int(processGID))
runMounts, mountArtifacts, err := b.runSetupRunMounts(runFileMounts, runMountInfo, idMaps)
if err != nil {
return nil, err
}
Expand All @@ -535,7 +551,7 @@ func (b *Builder) setupMounts(context *imagetypes.SystemContext, mountPoint stri
}

// Get the list of explicitly-specified volume mounts.
volumes, err := b.runSetupVolumeMounts(spec.Linux.MountLabel, volumeMounts, optionMounts, int(rootUID), int(rootGID), int(processUID), int(processGID))
volumes, err := b.runSetupVolumeMounts(spec.Linux.MountLabel, volumeMounts, optionMounts, idMaps)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -1803,7 +1819,7 @@ func (b *Builder) cleanupTempVolumes() {
}
}

func (b *Builder) runSetupVolumeMounts(mountLabel string, volumeMounts []string, optionMounts []specs.Mount, rootUID, rootGID, processUID, processGID int) (mounts []specs.Mount, Err error) {
func (b *Builder) runSetupVolumeMounts(mountLabel string, volumeMounts []string, optionMounts []specs.Mount, idMaps IDMaps) (mounts []specs.Mount, Err error) {
// Make sure the overlay directory is clean before running
containerDir, err := b.store.ContainerDirectory(b.ContainerID)
if err != nil {
Expand Down Expand Up @@ -1861,7 +1877,7 @@ func (b *Builder) runSetupVolumeMounts(mountLabel string, volumeMounts []string,
}
}
if foundU {
if err := chown.ChangeHostPathOwnership(host, true, processUID, processGID); err != nil {
if err := chown.ChangeHostPathOwnership(host, true, idMaps.processUID, idMaps.processGID); err != nil {
return specs.Mount{}, err
}
}
Expand All @@ -1875,13 +1891,14 @@ func (b *Builder) runSetupVolumeMounts(mountLabel string, volumeMounts []string,
return specs.Mount{}, err
}

contentDir, err := overlay.TempDir(containerDir, rootUID, rootGID)
contentDir, err := overlay.TempDir(containerDir, idMaps.rootUID, idMaps.rootGID)
if err != nil {
return specs.Mount{}, errors.Wrapf(err, "failed to create TempDir in the %s directory", containerDir)
}

overlayOpts := overlay.Options{RootUID: rootUID,
RootGID: rootGID,
overlayOpts := overlay.Options{
RootUID: idMaps.rootUID,
RootGID: idMaps.rootGID,
UpperDirOptionFragment: upperDir,
WorkDirOptionFragment: workDir,
GraphOpts: b.store.GraphOptions(),
Expand All @@ -1894,7 +1911,7 @@ func (b *Builder) runSetupVolumeMounts(mountLabel string, volumeMounts []string,

// If chown true, add correct ownership to the overlay temp directories.
if foundU {
if err := chown.ChangeHostPathOwnership(contentDir, true, processUID, processGID); err != nil {
if err := chown.ChangeHostPathOwnership(contentDir, true, idMaps.processUID, idMaps.processGID); err != nil {
return specs.Mount{}, err
}
}
Expand Down Expand Up @@ -2512,7 +2529,7 @@ func init() {
}

// runSetupRunMounts sets up mounts that exist only in this RUN, not in subsequent runs
func (b *Builder) runSetupRunMounts(context *imagetypes.SystemContext, mounts []string, secrets map[string]define.Secret, stageMountPoints map[string]internal.StageMountDetails, sshSources map[string]*sshagent.Source, containerWorkingDir string, contextDir string, uidmap []spec.LinuxIDMapping, gidmap []spec.LinuxIDMapping, rootUID int, rootGID int, processUID int, processGID int) ([]spec.Mount, *runMountArtifacts, error) {
func (b *Builder) runSetupRunMounts(mounts []string, sources runMountInfo, idMaps IDMaps) ([]spec.Mount, *runMountArtifacts, error) {
mountTargets := make([]string, 0, 10)
tmpFiles := make([]string, 0, len(mounts))
mountImages := make([]string, 0, 10)
Expand All @@ -2532,10 +2549,10 @@ func (b *Builder) runSetupRunMounts(context *imagetypes.SystemContext, mounts []
if len(arr) == 2 {
tokens = strings.Split(arr[1], ",")
}
// For now, we only support type secret.

switch kv[1] {
case "secret":
mount, envFile, err := getSecretMount(tokens, secrets, b.MountLabel, containerWorkingDir, uidmap, gidmap)
mount, envFile, err := b.getSecretMount(tokens, sources.Secrets, idMaps)
if err != nil {
return nil, nil, err
}
Expand All @@ -2547,7 +2564,7 @@ func (b *Builder) runSetupRunMounts(context *imagetypes.SystemContext, mounts []
}
}
case "ssh":
mount, agent, err := b.getSSHMount(tokens, sshCount, sshSources, b.MountLabel, uidmap, gidmap, b.ProcessLabel)
mount, agent, err := b.getSSHMount(tokens, sshCount, sources.SSHSources, idMaps)
if err != nil {
return nil, nil, err
}
Expand All @@ -2562,7 +2579,7 @@ func (b *Builder) runSetupRunMounts(context *imagetypes.SystemContext, mounts []
sshCount++
}
case "bind":
mount, image, err := b.getBindMount(context, tokens, contextDir, rootUID, rootGID, processUID, processGID, stageMountPoints)
mount, image, err := b.getBindMount(tokens, sources.SystemContext, sources.ContextDir, sources.StageMountPoints, idMaps)
if err != nil {
return nil, nil, err
}
Expand All @@ -2573,14 +2590,14 @@ func (b *Builder) runSetupRunMounts(context *imagetypes.SystemContext, mounts []
mountImages = append(mountImages, image)
}
case "tmpfs":
mount, err := b.getTmpfsMount(tokens, rootUID, rootGID, processUID, processGID)
mount, err := b.getTmpfsMount(tokens, idMaps)
if err != nil {
return nil, nil, err
}
finalMounts = append(finalMounts, *mount)
mountTargets = append(mountTargets, mount.Destination)
case "cache":
mount, lockedPaths, err := b.getCacheMount(tokens, rootUID, rootGID, processUID, processGID, stageMountPoints)
mount, lockedPaths, err := b.getCacheMount(tokens, sources.StageMountPoints, idMaps)
if err != nil {
return nil, nil, err
}
Expand All @@ -2602,7 +2619,7 @@ func (b *Builder) runSetupRunMounts(context *imagetypes.SystemContext, mounts []
return finalMounts, artifacts, nil
}

func (b *Builder) getBindMount(context *imagetypes.SystemContext, tokens []string, contextDir string, rootUID, rootGID, processUID, processGID int, stageMountPoints map[string]internal.StageMountDetails) (*spec.Mount, string, error) {
func (b *Builder) getBindMount(tokens []string, context *imagetypes.SystemContext, contextDir string, stageMountPoints map[string]internal.StageMountDetails, idMaps IDMaps) (*spec.Mount, string, error) {
if contextDir == "" {
return nil, "", errors.New("Context Directory for current run invocation is not configured")
}
Expand All @@ -2612,42 +2629,42 @@ func (b *Builder) getBindMount(context *imagetypes.SystemContext, tokens []strin
return nil, image, err
}
optionMounts = append(optionMounts, mount)
volumes, err := b.runSetupVolumeMounts(b.MountLabel, nil, optionMounts, rootUID, rootGID, processUID, processGID)
volumes, err := b.runSetupVolumeMounts(b.MountLabel, nil, optionMounts, idMaps)
if err != nil {
return nil, image, err
}
return &volumes[0], image, nil
}

func (b *Builder) getTmpfsMount(tokens []string, rootUID, rootGID, processUID, processGID int) (*spec.Mount, error) {
func (b *Builder) getTmpfsMount(tokens []string, idMaps IDMaps) (*spec.Mount, error) {
var optionMounts []specs.Mount
mount, err := internalParse.GetTmpfsMount(tokens)
if err != nil {
return nil, err
}
optionMounts = append(optionMounts, mount)
volumes, err := b.runSetupVolumeMounts(b.MountLabel, nil, optionMounts, rootUID, rootGID, processUID, processGID)
volumes, err := b.runSetupVolumeMounts(b.MountLabel, nil, optionMounts, idMaps)
if err != nil {
return nil, err
}
return &volumes[0], nil
}

func (b *Builder) getCacheMount(tokens []string, rootUID, rootGID, processUID, processGID int, stageMountPoints map[string]internal.StageMountDetails) (*spec.Mount, []string, error) {
func (b *Builder) getCacheMount(tokens []string, stageMountPoints map[string]internal.StageMountDetails, idMaps IDMaps) (*spec.Mount, []string, error) {
var optionMounts []specs.Mount
mount, lockedTargets, err := internalParse.GetCacheMount(tokens, b.store, b.MountLabel, stageMountPoints)
if err != nil {
return nil, lockedTargets, err
}
optionMounts = append(optionMounts, mount)
volumes, err := b.runSetupVolumeMounts(b.MountLabel, nil, optionMounts, rootUID, rootGID, processUID, processGID)
volumes, err := b.runSetupVolumeMounts(b.MountLabel, nil, optionMounts, idMaps)
if err != nil {
return nil, lockedTargets, err
}
return &volumes[0], lockedTargets, nil
}

func getSecretMount(tokens []string, secrets map[string]define.Secret, mountlabel string, containerWorkingDir string, uidmap []spec.LinuxIDMapping, gidmap []spec.LinuxIDMapping) (*spec.Mount, string, error) {
func (b *Builder) getSecretMount(tokens []string, secrets map[string]define.Secret, idMaps IDMaps) (*spec.Mount, string, error) {
errInvalidSyntax := errors.New("secret should have syntax id=id[,target=path,required=bool,mode=uint,uid=uint,gid=uint")
if len(tokens) == 0 {
return nil, "", errInvalidSyntax
Expand Down Expand Up @@ -2721,6 +2738,10 @@ func getSecretMount(tokens []string, secrets map[string]define.Secret, mountlabe
envFile = tmpFile.Name()
ctrFileOnHost = tmpFile.Name()
case "file":
containerWorkingDir, err := b.store.ContainerDirectory(b.ContainerID)
if err != nil {
return nil, "", err
}
data, err = ioutil.ReadFile(secr.Source)
if err != nil {
return nil, "", err
Expand All @@ -2739,10 +2760,10 @@ func getSecretMount(tokens []string, secrets map[string]define.Secret, mountlabe
return nil, "", err
}

if err := label.Relabel(ctrFileOnHost, mountlabel, false); err != nil {
if err := label.Relabel(ctrFileOnHost, b.MountLabel, false); err != nil {
return nil, "", err
}
hostUID, hostGID, err := util.GetHostIDs(uidmap, gidmap, uid, gid)
hostUID, hostGID, err := util.GetHostIDs(idMaps.uidmap, idMaps.gidmap, uid, gid)
if err != nil {
return nil, "", err
}
Expand All @@ -2762,7 +2783,7 @@ func getSecretMount(tokens []string, secrets map[string]define.Secret, mountlabe
}

// getSSHMount parses the --mount type=ssh flag in the Containerfile, checks if there's an ssh source provided, and creates and starts an ssh-agent to be forwarded into the container
func (b *Builder) getSSHMount(tokens []string, count int, sshsources map[string]*sshagent.Source, mountlabel string, uidmap []spec.LinuxIDMapping, gidmap []spec.LinuxIDMapping, processLabel string) (*spec.Mount, *sshagent.AgentServer, error) {
func (b *Builder) getSSHMount(tokens []string, count int, sshsources map[string]*sshagent.Source, idMaps IDMaps) (*spec.Mount, *sshagent.AgentServer, error) {
errInvalidSyntax := errors.New("ssh should have syntax id=id[,target=path,required=bool,mode=uint,uid=uint,gid=uint")

var err error
Expand Down Expand Up @@ -2829,25 +2850,24 @@ func (b *Builder) getSSHMount(tokens []string, count int, sshsources map[string]
return nil, nil, err
}
// Start ssh server, and get the host sock we're mounting in the container
hostSock, err := fwdAgent.Serve(processLabel)
hostSock, err := fwdAgent.Serve(b.ProcessLabel)
if err != nil {
return nil, nil, err
}

if err := label.Relabel(filepath.Dir(hostSock), mountlabel, false); err != nil {
if err := label.Relabel(filepath.Dir(hostSock), b.MountLabel, false); err != nil {
if shutdownErr := fwdAgent.Shutdown(); shutdownErr != nil {
b.Logger.Errorf("error shutting down agent: %v", shutdownErr)
}
return nil, nil, err
}
if err := label.Relabel(hostSock, mountlabel, false); err != nil {
if err := label.Relabel(hostSock, b.MountLabel, false); err != nil {
if shutdownErr := fwdAgent.Shutdown(); shutdownErr != nil {
b.Logger.Errorf("error shutting down agent: %v", shutdownErr)
}
return nil, nil, err
}

hostUID, hostGID, err := util.GetHostIDs(uidmap, gidmap, uid, gid)
hostUID, hostGID, err := util.GetHostIDs(idMaps.uidmap, idMaps.gidmap, uid, gid)
if err != nil {
if shutdownErr := fwdAgent.Shutdown(); shutdownErr != nil {
b.Logger.Errorf("error shutting down agent: %v", shutdownErr)
Expand Down

0 comments on commit 236c0ba

Please sign in to comment.