From cab811b89030d6e49f8ef85e29783fce94c763fc Mon Sep 17 00:00:00 2001
From: Chris Dent <cdent@anticdent.org>
Date: Fri, 2 Feb 2018 15:34:59 +0000
Subject: [PATCH] Don't rely on parse.urlencode in url comparisons

The tests for allocation_candidates query parameters in test_report.py
relied on urlencode with a dict arg to create the expected_url that is
compared with the actual URL that the report client creates when
communicating with the placement service.

This is risky because the ordering of the query parameters is
not reliable when the source data is a dict. This change expands
the tests where it was used to do more explicit comparisons.

Change-Id: I2e51a4574b20c0634ad83a53c0e68261bbf0ac82
Closes-Bug: #1747001
---
 .../unit/scheduler/client/test_report.py      | 41 +++++++++++++------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/nova/tests/unit/scheduler/client/test_report.py b/nova/tests/unit/scheduler/client/test_report.py
index 777e126643e4..3970e7317fbe 100644
--- a/nova/tests/unit/scheduler/client/test_report.py
+++ b/nova/tests/unit/scheduler/client/test_report.py
@@ -1497,6 +1497,10 @@ class TestProviderOperations(SchedulerReportClientTestCase):
             'trait:CUSTOM_TRAIT1': 'required',
             'trait:CUSTOM_TRAIT2': 'preferred',
         })
+        expected_path = '/allocation_candidates'
+        expected_query = {'resources': ['MEMORY_MB:1024,VCPU:1'],
+                          'required': ['CUSTOM_TRAIT1'],
+                          'limit': ['1000']}
 
         resp_mock.json.return_value = json_data
         self.ks_adap_mock.get.return_value = resp_mock
@@ -1504,12 +1508,13 @@ class TestProviderOperations(SchedulerReportClientTestCase):
         alloc_reqs, p_sums, allocation_request_version = \
                 self.client.get_allocation_candidates(resources)
 
-        expected_url = '/allocation_candidates?%s' % parse.urlencode(
-            {'resources': 'MEMORY_MB:1024,VCPU:1',
-             'required': 'CUSTOM_TRAIT1',
-             'limit': 1000})
         self.ks_adap_mock.get.assert_called_once_with(
-            expected_url, raise_exc=False, microversion='1.17')
+            mock.ANY, raise_exc=False, microversion='1.17')
+        url = self.ks_adap_mock.get.call_args[0][0]
+        split_url = parse.urlsplit(url)
+        query = parse.parse_qs(split_url.query)
+        self.assertEqual(expected_path, split_url.path)
+        self.assertEqual(expected_query, query)
         self.assertEqual(mock.sentinel.alloc_reqs, alloc_reqs)
         self.assertEqual(mock.sentinel.p_sums, p_sums)
 
@@ -1524,6 +1529,9 @@ class TestProviderOperations(SchedulerReportClientTestCase):
             'resources:MEMORY_MB': '1024',
             'resources1:DISK_GB': '30',
         })
+        expected_path = '/allocation_candidates'
+        expected_query = {'resources': ['MEMORY_MB:1024,VCPU:1'],
+                          'limit': ['1000']}
 
         resp_mock.json.return_value = json_data
         self.ks_adap_mock.get.return_value = resp_mock
@@ -1531,11 +1539,13 @@ class TestProviderOperations(SchedulerReportClientTestCase):
         alloc_reqs, p_sums, allocation_request_version = \
                 self.client.get_allocation_candidates(resources)
 
-        expected_url = '/allocation_candidates?%s' % parse.urlencode(
-            {'resources': 'MEMORY_MB:1024,VCPU:1',
-             'limit': 1000})
         self.ks_adap_mock.get.assert_called_once_with(
-            expected_url, raise_exc=False, microversion='1.17')
+            mock.ANY, raise_exc=False, microversion='1.17')
+        url = self.ks_adap_mock.get.call_args[0][0]
+        split_url = parse.urlsplit(url)
+        query = parse.parse_qs(split_url.query)
+        self.assertEqual(expected_path, split_url.path)
+        self.assertEqual(expected_query, query)
         self.assertEqual(mock.sentinel.alloc_reqs, alloc_reqs)
 
     def test_get_allocation_candidates_not_found(self):
@@ -1543,6 +1553,9 @@ class TestProviderOperations(SchedulerReportClientTestCase):
         # API doesn't find a resource provider matching a UUID
         resp_mock = mock.Mock(status_code=404)
         self.ks_adap_mock.get.return_value = resp_mock
+        expected_path = '/allocation_candidates'
+        expected_query = {'resources': ['MEMORY_MB:1024'],
+                          'limit': ['100']}
 
         # Make sure we're also honoring the configured limit
         self.flags(max_placement_results=100, group='scheduler')
@@ -1552,11 +1565,13 @@ class TestProviderOperations(SchedulerReportClientTestCase):
 
         res = self.client.get_allocation_candidates(resources)
 
-        expected_url = ('/allocation_candidates?%s' % parse.urlencode(
-            {'resources': 'MEMORY_MB:1024',
-             'limit': '100'}))
         self.ks_adap_mock.get.assert_called_once_with(
-            expected_url, raise_exc=False, microversion='1.17')
+            mock.ANY, raise_exc=False, microversion='1.17')
+        url = self.ks_adap_mock.get.call_args[0][0]
+        split_url = parse.urlsplit(url)
+        query = parse.parse_qs(split_url.query)
+        self.assertEqual(expected_path, split_url.path)
+        self.assertEqual(expected_query, query)
         self.assertIsNone(res[0])
 
     def test_get_resource_provider_found(self):