Re: [Netconf] YP#12 Value filtering on non-key objects

Hi Rob,

> Hi Eric,
> 
> 
> On 02/03/2018 22:47, Eric Voit (evoit) wrote:
> > Hi Rob,
> >
> >> Hi Eric,
> >>
> >>
> >> On 02/03/2018 17:22, Eric Voit (evoit) wrote:
> >>> Hi Rob,
> >>>
> >>>> Hi Eric,
> >>>>
> >>>>
> >>>> On 02/03/2018 14:44, Eric Voit (evoit) wrote:
> >>>>>> From: Eric Voit, December 22, 2017 2:52 PM
> >>>>>>
> >>>>>> I am looking to see if anyone has an issue with my opening and closing
> of:
> >>>>>> https://github.com/netconf-wg/yang-push/issues/12
> >>>>>> as described below...
> >>>>>>
> >>>>>> During Martin's recent review of yang-push, he argued that we
> >>>>>> should
> >> allow
> >>>>>> selection filtering using non-key properties.   I believe he is correct.  (In
> >> fact,
> >>>>>> this is what was supported in yang-push drafts -v00 through
> >>>>>> -v08.)
> >>>>>>
> >>>>>> The reason we changed in -v09 is that if a non-key property
> >>>>>> changes, it
> >>>> might
> >>>>>> mean that the object as a whole is no longer selected.    The result is
> that
> >> an
> >>>>>> entire subtree may no longer should be visible to the receiver.
> >>>>>> So the YANG patch sent from the subscriber to the receiver would
> >>>>>> need to add/remove entire subtree based on a simple value change
> >>>>>> for a non-key
> >>>> object in that
> >>>>>> subtree.   The thought for -v09 was that if we restricted on-change
> >> selection
> >>>>>> filtering just to keys, this might be simpler/easier to implement.
> >>>>>> But per Martin's comments below, he pointed out other issues
> >>>>>> which come with placing such restrictions in filters.
> >>>>>>
> >>>>>>
> >>>>>> Does anyone have an objections to once again allowing selection
> >>>>>> filters to
> >>>> be
> >>>>>> placed on non-key properties?    (Alex & Andy, this was something we
> >> went
> >>>>>> back-and-forth on as part of the Dezign Team discussions.)
> >>>>>>
> >>>>>> If there is no objection, this makes the conceptual selection
> >>>>>> process for creating an push-change-update:
> >>>>> Above in this thread I proposed closing the issue YP#12
> >>>>> https://github.com/netconf-wg/yang-push/issues/12
> >>>>>
> >>>>> In a recent review, Tim Jenkins privately pointed out a bug in the
> >>>>> process
> >>>> below.   This bug has been fixed in the current -v15.   Basically the process
> >> bug
> >>>> resulted from this thread last year when I forgot to document a
> >>>> step which
> >> has
> >>>> long been included in the draft text.    Specifically the text is from Section
> >> 3.3
> >>>> On-change considerations: "If an object has returned to its
> >>>> original value (or even has been created and then deleted) during
> >>>> the dampening-period, the last change will still be sent.  This
> >>>> will indicate churn is occurring on that object."
> >>>> I can't convince myself that this is really a robust way of
> >>>> indicating
> >>>> churn:
> >>>>
> >>>> - If an implementation retrieves an operational value via polling
> >>>> it may not know whether that value has changed between polls.
> >>> Yes, this is a real issue of polling.
> >>>
> >>>> - If a client ends up loosing the connection, then it may miss out
> >>>> on notification that values have changed.
> >>> Yes.  If the connection is lost, a dynamic subscription ends, so
> >>> there is no
> >> guarantees of change indication.
> >>>> - If you have a chain of controllers, it seems to me more useful to
> >>>> try and suppress duplicate updates then propagating them through
> >>>> the
> >> network.
> >>> Duplicate updates are suppressed with the dampening interval.
> >>>
> >>>> Instead, if it is important to know that a particular value has
> >>>> changed then it may be better to have a separate leaf defining a
> >>>> count of the number of transitions that has occurred (e.g. the
> >>>> number of times an interface has flapped),  or perhaps this could
> >>>> be defined more generically using a metadata annotation, or
> >>>> possibly a timestamp of when the value last changed (e.g. as is
> >>>> recommended in RESTCONF for configuration items).  Basically, any
> >>>> mechanism where the desired information is represented via explicit
> >>>> state rather than implicitly through
> >> the existence of the value in a notification.
> >>> This certainly is possible, and a quantity of flaps could be
> >>> augmented into the
> >> YANG patch definition for YANG-Push.  Or as you note, perhaps a
> >> timestamp with the latest flap could be placed into the YANG Patch
> >> definition.  However earlier discussions on flaps resulting in most
> >> just wanting a notification that a flap occurred.  Such a
> >> notification leaves open existing interfaces to directly query the logs for
> such counts.
> >>> I prefer the simpler definition.
> >> Sorry, but I don't see much real value in it:
> >> - clients cannot robustly infer anything from the duplicate value
> >> other that is the current value at the time of the notification.  In
> >> fact, I think that the proposed text is actually somewhat unhelpful,
> >> since clients will probably think that they can infer something that
> >> they probably shouldn't if they want to be robust to failure scenarios.
> > Consider the ACL security use case in slide 30:
> > https://datatracker.ietf.org/doc/slides-96-netconf-5/1/
> > what is needed is visibility into transient vulnerabilities.  However security
> solutions are not capable of ingesting undampened ACL changes in real time.
> So dampening is needed without losing visibility of churn.
> Yes, no argument that the requirement is a valid one. 

