[PATCH] test-in-container: Do not set GCONV_PATH, LOCPATH

[PATCH] test-in-container: Do not set GCONV_PATH, LOCPATH

[Florian Weimer]

These environment variables are incorrect inside the container.

2019-05-20  Florian Weimer  <fweimer@redhat.com>

	test-in-container: Do not set GCONV_PATH, LOCPATH.
	* Makeconfig (run-program-env-container): New variable.
	(run-program-env): Use it.
	* Rules ($(tests-container:%=$(objpfx)%.out)): Update comment.
	Use $(run-program-env-container) instead of $(run-program-env).

diff --git a/Makeconfig b/Makeconfig
index 0e386fbc19..2fbd3c0192 100644
--- a/Makeconfig
+++ b/Makeconfig
@@ -743,10 +743,13 @@ run-via-rtld-prefix =							      \
 else
 run-via-rtld-prefix =
 endif
+# Test-in-containers need fewer environment variables.
+run-program-env-container = LC_ALL=C
 # $(run-program-env) is the default environment variable settings to
 # use when running a program built with the newly built library.
 run-program-env = GCONV_PATH=$(common-objpfx)iconvdata \
-		  LOCPATH=$(common-objpfx)localedata LC_ALL=C
+		  LOCPATH=$(common-objpfx)localedata \
+		  $(run-program-env-container)
 # $(run-program-prefix) is a command that, when prepended to the name
 # of a program built with the newly built library, produces a command
 # that, executed on the build system on which "make" is run, runs that
diff --git a/Rules b/Rules
index 222dba6dcb..2c8f1eecb0 100644
--- a/Rules
+++ b/Rules
@@ -273,10 +273,15 @@ $(objpfx)%.out: /dev/null $(objpfx)%	# Make it 2nd arg for canned sequence.
 # Any tests that require an isolated container (filesystem, network
 # and pid namespaces) in which to run, should be added to
 # tests-container.
+#
+# Note: Outer use of $(run-program-env-container) instead of
+# $(run-program-env) assumes that support/test-container does not use
+# locales or gconv modules.
 $(tests-container:%=$(objpfx)%.out): $(objpfx)%.out : $(if $(wildcard $(objpfx)%.files),$(objpfx)%.files,/dev/null) $(objpfx)%
-	$(test-wrapper-env) $(run-program-env) $(run-via-rtld-prefix) \
-	  $(common-objpfx)support/test-container env $(run-program-env) $($*-ENV) \
-	  $(host-test-program-cmd) $($*-ARGS) > $@; \
+	$(test-wrapper-env) $(run-program-env-container) \
+          $(run-via-rtld-prefix) \
+	  $(common-objpfx)support/test-container env $($*-ENV) \
+	    $(host-test-program-cmd) $($*-ARGS) > $@; \
 	$(evaluate-test)
 
 

Re: [PATCH] test-in-container: Do not set GCONV_PATH, LOCPATH

[DJ Delorie]

