chore: fix oci disable bug in install-dynamic-plugins.py

[Zaperex]

Description

Added a pre-merge step to disable annotation fetch for metadata for disabled plugins without an explicitly defined plugin path

Which issue(s) does this PR fix

• Fixes RHDHBUGS-2533

PR acceptance criteria

Please make sure that the following steps are complete:

• GitHub Actions are completed and successful
• Unit Tests are updated and passing
• E2E Tests are updated and passing
• Documentation is updated if necessary (requirement for new features)
• Add a screenshot if the change is UX/UI related

How to test changes / Special notes to the reviewer

[github-actions]

The container image build workflow finished with status: cancelled.

[github-actions]

Image was built and published successfully. It is available at:

• quay.io/rhdh-community/rhdh:pr-4576
• quay.io/rhdh-community/rhdh:pr-4576-3178310b

[rm3l]

Wondering why we need to validate this.. IMO we should just skip any kind of validation for a package ref when disabled is true, no? Users may purposely want to disable the plugin definition completely (rather than commenting it out), even if the definition is invalid.

[Zaperex]

Hmm I guess that makes sense, but this is more for the general case for matching with entries that already exist in an includes file, which means we would need a consistent format for the package field to be able to properly match plugins to merge their disabled states.

[rm3l]

I see. How about skipping invalid entries that are disabled with just a warning? That way, if someone has an invalid disabled entry in their dynamic-plugins config but a valid enabled entry in an includes file, the valid one won't be disabled (as I guess there would be no matching here, but this is fine IMO). The warning on the invalid one would help users know something might be misconfigured. Would that make sense?
I was commenting on this specific package format validation error, but noticed there is also other InstallException raised in https://github.com/Zaperex/rhdh/blob/3178310b2e593c0b5ab187711dcc57c91626933a/scripts/install-dynamic-plugins/install-dynamic-plugins.py#L1204-L1209
These validations are all fine when the package is enabled, but should not block the init container on disabled plugins IMO.

[Zaperex]

Sure that makes sense

[github-actions]

Image was built and published successfully. It is available at:

• quay.io/rhdh-community/rhdh:pr-4576
• quay.io/rhdh-community/rhdh:pr-4576-bff89498

[rm3l]

/cherry-pick release-1.9

[openshift-cherrypick-robot]

@rm3l: once the present PR merges, I will cherry-pick it on top of release-1.9 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick release-1.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[github-actions]

The container image build and publish workflows were skipped (either due to [skip-build] tag or no relevant changes with existing image).

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[github-actions]

Image was built and published successfully. It is available at:

• quay.io/rhdh-community/rhdh:pr-4576
• quay.io/rhdh-community/rhdh:pr-4576-eb2b674a

[Zaperex]

/review

