Skip to content

Commit

Permalink
fix(oracle): only get ip address from primary vnic
Browse files Browse the repository at this point in the history
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
  • Loading branch information
a-dubs committed Oct 16, 2024
1 parent b24c14a commit b729cd3
Show file tree
Hide file tree
Showing 3 changed files with 285 additions and 51 deletions.
18 changes: 12 additions & 6 deletions pycloudlib/oci/instance.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
66 changes: 28 additions & 38 deletions tests/unit_tests/oci/test_cloud.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import pytest
import toml

from pycloudlib.config import Config
from pycloudlib.errors import (
InstanceNotFoundError,
PycloudlibException,
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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()
Expand All @@ -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",
Expand All @@ -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:
Expand Down Expand Up @@ -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,
Expand All @@ -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")
)
Expand Down
Loading

0 comments on commit b729cd3

Please sign in to comment.