Defining the behavior of dynamic attributes (interface hooks)

snapd
#1

Caveat: this topic is for a work-in-progress feature, part of which is implemented with https://github.com/snapcore/snapd/pull/3120

I’d like to gather feedback and discuss potential pitfalls / use cases regarding attributes of interface hooks.

Currently all interfaces implement these two methods:

  • SanitizePlug(plug)
  • SanitizeSlot(slot)

The role of these methods is to ensure all the attributes of a plug/slot from snap’s yaml file are valid and not missing - an early validation. Sanitization is performed on snap installation, without even connecting the interface.

With interface hooks we will get two new methods called when interface hooks get executed and interface is about to be connected:

  • ValidatePlug(plug, extraPlugAttributes)
  • ValidateSlot(slot, extraSlotAttributes)

The role of these methods is to validate any extra attributes set at runtime with snapctl when interface hooks are run. I think these methods should be optional, because not every interface is interested in supporting attributes set at runtime. However, if these methods are missing but hooks are present and set any extra attributes, we will not block them and they will be visible to the interface code. Also note, that with the restrictions discussed so far and implementation in the above PR, interface hooks will not be allowed to modify existing attributes coming from the snap yaml file or provided by Sanitization step - snapctl set will reject such attempts.

With all the above, the rules of where default values should be set may be a bit unclear as we will have two places where interface attributes can be added/modified. With the restriction of not allowing modifications to plug/slot attributes after Sanitize step it looks like the best place to fill in default values is in Validate* methods.
What do you think?

1 Like
The snapd roadmap
#2

I think doing default value handling in and only in Validate* seems viable, the problem is a bit that Validate* as a name doesn’t convey a lot that they can mutate extra*Attributes

#3

With the discussions we had so far in the last two weeks (IRC, hangouts) it became apparent, that the API that’s already in master and that I started using in interface hooks implementation in PR 3120 is confusing and needs changing. I’m currently pondering about three aspects of it:

  1. The Validate* and Connected methods receive both the plug/slot object and slot / plug attributes map respectively, which is a superset of initial attributes (after Sanitize*) and attributes added (or modified if we allow that) during the execution of hooks. This API is confusing, because it’s not obvious if interface code should be using attrs exposed via plug/slot object, or the ones from the atributes map - here is an example signature of one of affected methods of interfaces:
func (iface *hidrawInterface) AppArmorConnectedPlug(
        spec *apparmor.Specification,
        plug *interfaces.Plug, plugAttrs map[string]interface{},
        slot *interfaces.Slot, slotAttrs map[string]interface{}) error

  1. Per above definition, interface methods can currently modify atributes of plug/slot objects. They can also modify plugAttrs / slotAttrs maps they receive. The API should be structured in a way which makes it apparent when modifications are allowed, for example Plug and Slot could be wrapped to give read-only copy of attributes in Connected methods, and to provide transparent access to all attributes (including the ones coming from hooks).

  2. Since Validate* methods should be able to modify attributes , they may need a better name.

  3. I think we should allow snapctl to modify attributes coming from snap yaml, so that the yaml file can provide defaults for all attributes and they can be overwritten by hooks. Of course all attributes will be subject to assertions check at the end. However, it’s not apparent what should happen when attributes are changed and they were already used for permanent snippets.

The topic is open for comments.

#4

I think our original idea of preventing modifications to values that were set statically into the snap yaml still makes some sense. We got in trouble when we mixed that with the idea of introducing defaults via Sanitize, and then preventing changes to those values. That’s indeed problematic because a default is just that… a default. But something that was set explicitly into the yaml doesn’t feel like a default. If it is dynamic, then why pretending its value is well known?

So, responding to the points:

  1. I suggest turning the Attrs field into these methods:
  • StaticAttr reads the static value of an attribute; returns the value defined in snap.yaml.
  • StaticAttrs returns a full copy of the static attributes.
  • Attr returns a copy of an attribute; errors unless dynamic attributes were initialized.
  • Attrs returns a full copy of the dynamic attributes; errors unless dynamic attributes were initialized.
  • SetAttr changes the dynamic value of an attribute; errors unless dynamic attributes were initialized.

  1. How about BeforePrepare and BeforeConnect for the method names? This gives a proper hint of the lifecycle, instead of expecting people to remember how things fit together.

  2. I don’t think we should encourage people to hold defaults in the yaml. If the attribute will be decided at runtime, it’s better to not have it listed than to have a misleading hint pretending it’s being set to something specific. The method set above also means it’ll be easy to prevent snapctl from changing statically defined values, while allowing it to change defaults that were set in BeforePrepare.

