Automate validation of provider AuditRecords rules

[eliheady]

I asked about creating an 'expect failure' test mode in the last developer meeting. This is the issue I had in mind.

Providers have an 'AuditRecords' ruleset to filter out zone corrections that the service does not support.

A common example is a.Add("TXT", rejectif.TxtIsEmpty), which will prevent pushing a TXT RR with an empty value.

When service providers change their support for such records, DNSControl will continue blocking them until someone notices and raises a bug report. The rules defined in many of the providers include a comment along the lines of // last verified 20XX-XX-XX, suggesting some automation could save the effort of spot checking these to keep them up to date.

Current example: INWX provider blocks TXT RRs with a trailing space (noted 2021-03-01). The service provider currently allows such records, meaning sometime in the past 4 years the audit records filter became outdated.

Proposal: detect this situation in CI

• Proposal A: add a new TestCase receiver method that can be used to invert the test failure mode in regular integration test runs.

  • new testgroups for each provider that opts-in to validating the rules (guarded by only filter)
  • example:

    // integration_test.go
    testgroup("INWX AuditRecord tests",
      // this group contains tests that validate the rules in provider's
      // AuditRecord function.
      only("INWX"),
      tc("TXT trailing ws", txt("foows1", "trailingws ")).ExpectTestFailure(),
      tc("TXT with 1 backtick", txt("foobt", "blah`blah")).ExpectTestFailure(),
      tc("a 0-byte TXT", txt("foo0", "")).ExpectTestFailure(),
    ),

  • would create a coupling (at least in practice) of integrationTests/integration_test.go and $provider/audit_records.go
    that would require maintenance
  • would run with every integration test job - possibly harassing providers with permanently failing tests

• proposal B: add a new flag for an audit records test mode to the integration test suite

  • would add a bit more complication to current integration test than Proposal A (new workflow)
  • No changes to integration test groups or new tests, test selection would be guided by AuditRecord rules:
    • Use the processing of the AuditRecords rules to flip the test selection logic: skip all
      expected-good tests, run only the normally skipped tests.
  • AuditRecords testing could be done periodically, apart from regular integration tests

    # ... new GitHub workflow ...
    on:
      workflow_dispatch:
      schedule:
        - cron: '0 0 * */3 0' # every third month

• proposal C: make a new 'ignore' mode that runs all tests, ignoring the filtering done by AuditRecords

  • expected failures are just plain failures in the normal sense, making it easier to miss the unexpected successes
  • requires similar test suite changes as proposal B

• Other options? surely some other approach I didn't think of.

Right now I would lean toward proposal B as the exclusive implementation. I haven't come up with a use-case for a validation test that would only work with the option A approach. I can see downsides to both. Running the validation tests with every integration test run is excessive since the prohibition of the relevant RRs is unlikely to change often, maybe not ever. Option B and C are a bit more invasive to the overall integration test suite. The implementation I arrived at uses a new workflow - I think these rule validation tests should probably not be run frequently, so making a manual or schedule-able CI job fits, but does this issue warrant a special CI workflow?

I roughed out an implementation that is close to supporting all three modes but I think one should be chosen to keep the complexity down. I decided to stop and see what folks think of the idea in general.

Demo test output of proposal B, showing that the trailing spaces filter is not needed:
https://github.com/eliheady/dnscontrol/actions/runs/14646338089/job/41102042315#step:6:2305

edit, in case that job output is not available to everyone:

=== Failed
=== FAIL: integrationTest TestDNSProviders/dnscontrol-integrationtest-inwx.com/22:complex_TXT:TXT_trailing_ws (1.01s)
    helpers_integration_test.go:194: UNEXPECTED SUCCESS in test 'TXT trailing ws'
    helpers_integration_test.go:195: AuditRules may need to be updated if the second test pass reports UNEXPECTED SUCCESS
    helpers_integration_test.go:217: 
        - DELETE foobt.dnscontrol-integrationtest-inwx.com TXT "blahblah" ttl=300
    helpers_integration_test.go:217: 
        + CREATE foows1.dnscontrol-integrationtest-inwx.com TXT "trailingws " ttl=300
    helpers_integration_test.go:254: UNEXPECTED SUCCESS: Expected corrections on second run in test 'TXT trailing ws' but found none.
    helpers_integration_test.go:255: (Audit filter check) This provider's AuditRecords rules should be reviewed.
        --- FAIL: TestDNSProviders/dnscontrol-integrationtest-inwx.com/22:complex_TXT:TXT_trailing_ws (1.01s)

