Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

brbzull0
Copy link
Contributor

@brbzull0 brbzull0 commented Aug 12, 2024

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.

RecordConfig.cc
Also add this validation over records registered by plugins.
@brbzull0 brbzull0 added the Records Records related code. label Aug 12, 2024
@brbzull0 brbzull0 self-assigned this Aug 12, 2024
@brbzull0
Copy link
Contributor Author

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
Copy link
Contributor Author

[approve ci autest]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feedback Needed Request people to provide feedback. Records Records related code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant