Download certificates when goal state source is Fast Track #2761
Conversation
maddieford
requested review from
narrieta,
ZhidongPeng and
nagworld9
as code owners
Codecov Report
@@ Coverage Diff @@
## develop #2761 +/- ##
========================================
Coverage 71.99% 71.99%
========================================
Files 104 104
Lines 15831 15839 +8
Branches 2264 2265 +1
========================================
+ Hits 11397 11404 +7
Misses 3913 3913
- Partials 521 522 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| @@ -412,7 +412,7 @@ def http_get_handler(url, *_, **__): | |||
| goal_state = GoalState(protocol.client) | |||
|
|
|||
| self.assertEqual(2, protocol.mock_wire_data.call_counts['goalstate'], "There should have been exactly 2 requests for the goal state (original + refresh)") | |||
| self.assertEqual(2, http_get_handler.certificate_requests, "There should have been exactly 2 requests for the goal state certificates (original + refresh)") | |||
| self.assertEqual(4, http_get_handler.certificate_requests, "There should have been exactly 4 requests for the goal state certificates (2x original + 2x refresh)") | |||
There was a problem hiding this comment.
The previous 2 requests became 4 requests for this case because we're downloading certificates in _fetch_full_wire_server_goal_state and _update directly.
Should I add logic to only download certificates once per goals state?
| @@ -156,7 +156,7 @@ def http_get_handler(url, *_, **__): | |||
| protocol.set_http_handlers(http_get_handler=None) | |||
| goal_state.update() | |||
| self._assert_directory_contents( | |||
| self._find_history_subdirectory("234-987"), ["VmSettings.json"]) | |||
| self._find_history_subdirectory("234-987"), ["VmSettings.json", "Certificates.json"]) | |||
There was a problem hiding this comment.
This test shows that we create history subdirectory with Certificates.json when we have a fast track goal state. Should I add an additional test to verify the certificates are downloaded?
| @@ -1101,7 +1101,7 @@ def test_forced_update_should_update_the_goal_state_and_the_host_plugin_when_the | |||
|
|
|||
| def test_reset_should_init_provided_goal_state_properties(self): | |||
| with mock_wire_protocol(mockwiredata.DATA_FILE) as protocol: | |||
| protocol.client.reset_goal_state(goal_state_properties=GoalStateProperties.All & ~GoalStateProperties.ExtensionsGoalState) | |||
| protocol.client.reset_goal_state(goal_state_properties=GoalStateProperties.All & ~GoalStateProperties.Certificates) | |||
There was a problem hiding this comment.
I updated this test after separating ExtensionsGoalState and Certificates in GoalStateProperties.
| @@ -289,6 +290,9 @@ def _update(self, force_update): | |||
| # case, to ensure it fetches the new certificate. | |||
| # | |||
| if self._extensions_goal_state.source == GoalStateSource.FastTrack: | |||
| certs_uri = findtext(xml_doc, "Certificates") | |||
| if certs_uri is not None: | |||
| self._download_certificates(certs_uri) | |||
There was a problem hiding this comment.
In case if we downloaded new certs then we are not updating the state of goal_state certs in self._certs. Next line check certificates is using self._certs which will have old certs. Or am I missing something?
| @@ -289,9 +290,14 @@ def _update(self, force_update): | |||
| # case, to ensure it fetches the new certificate. | |||
| # | |||
| if self._extensions_goal_state.source == GoalStateSource.FastTrack: | |||
| self._check_certificates() | |||
| certs_uri = findtext(xml_doc, "Certificates") | |||
There was a problem hiding this comment.
Minor: let's add certs_uri as a data member of self and use that instead of picking it up from the xml_doc we just fetched. With the current code, this makes no difference functionally, but let's do that in case the code changes. That would also emphasize that we are redownloading the same certs as we did before.
Also, we should re-download only if self._goal_state_properties & GoalStateProperties.Certificates != 0
This PR includes a change to download certificates when goal state source is FastTrack
Fixes #2750