Skip to content

Conversation

ScheererJ
Copy link
Member

How to categorize this PR?

/area networking
/area robustness
/area control-plane
/kind enhancement

What this PR does / why we need it:

Allow non-canonical IPv6 addresses in DNSRecord validation.

Previously, the validation of DNSRecord resources checked the validity of IP addresses using a helper function from kubernetes. However, this function disallows non-canonical IPv6 addresses, e.g. 2001:db8:0:0:0:0:0:1. Unfortunately, there are some infrastructures, e.g. GCP, which use non-canonical IPv6 addresses for resources like load balancers. It also seems reasonable to allow them in DNSRecord resources.
This change adapts the validation accordingly so that non-canonical IPv6 addresses are allowed.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

`DNSRecord` may now use non-canonical IPv6 addresses.

/cc @MartinWeindel

Previously, the validation of `DNSRecord` resources checked the validity
of IP addresses using a helper function from kubernetes. However, this
function disallows non-canonical IPv6 addresses, e.g.
2001:db8:0:0:0:0:0:1. Unfortunately, there are some infrastructures,
e.g. GCP, which use non-canonical IPv6 addresses for resources like load
balancers. It also seems reasonable to allow them in `DNSRecord`
resources.
This change adapts the validation accordingly so that non-canonical IPv6
addresses are allowed.
@gardener-prow gardener-prow bot requested a review from MartinWeindel August 4, 2025 15:33
@gardener-prow gardener-prow bot added area/networking Networking related area/robustness Robustness, reliability, resilience related area/control-plane Control plane related kind/enhancement Enhancement, improvement, extension cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 4, 2025
@MartinWeindel
Copy link
Member

/lgtm

@gardener-prow gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label Aug 5, 2025
Copy link
Contributor

gardener-prow bot commented Aug 5, 2025

LGTM label has been added.

Git tree hash: 07c2cd00c42347e99213be9b5e285ce01eb7cc28

Copy link
Contributor

@plkokanov plkokanov left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this out and adding the unit test so it's not regressed in the future.
Potentially, I think you could also use

diff --git a/pkg/apis/extensions/validation/dnsrecord.go b/pkg/apis/extensions/validation/dnsrecord.go
index d6a0ba09d3..83aff88b26 100644
--- a/pkg/apis/extensions/validation/dnsrecord.go
+++ b/pkg/apis/extensions/validation/dnsrecord.go
@@ -113,7 +113,7 @@ func validateValue(recordType extensionsv1alpha1.DNSRecordType, value string, fl
 
        switch recordType {
        case extensionsv1alpha1.DNSRecordTypeA, extensionsv1alpha1.DNSRecordTypeAAAA:
-               allErrs = append(allErrs, isValidIP(fldPath, value)...)
+               allErrs = append(allErrs, validation.IsValidIPForLegacyField(fldPath, value, false, nil)...)
        case extensionsv1alpha1.DNSRecordTypeCNAME:
                allErrs = append(allErrs, validation.IsFullyQualifiedDomainName(fldPath, value)...)
        }

@ScheererJ
Copy link
Member Author

Thanks for taking care of this out and adding the unit test so it's not regressed in the future. Potentially, I think you could also use

diff --git a/pkg/apis/extensions/validation/dnsrecord.go b/pkg/apis/extensions/validation/dnsrecord.go
index d6a0ba09d3..83aff88b26 100644
--- a/pkg/apis/extensions/validation/dnsrecord.go
+++ b/pkg/apis/extensions/validation/dnsrecord.go
@@ -113,7 +113,7 @@ func validateValue(recordType extensionsv1alpha1.DNSRecordType, value string, fl
 
        switch recordType {
        case extensionsv1alpha1.DNSRecordTypeA, extensionsv1alpha1.DNSRecordTypeAAAA:
-               allErrs = append(allErrs, isValidIP(fldPath, value)...)
+               allErrs = append(allErrs, validation.IsValidIPForLegacyField(fldPath, value, false, nil)...)
        case extensionsv1alpha1.DNSRecordTypeCNAME:
                allErrs = append(allErrs, validation.IsFullyQualifiedDomainName(fldPath, value)...)
        }

Yes, validation.IsValidIPForLegacyField(...) could be an option. I was put off by the comment of the function (see here) as it suggests to use it only for legacy fields. We do not use it for a legacy field. Hence, it felt weird to use it.

@plkokanov
Copy link
Contributor

/lgtm
/approve

Copy link
Contributor

gardener-prow bot commented Aug 5, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: plkokanov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gardener-prow gardener-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2025
@gardener-prow gardener-prow bot merged commit b821199 into gardener:master Aug 5, 2025
19 checks passed
Duciwuci pushed a commit to stackitcloud/gardener that referenced this pull request Sep 1, 2025
…er#12667)

Previously, the validation of `DNSRecord` resources checked the validity
of IP addresses using a helper function from kubernetes. However, this
function disallows non-canonical IPv6 addresses, e.g.
2001:db8:0:0:0:0:0:1. Unfortunately, there are some infrastructures,
e.g. GCP, which use non-canonical IPv6 addresses for resources like load
balancers. It also seems reasonable to allow them in `DNSRecord`
resources.
This change adapts the validation accordingly so that non-canonical IPv6
addresses are allowed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/control-plane Control plane related area/networking Networking related area/robustness Robustness, reliability, resilience related cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. kind/enhancement Enhancement, improvement, extension lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants