brbzull0 opened a new pull request, #11667: URL: https://github.com/apache/trafficserver/pull/11667
Feedback was requested by [email](https://lists.apache.org/[email protected]), 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](https://github.com/apache/trafficserver/issues/11649) this week I found that we do not perform this check against the default values specified during [registration](https://github.com/apache/trafficserver/blob/6774984a3f71f7473092d1af7ba99a004e8890d9/src/records/RecordsConfig.cc#L39) (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](https://github.com/apache/trafficserver/blob/6774984a3f71f7473092d1af7ba99a004e8890d9/src/records/RecordsConfig.cc#L39) 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](https://github.com/apache/trafficserver/blob/6774984a3f71f7473092d1af7ba99a004e8890d9/src/records/RecordsConfigUtils.cc#L58-L60)(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.** -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
