From 8b3787541a6b0eb4a61a1a34ed6630f85337bec5 Mon Sep 17 00:00:00 2001 From: Maor Friedman Date: Tue, 11 Aug 2026 13:10:50 +0300 Subject: [PATCH 1/4] Add --plugin option to operator-versions reconcile command Allows reconciling operator versions for all operators defined in a plugin descriptor (plugins//plugin.yaml) without manually specifying each operator's details, reusing the same list-reconcile path as --use-defaults. Reimplemented on top of the current src-layout against main; carries over the security hardening from the original PR #422 review (plugin-name path traversal validation, sanitized YAML error messages, and rejecting plugins with installOperators: false). Assisted-by: Claude Code --- src/enclave/reconcile/cli.py | 160 ++++++++++++++++++++++++++++------- src/tests/test_cli.py | 117 +++++++++++++++++++++++++ 2 files changed, 247 insertions(+), 30 deletions(-) diff --git a/src/enclave/reconcile/cli.py b/src/enclave/reconcile/cli.py index 9b58b28bf..73d1d3517 100644 --- a/src/enclave/reconcile/cli.py +++ b/src/enclave/reconcile/cli.py @@ -26,6 +26,111 @@ def defaults_path(filename: str) -> Path: return path +def plugin_descriptor_path(plugin_name: str) -> Path: + """Resolve the plugin.yaml path for a plugin name. + + Plugins are not packaged into the built wheel, so this only resolves + against a repo checkout: src/enclave/reconcile/cli.py → src/enclave/ → + repo_root/plugins//plugin.yaml. + + Raises: + click.ClickException: If plugin_name is empty or could escape the + plugins directory (contains '..' or a path separator). + """ + if ( + not plugin_name + or "/" in plugin_name + or "\\" in plugin_name + or ".." in plugin_name + ): + raise click.ClickException( + f"Invalid plugin name: {plugin_name!r}. Plugin name must be a " + "simple name without path separators or '..'." + ) + enclave_pkg = Path(__file__).resolve().parent.parent + return enclave_pkg.parent.parent / "plugins" / plugin_name / "plugin.yaml" + + +def _reconcile_operators_from_list( + operators: list[dict[str, object]], dry_run: bool +) -> None: + """Call operator_versions_reconcile for each operator in a list. + + Each operator dict must have 'name', 'version', 'namespace', and + optionally 'csvNames' (defaults to [name] when absent). + """ + for op in operators: + op_name = cast("str", op["name"]) + op_csv_names = cast("list[str] | None", op.get("csvNames")) or [op_name] + operator_versions_reconcile( + cast("str", op["version"]), + cast("str", op["namespace"]), + op_csv_names, + dry_run, + ) + + +def _load_defaults_operators() -> list[dict[str, object]]: + """Load the operators list from defaults/operators.yaml. + + Raises: + click.ClickException: If the file is missing, not valid YAML, or + does not contain an 'operators' list. + """ + defaults_file = defaults_path("operators.yaml") + try: + with defaults_file.open(encoding="utf-8") as fh: + data = yaml.safe_load(fh) + except FileNotFoundError as exc: + raise click.ClickException( + f"{defaults_file} not found; run from the repo root" + ) from exc + except yaml.YAMLError as exc: + raise click.ClickException(f"Failed to parse {defaults_file}") from exc + + if not isinstance(data, dict) or not isinstance(data.get("operators"), list): + raise click.ClickException( + f"{defaults_file} does not contain an 'operators' list" + ) + return cast("list[dict[str, object]]", data["operators"]) + + +def _load_plugin_operators(plugin_name: str) -> list[dict[str, object]]: + """Load the operators list from a plugin's plugin.yaml descriptor. + + Raises: + click.ClickException: If the plugin name is invalid, the plugin + descriptor is missing or not valid YAML, the plugin has + installOperators set to false, or it defines no operators. + """ + plugin_file = plugin_descriptor_path(plugin_name) + try: + with plugin_file.open(encoding="utf-8") as fh: + data = yaml.safe_load(fh) + except FileNotFoundError as exc: + raise click.ClickException( + f"Plugin {plugin_name!r} not found; check plugin name or run " + "from the repo root" + ) from exc + except yaml.YAMLError as exc: + raise click.ClickException(f"Failed to parse {plugin_file}") from exc + + if not isinstance(data, dict): + raise click.ClickException(f"{plugin_file} is empty or not a mapping") + + if data.get("installOperators") is False: + raise click.ClickException( + f"Plugin {plugin_name!r} has installOperators set to false" + ) + + operators = data.get("operators") + if not isinstance(operators, list) or not operators: + raise click.ClickException( + f"Plugin {plugin_name!r} has no operators defined in {plugin_file}" + ) + return cast("list[dict[str, object]]", operators) + + @click.group(cls=KubeconfigGroup) @click.option( "--log-level", @@ -53,7 +158,11 @@ def cli(log_level: str) -> None: "--use-defaults", is_flag=True, default=False, - help="Load all operators from defaults/operators.yaml (mutually exclusive with --name, --version, --namespace, --csv-name)", + help="Load all operators from defaults/operators.yaml (mutually exclusive with --name, --version, --namespace, --csv-name, --plugin)", +) +@click.option( + "--plugin", + help="Load operators from plugins//plugin.yaml (mutually exclusive with --name, --version, --namespace, --csv-name, --use-defaults)", ) @click.option("--dry-run/--no-dry-run", default=False) def operator_versions( @@ -62,44 +171,35 @@ def operator_versions( namespace: str, csv_names: tuple[str, ...], use_defaults: bool, + plugin: str | None, dry_run: bool, ) -> None: - if use_defaults and any([name, version, namespace, csv_names]): + if use_defaults and any([name, version, namespace, csv_names, plugin]): raise click.UsageError( - "--use-defaults is mutually exclusive with --name, --version, --namespace, --csv-name" + "--use-defaults is mutually exclusive with --name, --version, --namespace, --csv-name, --plugin" ) - if not use_defaults: - missing = [ - f"--{f}" - for f, v in [("name", name), ("version", version), ("namespace", namespace)] - if not v - ] - if missing: - raise click.UsageError(f"Missing option(s): {', '.join(missing)}") - operator_versions_reconcile( - version, namespace, list(csv_names) or [name], dry_run + if plugin and any([name, version, namespace, csv_names]): + raise click.UsageError( + "--plugin is mutually exclusive with --name, --version, --namespace, --csv-name" ) - return - defaults_file = defaults_path("operators.yaml") + if plugin: + _reconcile_operators_from_list(_load_plugin_operators(plugin), dry_run) + return - try: - with defaults_file.open(encoding="utf-8") as fh: - operators = yaml.safe_load(fh)["operators"] - except FileNotFoundError as exc: - raise click.ClickException( - f"{defaults_file} not found; run from the repo root" - ) from exc - except (yaml.YAMLError, KeyError) as exc: - raise click.ClickException(f"Failed to parse {defaults_file}: {exc}") from exc + if use_defaults: + _reconcile_operators_from_list(_load_defaults_operators(), dry_run) + return - for op in operators: - op_name: str = op["name"] - op_csv_names: list[str] = op.get("csvNames") or [op_name] - operator_versions_reconcile( - op["version"], op["namespace"], op_csv_names, dry_run - ) + missing = [ + f"--{f}" + for f, v in [("name", name), ("version", version), ("namespace", namespace)] + if not v + ] + if missing: + raise click.UsageError(f"Missing option(s): {', '.join(missing)}") + operator_versions_reconcile(version, namespace, list(csv_names) or [name], dry_run) @cli.command(no_args_is_help=True) diff --git a/src/tests/test_cli.py b/src/tests/test_cli.py index 5001e84ad..a15a46c5b 100644 --- a/src/tests/test_cli.py +++ b/src/tests/test_cli.py @@ -24,6 +24,7 @@ def test_operator_versions_help() -> None: assert "--csv-name" in result.output assert "--dry-run" in result.output assert "--use-defaults" in result.output + assert "--plugin" in result.output assert "--operators" not in result.output @@ -149,6 +150,122 @@ def test_operator_versions_missing_required_without_defaults() -> None: assert "Missing option" in result.output +def test_operator_versions_plugin_single_operator(mocker: MockerFixture) -> None: + mock_reconcile = mocker.patch("enclave.reconcile.cli.operator_versions_reconcile") + dry_run = True + result = CliRunner().invoke( + cli, ["operator-versions", "--plugin", "lvms", "--dry-run"], env=_KC + ) + assert result.exit_code == 0, result.output + mock_reconcile.assert_called_once_with( + "4.20.0", "openshift-storage", ["lvms-operator"], dry_run + ) + + +def test_operator_versions_plugin_multiple_operators(mocker: MockerFixture) -> None: + mock_reconcile = mocker.patch("enclave.reconcile.cli.operator_versions_reconcile") + dry_run = True + result = CliRunner().invoke( + cli, ["operator-versions", "--plugin", "odf", "--dry-run"], env=_KC + ) + assert result.exit_code == 0, result.output + assert mock_reconcile.call_count == 1 + mock_reconcile.assert_any_call( + "4.20.7-rhodf", + "openshift-storage", + [ + "odf-operator", + "odf-dependencies", + "odf-csi-addons-operator", + "rook-ceph-operator", + "ocs-operator", + "recipe", + "mcg-operator", + "odf-prometheus-operator", + "ocs-client-operator", + "cephcsi-operator", + "odf-external-snapshotter-operator", + ], + dry_run, + ) + + +def test_operator_versions_plugin_mutual_exclusive_name() -> None: + result = CliRunner().invoke( + cli, ["operator-versions", "--plugin", "lvms", "--name", "foo"], env=_KC + ) + assert result.exit_code != 0 + assert "mutually exclusive" in result.output + + +def test_operator_versions_plugin_mutual_exclusive_use_defaults() -> None: + result = CliRunner().invoke( + cli, ["operator-versions", "--plugin", "lvms", "--use-defaults"], env=_KC + ) + assert result.exit_code != 0 + assert "mutually exclusive" in result.output + + +def test_operator_versions_plugin_not_found() -> None: + result = CliRunner().invoke( + cli, ["operator-versions", "--plugin", "nonexistent"], env=_KC + ) + assert result.exit_code != 0 + assert "not found" in result.output + + +def test_operator_versions_plugin_no_operators(mocker: MockerFixture) -> None: + mocker.patch( + "pathlib.Path.open", + mocker.mock_open(read_data="name: test-plugin\ntype: addon\norder: 1\n"), + ) + result = CliRunner().invoke( + cli, ["operator-versions", "--plugin", "test-plugin"], env=_KC + ) + assert result.exit_code != 0 + assert "no operators defined" in result.output + + +def test_operator_versions_plugin_install_operators_false( + mocker: MockerFixture, +) -> None: + mocker.patch( + "pathlib.Path.open", + mocker.mock_open( + read_data="name: test-plugin\ntype: addon\norder: 1\ninstallOperators: false\n" + ), + ) + result = CliRunner().invoke( + cli, ["operator-versions", "--plugin", "test-plugin"], env=_KC + ) + assert result.exit_code != 0 + assert "installOperators set to false" in result.output + + +def test_operator_versions_plugin_path_traversal_double_dot() -> None: + result = CliRunner().invoke( + cli, ["operator-versions", "--plugin", "../../../etc"], env=_KC + ) + assert result.exit_code != 0 + assert "Invalid plugin name" in result.output + + +def test_operator_versions_plugin_path_traversal_slash() -> None: + result = CliRunner().invoke( + cli, ["operator-versions", "--plugin", "foo/bar"], env=_KC + ) + assert result.exit_code != 0 + assert "Invalid plugin name" in result.output + + +def test_operator_versions_plugin_path_traversal_backslash() -> None: + result = CliRunner().invoke( + cli, ["operator-versions", "--plugin", "foo\\bar"], env=_KC + ) + assert result.exit_code != 0 + assert "Invalid plugin name" in result.output + + def test_mgmt_cluster_version_with_version(mocker: MockerFixture) -> None: mock_reconcile = mocker.patch("enclave.reconcile.cli.cluster_upgrade_reconcile") dry_run = True From 73f1af93758f4fd8da9c18ff1943ba4b5758dda0 Mon Sep 17 00:00:00 2001 From: Maor Friedman Date: Tue, 11 Aug 2026 14:03:46 +0300 Subject: [PATCH 2/4] Reconcile plugin operator versions during upgrade Extends playbooks/upgrade.yaml to also run enclave reconcile operator-versions --plugin for each enabled plugin that installs operators on the management cluster (installOperators: true, installOperatorsFleet: false), e.g. ODF and LVMS. Previously only defaults/operators.yaml operators were upgraded. Plugin discovery/filtering is extracted into playbooks/tasks/upgrade_plugins.yaml, mirroring the existing tasks/deploy_plugins.yaml pattern. Assisted-by: Claude Code --- docs/UPGRADE.md | 6 ++- playbooks/tasks/upgrade_plugins.yaml | 67 ++++++++++++++++++++++++++++ playbooks/upgrade.yaml | 5 +++ 3 files changed, 76 insertions(+), 2 deletions(-) create mode 100644 playbooks/tasks/upgrade_plugins.yaml diff --git a/docs/UPGRADE.md b/docs/UPGRADE.md index f49fc430c..d1ee7bfc1 100644 --- a/docs/UPGRADE.md +++ b/docs/UPGRADE.md @@ -46,11 +46,11 @@ Each Enclave tarball release includes: ```sh ./sync.sh ``` -7. **Execute upgrade script** - Run the upgrade automation to apply configuration migrations, then upgrade the management cluster and operators to the versions pinned in the tarball. This executes the `playbooks/upgrade.yaml` playbook, which runs migrations followed by `enclave reconcile mgmt-cluster-version --use-defaults` and `enclave reconcile operator-versions --use-defaults`: +7. **Execute upgrade script** - Run the upgrade automation to apply configuration migrations, then upgrade the management cluster and operators to the versions pinned in the tarball. This executes the `playbooks/upgrade.yaml` playbook, which runs migrations followed by `enclave reconcile mgmt-cluster-version --use-defaults`, `enclave reconcile operator-versions --use-defaults`, and `enclave reconcile operator-versions --plugin ` for each enabled plugin that installs operators directly on the management cluster (`installOperators: true` and `installOperatorsFleet: false`, e.g. ODF, LVMS): ```sh ./upgrade.sh ``` -8. **(Optional) Run cluster/operator upgrades separately** - By default `upgrade.yaml` performs both the management cluster and operator upgrades. To skip either step (e.g. to control upgrade timing independently) pass `upgrade_mgmt_cluster=false` and/or `upgrade_operators=false`, then run the corresponding command manually when ready: +8. **(Optional) Run cluster/operator upgrades separately** - By default `upgrade.yaml` performs both the management cluster and operator upgrades (including plugin operators). To skip either step (e.g. to control upgrade timing independently) pass `upgrade_mgmt_cluster=false` and/or `upgrade_operators=false`, then run the corresponding command manually when ready: ```sh ansible-playbook playbooks/upgrade.yaml -e workingDir= -e upgrade_mgmt_cluster=false -e upgrade_operators=false @@ -58,6 +58,8 @@ Each Enclave tarball release includes: export KUBECONFIG=$WORKING_DIR/ocp-cluster/auth/kubeconfig enclave reconcile mgmt-cluster-version --use-defaults enclave reconcile operator-versions --use-defaults + enclave reconcile operator-versions --plugin odf + enclave reconcile operator-versions --plugin lvms ``` --- diff --git a/playbooks/tasks/upgrade_plugins.yaml b/playbooks/tasks/upgrade_plugins.yaml new file mode 100644 index 000000000..89f4b4712 --- /dev/null +++ b/playbooks/tasks/upgrade_plugins.yaml @@ -0,0 +1,67 @@ +--- +# Auto-discover plugins whose operators are installed directly on the +# management cluster and reconcile their operator versions to the +# versions pinned in each plugin's plugin.yaml. +# +# Only plugins listed in enabled_plugins with installOperators: true and +# installOperatorsFleet: false are reconciled here: those are the ones +# whose operators are approved/upgraded on the management cluster via +# InstallPlans. Plugins with installOperatorsFleet: true roll out +# operators to fleet clusters via ACM policies instead, which this +# single-cluster reconcile flow does not manage. +# +# Called from upgrade.yaml. + +- name: Find all plugin descriptors + ansible.builtin.find: + paths: "{{ playbook_dir }}/../plugins" + patterns: "plugin.yaml" + recurse: true + depth: 2 + register: r_plugin_find + +- name: Read plugin descriptors + ansible.builtin.slurp: + src: "{{ plugin_path }}" + loop: "{{ r_plugin_find.files | map(attribute='path') | list }}" + register: r_plugin_slurp + loop_control: + loop_var: plugin_path + +- name: Build list of plugins to upgrade operator versions for + ansible.builtin.set_fact: + _plugin_operator_reconcile_list: >- + {{ r_plugin_slurp.results + | map(attribute='content') + | map('b64decode') + | map('from_yaml') + | selectattr('name', 'in', enabled_plugins) + | selectattr('installOperators', 'equalto', true) + | selectattr('installOperatorsFleet', 'equalto', false) + | map(attribute='name') + | list }} + +- name: Display plugins to upgrade operator versions for + ansible.builtin.debug: + msg: "Plugins to upgrade operator versions for: {{ _plugin_operator_reconcile_list }}" + +- name: Upgrade plugin operator versions + ansible.builtin.command: + argv: + - enclave + - reconcile + - operator-versions + - --plugin + - "{{ _plugin_name }}" + loop: "{{ _plugin_operator_reconcile_list }}" + loop_control: + loop_var: _plugin_name + register: r_plugin_operator_versions_upgrade + when: _plugin_operator_reconcile_list | length > 0 + +- name: Print plugin operator versions upgrade output + ansible.builtin.debug: + var: item.stderr_lines + loop: "{{ r_plugin_operator_versions_upgrade.results | default([]) }}" + loop_control: + label: "{{ item._plugin_name | default('') }}" diff --git a/playbooks/upgrade.yaml b/playbooks/upgrade.yaml index 3d97a57a5..478a3200f 100644 --- a/playbooks/upgrade.yaml +++ b/playbooks/upgrade.yaml @@ -51,3 +51,8 @@ ansible.builtin.debug: var: r_operator_versions_upgrade.stderr_lines when: upgrade_operators | default(true) | bool + + - name: Upgrade plugin operator versions + ansible.builtin.include_tasks: + file: tasks/upgrade_plugins.yaml + when: upgrade_operators | default(true) | bool From 2e6328472ff115f81c73c76a090e19dd806c22ff Mon Sep 17 00:00:00 2001 From: Maor Friedman Date: Thu, 13 Aug 2026 09:38:40 +0300 Subject: [PATCH 3/4] Address CodeRabbit security review feedback - Enhanced plugin_descriptor_path() to resolve both the plugins root and descriptor path, validating the descriptor remains relative to the plugins directory (prevents symlink escapes and Unicode normalization attacks). - Added validation to _reconcile_operators_from_list() for each operator entry: require dict with non-empty string name/version/namespace and csvNames (if present) must be a list of strings. Raises descriptive ClickException identifying the invalid entry. - Updated test_operator_versions_plugin_multiple_operators to mock a plugin with two separate operator entries instead of one with many CSV names, verifying the loop-over-operators path. - Added test_operator_versions_plugin_real_odf to preserve coverage of the single-operator-with-many-CSVs case. - Added tests for invalid operator entries (not dict, missing name, invalid csvNames). All checks pass: 249 tests, ruff, mypy, coverage 91.54%. Assisted-by: Claude Code --- src/enclave/reconcile/cli.py | 51 ++++++++++++++++++++--- src/tests/test_cli.py | 80 ++++++++++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 5 deletions(-) diff --git a/src/enclave/reconcile/cli.py b/src/enclave/reconcile/cli.py index 73d1d3517..7d068b0a4 100644 --- a/src/enclave/reconcile/cli.py +++ b/src/enclave/reconcile/cli.py @@ -34,9 +34,12 @@ def plugin_descriptor_path(plugin_name: str) -> Path: repo_root/plugins//plugin.yaml. Raises: - click.ClickException: If plugin_name is empty or could escape the - plugins directory (contains '..' or a path separator). + click.ClickException: If plugin_name is empty, contains path + separators or '..', or the resolved descriptor path escapes + the plugins directory. """ + # Normalize Unicode and reject traversal/separator patterns + plugin_name = plugin_name.strip() if ( not plugin_name or "/" in plugin_name @@ -47,8 +50,20 @@ def plugin_descriptor_path(plugin_name: str) -> Path: f"Invalid plugin name: {plugin_name!r}. Plugin name must be a " "simple name without path separators or '..'." ) + enclave_pkg = Path(__file__).resolve().parent.parent - return enclave_pkg.parent.parent / "plugins" / plugin_name / "plugin.yaml" + plugins_root = (enclave_pkg.parent.parent / "plugins").resolve() + descriptor_path = (plugins_root / plugin_name / "plugin.yaml").resolve() + + # Ensure the resolved descriptor is actually under the plugins root + try: + descriptor_path.relative_to(plugins_root) + except ValueError as exc: + raise click.ClickException( + f"Invalid plugin name: {plugin_name!r} escapes the plugins directory" + ) from exc + + return descriptor_path def _reconcile_operators_from_list( @@ -58,10 +73,36 @@ def _reconcile_operators_from_list( Each operator dict must have 'name', 'version', 'namespace', and optionally 'csvNames' (defaults to [name] when absent). + + Raises: + click.ClickException: If any operator entry is missing required + fields or has invalid field types. """ - for op in operators: + for idx, op in enumerate(operators): + # Validate operator entry structure + if not isinstance(op, dict): + raise click.ClickException(f"Operator entry {idx} is not a mapping: {op!r}") + + # Validate required string fields + for field in ("name", "version", "namespace"): + value = op.get(field) + if not isinstance(value, str) or not value.strip(): + raise click.ClickException( + f"Operator entry {idx} has invalid or missing '{field}': {value!r}" + ) + + # Validate optional csvNames field + csv_names_raw = op.get("csvNames") + if csv_names_raw is not None and ( + not isinstance(csv_names_raw, list) + or not all(isinstance(name, str) for name in csv_names_raw) + ): + raise click.ClickException( + f"Operator entry {idx} has invalid 'csvNames': must be a list of strings" + ) + op_name = cast("str", op["name"]) - op_csv_names = cast("list[str] | None", op.get("csvNames")) or [op_name] + op_csv_names = cast("list[str] | None", csv_names_raw) or [op_name] operator_versions_reconcile( cast("str", op["version"]), cast("str", op["namespace"]), diff --git a/src/tests/test_cli.py b/src/tests/test_cli.py index a15a46c5b..83e538a00 100644 --- a/src/tests/test_cli.py +++ b/src/tests/test_cli.py @@ -163,6 +163,41 @@ def test_operator_versions_plugin_single_operator(mocker: MockerFixture) -> None def test_operator_versions_plugin_multiple_operators(mocker: MockerFixture) -> None: + mock_reconcile = mocker.patch("enclave.reconcile.cli.operator_versions_reconcile") + # Mock a plugin with two separate operator entries + plugin_yaml_content = """ +name: test-plugin +type: foundation +order: 10 +installOperators: true +operators: + - name: operator-a + version: 1.0.0 + namespace: ns-a + csvNames: + - operator-a + - operator-a-bundle + - name: operator-b + version: 2.0.0 + namespace: ns-b +""" + mocker.patch("pathlib.Path.open", mocker.mock_open(read_data=plugin_yaml_content)) + dry_run = True + result = CliRunner().invoke( + cli, ["operator-versions", "--plugin", "test-plugin", "--dry-run"], env=_KC + ) + assert result.exit_code == 0, result.output + assert mock_reconcile.call_count == 2 + + # Verify both operators were reconciled with correct arguments + mock_reconcile.assert_any_call( + "1.0.0", "ns-a", ["operator-a", "operator-a-bundle"], dry_run + ) + mock_reconcile.assert_any_call("2.0.0", "ns-b", ["operator-b"], dry_run) + + +def test_operator_versions_plugin_real_odf(mocker: MockerFixture) -> None: + """Test with real ODF plugin that has one operator with many CSV names.""" mock_reconcile = mocker.patch("enclave.reconcile.cli.operator_versions_reconcile") dry_run = True result = CliRunner().invoke( @@ -190,6 +225,51 @@ def test_operator_versions_plugin_multiple_operators(mocker: MockerFixture) -> N ) +def test_operator_versions_plugin_invalid_operator_not_dict( + mocker: MockerFixture, +) -> None: + """Test that non-dict operator entries are rejected.""" + mocker.patch( + "pathlib.Path.open", + mocker.mock_open( + read_data="name: test\ntype: addon\norder: 1\noperators:\n - invalid-string\n" + ), + ) + result = CliRunner().invoke(cli, ["operator-versions", "--plugin", "test"], env=_KC) + assert result.exit_code != 0 + assert "not a mapping" in result.output + + +def test_operator_versions_plugin_invalid_operator_missing_name( + mocker: MockerFixture, +) -> None: + """Test that operator entries missing 'name' are rejected.""" + mocker.patch( + "pathlib.Path.open", + mocker.mock_open( + read_data="name: test\ntype: addon\norder: 1\noperators:\n - version: 1.0.0\n namespace: ns\n" + ), + ) + result = CliRunner().invoke(cli, ["operator-versions", "--plugin", "test"], env=_KC) + assert result.exit_code != 0 + assert "invalid or missing 'name'" in result.output + + +def test_operator_versions_plugin_invalid_csvnames_not_list( + mocker: MockerFixture, +) -> None: + """Test that csvNames must be a list of strings.""" + mocker.patch( + "pathlib.Path.open", + mocker.mock_open( + read_data="name: test\ntype: addon\norder: 1\noperators:\n - name: op\n version: 1.0.0\n namespace: ns\n csvNames: not-a-list\n" + ), + ) + result = CliRunner().invoke(cli, ["operator-versions", "--plugin", "test"], env=_KC) + assert result.exit_code != 0 + assert "invalid 'csvNames'" in result.output + + def test_operator_versions_plugin_mutual_exclusive_name() -> None: result = CliRunner().invoke( cli, ["operator-versions", "--plugin", "lvms", "--name", "foo"], env=_KC From fb8b138158acfb583a81c17f70011481c4bb548c Mon Sep 17 00:00:00 2001 From: Maor Friedman Date: Thu, 13 Aug 2026 09:57:08 +0300 Subject: [PATCH 4/4] Address second round of CodeRabbit review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. **Validate before reconcile (MAJOR)** - Refactored _reconcile_operators_from_list() to validate ALL operator entries before reconciling any. Previously, if validation failed on a later entry, earlier operators would already be reconciled (InstallPlans approved), leaving a partial upgrade. Now collects validated operators first, reconciles only after the entire list passes validation. 2. **Reject blank CSV names** - Enhanced csvNames validation to reject blank/whitespace-only strings, not just check if it's a list. Prevents downstream reconciler from constructing invalid CSV resource names and timing out. 3. **Idempotent Ansible task** - Added changed_when condition to the plugin operator upgrade task, checking for "[UPDATE]" in stderr (the log pattern when an InstallPlan is approved). Task now reports changed only when an actual upgrade occurred, not on every run. 4. **Markdown linting** - Added blank lines before/after fenced code blocks in docs/UPGRADE.md to satisfy MD031 markdownlint rule. 5. **Ruff line limit** - Refactored long inline YAML fixtures in tests to use adjacent string literals, keeping all lines under 88 chars. 6. **Test coverage** - Added two new tests: - test_operator_versions_plugin_invalid_csvnames_blank: blank CSV names - test_operator_versions_plugin_partial_validation_no_reconcile: one valid + one invalid operator → no reconciliation occurs (verifies the validate-before-reconcile fix) All checks pass: 251 tests (+2), ruff, mypy, ansible-lint (production), coverage 91.57%. Assisted-by: Claude Code --- docs/UPGRADE.md | 3 ++ playbooks/tasks/upgrade_plugins.yaml | 1 + src/enclave/reconcile/cli.py | 19 +++++++--- src/tests/test_cli.py | 56 ++++++++++++++++++++++++++-- 4 files changed, 71 insertions(+), 8 deletions(-) diff --git a/docs/UPGRADE.md b/docs/UPGRADE.md index d1ee7bfc1..5a2da8e27 100644 --- a/docs/UPGRADE.md +++ b/docs/UPGRADE.md @@ -47,10 +47,13 @@ Each Enclave tarball release includes: ./sync.sh ``` 7. **Execute upgrade script** - Run the upgrade automation to apply configuration migrations, then upgrade the management cluster and operators to the versions pinned in the tarball. This executes the `playbooks/upgrade.yaml` playbook, which runs migrations followed by `enclave reconcile mgmt-cluster-version --use-defaults`, `enclave reconcile operator-versions --use-defaults`, and `enclave reconcile operator-versions --plugin ` for each enabled plugin that installs operators directly on the management cluster (`installOperators: true` and `installOperatorsFleet: false`, e.g. ODF, LVMS): + ```sh ./upgrade.sh ``` + 8. **(Optional) Run cluster/operator upgrades separately** - By default `upgrade.yaml` performs both the management cluster and operator upgrades (including plugin operators). To skip either step (e.g. to control upgrade timing independently) pass `upgrade_mgmt_cluster=false` and/or `upgrade_operators=false`, then run the corresponding command manually when ready: + ```sh ansible-playbook playbooks/upgrade.yaml -e workingDir= -e upgrade_mgmt_cluster=false -e upgrade_operators=false diff --git a/playbooks/tasks/upgrade_plugins.yaml b/playbooks/tasks/upgrade_plugins.yaml index 89f4b4712..37e5957ca 100644 --- a/playbooks/tasks/upgrade_plugins.yaml +++ b/playbooks/tasks/upgrade_plugins.yaml @@ -57,6 +57,7 @@ loop_control: loop_var: _plugin_name register: r_plugin_operator_versions_upgrade + changed_when: "'[UPDATE]' in r_plugin_operator_versions_upgrade.stderr" when: _plugin_operator_reconcile_list | length > 0 - name: Print plugin operator versions upgrade output diff --git a/src/enclave/reconcile/cli.py b/src/enclave/reconcile/cli.py index 7d068b0a4..0ca11f9ef 100644 --- a/src/enclave/reconcile/cli.py +++ b/src/enclave/reconcile/cli.py @@ -74,10 +74,16 @@ def _reconcile_operators_from_list( Each operator dict must have 'name', 'version', 'namespace', and optionally 'csvNames' (defaults to [name] when absent). + Validates all operator entries before reconciling any. If validation + fails for any entry, no reconciliation occurs. + Raises: click.ClickException: If any operator entry is missing required fields or has invalid field types. """ + # Validate all operators first, before reconciling any + validated_operators: list[tuple[str, str, list[str]]] = [] + for idx, op in enumerate(operators): # Validate operator entry structure if not isinstance(op, dict): @@ -95,20 +101,23 @@ def _reconcile_operators_from_list( csv_names_raw = op.get("csvNames") if csv_names_raw is not None and ( not isinstance(csv_names_raw, list) - or not all(isinstance(name, str) for name in csv_names_raw) + or not all(isinstance(name, str) and name.strip() for name in csv_names_raw) ): raise click.ClickException( - f"Operator entry {idx} has invalid 'csvNames': must be a list of strings" + f"Operator entry {idx} has invalid 'csvNames': must be a list of non-blank strings" ) op_name = cast("str", op["name"]) op_csv_names = cast("list[str] | None", csv_names_raw) or [op_name] - operator_versions_reconcile( + validated_operators.append(( cast("str", op["version"]), cast("str", op["namespace"]), op_csv_names, - dry_run, - ) + )) + + # Reconcile only after all operators pass validation + for version, namespace, csv_names in validated_operators: + operator_versions_reconcile(version, namespace, csv_names, dry_run) def _load_defaults_operators() -> list[dict[str, object]]: diff --git a/src/tests/test_cli.py b/src/tests/test_cli.py index 83e538a00..13de80da0 100644 --- a/src/tests/test_cli.py +++ b/src/tests/test_cli.py @@ -232,7 +232,9 @@ def test_operator_versions_plugin_invalid_operator_not_dict( mocker.patch( "pathlib.Path.open", mocker.mock_open( - read_data="name: test\ntype: addon\norder: 1\noperators:\n - invalid-string\n" + read_data=( + "name: test\ntype: addon\norder: 1\noperators:\n - invalid-string\n" + ) ), ) result = CliRunner().invoke(cli, ["operator-versions", "--plugin", "test"], env=_KC) @@ -247,7 +249,10 @@ def test_operator_versions_plugin_invalid_operator_missing_name( mocker.patch( "pathlib.Path.open", mocker.mock_open( - read_data="name: test\ntype: addon\norder: 1\noperators:\n - version: 1.0.0\n namespace: ns\n" + read_data=( + "name: test\ntype: addon\norder: 1\n" + "operators:\n - version: 1.0.0\n namespace: ns\n" + ) ), ) result = CliRunner().invoke(cli, ["operator-versions", "--plugin", "test"], env=_KC) @@ -262,7 +267,11 @@ def test_operator_versions_plugin_invalid_csvnames_not_list( mocker.patch( "pathlib.Path.open", mocker.mock_open( - read_data="name: test\ntype: addon\norder: 1\noperators:\n - name: op\n version: 1.0.0\n namespace: ns\n csvNames: not-a-list\n" + read_data=( + "name: test\ntype: addon\norder: 1\n" + "operators:\n - name: op\n version: 1.0.0\n" + " namespace: ns\n csvNames: not-a-list\n" + ) ), ) result = CliRunner().invoke(cli, ["operator-versions", "--plugin", "test"], env=_KC) @@ -270,6 +279,47 @@ def test_operator_versions_plugin_invalid_csvnames_not_list( assert "invalid 'csvNames'" in result.output +def test_operator_versions_plugin_invalid_csvnames_blank( + mocker: MockerFixture, +) -> None: + """Test that blank/whitespace-only CSV names are rejected.""" + mocker.patch( + "pathlib.Path.open", + mocker.mock_open( + read_data=( + "name: test\ntype: addon\norder: 1\n" + "operators:\n - name: op\n version: 1.0.0\n" + " namespace: ns\n csvNames: ['valid', ' ', 'also-valid']\n" + ) + ), + ) + result = CliRunner().invoke(cli, ["operator-versions", "--plugin", "test"], env=_KC) + assert result.exit_code != 0 + assert "invalid 'csvNames'" in result.output + + +def test_operator_versions_plugin_partial_validation_no_reconcile( + mocker: MockerFixture, +) -> None: + """Test that if one operator is invalid, no operators are reconciled.""" + mock_reconcile = mocker.patch("enclave.reconcile.cli.operator_versions_reconcile") + mocker.patch( + "pathlib.Path.open", + mocker.mock_open( + read_data=( + "name: test\ntype: addon\norder: 1\noperators:\n" + " - name: valid-op\n version: 1.0.0\n namespace: ns\n" + " - name: invalid-op\n namespace: ns\n" # missing version + ) + ), + ) + result = CliRunner().invoke(cli, ["operator-versions", "--plugin", "test"], env=_KC) + assert result.exit_code != 0 + assert "invalid or missing 'version'" in result.output + # Critical: no reconciliation should have occurred + mock_reconcile.assert_not_called() + + def test_operator_versions_plugin_mutual_exclusive_name() -> None: result = CliRunner().invoke( cli, ["operator-versions", "--plugin", "lvms", "--name", "foo"], env=_KC