OTA-1975: add products-data container and service to service products.json if present in graph-data image#270
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ankitathomas: This pull request references OTA-1975 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this: 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughThe development image now prepares ChangesProducts data exposure
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)controllers/testdata/resources/zz_fixture_Test_newKubeResources_deployment.yamlTraceback (most recent call last): controllers/testdata/resources/zz_fixture_Test_newKubeResources_graph_builder_service.yamlTraceback (most recent call last): dev/DockerfileTraceback (most recent call last): Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ankitathomas The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
8e9a48e to
12324be
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
controllers/new.go (1)
1040-1133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider avoiding repeated magic port literal and hardcoded bind address.
8082is repeated 5 times across Args/Ports/Probes/Service, and"::"is hardcoded rather than sourced from a ConfigMap/env likeADDRESSis for policy-engine. Extracting a shared constant for the port would reduce drift risk, and aligning the address with the existing env-config pattern would improve consistency with the rest of the file.🤖 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 `@controllers/new.go` around lines 1040 - 1133, Update newProductsServiceContainer and newProductsService to use a shared named constant for port 8082 across arguments, container/service ports, and probes, and replace the hardcoded "::" bind address with the file’s established ConfigMap or environment-based ADDRESS pattern used by policy-engine.
🤖 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 `@controllers/new.go`:
- Line 1063: Update newProductsServiceContainer to use dedicated minimal volume
mounts instead of newGraphBuilderVolumeMounts(instance). Mount only the
cincinnati-graph-data volume at the products graph-data path needed to serve
products.json, excluding /etc/configs, pull-secret, and trusted-ca; then update
the affected deployment fixture to match.
In `@dev/Dockerfile`:
- Around line 7-10: Update the products.json setup commands so curl treats
non-2xx responses as failures and uses the existing empty-object fallback, then
remove the unconditional `|| true` from the mkdir/mv command. Ensure directory
creation and moving products.json fail the image build when placement is
unsuccessful.
---
Nitpick comments:
In `@controllers/new.go`:
- Around line 1040-1133: Update newProductsServiceContainer and
newProductsService to use a shared named constant for port 8082 across
arguments, container/service ports, and probes, and replace the hardcoded "::"
bind address with the file’s established ConfigMap or environment-based ADDRESS
pattern used by policy-engine.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3700c0d7-8643-43c6-b35b-5beb34cbac30
📒 Files selected for processing (5)
controllers/names.gocontrollers/new.gocontrollers/testdata/resources/zz_fixture_Test_newKubeResources_deployment.yamlcontrollers/updateservice_controller.godev/Dockerfile
| Protocol: corev1.ProtocolTCP, | ||
| }, | ||
| }, | ||
| VolumeMounts: k.newGraphBuilderVolumeMounts(instance), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Products container is over-mounted with unrelated secrets/config.
newProductsServiceContainer reuses k.newGraphBuilderVolumeMounts(instance) wholesale, which (per the fixture) mounts /etc/configs (gb.toml), pull-secret (registry credentials), and trusted-ca in addition to cincinnati-graph-data. The products-service only needs read access to /var/lib/cincinnati/graph-data/products to serve products.json; there's no evident need for registry pull-secret or trusted CA material in this container. This unnecessarily widens the container's access to sensitive data (registry credentials) for a component whose only job is serving a static JSON file.
🔒 Proposed fix: dedicated, minimal volume mounts
- VolumeMounts: k.newGraphBuilderVolumeMounts(instance),
+ VolumeMounts: []corev1.VolumeMount{
+ {
+ MountPath: "/var/lib/cincinnati/graph-data",
+ Name: "cincinnati-graph-data",
+ },
+ },Note: the fixture file controllers/testdata/resources/zz_fixture_Test_newKubeResources_deployment.yaml will need updating to match once this is fixed.
📝 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.
| VolumeMounts: k.newGraphBuilderVolumeMounts(instance), | |
| VolumeMounts: []corev1.VolumeMount{ | |
| { | |
| MountPath: "/var/lib/cincinnati/graph-data", | |
| Name: "cincinnati-graph-data", | |
| }, | |
| }, |
🤖 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 `@controllers/new.go` at line 1063, Update newProductsServiceContainer to use
dedicated minimal volume mounts instead of
newGraphBuilderVolumeMounts(instance). Mount only the cincinnati-graph-data
volume at the products graph-data path needed to serve products.json, excluding
/etc/configs, pull-secret, and trusted-ca; then update the affected deployment
fixture to match.
| RUN curl -L -o products.json https://access.redhat.com/product-life-cycles/api/v2/products || echo "{}" > products.json | ||
|
|
||
| RUN mkdir -p /var/lib/cincinnati-graph-data/products && mv products.json /var/lib/cincinnati-graph-data/products/ || true | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Curl/mv failures are silently swallowed — build succeeds even when products.json fetch/placement fails.
- Line 7:
curllacks-f/--fail, so a non-2xx HTTP response (e.g. API outage, 404/500) still exits 0 and its body gets written intoproducts.jsoninstead of triggering the{}fallback. - Line 9:
|| trueswallows anymkdir/mvfailure, so the build succeeds even ifproducts.jsonnever ends up under/products/, leavingproducts-servicewithout data at runtime with no build-time signal.
🛠️ Proposed fix
-RUN curl -L -o products.json https://access.redhat.com/product-life-cycles/api/v2/products || echo "{}" > products.json
+RUN curl -f -L -o products.json https://access.redhat.com/product-life-cycles/api/v2/products || echo "{}" > products.json
-RUN mkdir -p /var/lib/cincinnati-graph-data/products && mv products.json /var/lib/cincinnati-graph-data/products/ || true
+RUN mkdir -p /var/lib/cincinnati-graph-data/products && mv products.json /var/lib/cincinnati-graph-data/products/📝 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.
| RUN curl -L -o products.json https://access.redhat.com/product-life-cycles/api/v2/products || echo "{}" > products.json | |
| RUN mkdir -p /var/lib/cincinnati-graph-data/products && mv products.json /var/lib/cincinnati-graph-data/products/ || true | |
| RUN curl -f -L -o products.json https://access.redhat.com/product-life-cycles/api/v2/products || echo "{}" > products.json | |
| RUN mkdir -p /var/lib/cincinnati-graph-data/products && mv products.json /var/lib/cincinnati-graph-data/products/ |
🤖 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 `@dev/Dockerfile` around lines 7 - 10, Update the products.json setup commands
so curl treats non-2xx responses as failures and uses the existing empty-object
fallback, then remove the unconditional `|| true` from the mkdir/mv command.
Ensure directory creation and moving products.json fail the image build when
placement is unsuccessful.
Needs the binaries from openshift/cincinnati#1072 to pass. |
ankitathomas
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
12324be to
44c10ab
Compare
|
/retest |
1 similar comment
|
/retest |
Signed-off-by: Ankita Thomas <[email protected]> Co-authored by: Claude
44c10ab to
2511f54
Compare
|
@ankitathomas: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
2 participants
Summary by CodeRabbit
products-dataTCP port on 8090 for the graph-building service (and matching container port exposure).products.jsonexists by downloading it when missing, or initializing an empty file if the download fails.