Better regexp for validating thresholds

[q84fh]

I've observed panic when we were attempting to use some silly, not correct thresholds like:

• @4:~
• ::
• ~:~
• @@
• and so on...

It this MR I'm proposing little more strict regexp to validate this.
I think that I've covered all the cases from https://nagios-plugins.org/doc/guidelines.html

[atc0005]

@q84fh Thanks for this PR and examples of failed input. Those examples will be useful when expanding current test coverage.

Regarding the new regex pattern, some tests are now failing with the changes from 3def28f.

See https://github.com/atc0005/go-nagios/actions/runs/6171392393/job/16755839961?pr=226 for recent CI results.

Local test results using Go 1.20.8:

$ go test ./...
Skipping os.Exit call as requested.
Skipping os.Exit call as requested.
Skipping os.Exit call as requested.
Skipping os.Exit call as requested.
Skipping os.Exit call as requested.
Skipping os.Exit call as requested.
--- FAIL: TestParsePerfDataSucceedsForValidInput (0.00s)
    --- FAIL: TestParsePerfDataSucceedsForValidInput/Load_averages_unquoted (0.00s)
        perfdata_test.go:605: Evaluating input "load1=0.260;5.000;10.000;0; load5=0.320;4.000;6.000;0; load15=0.300;3.000;4.000;0;"
        perfdata_test.go:609: failed to parse perfdata input: failed to parse warn field: field Warn fails validation: invalid performance data format
    --- FAIL: TestParsePerfDataSucceedsForValidInput/Load_averages_double_quoted (0.00s)
        perfdata_test.go:605: Evaluating input "\"load1=0.260;5.000;10.000;0; load5=0.320;4.000;6.000;0; load15=0.300;3.000;4.000;0;\""
        perfdata_test.go:609: failed to parse perfdata input: failed to parse warn field: field Warn fails validation: invalid performance data format
FAIL
FAIL    github.com/atc0005/go-nagios    0.108s
FAIL

I hope to have some time to look further into this tomorrow or Friday.

[q84fh]

Sorry, I did not notices that tests are in place, cool! I'm glad that they catched that!

I've expended regexp to cover negative and float numbers (it is long and ugly, but works).

I've also expanded tests to check if ParsePerfData returned error or not and added some test cases with incorrect thresholds.

It now passes go test and I hope it will be fine!

[atc0005]

@q84fh: Sorry, I did not notices that tests are in place, cool! I'm glad that they catched that!

Not a problem and I'm also glad. While this project does have tests I don't have extensive experience crafting them yet.

I've expended regexp to cover negative and float numbers (it is long and ugly, but works).
I think that I've covered all the cases from https://nagios-plugins.org/doc/guidelines.html

That regex is complex, but I understand that it may be needed to catch all supported options.

Are you willing to document which portions of the regex does what?

https://stackoverflow.com/questions/54095244/regular-expressions-in-go-that-span-multiple-lines

I was hoping that Go supported an alternative syntax for complex regexes, but it appears it does not. There does appear to be a useful workaround though:

You have a few options to improve readability of a regular expression like this.

Split the string:

pattern := `(,\s*|\s+)` +
    `(\(?s\.\s?s\.|` +
    `\(?s\.\s?l\.|` +
    `\(?s\.\s?str\.|` +
    `\(?s\.\s?lat\.).*$`

Which would look something like this (mockup only, using example from Jonathan Hall):

	perfDataThresholdRangeSyntaxRegex string = `(,\s*|\s+)` +
		// explanation for pattern 1
		`(\(?s\.\s?s\.|` +

		// explanation for pattern 2
		`\(?s\.\s?l\.|` +

		// explanation for pattern 3
		`\(?s\.\s?str\.|` +

		// explanation for pattern 4
		`\(?s\.\s?lat\.).*$`

@q84fh: I've also expanded tests to check if ParsePerfData returned error or not and added some test cases with incorrect thresholds.

For the tests in this project I tried to adopt the advice of a Go author I admire and split the test cases into:

• performance data patterns that I expect to always pass (TestParsePerfDataSucceedsForValidInput)
• performance data patterns that I expect to always fail (TestParsePerfDataFailsForInvalidInput)

If we continue with that pattern then the new test cases would be added to the TestParsePerfDataFailsForInvalidInput function. Are you willing to make that change?

[q84fh]

Are you willing to document which portions of the regex does what?
(...)
Are you willing to make that change?

Sure! I was also thinking about moving away from such complex validation using regexp. Maybe we could use it just to extract values and do validation using Go? It should be better readable and understandable for everyone and also it would give us an opportunity to compare start and end of range (because we have "start ≤ end" requirement in Nagios Plugin Guideline).

[atc0005]

@q84fh At a surface level what you describe makes sense.

Clarity is the goal, so if breaking the field value into individual patterns for validation is something you're interested in drafting I'm happy to review and provide feedback.

Even if we go no further in that direction you've already helpfully identified several patterns that can be added to the TestParsePerfDataFailsForInvalidInput test cases to assert that they're not valid/allowed.

[q84fh]

I was also thinking about moving away from such complex validation using regexp. Maybe we could use it just to extract values and do validation using Go? It should be better readable and understandable for everyone and also it would give us an opportunity to compare start and end of range (because we have "start ≤ end" requirement in Nagios Plugin Guideline).

I will open separate PR once I will have more fancy validation working. For now I've finally found some time and committed explanation for each part of perfDataThresholdRangeSyntaxRegex regexp.

[atc0005]

@q84fh: I will open separate PR once I will have more fancy validation working.

Makes sense. Please free to open early and solicit feedback on the design at the early stages. I don't consider myself an expert by any means, but I'm happy to serve as a sounding board early on.

For now I've finally found some time and committed explanation for each part of perfDataThresholdRangeSyntaxRegex regexp.

Thank you! This makes the regex easier to reason about.

Are you up for also moving the test cases into TestParsePerfDataFailsForInvalidInput ?

[atc0005]

On a related note, I saw this comment in a repo I'm watching:

• How to deal with large regexp StefanSchroeder/Golang-Regex-Tutorial#19 (comment)

Thinking out loud (not necessarily specific to this PR):

This is pretty close to the approach we've already discussed, but could allow "naming" specific portions of a regex (to aid in documentation). The doc comments cover that, but having a name directly attached to the regex value could provide another layer of explanation. It could potentially make it easier to reuse later, though that would be more of a side effect that a primary goal.

One change I might make from the given example is using constants instead of variables.

[atc0005]

Hi @q84fh,

I wanted to touch base on this.

With recent changes applied by #236 and #237 are you still encountering the same threshold validation issue(s)?

The idea to split the regex to make it easier to read seems like it would still be a useful change.