refactoring suggestions (#517)

- I renamed confusing GslbLoggerAssistant to Gslb in package assistant
- I moved edgeDNSServerPort into GslbAssistant structure, instead of pass it through function arguments

Signed-off-by: kuritka <kuritka@gmail.com>

controllers/gslb_controller_test.go

@@ -475,8 +475,7 @@ func TestCanCheckExternalGslbTXTRecordForValidityAndFailIfItIsExpired(t *testing
 		RunTestFunc(func() {
 			settings := provideSettings(t, predefinedConfig)
 			// act
-			got := settings.assistant.InspectTXTThreshold("test-gslb-heartbeat-eu.example.com",
-				predefinedConfig.EdgeDNSServerPort, time.Minute*5)
+			got := settings.assistant.InspectTXTThreshold("test-gslb-heartbeat-eu.example.com", time.Minute*5)
 			want := errors.NewResourceExpired("Split brain TXT record expired the time threshold: (5m0s)")
 			// assert
 			assert.Equal(t, want, got, "got:\n %s from TXT split brain check,\n\n want error:\n %v", got, want)
@@ -491,8 +490,7 @@ func TestCanCheckExternalGslbTXTRecordForValidityAndPAssIfItISNotExpired(t *test
 		RunTestFunc(func() {
 			settings := provideSettings(t, predefinedConfig)
 			// act
-			err2 := settings.assistant.InspectTXTThreshold("test-gslb-heartbeat-za.example.com",
-				predefinedConfig.EdgeDNSServerPort, time.Minute*5)
+			err2 := settings.assistant.InspectTXTThreshold("test-gslb-heartbeat-za.example.com", time.Minute*5)
 			// assert
 			assert.NoError(t, err2, "got:\n %s from TXT split brain check,\n\n want error:\n %v", err2, nil)
 		}).RequireNoError(t)
@@ -1179,7 +1177,7 @@ func provideSettings(t *testing.T, expected depresolver.Config) (settings testSe
 		t.Fatalf("reconcile: (%v)", err)
 	}
 	r.DNSProvider = f.Provider()
-	a := assistant.NewGslbAssistant(r.Client, r.Config.K8gbNamespace, r.Config.EdgeDNSServer)
+	a := assistant.NewGslbAssistant(r.Client, r.Config.K8gbNamespace, r.Config.EdgeDNSServer, r.Config.EdgeDNSServerPort)
 	res, err := r.Reconcile(context.TODO(), req)
 	if err != nil {
 		t.Fatalf("reconcile: (%v)", err)

controllers/providers/assistant/assistant.go

@@ -30,12 +30,12 @@ type Assistant interface {
 	// GslbIngressExposedIPs retrieves list of IP's exposed by all GSLB ingresses
 	GslbIngressExposedIPs(gslb *k8gbv1beta1.Gslb) ([]string, error)
 	// GetExternalTargets retrieves slice of targets from external clusters
-	GetExternalTargets(host string, edgeDNSServerPort int, extClusterNsNames map[string]string) (targets []string)
+	GetExternalTargets(host string, extClusterNsNames map[string]string) (targets []string)
 	// SaveDNSEndpoint update DNS endpoint or create new one if doesnt exist
 	SaveDNSEndpoint(namespace string, i *externaldns.DNSEndpoint) error
 	// RemoveEndpoint removes endpoint
 	RemoveEndpoint(endpointName string) error
 	// InspectTXTThreshold inspects fqdn TXT record from edgeDNSServer. If record doesn't exists or timestamp is greater than
 	// splitBrainThreshold the error is returned. In case fakeDNSEnabled is true, 127.0.0.1:7753 is used as edgeDNSServer
-	InspectTXTThreshold(fqdn string, edgeDNSServerPort int, splitBrainThreshold time.Duration) error
+	InspectTXTThreshold(fqdn string, splitBrainThreshold time.Duration) error
 }

controllers/providers/assistant/gslb.go

@@ -40,26 +40,28 @@ import (
 
 const coreDNSExtServiceName = "k8gb-coredns-lb"
 
-// GslbLoggerAssistant is common wrapper operating on GSLB instance.
+// Gslb is common wrapper operating on GSLB instance.
 // It uses apimachinery client to call kubernetes API
-type GslbLoggerAssistant struct {
-	client        client.Client
-	k8gbNamespace string
-	edgeDNSServer string
+type Gslb struct {
+	client            client.Client
+	k8gbNamespace     string
+	edgeDNSServer     string
+	edgeDNSServerPort int
 }
 
 var log = logging.Logger()
 
-func NewGslbAssistant(client client.Client, k8gbNamespace, edgeDNSServer string) *GslbLoggerAssistant {
-	return &GslbLoggerAssistant{
-		client:        client,
-		k8gbNamespace: k8gbNamespace,
-		edgeDNSServer: edgeDNSServer,
+func NewGslbAssistant(client client.Client, k8gbNamespace, edgeDNSServer string, edgeDNSServerPort int) *Gslb {
+	return &Gslb{
+		client:            client,
+		k8gbNamespace:     k8gbNamespace,
+		edgeDNSServer:     edgeDNSServer,
+		edgeDNSServerPort: edgeDNSServerPort,
 	}
 }
 
 // CoreDNSExposedIPs retrieves list of IP's exposed by CoreDNS
-func (r *GslbLoggerAssistant) CoreDNSExposedIPs() ([]string, error) {
+func (r *Gslb) CoreDNSExposedIPs() ([]string, error) {
 	coreDNSService := &corev1.Service{}
 	err := r.client.Get(context.TODO(),
 		types.NamespacedName{Namespace: r.k8gbNamespace, Name: coreDNSExtServiceName}, coreDNSService)
@@ -87,7 +89,7 @@ func (r *GslbLoggerAssistant) CoreDNSExposedIPs() ([]string, error) {
 }
 
 // GslbIngressExposedIPs retrieves list of IP's exposed by all GSLB ingresses
-func (r *GslbLoggerAssistant) GslbIngressExposedIPs(gslb *k8gbv1beta1.Gslb) ([]string, error) {
+func (r *Gslb) GslbIngressExposedIPs(gslb *k8gbv1beta1.Gslb) ([]string, error) {
 	nn := types.NamespacedName{
 		Name:      gslb.Name,
 		Namespace: gslb.Namespace,
@@ -123,7 +125,7 @@ func (r *GslbLoggerAssistant) GslbIngressExposedIPs(gslb *k8gbv1beta1.Gslb) ([]s
 }
 
 // SaveDNSEndpoint update DNS endpoint or create new one if doesnt exist
-func (r *GslbLoggerAssistant) SaveDNSEndpoint(namespace string, i *externaldns.DNSEndpoint) error {
+func (r *Gslb) SaveDNSEndpoint(namespace string, i *externaldns.DNSEndpoint) error {
 	found := &externaldns.DNSEndpoint{}
 	err := r.client.Get(context.TODO(), types.NamespacedName{
 		Name:      i.Name,
@@ -165,7 +167,7 @@ func (r *GslbLoggerAssistant) SaveDNSEndpoint(namespace string, i *externaldns.D
 }
 
 // RemoveEndpoint removes endpoint
-func (r *GslbLoggerAssistant) RemoveEndpoint(endpointName string) error {
+func (r *Gslb) RemoveEndpoint(endpointName string) error {
 	log.Info().Msgf("Removing endpoint %s.%s", r.k8gbNamespace, endpointName)
 	dnsEndpoint := &externaldns.DNSEndpoint{}
 	err := r.client.Get(context.Background(), client.ObjectKey{Namespace: r.k8gbNamespace, Name: endpointName}, dnsEndpoint)
@@ -182,10 +184,10 @@ func (r *GslbLoggerAssistant) RemoveEndpoint(endpointName string) error {
 
 // InspectTXTThreshold inspects fqdn TXT record from edgeDNSServer. If record doesn't exists or timestamp is greater than
 // splitBrainThreshold the error is returned.
-func (r *GslbLoggerAssistant) InspectTXTThreshold(fqdn string, edgeDNSServerPort int, splitBrainThreshold time.Duration) error {
+func (r *Gslb) InspectTXTThreshold(fqdn string, splitBrainThreshold time.Duration) error {
 	m := new(dns.Msg)
 	m.SetQuestion(dns.Fqdn(fqdn), dns.TypeTXT)
-	ns := fmt.Sprintf("%s:%v", r.edgeDNSServer, edgeDNSServerPort)
+	ns := fmt.Sprintf("%s:%v", r.edgeDNSServer, r.edgeDNSServerPort)
 	txt, err := dns.Exchange(m, ns)
 	if err != nil {
 		log.Info().Msgf("Error contacting EdgeDNS server (%s) for TXT split brain record: (%s)", ns, err)
@@ -242,12 +244,12 @@ func dnsQuery(host string, nameserver string, nameserverport int) (*dns.Msg, err
 	return dnsMsgA, err
 }
 
-func (r *GslbLoggerAssistant) GetExternalTargets(host string, edgeDNSServerPort int, extClusterNsNames map[string]string) (targets []string) {
+func (r *Gslb) GetExternalTargets(host string, extClusterNsNames map[string]string) (targets []string) {
 	targets = []string{}
 	for _, cluster := range extClusterNsNames {
 		// Use edgeDNSServer for resolution of NS names and fallback to local nameservers
 		log.Info().Msgf("Adding external Gslb targets from %s cluster...", cluster)
-		glueA, err := dnsQuery(cluster, r.edgeDNSServer, edgeDNSServerPort)
+		glueA, err := dnsQuery(cluster, r.edgeDNSServer, r.edgeDNSServerPort)
 		if err != nil {
 			return
 		}
@@ -260,7 +262,7 @@ func (r *GslbLoggerAssistant) GetExternalTargets(host string, edgeDNSServerPort
 			nameServerToUse = cluster
 		}
 		host = fmt.Sprintf("localtargets-%s", host)
-		a, err := dnsQuery(host, nameServerToUse, edgeDNSServerPort)
+		a, err := dnsQuery(host, nameServerToUse, r.edgeDNSServerPort)
 		if err != nil {
 			return
 		}

controllers/providers/dns/empty.go

@@ -46,7 +46,7 @@ func (p *EmptyDNSProvider) GslbIngressExposedIPs(gslb *k8gbv1beta1.Gslb) (r []st
 }
 
 func (p *EmptyDNSProvider) GetExternalTargets(host string) (targets []string) {
-	return p.assistant.GetExternalTargets(host, p.config.EdgeDNSServerPort, p.config.GetExternalClusterNSNames())
+	return p.assistant.GetExternalTargets(host, p.config.GetExternalClusterNSNames())
 }
 
 func (p *EmptyDNSProvider) SaveDNSEndpoint(gslb *k8gbv1beta1.Gslb, i *externaldns.DNSEndpoint) error {

controllers/providers/dns/external.go

@@ -110,7 +110,7 @@ func (p *ExternalDNSProvider) Finalize(*k8gbv1beta1.Gslb) error {
 }
 
 func (p *ExternalDNSProvider) GetExternalTargets(host string) (targets []string) {
-	return p.assistant.GetExternalTargets(host, p.config.EdgeDNSServerPort, p.config.GetExternalClusterNSNames())
+	return p.assistant.GetExternalTargets(host, p.config.GetExternalClusterNSNames())
 }
 
 func (p *ExternalDNSProvider) GslbIngressExposedIPs(gslb *k8gbv1beta1.Gslb) ([]string, error) {

controllers/providers/dns/external_test.go

@@ -139,7 +139,7 @@ func TestSaveNewDNSEndpointOnExternalDNS(t *testing.T) {
 
 	var cl = fake.NewClientBuilder().WithScheme(runtimeScheme).WithObjects(ep).Build()
 
-	assistant := assistant.NewGslbAssistant(cl, a.Config.K8gbNamespace, a.Config.EdgeDNSServer)
+	assistant := assistant.NewGslbAssistant(cl, a.Config.K8gbNamespace, a.Config.EdgeDNSServer, a.Config.EdgeDNSServerPort)
 	p := NewExternalDNS(dnsType, a.Config, assistant)
 	// act, assert
 	err := p.SaveDNSEndpoint(a.Gslb, expectedDNSEndpoint)
@@ -161,7 +161,7 @@ func TestSaveExistingDNSEndpointOnExternalDNS(t *testing.T) {
 	require.NoError(t, schemeBuilder.AddToScheme(runtimeScheme))
 
 	var cl = fake.NewClientBuilder().WithScheme(runtimeScheme).WithObjects(endpointToSave).Build()
-	assistant := assistant.NewGslbAssistant(cl, a.Config.K8gbNamespace, a.Config.EdgeDNSServer)
+	assistant := assistant.NewGslbAssistant(cl, a.Config.K8gbNamespace, a.Config.EdgeDNSServer, a.Config.EdgeDNSServerPort)
 	p := NewExternalDNS(dnsType, a.Config, assistant)
 	// act, assert
 	err := p.SaveDNSEndpoint(a.Gslb, endpointToSave)

controllers/providers/dns/factory.go

@@ -43,7 +43,7 @@ func NewDNSProviderFactory(client client.Client, config depresolver.Config) (f *
 }
 
 func (f *ProviderFactory) Provider() Provider {
-	a := assistant.NewGslbAssistant(f.client, f.config.K8gbNamespace, f.config.EdgeDNSServer)
+	a := assistant.NewGslbAssistant(f.client, f.config.K8gbNamespace, f.config.EdgeDNSServer, f.config.EdgeDNSServerPort)
 	switch f.config.EdgeDNSType {
 	case depresolver.DNSTypeNS1:
 		return NewExternalDNS(externalDNSTypeNS1, f.config, a)

controllers/providers/dns/infoblox.go

@@ -96,7 +96,6 @@ func (p *InfobloxProvider) CreateZoneDelegationForExternalDNS(gslb *k8gbv1beta1.
 				for extClusterGeoTag, nsServerNameExt := range p.config.GetExternalClusterNSNames() {
 					err = p.assistant.InspectTXTThreshold(
 						extClusterHeartbeatFQDNs[extClusterGeoTag],
-						p.config.EdgeDNSServerPort,
 						time.Second*time.Duration(gslb.Spec.Strategy.SplitBrainThresholdSeconds))
 					if err != nil {
 						log.Err(err).Msgf("Got the error from TXT based checkAlive. External cluster (%s) doesn't "+
@@ -171,7 +170,7 @@ func (p *InfobloxProvider) Finalize(gslb *k8gbv1beta1.Gslb) error {
 }
 
 func (p *InfobloxProvider) GetExternalTargets(host string) (targets []string) {
-	return p.assistant.GetExternalTargets(host, p.config.EdgeDNSServerPort, p.config.GetExternalClusterNSNames())
+	return p.assistant.GetExternalTargets(host, p.config.GetExternalClusterNSNames())
 }
 
 func (p *InfobloxProvider) GslbIngressExposedIPs(gslb *k8gbv1beta1.Gslb) ([]string, error) {

controllers/providers/dns/infoblox_test.go

@@ -65,7 +65,7 @@ func TestCanFilterOutDelegatedZoneEntryAccordingFQDNProvided(t *testing.T) {
 	customConfig := predefinedConfig
 	customConfig.EdgeDNSZone = "example.com"
 	customConfig.ExtClustersGeoTags = []string{"za"}
-	a := assistant.NewGslbAssistant(nil, customConfig.K8gbNamespace, customConfig.EdgeDNSServer)
+	a := assistant.NewGslbAssistant(nil, customConfig.K8gbNamespace, customConfig.EdgeDNSServer, customConfig.EdgeDNSServerPort)
 	provider := NewInfobloxDNS(customConfig, a)
 	// act
 	extClusters := customConfig.GetExternalClusterNSNames()
@@ -100,7 +100,7 @@ func TestCanSanitizeDelegatedZone(t *testing.T) {
 	customConfig.EdgeDNSZone = "example.com"
 	customConfig.ExtClustersGeoTags = []string{"za"}
 	customConfig.ClusterGeoTag = "eu"
-	a := assistant.NewGslbAssistant(nil, customConfig.K8gbNamespace, customConfig.EdgeDNSServer)
+	a := assistant.NewGslbAssistant(nil, customConfig.K8gbNamespace, customConfig.EdgeDNSServer, customConfig.EdgeDNSServerPort)
 	provider := NewInfobloxDNS(customConfig, a)
 	// act
 	got := provider.sanitizeDelegateZone(local, upstream)