-
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
use port.Name
to populate srv records
#4001
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
Welcome @kerbaras! |
Hi @kerbaras. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
Mmmh, it's interesting but I'm not sure it makes sense 🤔 . Using multiple ports on the same service, on a => My understanding is that your issue should not be solved like this. It should be solved with another service on metrics. Let me know if I missed something. |
This is mainly to support different nodes with the same port mapped. Not for mapping multiple ports. If we follow the example linked we can see the following:
where there are 4 services exported the same sip port. In this case, the port would be 5060 and the 4 nodes here we have more than 1 port exported in a With the proposed example both cases are being supported. Multiple matches under To me it seems like an obscure case to have the following:
as But maybe i understood you wrongly |
Ok. I see your point. The record format specify that the first field is supposed to be the service and in Kubernetes, the port name can be a ref to the service name. I am like you on this:
It's a breaking change, but why not ? @Raffo @szuecs @johngmyers Wdyt about it ? @kerbaras This PR will need tests and documentation update to be reviewed. Wdyt about adding a CLI arg to enable this behavior ? This would avoid this feature to be a breaking change. |
Sounds good. I'll get to it as soon as I get some free time :) |
We need a test that proofs that it works or we will break it in the future. |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages 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 PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
Description
It is in the best intention of the user to name their services with a self-explanatory identifier and not with the canonical protocol specification.
Let's image we use the bitnami mongodb-sharded chart to create a database that we'll expose using srv records. The chart will create a service along the lines of:
Not only the mongodb client will expect the following srv to connect
_mongodb._tcp.orders.acme.com
when the actual dns record that will be created is_orders-db._tcp.orders.acme.com
but also this srv record will have 2 values:It will be much better to get the fqdn using the port's name to construct the following 2 records:
Fixes #4000
NOTE: As this is my first pr here and I don't have that much experience with go. i'll wait for a reply before changing the unit tests and run the whole suite
Checklist