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

refactoring suggestions #517

Merged
merged 1 commit into from
Jun 9, 2021
Merged
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
8 changes: 3 additions & 5 deletions controllers/gslb_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions controllers/providers/assistant/assistant.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
16 changes: 8 additions & 8 deletions controllers/providers/assistant/assistant_mock.go

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

40 changes: 21 additions & 19 deletions controllers/providers/assistant/gslb.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion controllers/providers/dns/empty.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion controllers/providers/dns/external.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
4 changes: 2 additions & 2 deletions controllers/providers/dns/external_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion controllers/providers/dns/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
3 changes: 1 addition & 2 deletions controllers/providers/dns/infoblox.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 "+
Expand Down Expand Up @@ -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) {
Expand Down
4 changes: 2 additions & 2 deletions controllers/providers/dns/infoblox_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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)
Expand Down