Adding changes for dyndns tests by danlavu · Pull Request #231 · SSSD/sssd-test-framework

danlavu · 2026-01-29T22:09:09Z

No description provided.

gemini-code-assist · 2026-01-29T22:09:21Z

Summary of Changes

Hello @danlavu, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the SSSD test framework by introducing new network utility functions and refining existing ones. It provides a dedicated method for managing hostnames, improves the nslookup utility for more straightforward test assertions, and adds a convenient way to configure SSSD's dynamic DNS features. These changes aim to streamline network-related test setups and improve the robustness of DNS-related test cases.

Highlights

New hostname utility: Introduced a new hostname method in NetworkUtils to allow getting or setting the hostname of a host, with automatic rollback capabilities for testing.
nslookup return type change: The nslookup method in NetworkUtils has been updated to return the integer exit code of the command, making it more direct and suitable for assertion checks in tests.
Dynamic DNS configuration: Added a dyndns method to SSSDUtils to simplify the configuration of SSSD for dynamic DNS updates, including setting update flags, refresh intervals, and the DNS server.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces a new utility for managing the hostname, adds a common configuration for dynamic DNS, and adjusts the nslookup helper for easier use in test assertions. My review includes suggestions to improve the efficiency and correctness of the new hostname utility and to enhance the flexibility of the dyndns configuration helper by making its parameters configurable.

sssd_test_framework/utils/network.py

sssd_test_framework/utils/sssd.py

danlavu · 2026-01-29T22:11:01Z

Writing the tests now, but they do seem to work.

def test_dynamic_dns__updates_forward_records(client: Client, provider: GenericProvider):
    """
    :title: SSSD updates DNS A records
    :setup:
        1. Check for existing records
        2. Configure and start SSSD
    :steps:
        1. Lookup the hostname
    :expectedresults:
        1. Hostname is found
    :customerscenario: False
    """
    assert client.net.nslookup(f"{client.host.hostname} {provider.server}") != 0, f"Host {client.host.hostname} not found!"
    client.sssd.common.dyndns(provider, f"client.{provider.domain}")
    client.sssd.start()

    assert client.net.nslookup(f"{client.host.hostname} {provider.server}"), f"Host {client.host.hostname} was not found!"

collected 18 items / 15 deselected / 3 selected

tests/test_dynamic_dns.py::test_dynamic_dns__updates_forward_records (ad) 
tests/test_dynamic_dns.py::test_dynamic_dns__updates_forward_records (ipa) 
tests/test_dynamic_dns.py::test_dynamic_dns__updates_forward_records (samba) 

================= 3 passed, 15 deselected in 65.76s (0:01:05) ==================

UPDATE: There was a problem changing the hostname during the test. The teardown did not always work leaving the domain. Updated sssd.conf to contain *_hostname.

spoore1

Just one question about the nslookup return value. Running the provided test passes currently:

PASSED tests/test_dynamic_dns.py::test_dynamic_dns__updates_forward_records (ad)                                                                                                                                    
PASSED tests/test_dynamic_dns.py::test_dynamic_dns__updates_forward_records (ipa)                                                                                                                                   
PASSED tests/test_dynamic_dns.py::test_dynamic_dns__updates_forward_records (samba)

sssd_test_framework/utils/network.py

hostname

danlavu · 2026-02-12T21:36:29Z

Just one question about the nslookup return value. Running the provided test passes currently:

PASSED tests/test_dynamic_dns.py::test_dynamic_dns__updates_forward_records (ad)                                                                                                                                    
PASSED tests/test_dynamic_dns.py::test_dynamic_dns__updates_forward_records (ipa)                                                                                                                                   
PASSED tests/test_dynamic_dns.py::test_dynamic_dns__updates_forward_records (samba)

Actually, it wasn't working correctly. things like if there were two A records but resolved to the wrong ip. Having the return code be 0, but the wrong IP is found, was just messy and complicated.

danlavu · 2026-02-12T21:37:18Z

This test works.

from __future__ import annotations

import time

import pytest

from sssd_test_framework.misc import ip_to_ptr
from sssd_test_framework.roles.client import Client
from sssd_test_framework.roles.generic import GenericProvider
from sssd_test_framework.topology import KnownTopologyGroup


def create_ptr_zones(client: Client, provider: GenericProvider, device: str = "dummy0") -> None:
    """
    Create PTR zones.
    :param client: Client object.
    :type client: Client.
    :param provider: Provider object.
    :type provider: GenericProvider
    :param device: Network device name.
    :type device: str
    """
    provider.dns().clear_forwarders()
    ip_address = client.net.ip(device).address
    if ip_to_ptr(ip_address) not in provider.dns().list_zones():
        provider.dns().zone(ip_to_ptr(ip_address)).create()


