search

openconfig / reference

This repository contains reference implementations, specifications and tooling related to OpenConfig-based network management.
Apache License 2.0
155 stars 88 forks source link

gNMI Specification: Root Path should be an empty slice #34

Closed aaronbee closed 7 years ago

aaronbee commented 7 years ago

In the specification the root path is defined to be a path with a single empty element in it, but this is inconsistent with non-root paths, which don't start with an empty element. This makes writing code to deal with paths more complex. Instead the root path should be defined to be the empty path.

An example path:

path: <
  element: "a"
  element: "b"
  element: "c"
>

A root path from the specification

path: <
  element: ""
>

The root path should be a prefix of every other path. It is not in this case because other paths do not start with "". This also means code can't simply concatenate two paths without doing special checks for a root path. The result of appending a path to a root path should be the same as the path.

Consider the Go code:

func pathAppend(x, y gnmi.Path) gnmi.Path {
    return gnmi.Path{Element: append(x.Element, y.Element...)}
}

func main() {
    root := gnmi.Path{Element: []string{""}}
    full := gnmi.Path{Element: []string{"a", "b", "c"}}
    part1 := gnmi.Path{Element: []string{"a"}}
    part2 := gnmi.Path{Element: []string{"b", "c"}}

    pathEqual(
        pathAppend(part1, part2),
        full) // This is true, as expected

    pathEqual(
        pathAppend(root, full),
        full) // Not true!

    isPrefix(root, full) // Not true!
}

Full code here: https://play.golang.org/p/_DAiVl4_1z

Whereas, if you change the definition of root to be 

    root := gnmi.Path{Element: []string{}}

Then all checks return true.

tsuna commented 7 years ago

Any thoughts to share @pborman @marcushines @gcsl @aashaikh @robshakir?

marcushines-zz commented 7 years ago

From a specification standpoint the root is infact a null path element The original intent was specifically for a query

root := gnmi.Path{Element: []string{}}

root := gnmi.Path{Element: []string{""}}

root := gnmi.Path{Element: []string{"*"}}

all would return the same paths

On Mon, Nov 21, 2016 at 3:56 PM Benoit Sigoure notifications@github.com wrote:

Any thoughts to share @pborman https://github.com/pborman @marcushines https://github.com/marcushines @gcsl https://github.com/gcsl @aashaikh https://github.com/aashaikh @robshakir https://github.com/robshakir?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/openconfig/reference/issues/34#issuecomment-262105936, or mute the thread https://github.com/notifications/unsubscribe-auth/ALKGmwwWW27hTF3GX6cPeihez85Rh8R2ks5rAi-9gaJpZM4KwNBe .

tsuna commented 7 years ago

