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

[Christian Boltz]

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.

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.

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.

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
-- 
> > > > Boah. Du liest Doku... cool. Wie ist das so? :-)
> > > Ist ganz interessant ;-)
> > Sind da auch Bilder drin? Von Mädchen?
> *LoL* Bisher hab ich noch keine gefunden ;-)
Dann find ich Doku doof. [> Christian Boltz und Ratti]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20170212/42d51078/attachment.pgp>