From b729cd383b43070bb2a366be2cc2bdcd803e7c1e Mon Sep 17 00:00:00 2001 From: a-dubs Date: Mon, 14 Oct 2024 12:44:59 -0400 Subject: [PATCH] fix(oracle): only get ip address from primary vnic Previously, the first vnic returned by the oci sdk is used to get the instance public IP from, but if a secondary vnic is created and attached after instance creation, this causes this logic to fail since the new secondary vnic is returned before the primary vnic by the oci sdk. This commit makes the ip property check for the is_primary property on all vnics before getting an IP address from the selected vnic. Also, this commit adds unit tests validating new IP property changes --- pycloudlib/oci/instance.py | 18 +- tests/unit_tests/oci/test_cloud.py | 66 +++---- tests/unit_tests/oci/test_instance.py | 252 +++++++++++++++++++++++++- 3 files changed, 285 insertions(+), 51 deletions(-) diff --git a/pycloudlib/oci/instance.py b/pycloudlib/oci/instance.py index 728bd732..fa11828a 100644 --- a/pycloudlib/oci/instance.py +++ b/pycloudlib/oci/instance.py @@ -76,17 +76,23 @@ def ip(self): vnic_attachment = self.compute_client.list_vnic_attachments( compartment_id=self.compartment_id, instance_id=self.instance_data.id, - ).data[0] - vnic_info = self.network_client.get_vnic(vnic_attachment.vnic_id).data + ).data + vnics = [ + self.network_client.get_vnic(vnic_attachment.vnic_id).data + for vnic_attachment in vnic_attachment + ] + # select vnic with is_primary = True + primary_vnic = [vnic for vnic in vnics if vnic.is_primary][0] # if not public IP, check for ipv6 - if vnic_info.public_ip is None: - if vnic_info.ipv6_addresses: - self._ip = vnic_info.ipv6_addresses[0] + # None is specifically returned by OCI when ipv6 only vnic + if primary_vnic.public_ip is None: + if primary_vnic.ipv6_addresses: + self._ip = primary_vnic.ipv6_addresses[0] self._log.info("Using ipv6 address: %s", self._ip) else: raise PycloudlibError("No public ipv4 address or ipv6 address found") else: - self._ip = vnic_info.public_ip + self._ip = primary_vnic.public_ip self._log.info("Using ipv4 address: %s", self._ip) return self._ip return self._ip diff --git a/tests/unit_tests/oci/test_cloud.py b/tests/unit_tests/oci/test_cloud.py index b6886809..833a65ac 100644 --- a/tests/unit_tests/oci/test_cloud.py +++ b/tests/unit_tests/oci/test_cloud.py @@ -6,7 +6,6 @@ import pytest import toml -from pycloudlib.config import Config from pycloudlib.errors import ( InstanceNotFoundError, PycloudlibException, @@ -16,14 +15,13 @@ @pytest.fixture -def oci_cloud(): +def oci_cloud(tmp_path): """ Fixture for OCI Cloud class. - + This fixture mocks the oci.config.validate_config function to not raise an error. It also mocks the oci.core.ComputeClient and oci.core.VirtualNetworkClient classes to return mock - instances of the clients. The fixture also mocks the pycloudlib.config.parse_config function - to return a Config object with the default values from the pycloudlib.toml.template file. + instances of the clients. """ oci_config = { "user": "ocid1.user.oc1..example", @@ -32,6 +30,8 @@ def oci_cloud(): "tenancy": "ocid1.tenancy.oc1..example", "region": "us-phoenix-1", } + pycloudlib_config = tmp_path / "pyproject.toml" + pycloudlib_config.write_text("[oci]\n") with mock.patch( "pycloudlib.oci.cloud.oci.config.validate_config", @@ -40,11 +40,7 @@ def oci_cloud(): "pycloudlib.oci.cloud.oci.core.ComputeClient" ) as mock_compute_client_class, mock.patch( "pycloudlib.oci.cloud.oci.core.VirtualNetworkClient" - ) as mock_network_client_class, mock.patch( - "pycloudlib.config.parse_config" - ) as mock_parse_config: - # mock loading the config file by just reading in the template, which we know will exist - mock_parse_config.return_value = toml.load("pycloudlib.toml.template", _dict=Config) + ) as mock_network_client_class: # Instantiate mocked clients mock_compute_client = mock.Mock() mock_network_client = mock.Mock() @@ -55,6 +51,7 @@ def oci_cloud(): oci_cloud = OCI( "test-instance", timestamp_suffix=True, + config_file=pycloudlib_config, availability_domain="PHX-AD-1", compartment_id="test-compartment-id", region="us-phoenix-1", @@ -70,21 +67,34 @@ def oci_cloud(): yield oci_cloud +OCI_PYCLOUDLIB_CONFIG = """\ +[oci] +availability_domain = "PYCL-AD-1" +compartment_id = "pycloudlib-compartment-id" +region = "pycl-pheonix-1" +""" + + class TestOciInit: """Tests for OCI Cloud __init__.""" def test_init_valid(self, oci_cloud): - """Test __init__ with valid parameters.""" - # Ensure that oci.config.from_file is mocked with a valid configuration + """Test __init__ with valid parameters matches .oci/config over pycloudlib.toml.""" assert oci_cloud.availability_domain == "PHX-AD-1" assert oci_cloud.compartment_id == "test-compartment-id" assert oci_cloud.region == "us-phoenix-1" - def test_init_invalid_config(self): - """Test __init__ with invalid OCI configuration.""" + def test_init_invalid_config(self, tmp_path): + """Test __init__ with invalid OCI configuration raises ValueError.""" + pycloudlib_config = tmp_path / "pyproject.toml" + pycloudlib_config.write_text(OCI_PYCLOUDLIB_CONFIG) with mock.patch("oci.config.from_file", side_effect=oci.exceptions.InvalidConfig): with pytest.raises(ValueError, match="Config dict is invalid"): - OCI(tag="test-instance", config_dict={"invalid": "config"}) + OCI( + tag="test-instance", + config_file=pycloudlib_config, + config_dict={"invalid": "config"}, + ) class TestOciImages: @@ -157,35 +167,16 @@ def test_snapshot(self, mock_wait_till_ready, oci_cloud): class TestOciInstances: """Tests for OCI Cloud instance-related functions.""" - @mock.patch("oci.config.from_file") - def test_get_instance(self, mock_from_file, oci_cloud): + def test_get_instance(self, oci_cloud): """Test get_instance method with valid instance.""" - valid_config = { - "user": "ocid1.user.oc1..example", - "fingerprint": "mock-fingerprint", - "key_file": "/path/to/key", - "tenancy": "ocid1.tenancy.oc1..example", - "region": "us-phoenix-1", - } - # Mock oci.config.from_file to return the valid configuration - mock_from_file.return_value = valid_config oci_cloud.compute_client.get_instance.return_value = mock.Mock() instance = oci_cloud.get_instance("test-instance-id", username="opc") assert isinstance(instance, OciInstance) oci_cloud.compute_client.get_instance.assert_called_once_with("test-instance-id") - @mock.patch("oci.config.from_file") - def test_get_instance_not_found(self, mock_from_file, oci_cloud): + def test_get_instance_not_found(self, oci_cloud): """Test get_instance raises InstanceNotFoundError when instance does not exist.""" - valid_config = { - "user": "ocid1.user.oc1..example", - "fingerprint": "mock-fingerprint", - "key_file": "/path/to/key", - "tenancy": "ocid1.tenancy.oc1..example", - "region": "us-phoenix-1", - } - mock_from_file.return_value = valid_config oci_cloud.compute_client.get_instance.side_effect = oci.exceptions.ServiceError( status=404, @@ -201,8 +192,7 @@ def test_get_instance_not_found(self, mock_from_file, oci_cloud): def test_launch_instance(self, mock_wait_till_ready, oci_cloud): """Test launch method with valid inputs.""" # mock the key pair - oci_cloud.key_pair = mock.Mock() - oci_cloud.key_pair.public_key_content = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC" + oci_cloud.key_pair = mock.Mock(public_key_config="ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC") oci_cloud.compute_client.launch_instance.return_value = mock.Mock( data=mock.Mock(id="instance-id") ) diff --git a/tests/unit_tests/oci/test_instance.py b/tests/unit_tests/oci/test_instance.py index 748346e3..7f552948 100644 --- a/tests/unit_tests/oci/test_instance.py +++ b/tests/unit_tests/oci/test_instance.py @@ -11,7 +11,7 @@ @pytest.fixture -def oci_instance(): +def oci_instance(tmp_path): """ Fixture for OciInstance class. @@ -33,6 +33,8 @@ def oci_instance(): "tenancy": "tenancy_ocid", "region": "us-phoenix-1", } + pycloudlib_config = tmp_path / "pyproject.toml" + pycloudlib_config.write_text("[oci]\n") with mock.patch( "pycloudlib.oci.instance.oci.config.from_file", @@ -41,12 +43,7 @@ def oci_instance(): "pycloudlib.oci.instance.oci.core.ComputeClient" ) as mock_compute_client_class, mock.patch( "pycloudlib.oci.instance.oci.core.VirtualNetworkClient" - ) as mock_network_client_class, mock.patch( - "pycloudlib.config.parse_config" - ) as mock_parse_config: - # mock loading the config file by just reading in the template, which we know will exist - mock_parse_config.return_value = toml.load("pycloudlib.toml.template", _dict=Config) - + ) as mock_network_client_class: # Instantiate mocked clients mock_compute_client = mock.Mock() mock_network_client = mock.Mock() @@ -232,3 +229,244 @@ def test_remove_network_interface_not_found(self, oci_instance, mock_vnic_pagina ): oci_instance.remove_network_interface("192.168.1.10") + +class TestOciInstanceIp: + """Tests for the ip property of OciInstance.""" + + def setup_mock_vnic(self, oci_instance, vnic_data_list): + """ + Mock VNIC attachments and VNIC data based on custom VNIC configurations. + + Args: + vnic_data_list (list): List of tuples containing VNIC data. + Each tuple should contain the following elements: + - vnic_id_suffix (str): Suffix to create a unique VNIC ID + - is_primary (bool): Whether the VNIC is primary + - public_ip (str): Public IPv4 address of the VNIC + - ipv6_addresses (list): List of IPv6 addresses of the VNIC + + The function sets up the mock behavior of the OCI instance to return the VNIC data + based on the vnic configuration provided in vnic_data_list. + """ + vnic_attachments = [] + vnics_data = {} + + for ( + vnic_id_suffix, + is_primary, + public_ip, + ipv6_addresses, + ) in vnic_data_list: + vnic_id = f"ocid1.vnic.oc1..vnicuniqueID{vnic_id_suffix}" + # Create VNIC attachment mock + vnic_attachment = mock.Mock() + vnic_attachment.vnic_id = vnic_id + vnic_attachments.append(vnic_attachment) + + # Create VNIC data mock + vnic = mock.Mock() + vnic.is_primary = is_primary + vnic.public_ip = public_ip + vnic.ipv6_addresses = ipv6_addresses + vnics_data[vnic_id] = mock.Mock(data=vnic) + + oci_instance.compute_client.list_vnic_attachments.return_value = mock.Mock( + data=vnic_attachments + ) + + # Mock get_vnic to return appropriate VNIC data based on vnic_id + def get_vnic_side_effect(vnic_id): + return vnics_data.get(vnic_id, mock.Mock(data=mock.Mock())) + + oci_instance.network_client.get_vnic.side_effect = get_vnic_side_effect + + @pytest.mark.parametrize( + [ + "primary_has_ipv4", + "primary_has_ipv6", + "secondary_has_ipv4", + "secondary_has_ipv6", + "primary_first", + "expected_ip", + "expect_error", + ], + [ + pytest.param( + True, # primary_has_ipv4 + False, # primary_has_ipv6 + False, # secondary_has_ipv4 + False, # secondary_has_ipv6 + True, # primary_first + "203.0.113.1", # expected_ip + False, # expect_error + id="Primary VNIC has IPv4 (primary first)", + ), + pytest.param( + False, # primary_has_ipv4 + True, # primary_has_ipv6 + False, # secondary_has_ipv4 + False, # secondary_has_ipv6 + False, # primary_first + "2001:db8::1", # expected_ip + False, # expect_error + id="Primary VNIC has IPv6 (primary not first)", + ), + pytest.param( + False, # primary_has_ipv4 + False, # primary_has_ipv6 + True, # secondary_has_ipv4 + False, # secondary_has_ipv6 + True, # primary_first + None, # expected_ip + True, # expect_error + id="Primary VNIC no IPs, secondary has IPv4 (expect error)", + ), + pytest.param( + False, # primary_has_ipv4 + False, # primary_has_ipv6 + False, # secondary_has_ipv4 + True, # secondary_has_ipv6 + False, # primary_first + None, # expected_ip + True, # expect_error + id="Primary VNIC no IPs, secondary has IPv6 (expect error)", + ), + pytest.param( + False, # primary_has_ipv4 + True, # primary_has_ipv6 + True, # secondary_has_ipv4 + False, # secondary_has_ipv6 + True, # primary_first + "2001:db8::1", # expected_ip + False, # expect_error + id="Primary IPv6, secondary IPv4", + ), + pytest.param( + True, # primary_has_ipv4 + True, # primary_has_ipv6 + False, # secondary_has_ipv4 + False, # secondary_has_ipv6 + True, # primary_first + "203.0.113.1", # expected_ip + False, # expect_error + id="Primary VNIC has both IPv4 and IPv6 (primary first)", + ), + pytest.param( + True, # primary_has_ipv4 + True, # primary_has_ipv6 + False, # secondary_has_ipv4 + False, # secondary_has_ipv6 + False, # primary_first + "203.0.113.1", # expected_ip + False, # expect_error + id="Primary VNIC has both IPv4 and IPv6 (primary not first)", + ), + pytest.param( + False, # primary_has_ipv4 + False, # primary_has_ipv6 + True, # secondary_has_ipv4 + True, # secondary_has_ipv6 + True, # primary_first + None, # expected_ip + True, # expect_error + id="Primary VNIC no IPs, secondary has both (expect error)", + ), + pytest.param( + True, # primary_has_ipv4 + False, # primary_has_ipv6 + True, # secondary_has_ipv4 + False, # secondary_has_ipv6 + False, # primary_first + "203.0.113.1", # expected_ip + False, # expect_error + id="Both VNICs have IPv4 (primary not first)", + ), + pytest.param( + False, # primary_has_ipv4 + False, # primary_has_ipv6 + False, # secondary_has_ipv4 + False, # secondary_has_ipv6 + True, # primary_first + None, # expected_ip + True, # expect_error + id="No VNICs have IPs (expect error)", + ), + pytest.param( + False, # primary_has_ipv4 + True, # primary_has_ipv6 + True, # secondary_has_ipv4 + False, # secondary_has_ipv6 + False, # primary_first + "2001:db8::1", # expected_ip + False, # expect_error + id="Multiple primary VNICs, primary not first (IPv6)", + ), + pytest.param( + True, # primary_has_ipv4 + False, # primary_has_ipv6 + False, # secondary_has_ipv4 + False, # secondary_has_ipv6 + False, # primary_first + "203.0.113.1", # expected_ip + False, # expect_error + id="Primary VNIC has IPv4 (primary not first)", + ), + ], + ) + def test_oci_instance_ip_parametrized( + self, + oci_instance, + primary_has_ipv4, + primary_has_ipv6, + secondary_has_ipv4, + secondary_has_ipv6, + primary_first, + expected_ip, + expect_error, + ): + """ + Test the ip property of OciInstance with various VNIC configurations. + + By testing a matrix of VNIC configurations, this test ensures that the ip property + of OciInstance behaves as expected in various scenarios, testing all meaningful + combinations of the following conditions: + - if the primary VNIC has an IPv4 address + - if the primary VNIC has an IPv6 address + - if the secondary VNIC has an IPv4 address + - if the secondary VNIC has an IPv6 address + - if the primary VNIC is listed first + """ + # Prepare VNIC configurations + # Primary VNIC configuration + primary_vnic = ( + "1", # vnic_id_suffix + True, # is_primary + "203.0.113.1" if primary_has_ipv4 else None, # public_ip + ["2001:db8::1"] if primary_has_ipv6 else [], # ipv6_addresses + ) + + # Secondary VNIC configuration + secondary_vnic = ( + "2", # vnic_id_suffix + False, # is_primary + "203.0.113.2" if secondary_has_ipv4 else None, # public_ip + ["2001:db8::2"] if secondary_has_ipv6 else [], # ipv6_addresses + ) + + # Arrange VNICs based on primary_first flag + if primary_first: + vnics_ordered = [primary_vnic, secondary_vnic] + else: + vnics_ordered = [secondary_vnic, primary_vnic] + + # Setup mock VNICs + self.setup_mock_vnic(oci_instance, vnics_ordered) + + if expect_error: + with pytest.raises( + PycloudlibError, + match="No public ipv4 address or ipv6 address found", + ): + _ = oci_instance.ip + else: + assert oci_instance.ip == expected_ip