Skip to content

Improve k8s canonical DNS lookup#138

Merged
astrojuanlu merged 1 commit intocanonical:8.0/edgefrom
gboutry:fix/fqdn-resolution
Mar 11, 2026
Merged

Improve k8s canonical DNS lookup#138
astrojuanlu merged 1 commit intocanonical:8.0/edgefrom
gboutry:fix/fqdn-resolution

Conversation

@gboutry
Copy link
Copy Markdown
Contributor

@gboutry gboutry commented Mar 10, 2026

Issue

Fixes #137.

Solution

Replace getfqdn-based Kubernetes hostname resolution with a canonical-name
lookup built on socket.getaddrinfo(..., AI_CANONNAME).

The helper now scans resolver results for a canonical name, handles resolver
failures explicitly, preserves the trailing dot returned by DNS, and updates
the unit tests covering the charm, provider, upgrade, and utility paths.

Checklist

  • I have added or updated any relevant documentation.
  • I have cleaned any remaining cloud resources from my accounts.

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 10, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (8.0/edge@be6df11). Learn more about missing BASE report.

Additional details and impacted files
@@             Coverage Diff             @@
##             8.0/edge     #138   +/-   ##
===========================================
  Coverage            ?   59.91%           
===========================================
  Files               ?       34           
  Lines               ?     6364           
  Branches            ?     1069           
===========================================
  Hits                ?     3813           
  Misses              ?     2233           
  Partials            ?      318           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gboutry gboutry force-pushed the fix/fqdn-resolution branch from d67481a to 3112e57 Compare March 10, 2026 08:01
Replace getfqdn-based k8s hostname resolution
with a canonical-name lookup built on
getaddrinfo(AI_CANONNAME).

Handle resolver failures, keep the trailing
dot returned by DNS, and update the
Kubernetes unit tests for the charm,
provider, upgrade, and utility helpers.

Fixes canonical#137

Signed-off-by: Guillaume Boutry <guillaume.boutry@canonical.com>
@gboutry gboutry force-pushed the fix/fqdn-resolution branch from 3112e57 to 647d6c1 Compare March 10, 2026 08:20
@astrojuanlu
Copy link
Copy Markdown
Contributor

Thank you @gboutry . We'll nudge the CI so that the s390x builds are retried and the integration tests are triggered.

I see you added new unit tests 👍🏼 Any chance to validate whether this PR fixes the original issue?

Comment thread kubernetes/src/utils.py
@astrojuanlu
Copy link
Copy Markdown
Contributor

Backups and subordinate tests are failing, as expected (see #119 (comment))

I will retry the 2 remaining tests a few more times just in case.

Copy link
Copy Markdown
Contributor

@paulomach paulomach left a comment

Choose a reason for hiding this comment

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

Exquisite

@astrojuanlu
Copy link
Copy Markdown
Contributor

Been retrying the last test a few times. Despite not passing, the timeouts seem to be happening in different places, and there's nothing in the logs that suggests that they're caused by this PR. So I'm going to go ahead and merge.

Thanks again @gboutry !

@astrojuanlu astrojuanlu merged commit 4fa3f07 into canonical:8.0/edge Mar 11, 2026
1879 of 2161 checks passed
@gboutry gboutry deleted the fix/fqdn-resolution branch March 11, 2026 08:57
@sinclert-canonical sinclert-canonical added the bug Something isn't working as expected label Mar 11, 2026
astrojuanlu pushed a commit that referenced this pull request Mar 17, 2026
Replace getfqdn-based k8s hostname resolution
with a canonical-name lookup built on
getaddrinfo(AI_CANONNAME).

Handle resolver failures, keep the trailing
dot returned by DNS, and update the
Kubernetes unit tests for the charm,
provider, upgrade, and utility helpers.

Fixes #137

Signed-off-by: Guillaume Boutry <guillaume.boutry@canonical.com>
astrojuanlu pushed a commit that referenced this pull request Mar 17, 2026
Replace getfqdn-based k8s hostname resolution
with a canonical-name lookup built on
getaddrinfo(AI_CANONNAME).

Handle resolver failures, keep the trailing
dot returned by DNS, and update the
Kubernetes unit tests for the charm,
provider, upgrade, and utility helpers.

Fixes #137

Signed-off-by: Guillaume Boutry <guillaume.boutry@canonical.com>
astrojuanlu added a commit that referenced this pull request Mar 17, 2026
Replace getfqdn-based k8s hostname resolution
with a canonical-name lookup built on
getaddrinfo(AI_CANONNAME).

Handle resolver failures, keep the trailing
dot returned by DNS, and update the
Kubernetes unit tests for the charm,
provider, upgrade, and utility helpers.

Fixes #137

Signed-off-by: Guillaume Boutry <guillaume.boutry@canonical.com>
Co-authored-by: Guillaume Boutry <boutryguillaume1@gmail.com>
Comment thread kubernetes/src/utils.py
Comment on lines +101 to +103
for entry in info:
if canonname := entry[3]:
return canonname
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't sure that logic is robust enough with different DNS engines for kubernetes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@delgod Are you concerned about any of those in particular? Tests have been passing on CI with microk8s and on my laptop with k3s. We're eyeing Canonical K8s but haven't finished it yet #142

Copy link
Copy Markdown
Member

@delgod delgod Mar 18, 2026

Choose a reason for hiding this comment

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

I think we should add the following function

import re

def is_srv_dns_record(s: str) -> bool:
    """Match pattern: pod-name.service-name.srv.some.domain.name."""
    pattern = r'^[a-z0-9]([a-z0-9-]*[a-z0-9])?\.([a-z0-9]([a-z0-9-]*[a-z0-9])?\.)+srv(\.[a-z0-9]([a-z0-9-]*[a-z0-9])?){2,}\.?$'
    return bool(re.match(pattern, s, re.IGNORECASE))

and use it here

Suggested change
for entry in info:
if canonname := entry[3]:
return canonname
for entry in info:
if canonname := entry[3] and is_srv_dns_record(canonname):
return canonname

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working as expected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants