Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support Oracle IMDSv2 API #528

Merged
merged 7 commits into from
Aug 13, 2020
Merged

Support Oracle IMDSv2 API #528

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3426289
Support Oracle IMDSv2 API
TheRealFalcon Jul 29, 2020
229f965
review comments
TheRealFalcon Aug 12, 2020
c90cef8
fix httpretty issue with headers
TheRealFalcon Aug 12, 2020
c2c2ba4
review comments
TheRealFalcon Aug 13, 2020
ef0e983
Merge branch 'master' into oracle-v2
OddBloke Aug 13, 2020
ce5b6b1
Merge branch 'master' into oracle-v2
OddBloke Aug 13, 2020
aea3173
Merge branch 'master' into oracle-v2
OddBloke Aug 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
227 changes: 131 additions & 96 deletions cloudinit/sources/DataSourceOracle.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@
"""

import base64
import json
from collections import namedtuple
from contextlib import suppress as noop

from cloudinit import log as logging
from cloudinit import net, sources, util
Expand All @@ -24,7 +25,7 @@
get_interfaces_by_mac,
is_netfail_master,
)
from cloudinit.url_helper import readurl
from cloudinit.url_helper import UrlError, readurl

LOG = logging.getLogger(__name__)

Expand All @@ -33,80 +34,13 @@
'configure_secondary_nics': False,
}
CHASSIS_ASSET_TAG = "OracleCloud.com"
METADATA_ROOT = "http://169.254.169.254/opc/v1/"
METADATA_ENDPOINT = METADATA_ROOT + "instance/"
VNIC_METADATA_URL = METADATA_ROOT + "vnics/"
METADATA_ROOT = "http://169.254.169.254/opc/v{version}/"
METADATA_PATTERN = METADATA_ROOT + "{path}/"
# https://docs.cloud.oracle.com/iaas/Content/Network/Troubleshoot/connectionhang.htm#Overview,
# indicates that an MTU of 9000 is used within OCI
MTU = 9000


def _add_network_config_from_opc_imds(network_config):
"""
Fetch data from Oracle's IMDS, generate secondary NIC config, merge it.

The primary NIC configuration should not be modified based on the IMDS
values, as it should continue to be configured for DHCP. As such, this
takes an existing network_config dict which is expected to have the primary
NIC configuration already present. It will mutate the given dict to
include the secondary VNICs.

:param network_config:
A v1 or v2 network config dict with the primary NIC already configured.
This dict will be mutated.

:raises:
Exceptions are not handled within this function. Likely exceptions are
those raised by url_helper.readurl (if communicating with the IMDS
fails), ValueError/JSONDecodeError (if the IMDS returns invalid JSON),
and KeyError/IndexError (if the IMDS returns valid JSON with unexpected
contents).
"""
resp = readurl(VNIC_METADATA_URL)
vnics = json.loads(str(resp))

if 'nicIndex' in vnics[0]:
# TODO: Once configure_secondary_nics defaults to True, lower the level
# of this log message. (Currently, if we're running this code at all,
# someone has explicitly opted-in to secondary VNIC configuration, so
# we should warn them that it didn't happen. Once it's default, this
# would be emitted on every Bare Metal Machine launch, which means INFO
# or DEBUG would be more appropriate.)
LOG.warning(
'VNIC metadata indicates this is a bare metal machine; skipping'
' secondary VNIC configuration.'
)
return

interfaces_by_mac = get_interfaces_by_mac()

for vnic_dict in vnics[1:]:
# We skip the first entry in the response because the primary interface
# is already configured by iSCSI boot; applying configuration from the
# IMDS is not required.
mac_address = vnic_dict['macAddr'].lower()
if mac_address not in interfaces_by_mac:
LOG.debug('Interface with MAC %s not found; skipping', mac_address)
continue
name = interfaces_by_mac[mac_address]

if network_config['version'] == 1:
subnet = {
'type': 'static',
'address': vnic_dict['privateIp'],
}
network_config['config'].append({
'name': name,
'type': 'physical',
'mac_address': mac_address,
'mtu': MTU,
'subnets': [subnet],
})
elif network_config['version'] == 2:
network_config['ethernets'][name] = {
'addresses': [vnic_dict['privateIp']],
'mtu': MTU, 'dhcp4': False, 'dhcp6': False,
'match': {'macaddress': mac_address}}
OpcMetadata = namedtuple("OpcMetadata", "version instance_data vnics_data")


def _ensure_netfailover_safe(network_config):
Expand Down Expand Up @@ -175,6 +109,7 @@ class DataSourceOracle(sources.DataSource):

def __init__(self, sys_cfg, *args, **kwargs):
super(DataSourceOracle, self).__init__(sys_cfg, *args, **kwargs)
self._vnics_data = sources.UNSET
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I'm not being distracted by the type annotation, I've realised that I have this question: is there any reason to use sources.UNSET over None here? The use of this attribute is entirely contained to this class, so I don't think we need to follow that (IMO, anti-)pattern.


self.ds_cfg = util.mergemanydict([
util.get_cfg_by_path(sys_cfg, ['datasource', self.dsname], {}),
Expand All @@ -192,13 +127,22 @@ def _get_data(self):

# network may be configured if iscsi root. If that is the case
# then read_initramfs_config will return non-None.
if _is_iscsi_root():
data = read_opc_metadata()
else:
with dhcp.EphemeralDHCPv4(net.find_fallback_nic()):
data = read_opc_metadata()
fetch_vnics_data = self.ds_cfg.get(
'configure_secondary_nics',
BUILTIN_DS_CONFIG["configure_secondary_nics"]
)
network_context = noop() if _is_iscsi_root(
) else dhcp.EphemeralDHCPv4(net.find_fallback_nic())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't love this continuation; black would format this as:

network_context = (
    noop()
    if _is_iscsi_root()
    else dhcp.EphemeralDHCPv4(net.find_fallback_nic())
)

which is actually more lines than:

if _is_iscsi_root():
    network_context = noop()
else:
    network_context = dhcp.EphemeralDHCPv4(net.find_fallback_nic())

and you can even golf that down a little further if you want:

network_context = noop()
if not _is_iscsi_root():
    network_context = dhcp.EphemeralDHCPv4(net.find_fallback_nic())

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(And before you say anything:

In [2]: len("        network_context = noop() if _is_iscsi_root() else dhcp.EphemeralDHCPv4(net.find_fallback_nic())") > 100
Out[2]: True

😁 )

with network_context:
fetched_metadata = read_opc_metadata(
fetch_vnics_data=fetch_vnics_data
)

self._crawled_metadata = data
data = self._crawled_metadata = fetched_metadata.instance_data
self.metadata_address = METADATA_ROOT.format(
version=fetched_metadata.version
)
self._vnics_data = fetched_metadata.vnics_data

self.metadata = {
"availability-zone": data["ociAdName"],
Expand All @@ -218,10 +162,6 @@ def _get_data(self):

return True

def _get_subplatform(self):
"""Return the subplatform metadata source details."""
return "metadata ({})".format(METADATA_ROOT)

def check_instance_id(self, sys_cfg):
"""quickly check (local only) if self.instance_id is still valid

Expand All @@ -246,14 +186,22 @@ def network_config(self):
# this is now v2
self._network_config = self.distro.generate_fallback_config()

if self.ds_cfg.get('configure_secondary_nics'):
try:
# Mutate self._network_config to include secondary VNICs
_add_network_config_from_opc_imds(self._network_config)
except Exception:
util.logexc(
LOG,
"Failed to fetch secondary network configuration!")
if self.ds_cfg.get(
'configure_secondary_nics',
BUILTIN_DS_CONFIG["configure_secondary_nics"]
):
if self._vnics_data == sources.UNSET:
LOG.warning(
"Secondary NIC data is UNSET but should not be")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think to moving this guard into _add_network_config_from_opc_imds? As currently written, _add_network_config_from_opc_imds will behave confusingly if called before self._vnics_data is set. It seems natural for that method to own its own input validation.

(As things stand, I think it will behave very confusingly: self._vnics_data will be "_unset", so we'll get a TypeError: string indices must be integers when we attempt to effectively do "u"['.macAddr'].lower(); if we do switch to self._vnics_data = None in __init__ then I think we'll get the less-confusing-but-still-avoidable: TypeError: 'NoneType' object is not subscriptable.)

else:
try:
# Mutate self._network_config to include secondary
# VNICs
self._add_network_config_from_opc_imds()
except Exception:
util.logexc(
LOG,
"Failed to parse secondary network configuration!")

# we need to verify that the nic selected is not a netfail over
# device and, if it is a netfail master, then we need to avoid
Expand All @@ -262,6 +210,65 @@ def network_config(self):

return self._network_config

def _add_network_config_from_opc_imds(self):
"""Generate secondary NIC config from IMDS and merge it.

The primary NIC configuration should not be modified based on the IMDS
values, as it should continue to be configured for DHCP. As such, this
uses the instance's network config dict which is expected to have the
primary NIC configuration already present.
It will mutate the network config to include the secondary VNICs.

:raises:
Exceptions are not handled within this function. Likely
exceptions are KeyError/IndexError
(if the IMDS returns valid JSON with unexpected contents).
"""
if 'nicIndex' in self._vnics_data[0]:
# TODO: Once configure_secondary_nics defaults to True, lower the
# level of this log message. (Currently, if we're running this
# code at all, someone has explicitly opted-in to secondary
# VNIC configuration, so we should warn them that it didn't
# happen. Once it's default, this would be emitted on every Bare
# Metal Machine launch, which means INFO or DEBUG would be more
# appropriate.)
LOG.warning(
'VNIC metadata indicates this is a bare metal machine; '
'skipping secondary VNIC configuration.'
)
return

interfaces_by_mac = get_interfaces_by_mac()

for vnic_dict in self._vnics_data[1:]:
# We skip the first entry in the response because the primary
# interface is already configured by iSCSI boot; applying
# configuration from the IMDS is not required.
mac_address = vnic_dict['macAddr'].lower()
if mac_address not in interfaces_by_mac:
LOG.debug('Interface with MAC %s not found; skipping',
mac_address)
continue
name = interfaces_by_mac[mac_address]

if self._network_config['version'] == 1:
subnet = {
'type': 'static',
'address': vnic_dict['privateIp'],
}
self._network_config['config'].append({
'name': name,
'type': 'physical',
'mac_address': mac_address,
'mtu': MTU,
'subnets': [subnet],
})
elif self._network_config['version'] == 2:
self._network_config['ethernets'][name] = {
'addresses': [vnic_dict['privateIp']],
'mtu': MTU, 'dhcp4': False, 'dhcp6': False,
'match': {'macaddress': mac_address}}


def _read_system_uuid():
sys_uuid = util.read_dmi_data('system-uuid')
Expand All @@ -277,15 +284,43 @@ def _is_iscsi_root():
return bool(cmdline.read_initramfs_config())


def read_opc_metadata():
"""
Fetch metadata from the /opc/ routes.
def read_opc_metadata(*, fetch_vnics_data: bool = False):
"""Fetch metadata from the /opc/ routes.

:return:
The JSON-decoded value of the /opc/v1/instance/ endpoint on the IMDS.
A namedtuple containing:
The metadata version as an integer
The JSON-decoded value of the instance data endpoint on the IMDS
The JSON-decoded value of the vnics data endpoint if
`fetch_vnics_data` is True, else None

"""
# retries=1 as requested by Oracle to address a potential race condition
return json.loads(readurl(METADATA_ENDPOINT, retries=1)._response.text)
retries = 1

def _fetch(metadata_version: int, path: str) -> dict:
headers = {
"Authorization": "Bearer Oracle"} if metadata_version > 1 else None
return readurl(
url=METADATA_PATTERN.format(version=metadata_version, path=path),
headers=headers,
retries=retries,
)._response.json()

metadata_version = 2
try:
instance_data = _fetch(metadata_version, path="instance")
except UrlError:
metadata_version = 1
instance_data = _fetch(metadata_version, path="instance")

vnics_data = None
if fetch_vnics_data:
try:
vnics_data = _fetch(metadata_version, path="vnics")
except UrlError:
util.logexc(LOG,
"Failed to fetch secondary network configuration!")
return OpcMetadata(metadata_version, instance_data, vnics_data)


# Used to match classes to dependencies
Expand Down
Loading