def create_dummy_interface(
    client: Client, provider: GenericProvider, device: str | None = "dummy0"
) -> tuple[str, str | None]:
    """
    Create a dummy network interface on the client.
    :param client: Client object.
    :type client: Client.
    :param provider: GenericProvider object.
    :type provider: GenericProvider
    :param device: Name of the dummy network interface.
    :type device: str | None
    :return: IP and device name.
    :rtype: tuple[str, str | None]
    """
    if provider.server == "dc.ad.test":
        ip = "10.255.250.40"
        client.net.ip(device).add_device("10.255.250.40", "255.255.255.0")
    elif provider.server == "master.ipa.test":
        ip = "10.255.251.40"
        client.net.ip("dummy0").add_device("10.255.251.40", "255.255.255.0")
    elif provider.server == "dc.samba.test":
        ip = "10.255.252.40"
        client.net.ip("dummy0").add_device("10.255.252.40", "255.255.255.0")
    else:
        raise ValueError("Invalid provider server!")

    return ip, device

def check_records(client: Client, provider: GenericProvider, hostname: str) -> None:
    """
    Clean hostname records.
    :param client: Client object.
    :type client: Client.
    :param provider: GenericProvider object.
    :type provider: GenericProvider
    :param hostname: Hostname to clean A records for.
    :type hostname: str
    """
    records = client.net.dig(hostname, provider.server)
    if records is not None:
        for record in records:
            provider.dns().zone(provider.domain).delete_record(hostname.split(".")[0], record["data"])


@pytest.mark.topology(KnownTopologyGroup.AnyDC)
@pytest.mark.importance("critical")
def test_dyndns__enabled(client: Client, provider: GenericProvider):
    """
    :title: Dynamic DNS updates all records
    :setup:
        1. Create dummy interface
        2. Create reverse zones
        3. Check for existing records
        4. Configure and start SSSD
    :steps:
        1. Lookup hostname
        2. Lookup ipv4 address
    :expectedresults:
        1. Hostname is found
        2. IP address is found
    customerscenario: True
    """
    short_hostname = client.net.hostname(short=True)
    hostname = f"{short_hostname}.{provider.domain}"
    ip, device = create_dummy_interface(client, provider)
    create_ptr_zones(client, provider)
    check_records(client, provider, hostname)
    client.sssd.common.dyndns()
    client.sssd.start()

    time.sleep(15)

    assert client.net.dig(hostname, provider.server), f"Host {hostname} was not found!"
    assert any(r["data"] == ip for r in client.net.dig(hostname, provider.server)), f"Host {hostname} found with invalid ip!"
    assert client.net.dig(ip, provider.server), f"PTR record for {ip} was not found!"
    assert any(r["data"] == hostname for r in client.net.dig(ip, provider.server)), f"Host {hostname} found with invalid ip!"

justin-stephenson · 2026-02-17T19:20:18Z

sssd_test_framework/misc/__init__.py



-def ip_to_ptr(ip_address: str) -> str:
+def ip_to_ptr(ip_address: str, prefixlen: int | None = None) -> str:


Why not use https://docs.python.org/3/library/ipaddress.html#ipaddress.IPv4Address.reverse_pointer ?

I'm not sure, I think I wrote most of the function, then stumbled upon ipaddress module.

justin-stephenson · 2026-02-17T19:20:59Z

tests/test_misc.py

    [
-        ("192.168.1.0", "1.168.192.in-addr.arpa."),
-        ("2001:db8::1", "1000000000000000000000008bd01002.ip6.arpa."),
+        ("192.168.1.0", "1.168.192.in-addr.arpa", None),


Shouldn't this be the expected value 0.1.168.192.in-addr.arpa ?

Technically, yes, if were looking for the ptr record, but we want the zone file. Which will always omit the last octet.

justin-stephenson · 2026-02-17T19:25:43Z

tests/test_misc.py

+        ("001:db8::", True),
+    ],
+)
+def test_ip_is_valid(value, expected):


test_ip_is_valid is essentially only executing ipaddress.ip_address(ip) which is part of the ipaddress python library. Due to this, I see no value in adding sssd-test-framework tests for this test_ip_is_valid method.

Ack, will remove it then.

sssd_test_framework/topology_controllers.py

justin-stephenson · 2026-02-17T19:30:30Z

sssd_test_framework/roles/ad.py

        return self

-    def delete_record(self, name: str) -> None:
+    def delete_record(self, name: str, data: str) -> None:


Is data argument really needed? That seems a bit cumbersome

It is very cumbersome.

If we wanted to delete the entire record, we couldn't remove it. IPA has sshfp records that I think we should keep? So the record data has to be specified so that record type is deleted.

IMO it would be better to have a record type argument (as enum) which specified the DNS record type instead of requiring passing the record data

I get it, it doesn't make sense to have this here, but it's to make it compatible with the generic provider. I'll rework the logic and make it easier.

jakub-vavra-cz · 2026-02-24T07:59:48Z

sssd_test_framework/roles/ad.py


        :param name: Name of the record.
        :type name: str
+        :param data: Record data.


What is data? Does not look clear to me what is expected there.

It depends on what kind of record, if it's an A record, it will be an IP, a ptr record one octet.

jakub-vavra-cz

I do not see what "data: str" is at first glance and the docstring does not elaborate.
Do we even need data when we want to delete some record by a name?

I think cleaner approach would be to have the dns records as objects (like users, groups, etc.) and keep the "data" needed from CRUD inside them. That way can providers implement the same approach including record removal on clenaup.

danlavu · 2026-02-25T16:37:38Z

I do not see what "data: str" is at first glance and the docstring does not elaborate. Do we even need data when we want to delete some record by a name?

I think cleaner approach would be to have the dns records as objects (like users, groups, etc.) and keep the "data" needed from CRUD inside them. That way can providers implement the same approach including record removal on clenaup.

So the cleanup is working by doing a diff. That is certainly an option that will take more time to implement. The reason why data exists is just for IPA, because when the record is created, it contains SSHFP records, to remove just the A/AAAA/PTR record, it needs the corresponding data to be explicitly defined. It's cumbersome.

I can do one or two things: add some logic inside the method to discover what data should be, and remove the parameter.
Make them into a class as you suggested; this is how servers and zones are, so it does make sense, it'll just take more time, and I'm not opposed to doing it.

The choice is quick, or do it better. Let's get @justin-stephenson thoughts.

justin-stephenson · 2026-02-25T19:24:31Z

The choice is quick, or do it better. Let's get @justin-stephenson thoughts.

I don't have a strong opinion either way, but "better" should always be preferred over "quicker".

I would expect the creating/deleting DNS record method interface only requires 'dns zone', 'record type', and 'name' arguments (similar to ipa dnsrecord-* commands)

danlavu requested review from spoore1 and thalman January 29, 2026 22:09

danlavu assigned thalman and spoore1 Jan 29, 2026

danlavu added the Waiting for review label Jan 29, 2026

gemini-code-assist bot reviewed Jan 29, 2026

View reviewed changes

sssd_test_framework/utils/network.py Outdated Show resolved Hide resolved

sssd_test_framework/utils/sssd.py Outdated Show resolved Hide resolved

danlavu marked this pull request as draft January 29, 2026 22:18

danlavu removed the Waiting for review label Jan 29, 2026

danlavu force-pushed the client-dyndns branch from b5083d4 to 195bf1c Compare January 30, 2026 04:00

danlavu marked this pull request as ready for review January 30, 2026 04:45

danlavu force-pushed the client-dyndns branch 2 times, most recently from 7128453 to 68b8673 Compare February 1, 2026 08:14

spoore1 reviewed Feb 2, 2026

View reviewed changes

sssd_test_framework/utils/network.py Outdated Show resolved Hide resolved

danlavu force-pushed the client-dyndns branch 2 times, most recently from 8a01b91 to 37868ad Compare February 3, 2026 14:01

danlavu marked this pull request as draft February 3, 2026 23:25

danlavu force-pushed the client-dyndns branch 3 times, most recently from 5d02429 to 9bbc0de Compare February 10, 2026 02:33

danlavu changed the title ~~adding dynamic dns comon configuration and changed nslookup to make it more suitable for assertions.~~ Adding changes for dyndns tests Feb 12, 2026

danlavu force-pushed the client-dyndns branch from 9bbc0de to c7c24ff Compare February 12, 2026 21:20

danlavu marked this pull request as ready for review February 12, 2026 21:22

Dan Lavu added 5 commits February 12, 2026 16:25

fixed ip_to_ptr method and added ip_is_valid method

334e798

modified topology controllers to realm join with a provider matching

b4bf967

hostname

Fixing and improving record_delete method.

54639e1

adding common configuration for dyndns

3ae439c

removing nslookup in favor for an improved dig method

d6f6e8b

danlavu force-pushed the client-dyndns branch from c7c24ff to d6f6e8b Compare February 12, 2026 21:26

danlavu added Waiting for review Needs testing Needs to be tested labels Feb 12, 2026

danlavu requested review from jakub-vavra-cz and justin-stephenson February 12, 2026 21:30

danlavu assigned thalman, justin-stephenson and jakub-vavra-cz and unassigned thalman and spoore1 Feb 12, 2026

justin-stephenson reviewed Feb 17, 2026

View reviewed changes

sssd_test_framework/topology_controllers.py Show resolved Hide resolved

justin-stephenson reviewed Feb 17, 2026

View reviewed changes

jakub-vavra-cz reviewed Feb 24, 2026

View reviewed changes

jakub-vavra-cz requested changes Feb 24, 2026

View reviewed changes



		def ip_to_ptr(ip_address: str) -> str:
		def ip_to_ptr(ip_address: str, prefixlen: int \| None = None) -> str:

Conversation

danlavu commented Jan 29, 2026

Uh oh!

gemini-code-assist bot commented Jan 29, 2026

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

danlavu commented Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spoore1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danlavu commented Feb 12, 2026

Uh oh!

danlavu commented Feb 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

justin-stephenson Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakub-vavra-cz left a comment

Choose a reason for hiding this comment

Uh oh!

danlavu commented Feb 25, 2026

Uh oh!

justin-stephenson commented Feb 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

danlavu commented Jan 29, 2026 •

edited

Loading

justin-stephenson Feb 17, 2026 •

edited

Loading