generate systemd: support entrypoint JSON strings #12545

vrothberg · 2021-12-08T13:37:56Z

Make sure to preserve the quoting of entrypoint JSON strings.

Fixes: #12477
Signed-off-by: Valentin Rothberg [email protected]

edsantiago · 2021-12-08T13:42:48Z

Any idea what this flake is about? I've seen it three times this week:

VERSION=1.36.0 GOBIN=/var/tmp/go/bin ./hack/install_golangci.sh
Installing golangci-lint v1.36.0 into ./bin/golangci-lint
golangci/golangci-lint info checking GitHub for tag 'v1.36.0'
golangci/golangci-lint info found version: 1.36.0 for v1.36.0/linux/amd64
make: *** [Makefile:846: .install.golangci-lint] Error 1

Unfortunately it's not one that my flake-logger catches.

vrothberg · 2021-12-08T13:46:31Z

No idea, probably need to make .install.golangci-lint more verbose?

edsantiago · 2021-12-08T14:13:26Z

Error is:

swagger generate spec -o swagger.yaml -i tags.yaml -w ./ -m
/usr/local/bin/swagger: line 1: syntax error near unexpected token `<'
/usr/local/bin/swagger: line 1: `<?xml version="1.0" encoding="utf-8"?><Error><Code>ServerBusy</Code><Message>Egress is over the account limit.'
make[1]: *** [Makefile:15: swagger.yaml] Error 2
make[1]: Leaving directory '/var/tmp/go/src/github.com/containers/podman/pkg/api'
make: *** [Makefile:446: pkg/api/swagger.yaml] Error 2

I don't know what swagger or Egress are, so am not restarting.

edsantiago · 2021-12-08T14:15:44Z

pkg/systemd/generate/common.go

@@ -86,6 +86,32 @@ func filterCommonContainerFlags(command []string, argCount int) []string {
 	return processed
 }

