cloud-init icon indicating copy to clipboard operation
cloud-init copied to clipboard

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

Open blackboxsw opened this issue 1 year ago • 1 comments

Proposed Commit Message

feat(net): provide network config to netplan.State for render (SC-1402)

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.

Additional Context

Some concerns with this approach:

  1. 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
  2. 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

Merge type

  • [x] 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 avatar Feb 29 '24 05:02 blackboxsw

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.

slyon avatar Mar 12 '24 11:03 slyon

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
 

TheRealFalcon avatar Apr 16 '24 22:04 TheRealFalcon

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

blackboxsw avatar Apr 17 '24 17:04 blackboxsw