T5069: warning in validator not visible without failure

[talmakion]

Change summary

In my related PR against vyos-1x, I'm adding a sanity-check warning to the bgp-large-community-list validator, but the output is eaten by validate_value.

This change allows validator scripts to request visible output, without completely failing validation. It prints results immediately rather than messing with buf and flagging output at the end, I don't know OCaml well.

Please advise if this change should also be applied to vyos1x-config or if a different method should be used, such as a regex over validator output.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Code style update (formatting, renaming)
• Refactoring (no functional changes)
• Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
• Other (please describe):

Related Task(s)

• https://vyos.dev/T5069

Related PR(s)

• policy: T5069: large-community-list regex validator disallows whitespace vyos-1x#4482

Checklist:

• I have read the CONTRIBUTING document
• I have linked this PR to one or more Phabricator Task(s)
• My commit headlines contain a valid Task id
• My change requires a change to the documentation
• I have updated the documentation accordingly

[dmbaturin]

I agree with the idea to make validator warnings visible but disagree with the idea to use non-zero exit codes for that.

For better or worse, UNIX exit codes have only two kinds of meanings: "executed successfully" (0) and "failed because of some reason" (>0). Specific values of non-zero codes can only be used to clarify the nature of the failure.

I think we can just always relay validator outputs to the user. For example, ignore stdout but always relay validator's stderr to validate_value's stderr so that the validator can use it for warnings.

You can use Unix.open_process_full to get all streams (see https://ocaml.org/manual/5.3/api/Unix.html).

[talmakion]

Spotted the shell fd redirect:

let chan = Unix.open_process_in (Printf.sprintf "%s \'%s\' 2>&1" c value) in

Removing the redirect, it'll pass through without having to juggle 2 separate pipes potentially overflowing and deadlocking against each other, on the low chance a validator breaks and spews output. After poking through the OCaml libs, open_process_in already only captures stdout and passes stderr right through. This works OK in testing.

However - I imagine that redirect was already there for a reason, are there any known issues with successful chatty validators that I should double check?

(I haven't yet made corresponding changes to the other PR)

[jestabro]

@talmakion standard practice has been to print warnings from the verify stage of the config mode script in question, instead of trying to report from the validator in the case of success. Granted this may lead to some redundancy of code, but it allows for a natural distinction: silent success, or failure with explanatory details; consequently no output indicates a valid config.

If there are compelling reasons why, in this case, warning output is better suited for validation, then we should consider a proper extension of the validator framework (as discussed briefly with @dmbaturin who has a suggested design). For the current case, let us consider firstly whether moving to the conf_mode script verify stage makes sense.

[talmakion]

@jestabro Cheers for the feedback.

In this case, I had considered moving the warning to commit-verify, but ended up settling on doing this in the validator for immediate feedback, with the warning right after the command that caused it. It's pretty common for my workflow to involve large commits when changing policy sections.

However, it's just a warning hinting that something might be wrong. Simply allowing through stderr has probable unwanted side effects, buffering stderr separately involves a bit of complexity plus signalling from the warning source and other parts of my changes are getting janky - for eg, the validators run twice (at set and commit time), so the warning comes up twice, which just looks a bit odd.

I'll rework my changes to generate the warning in verify, instead.

[jestabro]

@talmakion I presume that we are talking about the same thing, but as you mention 'commit-verify', and want to clarify that I am referring to the verify function in the standard config-mode script structure:
https://docs.vyos.io/en/latest/contributing/development.html#configuration-script-structure-and-behaviour