=== FAIL: integrationTest TestDNSProviders/dnscontrol-integrationtest-inwx.com (2.76s)
    --- FAIL: TestDNSProviders/dnscontrol-integrationtest-inwx.com (2.76s)

=== FAIL: integrationTest TestDNSProviders (3.58s)
Testing Profile="INWX" (TYPE="INWX")

DONE 297 tests, 289 skipped, 3 failures in 72.234s

[tlimoncelli]

Ah, you've definitely identified a real problem: APIs being improved and we don't notice.

Ideally we'd have a GHA that runs periodically that just tells us "Hey, the following providers can remove the rejectif line".

How to implement that.... let me give this more thought.

[eliheady]

Thanks Tom. I've sketched out something like option B above but it's kind of ugly. I'll share in a draft PR anyway. It relies on a new flag in the integration test to flip the mode to invert the normal test selection.

The thing I find ugly is how I arrived at setting that flag. The options seem to be: a near identical copy of the existing integration test, with only the new flag added, or a reusable workflow that exposes the test arguments in a base workflow that callers can set to their liking. That seemed really not bad at first, but then I ran into the issue of the secrets. Workflows triggered by a workflow_call action only have access to secrets that are defined in both the caller and the called workflows, so there is a huge duplication of secret references on both sides. scratch that. I found the secrets: inherit option

[eliheady]

#3548 has a serviceable take on option B. There are a couple of remnants of the earlier experiments with the tc.ExpectTestFailure approach, which I think wouldn't be so bad to add back in. I guess that might be useful if you know a provider is working on supporting a filtered record and you want to test more frequently?

But I like where you are going with the more actionable GHA output. The PR I have would be somewhat actionable, but requires the maintainer/user to parse the test failure output and also remember that this is a double negative situation, which is not the most intuitive way to get the information to the person that needs to make the code change ... I tried to make it clear enough in the log messages

[tlimoncelli]

Suggestion: (and this might be a big change from the direction you're currently going. It's not a requirement, just something to consider)

I notice that the logic in runTests() is getting a bit hairy with all the various modes. So many if/thens!

Consider moving all the tests related to validating rejectif statements to a separate test function (possibly named runRejectAuditTests()). That way runTests() can run faster (fewer tests) and runRejectAuditTests() can have very different reporting logic, and hopefully more simplifed the logic.

Go testing has a "fast tests only" mode (See go test -short) thus saving us from having to create logic around when to run the audit tests. That should simplify things further.

[eliheady]

Thank you for considering this Tom!

Consider moving all the tests related to validating rejectif statements to a separate test function

I see some consolidation that could be done to centralize the test planning after calling makeTests(): one new function where we could check the start and end index, check the test group filters, and also check the rejectif rules. Moving the AuditRecords call to the same place that the other test validity checks happen seems like a logical grouping to me, though it would mean iterating over the test cases in each group twice. If I try this I will benchmark it to see what the runtime difference is. Maybe it is worth doing even if it slows the test prep down a bit - I see comments around there about making a validation pass ... a tangent: having an option to just show the test plan after selection might be nice.

Go testing has a "fast tests only" mode (See go test -short)

I'm not sure how to apply this idea (and thank you for the link, I was not aware of it!). However, I rather like the idea of just using the AuditRecords rules themselves as the test selection criteria. That would remove the need to write and maintain integration test cases paired with the rejectif rules in place for each provider. Maybe I'm not really understanding how to use this in this test suite. I'll find some examples.

I will spend more time on this sometime later. I think it is pretty low priority.