DataSourceVMware: fix var use before init#1674
Merged
Merged
Conversation
akutz
force-pushed
the
fix/lp-1987005
branch
2 times, most recently
from
da38cb2 to
9c916ba
Compare
blackboxsw
reviewed
Aug 22, 2022
blackboxsw
left a comment
blackboxsw
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. Ideally we'd want a unittest exercising the logic that lead to exposing this missed variable initialization failure,
Contributor
Author
Np. I thought about it, but given that it would be proving a panic no longer exists, I was not sure if it was worth it beyond the manual demonstration. Honestly, tests to validate some error no longer occurs are always something that strike me as odd, and I'm never sure how to best represent/surface the absence of something. |
This patch fixes an issue in the DataSourceVMware code where the variable ipv6_ready was used in a logging statement before it was initialized. Now the variable is initialized, avoiding the panic. Fixes https://bugs.launchpad.net/cloud-init/+bug/1987005
Contributor
Author
|
Okay @blackboxsw, the unit test has been added :) |
blackboxsw
reviewed
Aug 22, 2022
blackboxsw
left a comment
blackboxsw
left a comment
Collaborator
There was a problem hiding this comment.
Here's a suggestion for some more unittest coverage.
diff --git a/tests/unittests/sources/test_vmware.py b/tests/unittests/sources/test_vmware.py
index 37a1f259d..57dfceff2 100644
--- a/tests/unittests/sources/test_vmware.py
+++ b/tests/unittests/sources/test_vmware.py
@@ -57,6 +57,8 @@ runcmd:
- echo "Hello, world."
"""
+M_PATH = "cloudinit.sources.DataSourceVMware."
+
@pytest.fixture(autouse=True)
def common_patches():
@@ -67,14 +69,8 @@ def common_patches():
is_container=mock.Mock(return_value=False),
is_FreeBSD=mock.Mock(return_value=False),
),
- mock.patch(
- "cloudinit.sources.DataSourceVMware.netifaces.interfaces",
- return_value=[],
- ),
- mock.patch(
- "cloudinit.sources.DataSourceVMware.getfqdn",
- return_value="host.cloudinit.test",
- ),
+ mock.patch(M_PATH + "netifaces.interfaces", return_value=[]),
+ mock.patch(M_PATH + "getfqdn", return_value="host.cloudinit.test"),
]
with ExitStack() as stack:
for some_mock in mocks:
@@ -97,7 +93,7 @@ class TestDataSourceVMware(CiTestCase):
ret = ds.get_data()
self.assertFalse(ret)
- @mock.patch("cloudinit.sources.DataSourceVMware.get_default_ip_addrs")
+ @mock.patch(M_PATH + "get_default_ip_addrs")
def test_get_host_info_ipv4(self, m_fn_ipaddr):
m_fn_ipaddr.return_value = ("10.10.10.1", None)
host_info = DataSourceVMware.get_host_info()
@@ -110,7 +106,7 @@ class TestDataSourceVMware(CiTestCase):
self.assertTrue(host_info[DataSourceVMware.LOCAL_IPV4] == "10.10.10.1")
self.assertFalse(host_info.get(DataSourceVMware.LOCAL_IPV6))
- @mock.patch("cloudinit.sources.DataSourceVMware.get_default_ip_addrs")
+ @mock.patch(M_PATH + "get_default_ip_addrs")
def test_get_host_info_ipv6(self, m_fn_ipaddr):
m_fn_ipaddr.return_value = (None, "2001:db8::::::8888")
host_info = DataSourceVMware.get_host_info()
@@ -125,7 +121,7 @@ class TestDataSourceVMware(CiTestCase):
)
self.assertFalse(host_info.get(DataSourceVMware.LOCAL_IPV4))
- @mock.patch("cloudinit.sources.DataSourceVMware.get_default_ip_addrs")
+ @mock.patch(M_PATH + "get_default_ip_addrs")
def test_get_host_info_dual(self, m_fn_ipaddr):
m_fn_ipaddr.return_value = ("10.10.10.1", "2001:db8::::::8888")
host_info = DataSourceVMware.get_host_info()
@@ -182,9 +178,7 @@ class TestDataSourceVMwareEnvVars(FilesystemMockingTestCase):
ds = self.assert_get_data_ok(m_fn, m_fn_call_count)
assert_metadata(self, ds, metadata)
- @mock.patch(
- "cloudinit.sources.DataSourceVMware.guestinfo_envvar_get_value"
- )
+ @mock.patch(M_PATH + "guestinfo_envvar_get_value")
def test_get_subplatform(self, m_fn):
m_fn.side_effect = [VMW_METADATA_YAML, "", "", "", "", ""]
ds = self.assert_get_data_ok(m_fn, m_fn_call_count=4)
@@ -197,46 +191,34 @@ class TestDataSourceVMwareEnvVars(FilesystemMockingTestCase):
),
)
- @mock.patch(
- "cloudinit.sources.DataSourceVMware.guestinfo_envvar_get_value"
- )
+ @mock.patch(M_PATH + "guestinfo_envvar_get_value")
def test_get_data_metadata_only(self, m_fn):
m_fn.side_effect = [VMW_METADATA_YAML, "", "", "", "", ""]
self.assert_get_data_ok(m_fn, m_fn_call_count=4)
- @mock.patch(
- "cloudinit.sources.DataSourceVMware.guestinfo_envvar_get_value"
- )
+ @mock.patch(M_PATH + "guestinfo_envvar_get_value")
def test_get_data_userdata_only(self, m_fn):
m_fn.side_effect = ["", VMW_USERDATA_YAML, "", ""]
self.assert_get_data_ok(m_fn, m_fn_call_count=4)
- @mock.patch(
- "cloudinit.sources.DataSourceVMware.guestinfo_envvar_get_value"
- )
+ @mock.patch(M_PATH + "guestinfo_envvar_get_value")
def test_get_data_vendordata_only(self, m_fn):
m_fn.side_effect = ["", "", VMW_VENDORDATA_YAML, ""]
self.assert_get_data_ok(m_fn, m_fn_call_count=4)
- @mock.patch(
- "cloudinit.sources.DataSourceVMware.guestinfo_envvar_get_value"
- )
+ @mock.patch(M_PATH + "guestinfo_envvar_get_value")
def test_get_data_metadata_base64(self, m_fn):
data = base64.b64encode(VMW_METADATA_YAML.encode("utf-8"))
m_fn.side_effect = [data, "base64", "", ""]
self.assert_get_data_ok(m_fn, m_fn_call_count=4)
- @mock.patch(
- "cloudinit.sources.DataSourceVMware.guestinfo_envvar_get_value"
- )
+ @mock.patch(M_PATH + "guestinfo_envvar_get_value")
def test_get_data_metadata_b64(self, m_fn):
data = base64.b64encode(VMW_METADATA_YAML.encode("utf-8"))
m_fn.side_effect = [data, "b64", "", ""]
self.assert_get_data_ok(m_fn, m_fn_call_count=4)
- @mock.patch(
- "cloudinit.sources.DataSourceVMware.guestinfo_envvar_get_value"
- )
+ @mock.patch(M_PATH + "guestinfo_envvar_get_value")
def test_get_data_metadata_gzip_base64(self, m_fn):
data = VMW_METADATA_YAML.encode("utf-8")
data = gzip.compress(data)
@@ -244,9 +226,7 @@ class TestDataSourceVMwareEnvVars(FilesystemMockingTestCase):
m_fn.side_effect = [data, "gzip+base64", "", ""]
self.assert_get_data_ok(m_fn, m_fn_call_count=4)
- @mock.patch(
- "cloudinit.sources.DataSourceVMware.guestinfo_envvar_get_value"
- )
+ @mock.patch(M_PATH + "guestinfo_envvar_get_value")
def test_get_data_metadata_gz_b64(self, m_fn):
data = VMW_METADATA_YAML.encode("utf-8")
data = gzip.compress(data)
@@ -254,9 +234,7 @@ class TestDataSourceVMwareEnvVars(FilesystemMockingTestCase):
m_fn.side_effect = [data, "gz+b64", "", ""]
self.assert_get_data_ok(m_fn, m_fn_call_count=4)
- @mock.patch(
- "cloudinit.sources.DataSourceVMware.guestinfo_envvar_get_value"
- )
+ @mock.patch(M_PATH + "guestinfo_envvar_get_value")
def test_metadata_single_ssh_key(self, m_fn):
metadata = DataSourceVMware.load_json_or_yaml(VMW_METADATA_YAML)
metadata["public_keys"] = VMW_SINGLE_KEY
@@ -264,9 +242,7 @@ class TestDataSourceVMwareEnvVars(FilesystemMockingTestCase):
m_fn.side_effect = [metadata_yaml, "", "", ""]
self.assert_metadata(metadata, m_fn, m_fn_call_count=4)
- @mock.patch(
- "cloudinit.sources.DataSourceVMware.guestinfo_envvar_get_value"
- )
+ @mock.patch(M_PATH + "guestinfo_envvar_get_value")
def test_metadata_multiple_ssh_keys(self, m_fn):
metadata = DataSourceVMware.load_json_or_yaml(VMW_METADATA_YAML)
metadata["public_keys"] = VMW_MULTIPLE_KEYS
@@ -316,7 +292,7 @@ class TestDataSourceVMwareGuestInfo(FilesystemMockingTestCase):
system_type = dmi.read_dmi_data("system-product-name")
self.assertEqual(system_type, PRODUCT_NAME)
- @mock.patch("cloudinit.sources.DataSourceVMware.guestinfo_get_value")
+ @mock.patch(M_PATH + "guestinfo_get_value")
def test_get_subplatform(self, m_fn):
m_fn.side_effect = [VMW_METADATA_YAML, "", "", "", "", ""]
ds = self.assert_get_data_ok(m_fn, m_fn_call_count=4)
@@ -329,17 +305,17 @@ class TestDataSourceVMwareGuestInfo(FilesystemMockingTestCase):
),
)
- @mock.patch("cloudinit.sources.DataSourceVMware.guestinfo_get_value")
+ @mock.patch(M_PATH + "guestinfo_get_value")
def test_get_data_userdata_only(self, m_fn):
m_fn.side_effect = ["", VMW_USERDATA_YAML, "", ""]
self.assert_get_data_ok(m_fn, m_fn_call_count=4)
- @mock.patch("cloudinit.sources.DataSourceVMware.guestinfo_get_value")
+ @mock.patch(M_PATH + "guestinfo_get_value")
def test_get_data_vendordata_only(self, m_fn):
m_fn.side_effect = ["", "", VMW_VENDORDATA_YAML, ""]
self.assert_get_data_ok(m_fn, m_fn_call_count=4)
- @mock.patch("cloudinit.sources.DataSourceVMware.guestinfo_get_value")
+ @mock.patch(M_PATH + "guestinfo_get_value")
def test_metadata_single_ssh_key(self, m_fn):
metadata = DataSourceVMware.load_json_or_yaml(VMW_METADATA_YAML)
metadata["public_keys"] = VMW_SINGLE_KEY
@@ -347,7 +323,7 @@ class TestDataSourceVMwareGuestInfo(FilesystemMockingTestCase):
m_fn.side_effect = [metadata_yaml, "", "", ""]
self.assert_metadata(metadata, m_fn, m_fn_call_count=4)
- @mock.patch("cloudinit.sources.DataSourceVMware.guestinfo_get_value")
+ @mock.patch(M_PATH + "guestinfo_get_value")
def test_metadata_multiple_ssh_keys(self, m_fn):
metadata = DataSourceVMware.load_json_or_yaml(VMW_METADATA_YAML)
metadata["public_keys"] = VMW_MULTIPLE_KEYS
@@ -355,19 +331,19 @@ class TestDataSourceVMwareGuestInfo(FilesystemMockingTestCase):
m_fn.side_effect = [metadata_yaml, "", "", ""]
self.assert_metadata(metadata, m_fn, m_fn_call_count=4)
- @mock.patch("cloudinit.sources.DataSourceVMware.guestinfo_get_value")
+ @mock.patch(M_PATH + "guestinfo_get_value")
def test_get_data_metadata_base64(self, m_fn):
data = base64.b64encode(VMW_METADATA_YAML.encode("utf-8"))
m_fn.side_effect = [data, "base64", "", ""]
self.assert_get_data_ok(m_fn, m_fn_call_count=4)
- @mock.patch("cloudinit.sources.DataSourceVMware.guestinfo_get_value")
+ @mock.patch(M_PATH + "guestinfo_get_value")
def test_get_data_metadata_b64(self, m_fn):
data = base64.b64encode(VMW_METADATA_YAML.encode("utf-8"))
m_fn.side_effect = [data, "b64", "", ""]
self.assert_get_data_ok(m_fn, m_fn_call_count=4)
- @mock.patch("cloudinit.sources.DataSourceVMware.guestinfo_get_value")
+ @mock.patch(M_PATH + "guestinfo_get_value")
def test_get_data_metadata_gzip_base64(self, m_fn):
data = VMW_METADATA_YAML.encode("utf-8")
data = gzip.compress(data)
@@ -375,7 +351,7 @@ class TestDataSourceVMwareGuestInfo(FilesystemMockingTestCase):
m_fn.side_effect = [data, "gzip+base64", "", ""]
self.assert_get_data_ok(m_fn, m_fn_call_count=4)
- @mock.patch("cloudinit.sources.DataSourceVMware.guestinfo_get_value")
+ @mock.patch(M_PATH + "guestinfo_get_value")
def test_get_data_metadata_gz_b64(self, m_fn):
data = VMW_METADATA_YAML.encode("utf-8")
data = gzip.compress(data)
@@ -404,7 +380,7 @@ class TestDataSourceVMwareGuestInfo_InvalidPlatform(FilesystemMockingTestCase):
)
self.assertTrue(self.reRoot(rootd))
- @mock.patch("cloudinit.sources.DataSourceVMware.guestinfo_get_value")
+ @mock.patch(M_PATH + "guestinfo_get_value")
def test_ds_invalid_on_non_vmware_platform(self, m_fn):
system_type = dmi.read_dmi_data("system-product-name")
self.assertEqual(system_type, None)
@@ -438,4 +414,105 @@ def get_ds(temp_dir):
return ds
+HOST_INFO_NO_IPS = {
+ "network": {"interfaces": {"by-mac": {}, "by-ipv4": {}, "by-ipv6": {}}},
+ "hostname": "host.cloudinit.test",
+ "local-hostname": "host.cloudinit.test",
+ "local_hostname": "host.cloudinit.test",
+ "local-ipv6": "2001:db8::::::8888",
+}
+
+HOST_INFO_IPV4 = {
+ "network": {
+ "interfaces": {
+ "by-mac": {},
+ "by-ipv4": {
+ "192.168.1.201": {
+ "netmask": "255.255.255.0",
+ "broadcast": "192.168.1.255",
+ "mac": "50:7b:9d:2c:af:91",
+ }
+ },
+ "by-ipv6": {},
+ }
+ },
+ "hostname": "host.cloudinit.test",
+ "local-hostname": "host.cloudinit.test",
+ "local_hostname": "host.cloudinit.test",
+ "local-ipv6": "2001:db8::::::8888",
+}
+
+HOST_INFO_IPV6 = {
+ "network": {
+ "interfaces": {
+ "by-mac": {},
+ "by-ipv4": {},
+ "by-ipv6": {
+ "fd42:fc73:a2ea:bdfb::1": {
+ "netmask": "ffff:ffff:ffff:ffff::",
+ "mac": "00:16:3e:62:b5:68",
+ },
+ },
+ }
+ },
+ "hostname": "host.cloudinit.test",
+ "local-hostname": "host.cloudinit.test",
+ "local_hostname": "host.cloudinit.test",
+ "local-ipv6": "2001:db8::::::8888",
+}
+
+
+@mock.patch(M_PATH + "time.sleep")
+@mock.patch(M_PATH + "get_host_info")
+class TestWaitOnNetwork:
+ @pytest.mark.parametrize(
+ "metadata,host_info_values,expected",
+ (
+ pytest.param( # No wait-on-network, no interfaces up, no retry
+ {},
+ (HOST_INFO_NO_IPS,),
+ HOST_INFO_NO_IPS,
+ id="no_wait_on_network_returns_host_info_no_retries",
+ ),
+ pytest.param( # wait-on-network, no ipv4/6 no retry
+ {"wait-on-network": {}},
+ (HOST_INFO_NO_IPS,),
+ HOST_INFO_NO_IPS,
+ id="wait_on_network_no_ipv46_returns_host_info_no_retries",
+ ),
+ pytest.param( # wait-on-network, ipv4/6 don't wait
+ {"wait-on-network": {"ipv4": False, "ipv6": False}},
+ (HOST_INFO_NO_IPS,),
+ HOST_INFO_NO_IPS,
+ id="wait_on_network_ipv46_false_returns_host_info_no_retries",
+ ),
+ pytest.param( # wait-on-network, ipv4, retry
+ {"wait-on-network": {"ipv4": True, "ipv6": False}},
+ (HOST_INFO_NO_IPS, HOST_INFO_IPV4),
+ HOST_INFO_IPV4,
+ id="no_wait_on_network_ipv4_true_returns_host_info_1_retry",
+ ),
+ pytest.param( # wait-on-network, ipv6, retry
+ {"wait-on-network": {"ipv4": False, "ipv6": True}},
+ (HOST_INFO_NO_IPS, HOST_INFO_IPV4, HOST_INFO_IPV6),
+ HOST_INFO_IPV6,
+ id="no_wait_on_network_ipv6_true_returns_host_info_2_retries",
+ ),
+ ),
+ )
+ def test_wait_on_network_loops_on_ipv4_ipv6(
+ self, get_host_info, sleep, metadata, host_info_values, expected
+ ):
+ if len(host_info_values) == 1:
+ get_host_info.return_value = host_info_values[0]
+ sleep_count = 0
+ else:
+ get_host_info.side_effect = host_info_values
+ sleep_count = len(host_info_values) - 1
+ assert expected == DataSourceVMware.wait_on_network(metadata)
+ # Only loop and call get_host_info until expected wait success
+ assert get_host_info.call_count == len(host_info_values)
+ assert sleep.call_count == sleep_count
+
+
# vi: ts=4 expandtab
Collaborator
@akutz you beat me too it. I felt 'dirty' suggesting something like unit test coverage for this. The only concern I had was that we had no current unittest coverage at all VMware.wait_on_network which would have shown us this type of error earlier |
blackboxsw
reviewed
Aug 22, 2022
blackboxsw
left a comment
blackboxsw
left a comment
Collaborator
There was a problem hiding this comment.
your changes are great and cover the use case I was looking for. At least that we properly get through wait_on_network calls without falling into the panic/exception path. We can build on this in the future as new logic is added and at least get coverage of that function to assert we didn't botch something that'll only show up in runtime.
blackboxsw
approved these changes
Aug 22, 2022
blackboxsw
left a comment
blackboxsw
left a comment
Collaborator
There was a problem hiding this comment.
Works for me Thanks @akutz for fast turnaround here on unittests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed Commit Message
Additional Context
Test Steps
I reproduced the bug by:
applying the following patch to move the logging statement into a position where it would always be executed:
and then running the following command locally:
then I applied the following patch on top of the one in this PR:
and tried to reproduce the issue once more:
Everything worked as expected!
I also added a unit test,
tests/unittests/sources/test_vmware.py::TestDataSourceVMware::test_wait_on_network:Checklist: