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.

Sign up for GitHub

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

remove provider store and don't initialize each DNS provider #96

Merged
merged 4 commits into from
Mar 16, 2017
Merged

remove provider store and don't initialize each DNS provider #96

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
793931a
fix(dnsprovider): do not always initialize each DNS provider
Mar 14, 2017
0eeeac3
fix(dnsprovider): fix unnecessary error return value
Mar 16, 2017
73cb8a5
Merge remote-tracking branch 'upstream/master' into feat/initialize
Mar 16, 2017
5819be5
ref(dnsprovider): drop the store and use a simple switch for lookup
Mar 16, 2017
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 controller/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ type mockDNSProvider struct {
ExpectChanges *plan.Changes
}

// Initialize is a no-op for the mockDNSProvider.
func (p *mockDNSProvider) Initialize() error {
return nil
}

// Records returns the desired mock endpoints.
func (p *mockDNSProvider) Records(zone string) ([]endpoint.Endpoint, error) {
return p.RecordsStore, nil
Expand Down
16 changes: 9 additions & 7 deletions dnsprovider/aws.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,23 +48,25 @@ type AWSProvider struct {
}

// NewAWSProvider initializes a new AWS Route53 based DNSProvider.
func NewAWSProvider(dryRun bool) (DNSProvider, error) {
func NewAWSProvider(dryRun bool) DNSProvider {
return &AWSProvider{DryRun: dryRun}
}

// Initialize sets up the AWS API client.
func (p *AWSProvider) Initialize() error {
config := aws.NewConfig()

session, err := session.NewSessionWithOptions(session.Options{
Config: *config,
SharedConfigState: session.SharedConfigEnable,
})
if err != nil {
return nil, err
return err
}

provider := &AWSProvider{
Client: route53.New(session),
DryRun: dryRun,
}
p.Client = route53.New(session)

return provider, nil
return nil
}

// Zones returns the list of hosted zones.
Expand Down
1 change: 1 addition & 0 deletions dnsprovider/dnsprovider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (

// DNSProvider defines the interface DNS providers should implement.
type DNSProvider interface {
Initialize() error
Records(zone string) ([]endpoint.Endpoint, error)
ApplyChanges(zone string, changes *plan.Changes) error
}
23 changes: 13 additions & 10 deletions dnsprovider/google.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,24 +112,27 @@ type googleProvider struct {
}

// NewGoogleProvider initializes a new Google CloudDNS based DNSProvider.
func NewGoogleProvider(project string, dryRun bool) (DNSProvider, error) {
func NewGoogleProvider(project string, dryRun bool) DNSProvider {
return &googleProvider{project: project, dryRun: dryRun}
}

// Initialize sets up the Google API client.
func (p *googleProvider) Initialize() error {
gcloud, err := google.DefaultClient(context.TODO(), dns.NdevClouddnsReadwriteScope)
if err != nil {
return nil, err
return err
}

dnsClient, err := dns.New(gcloud)
if err != nil {
return nil, err
return err
}

return &googleProvider{
project: project,
dryRun: dryRun,
resourceRecordSetsClient: resourceRecordSetsService{dnsClient.ResourceRecordSets},
managedZonesClient: managedZonesService{dnsClient.ManagedZones},
changesClient: changesService{dnsClient.Changes},
}, nil
p.resourceRecordSetsClient = resourceRecordSetsService{dnsClient.ResourceRecordSets}
p.managedZonesClient = managedZonesService{dnsClient.ManagedZones}
p.changesClient = changesService{dnsClient.Changes}

return nil
}

// Zones returns the list of hosted zones.
Expand Down
5 changes: 5 additions & 0 deletions dnsprovider/inmemory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ func NewInMemoryProvider() *InMemoryProvider {
}
}

// Initialize is a no-op for the InMemoryProvider.
func (im *InMemoryProvider) Initialize() error {
return nil
}

// InMemoryRecord - record stored in memory
// has additional fields:
// Type - type of string (TODO: Type should probably be part of endpoint struct)
Expand Down
17 changes: 7 additions & 10 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,23 +79,20 @@ func main() {

sources := source.NewMultiSource(source.LookupMultiple(cfg.Sources...)...)

googleProvider, err := dnsprovider.NewGoogleProvider(cfg.GoogleProject, cfg.DryRun)
if err != nil {
log.Fatal(err)
}
dnsprovider.Register("google", dnsprovider.NewGoogleProvider(cfg.GoogleProject, cfg.DryRun))
dnsprovider.Register("aws", dnsprovider.NewAWSProvider(cfg.DryRun))
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it not make more sense to only register/initialize the providers which are required?

E.g. the interface would be more like this:

provider, err := dnsprovider.Initialize(cfg.DNSProvider)

Disregard if I'm missing some context.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, sounds right. However, you would just move the following lines into a method in the dnsprovider package (I wouldn't consider this a change).

The registration part here is more annoying. It needs provider specific configuration passed in, e.g. the google project. So we still need it this way I believe. We could pass arbitrary config to your proposed Initialize method and then just forward to the picked provider, but I'm not sure if that would be any better.

Copy link

Choose a reason for hiding this comment

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

yes passing "arbitrary config" to Initialize method is not best approach IMO. Another way, might be to allow store return DNS provider with/without client. But registration should occur via passing raw object. E.g:

dnsprovider.Register("aws", &dnsprovider.AWSProvider{DryRun:DryRun})
...

then store

dnsprovider.LookUpWithClient(cfg.Dnsprovider) //creates client internally based on dns provider

not sure if this is any better

Copy link

Choose a reason for hiding this comment

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

and dnsprovider.NewAWSProvider() still initializes the client internally


awsProvider, err := dnsprovider.NewAWSProvider(cfg.DryRun)
if err != nil {
log.Fatal(err)
provider := dnsprovider.Lookup(cfg.DNSProvider)
if provider == nil {
log.Fatalf("unknown dns provider: %s", cfg.DNSProvider)
}

dnsprovider.Register("google", googleProvider)
dnsprovider.Register("aws", awsProvider)
provider.Initialize()

ctrl := controller.Controller{
Zone: cfg.Zone,
Source: sources,
DNSProvider: dnsprovider.Lookup(cfg.DNSProvider),
DNSProvider: provider,
}

if cfg.Once {
Expand Down