Make ext subnet config optional

It is common for Neutron deployment's policy to forbid GETs to the
public subnet, only allowing GETs for the public net. Since the only
required field of those two for creating a FIP is the public net, let's
change public net to be the only required config option and have the
subnet stick around as optional.

Change-Id: I31c3c51ad2dc12f8f560cbab01c86d04aabb754e
Closes-Bug: 1749921
Signed-off-by: Antoni Segura Puimedon <antonisp@celebdor.com>

devstack/plugin.sh

@@ -284,6 +284,9 @@ function configure_neutron_defaults {
     sg_ids=$(echo $(neutron security-group-list \
         --project-id "$project_id" -c id -f value) | tr ' ' ',')
 
+    ext_svc_net_id="$(neutron net-show -c id -f value \
+        "${KURYR_NEUTRON_DEFAULT_EXT_SVC_NET}")"
+
     ext_svc_subnet_id="$(neutron subnet-show -c id -f value \
         "${KURYR_NEUTRON_DEFAULT_EXT_SVC_SUBNET}")"
 
@@ -340,7 +343,7 @@ function configure_neutron_defaults {
     if [ -n "$OVS_BRIDGE" ]; then
         iniset "$KURYR_CONFIG" neutron_defaults ovs_bridge "$OVS_BRIDGE"
     fi
-    iniset "$KURYR_CONFIG" neutron_defaults external_svc_subnet "$ext_svc_subnet_id"
+    iniset "$KURYR_CONFIG" neutron_defaults external_svc_net "$ext_svc_net_id"
     iniset "$KURYR_CONFIG" octavia_defaults member_mode "$KURYR_K8S_OCTAVIA_MEMBER_MODE"
 }
 

devstack/settings

@@ -18,6 +18,7 @@ KURYR_NEUTRON_DEFAULT_POD_SUBNET=${KURYR_NEUTRON_DEFAULT_POD_SUBNET:-k8s-pod-sub
 KURYR_NEUTRON_DEFAULT_SERVICE_SUBNET=${KURYR_NEUTRON_DEFAULT_SERVICE_SUBNET:-k8s-service-subnet}
 KURYR_NEUTRON_DEFAULT_SUBNETPOOL_ID=${KURYR_NEUTRON_DEFAULT_SUBNETPOOL_ID:-}
 KURYR_NEUTRON_DEFAULT_ROUTER=${KURYR_NEUTRON_DEFAULT_ROUTER:-}
+KURYR_NEUTRON_DEFAULT_EXT_SVC_NET=${KURYR_NEUTRON_DEFAULT_EXT_SVC_NET:-public}
 KURYR_NEUTRON_DEFAULT_EXT_SVC_SUBNET=${KURYR_NEUTRON_DEFAULT_EXT_SVC_SUBNET:-public-subnet}
 
 # Etcd

doc/source/installation/services.rst

@@ -412,10 +412,13 @@ The services and pods subnets should be created.
    A. Create an external/provider network
    B. Create subnet/pool range of external CIDR
    C. Connect external subnet to kuryr-kubernetes router
-   D. Configure Kuryr.conf public ip subnet to point to the external subnet::
+   D. Configure external network details in Kuryr.conf as follows::
 
          [neutron_defaults]
-         external_svc_subnet=  external_subnet_id
+         external_svc_net= <id of external network>
+         # 'external_svc_subnet' field is optional, set this field in case
+         # multiple subnets attached to 'external_svc_net'
+         external_svc_subnet= <id of external subnet>
 
    From this point for each K8s service of type=LoadBalancer and in which
    'load-balancer-ip' is not specified, an external IP from

kuryr_kubernetes/config.py

@@ -153,8 +153,11 @@ neutron_defaults = [
                sample_default="br-int"),
     cfg.StrOpt('service_subnet',
                help=_("Default Neutron subnet ID for Kubernetes services")),
+    cfg.StrOpt('external_svc_net',
+               help=_("Default external network ID for Kubernetes services")),
     cfg.StrOpt('external_svc_subnet',
-               help=_("Default external subnet for Kubernetes services")),
+               help=_("Optional external subnet ID for Kubernetes services"),
+               default=None),
 ]
 
 octavia_defaults = [

kuryr_kubernetes/controller/drivers/lb_public_ip.py

@@ -12,8 +12,6 @@
 #    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
 #    License for the specific language governing permissions and limitations
 #    under the License.
-from kuryr.lib import exceptions as kl_exc
-from kuryr_kubernetes import clients
 from kuryr_kubernetes import config
 from kuryr_kubernetes.controller.drivers import base
 from kuryr_kubernetes.controller.drivers import public_ip
@@ -63,27 +61,17 @@ class FloatingIpServicePubIPDriver(base.ServicePubIpDriver):
         else:
             LOG.debug("Trying to allocate public ip from pool")
 
-        # get public subnet id from kuryr.conf
+        # get public network/subnet ids from kuryr.conf
-        external_svc_subnet = config.CONF.neutron_defaults.external_svc_subnet
+        public_network_id = config.CONF.neutron_defaults.external_svc_net
-        if not external_svc_subnet:
+        public_subnet_id = config.CONF.neutron_defaults.external_svc_subnet
-            raise cfg.RequiredOptError('external_svc_subnet',
+        if not public_network_id:
+            raise cfg.RequiredOptError('external_svc_net',
                                        cfg.OptGroup('neutron_defaults'))
 
-        neutron = clients.get_neutron_client()
+        res_id, alloc_ip_addr = (
-        n_subnet = neutron.show_subnet(external_svc_subnet).get('subnet')
+            self._drv_pub_ip.allocate_ip(public_network_id, project_id,
-        if not n_subnet:
+                                         pub_subnet_id=public_subnet_id,
-            LOG.error(
+                                         description='kuryr_lb'))
-                "No subnet found for external_svc_subnet=%s",
-                external_svc_subnet)
-            raise kl_exc.NoResourceException
-
-        public_network_id = n_subnet['network_id']
-
-        res_id, alloc_ip_addr = (self._drv_pub_ip.allocate_ip
-                                 (public_network_id,
-                                  external_svc_subnet,
-                                  project_id,
-                                  'kuryr_lb'))
         service_pub_ip_info = obj_lbaas.LBaaSPubIp(ip_id=res_id,
                                                    ip_addr=alloc_ip_addr,
                                                    alloc_method='pool')

kuryr_kubernetes/controller/drivers/public_ip.py

@@ -36,13 +36,14 @@ class BasePubIpDriver(object):
         raise NotImplementedError()
 
     @abc.abstractmethod
-    def allocate_ip(self, pub_net_id, pub_subnet_id, project_id, description):
+    def allocate_ip(self, pub_net_id, project_id, pub_subnet_id=None,
+                    description=None):
         """allocate ip address from public network id
 
         :param pub_net_id: public network id
-        :param pub_subnet_id: public subnet id
         :param project_id:
-        :param description: string describing request
+        :param pub_subnet_id: public subnet id (Optional)
+        :param description: string describing request (Optional)
         :returns res_id , ip_addr
                 :res_id - resource id
                 :ip_addr - ip aaddress
@@ -100,19 +101,23 @@ class FipPubIpDriver(BasePubIpDriver):
             LOG.error("Invalid parameter ip_addr=%s", ip_addr)
         return None
 
-    def allocate_ip(self, pub_net_id, pub_subnet_id, project_id, description):
+    def allocate_ip(self, pub_net_id, project_id, pub_subnet_id=None,
-
+                    description=None):
         neutron = clients.get_neutron_client()
+        request = {'floatingip': {
+            'tenant_id': project_id,
+            'project_id': project_id,
+            'floating_network_id': pub_net_id}}
+
+        if pub_subnet_id is not None:
+            request['floatingip']['subnet_id'] = pub_subnet_id
+        if description is not None:
+            request['floatingip']['description'] = description
+
         try:
-            response = neutron.create_floatingip({'floatingip': {
+            response = neutron.create_floatingip(request)
-                'tenant_id': project_id,
-                'project_id': project_id,
-                'floating_network_id': pub_net_id,
-                'subnet_id': pub_subnet_id,
-                'description': description}})
         except n_exc.NeutronClientException as ex:
-            LOG.error("Failed to create floating IP - subnetid=%s ",
+            LOG.error("Failed to create floating IP - netid=%s ", pub_net_id)
-                      pub_subnet_id)
             raise ex
         return response['floatingip']['id'], response[
             'floatingip']['floating_ip_address']

kuryr_kubernetes/controller/handlers/lbaas.py

@@ -272,8 +272,11 @@ class LoadBalancerHandler(k8s_base.ResourceEventHandler):
         return sorted(ep_ports) == sorted(spec_ports)
 
     def _has_pods(self, endpoints):
+        ep_subsets = endpoints.get('subsets', [])
+        if not ep_subsets:
+            return False
         return any(True
-                   for subset in endpoints.get('subsets', [])
+                   for subset in ep_subsets
                    for address in subset.get('addresses', [])
                    if address.get('targetRef', {}).get('kind') == 'Pod')
 

kuryr_kubernetes/handlers/retry.py

@@ -70,6 +70,7 @@ class Retry(base.EventHandler):
             except Exception:
                 LOG.debug('Report handler unhealthy %s', self._handler)
                 self._handler.set_health_status(healthy=False)
+                raise
 
     def _sleep(self, deadline, attempt, exception):
         now = time.time()

kuryr_kubernetes/tests/unit/controller/drivers/test_lb_public_ip.py

@@ -12,7 +12,6 @@
 #    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
 #    License for the specific language governing permissions and limitations
 #    under the License.
-from kuryr.lib import exceptions as kl_exc
 import mock
 from neutronclient.common import exceptions as n_exc
 
@@ -97,10 +96,10 @@ class TestFloatingIpServicePubIPDriverDriver(test_base.TestCase):
                            spec_lb_ip,  project_id))
 
     @mock.patch('kuryr_kubernetes.config.CONF')
-    def test_acquire_service_pub_ip_info_pool_subnet_not_defined(self, m_cfg):
+    def test_acquire_service_pub_ip_info_pool_net_not_defined(self, m_cfg):
         driver = d_lb_public_ip.FloatingIpServicePubIPDriver()
-        public_subnet = ''
+        public_net = ''
-        m_cfg.neutron_defaults.external_svc_subnet = public_subnet
+        m_cfg.neutron_defaults.external_svc_net = public_net
         neutron = self.useFixture(k_fix.MockNeutronClient()).client
         neutron.list_floatingips.return_value = {}
         project_id = mock.sentinel.project_id
@@ -113,22 +112,32 @@ class TestFloatingIpServicePubIPDriverDriver(test_base.TestCase):
             spec_type, spec_lb_ip, project_id)
 
     @mock.patch('kuryr_kubernetes.config.CONF')
-    def test_acquire_service_pub_ip_info_pool_subnet_not_exist(self, m_cfg):
+    def test_acquire_service_pub_ip_info_pool_subnet_is_none(self, m_cfg):
-        driver = d_lb_public_ip.FloatingIpServicePubIPDriver()
+        cls = d_lb_public_ip.FloatingIpServicePubIPDriver
+        m_driver = mock.Mock(spec=cls)
+        m_driver._drv_pub_ip = public_ip.FipPubIpDriver()
         neutron = self.useFixture(k_fix.MockNeutronClient()).client
-        public_subnet = mock.sentinel.public_subnet
+        public_net = mock.sentinel.public_subnet
-        m_cfg.neutron_defaults.external_svc_subnet = public_subnet
+        m_cfg.neutron_defaults.external_svc_net = public_net
+        m_cfg.neutron_defaults.external_svc_subnet = None
 
-        neutron.show_subnet.return_value = {}
+        neutron.show_subnet.return_value =\
+            {'subnet': {'network_id': 'ec29d641-fec4-4f67-928a-124a76b3a8e6'}}
+        floating_ip = {'floating_ip_address': '1.2.3.5',
+                       'id': 'ec29d641-fec4-4f67-928a-124a76b3a888'}
+        neutron.create_floatingip.return_value = {'floatingip': floating_ip}
 
         project_id = mock.sentinel.project_id
         spec_type = 'LoadBalancer'
         spec_lb_ip = None
 
-        self.assertRaises(
+        expected_resp = \
-            kl_exc.NoResourceException,
+            obj_lbaas.LBaaSPubIp(ip_id=floating_ip['id'],
-            driver.acquire_service_pub_ip_info,
+                                 ip_addr=floating_ip['floating_ip_address'],
-            spec_type, spec_lb_ip, project_id)
+                                 alloc_method='pool')
+
+        self.assertEqual(expected_resp, cls.acquire_service_pub_ip_info
+                         (m_driver, spec_type, spec_lb_ip,  project_id))
 
     @mock.patch('kuryr_kubernetes.config.CONF')
     def test_acquire_service_pub_ip_info_alloc_from_pool(self, m_cfg):

kuryr_kubernetes/tests/unit/controller/drivers/test_public_ip.py

@@ -101,7 +101,7 @@ class TestFipPubIpDriver(test_base.TestCase):
         neutron.create_floatingip.return_value = {'floatingip': floating_ip}
 
         fip_id, fip_addr = cls.allocate_ip(
-            m_driver, pub_net_id, pub_subnet_id, project_id, description)
+            m_driver, pub_net_id, project_id, pub_subnet_id, description)
         self.assertEqual(fip_id, floating_ip['id'])
         self.assertEqual(fip_addr, floating_ip['floating_ip_address'])
 
@@ -118,7 +118,7 @@ class TestFipPubIpDriver(test_base.TestCase):
 
         self.assertRaises(
             n_exc.NeutronClientException, cls.allocate_ip,
-            m_driver, pub_net_id, pub_subnet_id, project_id, description)
+            m_driver, pub_net_id, project_id, pub_subnet_id, description)
 
     def test_free_ip_neutron_exception(self):
         cls = d_public_ip.FipPubIpDriver

kuryr_kubernetes/tests/unit/handlers/test_retry.py

@@ -134,18 +134,3 @@ class TestRetryHandler(test_base.TestCase):
         m_sleep.assert_has_calls([
             mock.call(deadline, i + 1, failures[i])
             for i in range(len(failures))])
-
-    @mock.patch('itertools.count')
-    @mock.patch.object(h_retry.Retry, '_sleep')
-    def test_call_should_not_raise(self, m_sleep, m_count):
-        event = mock.sentinel.event
-        m_handler = mock.Mock()
-        m_handler.side_effect = _EX1()
-        m_count.return_value = list(range(1, 5))
-        retry = h_retry.Retry(m_handler, exceptions=(_EX11, _EX2))
-
-        retry(event)
-
-        m_handler.assert_called_with(event)
-        m_handler.set_health_status.assert_called_with(healthy=False)
-        m_sleep.assert_not_called()

releasenotes/notes/make-ext-subnet-optional-99e73bfcbde96c22.yaml

@@ -0,0 +1,24 @@
+---
+upgrade:
+  - |
+
+   For the external services (type=LoadBalancer) case,
+   a new field 'external_svc_net' was added and the 'external_svc_subnet'
+   field become optional.
+   The following should be modified at kuryr.conf::
+
+
+         [neutron_defaults]
+         external_svc_net= <id of external network>
+         # 'external_svc_subnet' field is optional, set this field in case
+         # multiple subnets attached to 'external_svc_net'
+         external_svc_subnet= <id of external subnet>
+
+fixes:
+  - |
+    It is very common for production environments to only allow access
+    to the public network and not the associated public subnets.
+    In that case, we fail to allocate a floating IP to the Loadbalancer service
+    type.
+    In order to fix it, we added an option for specifying the network id instead
+    and switch the subnet config option to being optional.