From 3a15502ebcb20c9ff4fcd13b1d16acd84dde1d0f Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Fri, 3 Dec 2021 13:34:37 -0700 Subject: [PATCH 01/10] cloud-config annotation throws exception, fix it `cloud-init devel schema --annotate` should not throw exception, return something meaningful instead --- cloudinit/config/schema.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/cloudinit/config/schema.py b/cloudinit/config/schema.py index d32b7c01f8f..c0dbf231565 100644 --- a/cloudinit/config/schema.py +++ b/cloudinit/config/schema.py @@ -201,7 +201,15 @@ def annotated_cloudconfig_file(cloudconfig, original_content, schema_errors): cloudconfig, original_content) errors_by_line = defaultdict(list) error_footer = [] + error_header = '# Errors: -------------\n{0}\n\n' annotated_content = [] + lines = original_content.decode().split('\n') + cloud_config_body = lines[1:] + + # Return a meaningful message on empty cloud-config + if not [line for line in cloud_config_body if line.strip()]: + return error_header.format("Empty cloud-config") + for path, msg in schema_errors: match = re.match(r'format-l(?P\d+)\.c(?P\d+).*', path) if match: @@ -213,7 +221,6 @@ def annotated_cloudconfig_file(cloudconfig, original_content, schema_errors): if col is not None: msg = 'Line {line} column {col}: {msg}'.format( line=line, col=col, msg=msg) - lines = original_content.decode().split('\n') error_index = 1 for line_number, line in enumerate(lines, 1): errors = errors_by_line[line_number] @@ -224,10 +231,10 @@ def annotated_cloudconfig_file(cloudconfig, original_content, schema_errors): error_footer.append('# E{0}: {1}'.format(error_index, error)) error_index += 1 annotated_content.append(line + '\t\t# ' + ','.join(error_label)) + else: annotated_content.append(line) - annotated_content.append( - '# Errors: -------------\n{0}\n\n'.format('\n'.join(error_footer))) + annotated_content.append(error_header.format('\n'.join(error_footer))) return '\n'.join(annotated_content) From f520e5ee48d5d9cb5b503d2cb5fa239dea33fff6 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Mon, 6 Dec 2021 22:12:19 -0700 Subject: [PATCH 02/10] Warn meaningfully when cloud config body empty --- cloudinit/handlers/cloud_config.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/cloudinit/handlers/cloud_config.py b/cloudinit/handlers/cloud_config.py index 2a307364bc4..451a1a0e1cc 100644 --- a/cloudinit/handlers/cloud_config.py +++ b/cloudinit/handlers/cloud_config.py @@ -92,6 +92,9 @@ def _extract_mergers(self, payload, headers): # or the merge type from the headers or default to our own set # if neither exists (or is empty) from the later. payload_yaml = util.load_yaml(payload) + if not payload_yaml: + raise ValueError("Empty cloud config") + mergers_yaml = mergers.dict_extract_mergers(payload_yaml) mergers_header = mergers.string_extract_mergers(merge_header_headers) all_mergers = [] @@ -114,6 +117,8 @@ def _merge_patch(self, payload): def _merge_part(self, payload, headers): (payload_yaml, my_mergers) = self._extract_mergers(payload, headers) + if not payload and not my_mergers: + return LOG.debug("Merging by applying %s", my_mergers) merger = mergers.construct(my_mergers) self.cloud_buf = merger.merge(self.cloud_buf, payload_yaml) @@ -142,6 +147,10 @@ def handle_part(self, data, ctype, filename, payload, frequency, headers): for i in ("\n", "\r", "\t"): filename = filename.replace(i, " ") self.file_names.append(filename.strip()) + except ValueError as err: + LOG.warning( + "Failed at merging in cloud config part from file %s: %s", + filename, str(err)) except Exception: util.logexc(LOG, "Failed at merging in cloud config part from %s", filename) From b15ab5cf5925fbf835e5066ff713ddf70761c833 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Tue, 4 Jan 2022 09:59:52 -0700 Subject: [PATCH 03/10] cleanup --- cloudinit/handlers/cloud_config.py | 8 ++------ cloudinit/util.py | 1 + 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/cloudinit/handlers/cloud_config.py b/cloudinit/handlers/cloud_config.py index 451a1a0e1cc..c9092e48b73 100644 --- a/cloudinit/handlers/cloud_config.py +++ b/cloudinit/handlers/cloud_config.py @@ -93,7 +93,7 @@ def _extract_mergers(self, payload, headers): # if neither exists (or is empty) from the later. payload_yaml = util.load_yaml(payload) if not payload_yaml: - raise ValueError("Empty cloud config") + raise ValueError("empty yaml file") mergers_yaml = mergers.dict_extract_mergers(payload_yaml) mergers_header = mergers.string_extract_mergers(merge_header_headers) @@ -117,8 +117,6 @@ def _merge_patch(self, payload): def _merge_part(self, payload, headers): (payload_yaml, my_mergers) = self._extract_mergers(payload, headers) - if not payload and not my_mergers: - return LOG.debug("Merging by applying %s", my_mergers) merger = mergers.construct(my_mergers) self.cloud_buf = merger.merge(self.cloud_buf, payload_yaml) @@ -148,9 +146,7 @@ def handle_part(self, data, ctype, filename, payload, frequency, headers): filename = filename.replace(i, " ") self.file_names.append(filename.strip()) except ValueError as err: - LOG.warning( - "Failed at merging in cloud config part from file %s: %s", - filename, str(err)) + LOG.warning("Failed at merging in %s", str(err)) except Exception: util.logexc(LOG, "Failed at merging in cloud config part from %s", filename) diff --git a/cloudinit/util.py b/cloudinit/util.py index 1b462a38334..575c736e4ee 100644 --- a/cloudinit/util.py +++ b/cloudinit/util.py @@ -847,6 +847,7 @@ def load_yaml(blob, default=None, allowed=(dict,)): LOG.debug("Attempting to load yaml from string " "of length %s with allowed root types %s", len(blob), allowed) + converted = safeyaml.load(blob) if converted is None: LOG.debug("loaded blob returned None, returning default.") From 1f8b03dff054a5076901616f9eb87c1d3510f558 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Tue, 4 Jan 2022 10:53:39 -0700 Subject: [PATCH 04/10] some code uses an empty dict to start merging with, which causes unwanted noise in the logs - elim by checking for nonetype --- cloudinit/handlers/cloud_config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cloudinit/handlers/cloud_config.py b/cloudinit/handlers/cloud_config.py index 68176dadf9b..5de95a109af 100644 --- a/cloudinit/handlers/cloud_config.py +++ b/cloudinit/handlers/cloud_config.py @@ -89,7 +89,7 @@ def _extract_mergers(self, payload, headers): # or the merge type from the headers or default to our own set # if neither exists (or is empty) from the later. payload_yaml = util.load_yaml(payload) - if not payload_yaml: + if payload_yaml is None: raise ValueError("empty yaml file") mergers_yaml = mergers.dict_extract_mergers(payload_yaml) From 8b7bf3e2ebb5cb409a4a0c4eb7730a8e87125a49 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Tue, 4 Jan 2022 10:54:51 -0700 Subject: [PATCH 05/10] black --- cloudinit/config/schema.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/cloudinit/config/schema.py b/cloudinit/config/schema.py index e9b562181f4..befad76ca1b 100644 --- a/cloudinit/config/schema.py +++ b/cloudinit/config/schema.py @@ -223,9 +223,9 @@ def annotated_cloudconfig_file(cloudconfig, original_content, schema_errors): ) errors_by_line = defaultdict(list) error_footer = [] - error_header = '# Errors: -------------\n{0}\n\n' + error_header = "# Errors: -------------\n{0}\n\n" annotated_content = [] - lines = original_content.decode().split('\n') + lines = original_content.decode().split("\n") cloud_config_body = lines[1:] # Return a meaningful message on empty cloud-config @@ -242,7 +242,8 @@ def annotated_cloudconfig_file(cloudconfig, original_content, schema_errors): errors_by_line[schemapaths[path]].append(msg) if col is not None: msg = "Line {line} column {col}: {msg}".format( - line=line, col=col, msg=msg) + line=line, col=col, msg=msg + ) error_index = 1 for line_number, line in enumerate(lines, 1): errors = errors_by_line[line_number] From 5ebb7dd13b4ba1f09db38102ac68305cb626aedd Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Tue, 4 Jan 2022 11:23:35 -0700 Subject: [PATCH 06/10] more explicit error message --- cloudinit/handlers/cloud_config.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cloudinit/handlers/cloud_config.py b/cloudinit/handlers/cloud_config.py index 5de95a109af..c6b2c3afb94 100644 --- a/cloudinit/handlers/cloud_config.py +++ b/cloudinit/handlers/cloud_config.py @@ -90,7 +90,7 @@ def _extract_mergers(self, payload, headers): # if neither exists (or is empty) from the later. payload_yaml = util.load_yaml(payload) if payload_yaml is None: - raise ValueError("empty yaml file") + raise ValueError("empty cloud config") mergers_yaml = mergers.dict_extract_mergers(payload_yaml) mergers_header = mergers.string_extract_mergers(merge_header_headers) @@ -143,7 +143,7 @@ def handle_part(self, data, ctype, filename, payload, frequency, headers): filename = filename.replace(i, " ") self.file_names.append(filename.strip()) except ValueError as err: - LOG.warning("Failed at merging in %s", str(err)) + LOG.warning("Failed at merging in %s", err) except Exception: util.logexc( LOG, "Failed at merging in cloud config part from %s", filename From 97ef39bf39d226eceae0c623b33a013d676acf53 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Wed, 5 Jan 2022 16:12:28 -0700 Subject: [PATCH 07/10] Suggestions per blackboxsw --- cloudinit/config/schema.py | 20 ++++++++++---------- cloudinit/handlers/cloud_config.py | 6 +++++- tests/unittests/config/test_schema.py | 25 +++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 11 deletions(-) diff --git a/cloudinit/config/schema.py b/cloudinit/config/schema.py index befad76ca1b..85cad3bb665 100644 --- a/cloudinit/config/schema.py +++ b/cloudinit/config/schema.py @@ -217,21 +217,21 @@ def annotated_cloudconfig_file(cloudconfig, original_content, schema_errors): if not schema_errors: return original_content schemapaths = {} - if cloudconfig: - schemapaths = _schemapath_for_cloudconfig( - cloudconfig, original_content - ) errors_by_line = defaultdict(list) error_footer = [] error_header = "# Errors: -------------\n{0}\n\n" annotated_content = [] lines = original_content.decode().split("\n") - cloud_config_body = lines[1:] - - # Return a meaningful message on empty cloud-config - if not [line for line in cloud_config_body if line.strip()]: - return error_header.format("Empty cloud-config") - + if not isinstance(cloudconfig, dict): + # Return a meaningful message on empty cloud-config + return "\n".join( + lines + + [error_header.format("# E1: Cloud-config is not a YAML dict.")] + ) + if cloudconfig: + schemapaths = _schemapath_for_cloudconfig( + cloudconfig, original_content + ) for path, msg in schema_errors: match = re.match(r"format-l(?P\d+)\.c(?P\d+).*", path) if match: diff --git a/cloudinit/handlers/cloud_config.py b/cloudinit/handlers/cloud_config.py index c6b2c3afb94..8070c6cb226 100644 --- a/cloudinit/handlers/cloud_config.py +++ b/cloudinit/handlers/cloud_config.py @@ -143,7 +143,11 @@ def handle_part(self, data, ctype, filename, payload, frequency, headers): filename = filename.replace(i, " ") self.file_names.append(filename.strip()) except ValueError as err: - LOG.warning("Failed at merging in %s", err) + LOG.warning( + "Failed at merging in cloud config part from %s: %s", + filename, + err, + ) except Exception: util.logexc( LOG, "Failed at merging in cloud config part from %s", filename diff --git a/tests/unittests/config/test_schema.py b/tests/unittests/config/test_schema.py index fb5b891d7f1..416cc46d5bc 100644 --- a/tests/unittests/config/test_schema.py +++ b/tests/unittests/config/test_schema.py @@ -546,6 +546,31 @@ def test_annotated_cloudconfig_file_no_schema_errors(self): content, annotated_cloudconfig_file({}, content, schema_errors=[]) ) + def test_annotated_cloudconfig_file_with_non_dict_cloud_config(self): + """Error when empty non-dict cloud-config is provided. + + OurJSON validation when user-data is None type generates a bunch + schema validation errors of the format: + ('', "None is not of type 'object'"). Ignore those symptoms and + report the general problem instead. + """ + content = b"\n\n\n" + expected = "\n".join( + [ + content.decode(), + "# Errors: -------------", + "# E1: Cloud-config is not a YAML dict.\n\n", + ] + ) + self.assertEqual( + expected, + annotated_cloudconfig_file( + None, + content, + schema_errors=[("", "None is not of type 'object'")], + ), + ) + def test_annotated_cloudconfig_file_schema_annotates_and_adds_footer(self): """With schema_errors, error lines are annotated and a footer added.""" content = dedent( From 793d7dfc601d300870c6d4c7f9f1f84d1960de7c Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Thu, 6 Jan 2022 08:20:59 -0700 Subject: [PATCH 08/10] --system --annotate --- cloudinit/config/schema.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cloudinit/config/schema.py b/cloudinit/config/schema.py index 85cad3bb665..7d271385080 100644 --- a/cloudinit/config/schema.py +++ b/cloudinit/config/schema.py @@ -181,6 +181,7 @@ def validate_cloudconfig_schema( @raises: SchemaValidationError when provided config does not validate against the provided schema. + @raises: RuntimeError when provided config sourced from YAML is not a dict. """ try: (cloudinitValidator, FormatChecker) = get_jsonschema_validator() @@ -324,6 +325,10 @@ def validate_cloudconfig_file(config_path, schema, annotate=False): if annotate: print(annotated_cloudconfig_file({}, content, error.schema_errors)) raise error from e + if not isinstance(cloudconfig, dict): + # Return a meaningful message on empty cloud-config + if not annotate: + raise RuntimeError("Cloud-config is not a YAML dict.") try: validate_cloudconfig_schema(cloudconfig, schema, strict=True) except SchemaValidationError as e: From 01e2f4fbca55f1a4e95998e0813e65303ddfb0f8 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Thu, 6 Jan 2022 09:01:19 -0700 Subject: [PATCH 09/10] error when using --annotate with --docs --- cloudinit/config/schema.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cloudinit/config/schema.py b/cloudinit/config/schema.py index 7d271385080..3a77ca00973 100644 --- a/cloudinit/config/schema.py +++ b/cloudinit/config/schema.py @@ -673,6 +673,8 @@ def handle_schema_args(name, args): 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") + if args.annotate and args.docs: + error("Invalid flag combination. Cannot use --annotate with --docs") full_schema = get_schema() if args.config_file or args.system: try: From 35cbfa039e7def334fdb4ae12f75e7229e611213 Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Thu, 6 Jan 2022 11:23:05 -0700 Subject: [PATCH 10/10] add test to verify invalid flag prints error --- tests/unittests/config/test_schema.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/unittests/config/test_schema.py b/tests/unittests/config/test_schema.py index 416cc46d5bc..822efe5a176 100644 --- a/tests/unittests/config/test_schema.py +++ b/tests/unittests/config/test_schema.py @@ -683,6 +683,19 @@ def test_main_absent_config_file(self, capsys): _out, err = capsys.readouterr() assert "Error:\nConfigfile NOT_A_FILE does not exist\n" == err + def test_main_invalid_flag_combo(self, capsys): + """Main exits non-zero when invalid flag combo used.""" + myargs = ["mycmd", "--annotate", "--docs", "DOES_NOT_MATTER"] + with mock.patch("sys.argv", myargs): + with pytest.raises(SystemExit) as context_manager: + main() + assert 1 == context_manager.value.code + _, err = capsys.readouterr() + assert ( + "Error:\nInvalid flag combination. " + "Cannot use --annotate with --docs\n" == err + ) + def test_main_prints_docs(self, capsys): """When --docs parameter is provided, main generates documentation.""" myargs = ["mycmd", "--docs", "all"]