Ingress target annotation should set, not append #318

claytono · 2017-08-11T21:57:30Z

If the user has specified a target for the ingress, treat that as
overriding any endpoints already set on the ingress, even if that list
is not empty. This allows overriding the IP address or hostname set
when using a service like kube-keepalived-vip.

This is related to #312

claytono · 2017-08-11T21:58:22Z

/assign hjacobs

hjacobs · 2017-08-12T13:03:20Z

@ClaytonONeill please also change the logic when FQDN template is used (endpointsFromTemplate) to make it consistent. Otherwise LGTM.

claytono · 2017-08-12T17:40:01Z

@hjacobs Thanks for catching that. Should be fixed in both places now. Not sure how I overlooked that.

hjacobs · 2017-08-14T14:55:17Z

@ideahitme could you have a quick look (also to get familiar with this feature). LGTM, but I already overlooked something the last time 😏

ideahitme

Some minor improvements can be done here

ideahitme · 2017-08-15T08:37:35Z

source/ingress.go

@@ -152,7 +160,11 @@ func endpointsFromIngress(ing *v1beta1.Ingress) []*endpoint.Endpoint {
 			continue
 		}

-		endpoints = addEndpointsFromTargetAnnotation(ing, rule.Host, endpoints)
+		endpoints = getEndpointsFromTargetAnnotation(ing, rule.Host)


I am not entirely sure about this, so this basically ignores all but lastrules.host and generate endpoint from it. Why not generate an endpoint for each of the hosts found ?

IMHO getEndpointsFromTargetAnnotation should be called from Endpoint() method above:

Try to generate endpoint from target annotation key, if none:

Try to generate endpoint from ingress status field, if none:

Try to generate endpoint from template

@ideahitme My thought was to move it to be called from Endpoint also, but then that leads to duplication of the code that loops over the rules gathering hosts.

As far as the call to it in the endpointsFromTemplate function, I'm not sure. If the consensus is to change that I'll be glad to do it, but I'd rather not have this fix get bogged down on something unrelated.

@ClaytonONeill it is okay, it was just a suggestion on to improve overall code structure - which can be done in subsequent PR. Otherwise this pull request looks okay to me. If no objections I will have this merged /cc @linki

@ClaytonONeill and also my first comment above (regarding ignoring all but last rule.host values), is that intentional ?

No, I think that's accidental. I'll take a look at fixing that and get this updated if I'm correct.

ideahitme · 2017-08-15T08:40:21Z

source/ingress.go

@@ -129,7 +133,11 @@ func (sc *ingressSource) endpointsFromTemplate(ing *v1beta1.Ingress) ([]*endpoin

 	hostname := buf.String()

-	endpoints = addEndpointsFromTargetAnnotation(ing, hostname, endpoints)
+	endpoints = getEndpointsFromTargetAnnotation(ing, hostname)


I think this does not belong here, endpointsFromTemplate is called only as a last resort to generate an endpoint, i.e. there is no target annotation

claytono · 2017-08-16T02:42:14Z

@ideahitme I think the latest update should fix the issue you identified.

ideahitme · 2017-08-16T09:03:47Z

@ClaytonONeill please update the changelog and good to be merged from my side

linki · 2017-08-16T15:49:39Z

@ideahitme @ClaytonONeill lgtm

If the user has specified a target for the ingress, treat that as overriding any endpoints already set on the ingress, even if that list is not empty. This allows overriding the IP address or hostname set when using a service like kube-keepalived-vip.

claytono · 2017-08-17T13:03:30Z

@ideahitme I wasn't 100% sure about the changelog, so I updated it as if it were a new release. If you want something different let me know and I can get it updated to match.

hjacobs · 2017-08-17T20:50:46Z

Released: https://github.com/kubernetes-incubator/external-dns/releases/tag/v0.4.4 🎉

james-masson · 2017-09-25T10:36:49Z

This seems to be non-functional on Google with v0.4.4

time="2017-09-25T10:28:20Z" level=debug msg="Endpoints generated from ingress: shared-services/nginx: [testconsul.<domain> -> helm-dev.<domain> (type "")]"
time="2017-09-25T10:28:20Z" level=info msg="Add records: testconsul.<domain>. CNAME [helm-dev.<domain>.]"
time="2017-09-25T10:28:20Z" level=info msg="Add records: testconsul.<domain>. TXT ["heritage=external-dns,external-dns/owner=helm-dev"]"
time="2017-09-25T10:28:20Z" level=error msg="googleapi: Error 400: The resource record set 'entity.change.additions[0]' is invalid because the DNS name 'testconsul.<domain>.' may have either one CNAME resource record set or resource record sets of other types, but not both.
More details:
Reason: cnameResourceRecordSetConflict, Message: The resource record set 'entity.change.additions[0]' is invalid because the DNS name 'testconsul.<domain>.' may have either one CNAME resource record set or resource record sets of other types, but not both.
Reason: cnameResourceRecordSetConflict, Message: The resource record set 'entity.change.additions[1]' is invalid because the DNS name 'testconsul.<domain>.' may have either one CNAME resource record set or resource record sets of other types, but not both.
"

claytono · 2017-09-28T19:36:47Z

@james-masson Does it work normally without target annotations? I saw the same problem on Route53 w/AWS when using a hostname for the target annotation, but ended up switching back to IP based target annotations for other reasons. I assumed this wasn't a target annotation specific issue at that time.

james-masson · 2017-09-29T10:01:48Z

Hmm. I think I put the wrong log in the issue - or perhaps the right log in the wrong issue.

I fixed that error with "txt-prefix" - as it's a CNAME/TXT conflict - nothing to do with this PR.

Apologies for the noise...

If the user has specified a target for the ingress, treat that as overriding any endpoints already set on the ingress, even if that list is not empty. This allows overriding the IP address or hostname set when using a service like kube-keepalived-vip.

related to kubernetes-sigs#292

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 11, 2017

k8s-ci-robot assigned hjacobs Aug 11, 2017

claytono force-pushed the fix-ingress-annotation-appending branch from bfd2e71 to ee783d6 Compare August 11, 2017 22:04

claytono force-pushed the fix-ingress-annotation-appending branch from ee783d6 to 29d6f86 Compare August 12, 2017 17:22

ideahitme self-requested a review August 13, 2017 08:16

ideahitme reviewed Aug 15, 2017

View reviewed changes

claytono force-pushed the fix-ingress-annotation-appending branch from 29d6f86 to 0ce65b1 Compare August 16, 2017 02:40

claytono force-pushed the fix-ingress-annotation-appending branch from 0ce65b1 to a934bcd Compare August 17, 2017 13:02

hjacobs merged commit ea4cbfe into kubernetes-sigs:master Aug 17, 2017

hjacobs mentioned this pull request Aug 17, 2017

ClusterIp Service support #278

Merged

lou-lan pushed a commit to lou-lan/external-dns that referenced this pull request May 11, 2022

lint: enable deadcode and varcheck (kubernetes-sigs#318)

74a3885

related to kubernetes-sigs#292

Max-Chandler mentioned this pull request Jun 20, 2023

external-dns not ignoring pre-existing DNS records it does not have ownership of #3715

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ingress target annotation should set, not append #318

Ingress target annotation should set, not append #318

claytono commented Aug 11, 2017 •

edited

Loading

claytono commented Aug 11, 2017

hjacobs commented Aug 12, 2017 •

edited

Loading

claytono commented Aug 12, 2017

hjacobs commented Aug 14, 2017

ideahitme left a comment

ideahitme Aug 15, 2017

ideahitme Aug 15, 2017

claytono Aug 15, 2017

ideahitme Aug 15, 2017

ideahitme Aug 15, 2017 •

edited

Loading

claytono Aug 16, 2017

ideahitme Aug 15, 2017

claytono commented Aug 16, 2017

ideahitme commented Aug 16, 2017

linki commented Aug 16, 2017

claytono commented Aug 17, 2017

hjacobs commented Aug 17, 2017

james-masson commented Sep 25, 2017

claytono commented Sep 28, 2017

james-masson commented Sep 29, 2017

Ingress target annotation should set, not append #318

Ingress target annotation should set, not append #318

Conversation

claytono commented Aug 11, 2017 • edited Loading

claytono commented Aug 11, 2017

hjacobs commented Aug 12, 2017 • edited Loading

claytono commented Aug 12, 2017

hjacobs commented Aug 14, 2017

ideahitme left a comment

Choose a reason for hiding this comment

ideahitme Aug 15, 2017

Choose a reason for hiding this comment

ideahitme Aug 15, 2017

Choose a reason for hiding this comment

claytono Aug 15, 2017

Choose a reason for hiding this comment

ideahitme Aug 15, 2017

Choose a reason for hiding this comment

ideahitme Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

claytono Aug 16, 2017

Choose a reason for hiding this comment

ideahitme Aug 15, 2017

Choose a reason for hiding this comment

claytono commented Aug 16, 2017

ideahitme commented Aug 16, 2017

linki commented Aug 16, 2017

claytono commented Aug 17, 2017

hjacobs commented Aug 17, 2017

james-masson commented Sep 25, 2017

claytono commented Sep 28, 2017

james-masson commented Sep 29, 2017

claytono commented Aug 11, 2017 •

edited

Loading

hjacobs commented Aug 12, 2017 •

edited

Loading

ideahitme Aug 15, 2017 •

edited

Loading