CESNET / libnetconf2

C NETCONF library
BSD 3-Clause "New" or "Revised" License
203 stars 147 forks source link

notifications.yang/nc-notifications.yang are required for client RFC5277 support #492

Closed esmasth closed 3 months ago

esmasth commented 4 months ago

Since notifications.yang and nc-notifications.yang are not defined by any RFC, they should not be expected as a dependency for RFC5277 operations.

michalvasko commented 4 months ago

The modules (soon to be standardized in a RFC) are only YANG forms of XML Schemas defined in RFC 5277. To avoid having to support XML Schema validation, it makes sense and requiring them should be expected.

esmasth commented 4 months ago

@michalvasko The models are still within CESNET/Yuma(?) implementation specific domain for now, and the name of the modules are likely not going to be the same as what is right now even when standardized, correct? Given that each implementation is free to choose their YANG implementation of RFC5277 if need be, expecting particular models on the client that are from CESNET/Yuma pedigree creates interoperability issues.

https://github.com/esmasth/libnetconf2/commit/f5705c2454fdac869ff9e999a67f30777ccb89e6 is a rudimentary approach in libnetconf2 client code to gracefully allow both presence or absence of the said models on a NETCONF server.

Also, I remember conversation on the XSD files not being precisely translatable to YANG, is that resolved to be feasible now?

michalvasko commented 4 months ago

I am not aware of other implementations (why would anyone do that?) and the assumption is the YANG modules are so old and used for such a long time that this is not an issue.

I do not think your changes will solve anything. notifications YANG defines RPC create-subscription, which is the main required RPC for creating subscriptions, how would they work without it? nc-notifications defines notifications replyComplete and notificationComplete, these are also a mandatory part of RFC 5277 subscriptions.

esmasth commented 4 months ago

I am not aware of other implementations (why would anyone do that?)

The choice of whether workaround RFC5277 internal modeling is used and which one to use (since non standard) is at the discretion of each NETCONF client/server realization. At least certain versions of ConfD I know of didn't have said models supported/advertised.

I do not think your changes will solve anything.

The way it works on the change above, is that the client side (prerequisite) local cache of notifications and nc-notifications is used as if the NETCONF server had the said models and the cache was populated based on get-schema, even if NETCONF server didn't have them, based on the only standard defined criteria of having RFC5277 support i.e., by checking the "urn:ietf:params:netconf:capability:notification:1.0" URN.

If the URN does not exist in NETCONF server capabilities then client is well within its rights to presume lack of RFC5277 support, but not otherwise, I think. Does that clarify @michalvasko?

michalvasko commented 4 months ago

The choice of whether workaround RFC5277 internal modeling is used and which one to use (since non standard) is at the discretion of each NETCONF client/server realization. At least certain versions of ConfD I know of didn't have said models supported/advertised.

Okay, so what RPC exactly was used to start a notification subscription, in which YANG module?

esmasth commented 4 months ago

RFC5277 create-subscription RPC was used, and if there are any internal YANG models used within the NETCONF server, to back the RFC5277 URN namespace XSDs, they are not advertised on YANG Library.

michalvasko commented 4 months ago

I see. Okay then, I suppose your proposed change should not cause any issues. Do you want to provide a PR or should I fix it?

esmasth commented 4 months ago

I can provide the linked change as a PR to start with. It would be great if you can suggest refinements there, as my current code is a quick hack. Possibly the case where NETCONF server has another set of YANG models serving RFC5277 after the standardization you mention (or otherwise) could be handled as a separate change.

michalvasko commented 4 months ago

I propose no changes to the patch, functionality-wise (I would just use realloc and only once the modules are to be added manually) . Whatever modules will be standardized later, we will update the code then.

esmasth commented 4 months ago

We can close the issue now. If it is important is:question could change to is:bug