test(dns): Verify DNS Endpoint Provider aggregation logic#830
Open
fabikova wants to merge 1 commit into
Open
Conversation
fabikova
force-pushed
the
feature/issue-811-dns-endpoint-provider-tests
branch
from
ef5e5db to
25cd7db
Compare
averevki
reviewed
Jan 6, 2026
fabikova
force-pushed
the
feature/issue-811-dns-endpoint-provider-tests
branch
from
1d9fab1 to
a9ed4ad
Compare
averevki
reviewed
Jan 28, 2026
|
Warning Review limit reached
More reviews will be available in 30 minutes and 33 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a pytest module that creates an endpoint provider Secret, a destination DNSRecord (zone), two source DNSRecords referencing the same provider, commits them, waits for readiness, and verifies DNS resolves each source hostname to its expected IP. ChangesDNS Endpoint Provider Aggregation Test
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
testsuite/tests/singlecluster/gateway/dnspolicy/dns_records/test_dns_endpoint_provider.py (2)
95-98:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDNS verification is too weak and timing-sensitive.
Single-shot resolution plus membership-only checks can be flaky and can miss over-aggregation. Please poll for resolution and assert exactly one A record per source hostname.
Suggested tightening
+import time ... def test_records_accessible(hostname): """Verify that endpoints are merged and accessible via DNS""" - assert SOURCE_IP1 in {r.address for r in dns.resolver.resolve(f"src1.{hostname.hostname}")} - assert SOURCE_IP2 in {r.address for r in dns.resolver.resolve(f"src2.{hostname.hostname}")} + for expected_ip, host in ( + (SOURCE_IP1, f"src1.{hostname.hostname}"), + (SOURCE_IP2, f"src2.{hostname.hostname}"), + ): + answers = None + for _ in range(30): + try: + answers = list(dns.resolver.resolve(host, "A")) + if expected_ip in {r.address for r in answers}: + break + except dns.resolver.DNSException: + pass + time.sleep(2) + + assert answers is not None, f"{host} did not resolve" + addresses = [r.address for r in answers] + assert addresses == [expected_ip], f"Unexpected A records for {host}: {addresses}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@testsuite/tests/singlecluster/gateway/dnspolicy/dns_records/test_dns_endpoint_provider.py` around lines 95 - 98, The current test_records_accessible uses a single-shot membership check which is flaky; modify test_records_accessible to poll (with a short timeout/retry loop) calling dns.resolver.resolve for f"src1.{hostname.hostname}" and f"src2.{hostname.hostname}" until each returns exactly one A record, then assert that the single record's address equals SOURCE_IP1 and SOURCE_IP2 respectively (i.e., replace the set-membership asserts with length==1 checks and exact address equality), using the same dns.resolver.resolve call sites and the SOURCE_IP1/SOURCE_IP2 and hostname.hostname identifiers to locate and update the logic.
21-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEndpoint-provider secret payload does not match the endpoint-provider contract.
This fixture writes AWS credential keys, but the endpoint-provider flow is expected to use endpoint-provider config keys (e.g., GVR/zone-label settings). As written, the test can miss the intended aggregation path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@testsuite/tests/singlecluster/gateway/dnspolicy/dns_records/test_dns_endpoint_provider.py` around lines 21 - 31, The fixture endpoint_provider_secret is creating AWS credential keys but should supply the endpoint-provider config keys used by the endpoint-provider contract; update the secret_data passed to Secret.create_instance in endpoint_provider_secret to include the expected endpoint-provider fields (e.g., provider, gvr and zone-label/zone_label or equivalent keys used by your endpoint-provider implementation) instead of AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY so tests exercise the endpoint-provider aggregation path correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@testsuite/tests/singlecluster/gateway/dnspolicy/dns_records/test_dns_endpoint_provider.py`:
- Around line 95-98: The current test_records_accessible uses a single-shot
membership check which is flaky; modify test_records_accessible to poll (with a
short timeout/retry loop) calling dns.resolver.resolve for
f"src1.{hostname.hostname}" and f"src2.{hostname.hostname}" until each returns
exactly one A record, then assert that the single record's address equals
SOURCE_IP1 and SOURCE_IP2 respectively (i.e., replace the set-membership asserts
with length==1 checks and exact address equality), using the same
dns.resolver.resolve call sites and the SOURCE_IP1/SOURCE_IP2 and
hostname.hostname identifiers to locate and update the logic.
- Around line 21-31: The fixture endpoint_provider_secret is creating AWS
credential keys but should supply the endpoint-provider config keys used by the
endpoint-provider contract; update the secret_data passed to
Secret.create_instance in endpoint_provider_secret to include the expected
endpoint-provider fields (e.g., provider, gvr and zone-label/zone_label or
equivalent keys used by your endpoint-provider implementation) instead of
AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY so tests exercise the endpoint-provider
aggregation path correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b8a1f6a-ba8f-4422-8f86-89ae5e313c6b
📒 Files selected for processing (3)
testsuite/kuadrant/policy/dns.pytestsuite/tests/singlecluster/gateway/dnspolicy/dns_records/__init__.pytestsuite/tests/singlecluster/gateway/dnspolicy/dns_records/test_dns_endpoint_provider.py
💤 Files with no reviewable changes (1)
- testsuite/kuadrant/policy/dns.py
fabikova
force-pushed
the
feature/issue-811-dns-endpoint-provider-tests
branch
from
07ed93e to
84adb7f
Compare
fabikova
force-pushed
the
feature/issue-811-dns-endpoint-provider-tests
branch
2 times, most recently
from
5348a3a to
742b85b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
testsuite/tests/singlecluster/gateway/dnspolicy/dns_records/test_dns_endpoint_provider.py (1)
23-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
ENDPOINT_GVRinstead ofENDPOINT_GVKin the endpoint provider secret payload.Line 23 uses
ENDPOINT_GVK, but this flow expects the endpoint provider config keyENDPOINT_GVR. With the wrong key, aggregation can silently fail because source records are not mapped to the provider contract.Suggested fix
secret_data = { - "ENDPOINT_GVK": "kuadrant.io/v1alpha1.DNSRecord", + "ENDPOINT_GVR": "kuadrant.io/v1alpha1.dnsrecords", "ENDPOINT_ZONE_RECORD_LABEL": "kuadrant.io/zone-record=true" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@testsuite/tests/singlecluster/gateway/dnspolicy/dns_records/test_dns_endpoint_provider.py` around lines 23 - 25, The secret payload uses the wrong key name: replace the ENDPOINT_GVK entry with ENDPOINT_GVR so the endpoint provider config uses the expected contract key; specifically update the payload map that currently contains "ENDPOINT_GVK" to instead use "ENDPOINT_GVR" (preserving the same value "kuadrant.io/v1alpha1.DNSRecord") so aggregation maps source records to the provider contract correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@testsuite/tests/singlecluster/gateway/dnspolicy/dns_records/test_dns_endpoint_provider.py`:
- Around line 94-103: The test_records_accessible(hostname) currently does
one-shot DNS resolves for src1.{hostname.hostname} and src2.{hostname.hostname}
(src1_answers, src2_answers) which can flake; change it to poll with a bounded
timeout/retry loop (or call the existing DNSRecord wait helper) until the
answers stabilize before doing the final assertions. Specifically, replace the
direct dns.resolver.resolve calls with a retry loop that re-resolves both
hostnames, sleeps between attempts, and exits early when each query returns an A
record whose address equals SOURCE_IP1 and SOURCE_IP2 respectively (or when
timeout is reached), then run the len(...) and address equality asserts.
---
Duplicate comments:
In
`@testsuite/tests/singlecluster/gateway/dnspolicy/dns_records/test_dns_endpoint_provider.py`:
- Around line 23-25: The secret payload uses the wrong key name: replace the
ENDPOINT_GVK entry with ENDPOINT_GVR so the endpoint provider config uses the
expected contract key; specifically update the payload map that currently
contains "ENDPOINT_GVK" to instead use "ENDPOINT_GVR" (preserving the same value
"kuadrant.io/v1alpha1.DNSRecord") so aggregation maps source records to the
provider contract correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8c941ffe-d3f4-4562-be03-ebca0d0dc763
📒 Files selected for processing (3)
testsuite/kuadrant/policy/dns.pytestsuite/tests/singlecluster/gateway/dnspolicy/dns_records/__init__.pytestsuite/tests/singlecluster/gateway/dnspolicy/dns_records/test_dns_endpoint_provider.py
💤 Files with no reviewable changes (1)
- testsuite/kuadrant/policy/dns.py
Comment on lines
+94
to
+103
| def test_records_accessible(hostname): | ||
| """Verify that endpoints are merged and accessible via DNS""" | ||
| src1_answers = dns.resolver.resolve(f"src1.{hostname.hostname}") | ||
| src2_answers = dns.resolver.resolve(f"src2.{hostname.hostname}") | ||
|
|
||
| assert len(src1_answers) == 1 | ||
| assert len(src2_answers) == 1 | ||
|
|
||
| assert src1_answers[0].address == SOURCE_IP1 | ||
| assert src2_answers[0].address == SOURCE_IP2 |
There was a problem hiding this comment.
Avoid one-shot DNS lookups for this e2e assertion path.
The test performs a single resolve per hostname (Lines 96-97). External DNS propagation is eventually consistent, so this can cause intermittent failures even when records are correct. Add a bounded wait/poll step (or reuse the DNSRecord wait helper introduced in this PR) before final equality assertions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@testsuite/tests/singlecluster/gateway/dnspolicy/dns_records/test_dns_endpoint_provider.py`
around lines 94 - 103, The test_records_accessible(hostname) currently does
one-shot DNS resolves for src1.{hostname.hostname} and src2.{hostname.hostname}
(src1_answers, src2_answers) which can flake; change it to poll with a bounded
timeout/retry loop (or call the existing DNSRecord wait helper) until the
answers stabilize before doing the final assertions. Specifically, replace the
direct dns.resolver.resolve calls with a retry loop that re-resolves both
hostnames, sleeps between attempts, and exits early when each query returns an A
record whose address equals SOURCE_IP1 and SOURCE_IP2 respectively (or when
timeout is reached), then run the len(...) and address equality asserts.
742b85b to
0a3e017
Compare
Signed-off-by: Martina Fabikova <mfabikov@redhat.com>
fabikova
force-pushed
the
feature/issue-811-dns-endpoint-provider-tests
branch
from
0a3e017 to
a2f50f5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR implements the test scenarios defined in issue #811 to verify the new DNS Endpoint Provider functionality.
It validates the aggregation workflow where multiple "Source" DNSRecords (acting as endpoint feeders) merge their endpoints into a single central "Destination" DNSRecord (acting as a Zone), which then propagates the records to an upstream provider (e.g., AWS).
Scenarios covered:
endpointprovider.Closes #811
Changes
testsuite/tests/singlecluster/gateway/dnspolicy/dns_records/test_dns_endpoint_provider.pywhich implements the test cases specified in the issue.DNSRecordclass intestsuite/kuadrant/policy/dns.pywith:wait_for_endpoints_mergedwait_until_resolvesSummary by CodeRabbit
Tests
Chores