Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal for clarifications when calling gNMI Set on a list node or element. #140

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wenovus
Copy link
Contributor

@wenovus wenovus commented May 10, 2021

  1. Add example JSON_IETF format for updating an individual list element.
  2. Clarify that an empty subset of attributes for a keyed-list is unacceptable as a path for gNMI Set.
  3. Clarify that in OpenConfig, MUST specify a path that terminates on the enclosing container in order to modify a whole list node.
  4. Minor grammar fixes.

Background

I found these points to be missing from either this doc or RFC7951 (#139). After some discussions with Anees, I'm proposing to add these clarifications that may or may not be the standard or established convention. In that case, modification suggestions are welcome to correct any inaccuracies.

…lement.

1. Add example JSON_IETF format for updating an individual list element.
1. Clarify that an empty subset of attributes for a keyed-list is unacceptable as a path for gNMI Set.
1. Clarify that in OpenConfig, MUST specify a path that terminates on the enclosing container in order to modify a whole list node.
1. Minor grammar fixes.

Background: I found these points to be missing from either this doc or
RFC7951. As a result, I'm proposing adding these clarifications that may
or may not be the standard or established convention. In that case,
modification suggestions are welcome to correct any inaccuracies.
@wenovus wenovus requested review from aashaikh, robshakir and gcsl May 10, 2021 19:51
then this MUST be considered an error by the target system - and an status
code of` InvalidArgument (3)` specified.
operation's path specifies a strict subset of the attributes (e.g.,
`/a/f[k1=10]`, `/a/f`), then this MUST be considered an error by the target
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change means that one cannot send /path/to/list with a body that is an array of objects in JSON (assuming JSON_IETF) right? Is there a reason to disallow this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't change anything here -- a strict/proper subset is likely what the original sentence meant to say.

I think there is no clear reason to disallow this, since the keys for each element in the array should be provided in the update message as well. However, that would now mean this spec needs to be changed, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's an ambiguity here, I think that this was intended to say a non-null subset - i.e., keys were partially specified.

I think there are a few cases here:

  1. No keys are specified - e.g., /interfaces/interface - in this case, I think I expect that the serialised content of the list would be specified. e.g., the array of objects for JSON_IETF.
  2. A partial set of keys are specified - this is an error with InvalidArgument in my view.

I think it'd be worth checking implementations to see whether the no-keys variant is supported. This was the original motivation for having surrounding containers in OpenConfig such that a get + set could be done on the surrounding container to get all entries.

all collection entries that are not supplied in the value provided in the
`SetRequest`. An update operation MUST be considered to add new entries to
the collection if they do not exist.
* In the case that key values are specified both as attributes of a node, and
as their own elements within the data tree, update or replace operations
that modify instances of the key in conflicting ways MUST be considered an
error. The target MUST return a status code of `InvalidArgument (3)`.
* In OpenConfig, a path which refers to a keyed YANG list node for
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that this should live here, but rather in a specific document that covers OpenConfig -- but I'm not quite sure that this is something that we want to enforce here, based on the question above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above question refers to modifying a specific list element, whereas this question refers to modifying a whole list, so I think they're unrelated from one another.

>
>
```

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth explicitly stating here that the contents of the update is the JSON object that would be expected at that list entry?

then this MUST be considered an error by the target system - and an status
code of` InvalidArgument (3)` specified.
operation's path specifies a strict subset of the attributes (e.g.,
`/a/f[k1=10]`, `/a/f`), then this MUST be considered an error by the target
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's an ambiguity here, I think that this was intended to say a non-null subset - i.e., keys were partially specified.

I think there are a few cases here:

  1. No keys are specified - e.g., /interfaces/interface - in this case, I think I expect that the serialised content of the list would be specified. e.g., the array of objects for JSON_IETF.
  2. A partial set of keys are specified - this is an error with InvalidArgument in my view.

I think it'd be worth checking implementations to see whether the no-keys variant is supported. This was the original motivation for having surrounding containers in OpenConfig such that a get + set could be done on the surrounding container to get all entries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants