Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow specification of targets in VirtualServer's spec.externalDNS field #7009

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions config/crd/bases/k8s.nginx.org_virtualservers.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,11 @@ spec:
type: integer
recordType:
type: string
targets:
description: Targets stores a list of specified targets
items:
type: string
type: array
type: object
gunzip:
type: boolean
Expand Down
5 changes: 5 additions & 0 deletions deploy/crds.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1415,6 +1415,11 @@ spec:
type: integer
recordType:
type: string
targets:
description: Targets stores a list of specified targets
items:
type: string
type: array
type: object
gunzip:
type: boolean
Expand Down
78 changes: 41 additions & 37 deletions internal/externaldns/sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,17 @@

import (
"context"
"errors"
"fmt"

"github.com/google/go-cmp/cmp"
nl "github.com/nginxinc/kubernetes-ingress/internal/logger"
vsapi "github.com/nginxinc/kubernetes-ingress/pkg/apis/configuration/v1"
extdnsapi "github.com/nginxinc/kubernetes-ingress/pkg/apis/externaldns/v1"
externaldns_validation "github.com/nginxinc/kubernetes-ingress/pkg/apis/externaldns/validation"
clientset "github.com/nginxinc/kubernetes-ingress/pkg/client/clientset/versioned"
extdnslisters "github.com/nginxinc/kubernetes-ingress/pkg/client/listers/externaldns/v1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
validators "k8s.io/apimachinery/pkg/util/validation"
"k8s.io/apimachinery/pkg/util/validation/field"

apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -45,16 +43,17 @@
}
l := nl.LoggerFromContext(ctx)

if vs.Status.ExternalEndpoints == nil {
if vs.Status.ExternalEndpoints == nil && len(vs.Spec.ExternalDNS.Targets) == 0 {
// It can take time for the external endpoints to sync - kick it back to the queue
nl.Info(l, "Failed to determine external endpoints - retrying")
return fmt.Errorf("failed to determine external endpoints")
}

targets, recordType, err := getValidTargets(ctx, vs.Status.ExternalEndpoints)
targets, recordType, err := getValidTargets(vs.Status.ExternalEndpoints, vs.Spec.ExternalDNS.Targets)
if err != nil {
nl.Error(l, "Invalid external endpoint")
rec.Eventf(vs, corev1.EventTypeWarning, reasonBadConfig, "Invalid external endpoint")
errorText := fmt.Sprintf("Invalid targets: %v", err)
nl.Error(l, errorText)
rec.Event(vs, corev1.EventTypeWarning, reasonBadConfig, errorText)
return err
}

Expand Down Expand Up @@ -103,47 +102,52 @@
}
}