Excellent

>  But I don't see how,
> once dampening is in place, that really solves the problem:  I would think that
> the only way a security solution would be able to determine that there was no
> vulnerable time interval would be by checking every ACL update.

Granular, targeted visibility of specific changes becomes available.  And then further analysis can be kicked off based on seeing a dampened change.  I.e., if there is something unexpected seen, targeted logs can be retrieved to get relevant ACL updates.  

I agree with you that if absolute, immediate validation is needed, a solution should request a dampening period of zero.  And of course there remains the option of pushing periodically if you are not worried about transient exposures.

The ability to support this range of possibilities is a reason the Security Automation and Continuous Monitoring WG added YANG Push on-change to their charter in January:
https://datatracker.ietf.org/doc/charter-ietf-sacm/

> My argument is whether the proposed solution is robust:  If you have an ACL
> update counter (acting as a logical timestamp) that is incremented whenever
> the ACL is changed then I think that there is a trivial solution to know whether
> an ACL has changed between two points in time (noting that it still doesn't tell
> you whether there was any window where there is a potential vulnerability).

There are different types of churning changes which can be made on objects (add/delete nodes, changing values).  These churning changes can be made up/down a tree.  Based on this, extending YANG patch seems a reasonable approach to identify churning changes made since the last update.

I don't dispute that a robust mechanism which augments YANG push to count the number of dampened changes might be constructed.  I would also think that any such a mechanism would *also* naturally be used to count all the changes made, even if the changes don't cancel each other out.  Agree?   If yes, it looks like we are looking for a different and robust type of summary information coming out of the patch operation.  And this is not a bad thing.   (Perhaps someone wants to propose a specific algorithm which can augment the YANG patch operation?)

> I guess that the crux of my concern is that I perceive that it natural for systems
> to want to suppress "no change" events rather than flooding them on to other
> systems, e.g. to handle device restarts where it is likely that the vast majority
> of notified data may be unchanged.  But here you have notifications that are
> expected to always flood onward, because it is the notification message itself
> that has semantic meaning beyond the change in value that is being
> notified.

Yes this makes sense.   But if a publisher makes a promise to a subscriber to inform them of changes to the datastore, then this is what it needs to do.

This is one of the reasons the RFC7923 requirements say:

     A Subscription Service SHOULD support the ability to subscribe to
     updates on-change, i.e., whenever values of subscribed data objects
     change.

     For on-change updates, the Subscription Service MUST support a
     dampening period that needs to be passed before the first or
     subsequent on-change updates are sent.  The dampening period SHOULD
     be configurable as part of the subscription request.

A publisher may certainly expose alternative/new types of subscription if it is unable to meet these bars.

>  Further, I think that generating those notifications robustly is going
> to require a complex implementation.

This can be true.  Different components have various levels of challenge in how to support.  This is why deployable solutions will need ways of indicating which objects support on-change.

