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

[Tyler Hicks]

On 02/15/2017 06:29 PM, Christian Boltz wrote:
> Hello,
> 
> Am Mittwoch, 15. Februar 2017, 12:21:05 CET schrieb Tyler Hicks:
>> On 02/12/2017 12:55 PM, Christian Boltz wrote:
>>> 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.
> 
> Silly question - are you aware of any external users of aa.py?

I'm not aware of any. I have very little involvement with aa.py so I'm
probably not the best person to ask.

If we regularly break the API, then I don't mind going the init_aa() route.

Tyler

> 
> I changed aa.py quite a lot, and never got any complaints IIRC. 
> So either we have no external users, or I was very lucky and only 
> touched code we only use internally ;-)  Given my luck in triggering 
> each and every bug, I'd consider this highly unlikely ;-)
> 
> Oh, and IIRC we never gave any guarantee about stable interfaces. This 
> doesn't mean we should break it without a good reason, but avoiding env 
> variables would be a good reason IMHO ;-)
> 
> I'm quite sure that apparmor/rule/*.py will have a stable interface 
> (unless something unexpected happens) - but I won't guarantee that for 
> aa.py because it contains too much code that is more or less on my 
> "needs a rewrite" list.
> 
> I even thought about making parts of aa.py (the part that handles and 
> stores profiles) a class, which would avoid some workarounds in aa-
> mergeprof. This won't happen in the near future, but maybe on the long 
> term. And there are some other things that need to be changed first ;-)
> 
> Side note: If someone needs stable interfaces for something, please 
> speak up. Ideally submit a test-external-interface.py which tests all 
> the function calls etc. you need - this is the fastest way to make 
> breakage visible ;-)
> 
>>> 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 
> 
> This is exactly why I don't like the env variable - if someone has it 
> set accidentially, funny things[tm] might happen. Especially the fact 
> that we only need it for internal usage (unit tests) means that exposing 
> it to production code can only hurt.
> 
> [Insert usual note about global variables (which are not too different 
> from env variables) and the "fun" you can have with them here]
> 
>> breaking existing, external code for such a reason
>> would be a bad thing for us to do.
> 
> We should split off a 2.11 branch before changing the interface - but 
> besides that, I see no problem with requiring an init_aa() call.
> 
>> Do we have any other good reasons, which benefit external users of
>> aa.py, to introduce a requirement to call init_aa()?
> 
> Well, actually _not_ calling init_aa() could be a reason ;-)
> Not all functions in aa.py depend on the config file etc., so for example 
> some of our test cases could work without it. (About half the functions 
> don't use one of the variables initialized in the global area of aa.py - 
> but I have to admit I didn't check if they depend on other functions.)
> 
> The only code that will/should set a non-default config dir are unit 
> tests. 
> 
> Everything else (aka "production code") would just call init_aa() 
> without any parameters.
> 
> Oh, and if we really want, we could introduce a --confdir option in the 
> aa-* tools. Again, this would probably be most interesting for 
> development and testing.
> 
> 
> BTW: The biggest problem with the current global code in aa.py are 
> probably these two lines:
> 
>     raise AppArmorException('Can\'t find AppArmor profiles')
> 
> and
> 
>     raise AppArmorException('Can\'t find apparmor_parser')
> 
> If you find a way _not_ to raise those exceptions, while still ensuring 
> parser and profile dir exist (before any attemp to access them), that 
> would be the best solution.
> 
> An easy way might be to replace the global parser and profile_dir 
> variables with functions that return their value (after checking they 
> are set and exist), but that might come with a performance impact.
> 
> Doing it for the parser should be cheap (in comparison to actually 
> running the parser), but profile_dir is used more often.
> 
> Oh, and while we are on it - currently, apparmor_parser calls are spread 
> over aa.py, easyprof.py, sandbox.py and tools.py. I always wanted to 
> have all parser calls in a separate module ;-)  with functions like 
> load_profile(), unload_profile() etc.  - and probably an init_parser() 
> that sets the parser path.
> 
> If we get rid of raising the exceptions, injecting special paths when 
> running the unit tests should be easy.
> 
> 
> 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/37b782a8/attachment-0001.pgp>