apache / trafficserver

Apache Traffic Server™ is a fast, scalable and extensible HTTP/1.1 and HTTP/2 compliant caching proxy server.
https://trafficserver.apache.org/
Apache License 2.0
1.8k stars 798 forks source link

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

Open brbzull0 opened 1 month ago

brbzull0 commented 1 month ago

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 commented 1 month ago

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 commented 1 week ago

[approve ci autest]