This looks good. Thanks for fixing them! -Qiuyu
On Tue, Jul 31, 2018 at 3:29 PM, Ben Pfaff <[email protected]> wrote: > On Tue, Jul 31, 2018 at 02:08:49PM -0700, Qiuyu Xiao wrote: >> This patch reintroduces ovs-monitor-ipsec daemon that >> was previously removed by commit 2b02d770 ("openvswitch: >> Allow external IPsec tunnel management.") >> >> After this patch, there are no IPsec flavored tunnels anymore. >> IPsec is enabled by setting up the right values in: >> 1. OVSDB:Interface:options column; >> 2. OVSDB:Open_vSwitch:other_config column; >> 3. OpenFlow pipeline. >> >> GRE, VXLAN, GENEVE, and STT IPsec tunnels are supported. LibreSwan and >> StrongSwan IKE daemons are supported. User can choose pre-shared key, >> self-signed peer certificate, or CA-signed certificate as authentication >> method. >> >> Signed-off-by: Qiuyu Xiao <[email protected]> >> Signed-off-by: Ansis Atteka <[email protected]> >> Co-authored-by: Ansis Atteka <[email protected]> > > Thanks for the patch. > > I think that you must not have flake8 installed. It gave me a long list > of stylistic errors. The following incremental fixes them. Does it > look OK? > > --8<--------------------------cut here-------------------------->8-- > > diff --git a/ipsec/ovs-monitor-ipsec b/ipsec/ovs-monitor-ipsec > index 580c8d8c80a6..163b04004f84 100755 > --- a/ipsec/ovs-monitor-ipsec > +++ b/ipsec/ovs-monitor-ipsec > @@ -14,16 +14,12 @@ > # limitations under the License. > > import argparse > -import glob > -import os > import re > import subprocess > import sys > import copy > from string import Template > > -from ovs.db import error > -from ovs.db import types > import ovs.daemon > import ovs.db.idl > import ovs.dirs > @@ -55,19 +51,19 @@ conn prevent_unencrypted_vxlan > mark={0} > > """ > -transp_tmpl = {"gre" : Template("""\ > +transp_tmpl = {"gre": Template("""\ > conn $ifname-$version > $auth_section > leftprotoport=gre > rightprotoport=gre > > -"""), "gre64" : Template("""\ > +"""), "gre64": Template("""\ > conn $ifname-$version > $auth_section > leftprotoport=gre > rightprotoport=gre > > -"""), "geneve" : Template("""\ > +"""), "geneve": Template("""\ > conn $ifname-in-$version > $auth_section > leftprotoport=udp/6081 > @@ -78,7 +74,7 @@ $auth_section > leftprotoport=udp > rightprotoport=udp/6081 > > -"""), "stt" : Template("""\ > +"""), "stt": Template("""\ > conn $ifname-in-$version > $auth_section > leftprotoport=tcp/7471 > @@ -89,7 +85,7 @@ $auth_section > leftprotoport=tcp > rightprotoport=tcp/7471 > > -"""), "vxlan" : Template("""\ > +"""), "vxlan": Template("""\ > conn $ifname-in-$version > $auth_section > leftprotoport=udp/4789 > @@ -106,6 +102,7 @@ exiting = False > monitor = None > xfrm = None > > + > class XFRM(object): > """This class is a simple wrapper around ip-xfrm (8) command line > utility. We are using this class only for informational purposes > @@ -130,11 +127,11 @@ class XFRM(object): > a = line.split(" ") > if len(a) >= 4 and a[0] == "src" and a[2] == "dst": > dst = (a[3].split("/"))[0] > - if not dst in policies: > + if dst not in policies: > policies[dst] = [] > policies[dst].append(line) > src = (a[3].split("/"))[0] > - if not src in policies: > + if src not in policies: > policies[src] = [] > policies[src].append(line) > return policies > @@ -155,14 +152,15 @@ class XFRM(object): > and a[1] == "src" and a[3] == "dst": > remote_ip = a[4].rstrip().split("/")[0] > local_ip = a[2].rstrip().split("/")[0] > - if not remote_ip in securities: > + if remote_ip not in securities: > securities[remote_ip] = [] > securities[remote_ip].append(line) > - if not local_ip in securities: > + if local_ip not in securities: > securities[local_ip] = [] > securities[local_ip].append(line) > return securities > > + > class StrongSwanHelper(object): > """This class does StrongSwan specific configurations.""" > > @@ -192,18 +190,18 @@ conn %%default > > """ > > - auth_tmpl = {"psk" : Template("""\ > + auth_tmpl = {"psk": Template("""\ > left=$local_ip > right=$remote_ip > authby=psk"""), > - "pki_remote" : Template("""\ > + "pki_remote": Template("""\ > left=$local_ip > right=$remote_ip > leftid=$local_name > rightid=$remote_name > leftcert=$certificate > rightcert=$remote_cert"""), > - "pki_ca" : Template("""\ > + "pki_ca": Template("""\ > left=$local_ip > right=$remote_ip > leftid=$local_name > @@ -256,7 +254,7 @@ conn %%default > if not m: > continue > ifname = m.group(1) > - if not ifname in conns: > + if ifname not in conns: > conns[ifname] = {} > (conns[ifname])[tunnel_name[0]] = line > > @@ -354,7 +352,10 @@ conn %%default > # interface name in IPsec "connection" > try: > ver = int(re.findall(r'\d+', conn[len(ifname):])[0]) > - except ValueError, IndexError: > + except IndexError: > + vlog.err("%s does not contain version number") > + continue > + except ValueError: > vlog.err("%s does not contain version number") > continue > > @@ -362,6 +363,7 @@ conn %%default > vlog.info("%s is outdated %u" % (conn, ver)) > subprocess.call([self.IPSEC, "stroke", "down-nb", conn]) > > + > class LibreSwanHelper(object): > """This class does LibreSwan specific configurations.""" > CONF_HEADER = """%s > @@ -378,11 +380,11 @@ conn %%default > > """ % (FILE_HEADER) > > - auth_tmpl = {"psk" : Template("""\ > + auth_tmpl = {"psk": Template("""\ > left=$local_ip > right=$remote_ip > authby=secret"""), > - "pki_remote" : Template("""\ > + "pki_remote": Template("""\ > left=$local_ip > right=$remote_ip > leftid=@$local_name > @@ -390,7 +392,7 @@ conn %%default > leftcert="$local_name" > rightcert="$remote_name" > leftrsasigkey=%cert"""), > - "pki_ca" : Template("""\ > + "pki_ca": Template("""\ > left=$local_ip > right=$remote_ip > leftid=@$local_name > @@ -500,7 +502,10 @@ conn %%default > # interface name in IPsec "connection" > try: > ver = int(re.findall(r'\d+', conn[len(ifname):])[0]) > - except ValueError, IndexError: > + except ValueError: > + vlog.err("%s does not contain version number") > + continue > + except IndexError: > vlog.err("%s does not contain version number") > continue > > @@ -577,7 +582,7 @@ conn %%default > continue > > ifname = m.group(1) > - if not ifname in conns: > + if ifname not in conns: > conns[ifname] = {} > (conns[ifname])[conn] = line > > @@ -665,11 +670,11 @@ conn %%default > raise Exception(proc.stderr.read()) > > if cacert: > - proc= subprocess.Popen(['certutil', '-A', '-a', '-i', > - cacert, '-d', 'sql:/etc/ipsec.d/', > - '-n', 'cacert', '-t', 'CT,,'], > - stdout=subprocess.PIPE, > - stderr=subprocess.PIPE) > + proc = subprocess.Popen(['certutil', '-A', '-a', '-i', > + cacert, '-d', 'sql:/etc/ipsec.d/', > + '-n', 'cacert', '-t', 'CT,,'], > + stdout=subprocess.PIPE, > + stderr=subprocess.PIPE) > proc.wait() > if proc.returncode: > raise Exception(proc.stderr.read()) > @@ -677,6 +682,7 @@ conn %%default > vlog.err("Import cert and key failed.\n" + str(e)) > subprocess.call(['rm', '-f', '/tmp/%s.p12' % name]) > > + > class IPsecTunnel(object): > """This is the base class for IPsec tunnel.""" > > @@ -721,18 +727,18 @@ class IPsecTunnel(object): > remote_name = monitor._get_cn_from_cert(remote_cert) > > new_conf = { > - "ifname" : self.name, > - "tunnel_type" : row.type, > - "remote_ip" : options.get("remote_ip"), > - "local_ip" : options.get("local_ip"), > - "skb_mark" : monitor.conf["skb_mark"], > - "certificate" : monitor.conf["pki"]["certificate"], > - "private_key" : monitor.conf["pki"]["private_key"], > - "ca_cert" : monitor.conf["pki"]["ca_cert"], > - "remote_cert" : remote_cert, > - "remote_name" : remote_name, > - "local_name" : monitor.conf["pki"]["local_name"], > - "psk" : options.get("psk")} > + "ifname": self.name, > + "tunnel_type": row.type, > + "remote_ip": options.get("remote_ip"), > + "local_ip": options.get("local_ip"), > + "skb_mark": monitor.conf["skb_mark"], > + "certificate": monitor.conf["pki"]["certificate"], > + "private_key": monitor.conf["pki"]["private_key"], > + "ca_cert": monitor.conf["pki"]["ca_cert"], > + "remote_cert": remote_cert, > + "remote_name": remote_name, > + "local_name": monitor.conf["pki"]["local_name"], > + "psk": options.get("psk")} > > if self.conf != new_conf: > # Configuration was updated in OVSDB. Validate it and figure > @@ -751,11 +757,11 @@ class IPsecTunnel(object): > self.state = "INVALID" > > new_status = { > - "cfm_state" : "Up" if row.cfm_fault == [False] else > - "Down" if row.cfm_fault == [True] else > - "Disabled", > - "ofport" : "Not assigned" if (row.ofport in [[], [-1]]) else > - row.ofport[0]} > + "cfm_state": "Up" if row.cfm_fault == [False] else > + "Down" if row.cfm_fault == [True] else > + "Disabled", > + "ofport": "Not assigned" if (row.ofport in [[], [-1]]) else > + row.ofport[0]} > > if self.status != new_status: > # Tunnel has become unhealthy or ofport changed. Simply log > this. > @@ -837,6 +843,7 @@ class IPsecTunnel(object): > > return True > > + > class IPsecMonitor(object): > """This class monitors and configures IPsec tunnels""" > > @@ -846,13 +853,13 @@ class IPsecMonitor(object): > > # Global configuration shared by all tunnels > self.conf = { > - "pki" : { > - "private_key" : None, > - "certificate" : None, > - "ca_cert" : None, > - "local_name" : None > + "pki": { > + "private_key": None, > + "certificate": None, > + "ca_cert": None, > + "local_name": None > }, > - "skb_mark" : None > + "skb_mark": None > } > self.conf_in_use = copy.deepcopy(self.conf) > > @@ -963,7 +970,7 @@ class IPsecMonitor(object): > > # Mark for removal those tunnels that just disappeared from OVSDB > for tunnel in self.tunnels.keys(): > - if not tunnel in ifaces: > + if tunnel not in ifaces: > self.del_tunnel(tunnel) > > def read_ovsdb(self, data): > @@ -1037,21 +1044,25 @@ class IPsecMonitor(object): > > return m.group(1) > > + > def unixctl_xfrm_policies(conn, unused_argv, unused_aux): > global xfrm > policies = xfrm.get_policies() > conn.reply(str(policies)) > > + > def unixctl_xfrm_state(conn, unused_argv, unused_aux): > global xfrm > securities = xfrm.get_securities() > conn.reply(str(securities)) > > + > def unixctl_ipsec_status(conn, unused_argv, unused_aux): > global monitor > conns = monitor.ike_helper.get_active_conns() > conn.reply(str(conns)) > > + > def unixctl_show(conn, unused_argv, unused_aux): > global monitor > global xfrm > @@ -1059,11 +1070,13 @@ def unixctl_show(conn, unused_argv, unused_aux): > securities = xfrm.get_securities() > monitor.show(conn, policies, securities) > > + > def unixctl_refresh(conn, unused_argv, unused_aux): > global monitor > monitor.ike_helper.refresh(monitor) > conn.reply(None) > > + > def unixctl_exit(conn, unused_argv, unused_aux): > global monitor > global exiting > @@ -1078,6 +1091,7 @@ def unixctl_exit(conn, unused_argv, unused_aux): > > conn.reply(None) > > + > def main(): > parser = argparse.ArgumentParser() > parser.add_argument("database", metavar="DATABASE", > @@ -1147,6 +1161,7 @@ def main(): > unixctl_server.close() > idl.close() > > + > if __name__ == '__main__': > try: > main() _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