#5

agreed

well Plug and Slot are shared between all connections, while dynamic attrs are per connection, so we also need to think on what those methods appear

#6

Yes. I think it makes sense to wrap plug/slot data into a transient short-lived object, just for the interfaces API (created before we call into interface methods, discarded afterwards); these methods would be exposed by this wrapper object. The outside representation would remain intact (i.e. dynamic attributes stored separetly in conns as proposed in the PR).

Yes, I like that. That’s pretty much self-explanatory and much better than Validate*.

#7

This topic was discussed further in a hangout and the following conclusion was reached with regard to the API changes and semantics on connect:

  • SanitizePlug and SanitizeSlot methods of the interfaces will be renamed to BeforePreparePlug and BeforePrepareSlot to better reflect their place in the flow.

  • The new API methods executed after prepare-plug-* and prepare-slot-* hooks and before connection is made (before the security is lifted) will be named AfterPreparePlug and AfterPrepareSlot (they were named ValidatePlug and ValidateSlot in previous discussions).

  • The complete sequence looks as follows (every step is only executed after previous one finishes):

  1. BeforePrepareSlot and BeforePreparePlug methods of interfaces called when snap gets installed.
  2. On connect: prepare-plug-* hook executed.
  3. prepare-slot-* hook executed.
  4. connect task executed: calls to AfterPreparePlug and AfterPrepareSlot methods of the interface called and
    assertions checked before making the connection alive.
  5. connect-slot-* and connect-plug-* hooks executed.

  • The prepare-slot-* and prepare-plug-* hooks can set own dynamic attributes and read static and dynamic attributes of “the other end”, however the prepare-plug-* will only have access to own attributes and static attributes of the slot, since it’s executed first in the sequence, so no dynamic attributes for the slot exist yet. Hooks are not allowed to modify static attributes.

  • The connect-slot-* and connect-plug-* hooks will have read-only access to all static/dynamic attributes of the connection. No modifications will be allowed.

  • The above hooks will read or modify attributes via snapctl (new synax added for interface hooks), e.g.
    snapctl set :bar target=slottarget snapctl get :bar target

  • All the BeforePrepare* and AfterPrepare* methods will receive respective plug/slot attributes (static ones coming from snap.yaml and dynamic ones set by prepare* hooks wrapped in Attrs objects.

  • The BeforePrepareSlot and BeforePreparePlug methods are executed when snap is installed, they are free to sanitize and modify “static” attributes coming from the snap.yaml file via Attrs API.

  • The AfterPrepareSlot and AfterPreparePlug methods can add/modify dynamic attributes via Attrs API. They could also modify static attributes at this point (the API will not prevent this), but this will be considered a programming error.

  • The connection assertions will use Attrs API as well for consistency.

The Attrs API will offers the following methods:

  • StaticAttr - reads the static value of an attribute; returns the value defined in snap.yaml
  • SetStaticAttr - sets value of a static attribute.
  • StaticAttrs - returns a full copy of the static attributes.
  • Attr - returns dynamic attribute with given name if present; otherwise falls back to a static attribute (errors if missing).
  • Attrs - returns a full copy of the dynamic attributes; errors unless dynamic attributes were initialized.
  • SetAttr - changes the dynamic value of an attribute; errors unless dynamic attributes were initialized
#8

Thanks for posting this, Pawel. It describes the agreements well. A few minor points:

  • The “Attrs API” needs a better name. :slight_smile: (and let’s please not use that as a type name).
  • The sentence “The connection assertions will Attrs API” is missing something (will what?)
  • The formatting was a bit off and I tried to fix it. Please see the diff to make sure I preserved your intentions.

Thanks again!

#10

As discussed in the hangout, the proposed (working) names for “Attrs” part is PlugData and SlotData - these types wrap Plug and Slot (they have getters for Name, Snap, Interface etc) and offer the API to access and manipulate attributes as outlined above.