Skip to content

Change pathz telemetry to be per policy rather than per path #1282

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.

Sign up for GitHub

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

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

Change pathz telemetry to be per policy rather than per path #1282

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e9692a1
Change pathz telemetry to be per policy rather than per path
marcushines Apr 16, 2025
c1743ab
add config false to container
marcushines Apr 16, 2025
85df6a2
Apply suggestions from code review
marcushines Apr 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 20 additions & 32 deletions release/models/gnsi/openconfig-gnsi-pathz.yang
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,13 @@ module openconfig-gnsi-pathz {
OpenConfig-path-based authorization policies installed on a networking
device.";

oc-ext:openconfig-version "0.3.0";
oc-ext:openconfig-version "0.4.0";

revision 2025-04-15 {
description
"Refactor gNSI counters to be only per policy.";
reference "0.4.0";
}

revision 2024-02-13 {
description
Expand Down Expand Up @@ -109,62 +115,44 @@ module openconfig-gnsi-pathz {
description
"A collection of counters collected by the gNSI.pathz module.";

container gnmi-pathz-policy-counters {
container policies {
Copy link

@charanjith-anet charanjith-anet Apr 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This should be called 'pathz' so that the yang tree looks as follows:

/system/grpc-servers/grpc-server
    +--ro pathz
       +--ro policy* [name]
          +--ro name     -> ../state/name
          +--ro state

Copy link
Member

@dplore dplore Apr 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Building on these suggestions:

Lists need to follow OC style guide, (lists/list) and strongly recommend using a counters container for those leaves, following precedent in most of the rest of the OC models.

  augment /oc-sys:system/oc-sys-grpc:grpc-servers/oc-sys-grpc:grpc-server:
  +--ro pathz
    +--ro policies
       +--ro policy* [name]
          +--ro name     -> ../state/name
          +--ro state
             +--ro name?     string
             +--ro last-read-reject?   oc-types:timeticks64
             +--ro last-read-accept?   oc-types:timeticks64
             +--ro last-write-reject?   oc-types:timeticks64
             +--ro last-write-accept?   oc-types:timeticks64
             +--ro counters
             |  +--ro read-rejects?       oc-yang:counter64
             |  +--ro read-accepts?       oc-yang:counter64
                +--ro write-rejects?       oc-yang:counter64
                +--ro write-accepts?       oc-yang:counter64

Copy link
Contributor

Choose a reason for hiding this comment

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

See previous comments - this should not be "per gRPC server instance" so I believe the anchor point is incorrect first and foremost.

The gNSI suite should be looked at hollisically of which there are some gRPC APIs to interact (there could very well be others both native or in other OC methods)

But wherever the anchor point ends up - +1 to the last comment in order to follow the encapsulation of lists inside containers

Copy link
Member

Choose a reason for hiding this comment

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

I see, your suggestion could perhaps be stated as reserving the /system/grpc-servers/grpc-server subtree to paths which are generic across any/all grpc servers (which means it will only have things like a connection list and packet/byte counters).

The anchor for pathz can be at the system level since pathz is a "system-wide" entity.

  augment /oc-sys:system/:
  +--ro pathz
    +--ro policies
       +--ro policy* [name]
          +--ro name     -> ../state/name
          +--ro state
             +--ro name?     string
             +--ro last-read-reject?   oc-types:timeticks64
             +--ro last-read-accept?   oc-types:timeticks64
             +--ro last-write-reject?   oc-types:timeticks64
             +--ro last-write-accept?   oc-types:timeticks64
             +--ro counters
             |  +--ro read-rejects?       oc-yang:counter64
             |  +--ro read-accepts?       oc-yang:counter64
                +--ro write-rejects?       oc-yang:counter64
                +--ro write-accepts?       oc-yang:counter64

Copy link

@LimeHat LimeHat Apr 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@earies

so I believe the anchor point is incorrect first and foremost.

It is not necessarily incorrect, but there's a mismatch between the PR description and the anchor point.

You can apply the same policy to multiple servers, but still implement per-server counters, especially if (as @dplore suggested) the purpose of this PR is to count number of accepted/denied RPCs (which hasn't been confirmed yet).

After changes in openconfig/gnsi/pull/219, and combined with the idea of doing per-RPC counters, we are essentially in the authz-like territory which already implements per-server counters
https://github.com/openconfig/public/blob/master/release/models/gnsi/openconfig-gnsi-authz.yang

Which kind of brings the question, do we really need all these separate counters? Might as well just do combined per-grpc server statistics for both authz and pathz rejects.

config false;
description
"A collection of per-OpenConfig path counters.";

uses gnmi-pathz-policy-xpath-success-failure-counters;
}
}

grouping gnmi-pathz-policy-xpath-success-failure-counters {
description
"A collection of per-OpenConfig path counters.";

container paths {
description
"Container for a collection of per-OpenConfig path counters.";
"Container for a collection of per-Pathz policy counters.";

list path {
list policy {
description
"List for a collection of per-OpenConfig path counters.";
"List for a collection of per-Pathz policy counters.";
key "name";
leaf name {
type leafref {
path "../state/name";
}
description
"A OpenConfig schema path the counter were
collected for.

For documentation on the naming of paths, see
https://github.com/openconfig/reference/blob/master/rpc/gnmi/gnmi-path-conventions.md";
"A OpenConfig Pathz policy name the counters were
collected for.";
}
container state {
description
"Operational state for per-OpenConfig path counters.";
"Operational state for per-Pathz policy counters.";
leaf name {
type string;
description
"A OpenConfig schema path the counter were
collected for.

For documentation on the naming of paths, see
https://github.com/openconfig/reference/blob/master/rpc/gnmi/gnmi-path-conventions.md";
"A per-Pathz policy the counters were
collected for.";
}
container reads {
description
"The counter were collected while
performing a read operation on the
schema path.";
performing read operations on the
Copy link

@LimeHat LimeHat Apr 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

what, exactly, is a read operation on the policy?

Is it a single RPC allowed/denied by the policy (regardless of a number of paths authorized, e.g. a GetRequest with 10 allowed paths will be counted as +1?)

Copy link
Member

Choose a reason for hiding this comment

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

Suggested description:

"The number of gnmi.Get and gnmi.Subscribe RPC's that match the policy and were accepted or rejected. "

Copy link
Contributor

Choose a reason for hiding this comment

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

By "accept/rejected" do you mean "permitted/denied by the policy"?
its not really that binary - a gNMI read operation can be a mix of permitted and denied paths.
For example, if the policy is "permit read /a and deny read /a/b", and /a/b exists in YANG, then a request to read /a will be partially permitted, partially denied.

policy.";
uses counters;
}
container writes {
description
"The counter were collected while
performing a write operation on the
schema path.";
performing write operations on the
Copy link

Choose a reason for hiding this comment

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

same question as for the reads, please specify the exact definition of what should be counted here (number of set requests, number of individual operations in each request, etc)

policy.";
uses counters;
}
}
Expand Down
Loading