> >
> > If a vulnerability is found off-box, more details can be pulled from existing
> logs using existing operations.  This provides a minimal, clean solution.
> For the security solution to be robust it must surely examine every change to
> the ACL.  I can't see how dampening can avoid that requirement in any sort of
> robust way.

It doesn't avoid it.   It just gives an indication to the application where more analysis might be needed.  It is up to the application to determine things which can be safely ignored, and/or make correlations to known configuration operations underway.

> >
> >
> > To build a more robust solution, we would need define YANG structures
> which are capable of summarizing the times and timeframes of different types
> of churning data.  (IPFIX does stuff like this with inter-packet-gap histograms,
> so it is possible.)  We would then need to place those structures into the YANG
> patch.  This would be non-trivial.  I would suggest we get some feedback from
> the existing draft before extending to do more here.
> I'm not suggest doing more now.  I'm suggesting that it could be simplified or
> made optional.

As per above, RFC 7923 defines on-change as "whenever values of subscribed data objects change".   And dampening support is mandatory where on-change is supported.

> It seems to me that this requirement adds a lot of complexity to the
> implementation.  Consider a list with entries that has been
> added/removed.  Presumably the implementation needs to track all of these
> transient list entries after they have gone away, to generate a delete
> notification for something that isn't there.  Similarly for containers that have
> been created and then deleted before a notification is due to be
> generated.  Perhaps there is a easy solution that I'm missing, but thinking
> about this, I think that this is going to be complex/costly to implement.

Yes, sometimes it will be expensive.   But the alternative is having a promise which isn't reliable/meaningful to the subscriber.  I.e., without such a guarantee it places into question the usefulness of the provided data.   And we end up having just another form of periodic reporting.

> >
> >> - servers may not be able to generate it reliably, or it may be
> >> expensive/complicated to do it.  It is no longer enough just to
> >> compare old and new values, instead servers will need to always
> >> maintain additional state to try and generate this notification.
> > What differentiates on-change from periodic is the guarantee to the
> subscriber that it gets to know if a change occurred.  An on-change which
> doesn't actually expose changes is not something we should allow.
> Why not?
> 
> We have exactly this sort of system for suppressing carrier level link flaps
> without propagating the flap to higher layers.  If a flap is short (e.g. perhaps 10
> msecs) then the link flap is suppressed entirely, and is not seen by the rest of
> the system.

Yes, those internally dampened changes are not propagated to the YANG datastore.   And that is what the boundary of measurement is here:  can you see a change in the YANG datastore.

What MUST be avoided is the possibility that someone did a GET to the YANG datastore, and found a value different than seen by a subscriber.  And that subscriber never was given any indication of churn.

> Instead we have a separate counter that reports the number of
> actual carrier transitions that occur.  Clients can monitor the carrier transition
> counter to determine whether flaps in carrier signals are occurring.

That counter may or may not be on-change subscribable.   It is great that some systems have this.

> Another example, would be a operator monitoring a network.  I've heard that
> some ISPs don't page their ops engineers if a link flaps once for a short period
> of time and then automatically recovers. They care a lot more if it is happening
> repeated, or if it doesn't recover.

Such an application is easily built on dampened changes *if* the changes are visible in the YANG datastore.   See slide 29 of:
https://datatracker.ietf.org/doc/slides-96-netconf-5/1/

Such an implementation would be very difficult if (a) dampening were not available, or suppressed, or (b) a solution doesn't have the transitions counter you mention above.

> Both of these systems are operating a dampened on change notification, but
> where the notifications are entirely suppressed if the system has reverted back
> to their original state.

Both seem quite doable with the current on-change YANG patch definition.

Thanks,
Eric

