From 71ef427e0a05c4ae6488b99f79592d9ae332dbc8 Mon Sep 17 00:00:00 2001 From: Ryan Haasken Date: Thu, 3 Oct 2024 18:21:40 -0500 Subject: [PATCH 1/3] CRAYSAT-1917: Fix jinja2 rendering of boot_sets data in `sat bootprep` Make the following fixes: - Remove the `boot_set` property from `BaseInputItem`. It is not appropriate there, and it is redundant with `boot_sets` property that already exists in the `InputSessionTemplate` - Modify the `jinja_rendered` decorator to recursively render more complex objects like lists and dictionaries. I think this is safe, but we should consider any edge cases more carefully. - Remove the now unnecessary code that does a one-off Jinja2 rendering of `rootfs_provider_passthrough` in the `boot_sets`. Could still use unit test enhancements that test this new ability to render fields in the boot_sets. Test Description: Tested on a simple bootprep input file that used a variable in the `rootfs_provider_passthrough` field of a boot set in a BOS session template. --- CHANGELOG.md | 5 ++++ sat/cli/bootprep/input/base.py | 32 +++++++++++++--------- sat/cli/bootprep/input/session_template.py | 7 +---- tests/cli/bootprep/input/test_base.py | 20 +++++++++++++- 4 files changed, 44 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0cb5c1cc..0760af28 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,11 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [3.32.10] - 2024-11-04 + +### Fixed +- Fix jinja2 rendering of `boot_sets` data in `sat bootprep`. + ## [3.32.9] - 2024-10-29 ### Fixed diff --git a/sat/cli/bootprep/input/base.py b/sat/cli/bootprep/input/base.py index 3b73516d..1ee242e1 100644 --- a/sat/cli/bootprep/input/base.py +++ b/sat/cli/bootprep/input/base.py @@ -108,14 +108,25 @@ def wrapper(self, *args, **kwargs): # Default to no additional context if not set context = getattr(self, 'jinja_context', {}) - try: - return self.jinja_env.from_string(unrendered_result).render(context) - except SecurityError as err: - raise error_cls(f'Jinja2 template {unrendered_result} for value ' - f'{func.__name__} tried to access unsafe attributes.') from err - except TemplateError as err: - raise error_cls(f'Failed to render Jinja2 template {unrendered_result} ' - f'for value {func.__name__}: {err}') from err + def render_object(obj): + """Render an object with the given context.""" + if isinstance(obj, str): + try: + return self.jinja_env.from_string(obj).render(context) + except SecurityError as err: + raise error_cls(f'Jinja2 template {obj} for value ' + f'{func.__name__} tried to access unsafe attributes.') from err + except TemplateError as err: + raise error_cls(f'Failed to render Jinja2 template {obj} ' + f'for value {func.__name__}: {err}') from err + elif isinstance(obj, dict): + return {key: render_object(value) for key, value in obj.items()} + elif isinstance(obj, list): + return [render_object(item) for item in obj] + else: + return obj + + return render_object(unrendered_result) return wrapper @@ -242,11 +253,6 @@ def name(self): # items that inherit from BaseInputItem. return self.data['name'] - @property - def boot_set(self): - """Return the full boot_sets dictionary.""" - return self.data.get('bos_parameters', {}).get('boot_sets', {}) - def __str__(self): # Since the name can be rendered, and when unrendered, it does not need # to be unique, just refer to this item by its index in the instance. diff --git a/sat/cli/bootprep/input/session_template.py b/sat/cli/bootprep/input/session_template.py index 7febcb02..4fc48353 100644 --- a/sat/cli/bootprep/input/session_template.py +++ b/sat/cli/bootprep/input/session_template.py @@ -98,6 +98,7 @@ def configuration(self): return self.data['configuration'] @property + @jinja_rendered def boot_sets(self): """dict: the boot sets specified for the session template""" # the 'bos_parameters' property is required, and 'boot_sets' is required within that @@ -186,12 +187,6 @@ def get_create_item_data(self): boot_set_api_data.update(boot_set_data) api_data['boot_sets'][boot_set_name] = boot_set_api_data - # Fetch full boot sets - boot_sets = self.boot_set - # Render the rootfs_provider_passthrough using Jinja2 - rendered_rootfs = self.jinja_env.from_string( - boot_sets[boot_set_name]['rootfs_provider_passthrough']).render(self.data) - api_data['boot_sets'][boot_set_name]['rootfs_provider_passthrough'] = rendered_rootfs return api_data def get_image_record_by_id(self, ims_image_id): diff --git a/tests/cli/bootprep/input/test_base.py b/tests/cli/bootprep/input/test_base.py index 3600db63..7814b024 100644 --- a/tests/cli/bootprep/input/test_base.py +++ b/tests/cli/bootprep/input/test_base.py @@ -1,7 +1,7 @@ # # MIT License # -# (C) Copyright 2022-2023 Hewlett Packard Enterprise Development LP +# (C) Copyright 2022-2024 Hewlett Packard Enterprise Development LP # # Permission is hereby granted, free of charge, to any person obtaining a # copy of this software and associated documentation files (the "Software"), @@ -26,6 +26,7 @@ """ import unittest +from jinja2 import Template from sat.cli.bootprep.input.base import BaseInputItem from sat.constants import MISSING_VALUE @@ -57,6 +58,12 @@ def get_create_item_data(self): def create(self, payload): return + + def render_jinja_template(self, template_str): + """Render a Jinja template with provided data.""" + template = Template(template_str) + return template.render(**self.data) + return SomeTestItem @@ -84,3 +91,14 @@ def test_missing_report_attrs_are_missing(self): report_row = item_cls(data).report_row() self.assertEqual(len(report_row), len(row_headings)) self.assertEqual(report_row[-1], MISSING_VALUE) + + def test_jinja_template_rendering(self): + """Test Jinja template rendering with data from the input item.""" + item_cls = create_input_item_cls(['name', 'type']) + data = {'name': 'foo', 'type': 'configuration'} + item = item_cls(data) + + template_str = "name: {{name}} and type: {{type}}" + rendered_output = item.render_jinja_template(template_str) + expected_output = "name: foo and type: configuration" + self.assertEqual(rendered_output, expected_output) From 6e42a20a1e7ec6911e230103cdb7aaef08c53437 Mon Sep 17 00:00:00 2001 From: Ryan Haasken Date: Tue, 5 Nov 2024 16:14:08 -0600 Subject: [PATCH 2/3] CRAYSAT-1917: Add unit tests for `jinja_rendered` decorator Add unit tests for usage of the `jinja_rendered` decorator on properties of the `BaseInputItem`. Modify these unit tests to simplify them slightly and to ensure that the `SomeTestItem` class calls the `BaseInputItem` constructor, so that it sets the `jinja_env` instance attribute, which is used by the `jinja_rendered` decorator. Test Description: Unit tests pass. --- tests/cli/bootprep/input/test_base.py | 180 +++++++++++++++++++------- 1 file changed, 132 insertions(+), 48 deletions(-) diff --git a/tests/cli/bootprep/input/test_base.py b/tests/cli/bootprep/input/test_base.py index 7814b024..3ef87dcf 100644 --- a/tests/cli/bootprep/input/test_base.py +++ b/tests/cli/bootprep/input/test_base.py @@ -26,32 +26,21 @@ """ import unittest -from jinja2 import Template +from unittest.mock import MagicMock -from sat.cli.bootprep.input.base import BaseInputItem +from jinja2.sandbox import SandboxedEnvironment + +from sat.cli.bootprep.input.base import BaseInputItem, jinja_rendered +from sat.cli.bootprep.input.instance import InputInstance from sat.constants import MISSING_VALUE -def create_input_item_cls(cls_report_attrs): +def create_input_item_cls(): class SomeTestItem(BaseInputItem): """A mock concrete BaseInputItem class""" - description = 'some input item' - report_attrs = cls_report_attrs - - def __init__(self, data): - # Since the test input item class doesn't need access - # to the input instance, we can just ignore it here. - # As a consequence, other methods on this class won't - # work as expected. - self.data = data - - def __getattr__(self, item): - if item in cls_report_attrs: - try: - return self.data[item] - except KeyError as err: - raise AttributeError(str(err)) + description = 'some test item' + report_attrs = ['name', 'type', 'complex_data'] def get_create_item_data(self): return @@ -59,46 +48,141 @@ def get_create_item_data(self): def create(self, payload): return - def render_jinja_template(self, template_str): - """Render a Jinja template with provided data.""" - template = Template(template_str) - return template.render(**self.data) + @property + @jinja_rendered + def name(self): + return self.data.get('name') + + @property + @jinja_rendered + def type(self): + return self.data.get('type') + + @property + @jinja_rendered + def complex_data(self): + return self.data.get('complex_data') return SomeTestItem class TestBaseInputItem(unittest.TestCase): + + def setUp(self): + self.mock_instance = MagicMock(spec=InputInstance) + self.jinja_env = SandboxedEnvironment() + self.jinja_env.globals = { + 'default': { + 'note': 'foo', + 'suffix': '-bar', + 'system_name': 'ryan', + 'site-domain': 'example.com' + } + } + def test_report_rows_have_correct_attrs(self): """Test that report rows are generated for input items""" - row_headings = ['name', 'type'] - row_vals = ['foo', 'configuration'] - item_cls = create_input_item_cls(row_headings) - self.assertEqual(item_cls(dict(zip(row_headings, row_vals))).report_row(), - row_vals) + data = {'name': 'foo', 'type': 'configuration', 'complex_data': 'columbo'} + item_cls = create_input_item_cls() + self.assertEqual(['foo', 'configuration', 'columbo'], + item_cls(data, self.mock_instance, 0, self.jinja_env).report_row()) def test_report_rows_excluded(self): """Test that report rows are not included if not present in report_attrs""" - item_cls = create_input_item_cls(['name']) - self.assertEqual(item_cls({'name': 'foo', 'something_else': 'another_thing'}).report_row(), ['foo']) + data = {'name': 'foo', 'something_else': 'another_thing'} + item_cls = create_input_item_cls() + item_cls.report_attrs = ['name'] + self.assertEqual(item_cls(data, self.mock_instance, 0, self.jinja_env).report_row(), ['foo']) def test_missing_report_attrs_are_missing(self): """Test that missing report attrs are reported as MISSING""" - row_headings = ['name', 'type', 'one_more_thing'] - row_vals = ['foo', 'configuration', 'columbo'] - item_cls = create_input_item_cls(row_headings) # Cut off last report attr - data = dict(zip(row_headings[:-1], row_vals[:-1])) - - report_row = item_cls(data).report_row() - self.assertEqual(len(report_row), len(row_headings)) - self.assertEqual(report_row[-1], MISSING_VALUE) - - def test_jinja_template_rendering(self): - """Test Jinja template rendering with data from the input item.""" - item_cls = create_input_item_cls(['name', 'type']) + # This data is missing a value for the 'complex_data' key data = {'name': 'foo', 'type': 'configuration'} - item = item_cls(data) - - template_str = "name: {{name}} and type: {{type}}" - rendered_output = item.render_jinja_template(template_str) - expected_output = "name: foo and type: configuration" - self.assertEqual(rendered_output, expected_output) + item_cls = create_input_item_cls() + item_cls.report_attrs = item_cls.report_attrs + ['nonexistent_property'] + report_row = item_cls(data, self.mock_instance, 0, self.jinja_env).report_row() + self.assertEqual(['foo', 'configuration', None, MISSING_VALUE], report_row) + + def test_jinja_rendered_string_value(self): + """Test that jinja_rendered works on a string value""" + data = {'name': 'foo{{default.suffix}}', 'type': 'configuration-{{default.system_name}}'} + item_cls = create_input_item_cls() + item = item_cls(data, self.mock_instance, 0, self.jinja_env) + self.assertEqual('foo-bar', item.name) + self.assertEqual('configuration-ryan', item.type) + + def test_jinja_rendered_complex_value(self): + """Test that jinja_rendered works on a more complex value""" + data = { + 'name': 'foo', + 'type': 'configuration', + 'complex_data': { + 'kitchen': { + 'furniture': ['{{furniture.brand}} table', '{{furniture.brand}} chair'], + 'appliances': ['oven', 'fridge'], + 'color': '{{paint.primary}}' + }, + 'living_room': { + 'furniture': ['couch', 'chair', 'rug'], + 'appliances': ['lamp', 'tv'], + 'color': '{{paint.secondary}}' + } + } + } + self.jinja_env.globals = { + 'furniture': { + 'brand': 'IKEA' + }, + 'paint': { + 'primary': 'blue', + 'secondary': 'red' + } + } + item_cls = create_input_item_cls() + item = item_cls(data, self.mock_instance, 0, self.jinja_env) + self.assertEqual( + { + 'kitchen': { + 'furniture': ['IKEA table', 'IKEA chair'], + 'appliances': ['oven', 'fridge'], + 'color': 'blue' + }, + 'living_room': { + 'furniture': ['couch', 'chair', 'rug'], + 'appliances': ['lamp', 'tv'], + 'color': 'red' + } + }, + item.complex_data + ) + + def test_jinja_template_rendering_undefined_variable(self): + """Test jinja_rendered property that uses an undefined variable.""" + item_cls = create_input_item_cls() + data = {'name': '{{undefined_variable}}', 'type': '{{nested.undefined_variable}}'} + item = item_cls(data, self.mock_instance, 0, self.jinja_env) + + self.assertEqual('', item.name) + err_regex = (r"Failed to render Jinja2 template {{nested\.undefined_variable}} " + r"for value type: 'nested' is undefined") + with self.assertRaisesRegex(item_cls.template_render_err, err_regex): + item.type + + def test_jinja_template_rendering_undefined_variable_nested(self): + """Test jinja_rendered property that uses an undefined variable in a nested field.""" + item_cls = create_input_item_cls() + data = { + 'name': 'foo', + 'type': 'configuration', + 'complex_data': { + 'kitchen': { + 'color': '{{colors.primary}}' + } + } + } + item = item_cls(data, self.mock_instance, 0, self.jinja_env) + + err_regex = (r"Failed to render Jinja2 template {{colors.primary}} " + r"for value complex_data: 'colors' is undefined") + with self.assertRaisesRegex(item_cls.template_render_err, err_regex): + item.complex_data From 8ff28747060a77386ecd38f11dc89d5c7e69d154 Mon Sep 17 00:00:00 2001 From: Ryan Haasken Date: Wed, 6 Nov 2024 08:28:08 -0600 Subject: [PATCH 3/3] CRAYSAT-1917: Update unit tests for jinja_rendered support of boot_sets Update the unit tests of the InputSessionTemplate class to include tests that check for proper Jinja2 template rendering of the boot_sets field. --- .../bootprep/input/test_session_template.py | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/tests/cli/bootprep/input/test_session_template.py b/tests/cli/bootprep/input/test_session_template.py index e97fbe22..26916867 100644 --- a/tests/cli/bootprep/input/test_session_template.py +++ b/tests/cli/bootprep/input/test_session_template.py @@ -38,6 +38,12 @@ def setUp(self): self.input_instance = Mock() self.index = 0 self.jinja_env = SandboxedEnvironment() + self.jinja_env.globals = { + 'default': { + 'system_name': 'drax', + 'site_domain': 'example.com', + } + } self.bos_client = Mock() self.mock_base_boot_set_data = { 'rootfs_provider': 'cpss3' @@ -93,16 +99,20 @@ def get_input_and_expected_bos_data(self, name='my-session-template', for boot_set_name, boot_set_arch in arch_by_bootset.items(): input_bootsets[boot_set_name] = { 'node_roles_groups': ['Compute'], - 'rootfs_provider_passthrough': "dvs:api-gw-service-nmn.local:300:hsn0,nmn0:0" + 'rootfs_provider': 'sbps', + 'rootfs_provider_passthrough': ('sbps:v1:iqn.2023-06.csm.iscsi:_sbps-hsn._tcp.' + '{{default.system_name}}.{{default.site_domain}}:300') } - bos_payload_bootsets[boot_set_name] = { + bos_payload_bootsets[boot_set_name] = self.mock_base_boot_set_data.copy() + bos_payload_bootsets[boot_set_name].update({ 'node_roles_groups': ['Compute'], - 'rootfs_provider_passthrough': "dvs:api-gw-service-nmn.local:300:hsn0,nmn0:0", + 'rootfs_provider': 'sbps', + 'rootfs_provider_passthrough': ('sbps:v1:iqn.2023-06.csm.iscsi:_sbps-hsn._tcp.' + 'drax.example.com:300'), 'path': self.mock_path, 'etag': self.mock_etag, 'type': self.mock_image_type - } - bos_payload_bootsets[boot_set_name].update(self.mock_base_boot_set_data) + }) if boot_set_arch: input_bootsets[boot_set_name]['arch'] = boot_set_arch bos_payload_bootsets[boot_set_name]['arch'] = boot_set_arch