There's code in test-container.c to not propogate environment variables
to the in-container test:

      while (is_env_setting (argv[1]))
	{
	  /* If there are variables we do NOT want to propogate, this
	     is where the test for them goes.  */
	    {

That way we can treate test-container itself like any other testcase,
and have it worry about "cleaning" the environment for the contained
tests.

The source/build directories are loop-mounted inside the container, so
such environment variables *should* work if set, and there were some
test when I was testing everything in containers that still needed those
vars set.  I didn't investigate why.

Re: [PATCH] test-in-container: Do not set GCONV_PATH, LOCPATH

[Florian Weimer]

* DJ Delorie:

> There's code in test-container.c to not propogate environment variables
> to the in-container test:
>
>       while (is_env_setting (argv[1]))
> 	{
> 	  /* If there are variables we do NOT want to propogate, this
> 	     is where the test for them goes.  */
> 	    {
>
> That way we can treate test-container itself like any other testcase,
> and have it worry about "cleaning" the environment for the contained
> tests.

Still there is a separate Rules recipe.

> The source/build directories are loop-mounted inside the container, so
> such environment variables *should* work if set, and there were some
> test when I was testing everything in containers that still needed those
> vars set.

They probably use locales that are not installed, or not installed under
the names configured for the test, so that's not unexpected.

The problem is that setting GCONV_PATH alters iconv behavior (it
disables the cache).  Similar for LOCPATH.  Setting those variables
really limits the scope of the test-in-container framework.

Thanks,
Florian

Re: [PATCH] test-in-container: Do not set GCONV_PATH, LOCPATH

[DJ Delorie]

Florian Weimer <fweimer@redhat.com> writes:
> Still there is a separate Rules recipe.

Sure, I'm just saying I thought about this problem when I initially
wrote it, so we don't have to worry about if test-container itself uses
a variable.  The goal back then was "test-container should be itself run
like any other test case".  That means same env vars.

So IMHO the right fix is to filter out those variables inside
test-container.c where the child's environment is set up.

Re: [PATCH] test-in-container: Do not set GCONV_PATH, LOCPATH

[Florian Weimer]

* DJ Delorie:

> Florian Weimer <fweimer@redhat.com> writes:
>> Still there is a separate Rules recipe.
>
> Sure, I'm just saying I thought about this problem when I initially
> wrote it, so we don't have to worry about if test-container itself uses
> a variable.  The goal back then was "test-container should be itself run
> like any other test case".  That means same env vars.
>
> So IMHO the right fix is to filter out those variables inside
> test-container.c where the child's environment is set up.

Should we perhaps start with an environment that contains just PATH, and
add all the *-ENV variables (but not LOCPATH and GCONV_PATH)?

Thanks,
Florian

Re: [PATCH] test-in-container: Do not set GCONV_PATH, LOCPATH

[DJ Delorie]

Florian Weimer <fweimer@redhat.com> writes:
> Should we perhaps start with an environment that contains just PATH, and
> add all the *-ENV variables (but not LOCPATH and GCONV_PATH)?

I hate to put in a "full clean environment" option because we don't know
what environment variables the user might have set intentionally for
running the testsuite (distcc, for example).  But I see the benefit of
sanitizing the environment, both from the usual environment and from the
command line.

Re: [PATCH] test-in-container: Do not set GCONV_PATH, LOCPATH

[Florian Weimer]

* DJ Delorie:

> Florian Weimer <fweimer@redhat.com> writes:
>> Should we perhaps start with an environment that contains just PATH, and
>> add all the *-ENV variables (but not LOCPATH and GCONV_PATH)?
>
> I hate to put in a "full clean environment" option because we don't know
> what environment variables the user might have set intentionally for
> running the testsuite (distcc, for example).  But I see the benefit of
> sanitizing the environment, both from the usual environment and from the
> command line.

Sorry, I don't understand.  Why would the user want to run distcc *in
the container*?  What reason do we have to believe that the host
environment variable settings (regarding terminal, locales and the like)
are even valid in the container?

Thanks,
Florian

Re: [PATCH] test-in-container: Do not set GCONV_PATH, LOCPATH

[DJ Delorie]

Florian Weimer <fweimer@redhat.com> writes:
> Sorry, I don't understand.  Why would the user want to run distcc *in
> the container*?

That was just an example of an environment variable that affects the
build.  I don't know what environment variables affect the test cases.
Timeout-related?  Without a review of *every* test that might be run in
a container, we won't know what environment variables are valid for the
user to specify.

> What reason do we have to believe that the host environment variable
> settings (regarding terminal, locales and the like) are even valid in
> the container?

We don't know they're valid, but we don't know they're invalid either.
We don't filter the environment for non-container variables.  Not
over-filtering the container is at least consistent.

Re: [PATCH] test-in-container: Do not set GCONV_PATH, LOCPATH

[Zack Weinberg]

On Tue, May 21, 2019 at 9:24 AM DJ Delorie <dj@redhat.com> wrote:
>
> Florian Weimer <fweimer@redhat.com> writes:
> > Sorry, I don't understand.  Why would the user want to run distcc *in
> > the container*?
>
> That was just an example of an environment variable that affects the
> build.  I don't know what environment variables affect the test cases.
> Timeout-related?  Without a review of *every* test that might be run in
> a container, we won't know what environment variables are valid for the
> user to specify.

Maybe we should filter the environment, but have a mechanisme for
explicitly setting environment variables within the container?  For
instance, setting CONTAINER_ENVNAME=value could translate to
ENVNAME=value within the container.

zw

Re: [PATCH] test-in-container: Do not set GCONV_PATH, LOCPATH

[Florian Weimer]

* DJ Delorie:

> Florian Weimer <fweimer@redhat.com> writes:
>> Sorry, I don't understand.  Why would the user want to run distcc *in
>> the container*?
>
> That was just an example of an environment variable that affects the
> build.  I don't know what environment variables affect the test cases.
> Timeout-related?  Without a review of *every* test that might be run in
> a container, we won't know what environment variables are valid for the
> user to specify.

But right now, only new tests run in a container, so I think we should
focus on what helps us to write such new tests, so that they run in an
environment that is as realistic as possible.

We do not have many environment dependencies in the test harness itself:

support/support_test_main.c:  char *envstr_timeoutfactor = getenv ("TIMEOUTFACTOR");
support/support_test_main.c:      test_dir = getenv ("TMPDIR");
support/support_test_main.c:  const char *envstr_direct = getenv ("TEST_DIRECT");
support/support_test_main.c:    const char *coredumps = getenv ("TEST_COREDUMPS");

TIMEOUTFACTOR and TEST_COREDUMPS are relevant, I think, and should not
be filtered out.  But TMPDIR and TEST_DIRECT are unlikely to work
because they specify paths which may not be relevant in the chroot.

Thanks,
Florian

Re: [PATCH] test-in-container: Do not set GCONV_PATH, LOCPATH

[Carlos O'Donell]

On 5/23/19 8:59 AM, Florian Weimer wrote:
> * DJ Delorie:
> 
>> Florian Weimer <fweimer@redhat.com> writes:
>>> Sorry, I don't understand.  Why would the user want to run distcc *in
>>> the container*?
>>
>> That was just an example of an environment variable that affects the
>> build.  I don't know what environment variables affect the test cases.
>> Timeout-related?  Without a review of *every* test that might be run in
>> a container, we won't know what environment variables are valid for the
>> user to specify.
> 
> But right now, only new tests run in a container, so I think we should
> focus on what helps us to write such new tests, so that they run in an
> environment that is as realistic as possible.
> 
> We do not have many environment dependencies in the test harness itself:
> 
> support/support_test_main.c:  char *envstr_timeoutfactor = getenv ("TIMEOUTFACTOR");
> support/support_test_main.c:      test_dir = getenv ("TMPDIR");
> support/support_test_main.c:  const char *envstr_direct = getenv ("TEST_DIRECT");
> support/support_test_main.c:    const char *coredumps = getenv ("TEST_COREDUMPS");
> 
> TIMEOUTFACTOR and TEST_COREDUMPS are relevant, I think, and should not
> be filtered out.  But TMPDIR and TEST_DIRECT are unlikely to work
> because they specify paths which may not be relevant in the chroot.

I agree.

I expect many refinements like this which hone our concept
of what it means to run a test in a clean environment.

Initially when we did the implementation we needed to keep
the environment variables because historically DJ's testing
included running *every* test through test-in-container just
to prove we could and see if anything failed. This was good
during the bootstrap process of writing test-in-container.

However, now that we have test-in-container upstream, we have
to use a guiding principle to make decisions.

I would suggest that test-in-container's initial environment
should be as clean as possible, free of any host environment
variables. Then the test sets whatever environment variables
are required. The only environment variables that are worth
passing down are those related to test framework control
e.g. TIMEOUTFACTOR.

-- 
Cheers,
Carlos.

Re: [PATCH] test-in-container: Do not set GCONV_PATH, LOCPATH

[DJ Delorie]

"Carlos O'Donell" <carlos@redhat.com> writes:
> I would suggest that test-in-container's initial environment
> should be as clean as possible, free of any host environment
> variables.

I don't feel particularly strongly about this as long as my concerns are
heard, although if we want a clean environment, we're back to filtering
inside test-container.c itself, since "just don't set it" won't stop the
user environment from leaking into the container.

Re: [PATCH] test-in-container: Do not set GCONV_PATH, LOCPATH

[Carlos O'Donell]

On 5/23/19 2:48 PM, DJ Delorie wrote:
> 
> "Carlos O'Donell" <carlos@redhat.com> writes:
>> I would suggest that test-in-container's initial environment
>> should be as clean as possible, free of any host environment
>> variables.
> 
> I don't feel particularly strongly about this as long as my concerns are
> heard, although if we want a clean environment, we're back to filtering
> inside test-container.c itself, since "just don't set it" won't stop the
> user environment from leaking into the container.

Correct. I believe we even discussed this during the initial design, and
rejected it as a non-goal for the first implementation.

I'm OK with filtering directly in test-container.c to leave a clean
environment for the program under test. Somebody needs to implement
that though. Are you volunteering for that Florian? :-)

-- 
Cheers,
Carlos.