Can a plugin modify module B based on changes in module A when validation of module A depends on module B? about sysrepo HOT 11 CLOSED

The UPDATE event you tried before was meant for this purpose except it did not support modifying data of another module because that required additional complex tasks. However, I have made the effort and this should now be supported.

from sysrepo.

I'm not sure this is necessary, seeing as you can just use the NACM to restrict access to itself

Of course, you can use NACM but that is not the same. I was talking about using YANG configuration for internal system configuration such as paths to libraries or installed packages. The user could decide what is considered internal and not accessible remotely at all (meaning not even the YANG module is sent to the clients).

For the candidate datastore backdoor problem, would you prefer a separate GitHub issue for it?

Yes, please.

from sysrepo.

I made the following changes:

0. (update to latest devel.)
1. Add the SR_SUBSCR_UPDATE flag to the subscribe call.
2. Change the code from if(event == SR_EV_CHANGE) to if(event == SR_EV_UPDATE).
3. Remove the sr_apply_changes call.

I get an assertion error:

$ sudo sysrepocfg --edit vim --module plants -v3
[INF] Connection 33 created.
[INF] Session 16 (user "root", CID 33) created.
[INF] EV ORIGIN: "plants" "update" ID 1 priority 1 for 1 subscribers published.
[INF] EV ORIGIN: "plants" "update" ID 1 priority 1 succeeded.
sysrepocfg: /home/kkovacs/oss_97542/third-party-libraries/sysrepo/src/modinfo.c:582: sr_modinfo_edit_apply: Assertion `mod->state & MOD_INFO_REQ' failed.
Aborted

I also tried sudo sysrepocfg --import my-xml-file.xml --module plants and got the same assertion error.

I noticed that mod->state was 292 in this case, which is equivalent to MOD_INFO_INV_DEP | MOD_INFO_RLOCK_UPGR | MOD_INFO_DATA. The assertion requires the MOD_INFO_REQ flag to be set. The module in question is the animals module, which is not the one being directly edited; it's the one that's getting changed by the plugin.

from sysrepo.

Okay, I have tested a simpler use-case and yours needed a bt more tweaking, try now.

from sysrepo.

The assertion error no longer occurs, but I think there is another problem. When using sysrepocfg --edit, there is a validation step that occurs before the SR_EV_UPDATE event changes come back from the plugins. This means that I cannot use the update event to make a would-be-invalid update valid.

Using my example modules, I have 2 lists that I restricted to be the same length. I want to be able to add an item to just one of the lists and have my plugin auto-add an item to the other list. I think the validation should wait until after the update event has completed.

Steps to Reproduce

Use the same example YANG files from above.

0. (update to latest devel).
1. sudo make sr_clean
2. sudo sysrepoctl --install [email protected]
3. sudo sysrepoctl --install [email protected]
4. import some data (both.xml)

<top xmlns="urn:plants">
    <plants>
        <name>tree</name>
    </plants>
    <plants>
        <name>grass</name>
    </plants>
</top>
<top xmlns="urn:animals">
    <animals>
        <name>dog</name>
    </animals>
    <animals>
        <name>cat</name>
    </animals>
</top>

sysrepocfg --import both.xml

5. Run the plugin, which uses this call:

rc = sr_module_change_subscribe(session,
                                "plants",
                                "/plants:top",
                                callback,
                                privateData == NULL ? NULL : *privateData,
                                1,
                                SR_SUBSCR_DEFAULT | SR_SUBSCR_UPDATE,
                                &subscriptionContext);

6. Try editing data with sudo sysrepocfg --edit vim --module plants -v3. Add or remove a <plants> node, then save and quit. The following message prints and the plugin does not trigger.

[INF] Connection 17 created.
[INF] Session 15 (user "root", CID 17) created.
[ERR] The number of plants must equal the number of animals (path "/animals:top")
[ERR] Validation failed.
sysrepocfg error: Replace config failed (Validation failed)

The validation is failing before the update event is processed, so the plugin code doesn't get a chance to run at all.

from sysrepo.

The validation step in question occurs when sr_replace_config calls sr_changes_notify_store, which calls sr_modinfo_validate. This validation does not wait for update events to modify the changes before requiring the data to be valid. I don't know if this is intended or not.

While poking around here, I noticed that the candidate datastore is not validated in sr_changes_notify_store. That led me to try making my change to the candidate datastore, allowing my plugin to modify the data, then doing sysrepocfg --copy-from to finish off. This might be the answer I need in theory, if the validate-before-update-event behavior is intentional. However, while doing that I did the following steps:

1. Kill all plugins.
2. Make an edit in the candidate datastore that would be invalid. No errors occur.
3. Copy the candidate data into the running datastore. No errors occur.
4. Export the running datastore. It's in an invalid state.

The candidate datastore is providing a backdoor method for getting invalid data into the running datastore. I think some necessary validation step is being skipped in this case. This may be a separate issue, so I could open a new GitHub issue with a minimal example if that makes more sense.

from sysrepo.

I think the validation should wait until after the update event has completed.

You are saying it should be possible to use an edit that would result in invalid datastore if the data are made valid in an UPDATE callback. I am not sure this should be supported and am leaning towards no. Why should an invalid edit be allowed? YANG is supposed to describe valid data both for users and for devices, users must not ignore it. So I would say your YANG design is wrong/unsupported.

1. Kill all plugins.
2. Make an edit in the candidate datastore that would be invalid. No errors occur.
3. Copy the candidate data into the running datastore. No errors occur.
4. Export the running datastore. It's in an invalid state.

More details should be sufficient, how exactly to perform step 3. I have tried it in a test and got the proper validation error.

from sysrepo.

it should be possible to use an edit that would result in invalid datastore if the data are made valid in an UPDATE callback.

Yes, precisely. I thought that was the point of the update event.

• update = modify incoming data before it hits the datastore
• change = accept/reject incoming changes after they are finalized
• done = take external actions based on changes after they are committed

But if that's not what it's for, then I just misunderstood.

• update = modify valid incoming data before it hits the datastore and keep it valid

I don't want to get too deep into my specific application (unless it's welcome) because I don't want to waste your time worrying about issues only relevant to me. But in my case, I am using some custom data that needs to be tightly coupled with ietf-netconf-acm. The nodes in the NACM and the nodes in my model are very related to each other, but not the same. So you can see why I want some must statements to help ensure that everything got updated correctly whenever something changed. That being said, I want to manage the actual NACM internally and not expose it to the user. I want to give the user some nice easy knobs to turn without making them worry about all the groups and rule-lists that I have set up. So they are expected to modify some data in my custom module and the NACM will update itself under the hood (via my plugin) and they don't need to worry about making 2 edits every time they want to make 1 edit. As it stands, when they turn the nice easy knob, it will be an illegal edit because they didn't also edit the NACM to match. But I don't want them worrying about the NACM; my plugin should be handling that part.

So anyway, I think there are 3 ways to go

1. Give the user that nice easy knob in the form of some RPCs, which will be able to take care of everything under the hood and still provide them a nice interface to work with.
2. Remove the must validation and block their access to the NACM so that it's truly internal and trust in my plugin code to handle it properly and never make a mistake. If I write the plugin correctly, the validation shouldn't be necessary.
3. Convince you to change the semantics of the update event... probably not a great idea, ha.

I think option 2 is probably the best, assuming option 3 is off the table.

how exactly to perform step 3

1. Set up

$ cd third-party-libraries/sysrepo/build/ && sudo make sr_clean && cd -
[ 50%] Removing all volatile SHM files prefixed with "sr"
[ 50%] Built target shm_clean
[100%] Removing the whole persistent repository "/home/kkovacs/oss_97542/third-party-libraries/sysrepo/build/repository"
[100%] Built target sr_clean
/home/kkovacs/oss_97542
$ sudo sysrepoctl --install [email protected]
$ sudo sysrepoctl --install [email protected]
$ sudo sysrepocfg --import both.xml
$ sudo sysrepocfg --export
<top xmlns="urn:animals">
  <animals>
    <name>cat</name>
  </animals>
  <animals>
    <name>dog</name>
  </animals>
</top>
<top xmlns="urn:plants">
  <plants>
    <name>grass</name>
  </plants>
  <plants>
    <name>tree</name>
  </plants>
</top>

2. Try to add a plant in the running datastore. Gets rejected.

$ echo '<top xmlns="urn:plants"><plants><name>shrub</name></plants></top>' | sudo sysrepocfg --edit --module plants --format xml
[ERR] The number of plants must equal the number of animals (path "/animals:top")
[ERR] Validation failed.
sysrepocfg error: Failed to merge edit data (Validation failed)
For more details you may try to increase the verbosity up to "-v3".

3. Try to add a plant in the candidate datastore. Does not get rejected. Invalid data is now in the candidate datastore.

$ echo '<top xmlns="urn:plants"><plants><name>shrub</name></plants></top>' | sudo sysrepocfg --edit --module plants --format xml --datastore candidate
$ sudo sysrepocfg --export --datastore candidate
<top xmlns="urn:animals">
  <animals>
    <name>cat</name>
  </animals>
  <animals>
    <name>dog</name>
  </animals>
</top>
<top xmlns="urn:plants">
  <plants>
    <name>grass</name>
  </plants>
  <plants>
    <name>shrub</name>
  </plants>
  <plants>
    <name>tree</name>
  </plants>
</top>

4. Copy candidate to running. Running was valid before and is not valid after.

$ sudo sysrepocfg --export
<top xmlns="urn:animals">
  <animals>
    <name>cat</name>
  </animals>
  <animals>
    <name>dog</name>
  </animals>
</top>
<top xmlns="urn:plants">
  <plants>
    <name>grass</name>
  </plants>
  <plants>
    <name>tree</name>
  </plants>
</top>
$ sudo sysrepocfg --copy-from candidate
$ sudo sysrepocfg --export
<top xmlns="urn:animals">
  <animals>
    <name>cat</name>
  </animals>
  <animals>
    <name>dog</name>
  </animals>
</top>
<top xmlns="urn:plants">
  <plants>
    <name>grass</name>
  </plants>
  <plants>
    <name>shrub</name>
  </plants>
  <plants>
    <name>tree</name>
  </plants>
</top>

from sysrepo.

So anyway, I think there are 3 ways to go

1. Definitely a possibility.
2. Based on the previous text I assumed this is what you did and was surprised to learn you did not. So yes, a half-way solution is probably not a good idea. We are actually periodically revisiting an idea of allowing to make some YANG configuration "internal" meaning not accessible using NETCONF, which is likely what you could use for ietf-netconf-acm. But the functionality is complex and needs to be carefully designed to work well and we have not yet made the effort.
3. I explained my reasoning why it works the way it does and I consider it correct. The original motivation for the update event was iana-crypt-hash module, which specifies a typedef for passwords and says that if a clear-text password is configured (a valid value), the server should hash it and actually store a hashed password.

from sysrepo.

I assumed this is what you did

Yes, I have done this now.

make some YANG configuration "internal"

I'm not sure this is necessary, seeing as you can just use the NACM to restrict access to itself (or any other "internal" module).

<nacm xmlns="urn:ietf:params:xml:ns:yang:ietf-netconf-acm">
  <rule-list>
    <name>block-nacm</name>
    <group>*</group>
    <rule>
      <name>nacm-access</name>
      <module-name>ietf-netconf-acm</module-name>
      <access-operations>*</access-operations>
      <action>deny</action>
      <comment>Nobody can touch the NACM</comment>
    </rule>
  </rule-list>
</nacm>

and/or

<nacm xmlns="urn:ietf:params:xml:ns:yang:ietf-netconf-acm">
  <read-default>deny</read-default>
  <write-default>deny</write-default>
  <exec-default>deny</exec-default>
</nacm>

needs to be carefully designed to work well

It does sound tricky, and possibly not worth the effort.

I consider it correct

I agree. It was my misunderstanding.

For the candidate datastore backdoor problem, would you prefer a separate GitHub issue for it?

from sysrepo.

Yes, please.

Opened #3313.

needed a bit more tweaking, try now.

After this, the issue was resolved.

from sysrepo.