Add schema for cc_chef module #375

lucasmoura · 2020-05-20T22:00:20Z

his PR creates a schema object for the chef module and validate this schema in the handle function of the module.

Some of the config keys description, so I tried looking at the code and chef documentation to provide an information to the user. However, I don't know if I have the best description for all fields. For example, for the key show_time I could not find an accurate description of what it did, so I used what was in our code base to infer what it should do.

LP: #1858888

igalic · 2020-05-22T13:30:23Z

cloudinit/config/cc_chef.py

+
+
+frequency = PER_ALWAYS
+distros = ["all"]


we should re-enable chef and puppet for *BSD in the default config

Sorry @igalic, but I don't think I have enough context to understand your comment. What do you mean by re-enable BSD in the default config ? I tried looking at the code to see if they are handling BSD in a different manner, but could not find any specific code that does that. So I am definitely missing something here.

OddBloke · 2020-05-22T14:49:17Z

On Fri, May 22, 2020 at 07:06:35AM -0700, lucasmoura wrote: Sorry @igalic, but I don't think I have enough context to understand your comment. What do you mean by re-enable BSD in the default config ? I tried looking at the code to see if they are handling BSD in a different manner, but could not find any specific code that does that. So I am definitely missing something here.

I don't want to put words in her mouth, but I think Meena was saying that (in a separate PR), we should modify these lines: https://github.com/canonical/cloud-init/blob/98a6d9a0da82a764943c9495d7f21f3acc5ccd05/config/cloud.cfg.tmpl#L128-L132 @igalic, do you want to work on that? Or could you file a bug for it at least?

igalic · 2020-05-23T06:29:54Z

step 1 https://bugs.launchpad.net/cloud-init/+bug/1880279
step 2 #385

blackboxsw

thanks @lucasmoura.

A couple of comments:

Generally for schema definitions let's try to source any global variables defined in the module for documentation and defaults so docs will update if the globals change in the future
Also the intro description for the module was sort of hard to follow and had broken grammar. a minor update to that.

A patch is below to start with which captures a couple of comments I made, but if you could scrub the remaining schema docs and make sure global defaults are being used where applicable that would be great

diff --git a/cloudinit/config/cc_chef.py b/cloudinit/config/cc_chef.py
index 7f194535e..5a25286bc 100644
--- a/cloudinit/config/cc_chef.py
+++ b/cloudinit/config/cc_chef.py
@@ -21,6 +21,67 @@ from cloudinit import util
 from cloudinit.settings import PER_ALWAYS
 
 
