Generate SRV records from Services to respect K8s DNS spec #1330

weihanglo · 2019-12-24T05:01:25Z

What

Generates SRV records from services to respect Kubernetes DNS specification.

Thanks for #559, ExternalDNS already got a basic implementation of generating SRV records from NodePort serivces. However, it is still far away from Kubernetes DNS specification. This pull request tries to implement full part of the spec concerning SRV records.

How

Simply follow the spec. Here are two common rules for all service types:

Unnamed ports are ignored.
Each name port, generate one corresponding SRV record.

Below are type specific rules:

NodePort/LoadBalancer
- Use Service.spec.nodePort as the port on the target host.
ClusterIP with a valid ClusterIP
- Use Service.spec.port as the port on the target host.
Headless Service
- Headless Service itself does not generate SRV records.
- Each endpoint of headless Service, particularly a Pod, generates its own SRV record. That is to say, if we get M named ports with N selected Pods, there would be M x N SRV records.
- The domain name of each target host would be the corresponding endpoint of querying A record for the headless Service.

Future works

If all changes are valid, I would get more tasks done in the following PRs (or push more commits if we are comfortable with PR bloating 😂).

Update plan/plan.go#filterRecordsForPlan to filter in SRV records.
- ⚠️ [Discussion needed] Shall we enable SRV records generating by default, or introduce another config/flag storing a whitelist of record types?
Add more tests for plan/plan_test.go for SRV records.
Update document of headless Service for generating SRV records.
Make sure each provider can generate correct SRV records.
- A known issue is that on Google Cloud DNS, it would get 400 Bad Request if target hostname in SRV record is not FQDN.
- ⚠️ [Discussion needed] I've already write a workaround to ensure FQDN of supporting record types with a target hostname. However, current logic is that we ensureTrailingDot on provider-sides. Supposed it's more appropriate to do that at the end of Endpoints() function for each source.

References

Signed-off-by: Weihang Lo <me@weihanglo.tw>

k8s-ci-robot · 2019-12-24T05:01:33Z

Welcome @weihanglo!

It looks like this is your first PR to kubernetes-sigs/external-dns 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/external-dns has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2019-12-24T05:01:46Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: weihanglo
To complete the pull request process, please assign njuettner
You can assign the PR to them by writing /assign @njuettner in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

weihanglo · 2019-12-27T16:13:57Z

/assign @njuettner

weihanglo · 2020-01-04T06:27:59Z

@Raffo Sorry for bothering you. Just want to make sure this PR is queued for reviews. Thanks.

JoaoBraveCoding · 2020-01-07T13:56:05Z

@weihanglo @Raffo I'm also interested in this pull request since I was about to open a pull request to add support for SRV records with provider rfc2136 and source CRD.

weihanglo · 2020-03-02T12:43:19Z

Hi, sorry for ping you again. Would this PR be reviewed after my resolving conflicts?
@njuettner @linki

weihanglo · 2020-04-03T12:27:15Z

@Raffo @hjacobs would you have time to take a look on this? Let me know if this PR should go on or not. Thanks!

Raffo · 2020-07-08T08:14:04Z

Sorry @weihanglo we totally missed that, apologies. Can you please resolve the conflict so that I can proceed to a review?

weihanglo · 2020-07-08T08:40:56Z

Sorry @weihanglo we totally missed that, apologies. Can you please resolve the conflict so that I can proceed to a review?

@Raffo Ok. I'll resolve this ASAP.

Signed-off-by: Weihang Lo <me@weihanglo.tw>

weihanglo · 2020-07-08T13:25:43Z

@Raffo
Updated. Please take a look. Thanks 😀

seanmalloy · 2020-08-19T05:02:07Z

/kind feature

weihanglo · 2020-08-20T04:49:14Z

@seanmalloy
Thanks for adding the label. This PR has been updated following current master.

fejta-bot · 2020-11-18T04:56:29Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

k8s-ci-robot · 2020-11-18T04:56:36Z

@weihanglo: PR needs rebase.

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.

seanmalloy · 2020-11-18T05:02:02Z

/remove-lifecycle stale

arianvp · 2020-11-26T11:00:18Z

One thing I noticed with external-dns that it does differently from the kubernetes internal DNS is that when you have headless service + statefulset in k8s then if a Pod goes UnReady it is ONLY removed from the SRV record; but the A record for the pod remains. This is important as the A record is the stable identity used in .e.g distributed databases.

However, external-dns removes my A record my-pod-0.myservice.mydomain.example immediately as soon as pod-0 goes in an NotReady which is very different behavior. Also very unwanted because the reason why databases go unhealthy is for example because they can't reach their peers. If that then causes their stable identity to disappear then the problem only worsens further.

When you gracefully terminate a pod in a distributed database; you don't want its identity to disappear before the pod is gone. So the A-record should stick around until the pod is properly gone instead of just NotReady

It wasn't very clear to me from this PR if this PR addresses this issue as well. But it would be great if it did.

vanekjar · 2021-01-09T07:13:45Z

source/service.go

+		if port.Name == "" {
+			// Unnamed ports do not have a SRV record.
+			continue
+		}
+
+		var exposedPort int32
+		switch svc.Spec.Type {
+		case v1.ServiceTypeLoadBalancer:
+			fallthrough
+		case v1.ServiceTypeNodePort:
+			exposedPort = port.NodePort


I am afraid this won't work for NodePort type. K8s uses unnamed ports when exposing NodePort services.

If you set the type field to NodePort, the Kubernetes control plane allocates a port from a range specified by --service-node-port-range flag (default: 30000-32767).

https://kubernetes.io/docs/concepts/services-networking/service/#nodeport

fejta-bot · 2021-04-09T07:29:17Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

seanmalloy · 2021-04-09T17:00:28Z

@weihanglo I know this PR has been open for a while now, but are you still interested in getting this merged? If yes then could you please resolve the conflicts?

We can remove the stale label too.

Thanks!

weihanglo · 2021-04-09T23:49:24Z

I would love to resolve the conflict if it has a chance getting merged. If maintainers think this issue looks reasonable to merge please tell me. I am open to discussion 😃

fejta-bot · 2021-05-10T00:13:56Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten

mindw · 2021-05-10T19:29:12Z

/remove-lifecycle rotten

k8s-triage-robot · 2021-08-08T19:35:23Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

mindw · 2021-08-08T22:58:21Z

/remove-lifecycle stale

k8s-triage-robot · 2021-11-06T23:51:36Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

mindw · 2021-11-08T07:23:27Z

/remove-lifecycle stale

weihanglo · 2021-12-15T16:40:06Z

Thanks for all your comments so far!
I am going to close it since I don't use this feature anymore. If someone want to push it forward, feel free to take it.

weihanglo added 5 commits December 24, 2019 01:05

refactor(source/service): extract selectTargetedPods

04c86d8

refactor(source/service): remove unused arg

521bdff

Signed-off-by: Weihang Lo <me@weihanglo.tw>

test(source/service): pass ports explicitly

8ecb91b

test(source/serivce): named port for SRV records

6eea33a

feat(source/service): extract SRV records for all

e0dfb7e

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 24, 2019

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 24, 2019

k8s-ci-robot requested review from linki and njuettner December 24, 2019 05:01

k8s-ci-robot assigned njuettner Dec 27, 2019

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

Merge branch 'master' into feat/extract-srv-record

7ddcc2a

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

k8s-ci-robot assigned Raffo May 11, 2020

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

Merge branch 'master' into backup/extract-srv-record

7052c63

Signed-off-by: Weihang Lo <me@weihanglo.tw>

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

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 19, 2020

Merge branch 'master' into feat/extract-srv-record

7addc3d

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

k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 18, 2020

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 18, 2020

vanekjar reviewed Jan 9, 2021

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 9, 2021

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 10, 2021

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label May 10, 2021

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 8, 2021

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 8, 2021

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 6, 2021

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 8, 2021

weihanglo closed this Dec 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate SRV records from Services to respect K8s DNS spec #1330

Generate SRV records from Services to respect K8s DNS spec #1330

weihanglo commented Dec 24, 2019 •

edited

Loading

k8s-ci-robot commented Dec 24, 2019

k8s-ci-robot commented Dec 24, 2019

weihanglo commented Dec 27, 2019

weihanglo commented Jan 4, 2020

JoaoBraveCoding commented Jan 7, 2020

weihanglo commented Mar 2, 2020

weihanglo commented Apr 3, 2020

Raffo commented Jul 8, 2020

weihanglo commented Jul 8, 2020

weihanglo commented Jul 8, 2020

seanmalloy commented Aug 19, 2020

weihanglo commented Aug 20, 2020

fejta-bot commented Nov 18, 2020

k8s-ci-robot commented Nov 18, 2020

seanmalloy commented Nov 18, 2020

arianvp commented Nov 26, 2020

vanekjar Jan 9, 2021

fejta-bot commented Apr 9, 2021

seanmalloy commented Apr 9, 2021

weihanglo commented Apr 9, 2021

fejta-bot commented May 10, 2021

mindw commented May 10, 2021

k8s-triage-robot commented Aug 8, 2021

mindw commented Aug 8, 2021

k8s-triage-robot commented Nov 6, 2021

mindw commented Nov 8, 2021

weihanglo commented Dec 15, 2021

Generate SRV records from Services to respect K8s DNS spec #1330

Generate SRV records from Services to respect K8s DNS spec #1330

Conversation

weihanglo commented Dec 24, 2019 • edited Loading

What

How

Future works

References

k8s-ci-robot commented Dec 24, 2019

k8s-ci-robot commented Dec 24, 2019

weihanglo commented Dec 27, 2019

weihanglo commented Jan 4, 2020

JoaoBraveCoding commented Jan 7, 2020

weihanglo commented Mar 2, 2020

weihanglo commented Apr 3, 2020

Raffo commented Jul 8, 2020

weihanglo commented Jul 8, 2020

weihanglo commented Jul 8, 2020

seanmalloy commented Aug 19, 2020

weihanglo commented Aug 20, 2020

fejta-bot commented Nov 18, 2020

k8s-ci-robot commented Nov 18, 2020

seanmalloy commented Nov 18, 2020

arianvp commented Nov 26, 2020

vanekjar Jan 9, 2021

Choose a reason for hiding this comment

fejta-bot commented Apr 9, 2021

seanmalloy commented Apr 9, 2021

weihanglo commented Apr 9, 2021

fejta-bot commented May 10, 2021

mindw commented May 10, 2021

k8s-triage-robot commented Aug 8, 2021

mindw commented Aug 8, 2021

k8s-triage-robot commented Nov 6, 2021

mindw commented Nov 8, 2021

weihanglo commented Dec 15, 2021

weihanglo commented Dec 24, 2019 •

edited

Loading