-
Notifications
You must be signed in to change notification settings - Fork 1.1k
cli: add --system param to allow validating system user-data on a machine #575
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
48c7e79
b364ce5
b3d7bcd
5187f62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| # This file is part of cloud-init. See LICENSE file for license information. | ||
| """schema.py: Set of module functions for processing cloud-config schema.""" | ||
|
|
||
| from cloudinit.cmd.devel import read_cfg_paths | ||
| from cloudinit import importer | ||
| from cloudinit.util import find_modules, load_file | ||
|
|
||
|
|
@@ -173,17 +174,33 @@ def annotated_cloudconfig_file(cloudconfig, original_content, schema_errors): | |
| def validate_cloudconfig_file(config_path, schema, annotate=False): | ||
| """Validate cloudconfig file adheres to a specific jsonschema. | ||
|
|
||
| @param config_path: Path to the yaml cloud-config file to parse. | ||
| @param config_path: Path to the yaml cloud-config file to parse, or None | ||
| to default to system userdata from Paths object. | ||
| @param schema: Dict describing a valid jsonschema to validate against. | ||
| @param annotate: Boolean set True to print original config file with error | ||
| annotations on the offending lines. | ||
|
|
||
| @raises SchemaValidationError containing any of schema_errors encountered. | ||
| @raises RuntimeError when config_path does not exist. | ||
| """ | ||
| if not os.path.exists(config_path): | ||
| raise RuntimeError('Configfile {0} does not exist'.format(config_path)) | ||
| content = load_file(config_path, decode=False) | ||
| if config_path is None: | ||
| # Use system's raw userdata path | ||
| if os.getuid() != 0: | ||
| raise RuntimeError( | ||
| "Unable to read system userdata as non-root user." | ||
| " Try using sudo" | ||
| ) | ||
| paths = read_cfg_paths() | ||
| user_data_file = paths.get_ipath_cur("userdata_raw") | ||
| content = load_file(user_data_file, decode=False) | ||
| else: | ||
| if not os.path.exists(config_path): | ||
| raise RuntimeError( | ||
| 'Configfile {0} does not exist'.format( | ||
| config_path | ||
| ) | ||
| ) | ||
| content = load_file(config_path, decode=False) | ||
| if not content.startswith(CLOUD_CONFIG_HEADER): | ||
| errors = ( | ||
| ('format-l1.c1', 'File {0} needs to begin with "{1}"'.format( | ||
|
|
@@ -425,6 +442,8 @@ def get_parser(parser=None): | |
| description='Validate cloud-config files or document schema') | ||
| parser.add_argument('-c', '--config-file', | ||
| help='Path of the cloud-config yaml file to validate') | ||
| parser.add_argument('--system', action='store_true', default=False, | ||
| help='Validate the system cloud-config userdata') | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't there be associated doc updates with adding a new argument to UI? |
||
| parser.add_argument('-d', '--docs', nargs='+', | ||
| help=('Print schema module docs. Choices: all or' | ||
| ' space-delimited cc_names.')) | ||
|
|
@@ -435,11 +454,11 @@ def get_parser(parser=None): | |
|
|
||
| def handle_schema_args(name, args): | ||
| """Handle provided schema args and perform the appropriate actions.""" | ||
| exclusive_args = [args.config_file, args.docs] | ||
| if not any(exclusive_args) or all(exclusive_args): | ||
| error('Expected either --config-file argument or --docs') | ||
| exclusive_args = [args.config_file, args.docs, args.system] | ||
| if len([arg for arg in exclusive_args if arg]) != 1: | ||
| error('Expected one of --config-file, --system or --docs arguments') | ||
| full_schema = get_schema() | ||
| if args.config_file: | ||
| if args.config_file or args.system: | ||
| try: | ||
| validate_cloudconfig_file( | ||
| args.config_file, full_schema, args.annotate) | ||
|
|
@@ -449,7 +468,11 @@ def handle_schema_args(name, args): | |
| except RuntimeError as e: | ||
| error(str(e)) | ||
| else: | ||
| print("Valid cloud-config file {0}".format(args.config_file)) | ||
| if args.config_file is None: | ||
| cfg_name = "system userdata" | ||
| else: | ||
| cfg_name = args.config_file | ||
| print("Valid cloud-config:", cfg_name) | ||
| elif args.docs: | ||
| schema_ids = [subschema['id'] for subschema in full_schema['allOf']] | ||
| schema_ids += ['all'] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -141,12 +141,12 @@ that can validate your user data offline. | |
|
|
||
| .. _validate-yaml.py: https://github.com/canonical/cloud-init/blob/master/tools/validate-yaml.py | ||
|
|
||
| Another option is to run the following on an instance when debugging: | ||
| Another option is to run the following on an instance to debug userdata | ||
| provided to the system: | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the doc update but it seemed an edit to an existing doc and didn't seem clear to me.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, devel commands, and general CLI documentation, isn't great. In the devel case I had originally left it fairly undocumented as these
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That said, I did just push a commit that renders cmdline devel schema to this branch. I think we can build on this if an acceptable format for all CLI commands. |
||
| .. code-block:: shell-session | ||
|
|
||
| $ sudo cloud-init query userdata > user-data.yaml | ||
| $ cloud-init devel schema -c user-data.yaml --annotate | ||
| $ cloud-init devel schema --system --annotate | ||
|
|
||
| As launching instances in the cloud can cost money and take a bit longer, | ||
| sometimes it is easier to launch instances locally using Multipass or LXD: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,9 +9,9 @@ | |
| from cloudinit.tests.helpers import CiTestCase, mock, skipUnlessJsonSchema | ||
|
|
||
| from copy import copy | ||
| import itertools | ||
| import os | ||
| import pytest | ||
| from io import StringIO | ||
| from pathlib import Path | ||
| from textwrap import dedent | ||
| from yaml import safe_load | ||
|
|
@@ -400,50 +400,97 @@ def test_annotated_cloudconfig_file_annotates_separate_line_items(self): | |
| annotated_cloudconfig_file(parsed_config, content, schema_errors)) | ||
|
|
||
|
|
||
| class MainTest(CiTestCase): | ||
| class TestMain: | ||
|
|
||
| def test_main_missing_args(self): | ||
| exclusive_combinations = itertools.combinations( | ||
| ["--system", "--docs all", "--config-file something"], 2 | ||
| ) | ||
|
|
||
| @pytest.mark.parametrize("params", exclusive_combinations) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: a not-particularly-important gap here is all three being specified at once. |
||
| def test_main_exclusive_args(self, params, capsys): | ||
| """Main exits non-zero and error on required exclusive args.""" | ||
| params = list(itertools.chain(*[a.split() for a in params])) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A nit: it took me a while to grok what this was doing; I think In [12]: params = ('--docs all', '--config-file something')
In [13]: list(itertools.chain(*[a.split() for a in params]))
Out[13]: ['--docs', 'all', '--config-file', 'something']
In [14]: " ".join(params).split()
Out[14]: ['--docs', 'all', '--config-file', 'something'] |
||
| with mock.patch('sys.argv', ['mycmd'] + params): | ||
| with pytest.raises(SystemExit) as context_manager: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A nit: the pytest docs use We also broadly follow the pattern in UA client and pycloudlib, though there is more divergence there. |
||
| main() | ||
| assert 1 == context_manager.value.code | ||
|
|
||
| _out, err = capsys.readouterr() | ||
| expected = ( | ||
| 'Expected one of --config-file, --system or --docs arguments\n' | ||
| ) | ||
| assert expected == err | ||
|
|
||
| def test_main_missing_args(self, capsys): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A nit (unless I'm wrong, then not even that): you could potentially fold this into the above definition by making the parameters |
||
| """Main exits non-zero and reports an error on missing parameters.""" | ||
| with mock.patch('sys.argv', ['mycmd']): | ||
| with mock.patch('sys.stderr', new_callable=StringIO) as m_stderr: | ||
| with self.assertRaises(SystemExit) as context_manager: | ||
| main() | ||
| self.assertEqual(1, context_manager.exception.code) | ||
| self.assertEqual( | ||
| 'Expected either --config-file argument or --docs\n', | ||
| m_stderr.getvalue()) | ||
| with pytest.raises(SystemExit) as context_manager: | ||
| main() | ||
| assert 1 == context_manager.value.code | ||
|
|
||
| _out, err = capsys.readouterr() | ||
| expected = ( | ||
| 'Expected one of --config-file, --system or --docs arguments\n' | ||
| ) | ||
| assert expected == err | ||
|
|
||
| def test_main_absent_config_file(self): | ||
| def test_main_absent_config_file(self, capsys): | ||
| """Main exits non-zero when config file is absent.""" | ||
| myargs = ['mycmd', '--annotate', '--config-file', 'NOT_A_FILE'] | ||
| with mock.patch('sys.argv', myargs): | ||
| with mock.patch('sys.stderr', new_callable=StringIO) as m_stderr: | ||
| with self.assertRaises(SystemExit) as context_manager: | ||
| main() | ||
| self.assertEqual(1, context_manager.exception.code) | ||
| self.assertEqual( | ||
| 'Configfile NOT_A_FILE does not exist\n', | ||
| m_stderr.getvalue()) | ||
| with pytest.raises(SystemExit) as context_manager: | ||
| main() | ||
| assert 1 == context_manager.value.code | ||
| _out, err = capsys.readouterr() | ||
| assert 'Configfile NOT_A_FILE does not exist\n' == err | ||
|
|
||
| def test_main_prints_docs(self): | ||
| def test_main_prints_docs(self, capsys): | ||
| """When --docs parameter is provided, main generates documentation.""" | ||
| myargs = ['mycmd', '--docs', 'all'] | ||
| with mock.patch('sys.argv', myargs): | ||
| with mock.patch('sys.stdout', new_callable=StringIO) as m_stdout: | ||
| self.assertEqual(0, main(), 'Expected 0 exit code') | ||
| self.assertIn('\nNTP\n---\n', m_stdout.getvalue()) | ||
| self.assertIn('\nRuncmd\n------\n', m_stdout.getvalue()) | ||
| assert 0 == main(), 'Expected 0 exit code' | ||
| out, _err = capsys.readouterr() | ||
| assert '\nNTP\n---\n' in out | ||
| assert '\nRuncmd\n------\n' in out | ||
|
|
||
| def test_main_validates_config_file(self): | ||
| def test_main_validates_config_file(self, tmpdir, capsys): | ||
| """When --config-file parameter is provided, main validates schema.""" | ||
| myyaml = self.tmp_path('my.yaml') | ||
| myargs = ['mycmd', '--config-file', myyaml] | ||
| write_file(myyaml, b'#cloud-config\nntp:') # shortest ntp schema | ||
| myyaml = tmpdir.join('my.yaml') | ||
| myargs = ['mycmd', '--config-file', myyaml.strpath] | ||
| myyaml.write(b'#cloud-config\nntp:') # shortest ntp schema | ||
| with mock.patch('sys.argv', myargs): | ||
| with mock.patch('sys.stdout', new_callable=StringIO) as m_stdout: | ||
| self.assertEqual(0, main(), 'Expected 0 exit code') | ||
| self.assertIn( | ||
| 'Valid cloud-config file {0}'.format(myyaml), m_stdout.getvalue()) | ||
| assert 0 == main(), 'Expected 0 exit code' | ||
| out, _err = capsys.readouterr() | ||
| assert 'Valid cloud-config: {0}\n'.format(myyaml) == out | ||
|
|
||
| @mock.patch('cloudinit.config.schema.read_cfg_paths') | ||
| @mock.patch('cloudinit.config.schema.os.getuid', return_value=0) | ||
| def test_main_validates_system_userdata( | ||
| self, m_getuid, m_read_cfg_paths, capsys, paths | ||
| ): | ||
| """When --system is provided, main validates system userdata.""" | ||
| m_read_cfg_paths.return_value = paths | ||
| ud_file = paths.get_ipath_cur("userdata_raw") | ||
| write_file(ud_file, b'#cloud-config\nntp:') | ||
| myargs = ['mycmd', '--system'] | ||
| with mock.patch('sys.argv', myargs): | ||
| assert 0 == main(), 'Expected 0 exit code' | ||
| out, _err = capsys.readouterr() | ||
| assert 'Valid cloud-config: system userdata\n' == out | ||
|
|
||
| @mock.patch('cloudinit.config.schema.os.getuid', return_value=1000) | ||
| def test_main_system_userdata_requires_root(self, m_getuid, capsys, paths): | ||
| """Non-root user can't use --system param""" | ||
| myargs = ['mycmd', '--system'] | ||
| with mock.patch('sys.argv', myargs): | ||
| with pytest.raises(SystemExit) as context_manager: | ||
| main() | ||
| assert 1 == context_manager.value.code | ||
| _out, err = capsys.readouterr() | ||
| expected = ( | ||
| 'Unable to read system userdata as non-root user. Try using sudo\n' | ||
| ) | ||
| assert expected == err | ||
|
|
||
|
|
||
| class CloudTestsIntegrationTest(CiTestCase): | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is always root:root-owned by default and not world-readable, so this is not even a nit: we could instead rely on the user's permissions to determine if they can use this path.