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

Refactor API and zone update handling #30

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tacerus
Copy link

@tacerus tacerus commented Nov 2, 2024

PR progress checklist (to be filled in by reviewers)

  • Changes to documentation are appropriate (or tick if not required)
  • Changes to tests are appropriate (or tick if not required)
  • Reviews completed

What type of PR is this?

Primary type

  • [build] Changes related to the build system
  • [chore] Changes to the build process or auxiliary tools and libraries such as documentation generation
  • [ci] Changes to the continuous integration configuration
  • [feat] A new feature
  • [fix] A bug fix
  • [perf] A code change that improves performance
  • [refactor] A code change that neither fixes a bug nor adds a feature
  • [revert] A change used to revert a previous commit
  • [style] Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)

Secondary type

  • [docs] Documentation changes
  • [test] Adding missing or correcting existing tests

Does this PR introduce a BREAKING CHANGE?

Yes.

Related issues and/or pull requests

Describe the changes you're proposing

  1. Replace proprietary and unmaintained library with native HTTP calls
  2. Support full zone metadata and rrset updates (additions, modifications, removals) with proper test=True/test=False handling and detailed diffs about changes utilizing the same data structure as the PowerDNS API itself

Pillar / config required to test the proposed changes

Pillar abstraction is not yet implemented. Hence a sample state file instead:

my_zone:
  powerdns.zone_present:
    - name: test2.pdns.home.lysergic.dev
    - kind: native
    - rrsets:
        - name: host5
          type: A
          ttl: 300
          records:
            - content: 127.0.0.3
            - content: 127.0.0.5
              disabled: True
        - name: host5
          type: AAAA
          ttl: 300
          records:
            - content: fe80::1
            - content: fe80::2
        - name: host6
          type: A
          ttl: 300
          records:
            - content: 127.0.0.3

Debug log showing how the proposed changes work

Exceeds GitHub maximum body size.

Documentation checklist

  • Updated the README (e.g. Available states).
  • Updated pillar.example.

Testing checklist

  • Included in Kitchen (i.e. under state_top).
  • Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).
  • Updated the relevant test pillar.

Additional context

This is a work in progress effort. I am sharing it already to gather potential early feedback and to learn whether such a big change is of interest to this project at all. The code still needs lots of cleanup, and it is of course lacking documentation and tests (for effective testing, the test suite will need to spin up a PowerDNS, which I am not yet sure how to integrate into the kitchen suite in this repository).

Most of the logic is around getting a usable diff for the changes to make for an experience as with other Salt states. Unfortunately, the PowerDNS API itself does not yet natively support operating in a dry-run mode with diff output.

The existing functions are for the most part refactored. Having a single zone_present() state function instead of separate rrset logic allows following of the upstream PowerDNS API data structure, making the state both easier to use for users (the same documentation as the upstream API can be followed) and easier to adapt in case of future changes to the upstream API.

The existing modules rely on the "pdnsapi" library which is proprietary
and has not received recent updates. Other Python libraries for
interfacing with the PowerDNS API suffer from the same issues or are
feature incomplete.
Solve this and make the modules more easily extensible by directly
interfacing with the PowerDNS HTTP REST API, utilizing the requests
library which is already installed as a dependency of a common Salt
deployment. For more efficient interaction as part of state modules
which repeatedly call the execution module, add the requests-toolbelt
library as the only additional dependency for easier implementation of
a base session.
As part of this refactoring the functions are renamed to follow a more
streamlined scheme differentiating between read and write operations,
and the output is aligned with what is returned directly by PowerDNS.
As a result, this is a breaking change.

Signed-off-by: Georg Pfuetzenreuter <[email protected]>
Avoid splitting rrsets updates to a separate state function when it is
handled in the same PowerDNS API payload, rather integrate the logic
from rrsets_presents() into zone_present(). The second function can be
deleted in a later patch once functionality is deemed equivalent.

Signed-off-by: Georg Pfuetzenreuter <[email protected]>
Factor in rrset and record changes and drop the separate
rrsets_present() state function. Utilize dictdiffer to avoid lots of the
complicated and convoluted comparison code at the cost of a slightly
less uniform but still useful diff in the changes output.

Signed-off-by: Georg Pfuetzenreuter <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant