[WIP] DNS in kube-proxy #11599
[WIP] DNS in kube-proxy #11599
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA). Once you've signed, please reply here (e.g.
|
|
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") If this message is too spammy, please complain to ixdy. |
| // TODO: return a real TTL | ||
| // TODO: support reverse lookup (currently, kube2sky doesn't set this up) | ||
| // TODO: investigate just using add/remove/update events instead of receiving | ||
| // the whole sevice set each time |
ncdc
Jul 20, 2015
Member
service
service
|
@googlebot I signed it! |
|
CLAs look good, thanks! |
|
FYI this was not quite decided on, but I'll take a look in a few days time. @uluyol too On Mon, Jul 20, 2015 at 1:30 PM, Solly notifications@github.com wrote:
|
| @@ -0,0 +1,455 @@ | |||
| package proxy | |||
smarterclayton
Jul 21, 2015
Contributor
Pull this out into its own package pkg/dns or pkg/proxy/dns
Pull this out into its own package pkg/dns or pkg/proxy/dns
| @@ -146,6 +152,13 @@ func (s *ProxyServer) Run(_ []string) error { | |||
| }, 5*time.Second) | |||
| } | |||
|
|
|||
| go func() { | |||
| err := proxy.ServeDNS(dnsHandler) | |||
smarterclayton
Jul 21, 2015
Contributor
This is typically in a go util.Forever loop (if ServeDNS doesn't spawn its own goroutines). If it does, this is fine.
This is typically in a go util.Forever loop (if ServeDNS doesn't spawn its own goroutines). If it does, this is fine.
DirectXMan12
Jul 21, 2015
Author
Contributor
SkyDNS spawns goroutines in Run() -- https://github.com/skynetservices/skydns/blob/master/server/server.go#L149
SkyDNS spawns goroutines in Run() -- https://github.com/skynetservices/skydns/blob/master/server/server.go#L149
smarterclayton
Jul 21, 2015
Contributor
Ok - add a comment to that effect here "cannot restart skydns without
leaking goroutines"
On Jul 21, 2015, at 2:08 PM, Solly notifications@github.com wrote:
In cmd/kube-proxy/app/server.go
#11599 (comment)
:
@@ -146,6 +152,13 @@ func (s ProxyServer) Run( []string) error {
}, 5_time.Second)
}
- go func() {
-
err := proxy.ServeDNS(dnsHandler)
SkyDNS spawns goroutines in Run() --
https://github.com/skynetservices/skydns/blob/master/server/server.go#L149
—
Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/11599/files#r35134201
.
Ok - add a comment to that effect here "cannot restart skydns without
leaking goroutines"
On Jul 21, 2015, at 2:08 PM, Solly notifications@github.com wrote:
In cmd/kube-proxy/app/server.go
#11599 (comment)
:
@@ -146,6 +152,13 @@ func (s ProxyServer) Run( []string) error {
}, 5_time.Second)
}
- go func() {
err := proxy.ServeDNS(dnsHandler)
SkyDNS spawns goroutines in Run() --
https://github.com/skynetservices/skydns/blob/master/server/server.go#L149
—
Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/11599/files#r35134201
.
|
|
||
| func ServeDNS(handler *DNSHandler) error { | ||
| config := &skyserver.Config{ | ||
| Domain: "cluster.local.", |
smarterclayton
Jul 21, 2015
Contributor
Should be domainSuffix
Should be domainSuffix
| return fmt.Sprintf("%x", h.Sum32()) | ||
| } | ||
|
|
||
| func (handler *DNSServiceHandler) OnUpdate(services []api.Service) { |
smarterclayton
Jul 21, 2015
Contributor
Why implement service lookup this way instead of simply using the cache directly to answer queries? Is it so you can handle the reverse record lookup?
I think that's an argument that we should be using client.Cache for the proxy instead of proxy.Config - that refactor is slightly larger, but that would dramatically reduce the code in the proxy to something more reasonable: cache.Reflector (indexer on cluster ip) for services, and a cache.Reflector for endpoints.
That would then allow us to answer queries in a single function vs having to materialize two separate structures.
Why implement service lookup this way instead of simply using the cache directly to answer queries? Is it so you can handle the reverse record lookup?
I think that's an argument that we should be using client.Cache for the proxy instead of proxy.Config - that refactor is slightly larger, but that would dramatically reduce the code in the proxy to something more reasonable: cache.Reflector (indexer on cluster ip) for services, and a cache.Reflector for endpoints.
That would then allow us to answer queries in a single function vs having to materialize two separate structures.
smarterclayton
Jul 21, 2015
Contributor
The config code here was written long before cache.Store existed, but the store solves both problems far more elegantly than the current setup.
The config code here was written long before cache.Store existed, but the store solves both problems far more elegantly than the current setup.
DirectXMan12
Jul 21, 2015
Author
Contributor
Ah, ok. The original suggestion was that "proxy already has the information", so I tried to mirror the existing code in proxier.go and endpoints.go. I will take a look at cache.Store and friends.
Ah, ok. The original suggestion was that "proxy already has the information", so I tried to mirror the existing code in proxier.go and endpoints.go. I will take a look at cache.Store and friends.
smarterclayton
Jul 21, 2015
Contributor
The DNS service resolver in Openshift could in theory be used whole cloth.
In the long run, the proxy should be using cache.Store. It may be good to
split that refactor if it looks feasible (proxy to cache.Store first, then
DNS). Im pretty sure the refactor is possible, but there may be a wrinkle
I'm missing. You will need the reverse cluster IP indexer anyway.
On Jul 21, 2015, at 10:31 AM, Solly notifications@github.com wrote:
In pkg/proxy/dns.go
#11599 (comment)
:
+type DNSServiceHandler struct {
- *DNSHandler
+}
+
+type DNSEndpointHandler struct {
- *DNSHandler
+}
+
+func getHash(text string) string {
- h := fnv.New32a()
- h.Write([]byte(text))
- return fmt.Sprintf("%x", h.Sum32())
+}
+
+func (handler *DNSServiceHandler) OnUpdate(services []api.Service) {
Ah, ok. The original suggestion was that "proxy already has the
information", so I tried to mirror the existing code in proxier.go and
endpoints.go. I will take a look at cache.Store and friends.
—
Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/11599/files#r35108136
.
The DNS service resolver in Openshift could in theory be used whole cloth.
In the long run, the proxy should be using cache.Store. It may be good to
split that refactor if it looks feasible (proxy to cache.Store first, then
DNS). Im pretty sure the refactor is possible, but there may be a wrinkle
I'm missing. You will need the reverse cluster IP indexer anyway.
On Jul 21, 2015, at 10:31 AM, Solly notifications@github.com wrote:
In pkg/proxy/dns.go
#11599 (comment)
:
+type DNSServiceHandler struct {
- *DNSHandler
+}
+
+type DNSEndpointHandler struct {- *DNSHandler
+}
+
+func getHash(text string) string {- h := fnv.New32a()
- h.Write([]byte(text))
- return fmt.Sprintf("%x", h.Sum32())
+}
+
+func (handler *DNSServiceHandler) OnUpdate(services []api.Service) {
Ah, ok. The original suggestion was that "proxy already has the
information", so I tried to mirror the existing code in proxier.go and
endpoints.go. I will take a look at cache.Store and friends.
—
Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/11599/files#r35108136
.
|
I think I'm pretty convinced on my end that this is the right way to handle A problem arises if we partition the service space, but even then we can The overhead per node is fairly low as well. |
|
Haven't looked at details but in favor of idea. |
| newEntry := normalServiceRecord{portRecords: make(map[string]skymsg.Service)} | ||
| srvPath := serviceSubdomain(name, "", "", "") | ||
| for _, port := range service.Spec.Ports { | ||
| newEntry.portRecords[makeServicePortSpec(port)] = skymsg.Service{ |
uluyol
Jul 23, 2015
Contributor
You have several of these that set a bunch of default values. Use a constructor.
You have several of these that set a bunch of default values. Use a constructor.
DirectXMan12
force-pushed the
DirectXMan12:feature/dns-in-kube-proxy
branch
from
209c2fa
to
d6a8e00
|
I switched over to using the cache. Essentially, the new code listens for service updates, and converts them into skydns entries, and stores those into the cache (and indexes them), pulling in endpoint information as needed. It also listens for endpoint updates, converting those into skydns services as well (when appropriate). It seems to preform fairly well compared to the first version as well as the current pod-based implementation. I added a couple of new methods to the cache code to facilitate bulk operations on the cache (add, replace entries matching index, delete entries matching index), since a single service yields multiple skydns entries. I still need to add support for a couple of wildcard options, but other than that it mirrors the responses of the current setup. |
DirectXMan12
force-pushed the
DirectXMan12:feature/dns-in-kube-proxy
branch
2 times, most recently
from
1f19d34
to
d0dcb35
This commit adds in dependencies on more parts of skydns to facilitate a Kubernetes SkyDNS backend.
This commit adds a new interface, `BulkIndexer` to the client cache package. `BulkIndexer` builds on `Indexer`, adding a bulk-add operation, as well as bulk-update and bulk-delete operations (based on a given index). There are useful for modifying sections of a store in an atomic fashion.
DirectXMan12
force-pushed the
DirectXMan12:feature/dns-in-kube-proxy
branch
from
d0dcb35
to
59d4954
This commit makes kube-proxy serve DNS requests. This is done with a custom SkyDNS backend that serves requests directly based on updates from Kubernetes about services and endpoints.
DirectXMan12
force-pushed the
DirectXMan12:feature/dns-in-kube-proxy
branch
from
59d4954
to
48f9cde
|
The currently "missing" parts are the following wildcard patterns, as well as TTLs that actual "age". For comparison, OpenShift's DNS implementation leaves the TTL as 30 (like this PR currently does), and doesn't implement wildcards at all. The wildcards supported in this code are prefix wildcards (e.g. ..svc.cluster.local), as well as using a wildcard instead of "svc". Not supported are wildcards in other positions. Wildcard support for the others could be added with new indices or simple loop-checking, but I wanted to confirm that these are desired (they don't seem to be mentioned in the Kubernetes DNS docs AFAICT -- it's just a side-effect of using the SkyDNS etcd backend). |
|
On Jul 29, 2015, at 6:25 PM, Solly notifications@github.com wrote: The currently "missing" parts are the following wildcard patterns, as well We implicitly support wildcards beyond the length of the name (extra The wildcards supported in this code are prefix wildcards (e.g. — |
| @@ -144,6 +145,14 @@ func (s *ProxyServer) Run(_ []string) error { | |||
| }, 5*time.Second) | |||
| } | |||
|
|
|||
| go func() { | |||
| // Note: Cannot restart SkyDNS without leaking goroutines | |||
| err := dns.ServeDNS(client) | |||
smarterclayton
Jul 29, 2015
Contributor
Does ServeDNS block? If so we may want to make the next line fatalf instead
Does ServeDNS block? If so we may want to make the next line fatalf instead
|
@karlkfi the idea was that kube-proxy already has a cache for the endpoints and service information, so it made sense to just reuse that. |
|
I would rather not have the node watching / caching twice. The kube proxy On Tue, Nov 17, 2015 at 10:06 AM, Solly Ross notifications@github.com
|
|
Thinking abstractly here, perhaps prematurely: |
|
In the DNS case, we need to manage an index (fairly atomically), so the On Tue, Nov 17, 2015 at 3:23 PM, Karl Isenberg notifications@github.com
|
|
Is this still the best place to track the progress of getting a dns service into kubernetes? |
|
We're discussing timing on it. For now, the standard path is still the On Sun, Jan 10, 2016 at 8:30 PM, Dusty Mabe notifications@github.com
|
|
I'm worried most about resources. Currently DNS watches all Services, all Other than that, having a local mirror of apiserver state (and an API to That said, why not jam it all into Kubelet? That also caches Service state On Sun, Jan 10, 2016 at 7:53 PM, Clayton Coleman notifications@github.com
|
|
I think we should stop mirroring pods :) On Mon, Jan 11, 2016 at 2:40 AM, Tim Hockin notifications@github.com
|
|
For instance we implemented the pod namespace without supporting the On Mon, Jan 11, 2016 at 10:30 AM, Clayton Coleman ccoleman@redhat.com
|
|
Sure, we don't have to mirror the whole pod structure, but it's still On Mon, Jan 11, 2016 at 7:31 AM, Clayton Coleman notifications@github.com
|
|
Why do you need the pod? I thought we specifically designed the DNS name On Mon, Jan 11, 2016 at 2:11 PM, Tim Hockin notifications@github.com
|
|
Ahh, that's a good point - we could just parse the question in the pod On Mon, Jan 11, 2016 at 11:22 AM, Clayton Coleman notifications@github.com
|
|
https://github.com/openshift/origin/blob/master/pkg/dns/serviceresolver.go#L85 On Mon, Jan 11, 2016 at 2:26 PM, Tim Hockin notifications@github.com
|
|
This PR has no activity for multiple months. Please reopen this PR once you rebase and push a new commit. |
|
@sross I have an alternate implementation in On Tue, Apr 26, 2016 at 3:15 AM, Erick Fejta notifications@github.com wrote:
|
|
Make that @DirectXMan12 |
|
Oops On Tue, Apr 26, 2016 at 10:31 AM, Andy Goldstein notifications@github.com
|
|
Is there any plan to revive this issue? |
|
Right now, maybe not. In Openshift we're planning on delivering
something for this, but it may be 1.4 or 1.5 before we reach
agreementZ
|
_Work In Progress_
This makes kube-proxy serve DNS as per #7469, using a custom SkyDNS backend. It functions similarly to the proxier part of kube-proxy -- it listens for updates to the service and enpoint list (using the same config listeners as proxier.go and loadbalancer.go), and converts those into SkyDNS service entries.
The responses should mirror the current SkyDNS setup.