feat(net): provide network config to netplan.State for render #4981

blackboxsw · 2024-02-29T05:42:03Z

Proposed Commit Message

feat(net): provide network config to netplan.State for render (#4981)

Rely on netplan API where present to write network config to
50-cloud-init.yaml. (SC-1402)

Allow cloud-init to be decoupled from security policies of netplan
libraries when rendering potentially senstive network configuratiion.

Newer netplan versions may filter sensitive network configuration parts
into separate files or subdirectories. Allow cloud-init to rely on
netplan's treatment of sensitive keys by using netplan.state.State to
write_yaml_file with cloud-init's requested base filename:
50-cloud-init.yaml.

If netplan security policy changes in the future, those changes will
be adopted under the hood of netplan.State.

Additional Context

Some concerns with this approach:

Given that netplan API doesn't yet surface a public method, we are using _write_yaml_file under expectation that this interface does't change (needs netplan team sign-off for awareness) CC: @slyon
The current State._write_yaml_file doesn't accept a header, strips any inline comments when processing network config yaml. This means that 50-cloud-init.yaml, as rendered by netplan's State._write_yaml will no long contain the helpful comment/header:

# This file is generated from information provided by the datasource.  Changes
# to it will not persist across an instance reboot.  To disable cloud-init's
# network configuration capabilities, write a file
# /etc/cloud/cloud.cfg.d/99-disable-network-config.cfg with the following:
# network: {config: disabled}

If we want to take this approach to use netplan API to avoid policy changes in netplan, we probably should look to a feature request for netplan.State._write_yaml_file to allow a header or footer parameter to allow extending the config content that is written by netplan.

Test Steps

Checklist

My code follows the process laid out in the documentation
I have updated or added any unit tests accordingly
I have updated or added any documentation accordingly

Merge type

Squash merge using "Proposed Commit Message"
Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

blackboxsw · 2024-02-29T13:53:53Z

cloudinit/net/netplan.py

+            # determine default root-dir /etc/netplan and/or specialized
+            # filenames or read permissions based on whether this config
+            # contains secrets.
+            state_output_file._write_yaml_file(


@slyon points here for awareness:

cloud-init is calling this private method from netplan in absence of a public netplan API intended to let netplan choose ultimately what file to write and in which directories. Can we consider this a safe method to call that won't change until cloud-init is able to call a public method in the future?

If netplan grows the ability in the future, do you expect the functionality to separate sensitive config from world-readable config will live under this method call?

Yes

Yes

There is an idea of introducing a public .write_yaml() method. But we have to work out some kinks first. _write_yaml_file() is good for now, as its underlying CAPI is a public symbol netplan_state_write_yaml_file(). And it's also what is being used in the Netplan CLI.

@slyon excellent and thank you. I was wondering what you thought about a feature request for an optional header param to _write_yaml_file in the future that would allow cloud-init to inject custom comments into the file being written?

I was already pondering about ways to keep the comments, but that's not currently possible with the underlying libyaml. I like the idea of a header parameter.

Another thing that might be helpful is returning the path(s) of any files written so that at the very least we could add our own header after the fact.

slyon

lgtm

slyon · 2024-02-29T14:24:39Z

cloudinit/net/netplan.py

+    cfg_dir = mkdtemp()
+    try:
+        with open(os.path.join(cfg_dir, "ci-net-cfg.yaml"), "wb+") as f:
+            f.write(util.encode_text(net_config_content))


note (non-blocking): You could consider doing this all in-memory, using io.StringIO and/or os.memfd_create

slyon · 2024-02-29T14:31:41Z

cloudinit/net/netplan.py

+            # determine default root-dir /etc/netplan and/or specialized
+            # filenames or read permissions based on whether this config
+            # contains secrets.
+            state_output_file._write_yaml_file(


Yes

Yes

There is an idea of introducing a public .write_yaml() method. But we have to work out some kinks first. _write_yaml_file() is good for now, as its underlying CAPI is a public symbol netplan_state_write_yaml_file(). And it's also what is being used in the Netplan CLI.

TheRealFalcon

Generally LGTM. I do agree with slyon that we should use something like StringIO or perhaps a SpooledTemporaryFile rather than the current tmpdir approach.

Also, will netplan currently write a separate private config for the sensitive parts? If so, I think it's worth a separate integration test ensuring the pieces get written correctly.

TheRealFalcon · 2024-02-29T15:22:18Z

cloudinit/net/netplan.py

+        return False
+    cfg_dir = mkdtemp()
+    try:
+        with open(os.path.join(cfg_dir, "ci-net-cfg.yaml"), "wb+") as f:


Not sure if this will still be relevant if you switch to StringIO, but why are we opening binary if we're writing text? We can pass the encoding to open() (or just not specify an encoding, but that's considered bad practice) rather than using a utility function.

This is resolved/dropped as I'm now using SpooledTemporaryFile which defaults to "w+b" and ensuring we are encoding as bytes

TheRealFalcon · 2024-03-01T17:06:08Z

cloudinit/net/netplan.py

+            # determine default root-dir /etc/netplan and/or specialized
+            # filenames or read permissions based on whether this config
+            # contains secrets.
+            state_output_file._write_yaml_file(


Another thing that might be helpful is returning the path(s) of any files written so that at the very least we could add our own header after the fact.

TheRealFalcon · 2024-03-01T17:12:51Z

cloudinit/net/netplan.py

-                mode = current_mode
-        util.write_file(fpnplan, content, mode=mode)
+        if not netplan_api_write_yaml_file(content):
+            # No netplan api, fallback to write config  directly.


Nit: As we're adding some extra complexity here, I think the code under this if block would be better as its own function. fallback_write_yaml_file or something like that.

slyon · 2024-03-12T11:32:13Z

Also, will netplan currently write a separate private config for the sensitive parts? If so, I think it's worth a separate integration test ensuring the pieces get written correctly.

Netplan does not currently do that. It's just a card in our backlog so far, see FR-3285 for more details.

Rely on netplan API, where present, to write network config to 50-cloud-init.yaml. Allow cloud-init to be decoupled from security policies of netplan libraries when rendering potentially senstive network configuratiion. Newer netplan versions may filter sensitive network configuration parts into separate files or directories. Allow cloud-init to rely on netplan's rendering behavior by using netplan.state.State to write_yaml_file with cloud-init's requested base filename: 50-cloud-init.yaml. If netplan security policy changes in the future, those changes will be honored under the hood of netplan.State.

TheRealFalcon · 2024-04-16T22:15:28Z

I think we can simplify a bit more, can't we?

diff --git a/cloudinit/net/netplan.py b/cloudinit/net/netplan.py
index fe97fdcc9..46cbe3a9d 100644
--- a/cloudinit/net/netplan.py
+++ b/cloudinit/net/netplan.py
@@ -250,11 +250,9 @@ def netplan_api_write_yaml_file(net_config_content: str) -> bool:
             CLOUDINIT_NETPLAN_FILE,
         )
         return False
-    cfg_dir = mkdtemp()
     try:
-        with SpooledTemporaryFile(dir=cfg_dir, mode="w+b") as f:
-            f.write(util.encode_text(net_config_content))
-            f.flush()
+        with SpooledTemporaryFile(mode="w") as f:
+            f.write(net_config_content)
             f.seek(0, io.SEEK_SET)
             parser = Parser()
             parser.load_yaml(f)
@@ -276,8 +274,6 @@ def netplan_api_write_yaml_file(net_config_content: str) -> bool:
             e,
         )
         return False
-    if os.path.exists(cfg_dir):
-        shutil.rmtree(cfg_dir)
     LOG.debug("Rendered netplan config using netplan python API")
     return True

blackboxsw · 2024-04-17T17:51:28Z

@TheRealFalcon correct, we can simplify this. I also adapted our netplan config delta check to allow it to be used for both netplan API and fallback method to ensure we don't run netplan generate unnecessarily when netplan config hasn't changed.
CLOUD_INIT_CLOUD_INIT_SOURCE=IN_PLACE CLOUD_INIT_OS_IMAGE=jammy .tox/integration-tests/bin/pytest -- tests/integration_tests/test_networking.py::TestNetplanGenerateBehaviorOnReboot::test_skip for testing.

TheRealFalcon

+1 to the primary changes, but you have test failures now.

Also check netplan config deltas for prior to using netplan API to avoid netplan generate cmd when netplan conf hasn't changed.

TheRealFalcon

LGTM!

Test nit: We don't really have any unit test coverage of the new functionality. But also:

We have integration test coverage.
The whole thing is essentially a wrapper to an outside call, so there's not really much value to a unit test.
It's all wrapped in exceptions to fallback on failure.

Given these 3 things, I'm ok with it.

blackboxsw commented Feb 29, 2024

View reviewed changes

TheRealFalcon self-assigned this Feb 29, 2024

slyon approved these changes Feb 29, 2024

View reviewed changes

TheRealFalcon reviewed Mar 1, 2024

View reviewed changes

github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label Mar 27, 2024

github-actions bot closed this Apr 3, 2024

blackboxsw reopened this Apr 15, 2024

blackboxsw force-pushed the SC-1402-use-netplan-api-to-write-50-cloud-init.yaml branch from 2756d2c to 0ad5268 Compare April 16, 2024 00:00

blackboxsw requested a review from TheRealFalcon April 16, 2024 00:00

blackboxsw force-pushed the SC-1402-use-netplan-api-to-write-50-cloud-init.yaml branch from 0ad5268 to 416dd69 Compare April 16, 2024 00:03

blackboxsw removed the stale-pr Pull request is stale; will be auto-closed soon label Apr 16, 2024

canonical deleted a comment from github-actions bot Apr 16, 2024

TheRealFalcon reviewed Apr 19, 2024

View reviewed changes

blackboxsw force-pushed the SC-1402-use-netplan-api-to-write-50-cloud-init.yaml branch 2 times, most recently from 495d08c to e7386fa Compare April 19, 2024 04:34

comments: avoid binary mode and mkdtemp for SpooledTempFile

ddf676d

Also check netplan config deltas for prior to using netplan API to avoid netplan generate cmd when netplan conf hasn't changed.

blackboxsw force-pushed the SC-1402-use-netplan-api-to-write-50-cloud-init.yaml branch from e7386fa to ddf676d Compare April 19, 2024 16:02

TheRealFalcon approved these changes Apr 19, 2024

View reviewed changes

blackboxsw merged commit c465de8 into canonical:main Apr 20, 2024
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(net): provide network config to netplan.State for render #4981

feat(net): provide network config to netplan.State for render #4981

blackboxsw commented Feb 29, 2024 •

edited

Loading

blackboxsw Feb 29, 2024 •

edited

Loading

slyon Feb 29, 2024

blackboxsw Feb 29, 2024 •

edited

Loading

slyon Feb 29, 2024

TheRealFalcon Mar 1, 2024

slyon left a comment

slyon Feb 29, 2024

slyon Feb 29, 2024

TheRealFalcon left a comment

TheRealFalcon Feb 29, 2024

blackboxsw Apr 16, 2024

TheRealFalcon Mar 1, 2024

TheRealFalcon Mar 1, 2024

slyon commented Mar 12, 2024 •

edited by jira bot

Loading

TheRealFalcon commented Apr 16, 2024

blackboxsw commented Apr 17, 2024

TheRealFalcon left a comment

TheRealFalcon left a comment

feat(net): provide network config to netplan.State for render #4981

feat(net): provide network config to netplan.State for render #4981

Conversation

blackboxsw commented Feb 29, 2024 • edited Loading

Proposed Commit Message

Additional Context

Test Steps

Checklist

Merge type

blackboxsw Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blackboxsw Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slyon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealFalcon 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

slyon commented Mar 12, 2024 • edited by jira bot Loading

TheRealFalcon commented Apr 16, 2024

blackboxsw commented Apr 17, 2024

TheRealFalcon left a comment

Choose a reason for hiding this comment

TheRealFalcon left a comment

Choose a reason for hiding this comment

blackboxsw commented Feb 29, 2024 •

edited

Loading

blackboxsw Feb 29, 2024 •

edited

Loading

blackboxsw Feb 29, 2024 •

edited

Loading

slyon commented Mar 12, 2024 •

edited by jira bot

Loading