sts-sponsors team mailing list archive

[Alberto Donato <mp+434522@xxxxxxxxxxxxxxxxxx>]

Review: Needs Information

some questions/suggestions inline

Diff comments:

> diff --git a/src/maasserver/dns/config.py b/src/maasserver/dns/config.py
> index c484570..d87af03 100644
> --- a/src/maasserver/dns/config.py
> +++ b/src/maasserver/dns/config.py
> @@ -366,23 +367,43 @@ def process_dns_update_notify(message):
>                              rectype="AAAA",
>                          )
>                      )
> -                resource = DNSResource.objects.get(
> -                    name=update_list[2], domain__name=zone
> -                )
> +
> +                ttl = None
> +                ip_addresses = []
> +                if op == "DELETE-IP":
> +                    resource = DNSResource.objects.get(
> +                        name=update_list[2], domain__name=zone
> +                    )
> +                    ttl = (
> +                        int(resource.address_ttl)
> +                        if resource.address_ttl
> +                        else None
> +                    )
> +                    ip_addresses = list(
> +                        resource.ip_addresses.exclude(ip__isnull=True)
> +                    )
> +                else:
> +                    iface_id = int(update_list[-1])
> +                    iface = Interface.objects.get(id=iface_id)
> +                    default_domain = Domain.objects.get_default_domain()
> +                    ttl = (
> +                        int(default_domain.ttl) if default_domain.ttl else None
> +                    )
> +                    ip_addresses = list(
> +                        iface.ip_addresses.exclude(ip__isnull=True)
> +                    )
> +                print(ip_addresses)

leftover print

