interfaces,dirs: add ldconfig backend #14926

alfonsosanchezbeato · 2025-01-14T17:57:00Z

This backend will expose libraries coming from snaps to either the
rootfs or to other snaps (currently supporting only the former).

github-actions · 2025-01-14T19:56:30Z

Mon Feb 3 14:25:56 UTC 2025
The following results are from: https://github.com/canonical/snapd/actions/runs/13077391096

Failures:

Preparing:

openstack:debian-sid-64:tests/regression/

Executing:

openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-gating
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-pre-download:restart
openstack:opensuse-tumbleweed-64:tests/main/snap-refresh-hold
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-gating-from-snap
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-backoff
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-retry
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh:parallel
openstack:opensuse-tumbleweed-64:tests/main/refresh-app-awareness-notify
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-pre-download:close_mid_restart
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-pre-download:close
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh:regular
openstack:opensuse-tumbleweed-64:tests/main/auto-refresh-pre-download:ignore

Restoring:

openstack:opensuse-tumbleweed-64:tests/main/refresh-app-awareness-notify

Meulengracht

This looks sensible to me, and clean. Thank you

pedronis

One main question.

Further usually interfaces decide to do different things depending if the slot is implicit (see the usage of the implicitSystemPermanent/ConnectedSlot helpers) but maybe here we want to do something at the backend level?

pedronis · 2025-01-16T10:22:45Z

dirs/dirs.go

@@ -465,6 +466,7 @@ func SetRootDir(rootdir string) {

 	SnapDataDir = filepath.Join(rootdir, "/var/snap")
 	SnapAppArmorDir = filepath.Join(rootdir, snappyDir, "apparmor", "profiles")
+	SnapLdconfigDir = filepath.Join(rootdir, "etc/ld.so.conf.d")


nitpick: the style here seems to be /etc...

pedronis · 2025-01-16T10:28:07Z

interfaces/ldconfig/backend.go

+	// the rootfs. For snaps, we will create files in
+	// /var/lib/snapd/ldconfig/ that will be used to generate a cache
+	// specific to each snap.
+	ldconfigPath := filepath.Join(dir, "snapd.conf")


I'm confused, specification are per snap app set but here we reuse the same file? won't multiple snap stomp on each ther?

I have changed this. However I started to re-think about this and I am not sure anymore, maybe I'm missing some piece. I created snapd.conf thinking of it as the only config file for the "system" (rootfs) snap (the implementation is not considering exporting libraries to snap yet). That is, the "snap app set" here is the rootfs. Afaiu any change in interfaces would trigger the call to Setup(), and having multiple files will help with knowing where libraries come from (this could still be done by adding comments in snapd.conf), but there is no other practical effect as the call to ldconfig will process all files in here.

My plan for the implementation for the snaps case, is to have also only one config file per snap, that will be seen in ld.so.conf.d/ but only for the mount namespace of the snap, that would be a mix of the contents of the app base and of these files generated by snapd (that will be found also in the rootfs in, maybe, /var/lib/snapd/ldconfig/).

@pedronis as discussed, I've looked with a bit more of detail into this and how it works now is

Tasks like "connect" and "setup-profiles" connect the interfaces. In a refresh case, the later does a set of operations (

snapd/overlord/ifacestate/handlers.go

Lines 244 to 253 in b85fde1

// The snap may have been updated so perform the following operation to

// ensure that we are always working on the correct state:

//

// - disconnect all connections to/from the given snap

// - remembering the snaps that were affected by this operation

// - remove the (old) snap from the interfaces repository

// - add the (new) snap to the interfaces repository

// - restore connections based on what is kept in the state

// - if a connection cannot be restored then remove it from the state

// - setup the security of all the affected snaps

) that in the end implies that repo.Connect is called for all connections in the state, filling the interfaces repo appropriately.

setupSecurityByBackend is then called in all these tasks, which calls interfaces.SetupMany for all backends of the affected snaps

When the backend's Setup is called it calls repo.SnapSpecification, which adds the connected plug for all connected slots to a plug, which covers the greedy plug case (

snapd/interfaces/repo.go

Lines 941 to 952 in b85fde1

// plug side

for _, plugInfo := range r.plugs[snapName] {

iface := r.ifaces[plugInfo.Interface]

if err := spec.AddPermanentPlug(iface, plugInfo); err != nil {

return nil, err

}

for _, conn := range r.plugSlots[plugInfo] {

if err := spec.AddConnectedPlug(iface, conn.Plug, conn.Slot); err != nil {

return nil, err

}

}

}

). This is symmetric to slots with multiple connections. Therefore the specification should contain the right information even on refreshing snaps due to step 1.

