-
Notifications
You must be signed in to change notification settings - Fork 307
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
Feature/etcd3 support with tests #308
base: master
Are you sure you want to change the base?
Changes from 3 commits
f6bd775
9684dde
75d5d30
7c5a5dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ import ( | |
"fmt" | ||
"github.com/coreos/etcd/mvcc/mvccpb" | ||
"encoding/json" | ||
//"github.com/skynetservices/skydns/backends/etcd" | ||
) | ||
|
||
type Config struct { | ||
|
@@ -41,11 +42,15 @@ func NewBackendv3 (client etcdv3.Client, ctx context.Context, config *Config) *B | |
} | ||
|
||
func (g *Backendv3) Records(name string, exact bool) ([]msg.Service, error) { | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmmm, whitespace is not but clutters this PR a bit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. okay will remove the whitespaces a bit |
||
path, star := msg.PathWithWildcard(name) | ||
|
||
r, err := g.get(path, true) | ||
|
||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
segments := strings.Split(msg.Path(name), "/") | ||
|
||
return g.loopNodes(r.Kvs, segments, star, nil) | ||
|
@@ -74,25 +79,36 @@ func (g *Backendv3) ReverseRecord(name string) (*msg.Service, error) { | |
} | ||
|
||
func (g *Backendv3) get(path string, recursive bool) (*etcdv3.GetResponse, error) { | ||
|
||
resp, err := g.inflight.Do(path, func() (interface{}, error){ | ||
if recursive == true { | ||
r, e := g.client.Get(g.ctx, path, etcdv3.WithPrefix()) | ||
|
||
if e != nil { | ||
return nil, e | ||
} | ||
if r.Kvs == nil { // there is no error thrown even if a key is not found in etcd3, we must check this instead. | ||
return nil, Etcd3Error{KEYNOTFOUND, "Etcd3 Key Not Found"}; | ||
} | ||
|
||
return r, e | ||
} else { | ||
r, e := g.client.Get(g.ctx, path) | ||
|
||
if e != nil { | ||
return nil, e | ||
} | ||
|
||
return r, e | ||
} | ||
}) | ||
|
||
if err != nil { | ||
return nil, err | ||
} | ||
if resp == nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why this? Isn't err != nil enough to bail out. Why not? |
||
return nil, err | ||
} | ||
return resp.(*etcdv3.GetResponse), err | ||
} | ||
|
||
|
@@ -104,13 +120,38 @@ type bareService struct { | |
Text string | ||
} | ||
|
||
/* | ||
* etcdv3 doesn't treat its keys as directories anymore | ||
* however skydns does, so this is needed to make the results | ||
* from etcd3.get to respect the rules of skydns. | ||
*/ | ||
func isItemDirTreeNode(nameParts []string, keyParts []string) bool { | ||
for i, w := range nameParts { | ||
if w == "*" || w == "any"{ | ||
return true; | ||
} | ||
if keyParts[i] != w && (!(w == "*" || w == "any")) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find this hard to parse, can we just add another if statement? |
||
return false; | ||
} | ||
} | ||
return true; | ||
} | ||
|
||
|
||
func (g *Backendv3) loopNodes(kv []*mvccpb.KeyValue, nameParts []string, star bool, bx map[bareService]bool) (sx []msg.Service, err error) { | ||
if bx == nil { | ||
bx = make(map[bareService]bool) | ||
} | ||
Nodes: | ||
for _, item := range kv { | ||
|
||
s := string(item.Key[:]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the |
||
keyParts := strings.Split(s, "/") | ||
|
||
if(!isItemDirTreeNode(nameParts, keyParts)) { | ||
continue; | ||
} | ||
|
||
if star { | ||
s := string(item.Key[:]) | ||
keyParts := strings.Split(s, "/") | ||
|
@@ -134,10 +175,14 @@ Nodes: | |
} | ||
|
||
b := bareService{serv.Host, | ||
serv.Port, | ||
serv.Priority, | ||
serv.Weight, | ||
serv.Text} | ||
serv.Port, | ||
serv.Priority, | ||
serv.Weight, | ||
serv.Text} | ||
|
||
if _, ok := bx[b]; ok { | ||
continue | ||
} | ||
|
||
bx[b] = true | ||
serv.Key = string(item.Key) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
package etcd3 | ||
|
||
import ( | ||
"fmt" | ||
) | ||
|
||
/* | ||
* etcdv3 doesn't throw errors anymore when a key is not found, | ||
* however skydns utilizes this feature, so we need to create our own | ||
* instance of error() and have skydns' algo use it accordingly as before. | ||
*/ | ||
type Etcd3Error struct { | ||
Code int | ||
Message string | ||
} | ||
|
||
func (e Etcd3Error) Error() string { | ||
return fmt.Sprintf("%v - %v", e.Code, e.Message); | ||
} | ||
|
||
const ( | ||
KEYNOTFOUND = 100 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a comment where this value comes from? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The value is arbitrary, so to respect on what skydns has been using with etcdv2 I used the value of 100. |
||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ import ( | |
etcd "github.com/coreos/etcd/client" | ||
"github.com/coreos/go-systemd/activation" | ||
"github.com/miekg/dns" | ||
etcd3 "github.com/skynetservices/skydns/backends/etcd3" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please sort this with the other skydns imports. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahhh noticed just now, it is sorted alphabetically. Okay will do |
||
) | ||
|
||
const Version = "2.5.3a" | ||
|
@@ -438,6 +439,7 @@ func (s *server) AddressRecords(q dns.Question, name string, previousRecords []d | |
} | ||
|
||
newRecord := serv.NewCNAME(q.Name, dns.Fqdn(serv.Host)) | ||
|
||
if len(previousRecords) > 7 { | ||
logf("CNAME lookup limit of 8 exceeded for %s", newRecord) | ||
// don't add it, and just continue | ||
|
@@ -450,6 +452,7 @@ func (s *server) AddressRecords(q dns.Question, name string, previousRecords []d | |
|
||
nextRecords, err := s.AddressRecords(dns.Question{Name: dns.Fqdn(serv.Host), Qtype: q.Qtype, Qclass: q.Qclass}, | ||
strings.ToLower(dns.Fqdn(serv.Host)), append(previousRecords, newRecord), bufsize, dnssec, both) | ||
|
||
if err == nil { | ||
// Only have we found something we should add the CNAME and the IP addresses. | ||
if len(nextRecords) > 0 { | ||
|
@@ -513,7 +516,9 @@ func (s *server) NSRecords(q dns.Question, name string) (records []dns.RR, extra | |
// SRVRecords returns SRV records from etcd. | ||
// If the Target is not a name but an IP address, a name is created. | ||
func (s *server) SRVRecords(q dns.Question, name string, bufsize uint16, dnssec bool) (records []dns.RR, extra []dns.RR, err error) { | ||
|
||
services, err := s.backend.Records(name, false) | ||
|
||
if err != nil { | ||
return nil, nil, err | ||
} | ||
|
@@ -522,6 +527,7 @@ func (s *server) SRVRecords(q dns.Question, name string, bufsize uint16, dnssec | |
|
||
// Looping twice to get the right weight vs priority | ||
w := make(map[int]int) | ||
|
||
for _, serv := range services { | ||
weight := 100 | ||
if serv.Weight != 0 { | ||
|
@@ -536,12 +542,14 @@ func (s *server) SRVRecords(q dns.Question, name string, bufsize uint16, dnssec | |
lookup := make(map[string]bool) | ||
for _, serv := range services { | ||
w1 := 100.0 / float64(w[serv.Priority]) | ||
|
||
if serv.Weight == 0 { | ||
w1 *= 100 | ||
} else { | ||
w1 *= float64(serv.Weight) | ||
} | ||
weight := uint16(math.Floor(w1)) | ||
|
||
ip := net.ParseIP(serv.Host) | ||
switch { | ||
case ip == nil: | ||
|
@@ -598,6 +606,7 @@ func (s *server) SRVRecords(q dns.Question, name string, bufsize uint16, dnssec | |
// MXRecords returns MX records from etcd. | ||
// If the Target is not a name but an IP address, a name is created. | ||
func (s *server) MXRecords(q dns.Question, name string, bufsize uint16, dnssec bool) (records []dns.RR, extra []dns.RR, err error) { | ||
|
||
services, err := s.backend.Records(name, false) | ||
if err != nil { | ||
return nil, nil, err | ||
|
@@ -877,14 +886,32 @@ func isTCP(w dns.ResponseWriter) bool { | |
return ok | ||
} | ||
|
||
// etcNameError return a NameError to the client if the error | ||
// returned from etcd has ErrorCode == 100. | ||
func isEtcdNameError(err error, s *server) bool { | ||
func verdictEtcdV2(err error, s *server) bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IsEtcd2NameError? |
||
if e, ok := err.(etcd.Error); ok && e.Code == etcd.ErrorCodeKeyNotFound { | ||
return true | ||
} | ||
if err != nil { | ||
logf("error from backend: %s", err) | ||
logf("error from backend v2: %s", err) | ||
} | ||
return false | ||
} | ||
|
||
func verdictEtcdV3(err error, s *server) bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. isEtcd3NameError? |
||
if e3, ok := err.(etcd3.Etcd3Error); ok && e3.Code == etcd3.KEYNOTFOUND { | ||
return true | ||
} | ||
if err != nil { | ||
logf("error from backend v3: %s", err) | ||
} | ||
return false | ||
} | ||
|
||
// etcNameError return a NameError to the client if the error | ||
// returned from etcd has ErrorCode == 100. | ||
func isEtcdNameError(err error, s *server) bool { | ||
if s.config.Etcd3 { | ||
return verdictEtcdV3(err, s); | ||
} else { | ||
return verdictEtcdV2(err, s); | ||
} | ||
} |
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.
?
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.
arrgh, yea my bad, missed deleting that one.