schema: add json defs for modules scripts-timezone (SC-801) #1365

TheRealFalcon · 2022-03-31T22:05:30Z

Proposed Commit Message

schema: add json defs for modules scripts-timezone

Additionally change module documentation to not show the property
header if no property definition exists.

Migrate legacy or define new schemas for the following:
- cc_scripts_per_boot
- cc_scripts_per_instance
- cc_scripts_per_once
- cc_scripts_user
- cc_scripts_vendor
- cc_seed_random
- cc_set_hostname
- cc_set_passwords
- cc_snap
- cc_spacewalk
- cc_ssh_authkey_fingerprints
- cc_ssh_import_id
- cc_ssh
- cc_timezone

cloudinit/config/cc_scripts_per_once.py

cloudinit/config/cc_scripts_vendor.py

blackboxsw · 2022-04-01T22:24:05Z

cloudinit/config/cc_scripts_vendor.py


+frequency = PER_INSTANCE
+MODULE_DESCRIPTION = """\
 Any scripts in the ``scripts/vendor`` directory in the datasource will be run
 when a new instance is first booted. Scripts will be run in alphabetical order.


I think we should update docs to somehow mention the run order of script config modules is [vendor, per-once, per-boot, per-instance, user].

If one cares about ordering, they would need to check /etc/cloud/cloud.cfg, and I think we need to find a way to document that separately. I don't disagree that it should be documented, but I'm not sure the module documentation is the best place for it.

cloudinit/config/cc_scripts_vendor.py

blackboxsw

almost done w/ cc_set_passwords but wanted to add comments before I EOW. cc_snap looks insteresting as I wonder if we want to use patternProperties schema to handle reprensenting the underlying lists below the opaque-keys. will get back to this monday.

cloudinit/config/cc_seed_random.py

cloudinit/config/cloud-init-schema.json

cloudinit/config/cc_set_hostname.py

blackboxsw · 2022-04-01T22:58:30Z

cloudinit/config/cc_set_hostname.py

+        "perserve_hostname: true",
+        dedent(
+            """\
+            hostname: myhost


We could explain in a comment expected behavior is for /etc/hostname and or /etc/hosts. But, maybe the description above handles that well enough.

Isn't a lot of it technically distro-dependent? Also the interaction between this module and update_hostname is still confusing to me and I think it could probably be simplified down into one module...but not here.

It is distro-dependent, so that extra context isn't helpful in all cases. We do have a number of questions about this behavior in bugs and IRC that keep surfacing. What we have in the MODULE_DESCRIPTION is probably good enough. Agreed that restructuring modules is not on our agenda for this effort and we can improve the documentation with tutorials you've started and or ammendment to use-cases.

cloudinit/config/cc_set_passwords.py

TheRealFalcon · 2022-04-03T03:31:23Z

Pushed 2 commits (one just for vendor data) addressing comments

blackboxsw

I need to come back to cc_ssh but wanted to give you a second round of feedback first.

cloudinit/config/cc_scripts_vendor.py

blackboxsw · 2022-04-04T17:10:51Z

cloudinit/config/cc_set_hostname.py