func getValidTargets(ctx context.Context, endpoints []vsapi.ExternalEndpoint) (extdnsapi.Targets, string, error) {
var targets extdnsapi.Targets
var recordType string
var recordA bool
var recordCNAME bool
var recordAAAA bool
var err error
l := nl.LoggerFromContext(ctx)
nl.Debugf(l, "Going through endpoints %v", endpoints)
func getValidTargets(endpoints []vsapi.ExternalEndpoint, chosenTargets extdnsapi.Targets) (extdnsapi.Targets, string, error) {
if len(chosenTargets) > 0 {
return externaldns_validation.ValidateTargetsAndDetermineRecordType(chosenTargets)
}

var ipv4Targets, ipv6Targets, cnameTargets []string
for _, e := range endpoints {
if e.IP != "" {
nl.Debugf(l, "IP is defined: %v", e.IP)
if errMsg := validators.IsValidIP(field.NewPath(""), e.IP); len(errMsg) > 0 {
ip := netutils.ParseIPSloppy(e.IP)
if ip == nil {
continue
}
ip := netutils.ParseIPSloppy(e.IP)
if ip.To4() != nil {
recordA = true
ipv4Targets = append(ipv4Targets, e.IP)
} else {
recordAAAA = true
ipv6Targets = append(ipv6Targets, e.IP)
}
targets = append(targets, e.IP)
} else if e.Hostname != "" {
nl.Debugf(l, "Hostname is defined: %v", e.Hostname)
targets = append(targets, e.Hostname)
recordCNAME = true
cnameTargets = append(cnameTargets, e.Hostname)
}
}
if len(targets) == 0 {
return targets, recordType, errors.New("valid targets not defined")
}
if recordA {
recordType = recordTypeA
} else if recordAAAA {
recordType = recordTypeAAAA
} else if recordCNAME {
recordType = recordTypeCNAME
} else {
err = errors.New("recordType could not be determined")

// priority: prefer IPv4 > IPv6 > CNAME
if len(ipv4Targets) > 0 {
if err := externaldns_validation.IsUnique(ipv4Targets); err != nil {
return nil, "", err
}

Check warning on line 131 in internal/externaldns/sync.go

View check run for this annotation

Codecov / codecov/patch

internal/externaldns/sync.go#L130-L131

Added lines #L130 - L131 were not covered by tests
return ipv4Targets, "A", nil
} else if len(ipv6Targets) > 0 {
if err := externaldns_validation.IsUnique(ipv6Targets); err != nil {
return nil, "", err
}

Check warning on line 136 in internal/externaldns/sync.go

View check run for this annotation

Codecov / codecov/patch

internal/externaldns/sync.go#L135-L136

Added lines #L135 - L136 were not covered by tests
return ipv6Targets, "AAAA", nil
} else if len(cnameTargets) > 0 {
if err := externaldns_validation.IsUnique(cnameTargets); err != nil {
return nil, "", err
}

Check warning on line 141 in internal/externaldns/sync.go

View check run for this annotation

Codecov / codecov/patch

internal/externaldns/sync.go#L140-L141

Added lines #L140 - L141 were not covered by tests
for _, h := range cnameTargets {
if err := externaldns_validation.IsFullyQualifiedDomainName(h); err != nil {
return nil, "", fmt.Errorf("%w: target %q is invalid: %w", externaldns_validation.ErrTypeInvalid, h, err)
}

Check warning on line 145 in internal/externaldns/sync.go

View check run for this annotation

Codecov / codecov/patch

internal/externaldns/sync.go#L144-L145

Added lines #L144 - L145 were not covered by tests
}
return cnameTargets, "CNAME", nil
}
return targets, recordType, err

return nil, "", fmt.Errorf("no valid external endpoints found")
}

func buildDNSEndpoint(ctx context.Context, extdnsLister extdnslisters.DNSEndpointLister, vs *vsapi.VirtualServer, targets extdnsapi.Targets, recordType string) (*extdnsapi.DNSEndpoint, *extdnsapi.DNSEndpoint, error) {
Expand Down
28 changes: 21 additions & 7 deletions internal/externaldns/sync_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,11 @@ func (EventRecorder) AnnotatedEventf(runtime.Object, map[string]string, string,
func TestGetValidTargets(t *testing.T) {
t.Parallel()
tt := []struct {
name string
wantTargets extdnsapi.Targets
wantRecord string
endpoints []vsapi.ExternalEndpoint
name string
wantTargets extdnsapi.Targets
wantRecord string
endpoints []vsapi.ExternalEndpoint
chosenEndpoints []string
}{
{
name: "from external endpoint with IPv4",
Expand Down Expand Up @@ -63,21 +64,34 @@ func TestGetValidTargets(t *testing.T) {
},
{
name: "from external endpoint with multiple targets",
wantTargets: extdnsapi.Targets{"2001:db8:0:0:0:0:2:1", "10.2.3.4"},
wantTargets: extdnsapi.Targets{"10.2.3.4"},
wantRecord: "A",
endpoints: []vsapi.ExternalEndpoint{
{
IP: "2001:db8:0:0:0:0:2:1",
IP: "2001:db8:0:0:0:0:2:1", // This IPv6 record will be ignored, as the priority is IPv4> IPv6> CNAME
},
{
IP: "10.2.3.4",
},
},
},
{
name: "from external endpoint with multiple targets",
wantTargets: extdnsapi.Targets{"1.2.3.4"},
wantRecord: "A",
endpoints: []vsapi.ExternalEndpoint{
{
IP: "10.2.3.4", // This extrenal IP will be ignored, as IPs have been chosen in the VS spec
},
},
chosenEndpoints: []string{
"1.2.3.4",
},
},
}
for _, tc := range tt {
t.Run(tc.name, func(t *testing.T) {
targets, recordType, err := getValidTargets(context.Background(), tc.endpoints)
targets, recordType, err := getValidTargets(tc.endpoints, tc.chosenEndpoints)
if err != nil {
t.Fatal(err)
}
Expand Down
2 changes: 2 additions & 0 deletions pkg/apis/configuration/v1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ type VirtualServerListener struct {
type ExternalDNS struct {
Enable bool `json:"enable"`
RecordType string `json:"recordType,omitempty"`
// Targets stores a list of specified targets
Targets []string `json:"targets,omitempty"`
// TTL for the record
RecordTTL int64 `json:"recordTTL,omitempty"`
// Labels stores labels defined for the Endpoint
Expand Down
5 changes: 5 additions & 0 deletions pkg/apis/configuration/v1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 12 additions & 2 deletions pkg/apis/configuration/validation/virtualserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
"github.com/dlclark/regexp2"
"github.com/nginxinc/kubernetes-ingress/internal/configs"
v1 "github.com/nginxinc/kubernetes-ingress/pkg/apis/configuration/v1"
externaldns "github.com/nginxinc/kubernetes-ingress/pkg/apis/externaldns/validation"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/util/validation"
"k8s.io/apimachinery/pkg/util/validation/field"
Expand Down Expand Up @@ -202,10 +203,19 @@
// valid, externalDNS is not required
return nil
}
allErrs := field.ErrorList{}
if !vsv.isExternalDNSEnabled {
return field.ErrorList{field.Forbidden(fieldPath, "field requires externalDNS enablement")}
allErrs = append(allErrs, field.Forbidden(fieldPath, "field requires externalDNS enablement"))
}
return nil

if len(ed.Targets) > 0 {
_, _, err := externaldns.ValidateTargetsAndDetermineRecordType(ed.Targets)
if err != nil {
allErrs = append(allErrs, field.Forbidden(fieldPath.Child("targets"), err.Error()))
}

Check warning on line 215 in pkg/apis/configuration/validation/virtualserver.go

View check run for this annotation

Codecov / codecov/patch

pkg/apis/configuration/validation/virtualserver.go#L212-L215

Added lines #L212 - L215 were not covered by tests
}

return allErrs
}

func validateTLSRedirect(redirect *v1.TLSRedirect, fieldPath *field.Path) field.ErrorList {
Expand Down
65 changes: 61 additions & 4 deletions pkg/apis/externaldns/validation/externaldns.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,63 @@ import (
v1 "github.com/nginxinc/kubernetes-ingress/pkg/apis/externaldns/v1"
"k8s.io/apimachinery/pkg/util/validation"
"k8s.io/apimachinery/pkg/util/validation/field"
netutils "k8s.io/utils/net"
)

// ValidateTargetsAndDetermineRecordType validates chosen endpoints
func ValidateTargetsAndDetermineRecordType(targets []string) ([]string, string, error) {
var recordA, recordAAAA, recordCNAME bool

for _, t := range targets {
if errMsg := validation.IsValidIP(field.NewPath(""), t); len(errMsg) == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

IsValidIP is a wrapper for netutils.ParseIPSloppy(), there is probably no need to call it twice

ip := netutils.ParseIPSloppy(t)
if ip.To4() != nil {
recordA = true
} else {
recordAAAA = true
}
} else {
if err := IsFullyQualifiedDomainName(t); err != nil {
return nil, "", fmt.Errorf("%w: target %q is invalid: %w", ErrTypeInvalid, t, err)
}
recordCNAME = true
}
}

if err := IsUnique(targets); err != nil {
return nil, "", err
}

count := 0
if recordA {
count++
}
if recordAAAA {
count++
}
if recordCNAME {
count++
}

if count > 1 {
return nil, "", errors.New("multiple record types (A, AAAA, CNAME) detected; must be homogeneous")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having both A & AAAA records is a valid use case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a valid point. However, given that the VirtualServer.spec.externalDNS object includes a recordType field, indicating that each DNS entry must be homogeneous, it’s reasonable to expect this limitation to a single record type.

}

var recordType string
switch {
case recordA:
recordType = "A"
case recordAAAA:
recordType = "AAAA"
case recordCNAME:
recordType = "CNAME"
default:
return nil, "", errors.New("could not determine record type from targets")
}

return targets, recordType, nil
}

// ValidateDNSEndpoint validates if all DNSEndpoint fields are valid.
func ValidateDNSEndpoint(dnsendpoint *v1.DNSEndpoint) error {
return validateDNSEndpointSpec(&dnsendpoint.Spec)
Expand Down Expand Up @@ -57,15 +112,16 @@ func validateTargets(targets v1.Targets) error {
return fmt.Errorf("%w: target %q is invalid: %s", ErrTypeInvalid, target, errMsg[0])
}
default:
if err := isFullyQualifiedDomainName(target); err != nil {
if err := IsFullyQualifiedDomainName(target); err != nil {
return fmt.Errorf("%w: target %q is invalid, it should be a valid IP address or hostname", ErrTypeInvalid, target)
}
}
}
return isUnique(targets)
return IsUnique(targets)
}

func isUnique(targets v1.Targets) error {
// IsUnique takes a list of targets and makes an error if a target appears more than once
func IsUnique(targets v1.Targets) error {
occurred := make(map[string]bool)
for _, target := range targets {
if occurred[target] {
Expand All @@ -90,7 +146,8 @@ func validateTTL(ttl v1.TTL) error {
return nil
}

func isFullyQualifiedDomainName(name string) error {
// IsFullyQualifiedDomainName checks if a string will be valid for a cname dns record
func IsFullyQualifiedDomainName(name string) error {
if name == "" {
return fmt.Errorf("%w: name not provided", ErrTypeInvalid)
}
Expand Down
Loading
Loading