trafficserver icon indicating copy to clipboard operation
trafficserver copied to clipboard

records. Add consistency check over default values defined in `RecordConfig.cc`

Open brbzull0 opened this issue 1 year ago • 2 comments

Feedback was requested by email, I'll add the details here anyway:

The goal:

Add records consistency check for default values on startup(core records and plugin records(TSMgmt*Create)

The outcome

A

If consistency check fails, the record will not be registered, I think this is the main discussion point, so leaving this as draft till we decide what would be the outcome(if any).

Details:

Working around some record consistency check issues this week I found that we do not perform this check against the default values specified during registration (4th parameter) of a record(either core or throughout the TS API).

What’s the consistency check?

This is supposed to be used to validate the value you set on a record, so if the check(regex) does not match the value then you get an error and the value is not set. This is quite clear when you try to set a new record value using traffic_ctl:

$ traffic_ctl config set proxy.config.accept_threads 99999999
Server Error found:
[9] Error during execution
- [2000] Validity check failed. [2004]

This PR adds a record consistency check on startup for default records values defined in (RecordsConfig.cc and by the TSMgmt*Create API) to avoid possible bugs.

As I said this is already done for values we read from the records.yaml and values set through traffic_ctl but not for the default value specified in the record registration code(RecordsConfig.cc and TS API)

This may break some instances with plugins which registers new records with wrong value/regex in it.

Is important to note that currently if you register a new record and configure the check to be performed but you do not specify a regex, then ATS will fail(fatal). I think something similar could be done.

Plan B:

If this is not accepted then I can add a new autest which parses the RecordsConfig.cc and generates a records.yaml file which then gets injected into ATS which OFC will error out if the consistency check fails, so at least we will be covered by a single test, existing behavior will not change.

In any case something should be done.

Probably we do not need this in 10.

brbzull0 avatar Aug 12 '24 14:08 brbzull0

AuTest errors are related this the error added in this PR. This will go away once some of my other PR's land on master.

traffic_server ERROR: XXXXXX: Consistency check for the default value=-1 failed. pattern=^-?[0-9]+$. Record will not be registered

brbzull0 avatar Aug 12 '24 14:08 brbzull0

[approve ci autest]

brbzull0 avatar Sep 24 '24 10:09 brbzull0

This pull request has been automatically marked as stale because it has not had recent activity. Marking it stale to flag it for further consideration by the community.

github-actions[bot] avatar Dec 24 '24 02:12 github-actions[bot]