[apparmor] [PATCH 3/8] utils: Add confdir env variable to aa.py for in-tree testing

[Tyler Hicks]

On 02/12/2017 12:55 PM, Christian Boltz wrote:
> Hello,
> 
> Am Mittwoch, 8. Februar 2017, 22:01:40 CET schrieb Tyler Hicks:
>> Instead of hard-coding the location of logprof.conf and other utils
>> related configuration files to /etc/apparmor/, this patch looks for
>> the "APPARMOR_PY_CONFDIR" environment variable and, when set, uses
>> its value for the configuration directory.
>>
>> This allows for the make check target to use the in-tree config file,
>> profiles, and parser by default. To override this behavior, the
>> USE_SYSTEM make variable needs to be set like so:
>>
>>   $ make USE_SYSTEM=1 -C utils check
>>
>> The APPARMOR_PY_CONFDIR should be considered somewhat user-facing,
>> although undocumented at this time.
> 
> I'm not the biggest fan of env variables, so I'm a bit undecided on this 
> patch.

Agreed. I don't like env vars for stuff like this.

> 
> Pro:
> - the env variable is an easy solution
> - It makes it easy to test aa-logprof, aa-complain etc. with a different
>   config dir. Otherwise we'd have to add commandline parameters similar 
>   to what 5/8 does in aa-easyprof.

I think the most important pro is that it doesn't break existing users
of aa.py.

> 
> Cons:
> - undocumented env vars are not nice[tm]
> - env variables are less "visible" than commandline parameters, so if
>   someone "accidently" has them set, it will make debugging more funny.
> 
> 
> An alternative solution would be to move the global code in aa.py into
>     def init_aa( <various optional parameters> )
> 
> This would mean every user of aa.py has to call that function (at least 
> if using a function that depends on the configfile), but it would also 
> prevent executing some code (like reading the config file) when "just" 
> importing apparmor.aa.

I like this idea but would hate to see it deployed for this use case.
AFAICT, this confdir change is really only useful for our internal test
suite and breaking existing, external code for such a reason would be a
bad thing for us to do.

Do we have any other good reasons, which benefit external users of
aa.py, to introduce a requirement to call init_aa()?

Tyler

> 
> BTW: Honoring the env variable in the tests would be acceptable - I'm 
> not too keen to handle a --system or --local option in all test-*.py
> 
> 
> As I said, I'm still undecided, so please try to convince me that the 
> env variable is the better choice ;-)
> 
>> Signed-off-by: Tyler Hicks <tyhicks at canonical.com>
>> Cc: Christian Boltz <apparmor at cboltz.de>
>> ---
>>  utils/apparmor/aa.py | 2 +-
>>  utils/test/Makefile  | 6 ++++--
>>  2 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py
>> index eecf8c7..9450baa 100644
>> --- a/utils/apparmor/aa.py
>> +++ b/utils/apparmor/aa.py
>> @@ -73,7 +73,7 @@ _ = init_translation()
>>  # Setup logging incase of debugging is enabled
>>  debug_logger = DebugLogger('aa')
>>
>> -CONFDIR = '/etc/apparmor'
>> +CONFDIR = os.getenv('APPARMOR_PY_CONFDIR', '/etc/apparmor')
> 
> As Seth already noted, the env variable could be empty.
> 
> Let me add that it could include crap, for exampe
>     APPARMOR_PY_CONFDIR=/you/will/never/find/me
> 
> I didn't test what happens in such cases, but maybe some sanity check 
> might be a good idea.
> 
> 
> Besides this (and my general concerns about using an env variable), the 
> patch looks good.
> 
> 
> 
> Regards,
> 
> Christian Boltz
> 
> 
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20170215/9b880eff/attachment.pgp>