+RUBY_VERSION_DEFAULT = "1.8"
+
+CHEF_DIRS = tuple([
+    '/etc/chef',
+    '/var/log/chef',
+    '/var/lib/chef',
+    '/var/cache/chef',
+    '/var/backups/chef',
+    '/var/run/chef',
+])
+REQUIRED_CHEF_DIRS = tuple([
+    '/etc/chef',
+])
+
+# Used if fetching chef from a omnibus style package
+OMNIBUS_URL = "https://www.chef.io/chef/install.sh"
+OMNIBUS_URL_RETRIES = 5
+
+CHEF_VALIDATION_PEM_PATH = '/etc/chef/validation.pem'
+CHEF_FB_PATH = '/etc/chef/firstboot.json'
+CHEF_RB_TPL_DEFAULTS = {
+    # These are ruby symbols...
+    'ssl_verify_mode': ':verify_none',
+    'log_level': ':info',
+    # These are not symbols...
+    'log_location': '/var/log/chef/client.log',
+    'validation_key': CHEF_VALIDATION_PEM_PATH,
+    'validation_cert': None,
+    'client_key': "/etc/chef/client.pem",
+    'json_attribs': CHEF_FB_PATH,
+    'file_cache_path': "/var/cache/chef",
+    'file_backup_path': "/var/backups/chef",
+    'pid_file': "/var/run/chef/client.pid",
+    'show_time': True,
+    'encrypted_data_bag_secret': None,
+}
+CHEF_RB_TPL_BOOL_KEYS = frozenset(['show_time'])
+CHEF_RB_TPL_PATH_KEYS = frozenset([
+    'log_location',
+    'validation_key',
+    'client_key',
+    'file_cache_path',
+    'json_attribs',
+    'pid_file',
+    'encrypted_data_bag_secret',
+])
+CHEF_RB_TPL_KEYS = list(CHEF_RB_TPL_DEFAULTS.keys())
+CHEF_RB_TPL_KEYS.extend(CHEF_RB_TPL_BOOL_KEYS)
+CHEF_RB_TPL_KEYS.extend(CHEF_RB_TPL_PATH_KEYS)
+CHEF_RB_TPL_KEYS.extend([
+    'server_url',
+    'node_name',
+    'environment',
+    'validation_name',
+])
+CHEF_RB_TPL_KEYS = frozenset(CHEF_RB_TPL_KEYS)
+CHEF_RB_PATH = '/etc/chef/client.rb'
+CHEF_EXEC_PATH = '/usr/bin/chef-client'
+CHEF_EXEC_DEF_ARGS = tuple(['-d', '-i', '1800', '-s', '20'])
+
+
 frequency = PER_ALWAYS
 distros = ["all"]
 schema = {
@@ -28,18 +89,14 @@ schema = {
     'name': 'Chef',
     'title': 'module that configures, starts and installs chef',
     'description': dedent("""\
-        This module enables chef to be installed (from packages or
-        from gems, or from omnibus). Before this occurs chef configurations are
+        This module enables chef to be installed (from packages,
+        gems, or from omnibus). Before this occurs, chef configuration is
         written to disk (validation.pem, client.pem, firstboot.json,
-        client.rb), and needed chef folders/directories are created
-        (/etc/chef and /var/log/chef and so-on). Then once installing proceeds
-        correctly if configured chef will be started (in daemon mode or in
-        non-daemon mode) and then once that has finished (if ran in non-daemon
-        mode this will be when chef finishes converging, if ran in daemon mode
-        then no further actions are possible since chef will have forked into
-        its own process) then a post run function can be executed, which
-        perform finishing activities (such as removing the validation pem
-        file)."""),
+        client.rb), and required directories are created (/etc/chef and
+        /var/log/chef and so-on). If configured, chef will be
+        installed and started in either daemon or non-daemon mode.
+        If run in non-daemon mode, post run actions are run to do finishing
+        activities such as removing validation.pem."""),
     'distros': distros,
     'examples': [dedent("""
         chef:
@@ -80,12 +137,8 @@ schema = {
                         Create the necessary directories for chef to run. By
                         default, it creates the following directories:
 
-                            - ``/etc/chef``
-                            - ``/var/log/chef``
-                            - ``/var/lib/chef``
-                            - ``/var/cache/chef``
-                            - ``/var/backups/chef``
-                            - ``/var/run/chef``""")
+                        {chef_dirs}""".format(chef_dirs="\n".join(
+                            ["    - ``{}``".format(d) for d in CHEF_DIRS])))
                 },
                 'validation_cert': {
                     'type': 'string',
@@ -159,7 +212,7 @@ schema = {
                 },
                 'json_attribs': {
                     'type': 'string',
-                    'default': '/etc/chef/firstboot.json',
+                    'default': CHEF_RB_TPL_DEFAULTS['json_attribs'],
                     'description': dedent("""\
                         Specifies the location in which some chef json data is
                         stored. By default, it uses the
@@ -226,7 +279,7 @@ schema = {
                 },
                 'ssl_verify_mode': {
                     'type': 'string',
-                    'default': ':verify_none',
+                    'default': CHEF_RB_TPL_DEFAULTS['ssl_verify_mode'],
                     'description': dedent("""\
                         Set the verify mode for HTTPS requests. We can have
                         two possible values for this parameter:
@@ -287,67 +340,6 @@ schema = {
 __doc__ = get_schema_doc(schema)
 
 
-RUBY_VERSION_DEFAULT = "1.8"
-
-CHEF_DIRS = tuple([
-    '/etc/chef',
-    '/var/log/chef',
-    '/var/lib/chef',
-    '/var/cache/chef',
-    '/var/backups/chef',
-    '/var/run/chef',
-])
-REQUIRED_CHEF_DIRS = tuple([
-    '/etc/chef',
-])
-
-# Used if fetching chef from a omnibus style package
-OMNIBUS_URL = "https://www.chef.io/chef/install.sh"
-OMNIBUS_URL_RETRIES = 5
-
-CHEF_VALIDATION_PEM_PATH = '/etc/chef/validation.pem'
-CHEF_FB_PATH = '/etc/chef/firstboot.json'
-CHEF_RB_TPL_DEFAULTS = {
-    # These are ruby symbols...
-    'ssl_verify_mode': ':verify_none',
-    'log_level': ':info',
-    # These are not symbols...
-    'log_location': '/var/log/chef/client.log',
-    'validation_key': CHEF_VALIDATION_PEM_PATH,
-    'validation_cert': None,
-    'client_key': "/etc/chef/client.pem",
-    'json_attribs': CHEF_FB_PATH,
-    'file_cache_path': "/var/cache/chef",
-    'file_backup_path': "/var/backups/chef",
-    'pid_file': "/var/run/chef/client.pid",
-    'show_time': True,
-    'encrypted_data_bag_secret': None,
-}
-CHEF_RB_TPL_BOOL_KEYS = frozenset(['show_time'])
-CHEF_RB_TPL_PATH_KEYS = frozenset([
-    'log_location',
-    'validation_key',
-    'client_key',
-    'file_cache_path',
-    'json_attribs',
-    'pid_file',
-    'encrypted_data_bag_secret',
-])
-CHEF_RB_TPL_KEYS = list(CHEF_RB_TPL_DEFAULTS.keys())
-CHEF_RB_TPL_KEYS.extend(CHEF_RB_TPL_BOOL_KEYS)
-CHEF_RB_TPL_KEYS.extend(CHEF_RB_TPL_PATH_KEYS)
-CHEF_RB_TPL_KEYS.extend([
-    'server_url',
-    'node_name',
-    'environment',
-    'validation_name',
-])
-CHEF_RB_TPL_KEYS = frozenset(CHEF_RB_TPL_KEYS)
-CHEF_RB_PATH = '/etc/chef/client.rb'
-CHEF_EXEC_PATH = '/usr/bin/chef-client'
-CHEF_EXEC_DEF_ARGS = tuple(['-d', '-i', '1800', '-s', '20'])
-
-
 def is_installed():
     if not os.path.isfile(CHEF_EXEC_PATH):
         return False

blackboxsw · 2020-06-05T19:13:04Z

cloudinit/config/cc_chef.py

+        This module enables chef to be installed (from packages or
+        from gems, or from omnibus). Before this occurs chef configurations are
+        written to disk (validation.pem, client.pem, firstboot.json,
+        client.rb), and needed chef folders/directories are created
+        (/etc/chef and /var/log/chef and so-on). Then once installing proceeds
+        correctly if configured chef will be started (in daemon mode or in
+        non-daemon mode) and then once that has finished (if ran in non-daemon
+        mode this will be when chef finishes converging, if ran in daemon mode
+        then no further actions are possible since chef will have forked into
+        its own process) then a post run function can be executed, which
+        perform finishing activities (such as removing the validation pem
+        file)."""),


The grammar in the text is a bit broken from the original authors. Here's another cut that tries to clean it up a bit.

Suggested change

This module enables chef to be installed (from packages or

from gems, or from omnibus). Before this occurs chef configurations are

written to disk (validation.pem, client.pem, firstboot.json,

client.rb), and needed chef folders/directories are created

(/etc/chef and /var/log/chef and so-on). Then once installing proceeds

correctly if configured chef will be started (in daemon mode or in

non-daemon mode) and then once that has finished (if ran in non-daemon

mode this will be when chef finishes converging, if ran in daemon mode

then no further actions are possible since chef will have forked into

its own process) then a post run function can be executed, which

perform finishing activities (such as removing the validation pem

file)."""),

This module enables chef to be installed (from packages or

from gems, or from omnibus). Before this occurs chef configurations are

written to disk (validation.pem, client.pem, firstboot.json,

client.rb), and needed chef folders/directories are created

(/etc/chef and /var/log/chef and so-on). Then once installing proceeds

correctly if configured chef will be started (in daemon mode or in

non-daemon mode) and then once that has finished (if ran in non-daemon

mode this will be when chef finishes converging, if ran in daemon mode

then no further actions are possible since chef will have forked into

its own process) then a post run function can be executed, which

perform finishing activities (such as removing the validation pem

file)."""),

blackboxsw · 2020-06-05T19:14:36Z

cloudinit/config/cc_chef.py

+                            - ``/etc/chef``
+                            - ``/var/log/chef``
+                            - ``/var/lib/chef``
+                            - ``/var/cache/chef``
+                            - ``/var/backups/chef``
+                            - ``/var/run/chef``""")


Let's source default strings from the actual global variable defined (in case this changes in the future).
Maybe something like this:

Suggested change

- ``/etc/chef``

- ``/var/log/chef``

- ``/var/lib/chef``

- ``/var/cache/chef``

- ``/var/backups/chef``

- ``/var/run/chef``""")

{chef_dirs}""".format(chef_dirs="\n".join(

[" - ``{}``".format(d) for d in CHEF_DIRS])))

blackboxsw · 2020-06-05T19:22:20Z

cloudinit/config/cc_chef.py

+                },
+                'json_attribs': {
+                    'type': 'string',
+                    'default': '/etc/chef/firstboot.json',


For any 'default values' that are defined elsewhere in a global, let's source those for the schema documentation so if they change in the future, docs are auto-updated.

Suggested change

'default': '/etc/chef/firstboot.json',

'default': CHEF_RB_TPL_DEFAULTS['json_attribs'],

blackboxsw · 2020-06-05T19:24:18Z

cloudinit/config/cc_chef.py

+                },
+                'omnibus_url': {
+                    'type': 'string',
+                    'default': 'https://www.chef.io/chef/install.sh',


Suggested change

'default': 'https://www.chef.io/chef/install.sh',

'default': OMNIBUS_URL,

blackboxsw · 2020-06-05T19:24:52Z

cloudinit/config/cc_chef.py

+                },
+                'omnibus_url_retries': {
+                    'type': 'integer',
+                    'default': 5,


Suggested change

'default': 5,

'default': OMNIBUS_URL_RETRIES,

blackboxsw · 2020-06-05T19:26:29Z

cloudinit/config/cc_chef.py

+                },
+                'ssl_verify_mode': {
+                    'type': 'string',
+                    'default': ':verify_none',


Suggested change

'default': ':verify_none',

'default': CHEF_RB_TPL_DEFAULTS['ssl_verify_mode'],

github-actions · 2020-06-20T00:12:08Z

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging powersj, and he will ensure that someone takes a look soon.

(If the pull request is closed, please do feel free to reopen it if you wish to continue working on it.)

lucasmoura · 2020-06-25T21:19:54Z

@blackboxsw Thanks for the review :) I have updated the schema with the proposed changes

