allow to associate aws dns records with health checks #1288

rajatjindal · 2019-11-21T08:05:04Z

This PR allows to associate the DNS records with health checks. This helps in auto fail over in case one of the record is having issues.

rajatjindal · 2019-11-25T16:37:06Z

requesting feedback from @njuettner

rajatjindal · 2019-12-05T15:39:23Z

gentle reminder @njuettner

marcostvz · 2019-12-18T16:56:05Z

I just tested this PR in my environment, and it worked fine, using the following annotation:

external-dns.alpha.kubernetes.io/aws-health-check-id: {my-health-check-identifier}

rajatjindal · 2020-01-14T17:20:27Z

kind reminder @njuettner

rajatjindal · 2020-01-18T04:54:32Z

Requesting review from @linki @njuettner

alfredkrohmer · 2020-02-03T09:35:17Z

provider/aws.go

@@ -317,6 +318,10 @@ func (p *AWSProvider) records(zones map[string]*route53.HostedZone) ([]*endpoint
 					default:
 						// one of the above needs to be set, otherwise SetIdentifier doesn't make sense
 					}
+
+					if r.HealthCheckId != nil {


This shouldn't depend on a SetIdentifier being set. Can you move that one level out?

good catch, @rajatjindal can you move it out of if r.SetIdentifier != nil ...? this shouldn't be in the clause.

got it. I will make this change

I am still testing it, in the meantime, can u confirm if we should check for aliasrecord? as this option is available only if we set aliasrecord to true?

requesting feedback from @devkid

Not sure what you mean, a health check works with any target, not only alias records.

I think @rajatjindal code is correct. Associating a custom health check to a record set in Route53 is only available to records with routing policies other than 'Simple'. Therefore, HealthCheckId should only be considered when SetIdentifier is also set. @njuettner could you please confirm?

njuettner

One comment from @devkid and me, PTAL. Other than that LGTM 👍

carlosrodf · 2020-05-25T19:41:03Z

@njuettner is there a plan to merge this?
i was looking for this feature

rajatjindal · 2020-06-21T15:59:26Z

Hi @devkid @njuettner

sorry for picking up this so late again.

I saw the inconsistency on health-check inside setIdentifier condition, and other outside that condition. thanks for pointing that out.

I think adding the health check annotation only works when the routing policy != "Simple", and also that would mean that setIdentifier should be set.

could you please confirm what u think about it.

many thanks

barucoh · 2020-08-06T13:31:20Z

Is there any plan to merge this? I am looking for this feature...

seanmalloy · 2020-08-19T05:05:03Z

/kind feature

njuettner · 2020-08-19T08:14:39Z

@rajatjindal to answer your question, from looking at the docs, healthcheckid seems not related if you set the identifier or not. https://docs.aws.amazon.com/Route53/latest/APIReference/API_ResourceRecordSet.html#Route53-Type-ResourceRecordSet-HealthCheckId

But it would make sense to check what ResourceRecordSet we have 'Alias' or 'Non-Alias' and checking if you have more than two or more resource record sets to respond to a DNS query.

And if I got this right you also need to have a health check in place which you have to create before right? Otherwise it wouldn't make sense to reference the id of the health check.

I think this needs to be added for our docs otherwise people don't know what external-dns is doing and what not.

seanmalloy · 2020-08-21T19:31:41Z

@rajatjindal can you add some end user documentation that describes how this feature works? Also I think it would be good to rebase this because the newer GitHub actions CI checks have not run. Thanks!

rajatjindal · 2020-09-03T03:33:31Z

sorry for replying late, was busy with some other work.

i will be looking at this PR today

volk1234 · 2020-09-17T11:48:50Z

Long story short - when this will be landed?

vinny-sabatini · 2020-09-28T18:55:09Z

@rajatjindal do you have time to add some end user documentation for this PR?

@volk1234 we are just waiting for the PR to be rebased and to get some documentation added, then we will work to get the official maintainers to review and hopefully merge this shortly after that.

rajatjindal · 2020-09-29T09:44:23Z

apologies for delayed responses. i was dealing with some personal stuff. I will try to keep my replies current until this is merged.

jbilliau-rcd · 2020-10-02T17:27:28Z

Is this ready to be reviewed @rajatjindal ? If so, it looks like next step is to /assign @njuettner? We have a custom fork using this functionality and would love to come back to using the standard charts.

rajatjindal · 2020-10-03T10:33:44Z

yes, this is ready for review. Happy to address if there are more review comments.

jbilliau-rcd · 2020-10-05T13:42:56Z

/assign @njuettner

We've been running this in production for months now, works great.

Raffo · 2020-10-08T15:41:23Z

/lgtm
/approve

k8s-ci-robot · 2020-10-08T15:41:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Raffo, rajatjindal

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Raffo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 21, 2019

k8s-ci-robot requested review from linki and njuettner November 21, 2019 08:05

rajatjindal mentioned this pull request Dec 6, 2019

question: feedback on health check implementation for multi cluster traffic with AWS provider #1203

Closed

alfredkrohmer reviewed Feb 3, 2020

View reviewed changes

njuettner requested changes Feb 4, 2020

View reviewed changes

njuettner self-assigned this Feb 4, 2020

joberdick mentioned this pull request Feb 5, 2020

Route53 healthcheck make sense for this project? zalando-incubator/kube-ingress-aws-controller#307

Open

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2020

rajatjindal force-pushed the health-check-support branch from 4aa3c4f to dc8a0a1 Compare June 21, 2020 15:45

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 21, 2020

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 19, 2020

rajatjindal added 3 commits September 29, 2020 15:11

allow to associate dns records with health checks

32b7452

allow setting healthCheckId when identifier not provided

72544c3

get healthcheckid whether set-identifier is set or not

625eaf4

add documentation for aws health check id

a3dc3e8

rajatjindal force-pushed the health-check-support branch from 21752a8 to a3dc3e8 Compare September 29, 2020 09:41

rajatjindal requested a review from njuettner September 29, 2020 09:43

k8s-ci-robot assigned Raffo Oct 8, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 8, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 8, 2020

k8s-ci-robot merged commit aea5b76 into kubernetes-sigs:master Oct 8, 2020

This was referenced Dec 16, 2020

fix doc: Correct document for aws.md #1897

Closed

fix doc: Correct document for aws.md about route53 health check association #1898

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow to associate aws dns records with health checks #1288

allow to associate aws dns records with health checks #1288

rajatjindal commented Nov 21, 2019

rajatjindal commented Nov 25, 2019

rajatjindal commented Dec 5, 2019

marcostvz commented Dec 18, 2019 •

edited

Loading

rajatjindal commented Jan 14, 2020

rajatjindal commented Jan 18, 2020

alfredkrohmer Feb 3, 2020

njuettner Feb 4, 2020

rajatjindal Feb 5, 2020

rajatjindal Feb 5, 2020

rajatjindal Feb 6, 2020

alfredkrohmer Feb 8, 2020

offzale Apr 2, 2020

njuettner left a comment

carlosrodf commented May 25, 2020

rajatjindal commented Jun 21, 2020

barucoh commented Aug 6, 2020

seanmalloy commented Aug 19, 2020

njuettner commented Aug 19, 2020 •

edited

Loading

seanmalloy commented Aug 21, 2020

rajatjindal commented Sep 3, 2020

volk1234 commented Sep 17, 2020

vinny-sabatini commented Sep 28, 2020

rajatjindal commented Sep 29, 2020

jbilliau-rcd commented Oct 2, 2020

rajatjindal commented Oct 3, 2020

jbilliau-rcd commented Oct 5, 2020

Raffo commented Oct 8, 2020

k8s-ci-robot commented Oct 8, 2020

allow to associate aws dns records with health checks #1288

allow to associate aws dns records with health checks #1288

Conversation

rajatjindal commented Nov 21, 2019

rajatjindal commented Nov 25, 2019

rajatjindal commented Dec 5, 2019

marcostvz commented Dec 18, 2019 • edited Loading

rajatjindal commented Jan 14, 2020

rajatjindal commented Jan 18, 2020

alfredkrohmer Feb 3, 2020

Choose a reason for hiding this comment

njuettner Feb 4, 2020

Choose a reason for hiding this comment

rajatjindal Feb 5, 2020

Choose a reason for hiding this comment

rajatjindal Feb 5, 2020

Choose a reason for hiding this comment

rajatjindal Feb 6, 2020

Choose a reason for hiding this comment

alfredkrohmer Feb 8, 2020

Choose a reason for hiding this comment

offzale Apr 2, 2020

Choose a reason for hiding this comment

njuettner left a comment

Choose a reason for hiding this comment

carlosrodf commented May 25, 2020

rajatjindal commented Jun 21, 2020

barucoh commented Aug 6, 2020

seanmalloy commented Aug 19, 2020

njuettner commented Aug 19, 2020 • edited Loading

seanmalloy commented Aug 21, 2020

rajatjindal commented Sep 3, 2020

volk1234 commented Sep 17, 2020

vinny-sabatini commented Sep 28, 2020

rajatjindal commented Sep 29, 2020

jbilliau-rcd commented Oct 2, 2020

rajatjindal commented Oct 3, 2020

jbilliau-rcd commented Oct 5, 2020

Raffo commented Oct 8, 2020

k8s-ci-robot commented Oct 8, 2020

marcostvz commented Dec 18, 2019 •

edited

Loading

njuettner commented Aug 19, 2020 •

edited

Loading