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

Add read only #65

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Add read only #65

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1e05b69
add host read-only option
dustinblack May 7, 2024
cac6b0b
WIP add read-only root parameter
dustinblack May 7, 2024
9a2e696
correct required and default
dustinblack May 8, 2024
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
3 changes: 2 additions & 1 deletion connector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ func (c *Connector) Deploy(ctx context.Context, image string) (deployer.Plugin,
SetVolumes(hostConfig.Binds).
SetCgroupNs(string(hostConfig.CgroupnsMode)).
SetNetworkMode(string(hostConfig.NetworkMode)).
SetPrivileged(hostConfig.Privileged)
SetPrivileged(hostConfig.Privileged).
SetReadOnlyRoot(&hostConfig.ReadonlyRootfs)
webbnh marked this conversation as resolved.
Show resolved Hide resolved

stdin, stdout, err := c.podmanCliWrapper.Deploy(image, commandArgs, []string{"--atp"})

Expand Down
8 changes: 8 additions & 0 deletions internal/argsbuilder/argsbuilder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package argsbuilder

import (
"strconv"
"strings"
)

Expand Down Expand Up @@ -53,3 +54,10 @@ func (a *argsBuilder) SetPrivileged(privileged bool) ArgsBuilder {
}
return a
}

func (a *argsBuilder) SetReadOnlyRoot(readOnlyRootfs *bool) ArgsBuilder {
if readOnlyRootfs != nil {
*a.commandArgs = append(*a.commandArgs, "--read-only="+strconv.FormatBool(*readOnlyRootfs))
}
return a
}
1 change: 1 addition & 0 deletions internal/argsbuilder/argsbuilder_interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ type ArgsBuilder interface {
SetContainerName(name string) ArgsBuilder
SetNetworkMode(networkMode string) ArgsBuilder
SetPrivileged(privileged bool) ArgsBuilder
SetReadOnlyRoot(readOnlyRootfs *bool) ArgsBuilder
Comment on lines 7 to +10
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any other options for which we should be implementing tri-state logic? (Like all of them? 😇)

(Also, the handling of -e looks...incomplete -- it seems like it silently ignores cases with no = as well as cases where the value of the environment variable contains a =....)

}

func NewBuilder(commandArgs *[]string) ArgsBuilder {
Expand Down
10 changes: 10 additions & 0 deletions schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,16 @@ var Schema = schema.NewTypedScopeSchema[*Config](
schema.PointerTo(util.JSONEncode(false)),
nil,
),
"ReadonlyRootfs": schema.NewPropertySchema(
schema.NewBoolSchema(),
schema.NewDisplayValue(schema.PointerTo("ReadonlyRootfs"), schema.PointerTo("Execute container process with or without a read only root file system"), nil),
false,
Copy link

Choose a reason for hiding this comment

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

Ah, yes. Good. We should have unit tests proving that the deployer module accepts true/false/missing ... I don't think the deployer tests have done all that much to prove that options passed in actually do what's intended (e.g., selinux labeling), but proving that we can/can't write to the root filesystem would be great.

Copy link
Contributor

Choose a reason for hiding this comment

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

Proving that we can/can't write to the root filesystem sounds like a functional test...but with a suitable mock, we could have unit tests for this stuff which would be great!

Comment on lines +345 to +346
Copy link
Contributor

Choose a reason for hiding this comment

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

At least as far as the Podman man page is concerned, "read-only" is hyphenated.

nil,
nil,
nil,
nil,
nil,
),
},
),
schema.NewStructMappedObjectSchema[*nat.PortBinding](
Expand Down
Loading