blackboxsw · 2020-06-29T16:10:45Z

@lucasmoura thanks for the update looks and behaves well.

blackboxsw · 2020-06-29T16:18:40Z

https://bugs.launchpad.net/cloud-init/+bug/1885598

blackboxsw · 2020-06-29T16:19:25Z

Found the above bug in schema --annotate while testing this branch. Just a gap in schema format handling unrelated to this branch

igalic reviewed May 22, 2020

View reviewed changes

blackboxsw self-assigned this May 26, 2020

blackboxsw requested changes Jun 5, 2020

View reviewed changes

github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label Jun 20, 2020

lucasmoura added 2 commits June 25, 2020 16:11

Add schema for cc_chef module

eb29e43

Update chef schema

0186b90

lucasmoura force-pushed the add-schema-to-chef branch from 8c5e91c to 0186b90 Compare June 25, 2020 21:19

lucasmoura requested a review from blackboxsw June 25, 2020 21:20

Merge branch 'master' into add-schema-to-chef

5999054

blackboxsw approved these changes Jun 29, 2020

View reviewed changes

blackboxsw merged commit b0eba6a into canonical:master Jun 29, 2020

raharper mentioned this pull request Jul 10, 2020

Ubuntu/devel #486

Merged

This was referenced May 10, 2023

