From 5cf4b4177254272cd15885411194d033a188c0d7 Mon Sep 17 00:00:00 2001 From: Maysa Macedo Date: Wed, 13 Feb 2019 10:45:37 +0000 Subject: [PATCH] Fix CRD podSelector update When the podSelector of a NP is updated, the podSelector on the respective CRD must also be updated with the same value. However, this do not happen in case the field of a label is updated, for example: Label {'app: demo'} is updated to {'context:demo'} the result given is {'app: demo', 'context:demo'} when should be {'context:demo'}. And after that, if the updated label {'context:demo'} is removed from the NP, it will not be removed from the CRD. These cases happen because the podSelector field is a dict and not a list. This commit fixes the issue by changing the merge strategy to JSON Patch, instead of JSON Merge Patch. Change-Id: Ic629c1ba4ac13c2bfaffdf7f904b69abf9521ed3 Closes-Bug: 1810394 --- kuryr_kubernetes/controller/drivers/utils.py | 10 +++---- kuryr_kubernetes/k8s_client.py | 29 ++++++++++++++++--- .../controller/drivers/test_network_policy.py | 2 +- 3 files changed, 31 insertions(+), 10 deletions(-) diff --git a/kuryr_kubernetes/controller/drivers/utils.py b/kuryr_kubernetes/controller/drivers/utils.py index dc758cd09..6b5141d77 100644 --- a/kuryr_kubernetes/controller/drivers/utils.py +++ b/kuryr_kubernetes/controller/drivers/utils.py @@ -225,11 +225,11 @@ def patch_kuryr_crd(crd, i_rules, e_rules, pod_selector, np_spec=None): np_spec = crd['spec']['networkpolicy_spec'] LOG.debug('Patching KuryrNetPolicy CRD %s' % crd_name) try: - kubernetes.patch('spec', crd['metadata']['selfLink'], - {'ingressSgRules': i_rules, - 'egressSgRules': e_rules, - 'podSelector': pod_selector, - 'networkpolicy_spec': np_spec}) + kubernetes.patch_crd('spec', crd['metadata']['selfLink'], + {'ingressSgRules': i_rules, + 'egressSgRules': e_rules, + 'podSelector': pod_selector, + 'networkpolicy_spec': np_spec}) except k_exc.K8sClientException: LOG.exception('Error updating kuryrnetpolicy CRD %s', crd_name) raise diff --git a/kuryr_kubernetes/k8s_client.py b/kuryr_kubernetes/k8s_client.py index 11dfedc82..8cdc7027d 100644 --- a/kuryr_kubernetes/k8s_client.py +++ b/kuryr_kubernetes/k8s_client.py @@ -83,9 +83,9 @@ class K8sClient(object): result = response.json() if json else response.text return result - def _get_url_and_header(self, path): + def _get_url_and_header(self, path, content_type): url = self._base_url + path - header = {'Content-Type': 'application/merge-patch+json', + header = {'Content-Type': content_type, 'Accept': 'application/json'} if self.token: header.update({'Authorization': 'Bearer %s' % self.token}) @@ -97,7 +97,8 @@ class K8sClient(object): 'path': path, 'data': data}) if field == 'status': path = path + '/' + str(field) - url, header = self._get_url_and_header(path) + content_type = 'application/merge-patch+json' + url, header = self._get_url_and_header(path, content_type) response = requests.patch(url, json={field: data}, headers=header, cert=self.cert, verify=self.verify_server) @@ -105,6 +106,25 @@ class K8sClient(object): return response.json().get('status') raise exc.K8sClientException(response.text) + def patch_crd(self, field, path, data): + content_type = 'application/json-patch+json' + url, header = self._get_url_and_header(path, content_type) + + data = [{'op': 'replace', + 'path': '/{}/{}'.format(field, np_field), + 'value': value} + for np_field, value in data.items()] + + LOG.debug("Patch %(path)s: %(data)s", { + 'path': path, 'data': data}) + + response = requests.patch(url, data=jsonutils.dumps(data), + headers=header, cert=self.cert, + verify=self.verify_server) + if response.ok: + return response.json().get('status') + raise exc.K8sClientException(response.text) + def post(self, path, body): LOG.debug("Post %(path)s: %(body)s", {'path': path, 'body': body}) url = self._base_url + path @@ -142,7 +162,8 @@ class K8sClient(object): LOG.debug("Annotate %(path)s: %(names)s", { 'path': path, 'names': list(annotations)}) - url, header = self._get_url_and_header(path) + content_type = 'application/merge-patch+json' + url, header = self._get_url_and_header(path, content_type) while itertools.count(1): metadata = {"annotations": annotations} diff --git a/kuryr_kubernetes/tests/unit/controller/drivers/test_network_policy.py b/kuryr_kubernetes/tests/unit/controller/drivers/test_network_policy.py index 053e55edc..5716796fe 100644 --- a/kuryr_kubernetes/tests/unit/controller/drivers/test_network_policy.py +++ b/kuryr_kubernetes/tests/unit/controller/drivers/test_network_policy.py @@ -293,7 +293,7 @@ class TestNetworkPolicyDriver(test_base.TestCase): 'parse_network_policy_rules') def test_update_security_group_rules_with_k8s_exc(self, m_parse, m_get_crd, m_create_sgr): - self._driver.kubernetes.patch.side_effect = ( + self._driver.kubernetes.patch_crd.side_effect = ( exceptions.K8sClientException()) m_get_crd.return_value = self._crd m_parse.return_value = (self._i_rules, self._e_rules)