feat(RHIDP-14601): enable dev-sandbox-catalog-plugin#403
Conversation
|
Skipping CI for Draft Pull Request. |
f4e644e to
52cdc24
Compare
bcbd3e9 to
50e2e22
Compare
|
/hold |
a91c8ea to
3ff758a
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (20)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (8)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR updates the Backstage CR and OLM templates, adds optional dev-sandbox catalog plugin wiring, propagates a cache-label filter flag through installation scripts, switches resource marking to an external-config label, and extends developer-hub monitoring. ChangesRHDH OLM Upgrade and Dev-Sandbox Plugin
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ci-scripts/dev-sandbox/rhdh-perf-workloads.backstages.template.yaml`:
- Line 134: The hardcoded BACKEND_SECRET value in the template file at line 134
should not be committed to version control as it exposes a predictable secret
across all deployments. Replace the hardcoded secret value with a reference to a
runtime-provided environment variable or secret management system (such as a
variable substitution pattern) that allows the actual secret to be injected at
deployment time rather than stored in git.
In `@ci-scripts/rhdh-setup/deploy.sh`:
- Around line 916-918: The failureThreshold variable calculated using the bc
command can evaluate to 0 for small datasets due to integer division, which
Kubernetes rejects. Modify the failureThreshold calculation to ensure it is
clamped to a minimum value of 1 by applying a conditional check or mathematical
operation (such as using the maximum function) after the bc calculation to
guarantee the value is at least 1 before it is substituted into the yq commands
that update the readinessProbe and livenessProbe configuration.
In
`@ci-scripts/rhdh-setup/template/backstage/olm/dev-sandbox/dev-sandbox-catalog-plugin-patch.yaml`:
- Around line 1-2: The OCI image reference for
quay.io/asoro/dev-sandbox-catalog-backend-module in the patch file uses a
mutable tag (0.4.0) which is insecure since this patch is appended directly to
the dynamic-plugins ConfigMap as trusted cluster code at runtime. Replace the
mutable tag with an immutable SHA256 digest by changing the package reference
from using the colon separator with the tag to the at symbol with the full
digest value provided in the comment, ensuring the image is pinned to a specific
immutable version.
In
`@ci-scripts/rhdh-setup/template/backstage/olm/dev-sandbox/dev-sandbox-catalog.rbac.yaml`:
- Around line 7-27: The ClusterRoleBinding named rhdh-useraccount-reader grants
unnecessary cluster-wide permissions for a namespace-scoped resource. Change the
kind from ClusterRoleBinding to RoleBinding in the second resource definition
and add a namespace field to its metadata set to ${RHDH_NAMESPACE} to properly
scope the binding to the specific namespace. The roleRef and subjects can remain
unchanged, but ensure the RoleBinding is now namespace-scoped rather than
cluster-scoped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 380fb108-65b7-416a-84f4-55424f6f64ab
📒 Files selected for processing (18)
ci-scripts/dev-sandbox/collect-results.shci-scripts/dev-sandbox/deploy.shci-scripts/dev-sandbox/metrics-config.yamlci-scripts/dev-sandbox/rhdh-perf-workloads.backstages.template.yamlci-scripts/rhdh-setup/common.shci-scripts/rhdh-setup/deploy.shci-scripts/rhdh-setup/install-rhdh-catalog-source.shci-scripts/rhdh-setup/template/backstage/app-config.yamlci-scripts/rhdh-setup/template/backstage/olm/app-config.rhdh.db.yamlci-scripts/rhdh-setup/template/backstage/olm/app-config.rhdh.urls.yamlci-scripts/rhdh-setup/template/backstage/olm/app-rbac-patch.yamlci-scripts/rhdh-setup/template/backstage/olm/backstage.yamlci-scripts/rhdh-setup/template/backstage/olm/dev-sandbox/app-config.rhdh.dev-sandbox-catalog-plugin.yamlci-scripts/rhdh-setup/template/backstage/olm/dev-sandbox/dev-sandbox-catalog-plugin-patch.yamlci-scripts/rhdh-setup/template/backstage/olm/dev-sandbox/dev-sandbox-catalog-plugin-volumemounts-patch.yamlci-scripts/rhdh-setup/template/backstage/olm/dev-sandbox/dev-sandbox-catalog-plugin-volumes-patch.yamlci-scripts/rhdh-setup/template/backstage/olm/dev-sandbox/dev-sandbox-catalog.rbac.yamltest.env
💤 Files with no reviewable changes (1)
- ci-scripts/rhdh-setup/template/backstage/app-config.yaml
| failureThreshold=$(bc -l <<<"scale=0; ($API_COUNT + $COMPONENT_COUNT + $BACKSTAGE_USER_COUNT + $GROUP_COUNT) / 2000") | ||
| yq -i '(.spec.deployment.patch.spec.template.spec.containers[] | select(.name == "backstage-backend") | .readinessProbe) = {"httpGet":{"path":"/healthcheck","port":7007,"scheme":"HTTP"},"initialDelaySeconds":60,"timeoutSeconds":5,"periodSeconds":60,"successThreshold":1,"failureThreshold":'"$failureThreshold"'}' "$backstage_yaml" | ||
| yq -i '(.spec.deployment.patch.spec.template.spec.containers[] | select(.name == "backstage-backend") | .livenessProbe) = {"httpGet":{"path":"/healthcheck","port":7007,"scheme":"HTTP"},"initialDelaySeconds":60,"timeoutSeconds":5,"periodSeconds":60,"successThreshold":1,"failureThreshold":'"$failureThreshold"'}' "$backstage_yaml" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Clamp probe failureThreshold to a valid minimum.
Line 916 can evaluate to 0 for common small datasets, and Lines 917-918 then write failureThreshold: 0, which Kubernetes rejects.
Suggested fix
- failureThreshold=$(bc -l <<<"scale=0; ($API_COUNT + $COMPONENT_COUNT + $BACKSTAGE_USER_COUNT + $GROUP_COUNT) / 2000")
+ totalEntities=$((API_COUNT + COMPONENT_COUNT + BACKSTAGE_USER_COUNT + GROUP_COUNT))
+ failureThreshold=$(((totalEntities + 1999) / 2000))
+ if [ "$failureThreshold" -lt 1 ]; then
+ failureThreshold=1
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| failureThreshold=$(bc -l <<<"scale=0; ($API_COUNT + $COMPONENT_COUNT + $BACKSTAGE_USER_COUNT + $GROUP_COUNT) / 2000") | |
| yq -i '(.spec.deployment.patch.spec.template.spec.containers[] | select(.name == "backstage-backend") | .readinessProbe) = {"httpGet":{"path":"/healthcheck","port":7007,"scheme":"HTTP"},"initialDelaySeconds":60,"timeoutSeconds":5,"periodSeconds":60,"successThreshold":1,"failureThreshold":'"$failureThreshold"'}' "$backstage_yaml" | |
| yq -i '(.spec.deployment.patch.spec.template.spec.containers[] | select(.name == "backstage-backend") | .livenessProbe) = {"httpGet":{"path":"/healthcheck","port":7007,"scheme":"HTTP"},"initialDelaySeconds":60,"timeoutSeconds":5,"periodSeconds":60,"successThreshold":1,"failureThreshold":'"$failureThreshold"'}' "$backstage_yaml" | |
| totalEntities=$((API_COUNT + COMPONENT_COUNT + BACKSTAGE_USER_COUNT + GROUP_COUNT)) | |
| failureThreshold=$(((totalEntities + 1999) / 2000)) | |
| if [ "$failureThreshold" -lt 1 ]; then | |
| failureThreshold=1 | |
| fi | |
| yq -i '(.spec.deployment.patch.spec.template.spec.containers[] | select(.name == "backstage-backend") | .readinessProbe) = {"httpGet":{"path":"/healthcheck","port":7007,"scheme":"HTTP"},"initialDelaySeconds":60,"timeoutSeconds":5,"periodSeconds":60,"successThreshold":1,"failureThreshold":'"$failureThreshold"'}' "$backstage_yaml" | |
| yq -i '(.spec.deployment.patch.spec.template.spec.containers[] | select(.name == "backstage-backend") | .livenessProbe) = {"httpGet":{"path":"/healthcheck","port":7007,"scheme":"HTTP"},"initialDelaySeconds":60,"timeoutSeconds":5,"periodSeconds":60,"successThreshold":1,"failureThreshold":'"$failureThreshold"'}' "$backstage_yaml" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ci-scripts/rhdh-setup/deploy.sh` around lines 916 - 918, The failureThreshold
variable calculated using the bc command can evaluate to 0 for small datasets
due to integer division, which Kubernetes rejects. Modify the failureThreshold
calculation to ensure it is clamped to a minimum value of 1 by applying a
conditional check or mathematical operation (such as using the maximum function)
after the bc calculation to guarantee the value is at least 1 before it is
substituted into the yq commands that update the readinessProbe and
livenessProbe configuration.
| - package: oci://quay.io/asoro/dev-sandbox-catalog-backend-module:0.4.0 | ||
| disabled: false |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
curl -fsSI \
-H 'Accept: application/vnd.oci.image.manifest.v1+json' \
'https://quay.io/v2/asoro/dev-sandbox-catalog-backend-module/manifests/0.4.0' \
| awk -F': ' 'tolower($1)=="docker-content-digest"{print $2}'Repository: redhat-performance/backstage-performance
Length of output: 251
🏁 Script executed:
# First, check the actual file content
cat -n ci-scripts/rhdh-setup/template/backstage/olm/dev-sandbox/dev-sandbox-catalog-plugin-patch.yaml | head -20Repository: redhat-performance/backstage-performance
Length of output: 294
🏁 Script executed:
# Search for deploy.sh to understand how this patch is used
fd deploy.shRepository: redhat-performance/backstage-performance
Length of output: 278
🏁 Script executed:
# Check if there are other image references with mutable tags
rg "oci://.*:[\d\.]+" ci-scripts/rhdh-setup/ -A 2 -B 2Repository: redhat-performance/backstage-performance
Length of output: 470
🏁 Script executed:
# Examine the rhdh-setup deploy.sh to understand how it uses the patch file
cat -n ci-scripts/rhdh-setup/deploy.sh | head -100Repository: redhat-performance/backstage-performance
Length of output: 6035
🏁 Script executed:
# Search for references to dev-sandbox-catalog-plugin-patch in the deploy script
rg "dev-sandbox-catalog-plugin-patch" ci-scripts/rhdh-setup/ -B 5 -A 5Repository: redhat-performance/backstage-performance
Length of output: 1162
🏁 Script executed:
# Look for dynamic-plugins or ConfigMap references in deploy.sh
rg "dynamic-plugins|ConfigMap" ci-scripts/rhdh-setup/deploy.sh -B 3 -A 3Repository: redhat-performance/backstage-performance
Length of output: 2005
Pin the OCI image to an immutable digest.
The patch file is appended directly to the dynamic-plugins ConfigMap at runtime by deploy.sh, making the mutable 0.4.0 tag trusted cluster code. Update to use the image digest instead:
- package: oci://quay.io/asoro/dev-sandbox-catalog-backend-module@sha256:4990dfec347d30177415fed79fd61ca6ec81843e3fac077c636a85c0d040cb28
disabled: falseAlternatively, move the image to a vetted internal registry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ci-scripts/rhdh-setup/template/backstage/olm/dev-sandbox/dev-sandbox-catalog-plugin-patch.yaml`
around lines 1 - 2, The OCI image reference for
quay.io/asoro/dev-sandbox-catalog-backend-module in the patch file uses a
mutable tag (0.4.0) which is insecure since this patch is appended directly to
the dynamic-plugins ConfigMap as trusted cluster code at runtime. Replace the
mutable tag with an immutable SHA256 digest by changing the package reference
from using the colon separator with the tag to the at symbol with the full
digest value provided in the comment, ensuring the image is pinned to a specific
immutable version.
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: ClusterRole | ||
| metadata: | ||
| name: rhdh-useraccount-reader | ||
| rules: | ||
| - apiGroups: ["toolchain.dev.openshift.com"] | ||
| resources: ["useraccounts"] | ||
| verbs: ["get", "list", "watch"] | ||
| --- | ||
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: ClusterRoleBinding | ||
| metadata: | ||
| name: rhdh-useraccount-reader | ||
| roleRef: | ||
| apiGroup: rbac.authorization.k8s.io | ||
| kind: ClusterRole | ||
| name: rhdh-useraccount-reader | ||
| subjects: | ||
| - kind: ServiceAccount | ||
| name: developer-hub | ||
| namespace: ${RHDH_NAMESPACE} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 'name: useraccounts(\.toolchain\.dev\.openshift\.com)?|group: toolchain.dev.openshift.com|scope:' .Repository: redhat-performance/backstage-performance
Length of output: 178
🏁 Script executed:
# Search for any reference to toolchain in the repository
rg -i 'toolchain' --type yaml --type yml -n | head -20Repository: redhat-performance/backstage-performance
Length of output: 206
🏁 Script executed:
# Also search for any CRD definitions or scope declarations in the repo
fd -name "*.yaml" -o -name "*.yml" | xargs grep -l "scope:" 2>/dev/null | head -10Repository: redhat-performance/backstage-performance
Length of output: 409
🌐 Web query:
toolchain.dev.openshift.com useraccounts CRD scope namespaced cluster
💡 Result:
The UserAccount custom resource definition (CRD) with the group toolchain.dev.openshift.com is namespaced, not cluster-scoped [1]. In the context of the CodeReady Toolchain architecture, UserAccount resources are created within specific namespaces (often managed by the member operator, such as toolchain-member-operator) to maintain information about users provisioned within that specific member cluster [1]. While the Toolchain control plane may manage resources across multiple clusters (host and member clusters), the UserAccount resource itself is scoped to a namespace within the cluster where it resides [1].
Citations:
Change ClusterRoleBinding to RoleBinding for namespace-scoped access.
The toolchain.dev.openshift.com/useraccounts resource is namespaced, not cluster-scoped. Using ClusterRoleBinding grants unnecessary cluster-wide permissions; a RoleBinding scoped to the ${RHDH_NAMESPACE} namespace aligns with the least-privilege principle.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ci-scripts/rhdh-setup/template/backstage/olm/dev-sandbox/dev-sandbox-catalog.rbac.yaml`
around lines 7 - 27, The ClusterRoleBinding named rhdh-useraccount-reader grants
unnecessary cluster-wide permissions for a namespace-scoped resource. Change the
kind from ClusterRoleBinding to RoleBinding in the second resource definition
and add a namespace field to its metadata set to ${RHDH_NAMESPACE} to properly
scope the binding to the specific namespace. The roleRef and subjects can remain
unchanged, but ensure the RoleBinding is now namespace-scoped rather than
cluster-scoped.
73f973c to
2caf5b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ci-scripts/dev-sandbox/rhdh-perf-workloads.backstages.template.yaml`:
- Around line 77-83: The legacy external access secret is hardcoded in the
generated backend secret config, so update the template section that renders
app-config.rhdh.backend-secret.yaml to source externalAccess.options.secret from
the BACKEND_SECRET environment value instead of a fixed string. Use the existing
BACKEND_SECRET injection in the pod spec as the runtime-provided secret source,
and keep the configuration aligned with the backend auth settings in this
template.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 36b2c455-e59f-434d-9ba8-ded3399e50b8
📒 Files selected for processing (18)
ci-scripts/dev-sandbox/collect-results.shci-scripts/dev-sandbox/deploy.shci-scripts/dev-sandbox/metrics-config.yamlci-scripts/dev-sandbox/rhdh-perf-workloads.backstages.template.yamlci-scripts/rhdh-setup/common.shci-scripts/rhdh-setup/deploy.shci-scripts/rhdh-setup/install-rhdh-catalog-source.shci-scripts/rhdh-setup/template/backstage/app-config.yamlci-scripts/rhdh-setup/template/backstage/olm/app-config.rhdh.db.yamlci-scripts/rhdh-setup/template/backstage/olm/app-config.rhdh.urls.yamlci-scripts/rhdh-setup/template/backstage/olm/app-rbac-patch.yamlci-scripts/rhdh-setup/template/backstage/olm/backstage.yamlci-scripts/rhdh-setup/template/backstage/olm/dev-sandbox/app-config.rhdh.dev-sandbox-catalog-plugin.yamlci-scripts/rhdh-setup/template/backstage/olm/dev-sandbox/dev-sandbox-catalog-plugin-patch.yamlci-scripts/rhdh-setup/template/backstage/olm/dev-sandbox/dev-sandbox-catalog-plugin-volumemounts-patch.yamlci-scripts/rhdh-setup/template/backstage/olm/dev-sandbox/dev-sandbox-catalog-plugin-volumes-patch.yamlci-scripts/rhdh-setup/template/backstage/olm/dev-sandbox/dev-sandbox-catalog.rbac.yamltest.env
💤 Files with no reviewable changes (1)
- ci-scripts/rhdh-setup/template/backstage/app-config.yaml
✅ Files skipped from review due to trivial changes (8)
- ci-scripts/rhdh-setup/template/backstage/olm/dev-sandbox/dev-sandbox-catalog-plugin-volumemounts-patch.yaml
- ci-scripts/rhdh-setup/template/backstage/olm/app-rbac-patch.yaml
- ci-scripts/dev-sandbox/collect-results.sh
- ci-scripts/rhdh-setup/template/backstage/olm/dev-sandbox/dev-sandbox-catalog-plugin-patch.yaml
- test.env
- ci-scripts/rhdh-setup/template/backstage/olm/dev-sandbox/dev-sandbox-catalog.rbac.yaml
- ci-scripts/rhdh-setup/template/backstage/olm/app-config.rhdh.urls.yaml
- ci-scripts/rhdh-setup/template/backstage/olm/app-config.rhdh.db.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- ci-scripts/rhdh-setup/template/backstage/olm/dev-sandbox/dev-sandbox-catalog-plugin-volumes-patch.yaml
- ci-scripts/rhdh-setup/template/backstage/olm/dev-sandbox/app-config.rhdh.dev-sandbox-catalog-plugin.yaml
- ci-scripts/dev-sandbox/metrics-config.yaml
- ci-scripts/rhdh-setup/template/backstage/olm/backstage.yaml
- ci-scripts/rhdh-setup/common.sh
- ci-scripts/dev-sandbox/deploy.sh
- ci-scripts/rhdh-setup/install-rhdh-catalog-source.sh
- ci-scripts/rhdh-setup/deploy.sh
Signed-off-by: Pavel Macík <pavel.macik@gmail.com>
|
/unhold |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pmacik, shashankkestwal The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ba39e41
into
redhat-performance:main
No description provided.