openstack
/
octavia
Code Issues Proposed changes

Fix prefix for vip_ipv6 

Currently we calculate prefix based on netmask when writing the vip
interface file. Since netmask has been converted to prefix in ipv6,
this patch will avoid converting it to prefix twice which could
result in a wrong prefix length.

Also fix a bug in another test that relies on osutils, but wasn't
mocking correctly.

Co-Authored-By: Adam Harwell <flux.adam@gmail.com>
Change-Id: I9ee0cce12a975f4ab8f3df2707b355aab35c6cb3
This commit is contained in:
Hang Yang 2018-10-13 12:34:37 -07:00 committed by Adam Harwell
parent 6307fbfa08
commit b2162c39a2
3 changed files with 159 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

42
octavia/amphorae/backends/agent/api_server/osutils.py
View File

@ -117,7 +117,9 @@ class BaseOS(object):
interface=primary_interface,
vip=vip,
vip_ipv6=ip.version == 6,
prefix=utils.netmask_to_prefix(netmask),
# For ipv6 the netmask is already the prefix
prefix=(netmask if ip.version == 6
else utils.netmask_to_prefix(netmask)),
broadcast=broadcast,
netmask=netmask,
gateway=gateway,
@ -147,11 +149,12 @@ class BaseOS(object):
with os.fdopen(os.open(interface_file_path, flags, mode),
'w') as text_file:
text = self._generate_network_file_text(netns_interface, fixed_ips,
mtu, template_port)
text = self._generate_network_file_text(
netns_interface, fixed_ips, mtu, template_port)
text_file.write(text)
def _generate_network_file_text(self, netns_interface, fixed_ips, mtu,
@classmethod
def _generate_network_file_text(cls, netns_interface, fixed_ips, mtu,
template_port):
text = ''
if fixed_ips is None:
@ -169,7 +172,7 @@ class BaseOS(object):
broadcast = network.broadcast_address.exploded
netmask = (network.prefixlen if ip.version == 6
else network.netmask.exploded)
host_routes = self.get_host_routes(fixed_ip)
host_routes = cls.get_host_routes(fixed_ip)
except ValueError:
return webob.Response(
@ -184,7 +187,8 @@ class BaseOS(object):
text = '\n'.join([text, new_text])
return text
def get_host_routes(self, fixed_ip):
@classmethod
def get_host_routes(cls, fixed_ip):
host_routes = []
for hr in fixed_ip.get('host_routes', []):
network = ipaddress.ip_network(
@ -194,7 +198,8 @@ class BaseOS(object):
host_routes.append({'network': network, 'gw': hr['nexthop']})
return host_routes
def _bring_if_up(self, interface, what):
@classmethod
def _bring_if_up(cls, interface, what):
# Note, we are not using pyroute2 for this as it is not /etc/netns
# aware.
cmd = ("ip netns exec {ns} ifup {params}".format(
@ -209,7 +214,8 @@ class BaseOS(object):
message='Error plugging {0}'.format(what),
details=e.output), status=500))
def _bring_if_down(self, interface):
@classmethod
def _bring_if_down(cls, interface):
# Note, we are not using pyroute2 for this as it is not /etc/netns
# aware.
cmd = ("ip netns exec {ns} ifdown {params}".format(
@ -220,13 +226,14 @@ class BaseOS(object):
LOG.info('Ignoring failure to ifdown %s due to error: %s %s',
interface, e, e.output)
def bring_interfaces_up(self, ip, primary_interface, secondary_interface):
self._bring_if_down(primary_interface)
@classmethod
def bring_interfaces_up(cls, ip, primary_interface, secondary_interface):
cls._bring_if_down(primary_interface)
if secondary_interface:
self._bring_if_down(secondary_interface)
self._bring_if_up(primary_interface, 'VIP')
cls._bring_if_down(secondary_interface)
cls._bring_if_up(primary_interface, 'VIP')
if secondary_interface:
self._bring_if_up(secondary_interface, 'VIP')
cls._bring_if_up(secondary_interface, 'VIP')
def has_ifup_all(self):
return True
@ -482,14 +489,15 @@ class RH(BaseOS):
host_routes, template_routes, None, None, None)
self._write_ifup_ifdown_local_scripts_if_possible()
def bring_interfaces_up(self, ip, primary_interface, secondary_interface):
@classmethod
def bring_interfaces_up(cls, ip, primary_interface, secondary_interface):
if ip.version == 4:
super(RH, self).bring_interfaces_up(
super(RH, cls).bring_interfaces_up(
ip, primary_interface, secondary_interface)
else:
# Secondary interface is not present in IPv6 configuration
self._bring_if_down(primary_interface)
self._bring_if_up(primary_interface, 'VIP')
cls._bring_if_down(primary_interface)
cls._bring_if_up(primary_interface, 'VIP')
def has_ifup_all(self):
return False

10
octavia/tests/functional/amphorae/backend/agent/api_server/test_server.py
View File

@ -2379,14 +2379,16 @@ class TestServerTestCase(base.TestCase):
@mock.patch('octavia.amphorae.backends.agent.api_server.util.'
'get_os_init_system', return_value=consts.INIT_SYSTEMD)
def test_ubuntu_upload_keepalived_config_systemd(self, mock_init_system):
self._test_upload_keepalived_config(consts.INIT_SYSTEMD,
consts.UBUNTU, mock_init_system)
with mock.patch('distro.id', return_value='ubuntu'):
self._test_upload_keepalived_config(
consts.INIT_SYSTEMD, consts.UBUNTU, mock_init_system)
@mock.patch('octavia.amphorae.backends.agent.api_server.util.'
'get_os_init_system', return_value=consts.INIT_SYSTEMD)
def test_centos_upload_keepalived_config_systemd(self, mock_init_system):
self._test_upload_keepalived_config(consts.INIT_SYSTEMD,
consts.CENTOS, mock_init_system)
with mock.patch('distro.id', return_value='centos'):
self._test_upload_keepalived_config(
consts.INIT_SYSTEMD, consts.CENTOS, mock_init_system)
@mock.patch('octavia.amphorae.backends.agent.api_server.util.'
'get_os_init_system', return_value=consts.INIT_UPSTART)

128
octavia/tests/unit/amphorae/backends/agent/api_server/test_osutils.py
View File

@ -12,6 +12,7 @@
# License for the specific language governing permissions and limitations
# under the License.
import ipaddress
import os
import shutil
@ -22,6 +23,8 @@ from octavia.amphorae.backends.agent.api_server import osutils
from octavia.common import config
from octavia.common import constants as consts
from octavia.common import exceptions as octavia_exceptions
from octavia.common import utils
from octavia.tests.common import utils as test_utils
from octavia.tests.unit import base
@ -187,6 +190,131 @@ class TestOSUtils(base.TestCase):
self.assertTrue(self.ubuntu_os_util.has_ifup_all())
self.assertFalse(self.rh_os_util.has_ifup_all())
def test_write_vip_interface_file(self):
netns_interface = u'eth1234'
FIXED_IP = u'192.0.2.2'
SUBNET_CIDR = u'192.0.2.0/24'
GATEWAY = u'192.51.100.1'
DEST1 = u'198.51.100.0/24'
DEST2 = u'203.0.113.0/24'
NEXTHOP = u'192.0.2.1'
MTU = 1450
FIXED_IP_IPV6 = u'2001:0db8:0000:0000:0000:0000:0000:0001'
# Subnet prefix is purposefully not 32, because that coincidentally
# matches the result of any arbitrary IPv4->prefixlen conversion
SUBNET_CIDR_IPV6 = u'2001:db8::/70'
ip = ipaddress.ip_address(FIXED_IP)
network = ipaddress.ip_network(SUBNET_CIDR)
broadcast = network.broadcast_address.exploded
netmask = network.netmask.exploded
netmask_prefix = utils.netmask_to_prefix(netmask)
ipv6 = ipaddress.ip_address(FIXED_IP_IPV6)
networkv6 = ipaddress.ip_network(SUBNET_CIDR_IPV6)
broadcastv6 = networkv6.broadcast_address.exploded
netmaskv6 = networkv6.prefixlen
host_routes = [
{'gw': NEXTHOP, 'network': ipaddress.ip_network(DEST1)},
{'gw': NEXTHOP, 'network': ipaddress.ip_network(DEST2)}
]
path = self.ubuntu_os_util.get_network_interface_file(netns_interface)
mock_open = self.useFixture(test_utils.OpenFixture(path)).mock_open
mock_template = mock.MagicMock()
# Test an IPv4 VIP
with mock.patch('os.open'), mock.patch.object(
os, 'fdopen', mock_open):
self.ubuntu_os_util.write_vip_interface_file(
interface_file_path=path,
primary_interface=netns_interface,
vip=FIXED_IP,
ip=ip,
broadcast=broadcast,
netmask=netmask,
gateway=GATEWAY,
mtu=MTU,
vrrp_ip=None,
vrrp_version=None,
render_host_routes=host_routes,
template_vip=mock_template)
mock_template.render.assert_called_once_with(
consts=consts,
interface=netns_interface,
vip=FIXED_IP,
vip_ipv6=False,
prefix=netmask_prefix,
broadcast=broadcast,
netmask=netmask,
gateway=GATEWAY,
network=SUBNET_CIDR,
mtu=MTU,
vrrp_ip=None,
vrrp_ipv6=False,
host_routes=host_routes,
topology="SINGLE",
)
# Now test with an IPv6 VIP
mock_template.reset_mock()
with mock.patch('os.open'), mock.patch.object(
os, 'fdopen', mock_open):
self.ubuntu_os_util.write_vip_interface_file(
interface_file_path=path,
primary_interface=netns_interface,
vip=FIXED_IP_IPV6,
ip=ipv6,
broadcast=broadcastv6,
netmask=netmaskv6,
gateway=GATEWAY,
mtu=MTU,
vrrp_ip=None,
vrrp_version=None,
render_host_routes=host_routes,
template_vip=mock_template)
mock_template.render.assert_called_once_with(
consts=consts,
interface=netns_interface,
vip=FIXED_IP_IPV6,
vip_ipv6=True,
prefix=netmaskv6,
broadcast=broadcastv6,
netmask=netmaskv6,
gateway=GATEWAY,
network=SUBNET_CIDR_IPV6,
mtu=MTU,
vrrp_ip=None,
vrrp_ipv6=False,
host_routes=host_routes,
topology="SINGLE",
)
def test_write_port_interface_file(self):
netns_interface = 'eth1234'
MTU = 1450
fixed_ips = []
path = 'mypath'
mock_template = mock.MagicMock()
mock_open = self.useFixture(test_utils.OpenFixture(path)).mock_open
mock_gen_text = mock.MagicMock()
with mock.patch('os.open'), mock.patch.object(
os, 'fdopen', mock_open), mock.patch.object(
osutils.BaseOS, '_generate_network_file_text', mock_gen_text):
self.base_os_util.write_port_interface_file(
netns_interface=netns_interface,
fixed_ips=fixed_ips,
mtu=MTU,
interface_file_path=path,
template_port=mock_template)
mock_gen_text.assert_called_once_with(
netns_interface, fixed_ips, MTU, mock_template)
@mock.patch('shutil.copy2')
@mock.patch('os.makedirs')
@mock.patch('shutil.copytree')