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

Support A record for multile IPs for a headless services. #645

Merged
merged 1 commit into from
Jan 23, 2019

Conversation

toshipp
Copy link
Contributor

@toshipp toshipp commented Jul 25, 2018

Non statefulset pods associating to a headless service have different
IPs, but have a same hostname. In this case, external-dns registered
only one A records due to attempting to register multiple A records for
a same hostname for each IP.
This patch now registers one A record having multiple IPs.

fixes #549

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
  • If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 25, 2018
@alfredkrohmer
Copy link
Contributor

I see that this is fixing a bug, but in general, shouldn't it be a configurable option if you want
a) one A record that has all pod IPs or
b) one A record per pod (where the record contains the pod name)
?

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 25, 2018
@toshipp
Copy link
Contributor Author

toshipp commented Jul 26, 2018

In AWS, the case b is not possible.
For example, A record hostname foo associating to 1.1.1.1 is registered, then try to register A record same hostname foo associating to 1.1.1.2, the second attempt is failed because hostname foo is duplicated.
It seems a restriction of route53.

@alfredkrohmer
Copy link
Contributor

I meant for case b) that the pod name (which has usually either a sequential number in it for static sets or a random string for replica sets) could be part of the hostname. E.g. a pod is named my-pod-2uzxy2-d826sh9a, so it would create a DNS record my-pod-2uzxy2-d826sh9a.example.com.

If I understand you pull request correctly, it will completely remove the ability to do that.

@alfredkrohmer
Copy link
Contributor

More specifically I'm talking about the host port use case:
https://github.com/Arttii/external-dns/blob/master/docs/tutorials/hostport.md

Nevertheless, even the usecase you mentioned is possible. You can simply created weighted or multi value records in Route53 which would allow to have multiple instances of the same record name (e.g. my-service.example.com) with a different IP each. (Multi value would return up to 8 IPs per record name, weighted would return one IP via weighted round robin.)

@toshipp
Copy link
Contributor Author

toshipp commented Jul 27, 2018

Ok, I got it.
The PR does only aggregate target IPs associating SAME hostname to one record.
So, if pods have unique hostname, external-dns registers records for each hostname.
This does not breaks current behavior.

In replica set case, pod names are unique, but the hostnames are same because it is given by the
pod resource definition.
see https://kubernetes.io/docs/concepts/services-networking/dns-pod-service/#pods

Currently when a pod is created, its hostname is the Pod’s metadata.name value.

The Pod spec has an optional hostname field, which can be used to specify the Pod’s hostname. When specified, it takes precedence over the Pod’s name to be the hostname of the pod. For example, given a Pod with hostname set to “my-host”, the Pod will have its hostname set to “my-host”.

My motivation is exactly creating multi value records for headless service with replica set.
Current implementation registers only one IP for the service due to a bug.
The PR registers all IPs for the service.

Non statefulset pods associating to a headless service have different
IPs, but have a same hostname. In this case, external-dns registered
only one A record due to attempting to register multiple A records for
a same hostname for each IP.
This patch now registers one A record having multiple IPs.
@toshipp toshipp force-pushed the support-multiple-ips branch from cbf2bbe to 44f319e Compare July 27, 2018 10:21
@dene14
Copy link

dene14 commented Oct 4, 2018

Any chance to get that merged?

@artsiukhou
Copy link

Struggling with the same problem, it will be extremely helpful to have a fix for the issue.

@alfredkrohmer
Copy link
Contributor

This would be really helpful, indeed. Can we have that merged please?

@xdrus
Copy link

xdrus commented Jan 17, 2019

Thanks, this PR works very well in our env. Our use case is that we use AWS CNI plugin that gives real VPC IPs to pods we want to expose via R53.

Look forward to having this merged!

@Raffo
Copy link
Contributor

Raffo commented Jan 18, 2019

@njuettner @linki let's review this one, do you have capacity? It looks like it was open for a long time.

@njuettner
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 23, 2019
@njuettner
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: njuettner

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:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AWS A Records incorrectly generated for multiple endpoints
8 participants