[rhdh-qodo-merge]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 RHDHBUGS-2533 - Partially compliant Compliant requirements: Check disabled: true before attempting OCI resolution/inspection Prevent crashes on broken OCI images by allowing them to be disabled Apply disabled overrides prior to OCI metadata resolution Non-compliant requirements: Requires further human verification: Validate behavior end-to-end in a real deployment using a catalog index containing an OCI plugin missing io.backstage.dynamic-packages, with disabled: true provided via Helm values (ensures no skopeo calls and no crash in real runtime conditions).

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Correctness pre_merge_oci_disabled_state returns a disabled set keyed by the regex capture match.group(1) (named registry in code). Ensure this value is exactly the same identifier later used by filter_disabled_oci_plugins (also using match.group(1)), and that it matches the intended semantics (“image reference without tag/digest”) across all supported OCI formats. A mismatch could cause disabled plugins to not be filtered (leading to skopeo/metadata fetch and the original crash). def pre_merge_oci_disabled_state( include_plugin_lists: list[tuple[str, list[dict]]], main_plugins: list[dict], dynamic_plugins_file: str ) -> set[str]: """ Pre-merge pass: determine which OCI registries will be disabled after all levels are applied. Only considers 'package' and 'disabled' fields. Does NOT merge pluginConfig. Args: include_plugin_lists: list of (filename, plugin_list) tuples from each included config file main_plugins: plugin list from the main config file dynamic_plugins_file: path to the main config file (for error messages) Returns: Set of OCI registry strings that should skip metadata fetch (final state = disabled) """ # Track the disabled state and level of each OCI plugin entry per_entry_state = {} # Track the source file of each path-less OCI plugin entry pathless_plugin_registries = {} # {registry: source_file} # Track the source file of each OCI plugin entry with a defined path defined_paths_per_plugin_registry = {} # {registry: {path: source_file}} def process_entry(plugin, level, source_file): package = plugin.get('package', '') if not isinstance(package, str) or not package.startswith(OCI_PROTOCOL_PREFIX): return disabled = plugin.get('disabled', False) match = re.match(OciPackageMerger.EXPECTED_OCI_PATTERN, package) if not match: if disabled: print(f"WARNING: Skipping disabled OCI plugin with invalid format: \'{package}\' in {source_file}. Expected format: \'{OCI_PROTOCOL_PREFIX}<registry>:<tag>\' or \'{OCI_PROTOCOL_PREFIX}<registry>@<algo>:<digest>\' (optionally followed by \'!<path>\') where <registry> may include a port (e.g. host:5000/path) and <algo> is one of {RECOGNIZED_ALGORITHMS}", flush=True) return raise InstallException(f"oci package \'{package}\' is not in the expected format \'{OCI_PROTOCOL_PREFIX}<registry>:<tag>\' or \'{OCI_PROTOCOL_PREFIX}<registry>@<algo>:<digest>\' (optionally followed by \'!<path>\') in {source_file} where <registry> may include a port (e.g. host:5000/path) and <algo> is one of {RECOGNIZED_ALGORITHMS}") registry = match.group(1) path = match.group(4) # None for path-less entry_key = (registry, path) if entry_key not in per_entry_state: per_entry_state[entry_key] = (disabled, level) elif per_entry_state[entry_key][1] == level: path_suffix = f"!{path}" if path else "" if disabled: print(f"WARNING: Skipping duplicate disabled OCI plugin configuration for {registry}{path_suffix} in {source_file}", flush=True) return raise InstallException( f"Duplicate OCI plugin configuration for {registry}{path_suffix} " f"found at the same level in {source_file}: {package}" ) elif level > per_entry_state[entry_key][1]: per_entry_state[entry_key] = (disabled, level) if path: defined_paths_per_plugin_registry.setdefault(registry, {})[path] = source_file else: pathless_plugin_registries[registry] = source_file for include_file, include_plugins in include_plugin_lists: for plugin in include_plugins: process_entry(plugin, level=0, source_file=include_file) for plugin in main_plugins: process_entry(plugin, level=1, source_file=dynamic_plugins_file) # Initial validation for no multi-plugin OCI packages being defined without explicit path for registry, pathless_source in pathless_plugin_registries.items(): if registry in defined_paths_per_plugin_registry and len(defined_paths_per_plugin_registry[registry]) > 1: path_entries = defined_paths_per_plugin_registry[registry] paths_formatted = '\n - '.join( f"{path} (in {src})" for path, src in sorted(path_entries.items()) ) pathless_disabled, _ = per_entry_state[(registry, None)] if pathless_disabled: print( f"WARNING: Skipping disabled ambiguous path-less OCI reference for {registry} in {pathless_source}: " f"multiple path-specific entries exist:\n - {paths_formatted}\n" f"Cannot use path-less syntax for multi-plugin images. " f"Please specify a !<plugin-path> suffix for the plugin", flush=True ) continue raise InstallException( f"Ambiguous path-less OCI reference for {registry} in {pathless_source}: " f"multiple path-specific entries exist:\n - {paths_formatted}\n" f"Cannot use path-less syntax for multi-plugin images. " f"Please specify a !<plugin-path> suffix for the plugin." ) # Determine effective disabled state for each registry without explicit path. disabled_plugin_registries = set() for registry in pathless_plugin_registries: pathless_disabled, pathless_level = per_entry_state[(registry, None)] effective_disabled = pathless_disabled # Check if a single entry with explicit path at a higher level overrides if registry in defined_paths_per_plugin_registry: single_path = next(iter(defined_paths_per_plugin_registry[registry])) defined_disabled, defined_level = per_entry_state[(registry, single_path)] if defined_level > pathless_level: effective_disabled = defined_disabled if effective_disabled: disabled_plugin_registries.add(registry) return disabled_plugin_registries def filter_disabled_oci_plugins(plugins: list[dict], disabled_plugin_registries: set[str]) -> list[dict]: """ Remove all OCI plugin entries whose registry is in the disabled set. Non-OCI entries are passed through unchanged. """ filtered = [] for plugin in plugins: package = plugin.get('package', '') if isinstance(package, str) and package.startswith(OCI_PROTOCOL_PREFIX): match = re.match(OciPackageMerger.EXPECTED_OCI_PATTERN, package) if match and match.group(1) in disabled_plugin_registries: print(f'\n======= Disabling OCI plugin {package}', flush=True) continue filtered.append(plugin) return filtered UX/Noise filter_disabled_oci_plugins prints a banner with a leading newline for each filtered plugin. In larger configs this can produce noisy logs and make troubleshooting harder. Consider standardizing warning/info output formatting (and avoiding the leading newline) so logs remain readable, especially when multiple plugins are disabled. def filter_disabled_oci_plugins(plugins: list[dict], disabled_plugin_registries: set[str]) -> list[dict]: """ Remove all OCI plugin entries whose registry is in the disabled set. Non-OCI entries are passed through unchanged. """ filtered = [] for plugin in plugins: package = plugin.get('package', '') if isinstance(package, str) and package.startswith(OCI_PROTOCOL_PREFIX): match = re.match(OciPackageMerger.EXPECTED_OCI_PATTERN, package) if match and match.group(1) in disabled_plugin_registries: print(f'\n======= Disabling OCI plugin {package}', flush=True) continue filtered.append(plugin) return filtered