> Thanks,
> Rob
> 
> 
> >
> > (Of course it is ok to reject a subscription for an object which can't support
> dampened on-change for reasons described above.   And periodic is always an
> option.  Another option might be that a publisher suggesting a dampening
> period hint of zero.)
> >
> >> I would actually propose that you write something along the opposite lines:
> >> - servers may generate duplicate notifications even though the value
> >> hasn't changed (e.g. due to the state flapping).
> >> - clients cannot reliably infer anything from a duplicate value.
> > I think you are saying On-change is accomplished via a YANG patch operation.
> I am not sure how duplicates patches would play out here.
> >
> >> Alternatively, on the presumption that you don't want to change this,
> >> then would it make sense to make this optional/conditional in some way?
> > Optional makes it very close to the periodic behavior.   An on-change needs
> to indicate when some change has occurred.
> >
> > As an FYI, for some earlier conversations in this space, check the "Periodic
> Eventing" options within:
> > https://www.ietf.org/mail-archive/web/netconf/current/msg12375.html
> > There are lots of ways this work could be extended.
> >
> > Thanks
> > Eric
> >
> >
> >> Thanks,
> >> Rob
> >>
> >>
> >>>
> >>> Eric
> >>>
> >>>> Thanks,
> >>>> Rob
> >>>>
> >>>>
> >>>>> To fix this bug the following text has been inserted as an
> >>>>> independent step
> >>>> (4)...
> >>>>>>     (1) Just before a change, or at the start of a dampening
> >>>>>> period, evaluate
> >>>> any
> >>>>>> filtering and any access control rules.  The result is a set "A"
> >>>>>> of datastore
> >>>> nodes
> >>>>>> and subtrees.
> >>>>>>
> >>>>>>     (2) Just after a change, or at the end of a dampening period,
> >>>>>> evaluate any filtering and any (possibly new) access control
> >>>>>> rules.  The
> >> result is a set "B"
> >>>> of
> >>>>>> datastore nodes and subtrees.
> >>>>>>
> >>>>>>     (3) Construct a YANG patch record for going from A to B.
> >>>>>     (4) If there were multiple changes made to a subscribed object
> >>>>> between
> >> "A"
> >>>> and "B" which fully canceled each other out, then if access control
> >>>> permits read access, insert into the YANG patch record the last
> >>>> change
> >> made to that object.
> >>>>>>     (5) If the resulting patch record is non-empty, send it to the receiver.
> >>>>> Final note, while writing this email update, I did a text tweak
> >>>>> from step (4)
> >>>> from what is in -v15.  But the intent of the text is the same.  And
> >>>> the process now returns to matching both earlier versions of the
> >>>> draft, and historical documentation provided such as slides 29 & 30
> >>>> from
> >> IETF96.
> >>>>> https://datatracker.ietf.org/doc/slides-96-netconf-5/1/
> >>>>>
> >>>>> Eric
> >>>>>
> >>>>>> Eric
> >>>>>>
> >>>>>>
> >>>>>>> From: Martin Bjorklund,  December 5, 2017 5:03 AM
> >>>>>>>
> >>>>>>>>>>>         the goal is to
> >>>>>>>>>>>         provide equivalent capabilities to what is available with a
> GET.
> >>>>>>>>>>>
> >>>>>>>>>>>      In GET you can filter on "non-key properties".
> >>>>>>>>>>>
> >>>>>>>>>>>      I think you should remove the text about "non-key
> properties".
> >> If
> >>>>>>>>>>>      anything, I think you can allow an implementation to
> >>>>>>>>>>> reject a
> >> filter
> >>>>>>>>>>>      that would be too complex to implement / evaluate (in
> >>>>>>>>>>> fact I
> >> think
> >>>>>>>>>>>      the text already allows a server to reject such
> >>>>>>>>>>> filters.)
> >>>>>>>>>> The issue with non-key properties is how to deal with the
> >>>>>>>>>> creation and deletion of objects when a non-key property goes
> >>>>>>>>>> in/out of the selection.  Others were uncomfortable with
> >>>>>>>>>> things like passing along a patch showing a node was deleted,
> >>>>>>>>>> when in reality the property simply changed from meeting the
> >>>>>>>>>> selection filter to failing the selection filter.  So people
> >>>>>>>>>> wanted to default to the simpler case of key properties for
> selection.
> >>>>>>>>> But you are constraining the entire solution to support just
> >>>>>>>>> this use case, when a simpler unconstrained solution just as
> >>>>>>>>> easily supports this use case, *plus other use cases* - if a
> >>>>>>>>> client does not want these "fake *deletes", they can simply
> >>>>>>>>> construct filters based on keys only, which they would in the
> >>>>>>>>> current
> >> solution anyway.
> >>>>>>>> This is true.
> >>>>>>>>
> >>>>>>>>> Note also that b/c of the last paragraph in section 3.9, a
> >>>>>>>>> client must deal with such deletes anyway, even if it
> >>>>>>>>> specificied only keys.
> >>>>>>>> This is true.
> >>>>>>>>
> >>>>>>>>> But I do understand your point.  This is also related to my
> >>>>>>>>> comment below about when a filter is evaluated.  Based on you
> >>>>>>>>> reply to that comment, I think the conceptual algorithm is like this:
> >>>>>>>>>
> >>>>>>>>>      Just before a change, evaluate the filter and any access control
> >>>>>>>>>      rules.  The result is a set "A" of nodes (including subnodes).
> >>>>>>>>>
> >>>>>>>>>      Just after a change, evaluate the filter and any (possibly new)
> >>>>>>>>>      access control rules.  The result is a set "B" of nodes (including
> >>>>>>>>>      subnodes).
> >>>>>>>>>
> >>>>>>>>>      Construct a YANG patch record for going from A to B.   If the
> record
> >>>>>>>>>      is non-empty, send it to the subscriber.
> >>>>>>>>>
> >>>>>>>>> (this is conceptual, an implementation can do lots of
> >>>>>>>>> optimizations to avoid evaluating filters on un-related
> >>>>>>>>> changes)
> >>>>>>>> Yes, this is it.  The only tweak I would make is to add the
> >>>>>>>> dampening period.  As a result, I have added the following text
> >>>>>>>> at the end of the "On-Change Considerations" section
> >>>>>>>>
> >>>>>>>> "Putting it all together, following is the conceptual process
> >>>>>>>> for creating an push-change-update notification:
> >>>>>>>>
> >>>>>>>>      1.Just before a change, or at the start of a dampening period,
> >>>>>>>>      evaluate any filtering and any access control rules.  The result is a
> >>>>>>>>      set "A" of datastore nodes and subtrees.
> >>>>>>>>
> >>>>>>>>      2.Just after a change, or at the end of a dampening period,
> evaluate
> >>>>>>>>      any filtering and any (possibly new) access control rules.  The
> result
> >>>>>>>>      is a set "B" of datastore nodes and subtrees.
> >>>>>>>>
> >>>>>>>>      3. Construct a YANG patch record for going from A to B.  If the
> record
> >>>>>>>>      is non-empty, send it to the receiver."
> >>>>>>>>
> >>>>>>>>>> Based on that I can delete the words "but the goal is to
> >>>>>>>>>> provide equivalent capabilities to what is available with a GET".
> >>>>>>>>>>
> >>>>>>>>>>>      With the current text, is this filter ok:
> >>>>>>>>>>>
> >>>>>>>>>>>         /interfaces/interface[contains(name, "eth")]
> >>>>>>>>>>>
> >>>>>>>>>>>      It filters on a key, so it should be ok, right?
> >>>>>>>>>> Yes
> >>>>>>>>> What about this one:
> >>>>>>>>>
> >>>>>>>>>       /interfaces/interface[contains(name, //name)]
> >>>>>>>>>
> >>>>>>>>> it also filters on a key is it ok?
> >>>>>>>>>
> >>>>>>>>> [if the answer is "yes" we have a problem; and if it is "no",
> >>>>>>>>> we need to tighten the text]
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> All this makes me wonder if it is correct to have "subtree"
> >>>>>>>>> and "xpath"
> >>>>>>>>> filters at all.  If they only can match on keys, they become
> >>>>>>>>> severely constrained.
> >>>>>>>>>
> >>>>>>>>> An alternative could be to specify filters as
> >>>>>>>>> "nacm:node-instance-identifier"
> >>>>>>>>> instead; these are instance-identitifiers that allow missing keys.
> >>>>>>>>> For
> >>>>>>>>> example:
> >>>>>>>>>
> >>>>>>>>>      /if:interfaces/if:interface/if:oper-status
> >>>>>>>>>
> >>>>>>>>> This would be easier to understand and probably easier to
> >>>>>>>>> optimize for in the server code.
> >>>>>>>> I want to generalize this "only-keys?" issue for the full WG.
> >>>>>>>> It is too important to handle this deep in the thread.  Look
> >>>>>>>> for me to open an issue shortly.
> >>>>>>> Ok!
> >>>>>>>
> >>>>>>> /martin
> >>>>> _______________________________________________
> >>>>> Netconf mailing list
> >>>>> Netconf@ietf.org
> >>>>> https://www.ietf.org/mailman/listinfo/netconf
> >>>>> .
> >>>>>