many: add components to interfaces.SnapAppSet #13837

andrewphelpsj · 2024-04-15T17:04:33Z

This change adds components to interfaces.SnapAppSet, and it adds an error to be returned from interfaces.NewSnapAppSet. This error currently is currently only used to indicate if any components added to the app set are not from the snap that the app set is based off of.

Based on #13774, first relevant commit is 58a0ab3

pedronis

thanks, here I see an issue with unit tests? missing a dirs.SetRootDir?

pedronis · 2024-04-17T14:42:46Z

interfaces/snap_app_set.go

+func NewSnapAppSet(info *snap.Info, components []*snap.ComponentInfo) (*SnapAppSet, error) {
+	for _, c := range components {
+		if c.Component.SnapName != info.SnapName() {
+			return nil, fmt.Errorf("internal error: snap %q does not own component %q", info.SnapName(), c.Component)


do we want a test for this?

pedronis · 2024-04-17T14:43:50Z

interfaces/snap_app_set_test.go

I'm getting:

FAIL: snap_app_set_test.go:155: snapAppSetSuite.TestPlugSecurityTags snap_app_set_test.go:168: compInfo := snaptest.MockComponent(c, "component: name+comp\ntype: test\nversion: 1", info) /home/pedronis/canonical/go-ws/src/github.com/snapcore/snapd/snap/snaptest/snaptest.go:102: c.Assert(err, check.IsNil) ... value *fs.PathError = &fs.PathError{Op:"mkdir", Path:"/snap/name", Err:0xd} ("mkdir /snap/name: permission denied")

pedronis · 2024-04-17T14:44:27Z

overlord/ifacestate/helpers.go

@@ -175,7 +175,13 @@ func (m *InterfaceManager) regenerateAllSecurityProfiles(tm timings.Measurer) er
 	// TODO: should snapsWithSecurityProfiles return app sets instead of snap infos?
 	appSets := make([]*interfaces.SnapAppSet, 0, len(snaps))
 	for _, sn := range snaps {
-		appSets = append(appSets, interfaces.NewSnapAppSet(sn))
+


not sure we need/want this newline

pedronis

thank you

alfonsosanchezbeato

LGTM, thanks. Just a small question.

interfaces/snap_app_set.go

zyga · 2024-05-06T12:36:24Z

@andrewphelpsj can you please merge or rebase to reduce the diff and see how it fares on latest code please?

andrewphelpsj · 2024-05-06T13:10:21Z

@zyga rebased! would like to get this merged soon if I can get a clean test run.

zyga · 2024-05-06T16:04:52Z

@zyga rebased! would like to get this merged soon if I can get a clean test run.

The diff is still 2500 lines or so, is that expected?

andrewphelpsj · 2024-05-06T16:09:53Z

Unfortunately yes, there were a lot of unit tests to update for all of the different interfaces. The actual code changes are pretty small:

phelps@arch ~/w/components-in-app-set> git diff upstream/master --name-only | rg -v _test | xargs git diff upstream/master --stat
 interfaces/ifacetest/backendtest.go | 14 ++++++++++----
 interfaces/snap_app_set.go          | 13 +++++++++----
 overlord/ifacestate/handlers.go     | 57 +++++++++++++++++++++++++++++++++++++++++++++++----------
 overlord/ifacestate/helpers.go      |  7 ++++++-
 packaging/fedora/snapd.spec         | 10 ++++------
 spread.yaml                         |  6 ++----
 6 files changed, 78 insertions(+), 29 deletions(-)

… components from the wrong snap

… from the wrong snap

andrewphelpsj · 2024-05-06T16:48:14Z

Rebased again to bring in 58dfc18 so that spread can pass on amazon linux.

…components to interfaces.SnapAppSet (canonical#13837))

* sandbox/apparmor: add GenerateAAREExclusionPatterns This function is generic (and complex) enough to be able to handle all of the overlapping and wildcard behavior we need in docker-support, and it could also serve to replace numerous other places in the codebase where we need this sort of complex behavior. It is a generalization of the existing aareExclusionPatterns helper, though it's actually unclear if this exact implementation will currently be able to serve the use case from that helper directly or if more options/adjustments are needed to enable that use case as well. To keep the diff smaller, this patch does not actually change any of the profiles/interfaces, just TODO's are left for where to use it. Note that the generated rules are slightly more condensed in terms of number of rules but significantly more verbose in terms of alternations, not sharing more of repeated substrings between alternations inside the patterns. This was done explicitly to keep the generating code simpler and easier to understand, but it may prove to have performance effects, either detrimental or benevolent but that should be measured before deciding to make the generation code even more complex than it already is. Signed-off-by: Ian Johnson <[email protected]> * interfaces/docker-support: generate AARE exclusion patterns with helper func Signed-off-by: Ian Johnson <[email protected]> * sandbox/apparmor: unexport helper functions These were not meant to be exported, only the fully generic one is meant to be exported. Signed-off-by: Ian Johnson <[email protected]> * sandbox/apparmor: fix bug mis-sorting capitalized letters in AARE exclude patt Thanks to Alberto for spotting this :-) Signed-off-by: Ian Johnson <[email protected]> * sandbox/apparmor: fix format issues introduced during rebase * sandbox/apparmor: simplify generateAAREExclusionPatternsGenericImpl * sandbox/apparmor: add checks for unsupported cases and improve documentation * sandbox/apparmor: update tests to compare the apparmor binary instead of source * interfaces/builtin/docker_support: check if userns is supported before adding it to the profile * interfaces/builtin/docker_support: fix dependencies * sandbox/apparmor: use placeholders * i/b/docker_support_test: update TestGenerateAAREExclusionPatterns to use SnapAppSet * testutil/apparmor: use go crypto/sha1 module instead of system sha1sum command * {sandbox,testutil}/apparmor: minor format fixes * move helper to find common prefix to strutil * add copyright info * use string builder * i/b/docker_support_test.go: update accordingly to 277fbc2 (many: add components to interfaces.SnapAppSet (#13837)) * strutil/commonprefix.go: remove extra empty line * sandbox/apparmor/apparmor.go: sort prefixes to ensure profile is always the same * sandbox/apparmor/apparmor.go: remove extra empty line * i/b/docker_support_test: skip TestGenerateAAREExclusionPatterns is apparmor_parser is not usable --------- Signed-off-by: Ian Johnson <[email protected]> Co-authored-by: Ian Johnson <[email protected]>

andrewphelpsj requested review from pedronis and alfonsosanchezbeato April 15, 2024 17:04

github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Apr 15, 2024

andrewphelpsj force-pushed the components-in-app-set branch from ad8a7c7 to 8145020 Compare April 16, 2024 19:18

andrewphelpsj mentioned this pull request Apr 17, 2024

many: generate security profiles from component hooks #13686

Merged

andrewphelpsj force-pushed the components-in-app-set branch from 8145020 to 5f5b3ab Compare April 17, 2024 14:26

github-actions bot removed the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Apr 17, 2024

pedronis reviewed Apr 17, 2024

View reviewed changes

andrewphelpsj requested a review from pedronis April 17, 2024 15:05

pedronis approved these changes Apr 17, 2024

View reviewed changes

andrewphelpsj force-pushed the components-in-app-set branch from aeb321c to 55c399a Compare April 30, 2024 18:04

alfonsosanchezbeato approved these changes May 2, 2024

View reviewed changes

interfaces/snap_app_set.go Outdated Show resolved Hide resolved

andrewphelpsj force-pushed the components-in-app-set branch from 0f3233b to ae6e4f2 Compare May 6, 2024 13:06

andrewphelpsj added 6 commits May 6, 2024 12:47

many: add components to SnapAppSet

5ab78e1

many: make interfaces.NewSnapAppSet return an error to prevent adding…

0e04cf2

… components from the wrong snap

interfaces: fix app set unit test

1b06efa

o/ifacestate: remove stray newline

07d1a86

interfaces: add test that checks when app set is made with components…

3c158c4

… from the wrong snap

interfaces: remove done TODO

ad61f0a

andrewphelpsj force-pushed the components-in-app-set branch from ae6e4f2 to ad61f0a Compare May 6, 2024 16:47

andrewphelpsj merged commit 277fbc2 into canonical:master May 7, 2024
45 of 48 checks passed

andrewphelpsj deleted the components-in-app-set branch May 7, 2024 13:54

jslarraz added a commit to jslarraz/snapd that referenced this pull request Jun 10, 2024

i/b/docker_support_test.go: update accordingly to 277fbc2 (many: add …

999ada8

…components to interfaces.SnapAppSet (canonical#13837))

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

many: add components to interfaces.SnapAppSet #13837

many: add components to interfaces.SnapAppSet #13837

andrewphelpsj commented Apr 15, 2024 •

edited

Loading

pedronis left a comment

pedronis Apr 17, 2024

pedronis Apr 17, 2024

pedronis Apr 17, 2024

pedronis left a comment

alfonsosanchezbeato left a comment

zyga commented May 6, 2024

andrewphelpsj commented May 6, 2024

zyga commented May 6, 2024

andrewphelpsj commented May 6, 2024

andrewphelpsj commented May 6, 2024

many: add components to interfaces.SnapAppSet #13837

many: add components to interfaces.SnapAppSet #13837

Conversation

andrewphelpsj commented Apr 15, 2024 • edited Loading

pedronis left a comment

Choose a reason for hiding this comment

pedronis Apr 17, 2024

Choose a reason for hiding this comment

pedronis Apr 17, 2024

Choose a reason for hiding this comment

pedronis Apr 17, 2024

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

zyga commented May 6, 2024

andrewphelpsj commented May 6, 2024

zyga commented May 6, 2024

andrewphelpsj commented May 6, 2024

andrewphelpsj commented May 6, 2024

andrewphelpsj commented Apr 15, 2024 •

edited

Loading