📚 Focus areas based on broader codebase context Duplicate Handling pre_merge_oci_disabled_state() warns and “skips” duplicate disabled entries at the same level, but it does not actually remove any entries from the plugin lists. In the main flow, those duplicates can still reach merge_plugin() and trigger the existing same-level duplicate error, creating inconsistent behavior (warning-only in pre-merge vs exception during merge). Consider aligning pre-merge duplicate handling with the merge behavior (either error early, or actually de-duplicate/filter the conflicting entry before merge_plugin() runs). (Ref 1) def process_entry(plugin, level, source_file): package = plugin.get('package', '') if not isinstance(package, str) or not package.startswith(OCI_PROTOCOL_PREFIX): return disabled = plugin.get('disabled', False) match = re.match(OciPackageMerger.EXPECTED_OCI_PATTERN, package) if not match: if disabled: print(f"WARNING: Skipping disabled OCI plugin with invalid format: \'{package}\' in {source_file}. Expected format: \'{OCI_PROTOCOL_PREFIX}<registry>:<tag>\' or \'{OCI_PROTOCOL_PREFIX}<registry>@<algo>:<digest>\' (optionally followed by \'!<path>\') where <registry> may include a port (e.g. host:5000/path) and <algo> is one of {RECOGNIZED_ALGORITHMS}", flush=True) return raise InstallException(f"oci package \'{package}\' is not in the expected format \'{OCI_PROTOCOL_PREFIX}<registry>:<tag>\' or \'{OCI_PROTOCOL_PREFIX}<registry>@<algo>:<digest>\' (optionally followed by \'!<path>\') in {source_file} where <registry> may include a port (e.g. host:5000/path) and <algo> is one of {RECOGNIZED_ALGORITHMS}") registry = match.group(1) path = match.group(4) # None for path-less entry_key = (registry, path) if entry_key not in per_entry_state: per_entry_state[entry_key] = (disabled, level) elif per_entry_state[entry_key][1] == level: path_suffix = f"!{path}" if path else "" if disabled: print(f"WARNING: Skipping duplicate disabled OCI plugin configuration for {registry}{path_suffix} in {source_file}", flush=True) return raise InstallException( f"Duplicate OCI plugin configuration for {registry}{path_suffix} " f"found at the same level in {source_file}: {package}" ) Reference reasoning: The existing OCI merge logic raises an InstallException when a duplicate plugin configuration is found at the same level (tracked via last_modified_level). Since this enforcement already exists later in the pipeline, pre-merge should not silently allow same-level duplicates to pass through unless it also prevents them from being merged (e.g., by removing them), otherwise the pipeline behavior becomes contradictory.

📄 References redhat-developer/rhdh/scripts/install-dynamic-plugins/install-dynamic-plugins.py [476-647] redhat-developer/rhdh/scripts/install-dynamic-plugins/test_install-dynamic-plugins.py [632-652] redhat-developer/rhdh/scripts/install-dynamic-plugins/test_install-dynamic-plugins.py [654-674] redhat-developer/rhdh/scripts/install-dynamic-plugins/test_install-dynamic-plugins.py [859-892] redhat-developer/rhdh/scripts/install-dynamic-plugins/test_install-dynamic-plugins.py [1046-1064] redhat-developer/rhdh/scripts/install-dynamic-plugins/test_install-dynamic-plugins.py [1014-1044] redhat-developer/rhdh/scripts/install-dynamic-plugins/test_install-dynamic-plugins.py [917-942] redhat-developer/rhdh/scripts/install-dynamic-plugins/test_install-dynamic-plugins.py [960-985]