CORS-4372: Create IPv6 records for GCP#1410
Conversation
pkg/util/aws/dualstack.go: - Removed the "aws specific" capability of determining stack type pkg/util/ipfamily/ipfamily.go: - Created the common function to determine stack type dynamically. pkg/util/ipfamily/ipfamily_test.go: - Generated tests for the stack determination pkg/dns/gcp/provider.go: - Added dual-stack DNS support for GCP *.apps records pkg/operator/controller/dns/controller.go: - Add IP family to the controller call.
openshift-ci-robot
added
the
jira/valid-reference
|
@barbacbd: This pull request references CORS-4372 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
📝 WalkthroughWalkthroughThe changes add dual-stack DNS record support to the GCP provider by introducing an ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/util/ipfamily/ipfamily_test.go (1)
9-45: Consider adding a test case forIPv6.The test covers IPv4 but not the single-stack IPv6 case (
configv1.IPv6). While not critical, adding this case would ensure complete coverage of all expectedIPFamilyTypevalues.Suggested addition
{ name: "IPv4 is not dual-stack", ipFamily: configv1.IPv4, expected: false, }, + { + name: "IPv6 is not dual-stack", + ipFamily: configv1.IPv6, + expected: false, + }, { name: "DualStackIPv4Primary is dual-stack",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/ipfamily/ipfamily_test.go` around lines 9 - 45, TestIsDualStack is missing a case for single-stack IPv6; add a table-driven test entry with name like "IPv6 is not dual-stack", ipFamily: configv1.IPv6 and expected: false so IsDualStack(configv1.IPv6) is asserted; update the tests slice in TestIsDualStack to include this new struct entry (refer to the TestIsDualStack function and the tests []struct with fields name, ipFamily, expected).pkg/dns/gcp/provider_dualstack_test.go (1)
107-139: Replace the trivialTestConfigIPFamilywith tests for actual dual-stack behavior.The
TestConfigIPFamilytest only verifies struct field assignment, which Go guarantees. Meanwhile, theEnsure,Replace, andDeletemethods contain the actual dual-stack logic (filtering IPv4/IPv6, creating/deleting separate A and AAAA records) but have no test coverage. Since this file is specifically for dual-stack testing, it should prioritize tests for these main methods instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/dns/gcp/provider_dualstack_test.go` around lines 107 - 139, Replace the trivial TestConfigIPFamily with table-driven tests that exercise the actual dual-stack logic in Ensure, Replace, and Delete rather than just struct assignment: write tests that construct a Config (with IPFamily set to configv1.IPv4, configv1.IPv6/DualStackIPv4Primary/DualStackIPv6Primary) and call Config.Ensure, Config.Replace, and Config.Delete using a mocked GCP DNS client to assert that the code filters addresses correctly and issues the expected A and/or AAAA record create/update/delete calls (check calls or requests made by the mock), and include cases that validate separate handling of IPv4 vs IPv6 records and order/primary behavior; reference Config, Ensure, Replace, Delete and the IPFamily values when implementing these tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/dns/gcp/provider.go`:
- Around line 230-237: The Delete branch currently always constructs deletions
for A records even when a target is IPv6-only and dual-stack mode is disabled
(mirroring the Ensure issue); update Delete in pkg/dns/gcp/provider.go (the
Delete method) to detect the same condition that Ensure uses for A records with
IPv6-only targets and, when dual-stack is not enabled, skip creating/deleting
the malformed A record (no-op) instead of calling resourceRecordSet(record);
keep the existing http.StatusNotFound handling unchanged so behavior remains
consistent with Ensure.
- Around line 125-132: The current logic in the block handling RecordType ==
ARecordType falls back to resourceRecordSet(record) even when targets are
IPv6-only, which would produce invalid A records; update the code in the
provider.go path that builds additions (the branch that sets additions =
[]*gdnsv1.ResourceRecordSet{resourceRecordSet(record)}) to detect when
RecordType == ARecordType and the record's Targets contain only IPv6 addresses
(no IPv4), and in that case either (a) construct an AAAA record instead (convert
RecordType to AAAA and call resourceRecordSet on a modified record) or (b) skip
creating the record and emit a warning via the logger; ensure the check
references RecordType, ARecordType, IPFamily/dual-stack logic and uses
resourceRecordSet(record) only for valid IPv4 targets.
---
Nitpick comments:
In `@pkg/dns/gcp/provider_dualstack_test.go`:
- Around line 107-139: Replace the trivial TestConfigIPFamily with table-driven
tests that exercise the actual dual-stack logic in Ensure, Replace, and Delete
rather than just struct assignment: write tests that construct a Config (with
IPFamily set to configv1.IPv4,
configv1.IPv6/DualStackIPv4Primary/DualStackIPv6Primary) and call Config.Ensure,
Config.Replace, and Config.Delete using a mocked GCP DNS client to assert that
the code filters addresses correctly and issues the expected A and/or AAAA
record create/update/delete calls (check calls or requests made by the mock),
and include cases that validate separate handling of IPv4 vs IPv6 records and
order/primary behavior; reference Config, Ensure, Replace, Delete and the
IPFamily values when implementing these tests.
In `@pkg/util/ipfamily/ipfamily_test.go`:
- Around line 9-45: TestIsDualStack is missing a case for single-stack IPv6; add
a table-driven test entry with name like "IPv6 is not dual-stack", ipFamily:
configv1.IPv6 and expected: false so IsDualStack(configv1.IPv6) is asserted;
update the tests slice in TestIsDualStack to include this new struct entry
(refer to the TestIsDualStack function and the tests []struct with fields name,
ipFamily, expected).
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 67f53d13-2707-4a92-9f73-ddbaf2c1f3d8
📒 Files selected for processing (6)
pkg/dns/gcp/provider.gopkg/dns/gcp/provider_dualstack_test.gopkg/operator/controller/dns/controller.gopkg/util/aws/dualstack.gopkg/util/ipfamily/ipfamily.gopkg/util/ipfamily/ipfamily_test.go
| } else { | ||
| // Not dual-stack, create A record as-is. | ||
| additions = []*gdnsv1.ResourceRecordSet{resourceRecordSet(record)} | ||
| } | ||
| } else { | ||
| // For non-A records (CNAME, etc.), create the record as-is. | ||
| additions = []*gdnsv1.ResourceRecordSet{resourceRecordSet(record)} | ||
| } |
There was a problem hiding this comment.
Potential issue: IPv6-only targets may create invalid A records.
When RecordType is ARecordType and targets contain only IPv6 addresses (no IPv4), but IPFamily is not dual-stack, the code falls through to line 127 and calls resourceRecordSet(record). This creates an A record with IPv6 addresses in Rrdatas, which is invalid—IPv6 addresses must be in AAAA records.
Consider handling this edge case by checking if targets contain only IPv6 addresses and creating an AAAA record instead, or skipping record creation entirely with a warning.
Suggested fix
} else {
- // Not dual-stack, create A record as-is.
- additions = []*gdnsv1.ResourceRecordSet{resourceRecordSet(record)}
+ // Not dual-stack. Create A record with IPv4 targets only.
+ // If there are IPv6 targets but no IPv4, this is unexpected for A record type.
+ if len(ipv4Targets) > 0 {
+ aRecord := &gdnsv1.ResourceRecordSet{
+ Name: record.Spec.DNSName,
+ Rrdatas: ipv4Targets,
+ Type: "A",
+ Ttl: record.Spec.RecordTTL,
+ }
+ additions = append(additions, aRecord)
+ } else if len(ipv6Targets) > 0 {
+ log.Info("A record requested but only IPv6 targets found, skipping", "record", record.Spec.DNSName)
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/dns/gcp/provider.go` around lines 125 - 132, The current logic in the
block handling RecordType == ARecordType falls back to resourceRecordSet(record)
even when targets are IPv6-only, which would produce invalid A records; update
the code in the provider.go path that builds additions (the branch that sets
additions = []*gdnsv1.ResourceRecordSet{resourceRecordSet(record)}) to detect
when RecordType == ARecordType and the record's Targets contain only IPv6
addresses (no IPv4), and in that case either (a) construct an AAAA record
instead (convert RecordType to AAAA and call resourceRecordSet on a modified
record) or (b) skip creating the record and emit a warning via the logger;
ensure the check references RecordType, ARecordType, IPFamily/dual-stack logic
and uses resourceRecordSet(record) only for valid IPv4 targets.
| } else { | ||
| // Not dual-stack, delete A record as-is. | ||
| deletions = []*gdnsv1.ResourceRecordSet{resourceRecordSet(record)} | ||
| } | ||
| } else { | ||
| // For non-A records (CNAME, etc.), delete the record as-is. | ||
| deletions = []*gdnsv1.ResourceRecordSet{resourceRecordSet(record)} | ||
| } |
There was a problem hiding this comment.
Same concern applies here: IPv6-only targets with non-dual-stack config.
This mirrors the issue in Ensure. However, since Delete gracefully handles http.StatusNotFound on line 247, attempting to delete a non-existent malformed record won't cause failures—it will just silently succeed. Consider aligning the fix with Ensure for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/dns/gcp/provider.go` around lines 230 - 237, The Delete branch currently
always constructs deletions for A records even when a target is IPv6-only and
dual-stack mode is disabled (mirroring the Ensure issue); update Delete in
pkg/dns/gcp/provider.go (the Delete method) to detect the same condition that
Ensure uses for A records with IPv6-only targets and, when dual-stack is not
enabled, skip creating/deleting the malformed A record (no-op) instead of
calling resourceRecordSet(record); keep the existing http.StatusNotFound
handling unchanged so behavior remains consistent with Ensure.
|
@barbacbd: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
pkg/util/aws/dualstack.go:
pkg/util/ipfamily/ipfamily.go:
pkg/util/ipfamily/ipfamily_test.go:
pkg/dns/gcp/provider.go:
pkg/operator/controller/dns/controller.go: