From c37bf56b1e30ebc9bc94efca0ea12158d5648463 Mon Sep 17 00:00:00 2001 From: Miek Gieben Date: Mon, 13 Nov 2017 21:51:51 +0000 Subject: [PATCH] plugin/kubernetes: correctly set NODATA for ns (#1229) * plugin/kubernetes: Add GetNamespaceByName A bare or wildcard query for just the namespace should return NODATA, not NXDOMAIN, otherwise we deny the entirety of the names under the namespace. Add test to check for this in pod verified mode. * Review More comments and move namespace code to namespace.go --- plugin/kubernetes/controller.go | 15 +++++++++++ .../kubernetes/handler_pod_verified_test.go | 21 ++++++++++++++++ plugin/kubernetes/handler_test.go | 11 ++++++++ plugin/kubernetes/kubernetes.go | 25 +++++++++++-------- plugin/kubernetes/kubernetes_test.go | 8 ++++++ plugin/kubernetes/namespace.go | 20 +++++++++++++++ plugin/kubernetes/ns_test.go | 3 +++ plugin/kubernetes/reverse_test.go | 8 ++++++ 8 files changed, 101 insertions(+), 10 deletions(-) create mode 100644 plugin/kubernetes/namespace.go diff --git a/plugin/kubernetes/controller.go b/plugin/kubernetes/controller.go index 89b608703..c47c33819 100644 --- a/plugin/kubernetes/controller.go +++ b/plugin/kubernetes/controller.go @@ -36,6 +36,7 @@ type dnsController interface { EndpointsList() []*api.Endpoints GetNodeByName(string) (*api.Node, error) + GetNamespaceByName(string) (*api.Namespace, error) Run() HasSynced() bool @@ -388,6 +389,9 @@ func (dns *dnsControl) EndpointsList() (eps []*api.Endpoints) { return eps } +// GetNodeByName return the node by name. If nothing is found an error is +// returned. This query causes a roundtrip to the k8s API server, so use +// sparingly. Currently this is only used for Federation. func (dns *dnsControl) GetNodeByName(name string) (*api.Node, error) { v1node, err := dns.client.Nodes().Get(name, meta.GetOptions{}) if err != nil { @@ -395,3 +399,14 @@ func (dns *dnsControl) GetNodeByName(name string) (*api.Node, error) { } return v1node, nil } + +// GetNamespaceByName returns the namespace by name. If nothing is found an +// error is returned. This query causes a roundtrip to the k8s API server, so +// use sparingly. +func (dns *dnsControl) GetNamespaceByName(name string) (*api.Namespace, error) { + v1ns, err := dns.client.Namespaces().Get(name, meta.GetOptions{}) + if err != nil { + return &api.Namespace{}, err + } + return v1ns, nil +} diff --git a/plugin/kubernetes/handler_pod_verified_test.go b/plugin/kubernetes/handler_pod_verified_test.go index 35aa92c05..e2c488e5e 100644 --- a/plugin/kubernetes/handler_pod_verified_test.go +++ b/plugin/kubernetes/handler_pod_verified_test.go @@ -18,6 +18,27 @@ var podModeVerifiedCases = []test.Case{ test.A("10-240-0-1.podns.pod.cluster.local. 0 IN A 10.240.0.1"), }, }, + { + Qname: "podns.pod.cluster.local.", Qtype: dns.TypeA, + Rcode: dns.RcodeSuccess, + Ns: []dns.RR{ + test.SOA("cluster.local. 300 IN SOA ns.dns.cluster.local. hostmaster.cluster.local. 1499347823 7200 1800 86400 60"), + }, + }, + { + Qname: "svcns.svc.cluster.local.", Qtype: dns.TypeA, + Rcode: dns.RcodeSuccess, + Ns: []dns.RR{ + test.SOA("cluster.local. 300 IN SOA ns.dns.cluster.local. hostmaster.cluster.local. 1499347823 7200 1800 86400 60"), + }, + }, + { + Qname: "pod-nons.pod.cluster.local.", Qtype: dns.TypeA, + Rcode: dns.RcodeNameError, + Ns: []dns.RR{ + test.SOA("cluster.local. 300 IN SOA ns.dns.cluster.local. hostmaster.cluster.local. 1499347823 7200 1800 86400 60"), + }, + }, { Qname: "172-0-0-2.podns.pod.cluster.local.", Qtype: dns.TypeA, Rcode: dns.RcodeNameError, diff --git a/plugin/kubernetes/handler_test.go b/plugin/kubernetes/handler_test.go index 5e9743b68..f9a600868 100644 --- a/plugin/kubernetes/handler_test.go +++ b/plugin/kubernetes/handler_test.go @@ -476,3 +476,14 @@ func (APIConnServeTest) GetNodeByName(name string) (*api.Node, error) { }, }, nil } + +func (APIConnServeTest) GetNamespaceByName(name string) (*api.Namespace, error) { + if name == "pod-nons" { // hanlder_pod_verified_test.go uses this for non-existent namespace. + return &api.Namespace{}, nil + } + return &api.Namespace{ + ObjectMeta: meta.ObjectMeta{ + Name: name, + }, + }, nil +} diff --git a/plugin/kubernetes/kubernetes.go b/plugin/kubernetes/kubernetes.go index 625422935..0168ab52a 100644 --- a/plugin/kubernetes/kubernetes.go +++ b/plugin/kubernetes/kubernetes.go @@ -304,7 +304,14 @@ func (k *Kubernetes) findPods(r recordRequest, zone string) (pods []msg.Service, podname := r.service zonePath := msg.Path(zone, "coredns") ip := "" + err = errNoItems + if wildcard(podname) && !wildcard(namespace) { + // If namespace exist, err should be nil, so that we return nodata instead of NXDOMAIN + if k.namespace(namespace) { + err = nil + } + } if strings.Count(podname, "-") == 3 && !strings.Contains(podname, "--") { ip = strings.Replace(podname, "-", ".", -1) @@ -336,7 +343,14 @@ func (k *Kubernetes) findPods(r recordRequest, zone string) (pods []msg.Service, // findServices returns the services matching r from the cache. func (k *Kubernetes) findServices(r recordRequest, zone string) (services []msg.Service, err error) { zonePath := msg.Path(zone, "coredns") - err = errNoItems // Set to errNoItems to signal really nothing found, gets reset when name is matched. + + err = errNoItems + if wildcard(r.service) && !wildcard(r.namespace) { + // If namespace exist, err should be nil, so that we return nodata instead of NXDOMAIN + if k.namespace(namespace) { + err = nil + } + } var ( endpointsListFunc func() []*api.Endpoints @@ -449,15 +463,6 @@ func wildcard(s string) bool { return s == "*" || s == "any" } -// namespaceExposed returns true when the namespace is exposed. -func (k *Kubernetes) namespaceExposed(namespace string) bool { - _, ok := k.Namespaces[namespace] - if len(k.Namespaces) > 0 && !ok { - return false - } - return true -} - const ( // Svc is the DNS schema for kubernetes services Svc = "svc" diff --git a/plugin/kubernetes/kubernetes_test.go b/plugin/kubernetes/kubernetes_test.go index de8c3f025..f05693d71 100644 --- a/plugin/kubernetes/kubernetes_test.go +++ b/plugin/kubernetes/kubernetes_test.go @@ -332,6 +332,14 @@ func (APIConnServiceTest) GetNodeByName(name string) (*api.Node, error) { }, nil } +func (APIConnServiceTest) GetNamespaceByName(name string) (*api.Namespace, error) { + return &api.Namespace{ + ObjectMeta: meta.ObjectMeta{ + Name: name, + }, + }, nil +} + func TestServices(t *testing.T) { k := New([]string{"interwebs.test."}) diff --git a/plugin/kubernetes/namespace.go b/plugin/kubernetes/namespace.go new file mode 100644 index 000000000..7dafc7ab3 --- /dev/null +++ b/plugin/kubernetes/namespace.go @@ -0,0 +1,20 @@ +package kubernetes + +// namespace checks if namespace n exists in this cluster. This returns true +// even for non exposed namespaces, see namespaceExposed. +func (k *Kubernetes) namespace(n string) bool { + ns, err := k.APIConn.GetNamespaceByName(n) + if err != nil { + return false + } + return ns.ObjectMeta.Name == n +} + +// namespaceExposed returns true when the namespace is exposed. +func (k *Kubernetes) namespaceExposed(namespace string) bool { + _, ok := k.Namespaces[namespace] + if len(k.Namespaces) > 0 && !ok { + return false + } + return true +} diff --git a/plugin/kubernetes/ns_test.go b/plugin/kubernetes/ns_test.go index 70e297054..853ef9288 100644 --- a/plugin/kubernetes/ns_test.go +++ b/plugin/kubernetes/ns_test.go @@ -55,6 +55,9 @@ func (APIConnTest) EpIndexReverse(string) []*api.Endpoints { } func (APIConnTest) GetNodeByName(name string) (*api.Node, error) { return &api.Node{}, nil } +func (APIConnTest) GetNamespaceByName(name string) (*api.Namespace, error) { + return &api.Namespace{}, nil +} func TestNsAddr(t *testing.T) { diff --git a/plugin/kubernetes/reverse_test.go b/plugin/kubernetes/reverse_test.go index 604dff0c9..4f17a2ae5 100644 --- a/plugin/kubernetes/reverse_test.go +++ b/plugin/kubernetes/reverse_test.go @@ -86,6 +86,14 @@ func (APIConnReverseTest) GetNodeByName(name string) (*api.Node, error) { }, nil } +func (APIConnReverseTest) GetNamespaceByName(name string) (*api.Namespace, error) { + return &api.Namespace{ + ObjectMeta: meta.ObjectMeta{ + Name: name, + }, + }, nil +} + func TestReverse(t *testing.T) { k := New([]string{"cluster.local.", "0.10.in-addr.arpa.", "168.192.in-addr.arpa."})