Revert "overlord: update static attrs when reloading connections"revert-lp-1825883

This reverts commit e1bddc9d74fe5c08c7dd2c23f1312a6e31b1b25c. 

13 files changed, 15 insertions, 341 deletions

diff --git a/overlord/ifacestate/helpers.go b/overlord/ifacestate/helpers.go
index a327c0c2df..67044feaab 100644
--- a/overlord/ifacestate/helpers.go
+++ b/overlord/ifacestate/helpers.go
@@ -296,64 +296,34 @@ func (m *InterfaceManager) reloadConnections(snapName string) ([]string, error)
	if err != nil {
	return nil, err
	}
-
-	connStateChanged := false
	affected := make(map[string]bool)
-	for connId, connState := range conns {
-	// Skip entries that just mark a connection as undesired. Those don't
-	// carry attributes that can go stale. In the same spirit, skip
-	// information about hotplug connections that don't have the associated
-	// hotplug hardware.
-	if connState.Undesired || connState.HotplugGone {
+	for id, conn := range conns {
+	if conn.Undesired || conn.HotplugGone {
	continue
	}
-	connRef, err := interfaces.ParseConnRef(connId)
+	connRef, err := interfaces.ParseConnRef(id)
	if err != nil {
	return nil, err
	}
-	// Apply filtering, this allows us to reload only a subset of
-	// connections (and similarly, refresh the static attributes of only a
-	// subset of connections).
	if snapName != "" && connRef.PlugRef.Snap != snapName && connRef.SlotRef.Snap != snapName {
	continue
	}
 
-	// Some versions of snapd may have left stray connections that don't
-	// have the corresponding plug or slot anymore. Before we choose how to
-	// deal with this data we want to silently ignore that error not to
-	// worry the users.
-	plugInfo := m.repo.Plug(connRef.PlugRef.Snap, connRef.PlugRef.Name)
-	slotInfo := m.repo.Slot(connRef.SlotRef.Snap, connRef.SlotRef.Name)
-	if plugInfo == nil || slotInfo == nil {
-	continue
-	}
-	// XXX: Refresh the copy of the static connection attributes. This is a
-	// partial solution to https://bugs.launchpad.net/snapd/+bug/1825883
-	// Once interface hooks are invoked for refreshed snaps then the update
-	// of static and dynamic connection attributes should happen there.
-	updatedStaticPlugAttrs := utils.NormalizeInterfaceAttributes(plugInfo.Attrs).(map[string]interface{})
-	updatedStaticSlotAttrs := utils.NormalizeInterfaceAttributes(slotInfo.Attrs).(map[string]interface{})
-
	// Note: reloaded connections are not checked against policy again, and also we don't call BeforeConnect* methods on them.
-	if _, err := m.repo.Connect(connRef, updatedStaticPlugAttrs, connState.DynamicPlugAttrs, updatedStaticSlotAttrs, connState.DynamicSlotAttrs, nil); err != nil {
+	if _, err := m.repo.Connect(connRef, conn.StaticPlugAttrs, conn.DynamicPlugAttrs, conn.StaticSlotAttrs, conn.DynamicSlotAttrs, nil); err != nil {
+	if _, ok := err.(*interfaces.UnknownPlugSlotError); ok {
+	// Some versions of snapd may have left stray connections that
+	// don't have the corresponding plug or slot anymore. Before we
+	// choose how to deal with this data we want to silently ignore
+	// that error not to worry the users.
+	continue
+	}
	logger.Noticef("%s", err)
	} else {
-	// If the connection succeeded update the connection state and keep
-	// track of the snaps that were affected.
	affected[connRef.PlugRef.Snap] = true
	affected[connRef.SlotRef.Snap] = true
-	// XXX: ideally we'd know the revision associated with the
-	// attributes _or_ did a deep comparison but for the sake of
-	// simplicity this is not done and attributes are always refreshed.
-	connState.StaticPlugAttrs = updatedStaticPlugAttrs
-	connState.StaticSlotAttrs = updatedStaticSlotAttrs
-	connStateChanged = true
	}
	}
-	if connStateChanged {
-	setConns(m.state, conns)
-	}
-
	result := make([]string, 0, len(affected))
	for name := range affected {
	result = append(result, name)
diff --git a/overlord/ifacestate/ifacestate_test.go b/overlord/ifacestate/ifacestate_test.go
index 63037fe923..27c05aa5c8 100644
--- a/overlord/ifacestate/ifacestate_test.go
+++ b/overlord/ifacestate/ifacestate_test.go
@@ -1594,10 +1594,6 @@ func (s *interfaceManagerSuite) TestStaleConnectionsRemoved(c *C) {
	c.Assert(ifaces.Connections, HasLen, 0)
 }
 
-func (s *interfaceManagerSuite) mockSecBackend(c *C, backend interfaces.SecurityBackend) {
-	s.extraBackends = append(s.extraBackends, backend)
-}
-
 func (s *interfaceManagerSuite) mockIface(c *C, iface interfaces.Interface) {
	s.extraIfaces = append(s.extraIfaces, iface)
 }
@@ -2377,221 +2373,6 @@ func (s *interfaceManagerSuite) TestDoSetupSnapSecurityKeepsExistingConnectionSt
	})
 }
 
-func (s *interfaceManagerSuite) TestReloadingConnectionsOnStartupUpdatesStaticAttributes(c *C) {
-	// Put a connection in the state. The connection binds the two snaps we are
-	// adding below. The connection contains a copy of the static attributes
-	// but refers to the "old" values, in contrast to what the snaps define.
-	s.state.Lock()
-	s.state.Set("conns", map[string]interface{}{
-	"consumer:plug producer:slot": map[string]interface{}{
-	"interface": "test",
-	"plug-static": map[string]interface{}{"attr": "old-plug-attr"},
-	"slot-static": map[string]interface{}{"attr": "old-slot-attr"},
-	},
-	})
-	s.state.Unlock()
-
-	// Add consumer and producer snaps, with a plug and slot respectively, each
-	// carrying a single attribute with a "new" value. The "new" value is in
-	// contrast to the old value in the connection state.
-	const consumerYaml = `
-name: consumer
-version: 1
-plugs:
- plug:
- interface: test
- attr: new-plug-attr
-`
-	const producerYaml = `
-name: producer
-version: 1
-slots:
- slot:
- interface: test
- attr: new-slot-attr
-`
-	s.mockSnap(c, producerYaml)
-	s.mockSnap(c, consumerYaml)
-
-	// Create a connection reference, it's just verbose and used a few times
-	// below so it's put up here.
-	connRef := &interfaces.ConnRef{
-	PlugRef: interfaces.PlugRef{Snap: "consumer", Name: "plug"},
-	SlotRef: interfaces.SlotRef{Snap: "producer", Name: "slot"}}
-
-	// Add a test security backend and a test interface. We want to use them to
-	// observe the interaction with the security backend and to allow the
-	// interface manager to keep the test plug and slot of the consumer and
-	// producer snaps we introduce below.
-	secBackend := &ifacetest.TestSecurityBackend{
-	BackendName: "test",
-	SetupCallback: func(snapInfo *snap.Info, opts interfaces.ConfinementOptions, repo *interfaces.Repository) error {
-	// Whenever this function is invoked to setup security for a snap
-	// we check the connection attributes that it would act upon.
-	// Because of how connection state is refreshed we never expect to
-	// see the old attribute values.
-	conn, err := repo.Connection(connRef)
-	c.Assert(err, IsNil)
-	c.Check(conn.Plug.StaticAttrs(), DeepEquals, map[string]interface{}{"attr": "new-plug-attr"})
-	c.Check(conn.Slot.StaticAttrs(), DeepEquals, map[string]interface{}{"attr": "new-slot-attr"})
-	return nil
-	},
-	}
-	s.mockSecBackend(c, secBackend)
-	s.mockIfaces(c, &ifacetest.TestInterface{InterfaceName: "test"})
-
-	// Create the interface manager. This indirectly adds the snaps to the
-	// repository and re-connects them using the stored connection information.
-	mgr := s.manager(c)
-
-	// Inspect the repository connection data. The data no longer refers to the
-	// old connection attributes because they were updated when the connections
-	// were reloaded from the state.
-	repo := mgr.Repository()
-	conn, err := repo.Connection(connRef)
-	c.Assert(err, IsNil)
-	c.Check(conn.Plug.StaticAttrs(), DeepEquals, map[string]interface{}{"attr": "new-plug-attr"})
-	c.Check(conn.Slot.StaticAttrs(), DeepEquals, map[string]interface{}{"attr": "new-slot-attr"})
-
-	// Because of the fact that during testing the system key always
-	// mismatches, the security setup is performed.
-	c.Check(secBackend.SetupCalls, HasLen, 2)
-}
-
-// LP:#1825883; make sure static attributes in conns state are updated from the snap yaml on snap refresh
-func (s *interfaceManagerSuite) testDoSetupProfilesUpdatesStaticAttributes(c *C, snapNameToSetup string) {
-	// Put a connection in the state. The connection binds the two snaps we are
-	// adding below. The connection reflects the snaps as they are now, and
-	// carries no attribute data.
-	s.state.Lock()
-	s.state.Set("conns", map[string]interface{}{
-	"consumer:plug producer:slot": map[string]interface{}{
-	"interface": "test",
-	},
-	})
-	s.state.Unlock()
-
-	// Add a pair of snap versions for producer and consumer snaps, with a plug
-	// and slot respectively. The second version producer and consumer snaps
-	// where the interfaces carry additional attributes.
-	const consumerV1Yaml = `
-name: consumer
-version: 1
-plugs:
- plug:
- interface: test
-`
-	const producerV1Yaml = `
-name: producer
-version: 1
-slots:
- slot:
- interface: test
-`
-	const consumerV2Yaml = `
-name: consumer
-version: 2
-plugs:
- plug:
- interface: test
- attr: plug-value
-`
-	const producerV2Yaml = `
-name: producer
-version: 2
-slots:
- slot:
- interface: test
- attr: slot-value
-`
-	// NOTE: s.mockSnap sets the state and calls MockSnapInstance internally,
-	// which puts the snap on disk. This gives us all four YAMLs on disk and
-	// just the first version of both in the state.
-	s.mockSnap(c, producerV1Yaml)
-	s.mockSnap(c, consumerV1Yaml)
-	snaptest.MockSnapInstance(c, "", consumerV2Yaml, &snap.SideInfo{Revision: snap.R(2)})
-	snaptest.MockSnapInstance(c, "", producerV2Yaml, &snap.SideInfo{Revision: snap.R(2)})
-
-	// Create a connection reference, it's just verbose and used a few times
-	// below so it's put up here.
-	connRef := &interfaces.ConnRef{
-	PlugRef: interfaces.PlugRef{Snap: "consumer", Name: "plug"},
-	SlotRef: interfaces.SlotRef{Snap: "producer", Name: "slot"}}
-
-	// Add a test security backend and a test interface. We want to use them to
-	// observe the interaction with the security backend and to allow the
-	// interface manager to keep the test plug and slot of the consumer and
-	// producer snaps we introduce below.
-	secBackend := &ifacetest.TestSecurityBackend{
-	BackendName: "test",
-	SetupCallback: func(snapInfo *snap.Info, opts interfaces.ConfinementOptions, repo *interfaces.Repository) error {
-	// Whenever this function is invoked to setup security for a snap
-	// we check the connection attributes that it would act upon.
-	// Those attributes should always match those of the snap version.
-	conn, err := repo.Connection(connRef)
-	c.Assert(err, IsNil)
-	switch snapInfo.Version {
-	case "1":
-	c.Check(conn.Plug.StaticAttrs(), DeepEquals, map[string]interface{}{})
-	c.Check(conn.Slot.StaticAttrs(), DeepEquals, map[string]interface{}{})
-	case "2":
-	switch snapNameToSetup {
-	case "consumer":
-	// When the consumer has security setup the consumer's plug attribute is updated.
-	c.Check(conn.Plug.StaticAttrs(), DeepEquals, map[string]interface{}{"attr": "plug-value"})
-	c.Check(conn.Slot.StaticAttrs(), DeepEquals, map[string]interface{}{})
-	case "producer":
-	// When the producer has security setup the producer's slot attribute is updated.
-	c.Check(conn.Plug.StaticAttrs(), DeepEquals, map[string]interface{}{})
-	c.Check(conn.Slot.StaticAttrs(), DeepEquals, map[string]interface{}{"attr": "slot-value"})
-	}
-	}
-	return nil
-	},
-	}
-	s.mockSecBackend(c, secBackend)
-	s.mockIfaces(c, &ifacetest.TestInterface{InterfaceName: "test"})
-
-	// Create the interface manager. This indirectly adds the snaps to the
-	// repository and reloads the connection.
-	s.manager(c)
-
-	// Alter the state of producer and consumer snaps to get new revisions.
-	s.state.Lock()
-	for _, snapName := range []string{"producer", "consumer"} {
-	snapstate.Set(s.state, snapName, &snapstate.SnapState{
-	Active: true,
-	Sequence: []*snap.SideInfo{{Revision: snap.R(1)}, {Revision: snap.R(2)}},
-	Current: snap.R(2),
-	SnapType: string("app"),
-	})
-	}
-	s.state.Unlock()
-
-	// Setup profiles for the given snap, either consumer or producer.
-	s.state.Lock()
-	change := s.state.NewChange("test", "")
-	task := s.state.NewTask("setup-profiles", "")
-	task.Set("snap-setup", &snapstate.SnapSetup{
-	SideInfo: &snap.SideInfo{RealName: snapNameToSetup, Revision: snap.R(2)}})
-	change.AddTask(task)
-	s.state.Unlock()
-
-	// Spin the wheels to run the tasks we added.
-	s.settle(c)
-	s.state.Lock()
-	defer s.state.Unlock()
-	c.Assert(change.Status(), Equals, state.DoneStatus)
-}
-
-func (s *interfaceManagerSuite) TestDoSetupProfilesUpdatesStaticAttributesPlugSnap(c *C) {
-	s.testDoSetupProfilesUpdatesStaticAttributes(c, "consumer")
-}
-
-func (s *interfaceManagerSuite) TestDoSetupProfilesUpdatesStaticAttributesSlotSnap(c *C) {
-	s.testDoSetupProfilesUpdatesStaticAttributes(c, "producer")
-}
-
 func (s *interfaceManagerSuite) TestDoSetupSnapSecurityIgnoresStrayConnection(c *C) {
	// Add an OS snap
	snapInfo := s.mockSnap(c, ubuntuCoreSnapYaml)
@@ -3332,7 +3113,6 @@ plugs:
	c.Check(conns, DeepEquals, map[string]interface{}{
	"consumer:plug core:hotplug-slot": map[string]interface{}{
	"interface": "test",
-	"plug-static": map[string]interface{}{"attr": "plug-attr"},
	"hotplug-gone": true,
	},
	"consumer:plug core:slot2": map[string]interface{}{
@@ -3390,11 +3170,13 @@ slots:
	c.Assert(err, IsNil)
	c.Assert(conn.Plug.Name(), Equals, "plug")
	c.Assert(conn.Plug.StaticAttrs(), DeepEquals, map[string]interface{}{
-	"attr": "plug-value",
+	"other-attr": "irrelevant-value",
+	"attr": "stored-plug-value",
	})
	c.Assert(conn.Slot.Name(), Equals, "slot")
	c.Assert(conn.Slot.StaticAttrs(), DeepEquals, map[string]interface{}{
-	"attr": "slot-value",
+	"other-attr": "irrelevant-value",
+	"attr": "stored-slot-value",
	})
 }
 
diff --git a/tests/regression/lp-1825883/task.yaml b/tests/regression/lp-1825883/task.yaml
deleted file mode 100644
index 607f0c61d4..0000000000
--- a/tests/regression/lp-1825883/task.yaml
+++ /dev/null
@@ -1,33 +0,0 @@
-summary: Ensure that content meta-data is updated on refresh 
-details: |
- When a snap providing content is refreshed the peers of that snap would see
- stale information about the provided content.
-prepare: |
- snap pack test-snapd-app
- snap pack test-snapd-content.v1
- snap pack test-snapd-content.v2
-restore: |
- snap remove test-snapd-app
- snap remove test-snapd-content
- rm -f test-snapd-content-app_1_all.snap test-snapd-content-{1,2}_all.snap
-execute: |
- snap install --dangerous test-snapd-app_1_all.snap
- snap install --dangerous test-snapd-content_1_all.snap
- snap connect test-snapd-app:things test-snapd-content:things
-
- # Inspect the things that are available. We should see A and B now.
- #shellcheck disable=SC2016
- test-snapd-app.sh -c 'cat $SNAP/things/*/thing' | MATCH THING-A
- #shellcheck disable=SC2016
- test-snapd-app.sh -c 'cat $SNAP/things/*/thing' | MATCH THING-B
- #shellcheck disable=SC2016
- test-snapd-app.sh -c 'cat $SNAP/things/*/thing' | MATCH -v THING-C
-
- # Install the 2nd version of the content snap, it should also provide THING-C
- snap install --dangerous test-snapd-content_2_all.snap
- #shellcheck disable=SC2016
- test-snapd-app.sh -c 'cat $SNAP/things/*/thing' | MATCH THING-A
- #shellcheck disable=SC2016
- test-snapd-app.sh -c 'cat $SNAP/things/*/thing' | MATCH THING-B
- #shellcheck disable=SC2016
- test-snapd-app.sh -c 'cat $SNAP/things/*/thing' | MATCH THING-C
diff --git a/tests/regression/lp-1825883/test-snapd-app/bin/sh b/tests/regression/lp-1825883/test-snapd-app/bin/sh
deleted file mode 100755
index d32c6d8994..0000000000
--- a/tests/regression/lp-1825883/test-snapd-app/bin/sh
+++ /dev/null
@@ -1,2 +0,0 @@
-#!/bin/sh
-exec /bin/sh "$@"
diff --git a/tests/regression/lp-1825883/test-snapd-app/meta/snap.yaml b/tests/regression/lp-1825883/test-snapd-app/meta/snap.yaml
deleted file mode 100644
index 6fd5bc69de..0000000000
--- a/tests/regression/lp-1825883/test-snapd-app/meta/snap.yaml
+++ /dev/null
@@ -1,13 +0,0 @@
-name: test-snapd-app
-version: 1
-summary: Snap using the content snap for shared resources
-plugs:
- things:
- interface: content 
- # NOTE: in the original bug the target directory was $SNAP/data-dir/themes
- # and the $SNAP/data-dir directory was not present in the application snap.
- target: $SNAP/things
-architectures: [all]
-apps:
- sh:
- command: bin/sh
diff --git a/tests/regression/lp-1825883/test-snapd-app/things/README b/tests/regression/lp-1825883/test-snapd-app/things/README
deleted file mode 100644
index 7eb6b2a05c..0000000000
--- a/tests/regression/lp-1825883/test-snapd-app/things/README
+++ /dev/null
@@ -1,2 +0,0 @@
-This directory exists in the snap to avert a bug related to writable mimic
-behavior during refresh.
diff --git a/tests/regression/lp-1825883/test-snapd-content.v1/meta/snap.yaml b/tests/regression/lp-1825883/test-snapd-content.v1/meta/snap.yaml
deleted file mode 100644
index 0d29ce8c65..0000000000
--- a/tests/regression/lp-1825883/test-snapd-content.v1/meta/snap.yaml
+++ /dev/null
@@ -1,11 +0,0 @@
-name: test-snapd-content
-version: 1
-summary: Snap providing shared things
-architectures: [all]
-slots:
- things:
- interface: content
- source:
- read:
- - $SNAP/things/a
- - $SNAP/things/b
diff --git a/tests/regression/lp-1825883/test-snapd-content.v1/things/a/thing b/tests/regression/lp-1825883/test-snapd-content.v1/things/a/thing
deleted file mode 100644
index 833adb1dc9..0000000000
--- a/tests/regression/lp-1825883/test-snapd-content.v1/things/a/thing
+++ /dev/null
@@ -1 +0,0 @@
-THING-A
diff --git a/tests/regression/lp-1825883/test-snapd-content.v1/things/b/thing b/tests/regression/lp-1825883/test-snapd-content.v1/things/b/thing
deleted file mode 100644
index 5ff6853fad..0000000000
--- a/tests/regression/lp-1825883/test-snapd-content.v1/things/b/thing
+++ /dev/null
@@ -1 +0,0 @@
-THING-B
diff --git a/tests/regression/lp-1825883/test-snapd-content.v2/meta/snap.yaml b/tests/regression/lp-1825883/test-snapd-content.v2/meta/snap.yaml
deleted file mode 100644
index 0e21b603fd..0000000000
--- a/tests/regression/lp-1825883/test-snapd-content.v2/meta/snap.yaml
+++ /dev/null
@@ -1,12 +0,0 @@
-name: test-snapd-content
-version: 2
-summary: Snap providing shared things
-architectures: [all]
-slots:
- things:
- interface: content
- source:
- read:
- - $SNAP/things/a
- - $SNAP/things/b
- - $SNAP/things/c
diff --git a/tests/regression/lp-1825883/test-snapd-content.v2/things/a/thing b/tests/regression/lp-1825883/test-snapd-content.v2/things/a/thing
deleted file mode 100644
index 833adb1dc9..0000000000
--- a/tests/regression/lp-1825883/test-snapd-content.v2/things/a/thing
+++ /dev/null
@@ -1 +0,0 @@
-THING-A
diff --git a/tests/regression/lp-1825883/test-snapd-content.v2/things/b/thing b/tests/regression/lp-1825883/test-snapd-content.v2/things/b/thing
deleted file mode 100644
index 5ff6853fad..0000000000
--- a/tests/regression/lp-1825883/test-snapd-content.v2/things/b/thing
+++ /dev/null
@@ -1 +0,0 @@
-THING-B
diff --git a/tests/regression/lp-1825883/test-snapd-content.v2/things/c/thing b/tests/regression/lp-1825883/test-snapd-content.v2/things/c/thing
deleted file mode 100644
index 0e910a4936..0000000000
--- a/tests/regression/lp-1825883/test-snapd-content.v2/things/c/thing
+++ /dev/null
@@ -1 +0,0 @@
-THING-C