sandbox/apparmor: aare exclusion rule generation

[jslarraz]

Followup of PR#11567

• Core function generateAAREExclusionPatternsGenericImpl has been reworked to simplify it as much as possible
• Tests have been updated to compare the compiled profile (generated by apparmor_parser) instead of profile strings

[codecov-commenter]

Codecov Report

Attention: 70 lines in your changes are missing coverage. Please review.

Comparison is base (a6a335b) 78.99% compared to head (b0b48e0) 78.89%. Report is 55 commits behind head on master.

Files	Patch %	Lines
testutil/apparmor.go	0.00%	52 Missing :warning:
interfaces/builtin/docker_support.go	78.94%	8 Missing and 4 partials :warning:
sandbox/apparmor/apparmor.go	93.02%	4 Missing and 2 partials :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #13488      +/-   ##
==========================================
- Coverage   78.99%   78.89%   -0.11%     
==========================================
  Files        1030     1033       +3     
  Lines      130310   131580    +1270     
==========================================
+ Hits       102940   103804     +864     
- Misses      20965    21321     +356     
- Partials     6405     6455      +50     

Flag	Coverage Δ
unittests	78.89% <64.10%> (-0.11%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[alexmurray]

@jslarraz unit tests appear to be failing:

FAIL: docker_support_test.go:291: DockerSupportInterfaceSuite.TestGenerateAAREExclusionPatterns

[alexmurray]

@zyga hey, any chance you or someone else from the snapd team can take a look at this one? Thanks!

[jslarraz]

Updated to use place holders as suggested

[pedronis]

Separately: can we agree to move away from ###STUFF### replacements and move to text/template which is safer and more efficient. This is a follow-up for this but I just want say we should stop pushing down that path where we had serious release-breaking bugs because we forgot to do string replacement right and it was slient.

that's a discussion for snapd, the PR is just following what most things are doing in interfaces, which is placeholders/replacement based, that's an old decision and I wasn't actually part of it. There are probably challenges to use templates as well that need to be figured out on their own.

At least the code here is checking that the placeholder is present

[jslarraz]

Hey @pedronis @zyga @bboozzoo

I did a first pass on your commentaries. Waiting for feedback.

[jslarraz]

Hey @zyga @bboozzoo,

Any update so far? specially regarding https://github.com/snapcore/snapd/pull/13488#discussion_r1587274327

[zyga]

Hey @zyga @bboozzoo,

Any update so far? specially regarding https://github.com/snapcore/snapd/pull/13488#discussion_r1587274327

I'll review this soon

[jslarraz]

Updated to use string builder in https://github.com/snapcore/snapd/pull/13488/commits/5d3279b19f6c81d917f386a1d3985abccb317ff7. Master merged and i/b/docker_support_tests updated to add components to interfaces.SnapAppSet

[bboozzoo]

cross distro unit tests are failing, please have a look. I suspect it's likely due to SnapMountDir being different on Fedora/openSUSE. You'll likely need to call release.MockOSRelease(..)

[jslarraz]

Tests in sandbox/apparmor/apparmor_test.go were designed to compile the apparmor profile under tests using apparmor_parser and compare compiled version against the expected. When apparmor_parser is not available (as it seems to be the case in Fedora/openSUSE) test falls back to raw string comparison of the profiles. That caused some issues I addressed in https://github.com/snapcore/snapd/pull/13488/commits/e43153dc086140704d7392ebe407a37e9e34b786 and https://github.com/snapcore/snapd/pull/13488/commits/d064ae5b35fec8907d222c046b2e7616d0f2d362.

For interfaces/builtin/docker_support_test.go, falling back to string comparison is not desirable as it will require to modify the current version of the apparmor rules to fit the exact string generated by GenerateAAREExclusionPatterns, loosing its intention to act as a regression test. Maybe disabling this tests if apparmor_parser is not available is a better approach here. Thoughts?

docker_support_test.go:859:
    c.Assert(err, IsNil)
... value *exec.Error = &exec.Error{Name:"apparmor_parser", Err:(*errors.errorString)(0x1242140)} ("exec: \"apparmor_parser\": executable file not found in $PATH")

[bboozzoo]

Tests in sandbox/apparmor/apparmor_test.go were designed to compile the apparmor profile under tests using apparmor_parser and compare compiled version against the expected. When apparmor_parser is not available (as it seems to be the case in Fedora/openSUSE) test falls back to raw string comparison of the profiles. That caused some issues I addressed in e43153d and d064ae5.

For interfaces/builtin/docker_support_test.go, falling back to string comparison is not desirable as it will require to modify the current version of the apparmor rules to fit the exact string generated by GenerateAAREExclusionPatterns, loosing its intention to act as a regression test. Maybe disabling this tests if apparmor_parser is not available is a better approach here. Thoughts?

docker_support_test.go:859:
    c.Assert(err, IsNil)
... value *exec.Error = &exec.Error{Name:"apparmor_parser", Err:(*errors.errorString)(0x1242140)} ("exec: \"apparmor_parser\": executable file not found in $PATH")

Yes, skipping the test sounds fine if the parser isn't available. I think I should ultimately fix the unit test and install runtime dependencies as well, but it's a lower priority, so feel free to simply call c.Skip() when the parser is missing.

[jslarraz]

@bboozzoo Updated to skip the test when apparmorparser is not usable

[ernestl]

Not critical for 2.64, but nice to have

[ernestl]

@zyga, please have another look

[bboozzoo]

@jslarraz can you rebase once more?