Skip to content

Refactor host remove override check, add CNAME+MX confirmation#444

Open
pederhan wants to merge 3 commits into
masterfrom
refactor-override-check
Open

Refactor host remove override check, add CNAME+MX confirmation#444
pederhan wants to merge 3 commits into
masterfrom
refactor-override-check

Conversation

@pederhan
Copy link
Copy Markdown
Member

@pederhan pederhan commented Mar 27, 2026

This PR removes some code duplication and makes the override checking + warning message construction more consistent in host remove.

Furthermore, it now ensures associated MX and CNAME records are also printed when they are deleted.

Future work

This PR does not touch the A/AAAA record override checking, nor does it make any modifications to forced(). We rely on different, more complicated logic for IP address checking that cannot be defined as neatly and declaratively as the other record types.

A potential improvement would be to move the entire override checking logic into a separate class. That way, one can more easily share state between checks (active overrides, force enabled, etc.) while being able to include IP address checks into the same declarative framework that this PR adds.

Something like this (rough sketch):

class OverrideChecker:
    def __init__(self, host: Host, overrides: list[Override], force: bool) -> None:
        self.host = host
        self.overrides = overrides
        self.force = force

        self.overrides_required: set[Override] = set()
        self.warnings: list[str] = []

    def forced(self, override_required: Override | None = None) -> bool:
        # If we require an override, check if it's in the list of provided overrides.
        if override_required:
            return override_required in self.overrides

        # We didn't require an override, so we only need to check for force.
        if self.force:
            return True

        # And the fallback is "no".
        return False

    def check(self) -> tuple[list[str, list[Override]]]:
        """Check records, return warnings and list of required overrides."""
        override_checks: list[OverrideRequired[Any]] = [
            OverrideRequired(Override.CNAME, "CNAME", host.cnames),
            OverrideRequired(Override.MX, "MX", host.mxs),
            OverrideRequired(Override.SRV, "SRV", host.srvs),
            OverrideRequired(Override.PTR, "PTR", host.ptr_overrides),
            OverrideRequired(Override.NAPTR, "NAPTR", host.naptrs),
            OverrideRequired(Override.IPADDRESS, "A/AAAA", host.ipaddresses),
        ]

        for check in override_checks:
            if check.override == Override.IPADDRESS:
                self._check_ip(check)
            else:
                self._check(check)

        return self.warnings, sorted(self.overrides_required)

    def _check(self, check: OverrideRequired) -> None:
        if check.items and not forced(check.override):
            self.overrides_required.add(check.override)
            self.warnings.append(f"  {len(check.items)} {check.name} records")
            for item in check.items:
                value = get_record_identifier(item)
                self.warnings.append(f"    - {value}")

    def _check_ip(self, OverrideRequired[IPAddress]) -> None:
        if len(host.ipaddresses) > 1:
            host_vlans = host.vlans()
            same_vlan = len(host_vlans) == 1

            if same_vlan and not forced():
                self.warnings.append("  multiple ipaddresses on the same VLAN")
            elif not same_vlan and not forced(Override.IPADDRESS):
                self.overrides_required.add(Override.IPADDRESS)
                self.warnings.append("  {} ipaddresses on distinct VLANs".format(len(host.ipaddresses)))
                for vlan_id, vlans in host_vlans.items():
                    ip_strings = [str(ip.ipaddress) for ip in vlans]
                    ip_strings.sort()
                    self.warnings.append(f"    - {', '.join(ip_strings)} (vlan: {vlan_id})")

To use:

warnings, overrides_required = OverrideChecker(host, force, overrides).check()

One could even imagine we construct the entire message via the check() method (or return some data structure):

if message := OverrideChecker(host, force, overrides).check():
    raise ForceMissing(message)

Thus we can move all this complicated checking and message construction logic out of host remove and make it testable via unit tests.

@pederhan pederhan requested a review from terjekv March 27, 2026 11:18
Copy link
Copy Markdown
Collaborator

@terjekv terjekv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do feel like we're patching over a larger underlying problem around both force and how hosts are tied to RRs. This is a solid improvement considering the existing design, but we should ideally be able to generalise RR management.

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.

2 participants