-
Notifications
You must be signed in to change notification settings - Fork 39.3k
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
ServicePort -> DNS SRV does not allow services with same name but different protocol #97149
Comments
/sig network |
Creating a service with two ports that have the same name, but different protocol, fails: $ cat service-test.yaml apiVersion: v1 kind: Service metadata: name: service-test labels: app: service-test spec: selector: app: service-test clusterIP: None ports: - name: ldap protocol: TCP port: 389 - name: kerberos protocol: TCP port: 88 - name: kerberos protocol: UDP port: 88 $ oc replace -f service-test.yaml The Service "service-test" is invalid: spec.ports[2].name: Duplicate value: "kerberos" Some services operate over both TCP and UDP (e.g. DNS). Furthermore, some of these also require DNS SRV records for both _tcp and _udp with the same service name (e.g. Kerberos). The current ServicePort validation behaviour - `name` must be unique - does not admit this use case. Relax the uniqueness check, ensuring that name/protocol *pairs* are unique. Fixes: kubernetes#97149
This is working as expected, names for ports must be unique kubernetes/staging/src/k8s.io/api/core/v1/types.go Lines 4256 to 4264 in f2fb77a
what's the problem on using? apiVersion: v1
kind: Service
metadata:
name: service-test
labels:
app: service-test
spec:
selector:
app: service-test
clusterIP: None
ports:
- name: ldap
protocol: TCP
port: 389
- name: kerberos-tcp
protocol: TCP
port: 88
- name: kerberos-udp
protocol: UDP
port: 88
|
@aojea doing that means you can create the service object. But the resulting SRV records will be wrong. They need to be So yes, "names for ports must be unique" - in the current Kubernetes implementation. That behaviour is the bug - it obstructs this important use case. |
I think that there are solutions to this problem, but they have to be backwards compatible To be fair, this "technically" doesn´t obstruct Kerberos to work, AFAIK it only needs one protocol to work (not much into Kerberos so I can be wrong here) , it also seems that TCP is not enabled by default in some kerberos implementation https://web.mit.edu/kerberos/krb5-1.4/krb5-1.4.1/doc/krb5-admin/Hostnames-for-KDCs.html
However, I do see that this is a real use case. kubernetes/pkg/apis/core/types.go Lines 3734 to 3741 in 5996839
https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/1507-app-protocol IIUIC current DNS specifications use the port to create the srv record, https://github.com/kubernetes/dns/blob/master/docs/specification.md#232---srv-records can we use appProtocol instead, if it is defined, or both, or find a way to do it :) ? |
FreeIPA (our use case) requires both TCP and UDP for Kerberos. UDP is used by default for simple authentication requests. In some cases the payload does not fit into an UDP package. KRB5 automatically switches TCP. We cannot just use TCP-only. Some application assume UDP as primary protocol. TCP would also slow down Kerberos. We really need both.
Can you change the code to use a combination of port and protocol? Some like |
From my POV, though I have not tested it (I really have no idea how to build and test Kubernetes locally with my own changes), if you relax the uniqueness constraint from ServicePort "name" to the pair of "name" and "protocol", that might be enough. It would seem to be backwards compatible, up to any other part of Kubernetes, or third party programs, not handling the possibility of ServicePorts with duplicate names. Certainly there would be no change to the behaviour for any currently-accepted Service spec. If there are other parts of Kubernetes (including docs) that need changing to handle the possibility of duplicate Service name, they can be changed. But there might not even be much else that needs changing. EndpointPort handles duplicate names fine, as does the OpenShift cluster-dns-operator (or whatever is the component in OpenShift that turns Endpoints into SRV records). See https://frasertweedale.github.io/blog-redhat/posts/2020-12-08-k8s-srv-limitation.html#endpoints-do-not-have-the-limitation for proof. |
I´m just highlighting the fact that the API explicitly mentions that the name should be unique , let´s wait for @thockin , this requirement in the API comes from pre1.0 kubernetes versions, I think he can explain it better than anybody ...
right now the SRV record is built like what I´m suggesting is an additional record based on the new field appProtocol |
Based on what I have read about the purpose of |
For a for-now one-off workaround, I think you could use CoreDNS's rewrite plugin to add -tcp/-udp to the port name segment, based on the protocol segment. The rewrite plugin allows re-writing of incoming queries using regular expressions. The rewrite could look something like the following untested snip ...
which essentially would rewrite, for example ... the above rewrite snip would also need a response rewrite to make it compatible with clients that are sensitive to name mismatches. |
@chrisohaver you have it backwards. We require the service name in the SRV records to be Did you mean you could use a rewrite the remove the |
I think I got the direction correct...
|
The problem here is compatibility. I swear I opened this same issue years ago, but I can't find it. The way service ports are defined sort of predates the strategic merge capabilities. It ends up needing a compound key and we STILL have cases where various patch/merge cases do not work and can't really be fixed. To allow this would be incompatible with all existing client-side merging (including apply). I'm not HAPPY about this, but that's where it is. Here's the part where I make suggestions that I don't like. We could consider adding a way to override the SRV name. E.g. a net-new field like @prameshj because she always has more state on DNS than me. |
+1 Using the rewrite plugin workaround as @chrisohaver suggested As a longer term fix, introducing a new "ianaName" or "dnsName" in the ServicePort seems like the best solution, imo. |
OK, I suppose my next step is to write a KEP to add a new ServicePort field then? |
I don't LIKE that idea, so I am eager to hear alternatives. |
Can we take the conversation to #47249 to keep it all together? |
@thockin I suggested one
why current field kubernetes/staging/src/k8s.io/api/core/v1/types.go Lines 4239 to 4248 in 94b1788
kerberos is already in the IANA |
Is this suggesting reuse of appProtocol to include values that need not be valid protocols? In this specific example, the appProtocol value will be"kerberos", which is a valid protocol as you linked. But will it always be the case? Can someone not have "mytestport_tcp.mytestsvc.default.svc.cluster.local"? Sorry for commenting on the closed issue. I am happy to re-comment in the other issue if we want to add this proposal there.
|
indeed, the suggestion is to use the appProtocol field in Service Spec to generate a DNS SRV record based on that field, because the field description sounds correct to me.
it will have
I think that the other issue is related, or this is a consequence of it, but this discuss the SRV record port thing, the other the service port name uniqeness |
if they wanted to reuse the name "mytestport" for both tcp and udp protocol.. the current implementation won't let them do it. I realise this is not a concrete usecase, but if we are making an API change to support it, we should try to support all cases.
|
sorry, I realise my comment wasn't clear, if they want to reuse the name on both ports, can they do it with the AppProtocol field without any additional API change? the " what happens if the user sets a non-service name in the AppProtocol field", is something possible today, if a user want to general DNS SRV records with a non-service name, they can do it today with the Port Name field |
The Kubernetes project currently lacks enough active 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. /close |
@k8s-triage-robot: Closing this issue. In response to this:
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. |
/reopen Resurrecting this issue as I (yes after a long time) submitted a KEP: kubernetes/enhancements#3242 |
@frasertweedale: Reopened this issue. In response to this:
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. |
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 |
I was scrubbing issues assigned and I realize I lost track of this one. The KEP aged-out. I apologize. Do you want to revive it? A few thoughts as I paged it back in, some of which contradict past-me. 1
While it WOULD be a break, we could consider it. Why the change in heart here? Well, I was wrong - the merge keys for e.g.
There's still a risk that some other entity is relying on name uniqueness, so I am not convinced this path is best, but it would be nicest. 2
I could maybe get behind a model where we said: if
We would get DNS records:
or even just:
3I could also maybe see formalizing the rewrite proposed above. E.g. If you specify your The first idea seems scary, but the last 2 ideas both seem simpler than adding a new field, no? |
you can start with 2 right now creating your own CoreDNS plugin ... .... and if there is traction it can be formalized |
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 |
This issue has not been updated in over 1 year, and should be re-triaged. You can:
For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/ /remove-triage accepted |
The Kubernetes project currently lacks enough contributors to adequately respond to all issues. This bot triages un-triaged issues according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues. This bot triages un-triaged issues according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
What happened:
Creating a service with two ports that have the same name, but different protocol, fails:
This is an important use case for some applications, and the limitation is surprising. More information in my blog post: https://frasertweedale.github.io/blog-redhat/posts/2020-12-08-k8s-srv-limitation.html#kubernetes-srv-limitation
What you expected to happen:
Such Service objects should be accepted, and should result in the creation of the corresponding DNS SRV records that have the same service name, but different protocol ID, e.g., from the object above:
How to reproduce it (as minimally and precisely as possible):f
As shown above.
Anything else we need to know?:
I'm happy to file a KEP if it is deemed necessary. However, all that is required conceptually is to relax the uniqueness check to the name/protocol pairs instead of service name only. I hope it could be addressed without adding new fields, i.e. aligning the ServicePort semantics with the established semantics of SRV records.
Environment:
kubectl version
):Cloud provider or hardware configuration:
OS (e.g:
cat /etc/os-release
):Kernel (e.g.
uname -a
):Install tools:
Network plugin and version (if this is a network-related bug):
Others:
The text was updated successfully, but these errors were encountered: