From b9be59ed0b04ec95fefcd907c96cd9b01ea92035 Mon Sep 17 00:00:00 2001 From: Antoni Segura Puimedon Date: Fri, 16 Feb 2018 12:13:17 +0100 Subject: [PATCH] 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 (cherry picked from commit 20bc89ff87952d53249ea213d149ba315d97644f) --- devstack/plugin.sh | 5 ++- devstack/settings | 1 + doc/source/installation/services.rst | 7 ++-- kuryr_kubernetes/config.py | 5 ++- .../controller/drivers/lb_public_ip.py | 30 +++++----------- .../controller/drivers/public_ip.py | 31 +++++++++------- kuryr_kubernetes/controller/handlers/lbaas.py | 5 ++- kuryr_kubernetes/handlers/retry.py | 1 + .../controller/drivers/test_lb_public_ip.py | 35 ++++++++++++------- .../unit/controller/drivers/test_public_ip.py | 4 +-- .../tests/unit/handlers/test_retry.py | 15 -------- ...-ext-subnet-optional-99e73bfcbde96c22.yaml | 24 +++++++++++++ 12 files changed, 94 insertions(+), 69 deletions(-) create mode 100644 releasenotes/notes/make-ext-subnet-optional-99e73bfcbde96c22.yaml diff --git a/devstack/plugin.sh b/devstack/plugin.sh index e7d0c7059..828f8980a 100644 --- a/devstack/plugin.sh +++ b/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" } diff --git a/devstack/settings b/devstack/settings index 3d4e56468..fa8574f24 100644 --- a/devstack/settings +++ b/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 diff --git a/doc/source/installation/services.rst b/doc/source/installation/services.rst index 4e1ebc5aa..a914f49d8 100644 --- a/doc/source/installation/services.rst +++ b/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= + # 'external_svc_subnet' field is optional, set this field in case + # multiple subnets attached to 'external_svc_net' + external_svc_subnet= From this point for each K8s service of type=LoadBalancer and in which 'load-balancer-ip' is not specified, an external IP from diff --git a/kuryr_kubernetes/config.py b/kuryr_kubernetes/config.py index 463e03b0c..4bd3a1127 100644 --- a/kuryr_kubernetes/config.py +++ b/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 = [ diff --git a/kuryr_kubernetes/controller/drivers/lb_public_ip.py b/kuryr_kubernetes/controller/drivers/lb_public_ip.py index 386e0c4a3..3fc32d8c5 100644 --- a/kuryr_kubernetes/controller/drivers/lb_public_ip.py +++ b/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 - external_svc_subnet = config.CONF.neutron_defaults.external_svc_subnet - if not external_svc_subnet: - raise cfg.RequiredOptError('external_svc_subnet', + # get public network/subnet ids from kuryr.conf + public_network_id = config.CONF.neutron_defaults.external_svc_net + public_subnet_id = config.CONF.neutron_defaults.external_svc_subnet + if not public_network_id: + raise cfg.RequiredOptError('external_svc_net', cfg.OptGroup('neutron_defaults')) - neutron = clients.get_neutron_client() - n_subnet = neutron.show_subnet(external_svc_subnet).get('subnet') - if not n_subnet: - LOG.error( - "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')) + res_id, alloc_ip_addr = ( + self._drv_pub_ip.allocate_ip(public_network_id, project_id, + pub_subnet_id=public_subnet_id, + description='kuryr_lb')) service_pub_ip_info = obj_lbaas.LBaaSPubIp(ip_id=res_id, ip_addr=alloc_ip_addr, alloc_method='pool') diff --git a/kuryr_kubernetes/controller/drivers/public_ip.py b/kuryr_kubernetes/controller/drivers/public_ip.py index 690ae61d3..e92541598 100644 --- a/kuryr_kubernetes/controller/drivers/public_ip.py +++ b/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': { - 'tenant_id': project_id, - 'project_id': project_id, - 'floating_network_id': pub_net_id, - 'subnet_id': pub_subnet_id, - 'description': description}}) + response = neutron.create_floatingip(request) except n_exc.NeutronClientException as ex: - LOG.error("Failed to create floating IP - subnetid=%s ", - pub_subnet_id) + LOG.error("Failed to create floating IP - netid=%s ", pub_net_id) raise ex return response['floatingip']['id'], response[ 'floatingip']['floating_ip_address'] diff --git a/kuryr_kubernetes/controller/handlers/lbaas.py b/kuryr_kubernetes/controller/handlers/lbaas.py index 887a7a14a..64cf06038 100644 --- a/kuryr_kubernetes/controller/handlers/lbaas.py +++ b/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') diff --git a/kuryr_kubernetes/handlers/retry.py b/kuryr_kubernetes/handlers/retry.py index 38dedf28f..a4671a14e 100644 --- a/kuryr_kubernetes/handlers/retry.py +++ b/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() diff --git a/kuryr_kubernetes/tests/unit/controller/drivers/test_lb_public_ip.py b/kuryr_kubernetes/tests/unit/controller/drivers/test_lb_public_ip.py index bf54ea684..b5de2c2e2 100644 --- a/kuryr_kubernetes/tests/unit/controller/drivers/test_lb_public_ip.py +++ b/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 = '' - m_cfg.neutron_defaults.external_svc_subnet = public_subnet + public_net = '' + 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): - driver = d_lb_public_ip.FloatingIpServicePubIPDriver() + def test_acquire_service_pub_ip_info_pool_subnet_is_none(self, m_cfg): + 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 - m_cfg.neutron_defaults.external_svc_subnet = public_subnet + public_net = mock.sentinel.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( - kl_exc.NoResourceException, - driver.acquire_service_pub_ip_info, - spec_type, spec_lb_ip, project_id) + expected_resp = \ + obj_lbaas.LBaaSPubIp(ip_id=floating_ip['id'], + ip_addr=floating_ip['floating_ip_address'], + 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): diff --git a/kuryr_kubernetes/tests/unit/controller/drivers/test_public_ip.py b/kuryr_kubernetes/tests/unit/controller/drivers/test_public_ip.py index 77f2fc339..ffa7ba43f 100644 --- a/kuryr_kubernetes/tests/unit/controller/drivers/test_public_ip.py +++ b/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 diff --git a/kuryr_kubernetes/tests/unit/handlers/test_retry.py b/kuryr_kubernetes/tests/unit/handlers/test_retry.py index 849e4cacd..4e1be39d0 100644 --- a/kuryr_kubernetes/tests/unit/handlers/test_retry.py +++ b/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() diff --git a/releasenotes/notes/make-ext-subnet-optional-99e73bfcbde96c22.yaml b/releasenotes/notes/make-ext-subnet-optional-99e73bfcbde96c22.yaml new file mode 100644 index 000000000..12dc0e9e5 --- /dev/null +++ b/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= + # 'external_svc_subnet' field is optional, set this field in case + # multiple subnets attached to 'external_svc_net' + external_svc_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. \ No newline at end of file