Add TERM iff TERM not defined in container when podman exec -t #20357

rhatdan · 2023-10-13T17:57:02Z

Fixes: #20334

Does this PR introduce a user-facing change?

Podman exec -t will define TERM environment variable even if container is not running with a terminal.

rhatdan · 2023-10-13T17:57:49Z

@edsantiago Not sure what is going on, but the out put of env from inside of container seems to have a ^M associated with it, not sure how to remove it nicely.

vrothberg · 2023-10-16T07:28:49Z

libpod/container.go

@@ -713,6 +713,13 @@ func (c *Container) LinuxResources() *spec.LinuxResources {
 	return nil
 }

+func (c *Container) Env() []string {


Please add a doc comment.

vrothberg · 2023-10-16T07:29:21Z

pkg/util/utils.go

@@ -1219,3 +1219,28 @@ func ConvertTimeout(timeout int) uint {
 	}
 	return uint(timeout)
 }
+
+func ExecAddTERM(useTTY bool, existingEnv []string, execEnvs map[string]string) {


Since there's only one caller and no uni tests, can we move it directly to pkg/domain/infra/abi/containers.go?

There are two callers, one from api/handlers an one from domain/infra/abi

vrothberg · 2023-10-16T07:31:17Z

test/system/075-exec.bats

+
+@test "podman exec --tty" {
+    run_podman run -d --name test $IMAGE top
+    run_podman 1 exec test printenv echo $TERM


The printenv echo $TERM confuses me. Do you want to echo or printenv?

Not only that, the echo $TERM (without single quotes) will interpret $TERM in the context of the running shell (BATS), not the container. This should be printenv TERM, as it is below.

Yes this was a leak, attempting to get this, should all be printenv TERM.

vrothberg · 2023-10-16T07:32:11Z

test/system/075-exec.bats

+    fi
+    run_podman rm -f -t 0 test
+
+    run_podman run -t -d --name test $IMAGE top


This entire block looks a duplicate of the previous one.

First one is a container run without a -t
Second one is a container run with -t
Third is container run with -t --env TERM=banana

Way too many confusing combinations. Might I make a suggestion?

@test "podman exec --tty" { # Outer loops: different variations on the RUN container for run_opt_t in "" "-t"; do for run_term_env in "" "explicit_RUN_term"; do local run_opt_env= if [[ -n "$run_term_env" ]]; then run_opt_env="--env=TERM=$run_term_env" fi run_podman run -d $run_opt_t $run_opt_env --name test $IMAGE top # Inner loops: different variations on EXEC for exec_opt_t in "" "-t"; do for exec_term_env in "" "explicit_EXEC_term"; do # What to expect. local expected= # "exec -t" defaults to current $TERM if [[ -n "$exec_opt_t" ]]; then expected="$TERM" fi # ...unless "run -t", which forces xterm (always xterm) if [[ -n "$run_opt_t" ]]; then expected="xterm" fi # ...unless overridden by explicit --env if [[ -n "$run_term_env$exec_term_env" ]]; then # (exec overrides run) expected="${exec_term_env:-$run_term_env}" fi local exec_opt_env= if [[ -n "$exec_term_env" ]]; then exec_opt_env="--env=TERM=$exec_term_env" fi run_podman exec $exec_opt_t $exec_opt_env test sh -c 'echo -n $TERM' assert "$output" = "$expected" "run $run_opt_t $run_opt_env, exec $exec_opt_t $exec_opt_env" done done run_podman rm -f -t0 test done done

This very clearly demonstrates what I consider a confusing aspect of run -t with exec -t. If it's deliberate, fine, but to me it violates POLA that exec -t depends on whether the container was run with/without -t.

This is fine, it does loose the test to make sure TERM is not set in the non run -t and non exec -t case, since theoretically TERM="" versus TERM not set. But really not important.

That would be fairly easy to fix:

local expect_rc=0 if [[ -z "$expect_output" ]]; then expect_rc=1 fi ... run_podman $expect_rc exec [...] printenv TERM

...but that then requires defining cr=$'\r' and requiring it in the -t cases. Which I think might be fine, but it makes the test more complicated, and the loop is already too unwieldy. LMK if you'd like the full \r set of changes.

Luap99 · 2023-10-16T09:13:09Z

pkg/api/handlers/compat/exec.go

@@ -58,6 +59,7 @@ func ExecCreateHandler(w http.ResponseWriter, r *http.Request) {
 	libpodConfig.WorkDir = input.WorkingDir
 	libpodConfig.Privileged = input.Privileged
 	libpodConfig.User = input.User
+	util.ExecAddTERM(input.Tty && !ctr.Terminal(), ctr.Env(), libpodConfig.Environment)


I don't see why this should ever depend on ctr.Terminal()? Either exec is started with tty or not, I don't see why the container option matters in this case.

I guess you are right, originally I was checking that to look for TERM, but now, I just can use TERM if it is defined within the container and only force it if it is not.

edsantiago

^M is the result of -t. For a tighter test, I recommend:

    cr=$'\r'
    ...
    assert "$output" = "whatever$cr" "..."

edsantiago · 2023-10-16T12:01:59Z

test/system/075-exec.bats

+
+@test "podman exec --tty" {
+    run_podman run -d --name test $IMAGE top
+    run_podman 1 exec test printenv echo $TERM


Not only that, the echo $TERM (without single quotes) will interpret $TERM in the context of the running shell (BATS), not the container. This should be printenv TERM, as it is below.

rhatdan · 2023-10-16T21:19:27Z

@edsantiago it is blowing up with an unexpected terminal name dumb?

edsantiago · 2023-10-16T21:29:59Z

Argh! I can reproduce via env -u TERM hack/bats --rootless 075:tty </dev/null. (I had only tested via TERM= hack/bats, which is obviously not the same). Now looking for a fix.

edsantiago · 2023-10-16T21:49:08Z

This fixes the tests..... but only for local podman. For remote, podman run (no opts) with exec -t gets the podman-system-service $TERM, not the invoking client's. My gut tells me that's wrong, but it's too late in my day right now for me to fully think it through.

diff --git a/test/system/075-exec.bats b/test/system/075-exec.bats
index ec7d59536..324c9bbfe 100644
--- a/test/system/075-exec.bats
+++ b/test/system/075-exec.bats
@@ -177,3 +177,3 @@ load helpers
             fi
-            run_podman run -d $run_opt_t $run_opt_env --name test $IMAGE top
+            TERM=run-term run_podman run -d $run_opt_t $run_opt_env --name test $IMAGE top
 
@@ -184,5 +184,5 @@ load helpers
                     local expected=
-                    # "exec -t" defaults to current $TERM
+                    # "exec -t" defaults to the process TERM
                     if [[ -n "$exec_opt_t" ]]; then
-                        expected="$TERM"
+                        expected="exec-term"
                     fi
@@ -203,3 +203,3 @@ load helpers
 
-                    run_podman exec $exec_opt_t $exec_opt_env test sh -c 'echo -n $TERM'
+                    TERM=exec-term run_podman exec $exec_opt_t $exec_opt_env test sh -c 'echo -n $TERM'
                     assert "$output" = "$expected" "run $run_opt_t $run_opt_env, exec $exec_opt_t $exec_opt_env"

Note that this does not test the env -u TERM run_podman case, because we can't do that. Can't do env with run_podman. This keep getting trickier. I'm sorry to have introduced so much hassle, but am slightly glad that we're giving attention to the corner cases.

edsantiago · 2023-10-17T12:16:35Z

@rhatdan this is going to fail. I guess I wasn't clear in my comments, sorry.

The podman-remote case uses $TERM on the server. I think this is wrong:

$ TERM=sdfsdf bin/podman system service --timeout=0

$ bin/podman-remote run -d --name foo quay.io/libpod/testimage:20221018 top
<sha>
$ bin/podman-remote exec -t foo printenv TERM
sdfsdf

Note that podman-remote run -t shows xterm, not sdf.

Anyhow, tests will fail until this is resolved. Either skip part of the tests under remote, or fix the underlying code, and I would prefer to fix the underlying code. I think podman-remote exec should use client TERM, not server. But I may be wrong.

rhatdan · 2023-10-17T19:51:02Z

@edsantiago I changed the podman run -t behavior to do a Getenv("TERM") before failing over to "xterm"
Now it should work on --remote.

edsantiago · 2023-10-17T20:00:49Z

I'm not sure this is better? It still fails remote (no change), and now I think the exec behavior is worse: exec -t, on a container with run -t, now gets the run TERM, not the exec one. I think that's a step in the opposite direction than we want?

Anyhow, this is going to fail all system tests, not just remote.

edsantiago · 2023-10-17T20:09:32Z

My bad. This is actually a very small net gain (run -t now honors TERM) but I still think that exec -t should win. This patch makes it work under podman-local:

diff --git a/test/system/075-exec.bats b/test/system/075-exec.bats
index 324c9bbfe..14f965063 100644
--- a/test/system/075-exec.bats
+++ b/test/system/075-exec.bats
@@ -188,5 +188,5 @@ load helpers
                     fi
-                    # ...unless "run -t", which forces xterm (always xterm)
+                    # ...unless "run -t", which forces the run-process TERM
                     if [[ -n "$run_opt_t" ]]; then
-                        expected="xterm"
+                        expected="run-term"
                     fi

...but remote is still 100% broken and I still lean toward thinking that it should be fixed in the code, not the tests.

What I can do, though, is kludge up the tests so they pass under remote, then sweep the podman-remote-exec problem under the rug for another bug week. Should I do that?

edsantiago · 2023-10-17T20:18:36Z

copy-paste this, it will work under local and remote. I've made the warning messages very obnoxious as an incentive to getting that bug fixed (or at least getting the issue resolved one way or another).

@test "podman exec --tty" {
    # Outer loops: different variations on the RUN container
    for run_opt_t in "" "-t"; do
        for run_term_env in "" "explicit_RUN_term"; do
            local run_opt_env=
            if [[ -n "$run_term_env" ]]; then
                run_opt_env="--env=TERM=$run_term_env"
            fi
            TERM=run-term run_podman run -d $run_opt_t $run_opt_env --name test $IMAGE top

            # Inner loops: different variations on EXEC
            for exec_opt_t in "" "-t"; do
                for exec_term_env in "" "explicit_EXEC_term"; do
                    # What to expect.
                    local expected=
                    # "exec -t" defaults to the process TERM
                    if [[ -n "$exec_opt_t" ]]; then
                        expected="exec-term"
                    fi
                    # ...unless "run -t", which forces the run-process TERM
                    if [[ -n "$run_opt_t" ]]; then
                        expected="run-term"
                    fi
                    # ...unless overridden by explicit --env
                    if [[ -n "$run_term_env$exec_term_env" ]]; then
                        # (exec overrides run)
                        expected="${exec_term_env:-$run_term_env}"
                    fi

                    local exec_opt_env=
                    if [[ -n "$exec_term_env" ]]; then
                        exec_opt_env="--env=TERM=$exec_term_env"
                    fi

                    local desc="run $run_opt_t $run_opt_env, exec $exec_opt_t $exec_opt_env"
                    TERM=exec-term run_podman exec $exec_opt_t $exec_opt_env test sh -c 'echo -n $TERM'
                    # FIXME FIXME FIXME! #xxxxx: podman-remote run/exec -t use server TERM, not client
                    if is_remote && [[ -n "$run_opt_t$exec_opt_t" ]] && [[ -z "$run_term_env$exec_term_env" ]]; then
                        echo "# [[ FIXME: podman-remote -t uses server TERM, not client : $desc ]]" >&3
                    else
                        # The normal case
                        assert "$output" = "$expected" "$desc"
                    fi
                done
            done

            run_podman rm -f -t0 test
        done
    done
}

rhatdan · 2023-10-18T14:27:27Z

Switching to TERM on the client side is a potential breaking change, but might make the most sense.

Up til now, exec used TERM from the Container, the change that triggered this PR was that podman run --tty=false, no longer sets a TERM environment variable. This was causing podman exec --tty=true to said terminals to not work correctly without a TERM set.

This PR still takes the TERM from the container if it is set, and then takes if from the client, when it is not set.

Changing to always take TERM from the client for podman exec, might make the most sense, although in the server case, you could have a TERM on the client side that is not understood on the server side. Perhaps a TERM from a Mac or Windows is not understood in linux. (The current code is susceptible to the same issue).

@vrothberg @Luap99 @edsantiago @baude Thoughts?

vrothberg

I am OK with the change
LGTM

Luap99 · 2023-10-18T14:47:23Z

Why are we looking up the TERM from the host environment exactly? This seems to be a new change made by this PR so I don't see how this effects any kind of backwards compatibility. More the other way around using the TERM env from the environment might be breaking (although I don't see how but I don't really understand how $TERM is used by applications so I wouldn't really comment on that)

I don't see why we should make such change, nobody seems to have asked for that. The old logic is use --env TERM when set otherwise default to xterm. I see no reason to switch the behavior right now, the bug is about exec -t not setting TERM so I don't see why we should just do the same thing as done by podman run right now.

Luap99 · 2023-10-18T14:48:52Z

Also worth pointing out before committing to such changes we should verify what docker does. If docker doesn't read from the environment neither should we IMO.

edsantiago · 2023-10-18T14:50:59Z

Why are we looking up the TERM from the host environment exactly?

My fault: I got sick of all the churn on this, and added tests, and my new test found what I consider to be weird behavior. But yeah, I guess we should figure out what docker does and copy it.

rhatdan · 2023-10-18T15:42:59Z

Basically docker sets the TERM=xterm if it not defined in the container image on docker run.
Docker sets the TERM=xterm if TERM is not defined in the container on docker exec.

edsantiago · 2023-10-18T15:44:16Z

Something changed in defaultenv, xterm vs xterm-256color.

And, I'm ultra-concerned that we have a brand-new test, exec leak something added in #20394, that is failing hard (not flaking) on podman-remote.

edsantiago · 2023-10-18T15:44:52Z

exec something leak, @giuseppe PTAL

edsantiago

You have to repush (once #20404 merges) to fix CI anyway, so, some fix requests

edsantiago · 2023-10-18T18:16:57Z

test/system/075-exec.bats

+                for exec_term_env in "" "explicit_EXEC_term"; do
+                    # What to expect.
+                    local expected=
+                    # "exec -t" defaults to the process xterm


Just "xterm", not "the process xterm". (You global-replaced TERM, that yielded some nonsense results)

edsantiago · 2023-10-18T18:17:03Z

test/system/075-exec.bats

+                    if [[ -n "$exec_opt_t" ]]; then
+                        expected="xterm"
+                    fi
+                    # ...unless "run -t", which forces the run-process xterm


Actually, this block is now absurd. It should be collapsed, both ifs should become:

# if -t is set anywhere, either run or exec, go with xterm if [[ -n "$run_opt_t$exec_opt_t" ]]; then

edsantiago · 2023-10-18T18:17:56Z

test/system/075-exec.bats

+
+                    local desc="run $run_opt_t $run_opt_env, exec $exec_opt_t $exec_opt_env"
+                    TERM=exec-term run_podman exec $exec_opt_t $exec_opt_env test sh -c 'echo -n $TERM'
+                    # The normal case


No need for this comment any more, please remove

edsantiago · 2023-10-18T19:29:09Z

20404 (CI fix) is merged. You can now rebase. Please do fix the issues I reported above.

Fixes: containers#20334 Signed-off-by: Daniel J Walsh <[email protected]>

openshift-ci · 2023-10-18T21:47:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, rhatdan, 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~~ [edsantiago,rhatdan,vrothberg]

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

edsantiago · 2023-10-18T21:48:49Z

Flake is new to me. Restarted.

/lgtm
/hold

This has been quite the ordeal. Thank you @rhatdan for your patience with my obnoxious insistence on testing.

edsantiago · 2023-10-18T23:48:48Z

/hold cancel

openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 13, 2023

github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Oct 13, 2023

rhatdan force-pushed the TERM branch from b9ca0b7 to 6ad629d Compare October 13, 2023 18:43

vrothberg reviewed Oct 16, 2023

View reviewed changes

Luap99 reviewed Oct 16, 2023

View reviewed changes

edsantiago reviewed Oct 16, 2023

View reviewed changes

rhatdan force-pushed the TERM branch 2 times, most recently from 3947965 to 25bf9c1 Compare October 16, 2023 18:15

rhatdan force-pushed the TERM branch from 25bf9c1 to cd5974e Compare October 17, 2023 11:57

rhatdan force-pushed the TERM branch from cd5974e to 3c90007 Compare October 17, 2023 19:19

rhatdan force-pushed the TERM branch from 3c90007 to f63404f Compare October 18, 2023 14:27

vrothberg approved these changes Oct 18, 2023

View reviewed changes

rhatdan force-pushed the TERM branch from f63404f to 69a34f6 Compare October 18, 2023 17:13

edsantiago suggested changes Oct 18, 2023

View reviewed changes

rhatdan force-pushed the TERM branch from 69a34f6 to d434e9b Compare October 18, 2023 19:33

Add TERM iff TERM not defined in container when podman exec -t

b72bb11

Fixes: containers#20334 Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the TERM branch from d434e9b to b72bb11 Compare October 18, 2023 19:40

edsantiago approved these changes Oct 18, 2023

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 18, 2023

openshift-ci bot assigned edsantiago Oct 18, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 18, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 18, 2023

openshift-ci bot merged commit 6863641 into containers:main Oct 18, 2023
98 checks passed

edsantiago mentioned this pull request Oct 25, 2023

podman logs: missing output #18501

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 Jan 17, 2024

github-actions bot locked as resolved and limited conversation to collaborators Jan 17, 2024

Add TERM iff TERM not defined in container when podman exec -t #20357

Add TERM iff TERM not defined in container when podman exec -t #20357

Conversation

rhatdan commented Oct 13, 2023

Does this PR introduce a user-facing change?

rhatdan commented Oct 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsantiago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Oct 16, 2023

edsantiago commented Oct 16, 2023

edsantiago commented Oct 16, 2023

edsantiago commented Oct 17, 2023

rhatdan commented Oct 17, 2023

edsantiago commented Oct 17, 2023

edsantiago commented Oct 17, 2023

edsantiago commented Oct 17, 2023

rhatdan commented Oct 18, 2023

vrothberg left a comment

Choose a reason for hiding this comment

Luap99 commented Oct 18, 2023

Luap99 commented Oct 18, 2023

edsantiago commented Oct 18, 2023

rhatdan commented Oct 18, 2023

edsantiago commented Oct 18, 2023

edsantiago commented Oct 18, 2023

edsantiago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsantiago commented Oct 18, 2023

openshift-ci bot commented Oct 18, 2023

edsantiago commented Oct 18, 2023

edsantiago commented Oct 18, 2023