cloud-init searches for ec2 mirrors regardless of what cloud its on #2549

Closed

#include fails silently. #3069

Closed

cloud-init status broken in groovy lxd containers #3758

Closed

Release 20.3 #3774

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add schema for cc_chef module #375

Add schema for cc_chef module #375

lucasmoura commented May 20, 2020 •

edited

Loading

igalic May 22, 2020

lucasmoura May 22, 2020

OddBloke commented May 22, 2020 via email

igalic commented May 23, 2020 •

edited

Loading

blackboxsw left a comment

blackboxsw Jun 5, 2020

blackboxsw Jun 5, 2020

blackboxsw Jun 5, 2020

blackboxsw Jun 5, 2020

blackboxsw Jun 5, 2020

blackboxsw Jun 5, 2020

github-actions bot commented Jun 20, 2020

lucasmoura commented Jun 25, 2020

blackboxsw commented Jun 29, 2020

blackboxsw commented Jun 29, 2020

blackboxsw commented Jun 29, 2020

	'default': '/etc/chef/firstboot.json',
	'default': CHEF_RB_TPL_DEFAULTS['json_attribs'],

	'default': 'https://www.chef.io/chef/install.sh',
	'default': OMNIBUS_URL,

	'default': ':verify_none',
	'default': CHEF_RB_TPL_DEFAULTS['ssl_verify_mode'],

Add schema for cc_chef module #375

Add schema for cc_chef module #375

Conversation

lucasmoura commented May 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OddBloke commented May 22, 2020 via email

igalic commented May 23, 2020 • edited Loading

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

github-actions bot commented Jun 20, 2020

lucasmoura commented Jun 25, 2020

blackboxsw commented Jun 29, 2020

blackboxsw commented Jun 29, 2020

blackboxsw commented Jun 29, 2020

lucasmoura commented May 20, 2020 •

edited

Loading

igalic commented May 23, 2020 •

edited

Loading