I think then that greedy plugs in principle work as expected, and the unit test that I have implemented for the ldconfig backend does similar things to what is done by the taks. I lean now more towards the option of having just one configuration file in /etc/ld.conf.so.d/ for hybrid, to avoid polluting too much that user-exposed folder. Inside it we could add comments to specify what parts come from a given snap/slot. Thoughts?

So if I follow if we have a greedy plug on connected to many slot snaps <slot-snap1,2...>, when e.g. slot-snap2 refreshes:

will add to affected snaps

When we construct SnapSpecification for 3. will call AddConnectedPlug for all the slots is connected to and not just the one for slot-snap2?

r.plugSlots is filled by calls to Repository.Connect. Afaiu in a refresh all connections are restored in the repository by InterfaceManager.reloadConnections() (

snapd/overlord/ifacestate/helpers.go

Line 472 in b85fde1

if _, err := m.repo.Connect(connRef, staticPlugAttrs, connState.DynamicPlugAttrs, staticSlotAttrs, connState.DynamicSlotAttrs, nil); err != nil {

).

So unless I got something wrong, the specification should contain information for all the connected slots, that is, for slot-snap1 and 2.

Afaiu in a refresh all connections are restored in the repository by InterfaceManager.reloadConnections()

all connections for the given snap

I created the tests we were discussing:

#14985

the conclusion from there is that we should probably create a file per consumer side

pedronis

comment

pedronis · 2025-01-21T17:35:27Z

interfaces/ldconfig/spec.go

+// Methods called by interfaces
+
+// AddLibDirs add dirs with libraries to the specification.
+func (spec *Specification) AddLibDirs(snapName, slotName string, dirs []string) {


we can capture the contextual info on the spec in AddConnectedPlug, I would expect that's the place we want this to work from? then we should error or panic if it's called with the contextual info unse

see something like setScope in the apparmor backend, though what we need here is simpler

alfonsosanchezbeato

@pedronis thanks, comments addressed now

alfonsosanchezbeato · 2025-01-21T22:42:35Z

interfaces/ldconfig/spec.go

+// Methods called by interfaces
+
+// AddLibDirs add dirs with libraries to the specification.
+func (spec *Specification) AddLibDirs(snapName, slotName string, dirs []string) {


codecov · 2025-01-21T23:01:21Z

Codecov Report

Attention: Patch coverage is 68.92655% with 55 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@6fa2df6). Learn more about missing BASE report.
Report is 59 commits behind head on master.

Files with missing lines	Patch %	Lines
interfaces/ldconfig/backend.go	68.88%	23 Missing and 5 partials ⚠️
interfaces/ifacetest/testiface.go	0.00%	20 Missing ⚠️
interfaces/ldconfig/spec.go	86.79%	6 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #14926   +/-   ##
=========================================
  Coverage          ?   78.22%           
=========================================
  Files             ?     1167           
  Lines             ?   154243           
  Branches          ?        0           
=========================================
  Hits              ?   120656           
  Misses            ?    26155           
  Partials          ?     7432

Flag	Coverage Δ
unittests	`78.22% <68.92%> (?)`

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pedronis · 2025-01-27T13:49:22Z

interfaces/ldconfig/backend.go

+		content += "\n"
+
+		// File name is snap.<snap_name>.<slot_name>.conf
+		ldconfigPath := fmt.Sprintf("snap.%s.conf", key)


based on what I found we want this to be based on the plug snap name I think. OTOH if the plug snap is snapd we could use a special name like snapd.conf or snap.system.conf ?

Changed as discussed, this is ready for review now

alfonsosanchezbeato · 2025-01-27T17:08:30Z

@Meulengracht I've re-asked you for review as there have been a few changes

pedronis

suggestion

pedronis · 2025-01-27T19:10:27Z

interfaces/ldconfig/spec.go

+type Specification struct {
+	// libDirs is the list of directories with libraries coming from
+	// different slots. The key has the form "<snap_name>.<slot>".
+	libDirs map[string][]string


maybe we should use a struct:

type sourceSlot {
Snap string
Slot string
}

for the key? to avoid the string mangling?

Good point, done now

pedronis

thank you

this needs to be re-reviewed

bboozzoo · 2025-01-29T14:13:16Z

interfaces/system.go

+// names for the snap representing the system.
+func IsTheSystemSnap(snapName string) bool {
+	switch snapName {
+	case "", "snapd", "core", "ubuntu-core":


do we still care about "ubuntu-core" snap?

I've removed, thinking about this we already branched for UC16, so we do not need to care (we probably would not care even for UC16 nowadays).

bboozzoo · 2025-01-29T14:31:34Z

interfaces/ldconfig/backend_test.go

+	libcConfPath := filepath.Join(confDir, "libc.conf")
+	c.Assert(os.WriteFile(libcConfPath, []byte{}, 0644), IsNil)
+
+	// Add cakkback and register the interface


Suggested change

// Add cakkback and register the interface

// Add callback and register the interface

bboozzoo · 2025-01-29T15:21:42Z

interfaces/system.go

+// names for the snap representing the system.
+func IsTheSystemSnap(snapName string) bool {
+	switch snapName {
+	case "", "snapd", "core", "ubuntu-core":


we have some code in repo.go in guessSystemSnapName() which maybe could reuse the list.

Changed so at least both share the list of the names of the system snap

bboozzoo · 2025-01-29T15:22:05Z

interfaces/system.go

+
+// IsTheSystemSnap returns true if snapName is one of the possible
+// names for the snap representing the system.
+func IsTheSystemSnap(snapName string) bool {


Suggested change

func IsTheSystemSnap(snapName string) bool {

func IsSystemSnap(snapName string) bool {

I prefer to keep this, it makes clear that there can be only one system snap

bboozzoo · 2025-01-29T15:23:13Z

interfaces/system.go

+// names for the snap representing the system.
+func IsTheSystemSnap(snapName string) bool {
+	switch snapName {
+	case "", "snapd", "core", "ubuntu-core":


is "" for implicit plugs/slots? Aren't those always attached to either core or snapd?

I think so, yes, afaiu IsTheSystemSnap could be called with "".

interfaces/ldconfig/backend.go

alfonsosanchezbeato

@bboozzoo thanks for the review, I've addressed your comments now

interfaces/ldconfig/backend.go

alfonsosanchezbeato · 2025-01-29T16:44:54Z

interfaces/system.go

+
+// IsTheSystemSnap returns true if snapName is one of the possible
+// names for the snap representing the system.
+func IsTheSystemSnap(snapName string) bool {


I prefer to keep this, it makes clear that there can be only one system snap

alfonsosanchezbeato · 2025-01-29T16:49:07Z

interfaces/system.go

+// names for the snap representing the system.
+func IsTheSystemSnap(snapName string) bool {
+	switch snapName {
+	case "", "snapd", "core", "ubuntu-core":


I've removed, thinking about this we already branched for UC16, so we do not need to care (we probably would not care even for UC16 nowadays).

alfonsosanchezbeato · 2025-01-29T17:03:51Z

interfaces/system.go

+// names for the snap representing the system.
+func IsTheSystemSnap(snapName string) bool {
+	switch snapName {
+	case "", "snapd", "core", "ubuntu-core":


Changed so at least both share the list of the names of the system snap

alfonsosanchezbeato · 2025-01-29T17:13:25Z

interfaces/system.go

+// names for the snap representing the system.
+func IsTheSystemSnap(snapName string) bool {
+	switch snapName {
+	case "", "snapd", "core", "ubuntu-core":


I think so, yes, afaiu IsTheSystemSnap could be called with "".

bboozzoo

LGTM

alfonsosanchezbeato · 2025-01-30T22:44:46Z

@pedronis @bboozzoo I had to do some changes here as spread tests were failing - I was being naive on when Setup was being called, which is basically all the time for all backends for any interface change. So I had to make sure that the setup for ldconfig only happened for the system snap, and also minimize the calls to ldconfig, otherwise we try to touch files on UC that cannot be written. Please take a look at 2c7d844

needs re-review

pedronis

I think we want to be a bit more careful with the checks instead

pedronis · 2025-01-31T09:17:07Z

interfaces/ldconfig/spec.go

@@ -87,7 +87,7 @@ func (spec *Specification) AddConnectedPlug(iface interfaces.Interface, plug *in

 // AddConnectedSlot records ldconfig-specific side-effects of having a connected slot.
 func (spec *Specification) AddConnectedSlot(iface interfaces.Interface, plug *interfaces.ConnectedPlug, slot *interfaces.ConnectedSlot) error {
-	if !interfaces.IsTheSystemSnap(plug.Snap().InstanceName()) {
+	if !interfaces.IsTheSystemSnap(slot.Snap().InstanceName()) {


this change seems definitely wrong we probably need to do something else and move the check only if the interface has the definer no?

Also in either case this definitely warrants a comment

Right, the check really needs to happen only for interfaces supporting ldconfig, I've changed this now and reverted to check for the plug here too.

pedronis · 2025-01-31T09:17:38Z

interfaces/ldconfig/backend.go

@@ -52,6 +53,12 @@ func (b *Backend) Name() interfaces.SecuritySystem {
 // If the method fails it should be re-tried (with a sensible strategy) by the caller.
 func (b *Backend) Setup(appSet *interfaces.SnapAppSet, opts interfaces.ConfinementOptions, repo *interfaces.Repository, tm timings.Measurer) error {
 	snapName := appSet.InstanceName()
+	if !interfaces.IsTheSystemSnap(snapName) {


this might be ok for now but we probably want to instead change the rest of the code to work even if this is not here

With the change in the checks in the specification, this check is not needed anymore. But that is sort of by chance as the spec is empty, if it wasn't it would try to write in /etc/ld.so.conf.d/snap.system.conf for a snap that is not the system snap. So I think it is better to leave for the moment just to make clear we do not support normal snaps for the moment.

but there two sides to things, you need to be abel to update things both when the system snap is update, but also when the providing snap with the slot which a kernel or something is updated, so I don't think we can really have this check

changed now

Meulengracht

Also this is a +1 from me if comments are resolved, but not approving since it's awaitings Samuele's approval and you already have maciejs

alfonsosanchezbeato · 2025-01-31T13:44:21Z

@pedronis @Meulengracht this is ready for another round now

pedronis

sorry, another question

pedronis · 2025-01-31T16:27:33Z

interfaces/ldconfig/backend.go

+// This method should be called after removing a snap.
+//
+// If the method fails it should be re-tried (with a sensible strategy) by the caller.
+func (b *Backend) Remove(snapName string) error {


don't we need to do something here to remove the entries for a snap that had ldconfig providing slot?

@alfonsosanchezbeato remarked that a slot side going away is taken care by this logic:

snapd/overlord/ifacestate/handlers.go

Lines 385 to 396 in 81fd929

func (m *InterfaceManager) removeProfilesForSnap(task *state.Task, _ *tomb.Tomb, snapName string, tm timings.Measurer) error {

// Disconnect the snap entirely.

// This is required to remove the snap from the interface repository.

// The returned list of affected snaps will need to have its security setup

// to reflect the change.

affectedSnaps, err := m.repo.DisconnectSnap(snapName)

if err != nil {

return err

}

if err := m.setupAffectedSnaps(task, snapName, affectedSnaps, tm); err != nil {

return err

}

as the plug side will be Setup

this will need a re-review

pedronis

thanks

This backend will expose libraries coming from snaps to either the rootfs or to other snaps (currently supporting only the former).

alfonsosanchezbeato requested review from bboozzoo, pedronis and Meulengracht January 14, 2025 17:57

Meulengracht previously approved these changes Jan 15, 2025

View reviewed changes

pedronis requested changes Jan 16, 2025

View reviewed changes

alfonsosanchezbeato force-pushed the ldconfig-backend branch from df1088b to c5cad2c Compare January 17, 2025 21:56

alfonsosanchezbeato requested a review from pedronis January 17, 2025 22:11

pedronis requested changes Jan 21, 2025

View reviewed changes

alfonsosanchezbeato commented Jan 21, 2025

View reviewed changes

alfonsosanchezbeato requested a review from pedronis January 21, 2025 22:43

pedronis requested changes Jan 27, 2025

View reviewed changes

alfonsosanchezbeato force-pushed the ldconfig-backend branch from 264f7be to 01acd1d Compare January 27, 2025 17:05

alfonsosanchezbeato requested review from pedronis and Meulengracht January 27, 2025 17:07

pedronis reviewed Jan 27, 2025

View reviewed changes

alfonsosanchezbeato requested a review from pedronis January 27, 2025 19:38

pedronis previously approved these changes Jan 28, 2025

View reviewed changes

bboozzoo reviewed Jan 29, 2025

View reviewed changes

alfonsosanchezbeato force-pushed the ldconfig-backend branch from 57e771f to 532b0d9 Compare January 29, 2025 17:15

alfonsosanchezbeato commented Jan 29, 2025

View reviewed changes

alfonsosanchezbeato requested a review from bboozzoo January 29, 2025 17:16

bboozzoo previously approved these changes Jan 30, 2025

View reviewed changes

alfonsosanchezbeato added 2 commits January 30, 2025 09:24

interfaces: add function to check if a snap is a system snap

15ad21f

dirs: add directory where ldconfig configuration lives

8ca67f2

alfonsosanchezbeato force-pushed the ldconfig-backend branch from 532b0d9 to 877cc4f Compare January 30, 2025 14:36

pedronis self-requested a review January 31, 2025 07:49

pedronis requested changes Jan 31, 2025

View reviewed changes

Meulengracht reviewed Jan 31, 2025

View reviewed changes

alfonsosanchezbeato requested review from pedronis and Meulengracht January 31, 2025 13:44

pedronis reviewed Jan 31, 2025

View reviewed changes

pedronis approved these changes Jan 31, 2025

View reviewed changes

alfonsosanchezbeato requested a review from bboozzoo January 31, 2025 17:01

interfaces: add ldconfig backend

e67495c

This backend will expose libraries coming from snaps to either the rootfs or to other snaps (currently supporting only the former).

alfonsosanchezbeato force-pushed the ldconfig-backend branch from 8692ae6 to e67495c Compare January 31, 2025 17:07

Meulengracht approved these changes Feb 3, 2025

View reviewed changes

alfonsosanchezbeato merged commit f2f4d10 into canonical:master Feb 3, 2025
65 of 67 checks passed

	// The snap may have been updated so perform the following operation to
	// ensure that we are always working on the correct state:
	//
	// - disconnect all connections to/from the given snap
	// - remembering the snaps that were affected by this operation
	// - remove the (old) snap from the interfaces repository
	// - add the (new) snap to the interfaces repository
	// - restore connections based on what is kept in the state
	// - if a connection cannot be restored then remove it from the state
	// - setup the security of all the affected snaps

	// plug side
	for _, plugInfo := range r.plugs[snapName] {
	iface := r.ifaces[plugInfo.Interface]
	if err := spec.AddPermanentPlug(iface, plugInfo); err != nil {
	return nil, err
	}
	for _, conn := range r.plugSlots[plugInfo] {
	if err := spec.AddConnectedPlug(iface, conn.Plug, conn.Slot); err != nil {
	return nil, err
	}
	}
	}

	// Add cakkback and register the interface
	// Add callback and register the interface

	func IsTheSystemSnap(snapName string) bool {
	func IsSystemSnap(snapName string) bool {

	func (m InterfaceManager) removeProfilesForSnap(task state.Task, _ *tomb.Tomb, snapName string, tm timings.Measurer) error {
	// Disconnect the snap entirely.
	// This is required to remove the snap from the interface repository.
	// The returned list of affected snaps will need to have its security setup
	// to reflect the change.
	affectedSnaps, err := m.repo.DisconnectSnap(snapName)
	if err != nil {
	return err
	}
	if err := m.setupAffectedSnaps(task, snapName, affectedSnaps, tm); err != nil {
	return err
	}

interfaces,dirs: add ldconfig backend #14926

interfaces,dirs: add ldconfig backend #14926

Conversation

alfonsosanchezbeato commented Jan 14, 2025

github-actions bot commented Jan 14, 2025 • edited Loading

Failures:

Preparing:

Executing:

Restoring:

Meulengracht left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

alfonsosanchezbeato Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 21, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfonsosanchezbeato commented Jan 27, 2025

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboozzoo left a comment

Choose a reason for hiding this comment

alfonsosanchezbeato commented Jan 30, 2025

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Meulengracht left a comment

Choose a reason for hiding this comment

alfonsosanchezbeato commented Jan 31, 2025

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 14, 2025 •

edited

Loading

pedronis Jan 16, 2025 •

edited

Loading

alfonsosanchezbeato Jan 17, 2025 •

edited

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading