-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Generate SRV records from Services to respect K8s DNS spec #1330
Conversation
Signed-off-by: Weihang Lo <[email protected]>
Welcome @weihanglo! |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: weihanglo The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/assign @njuettner |
@Raffo Sorry for bothering you. Just want to make sure this PR is queued for reviews. Thanks. |
@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 |
Hi, sorry for ping you again. Would this PR be reviewed after my resolving conflicts? |
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 <[email protected]>
@Raffo |
/kind feature |
@seanmalloy |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
@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. |
/remove-lifecycle stale |
One thing I noticed with However, 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 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. |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-contributor-experience at kubernetes/community. |
@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! |
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 😃 |
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-contributor-experience at kubernetes/community. |
/remove-lifecycle rotten |
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:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
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:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
Thanks for all your comments so far! |
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:
Below are type specific rules:
NodePort
/LoadBalancer
Service.spec.nodePort
as the port on the target host.ClusterIP
with a validClusterIP
Service.spec.port
as the port on the target host.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 😂).
plan/plan.go#filterRecordsForPlan
to filter in SRV records.plan/plan_test.go
for SRV records.400 Bad Request
if target hostname in SRV record is not FQDN.ensureTrailingDot
on provider-sides. Supposed it's more appropriate to do that at the end ofEndpoints()
function for each source.References