+        "perserve_hostname: true",
+        dedent(
+            """\
+            hostname: myhost


It is distro-dependent, so that extra context isn't helpful in all cases. We do have a number of questions about this behavior in bugs and IRC that keep surfacing. What we have in the MODULE_DESCRIPTION is probably good enough. Agreed that restructuring modules is not on our agenda for this effort and we can improve the documentation with tutorials you've started and or ammendment to use-cases.

cloudinit/config/cc_set_passwords.py

blackboxsw · 2022-04-04T18:14:00Z

cloudinit/config/cloud-init-schema.json

+            },
+            "squashfuse_in_container": {
+              "type": "boolean",
+              "description": "**Development only**: The ``squashfuse_in_container`` boolean can be set true to install squashfuse package when in a container to enable snap installs. Default: ``false``."


Let's add DEPRECATION to this key too. we no longer officially need this on Bionic++ because to install snaps because https://bugs.launchpad.net/snappy/+bug/1628289 has been fix released a while back (and we no longer support Xenial). I think we can actually drop the maybe_install_squashfuse function from cc_snap Let's take it as a separate work item though from this PR though.

Created https://warthogs.atlassian.net/browse/SC-905

Duplicate of https://warthogs.atlassian.net/browse/SC-903

Ahhh, thanks. Deleted

cloudinit/config/cloud-init-schema.json

blackboxsw · 2022-04-04T20:33:56Z

cloudinit/config/cloud-init-schema.json

+            },
+            "blacklist": {
+              "type": "array",
+              "description": "The SSH key types to blacklist when publishing. Default: ``[dsa]``",


Suggested change

"description": "The SSH key types to blacklist when publishing. Default: ``[dsa]``",

"description": "The SSH key types to ignore when publishing. Default: ``[dsa]``",

Once we add the ability to hide schema properties from the docs, I'd like to deprecate this and replace it with 'denylist'

cloudinit/config/cloud-init-schema.json

blackboxsw · 2022-04-04T20:37:10Z

cloudinit/stages.py

@@ -757,6 +757,12 @@ def _consume_vendordata(self, vendor_source, frequency=PER_INSTANCE):
            LOG.debug("%s consumption is disabled.", vendor_source)
            return

+        if isinstance(enabled, str):
+            LOG.debug(
+                "Use of string for 'vendor_data:enabled' field is "


Let's add the value here for debug/triage purposes.

Suggested change

"Use of string for 'vendor_data:enabled' field is "

f"Use of string '{enabled}' for 'vendor_data:enabled' field is "

doc/examples/cloud-config-seed-random.txt

blackboxsw

Minor schema fix with vendor_data.prefix otherwise LGTM!

diff --git a/cloudinit/config/cloud-init-schema.json b/cloudinit/config/cloud-init-schema.json
index 43e90e1a4..96dec0a7c 100644
--- a/cloudinit/config/cloud-init-schema.json
+++ b/cloudinit/config/cloud-init-schema.json
@@ -1356,7 +1356,9 @@
               "description": "Whether vendor data is enabled or not. Use of string for this value is DEPRECATED. Default: ``true``"
             },
             "prefix": {
-              "type": ["boolean", "string"],
+              "type": ["array", "string"],
+              "items": {"type": ["string", "integer"]},
+              "minItems": 1,
               "description": "The command to run before any vendor scripts. Its primary use case is for profiling a script, not to prevent its run"
             }
           }
diff --git a/tests/unittests/config/test_cc_scripts_vendor.py b/tests/unittests/config/test_cc_scripts_vendor.py
index a8cbfb4f4..69a5fe801 100644
--- a/tests/unittests/config/test_cc_scripts_vendor.py
+++ b/tests/unittests/config/test_cc_scripts_vendor.py
@@ -1,3 +1,5 @@
+import re
+
 import pytest
 
 from cloudinit.config.schema import (
@@ -14,6 +16,10 @@ class TestScriptsVendorSchema:
         (
             ({"vendor_data": {"enabled": True}}, None),
             ({"vendor_data": {"enabled": "yes"}}, None),
+            (
+                {"vendor_data": {"prefix": []}},
+                re.escape("vendor_data.prefix: [] is too short"),
+            ),
         ),
     )
     @skipUnlessJsonSchema()

blackboxsw · 2022-04-05T19:04:38Z

cloudinit/config/cloud-init-schema.json

+              "description": "Whether vendor data is enabled or not. Use of string for this value is DEPRECATED. Default: ``true``"
+            },
+            "prefix": {
+              "type": ["boolean", "string"],


I think we added wrong "types" in the latest commit for prefix it should be a

Suggested change

"type": ["boolean", "string"],

"type": ["array", "string"],

"items": {"type": ["string", "integer"]},

"minItems": 1,

@TheRealFalcon CI is failing on this invalid schema definition I believe. Otherwise I think we are good to go.

Gah, sorry. I made the change but never pushed it.

blackboxsw · 2022-04-05T19:57:09Z

cloudinit/config/cloud-init-schema.json

+          "type": "object",
+          "description": "A dictionary entries for the public and private host keys of each desired key type. Entries in the ``ssh_keys`` config dict should have keys in the format ``<key type>_private``, ``<key type>_public``, and, optionally, ``<key type>_certificate``, e.g. ``rsa_private: <key>``, ``rsa_public: <key>``, and ``rsa_certificate: <key>``. Not all key types have to be specified, ones left unspecified will not be used. If this config option is used, then separate keys will not be automatically generated. In order to specify multiline private host keys and certificates, use yaml multiline syntax.",
+          "patternProperties": {
+            "^(dsa|ecdsa|ecdsa-sk|ed25519|ed25519-sk|rsa)_(public|private|certificate)$": {


This schema looks ok, but the corresponding code in [cc_ssh.py will silently skip any key type that is not in GENERATE_KEY_NAMES

I think we need to properly handle these other keys and integration testing of them to make sure all is well. Since it's ssh-related I'd like that work to be a separate PR to make sure we get good eyes on it separate from schema coverage

I'll make a ticket

Additionally change module documentation to not show the property header if no property definition exists. Migrate legacy or define new schemas for the following: - cc_scripts_per_boot - cc_scripts_per_instance - cc_scripts_per_once - cc_scripts_user - cc_scripts_vendor - cc_seed_random - cc_set_hostname - cc_set_passwords - cc_snap - cc_spacewalk - cc_ssh_authkey_fingerprints - cc_ssh_import_id - cc_ssh - cc_timezone

TheRealFalcon · 2022-04-08T13:25:26Z

Fixed the CI issues. Note that this includes a small removal to the vendordata schema. On default LXD launch I'm getting

2022-04-08 13:08:59,487 - schema.py[WARNING]: Invalid cloud-config provided:
vendor_data.prefix: [] is too short

so we don't want a minItems defined on that.

blackboxsw

Bring it home!

sshedi · 2022-08-03T13:19:22Z

@TheRealFalcon I am seeing something weird after this change.

My /etc/hosts looks like this:

$ cat /etc/hosts 
127.0.1.1    ph3dev.local ph3dev
127.0.0.1    localhost

And my /etc/hostname

ph3dev

Now if I do systemctl restart cloud-init , /etc/hostname is getting changed.

$ cat /etc/hostname 
ph3dev.local

And hostname command return fqdn instead of short name. Isn't it incorrect to have fqdn in /etc/hostname?

holmanb · 2022-08-03T15:28:48Z

TheRealFalcon I am seeing something weird after this change.

The changes to the hostname module appear docs related. Did you bisect to this commit specifically?

Isn't it incorrect to have fqdn in /etc/hostname?

This is treated as distro-dependant if prefer_fqdn_over_hostname is unset. We would need to know what is in your user-data to know what behavior to expect.

@sshedi Please file a bug report with logs if you still believe this to be an issue.

sshedi · 2022-08-03T16:39:55Z

Thanks for the response @holmanb.

I changed frequency = PER_ONCE in cc_set_hostname.py and it doesn't modify /etc/hostname after that and this is how it was in cloud-init <= v22.1 and even in v22.1 we have prefer_fqdn_over_hostname set in photon.py but it doesn't modify /etc/hostname file.

Sure, I will file a bug report with necessary data.

holmanb · 2022-08-03T17:07:17Z

Sure, I will file a bug report with necessary data.

@sshedi Excellent, thank you.

FWIW, I do see prefer_fqdn = True in photon.py, so I would expect cloud-init to set the hostname with a fqdn (see how this changes behavior in hostname setting inside of cloudinit/distros/__init__.py:Distro). Let me know if I missed anything.

sshedi · 2022-08-08T07:10:29Z

I have created bug with necessary info https://bugs.launchpad.net/cloud-init/+bug/1983811

Please let me know if you need more info.

TheRealFalcon changed the title ~~schema: add json defs for modules scripts-timezone~~ schema: add json defs for modules scripts-timezone (SC-801) Apr 1, 2022

blackboxsw reviewed Apr 1, 2022

View reviewed changes

cloudinit/config/cc_scripts_per_once.py Show resolved Hide resolved

blackboxsw requested changes Apr 1, 2022

View reviewed changes

blackboxsw requested changes Apr 4, 2022

View reviewed changes

blackboxsw reviewed Apr 5, 2022

View reviewed changes

TheRealFalcon added 6 commits April 5, 2022 16:03

remove more tests

7dccc07

scripts_vendor changes

942d6d3

remaining review comments

0b15c11

comments

00ecc10

comments

bd486c9

TheRealFalcon force-pushed the schema-scripts-timezone branch from 0731b90 to bd486c9 Compare April 8, 2022 03:16

fix CI issues

adfcdc7

blackboxsw approved these changes Apr 8, 2022

View reviewed changes

blackboxsw merged commit 96eb95a into canonical:main Apr 8, 2022

TheRealFalcon deleted the schema-scripts-timezone branch August 8, 2022 19:19

This was referenced May 12, 2023

Top-level 'network' key results in error using v2 config #3815

Closed

fqdn does not accept terminal dot #3962

Closed

Release 22.2 #3976

Closed

Hostname getting set to fqdn after upgrading to cloud-init-22.2 #4006

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schema: add json defs for modules scripts-timezone (SC-801) #1365

schema: add json defs for modules scripts-timezone (SC-801) #1365

TheRealFalcon commented Mar 31, 2022

blackboxsw Apr 1, 2022

TheRealFalcon Apr 3, 2022

blackboxsw left a comment

blackboxsw Apr 1, 2022

TheRealFalcon Apr 3, 2022

blackboxsw Apr 4, 2022

TheRealFalcon commented Apr 3, 2022

blackboxsw left a comment

blackboxsw Apr 4, 2022

blackboxsw Apr 4, 2022

TheRealFalcon Apr 5, 2022

blackboxsw Apr 5, 2022

TheRealFalcon Apr 5, 2022

blackboxsw Apr 4, 2022

TheRealFalcon Apr 5, 2022

blackboxsw Apr 4, 2022

blackboxsw left a comment

blackboxsw Apr 5, 2022

blackboxsw Apr 8, 2022

TheRealFalcon Apr 8, 2022

blackboxsw Apr 5, 2022

TheRealFalcon Apr 5, 2022

TheRealFalcon commented Apr 8, 2022

blackboxsw left a comment •

edited

Loading

sshedi commented Aug 3, 2022

holmanb commented Aug 3, 2022

sshedi commented Aug 3, 2022

holmanb commented Aug 3, 2022

sshedi commented Aug 8, 2022

	"description": "The SSH key types to blacklist when publishing. Default: ``[dsa]``",
	"description": "The SSH key types to ignore when publishing. Default: ``[dsa]``",

	"Use of string for 'vendor_data:enabled' field is "
	f"Use of string '{enabled}' for 'vendor_data:enabled' field is "

schema: add json defs for modules scripts-timezone (SC-801) #1365

schema: add json defs for modules scripts-timezone (SC-801) #1365

Conversation

TheRealFalcon commented Mar 31, 2022

Proposed Commit Message

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blackboxsw 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

TheRealFalcon commented Apr 3, 2022

blackboxsw 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

Choose a reason for hiding this comment

blackboxsw 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

TheRealFalcon commented Apr 8, 2022

blackboxsw left a comment • edited Loading

Choose a reason for hiding this comment

sshedi commented Aug 3, 2022

holmanb commented Aug 3, 2022

sshedi commented Aug 3, 2022

holmanb commented Aug 3, 2022

sshedi commented Aug 8, 2022

blackboxsw left a comment •

edited

Loading