>                  updates += [
>                      DynamicDNSUpdate.create_from_trigger(
>                          operation="INSERT",
>                          zone=zone,
>                          name=name,
>                          rectype=rectype,
> -                        ttl=int(resource.address_ttl)
> -                        if resource.address_ttl
> -                        else None,
> +                        ttl=ttl,
>                          answer=ip.ip,
>                      )
> -                    for ip in resource.ip_addresses.all()
> +                    for ip in ip_addresses
>                  ]
> -
>              elif len(update_list) > 4:  # has an answer
>                  updates.append(
>                      DynamicDNSUpdate.create_from_trigger(
> diff --git a/src/maasserver/triggers/system.py b/src/maasserver/triggers/system.py
> index a18fe8a..6b19e2b 100644
> --- a/src/maasserver/triggers/system.py
> +++ b/src/maasserver/triggers/system.py
> @@ -2054,6 +2054,134 @@ def render_dns_dynamic_update_subnet_procedure(op):
>      )
>  
>  
> +def render_dns_dynamic_update_interface_static_ip_address(op):
> +    return dedent(
> +        f"""\
> +        CREATE OR REPLACE FUNCTION sys_dns_updates_interface_ip_{op}()
> +        RETURNS trigger as $$
> +        DECLARE
> +          current_hostname text;
> +          default_domain_id bigint;
> +          domain text;
> +          iface_name text;
> +          ip_addr text;
> +          address_ttl int;
> +          current_node_config_id bigint;
> +          current_node_id bigint;
> +        BEGIN
> +          ASSERT TG_WHEN = 'AFTER', 'May only run as an AFTER trigger';
> +          ASSERT TG_LEVEL <> 'STATEMENT', 'Should not be used as a STATEMENT level trigger', TG_NAME;
> +          SELECT domain_id INTO default_domain_id FROM maasserver_globaldefault LIMIT 1;

maybe I'm missing something, but why is this using the default domain?
Shouldn't we get the domain associated with the node the IP is assigned to? (and similarly in other places below where the globaldefault is used)

> +          SELECT name, COALESCE(ttl, 0) INTO domain, address_ttl FROM maasserver_domain WHERE id=default_domain_id;
> +          IF (TG_OP = 'INSERT' AND TG_LEVEL = 'ROW') THEN
> +            SELECT name, node_config_id INTO iface_name, current_node_config_id FROM maasserver_interface WHERE id=NEW.interface_id;
> +            SELECT node_id INTO current_node_id FROM maasserver_nodeconfig WHERE id=current_node_config_id;
> +            SELECT hostname INTO current_hostname FROM maasserver_node WHERE id=current_node_id;

these 3 queries could be combined into a single one, something like

SELECT iface.name, node.hostname INTO iface_name, hostname FRROM maasserver_interface AS iface JOIN maasserver_node AS node ON iface.node_config_id = node.current_config_id;

> +            SELECT host(ip) INTO ip_addr FROM maasserver_staticipaddress WHERE id=NEW.staticipaddress_id;
> +            PERFORM pg_notify('sys_dns_updates', 'INSERT ' || domain || ' ' || current_hostname || ' A ' || address_ttl || ' ' || ip_addr);
> +            PERFORM pg_notify('sys_dns_updates', 'INSERT ' || domain || ' ' || iface_name || '.' || current_hostname || ' A ' || address_ttl || ' ' || ip_addr);
> +          ELSIF (TG_OP = 'DELETE' AND TG_LEVEL = 'ROW') THEN
> +            IF EXISTS(SELECT id FROM maasserver_interface WHERE id=OLD.interface_id) THEN
> +                SELECT name, node_config_id INTO iface_name, current_node_config_id FROM maasserver_interface WHERE id=OLD.interface_id;
> +                SELECT node_id INTO current_node_id FROM maasserver_nodeconfig WHERE id=current_node_config_id;
> +                SELECT hostname INTO current_hostname FROM maasserver_node WHERE id=current_node_id;

similarly here

> +                IF EXISTS(SELECT id FROM maasserver_staticipaddress WHERE id=OLD.staticipaddress_id) THEN
> +                  SELECT host(ip) INTO ip_addr FROM maasserver_staticipaddress WHERE id=OLD.staticipaddress_id;
> +                  PERFORM pg_notify('sys_dns_updates', 'DELETE ' || domain || ' ' || current_hostname || ' A ' || ip_addr);
> +                  PERFORM pg_notify('sys_dns_updates', 'DELETE ' || domain || ' ' || iface_name || '.' || current_hostname || ' A ' || ip_addr);
> +                ELSE
> +                  PERFORM pg_notify('sys_dns_updates', 'DELETE-IFACE-IP ' || domain || ' ' || current_hostname || ' A ' || OLD.interface_id);
> +                  PERFORM pg_notify('sys_dns_updates', 'DELETE-IFACE-IP ' || domain || ' ' || current_hostname || ' AAAA ' || OLD.interface_id);
> +                END IF;
> +            END IF;
> +          END IF;
> +          RETURN NULL;
> +        END;
> +        $$ LANGUAGE plpgsql;
> +        """
> +    )
> +
> +
> +dns_dynamic_update_static_ip_address_update = dedent(
> +    """\
> +    CREATE OR REPLACE FUNCTION sys_dns_updates_ip_update()
> +    RETURNS trigger as $$
> +    DECLARE
> +      current_hostname text;
> +      default_domain_id bigint;
> +      domain text;
> +      iface_name text;
> +      address_ttl int;
> +      current_node_config_id bigint;
> +      current_node_id bigint;
> +      current_interface_id bigint;
> +    BEGIN
> +      IF NEW IS DISTINCT FROM OLD THEN
> +        IF EXISTS(SELECT id FROM maasserver_interface_ip_addresses WHERE staticipaddress_id=NEW.id) THEN
> +          SELECT domain_id INTO default_domain_id FROM maasserver_globaldefault LIMIT 1;
> +          SELECT name, COALESCE(ttl, 0) INTO domain, address_ttl FROM maasserver_domain WHERE id=default_domain_id;
> +          SELECT interface_id INTO current_interface_id FROM maasserver_interface_ip_addresses WHERE staticipaddress_id=NEW.id;
> +          SELECT node_config_id, name INTO current_node_config_id, iface_name FROM maasserver_interface WHERE id=current_interface_id;
> +          SELECT node_id INTO current_node_id FROM maasserver_nodeconfig WHERE id=current_node_config_id;

and here

> +          SELECT hostname INTO current_hostname FROM maasserver_node WHERE id=current_node_id;
> +          IF OLD.ip IS NOT NULL THEN
> +            PERFORM pg_notify('sys_dns_updates', 'DELETE ' || domain || ' ' || current_hostname || ' A ' || host(OLD.ip));
> +            PERFORM pg_notify('sys_dns_updates', 'DELETE ' || domain || ' ' || iface_name || '.' || current_hostname || ' A ' || host(OLD.ip));
> +          END IF;
> +          PERFORM pg_notify('sys_dns_updates', 'INSERT ' || domain || ' ' || current_hostname || ' A ' || address_ttl || ' ' || host(NEW.ip));
> +          PERFORM pg_notify('sys_dns_updates', 'INSERT ' || domain || ' ' || iface_name || '.' || current_hostname || ' A ' || address_ttl || ' ' || host(NEW.ip));
> +        END IF;
> +      END IF;
> +      RETURN NULL;
> +    END;
> +    $$ LANGUAGE plpgsql;
> +    """
> +)
> +
> +dns_dynamic_update_node_delete = dedent(
> +    """\
> +    CREATE OR REPLACE FUNCTION sys_dns_updates_maasserver_node_delete()
> +    RETURNS trigger as $$
> +    DECLARE
> +      hostname text;
> +      default_domain_id bigint;
> +      domain text;
> +      address_ttl int;
> +    BEGIN
> +      SELECT domain_id INTO default_domain_id FROM maasserver_globaldefault LIMIT 1;
> +      SELECT name, COALESCE(ttl, 0) INTO domain, address_ttl FROM maasserver_domain WHERE id=default_domain_id;
> +      PERFORM pg_notify('sys_dns_updates', 'DELETE ' || domain || ' ' || OLD.hostname || ' A');

don't we also need to delete AAAA records here?

> +      RETURN NULL;
> +    END;
> +    $$ LANGUAGE plpgsql;
> +    """
> +)
> +
> +
> +dns_dynamic_update_interface_delete = dedent(
> +    """\
> +    CREATE OR REPLACE FUNCTION sys_dns_updates_maasserver_interface_delete()
> +    RETURNS trigger as $$
> +    DECLARE
> +      current_hostname text;
> +      default_domain_id bigint;
> +      domain text;
> +      current_node_id bigint;
> +    BEGIN
> +      SELECT domain_id INTO default_domain_id FROM maasserver_globaldefault LIMIT 1;
> +      SELECT name INTO domain FROM maasserver_domain WHERE id=default_domain_id;
> +      IF EXISTS(SELECT id FROM maasserver_nodeconfig WHERE id=OLD.node_config_id) THEN
> +        SELECT node_id INTO current_node_id FROM maasserver_nodeconfig WHERE id=OLD.node_config_id;
> +        SELECT hostname INTO current_hostname FROM maasserver_node WHERE id=current_node_id;
> +        PERFORM pg_notify('sys_dns_updates', 'DELETE ' || domain || ' ' || OLD.name || '.' || current_hostname || ' A');

and here?

> +      END IF;
> +      RETURN NULL;
> +    END;
> +    $$ LANGUAGE plpgsql;
> +    """
> +)
> +
> +
>  def render_sys_proxy_procedure(proc_name, on_delete=False):
>      """Render a database procedure with name `proc_name` that notifies that a
>      proxy update is needed.


-- 
https://code.launchpad.net/~cgrabowski/maas/+git/maas/+merge/434522
Your team MAAS Committers is subscribed to branch maas:master.