From ec731218893ca4bc8ab652e6508428294b2c2a2d Mon Sep 17 00:00:00 2001 From: Maskym Vavilov Date: Fri, 11 Oct 2024 10:10:32 +0100 Subject: [PATCH] gh-144 create health probe CR from DNSRecord reconciler Signed-off-by: Maskym Vavilov --- Makefile | 2 - api/v1alpha1/dnshealthcheckprobe_types.go | 2 +- api/v1alpha1/dnsrecord_types.go | 4 +- api/v1alpha1/zz_generated.deepcopy.go | 10 - .../dns-operator.clusterserviceversion.yaml | 2 +- .../kuadrant.io_dnshealthcheckprobes.yaml | 2 +- charts/dns-operator/templates/manifests.yaml | 2 +- cmd/main.go | 4 +- .../kuadrant.io_dnshealthcheckprobes.yaml | 2 +- config/deploy/local/kustomization.yaml | 3 + config/manager/manager.yaml | 1 + internal/common/tree.go | 134 ++++ internal/common/tree_test.go | 713 ++++++++++++++++++ .../dnshealthcheckprobe_reconciler.go | 4 + internal/controller/dnsrecord_controller.go | 19 +- internal/controller/dnsrecord_healthchecks.go | 259 +++---- .../controller/dnsrecord_healthchecks_test.go | 181 +++++ internal/controller/helper_test.go | 107 +++ internal/controller/suite_test.go | 2 +- internal/probes/main.go | 5 +- test/e2e/healthcheck_test.go | 184 +---- 21 files changed, 1294 insertions(+), 348 deletions(-) create mode 100644 internal/common/tree.go create mode 100644 internal/common/tree_test.go create mode 100644 internal/controller/dnsrecord_healthchecks_test.go diff --git a/Makefile b/Makefile index a2f13c81..bbcbfdff 100644 --- a/Makefile +++ b/Makefile @@ -241,14 +241,12 @@ run: DIRTY=$(shell hack/check-git-dirty.sh || echo "unknown") run: manifests generate fmt vet ## Run a controller from your host. go run -ldflags "-X main.gitSHA=${GIT_SHA} -X main.dirty=${DIRTY}" ./cmd/main.go --zap-devel --provider inmemory,aws,google,azure - .PHONY: run-with-probes run-with-probes: GIT_SHA=$(shell git rev-parse HEAD || echo "unknown") run-with-probes: DIRTY=$(shell hack/check-git-dirty.sh || echo "unknown") run-with-probes: manifests generate fmt vet ## Run a controller from your host. go run -ldflags "-X main.gitSHA=${GIT_SHA} -X main.dirty=${DIRTY}" ./cmd/main.go --zap-devel --provider inmemory,aws,google,azure --enable-probes - # If you wish built the manager image targeting other platforms you can use the --platform flag. # (i.e. docker build --platform linux/arm64 ). However, you must enable docker buildKit for it. # More info: https://docs.docker.com/develop/develop-images/build_enhancements/ diff --git a/api/v1alpha1/dnshealthcheckprobe_types.go b/api/v1alpha1/dnshealthcheckprobe_types.go index c6c55c84..6fcd0c19 100644 --- a/api/v1alpha1/dnshealthcheckprobe_types.go +++ b/api/v1alpha1/dnshealthcheckprobe_types.go @@ -32,7 +32,7 @@ type DNSHealthCheckProbeSpec struct { // +kubebuilder:validation:Pattern=`^[a-z][a-z0-9\-]+\.([a-z][a-z0-9\-]+\.)*[a-z][a-z0-9\-]+$` Hostname string `json:"hostname,omitempty"` // Address to connect to the host on (IP Address (A Record) or hostname (CNAME)). - // +kubebuilder:validation:Pattern=`^([1-9][0-9]?[0-9]?\.[0-9][0-9]?[0-9]?\.[0-9][0-9]?[0-9]?\.[0-9][0-9]?[0-9]|[a-z][a-z0-9\-]+\.([a-z][a-z0-9\-]+\.)*[a-z][a-z0-9\-]+)?$` + // +kubebuilder:validation:Pattern=`^([1-9][0-9]?[0-9]?\.[0-9][0-9]?[0-9]?\.[0-9][0-9]?[0-9]?\.[0-9][0-9]?[0-9]?|[a-z][a-z0-9\-]+\.([a-z][a-z0-9\-]+\.)*[a-z][a-z0-9\-]+)?$` Address string `json:"address,omitempty"` // Path is the path to append to the host to reach the expected health check. // Must start with "?" or "/", contain only valid URL characters and end with alphanumeric char or "/". For example "/" or "/healthz" are common diff --git a/api/v1alpha1/dnsrecord_types.go b/api/v1alpha1/dnsrecord_types.go index 46b10ee6..0ecd86bc 100644 --- a/api/v1alpha1/dnsrecord_types.go +++ b/api/v1alpha1/dnsrecord_types.go @@ -37,7 +37,7 @@ const HttpsProtocol Protocol = "HTTPS" type HealthCheckSpec struct { // Port to connect to the host on. Must be either 80, 443 or 1024-49151 // +kubebuilder:validation:XValidation:rule="self in [80, 443] || (self >= 1024 && self <= 49151)",message="Only ports 80, 443, 1024-49151 are allowed" - Port *int `json:"port,omitempty"` + Port int `json:"port,omitempty"` // Path is the path to append to the host to reach the expected health check. // Must start with "?" or "/", contain only valid URL characters and end with alphanumeric char or "/". For example "/" or "/healthz" are common // +kubebuilder:validation:Pattern=`^(?:\?|\/)[\w\-.~:\/?#\[\]@!$&'()*+,;=]+(?:[a-zA-Z0-9]|\/){1}$` @@ -52,7 +52,7 @@ type HealthCheckSpec struct { AdditionalHeadersRef *AdditionalHeadersRef `json:"additionalHeadersRef,omitempty"` // FailureThreshold is a limit of consecutive failures that must occur for a host to be considered unhealthy // +kubebuilder:validation:XValidation:rule="self > 0",message="Failure threshold must be greater than 0" - FailureThreshold *int `json:"failureThreshold,omitempty"` + FailureThreshold int `json:"failureThreshold,omitempty"` // AllowInsecureCertificate will instruct the health check probe to not fail on a self-signed or otherwise invalid SSL certificate // this is primarily used in development or testing environments AllowInsecureCertificate bool `json:"allowInsecureCertificate,omitempty"` diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 2d479a79..c9aef4b2 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -314,22 +314,12 @@ func (in *DNSRecordStatus) DeepCopy() *DNSRecordStatus { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *HealthCheckSpec) DeepCopyInto(out *HealthCheckSpec) { *out = *in - if in.Port != nil { - in, out := &in.Port, &out.Port - *out = new(int) - **out = **in - } out.Interval = in.Interval if in.AdditionalHeadersRef != nil { in, out := &in.AdditionalHeadersRef, &out.AdditionalHeadersRef *out = new(AdditionalHeadersRef) **out = **in } - if in.FailureThreshold != nil { - in, out := &in.FailureThreshold, &out.FailureThreshold - *out = new(int) - **out = **in - } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new HealthCheckSpec. diff --git a/bundle/manifests/dns-operator.clusterserviceversion.yaml b/bundle/manifests/dns-operator.clusterserviceversion.yaml index d1e56bc1..96b5f035 100644 --- a/bundle/manifests/dns-operator.clusterserviceversion.yaml +++ b/bundle/manifests/dns-operator.clusterserviceversion.yaml @@ -58,7 +58,7 @@ metadata: capabilities: Basic Install categories: Integration & Delivery containerImage: quay.io/kuadrant/dns-operator:latest - createdAt: "2024-10-02T13:55:34Z" + createdAt: "2024-10-15T15:38:07Z" description: A Kubernetes Operator to manage the lifecycle of DNS resources operators.operatorframework.io/builder: operator-sdk-v1.33.0 operators.operatorframework.io/project_layout: go.kubebuilder.io/v4 diff --git a/bundle/manifests/kuadrant.io_dnshealthcheckprobes.yaml b/bundle/manifests/kuadrant.io_dnshealthcheckprobes.yaml index b355d6f7..ca326c45 100644 --- a/bundle/manifests/kuadrant.io_dnshealthcheckprobes.yaml +++ b/bundle/manifests/kuadrant.io_dnshealthcheckprobes.yaml @@ -62,7 +62,7 @@ spec: address: description: Address to connect to the host on (IP Address (A Record) or hostname (CNAME)). - pattern: ^([1-9][0-9]?[0-9]?\.[0-9][0-9]?[0-9]?\.[0-9][0-9]?[0-9]?\.[0-9][0-9]?[0-9]|[a-z][a-z0-9\-]+\.([a-z][a-z0-9\-]+\.)*[a-z][a-z0-9\-]+)?$ + pattern: ^([1-9][0-9]?[0-9]?\.[0-9][0-9]?[0-9]?\.[0-9][0-9]?[0-9]?\.[0-9][0-9]?[0-9]?|[a-z][a-z0-9\-]+\.([a-z][a-z0-9\-]+\.)*[a-z][a-z0-9\-]+)?$ type: string allowInsecureCertificate: description: |- diff --git a/charts/dns-operator/templates/manifests.yaml b/charts/dns-operator/templates/manifests.yaml index a0991668..976774c0 100644 --- a/charts/dns-operator/templates/manifests.yaml +++ b/charts/dns-operator/templates/manifests.yaml @@ -63,7 +63,7 @@ spec: address: description: Address to connect to the host on (IP Address (A Record) or hostname (CNAME)). - pattern: ^([1-9][0-9]?[0-9]?\.[0-9][0-9]?[0-9]?\.[0-9][0-9]?[0-9]?\.[0-9][0-9]?[0-9]|[a-z][a-z0-9\-]+\.([a-z][a-z0-9\-]+\.)*[a-z][a-z0-9\-]+)?$ + pattern: ^([1-9][0-9]?[0-9]?\.[0-9][0-9]?[0-9]?\.[0-9][0-9]?[0-9]?\.[0-9][0-9]?[0-9]?|[a-z][a-z0-9\-]+\.([a-z][a-z0-9\-]+\.)*[a-z][a-z0-9\-]+)?$ type: string allowInsecureCertificate: description: |- diff --git a/cmd/main.go b/cmd/main.go index 03a338bf..a68db860 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -79,8 +79,10 @@ func main() { var maxRequeueTime time.Duration var providers stringSliceFlags var dnsProbesEnabled bool + var allowInsecureCerts bool flag.BoolVar(&dnsProbesEnabled, "enable-probes", false, "Enable DNSHealthProbes controller.") + flag.BoolVar(&allowInsecureCerts, "insecure-health-checks", true, "Allow DNSHealthProbes to use insecure certificates") flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.") flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.") @@ -153,7 +155,7 @@ func main() { Client: mgr.GetClient(), Scheme: mgr.GetScheme(), ProviderFactory: providerFactory, - }).SetupWithManager(mgr, maxRequeueTime, validFor, minRequeueTime); err != nil { + }).SetupWithManager(mgr, maxRequeueTime, validFor, minRequeueTime, dnsProbesEnabled, allowInsecureCerts); err != nil { setupLog.Error(err, "unable to create controller", "controller", "DNSRecord") os.Exit(1) } diff --git a/config/crd/bases/kuadrant.io_dnshealthcheckprobes.yaml b/config/crd/bases/kuadrant.io_dnshealthcheckprobes.yaml index 8b24fea8..f8218dbb 100644 --- a/config/crd/bases/kuadrant.io_dnshealthcheckprobes.yaml +++ b/config/crd/bases/kuadrant.io_dnshealthcheckprobes.yaml @@ -62,7 +62,7 @@ spec: address: description: Address to connect to the host on (IP Address (A Record) or hostname (CNAME)). - pattern: ^([1-9][0-9]?[0-9]?\.[0-9][0-9]?[0-9]?\.[0-9][0-9]?[0-9]?\.[0-9][0-9]?[0-9]|[a-z][a-z0-9\-]+\.([a-z][a-z0-9\-]+\.)*[a-z][a-z0-9\-]+)?$ + pattern: ^([1-9][0-9]?[0-9]?\.[0-9][0-9]?[0-9]?\.[0-9][0-9]?[0-9]?\.[0-9][0-9]?[0-9]?|[a-z][a-z0-9\-]+\.([a-z][a-z0-9\-]+\.)*[a-z][a-z0-9\-]+)?$ type: string allowInsecureCertificate: description: |- diff --git a/config/deploy/local/kustomization.yaml b/config/deploy/local/kustomization.yaml index a3efa11f..b76b09e7 100644 --- a/config/deploy/local/kustomization.yaml +++ b/config/deploy/local/kustomization.yaml @@ -14,6 +14,9 @@ patches: - op: add path: /spec/template/spec/containers/0/args/- value: --zap-log-level=debug + - op: add + path: /spec/template/spec/containers/0/args/- + value: --enable-probes target: kind: Deployment - path: manager_config_patch.yaml diff --git a/config/manager/manager.yaml b/config/manager/manager.yaml index 8ef2267d..5733a5e1 100644 --- a/config/manager/manager.yaml +++ b/config/manager/manager.yaml @@ -43,6 +43,7 @@ spec: - /manager args: - --leader-elect + - --enable-probes env: - name: WATCH_NAMESPACES value: "" diff --git a/internal/common/tree.go b/internal/common/tree.go new file mode 100644 index 00000000..c429b51f --- /dev/null +++ b/internal/common/tree.go @@ -0,0 +1,134 @@ +package common + +import ( + "sigs.k8s.io/external-dns/endpoint" + + "github.com/kuadrant/dns-operator/api/v1alpha1" +) + +type DNSTreeNode struct { + Name string + Children []*DNSTreeNode + DataSets []DNSTreeNodeData +} + +type DNSTreeNodeData struct { + RecordType string + SetIdentifier string + RecordTTL endpoint.TTL + Labels endpoint.Labels + ProviderSpecific endpoint.ProviderSpecific + Targets []string +} + +func (n *DNSTreeNode) RemoveNode(deleteNode *DNSTreeNode) { + for i, node := range n.Children { + if node.Name == deleteNode.Name { + n.Children = append(n.Children[:i], n.Children[i+1:]...) + return + } + + // no children matched, try on each child + node.RemoveNode(deleteNode) + } +} + +// GetLeafsTargets returns IP or CNAME of the leafs of a tree. +func GetLeafsTargets(node *DNSTreeNode, targets *[]string) *[]string { + // no children means this is pointing to an IP or a host outside of the DNS Record + if len(node.Children) == 0 { + *targets = append(*targets, node.Name) + return nil + } + for _, child := range node.Children { + GetLeafsTargets(child, targets) + } + return targets +} + +func ToEndpoints(node *DNSTreeNode, endpoints *[]*endpoint.Endpoint) *[]*endpoint.Endpoint { + // no children means this is pointing to an IP or a host outside of the DNS Record + if len(node.Children) == 0 { + return endpoints + } + targets := []string{} + for _, child := range node.Children { + targets = append(targets, child.Name) + ToEndpoints(child, endpoints) + } + + if node.DataSets == nil { + *endpoints = append(*endpoints, &endpoint.Endpoint{ + DNSName: node.Name, + Targets: targets, + }) + return endpoints + } + + for _, data := range node.DataSets { + *endpoints = append(*endpoints, &endpoint.Endpoint{ + DNSName: node.Name, + Targets: data.Targets, + RecordType: data.RecordType, + RecordTTL: data.RecordTTL, + SetIdentifier: data.SetIdentifier, + Labels: data.Labels, + ProviderSpecific: data.ProviderSpecific, + }) + } + return endpoints +} + +func MakeTreeFromDNSRecord(record *v1alpha1.DNSRecord) *DNSTreeNode { + rootNode := &DNSTreeNode{Name: record.Spec.RootHost} + populateNode(rootNode, record) + return rootNode +} + +func populateNode(node *DNSTreeNode, record *v1alpha1.DNSRecord) { + node.DataSets = findDataSets(node.Name, record) + + children := findChildren(node.Name, record) + if len(children) == 0 { + return + } + + for _, c := range children { + populateNode(c, record) + } + node.Children = children +} + +func findChildren(name string, record *v1alpha1.DNSRecord) []*DNSTreeNode { + nodes := []*DNSTreeNode{} + targets := map[string]string{} + for _, ep := range record.Spec.Endpoints { + if ep.DNSName == name { + for _, t := range ep.Targets { + targets[t] = t + } + } + } + for _, t := range targets { + nodes = append(nodes, &DNSTreeNode{Name: t}) + } + + return nodes +} + +func findDataSets(name string, record *v1alpha1.DNSRecord) []DNSTreeNodeData { + dataSets := []DNSTreeNodeData{} + for _, ep := range record.Spec.Endpoints { + if ep.DNSName == name { + dataSets = append(dataSets, DNSTreeNodeData{ + RecordType: ep.RecordType, + RecordTTL: ep.RecordTTL, + SetIdentifier: ep.SetIdentifier, + Labels: ep.Labels, + ProviderSpecific: ep.ProviderSpecific, + Targets: ep.Targets, + }) + } + } + return dataSets +} diff --git a/internal/common/tree_test.go b/internal/common/tree_test.go new file mode 100644 index 00000000..ffc4732a --- /dev/null +++ b/internal/common/tree_test.go @@ -0,0 +1,713 @@ +package common + +import ( + "testing" + + . "github.com/onsi/gomega" + + "k8s.io/utils/ptr" + "sigs.k8s.io/external-dns/endpoint" + + "github.com/kuadrant/dns-operator/api/v1alpha1" +) + +func Test_MakeTreeFromDNSRecord(t *testing.T) { + RegisterTestingT(t) + + scenarios := []struct { + Name string + DNSRecord *v1alpha1.DNSRecord + Verify func(tree *DNSTreeNode) + }{ + { + Name: "geo to tree", + DNSRecord: &v1alpha1.DNSRecord{ + Spec: v1alpha1.DNSRecordSpec{ + RootHost: "app.testdomain.com", + Endpoints: []*endpoint.Endpoint{ + { + DNSName: "app.testdomain.com", + RecordTTL: 300, + RecordType: "CNAME", + Targets: []string{ + "klb.testdomain.com", + }, + }, + { + DNSName: "ip1.testdomain.com", + RecordTTL: 60, + RecordType: "A", + Targets: []string{ + "172.32.200.1", + }, + }, + { + DNSName: "ip2.testdomain.com", + RecordTTL: 60, + RecordType: "A", + Targets: []string{ + "172.32.200.2", + }, + }, + { + DNSName: "eu.klb.testdomain.com", + RecordTTL: 60, + RecordType: "CNAME", + Targets: []string{ + "ip2.testdomain.com", + }, + }, + { + DNSName: "us.klb.testdomain.com", + RecordTTL: 60, + RecordType: "CNAME", + Targets: []string{ + "ip1.testdomain.com", + }, + }, + { + DNSName: "klb.testdomain.com", + ProviderSpecific: []endpoint.ProviderSpecificProperty{ + { + Name: "geo-code", + Value: "*", + }, + }, + RecordTTL: 300, + RecordType: "CNAME", + SetIdentifier: "", + Targets: []string{ + "eu.klb.testdomain.com", + }, + }, + { + DNSName: "klb.testdomain.com", + ProviderSpecific: []endpoint.ProviderSpecificProperty{ + { + Name: "geo-code", + Value: "GEO-NA", + }, + }, + RecordTTL: 300, + RecordType: "CNAME", + SetIdentifier: "", + Targets: []string{ + "us.klb.testdomain.com", + }, + }, + { + DNSName: "klb.testdomain.com", + ProviderSpecific: []endpoint.ProviderSpecificProperty{ + { + Name: "geo-code", + Value: "GEO-EU", + }, + }, + RecordTTL: 300, + RecordType: "CNAME", + SetIdentifier: "", + Targets: []string{ + "eu.klb.testdomain.com", + }, + }, + }, + }, + }, + Verify: func(tree *DNSTreeNode) { + Expect(tree.Name).To(Equal("app.testdomain.com")) + Expect(len(tree.Children)).To(Equal(1)) + + child := tree.Children[0] + Expect(child.Name).To(Equal("klb.testdomain.com")) + Expect(len(child.Children)).To(Equal(2)) + + Expect(len(child.DataSets)).To(Equal(3)) + Expect(child.DataSets[0]).To(Equal(DNSTreeNodeData{ + RecordType: endpoint.RecordTypeCNAME, + SetIdentifier: "", + RecordTTL: 300, + ProviderSpecific: endpoint.ProviderSpecific{ + { + Name: "geo-code", + Value: "*", + }, + }, + Targets: []string{ + "eu.klb.testdomain.com", + }, + })) + Expect(child.DataSets[1]).To(Equal(DNSTreeNodeData{ + RecordType: endpoint.RecordTypeCNAME, + SetIdentifier: "", + RecordTTL: 300, + ProviderSpecific: endpoint.ProviderSpecific{ + { + Name: "geo-code", + Value: "GEO-NA", + }, + }, + Targets: []string{ + "us.klb.testdomain.com", + }, + })) + Expect(child.DataSets[2]).To(Equal(DNSTreeNodeData{ + RecordType: endpoint.RecordTypeCNAME, + SetIdentifier: "", + RecordTTL: 300, + ProviderSpecific: endpoint.ProviderSpecific{ + { + Name: "geo-code", + Value: "GEO-EU", + }, + }, + Targets: []string{ + "eu.klb.testdomain.com", + }, + })) + + for _, c := range child.Children { + Expect([]string{"eu.klb.testdomain.com", "us.klb.testdomain.com"}).To(ContainElement(c.Name)) + Expect(len(c.Children)).To(Equal(1)) + + if c.Name == "eu.klb.testdomain.com" { + Expect(c.Children[0].Name).To(Equal("ip2.testdomain.com")) + Expect(len(c.Children[0].Children)).To(Equal(1)) + + Expect(c.Children[0].Children[0].Name).To(Equal("172.32.200.2")) + Expect(len(c.Children[0].Children[0].Children)).To(Equal(0)) + + } else if c.Name == "us.klb.testdomain.com" { + Expect(c.Children[0].Name).To(Equal("ip1.testdomain.com")) + Expect(len(c.Children[0].Children)).To(Equal(1)) + + Expect(c.Children[0].Children[0].Name).To(Equal("172.32.200.1")) + Expect(len(c.Children[0].Children[0].Children)).To(Equal(0)) + } + } + }, + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.Name, func(t *testing.T) { + tree := MakeTreeFromDNSRecord(scenario.DNSRecord) + scenario.Verify(tree) + }) + } +} + +func Test_GetLeafsTargets(t *testing.T) { + RegisterTestingT(t) + + scenarios := []struct { + Name string + DNSRecord *v1alpha1.DNSRecord + Verify func(targets []string) + }{ + { + Name: "geo targets", + DNSRecord: &v1alpha1.DNSRecord{ + Spec: v1alpha1.DNSRecordSpec{ + RootHost: "app.testdomain.com", + Endpoints: []*endpoint.Endpoint{ + { + DNSName: "app.testdomain.com", + RecordTTL: 300, + RecordType: "CNAME", + Targets: []string{ + "klb.testdomain.com", + }, + }, + { + DNSName: "ip1.testdomain.com", + RecordTTL: 60, + RecordType: "A", + Targets: []string{ + "172.32.200.1", + }, + }, + { + DNSName: "ip2.testdomain.com", + RecordTTL: 60, + RecordType: "A", + Targets: []string{ + "172.32.200.2", + }, + }, + { + DNSName: "eu.klb.testdomain.com", + RecordTTL: 60, + RecordType: "CNAME", + Targets: []string{ + "ip2.testdomain.com", + }, + }, + { + DNSName: "us.klb.testdomain.com", + RecordTTL: 60, + RecordType: "CNAME", + Targets: []string{ + "ip1.testdomain.com", + }, + }, + { + DNSName: "klb.testdomain.com", + ProviderSpecific: []endpoint.ProviderSpecificProperty{ + { + Name: "geo-code", + Value: "*", + }, + }, + RecordTTL: 300, + RecordType: "CNAME", + SetIdentifier: "", + Targets: []string{ + "eu.klb.testdomain.com", + }, + }, + { + DNSName: "klb.testdomain.com", + ProviderSpecific: []endpoint.ProviderSpecificProperty{ + { + Name: "geo-code", + Value: "GEO-NA", + }, + }, + RecordTTL: 300, + RecordType: "CNAME", + SetIdentifier: "", + Targets: []string{ + "us.klb.testdomain.com", + }, + }, + { + DNSName: "klb.testdomain.com", + ProviderSpecific: []endpoint.ProviderSpecificProperty{ + { + Name: "geo-code", + Value: "GEO-EU", + }, + }, + RecordTTL: 300, + RecordType: "CNAME", + SetIdentifier: "", + Targets: []string{ + "eu.klb.testdomain.com", + }, + }, + }, + }, + }, + Verify: func(targets []string) { + Expect(targets).To(HaveLen(2)) + for _, target := range targets { + Expect(target).To(Or( + Equal("172.32.200.1"), + Equal("172.32.200.2"))) + } + }, + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.Name, func(t *testing.T) { + targets := make([]string, 0) + GetLeafsTargets(MakeTreeFromDNSRecord(scenario.DNSRecord), &targets) + + // verify for passing targets in + scenario.Verify(targets) + + // verify for return value + scenario.Verify(*GetLeafsTargets(MakeTreeFromDNSRecord(scenario.DNSRecord), ptr.To([]string{}))) + }) + } +} + +func Test_RemoveNode(t *testing.T) { + RegisterTestingT(t) + + scenarios := []struct { + Name string + Tree *DNSTreeNode + RemoveNodes []*DNSTreeNode + Verify func(tree *DNSTreeNode) + }{ + { + Name: "remove klb", + Tree: &DNSTreeNode{ + Name: "app.testdomain.com", + Children: []*DNSTreeNode{ + { + Name: "klb.testdomain.com", + Children: []*DNSTreeNode{ + { + Name: "eu.klb.testdomain.com", + Children: []*DNSTreeNode{ + { + Name: "ip1.testdomain.com", + Children: []*DNSTreeNode{ + { + Name: "172.32.200.1", + }, + }, + }, + }, + }, + { + Name: "us.klb.testdomain.com", + Children: []*DNSTreeNode{ + { + Name: "ip2.testdomain.com", + Children: []*DNSTreeNode{ + { + Name: "172.32.200.2", + }, + }, + }, + }, + }, + }, + }, + }, + }, + RemoveNodes: []*DNSTreeNode{ + { + Name: "klb.testdomain.com", + }, + }, + Verify: func(tree *DNSTreeNode) { + Expect(len(tree.Children)).To(Equal(0)) + }, + }, + { + Name: "remove eu", + Tree: &DNSTreeNode{ + Name: "app.testdomain.com", + Children: []*DNSTreeNode{ + { + Name: "klb.testdomain.com", + Children: []*DNSTreeNode{ + { + Name: "eu.klb.testdomain.com", + Children: []*DNSTreeNode{ + { + Name: "ip1.testdomain.com", + Children: []*DNSTreeNode{ + { + Name: "172.32.200.1", + }, + }, + }, + }, + }, + { + Name: "us.klb.testdomain.com", + Children: []*DNSTreeNode{ + { + Name: "ip2.testdomain.com", + Children: []*DNSTreeNode{ + { + Name: "172.32.200.2", + }, + }, + }, + }, + }, + }, + }, + }, + }, + RemoveNodes: []*DNSTreeNode{ + { + Name: "eu.klb.testdomain.com", + }, + }, + Verify: func(tree *DNSTreeNode) { + Expect(tree.Name).To(Equal("app.testdomain.com")) + Expect(len(tree.Children)).To(Equal(1)) + + child := tree.Children[0] + Expect(child.Name).To(Equal("klb.testdomain.com")) + Expect(len(child.Children)).To(Equal(1)) + + for _, c := range child.Children { + Expect([]string{"us.klb.testdomain.com"}).To(ContainElement(c.Name)) + Expect(len(c.Children)).To(Equal(1)) + + Expect(c.Children[0].Name).To(Equal("ip2.testdomain.com")) + Expect(len(c.Children[0].Children)).To(Equal(1)) + + Expect(c.Children[0].Children[0].Name).To(Equal("172.32.200.2")) + Expect(len(c.Children[0].Children[0].Children)).To(Equal(0)) + } + }, + }, + { + Name: "remove eu", + Tree: &DNSTreeNode{ + Name: "app.testdomain.com", + Children: []*DNSTreeNode{ + { + Name: "klb.testdomain.com", + Children: []*DNSTreeNode{ + { + Name: "eu.klb.testdomain.com", + Children: []*DNSTreeNode{ + { + Name: "ip1.testdomain.com", + Children: []*DNSTreeNode{ + { + Name: "172.32.200.1", + }, + }, + }, + }, + }, + { + Name: "us.klb.testdomain.com", + Children: []*DNSTreeNode{ + { + Name: "ip2.testdomain.com", + Children: []*DNSTreeNode{ + { + Name: "172.32.200.2", + }, + }, + }, + }, + }, + }, + }, + }, + }, + RemoveNodes: []*DNSTreeNode{ + { + Name: "eu.klb.testdomain.com", + }, + { + Name: "us.klb.testdomain.com", + }, + }, + Verify: func(tree *DNSTreeNode) { + Expect(tree.Name).To(Equal("app.testdomain.com")) + Expect(len(tree.Children)).To(Equal(1)) + + child := tree.Children[0] + Expect(child.Name).To(Equal("klb.testdomain.com")) + Expect(len(child.Children)).To(Equal(0)) + }, + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.Name, func(t *testing.T) { + for _, rn := range scenario.RemoveNodes { + scenario.Tree.RemoveNode(rn) + } + scenario.Verify(scenario.Tree) + }) + } +} + +func Test_ToEndpoints(t *testing.T) { + RegisterTestingT(t) + + scenarios := []struct { + Name string + Record *v1alpha1.DNSRecord + Verify func(endpoints *[]*endpoint.Endpoint) + }{ + { + Name: "geo tree", + Record: &v1alpha1.DNSRecord{ + Spec: v1alpha1.DNSRecordSpec{ + RootHost: "app.testdomain.com", + Endpoints: []*endpoint.Endpoint{ + { + DNSName: "app.testdomain.com", + RecordTTL: 300, + RecordType: "CNAME", + Targets: []string{ + "klb.testdomain.com", + }, + }, + { + DNSName: "ip1.testdomain.com", + RecordTTL: 60, + RecordType: "A", + Targets: []string{ + "172.32.200.1", + }, + }, + { + DNSName: "ip2.testdomain.com", + RecordTTL: 60, + RecordType: "A", + Targets: []string{ + "172.32.200.2", + }, + }, + { + DNSName: "eu.klb.testdomain.com", + RecordTTL: 60, + RecordType: "CNAME", + Targets: []string{ + "ip2.testdomain.com", + }, + }, + { + DNSName: "us.klb.testdomain.com", + RecordTTL: 60, + RecordType: "CNAME", + Targets: []string{ + "ip1.testdomain.com", + }, + }, + { + DNSName: "klb.testdomain.com", + ProviderSpecific: []endpoint.ProviderSpecificProperty{ + { + Name: "geo-code", + Value: "*", + }, + }, + RecordTTL: 300, + RecordType: "CNAME", + SetIdentifier: "", + Targets: []string{ + "eu.klb.testdomain.com", + }, + }, + { + DNSName: "klb.testdomain.com", + ProviderSpecific: []endpoint.ProviderSpecificProperty{ + { + Name: "geo-code", + Value: "GEO-NA", + }, + }, + RecordTTL: 300, + RecordType: "CNAME", + SetIdentifier: "", + Targets: []string{ + "us.klb.testdomain.com", + }, + }, + { + DNSName: "klb.testdomain.com", + ProviderSpecific: []endpoint.ProviderSpecificProperty{ + { + Name: "geo-code", + Value: "GEO-EU", + }, + }, + RecordTTL: 300, + RecordType: "CNAME", + SetIdentifier: "", + Targets: []string{ + "eu.klb.testdomain.com", + }, + }, + }, + }, + }, + Verify: func(endpoints *[]*endpoint.Endpoint) { + Expect(len(*endpoints)).To(Equal(8)) + Expect(*endpoints).To(ContainElement(&endpoint.Endpoint{ + DNSName: "app.testdomain.com", + RecordTTL: 300, + RecordType: "CNAME", + Targets: []string{ + "klb.testdomain.com", + }, + })) + Expect(*endpoints).To(ContainElement(&endpoint.Endpoint{ + DNSName: "ip1.testdomain.com", + RecordTTL: 60, + RecordType: "A", + Targets: []string{ + "172.32.200.1", + }, + })) + Expect(*endpoints).To(ContainElement(&endpoint.Endpoint{ + DNSName: "ip2.testdomain.com", + RecordTTL: 60, + RecordType: "A", + Targets: []string{ + "172.32.200.2", + }, + })) + Expect(*endpoints).To(ContainElement(&endpoint.Endpoint{ + DNSName: "eu.klb.testdomain.com", + RecordTTL: 60, + RecordType: "CNAME", + Targets: []string{ + "ip2.testdomain.com", + }, + })) + Expect(*endpoints).To(ContainElement(&endpoint.Endpoint{ + DNSName: "us.klb.testdomain.com", + RecordTTL: 60, + RecordType: "CNAME", + Targets: []string{ + "ip1.testdomain.com", + }, + })) + Expect(*endpoints).To(ContainElement(&endpoint.Endpoint{ + DNSName: "klb.testdomain.com", + ProviderSpecific: []endpoint.ProviderSpecificProperty{ + { + Name: "geo-code", + Value: "*", + }, + }, + RecordTTL: 300, + RecordType: "CNAME", + SetIdentifier: "", + Targets: []string{ + "eu.klb.testdomain.com", + }, + })) + Expect(*endpoints).To(ContainElement(&endpoint.Endpoint{ + DNSName: "klb.testdomain.com", + ProviderSpecific: []endpoint.ProviderSpecificProperty{ + { + Name: "geo-code", + Value: "GEO-NA", + }, + }, + RecordTTL: 300, + RecordType: "CNAME", + SetIdentifier: "", + Targets: []string{ + "us.klb.testdomain.com", + }, + })) + Expect(*endpoints).To(ContainElement(&endpoint.Endpoint{ + DNSName: "klb.testdomain.com", + ProviderSpecific: []endpoint.ProviderSpecificProperty{ + { + Name: "geo-code", + Value: "GEO-EU", + }, + }, + RecordTTL: 300, + RecordType: "CNAME", + SetIdentifier: "", + Targets: []string{ + "eu.klb.testdomain.com", + }, + })) + }, + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.Name, func(t *testing.T) { + endpoints := &[]*endpoint.Endpoint{} + tree := MakeTreeFromDNSRecord(scenario.Record) + ToEndpoints(tree, endpoints) + scenario.Verify(endpoints) + }) + } +} diff --git a/internal/controller/dnshealthcheckprobe_reconciler.go b/internal/controller/dnshealthcheckprobe_reconciler.go index 1fcf41ef..be9ab04e 100644 --- a/internal/controller/dnshealthcheckprobe_reconciler.go +++ b/internal/controller/dnshealthcheckprobe_reconciler.go @@ -15,6 +15,10 @@ import ( "github.com/kuadrant/dns-operator/internal/probes" ) +const ( + ProbeOwnerLabel = "dns.kuadrant.io/probes-owner" +) + // DNSProbeReconciler reconciles a DNSRecord object type DNSProbeReconciler struct { client.Client diff --git a/internal/controller/dnsrecord_controller.go b/internal/controller/dnsrecord_controller.go index 3d4aef2a..5f58cfdd 100644 --- a/internal/controller/dnsrecord_controller.go +++ b/internal/controller/dnsrecord_controller.go @@ -66,6 +66,9 @@ var ( randomizedValidationRequeue time.Duration validFor time.Duration reconcileStart metav1.Time + + probesEnabled bool + allowInsecureCert bool ) // DNSRecordReconciler reconciles a DNSRecord object @@ -125,8 +128,10 @@ func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return r.updateStatus(ctx, previous, dnsRecord, false, err) } - if err = r.ReconcileHealthChecks(ctx, dnsRecord); client.IgnoreNotFound(err) != nil { - return ctrl.Result{}, err + if probesEnabled { + if err = r.DeleteHealthChecks(ctx, dnsRecord); client.IgnoreNotFound(err) != nil { + return ctrl.Result{}, err + } } hadChanges, err := r.deleteRecord(ctx, dnsRecord, dnsProvider) if err != nil { @@ -227,8 +232,10 @@ func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return r.updateStatus(ctx, previous, dnsRecord, hadChanges, err) } - if err = r.ReconcileHealthChecks(ctx, dnsRecord); err != nil { - return ctrl.Result{}, err + if probesEnabled { + if err = r.ReconcileHealthChecks(ctx, dnsRecord, allowInsecureCert); err != nil { + return ctrl.Result{}, err + } } return r.updateStatus(ctx, previous, dnsRecord, hadChanges, nil) @@ -318,10 +325,12 @@ func (r *DNSRecordReconciler) updateStatus(ctx context.Context, previous, curren } // SetupWithManager sets up the controller with the Manager. -func (r *DNSRecordReconciler) SetupWithManager(mgr ctrl.Manager, maxRequeue, validForDuration, minRequeue time.Duration) error { +func (r *DNSRecordReconciler) SetupWithManager(mgr ctrl.Manager, maxRequeue, validForDuration, minRequeue time.Duration, healthProbesEnabled, allowInsecureHealthCert bool) error { defaultRequeueTime = maxRequeue validFor = validForDuration defaultValidationRequeue = minRequeue + probesEnabled = healthProbesEnabled + allowInsecureCert = allowInsecureHealthCert return ctrl.NewControllerManagedBy(mgr). For(&v1alpha1.DNSRecord{}). diff --git a/internal/controller/dnsrecord_healthchecks.go b/internal/controller/dnsrecord_healthchecks.go index 418c6ba1..30c78642 100644 --- a/internal/controller/dnsrecord_healthchecks.go +++ b/internal/controller/dnsrecord_healthchecks.go @@ -2,180 +2,161 @@ package controller import ( "context" - "crypto/md5" "fmt" - "io" "reflect" + "strings" - "k8s.io/apimachinery/pkg/api/meta" + "github.com/go-logr/logr" + + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - externaldns "sigs.k8s.io/external-dns/endpoint" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/utils/ptr" + controllerruntime "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log" "github.com/kuadrant/dns-operator/api/v1alpha1" - "github.com/kuadrant/dns-operator/internal/provider" + "github.com/kuadrant/dns-operator/internal/common" ) -// healthChecksConfig represents the user configuration for the health checks -type healthChecksConfig struct { - Endpoint string - Port *int64 - FailureThreshold *int64 - Protocol *provider.HealthCheckProtocol -} - -func (r *DNSRecordReconciler) ReconcileHealthChecks(ctx context.Context, dnsRecord *v1alpha1.DNSRecord) error { - var results []provider.HealthCheckResult - var err error +func (r *DNSRecordReconciler) ReconcileHealthChecks(ctx context.Context, dnsRecord *v1alpha1.DNSRecord, allowInsecureCerts bool) error { + logger := log.FromContext(ctx).WithName("healthchecks") + logger.Info("Reconciling healthchecks") - dnsProvider, err := r.getDNSProvider(ctx, dnsRecord) - if err != nil { - return err + // Probes enabled but no health check spec yet. Nothing to do + if dnsRecord.Spec.HealthCheck == nil { + return nil } - healthCheckReconciler := dnsProvider.HealthCheckReconciler() - - // Get the configuration for the health checks. If no configuration is - // set, ensure that the health checks are deleted - config := getHealthChecksConfig(dnsRecord) - - for _, dnsEndpoint := range dnsRecord.Spec.Endpoints { - addresses := provider.GetExternalAddresses(dnsEndpoint, dnsRecord) - for _, address := range addresses { - probeStatus := r.getProbeStatus(address, dnsRecord) - - // no config means delete the health checks - if config == nil { - result, err := healthCheckReconciler.Delete(ctx, dnsEndpoint, probeStatus) - if err != nil { - return err - } - - results = append(results, result) - continue - } - - // creating / updating health checks - endpointId, err := idForEndpoint(dnsRecord, dnsEndpoint, address) - if err != nil { - return err - } - - spec := provider.HealthCheckSpec{ - Id: endpointId, - Name: fmt.Sprintf("%s-%s-%s", dnsRecord.Spec.RootHost, dnsEndpoint.DNSName, address), - Host: &dnsRecord.Spec.RootHost, - Path: config.Endpoint, - Port: config.Port, - Protocol: config.Protocol, - FailureThreshold: config.FailureThreshold, - } + desiredProbes := buildDesiredProbes(dnsRecord, common.GetLeafsTargets(common.MakeTreeFromDNSRecord(dnsRecord), ptr.To([]string{})), allowInsecureCerts) - result := healthCheckReconciler.Reconcile(ctx, spec, dnsEndpoint, probeStatus, address) - results = append(results, result) + for _, probe := range desiredProbes { + // if one of them fails - health checks for this record are invalid anyway, so no sense to continue + if err := controllerruntime.SetControllerReference(dnsRecord, probe, r.Scheme); err != nil { + return err } - } - - result := r.reconcileHealthCheckStatus(results, dnsRecord) - return result -} - -func (r *DNSRecordReconciler) getProbeStatus(address string, dnsRecord *v1alpha1.DNSRecord) *v1alpha1.HealthCheckStatusProbe { - if dnsRecord.Status.HealthCheck == nil || dnsRecord.Status.HealthCheck.Probes == nil { - return nil - } - for _, probeStatus := range dnsRecord.Status.HealthCheck.Probes { - if probeStatus.IPAddress == address { - return &probeStatus + if err := r.ensureProbe(ctx, probe, logger); err != nil { + return err } } - + logger.Info("Healthecks reconciled") return nil } -func (r *DNSRecordReconciler) reconcileHealthCheckStatus(results []provider.HealthCheckResult, dnsRecord *v1alpha1.DNSRecord) error { - var previousCondition *metav1.Condition - probesCondition := &metav1.Condition{ - Reason: "AllProbesSynced", - Type: "healthProbesSynced", - } +// DeleteHealthChecks deletes all v1alpha1.DNSHealthCheckProbe that have ProbeOwnerLabel of passed in DNSRecord +func (r *DNSRecordReconciler) DeleteHealthChecks(ctx context.Context, dnsRecord *v1alpha1.DNSRecord) error { + logger := log.FromContext(ctx).WithName("healthchecks") + logger.Info("Deleting healthchecks") - var allSynced = metav1.ConditionTrue + healthProbes := v1alpha1.DNSHealthCheckProbeList{} - if dnsRecord.Status.HealthCheck == nil { - dnsRecord.Status.HealthCheck = &v1alpha1.HealthCheckStatus{ - Conditions: []metav1.Condition{}, - Probes: []v1alpha1.HealthCheckStatusProbe{}, - } + if err := r.List(ctx, &healthProbes, &client.ListOptions{ + LabelSelector: labels.SelectorFromSet(map[string]string{ + ProbeOwnerLabel: BuildOwnerLabelValue(dnsRecord), + }), + Namespace: dnsRecord.Namespace, + }); err != nil { + return err } - previousCondition = meta.FindStatusCondition(dnsRecord.Status.HealthCheck.Conditions, "HealthProbesSynced") - if previousCondition != nil { - probesCondition = previousCondition + for _, probe := range healthProbes.Items { + logger.V(1).Info(fmt.Sprintf("Deleting probe: %s", probe.Name)) + if err := r.Delete(ctx, &probe); err != nil { + return err + } } + return nil +} - dnsRecord.Status.HealthCheck.Probes = []v1alpha1.HealthCheckStatusProbe{} +func (r *DNSRecordReconciler) ensureProbe(ctx context.Context, generated *v1alpha1.DNSHealthCheckProbe, logger logr.Logger) error { + current := &v1alpha1.DNSHealthCheckProbe{} - for _, result := range results { - if result.Host == "" { - continue - } - status := true - if result.Result == provider.HealthCheckFailed { - status = false - allSynced = metav1.ConditionFalse + if err := r.Get(ctx, client.ObjectKeyFromObject(generated), current); err != nil { + if errors.IsNotFound(err) { + logger.V(1).Info(fmt.Sprintf("Creating probe: %s", generated.Name)) + return r.Create(ctx, generated) } - - dnsRecord.Status.HealthCheck.Probes = append(dnsRecord.Status.HealthCheck.Probes, v1alpha1.HealthCheckStatusProbe{ - ID: result.ID, - IPAddress: result.IPAddress, - Host: result.Host, - Synced: status, - Conditions: []metav1.Condition{result.Condition}, - }) + return err } - probesCondition.ObservedGeneration = dnsRecord.Generation - probesCondition.Status = allSynced - - if allSynced == metav1.ConditionTrue { - probesCondition.Message = fmt.Sprintf("all %v probes synced successfully", len(dnsRecord.Status.HealthCheck.Probes)) - probesCondition.Reason = "AllProbesSynced" - } else { - probesCondition.Reason = "UnsyncedProbes" - probesCondition.Message = "some probes have not yet successfully synced to the DNS Provider" - } + desired := current.DeepCopy() + desired.Spec = generated.Spec - //probe condition changed? - update transition time - if !reflect.DeepEqual(previousCondition, probesCondition) { - probesCondition.LastTransitionTime = metav1.Now() + if !reflect.DeepEqual(current, desired) { + logger.V(1).Info(fmt.Sprintf("Updating probe: %s", desired.Name)) + if err := r.Update(ctx, desired); err != nil { + return err + } } - - dnsRecord.Status.HealthCheck.Conditions = []metav1.Condition{*probesCondition} - + logger.V(1).Info(fmt.Sprintf("No updates needed for probe: %s", desired.Name)) return nil } -func getHealthChecksConfig(dnsRecord *v1alpha1.DNSRecord) *healthChecksConfig { - if dnsRecord.Spec.HealthCheck == nil || dnsRecord.DeletionTimestamp != nil { - return nil - } - - port := int64(*dnsRecord.Spec.HealthCheck.Port) - failureThreshold := int64(*dnsRecord.Spec.HealthCheck.FailureThreshold) - - return &healthChecksConfig{ - Endpoint: dnsRecord.Spec.HealthCheck.Path, - Port: &port, - FailureThreshold: &failureThreshold, - Protocol: (*provider.HealthCheckProtocol)(&dnsRecord.Spec.HealthCheck.Protocol), +func buildDesiredProbes(dnsRecord *v1alpha1.DNSRecord, leafs *[]string, allowInsecureCerts bool) []*v1alpha1.DNSHealthCheckProbe { + var probes []*v1alpha1.DNSHealthCheckProbe + + for _, leaf := range *leafs { + probes = append(probes, &v1alpha1.DNSHealthCheckProbe{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("%s-%s", dnsRecord.Name, leaf), + Namespace: dnsRecord.Namespace, + Labels: map[string]string{ProbeOwnerLabel: BuildOwnerLabelValue(dnsRecord)}, + }, + Spec: v1alpha1.DNSHealthCheckProbeSpec{ + Port: dnsRecord.Spec.HealthCheck.Port, + Hostname: dnsRecord.Spec.RootHost, + Address: leaf, + Path: dnsRecord.Spec.HealthCheck.Path, + Protocol: dnsRecord.Spec.HealthCheck.Protocol, + Interval: dnsRecord.Spec.HealthCheck.Interval, + AdditionalHeadersRef: dnsRecord.Spec.HealthCheck.AdditionalHeadersRef, + FailureThreshold: dnsRecord.Spec.HealthCheck.FailureThreshold, + AllowInsecureCertificate: allowInsecureCerts, + }, + }) } + return probes } -// idForEndpoint returns a unique identifier for an endpoint -func idForEndpoint(dnsRecord *v1alpha1.DNSRecord, endpoint *externaldns.Endpoint, address string) (string, error) { - hash := md5.New() - if _, err := io.WriteString(hash, fmt.Sprintf("%s/%s@%s:%s-%v", dnsRecord.Name, endpoint.SetIdentifier, endpoint.DNSName, address, dnsRecord.Generation)); err != nil { - return "", fmt.Errorf("unexpected error creating ID for endpoint %s", endpoint.SetIdentifier) +// BuildOwnerLabelValue ensures label value does not exceed the 63 char limit +// It adds namespace and name of the record, +// if the resulting string longer than 63 chars it attempts the following in +// a stated order and uses the first solution that produces value of less than 63 characters: +// +// 1. Trims namespace part to get under the limit. +// Result: "short-namespace_hostname" +// +// 2. Uses the first two subdomains of host instead of the name (e.g. will use "pat.the" from "pat.the.cat.com"). +// Result: "original-namespace_pat.the" +// +// 3. Uses GetUIDHash() of v1alpha1.DNSRecord struct +// Result: "UIDHash" +func BuildOwnerLabelValue(record *v1alpha1.DNSRecord) string { + value := fmt.Sprintf("%s_%s", record.Namespace, record.Name) + + // using the name of the dns record (hostname) and namespace is likely to exceed a 63 char limit + if len(value) > 63 { + // determine how much we need to remove + overshoot := len(value) - 63 + + // if we can fix it by trimming NS + if len(record.Namespace) > overshoot { + value = fmt.Sprintf("%s_%s", record.Namespace[:len(record.Namespace)-overshoot], record.Name) + } else { + // trimming namespace is not an option - too long hostname + // the name of the probe will be fine - it has a limit of 253 + shortHost := strings.Join(strings.Split(record.Name, ".")[:3], ".") + + // if this the case we can reduce just host + if len(record.Name)-len(shortHost) > overshoot { + value = fmt.Sprintf("%s_%s", record.Namespace, shortHost) + } else { + // we can't deal with it just by shortening one of them + // default to UID of record + value = record.GetUIDHash() + } + } } - return fmt.Sprintf("%x", hash.Sum(nil)), nil + return value } diff --git a/internal/controller/dnsrecord_healthchecks_test.go b/internal/controller/dnsrecord_healthchecks_test.go new file mode 100644 index 00000000..fd2bfba2 --- /dev/null +++ b/internal/controller/dnsrecord_healthchecks_test.go @@ -0,0 +1,181 @@ +//go:build integration + +package controller + +import ( + "fmt" + "strings" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + . "github.com/onsi/gomega/gstruct" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/kuadrant/dns-operator/api/v1alpha1" + "github.com/kuadrant/dns-operator/pkg/builder" +) + +var _ = Describe("DNSRecordReconciler_HealthChecks", func() { + var ( + dnsRecord *v1alpha1.DNSRecord + + testNamespace, testHostname string + ) + + BeforeEach(func() { + CreateNamespace(&testNamespace) + + testZoneDomainName := strings.Join([]string{GenerateName(), "example.com"}, ".") + testHostname = strings.Join([]string{"foo", testZoneDomainName}, ".") + + dnsProviderSecret := builder.NewProviderBuilder("inmemory-credentials", testNamespace). + For(v1alpha1.SecretTypeKuadrantInmemory). + WithZonesInitialisedFor(testZoneDomainName). + Build() + Expect(k8sClient.Create(ctx, dnsProviderSecret)).To(Succeed()) + + dnsRecord = &v1alpha1.DNSRecord{ + ObjectMeta: metav1.ObjectMeta{ + Name: testHostname, + Namespace: testNamespace, + }, + Spec: v1alpha1.DNSRecordSpec{ + RootHost: testHostname, + ProviderRef: v1alpha1.ProviderRef{ + Name: dnsProviderSecret.Name, + }, + Endpoints: getTestLBEndpoints(testHostname), + HealthCheck: getTestHealthCheckSpec(), + }, + } + }) + + It("Should create valid probe CRs and remove them on DNSRecord deletion", func() { + //Create default test dnsrecord + Expect(k8sClient.Create(ctx, dnsRecord)).To(Succeed()) + Eventually(func(g Gomega) { + err := k8sClient.Get(ctx, client.ObjectKeyFromObject(dnsRecord), dnsRecord) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(dnsRecord.Status.Conditions).To( + ContainElement(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(string(v1alpha1.ConditionTypeReady)), + "Status": Equal(metav1.ConditionTrue), + "Reason": Equal("ProviderSuccess"), + "Message": Equal("Provider ensured the dns record"), + })), + ) + }, TestTimeoutMedium, time.Second).Should(Succeed()) + + By("Validating created probes") + Eventually(func(g Gomega) { + probes := &v1alpha1.DNSHealthCheckProbeList{} + + g.Expect(k8sClient.List(ctx, probes, &client.ListOptions{ + LabelSelector: labels.SelectorFromSet(map[string]string{ + ProbeOwnerLabel: BuildOwnerLabelValue(dnsRecord), + }), + Namespace: dnsRecord.Namespace, + })).To(Succeed()) + + g.Expect(probes.Items).To(HaveLen(2)) + g.Expect(probes.Items).To(ConsistOf( + MatchFields(IgnoreExtras, Fields{ + "ObjectMeta": MatchFields(IgnoreExtras, Fields{ + "Name": Equal(fmt.Sprintf("%s-%s", dnsRecord.Name, "172.32.200.1")), + "Namespace": Equal(testNamespace), + }), + "Spec": MatchFields(IgnoreExtras, Fields{ + "Port": PointTo(Equal(443)), + "Hostname": Equal(testHostname), + "Address": Equal("172.32.200.1"), + "Path": Equal("/healthz"), + "Protocol": Equal(v1alpha1.Protocol("HTTPS")), + "Interval": Equal(metav1.Duration{Duration: time.Minute}), + "AdditionalHeadersRef": PointTo(MatchFields(IgnoreExtras, Fields{ + "Name": Equal("headers"), + })), + "FailureThreshold": PointTo(Equal(5)), + "AllowInsecureCertificate": Equal(true), + }), + }), + MatchFields(IgnoreExtras, Fields{ + "ObjectMeta": MatchFields(IgnoreExtras, Fields{ + "Name": Equal(fmt.Sprintf("%s-%s", dnsRecord.Name, "172.32.200.2")), + "Namespace": Equal(testNamespace), + }), + "Spec": MatchFields(IgnoreExtras, Fields{ + "Port": PointTo(Equal(443)), + "Hostname": Equal(testHostname), + "Address": Equal("172.32.200.2"), + "Path": Equal("/healthz"), + "Protocol": Equal(v1alpha1.Protocol("HTTPS")), + "Interval": Equal(metav1.Duration{Duration: time.Minute}), + "AdditionalHeadersRef": PointTo(MatchFields(IgnoreExtras, Fields{ + "Name": Equal("headers"), + })), + "FailureThreshold": PointTo(Equal(5)), + "AllowInsecureCertificate": Equal(true), + }), + }), + )) + }, TestTimeoutMedium, time.Second).Should(Succeed()) + + By("Ensuring probes block DNSRecord deletion and correctly removed") + Eventually(func(g Gomega) { + probes := &v1alpha1.DNSHealthCheckProbeList{} + + g.Expect(k8sClient.List(ctx, probes, &client.ListOptions{ + LabelSelector: labels.SelectorFromSet(map[string]string{ + ProbeOwnerLabel: BuildOwnerLabelValue(dnsRecord), + }), + Namespace: dnsRecord.Namespace, + })).To(Succeed()) + + g.Expect(probes.Items).To(HaveLen(2)) + g.Expect(probes.Items).To(ConsistOf( + MatchFields(IgnoreExtras, Fields{ + "ObjectMeta": MatchFields(IgnoreExtras, Fields{ + "Name": Equal(fmt.Sprintf("%s-%s", dnsRecord.Name, "172.32.200.1")), + "Namespace": Equal(testNamespace), + "OwnerReferences": ConsistOf(MatchFields(IgnoreExtras, Fields{ + "Name": Equal(dnsRecord.Name), + "UID": Equal(dnsRecord.UID), + "Controller": PointTo(Equal(true)), + "BlockOwnerDeletion": PointTo(Equal(true)), + })), + }), + }), + MatchFields(IgnoreExtras, Fields{ + "ObjectMeta": MatchFields(IgnoreExtras, Fields{ + "Name": Equal(fmt.Sprintf("%s-%s", dnsRecord.Name, "172.32.200.2")), + "Namespace": Equal(testNamespace), + "OwnerReferences": ConsistOf(MatchFields(IgnoreExtras, Fields{ + "Name": Equal(dnsRecord.Name), + "UID": Equal(dnsRecord.UID), + "Controller": PointTo(Equal(true)), + "BlockOwnerDeletion": PointTo(Equal(true)), + })), + }), + }), + )) + + g.Expect(k8sClient.Delete(ctx, dnsRecord)).To(Succeed()) + Eventually(func(g Gomega) { + g.Expect(k8sClient.List(ctx, probes, &client.ListOptions{ + LabelSelector: labels.SelectorFromSet(map[string]string{ + ProbeOwnerLabel: BuildOwnerLabelValue(dnsRecord), + }), + Namespace: dnsRecord.Namespace, + })).To(Succeed()) + + g.Expect(probes.Items).To(HaveLen(0)) + }, TestTimeoutShort, time.Second) + + }, TestTimeoutMedium, time.Second).Should(Succeed()) + }) + +}) diff --git a/internal/controller/helper_test.go b/internal/controller/helper_test.go index dd90b04c..b151849e 100644 --- a/internal/controller/helper_test.go +++ b/internal/controller/helper_test.go @@ -9,7 +9,11 @@ import ( "github.com/goombaio/namegenerator" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" externaldnsendpoint "sigs.k8s.io/external-dns/endpoint" + + "github.com/kuadrant/dns-operator/api/v1alpha1" ) const ( @@ -42,3 +46,106 @@ func getTestEndpoints(dnsName, ip string) []*externaldnsendpoint.Endpoint { }, } } + +func getTestHealthCheckSpec() *v1alpha1.HealthCheckSpec { + return &v1alpha1.HealthCheckSpec{ + Path: "/healthz", + Port: ptr.To(443), + Protocol: "HTTPS", + FailureThreshold: ptr.To(5), + Interval: metav1.Duration{Duration: time.Minute}, + AdditionalHeadersRef: &v1alpha1.AdditionalHeadersRef{ + Name: "headers", + }, + AllowInsecureCertificate: true, + } +} +func getTestLBEndpoints(testDomain string) []*externaldnsendpoint.Endpoint { + return []*externaldnsendpoint.Endpoint{ + { + DNSName: testDomain, + RecordTTL: 300, + RecordType: "CNAME", + Targets: []string{ + "klb." + testDomain, + }, + }, + { + DNSName: "ip1." + testDomain, + RecordTTL: 60, + RecordType: "A", + Targets: []string{ + "172.32.200.1", + }, + }, + { + DNSName: "ip2." + testDomain, + RecordTTL: 60, + RecordType: "A", + Targets: []string{ + "172.32.200.2", + }, + }, + { + DNSName: "eu.klb." + testDomain, + RecordTTL: 60, + RecordType: "CNAME", + Targets: []string{ + "ip2." + testDomain, + }, + }, + { + DNSName: "us.klb." + testDomain, + RecordTTL: 60, + RecordType: "CNAME", + Targets: []string{ + "ip1." + testDomain, + }, + }, + { + DNSName: "klb." + testDomain, + ProviderSpecific: []externaldnsendpoint.ProviderSpecificProperty{ + { + Name: "geo-code", + Value: "*", + }, + }, + RecordTTL: 300, + RecordType: "CNAME", + SetIdentifier: "", + Targets: []string{ + "eu.klb." + testDomain, + }, + }, + { + DNSName: "klb." + testDomain, + ProviderSpecific: []externaldnsendpoint.ProviderSpecificProperty{ + { + Name: "geo-code", + Value: "GEO-NA", + }, + }, + RecordTTL: 300, + RecordType: "CNAME", + SetIdentifier: "", + Targets: []string{ + "us.klb." + testDomain, + }, + }, + { + DNSName: "klb." + testDomain, + ProviderSpecific: []externaldnsendpoint.ProviderSpecificProperty{ + { + Name: "geo-code", + Value: "GEO-EU", + }, + }, + RecordTTL: 300, + RecordType: "CNAME", + SetIdentifier: "", + Targets: []string{ + "eu.klb." + testDomain, + }, + }, + } +} diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index adc88ad2..3f0ed7ff 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -104,7 +104,7 @@ var _ = BeforeSuite(func() { Client: mgr.GetClient(), Scheme: mgr.GetScheme(), ProviderFactory: providerFactory, - }).SetupWithManager(mgr, RequeueDuration, ValidityDuration, DefaultValidationDuration) + }).SetupWithManager(mgr, RequeueDuration, ValidityDuration, DefaultValidationDuration, true, true) Expect(err).ToNot(HaveOccurred()) go func() { diff --git a/internal/probes/main.go b/internal/probes/main.go index 67f60f9e..f518da94 100644 --- a/internal/probes/main.go +++ b/internal/probes/main.go @@ -25,7 +25,10 @@ var ( ) const ( - PROBE_TIMEOUT = 3 * time.Second + PROBE_TIMEOUT = 3 * time.Second + DefaultProbeInterval = time.Second + DefaultFailureThreshold = 5 + DefaultInsecureCertAllowed = true ) type ProbeResult struct { diff --git a/test/e2e/healthcheck_test.go b/test/e2e/healthcheck_test.go index a4cc3bb7..b945dd03 100644 --- a/test/e2e/healthcheck_test.go +++ b/test/e2e/healthcheck_test.go @@ -3,18 +3,12 @@ package e2e import ( - "fmt" "strings" - "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - . "github.com/onsi/gomega/gstruct" - v1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/utils/strings/slices" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/kuadrant/dns-operator/api/v1alpha1" @@ -54,181 +48,7 @@ var _ = Describe("Health Check Test", Serial, Labels{"health_checks"}, func() { } }) - Context("DNS Provider health checks", func() { - It("creates health checks for a health check spec", func(ctx SpecContext) { - healthChecksSupported := false - if slices.Contains(supportedHealthCheckProviders, strings.ToLower(testDNSProvider)) { - healthChecksSupported = true - } - - dnsRecord = &v1alpha1.DNSRecord{} - err := ResourceFromFile("./fixtures/healthcheck_test/geo-dnsrecord-healthchecks.yaml", dnsRecord, GetTestEnv) - Expect(err).ToNot(HaveOccurred()) - - By("creating dnsrecord " + dnsRecord.Name) - err = k8sClient.Create(ctx, dnsRecord) - Expect(err).ToNot(HaveOccurred()) - - By("checking " + dnsRecord.Name + " becomes ready") - Eventually(func(g Gomega) { - err := k8sClient.Get(ctx, client.ObjectKeyFromObject(dnsRecord), dnsRecord) - g.Expect(err).NotTo(HaveOccurred()) - g.Expect(dnsRecord.Status.Conditions).To( - ContainElement(MatchFields(IgnoreExtras, Fields{ - "Type": Equal(string(v1alpha1.ConditionTypeReady)), - "Status": Equal(metav1.ConditionTrue), - })), - ) - }, recordsReadyMaxDuration, 10*time.Second, ctx).Should(Succeed()) - - By("Confirming the DNS Record status") - Eventually(func(g Gomega) { - err := k8sClient.Get(ctx, client.ObjectKeyFromObject(dnsRecord), dnsRecord) - g.Expect(err).ToNot(HaveOccurred()) - - if healthChecksSupported { - g.Expect(dnsRecord.Status.HealthCheck).ToNot(BeNil()) - g.Expect(&dnsRecord.Status.HealthCheck.Probes).ToNot(BeNil()) - g.Expect(len(dnsRecord.Status.HealthCheck.Probes)).ToNot(BeZero()) - for _, condition := range dnsRecord.Status.HealthCheck.Conditions { - if condition.Type == "healthProbesSynced" { - g.Expect(condition.Status).To(Equal(metav1.ConditionTrue)) - g.Expect(condition.Reason).To(Equal("AllProbesSynced")) - } - } - } else { - g.Expect(dnsRecord.Status.HealthCheck).ToNot(BeNil()) - g.Expect(dnsRecord.Status.HealthCheck.Probes).To(BeNil()) - } - - for _, probe := range dnsRecord.Status.HealthCheck.Probes { - g.Expect(probe.Host).To(Equal(testHostname)) - g.Expect(probe.IPAddress).To(Equal("172.32.200.1")) - g.Expect(probe.ID).ToNot(Equal("")) - - for _, probeCondition := range probe.Conditions { - g.Expect(probeCondition.Type).To(Equal("ProbeSynced")) - g.Expect(probeCondition.Status).To(Equal(metav1.ConditionTrue)) - g.Expect(probeCondition.Message).To(ContainSubstring(fmt.Sprintf("id: %v, address: %v, host: %v", probe.ID, probe.IPAddress, probe.Host))) - } - } - }, TestTimeoutMedium, time.Second).Should(Succeed()) - - provider, err := ProviderForDNSRecord(ctx, dnsRecord, k8sClient) - Expect(err).To(BeNil()) - - By("confirming the health checks exist in the provider") - Eventually(func(g Gomega) { - if !healthChecksSupported { - g.Expect(len(dnsRecord.Status.HealthCheck.Probes)).To(BeZero()) - } - for _, healthCheck := range dnsRecord.Status.HealthCheck.Probes { - exists, err := provider.HealthCheckReconciler().HealthCheckExists(ctx, &healthCheck) - g.Expect(err).To(BeNil()) - g.Expect(exists).To(BeTrue()) - } - }, TestTimeoutMedium, time.Second).Should(Succeed()) - - By("Removing the health check") - oldHealthCheckStatus := dnsRecord.Status.HealthCheck.DeepCopy() - Eventually(func(g Gomega) { - patchFrom := client.MergeFrom(dnsRecord.DeepCopy()) - dnsRecord.Spec.HealthCheck = nil - err := k8sClient.Patch(ctx, dnsRecord, patchFrom) - g.Expect(err).To(BeNil()) - }, TestTimeoutMedium, time.Second).Should(Succeed()) - - By("Confirming the DNS Record status") - Eventually(func(g Gomega) { - err := k8sClient.Get(ctx, client.ObjectKeyFromObject(dnsRecord), dnsRecord) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(dnsRecord.Status.HealthCheck).To(BeNil()) - }) - - By("confirming the health checks were removed in the provider") - Eventually(func(g Gomega) { - for _, healthCheck := range oldHealthCheckStatus.Probes { - exists, err := provider.HealthCheckReconciler().HealthCheckExists(ctx, &healthCheck) - g.Expect(err).NotTo(BeNil()) - g.Expect(exists).To(BeFalse()) - } - }, TestTimeoutMedium, time.Second).Should(Succeed()) - - By("Adding a health check spec") - Eventually(func(g Gomega) { - patchFrom := client.MergeFrom(dnsRecord.DeepCopy()) - err = ResourceFromFile("./fixtures/healthcheck_test/geo-dnsrecord-healthchecks.yaml", dnsRecord, GetTestEnv) - g.Expect(err).ToNot(HaveOccurred()) - err := k8sClient.Patch(ctx, dnsRecord, patchFrom) - g.Expect(err).To(BeNil()) - }, TestTimeoutMedium, time.Second).Should(Succeed()) - - By("Confirming the DNS Record status") - Eventually(func(g Gomega) { - err := k8sClient.Get(ctx, client.ObjectKeyFromObject(dnsRecord), dnsRecord) - g.Expect(err).ToNot(HaveOccurred()) - - if healthChecksSupported { - g.Expect(dnsRecord.Status.HealthCheck).ToNot(BeNil()) - g.Expect(&dnsRecord.Status.HealthCheck.Probes).ToNot(BeNil()) - g.Expect(len(dnsRecord.Status.HealthCheck.Probes)).ToNot(BeZero()) - for _, condition := range dnsRecord.Status.HealthCheck.Conditions { - if condition.Type == "healthProbesSynced" { - g.Expect(condition.Status).To(Equal(metav1.ConditionTrue)) - g.Expect(condition.Reason).To(Equal("AllProbesSynced")) - } - } - } else { - g.Expect(dnsRecord.Status.HealthCheck).ToNot(BeNil()) - g.Expect(dnsRecord.Status.HealthCheck.Probes).To(BeNil()) - } - - for _, probe := range dnsRecord.Status.HealthCheck.Probes { - g.Expect(probe.Host).To(Equal(testHostname)) - g.Expect(probe.IPAddress).To(Equal("172.32.200.1")) - g.Expect(probe.ID).ToNot(Equal("")) - - for _, probeCondition := range probe.Conditions { - g.Expect(probeCondition.Type).To(Equal("ProbeSynced")) - g.Expect(probeCondition.Status).To(Equal(metav1.ConditionTrue)) - g.Expect(probeCondition.Message).To(ContainSubstring(fmt.Sprintf("id: %v, address: %v, host: %v", probe.ID, probe.IPAddress, probe.Host))) - } - } - }, TestTimeoutMedium, time.Second).Should(Succeed()) - - By("confirming the health checks exist in the provider") - Eventually(func(g Gomega) { - if !healthChecksSupported { - g.Expect(len(dnsRecord.Status.HealthCheck.Probes)).To(BeZero()) - } - for _, healthCheck := range dnsRecord.Status.HealthCheck.Probes { - exists, err := provider.HealthCheckReconciler().HealthCheckExists(ctx, &healthCheck) - g.Expect(err).To(BeNil()) - g.Expect(exists).To(BeTrue()) - } - }, TestTimeoutMedium, time.Second).Should(Succeed()) - - By("Deleting the DNS Record") - oldHealthCheckStatus = dnsRecord.Status.HealthCheck.DeepCopy() - err = ResourceFromFile("./fixtures/healthcheck_test/geo-dnsrecord-healthchecks.yaml", dnsRecord, GetTestEnv) - Expect(err).ToNot(HaveOccurred()) - Eventually(func(g Gomega) { - err := k8sClient.Delete(ctx, dnsRecord) - g.Expect(client.IgnoreNotFound(err)).To(BeNil()) - - err = k8sClient.Get(ctx, client.ObjectKeyFromObject(dnsRecord), dnsRecord) - g.Expect(errors.IsNotFound(err)).Should(BeTrue()) - }, recordsRemovedMaxDuration, time.Second).Should(Succeed()) - - By("confirming the health checks were removed in the provider") - Eventually(func(g Gomega) { - for _, healthCheck := range oldHealthCheckStatus.Probes { - exists, err := provider.HealthCheckReconciler().HealthCheckExists(ctx, &healthCheck) - g.Expect(err).NotTo(BeNil()) - g.Expect(exists).To(BeFalse()) - } - }, TestTimeoutMedium, time.Second).Should(Succeed()) - - }) + Context("On-cluster healthchecks", func() { + // TODO test case }) })