pr feedback and minor fixes

Signed-off-by: Maksim An <maksiman@microsoft.com>

internal/guest/runtime/hcsv2/workload_container.go

@@ -5,7 +5,6 @@ package hcsv2
 
 import (
 	"context"
-	"fmt"
 	"os"
 	"path/filepath"
 	"strings"
@@ -36,56 +35,56 @@ func mkdirAllModePerm(target string) error {
 }
 
 func updateSandboxMounts(sbid string, spec *oci.Spec) error {
-	for i, specMount := range spec.Mounts {
-		for _, updMount := range specInternal.GenerateSandboxMounts(sbid, spec) {
-			if specMount.Destination != updMount.Destination {
-				continue
-			}
-			src := filepath.Clean(updMount.Source)
-			// Make sure that the cleaned path is still under the correct directory.
-			if !strings.HasPrefix(src, guestpath.SandboxMountsDir(sbid)) {
-				return fmt.Errorf("mount path %v for mount %v is not within sandbox's mounts dir", updMount.Source, specMount.Source)
+	for i, m := range spec.Mounts {
+		if strings.HasPrefix(m.Source, guestpath.SandboxMountPrefix) {
+			mountsDir := guestpath.SandboxMountsDir(sbid)
+			subPath := strings.TrimPrefix(m.Source, guestpath.SandboxMountPrefix)
+			sandboxSource := filepath.Join(mountsDir, subPath)
+
+			// filepath.Join cleans the resulting path before returning, so it would resolve the relative path if one was given.
+			// Hence, we need to ensure that the resolved path is still under the correct directory
+			if !strings.HasPrefix(sandboxSource, mountsDir) {
+				return errors.Errorf("mount path %v for mount %v is not within sandbox's mounts dir", sandboxSource, m.Source)
 			}
 
-			spec.Mounts[i].Source = updMount.Source
+			spec.Mounts[i].Source = sandboxSource
 
-			if _, err := os.Stat(src); os.IsNotExist(err) {
-				if err := mkdirAllModePerm(src); err != nil {
+			_, err := os.Stat(sandboxSource)
+			if os.IsNotExist(err) {
+				if err := mkdirAllModePerm(sandboxSource); err != nil {
 					return err
 				}
 			}
-			break
 		}
 	}
 	return nil
 }
 
 func updateHugePageMounts(sbid string, spec *oci.Spec) error {
-	for i, specMount := range spec.Mounts {
-		for _, updMount := range specInternal.GenerateHugePageMounts(sbid, spec) {
-			if specMount.Destination != updMount.Destination {
-				continue
-			}
-			hugePagesMountDir := guestpath.HugePagesMountsDir(sbid)
-			src := filepath.Clean(updMount.Source)
-			// Make sure that the cleaned path is still under the correct directory.
-			if !strings.HasPrefix(src, hugePagesMountDir) {
-				return fmt.Errorf("mount path %v for mount %v is not within hugepages's mounts dir", src, specMount.Source)
+	for i, m := range spec.Mounts {
+		if strings.HasPrefix(m.Source, guestpath.HugePagesMountPrefix) {
+			mountsDir := guestpath.HugePagesMountsDir(sbid)
+			subPath := strings.TrimPrefix(m.Source, guestpath.HugePagesMountPrefix)
+			pageSize := strings.Split(subPath, string(os.PathSeparator))[0]
+			hugePageMountSource := filepath.Join(mountsDir, subPath)
+
+			// filepath.Join cleans the resulting path before returning so it would resolve the relative path if one was given.
+			// Hence, we need to ensure that the resolved path is still under the correct directory
+			if !strings.HasPrefix(hugePageMountSource, mountsDir) {
+				return errors.Errorf("mount path %v for mount %v is not within hugepages's mounts dir", hugePageMountSource, m.Source)
 			}
 
-			spec.Mounts[i].Source = src
+			spec.Mounts[i].Source = hugePageMountSource
 
-			subPath := strings.TrimPrefix(src, hugePagesMountDir)
-			pageSize := strings.Split(subPath, string(os.PathSeparator))[0]
-			if _, err := os.Stat(src); os.IsNotExist(err) {
-				if err := mkdirAllModePerm(src); err != nil {
+			_, err := os.Stat(hugePageMountSource)
+			if os.IsNotExist(err) {
+				if err := mkdirAllModePerm(hugePageMountSource); err != nil {
 					return err
 				}
-				if err := unix.Mount("none", src, "hugetlbfs", 0, "pageSize="+pageSize); err != nil {
-					return fmt.Errorf("mount operation failed for %v failed with error %v", src, err)
+				if err := unix.Mount("none", hugePageMountSource, "hugetlbfs", 0, "pagesize="+pageSize); err != nil {
+					return errors.Errorf("mount operation failed for %v failed with error %v", hugePageMountSource, err)
 				}
 			}
-			break
 		}
 	}
 	return nil

internal/spec/spec.go

@@ -72,56 +72,13 @@ func SandboxMountSource(sandboxID, path string) string {
 	return filepath.Join(mountsDir, subPath)
 }
 
-// GenerateSandboxMounts generates an array of specs.Mount by replacing sandbox
-// prefix with the actual path inside UVM. Original spec is left untouched and
-// it's the responsibility of a caller to update it.
-func GenerateSandboxMounts(sandboxID string, spec *specs.Spec) []specs.Mount {
-	var sandboxMounts []specs.Mount
-
-	for _, m := range spec.Mounts {
-		if !strings.HasPrefix(m.Source, guestpath.SandboxMountPrefix) {
-			continue
-		}
-		// copy over all fields except for the Source.
-		mnt := specs.Mount{
-			Source:      SandboxMountSource(sandboxID, m.Source),
-			Destination: m.Destination,
-			Type:        m.Type,
-			Options:     m.Options,
-		}
-		sandboxMounts = append(sandboxMounts, mnt)
-	}
-	return sandboxMounts
-}
-
 // HugePagesMountSource returns hugepages mount path inside UVM
 func HugePagesMountSource(sandboxID, path string) string {
 	mountsDir := guestpath.HugePagesMountsDir(sandboxID)
 	subPath := strings.TrimPrefix(path, guestpath.HugePagesMountPrefix)
 	return filepath.Join(mountsDir, subPath)
 }
 
-// GenerateHugePageMounts generates an array of specs.Mount by replacing
-// hugepages prefix with the actual path inside UVM. Original spec is left
-// untouched and it's the responsibility of a caller to update it.
-func GenerateHugePageMounts(sandboxID string, spec *specs.Spec) []specs.Mount {
-	var hpMounts []specs.Mount
-	for _, m := range spec.Mounts {
-		if !strings.HasPrefix(m.Source, guestpath.HugePagesMountPrefix) {
-			continue
-		}
-		// copy over all fields except for the Source
-		mnt := specs.Mount{
-			Source:      HugePagesMountSource(sandboxID, m.Source),
-			Destination: m.Destination,
-			Type:        m.Type,
-			Options:     m.Options,
-		}
-		hpMounts = append(hpMounts, mnt)
-	}
-	return hpMounts
-}
-
 // MountPresent checks if mountPath is present in the specMounts array.
 func MountPresent(mountPath string, specMounts []specs.Mount) bool {
 	for _, m := range specMounts {

pkg/securitypolicy/securitypolicyenforcer.go

@@ -129,7 +129,7 @@ func NewStandardSecurityPolicyEnforcer(containers []securityPolicyContainer, enc
 	}
 }
 
-func (c *Containers) toInternal() ([]securityPolicyContainer, error) {
+func (c Containers) toInternal() ([]securityPolicyContainer, error) {
 	containerMapLength := len(c.Elements)
 	if c.Length != containerMapLength {
 		err := fmt.Errorf(
@@ -153,33 +153,33 @@ func (c *Containers) toInternal() ([]securityPolicyContainer, error) {
 			return nil, err
 		}
 		// save off new container
-		internal[i] = *cInternal
+		internal[i] = cInternal
 	}
 
 	return internal, nil
 }
 
-func (c *Container) toInternal() (*securityPolicyContainer, error) {
+func (c Container) toInternal() (securityPolicyContainer, error) {
 	command, err := c.Command.toInternal()
 	if err != nil {
-		return nil, err
+		return securityPolicyContainer{}, err
 	}
 
 	envRules, err := c.EnvRules.toInternal()
 	if err != nil {
-		return nil, err
+		return securityPolicyContainer{}, err
 	}
 
 	layers, err := c.Layers.toInternal()
 	if err != nil {
-		return nil, err
+		return securityPolicyContainer{}, err
 	}
 
 	mounts, err := c.Mounts.toInternal()
 	if err != nil {
-		return nil, err
+		return securityPolicyContainer{}, err
 	}
-	return &securityPolicyContainer{
+	return securityPolicyContainer{
 		Command:  command,
 		EnvRules: envRules,
 		Layers:   layers,
@@ -190,7 +190,7 @@ func (c *Container) toInternal() (*securityPolicyContainer, error) {
 	}, nil
 }
 
-func (c *CommandArgs) toInternal() ([]string, error) {
+func (c CommandArgs) toInternal() ([]string, error) {
 	if c.Length != len(c.Elements) {
 		return nil, fmt.Errorf("command argument numbers don't match in policy. expected: %d, actual: %d", c.Length, len(c.Elements))
 	}
@@ -217,23 +217,23 @@ func (e EnvRules) toInternal() ([]EnvRuleConfig, error) {
 	return envRules, nil
 }
 
-func (l *Layers) toInternal() ([]string, error) {
+func (l Layers) toInternal() ([]string, error) {
 	if l.Length != len(l.Elements) {
 		return nil, fmt.Errorf("layer numbers don't match in policy. expected: %d, actual: %d", l.Length, len(l.Elements))
 	}
 
 	return stringMapToStringArray(l.Elements)
 }
 
-func (o *Options) toInternal() ([]string, error) {
+func (o Options) toInternal() ([]string, error) {
 	optLength := len(o.Elements)
 	if o.Length != optLength {
 		return nil, fmt.Errorf("mount option numbers don't match in policy. expected: %d, actual: %d", o.Length, optLength)
 	}
 	return stringMapToStringArray(o.Elements)
 }
 
-func (m *Mounts) toInternal() ([]mountInternal, error) {
+func (m Mounts) toInternal() ([]mountInternal, error) {
 	mountLength := len(m.Elements)
 	if m.Length != mountLength {
 		return nil, fmt.Errorf("mount constraint numbers don't match in policy. expected: %d, actual: %d", m.Length, mountLength)