T5083: extend xml schema definitions to support child requirements #3575
Conversation
thomasfinstad
requested review from
dmbaturin,
sarthurdev,
zdc,
jestabro,
sever-sever and
c-po
schema/interface_definition.rnc
Outdated
| # requirements of node or tagnode children. | ||
| childRequirements = element childRequirements { | ||
| (element require { child+ } )? & | ||
| (element conflict { nodeNameAttr, child+ })* & |
There was a problem hiding this comment.
One issue I see is that nodeNameAttr is now assumed to always be a single word, but conflicts are often between children at different levels
My feeling is that there are actually two different situations we need to handle, if we want to generate helpful error messages.
The first one is a simple conflict when two nodes cannot appear together. For example, route-reflector-client and route-server-client in BGP — a neighbor cannot be both, under any circumstances. We may use something like <mutuallyExclusiveChildren> for that. In that case if both are found, we can show a message like $nodeA and $nodeB cannot be configured at the same time if any nodes from the list are found.
The second situation is a context-dependent conflict. For example, in OpenVPN, the server node and any of its children like server topology conflict with mode client and mode site-to-site. It's possible for nodes to conflict with nodes in general or with specific valued of leaf nodes.
If we find a way to handle both, that may indeed limit the need for in-script validation to ultra-context-dependent things like route-reflector-client allowed only if the local AS is the same as the remote AS (i.e., the peer is an iBGP peer).
The way that you are proposing will lead to a combinatorial explosion when there are more than two conflicting options.
There was a problem hiding this comment.
The first one is a simple conflict when two nodes cannot appear together. For example,
route-reflector-clientandroute-server-clientin BGP — a neighbor cannot be both, under any circumstances. We may use something like<mutuallyExclusiveChildren>for that. In that case if both are found, we can show a message like$nodeA and $nodeB cannot be configured at the same timeif any nodes from the list are found.
It would not cover cases where there are 3 children X, Y and Z where X can not be configured with Y or Z, but Y and X can both be configured as long as X is not.
This might however be less useful that what you suggest having a simple list of all mutually exclusive / conflicting children.
I have no issue with reworking (element conflict { nodeNameAttr, child+ })* & to be (element mutuallyExclusiveChildren { child+ } )? & if that is preferred by those of you that know the system and its needs better than me.
One issue I see is that
nodeNameAttris now assumed to always be a single word, but conflicts are often between children at different levels
[...]
The second situation is a context-dependent conflict. For example, in OpenVPN, theservernode and any of its children likeserver topologyconflict withmode clientandmode site-to-site. It's possible for nodes to conflict with nodes in general or with specific valued of leaf nodes.If we find a way to handle both, that may indeed limit the need for in-script validation to ultra-context-dependent things like
route-reflector-clientallowed only if the local AS is the same as the remote AS (i.e., the peer is an iBGP peer).
While what I have here does not currently support this level of logic I am willing to take a stab at creating the required logic if you believe it will be a useful feature to have declarative configurations for conflicting descendants of different levels. The reason I did not add it is because my use-case will not benefit from it, but I would be happy to add it.
Does these changes sounds reasonable to accept?
schema/interface_definition.rnc
Outdated
| (element require { child+ } )? & | ||
| (element conflict { nodeNameAttr, child+ })* & | ||
| (element atLeastOneOf { child+ } )* & | ||
| (element depend { nodeNameAttr, child+ })* |
There was a problem hiding this comment.
I'm not sure if we should use both require and depend, from the maintainability perspective.
There was a problem hiding this comment.
I assume what you mean here is that since both fields can be intepreted as "this node must have these children configured" it can be confusing when not taken with extra context, which will waste mental bandwidth.
If that is the case what about (element requiredChildren { child+ } )? & and (element childDependsOn { nodeNameAttr, child+ })* instead?
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
There was a problem hiding this comment.
I agree that this is an interesting idea; the implementation is nicely done. However, I am concerned that it simply internalizes the complexity of inter-node dependence from the conf-mode script verify stage to the XML definitions and overhead of the ConfigDict class: this poses two immediate issues:
(1) will authors of conf-mode scripts tend to write verify checks simply, rather than properly, according to this design, leaving it underused
(2) the introduction of the ConfigDict to handle defaults tracking (T5330) was meant as a lightweight overhead --- subsequent problems have suggested packing class attributes with further bookkeeping, but in each case I considered that to indicate a mistaken design: my sense is that if we had a general definition of inter-node dependencies (cf. comments of @dmbaturin), we could justify the overhead, but lacking that, I am hesitant.
I agree this requires further discussion to consider.
|
I am currently working on expanding the capabilities as suggested by @dmbaturin where I am looking to add features to allow internode specifications as well as some basic support for leafnode values eg: if Due to these new features renamed it from I hope that all non-dynamic type of requirements would then be possible to have declaratively defined. Currently this is the schema I am working towards supporting: So here is a snippet of As you can see setting the I would love to know if these changes are acceptable, if changes need to be made, or if this is a dead end and I need to find a different way to fulfill my use-case. (which is a terraform provider: https://registry.terraform.io/providers/thomasfinstad/vyos-rolling/latest/docs) |
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
|
👍 |
|
I pushed up what I currently have to get some more input. What I think might be a good idea:
I hope these changes (current new code + what I suggest above) gives enough of a picture of how it will end up if we wish move forward with this. Edit <mutuallyExclusiveChildren>
<descendant name="mode">
<value>client</value>
</descendant>
<descendant name="tls">
<child>dh-params</child>
</descendant>
<mutuallyExclusiveChildren>into this <mutuallyExclusiveChildren>
<child name="mode" value="client"/>
<in name="tls">
<child name="dh-params"/>
</in>
<mutuallyExclusiveChildren> |
One other option would be to utilize logical operators to fulfill required flexibility instead. Meaning adding in generic |
There was a problem hiding this comment.
@thomasfinstad Please don't feel discouraged — this is just a really big and interesting problems that takes time and iterations! I've been thinking about it a lot lately, now I'm ready to offer some suggestions.
I'm wondering if we can find a better terminology that will allow more concise and expressive syntax.
I left a comment with an example there in the review. My thinking is that we may want to think in terms of just two entities: requirements and conflicts.
Here's what a requirement may look like:
<requirement>
<source node="mode" value="client" />
<requiredSibling>remote-host</requiredSibling>
<requiredDescendant>...
Conflicts also have a source and a bunch of things that must not appear when the source node exists (and, optionally, has a particular value) — see an example above.
I've also been thinking about mutually exclusive children, and I wonder if the possibility of them is a sign that the CLI can be improved. For example, in BGP we have set protocols bgp neighbor x.x.x.x address-family ipv4-unicast route-server-client and set protocols bgp neighbor x.x.x.x address-family ipv4-unicast route-reflector-client.
Should we? If we make both those options values or something like role (as it's done elsewhere in BGP), the illegal state will be unrepresentable.
BGP is also an example where full validation is impossible without a Turing-complete check. One cannot set route-reflector-client unless local-as == ../system-as, since it only makes sense in iBGP — eBGP has no concept of route reflectors. So some checks will always reside in the code, and I don't think we should try to make our XML Turing-complete.
| @@ -149,3 +149,6 @@ python/vyos/xml_ref/pkg_cache/*_cache.py | |||
| # We do not use pip | |||
| Pipfile | |||
| Pipfile.lock | |||
|
|
|||
| # KDE | |||
There was a problem hiding this comment.
This change might be sensible for KDE users (not for me — I use MATE, btw; but @jestabro does use KDE :).
But it's out of scope of this PR, could you make it separate please?
| @@ -1,26 +0,0 @@ | |||
| { | |||
There was a problem hiding this comment.
If this file doesn't prevent the changes from working (I believe it doesn't), its removal shouldn't be in this PR.
| <child>allow-host-networks</child> | ||
| <child>network</child> | ||
| </atLeastOneOf> | ||
| <oneWayDependantChildren> |
There was a problem hiding this comment.
The word "dependent" is misspelled here (see https://en.wiktionary.org/wiki/dependent)
I also wonder if it should be just "dependentChildren" rather than "oneWayDependentChildren".
| <oneWayDependantChildren> | ||
| <dependants><descendant name="mode"><value>client</value></descendant></dependants> | ||
| <dependees> | ||
| <child>remote_host</child> |
| <oneWayDependantChildren> | ||
| <dependants><descendant name="mode"><value>site-to-site</value></descendant></dependants> | ||
| <dependees> | ||
| <child>remote-host</child> |
There was a problem hiding this comment.
Site-to-site mode doesn't actually require remote-host. One side of a site-to-site tunnel is a listener.
| <descendant name="mode"><value>server</value></descendant> | ||
| <child>authentication</child> | ||
| </mutuallyExclusiveChildren> | ||
| <mutuallyExclusiveChildren> |
There was a problem hiding this comment.
This sequence of <mutuallyExclusiveChildren> tags is very tedious to read, to be honest.
I'm also thinking that <mutuallyExclusiveChildren> should only be for children that simply cannot appear together under any circumstances, like server and remote-host.
In a situation when a node value is incompatible with a bunch of children, we may want to call it something else.
At the very least I'd like a more compact syntax, like:
<conflict>
<source> <descendant name="mode" value="server"> </source>
<excludes>remote-host</excludes>
<excludes>remote-port</excludes>
...
</conflict>
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
Note for passers by, the discussion is currently taking place in slack until we land on how to move forward. |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Change Summary
Adds declarative child requirements support to XML schema definitions, and converts the container service to utilize this new functionality as a proof of concept.
Types of changes
Related Task(s)
https://vyos.dev/T5083
Related PR(s)
vyos/vyos1x-config#29
Component(s) name
Proposed changes
This extends the XML schema to include an optional block where a node/tagnode can declaratively define simple requirements for children.
Currently 4 requirement types are supported:
This feature allows the simpler checks to be moved out of the .py file to clear it up and make more room for the more complex checks without extra clutter. It will also allow 3rd party tooling to better understand vyos command / api structure via the schemas.
While this PR contains changes for the container service, other services can be migrated over time.
How to test
Extra tests to assure expected behavior has been added to the container smoke test.
Smoketest result
Checklist: