Support Oracle IMDSv2 API #528

TheRealFalcon · 2020-08-10T20:16:25Z

v2 of the API is now default with fallback to v1.
Refactored the Oracle datasource to fetch version, instance, and vnic metadata simultaneously.

TheRealFalcon · 2020-08-10T20:21:46Z

cloudinit/sources/DataSourceOracle.py

@@ -171,10 +105,11 @@ class DataSourceOracle(sources.DataSource):
        sources.NetworkConfigSource.system_cfg,
    )

-    _network_config = sources.UNSET
+    _network_config = sources.UNSET  # type: dict


Wasn't sure how to declare these types. If I don't declare anything, my type checking complains anywhere I use _network_config as a dict because sources.UNSET is a string. I still get a complaint on this line because I'm declaring a string as a dict, but it has quieted the rest of the warnings. If we were being strict it would be a Union[dict|str], but since we don't ever actually want to use it as a string, that also seemed wrong to me (and I didn't want to add the union import just for that).

Shrug. Something to think about if we start to incorporate more typing.

Yeah, I think sources.UNSET is going to be a barrier to typing the codebase more generally, so I think we should set it to one side here. I'd probably leave this untyped, rather than applying a not-quite-correct annotation; we'll correct it as part of a broader change.

TheRealFalcon · 2020-08-10T20:23:27Z

cloudinit/sources/DataSourceOracle.py

-                data = read_opc_metadata()
+        fetch_vnics_data = self.ds_cfg.get('configure_secondary_nics', False)
+        # suppress() in this context is just a no-op context manager
+        network_context = suppress() if _is_iscsi_root(


I don't love using suppress here, but duplicating the read_opc_metadata() call (especially if it's taking arguments now) seems worse to me.

Agreed; elsewhere in the codebase (albeit in testing code) we use ExitStack for this, and we import it as something that indicates how we're using it:

cloud-init/cloudinit/distros/tests/test_networking.py

Line 14 in a6bb375

from contextlib import ExitStack as does_not_raise

What do you think to following that pattern here? (Perhaps as noop is more apropos here? Or maybe nullcontext?)

cloudinit/sources/tests/test_oracle.py

* v2 of the API is now default with fallback to v1. * Refactored the Oracle datasource to fetch version, instance, and vnic metadata simultaneously.

OddBloke

Thanks for this, James! This is a complex change, and you've done a great job with it. Comments are inline.

OddBloke · 2020-08-10T21:09:06Z

cloudinit/sources/DataSourceOracle.py

-                data = read_opc_metadata()
+        fetch_vnics_data = self.ds_cfg.get('configure_secondary_nics', False)
+        # suppress() in this context is just a no-op context manager
+        network_context = suppress() if _is_iscsi_root(


Agreed; elsewhere in the codebase (albeit in testing code) we use ExitStack for this, and we import it as something that indicates how we're using it:

cloud-init/cloudinit/distros/tests/test_networking.py

Line 14 in a6bb375

from contextlib import ExitStack as does_not_raise

What do you think to following that pattern here? (Perhaps as noop is more apropos here? Or maybe nullcontext?)

cloudinit/sources/tests/test_oracle.py

OddBloke · 2020-08-10T21:29:33Z

cloudinit/sources/DataSourceOracle.py

@@ -171,10 +105,11 @@ class DataSourceOracle(sources.DataSource):
        sources.NetworkConfigSource.system_cfg,
    )

-    _network_config = sources.UNSET
+    _network_config = sources.UNSET  # type: dict


Yeah, I think sources.UNSET is going to be a barrier to typing the codebase more generally, so I think we should set it to one side here. I'd probably leave this untyped, rather than applying a not-quite-correct annotation; we'll correct it as part of a broader change.

OddBloke · 2020-08-11T13:43:37Z

cloudinit/sources/DataSourceOracle.py

@@ -247,13 +185,16 @@ def network_config(self):
                self._network_config = self.distro.generate_fallback_config()

            if self.ds_cfg.get('configure_secondary_nics'):
+                if self._vnics_data == sources.UNSET:
+                    LOG.warning("Network config is UNSET but should not be")


"Network config" is a little ambiguous in this context; I had to read this a couple of times to see that this was testing self._vnics_data, not self._network_config.

OddBloke · 2020-08-11T14:03:20Z

cloudinit/sources/DataSourceOracle.py

@@ -247,13 +185,16 @@ def network_config(self):
                self._network_config = self.distro.generate_fallback_config()

            if self.ds_cfg.get('configure_secondary_nics'):
+                if self._vnics_data == sources.UNSET:
+                    LOG.warning("Network config is UNSET but should not be")
+                    return None


I'm not sure return None is the right behaviour here: _add_network_config_from_opc_imds only adds to self._network_config, so we do have some valid config already determined at this point. If we return None from .network_config then _find_networking_config in stages.py will move onto considering other sources of network config.

For Oracle, the order in which network config sources will be considered is:

cloud-init/cloudinit/sources/DataSourceOracle.py

Lines 167 to 172 in 6d74815

network_config_sources = (

sources.NetworkConfigSource.cmdline,

sources.NetworkConfigSource.ds,

sources.NetworkConfigSource.initramfs,

sources.NetworkConfigSource.system_cfg,

)

So if we return None here: if there is initramfs configuration then that will be used, if not then fallback config will be generated (the system_cfg source is used rarely). That logic is identical to the logic here, but excludes the addition of secondary NICs (which is fine; we know there aren't any if we're executing this line) and the call to _ensure_netfailover_safe. The message on the commit that introduced _ensure_netfailover_safe (fa47d52) indicates that this should be called for fallback network configuration, so this is less fine.

(TL;DR: I think we need to skip the addition of the secondary NICs here, rather than aborting processing entirely.)

OddBloke · 2020-08-11T16:32:49Z

cloudinit/sources/tests/test_oracle.py

+            (2, does_not_raise()),
+            (3, does_not_raise()),
+            (4, pytest.raises(UrlError)),
+        ],
    )
    def test_retries(self, expectation, failure_count, httpretty):


This isn't testing retries on the VNIC route. (I suspect a separate test which just takes the happy path for instance data so it can test VNIC fetching in isolation would be easiest.)

Having reviewed the whole PR now: I think we're missing fetch_vnics_data testing more generally: all the tests still pass with this patch applied:

--- a/cloudinit/sources/DataSourceOracle.py +++ b/cloudinit/sources/DataSourceOracle.py @@ -305,6 +305,7 @@ def read_opc_metadata(*, fetch_vnics_data: bool = False): )._response.json() vnics_data = None if fetch_vnics_data: + assert False try: vnics_data = readurl( METADATA_PATTERN.format(

cloudinit/sources/tests/test_oracle.py

OddBloke · 2020-08-11T17:56:42Z

cloudinit/sources/DataSourceOracle.py

-        else:
-            with dhcp.EphemeralDHCPv4(net.find_fallback_nic()):
-                data = read_opc_metadata()
+        fetch_vnics_data = self.ds_cfg.get('configure_secondary_nics', False)


We now perform the defaulting of configure_secondary_nics in two places: here and in .network_config. It would be preferable to have the default configurable in a single place for ease of maintenance.

I'm don't understand what you mean by 'defaulting of configure_secondary_nics'. Can you explain it?

This line determines what the default value of configure_secondary_nics will be if it isn't present in self.ds_cfg: False. In .network_config, we also determine what the default value of configure_secondary_nics will be if it isn't present (and in that case as we don't supply a second argument to .get, it will be None).

Though, further confusing matters, we actually set the default in BUILTIN_DS_CONFIG which should mean that configure_secondary_nics is always set in self.ds_cfg (so we could use regular dict access instead of .get). However, that goes through util.mergemanydict and I don't know for sure that that couldn't result in a dict without the key present under certain user inputs.

Does that make any more sense?

Yes, mainly because of this

so we could use regular dict access instead of .get

I guess I don't see the problem with using .get here (or anywhere and everything 😉 ). Since we're not persisting the result of this anywhere, I don't see it as "defaulting a value", but rather eliminating the need for None checks in case something hasn't been set. Why would having things as they are increase the maintenance burden?

OddBloke · 2020-08-11T18:00:20Z

cloudinit/sources/DataSourceOracle.py

+        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.


This is still written as if this callable takes a parameter.

OddBloke · 2020-08-11T18:04:57Z

cloudinit/sources/tests/test_oracle.py

        # read_opc_metadata should JSON decode the response and return it
+        expected = json.loads(OPC_V2_METADATA)
+        assert expected == oracle.read_opc_metadata().instance_data


Should we also be testing the version attribute of read_opc_metadata's return here (and in the test below)?

OddBloke · 2020-08-12T13:47:06Z

On Tue, Aug 11, 2020 at 05:05:03PM -0700, James Falcon wrote: @TheRealFalcon commented on this pull request. > @@ -192,13 +127,20 @@ 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', False) Yes, mainly because of this > so we could use regular dict access instead of .get I guess I don't see the problem with using `.get` here (or anywhere and everything 😉 ).

My concern here is not with the use of `.get` (though we can have that conversation at some point ;).

Since we're not persisting the result of this anywhere, I don't see it as "defaulting a value"

This does declare the default in the case where the key is absent though; the whole point of using `.get` is that it returns a default instead of raising a `KeyError` on absence.

but rather eliminating the need for None checks in case something hasn't been set.

This would also be a problem with None checks; this code is equivalent to yours (and worse IMO, to be clear!) and is also performing defaulting in this specific codepath: ```py fetch_vnics_data = self.ds_cfg.get("configure_secondary_nics") if fetch_vnics_data is None: fetch_vnics_data = False ``` Regardless of the exact spelling, the problem is that defaulting is happening in this codepath as well as in the BUILTIN_DS_CONFIG dict (and also in `network_config` in the faulty line I introduced).

Why would having things as they are increase the maintenance burden?

When we want to update the default (which we do intend to do at some point), we now have three places where we need to update the value, instead of one. (We had two before your PR, my code is also at fault. :) To be clear, I think something like `.get("configure_secondary_nics", BUILTIN_DS_CONFIG["configure_secondary_nics"])` would do the job.

OddBloke · 2020-08-12T19:49:52Z

cloudinit/sources/tests/test_oracle.py

+def _mock_v2_urls(httpretty):
+    def instance_callback(request, uri, response_headers):
+        assert request.headers.get("Authorization") == "Bearer Oracle"
+        return [200, {}, OPC_V2_METADATA]


It looks like the package build in xenial is failing during the tests: https://travis-ci.com/github/canonical/cloud-init/jobs/371325182

I believe this is because the version of httpretty (0.8.6) in xenial does not include gabrielfalcao/HTTPretty@bdb7ee7 (which was first included in 0.8.11). So I think you'll need to add some default response header values to avoid these tracebacks.

(This isn't caught by the "xenial" unittests in Travis because we don't have HTTPretty pinned correctly; I'll be submitting a PR to address that shortly.)

#531 is that PR.

TheRealFalcon · 2020-08-12T21:07:46Z

@OddBloke test fixed

OddBloke

Thanks for the speedy turnaround, James! Just two remaining (minor) areas of concern.

Have you performed any testing of this in Oracle yet? (Do you have access?)

OddBloke · 2020-08-12T20:52:44Z

cloudinit/sources/DataSourceOracle.py

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


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())

(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

😁 )

OddBloke · 2020-08-12T21:43:29Z

cloudinit/sources/DataSourceOracle.py

@@ -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


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.

OddBloke · 2020-08-12T21:51:30Z

cloudinit/sources/DataSourceOracle.py

+                if self._vnics_data == sources.UNSET:
+                    LOG.warning(
+                        "Secondary NIC data is UNSET but should not be")


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.)

OddBloke

I'm happy with this code now, thanks James! I'm going to wait for you to complete the testing you're doing before landing this.

TheRealFalcon · 2020-08-13T16:37:48Z

@OddBloke , I manually checked the following:

cloud-init query -a after installing the new deb and running a clean with reboot returns the new metadata, including "subplatform": "metadata (http://169.254.169.254/opc/v2/)"
cloud-init status is done
nothing strange in cloud-init.log or cloud-init-output.log
main of DataSourceOracle.py returns the expected metadata and v2

If there's nothing else, I'm comfortable with where this is at.

josh-potter · 2020-08-13T16:42:19Z

@OddBloke , I manually checked the following:

cloud-init query -a after installing the new deb and running a clean with reboot returns the new metadata, including "subplatform": "metadata (http://169.254.169.254/opc/v2/)"

cloud-init status is done

nothing strange in cloud-init.log or cloud-init-output.log

main of DataSourceOracle.py returns the expected metadata and v2

If there's nothing else, I'm comfortable with where this is at.

I can manually kill the v2 endpoint on your test instance if we want to validate failover logic as well. I'll just need the instance id (e.g. ocid1.instance.foo) and a time window to have it disabled.

TheRealFalcon · 2020-08-13T16:55:58Z

@josh-potter Great, thanks. Instance id is ocid1.instance.oc1.phx.anyhqljtniwq6sycegkrokuyq2w27j4rm43i6zvgzphk5our2mle2j2rszrq

Time frame can be as soon as works for you. I should only need a couple minutes, but if you need a specific time frame, lets say 30 minutes. If you just let me know when you plan on turning it off I can get to it right away.

josh-potter · 2020-08-13T17:00:45Z

@TheRealFalcon I just need to document the change on our side before executing. That'll take me a little bit to get together. Let's target 2:00 PM EST. I'll leave the v2 endpoint disabled on that instance for 30 minutes.

josh-potter · 2020-08-13T18:03:58Z

@TheRealFalcon the v2 endpoint is now disabled. I'll add it back at 2:30 PM EST or sooner if you get finished up with testing.

TheRealFalcon · 2020-08-13T18:23:51Z

Thanks @josh-potter . I got what I need

TheRealFalcon · 2020-08-13T18:34:37Z

I manually checked the same things I mentioned earlier, except checking for v1 instead of v2. Also, I diff'ed the cloud-init query -a and python3 DataSourceOracle.py outputs comparing v2 to v1, and the only differences were in the version strings.

I'm comfortable to merge.

OddBloke · 2020-08-13T19:27:43Z

@TheRealFalcon Do you want to author the squash commit message, or shall I come up with something?

TheRealFalcon · 2020-08-13T19:34:50Z

@OddBloke I was thinking the title plus first comment. If you think there's more or less we should add, use your creative liberties 😉

"Support Oracle IMDSv2 API

v2 of the API is now default with fallback to v1.
Refactored the Oracle datasource to fetch version, instance, and vnic metadata simultaneously."

TheRealFalcon commented Aug 10, 2020

View reviewed changes

cloudinit/sources/tests/test_oracle.py Show resolved Hide resolved

TheRealFalcon requested a review from OddBloke August 10, 2020 20:28

TheRealFalcon force-pushed the oracle-v2 branch from b667c56 to d44e951 Compare August 11, 2020 14:10

Support Oracle IMDSv2 API

3426289

* v2 of the API is now default with fallback to v1. * Refactored the Oracle datasource to fetch version, instance, and vnic metadata simultaneously.

TheRealFalcon force-pushed the oracle-v2 branch from d44e951 to 3426289 Compare August 11, 2020 14:26

OddBloke requested changes Aug 11, 2020

View reviewed changes

review comments

229f965

OddBloke reviewed Aug 12, 2020

View reviewed changes

fix httpretty issue with headers

c90cef8

TheRealFalcon requested a review from OddBloke August 12, 2020 21:07

OddBloke requested changes Aug 12, 2020

View reviewed changes

review comments

c2c2ba4

TheRealFalcon requested a review from OddBloke August 13, 2020 00:43

Merge branch 'master' into oracle-v2

ef0e983

OddBloke approved these changes Aug 13, 2020

View reviewed changes

Merge branch 'master' into oracle-v2

ce5b6b1

Merge branch 'master' into oracle-v2

aea3173

OddBloke merged commit 546617c into canonical:master Aug 13, 2020

TheRealFalcon deleted the oracle-v2 branch August 13, 2020 19:39

ubuntu-server-builder mentioned this pull request May 12, 2023

Release 20.3 #3774

Closed

	network_config_sources = (
	sources.NetworkConfigSource.cmdline,
	sources.NetworkConfigSource.ds,
	sources.NetworkConfigSource.initramfs,
	sources.NetworkConfigSource.system_cfg,
	)

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

Support Oracle IMDSv2 API #528

Support Oracle IMDSv2 API #528

Conversation

TheRealFalcon commented Aug 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OddBloke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealFalcon Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OddBloke commented Aug 12, 2020 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealFalcon commented Aug 12, 2020

OddBloke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OddBloke left a comment

Choose a reason for hiding this comment

TheRealFalcon commented Aug 13, 2020

josh-potter commented Aug 13, 2020

TheRealFalcon commented Aug 13, 2020

josh-potter commented Aug 13, 2020

josh-potter commented Aug 13, 2020

TheRealFalcon commented Aug 13, 2020

TheRealFalcon commented Aug 13, 2020

OddBloke commented Aug 13, 2020

TheRealFalcon commented Aug 13, 2020 • edited Loading

TheRealFalcon Aug 11, 2020 •

edited

Loading

TheRealFalcon commented Aug 13, 2020 •

edited

Loading