Jakub Kicinski [off-list ref] writes:

On Fri, 26 Jan 2024 12:44:57 +0000 Donald Hunter wrote:

quoted

I was quite pleased with how simple the patch turned out to be when I
used ChainMap, but it does have this weakness.

It is very neat, no question about it :(

quoted

In practice, the only place this could be a problem is with
tc-act-attrs which has the same attribute name 'kind' in the nest and
in tc-attrs at the top level. If you send a create message with ynl,
you could omit the 'kind' attr in the 'act' nest and ynl would
incorrectly resolve to the top level 'kind'. The kernel would reject
the action with a missing 'kind' but the rest of payload would be
encoded wrongly and/or could break ynl.

We can detect the problem post-fact and throw an exception. I primarily
care about removing the ambiguity.

Agreed.

Is it possible to check at which "level" of the chainmap the key was
found? If so we can also construct a 'chainmap of attr sets' and make
sure that the key level == attr set level. I.e. that we got a hit at
the first level which declares a key of that name.

More crude option - we could construct a list of dicts (the levels
within the chainmap) and keys they can't contain. Once we got a hit
for a sub-message key at level A, all dicts currently on top of A
are not allowed to add that key. Once we're done with the message we
scan thru the list and make sure the keys haven't appeared?

Another random thought, should we mark the keys which can "descend"
somehow? IDK, put a ~ in front?

	selector: ~kind

or some other char?

Okay, so I think the behaviour we need is to either search current scope
or search the outermost scope. My suggestion would be to replace the
ChainMap approach with just choosing between current and outermost
scope. The unusual case is needing to search the outermost scope so
using a prefix e.g. '/' for that would work.

We can have 'selector: kind' continue to refer to current scope and then
have 'selector: /kind' refer to the outermost scope.

If we run into a case that requires something other than current or
outermost then we could add e.g. '../kind' so that the scope to search
is always explicitly identified.

quoted

My initial thought is that this might be better handled as input
validation, e.g. adding 'required: true' to the spec for 'act/kind'.
After using ynl for a while, I think it would help to specify required
attributes for messages, nests and sub-messsages. It's very hard to
discover the required attributes for families that don't provide
extack responses for errors.

Hah, required attrs. I have been sitting on patches for the kernel for
over a year - https://github.com/kuba-moo/linux/tree/req-args
Not sure if they actually work but for the kernel I was curious if it's
possible to do the validation in constant time (in relation to the
policy size, i.e. without scanning the entire policy at the end to
confirm that all required attrs are present). And that's what I came up
with.

Interesting. It's definitely a thorny problem with varying sets of
'required' attributes. It could be useful to report the absolutely
required attributes in policy responses, without any actual enforcement.
Would it be possible to report policy for legacy netlink-raw families?

Thinking about it, usability would probably be most improved by adding
extack messages to more of the tc error paths.

I haven't posted it because I was a tiny bit worried that required args
will cause bugs (people forgetting to null check attrs) and may cause
uAPI breakage down the line (we should clearly state that "required"
status is just advisory, and can go away in future kernel release).
But that was more of a on-the-fence situation. If you find them useful
feel free to move forward!

I do think that's a separate story, tho. For sub-message selector
- isn't the key _implicitly_ required, in the first attr set where
it is defined? Conversely if the sub-message isn't present the key
isn't required any more either?

Yes, the key is implicitly required for sub-messages. The toplevel key
is probably required regardless of the presence of a sub-message.