Telechat Review of draft-ietf-anima-grasp-api-08
review-ietf-anima-grasp-api-08-genart-telechat-kyzivat-2020-11-30-00

I am the assigned Gen-ART reviewer for this draft. The General Area 
Review Team (Gen-ART) reviews all IETF documents being processed by the 
IESG for the IETF Chair. Please wait for direction from your document 
shepherd or AD before posting a new version of the draft. For more 
information, please see the FAQ at <​ 
http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Document: draft-ietf-anima-grasp-api-08
Reviewer: Paul Kyzivat
Review Date: 2020-11-30
IETF LC End Date: 2020-10-28
IESG Telechat date: 2020-12-01

Summary:

This draft is on the right track but has open issues, described in the 
review.

General:

This document has addressed some of the concerns I had during the last 
call review. However some of my concerns remain and some new ones have 
arisen in this version.

Issues:

Major: 3
Minor: 6
Nits:  1

1) MAJOR: Negotiation

The text in section 2.3.5 now makes clear that the sequence of steps in 
the negotiation is non-deterministic - both sides can call 
negotiate_step and negotiate_wait. I believe this can result in the two 
sides not agreeing on what values have been negotiated. (For instance, 
what if one side calls negotiate_step concurrently with the other side 
calling end_negotiate? Which value has been agreed upon?) The loop_count 
adds to the confusion. Are the two sides intended to have independent 
loop count values? It seems these too can become unsynchronized.

Also, the goal of negotiation isn't clear to me. I gather it must be for 
the two sides to agree on a particular value for the objective. But for 
that to work there must be some rules about how values can change in 
each step so that the result stabililizes, rather than causing a battle 
that ends with loop count exhaustion. This could be achieved by always 
negotiating *down*, or always *up*. But that requires that the objective 
value type have an ordering function. Given the general nature of the 
objective I don't think that can be assumed.

ISTM that more work is needed to define the negotiation process in a way 
that ensures it ends with both sides agreeing on a single value for the 
objective.

2) MINOR: Dry Run Negotiation

Dry Run negotiation is very under-specified. Why would it be used? I 
guess that an ASA might use dry run negotiation to inform future actual 
negotiation. Can anything be inferred from a dry run negotiation about 
how an actual negotiation will go? When participating in a dry run 
negotiation, how should an ASA decide what response to make? Should it 
take into account current resource availability? Or should it respond 
based on best-case or worst-case resource availability? Or what?

This requires further clarification.

3) MAJOR: Confusing semantics of 'request_negotiate'

In section 2.3.5 I don't understand the following:

          1.  The 'session_nonce' parameter is null.  In this case the
              negotiation has succeeded in one step and the peer has
              accepted the request.  The returned 'proffered_objective'
              contains the value accepted by the peer, which is therefore
              equal to the value in the requested 'objective'.  For this
              reason, no session nonce is needed, since the session has
              ended.

IIUC this requires a network exchange with the peer. I don't see how 
this can complete *immediately*. ISTM that this could only complete 
immediately if it were satisfied from a local cache. That doesn't seem 
appropriate for this function.

Similarly, in bullet 2 I don't see how the proffered_objective would be 
available in the initial call, before a response has been received from 
the peer..

Does "immediately" here simply mean that the negotiation is completed in 
one exchange between the two ends? If so, isn't a session nonce still 
required in an event loop implementation in order to handle the one 
response?

Bullet 2 also says:

              ... The
              returned 'proffered_objective' contains the first value
              proffered by the negotiation peer.  The contents of this
              instance of the objective must be used to prepare the next
              negotiation step (see negotiate_step() below) because it
              contains the updated loop count, sent by the negotiation
              peer.  The GRASP code automatically decrements the loop
              count by 1 at each step, and returns an error if it becomes
              zero.

I guess that the 'proffered_objective' in the return parameters is the 
counter-offer to the objective passed in the call. And that you expect 
the objective value used in any subsequent negotiate_step to be derived 
by modifying this value. So far this new wording has improved my 
understanding.

But the loop_count in the objective is especially confusing. It seems 
that it is handled quite differently from the rest of the objective. You 
specify (in 2.3.2.3) that it has a default value of GRASP_DEP_LOOPCT. 
But who is expected to initialize this? (Is it simply that the ASP 
should use this value if it doesn't have any particular preference?)

Then you say that the GRASP decrements this. Is this decrementing done 
on the calling side before sending the message, the calling side after 
receiving the response? Or by the peer, on receipt or when sending the 
response? Is it permissible for the ASA to modify this value during 
negotiation? Since this seems intended to prevent a loop, having clarity 
about how this value is managed seems important.

4) MINOR: negotiate_wait

The negotiate_wait call allows one ASA to extend the timeout of another 
ASA. This could, in perverse cases, cause an ASA to wait indefinitely. 
ISTM that this is dangerous. I would think it better make the other ASA 
aware of the desire to extend the timeout and let it decide whether to 
do so.

5) MAJOR: Consistency of Objective definitions

In section 2.3.2.3 and elsewhere, presumably all parties that use a 
particular objective must agree on the values of synch, neg, dry, and 
the size and structure of the value.

There is no communication of the size and structure in the abstract API. 
Presumably the implementation of a language binding to the API is 
required to at least communicate the size and alignment requirements to 
the core. The matching of definitions between nodes must be achieved 
solely by the name, the respective language bindings at the two ends, 
and out of band mutual agreement. Furthermore, different language 
bindings may use different in-memory representations of the value. In 
such cases, how is the on-wire format to be determined?

If the two ends disagree on size and structure then problems will occur. 
Perhaps the core can identify size mismatches based on size communicated 
on the wire vs the size defined by the language binding, but there are 
no error codes defined for this situation. And of course differing 
structures with the same size would not be detectable.

Furthermore, there is potential for different ASAs to (accidentally) 
have incompatible definitions for the same objective. What happens in 
this case? How can blame be ascribed so that the problem can be fixed?

IMO more needs to be said about all of this. At the least a number of 
disclaimers that put the burden on the ASAs to recognize the risk, take 
these potential problems into account and avoid them. But there could be 
some requirements placed on API language bindings and core 
implementations to deal with some of these. And probably some added 
error codes to report what problems can be detected.

6) MINOR/MAJOR: Session State

I continue to find the lifetime and state of a session to be unclear. 
The API calls that return session_nonce seem to signal creation of a new 
session. The end_negotiate() call seems to terminate a negotiation 
session. But what causes other sessions to end? This seems important 
because there is state associated with a session that consumes resources 
and can't be reclaimed until the session ends. So it should be important 
for the ASA to end all sessions. Some clarification of this seems 
important both for core implementors and for ASA developers that will be 
using the API.

(Or is this document only for implementors of core and those 
instantiating a particular language binding of the API, with 
documentation for end users left to others?)

7) MINOR/MAJOR: Timeout

Section 2.3.2.2 indicates that the API returns an error response to the 
ASA if the timeout expires. But the other end is presumably still 
working on the request and will eventually send a response. What does 
the core do when it receives this? Must it retain state so that it can 
detect the case and ignore the message? It seems that this could result 
in the two peers disagreeing on some state.

8) MINOR: Text regarding "minimum_TTL"

There is a small problem with the following in section 2.3.4:

       -  If the parameter 'minimum_TTL' is greater than zero, any
          locally cached locators for the objective whose remaining time
          to live in milliseconds is less than or equal to 'minimum_TTL'
          are deleted first.  Thus 'minimum_TTL' = 0 will flush all
          entries.

The first sentence qualifies the paragraph to cases where minimum_TTL is 
greater than zero. But the final sentence then infers the behavior when 
minimum_TTL is equal to zero.

Also, minimum_TTL is typed as an integer, which permits negative values. 
I gather that negative values are not allowed. I can suggest two ways to 
fix this:

       -  The parameter 'minimum_TTL' MUST be greater than or equal to
          zero. Any locally cached locators for the objective whose
          remaining time to live in milliseconds is less than or equal to
          'minimum_TTL' are deleted first.  Thus 'minimum_TTL' = 0 will
          flush all entries.

Or, change they type to unsigned integer. Then the statement can be 
simplified by removing the first sentence:

       -  Any locally cached locators for the objective whose remaining
          time to live in milliseconds is less than or equal to
          'minimum_TTL' are deleted first.  Thus 'minimum_TTL' = 0 will
          flush all entries.

9) MINOR: Terminology - Session nonce

The new first paragraph of section 2.2.3 talks about identifying the 
session by a pseudo-random session identifier, and tagging it with an IP 
for further uniqueness. The 2nd paragraph talks about a session_nonce. 
It isn't clear at this point in the text if these the same thing. Or is 
the session id shared on the wire, the IP tag added by the core, and the 
session_nonce an artifact of the API, shared only between the ASA and 
the core?

Section 2.3.2.7 seems to confirm that the nonce is just an identifier 
used between the core and the ASA. But here it says that using the id 
plus the IP is simply one possible implementation choice.

Further, I question whether "nonce" is the best term to use here. ISTM 
that "handle" (session_handle) would more clearly reflect the purpose of 
this item.

I think it would be helpful to be clearer in distinguishing what is 
fundamental vs what is implementation choice. For instance, in section 
2.2.3:

    A GRASP session consists of a finite sequence of messages (for
    discovery, synchronization, or negotiation) between a pair of ASAs.
    The core identifies it on the wire by a pseudo-random session
    identifier. Further details are given in [I-D.ietf-anima-grasp].

    On the first call in a new GRASP session, the API returns a
    'session_handle' value used to identify the session. This
    value must be used in all subsequent calls for the same session, and
    will be provided as a parameter in the callback functions.  By this
    mechanism, multiple overlapping sessions can be distinguished, both
    in the ASA and in the GRASP core.  The value of the 'session_handle"
    is opaque to the ASA.

This establishes the role and relationship of the two terms, while 
section 2.3.2.7 gives a possible implementation without as much 
confusion. (It will require some rewording to switch from session_nonce 
to session_handle. It already uses "session handle" in passing.)

10) NIT: Terminology - ASA nonce

For similar reasons to those above for session_nonce/session_handle, IMO 
it would be clearer to use asa_handle rather than asa_nonce. But this is 
only a suggestion.