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

Fix handling of readonly containers when defined in kube.yaml #16682

Merged
merged 1 commit into from
Dec 4, 2022
Merged

Fix handling of readonly containers when defined in kube.yaml #16682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 39 additions & 34 deletions libpod/kube.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,7 @@ func (p *Pod) podWithContainers(ctx context.Context, containers []*Container, po
}
}
}
podVolumes := make([]v1.Volume, 0, len(deDupPodVolumes))
podVolumes := []v1.Volume{}
for _, vol := range deDupPodVolumes {
podVolumes = append(podVolumes, *vol)
}
Expand Down Expand Up @@ -471,18 +471,12 @@ func newPodObject(podName string, annotations map[string]string, initCtrs, conta
CreationTimestamp: v12.Now(),
Annotations: annotations,
}
// Set enableServiceLinks to false as podman doesn't use the service port environment variables
enableServiceLinks := false
Comment on lines -474 to -475
Copy link
Member

Choose a reason for hiding this comment

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

This is what's causing e2e test failures:

Value for field 'EnableServiceLinks' failed to satisfy matcher.
           Expected
               <*bool | 0x0>: nil
           to equal
               <*bool | 0xc0007deab0>: false

// Set automountServiceAccountToken to false as podman doesn't use service account tokens
automountServiceAccountToken := false
ps := v1.PodSpec{
Containers: containers,
Hostname: hostname,
HostNetwork: hostNetwork,
InitContainers: initCtrs,
Volumes: volumes,
EnableServiceLinks: &enableServiceLinks,
AutomountServiceAccountToken: &automountServiceAccountToken,
Containers: containers,
Hostname: hostname,
HostNetwork: hostNetwork,
InitContainers: initCtrs,
Volumes: volumes,
}
if !hostUsers {
ps.HostUsers = &hostUsers
Expand Down Expand Up @@ -894,35 +888,46 @@ func generateKubeVolumeMount(m specs.Mount) (v1.VolumeMount, v1.Volume, error) {
vm := v1.VolumeMount{}
vo := v1.Volume{}

name, err := convertVolumePathToName(m.Source)
if err != nil {
return vm, vo, err
var (
name string
err error
)
if m.Type == define.TypeTmpfs {
name = "tmp"
vo.EmptyDir = &v1.EmptyDirVolumeSource{
Medium: v1.StorageMediumMemory,
}
vo.Name = name
} else {
name, err = convertVolumePathToName(m.Source)
if err != nil {
return vm, vo, err
}
// To avoid naming conflicts with any persistent volume mounts, add a unique suffix to the volume's name.
name += "-host"
vo.Name = name
vo.HostPath = &v1.HostPathVolumeSource{}
vo.HostPath.Path = m.Source
isDir, err := isHostPathDirectory(m.Source)
// neither a directory or a file lives here, default to creating a directory
// TODO should this be an error instead?
var hostPathType v1.HostPathType
switch {
case err != nil:
hostPathType = v1.HostPathDirectoryOrCreate
case isDir:
hostPathType = v1.HostPathDirectory
default:
hostPathType = v1.HostPathFile
}
vo.HostPath.Type = &hostPathType
}
// To avoid naming conflicts with any persistent volume mounts, add a unique suffix to the volume's name.
name += "-host"
vm.Name = name
vm.MountPath = m.Destination
if cutil.StringInSlice("ro", m.Options) {
vm.ReadOnly = true
}

vo.Name = name
vo.HostPath = &v1.HostPathVolumeSource{}
vo.HostPath.Path = m.Source
isDir, err := isHostPathDirectory(m.Source)
// neither a directory or a file lives here, default to creating a directory
// TODO should this be an error instead?
var hostPathType v1.HostPathType
switch {
case err != nil:
hostPathType = v1.HostPathDirectoryOrCreate
case isDir:
hostPathType = v1.HostPathDirectory
default:
hostPathType = v1.HostPathFile
}
vo.HostPath.Type = &hostPathType

return vm, vo, nil
}

Expand Down
7 changes: 7 additions & 0 deletions pkg/specgen/generate/kube/kube.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"github.com/containers/common/pkg/secrets"
cutil "github.com/containers/common/pkg/util"
"github.com/containers/image/v5/manifest"
itypes "github.com/containers/image/v5/types"
"github.com/containers/podman/v4/libpod/define"
ann "github.com/containers/podman/v4/pkg/annotations"
"github.com/containers/podman/v4/pkg/domain/entities"
Expand Down Expand Up @@ -119,6 +120,8 @@ type CtrSpecGenOptions struct {
ConfigMaps []v1.ConfigMap
// SeccompPaths for finding the seccomp profile path
SeccompPaths *KubeSeccompPaths
// ReadOnly make all containers root file system readonly
ReadOnly itypes.OptionalBool
// RestartPolicy defines the restart policy of the container
RestartPolicy string
// NetNSIsHost tells the container to use the host netns
Expand Down Expand Up @@ -444,6 +447,10 @@ func ToSpecGen(ctx context.Context, opts *CtrSpecGenOptions) (*specgen.SpecGener
}
}

if ro := opts.ReadOnly; ro != itypes.OptionalBoolUndefined {
s.ReadOnlyFilesystem = (ro == itypes.OptionalBoolTrue)
}

// Make sure the container runs in a systemd unit which is
// stored as a label at container creation.
if unit := os.Getenv(systemdDefine.EnvVariable); unit != "" {
Expand Down
6 changes: 5 additions & 1 deletion pkg/specgenutil/specgen.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,7 +676,7 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *entities.ContainerCreateOptions

// Only add read-only tmpfs mounts in case that we are read-only and the
// read-only tmpfs flag has been set.
mounts, volumes, overlayVolumes, imageVolumes, err := parseVolumes(c.Volume, c.Mount, c.TmpFS, c.ReadOnlyTmpFS && c.ReadOnly)
mounts, volumes, overlayVolumes, imageVolumes, err := parseVolumes(c.Volume, c.Mount, c.TmpFS)
if err != nil {
return err
}
Expand Down Expand Up @@ -854,6 +854,10 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *entities.ContainerCreateOptions
s.PasswdEntry = c.PasswdEntry
}

if c.ReadOnly && c.ReadOnlyTmpFS {
s.Mounts = addReadOnlyMounts(s.Mounts)
}

return nil
}

Expand Down
45 changes: 24 additions & 21 deletions pkg/specgenutil/volumes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ var (
// Does not handle image volumes, init, and --volumes-from flags.
// Can also add tmpfs mounts from read-only tmpfs.
// TODO: handle options parsing/processing via containers/storage/pkg/mount
func parseVolumes(volumeFlag, mountFlag, tmpfsFlag []string, addReadOnlyTmpfs bool) ([]spec.Mount, []*specgen.NamedVolume, []*specgen.OverlayVolume, []*specgen.ImageVolume, error) {
func parseVolumes(volumeFlag, mountFlag, tmpfsFlag []string) ([]spec.Mount, []*specgen.NamedVolume, []*specgen.OverlayVolume, []*specgen.ImageVolume, error) {
// Get mounts from the --mounts flag.
unifiedMounts, unifiedVolumes, unifiedImageVolumes, err := Mounts(mountFlag)
if err != nil {
Expand Down Expand Up @@ -78,26 +78,6 @@ func parseVolumes(volumeFlag, mountFlag, tmpfsFlag []string, addReadOnlyTmpfs bo
unifiedMounts[dest] = tmpfs
}

// If requested, add tmpfs filesystems for read-only containers.
if addReadOnlyTmpfs {
readonlyTmpfs := []string{"/tmp", "/var/tmp", "/run"}
options := []string{"rw", "rprivate", "nosuid", "nodev", "tmpcopyup"}
for _, dest := range readonlyTmpfs {
if _, ok := unifiedMounts[dest]; ok {
continue
}
if _, ok := unifiedVolumes[dest]; ok {
continue
}
unifiedMounts[dest] = spec.Mount{
Destination: dest,
Type: define.TypeTmpfs,
Source: "tmpfs",
Options: options,
}
}
}

// Check for conflicts between named volumes, overlay & image volumes,
// and mounts
allMounts := make(map[string]bool)
Expand Down Expand Up @@ -723,3 +703,26 @@ func validChownFlag(flag string) (bool, error) {
func unixPathClean(p string) string {
return path.Clean(p)
}

func addReadOnlyMounts(mounts []spec.Mount) []spec.Mount {
readonlyTmpfs := []string{"/tmp", "/var/tmp", "/run"}
options := []string{"rw", "rprivate", "nosuid", "nodev", "tmpcopyup"}
for _, dest := range readonlyTmpfs {
for _, m := range mounts {
if m.Destination == dest {
continue
}
}
mnt := spec.Mount{
Destination: dest,
Type: define.TypeTmpfs,
Source: define.TypeTmpfs,
Options: options,
}
if dest != "/run" {
mnt.Options = append(mnt.Options, "noexec")
}
mounts = append(mounts, mnt)
}
return mounts
}
8 changes: 0 additions & 8 deletions test/e2e/generate_kube_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,6 @@ var _ = Describe("Podman generate kube", func() {
Expect(pod.Spec.Containers[0]).To(HaveField("WorkingDir", ""))
Expect(pod.Spec.Containers[0].Env).To(BeNil())
Expect(pod).To(HaveField("Name", "top-pod"))
enableServiceLinks := false
Expect(pod.Spec).To(HaveField("EnableServiceLinks", &enableServiceLinks))
automountServiceAccountToken := false
Expect(pod.Spec).To(HaveField("AutomountServiceAccountToken", &automountServiceAccountToken))

numContainers := 0
for range pod.Spec.Containers {
Expand Down Expand Up @@ -169,10 +165,6 @@ var _ = Describe("Podman generate kube", func() {
err := yaml.Unmarshal(kube.Out.Contents(), pod)
Expect(err).ToNot(HaveOccurred())
Expect(pod.Spec).To(HaveField("HostNetwork", false))
enableServiceLinks := false
Expect(pod.Spec).To(HaveField("EnableServiceLinks", &enableServiceLinks))
automountServiceAccountToken := false
Expect(pod.Spec).To(HaveField("AutomountServiceAccountToken", &automountServiceAccountToken))

numContainers := 0
for range pod.Spec.Containers {
Expand Down
27 changes: 27 additions & 0 deletions test/system/700-play.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,30 @@ EOF
run_podman rm -a
}

@test "podman kube play read-only" {
YAML=$PODMAN_TMPDIR/test.yml
run_podman create --pod new:pod1 --name test1 $IMAGE touch /testrw
run_podman create --pod pod1 --read-only --name test2 $IMAGE touch /testro
run_podman create --pod pod1 --read-only --name test3 $IMAGE touch /tmp/testtmp
run_podman kube generate pod1 -f $YAML
Copy link
Member

Choose a reason for hiding this comment

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

Can we check the output to verify that read-only field is properly set?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.


run_podman kube play --replace $YAML
run_podman container inspect --format '{{.HostConfig.ReadonlyRootfs}}' pod1-test1 pod1-test2 pod1-test3
is "$output" "false.*true.*true" "Rootfs should be read/only"

run_podman inspect --format "{{.State.ExitCode}}" pod1-test1
Copy link
Member

Choose a reason for hiding this comment

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

Can we also check for the read-only fields to be set for the two containers?

is "$output" "0" "Container / should be read/write"
run_podman inspect --format "{{.State.ExitCode}}" pod1-test2
is "$output" "1" "Container / should be read/only"
run_podman inspect --format "{{.State.ExitCode}}" pod1-test3
is "$output" "0" "/tmp in a read-only container should be read/write"

run_podman kube down - < $YAML
run_podman 1 container exists pod1-test1
run_podman 1 container exists pod1-test2
run_podman 1 container exists pod1-test3
}

@test "podman play with user from image" {
TESTDIR=$PODMAN_TMPDIR/testdir
mkdir -p $TESTDIR
Expand Down Expand Up @@ -416,4 +440,7 @@ spec:
run_podman pod inspect test_pod --format "{{.InfraConfig.PortBindings}}"
assert "$output" = "map[$HOST_PORT/tcp:[{ $HOST_PORT}]]"
run_podman kube down $PODMAN_TMPDIR/testpod.yaml

run_podman pod rm -a -f
run_podman rm -a -f
}