RHIDP-14785: Add OLM v1 code path to prepare-restricted-environment.sh#3046
Open
Fortune-Ndlovu wants to merge 1 commit into
Conversation
PR Summary by QodoAdd OLM v1 support to install-rhdh-catalog-source.sh with auto-detection Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1. Hardcoded ClusterCatalog pullSecret
|
rhdh-qodo-merge
Bot
added
documentation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if [[ "${RESOLVED_OLM_VERSION}" == "v1" ]]; then | ||
| debugf "Converting CatalogSource to ClusterCatalog: ${manifest}" | ||
| catalogImage=$("$YQ" '.spec.image' "${manifest}") | ||
| catalogImage=$(echo "$catalogImage" | sed 's|default-route-openshift-image-registry\.apps\.[^/]*|image-registry.openshift-image-registry.svc:5000|') |
Add OLM version detection and branching to the airgap script so it generates the correct manifests for both OLM v0 and OLM v1 clusters. When OLM v1 is detected (or forced via --olm-version), the script produces ClusterCatalog, ClusterExtension, ServiceAccount, and ClusterRoleBinding instead of CatalogSource, OperatorGroup, and Subscription. The oc-mirror path converts CatalogSource output to ClusterCatalog. Export-only mode generates both manifest sets. Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com> Assisted-by: Claude (Opus 4.6)
Fortune-Ndlovu
force-pushed
the
RHIDP-14785-operator-add-olm-v-1-path-to-prepare-restricted-environment-sh
branch
from
0137c0e to
c860d35
Compare
|
Fortune-Ndlovu
changed the title
Member
Author
|
/agentic_review |
![rhdh-qodo-merge[bot]](https://avatars.githubusercontent.com/in/1961108?s=60&v=4){kind=small}
Comment on lines
+1402
to
+1415
| cat <<EOF >"${manifestsTargetDir}/clusterRoleBinding.yaml" | ||
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: ClusterRoleBinding | ||
| metadata: | ||
| name: ${CRB_NAME} | ||
| roleRef: | ||
| apiGroup: rbac.authorization.k8s.io | ||
| kind: ClusterRole | ||
| name: cluster-admin | ||
| subjects: | ||
| - kind: ServiceAccount | ||
| name: ${SA_NAME} | ||
| namespace: ${NAMESPACE_OPERATOR} | ||
| EOF |
There was a problem hiding this comment.
1. cluster-admin clusterrolebinding created 📘 Rule violation ⛨ Security
The script generates OLM v1 install manifests that create a ClusterRoleBinding binding the installer namespaced ServiceAccount to the built-in cluster-admin ClusterRole, granting full cluster-wide privileges. This RBAC is overly broad for managed resources and substantially increases the blast radius and risk of privilege escalation if the ServiceAccount token is misused, especially compared to the OLM v0 flow where no such cluster-admin binding is created.
Agent Prompt
## Issue description
The generated OLM v1 installer RBAC creates a `ClusterRoleBinding` (via `clusterRoleBinding.yaml`) that binds the installer namespaced ServiceAccount to the built-in `cluster-admin` ClusterRole, granting permanent, full cluster-wide privileges; this is overly privileged, increases blast radius, and enables privilege escalation if the ServiceAccount token is used outside its intended purpose.
## Issue Context
Per compliance requirements, shipped RBAC should be scoped to the minimal apiGroups/resources/verbs needed by the installer/ClusterExtension installation workflow rather than using broad `cluster-admin` permissions. This binding is created whenever OLM v1 manifests are generated/applied, and even if the script is typically run by cluster admins, the resulting RBAC object persists and can be abused later by any principal that can run pods as that ServiceAccount in the operator namespace; additionally, this is a regression in security posture compared to the OLM v0 flow, where no cluster-admin binding is created.
## Fix Focus Areas
- .rhdh/scripts/prepare-restricted-environment.sh[1402-1415]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1240
to
+1250
| apiVersion: catalogd.operatorframework.io/v1 | ||
| kind: ClusterCatalog | ||
| metadata: | ||
| name: rhdh-catalog | ||
| spec: | ||
| source: | ||
| type: Image | ||
| image: | ||
| ref: ${my_operator_index} | ||
| pullSecret: internal-reg-auth-for-rhdh | ||
| EOF |
There was a problem hiding this comment.
2. Hardcoded clustercatalog pullsecret 🐞 Bug ≡ Correctness
The OLM v1 ClusterCatalog manifest hardcodes pullSecret: internal-reg-auth-for-rhdh, but the script only creates that secret in the OCP_INTERNAL registry flow. With `--to-registry <external-registry>` (or any non-OCP_INTERNAL mirror), the script can apply a ClusterCatalog that references a secret that was never created, preventing catalogd from pulling the catalog image.
Agent Prompt
### Issue description
The OLM v1 `ClusterCatalog` YAML is generated (and also created during oc-mirror conversion) with a hardcoded `pullSecret: internal-reg-auth-for-rhdh`. That secret is only created when `TO_REGISTRY=OCP_INTERNAL`, so for `--to-registry <external>` the applied `ClusterCatalog` can reference a non-existent secret and fail to pull.
### Issue Context
- In OLM v0, the CatalogSource includes multiple secrets including a user-provided `reg-pull-secret` option; the v1 path currently doesn’t provide an equivalent.
- For OLM v1, the pull secret likely needs to exist in the namespace where catalogd expects it (the script already special-cases this for OCP internal by creating secrets in the detected catalogd namespace).
### Fix Focus Areas
- .rhdh/scripts/prepare-restricted-environment.sh[1238-1250]
- .rhdh/scripts/prepare-restricted-environment.sh[1136-1151]
- .rhdh/scripts/prepare-restricted-environment.sh[964-980]
- .rhdh/scripts/prepare-restricted-environment.sh[382-419]
### Suggested implementation direction
- Introduce a variable (and optionally a CLI flag) for the v1 catalog pull secret name, e.g. `CATALOG_PULL_SECRET`.
- Default behavior:
- If `TO_REGISTRY=OCP_INTERNAL`, keep using/creating `internal-reg-auth-for-rhdh` (current behavior).
- Otherwise, either:
- omit `pullSecret` entirely (if you want to support public registries by default), or
- default it to `reg-pull-secret` and document that the user must create it in the catalogd namespace.
- Ensure any auto-created secret is created in the correct namespace for OLM v1 (catalogd namespace), not just the operator namespace.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit c860d35 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds an
--olm-version v0|v1|autoCLI flag (default:auto) toprepare-restricted-environment.shwith CRD-based OLM v1 detection. When OLM v1 is detected (or forced), the script creates a ClusterCatalog and ClusterExtension with a ServiceAccount and ClusterRoleBinding, instead of the OLM v0 CatalogSource, Subscription, and OperatorGroup. Resolution is pinned to the custom catalog viaselector.matchLabels, and CRD upgrade safety preflight is disabled (enforcement: None) per the known blocker RHIDP-8656. The OLM v0 path is fully preserved for backward compatibility on older clusters.Which issue(s) does this PR fix or relate to
Resolves: https://redhat.atlassian.net/browse/RHIDP-14785
PR acceptance criteria
How to test changes / Special notes to the reviewer
Signed-off-by: Fortune-Ndlovu fndlovu@redhat.com
Assisted-by: Claude (claude-opus-4-6)