[PATCH] Keep DNS mappings fresh (was Re: [Sks-devel] keyserver.gingerbear.net: Dynamic IP Update didn't)

[Kim Minh Kaplan]

The current situation regarding membership and DNS is this: DNS lookup 
is done only when the membership file is modified[1].  This incurs some 
problematic behaviors that remain until the next time membership is 
modified: 

  * if some partners' DNS is down during at lookup time, they are 
    excluded from the membership, 

  * when a partner changes IP address SKS keeps using a stale address. 

To solve these I separated the loading of membership file and actual DNS 
lookup.  The membership file is loaded only when it is modified (like 
before) but no DNS lookup is done at that time. 

Every time the recon process connects to a partner it does a DNS 
lookup.  The caching is thus done at the hoster's DNS cache server which 
will do a better job at handling expiry and refreshing.  For the purpose 
of recon server authorization handling (see below) the result is also 
cached. 

Whenever the recon server receives a recon request it scans all the 
cached results and fails if the connecting client IP is not among 
them[2]. 

I have been running this on keyserver.kim-minh.com for a week now and it 
gives nice results (it has seen some name come back to life).  Attached 
is a patch to sks-1.1.0 (My server actually runs with my patch on Yaron 
Minsky's mercurial version) 

I plan to change it to support multiple IP on a single name and then 
integrate Phil Pennock's IPv6 support. 

Please try the code and/or comment on the approach. 

Kim Minh. 

[1] In this context modified simply means the first time membership is 
   needed and when the modification time of the file changes. 

[2] There is a small hack to make sure every member gets included in the 
   cache at some time. 

diff -ruw sks-1.1.0.orig/membership.ml sks-1.1.0-dnsfix/membership.ml
--- sks-1.1.0.orig/membership.ml        2008-05-08 01:05:54.000000000 +0000
+++ sks-1.1.0-dnsfix/membership.ml      2009-03-19 09:16:23.000000000 +0000
@@ -44,33 +44,34 @@
 
 let local_recon_addr = Utils.unit_memoize local_recon_addr
 
-let remove_self addresses = 
-  List.filter ~f:(fun (addr,str) -> addr <> local_recon_addr ()) addresses
-
-let convert_address l =
+let parse_address l =
   try 
     sscanf l "%s %d" 
-    (fun addr port -> Unix.ADDR_INET (lookup_hostname addr,port))
+      (fun addr port -> addr, port)
   with 
     Scanf.Scan_failure _ | End_of_file | Failure _ -> raise (Malformed_entry l)
 
+let convert_address l =
+  let (addr, port) = parse_address l in
+  try 
+    Some (Unix.ADDR_INET (lookup_hostname addr, port))
+  with 
+  | Lookup_failure addr -> perror "Lookup failure on address %s" addr;
+      None
+
 let load_membership_file file =
   let rec loop list =
     try
       let line = decomment (input_line file) in
-      let addr = convert_address line in
-      (addr,line) :: loop list
+      ignore (parse_address line);
+      loop (line :: list)
     with
       | End_of_file -> list
-      | Lookup_failure addr -> 
-         perror "Lookup failure on address %s" addr;
-         loop list
       | Malformed_entry line -> 
          perror "Malformed entry %s" line;
          loop list
   in
-  let raw_membership = loop [] in
-  Array.of_list (remove_self raw_membership)
+  loop []
 
 let get_mtime fname = 
   try
@@ -80,22 +81,10 @@
   with 
       Unix.Unix_error _ -> None
 
-let load_membership fname = 
-  let file = open_in fname in
-  protect ~f:(fun () -> 
-               let mshp = load_membership_file file in
-               match get_mtime fname with
-                 | None -> 
-                     plerror 2 "%s" 
-                       ("Unable to get mtime for membership. " ^
-                        "Failed to reload.")
-                 | Some mtime -> membership := (mshp,mtime)
-            )
-    ~finally:(fun () -> close_in file)
-
-let sockaddr_to_string sockaddr = match sockaddr with
-    Unix.ADDR_UNIX s -> sprintf "<ADDR_UNIX %s>" s
-  | Unix.ADDR_INET (addr,p) -> sprintf "<ADDR_INET %s:%d>" 
+let sockaddr_to_string = function
+    None -> "<ADDR_UNKNOWN>"
+  | Some Unix.ADDR_UNIX s -> sprintf "<ADDR_UNIX %s>" s
+  | Some Unix.ADDR_INET (addr,p) -> sprintf "<ADDR_INET %s:%d>" 
       (Unix.string_of_inet_addr addr) p
 
 let membership_string () = 
@@ -106,19 +95,31 @@
   let strings = List.map ~f:to_string (Array.to_list mshp) in
   "Membership: " ^ String.concat ~sep:", " strings
     
+(* Returns true if membership has been reloaded, false otherwise *)
+let load_membership fname = 
+  let mtime = (Unix.stat fname).Unix.st_mtime in
+  if mtime <> (snd !membership) then
+    let memberlines =
+      let file = open_in fname in
+      protect ~f:(fun () -> load_membership_file file)
+       ~finally:(fun () -> close_in file)
+    in
+    let old = Array.to_list (fst !membership) in
+    let f line =
+      try
+       List.find ~f:(fun (_, old_line) -> line = old_line) old
+      with
+       Not_found -> (None, line)
+    in
+    let merged = List.map ~f memberlines in
+    membership := (Array.of_list merged, mtime);
+    plerror 5 "%s" (membership_string ())
 
 let reload_if_changed () = 
   let fname = Lazy.force Settings.membership_file in
-  let (mshp,old_mtime) = !membership in
-  match get_mtime fname with
-    | None -> 
-       plerror 2 "%s" ("Unable to get mtime for membership file. " ^
-                       "Can't decide whether to reload")
-    | Some mtime ->
-       if old_mtime <> mtime then 
-         ( load_membership fname;
-           plerror 5 "%s" (membership_string ())
-         )
+  try
+    load_membership fname
+  with _ -> plerror 2 "Failed to reload %s." fname
 
 let get_names () = 
   let mshp = 
@@ -136,16 +137,39 @@
   let (m,mtime) = !membership in
   membership := (m,0.)
 
-let get () = 
-  let mshp = 
-    if Sys.file_exists (Lazy.force Settings.membership_file) then (
+(* Demote a member to the end of the membership table *)
+let demote_member members n =
+  let m = members.(n) in
+  plerror 5 "Demoting %s" (snd m);
+  let count = Array.length members in
+  Array.blit members (n + 1) members n (count - n - 1);
+  members.(count - 1) <- m
+
+(* Refresh member n's address *)
+let refresh_member members n =
+  match members.(n) with
+    (addr, line) ->
+      let fresh_addr = convert_address line in
+      if addr <> fresh_addr then begin
+       members.(n) <- (fresh_addr, line);
+       plerror 3 "address for %s changed from %s to %s"
+         line (sockaddr_to_string addr) (sockaddr_to_string fresh_addr)
+      end
+
+let rec choose_partner () =
+  if Sys.file_exists (Lazy.force Settings.membership_file) then begin
       reload_if_changed ();
-      let (m,mtime) = !membership in 
-      m
-    )
-    else [| |]
-  in
-  Array.map ~f:fst mshp
+    let (mshp, _) = !membership in
+    let choice = Random.int (Array.length mshp) in
+    refresh_member mshp choice;
+    match mshp.(choice) with
+      (None, _) -> demote_member mshp choice;
+       choose_partner()
+    | (Some addr, _) -> if addr = local_recon_addr() then
+       choose_partner () else
+       addr
+  end else
+    raise Not_found
 
 let same_inet_addr addr1 addr2 = 
   match (addr1,addr2) with
@@ -159,8 +183,12 @@
   let found = ref false in
   let i = ref 0 in
   while !i < Array.length m && not !found do 
-    if same_inet_addr addr (fst m.(!i)) then
-      found := true;
+    if fst m.(!i) = None then
+      refresh_member m !i;
+    match m.(!i) with
+      (None, _) -> demote_member m !i;
+       i := Array.length m
+    | (Some iaddr, _) -> found := same_inet_addr addr iaddr;
     incr i
   done;
   !found
diff -ruw sks-1.1.0.orig/reconserver.ml sks-1.1.0-dnsfix/reconserver.ml
--- sks-1.1.0.orig/reconserver.ml       2008-05-08 01:05:54.000000000 +0000
+++ sks-1.1.0-dnsfix/reconserver.ml     2009-03-19 09:16:23.000000000 +0000
@@ -74,13 +74,9 @@
        handle addr cin cout
     )
 
-  let choose array = 
-    if Array.length array = 0 then raise Not_found
-    else array.(Random.int (Array.length array))
-
   let choose_partner () = 
     try
-      choose (Membership.get ())
+      Membership.choose_partner ()
     with
        Not_found | Invalid_argument _ -> 
          failwith "No gossip partners available"