Skip to content

Commit

Permalink
source: explicitly handle default annotations
Browse files Browse the repository at this point in the history 
This commit fixes a bug that causes the Cloudflare provider to delete
and recreate all records every minute.
The controller is running into trouble when reconciling endpoints with
values with implicit defaults. This is caused because the providers
produce records with all values explicitly set, however the sources
generate endpoints with implicit provider-specific values, i.e. when an
annotation is not set. This causes the planner to always detect a diff.

xref: #1540

Signed-off-by: Lucas Servén Marín <lserven@gmail.com>
  • Loading branch information
@squat
squat committed May 11, 2020
1 parent 2ef1503 commit 3448610
Show file tree
Hide file tree
Showing 15 changed files with 106 additions and 78 deletions.
3 changes: 2 additions & 1 deletion main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ func main() {
ContourLoadBalancerService: cfg.ContourLoadBalancerService,
SkipperRouteGroupVersion: cfg.SkipperRouteGroupVersion,
RequestTimeout: cfg.RequestTimeout,
DefaultAnnotations: source.DefaultAnnotations(cfg.CloudflareProxied),
}

// Lookup all the selected sources by names and pass them the desired configuration.
Expand Down Expand Up @@ -194,7 +195,7 @@ func main() {
case "vultr":
p, err = vultr.NewVultrProvider(domainFilter, cfg.DryRun)
case "cloudflare":
p, err = cloudflare.NewCloudFlareProvider(domainFilter, zoneIDFilter, cfg.CloudflareZonesPerPage, cfg.CloudflareProxied, cfg.DryRun)
p, err = cloudflare.NewCloudFlareProvider(domainFilter, zoneIDFilter, cfg.CloudflareZonesPerPage, cfg.DryRun)
case "rcodezero":
p, err = rcode0.NewRcodeZeroProvider(domainFilter, cfg.DryRun, cfg.RcodezeroTXTEncrypt)
case "google":
Expand Down
35 changes: 18 additions & 17 deletions provider/cloudflare/cloudflare.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@ type CloudFlareProvider struct {
// only consider hosted zones managing domains ending in this suffix
domainFilter endpoint.DomainFilter
zoneIDFilter provider.ZoneIDFilter
proxiedByDefault bool
DryRun bool
PaginationOptions cloudflare.PaginationOptions
}
Expand All @@ -117,7 +116,7 @@ type cloudFlareChange struct {
}

// NewCloudFlareProvider initializes a new CloudFlare DNS based Provider.
func NewCloudFlareProvider(domainFilter endpoint.DomainFilter, zoneIDFilter provider.ZoneIDFilter, zonesPerPage int, proxiedByDefault bool, dryRun bool) (*CloudFlareProvider, error) {
func NewCloudFlareProvider(domainFilter endpoint.DomainFilter, zoneIDFilter provider.ZoneIDFilter, zonesPerPage int, dryRun bool) (*CloudFlareProvider, error) {
// initialize via chosen auth method and returns new API object
var (
config *cloudflare.API
Expand All @@ -133,11 +132,10 @@ func NewCloudFlareProvider(domainFilter endpoint.DomainFilter, zoneIDFilter prov
}
provider := &CloudFlareProvider{
//Client: config,
Client: zoneService{config},
domainFilter: domainFilter,
zoneIDFilter: zoneIDFilter,
proxiedByDefault: proxiedByDefault,
DryRun: dryRun,
Client: zoneService{config},
domainFilter: domainFilter,
zoneIDFilter: zoneIDFilter,
DryRun: dryRun,
PaginationOptions: cloudflare.PaginationOptions{
PerPage: zonesPerPage,
Page: 1,
Expand Down Expand Up @@ -200,13 +198,16 @@ func (p *CloudFlareProvider) Records(ctx context.Context) ([]*endpoint.Endpoint,

// ApplyChanges applies a given set of changes in a given zone.
func (p *CloudFlareProvider) ApplyChanges(ctx context.Context, changes *plan.Changes) error {
proxiedByDefault := p.proxiedByDefault
for i := range changes.UpdateOld {
fmt.Println(changes.UpdateOld[i].String())
fmt.Println(changes.UpdateNew[i].String())
}

combinedChanges := make([]*cloudFlareChange, 0, len(changes.Create)+len(changes.UpdateNew)+len(changes.Delete))

combinedChanges = append(combinedChanges, newCloudFlareChanges(cloudFlareCreate, changes.Create, proxiedByDefault)...)
combinedChanges = append(combinedChanges, newCloudFlareChanges(cloudFlareUpdate, changes.UpdateNew, proxiedByDefault)...)
combinedChanges = append(combinedChanges, newCloudFlareChanges(cloudFlareDelete, changes.Delete, proxiedByDefault)...)
combinedChanges = append(combinedChanges, newCloudFlareChanges(cloudFlareCreate, changes.Create)...)
combinedChanges = append(combinedChanges, newCloudFlareChanges(cloudFlareUpdate, changes.UpdateNew)...)
combinedChanges = append(combinedChanges, newCloudFlareChanges(cloudFlareDelete, changes.Delete)...)

return p.submitChanges(ctx, combinedChanges)
}
Expand Down Expand Up @@ -305,19 +306,19 @@ func (p *CloudFlareProvider) getRecordIDs(records []cloudflare.DNSRecord, record
}

// newCloudFlareChanges returns a collection of Changes based on the given records and action.
func newCloudFlareChanges(action string, endpoints []*endpoint.Endpoint, proxiedByDefault bool) []*cloudFlareChange {
func newCloudFlareChanges(action string, endpoints []*endpoint.Endpoint) []*cloudFlareChange {
changes := make([]*cloudFlareChange, 0, len(endpoints))

for _, endpoint := range endpoints {
changes = append(changes, newCloudFlareChange(action, endpoint, proxiedByDefault))
changes = append(changes, newCloudFlareChange(action, endpoint))
}

return changes
}

func newCloudFlareChange(action string, endpoint *endpoint.Endpoint, proxiedByDefault bool) *cloudFlareChange {
func newCloudFlareChange(action string, endpoint *endpoint.Endpoint) *cloudFlareChange {
ttl := defaultCloudFlareRecordTTL
proxied := shouldBeProxied(endpoint, proxiedByDefault)
proxied := shouldBeProxied(endpoint)

if endpoint.RecordTTL.IsConfigured() {
ttl = int(endpoint.RecordTTL)
Expand All @@ -341,8 +342,8 @@ func newCloudFlareChange(action string, endpoint *endpoint.Endpoint, proxiedByDe
}
}

func shouldBeProxied(endpoint *endpoint.Endpoint, proxiedByDefault bool) bool {
proxied := proxiedByDefault
func shouldBeProxied(endpoint *endpoint.Endpoint) bool {
var proxied bool

for _, v := range endpoint.ProviderSpecific {
if v.Name == source.CloudflareProxiedKey {
Expand Down
39 changes: 6 additions & 33 deletions provider/cloudflare/cloudflare_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -346,30 +346,6 @@ func TestCloudflareCustomTTL(t *testing.T) {
})
}

func TestCloudflareProxiedDefault(t *testing.T) {
endpoints := []*endpoint.Endpoint{
{
RecordType: "A",
DNSName: "bar.com",
Targets: endpoint.Targets{"127.0.0.1"},
},
}

AssertActions(t, &CloudFlareProvider{proxiedByDefault: true}, endpoints, []MockAction{
{
Name: "Create",
ZoneId: "001",
RecordData: cloudflare.DNSRecord{
Type: "A",
Name: "bar.com",
Content: "127.0.0.1",
TTL: 1,
Proxied: true,
},
},
})
}

func TestCloudflareProxiedOverrideTrue(t *testing.T) {
endpoints := []*endpoint.Endpoint{
{
Expand Down Expand Up @@ -415,7 +391,7 @@ func TestCloudflareProxiedOverrideFalse(t *testing.T) {
},
}

AssertActions(t, &CloudFlareProvider{proxiedByDefault: true}, endpoints, []MockAction{
AssertActions(t, &CloudFlareProvider{}, endpoints, []MockAction{
{
Name: "Create",
ZoneId: "001",
Expand Down Expand Up @@ -445,7 +421,7 @@ func TestCloudflareProxiedOverrideIllegal(t *testing.T) {
},
}

AssertActions(t, &CloudFlareProvider{proxiedByDefault: true}, endpoints, []MockAction{
AssertActions(t, &CloudFlareProvider{}, endpoints, []MockAction{
{
Name: "Create",
ZoneId: "001",
Expand All @@ -454,7 +430,7 @@ func TestCloudflareProxiedOverrideIllegal(t *testing.T) {
Name: "bar.com",
Content: "127.0.0.1",
TTL: 1,
Proxied: true,
Proxied: false,
},
},
})
Expand Down Expand Up @@ -558,8 +534,7 @@ func TestCloudflareProvider(t *testing.T) {
endpoint.NewDomainFilter([]string{"bar.com"}),
provider.NewZoneIDFilter([]string{""}),
25,
false,
true)
false)
if err != nil {
t.Errorf("should not fail, %s", err)
}
Expand All @@ -570,8 +545,7 @@ func TestCloudflareProvider(t *testing.T) {
endpoint.NewDomainFilter([]string{"bar.com"}),
provider.NewZoneIDFilter([]string{""}),
1,
false,
true)
false)
if err != nil {
t.Errorf("should not fail, %s", err)
}
Expand All @@ -581,8 +555,7 @@ func TestCloudflareProvider(t *testing.T) {
endpoint.NewDomainFilter([]string{"bar.com"}),
provider.NewZoneIDFilter([]string{""}),
50,
false,
true)
false)
if err == nil {
t.Errorf("expected to fail")
}
Expand Down
7 changes: 5 additions & 2 deletions source/gateway.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ type gatewaySource struct {
combineFQDNAnnotation bool
ignoreHostnameAnnotation bool
serviceInformer coreinformers.ServiceInformer
defaultAnnotations map[string]string
}

// NewIstioGatewaySource creates a new gatewaySource with the given config.
Expand All @@ -61,6 +62,7 @@ func NewIstioGatewaySource(
fqdnTemplate string,
combineFqdnAnnotation bool,
ignoreHostnameAnnotation bool,
defaultAnnotations map[string]string,
) (Source, error) {
var (
tmpl *template.Template
Expand Down Expand Up @@ -110,6 +112,7 @@ func NewIstioGatewaySource(
combineFQDNAnnotation: combineFqdnAnnotation,
ignoreHostnameAnnotation: ignoreHostnameAnnotation,
serviceInformer: serviceInformer,
defaultAnnotations: defaultAnnotations,
}, nil
}

Expand Down Expand Up @@ -200,7 +203,7 @@ func (sc *gatewaySource) endpointsFromTemplate(config *istiomodel.Config) ([]*en
}
}

providerSpecific, setIdentifier := getProviderSpecificAnnotations(config.Annotations)
providerSpecific, setIdentifier := getProviderSpecificAnnotations(annotationsWithDefaults(config.Annotations, sc.defaultAnnotations))

var endpoints []*endpoint.Endpoint
// splits the FQDN template and removes the trailing periods
Expand Down Expand Up @@ -300,7 +303,7 @@ func (sc *gatewaySource) endpointsFromGatewayConfig(config istiomodel.Config) ([

gateway := config.Spec.(*istionetworking.Gateway)

providerSpecific, setIdentifier := getProviderSpecificAnnotations(config.Annotations)
providerSpecific, setIdentifier := getProviderSpecificAnnotations(annotationsWithDefaults(config.Annotations, sc.defaultAnnotations))

for _, server := range gateway.Servers {
for _, host := range server.Hosts {
Expand Down
4 changes: 4 additions & 0 deletions source/gateway_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ func (suite *GatewaySuite) SetupTest() {
"{{.Name}}",
false,
false,
DefaultAnnotations(false),
)
suite.NoError(err, "should initialize gateway source")

Expand Down Expand Up @@ -152,6 +153,7 @@ func TestNewIstioGatewaySource(t *testing.T) {
ti.fqdnTemplate,
ti.combineFQDNAndAnnotation,
false,
DefaultAnnotations(false),
)
if ti.expectError {
assert.Error(t, err)
Expand Down Expand Up @@ -1113,6 +1115,7 @@ func testGatewayEndpoints(t *testing.T) {
ti.fqdnTemplate,
ti.combineFQDNAndAnnotation,
ti.ignoreHostnameAnnotation,
DefaultAnnotations(false),
)
require.NoError(t, err)

Expand Down Expand Up @@ -1149,6 +1152,7 @@ func newTestGatewaySource(loadBalancerList []fakeIngressGatewayService) (*gatewa
"{{.Name}}",
false,
false,
DefaultAnnotations(false),
)
if err != nil {
return nil, err
Expand Down
12 changes: 7 additions & 5 deletions source/ingress.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,11 @@ type ingressSource struct {
ignoreHostnameAnnotation bool
ingressInformer extinformers.IngressInformer
runner *async.BoundedFrequencyRunner
defaultAnnotations map[string]string
}

// NewIngressSource creates a new ingressSource with the given config.
func NewIngressSource(kubeClient kubernetes.Interface, namespace, annotationFilter string, fqdnTemplate string, combineFqdnAnnotation bool, ignoreHostnameAnnotation bool) (Source, error) {
func NewIngressSource(kubeClient kubernetes.Interface, namespace, annotationFilter string, fqdnTemplate string, combineFqdnAnnotation bool, ignoreHostnameAnnotation bool, defaultAnnotations map[string]string) (Source, error) {
var (
tmpl *template.Template
err error
Expand Down Expand Up @@ -106,6 +107,7 @@ func NewIngressSource(kubeClient kubernetes.Interface, namespace, annotationFilt
combineFQDNAnnotation: combineFqdnAnnotation,
ignoreHostnameAnnotation: ignoreHostnameAnnotation,
ingressInformer: ingressInformer,
defaultAnnotations: defaultAnnotations,
}
return sc, nil
}
Expand Down Expand Up @@ -133,7 +135,7 @@ func (sc *ingressSource) Endpoints() ([]*endpoint.Endpoint, error) {
continue
}

ingEndpoints := endpointsFromIngress(ing, sc.ignoreHostnameAnnotation)
ingEndpoints := sc.endpointsFromIngress(ing, sc.ignoreHostnameAnnotation)

// apply template if host is missing on ingress
if (sc.combineFQDNAnnotation || len(ingEndpoints) == 0) && sc.fqdnTemplate != nil {
Expand Down Expand Up @@ -188,7 +190,7 @@ func (sc *ingressSource) endpointsFromTemplate(ing *v1beta1.Ingress) ([]*endpoin
targets = targetsFromIngressStatus(ing.Status)
}

providerSpecific, setIdentifier := getProviderSpecificAnnotations(ing.Annotations)
providerSpecific, setIdentifier := getProviderSpecificAnnotations(annotationsWithDefaults(ing.Annotations, sc.defaultAnnotations))

var endpoints []*endpoint.Endpoint
// splits the FQDN template and removes the trailing periods
Expand Down Expand Up @@ -241,7 +243,7 @@ func (sc *ingressSource) setDualstackLabel(ingress *v1beta1.Ingress, endpoints [
}

// endpointsFromIngress extracts the endpoints from ingress object
func endpointsFromIngress(ing *v1beta1.Ingress, ignoreHostnameAnnotation bool) []*endpoint.Endpoint {
func (sc *ingressSource) endpointsFromIngress(ing *v1beta1.Ingress, ignoreHostnameAnnotation bool) []*endpoint.Endpoint {
var endpoints []*endpoint.Endpoint

ttl, err := getTTLFromAnnotations(ing.Annotations)
Expand All @@ -255,7 +257,7 @@ func endpointsFromIngress(ing *v1beta1.Ingress, ignoreHostnameAnnotation bool) [
targets = targetsFromIngressStatus(ing.Status)
}

providerSpecific, setIdentifier := getProviderSpecificAnnotations(ing.Annotations)
providerSpecific, setIdentifier := getProviderSpecificAnnotations(annotationsWithDefaults(ing.Annotations, sc.defaultAnnotations))

for _, rule := range ing.Spec.Rules {
if rule.Host == "" {
Expand Down
5 changes: 4 additions & 1 deletion source/ingress_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ func (suite *IngressSuite) SetupTest() {
"{{.Name}}",
false,
false,
DefaultAnnotations(false),
)
suite.NoError(err, "should initialize ingress source")

Expand Down Expand Up @@ -134,6 +135,7 @@ func TestNewIngressSource(t *testing.T) {
ti.fqdnTemplate,
ti.combineFQDNAndAnnotation,
false,
DefaultAnnotations(false),
)
if ti.expectError {
assert.Error(t, err)
Expand Down Expand Up @@ -221,7 +223,7 @@ func testEndpointsFromIngress(t *testing.T) {
} {
t.Run(ti.title, func(t *testing.T) {
realIngress := ti.ingress.Ingress()
validateEndpoints(t, endpointsFromIngress(realIngress, false), ti.expected)
validateEndpoints(t, (&ingressSource{}).endpointsFromIngress(realIngress, false), ti.expected)
})
}
}
Expand Down Expand Up @@ -1008,6 +1010,7 @@ func testIngressEndpoints(t *testing.T) {
ti.fqdnTemplate,
ti.combineFQDNAndAnnotation,
ti.ignoreHostnameAnnotation,
DefaultAnnotations(false),
)
for _, ingress := range ingresses {
_, err := fakeClient.Extensions().Ingresses(ingress.Namespace).Create(ingress)
Expand Down
7 changes: 5 additions & 2 deletions source/ingressroute.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ type ingressRouteSource struct {
combineFQDNAnnotation bool
ignoreHostnameAnnotation bool
ingressRouteInformer extinformers.IngressRouteInformer
defaultAnnotations map[string]string
}

// NewContourIngressRouteSource creates a new contourIngressRouteSource with the given config.
Expand All @@ -64,6 +65,7 @@ func NewContourIngressRouteSource(
fqdnTemplate string,
combineFqdnAnnotation bool,
ignoreHostnameAnnotation bool,
defaultAnnotations map[string]string,
) (Source, error) {
var (
tmpl *template.Template
Expand Down Expand Up @@ -120,6 +122,7 @@ func NewContourIngressRouteSource(
combineFQDNAnnotation: combineFqdnAnnotation,
ignoreHostnameAnnotation: ignoreHostnameAnnotation,
ingressRouteInformer: ingressRouteInformer,
defaultAnnotations: defaultAnnotations,
}, nil
}

Expand Down Expand Up @@ -210,7 +213,7 @@ func (sc *ingressRouteSource) endpointsFromTemplate(ingressRoute *contourapi.Ing
}
}

providerSpecific, setIdentifier := getProviderSpecificAnnotations(ingressRoute.Annotations)
providerSpecific, setIdentifier := getProviderSpecificAnnotations(annotationsWithDefaults(ingressRoute.Annotations, sc.defaultAnnotations))

var endpoints []*endpoint.Endpoint
// splits the FQDN template and removes the trailing periods
Expand Down Expand Up @@ -303,7 +306,7 @@ func (sc *ingressRouteSource) endpointsFromIngressRoute(ingressRoute *contourapi
}
}

providerSpecific, setIdentifier := getProviderSpecificAnnotations(ingressRoute.Annotations)
providerSpecific, setIdentifier := getProviderSpecificAnnotations(annotationsWithDefaults(ingressRoute.Annotations, sc.defaultAnnotations))

if virtualHost := ingressRoute.Spec.VirtualHost; virtualHost != nil {
if fqdn := virtualHost.Fqdn; fqdn != "" {
Expand Down
Loading

0 comments on commit 3448610

Please sign in to comment.