I find it weird that gnmi.Path{Element: []string{""} is a valid representation of the root path. For example there are parts of the spec that read like this:

In the case that a prefix is specified, the absolute path is comprised of the concatenation of the list of path elements representing the prefix and the list of path elements in the path field.

Here concatenation seems to imply simple append()-style concatenation, but then as @aaronbee explained above, the implementation cannot actually be that naive.

Either the spec should clarify the algorithm that is meant by concatenation, or we should replace the provision about the root path from:

The root node (/) is indicated by encoding a single path element which is an empty string, as per the following example:

path: <
  element: ""
>

to something along the lines of:

The root node (/) is indicated by encoding a path with no element, as per the following example:

path: <
>

and then we can keep the commonly understood, simple use of concatenation.

My vote, obviously, would be for the empty path representation (in case it matters).

robshakir commented 7 years ago

The intent here is to have a way in which we can distinguish the root from an uninitialised path. In the case that we have zero specified elements, then it is difficult to gauge that the client specifically meant "/" vs. did not form the path correctly. Since we also support paths that have wildcard elements (e.g., []string{"a", "", "c"} to mean /a/*/c then we have consistency that the empty element ("" means 'wildcard anything in this context') when querying. It also allows us to keep consistency with specifically being able to set to the root without a somewhat strange looking push to []string{"*"}.

The primary case for concatenating paths seems (to me) to be where we have path plus prefix specified. In this case, then I think that if one explicitly has set a prefix to be []string{""} then the intention is actually to say //some/path/elements.

What is the case in which you expect to concatenate paths that are []string{""} plus some other elements that cause this to be a problem? What would be the alternate proposal for a query that covers /*/state for example? [The current scheme allows this to be encoded as prefix: []string{""} + path: []string{"state"}]

tsuna commented 7 years ago

On Mon, Nov 21, 2016 at 6:32 PM, Rob Shakir wrote:

The intent here is to have a way in which we can distinguish the root from an uninitialised path. In the case that we have zero specified elements, then it is difficult to gauge that the client specifically meant "/" vs. did not form the path correctly.

If we agree that the zero-element path is the root directory, then it's not difficult to know whether the client specifically meant / vs did not form the path correctly, since the representation unambiguously means /. If you're talking about the case of "oops I forgot to set this field in the protobuf" then yes in that case that you would be referring to the root directory, but we don't really see this as a problem as much as we see the []string{""} being an annoyance and an inconsistency.

Since we also support paths that have wildcard elements (e.g., []string{"a", "", "c"} to mean /a/*/c then we have consistency that the empty element ("" means 'wildcard anything in this context') when querying.

I didn't understand the consistency argument, in fact to me it's the complete opposite: it's inconsistent that []string{""} means both / and // (aka recursive wildcard starting from the root).

It also allows us to keep consistency with specifically being able to set to the root without a somewhat strange looking push to []string{"*"}.

If you want to set the root, you just use []string{} as the path, no need to push to []string{"*"}.

The primary case for concatenating paths seems (to me) to be where we have path plus prefix specified. In this case, then I think that if one explicitly has set a prefix to be []string{""} then the intention is actually to say //some/path/elements.

I agree that if the prefix is set to be []string{""} then the intention is actually to say //some/path/elements, and I believe this is consistent with the view I am holding that []string{} should be the root path, but inconsistent with the view that []string{""} is the root path.

What is the case in which you expect to concatenate paths that are []string{""} plus some other elements that cause this to be a problem? What would be the alternate proposal for a query that covers /*/state for example? [The current scheme allows this to be encoded as prefix: []string{""} + path: []string{"state"}]

I'm a bit confused about your last question, especially about what /*/state means exactly, so I'll reply with a question for clarification first: what does /a/*/c mean in gNMI?

  1. It's a single-level wildcard (i.e. it matches /a/b/c/ but not /a/x/y/c)
  2. It's another notation for /a//c (i.e. recursive wildcard, matches both /a/b/c/ and /a/x/y/c), and it's accepted by the spec as an equivalent form of /a//c.
  3. It's another notation for /a//c (i.e. recursive wildcard, matches both /a/b/c/ and /a/x/y/c), and is only used informally when talking about path wildcards, but it's not something that could be used in practice (not allowed by the spec).
  4. Something else.

At first I thought that this notation was used for single-level wildcards (case 1), which is the XPath meaning of * (if I understand the spec correctly), but the gNMI path convention doc didn't totally clarify this to me, and the question you asked makes me think that the reality is otherwise (probably case 3?).

But again, as mentioned above, prefix: []string{""} + path: []string{"state"} to me means //state, i.e. anything starting from the root up until something called state (recursive wildcard) – and up until now I thought this was different than /*/state (which you could encode as prefix: []string{"*"} + path: []string{"state"} and which means "any grand-children of the root named state, but not immediate children or great-grand children of the root")

We're not saying that []string{""} is not allowed at all, just that it shouldn't be used to refer to the root directory.

robshakir commented 7 years ago

[]string{""} corresponds directly to /, not to // - since to specify // one needs to specify two empty elements - i.e., one after the first / and the second after the second - i.e., []string{"", ""}.

In the path specification document, we have an empty element indicating recursive matches - i.e., /a//c matches all c elements under /a (all descendants). The * character matches children only - i.e., /a//c. My example above should be //state (apologies).

robshakir commented 7 years ago

Hi All,

@gcsl, @aashaikh and I spent some time discussing this, conclusions:

PTAL at these proposals; I'll plan to include them in a forthcoming update of the specification.

pborman commented 7 years ago

A point of history, I came up with the concept of all paths starting with the "" element (e.g., "" was root). Originally, a path was a single string and was separated by a /. Element names such as "Ethernet0/1/2" conflicted with this. One solution was to find a new path separator (a NUL byte was the safest, but hard to type and print) or simply provide the path as a series of path elements ([]string). The following Go statements were used to clarify how this could be used:

elements = strings.Split(path, "/")
path = strings.Join(elements, "/")

Clearly the first one was not foolproof, but it clearly demonstrated the concept.

The path /foo/bar turns into []string{"", "foo", "bar"} and the Join will once again produce /foo/bar. On the other hand, []string{"foo", "bar"} produced foo/bar and would have been considered a relative path. A prefix was always rooted at root, so foo/bar would become /foo/bar when joined with the root. In theory, if the prefix was []string{"foo"} and the path component was []string{"", "bar"] the resulting path would be /bar since the path portion was already absolute (think of the prefix as your current working directory).

At this point, I believe that the path is always relative to the prefix (i.e., cannot start with a "" element) and the "" in the prefix is unnecessary because without it it is relative to the root ("") in the first place. The full path is always generate by full := append(prefix, path).

Anyhow, that is where this all came from (it actually predates the OpenConfig proto).

The one thing I see that needs to be addressed is how you wildcard a path in a subscription (there are never any wildcards in the responses to a subscription). Initially, * wild carded a single element in the path and ** wildcarded any number of elements in a path. The path []string{"interfaces", "**", "status} would match all status nodes (or leaves named status) in the interfaces tree. The following all essentially would represent the same thing: []string{"foo"}, []string{"foo", "*"}, and []string{"foo", "**"} but []string{"foo", "*", "bar"} and []string{"foo", "**", "bar"} are very different, the latter matches everything the former does, but the former, unlike the later, does not match []string{"foo", "bar"} or []string{"foo", "a", "b", "bar"}. Our internal software does not allow "**" in a query.

Coming back to today, a ""should never be needed at the start or end of a path or prefix, and if presented in either of those places, should either be an error or simply ignored. Inside of a path, if allowed, is it a wildcard for a single element (i.e., "*") or 0 or more elements (i.e., "*")? I think matching exactly one element is much simpler, but someday someone is going to ask for the other. I would probably stick with it being disallowed and use ```""for a single element match, and if needed later, add"*"``` for a multi-element match.

nileshsimaria commented 7 years ago

Hi @robshakir

We think that there is consistency in how an empty string is used in paths - []string{""} expands to "match all descendants of the preceding path" - which is nothing - so all descendants are matched. It then has identical semantics in []string{"interfaces", ""} where it means "match all descendants of the preceding path again, which matches all descendants of the interfaces node of the tree.

So []string{"a", "", "c"} is for path /a//c or /a/*/c ? Looks like former, if yes, []string{"a", "*", "c"} is for path /a/*/c ?

nileshsimaria commented 7 years ago

Thanks for the history @pborman

A point of history, I came up with the concept of all paths starting with the "" element (e.g., "" was root). Originally, a path was a single string and was separated by a /. Element names such as "Ethernet0/1/2" conflicted with this. One solution was to find a new path separator (a NUL byte was the safest, but hard to type and print) or simply provide the path as a series of path elements ([]string).

How about XPath style quotes for the literal values ? Something like ...

/interfaces/interface[name='Ethernet1/2/3']/state/count

Coming back to today, a ""should never be needed at the start or end of a path or prefix, and if presented in either of those places, should either be an error or simply ignored. Inside of a path, if allowed, is it a wildcard for a single element (i.e., "*") or 0 or more elements (i.e., "*")? I think matching exactly one element is much simpler, but someday someone is going to ask for the other. I would probably stick with it being disallowed and use "" for a single element match, and if needed later, add "*" for a multi-element match.

Sticking to Xpath abbreviated syntax would help ?

/a/*/c matches c which is grand children of /a e.g. /a/b1/c, /a/b2/c etc.

/a/*/*/c matches c which is great grand children of /a e.g. /a/b1/bb1/c, /a/b1/bb2/c etc

/a//c matches c at any level e.g. /a/c, /a/b/c, /a/b/b1/c /a/b/b1/b2/c etc.

robshakir commented 7 years ago

Nilesh,

1) Yes, /a/*/c is explicitly represented as []string{"a", "*", "c"} where "*" means "match direct children" rather than "" which means "match all descendants". The intention here was to keep some consistency for implementations.

2) The internal representation that we propose to use for tooling is as @pborman states - using Go []string or an array of strings in any other language. The path specification in the gNMI directory explains why there is no need for quoting using this syntax, plus it means less manipulation of paths internally for tooling.

r.

pborman commented 7 years ago

The last I heard (I am no longer heavily involved, but do have the historical perspective), XPath is going to be the way to specify queries and []string{"interfaces", "interface[name='Ethernet1/2/3']"} is valid. Initially we were not planning to use XPath so we would have used []string{"interfaces", "Ethernet1/2/3"}. This works fine until you have multi-part keys, which is why XPath like syntax was considered.

Some of us strongly did not want to have to stutter and say interface twice in the path.

pborman commented 7 years ago

So Rob, you are proposing that "" be the equivalent of what "**" as described in my historical post? I think you are better off using "**" and making "" invalid inside a path (or perhaps everyplace).

nileshsimaria commented 7 years ago

Thanks Rob for clarification.

On Fri, Dec 2, 2016 at 10:06 AM, Rob Shakir notifications@github.com wrote:

Nilesh,

1.

Yes, /a//c is explicitly represented as []string{"a", "", "c"} where "*" means "match direct children" rather than "" which means "match all descendants". The intention here was to keep some consistency for implementations. 2.

The internal representation that we propose to use for tooling is as @pborman https://github.com/pborman states - using Go []string or an array of strings in any other language. The path specification in the gNMI directory explains why there is no need for quoting using this syntax, plus it means less manipulation of paths internally for tooling.

r.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/openconfig/reference/issues/34#issuecomment-264520941, or mute the thread https://github.com/notifications/unsubscribe-auth/AEOwgHaOHgmOBBcaXVcswtJE51AbDF29ks5rEF48gaJpZM4KwNBe .

robshakir commented 7 years ago

Per discussions on this issue and elsewhere, we have updated the specification for paths. The consensus was that the proposal to adopt []string{} as the root logically made more sense. Please take a look at the update to the paths specification.

aaronbee commented 7 years ago

Hey @robshakir, just noticed that the path specification doc has been updated, but the gnmi-specification.md has not:

The root node (/) is indicated by encoding a single path element which is an empty string

robshakir commented 7 years ago

Thanks Aaron, I'll fix this in the 0.3.1 release we have.