+func escapeJSONString(s string) string {
+	if strings.HasPrefix(s, "[") && strings.HasSuffix(s, "]") {
+		return "'" + s + "'"


my first question here is, what if s includes singlequotes? But I can't figure out how to trigger this code, so I can't write a test case, and I need to move on to other stuff. Anyhow, maybe you could add that to your unit tests? Something like:

... --entrypoint "[\"sh\", \"-c\", \"date '+%s'\"]"

Luap99

This doesn't look like the correct fix to me. IMO the problem is that the quotes are not escaped what needs to be done is add " to the match in escapeSystemdArg() -> if strings.ContainsAny(arg, " \t\"") { with that fix it should now correctly escape the qutoes.

vrothberg · 2021-12-08T14:25:28Z

This doesn't look like the correct fix to me. IMO the problem is that the quotes are not escaped what needs to be done is add " to the match in escapeSystemdArg() -> if strings.ContainsAny(arg, " \t"") { with that fix it should now correctly escape the qutoes.

Did you actually test it or do you guess? It's already escaped but the surrounding single quotes are the crux of the matter as far as I could see and test.

Luap99 · 2021-12-08T14:33:51Z

$ podman create --name test --entrypoint '["top"]' alpine
2d89ca9de2e1d1962c23e89f1065aedc5b760dd2db1b0f74ef0087bc70f5205d
pholzing ~ ✓ $ podman generate systemd --new test 
# container-2d89ca9de2e1d1962c23e89f1065aedc5b760dd2db1b0f74ef0087bc70f5205d.service
# autogenerated by Podman 3.4.2
# Wed Dec  8 15:31:09 CET 2021

[Unit]
Description=Podman container-2d89ca9de2e1d1962c23e89f1065aedc5b760dd2db1b0f74ef0087bc70f5205d.service
Documentation=man:podman-generate-systemd(1)
Wants=network-online.target
After=network-online.target
RequiresMountsFor=%t/containers

[Service]
Environment=PODMAN_SYSTEMD_UNIT=%n
Restart=on-failure
TimeoutStopSec=70
ExecStartPre=/bin/rm -f %t/%n.ctr-id
ExecStart=/usr/bin/podman run --cidfile=%t/%n.ctr-id --cgroups=no-conmon --rm --sdnotify=conmon -d --replace --name test --entrypoint ["top"] alpine
ExecStop=/usr/bin/podman stop --ignore --cidfile=%t/%n.ctr-id
ExecStopPost=/usr/bin/podman rm -f --ignore --cidfile=%t/%n.ctr-id
Type=notify
NotifyAccess=all

[Install]
WantedBy=multi-user.target default.target

It not escaped at the moment.

With my proposed change:

...
ExecStart=/home/pholzing/go/src/github.com/containers/podman/bin/podman run --cidfile=%t/%n.ctr-id --cgroups=no-conmon --rm --sdnotify=conmon -d --replace --name test --entrypoint "[\"top\"]" alpine
...

$ git diff
diff --git a/pkg/systemd/generate/common.go b/pkg/systemd/generate/common.go
index 24c85a27e..8689e084c 100644
--- a/pkg/systemd/generate/common.go
+++ b/pkg/systemd/generate/common.go
@@ -101,7 +101,7 @@ func escapeSystemdArguments(command []string) []string {
 func escapeSystemdArg(arg string) string {
        arg = strings.ReplaceAll(arg, "$", "$$")
        arg = strings.ReplaceAll(arg, "%", "%%")
-       if strings.ContainsAny(arg, " \t") {
+       if strings.ContainsAny(arg, " \t\"") {
                arg = strconv.Quote(arg)
        } else if strings.Contains(arg, `\`) {
                // strconv.Quote also escapes backslashes so

mheon · 2021-12-08T14:34:22Z

/usr/local/bin/swagger: line 1: `<?xml version="1.0" encoding="utf-8"?><Error><Code>ServerBusy</Code><Message>Egress is over the account limit.'

@cevich PTAL

vrothberg · 2021-12-08T14:40:20Z

Thanks, @Luap99. I'll take a look.

Make sure to preserve the quoting of entrypoint JSON strings. Fixes: containers#12477 Signed-off-by: Valentin Rothberg <[email protected]>

vrothberg · 2021-12-08T14:53:00Z

@edsantiago @Luap99 PTanotherL

cevich · 2021-12-08T15:34:54Z

ServerBusyEgress is over the account limit.

I don't think this is us or our stuff (directly). This is coming from contrib/cirrus/runner.sh line 169 where we download the swagger 'binary'. Though, given the number of PRs we run, it's conceivable we're not helping the situation by downloading the exact same binary every time this task runs 😖

Let me move this download into the VM Image build process, that will help their egress limit as well as reduce the number of ways our CI can flake...

Luap99

LGTM

cevich · 2021-12-08T15:43:26Z

...opened containers/automation_images#103 I'll work it through the process into a separate PR here.

edsantiago · 2021-12-08T15:58:04Z

Oh, I get it now. My earlier tests were useless because the existing strings.ContainsAny(arg, " \t") was already triggering strconv.Quote().

I'm not convinced that this PR totally fixes everything. Could I suggest:

-       if strings.ContainsAny(arg, " \t\"") {
+       if strings.ContainsAny(arg, " \t\"'[]") {

This will, I think, properly handle:

$ ... --entrypoint "['top']"

But I might be wrong. I don't understand the use cases for this.

Luap99 · 2021-12-08T16:00:26Z

['top'] is not valid json

edsantiago · 2021-12-08T18:54:32Z

Then I don't know enough to approve this PR. What I can do is confirm that this PR behaves differently from podman-3.4.2-1.fc35, and in a way that (to my eye) looks improved:

$ bin/podman create --name foo --entrypoint '["top"]' quay.io/libpod/testimage:20210610
838b457d88e3f5d9075ec41eb6fdb92e4f332f6ba3451d513a384608b23b787b
$ bin/podman generate systemd foo --new | grep -i entr
.... --entrypoint "[\"top\"]"     <--- with /usr/bin/podman, yields: --entrypoint ["top"], no escapes

TomSweeneyRedHat · 2021-12-08T22:43:21Z

LGTM
but I'd like another set of eyeballs on this before the merge.

giuseppe

/lgtm

openshift-ci · 2021-12-09T08:39:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [giuseppe,vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 8, 2021

vrothberg force-pushed the fix-12477 branch from 9e47126 to 588854e Compare December 8, 2021 13:38

vrothberg mentioned this pull request Dec 8, 2021

Array-like --entrypoint [ "cmd" ] syntax not supported in podman run (in systemd file), but supported in podman create (via podman-compose)? #12477

Closed

edsantiago reviewed Dec 8, 2021

View reviewed changes

Luap99 reviewed Dec 8, 2021

View reviewed changes

generate systemd: support entrypoint JSON strings

2d517b6

Make sure to preserve the quoting of entrypoint JSON strings. Fixes: containers#12477 Signed-off-by: Valentin Rothberg <[email protected]>

vrothberg force-pushed the fix-12477 branch from 588854e to 2d517b6 Compare December 8, 2021 14:52

Luap99 reviewed Dec 8, 2021

View reviewed changes

cevich mentioned this pull request Dec 8, 2021

[CI:DOCS] Cirrus: Use cached swagger binary #12547

Merged

giuseppe approved these changes Dec 9, 2021

View reviewed changes

openshift-ci bot assigned giuseppe Dec 9, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 9, 2021

openshift-merge-robot merged commit 4a52a45 into containers:main Dec 9, 2021

Luap99 mentioned this pull request Dec 31, 2021

podman generate systemd doesn't escape the value for --entrypoint #12723

Closed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

generate systemd: support entrypoint JSON strings #12545

generate systemd: support entrypoint JSON strings #12545

vrothberg commented Dec 8, 2021 •

edited

Loading

edsantiago commented Dec 8, 2021

vrothberg commented Dec 8, 2021

edsantiago commented Dec 8, 2021

edsantiago Dec 8, 2021

Luap99 left a comment

vrothberg commented Dec 8, 2021

Luap99 commented Dec 8, 2021

mheon commented Dec 8, 2021

vrothberg commented Dec 8, 2021

vrothberg commented Dec 8, 2021

cevich commented Dec 8, 2021

Luap99 left a comment

cevich commented Dec 8, 2021

edsantiago commented Dec 8, 2021

Luap99 commented Dec 8, 2021

edsantiago commented Dec 8, 2021

TomSweeneyRedHat commented Dec 8, 2021

giuseppe left a comment

openshift-ci bot commented Dec 9, 2021

generate systemd: support entrypoint JSON strings #12545

generate systemd: support entrypoint JSON strings #12545

Conversation

vrothberg commented Dec 8, 2021 • edited Loading

edsantiago commented Dec 8, 2021

vrothberg commented Dec 8, 2021

edsantiago commented Dec 8, 2021

edsantiago Dec 8, 2021

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

vrothberg commented Dec 8, 2021

Luap99 commented Dec 8, 2021

mheon commented Dec 8, 2021

vrothberg commented Dec 8, 2021

vrothberg commented Dec 8, 2021

cevich commented Dec 8, 2021

Luap99 left a comment

Choose a reason for hiding this comment

cevich commented Dec 8, 2021

edsantiago commented Dec 8, 2021

Luap99 commented Dec 8, 2021

edsantiago commented Dec 8, 2021

TomSweeneyRedHat commented Dec 8, 2021

giuseppe left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Dec 9, 2021

vrothberg commented Dec 8, 2021 •

edited

Loading